diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e8b31f..507b117 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes to VaultSync are documented here. ### Fixed +- **Folder access now stays intact when reconnecting or syncing in the background** ([#147](https://github.com/psimaker/vaultsync/issues/147)) — reselecting the same Obsidian folder no longer accumulates access claims. Switching folders takes effect only after the new location is readable, scanned, and its permission is saved; any failure keeps the previous folder connected. Background runs release only their own access on completion, restart, or cancellation. - **Background sync no longer reports unfinished work as completed** ([#146](https://github.com/psimaker/vaultsync/issues/146)) — continued processing now reports success only after every expected vault is confirmed fully idle. If the sync engine stops, vault status cannot be read, a vault reports an error, the run expires or is cancelled, or the app returns to the foreground, the background run reports failure instead; conflict checks happen only after idle is proven. - **Conflicting Obsidian settings now wait for your decision** ([#145](https://github.com/psimaker/vaultsync/issues/145)) — VaultSync no longer automatically deletes, replaces, or promotes `.obsidian` conflict copies by modification time. The legacy preference stays stored but cannot re-enable the retired behavior, and detected conflicts remain visible for manual review. Syncthing's separate conflict-copy retention is not guaranteed. - **Keep Both no longer overwrites an existing conflict copy** ([#144](https://github.com/psimaker/vaultsync/issues/144)) — when the intended copy name is already occupied, VaultSync leaves all existing files untouched instead of replacing previously saved bytes. diff --git a/docs/decisions/030-security-scoped-lease-takeover.md b/docs/decisions/030-security-scoped-lease-takeover.md new file mode 100644 index 0000000..82711e3 --- /dev/null +++ b/docs/decisions/030-security-scoped-lease-takeover.md @@ -0,0 +1,9 @@ +# 030 — Security-scoped access uses owned transactional leases + +- Context: Folder grants could start access repeatedly, replace visible/bookmark state before a successful scan, and let a stale background refresh overwrite a newer folder choice (#147). +- Decision: Every successful security-scope start creates one idempotent owner token that performs exactly one matching stop; a failed start creates no token. +- Decision: A foreground replacement validates, scans, and prepares its bookmark before committing the new URL, bookmark, lease, and visible state. The prior lease remains active until adoption; failure preserves it, and reselecting the same resource reuses it. +- Decision: Stale bookmark refreshes compare-and-swap the exact bytes they resolved, while each background run owns and releases only its distinct token on every terminal path, including restart and cancellation. +- Why: Explicit ownership and commit ordering prevent leaked access, double stops, false success, lost prior access, and stale last-writer-wins rollback. +- Rejected alternative: URL/Boolean bookkeeping, stop calls hidden in scan helpers, or unconditional stale-bookmark writes, because none proves which successful start is being released or whether a newer choice must win. +- Links: issue [#147](https://github.com/psimaker/vaultsync/issues/147); `BookmarkService.swift`; `VaultManager.swift`; `BackgroundSyncService.swift`. diff --git a/ios/VaultSync/Services/BackgroundSyncService.swift b/ios/VaultSync/Services/BackgroundSyncService.swift index 4592e07..fc8dd08 100644 --- a/ios/VaultSync/Services/BackgroundSyncService.swift +++ b/ios/VaultSync/Services/BackgroundSyncService.swift @@ -12,6 +12,57 @@ private struct UnsafeSendable: @unchecked Sendable { let value: T } +/// Closes the race between installing a BGTask expiration handler and starting +/// the Swift task it must cancel. A new task gates its operation on this lock, +/// so expiry is either already recorded or has a published cancellation before +/// any background work begins. +final class BackgroundTaskCancellationRelay: @unchecked Sendable { + private struct State { + var expired = false + var cancel: (@Sendable () -> Void)? + } + + private let state = OSAllocatedUnfairLock(initialState: State()) + + func makeTask( + operation: @escaping @Sendable () async -> Success + ) -> Task { + state.withLock { state in + let task = Task { [self] in + // Task creation happens while the relay lock is held. This + // first read cannot complete until cancellation is published + // or prior expiry has cancelled the task. + waitUntilPublished() + return await operation() + } + if state.expired { + task.cancel() + } else { + state.cancel = { task.cancel() } + } + return task + } + } + + private func waitUntilPublished() { + state.withLock { _ in } + } + + func expire() { + let cancel = state.withLock { state -> (@Sendable () -> Void)? in + guard !state.expired else { return nil } + state.expired = true + defer { state.cancel = nil } + return state.cancel + } + cancel?() + } + + func finish() { + state.withLock { $0.cancel = nil } + } +} + /// Tracks whether the foreground (SyncthingManager) owns the Syncthing lifecycle. /// Both SyncthingManager and BackgroundSyncService coordinate through this lock /// to prevent races where background stops a foreground-managed instance or vice versa. @@ -578,6 +629,22 @@ enum BackgroundSyncService { static func performBackgroundSync( reason: String, maxDuration: TimeInterval = 25 + ) async -> SyncResult { + await BackgroundSecurityScopedAccess.withRunOwnedLease { managedAccess, accessOwner in + await performBackgroundSyncRun( + reason: reason, + maxDuration: maxDuration, + managedAccess: managedAccess, + accessOwner: accessOwner + ) + } + } + + private static func performBackgroundSyncRun( + reason: String, + maxDuration: TimeInterval, + managedAccess: BackgroundSecurityScopedAccess, + accessOwner: SecurityScopedLeaseOwner ) async -> SyncResult { logger.info("Background sync starting (reason=\(reason))") trace("Starting background sync (reason=\(reason), maxDuration=\(Int(maxDuration))s).") @@ -606,6 +673,14 @@ enum BackgroundSyncService { var telemetryEventCursor = latestBridgeEventID() let syncStartEventCursor = telemetryEventCursor + if Task.isCancelled { + return completeCancelledSync( + reason: reason, + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + } + // Lifecycle decisions run through the injectable guard core — the // decision logic and its read-timing are pinned by unit tests (#61). let guards = BackgroundSyncGuards(environment: .live) @@ -627,10 +702,10 @@ enum BackgroundSyncService { var ownsLifecycle = ownership.backgroundOwns trace("Lifecycle ownership: foregroundOwns=\(ownership.foregroundOwns), backgroundOwns=\(ownsLifecycle).") - var managedURLs: [URL] = [] if ownsLifecycle { - managedURLs = restoreBookmarkAccess() - guard !managedURLs.isEmpty else { + guard managedAccess.ensureAccess(using: { + restoreBookmarkAccess(owner: accessOwner) + }) else { logger.info("No bookmarks restored") trace("Bookmark restore failed: no security-scoped access available.") return completeSync( @@ -641,14 +716,21 @@ enum BackgroundSyncService { initialEventCursor: syncStartEventCursor ) } - trace("Managed access restored (count=\(managedURLs.count)).") + trace("Managed security-scoped access restored.") + + if Task.isCancelled { + return completeCancelledSync( + reason: reason, + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + } let configDir = syncthingConfigDir() let err = SyncBridgeService.startSyncthing(configDir: configDir) if let err, !err.isEmpty, !SyncBridgeService.isRunning() { logger.error("Background bridge start failed") trace("Bridge start failed.") - releaseAccess(managedURLs) return completeSync( reason: reason, result: .bridgeStartFailed, @@ -661,6 +743,15 @@ enum BackgroundSyncService { } let hasFolders = await waitForAnyFolders(maxWait: 3) + if Task.isCancelled { + let completion = completeCancelledSync( + reason: reason, + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + if ownsLifecycle { cleanupBackgroundManaged() } + return completion + } trace("Folder availability check completed: hasFolders=\(hasFolders).") traceFolderStatuses(label: "post-folder-availability") guard hasFolders else { @@ -672,7 +763,7 @@ enum BackgroundSyncService { initialEventCursor: syncStartEventCursor ) if ownsLifecycle { - cleanupBackgroundManaged(managedURLs) + cleanupBackgroundManaged() } return completion } @@ -680,7 +771,7 @@ enum BackgroundSyncService { if ownsLifecycle { // Re-point any folder whose stored absolute path went stale after an // iOS container change, before syncing against it (issue #25). - FolderPathReconciler.reconcileLive(obsidianRoot: managedURLs.first?.path) + FolderPathReconciler.reconcileLive(obsidianRoot: managedAccess.url?.path) } var progressTracker = reason == "silent-push" @@ -693,6 +784,15 @@ enum BackgroundSyncService { if reason == "silent-push" { let sawWakeEvidence = await waitForSilentPushWakeEvidence(maxWait: 4) + if Task.isCancelled { + let completion = completeCancelledSync( + reason: reason, + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + if ownsLifecycle { cleanupBackgroundManaged() } + return completion + } trace("Silent push wake evidence: \(sawWakeEvidence).") traceRelevantBridgeEvents(since: &telemetryEventCursor, label: "post-wake-evidence-window") traceFolderStatuses(label: "post-wake-evidence-window") @@ -703,7 +803,51 @@ enum BackgroundSyncService { if guards.shouldForceRestartForSilentPush(sawWakeEvidence: sawWakeEvidence) { logger.warning("Silent push showed no peer/sync activity after rescan — forcing Syncthing restart") trace("No wake evidence after rescan. Forcing Syncthing restart.") - let restart = await forceRestartForSilentPush(managedURLs: managedURLs) + // Security-scope ownership is independent: reuse this run's + // token, or acquire exactly one before stopping/restarting. + // Do not adopt lifecycle ownership until the restart will + // actually begin; a pre-restart failure must not stop an + // already-running engine this cycle did not start. + guard managedAccess.ensureAccess(using: { + restoreBookmarkAccess(owner: accessOwner) + }) else { + trace("Forced restart could not restore bookmark access.") + let completion = completeSync( + reason: reason, + result: .bridgeStartFailed, + detail: L10n.tr("No security-scoped bookmark access was available."), + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + if ownsLifecycle { + cleanupBackgroundManaged() + } + return completion + } + + if Task.isCancelled { + let completion = completeCancelledSync( + reason: reason, + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + if ownsLifecycle { + cleanupBackgroundManaged() + } + return completion + } + + ownsLifecycle = true + let restart = await forceRestartForSilentPush() + if Task.isCancelled { + let completion = completeCancelledSync( + reason: reason, + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + cleanupBackgroundManaged() + return completion + } guard restart.success else { trace("Forced restart failed.") let completion = completeSync( @@ -713,14 +857,10 @@ enum BackgroundSyncService { startedAt: syncStartedAt, initialEventCursor: syncStartEventCursor ) - if restart.ownsLifecycle { - cleanupBackgroundManaged(restart.managedURLs) - } + cleanupBackgroundManaged() return completion } - managedURLs = restart.managedURLs - ownsLifecycle = restart.ownsLifecycle forcedRestartPerformed = true progressTracker?.requiresLocalDataProgress = true progressTracker?.lastEventID = latestBridgeEventID() @@ -728,8 +868,17 @@ enum BackgroundSyncService { trace("Silent push local-progress tracking reset after forced restart.") let recoveredFolders = await waitForAnyFolders(maxWait: 3) + if Task.isCancelled { + let completion = completeCancelledSync( + reason: reason, + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + cleanupBackgroundManaged() + return completion + } trace("Post-restart folder availability: \(recoveredFolders).") - trace("Managed access restored after restart (count=\(managedURLs.count)).") + trace("Managed security-scoped access retained after restart.") traceFolderStatuses(label: "post-forced-restart") guard recoveredFolders else { let completion = completeSync( @@ -740,13 +889,13 @@ enum BackgroundSyncService { initialEventCursor: syncStartEventCursor ) if ownsLifecycle { - cleanupBackgroundManaged(managedURLs) + cleanupBackgroundManaged() } return completion } if ownsLifecycle { - FolderPathReconciler.reconcileLive(obsidianRoot: managedURLs.first?.path) + FolderPathReconciler.reconcileLive(obsidianRoot: managedAccess.url?.path) } if let rescanCount = requestFolderRescans() { @@ -768,6 +917,15 @@ enum BackgroundSyncService { // the conflict scan reads through it. notifyConflictsIfAny is // internally gated (foreground/toggle/suppression), so it's cheap. await notifyConflictsIfAny() + if Task.isCancelled { + let completion = completeCancelledSync( + reason: reason, + startedAt: syncStartedAt, + initialEventCursor: syncStartEventCursor + ) + if ownsLifecycle { cleanupBackgroundManaged() } + return completion + } let completion = completeSync( reason: reason, result: .alreadyIdle, @@ -777,7 +935,7 @@ enum BackgroundSyncService { localDataProgressObserved: setupProgressSnapshot?.sawLocalDataProgress == true ) if ownsLifecycle { - cleanupBackgroundManaged(managedURLs) + cleanupBackgroundManaged() } return completion } @@ -824,7 +982,10 @@ enum BackgroundSyncService { let result: SyncResult let detail: String? - if let progressSnapshot, forcedRestartPerformed, progressSnapshot.requiresLocalDataProgress { + if Task.isCancelled { + result = .failed + detail = SyncResult.failed.issueMessage + } else if let progressSnapshot, forcedRestartPerformed, progressSnapshot.requiresLocalDataProgress { result = .failed detail = L10n.tr("Silent push restarted Syncthing, but no real sync progress was observed before the app returned to idle.") } else if idle { @@ -852,7 +1013,7 @@ enum BackgroundSyncService { ) // Only stop if we started it and foreground hasn't taken over. if ownsLifecycle { - cleanupBackgroundManaged(managedURLs) + cleanupBackgroundManaged() } return completion } @@ -865,7 +1026,9 @@ enum BackgroundSyncService { scheduleAppRefresh() } + let cancellationRelay = BackgroundTaskCancellationRelay() task.expirationHandler = { + cancellationRelay.expire() let shouldStop = lifecycleLock.withLock { !$0.foregroundActive } if shouldStop { SyncBridgeService.stopSyncthing() @@ -875,7 +1038,11 @@ enum BackgroundSyncService { } } - let result = await performBackgroundSync(reason: "app-refresh") + let syncTask = cancellationRelay.makeTask { + await performBackgroundSync(reason: "app-refresh") + } + let result = await syncTask.value + cancellationRelay.finish() task.setTaskCompleted(success: result.isSuccessful) } @@ -892,7 +1059,9 @@ enum BackgroundSyncService { scheduleProcessing() } + let cancellationRelay = BackgroundTaskCancellationRelay() task.expirationHandler = { + cancellationRelay.expire() let shouldStop = lifecycleLock.withLock { !$0.foregroundActive } if shouldStop { SyncBridgeService.stopSyncthing() @@ -902,7 +1071,11 @@ enum BackgroundSyncService { } } - let result = await performBackgroundSync(reason: "processing", maxDuration: 180) + let syncTask = cancellationRelay.makeTask { + await performBackgroundSync(reason: "processing", maxDuration: 180) + } + let result = await syncTask.value + cancellationRelay.finish() task.setTaskCompleted(success: result.isSuccessful) } @@ -1217,6 +1390,7 @@ enum BackgroundSyncService { private static func waitForAnyFolders(maxWait: TimeInterval) async -> Bool { let deadline = Date(timeIntervalSinceNow: maxWait) while Date() < deadline { + if Task.isCancelled { return false } if hasConfiguredFolders() { return true } @@ -1225,6 +1399,7 @@ enum BackgroundSyncService { } try? await Task.sleep(for: .milliseconds(250)) } + if Task.isCancelled { return false } return hasConfiguredFolders() } @@ -1237,12 +1412,13 @@ enum BackgroundSyncService { return !folders.isEmpty } - private static func cleanupBackgroundManaged(_ managedURLs: [URL]) { + /// Engine cleanup only. The run-owned security-scope token is released by + /// `withRunOwnedLease` regardless of lifecycle takeover. + private static func cleanupBackgroundManaged() { let shouldStop = lifecycleLock.withLock { !$0.foregroundActive } if shouldStop && SyncBridgeService.isRunning() { SyncBridgeService.stopSyncthing() } - releaseAccess(managedURLs) } /// Widget tier for a background completion (#76). Pure so the matrix is @@ -1329,6 +1505,21 @@ enum BackgroundSyncService { /// `cleanupBackgroundManaged` — the snapshot reads the folder count and /// the synced-file events through the bridge, and cleanup may stop it /// (#76 follow-up). + private static func completeCancelledSync( + reason: String, + startedAt: Date, + initialEventCursor: Int + ) -> SyncResult { + trace("Background sync cancelled; releasing run-owned access.") + return completeSync( + reason: reason, + result: .failed, + detail: SyncResult.failed.issueMessage, + startedAt: startedAt, + initialEventCursor: initialEventCursor + ) + } + @discardableResult private static func completeSync( reason: String, @@ -1605,31 +1796,42 @@ enum BackgroundSyncService { return count } - private static func restoreBookmarkAccess() -> [URL] { + private static func restoreBookmarkAccess( + owner: SecurityScopedLeaseOwner + ) -> SecurityScopedLease? { let id = "obsidian-root" - guard let (url, isStale) = BookmarkService.resolveBookmark(identifier: id) else { + guard let resolvedBookmark = BookmarkService.resolveBookmark(identifier: id) else { trace("Bookmark lookup returned no stored Obsidian root.") - return [] + return nil } - guard BookmarkService.startAccessing(url: url) else { + let url = resolvedBookmark.url + guard let lease = BookmarkService.acquireAccess(to: url, owner: owner) else { trace("Bookmark access start failed.") - return [] + return nil } - if isStale { + if resolvedBookmark.isStale { + let refreshedData: Data do { - try BookmarkService.saveBookmark(for: url, identifier: id) - logger.info("Refreshed stale Obsidian bookmark in background") - trace("Refreshed stale bookmark during background sync.") + refreshedData = try BookmarkService.makeBookmarkData(for: url) } catch { logger.warning("Could not refresh stale Obsidian bookmark") trace("Failed to refresh stale bookmark during background sync.") + lease.release() + return nil + } + guard BookmarkService.refreshBookmarkData( + refreshedData, + replacing: resolvedBookmark.sourceData, + identifier: id + ) else { + trace("Skipped stale bookmark refresh after a concurrent permission change.") + lease.release() + return nil } + logger.info("Refreshed stale Obsidian bookmark in background") + trace("Refreshed stale bookmark during background sync.") } - return [url] - } - - private static func releaseAccess(_ urls: [URL]) { - urls.forEach { BookmarkService.stopAccessing(url: $0) } + return lease } private static func syncthingConfigDir() -> String { @@ -1652,6 +1854,7 @@ enum BackgroundSyncService { private static func waitForSilentPushWakeEvidence(maxWait: TimeInterval) async -> Bool { let deadline = Date(timeIntervalSinceNow: maxWait) while Date() < deadline { + if Task.isCancelled { return false } let hasPeer = hasAnyConnectedPeer() let idle = allFoldersIdle() if hasPeer || !idle { @@ -1664,6 +1867,7 @@ enum BackgroundSyncService { } try? await Task.sleep(for: .milliseconds(400)) } + if Task.isCancelled { return false } let hasPeer = hasAnyConnectedPeer() let idle = allFoldersIdle() trace("Wake evidence window ended: connectedPeer=\(hasPeer), allFoldersIdle=\(idle).") @@ -1679,33 +1883,26 @@ enum BackgroundSyncService { return devices.contains { $0.connected && !$0.paused } } - private static func forceRestartForSilentPush( - managedURLs existingManagedURLs: [URL] - ) async -> (success: Bool, ownsLifecycle: Bool, managedURLs: [URL], errorDetail: String?) { - var managedURLs = existingManagedURLs - if managedURLs.isEmpty { - managedURLs = restoreBookmarkAccess() - if managedURLs.isEmpty { - trace("Forced restart could not restore bookmark access.") - return (false, true, [], "No security-scoped bookmark access was available for forced restart.") - } - } - + private static func forceRestartForSilentPush() + async -> (success: Bool, errorDetail: String?) { if SyncBridgeService.isRunning() { trace("Forced restart stopping running bridge first.") SyncBridgeService.stopSyncthing() try? await Task.sleep(for: .milliseconds(350)) + if Task.isCancelled { + return (false, SyncResult.failed.issueMessage) + } } let configDir = syncthingConfigDir() let err = SyncBridgeService.startSyncthing(configDir: configDir) if let err, !err.isEmpty, !SyncBridgeService.isRunning() { trace("Forced restart start failed.") - return (false, true, managedURLs, L10n.tr("The embedded sync engine could not restart.")) + return (false, L10n.tr("The embedded sync engine could not restart.")) } trace("Forced restart started bridge successfully.") - return (true, true, managedURLs, nil) + return (true, nil) } private static func traceFolderStatuses(label: String) { diff --git a/ios/VaultSync/Services/BookmarkService.swift b/ios/VaultSync/Services/BookmarkService.swift index 6eec5c1..1e1625e 100644 --- a/ios/VaultSync/Services/BookmarkService.swift +++ b/ios/VaultSync/Services/BookmarkService.swift @@ -8,25 +8,75 @@ private let logger = Logger(subsystem: "eu.vaultsync.app", category: "bookmarks" struct BookmarkService { private static let bookmarkPrefix = "vault_bookmark_" + /// Serializes bookmark snapshots and writes so stale-refresh compare-and- + /// swap cannot overwrite a newer foreground folder takeover. + private static let storeLock = OSAllocatedUnfairLock(initialState: ()) - static func saveBookmark(for url: URL, identifier: String) throws { - let data = try url.bookmarkData( + struct ResolvedBookmark: Equatable, Sendable { + let url: URL + let isStale: Bool + /// Exact bytes read before resolution. A stale refresh may replace + /// them only while they are still the committed value. + let sourceData: Data + } + + /// The only fallible bookmark step. Keeping data creation separate from + /// persistence lets a takeover prove every failure before replacing the + /// previously committed bookmark. + static func makeBookmarkData(for url: URL) throws -> Data { + try url.bookmarkData( options: .minimalBookmark, includingResourceValuesForKeys: nil, relativeTo: nil ) - UserDefaults.standard.set(data, forKey: bookmarkPrefix + identifier) + } + + /// Non-throwing commit to the process store. Call only after all candidate + /// validation and scanning has succeeded. + static func persistBookmarkData(_ data: Data, identifier: String) { + storeLock.withLock { + UserDefaults.standard.set(data, forKey: bookmarkPrefix + identifier) + } logger.info("Security-scoped bookmark saved") } + /// Refresh stale bytes only if no newer grant or refresh has replaced the + /// exact bookmark snapshot that was resolved. This is a single locked + /// compare-and-swap with every other BookmarkService write. + static func refreshBookmarkData( + _ data: Data, + replacing sourceData: Data, + identifier: String + ) -> Bool { + let didRefresh = storeLock.withLock { + let key = bookmarkPrefix + identifier + guard UserDefaults.standard.data(forKey: key) == sourceData else { + return false + } + UserDefaults.standard.set(data, forKey: key) + return true + } + if didRefresh { + logger.info("Security-scoped bookmark refreshed") + } else { + logger.info("Skipped stale bookmark refresh because the stored permission changed") + } + return didRefresh + } + static func deleteBookmark(identifier: String) { - UserDefaults.standard.removeObject(forKey: bookmarkPrefix + identifier) + storeLock.withLock { + UserDefaults.standard.removeObject(forKey: bookmarkPrefix + identifier) + } logger.info("Security-scoped bookmark deleted") } - /// Returns the resolved URL and whether the bookmark is stale (file moved/renamed). - static func resolveBookmark(identifier: String) -> (url: URL, isStale: Bool)? { - guard let data = UserDefaults.standard.data(forKey: bookmarkPrefix + identifier) else { + /// Returns the resolved URL, stale flag, and the exact source bytes needed + /// for an atomic stale refresh. + static func resolveBookmark(identifier: String) -> ResolvedBookmark? { + guard let data = storeLock.withLock({ + UserDefaults.standard.data(forKey: bookmarkPrefix + identifier) + }) else { logger.warning("No security-scoped bookmark data available") return nil } @@ -40,38 +90,56 @@ struct BookmarkService { if isStale { logger.warning("Security-scoped bookmark is stale") } - return (url, isStale) + return ResolvedBookmark(url: url, isStale: isStale, sourceData: data) } catch { logger.error("Failed to resolve security-scoped bookmark") return nil } } - /// Access is process-wide — Go code via gomobile also gains access. - @discardableResult - static func startAccessing(url: URL) -> Bool { - let success = url.startAccessingSecurityScopedResource() - if success { - logger.info("Started security-scoped access") - } else { + /// Injectable boundary around the Foundation security-scope calls. The + /// live callbacks stay here so every successful start can be represented + /// by one owned `SecurityScopedLease` in app code and a counter in tests. + struct AccessEnvironment: Sendable { + var start: @Sendable (URL) -> Bool + var stop: @Sendable (URL) -> Void + + static let live = Self( + start: { $0.startAccessingSecurityScopedResource() }, + stop: { $0.stopAccessingSecurityScopedResource() } + ) + } + + /// Access is process-wide — Go code via gomobile also gains access. A + /// failed start returns no token and therefore can never cause a stop. + static func acquireAccess( + to url: URL, + owner: SecurityScopedLeaseOwner, + environment: AccessEnvironment = .live + ) -> SecurityScopedLease? { + guard environment.start(url) else { logger.error("Failed to start security-scoped access") + return nil } - return success - } - /// Only call after Syncthing has stopped using the directory. - static func stopAccessing(url: URL) { - url.stopAccessingSecurityScopedResource() - logger.info("Stopped security-scoped access") + logger.info("Started security-scoped access") + return SecurityScopedLease(url: url, owner: owner) { releasedURL in + environment.stop(releasedURL) + logger.info("Stopped security-scoped access") + } } static func allBookmarkIdentifiers() -> [String] { - UserDefaults.standard.dictionaryRepresentation().keys - .filter { $0.hasPrefix(bookmarkPrefix) } - .map { String($0.dropFirst(bookmarkPrefix.count)) } + storeLock.withLock { + UserDefaults.standard.dictionaryRepresentation().keys + .filter { $0.hasPrefix(bookmarkPrefix) } + .map { String($0.dropFirst(bookmarkPrefix.count)) } + } } static func hasBookmark(identifier: String) -> Bool { - UserDefaults.standard.data(forKey: bookmarkPrefix + identifier) != nil + storeLock.withLock { + UserDefaults.standard.data(forKey: bookmarkPrefix + identifier) != nil + } } } diff --git a/ios/VaultSync/Services/SecurityScopedLease.swift b/ios/VaultSync/Services/SecurityScopedLease.swift new file mode 100644 index 0000000..538ac58 --- /dev/null +++ b/ios/VaultSync/Services/SecurityScopedLease.swift @@ -0,0 +1,121 @@ +import Foundation +import os + +/// The subsystem that owns one successful security-scoped access start. +/// Foreground and background starts remain distinct even for the same URL. +enum SecurityScopedLeaseOwner: Equatable, Sendable { + case foreground + case background(UUID) +} + +/// A one-shot capability to balance exactly one successful +/// `startAccessingSecurityScopedResource()` call. +/// +/// The unchecked Sendable conformance is narrow: the URL, owner, identifier, +/// and stop callback are immutable, while the only mutable bit is protected by +/// `OSAllocatedUnfairLock`. Callers still release explicitly; deinit is only a +/// final leak backstop. +final class SecurityScopedLease: @unchecked Sendable { + let id: UUID + let url: URL + let owner: SecurityScopedLeaseOwner + + private let active = OSAllocatedUnfairLock(initialState: true) + private let stop: @Sendable (URL) -> Void + + init( + id: UUID = UUID(), + url: URL, + owner: SecurityScopedLeaseOwner, + stop: @escaping @Sendable (URL) -> Void + ) { + self.id = id + self.url = url + self.owner = owner + self.stop = stop + } + + var isActive: Bool { + active.withLock { $0 } + } + + /// Releases this lease at most once, even when terminal paths race or a + /// defensive cleanup repeats. + func release() { + let shouldStop = active.withLock { isActive in + guard isActive else { return false } + isActive = false + return true + } + if shouldStop { + stop(url) + } + } + + deinit { + release() + } +} + +/// Owns the single security-scoped lease acquired by one background run. +/// Production and tests use this same core for forced-restart reuse and +/// idempotent terminal cleanup. +final class BackgroundSecurityScopedAccess: @unchecked Sendable { + private let lease = OSAllocatedUnfairLock(initialState: nil) + + /// Runs one background operation with a distinct owner and guarantees the + /// operation's token is released on every return, including cancellation. + /// `BackgroundSyncService` and its cancellation regression use this exact + /// scope rather than duplicating terminal cleanup logic. + static func withRunOwnedLease( + _ operation: @Sendable ( + BackgroundSecurityScopedAccess, + SecurityScopedLeaseOwner + ) async -> Result + ) async -> Result { + let managedAccess = BackgroundSecurityScopedAccess() + let owner = SecurityScopedLeaseOwner.background(UUID()) + defer { managedAccess.release() } + return await operation(managedAccess, owner) + } + + var url: URL? { + lease.withLock { $0?.url } + } + + var hasLease: Bool { + lease.withLock { $0 != nil } + } + + /// Keeps an existing run-owned lease (forced restart) or acquires exactly + /// one new lease. A failed acquire leaves the owner empty. + @discardableResult + func ensureAccess( + using acquire: @Sendable () -> SecurityScopedLease? + ) -> Bool { + lease.withLock { current in + if current != nil { + return true + } + guard let acquired = acquire() else { + return false + } + current = acquired + return true + } + } + + /// Detaches before stopping so re-entrant or repeated cleanup cannot + /// consume the same token twice. + func release() { + let owned = lease.withLock { current in + defer { current = nil } + return current + } + owned?.release() + } + + deinit { + release() + } +} diff --git a/ios/VaultSync/Services/VaultManager.swift b/ios/VaultSync/Services/VaultManager.swift index c6e9034..3a9fec5 100644 --- a/ios/VaultSync/Services/VaultManager.swift +++ b/ios/VaultSync/Services/VaultManager.swift @@ -36,71 +36,147 @@ final class VaultManager { private static let obsidianBookmarkID = "obsidian-root" + /// Lease ownership is separate from UI accessibility. A visible Boolean + /// cannot prove which successful start a later stop is allowed to consume. + @ObservationIgnored private var foregroundLease: SecurityScopedLease? + + /// External effects used by the foreground access path. Production uses + /// the real bookmark/filesystem APIs; tests inject deterministic counters + /// and failures so lease ownership can be proven without a file provider. + struct Environment { + var acquireAccess: @MainActor (URL, SecurityScopedLeaseOwner) -> SecurityScopedLease? + var sameResource: @MainActor (URL, URL) -> Bool + var validateDirectory: @MainActor (URL) -> String? + var makeBookmarkData: @MainActor (URL) throws -> Data + var persistBookmarkData: @MainActor (Data) -> Void + var refreshBookmarkData: @MainActor (Data, Data) -> Bool + var resolveBookmark: @MainActor () -> BookmarkService.ResolvedBookmark? + var hasBookmark: @MainActor () -> Bool + var scanVaults: @MainActor (URL) -> [String]? + var cleanupLegacyBookmarks: @MainActor () -> Void + + @MainActor + static var live: Self { + Self( + acquireAccess: { url, owner in + BookmarkService.acquireAccess(to: url, owner: owner) + }, + // Conservative equality: a false negative only performs a + // balanced handoff, while a false positive could reuse access + // for a different provider resource. + sameResource: { $0.standardizedFileURL == $1.standardizedFileURL }, + validateDirectory: { VaultManager.liveValidationError(for: $0) }, + makeBookmarkData: { try BookmarkService.makeBookmarkData(for: $0) }, + persistBookmarkData: { + BookmarkService.persistBookmarkData( + $0, + identifier: VaultManager.obsidianBookmarkID + ) + }, + refreshBookmarkData: { data, sourceData in + BookmarkService.refreshBookmarkData( + data, + replacing: sourceData, + identifier: VaultManager.obsidianBookmarkID + ) + }, + resolveBookmark: { + BookmarkService.resolveBookmark(identifier: VaultManager.obsidianBookmarkID) + }, + hasBookmark: { + BookmarkService.hasBookmark(identifier: VaultManager.obsidianBookmarkID) + }, + scanVaults: { VaultManager.vaultSubfolderNames(in: $0) }, + cleanupLegacyBookmarks: { + let legacyIDs = BookmarkService.allBookmarkIdentifiers() + .filter { $0.hasPrefix("vault-") } + for id in legacyIDs { + BookmarkService.deleteBookmark(identifier: id) + logger.info("Cleaned up legacy bookmark") + } + } + ) + } + } + + private let environment: Environment + + init(environment: Environment = .live) { + self.environment = environment + } + // MARK: - Access Grant (one-time, from onboarding) /// Grant access to the Obsidian root directory via a user-picked URL. /// Returns nil on success, error message on failure. func grantAccess(url: URL) -> String? { - guard BookmarkService.startAccessing(url: url) else { - return L10n.tr("Could not access the selected folder.") + let priorLease = foregroundLease + let reusesPriorLease = priorLease.map { + $0.isActive && environment.sameResource($0.url, url) + } ?? false + + var candidateLease: SecurityScopedLease? + if !reusesPriorLease { + guard let acquired = environment.acquireAccess(url, .foreground) else { + return L10n.tr("Could not access the selected folder.") + } + candidateLease = acquired } + // Only a newly acquired, not-yet-adopted candidate is released on + // failure. Same-URL validation reuses the existing foreground lease. + defer { candidateLease?.release() } - if let validationError = validateSelectedDirectory(url: url) { - BookmarkService.stopAccessing(url: url) + if let validationError = environment.validateDirectory(url) { return validationError } + guard let names = environment.scanVaults(url) else { + return L10n.tr("VaultSync can no longer read your Obsidian directory. Reconnect the folder to restore sync access.") + } + + let advisory = preparedSelectionAdvisory(for: url, detectedVaults: names) + + let bookmarkData: Data do { - try BookmarkService.saveBookmark(for: url, identifier: Self.obsidianBookmarkID) + bookmarkData = try environment.makeBookmarkData(url) } catch { - BookmarkService.stopAccessing(url: url) return L10n.fmt("Failed to save access permission: %@", error.localizedDescription) } + environment.persistBookmarkData(bookmarkData) + // No fallible step may occur between the bookmark commit and this + // synchronous MainActor commit. The old lease stays active until all + // candidate proof has succeeded and the new state is adopted. + if let acquired = candidateLease { + foregroundLease = acquired + candidateLease = nil + } obsidianDirectoryURL = url isAccessible = true needsReconnect = false accessIssue = nil - scanForVaults() - cleanupLegacyBookmarks() + detectedVaults = names + selectionAdvisory = advisory.message - selectionAdvisory = nil - var pickedConfigIsDirectory: ObjCBool = false - let pickedFolderHasOwnConfig = FileManager.default.fileExists( - atPath: url.appendingPathComponent(".obsidian", isDirectory: true).path, - isDirectory: &pickedConfigIsDirectory - ) && pickedConfigIsDirectory.boolValue - // scanForVaults() ran above, so detectedVaults reflects this URL. - let pickedFolderIsVault = Self.rootIsItselfVault( - hasOwnConfig: pickedFolderHasOwnConfig, - hasVaultSubfolders: !detectedVaults.isEmpty - ) - switch Self.selectionAdvisoryKind( - isUbiquitous: Self.urlLooksUbiquitous(url), - pickedFolderIsVault: pickedFolderIsVault - ) { - case .iCloudRoot: - selectionAdvisory = L10n.tr("The folder you selected is stored in iCloud Drive. iCloud can keep files as placeholders that are not fully downloaded on this iPhone, which can stall syncing and create conflicts. For reliable syncing, use your vaults under \"On My iPhone\" → \"Obsidian\" and select that folder instead.") - case .rootIsVault: - selectionAdvisory = L10n.tr("The folder you selected is itself a vault. Syncing this one vault works, but additional vaults cannot get their own folder next to it. If you plan to sync more than one vault, select the folder that contains your vaults instead (\"On My iPhone\" → \"Obsidian\").") - case nil: - break + if !reusesPriorLease { + priorLease?.release() } + cleanupLegacyBookmarks() - logger.info("Obsidian directory access granted (selectedFolderIsVault=\(pickedFolderIsVault))") + logger.info("Obsidian directory access granted (selectedFolderIsVault=\(advisory.pickedFolderIsVault))") return nil } // MARK: - Restore on Launch - /// Restore access from saved bookmark. Safe to call multiple times. - /// The `isAccessible` guard prevents double-calling `startAccessingSecurityScopedResource()`, - /// which must be balanced 1:1 with `stopAccessingSecurityScopedResource()`. + /// Restore access from saved bookmark. Safe to call multiple times: the + /// owned token, not a UI Boolean, is the no-double-start guard. func restoreAccess() { - if isAccessible { return } + if foregroundLease?.isActive == true { return } + foregroundLease = nil - guard let (url, isStale) = BookmarkService.resolveBookmark(identifier: Self.obsidianBookmarkID) else { - if BookmarkService.hasBookmark(identifier: Self.obsidianBookmarkID) { + guard let resolvedBookmark = environment.resolveBookmark() else { + if environment.hasBookmark() { markReconnectRequired( reason: L10n.tr("VaultSync can no longer resolve the saved Obsidian folder permission. Reconnect the Obsidian directory to continue syncing.") ) @@ -110,36 +186,64 @@ final class VaultManager { } return } + let url = resolvedBookmark.url - guard BookmarkService.startAccessing(url: url) else { + guard let acquiredLease = environment.acquireAccess(url, .foreground) else { markReconnectRequired( reason: L10n.tr("VaultSync cannot access the saved Obsidian folder anymore. Reconnect the Obsidian directory to continue syncing.") ) logger.warning("Cannot access Obsidian directory") return } + var candidateLease: SecurityScopedLease? = acquiredLease + defer { candidateLease?.release() } - if let validationError = validateSelectedDirectory(url: url) { - BookmarkService.stopAccessing(url: url) + if let validationError = environment.validateDirectory(url) { markReconnectRequired(reason: validationError) logger.warning("Saved Obsidian directory failed validation") return } - if isStale { + guard let names = environment.scanVaults(url) else { + markReconnectRequired( + reason: L10n.tr("VaultSync can no longer read your Obsidian directory. Reconnect the folder to restore sync access.") + ) + logger.warning("Saved Obsidian directory scan failed") + return + } + + if resolvedBookmark.isStale { + let bookmarkData: Data do { - try BookmarkService.saveBookmark(for: url, identifier: Self.obsidianBookmarkID) - logger.info("Refreshed stale Obsidian bookmark") + bookmarkData = try environment.makeBookmarkData(url) } catch { logger.warning("Could not refresh stale Obsidian bookmark") + markReconnectRequired( + reason: L10n.fmt("Failed to save access permission: %@", error.localizedDescription) + ) + return + } + guard environment.refreshBookmarkData( + bookmarkData, + resolvedBookmark.sourceData + ) else { + markReconnectRequired( + reason: L10n.tr("VaultSync can no longer resolve the saved Obsidian folder permission. Reconnect the Obsidian directory to continue syncing.") + ) + logger.warning("Skipped stale Obsidian bookmark refresh after a concurrent permission change") + return } + logger.info("Refreshed stale Obsidian bookmark") } + foregroundLease = candidateLease + candidateLease = nil obsidianDirectoryURL = url isAccessible = true needsReconnect = false accessIssue = nil - scanForVaults() + detectedVaults = names + selectionAdvisory = nil logger.info("Obsidian directory access restored") } @@ -153,9 +257,10 @@ final class VaultManager { return } - guard let names = Self.vaultSubfolderNames(in: url) else { - BookmarkService.stopAccessing(url: url) - detectedVaults = [] + guard let names = environment.scanVaults(url) else { + // This wrapper is the foreground owner. The scan primitive itself + // is read-only and never releases a lease it did not acquire. + releaseForegroundLease() markReconnectRequired( reason: L10n.tr("VaultSync can no longer read your Obsidian directory. Reconnect the folder to restore sync access.") ) @@ -720,16 +825,47 @@ final class VaultManager { // MARK: - Legacy Migration + private func preparedSelectionAdvisory( + for url: URL, + detectedVaults: [String] + ) -> (message: String?, pickedFolderIsVault: Bool) { + var pickedConfigIsDirectory: ObjCBool = false + let pickedFolderHasOwnConfig = FileManager.default.fileExists( + atPath: url.appendingPathComponent(".obsidian", isDirectory: true).path, + isDirectory: &pickedConfigIsDirectory + ) && pickedConfigIsDirectory.boolValue + let pickedFolderIsVault = Self.rootIsItselfVault( + hasOwnConfig: pickedFolderHasOwnConfig, + hasVaultSubfolders: !detectedVaults.isEmpty + ) + + let message: String? + switch Self.selectionAdvisoryKind( + isUbiquitous: Self.urlLooksUbiquitous(url), + pickedFolderIsVault: pickedFolderIsVault + ) { + case .iCloudRoot: + message = L10n.tr("The folder you selected is stored in iCloud Drive. iCloud can keep files as placeholders that are not fully downloaded on this iPhone, which can stall syncing and create conflicts. For reliable syncing, use your vaults under \"On My iPhone\" → \"Obsidian\" and select that folder instead.") + case .rootIsVault: + message = L10n.tr("The folder you selected is itself a vault. Syncing this one vault works, but additional vaults cannot get their own folder next to it. If you plan to sync more than one vault, select the folder that contains your vaults instead (\"On My iPhone\" → \"Obsidian\").") + case nil: + message = nil + } + return (message, pickedFolderIsVault) + } + /// Remove old per-vault bookmarks after migration to obsidian-root. private func cleanupLegacyBookmarks() { - let legacyIDs = BookmarkService.allBookmarkIdentifiers().filter { $0.hasPrefix("vault-") } - for id in legacyIDs { - BookmarkService.deleteBookmark(identifier: id) - logger.info("Cleaned up legacy bookmark") - } + environment.cleanupLegacyBookmarks() + } + + private func releaseForegroundLease() { + let lease = foregroundLease + foregroundLease = nil + lease?.release() } - private func validateSelectedDirectory(url: URL) -> String? { + nonisolated private static func liveValidationError(for url: URL) -> String? { let fm = FileManager.default var isDirectory: ObjCBool = false guard fm.fileExists(atPath: url.path, isDirectory: &isDirectory), isDirectory.boolValue else { diff --git a/ios/VaultSyncTests/SecurityScopedLeaseOwnershipTests.swift b/ios/VaultSyncTests/SecurityScopedLeaseOwnershipTests.swift new file mode 100644 index 0000000..b6b3b75 --- /dev/null +++ b/ios/VaultSyncTests/SecurityScopedLeaseOwnershipTests.swift @@ -0,0 +1,325 @@ +import Foundation +import os +import Testing +@testable import VaultSync + +@Suite("Security-scoped lease ownership (#147)") +struct SecurityScopedLeaseOwnershipTests { + + private final class LeaseLedger: @unchecked Sendable { + private struct State { + var startAttempts = 0 + var starts = 0 + var stops = 0 + } + + private let state = OSAllocatedUnfairLock(initialState: State()) + + var startAttempts: Int { + state.withLock { $0.startAttempts } + } + + var starts: Int { + state.withLock { $0.starts } + } + + var stops: Int { + state.withLock { $0.stops } + } + + var active: Int { + state.withLock { $0.starts - $0.stops } + } + + func environment(startSucceeds: Bool = true) -> BookmarkService.AccessEnvironment { + BookmarkService.AccessEnvironment( + start: { [self] _ in + state.withLock { + $0.startAttempts += 1 + if startSucceeds { + $0.starts += 1 + } + } + return startSucceeds + }, + stop: { [self] _ in + state.withLock { $0.stops += 1 } + } + ) + } + } + + private let url = URL(fileURLWithPath: "/issue-147/Obsidian", isDirectory: true) + + @Test("A failed acquire never produces a stop") + func issue147FailedAcquireDoesNotStop() { + let ledger = LeaseLedger() + + let lease = BookmarkService.acquireAccess( + to: url, + owner: .foreground, + environment: ledger.environment(startSucceeds: false) + ) + + #expect(lease == nil) + #expect(ledger.startAttempts == 1) + #expect(ledger.starts == 0) + #expect(ledger.stops == 0) + #expect(ledger.active == 0) + } + + @Test("Explicit release and deinit remain idempotent") + func issue147ExplicitReleaseAndDeinitStopExactlyOnce() { + let ledger = LeaseLedger() + var lease = BookmarkService.acquireAccess( + to: url, + owner: .foreground, + environment: ledger.environment() + ) + + #expect(lease != nil) + lease?.release() + lease?.release() + #expect(lease?.isActive == false) + #expect(ledger.starts == 1) + #expect(ledger.stops == 1) + + lease = nil + #expect(ledger.stops == 1) + #expect(ledger.active == 0) + } + + @Test("Deinit balances an otherwise unreleased lease") + func issue147DeinitBackstopStopsExactlyOnce() { + let ledger = LeaseLedger() + var lease = BookmarkService.acquireAccess( + to: url, + owner: .foreground, + environment: ledger.environment() + ) + weak let weakLease = lease + + #expect(lease != nil) + #expect(ledger.starts == 1) + #expect(ledger.stops == 0) + + lease = nil + + #expect(weakLease == nil) + #expect(ledger.stops == 1) + #expect(ledger.active == 0) + } + + @Test("Concurrent terminal cleanup stops one lease exactly once") + func issue147ConcurrentReleaseStopsExactlyOnce() async throws { + let ledger = LeaseLedger() + let lease = try #require(BookmarkService.acquireAccess( + to: url, + owner: .background(UUID()), + environment: ledger.environment() + )) + + await withTaskGroup(of: Void.self) { group in + for _ in 0..<32 { + group.addTask { + lease.release() + } + } + } + + #expect(!lease.isActive) + #expect(ledger.starts == 1) + #expect(ledger.stops == 1) + #expect(ledger.active == 0) + } + + @Test("Background cleanup releases only its token for a shared URL") + func issue147BackgroundReleasePreservesForegroundToken() throws { + let ledger = LeaseLedger() + let environment = ledger.environment() + let runID = UUID() + let foreground = try #require(BookmarkService.acquireAccess( + to: url, + owner: .foreground, + environment: environment + )) + let background = try #require(BookmarkService.acquireAccess( + to: url, + owner: .background(runID), + environment: environment + )) + + #expect(foreground.id != background.id) + #expect(foreground.owner == .foreground) + #expect(background.owner == .background(runID)) + #expect(ledger.starts == 2) + #expect(ledger.active == 2) + + background.release() + + #expect(foreground.isActive) + #expect(!background.isActive) + #expect(ledger.stops == 1) + #expect(ledger.active == 1) + + foreground.release() + + #expect(ledger.stops == 2) + #expect(ledger.active == 0) + } + + @Test("Forced restart reuses one run-owned background lease") + func issue147ForcedRestartEnsureReusesAndReleasesLease() { + let ledger = LeaseLedger() + let runID = UUID() + let managedAccess = BackgroundSecurityScopedAccess() + let testURL = url + + let acquire: @Sendable () -> SecurityScopedLease? = { + BookmarkService.acquireAccess( + to: testURL, + owner: .background(runID), + environment: ledger.environment() + ) + } + + #expect(managedAccess.ensureAccess(using: acquire)) + #expect(managedAccess.ensureAccess(using: acquire)) + #expect(managedAccess.hasLease) + #expect(managedAccess.url == url) + #expect(ledger.starts == 1) + #expect(ledger.stops == 0) + + managedAccess.release() + managedAccess.release() + + #expect(!managedAccess.hasLease) + #expect(managedAccess.url == nil) + #expect(ledger.stops == 1) + #expect(ledger.active == 0) + } + + @Test("Failed background ensure stays empty and never stops") + func issue147FailedBackgroundEnsureDoesNotOwnOrStopLease() { + let ledger = LeaseLedger() + let managedAccess = BackgroundSecurityScopedAccess() + let testURL = url + + let acquired = managedAccess.ensureAccess { + BookmarkService.acquireAccess( + to: testURL, + owner: .background(UUID()), + environment: ledger.environment(startSucceeds: false) + ) + } + + #expect(!acquired) + #expect(!managedAccess.hasLease) + #expect(managedAccess.url == nil) + + managedAccess.release() + + #expect(ledger.startAttempts == 1) + #expect(ledger.starts == 0) + #expect(ledger.stops == 0) + } + + @Test("Task cancellation balances the production background run lease scope") + func issue147CancellationBalancesProductionBackgroundRunLeaseExactlyOnce() async { + let ledger = LeaseLedger() + let (events, eventContinuation) = AsyncStream.makeStream() + let observedAccess = OSAllocatedUnfairLock( + initialState: nil + ) + let testURL = url + + let task = Task { + await BackgroundSecurityScopedAccess.withRunOwnedLease { managedAccess, owner in + observedAccess.withLock { $0 = managedAccess } + guard managedAccess.ensureAccess(using: { + BookmarkService.acquireAccess( + to: testURL, + owner: owner, + environment: ledger.environment() + ) + }) else { + Issue.record("The deterministic background acquire unexpectedly failed") + eventContinuation.finish() + return + } + + eventContinuation.yield(()) + eventContinuation.finish() + do { + try await Task.sleep(for: .seconds(30)) + } catch { + // Cancellation returns through the exact defer scope used + // by BackgroundSyncService.performBackgroundSync. + } + } + } + + var observedAcquire = false + for await _ in events { + observedAcquire = true + } + #expect(observedAcquire) + #expect(observedAccess.withLock { $0?.hasLease } == true) + #expect(ledger.starts == 1) + #expect(ledger.stops == 0) + + task.cancel() + await task.value + + #expect(observedAccess.withLock { $0?.hasLease } == false) + #expect(ledger.stops == 1) + #expect(ledger.active == 0) + + observedAccess.withLock { $0 }?.release() + #expect(ledger.stops == 1) + } + + @Test("Expiration before task creation gates work as cancelled") + func issue147ExpirationBeforeTaskCreationStartsCancelled() async { + let relay = BackgroundTaskCancellationRelay() + let cancellations = OSAllocatedUnfairLock(initialState: 0) + + relay.expire() + let task = relay.makeTask { + if Task.isCancelled { + cancellations.withLock { $0 += 1 } + } + } + await task.value + relay.expire() + relay.finish() + + #expect(cancellations.withLock { $0 } == 1) + } + + @Test("Expiration after task creation cancels running work exactly once") + func issue147ExpirationAfterTaskCreationCancelsExactlyOnce() async { + let relay = BackgroundTaskCancellationRelay() + let cancellations = OSAllocatedUnfairLock(initialState: 0) + let (events, eventContinuation) = AsyncStream.makeStream() + + let task = relay.makeTask { + eventContinuation.yield(()) + eventContinuation.finish() + do { + try await Task.sleep(for: .seconds(30)) + } catch { + cancellations.withLock { $0 += 1 } + } + } + for await _ in events { + break + } + relay.expire() + relay.expire() + await task.value + relay.finish() + + #expect(cancellations.withLock { $0 } == 1) + } +} diff --git a/ios/VaultSyncTests/SecurityScopedLeaseTakeoverTests.swift b/ios/VaultSyncTests/SecurityScopedLeaseTakeoverTests.swift new file mode 100644 index 0000000..717372a --- /dev/null +++ b/ios/VaultSyncTests/SecurityScopedLeaseTakeoverTests.swift @@ -0,0 +1,797 @@ +import Foundation +import Testing +import os +@testable import VaultSync + +@MainActor +@Suite("Security-scoped lease takeover (#147)") +struct SecurityScopedLeaseTakeoverTests { + + private enum Event: Equatable, Sendable { + case start(URL) + case startFailed(URL) + case stop(URL) + case validate(URL) + case scan(URL) + case makeBookmark(URL) + case persistBookmark(URL) + case refreshBookmark(URL) + case resolveBookmark + case hasBookmark + case cleanupLegacyBookmarks + } + + private final class EventLog: @unchecked Sendable { + private let state = OSAllocatedUnfairLock(initialState: [Event]()) + + var values: [Event] { + state.withLock { $0 } + } + + func append(_ event: Event) { + state.withLock { $0.append(event) } + } + + func reset() { + state.withLock { $0.removeAll() } + } + } + + private final class LeaseLedger: @unchecked Sendable { + private struct State: Sendable { + var attempts: [URL: Int] = [:] + var starts: [URL: Int] = [:] + var stops: [URL: Int] = [:] + var startFailures: Set = [] + } + + private let state = OSAllocatedUnfairLock(initialState: State()) + private let events: EventLog + private let onStop: @Sendable (URL) -> Void + + init( + events: EventLog, + onStop: @escaping @Sendable (URL) -> Void = { _ in } + ) { + self.events = events + self.onStop = onStop + } + + var attempts: [URL: Int] { + state.withLock { $0.attempts } + } + + var starts: [URL: Int] { + state.withLock { $0.starts } + } + + var stops: [URL: Int] { + state.withLock { $0.stops } + } + + func failStart(for url: URL) { + _ = state.withLock { $0.startFailures.insert(url) } + } + + func start(_ url: URL) -> Bool { + let succeeded = state.withLock { state in + state.attempts[url, default: 0] += 1 + guard !state.startFailures.contains(url) else { return false } + state.starts[url, default: 0] += 1 + return true + } + events.append(succeeded ? .start(url) : .startFailed(url)) + return succeeded + } + + func stop(_ url: URL) { + state.withLock { $0.stops[url, default: 0] += 1 } + events.append(.stop(url)) + onStop(url) + } + + func activeCount(for url: URL) -> Int { + state.withLock { + $0.starts[url, default: 0] - $0.stops[url, default: 0] + } + } + } + + private enum ScanResult { + case vaults([String]) + case failure + } + + private struct InjectedBookmarkError: LocalizedError { + var errorDescription: String? { "Injected bookmark failure" } + } + + @MainActor + private final class Harness { + let events: EventLog + let leases: LeaseLedger + + var validationErrors: [URL: String] = [:] + var scanResults: [URL: ScanResult] = [:] + var bookmarkFailures: Set = [] + var resolvedBookmark: BookmarkService.ResolvedBookmark? + var bookmarkRefreshSucceeds = true + var hasBookmarkValue = false + var storedBookmarkURL: URL? + var legacyCleanupCount = 0 + + private var nextBookmarkPayload = 0 + private var preparedBookmarks: [Data: URL] = [:] + + init(onStop: @escaping @Sendable (URL) -> Void = { _ in }) { + let events = EventLog() + self.events = events + self.leases = LeaseLedger(events: events, onStop: onStop) + } + + func environment() -> VaultManager.Environment { + let leases = leases + let accessEnvironment = BookmarkService.AccessEnvironment( + start: { leases.start($0) }, + stop: { leases.stop($0) } + ) + + return VaultManager.Environment( + acquireAccess: { url, owner in + BookmarkService.acquireAccess( + to: url, + owner: owner, + environment: accessEnvironment + ) + }, + sameResource: { $0.standardizedFileURL == $1.standardizedFileURL }, + validateDirectory: { [self] url in + events.append(.validate(url)) + return validationErrors[url] + }, + makeBookmarkData: { [self] url in + events.append(.makeBookmark(url)) + if bookmarkFailures.contains(url) { + throw InjectedBookmarkError() + } + nextBookmarkPayload += 1 + let data = Data("issue-147-bookmark-\(nextBookmarkPayload)".utf8) + preparedBookmarks[data] = url + return data + }, + persistBookmarkData: { [self] data in + guard let url = preparedBookmarks[data] else { + preconditionFailure("Persisted an unknown prepared bookmark") + } + storedBookmarkURL = url + events.append(.persistBookmark(url)) + }, + refreshBookmarkData: { [self] data, _ in + guard let url = preparedBookmarks[data] else { + preconditionFailure("Refreshed with an unknown prepared bookmark") + } + events.append(.refreshBookmark(url)) + if bookmarkRefreshSucceeds { + storedBookmarkURL = url + } + return bookmarkRefreshSucceeds + }, + resolveBookmark: { [self] in + events.append(.resolveBookmark) + return resolvedBookmark + }, + hasBookmark: { [self] in + events.append(.hasBookmark) + return hasBookmarkValue + }, + scanVaults: { [self] url in + events.append(.scan(url)) + switch scanResults[url] ?? .vaults([]) { + case .vaults(let names): + return names + case .failure: + return nil + } + }, + cleanupLegacyBookmarks: { [self] in + legacyCleanupCount += 1 + events.append(.cleanupLegacyBookmarks) + } + ) + } + } + + private struct ManagerSnapshot: Equatable, Sendable { + let url: URL? + let isAccessible: Bool + let detectedVaults: [String] + let needsReconnect: Bool + let hasAccessIssue: Bool + let selectionAdvisory: String? + } + + /// The lease callback is `@Sendable`, while the manager is MainActor-bound. + /// Production releases the replaced foreground token synchronously inside + /// `grantAccess`, so this probe safely verifies the state at that exact stop. + private final class CommitBeforeStopProbe: @unchecked Sendable { + private let replacedURL: URL + private let snapshots = OSAllocatedUnfairLock(initialState: [ManagerSnapshot]()) + weak var manager: VaultManager? + + init(replacedURL: URL) { + self.replacedURL = replacedURL + } + + func recordIfReplacedLease(_ stoppedURL: URL) { + guard stoppedURL == replacedURL else { return } + let snapshot = MainActor.assumeIsolated { [weak self] () -> ManagerSnapshot? in + guard let manager = self?.manager else { return nil } + return ManagerSnapshot( + url: manager.obsidianDirectoryURL, + isAccessible: manager.isAccessible, + detectedVaults: manager.detectedVaults, + needsReconnect: manager.needsReconnect, + hasAccessIssue: manager.accessIssue != nil, + selectionAdvisory: manager.selectionAdvisory + ) + } + if let snapshot { + snapshots.withLock { $0.append(snapshot) } + } + } + + var values: [ManagerSnapshot] { + snapshots.withLock { $0 } + } + } + + private static func url(_ name: String) -> URL { + URL( + fileURLWithPath: "/VaultSyncTests/Issue147/\(name)/Obsidian", + isDirectory: true + ) + } + + private static func resolvedBookmark( + _ url: URL, + isStale: Bool, + source: String + ) -> BookmarkService.ResolvedBookmark { + BookmarkService.ResolvedBookmark( + url: url, + isStale: isStale, + sourceData: Data(source.utf8) + ) + } + + private func adoptPrior( + _ url: URL, + vaults: [String] = ["PriorVault"], + harness: Harness + ) -> VaultManager { + harness.scanResults[url] = .vaults(vaults) + let manager = VaultManager(environment: harness.environment()) + #expect(manager.grantAccess(url: url) == nil) + expectAdopted(manager, url: url, vaults: vaults) + #expect(harness.leases.activeCount(for: url) == 1) + #expect(harness.storedBookmarkURL == url) + return manager + } + + private func expectAdopted( + _ manager: VaultManager, + url: URL, + vaults: [String] + ) { + #expect(manager.obsidianDirectoryURL == url) + #expect(manager.isAccessible) + #expect(manager.detectedVaults == vaults) + #expect(!manager.needsReconnect) + #expect(manager.accessIssue == nil) + } + + private func expectReconnectRequired(_ manager: VaultManager) { + #expect(manager.obsidianDirectoryURL == nil) + #expect(!manager.isAccessible) + #expect(manager.detectedVaults.isEmpty) + #expect(manager.needsReconnect) + #expect(manager.accessIssue != nil) + } + + @Test("Repeated selection of the same URL reuses one active lease (#147)") + func issue147SameURLDoesNotAcquireAnotherLease() { + let url = Self.url("same-url") + let harness = Harness() + let manager = adoptPrior(url, harness: harness) + let cleanupBefore = harness.legacyCleanupCount + + harness.scanResults[url] = .vaults(["RefreshedVault"]) + harness.events.reset() + + #expect(manager.grantAccess(url: url) == nil) + + #expect(harness.leases.attempts[url] == 1) + #expect(harness.leases.starts[url] == 1) + #expect(harness.leases.stops[url, default: 0] == 0) + #expect(harness.leases.activeCount(for: url) == 1) + #expect(harness.events.values == [ + .validate(url), + .scan(url), + .makeBookmark(url), + .persistBookmark(url), + .cleanupLegacyBookmarks, + ]) + #expect(harness.legacyCleanupCount == cleanupBefore + 1) + expectAdopted(manager, url: url, vaults: ["RefreshedVault"]) + } + + @Test("A successful URL change commits B before stopping A (#147)") + func issue147SuccessfulChangeCommitsBeforeStoppingPriorLease() { + let priorURL = Self.url("successful-change-a") + let candidateURL = Self.url("successful-change-b") + let probe = CommitBeforeStopProbe(replacedURL: priorURL) + let harness = Harness(onStop: { probe.recordIfReplacedLease($0) }) + let manager = adoptPrior(priorURL, harness: harness) + probe.manager = manager + harness.scanResults[candidateURL] = .vaults(["CandidateVault"]) + harness.events.reset() + + #expect(manager.grantAccess(url: candidateURL) == nil) + + #expect(harness.events.values == [ + .start(candidateURL), + .validate(candidateURL), + .scan(candidateURL), + .makeBookmark(candidateURL), + .persistBookmark(candidateURL), + .stop(priorURL), + .cleanupLegacyBookmarks, + ]) + #expect(probe.values == [ManagerSnapshot( + url: candidateURL, + isAccessible: true, + detectedVaults: ["CandidateVault"], + needsReconnect: false, + hasAccessIssue: false, + selectionAdvisory: nil + )]) + #expect(harness.storedBookmarkURL == candidateURL) + #expect(harness.leases.activeCount(for: priorURL) == 0) + #expect(harness.leases.activeCount(for: candidateURL) == 1) + expectAdopted(manager, url: candidateURL, vaults: ["CandidateVault"]) + } + + @Test("A failed start creates no lease and preserves A (#147)") + func issue147StartFailurePreservesPriorStateWithoutStop() { + let priorURL = Self.url("start-failure-a") + let candidateURL = Self.url("start-failure-b") + let harness = Harness() + let manager = adoptPrior(priorURL, harness: harness) + let cleanupBefore = harness.legacyCleanupCount + harness.leases.failStart(for: candidateURL) + harness.events.reset() + + let error = manager.grantAccess(url: candidateURL) + + #expect(error == L10n.tr("Could not access the selected folder.")) + #expect(harness.events.values == [.startFailed(candidateURL)]) + #expect(harness.leases.attempts[candidateURL] == 1) + #expect(harness.leases.starts[candidateURL, default: 0] == 0) + #expect(harness.leases.stops[candidateURL, default: 0] == 0) + #expect(harness.leases.activeCount(for: priorURL) == 1) + #expect(harness.storedBookmarkURL == priorURL) + #expect(harness.legacyCleanupCount == cleanupBefore) + expectAdopted(manager, url: priorURL, vaults: ["PriorVault"]) + } + + @Test("Validation failure releases only B and preserves A (#147)") + func issue147ValidationFailureRollsBackCandidate() { + let priorURL = Self.url("validation-failure-a") + let candidateURL = Self.url("validation-failure-b") + let validationError = "Injected validation failure" + let harness = Harness() + let manager = adoptPrior(priorURL, harness: harness) + let cleanupBefore = harness.legacyCleanupCount + harness.validationErrors[candidateURL] = validationError + harness.events.reset() + + #expect(manager.grantAccess(url: candidateURL) == validationError) + + #expect(harness.events.values == [ + .start(candidateURL), + .validate(candidateURL), + .stop(candidateURL), + ]) + #expect(harness.leases.activeCount(for: priorURL) == 1) + #expect(harness.leases.activeCount(for: candidateURL) == 0) + #expect(harness.leases.stops[candidateURL] == 1) + #expect(harness.storedBookmarkURL == priorURL) + #expect(harness.legacyCleanupCount == cleanupBefore) + expectAdopted(manager, url: priorURL, vaults: ["PriorVault"]) + } + + @Test("Bookmark creation failure releases only B and preserves A (#147)") + func issue147BookmarkFailureRollsBackCandidate() { + let priorURL = Self.url("bookmark-failure-a") + let candidateURL = Self.url("bookmark-failure-b") + let harness = Harness() + let manager = adoptPrior(priorURL, harness: harness) + let cleanupBefore = harness.legacyCleanupCount + harness.scanResults[candidateURL] = .vaults(["CandidateVault"]) + harness.bookmarkFailures.insert(candidateURL) + harness.events.reset() + + let error = manager.grantAccess(url: candidateURL) + + #expect(error?.contains("Injected bookmark failure") == true) + #expect(harness.events.values == [ + .start(candidateURL), + .validate(candidateURL), + .scan(candidateURL), + .makeBookmark(candidateURL), + .stop(candidateURL), + ]) + #expect(harness.leases.activeCount(for: priorURL) == 1) + #expect(harness.leases.activeCount(for: candidateURL) == 0) + #expect(harness.leases.stops[candidateURL] == 1) + #expect(harness.storedBookmarkURL == priorURL) + #expect(harness.legacyCleanupCount == cleanupBefore) + expectAdopted(manager, url: priorURL, vaults: ["PriorVault"]) + } + + @Test("Scan failure preserves the prior adopted lease and reports failure (#147)") + func issue147ScanFailurePreservesPriorLeaseAndReturnsFailure() { + let priorURL = Self.url("scan-failure-a") + let candidateURL = Self.url("scan-failure-b") + let harness = Harness() + let manager = adoptPrior(priorURL, harness: harness) + let cleanupBefore = harness.legacyCleanupCount + harness.scanResults[candidateURL] = .failure + harness.events.reset() + + let error = manager.grantAccess(url: candidateURL) + + #expect(error == L10n.tr("VaultSync can no longer read your Obsidian directory. Reconnect the folder to restore sync access.")) + #expect(harness.events.values == [ + .start(candidateURL), + .validate(candidateURL), + .scan(candidateURL), + .stop(candidateURL), + ]) + #expect(harness.leases.starts[priorURL] == 1) + #expect(harness.leases.stops[priorURL, default: 0] == 0) + #expect(harness.leases.starts[candidateURL] == 1) + #expect(harness.leases.stops[candidateURL] == 1) + #expect(harness.leases.activeCount(for: priorURL) == 1) + #expect(harness.leases.activeCount(for: candidateURL) == 0) + #expect(harness.storedBookmarkURL == priorURL) + #expect(harness.legacyCleanupCount == cleanupBefore) + expectAdopted(manager, url: priorURL, vaults: ["PriorVault"]) + } + + @Test("Restore while a foreground lease is active is a no-op (#147)") + func issue147RestoreWhileActiveDoesNotAcquireOrResolveAgain() { + let activeURL = Self.url("restore-no-op-a") + let ignoredURL = Self.url("restore-no-op-b") + let harness = Harness() + let manager = adoptPrior(activeURL, harness: harness) + harness.resolvedBookmark = Self.resolvedBookmark( + ignoredURL, + isStale: false, + source: "ignored-active-restore" + ) + harness.events.reset() + + manager.restoreAccess() + + #expect(harness.events.values.isEmpty) + #expect(harness.leases.attempts[activeURL] == 1) + #expect(harness.leases.attempts[ignoredURL, default: 0] == 0) + #expect(harness.leases.activeCount(for: activeURL) == 1) + expectAdopted(manager, url: activeURL, vaults: ["PriorVault"]) + } + + @Test("Fresh restore adopts one lease after validation and scan (#147)") + func issue147FreshRestoreAdoptsOneLease() { + let url = Self.url("fresh-restore") + let harness = Harness() + harness.resolvedBookmark = Self.resolvedBookmark( + url, + isStale: false, + source: "fresh-restore" + ) + harness.hasBookmarkValue = true + harness.storedBookmarkURL = url + harness.scanResults[url] = .vaults(["RestoredVault"]) + let manager = VaultManager(environment: harness.environment()) + + manager.restoreAccess() + + #expect(harness.events.values == [ + .resolveBookmark, + .start(url), + .validate(url), + .scan(url), + ]) + #expect(harness.leases.starts[url] == 1) + #expect(harness.leases.stops[url, default: 0] == 0) + #expect(harness.leases.activeCount(for: url) == 1) + #expect(harness.storedBookmarkURL == url) + expectAdopted(manager, url: url, vaults: ["RestoredVault"]) + } + + @Test("Restore acquire failure creates no lease or stop and requires reconnect (#147)") + func issue147RestoreAcquireFailureDoesNotStop() { + let url = Self.url("restore-acquire-failure") + let harness = Harness() + harness.resolvedBookmark = Self.resolvedBookmark( + url, + isStale: false, + source: "restore-acquire-failure" + ) + harness.hasBookmarkValue = true + harness.storedBookmarkURL = url + harness.leases.failStart(for: url) + let manager = VaultManager(environment: harness.environment()) + + manager.restoreAccess() + + #expect(harness.events.values == [ + .resolveBookmark, + .startFailed(url), + ]) + #expect(harness.leases.attempts[url] == 1) + #expect(harness.leases.starts[url, default: 0] == 0) + #expect(harness.leases.stops[url, default: 0] == 0) + #expect(harness.leases.activeCount(for: url) == 0) + #expect(harness.storedBookmarkURL == url) + expectReconnectRequired(manager) + } + + @Test("Restore validation failure releases its candidate and requires reconnect (#147)") + func issue147RestoreValidationFailureReleasesCandidate() { + let url = Self.url("restore-validation-failure") + let validationError = "Injected restore validation failure" + let harness = Harness() + harness.resolvedBookmark = Self.resolvedBookmark( + url, + isStale: false, + source: "restore-validation-failure" + ) + harness.hasBookmarkValue = true + harness.storedBookmarkURL = url + harness.validationErrors[url] = validationError + let manager = VaultManager(environment: harness.environment()) + + manager.restoreAccess() + + #expect(harness.events.values == [ + .resolveBookmark, + .start(url), + .validate(url), + .stop(url), + ]) + #expect(harness.leases.starts[url] == 1) + #expect(harness.leases.stops[url] == 1) + #expect(harness.leases.activeCount(for: url) == 0) + #expect(harness.storedBookmarkURL == url) + expectReconnectRequired(manager) + } + + @Test("Restore scan failure releases its candidate and requires reconnect (#147)") + func issue147RestoreScanFailureReleasesCandidate() { + let url = Self.url("restore-scan-failure") + let harness = Harness() + harness.resolvedBookmark = Self.resolvedBookmark( + url, + isStale: false, + source: "restore-scan-failure" + ) + harness.hasBookmarkValue = true + harness.storedBookmarkURL = url + harness.scanResults[url] = .failure + let manager = VaultManager(environment: harness.environment()) + + manager.restoreAccess() + + #expect(harness.events.values == [ + .resolveBookmark, + .start(url), + .validate(url), + .scan(url), + .stop(url), + ]) + #expect(harness.leases.starts[url] == 1) + #expect(harness.leases.stops[url] == 1) + #expect(harness.leases.activeCount(for: url) == 0) + #expect(harness.storedBookmarkURL == url) + expectReconnectRequired(manager) + } + + @Test("Stale restore bookmark failure releases its candidate without replacing the bookmark (#147)") + func issue147StaleRestoreBookmarkFailureReleasesCandidate() { + let url = Self.url("restore-stale-bookmark-failure") + let harness = Harness() + harness.resolvedBookmark = Self.resolvedBookmark( + url, + isStale: true, + source: "restore-stale-bookmark-failure" + ) + harness.hasBookmarkValue = true + harness.storedBookmarkURL = url + harness.scanResults[url] = .vaults(["RestoredVault"]) + harness.bookmarkFailures.insert(url) + let manager = VaultManager(environment: harness.environment()) + + manager.restoreAccess() + + #expect(harness.events.values == [ + .resolveBookmark, + .start(url), + .validate(url), + .scan(url), + .makeBookmark(url), + .stop(url), + ]) + #expect(harness.leases.starts[url] == 1) + #expect(harness.leases.stops[url] == 1) + #expect(harness.leases.activeCount(for: url) == 0) + #expect(harness.storedBookmarkURL == url) + expectReconnectRequired(manager) + } + + @Test("Successful stale restore refreshes by CAS and adopts exactly one lease (#147)") + func issue147SuccessfulStaleRestoreRefreshesAndAdoptsOneLease() { + let url = Self.url("restore-stale-success") + let harness = Harness() + harness.resolvedBookmark = Self.resolvedBookmark( + url, + isStale: true, + source: "restore-stale-success-source" + ) + harness.hasBookmarkValue = true + harness.storedBookmarkURL = url + harness.scanResults[url] = .vaults(["RestoredVault"]) + let manager = VaultManager(environment: harness.environment()) + + manager.restoreAccess() + + #expect(harness.events.values == [ + .resolveBookmark, + .start(url), + .validate(url), + .scan(url), + .makeBookmark(url), + .refreshBookmark(url), + ]) + #expect(harness.leases.attempts[url] == 1) + #expect(harness.leases.starts[url] == 1) + #expect(harness.leases.stops[url, default: 0] == 0) + #expect(harness.leases.activeCount(for: url) == 1) + #expect(harness.storedBookmarkURL == url) + expectAdopted(manager, url: url, vaults: ["RestoredVault"]) + } + + @Test("A stale restore CAS conflict cannot overwrite newer permission state (#147)") + func issue147StaleRestoreCASConflictReleasesCandidateWithoutAdoption() { + let staleURL = Self.url("restore-stale-cas-source") + let newerURL = Self.url("restore-stale-cas-newer") + let harness = Harness() + harness.resolvedBookmark = Self.resolvedBookmark( + staleURL, + isStale: true, + source: "stale-source-before-newer-grant" + ) + harness.hasBookmarkValue = true + // Models a foreground grant that replaced the stored permission after + // the stale bookmark snapshot above was resolved. + harness.storedBookmarkURL = newerURL + harness.scanResults[staleURL] = .vaults(["StaleVault"]) + harness.bookmarkRefreshSucceeds = false + let manager = VaultManager(environment: harness.environment()) + + manager.restoreAccess() + + #expect(harness.events.values == [ + .resolveBookmark, + .start(staleURL), + .validate(staleURL), + .scan(staleURL), + .makeBookmark(staleURL), + .refreshBookmark(staleURL), + .stop(staleURL), + ]) + #expect(harness.leases.starts[staleURL] == 1) + #expect(harness.leases.stops[staleURL] == 1) + #expect(harness.leases.activeCount(for: staleURL) == 0) + #expect(harness.storedBookmarkURL == newerURL) + #expect(harness.legacyCleanupCount == 0) + expectReconnectRequired(manager) + } + + @Test("Bookmark refresh compare-and-swap preserves a newer committed bookmark (#147)") + func issue147BookmarkRefreshCASRejectsStaleSourceBytes() { + let identifier = "issue-147-cas-\(UUID().uuidString)" + let sourceA = Data("bookmark-a".utf8) + let staleRefreshA = Data("bookmark-a-refreshed".utf8) + let newerB = Data("bookmark-b".utf8) + let refreshedB = Data("bookmark-b-refreshed".utf8) + let finalB = Data("bookmark-b-final".utf8) + defer { BookmarkService.deleteBookmark(identifier: identifier) } + + BookmarkService.persistBookmarkData(sourceA, identifier: identifier) + BookmarkService.persistBookmarkData(newerB, identifier: identifier) + + #expect(!BookmarkService.refreshBookmarkData( + staleRefreshA, + replacing: sourceA, + identifier: identifier + )) + // Success against B proves the rejected A refresh left B untouched. + #expect(BookmarkService.refreshBookmarkData( + refreshedB, + replacing: newerB, + identifier: identifier + )) + #expect(BookmarkService.refreshBookmarkData( + finalB, + replacing: refreshedB, + identifier: identifier + )) + } + + @Test("An active rescan failure releases the owned foreground lease exactly once (#147)") + func issue147ActiveRescanFailureReleasesForegroundLeaseOnce() { + let url = Self.url("active-rescan-failure") + let harness = Harness() + let manager = adoptPrior(url, harness: harness) + let cleanupBefore = harness.legacyCleanupCount + harness.scanResults[url] = .failure + harness.events.reset() + + manager.scanForVaults() + + #expect(harness.events.values == [.scan(url), .stop(url)]) + #expect(harness.leases.starts[url] == 1) + #expect(harness.leases.stops[url] == 1) + #expect(harness.leases.activeCount(for: url) == 0) + #expect(harness.storedBookmarkURL == url) + #expect(harness.legacyCleanupCount == cleanupBefore) + expectReconnectRequired(manager) + + harness.events.reset() + manager.scanForVaults() + #expect(harness.events.values.isEmpty) + #expect(harness.leases.stops[url] == 1) + } + + @Test("A real failed grant suppresses reconnect success, reconcile, and retry (#147)") + func issue147FailedGrantShortCircuitsObsidianReconnectFlow() async { + let priorURL = Self.url("flow-failure-a") + let candidateURL = Self.url("flow-failure-b") + let harness = Harness() + let manager = adoptPrior(priorURL, harness: harness) + let cleanupBefore = harness.legacyCleanupCount + harness.scanResults[candidateURL] = .failure + var flowEvents: [String] = [] + + let error = await ObsidianReconnectFlow.run( + grantAccess: { + flowEvents.append("grant") + return manager.grantAccess(url: candidateURL) + }, + onGrantSucceeded: { flowEvents.append("success") }, + reconcile: { flowEvents.append("reconcile") }, + retryPendingShares: { flowEvents.append("retry") } + ) + + #expect(error != nil) + #expect(flowEvents == ["grant"]) + #expect(harness.leases.activeCount(for: priorURL) == 1) + #expect(harness.leases.activeCount(for: candidateURL) == 0) + #expect(harness.leases.stops[candidateURL] == 1) + #expect(harness.storedBookmarkURL == priorURL) + #expect(harness.legacyCleanupCount == cleanupBefore) + expectAdopted(manager, url: priorURL, vaults: ["PriorVault"]) + } +}