Merge pull request #96 from bitofmind/claude/transaction-fallback-lock

Take the tester lock in transactions through a creation handle (1.1.4 deadlock)

Author: mansbernhardt

CHANGELOG.md

@@ -8,6 +8,9 @@ All notable changes are documented here. The format follows [Keep a Changelog](h
 
 ### Fixed
 
+- **1.1.4 could deadlock a test that ran `node.transaction` through a model's creation handle.** 1.1.4 made a write through a handle with no access fall back to the tree's tester, and that write takes the tester's lock. A transaction decides which lock to take before it starts, but its chain lacked the fallback. So `dup.node.transaction { dup.value = … }` took the context lock first and then waited for the tester lock, while any reader holding the tester lock (an `expect` evaluation, a model task reading state) waited for the context lock. Downstream this hung a stream-environment test every time, on 1.1.4 only.
+  - `node.transaction`, and the internal `ModelContext.transaction` / `stateTransaction`, now include the same fallback, so the transaction takes the tester lock before the context lock, the same order as every other writer.
+  - `CreationHandleTransactionLockTests` checks that, inside such a transaction, another thread finds the tester lock already held. Before the fix it found the lock free, every run.
 - **An exhaustivity report named the wrong property when the model held a collection of models before it.** The name comes from counting the model's properties as they are visited, but a collection of models (`[Item]`, `IdentifiedArray`, a collection of `@ModelContainer` enums) was never counted. Each one shifted every later property's name back by one, so a write to `trigger` after `var items: [Item]` was reported as `Root.items: 0 → 1`. Collections are now counted, so the report reads `Root.trigger: 0 → 1`.
   - `PropertyNameAfterCollectionTests` covers a property after a model array and after a collection of `@ModelContainer` enums. Before the fix, both reported the wrong name.
 

Sources/SwiftModel/Internal/ModelContext+Internal.swift

@@ -231,7 +231,9 @@ extension ModelContext {
             // ?? ModelAccess.current`), so the outer transaction's
             // `TestAccess.lock` matches the nested writes' `TestAccess.lock`
             // by identity. See the long comment at `Context.transaction(writeLockHolder:_:)`.
-            let writeLockHolder = ModelAccess.active?.writeLockOwner ?? _access._reference?.access?.writeLockOwner ?? ModelAccess.current?.writeLockOwner
+            // The trailing `fallbackTestAccess` matches the nested writes' fallback;
+            // see `ModelNode.transaction`.
+            let writeLockHolder = ModelAccess.active?.writeLockOwner ?? _access._reference?.access?.writeLockOwner ?? ModelAccess.current?.writeLockOwner ?? context.fallbackTestAccess?.writeLockOwner
             return try context.transaction(writeLockHolder: writeLockHolder, callback)
         } else {
             return try callback()
@@ -245,7 +247,7 @@ extension ModelContext {
     func stateTransaction<T>(_ callback: () throws -> T) rethrows -> T {
         if let context {
             // Same writeLockHolder chain as `transaction(_:)` above.
-            let writeLockHolder = ModelAccess.active?.writeLockOwner ?? _access._reference?.access?.writeLockOwner ?? ModelAccess.current?.writeLockOwner
+            let writeLockHolder = ModelAccess.active?.writeLockOwner ?? _access._reference?.access?.writeLockOwner ?? ModelAccess.current?.writeLockOwner ?? context.fallbackTestAccess?.writeLockOwner
             return try context.transaction(writeLockHolder: writeLockHolder, callback)
         } else {
             return try callback()

Sources/SwiftModel/ModelNode.swift

@@ -348,7 +348,13 @@ public extension ModelNode {
                 // Use the same writeLockHolder chain that `stateTransaction`
                 // uses for nested property writes — see the comment block in
                 // `Context.transaction(writeLockHolder:_:)`.
-                let writeLockHolder = ModelAccess.active?.writeLockOwner ?? _$modelContext._access._reference?.access?.writeLockOwner ?? ModelAccess.current?.writeLockOwner
+                // The trailing `fallbackTestAccess` matches the nested writes: a write
+                // whose chain resolves nothing else falls back to the tree's tester
+                // and takes its lock, so this transaction must take it first. Without
+                // it, a transaction through a creation handle held the context lock
+                // while a nested write waited for the tester lock: AB-BA against any
+                // tester-locked reader.
+                let writeLockHolder = ModelAccess.active?.writeLockOwner ?? _$modelContext._access._reference?.access?.writeLockOwner ?? ModelAccess.current?.writeLockOwner ?? context.fallbackTestAccess?.writeLockOwner
                 return context.transaction(writeLockHolder: writeLockHolder, callback)
             }
         } else {

Tests/SwiftModelTests/CreationHandleTransactionLockTests.swift

@@ -0,0 +1,75 @@
+#if canImport(Dispatch)
+import Foundation
+import Dispatch
+import Testing
+import ConcurrencyExtras
+@testable import SwiftModel
+
+/// A transaction through a model's creation handle must take the tester's write lock
+/// before the context lock, like every other writer.
+///
+/// 1.1.4 let a write through a handle with no access fall back to the tree's tester
+/// (`fallbackTestAccess`). Writes inside a transaction resolve that fallback and take
+/// the tester lock, but the transaction's own lock chain didn't include it. So
+/// `dup.node.transaction { dup.x = … }` held the context lock and then waited for the
+/// tester lock, while a reader holding the tester lock waited for the context lock.
+/// Downstream that was a deterministic deadlock: a stream-environment model updated
+/// in a transaction while a media controller's task read model state.
+///
+/// The deadlock needs a racing reader, so the test checks the invariant directly:
+/// while the transaction body runs, another thread must find the tester lock held.
+@Suite
+struct CreationHandleTransactionLockTests {
+
+    @Test func nodeTransactionThroughCreationHandleHoldsTesterLock() {
+        let tester = ModelTester(LockRoot(items: []), exhaustivity: .off)
+        let root = tester.model
+        let dup = LockItem(id: -1, value: 0)
+        root.items.append(dup)
+
+        let testerLock = tester.access.lock
+        let heldByTransaction = LockIsolated<Bool?>(nil)
+        dup.node.transaction {
+            heldByTransaction.setValue(Self.isHeldByAnotherThread(testerLock))
+            dup.value = 1
+        }
+
+        #expect(heldByTransaction.value == true)
+        #expect(root.items[0].value == 1)
+    }
+
+    /// `NSRecursiveLock.try()` from a different thread fails exactly when some thread
+    /// holds the lock.
+    ///
+    /// The probe runs on a dedicated `Thread`, not a GCD queue. The caller blocks until
+    /// it answers, and it blocks a Swift-concurrency thread while holding locks. A
+    /// `DispatchQueue.global()` block took ~60 s to be scheduled on a TSan CI runner,
+    /// and parking a cooperative thread that long starved the parallel tests' model
+    /// tasks. A new thread starts at once, so the caller waits only for one `try()`.
+    static func isHeldByAnotherThread(_ lock: NSRecursiveLock) -> Bool {
+        let result = LockIsolated(false)
+        let done = DispatchSemaphore(value: 0)
+        let probe = Thread {
+            if lock.try() {
+                lock.unlock()
+                result.setValue(false)
+            } else {
+                result.setValue(true)
+            }
+            done.signal()
+        }
+        probe.start()
+        done.wait()
+        return result.value
+    }
+}
+
+@Model private struct LockItem: Identifiable {
+    var id: Int
+    var value: Int
+}
+
+@Model private struct LockRoot {
+    var items: [LockItem]
+}
+#endif