Merge pull request #20663 from vespa-engine/hmusum/dont-delete-new-sessions-in-file-system-that-does-not-exist-in-zookeeper

Don't delete new sessions in file system that does not exist in zookeeper [run-systemtest]

2 files changed, 40 insertions, 10 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
index 07cdc910253..c6a37e57e8a 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
@@ -572,8 +572,16 @@ public class SessionRepository {
     public void deleteExpiredSessions(Map<ApplicationId, Long> activeSessions) {
         log.log(Level.FINE, () -> "Purging old sessions for tenant '" + tenantName + "'");
         Set<LocalSession> toDelete = new HashSet<>();
+        Set<Long> newSessions = findNewSessionsInFileSystem();
         try {
             for (LocalSession candidate : getLocalSessionsFromFileSystem()) {
+                // Skip sessions newly added (we might have a session in the file system, but not in ZooKeeper,
+                // we don't want to touch any of them)
+                if (newSessions.contains(candidate.getSessionId())) {
+                    log.log(Level.INFO, () -> "Skipping session " + candidate.getSessionId() + ", newly created: ");
+                    continue;
+                }
+
                 Instant createTime = candidate.getCreateTime();
                 log.log(Level.FINE, () -> "Candidate session for deletion: " + candidate.getSessionId() + ", created: " + createTime);
 
@@ -618,6 +626,23 @@ public class SessionRepository {
                 && created(sessionDir).plus(Duration.ofDays(30)).isBefore(clock.instant());
     }
 
+    private Set<Long> findNewSessionsInFileSystem() {
+        File[] sessions = tenantFileSystemDirs.sessionsPath().listFiles(sessionApplicationsFilter);
+        Set<Long> newSessions = new HashSet<>();
+        if (sessions != null) {
+            for (File session : sessions) {
+                try {
+                    if (Files.getLastModifiedTime(session.toPath()).toInstant()
+                             .isAfter(clock.instant().minus(Duration.ofSeconds(30))))
+                        newSessions.add(Long.parseLong(session.getName()));
+                } catch (IOException e) {
+                    log.log(Level.INFO, "Unable to find last modified time for " + session.toPath());
+                };
+            }
+        }
+        return newSessions;
+    }
+
     private Instant created(File file) {
         BasicFileAttributes fileAttributes;
         try {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
index d3dfae3a6be..7baad75ebc5 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
@@ -480,22 +480,20 @@ public class ApplicationRepositoryTest {
         // so will be candidate for expiry)
         Session session = sessionRepository.createRemoteSession(7);
         sessionRepository.createSetStatusTransaction(session, Session.Status.UNKNOWN);
+        assertEquals(2, sessionRepository.getLocalSessions().size());  // Still 2, no new local session
 
-        sessionRepository.addLocalSession(localSession2);
-        assertEquals(2, sessionRepository.getLocalSessions().size());
-
-        // Check that trying to expire local session when there exists a local session with no zookeeper data works
-        tester.applicationRepository().deleteExpiredLocalSessions();
-        assertEquals(2, sessionRepository.getLocalSessions().size());
+        // Check that trying to expire local session when there exists a local session without any data in zookeeper
+        // should not delete session if this is a new file ...
+        deleteExpiredLocalSessionsAndAssertNumberOfSessions(2, tester, sessionRepository);
 
-        // Check that trying to expire when there are no active sessions works
+        // ... but it should be deleted if some time has passed
+        clock.advance(Duration.ofSeconds(60));
         tester.applicationRepository().deleteExpiredLocalSessions();
-        assertEquals(2, sessionRepository.getLocalSessions().size());
+        assertEquals(1, sessionRepository.getLocalSessions().size());
 
         // Set older created timestamp for session dir for local session without any data in zookeeper, should be deleted
         setCreatedTime(dir, Instant.now().minus(Duration.ofDays(31)));
-        tester.applicationRepository().deleteExpiredLocalSessions();
-        assertEquals(1, sessionRepository.getLocalSessions().size());
+        deleteExpiredLocalSessionsAndAssertNumberOfSessions(0, tester, sessionRepository);
     }
 
     @Test
@@ -836,4 +834,11 @@ public class ApplicationRepositoryTest {
         return tenantRepository.getTenant(applicationId.tenant()).getRequestHandler();
     }
 
+    private static void deleteExpiredLocalSessionsAndAssertNumberOfSessions(int expectedNumberOfSessions,
+                                                                            DeployTester tester,
+                                                                            SessionRepository sessionRepository) {
+        tester.applicationRepository().deleteExpiredLocalSessions();
+        assertEquals(expectedNumberOfSessions, sessionRepository.getLocalSessions().size());
+    }
+
 }