fix(files): balance security-scoped resource leases

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.

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`.

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
+        }
     }
 }

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<SecurityScopedLease?>(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<Result: Sendable>(
+        _ 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()
+    }
+}