fix(welcome): remove the sample database journal files on reset

CHANGELOG.md

@@ -106,6 +106,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Two rename fields when renaming a favorite on iPhone and iPad.
 - Connection names cut off at large text sizes on iPhone and iPad.
 - `Cmd+W` closing the whole connection instead of the current tab until something in the window was clicked.
+- **Reset Sample Database** leaving the old Chinook copy's journal files beside the fresh one.
 
 ### Security
 

TablePro/Core/Database/RemoteDatabaseFileTransfer.swift

@@ -58,6 +58,20 @@ struct RemoteFetchResult: Sendable {
     let plan: RemoteFetchPlan
 }
 
+internal protocol RemoteFileSource {
+    func exists(_ path: String) -> Bool
+
+    @discardableResult
+    func download(
+        remotePath: String,
+        to localURL: URL,
+        progress: (@Sendable (UInt64, UInt64) -> Void)?,
+        isCancelled: @escaping @Sendable () -> Bool
+    ) throws -> (bytes: UInt64, sha256: String)
+}
+
+extension LibSSH2SFTPSession: RemoteFileSource {}
+
 /// Copies a database file from a server into a local working copy.
 ///
 /// Every rule here comes from something that was measured rather than assumed. The three that
@@ -156,6 +170,7 @@ enum RemoteDatabaseFileTransfer {
         try? FileManager.default.removeItem(at: staging)
 
         let downloaded: (bytes: UInt64, sha256: String)
+        let fetchedSidecars: Set<String>
         switch plan {
         case .remoteSnapshot(let executable):
             downloaded = try fetchViaRemoteSnapshot(
@@ -166,12 +181,13 @@ enum RemoteDatabaseFileTransfer {
                 progress: progress,
                 isCancelled: isCancelled
             )
+            fetchedSidecars = []
         case .directCopy(let sidecars):
             downloaded = try session.download(
                 remotePath: remotePath, to: staging, progress: progress, isCancelled: isCancelled
             )
-            try fetchSidecars(
-                session: session,
+            fetchedSidecars = try fetchSidecars(
+                from: session,
                 remotePath: remotePath,
                 sidecars: sidecars,
                 destinationDirectory: destinationDirectory,
@@ -195,7 +211,7 @@ enum RemoteDatabaseFileTransfer {
         try replaceLocalItem(at: workingCopy, with: staging)
         clearStaleSidecars(
             layout: layout,
-            plan: plan,
+            keeping: fetchedSidecars,
             destinationDirectory: destinationDirectory,
             fileName: fileName
         )
@@ -224,25 +240,21 @@ enum RemoteDatabaseFileTransfer {
         return RemoteFetchResult(workingCopy: workingCopy, manifest: manifest, plan: plan)
     }
 
-    /// A reader that opens a working copy must not find a `-wal` or `-shm` left over from a previous
-    /// copy of a different file, because SQLite would replay it against bytes it no longer matches.
+    /// A reader that opens a working copy must not find a `-journal`, `-wal` or `-shm` left over from
+    /// a previous copy of a different file, because SQLite would roll it back or replay it against
+    /// bytes it no longer matches.
     ///
-    /// A snapshot is fully checkpointed and carries no log, so every stale sidecar goes. A direct
-    /// copy keeps the ones it just fetched (the server had them) and clears the rest, which is what
-    /// removes a `-wal` that the server has since checkpointed away.
+    /// Only a sidecar this fetch downloaded is kept, never one its plan merely listed. A snapshot is
+    /// fully checkpointed and downloads none. A `-wal` the server checkpoints away, or a `-journal`
+    /// whose transaction ends, between planning and fetching is never downloaded, and the local file
+    /// of that name still belongs to the previous copy.
     static func clearStaleSidecars(
         layout: DatabaseFileLayout,
-        plan: RemoteFetchPlan,
+        keeping fetchedSidecars: Set<String>,
         destinationDirectory: URL,
         fileName: String
     ) {
-        let kept: Set<String>
-        if case .directCopy(let sidecars) = plan {
-            kept = Set(sidecars)
-        } else {
-            kept = []
-        }
-        for suffix in layout.staleAfterReplaceSuffixes where !kept.contains(suffix) {
+        for suffix in layout.staleAfterReplaceSuffixes where !fetchedSidecars.contains(suffix) {
             try? FileManager.default.removeItem(
                 at: destinationDirectory.appendingPathComponent(fileName + suffix)
             )
@@ -286,22 +298,28 @@ enum RemoteDatabaseFileTransfer {
         )
     }
 
-    private static func fetchSidecars(
-        session: LibSSH2SFTPSession,
+    static func fetchSidecars(
+        from source: some RemoteFileSource,
         remotePath: String,
         sidecars: [String],
         destinationDirectory: URL,
         fileName: String,
         isCancelled: @escaping @Sendable () -> Bool
-    ) throws {
+    ) throws -> Set<String> {
+        var fetched: Set<String> = []
         for suffix in sidecars {
             if isCancelled() { throw SFTPError.cancelled }
-            let source = remotePath + suffix
-            guard session.exists(source) else { continue }
+            let remoteSidecar = remotePath + suffix
+            guard source.exists(remoteSidecar) else {
+                Self.logger.info("The \(suffix, privacy: .public) sidecar was gone before it was fetched")
+                continue
+            }
             let target = destinationDirectory.appendingPathComponent(fileName + suffix)
-            try session.download(remotePath: source, to: target, isCancelled: isCancelled)
+            try source.download(remotePath: remoteSidecar, to: target, progress: nil, isCancelled: isCancelled)
+            fetched.insert(suffix)
             Self.logger.info("Fetched the \(suffix, privacy: .public) sidecar")
         }
+        return fetched
     }
 
     // MARK: - Helpers

TablePro/Core/Services/Infrastructure/SampleDatabaseService.swift

@@ -35,6 +35,7 @@ internal final class SampleDatabaseService {
     )
 
     nonisolated private static let logger = Logger(subsystem: "com.TablePro", category: "SampleDatabaseService")
+    private static let installedFileName = "Chinook.sqlite"
 
     private let bundledFileResolver: () -> URL?
     private let fileManager: FileManager
@@ -58,7 +59,14 @@ internal final class SampleDatabaseService {
     }
 
     internal var installedFileURL: URL {
-        baseDirectoryProvider().appendingPathComponent("Chinook.sqlite", isDirectory: false)
+        baseDirectoryProvider().appendingPathComponent(Self.installedFileName, isDirectory: false)
+    }
+
+    private var installedSidecarURLs: [URL] {
+        let directory = baseDirectoryProvider()
+        return DatabaseFileLayout.sqliteFamily.staleAfterReplaceSuffixes.map { suffix in
+            directory.appendingPathComponent(Self.installedFileName + suffix, isDirectory: false)
+        }
     }
 
     internal func installIfNeeded() throws {
@@ -80,6 +88,8 @@ internal final class SampleDatabaseService {
             return
         }
 
+        try removeInstalledDatabaseFiles()
+
         do {
             try fileManager.copyItem(at: bundled, to: installed)
             Self.logger.info("Installed sample database to \(installed.path, privacy: .private(mask: .hash))")
@@ -105,13 +115,7 @@ internal final class SampleDatabaseService {
             throw SampleDatabaseError.copyFailed(message: error.localizedDescription)
         }
 
-        if fileManager.fileExists(atPath: installed.path) {
-            do {
-                try fileManager.removeItem(at: installed)
-            } catch {
-                throw SampleDatabaseError.copyFailed(message: error.localizedDescription)
-            }
-        }
+        try removeInstalledDatabaseFiles()
 
         do {
             try fileManager.copyItem(at: bundled, to: installed)
@@ -121,6 +125,17 @@ internal final class SampleDatabaseService {
         }
     }
 
+    private func removeInstalledDatabaseFiles() throws {
+        for url in [installedFileURL] + installedSidecarURLs where fileManager.fileExists(atPath: url.path) {
+            do {
+                try fileManager.removeItem(at: url)
+            } catch {
+                throw SampleDatabaseError.copyFailed(message: error.localizedDescription)
+            }
+            Self.logger.info("Removed sample database file \(url.lastPathComponent, privacy: .public)")
+        }
+    }
+
     internal func isSampleConnection(_ connection: DatabaseConnection) -> Bool {
         if connection.isSample { return true }
         guard connection.type == .sqlite else { return false }

TablePro/Models/Connection/DatabaseFileLayout.swift

@@ -34,11 +34,12 @@ enum DatabaseFileLayout: Sendable, Equatable {
         }
     }
 
-    /// Suffixes that are rebuilt from the main file and must be cleared after it is replaced, so a
-    /// reader cannot apply a log that belongs to the file that used to be there.
+    /// Suffixes of every file the engine keeps beside a database that must not outlive a replaced
+    /// main file, so a reader cannot roll back or replay a journal that belongs to the file that
+    /// used to be there.
     var staleAfterReplaceSuffixes: [String] {
         switch self {
-        case .sqliteFamily: return ["-wal", "-shm"]
+        case .sqliteFamily: return ["-journal", "-wal", "-shm"]
         case .duckdb: return [".wal"]
         case .plainText: return []
         }

TableProTests/Core/Database/RemoteDatabaseFileCorrectnessTests.swift

@@ -55,20 +55,39 @@ struct RemoteDatabaseFileCorrectnessTests {
 
     // MARK: - Stale sidecar clearing
 
-    @Test("A snapshot fetch clears a stale write-ahead log and shared-memory index")
-    func snapshotClearsStaleSidecars() throws {
+    @Test("A fetch that downloaded no sidecar clears a stale rollback journal, write-ahead log and shared-memory index")
+    func fetchWithoutSidecarsClearsEveryStaleSidecar() throws {
         let directory = try temporaryDirectory()
         let fileName = "app.db"
-        for suffix in ["", "-wal", "-shm"] {
+        for suffix in ["", "-journal", "-wal", "-shm"] {
             try Data("x".utf8).write(to: directory.appendingPathComponent(fileName + suffix))
         }
         RemoteDatabaseFileTransfer.clearStaleSidecars(
             layout: .sqliteFamily,
-            plan: .remoteSnapshot(executable: "sqlite3"),
+            keeping: [],
             destinationDirectory: directory,
             fileName: fileName
         )
         #expect(FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName).path))
+        #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-journal").path))
+        #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-wal").path))
+        #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-shm").path))
+    }
+
+    @Test("A direct copy keeps a rollback journal it fetched")
+    func directCopyKeepsFetchedJournal() throws {
+        let directory = try temporaryDirectory()
+        let fileName = "app.db"
+        for suffix in ["", "-journal", "-wal", "-shm"] {
+            try Data("x".utf8).write(to: directory.appendingPathComponent(fileName + suffix))
+        }
+        RemoteDatabaseFileTransfer.clearStaleSidecars(
+            layout: .sqliteFamily,
+            keeping: ["-journal"],
+            destinationDirectory: directory,
+            fileName: fileName
+        )
+        #expect(FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-journal").path))
         #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-wal").path))
         #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-shm").path))
     }
@@ -82,14 +101,47 @@ struct RemoteDatabaseFileCorrectnessTests {
         }
         RemoteDatabaseFileTransfer.clearStaleSidecars(
             layout: .sqliteFamily,
-            plan: .directCopy(sidecars: ["-wal"]),
+            keeping: ["-wal"],
             destinationDirectory: directory,
             fileName: fileName
         )
         #expect(FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-wal").path))
         #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-shm").path))
     }
 
+    @Test("A direct copy clears a stale rollback journal its plan listed but the server had dropped by fetch time")
+    func directCopyClearsAJournalTheServerDroppedAfterPlanning() throws {
+        let directory = try temporaryDirectory()
+        defer { try? FileManager.default.removeItem(at: directory) }
+        let fileName = "app.db"
+        for suffix in ["", "-journal", "-wal", "-shm"] {
+            try Data("stale".utf8).write(to: directory.appendingPathComponent(fileName + suffix))
+        }
+        let freshLog = Data("fresh log".utf8)
+        let server = StubRemoteFileSource(files: ["/srv/app.db-wal": freshLog])
+
+        let fetched = try RemoteDatabaseFileTransfer.fetchSidecars(
+            from: server,
+            remotePath: "/srv/app.db",
+            sidecars: ["-wal", "-journal"],
+            destinationDirectory: directory,
+            fileName: fileName,
+            isCancelled: { false }
+        )
+        RemoteDatabaseFileTransfer.clearStaleSidecars(
+            layout: .sqliteFamily,
+            keeping: fetched,
+            destinationDirectory: directory,
+            fileName: fileName
+        )
+
+        #expect(fetched == ["-wal"])
+        let log = try Data(contentsOf: directory.appendingPathComponent(fileName + "-wal"))
+        #expect(log == freshLog)
+        #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-journal").path))
+        #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent(fileName + "-shm").path))
+    }
+
     // MARK: - Killed remote command
 
     @Test("A signal-killed remote command does not report success")
@@ -123,3 +175,24 @@ struct RemoteDatabaseFileCorrectnessTests {
         #expect(!FileManager.default.fileExists(atPath: stale.path))
     }
 }
+
+private struct StubRemoteFileSource: RemoteFileSource {
+    let files: [String: Data]
+
+    func exists(_ path: String) -> Bool {
+        files[path] != nil
+    }
+
+    func download(
+        remotePath: String,
+        to localURL: URL,
+        progress: (@Sendable (UInt64, UInt64) -> Void)?,
+        isCancelled: @escaping @Sendable () -> Bool
+    ) throws -> (bytes: UInt64, sha256: String) {
+        guard let data = files[remotePath] else {
+            throw SFTPError.noSuchFile(path: remotePath)
+        }
+        try data.write(to: localURL)
+        return (bytes: UInt64(data.count), sha256: "")
+    }
+}

TableProTests/Core/Database/RemoteDatabaseFileTests.swift

@@ -86,6 +86,12 @@ struct RemoteDatabaseFileTests {
         #expect(!suffixes.contains("-shm"))
     }
 
+    @Test("Replacing a SQLite file clears its rollback journal, write-ahead log and shared-memory index")
+    func sqliteStaleSidecarsIncludeTheRollbackJournal() {
+        let suffixes = Set(DatabaseFileLayout.sqliteFamily.staleAfterReplaceSuffixes)
+        #expect(suffixes == ["-journal", "-wal", "-shm"])
+    }
+
     /// DuckDB writes `app.duckdb.wal`, with a dot. Taking SQLite's hyphen to it fetches nothing and
     /// leaves the real log behind to be replayed against a file it no longer matches.
     @Test("DuckDB's log is named with a dot, and SQLite's suffixes never reach it")