Merge pull request #18625 from vespa-engine/hmusum/handle-sessions-with-status-NONE-when-expiring

Do not expire sessions with state UNKNOWN

5 files changed, 18 insertions, 10 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java
index 542b54d877e..b8f688e4998 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java
@@ -77,7 +77,7 @@ public abstract class Session implements Comparable<Session>  {
      * The status of this session.
      */
     public enum Status {
-        NEW, PREPARE, ACTIVATE, DEACTIVATE, NONE, DELETE;
+        NEW, PREPARE, ACTIVATE, DEACTIVATE, UNKNOWN, DELETE;
 
         public static Status parse(String data) {
             for (Status status : Status.values()) {
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 de2e4d54eec..3c8437cfe18 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
@@ -551,8 +551,7 @@ public class SessionRepository {
                 Instant createTime = candidate.getCreateTime();
                 log.log(Level.FINE, () -> "Candidate session for deletion: " + candidate.getSessionId() + ", created: " + createTime);
 
-                // Sessions with state other than ACTIVATE
-                if (hasExpired(candidate) && !isActiveSession(candidate)) {
+                if (hasExpired(candidate) && 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
@@ -580,8 +579,9 @@ public class SessionRepository {
         return candidate.getCreateTime().plus(sessionLifetime).isBefore(clock.instant());
     }
 
-    private boolean isActiveSession(LocalSession candidate) {
-        return candidate.getStatus() == Session.Status.ACTIVATE;
+    // Sessions with state other than UNKNOWN or ACTIVATE
+    private boolean canBeDeleted(LocalSession candidate) {
+        return  ! List.of(Session.Status.UNKNOWN, Session.Status.ACTIVATE).contains(candidate.getStatus());
     }
 
     private void ensureSessionPathDoesNotExist(long sessionId) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionStateWatcher.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionStateWatcher.java
index 880519b3d61..e198307f242 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionStateWatcher.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionStateWatcher.java
@@ -47,7 +47,7 @@ public class SessionStateWatcher {
         long sessionId = session.getSessionId();
         switch (newStatus) {
             case NEW:
-            case NONE:
+            case UNKNOWN:
                 break;
             case DELETE:
                 sessionRepository.deactivateAndUpdateCache(session);
@@ -86,7 +86,7 @@ public class SessionStateWatcher {
 
     private void nodeChanged() {
         zkWatcherExecutor.execute(() -> {
-            Status newStatus = Status.NONE;
+            Status newStatus = Status.UNKNOWN;
             try {
                 ChildData node = fileCache.getCurrentData();
                 if (node != null) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
index c7b19968f0a..1b23c3bf37a 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
@@ -93,11 +93,11 @@ public class SessionZooKeeperClient {
     public Session.Status readStatus() {
         try {
             Optional<byte[]> data = curator.getData(sessionStatusPath);
-            return data.map(d -> Session.Status.parse(Utf8.toString(d))).orElse(Session.Status.NONE);
+            return data.map(d -> Session.Status.parse(Utf8.toString(d))).orElse(Session.Status.UNKNOWN);
         } catch (Exception e) {
             log.log(Level.INFO, "Failed to read session status at " + sessionStatusPath.getAbsolute() +
                     ", will assume session has been removed: ", e);
-            return Session.Status.NONE;
+            return Session.Status.UNKNOWN;
         }
     }
 
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 8f8005894e5..8d659e02a52 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
@@ -477,9 +477,17 @@ public class ApplicationRepositoryTest {
         sessionRepository.addLocalSession(localSession2);
         assertEquals(2, sessionRepository.getLocalSessions().size());
 
+        // Create a session, set status to UNKNOWN, we don't want to expire those (creation time is then EPOCH,
+        // so will be candidate for expiry)
+        Session session = sessionRepository.createRemoteSession(7);
+        sessionRepository.createSetStatusTransaction(session, Session.Status.UNKNOWN);
+
+        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(1, sessionRepository.getLocalSessions().size());
+        assertEquals(2, sessionRepository.getLocalSessions().size());
 
         // Check that trying to expire when there are no active sessions works
         tester.applicationRepository().deleteExpiredLocalSessions();