Ignore sessions in state NEW when initializing

2 files changed, 41 insertions, 14 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 f82aa405380..65ac68ac234 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
@@ -182,7 +182,7 @@ public class SessionRepository {
         loadSessions(executor);
     }
 
-    // For testing
+    // non-private for testing
     void loadSessions(ExecutorService executor) {
         loadRemoteSessions(executor);
         try {
@@ -410,14 +410,15 @@ public class SessionRepository {
     }
 
     private void loadRemoteSessions(ExecutorService executor) throws NumberFormatException {
-        Map<Long, Future<?>> futures = new HashMap<>();
+        Map<Long, Future<Boolean>> futures = new HashMap<>();
         for (long sessionId : getRemoteSessionsFromZooKeeper()) {
-            futures.put(sessionId, executor.submit(() -> sessionAdded(sessionId)));
+            futures.put(sessionId, executor.submit(() -> addSession(sessionId, true)));
         }
         futures.forEach((sessionId, future) -> {
             try {
-                future.get();
-                log.log(Level.FINE, () -> "Remote session " + sessionId + " loaded");
+                boolean loaded = future.get();
+                if (loaded)
+                    log.log(Level.FINE, () -> "Remote session " + sessionId + " loaded");
             } catch (ExecutionException | InterruptedException e) {
                 throw new RuntimeException("Could not load remote session " + sessionId, e);
             }
@@ -425,26 +426,32 @@ public class SessionRepository {
     }
 
     /**
-     * A session for which we don't have a watcher, i.e. hitherto unknown to us.
+     * Add session if it is not already added and is not in a state where it should be ignored.
      *
      * @param sessionId session id for the new session
+     * @return true if added, false otherwise
      */
-    public void sessionAdded(long sessionId) {
-        if (hasStatusDeleted(sessionId)) return;
+    public boolean addSession(long sessionId, boolean initializing) {
+        if (sessionShouldBeIgnored(sessionId, initializing)) return false;
 
         log.log(Level.FINE, () -> "Adding remote session " + sessionId);
         Session session = createRemoteSession(sessionId);
-        if (session.getStatus() == Session.Status.NEW) {
-            log.log(Level.FINE, () -> session.logPre() + "Confirming upload for session " + sessionId);
+        if (session.getStatus() == Session.Status.NEW)
             confirmUpload(session);
-        }
         createLocalSessionFromDistributedApplicationPackage(sessionId);
+        return true;
     }
 
-    private boolean hasStatusDeleted(long sessionId) {
+    private boolean sessionShouldBeIgnored(long sessionId, boolean initializing) {
+        var ignorableStates = new HashSet<>(Set.of(Session.Status.DELETE));
+        // Also ignore sessions in NEW state when initializing, they are missing application package
+        // and other session data, usually because deployment failed
+        if (initializing)
+            ignorableStates.add(Session.Status.NEW);
+
         SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId);
         RemoteSession session = new RemoteSession(tenantName, sessionId, sessionZKClient);
-        return session.getStatus() == Session.Status.DELETE;
+        return ignorableStates.contains(session.getStatus());
     }
 
     void activate(long sessionId) {
@@ -960,7 +967,7 @@ public class SessionRepository {
     private void checkForAddedSessions(List<Long> sessions) {
         for (Long sessionId : sessions)
             if (remoteSessionCache.get(sessionId) == null)
-                sessionAdded(sessionId);
+                addSession(sessionId, false);
     }
 
     public Transaction createActivateTransaction(Session session) {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java
index 07d3aac5a52..fec4c096c7b 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java
@@ -83,6 +83,10 @@ public class SessionRepositoryTest {
 
     private void setup(ModelFactoryRegistry modelFactoryRegistry) throws Exception {
         curator = new MockCurator();
+        setup(modelFactoryRegistry, curator);
+    }
+
+    private void setup(ModelFactoryRegistry modelFactoryRegistry, MockCurator curator) throws Exception {
         File configserverDbDir = temporaryFolder.newFolder().getAbsoluteFile();
         ConfigserverConfig configserverConfig = new ConfigserverConfig.Builder()
                 .configServerDBDir(configserverDbDir.getAbsolutePath())
@@ -190,6 +194,22 @@ public class SessionRepositoryTest {
         assertTrue(sessionRepository.getRemoteSessionsFromZooKeeper().isEmpty());
     }
 
+    // If reading a session that originally failed deployment we should just skip it
+    @Test
+    public void testThatSessionThatFailedDeploymentIsIgnored() throws Exception {
+        setup();
+
+        // Create a new session, which will have status NEW, which is what will be the state if e.g.
+        // a deployment failed when building the config model
+        createSession(3, false);
+
+        // setup() will load remote sessions, session 3 should not be loaded, since it is in state NEW
+        setup(new ModelFactoryRegistry(List.of(VespaModelFactory.createTestFactory())), curator);
+        assertTrue(sessionRepository.getRemoteSessions().isEmpty());
+        Path sessionPath = TenantRepository.getSessionsPath(tenantName).append(3 + "");
+        assertTrue(curator.exists(sessionPath));
+    }
+
     @Test
     public void require_that_new_invalid_application_throws_exception() throws Exception {
         MockModelFactory failingFactory = new MockModelFactory();