Add config for when to expire local sessions with unknown status

2 files changed, 23 insertions, 19 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 059d192e7d2..7d10acaf3ae 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
@@ -575,32 +575,31 @@ public class SessionRepository {
     // ---------------- Common stuff ----------------------------------------------------------------
 
     public void deleteExpiredSessions(Map<ApplicationId, Long> activeSessions) {
-        log.log(Level.FINE, () -> "Purging old sessions for tenant '" + tenantName + "'");
+        log.log(Level.FINE, () -> "Deleting expired local 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.FINE, () -> "Skipping expiring newly created session " + candidate.getSessionId());
+                if (newSessions.contains(candidate.getSessionId()))
                     continue;
-                }
 
                 Instant createTime = candidate.getCreateTime();
-                log.log(Level.FINE, () -> "Candidate session for deletion: " + candidate.getSessionId() + ", created: " + createTime);
+                log.log(Level.FINE, () -> "Candidate local session for deletion: " + candidate.getSessionId() +
+                        ", created: " + createTime + ", state " + candidate.getStatus() + ", can be deleted: " + canBeDeleted(candidate));
 
-                if (hasExpired(candidate) && canBeDeleted(candidate)) {
+                if (hasExpired(createTime) && canBeDeleted(candidate)) {
                     toDelete.add(candidate);
                 } else if (createTime.plus(Duration.ofDays(1)).isBefore(clock.instant())) {
-                    //  Sessions with state ACTIVATE, but which are not actually active
                     Optional<ApplicationId> applicationId = candidate.getOptionalApplicationId();
                     if (applicationId.isEmpty()) continue;
+
                     Long activeSession = activeSessions.get(applicationId.get());
                     if (activeSession == null || activeSession != candidate.getSessionId()) {
                         toDelete.add(candidate);
-                        log.log(Level.INFO, "Deleted inactive session " + candidate.getSessionId() + " created " +
-                                            createTime + " for '" + applicationId + "'");
+                        log.log(Level.FINE, () -> "Will delete inactive session " + candidate.getSessionId() + " created " +
+                                createTime + " for '" + applicationId + "'");
                     }
                 }
             }
@@ -614,21 +613,22 @@ public class SessionRepository {
         log.log(Level.FINE, () -> "Done purging old sessions");
     }
 
-    private boolean hasExpired(LocalSession candidate) {
-        return candidate.getCreateTime().plus(sessionLifetime).isBefore(clock.instant());
+    private boolean hasExpired(Instant created) {
+        return created.plus(sessionLifetime).isBefore(clock.instant());
     }
 
     // Sessions with state other than UNKNOWN or ACTIVATE or old sessions in UNKNOWN state
     private boolean canBeDeleted(LocalSession candidate) {
-        return ! List.of(Session.Status.UNKNOWN, Session.Status.ACTIVATE).contains(candidate.getStatus())
-                || oldSessionDirWithNonExistingSession(candidate);
+        return ( ! List.of(Session.Status.UNKNOWN, Session.Status.ACTIVATE).contains(candidate.getStatus()))
+                || oldSessionDirWithUnknownStatus(candidate);
     }
 
-    private boolean oldSessionDirWithNonExistingSession(LocalSession session) {
+    private boolean oldSessionDirWithUnknownStatus(LocalSession session) {
+        Duration expiryTime = Duration.ofHours(configserverConfig.keepSessionsWithUnknownStatusHours());
         File sessionDir = tenantFileSystemDirs.getUserApplicationDir(session.getSessionId());
         return sessionDir.exists()
                 && session.getStatus() == Session.Status.UNKNOWN
-                && created(sessionDir).plus(Duration.ofDays(30)).isBefore(clock.instant());
+                && created(sessionDir).plus(expiryTime).isBefore(clock.instant());
     }
 
     private Set<Long> findNewSessionsInFileSystem() {
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 ee2a979be7a..46cf84e19c1 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
@@ -464,7 +464,8 @@ public class ApplicationRepositoryTest {
         // and check that expiring local sessions still works
         int sessionId = 6;
         TenantName tenantName = tester.tenant().getName();
-        java.nio.file.Path dir = Files.createDirectory(new TenantFileSystemDirs(serverdb, tenantName).getUserApplicationDir(sessionId).toPath());
+        Instant session6CreateTime = clock.instant();
+        Files.createDirectory(new TenantFileSystemDirs(serverdb, tenantName).getUserApplicationDir(sessionId).toPath());
         LocalSession localSession2 = new LocalSession(tenant1,
                                                       sessionId,
                                                       FilesApplicationPackage.fromFile(testApp),
@@ -479,7 +480,7 @@ 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
+        assertEquals(2, sessionRepository.getLocalSessions().size()); // Still 2, no new local session
 
         // 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 ...
@@ -489,8 +490,11 @@ public class ApplicationRepositoryTest {
         clock.advance(Duration.ofSeconds(60));
         deleteExpiredLocalSessionsAndAssertNumberOfSessions(1, tester, sessionRepository);
 
-        // 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)));
+        // Reset clock to the time session was created, session should NOT be deleted
+        clock.setInstant(session6CreateTime);
+        deleteExpiredLocalSessionsAndAssertNumberOfSessions(1, tester, sessionRepository);
+        // Advance time, session SHOULD be deleted
+        clock.advance(Duration.ofHours(configserverConfig.keepSessionsWithUnknownStatusHours()).plus(Duration.ofMinutes(1)));
         deleteExpiredLocalSessionsAndAssertNumberOfSessions(0, tester, sessionRepository);
     }