Fix SwiftData persistence concurrency stability

2026-05-29 00:06:57 +02:00
parent e5027152ab
commit ebc187f4a1
19 changed files with 554 additions and 132 deletions
@@ -1,7 +1,7 @@
 import Foundation
 import VelodyDomain

-public protocol LocalCatalogService: Sendable {
+public protocol LocalCatalogService: Actor {
    func loadActiveLocalTracks() async throws -> [LibraryTrack]
    func reconcileScanResults(
        _ scannedTracks: [ScannedLocalTrack],
@@ -1,7 +1,7 @@
 import Foundation
 import VelodyDomain

-public protocol LocalLibraryStore: Sendable {
+public protocol LocalLibraryStore: Actor {
    func loadTracks() async throws -> [LibraryTrack]
    func replaceTracks(_ tracks: [LibraryTrack]) async throws
 }
@@ -1,5 +1,5 @@
 import Foundation
-@preconcurrency import SwiftData
+import SwiftData
 import VelodyDomain

 public protocol TrackRepository: LocalLibraryStore {
@@ -23,8 +23,7 @@ public protocol TrackRepository: LocalLibraryStore {
 }

 public actor SwiftDataTrackRepository: TrackRepository {
-    private let modelContainer: ModelContainer
-    private let modelContext: ModelContext
+    private let database: SwiftDataCatalogDatabase

    public init(
        databaseURL: URL? = nil,
@@ -49,111 +48,55 @@ public actor SwiftDataTrackRepository: TrackRepository {
            configuration = ModelConfiguration(url: storeURL)
        }

-        modelContainer = try ModelContainer(
+        let modelContainer = try ModelContainer(
            for: TrackEntity.self,
            configurations: configuration
        )
-        modelContext = ModelContext(modelContainer)
-        modelContext.autosaveEnabled = false
+        database = SwiftDataCatalogDatabase(modelContainer: modelContainer)
    }

    public func loadTracks() async throws -> [LibraryTrack] {
-        try await loadLocalTracks(origin: nil, includeDeleted: false)
-            .map(\.libraryTrack)
+        try await database.loadTracks()
    }

    public func replaceTracks(_ tracks: [LibraryTrack]) async throws {
-        try await removeTracks(origin: .syncBootstrap)
-
-        let observedAt = Date()
-        for track in tracks {
-            try await saveLocalTrack(
-                LocalTrack(
-                    libraryTrack: track,
-                    origin: .syncBootstrap,
-                    observedAt: observedAt
-                )
-            )
-        }
+        try await database.replaceTracks(tracks)
    }

    public func loadLocalTracks(
        origin: LocalTrackOrigin?,
        includeDeleted: Bool
    ) async throws -> [LocalTrack] {
-        try fetchEntities()
-            .map(\.localTrack)
-            .filter { track in
-                let originMatches = origin.map { track.origin == $0 } ?? true
-                let deletedMatches = includeDeleted || !track.isDeleted
-                return originMatches && deletedMatches
-            }
-            .sorted(by: sortTracks(_:_:))
+        try await database.loadLocalTracks(
+            origin: origin,
+            includeDeleted: includeDeleted
+        )
    }

    public func findTrack(trackID: String) async throws -> LocalTrack? {
-        try fetchEntities()
-            .first(where: { $0.trackID == trackID })?
-            .localTrack
+        try await database.findTrack(trackID: trackID)
    }

    public func findTrack(deduplicationKey: String) async throws -> LocalTrack? {
-        try fetchEntities()
-            .first(where: { $0.deduplicationKey == deduplicationKey })?
-            .localTrack
+        try await database.findTrack(deduplicationKey: deduplicationKey)
    }

    public func findTrack(
        localFilePath: String,
        origin: LocalTrackOrigin?
    ) async throws -> LocalTrack? {
-        let matches = try fetchEntities()
-            .map(\.localTrack)
-            .filter { track in
-                let pathMatches = track.localFilePath == localFilePath
-                let originMatches = origin.map { track.origin == $0 } ?? true
-                return pathMatches && originMatches
-            }
-            .sorted { lhs, rhs in
-                if lhs.isDeleted != rhs.isDeleted {
-                    return !lhs.isDeleted
-                }
-
-                return lhs.updatedAt > rhs.updatedAt
-            }
-
-        return matches.first
+        try await database.findTrack(
+            localFilePath: localFilePath,
+            origin: origin
+        )
    }

    public func saveLocalTrack(_ track: LocalTrack) async throws {
-        let existingEntity = try findEntity(trackID: track.id)
-            ?? (try findEntity(deduplicationKey: track.deduplicationKey))
-
-        if let existingEntity {
-            existingEntity.apply(track)
-        } else {
-            modelContext.insert(TrackEntity(track: track))
-        }
-
-        try modelContext.save()
+        try await database.saveLocalTrack(track)
    }

    public func removeTracks(origin: LocalTrackOrigin?) async throws {
-        let matchingEntities = try fetchEntities().filter { entity in
-            guard let origin else {
-                return true
-            }
-
-            return entity.originRawValue == origin.rawValue
-        }
-
-        for entity in matchingEntities {
-            modelContext.delete(entity)
-        }
-
-        if !matchingEntities.isEmpty {
-            try modelContext.save()
-        }
+        try await database.removeTracks(origin: origin)
    }

    public func markDeletedLocalTracks(
@@ -161,26 +104,11 @@ public actor SwiftDataTrackRepository: TrackRepository {
        under rootFolderPath: String,
        scannedAt: Date
    ) async throws -> Int {
-        let localTracks = try fetchEntities()
-            .filter { entity in
-                entity.originRawValue == LocalTrackOrigin.localScan.rawValue
-                    && !entity.isMarkedDeleted
-                    && isWithinRootFolder(entity.localFilePath, rootFolderPath: rootFolderPath)
-                    && !scannedFilePaths.contains(entity.localFilePath)
-            }
-
-        for entity in localTracks {
-            entity.isMarkedDeleted = true
-            entity.deletedAt = scannedAt
-            entity.lastScannedAt = scannedAt
-            entity.updatedAt = scannedAt
-        }
-
-        if !localTracks.isEmpty {
-            try modelContext.save()
-        }
-
-        return localTracks.count
+        try await database.markDeletedLocalTracks(
+            missingFrom: scannedFilePaths,
+            under: rootFolderPath,
+            scannedAt: scannedAt
+        )
    }

    private static func defaultStoreURL(fileManager: FileManager) throws -> URL {
@@ -195,19 +123,6 @@ public actor SwiftDataTrackRepository: TrackRepository {
            .appendingPathComponent("Velody", isDirectory: true)
            .appendingPathComponent("local-catalog.store")
    }
-
-    private func fetchEntities() throws -> [TrackEntity] {
-        try modelContext.fetch(FetchDescriptor<TrackEntity>())
-    }
-
-    private func findEntity(trackID: String) throws -> TrackEntity? {
-        try fetchEntities().first(where: { $0.trackID == trackID })
-    }
-
-    private func findEntity(deduplicationKey: String) throws -> TrackEntity? {
-        try fetchEntities().first(where: { $0.deduplicationKey == deduplicationKey })
-    }
-
 }

 public actor InMemoryTrackRepository: TrackRepository {
@@ -335,6 +250,159 @@ public actor InMemoryTrackRepository: TrackRepository {

 public typealias InMemoryLocalLibraryStore = InMemoryTrackRepository

+@ModelActor
+private actor SwiftDataCatalogDatabase {
+    func loadTracks() throws -> [LibraryTrack] {
+        try loadLocalTracks(origin: nil, includeDeleted: false)
+            .map(\.libraryTrack)
+    }
+
+    func replaceTracks(_ tracks: [LibraryTrack]) throws {
+        let matchingEntities = try fetchEntities().filter { entity in
+            entity.originRawValue == LocalTrackOrigin.syncBootstrap.rawValue
+        }
+
+        for entity in matchingEntities {
+            modelContext.delete(entity)
+        }
+
+        let observedAt = Date()
+        for track in tracks {
+            modelContext.insert(
+                TrackEntity(
+                    track: LocalTrack(
+                        libraryTrack: track,
+                        origin: .syncBootstrap,
+                        observedAt: observedAt
+                    )
+                )
+            )
+        }
+
+        if !matchingEntities.isEmpty || !tracks.isEmpty {
+            try modelContext.save()
+        }
+    }
+
+    func loadLocalTracks(
+        origin: LocalTrackOrigin?,
+        includeDeleted: Bool
+    ) throws -> [LocalTrack] {
+        try fetchEntities()
+            .map(\.localTrack)
+            .filter { track in
+                let originMatches = origin.map { track.origin == $0 } ?? true
+                let deletedMatches = includeDeleted || !track.isDeleted
+                return originMatches && deletedMatches
+            }
+            .sorted(by: sortTracks(_:_:))
+    }
+
+    func findTrack(trackID: String) throws -> LocalTrack? {
+        try fetchEntities()
+            .first(where: { $0.trackID == trackID })?
+            .localTrack
+    }
+
+    func findTrack(deduplicationKey: String) throws -> LocalTrack? {
+        try fetchEntities()
+            .first(where: { $0.deduplicationKey == deduplicationKey })?
+            .localTrack
+    }
+
+    func findTrack(
+        localFilePath: String,
+        origin: LocalTrackOrigin?
+    ) throws -> LocalTrack? {
+        let matches = try fetchEntities()
+            .map(\.localTrack)
+            .filter { track in
+                let pathMatches = track.localFilePath == localFilePath
+                let originMatches = origin.map { track.origin == $0 } ?? true
+                return pathMatches && originMatches
+            }
+            .sorted { lhs, rhs in
+                if lhs.isDeleted != rhs.isDeleted {
+                    return !lhs.isDeleted
+                }
+
+                return lhs.updatedAt > rhs.updatedAt
+            }
+
+        return matches.first
+    }
+
+    func saveLocalTrack(_ track: LocalTrack) throws {
+        let existingEntity = try findEntity(trackID: track.id)
+            ?? (try findEntity(deduplicationKey: track.deduplicationKey))
+
+        if let existingEntity {
+            existingEntity.apply(track)
+        } else {
+            modelContext.insert(TrackEntity(track: track))
+        }
+
+        try modelContext.save()
+    }
+
+    func removeTracks(origin: LocalTrackOrigin?) throws {
+        let matchingEntities = try fetchEntities().filter { entity in
+            guard let origin else {
+                return true
+            }
+
+            return entity.originRawValue == origin.rawValue
+        }
+
+        for entity in matchingEntities {
+            modelContext.delete(entity)
+        }
+
+        if !matchingEntities.isEmpty {
+            try modelContext.save()
+        }
+    }
+
+    func markDeletedLocalTracks(
+        missingFrom scannedFilePaths: Set<String>,
+        under rootFolderPath: String,
+        scannedAt: Date
+    ) throws -> Int {
+        let localTracks = try fetchEntities()
+            .filter { entity in
+                entity.originRawValue == LocalTrackOrigin.localScan.rawValue
+                    && !entity.isMarkedDeleted
+                    && isWithinRootFolder(entity.localFilePath, rootFolderPath: rootFolderPath)
+                    && !scannedFilePaths.contains(entity.localFilePath)
+            }
+
+        for entity in localTracks {
+            entity.isMarkedDeleted = true
+            entity.deletedAt = scannedAt
+            entity.lastScannedAt = scannedAt
+            entity.updatedAt = scannedAt
+        }
+
+        if !localTracks.isEmpty {
+            try modelContext.save()
+        }
+
+        return localTracks.count
+    }
+
+    private func fetchEntities() throws -> [TrackEntity] {
+        try modelContext.fetch(FetchDescriptor<TrackEntity>())
+    }
+
+    private func findEntity(trackID: String) throws -> TrackEntity? {
+        try fetchEntities().first(where: { $0.trackID == trackID })
+    }
+
+    private func findEntity(deduplicationKey: String) throws -> TrackEntity? {
+        try fetchEntities().first(where: { $0.deduplicationKey == deduplicationKey })
+    }
+}
+
 private func sortTracks(_ lhs: LocalTrack, _ rhs: LocalTrack) -> Bool {
    let titleOrder = lhs.title.localizedCaseInsensitiveCompare(rhs.title)
    if titleOrder == .orderedSame {
@@ -1,4 +1,5 @@
 import Foundation
+import SwiftData
 import XCTest
@testable import VelodyPersistence
 import VelodyDomain
@@ -254,6 +255,119 @@ final class LocalCatalogServiceTests: XCTestCase {
        XCTAssertEqual(allTracks.count, 2)
    }

+    func testConcurrentScanCallsDoNotCrashAndRetainCatalogEntries() async throws {
+        let repository = try SwiftDataTrackRepository(isStoredInMemoryOnly: true)
+        let service = DefaultLocalCatalogService(repository: repository)
+        let folderURL = URL(fileURLWithPath: "/Music")
+
+        try await withThrowingTaskGroup(of: Void.self) { group in
+            for index in 0..<12 {
+                group.addTask {
+                    _ = try await service.reconcileScanResults(
+                        [
+                            self.makeScannedTrack(
+                                title: "Track \(index)",
+                                path: "/Music/Track-\(index).mp3",
+                                sha256: "sha-\(index)",
+                                modifiedAt: Date(timeIntervalSince1970: 6_000 + Double(index))
+                            ),
+                        ],
+                        in: folderURL,
+                        scannedAt: Date(timeIntervalSince1970: 6_100 + Double(index))
+                    )
+                }
+            }
+
+            try await group.waitForAll()
+        }
+
+        let storedTracks = try await repository.loadLocalTracks(
+            origin: .localScan,
+            includeDeleted: true
+        )
+        let activeTracks = try await repository.loadLocalTracks(
+            origin: .localScan,
+            includeDeleted: false
+        )
+
+        XCTAssertEqual(storedTracks.count, 12)
+        XCTAssertLessThanOrEqual(activeTracks.count, 1)
+    }
+
+    func testConcurrentSaveCallsDoNotCrash() async throws {
+        let repository = try SwiftDataTrackRepository(isStoredInMemoryOnly: true)
+
+        try await withThrowingTaskGroup(of: Void.self) { group in
+            for index in 0..<24 {
+                group.addTask {
+                    try await repository.saveLocalTrack(
+                        self.makeLocalTrack(
+                            id: "track-\(index)",
+                            title: "Track \(index)",
+                            path: "/Music/Track-\(index).mp3",
+                            sha256: "sha-save-\(index)",
+                            observedAt: Date(timeIntervalSince1970: 7_000 + Double(index))
+                        )
+                    )
+                }
+            }
+
+            try await group.waitForAll()
+        }
+
+        let storedTracks = try await repository.loadLocalTracks(
+            origin: .localScan,
+            includeDeleted: false
+        )
+
+        XCTAssertEqual(storedTracks.count, 24)
+    }
+
+    func testUploadStatusPersistenceSurvivesReload() async throws {
+        let repository = try SwiftDataTrackRepository(isStoredInMemoryOnly: true)
+        let observedAt = Date(timeIntervalSince1970: 8_000)
+        var track = makeLocalTrack(
+            id: "upload-track",
+            title: "Uploadable",
+            path: "/Music/Uploadable.mp3",
+            sha256: "sha-uploadable",
+            observedAt: observedAt
+        )
+
+        try await repository.saveLocalTrack(track)
+
+        track.uploadStatus = .uploaded
+        track.remoteTrackId = "remote-track-42"
+        track.lastUploadError = nil
+        track.updatedAt = Date(timeIntervalSince1970: 8_100)
+
+        try await repository.saveLocalTrack(track)
+
+        let persistedTrack = try await repository.findTrack(trackID: track.id)
+        let reloadedTrack = try XCTUnwrap(persistedTrack)
+
+        XCTAssertEqual(reloadedTrack.uploadStatus, .uploaded)
+        XCTAssertEqual(reloadedTrack.remoteTrackId, "remote-track-42")
+        XCTAssertNil(reloadedTrack.lastUploadError)
+    }
+
+    func testRepositoryAndCatalogServiceAreActorTypes() throws {
+        let repository = try SwiftDataTrackRepository(isStoredInMemoryOnly: true)
+        let service = DefaultLocalCatalogService(repository: repository)
+
+        assertActorIsolation(repository)
+        assertActorIsolation(service)
+    }
+
+    func testRepositoryDoesNotStoreModelContext() throws {
+        let repository = try SwiftDataTrackRepository(isStoredInMemoryOnly: true)
+        let storesModelContext = Mirror(reflecting: repository).children.contains { child in
+            child.value is ModelContext
+        }
+
+        XCTAssertFalse(storesModelContext)
+    }
+
    private func makeScannedTrack(
        title: String,
        path: String,
@@ -270,4 +384,36 @@ final class LocalCatalogServiceTests: XCTestCase {
            fileModifiedAt: modifiedAt
        )
    }
+
+    private func makeLocalTrack(
+        id: String,
+        title: String,
+        path: String,
+        sha256: String,
+        observedAt: Date
+    ) -> LocalTrack {
+        LocalTrack(
+            id: id,
+            origin: .localScan,
+            title: title,
+            artist: "Artist",
+            album: "Album",
+            durationSeconds: 180,
+            localFilePath: path,
+            sha256: sha256,
+            uploadStatus: .localOnly,
+            remoteTrackId: nil,
+            lastUploadError: nil,
+            fileModifiedAt: observedAt,
+            lastScannedAt: observedAt,
+            isDeleted: false,
+            deletedAt: nil,
+            createdAt: observedAt,
+            updatedAt: observedAt
+        )
+    }
+
+    private func assertActorIsolation<T: Actor>(_ value: T) {
+        XCTAssertNotNil(value as AnyObject)
+    }
 }