Delete expired local sessions with errors

Some applications might have issues, incomplete data on disk etc.
Handle deleting those by just looking at age and status.

6 files changed, 181 insertions, 40 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
index 1aa70ff4b5b..59a48ad3c7e 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
@@ -25,7 +25,6 @@ import com.yahoo.vespa.config.server.application.ConfigNotConvergedException;
 import com.yahoo.vespa.config.server.configchange.ConfigChangeActions;
 import com.yahoo.vespa.config.server.configchange.ReindexActions;
 import com.yahoo.vespa.config.server.configchange.RestartActions;
-import com.yahoo.vespa.config.server.session.LocalSession;
 import com.yahoo.vespa.config.server.session.PrepareParams;
 import com.yahoo.vespa.config.server.session.Session;
 import com.yahoo.vespa.config.server.session.SessionRepository;
@@ -161,9 +160,7 @@ public class Deployment implements com.yahoo.config.provision.Deployment {
     }
 
     private void deleteSession() {
-        SessionRepository sessionRepository = sessionRepository();
-        LocalSession localSession = sessionRepository.getLocalSession(session.getSessionId());
-        sessionRepository.deleteLocalSession(localSession);
+        sessionRepository().deleteLocalSession(session.getSessionId());
     }
 
     private SessionRepository sessionRepository() {
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 a6bbd6c20a2..2e9c28bdc3b 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
@@ -221,15 +221,31 @@ public class SessionRepository {
         Set<LocalSession> sessionIds = new HashSet<>();
         for (File session : sessions) {
             long sessionId = Long.parseLong(session.getName());
-            SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId);
-            File sessionDir = getAndValidateExistingSessionAppDir(sessionId);
-            ApplicationPackage applicationPackage = FilesApplicationPackage.fromFile(sessionDir);
-            LocalSession localSession = new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient);
+            LocalSession localSession = getSessionFromFile(sessionId);
             sessionIds.add(localSession);
         }
         return sessionIds;
     }
 
+    private LocalSession getSessionFromFile(long sessionId) {
+        SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId);
+        File sessionDir = getAndValidateExistingSessionAppDir(sessionId);
+        ApplicationPackage applicationPackage = FilesApplicationPackage.fromFile(sessionDir);
+        return new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient);
+    }
+
+    public Set<Long> getLocalSessionsIdsFromFileSystem() {
+        File[] sessions = tenantFileSystemDirs.sessionsPath().listFiles(sessionApplicationsFilter);
+        if (sessions == null) return Set.of();
+
+        Set<Long> sessionIds = new HashSet<>();
+        for (File session : sessions) {
+            long sessionId = Long.parseLong(session.getName());
+            sessionIds.add(sessionId);
+        }
+        return sessionIds;
+    }
+
     public ConfigChangeActions prepareLocalSession(Session session, DeployLogger logger, PrepareParams params, Instant now) {
         params.vespaVersion().ifPresent(version -> {
             if ( ! params.isBootstrap() && ! modelFactoryRegistry.allVersions().contains(version))
@@ -310,8 +326,7 @@ public class SessionRepository {
     }
 
     // Will delete session data in ZooKeeper and file system
-    public void deleteLocalSession(LocalSession session) {
-        long sessionId = session.getSessionId();
+    public void deleteLocalSession(long sessionId) {
         log.log(Level.FINE, () -> "Deleting local session " + sessionId);
         SessionStateWatcher watcher = sessionStateWatchers.remove(sessionId);
         if (watcher != null) watcher.close();
@@ -323,7 +338,7 @@ public class SessionRepository {
 
     private void deleteAllSessions() {
         for (LocalSession session : getLocalSessions()) {
-            deleteLocalSession(session);
+            deleteLocalSession(session.getSessionId());
         }
     }
 
@@ -586,35 +601,48 @@ public class SessionRepository {
 
     public void deleteExpiredSessions(Map<ApplicationId, Long> activeSessions) {
         log.log(Level.FINE, () -> "Deleting expired local sessions for tenant '" + tenantName + "'");
-        Set<LocalSession> toDelete = new HashSet<>();
+        Set<Long> sessionIdsToDelete = new HashSet<>();
         Set<Long> newSessions = findNewSessionsInFileSystem();
         try {
-            for (LocalSession candidate : getLocalSessionsFromFileSystem()) {
+            for (long sessionId : getLocalSessionsIdsFromFileSystem()) {
                 // 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()))
+                if (newSessions.contains(sessionId))
                     continue;
 
-                Instant createTime = candidate.getCreateTime();
-                log.log(Level.FINE, () -> "Candidate local session for deletion: " + candidate.getSessionId() +
-                        ", created: " + createTime + ", state " + candidate.getStatus() + ", can be deleted: " + canBeDeleted(candidate));
+                var sessionZooKeeperClient = createSessionZooKeeperClient(sessionId);
+                Instant createTime = sessionZooKeeperClient.readCreateTime();
+                Session.Status status = sessionZooKeeperClient.readStatus();
 
-                if (hasExpired(createTime) && canBeDeleted(candidate)) {
-                    toDelete.add(candidate);
+                log.log(Level.FINE, () -> "Candidate local session for deletion: " + sessionId +
+                        ", created: " + createTime + ", status " + status + ", can be deleted: " + canBeDeleted(sessionId, status) +
+                        ", hasExpired: " + hasExpired(createTime));
+
+                if (hasExpired(createTime) && canBeDeleted(sessionId, status)) {
+                    log.log(Level.FINE, () -> "expired: " + hasExpired(createTime) + ", can be deleted: " + canBeDeleted(sessionId, status));
+                    sessionIdsToDelete.add(sessionId);
                 } else if (createTime.plus(Duration.ofDays(1)).isBefore(clock.instant())) {
-                    Optional<ApplicationId> applicationId = candidate.getOptionalApplicationId();
+                    LocalSession session;
+                    log.log(Level.FINE, () -> "not expired, but more than 1 day old: " + sessionId);
+                    try {
+                        session = getSessionFromFile(sessionId);
+                    } catch (Exception e) {
+                        log.log(Level.FINE, () -> "could not get session from file: " + sessionId + ": " + e.getMessage());
+                        continue;
+                    }
+                    Optional<ApplicationId> applicationId = session.getOptionalApplicationId();
                     if (applicationId.isEmpty()) continue;
 
                     Long activeSession = activeSessions.get(applicationId.get());
-                    if (activeSession == null || activeSession != candidate.getSessionId()) {
-                        toDelete.add(candidate);
-                        log.log(Level.FINE, () -> "Will delete inactive session " + candidate.getSessionId() + " created " +
+                    if (activeSession == null || activeSession != sessionId) {
+                        sessionIdsToDelete.add(sessionId);
+                        log.log(Level.FINE, () -> "Will delete inactive session " + sessionId + " created " +
                                 createTime + " for '" + applicationId + "'");
                     }
                 }
             }
 
-            toDelete.forEach(this::deleteLocalSession);
+            sessionIdsToDelete.forEach(this::deleteLocalSession);
 
             // Make sure to catch here, to avoid executor just dying in case of issues ...
         } catch (Throwable e) {
@@ -628,16 +656,16 @@ public class SessionRepository {
     }
 
     // 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()))
-                || oldSessionDirWithUnknownStatus(candidate);
+    private boolean canBeDeleted(long sessionId, Session.Status status) {
+        return ( ! List.of(Session.Status.UNKNOWN, Session.Status.ACTIVATE).contains(status))
+                || oldSessionDirWithUnknownStatus(sessionId, status);
     }
 
-    private boolean oldSessionDirWithUnknownStatus(LocalSession session) {
+    private boolean oldSessionDirWithUnknownStatus(long sessionId, Session.Status status) {
         Duration expiryTime = Duration.ofHours(configserverConfig.keepSessionsWithUnknownStatusHours());
-        File sessionDir = tenantFileSystemDirs.getUserApplicationDir(session.getSessionId());
+        File sessionDir = tenantFileSystemDirs.getUserApplicationDir(sessionId);
         return sessionDir.exists()
-                && session.getStatus() == Session.Status.UNKNOWN
+                && status == Session.Status.UNKNOWN
                 && created(sessionDir).plus(expiryTime).isBefore(clock.instant());
     }
 
diff --git a/configserver/src/test/apps/illegalApp2/hosts.xml b/configserver/src/test/apps/illegalApp2/hosts.xml
new file mode 100644
index 00000000000..a515a4e97da
--- /dev/null
+++ b/configserver/src/test/apps/illegalApp2/hosts.xml
@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<!-- Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -->
+<hosts>
+  <host name="mytesthost">
+    <alias>node1</alias>
+  </host>
+</hosts>
diff --git a/configserver/src/test/apps/illegalApp2/schemas/music.sd b/configserver/src/test/apps/illegalApp2/schemas/music.sd
new file mode 100644
index 00000000000..f4b11d1e8e4
--- /dev/null
+++ b/configserver/src/test/apps/illegalApp2/schemas/music.sd
@@ -0,0 +1,50 @@
+# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+# A basic search definition - called music, should be saved to music.sd
+search music {
+
+  # It contains one document type only - called music as well
+  document music {
+
+    field title type string {
+      indexing: summary | index   # How this field should be indexed
+      # index-to: title, default    # Create two indexes
+      weight: 75 # Ranking importancy of this field, used by the built in nativeRank feature
+    }
+
+    field artist type string {
+      indexing: summary | attribute | index
+      # index-to: artist, default
+
+      weight: 25 
+    }
+
+    field year type int {
+      indexing: summary | attribute
+    }
+
+    # Increase query
+    field popularity type int {
+      indexing: summary | attribute
+    }
+
+    field url type uri {
+      indexing: summary | index
+    }
+
+  }
+
+  rank-profile default inherits default {
+    first-phase {
+      expression: nativeRank(title,artist) + attribute(popularity)
+    }
+
+  }
+
+  rank-profile textmatch inherits default {
+    first-phase {
+      expression: nativeRank(title,artist) 
+    }
+
+  }
+
+}
diff --git a/configserver/src/test/apps/illegalApp2/services.xml b/configserver/src/test/apps/illegalApp2/services.xml
new file mode 100644
index 00000000000..3e957a0e228
--- /dev/null
+++ b/configserver/src/test/apps/illegalApp2/services.xml
@@ -0,0 +1,39 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<!-- Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -->
+<services version="1.0">
+
+    <admin version="2.0">
+        <adminserver hostalias="node1"/>
+        <logserver hostalias="node1" />
+    </admin>
+
+    <content version="1.0">
+        <redundancy>2</redundancy>
+        <documents>
+            <document type="music" mode="index"/>
+        </documents>
+        <nodes>
+            <node hostalias="node1" distribution-key="0"/>
+        </nodes>
+
+    </content>
+
+    <container version="1.0">
+        <include dir='file:///etc/passwd'/>
+        <document-processing compressdocuments="true">
+            <chain id="ContainerWrapperTest">
+                <documentprocessor id="com.yahoo.vespa.config.AppleDocProc"/>
+            </chain>
+        </document-processing>
+
+        <config name="project.specific">
+            <value>someval</value>
+        </config>
+
+        <nodes>
+            <node hostalias="node1" />
+        </nodes>
+
+    </container>
+
+</services>
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 d1d8c165124..e8eb49c72cf 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
@@ -61,6 +61,7 @@ import java.io.File;
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.nio.file.attribute.BasicFileAttributeView;
 import java.nio.file.attribute.FileTime;
 import java.time.Duration;
@@ -94,6 +95,7 @@ public class ApplicationRepositoryTest {
     private final static File testAppLogServerWithContainer = new File("src/test/apps/app-logserver-with-container");
     private final static File app1 = new File("src/test/apps/cs1");
     private final static File app2 = new File("src/test/apps/cs2");
+    private final static File illegalApp2 = new File("src/test/apps/illegalapp2");
 
     private final static TenantName tenant1 = TenantName.from("test1");
     private final static TenantName tenant2 = TenantName.from("test2");
@@ -456,7 +458,7 @@ public class ApplicationRepositoryTest {
         deployment4.get().prepare();  // session 5 (not activated)
 
         assertEquals(2, sessionRepository.getLocalSessions().size());
-        sessionRepository.deleteLocalSession(localSession);
+        sessionRepository.deleteLocalSession(localSession.getSessionId());
         assertEquals(1, sessionRepository.getLocalSessions().size());
 
         // Create a local session without any data in zookeeper (corner case seen in production occasionally)
@@ -464,28 +466,29 @@ public class ApplicationRepositoryTest {
         int sessionId = 6;
         TenantName tenantName = tester.tenant().getName();
         Instant session6CreateTime = clock.instant();
-        Files.createDirectory(new TenantFileSystemDirs(serverdb, tenantName).getUserApplicationDir(sessionId).toPath());
-        LocalSession localSession2 = new LocalSession(tenant1,
+        TenantFileSystemDirs tenantFileSystemDirs = new TenantFileSystemDirs(serverdb, tenantName);
+        Files.createDirectory(tenantFileSystemDirs.getUserApplicationDir(sessionId).toPath());
+        String hostName = ConfigUtils.getCanonicalHostName();
+        LocalSession localSession2 = new LocalSession(tenantName,
                                                       sessionId,
                                                       FilesApplicationPackage.fromFile(testApp),
-                                                      new SessionZooKeeperClient(curator,
-                                                                                 tenantName,
-                                                                                 sessionId,
-                                                                                 ConfigUtils.getCanonicalHostName()));
+                                                      new SessionZooKeeperClient(curator, tenantName, sessionId, hostName));
         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);
+        sessionId = 7;
+        Session session = sessionRepository.createRemoteSession(sessionId);
+        sessionRepository.createSessionZooKeeperClient(sessionId).createNewSession(clock.instant());
+        sessionRepository.createSetStatusTransaction(session, Session.Status.UNKNOWN).commit();
         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 ...
         deleteExpiredLocalSessionsAndAssertNumberOfSessions(2, tester, sessionRepository);
 
-        // ... but it should be deleted if some time has passed
+        // ... but it should be deleted when some time has passed
         clock.advance(Duration.ofSeconds(60));
         deleteExpiredLocalSessionsAndAssertNumberOfSessions(1, tester, sessionRepository);
 
@@ -495,6 +498,23 @@ public class ApplicationRepositoryTest {
         // Advance time, session SHOULD be deleted
         clock.advance(Duration.ofHours(configserverConfig.keepSessionsWithUnknownStatusHours()).plus(Duration.ofMinutes(1)));
         deleteExpiredLocalSessionsAndAssertNumberOfSessions(0, tester, sessionRepository);
+
+        // Create a local session with invalid application package and check that expiring local sessions still works
+        sessionId = 8;
+        java.nio.file.Path applicationPath = tenantFileSystemDirs.getUserApplicationDir(sessionId).toPath();
+        Files.createDirectory(applicationPath);
+        Files.writeString(Files.createFile(applicationPath.resolve("services.xml")),
+                          Files.readString(Paths.get(illegalApp2.getAbsolutePath(), "services.xml")));
+        assertTrue(applicationPath.toFile().exists());  // App exists on disk
+        session = sessionRepository.createRemoteSession(sessionId);
+        sessionRepository.createSessionZooKeeperClient(sessionId).createNewSession(clock.instant());
+        sessionRepository.createSetStatusTransaction(session, Session.Status.PREPARE).commit();
+        assertEquals(0, sessionRepository.getLocalSessions().size());  // Will not show up in local sessions
+
+        // Advance time, session SHOULD be deleted
+        clock.advance(Duration.ofHours(configserverConfig.keepSessionsWithUnknownStatusHours()).plus(Duration.ofMinutes(1)));
+        deleteExpiredLocalSessionsAndAssertNumberOfSessions(0, tester, sessionRepository);
+        assertFalse(applicationPath.toFile().exists());  // App has been deleted
     }
 
     @Test