From a44952455c311204e96ee3f68e45fd2129f3ca4e Mon Sep 17 00:00:00 2001 From: Dimitri Bouniol Date: Thu, 25 Dec 2025 06:14:13 -0800 Subject: [PATCH 1/4] Fixed an issue where accessing multiple persistences within a transaction could lead to data loss --- .../Disk Persistence/DiskPersistence.swift | 25 ++++++++++++---- .../Disk Persistence/Snapshot/Snapshot.swift | 26 +++++++++++++---- .../Transaction/Transaction.swift | 29 +++++++++++++++---- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift b/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift index 1a99c3c..46533b8 100644 --- a/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift +++ b/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift @@ -179,7 +179,7 @@ extension DiskPersistence { func updateStoreInfo( @_inheritActorContext updater: @Sendable @escaping (_ storeInfo: inout StoreInfo) async throws -> T ) -> Task where AccessMode == ReadWrite { - if let storeInfo = DiskPersistenceTaskLocals.storeInfo { + if let storeInfo = DiskPersistenceTaskLocals.storeInfo(for: self) { return Task { var updatedStoreInfo = storeInfo let returnValue = try await updater(&updatedStoreInfo) @@ -202,7 +202,7 @@ extension DiskPersistence { var storeInfo = try cachedStoreInfo ?? self.loadStoreInfo() /// Let the updater do something with the store info, storing the variable on the Task Local stack. - let returnValue = try await DiskPersistenceTaskLocals.$storeInfo.withValue(storeInfo) { + let returnValue = try await DiskPersistenceTaskLocals.with(storeInfo: storeInfo, for: self) { try await updater(&storeInfo) } @@ -227,7 +227,7 @@ extension DiskPersistence { func updateStoreInfo( @_inheritActorContext accessor: @Sendable @escaping (_ storeInfo: StoreInfo) async throws -> T ) -> Task { - if let storeInfo = DiskPersistenceTaskLocals.storeInfo { + if let storeInfo = DiskPersistenceTaskLocals.storeInfo(for: self) { return Task { try await accessor(storeInfo) } } @@ -241,7 +241,7 @@ extension DiskPersistence { let storeInfo = try cachedStoreInfo ?? self.loadStoreInfo() /// Let the accessor do something with the store info, storing the variable on the Task Local stack. - return try await DiskPersistenceTaskLocals.$storeInfo.withValue(storeInfo) { + return try await DiskPersistenceTaskLocals.with(storeInfo: storeInfo, for: self) { try await accessor(storeInfo) } } @@ -713,6 +713,21 @@ enum ModificationUpdate { private enum DiskPersistenceTaskLocals { @TaskLocal - static var storeInfo: StoreInfo? + static var storeInfoStorage: [ObjectIdentifier : StoreInfo] = [:] + + static func storeInfo(for persistence: DiskPersistence) -> StoreInfo? { + storeInfoStorage[ObjectIdentifier(persistence)] + } + + static func with( + storeInfo: StoreInfo, + for persistence: DiskPersistence, + operation: () async throws -> R + ) async rethrows -> R { + var currentStorage = storeInfoStorage + currentStorage[ObjectIdentifier(persistence)] = storeInfo + + return try await $storeInfoStorage.withValue(currentStorage, operation: operation) + } } diff --git a/Sources/CodableDatastore/Persistence/Disk Persistence/Snapshot/Snapshot.swift b/Sources/CodableDatastore/Persistence/Disk Persistence/Snapshot/Snapshot.swift index 6aad4ef..e2c406b 100644 --- a/Sources/CodableDatastore/Persistence/Disk Persistence/Snapshot/Snapshot.swift +++ b/Sources/CodableDatastore/Persistence/Disk Persistence/Snapshot/Snapshot.swift @@ -179,7 +179,7 @@ extension Snapshot { func updateManifest( updater: @Sendable @escaping (_ manifest: inout SnapshotManifest, _ iteration: inout SnapshotIteration) async throws -> T ) -> Task where AccessMode == ReadWrite { - if let (manifest, iteration) = SnapshotTaskLocals.manifest { + if let (manifest, iteration) = SnapshotTaskLocals.manifest(for: persistence) { return Task { var updatedManifest = manifest var updatedIteration = iteration @@ -211,7 +211,7 @@ extension Snapshot { } /// Let the updater do something with the manifest, storing the variable on the Task Local stack. - let returnValue = try await SnapshotTaskLocals.$manifest.withValue((manifest, iteration)) { + let returnValue = try await SnapshotTaskLocals.with(manifest: manifest, iteration: iteration, for: persistence) { try await updater(&manifest, &iteration) } @@ -248,7 +248,7 @@ extension Snapshot { accessor: @Sendable @escaping (_ manifest: SnapshotManifest, _ iteration: SnapshotIteration) async throws -> T ) -> Task { - if let (manifest, iteration) = SnapshotTaskLocals.manifest { + if let (manifest, iteration) = SnapshotTaskLocals.manifest(for: persistence) { return Task { try await accessor(manifest, iteration) } } @@ -271,7 +271,7 @@ extension Snapshot { } /// Let the accessor do something with the manifest, storing the variable on the Task Local stack. - return try await SnapshotTaskLocals.$manifest.withValue((manifest, iteration)) { + return try await SnapshotTaskLocals.with(manifest: manifest, iteration: iteration, for: persistence) { try await accessor(manifest, iteration) } } @@ -314,7 +314,23 @@ extension Snapshot { private enum SnapshotTaskLocals { @TaskLocal - static var manifest: (SnapshotManifest, SnapshotIteration)? + static var manifestStorage: [ObjectIdentifier : (SnapshotManifest, SnapshotIteration)] = [:] + + static func manifest(for persistence: DiskPersistence) -> (SnapshotManifest, SnapshotIteration)? { + manifestStorage[ObjectIdentifier(persistence)] + } + + static func with( + manifest: SnapshotManifest, + iteration: SnapshotIteration, + for persistence: DiskPersistence, + operation: () async throws -> R + ) async rethrows -> R { + var currentStorage = manifestStorage + currentStorage[ObjectIdentifier(persistence)] = (manifest, iteration) + + return try await $manifestStorage.withValue(currentStorage, operation: operation) + } } // MARK: - Datastore Management diff --git a/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift b/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift index d7c64a9..47713f3 100644 --- a/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift +++ b/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift @@ -56,7 +56,7 @@ extension DiskPersistence { ) async -> Task { let task = Task { isActive = true - let returnValue = try await TransactionTaskLocals.$transaction.withValue(self) { + let returnValue = try await TransactionTaskLocals.with(transaction: self, for: persistence) { try await handler() } isActive = false @@ -210,8 +210,8 @@ extension DiskPersistence { options: UnsafeTransactionOptions, @_inheritActorContext handler: @Sendable @escaping (_ transaction: Transaction, _ isDurable: Bool) async throws -> T ) async -> (Transaction, Task) { - if let parent = Self.unsafeCurrentTransaction { -// print("[CDS] Found parent, making child \(transactionIndex)") + if let parent = Self.unsafeCurrentTransaction(for: persistence) { +// print("[CDS] Found parent \(parent.transactionIndex), making child \(transactionIndex)") let (child, task) = await parent.childTransaction( transactionIndex: transactionIndex, actionName: actionName, @@ -326,8 +326,10 @@ extension DiskPersistence { } } - nonisolated static var unsafeCurrentTransaction: Self? { - TransactionTaskLocals.transaction.map({ $0 as! Self }) + nonisolated static func unsafeCurrentTransaction( + for persistence: DiskPersistence + ) -> Self? { + TransactionTaskLocals.transaction(for: persistence).map({ $0 as! Self }) } } } @@ -1373,5 +1375,20 @@ fileprivate protocol AnyDiskTransaction: Sendable {} fileprivate enum TransactionTaskLocals { @TaskLocal - static var transaction: AnyDiskTransaction? + static var transactionStorage: [ObjectIdentifier : AnyDiskTransaction] = [:] + + static func transaction(for persistence: DiskPersistence) -> AnyDiskTransaction? { + transactionStorage[ObjectIdentifier(persistence)] + } + + static func with( + transaction: AnyDiskTransaction, + for persistence: DiskPersistence, + operation: () async throws -> R + ) async rethrows -> R { + var currentStorage = transactionStorage + currentStorage[ObjectIdentifier(persistence)] = transaction + + return try await $transactionStorage.withValue(currentStorage, operation: operation) + } } From 8fa4c0541436515a613aed77abe1efb1dd10f97b Mon Sep 17 00:00:00 2001 From: Dimitri Bouniol Date: Thu, 25 Dec 2025 06:57:21 -0800 Subject: [PATCH 2/4] Added runtime validation to make sure multiple persistences are not used in a way that would lead to inconsistent outcomes --- .../Persistence/DatastoreInterfaceError.swift | 5 +++++ .../Persistence/Disk Persistence/DiskPersistence.swift | 9 +++++++++ .../Disk Persistence/Transaction/Transaction.swift | 7 +++++++ 3 files changed, 21 insertions(+) diff --git a/Sources/CodableDatastore/Persistence/DatastoreInterfaceError.swift b/Sources/CodableDatastore/Persistence/DatastoreInterfaceError.swift index 680bd04..b291b19 100644 --- a/Sources/CodableDatastore/Persistence/DatastoreInterfaceError.swift +++ b/Sources/CodableDatastore/Persistence/DatastoreInterfaceError.swift @@ -37,6 +37,9 @@ public enum DatastoreInterfaceError: LocalizedError { /// The transaction was accessed outside of its activity window. case transactionInactive + /// The transaction was started in the context of another persistence, disqualifying consistency guarantees. + case transactingWithinExternalPersistence + /// The cursor does not match the one provided by the persistence. case unknownCursor @@ -63,6 +66,8 @@ public enum DatastoreInterfaceError: LocalizedError { "The index being manipulated does not yet exist in the datastore." case .transactionInactive: "The transaction was accessed outside of its activity window. Please make sure the transaction wasn't escaped." + case .transactingWithinExternalPersistence: + "The transaction was started in the context of another persistence, disqualifying consistency guarantees. Wrap calls to this persistence's datastores and transactions in Task.detached to acknowledge this risk." case .unknownCursor: "The cursor does not match the one provided by the persistence." case .staleCursor: diff --git a/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift b/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift index 46533b8..a254012 100644 --- a/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift +++ b/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift @@ -555,6 +555,15 @@ extension DiskPersistence { transaction: @Sendable (_ transaction: DatastoreInterfaceProtocol, _ isDurable: Bool) async throws -> T ) async throws -> T { try await withoutActuallyEscaping(transaction) { escapingTransaction in + /// If the transaction is starting in the context of another persistence's transaction, make sure it is a read-only one. Otherwise assert and throw an error as it likely indicates a mistake and could lead to unexpected consistency violations if one persistence succeeds while the other fails. + if + Transaction.isTransactingExternally(to: self), + !options.contains(.readOnly) + { + assertionFailure(DatastoreInterfaceError.transactingWithinExternalPersistence.localizedDescription) + throw DatastoreInterfaceError.transactingWithinExternalPersistence + } + let currentCounter = nextTransactionCounter() // print("[CDS] Starting transaction \(currentCounter) “\(actionName ?? "")” - \(options)") let (transaction, task) = await Transaction.makeTransaction( diff --git a/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift b/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift index 47713f3..f9d3b2b 100644 --- a/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift +++ b/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift @@ -331,6 +331,13 @@ extension DiskPersistence { ) -> Self? { TransactionTaskLocals.transaction(for: persistence).map({ $0 as! Self }) } + + /// Check to see if we are in a transaction that pertains to a different persistence than the one provided. + /// + /// This is determined by checking if the only transactions present belong to other persistences. If there are no other transactions, or a transaction for the persistence is already registered, it has already been vetted and is no longer considered external. + nonisolated static func isTransactingExternally(to persistence: DiskPersistence) -> Bool { + !TransactionTaskLocals.transactionStorage.isEmpty && TransactionTaskLocals.transactionStorage[ObjectIdentifier(persistence)] == nil + } } } From 1f01025ceeb8ed65d5a40bce7a47ef2503b06ca3 Mon Sep 17 00:00:00 2001 From: Dimitri Bouniol Date: Fri, 26 Dec 2025 03:33:56 -0800 Subject: [PATCH 3/4] Updated debug logs to include the persistence name --- .../Persistence/Disk Persistence/DiskPersistence.swift | 6 +++--- .../Disk Persistence/Transaction/Transaction.swift | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift b/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift index a254012..d6e2568 100644 --- a/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift +++ b/Sources/CodableDatastore/Persistence/Disk Persistence/DiskPersistence.swift @@ -565,7 +565,7 @@ extension DiskPersistence { } let currentCounter = nextTransactionCounter() -// print("[CDS] Starting transaction \(currentCounter) “\(actionName ?? "")” - \(options)") +// print("[CDS] [\(storeURL.lastPathComponent)] Starting transaction \(currentCounter) “\(actionName ?? "")” - \(options)") let (transaction, task) = await Transaction.makeTransaction( persistence: self, transactionIndex: currentCounter, @@ -582,7 +582,7 @@ extension DiskPersistence { } let result = try await task.value -// print("[CDS] Finished transaction \(currentCounter) “\(actionName ?? "")” - \(options)") +// print("[CDS] [\(storeURL.lastPathComponent)] Finished transaction \(currentCounter) “\(actionName ?? "")” - \(options)") return result } } @@ -607,7 +607,7 @@ extension DiskPersistence { /// If nothing changed, don't bother writing anything. if !containsEdits { return } -// print("[CDS] Persisting \(transactionIndex) “\(actionName ?? "")” - \(roots.keys), added \(addedDatastoreRoots), removed \(removedDatastoreRoots)") +// print("[CDS] [\(storeURL.lastPathComponent)] Persisting \(transactionIndex) “\(actionName ?? "")” - \(roots.keys), added \(addedDatastoreRoots), removed \(removedDatastoreRoots)") /// If we are read-only, make sure no edits have been made guard let self = self as? DiskPersistence diff --git a/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift b/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift index f9d3b2b..a1b7fbe 100644 --- a/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift +++ b/Sources/CodableDatastore/Persistence/Disk Persistence/Transaction/Transaction.swift @@ -152,7 +152,7 @@ extension DiskPersistence { /// If the transaction has a parent, offload responsibility to it to persist the changes along with other children. if let parent { -// print("[CDS] Offloading persist to parent \(transactionIndex) - \(options)") +// print("[CDS] [\(persistence.storeURL.lastPathComponent)] Offloading persist to parent \(transactionIndex) - \(options)") try await parent.apply( rootObjects: rootObjects, entryMutations: entryMutations, @@ -211,7 +211,7 @@ extension DiskPersistence { @_inheritActorContext handler: @Sendable @escaping (_ transaction: Transaction, _ isDurable: Bool) async throws -> T ) async -> (Transaction, Task) { if let parent = Self.unsafeCurrentTransaction(for: persistence) { -// print("[CDS] Found parent \(parent.transactionIndex), making child \(transactionIndex)") +// print("[CDS] [\(persistence.storeURL.lastPathComponent)] Found parent \(parent.transactionIndex), making child \(transactionIndex)") let (child, task) = await parent.childTransaction( transactionIndex: transactionIndex, actionName: actionName, From cd5dcce20d5bad4324c9214be469df920695d8e5 Mon Sep 17 00:00:00 2001 From: Dimitri Bouniol Date: Fri, 26 Dec 2025 03:35:20 -0800 Subject: [PATCH 4/4] Added tests for validating multiple persistences don't overwrite each other --- .../MultiplePersistenceTests.swift | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 Tests/CodableDatastoreTests/MultiplePersistenceTests.swift diff --git a/Tests/CodableDatastoreTests/MultiplePersistenceTests.swift b/Tests/CodableDatastoreTests/MultiplePersistenceTests.swift new file mode 100644 index 0000000..1847649 --- /dev/null +++ b/Tests/CodableDatastoreTests/MultiplePersistenceTests.swift @@ -0,0 +1,108 @@ +// +// MultiplePersistenceTests.swift +// CodableDatastore +// +// Created by Dimitri Bouniol on 2025-12-26. +// Copyright © 2023-25 Mochi Development, Inc. All rights reserved. +// + +#if !canImport(Darwin) +@preconcurrency import Foundation +#endif +import XCTest +@testable import CodableDatastore + +final class MultiplePersistenceTests: XCTestCase, @unchecked Sendable { + var temporaryStoreURLOuter: URL = FileManager.default.temporaryDirectory + var temporaryStoreURLInner: URL = FileManager.default.temporaryDirectory + + override func setUp() async throws { + temporaryStoreURLOuter = FileManager.default.temporaryDirectory.appendingPathComponent(ProcessInfo.processInfo.globallyUniqueString + "-Outer", isDirectory: true); + temporaryStoreURLInner = FileManager.default.temporaryDirectory.appendingPathComponent(ProcessInfo.processInfo.globallyUniqueString + "-Inner", isDirectory: true); + } + + override func tearDown() async throws { + try? FileManager.default.removeItem(at: temporaryStoreURLOuter) + try? FileManager.default.removeItem(at: temporaryStoreURLInner) + } + + func testCanWorkWithMultiplePersistences() async throws { + let outerPersistence = try DiskPersistence(readWriteURL: temporaryStoreURLOuter) + let innerPersistence = try DiskPersistence(readWriteURL: temporaryStoreURLInner) + + struct TestFormat: DatastoreFormat { + enum Version: Int, CaseIterable { + case zero + } + + struct Instance: Codable, Identifiable { + var id: String + var value: String + } + + static let defaultKey: DatastoreKey = "test" + static let currentVersion = Version.zero + } + + let outerDatastore = Datastore.JSONStore( + persistence: outerPersistence, + format: TestFormat.self, + migrations: [ + .zero: { data, decoder in + try decoder.decode(TestFormat.Instance.self, from: data) + } + ] + ) + + let innerDatastore = Datastore.JSONStore( + persistence: innerPersistence, + format: TestFormat.self, + migrations: [ + .zero: { data, decoder in + try decoder.decode(TestFormat.Instance.self, from: data) + } + ] + ) + + /// Set some default starting values + try await outerDatastore.persist(.init(id: "A", value: "OuterA")) + try await outerDatastore.persist(.init(id: "B", value: "OuterB")) + try await innerDatastore.persist(.init(id: "A", value: "InnerA")) + try await innerDatastore.persist(.init(id: "B", value: "InnerB")) + + /// Start a transaction in the outer persistence, and modify a record. + try await outerPersistence.perform { + try await outerDatastore.persist(.init(id: "A", value: "OuterA-New")) + try await innerPersistence.perform(options: .readOnly) { + /// Access the inner persistence within a read-only transaction. + let innerValueA = try await innerDatastore.load(id: "A") + XCTAssertEqual(innerValueA?.value, "InnerA") + + try await Task.detached { + /// Attempt to modify the inner persistence in a detached task, which should succeed without issue. + try await innerDatastore.persist(.init(id: "B", value: "InnerB-New")) + let innerValueB = try await innerDatastore.load(id: "B") + XCTAssertEqual(innerValueB?.value, "InnerB-New") + }.value + + /// Check for the old value since we are locked to a transaction where reads already started. + let innerValueB = try await innerDatastore.load(id: "B") + XCTAssertEqual(innerValueB?.value, "InnerB") + } + + /// Check to see that the inner persistence value is unchanged. + let innerValue = try await innerDatastore.load(id: "A") + XCTAssertEqual(innerValue?.value, "InnerA") + } + + /// Make sure final values all agree. + let outerValueA = try await outerDatastore.load(id: "A") + let outerValueB = try await outerDatastore.load(id: "B") + let innerValueA = try await innerDatastore.load(id: "A") + let innerValueB = try await innerDatastore.load(id: "B") + XCTAssertEqual(outerValueA?.value, "OuterA-New") + XCTAssertEqual(outerValueB?.value, "OuterB") + XCTAssertEqual(innerValueA?.value, "InnerA") + XCTAssertEqual(innerValueB?.value, "InnerB-New") + } +}