diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d91f85..88cbb23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ All notable changes are documented here. The format follows [Keep a Changelog](h ## [Unreleased] +### Fixed + +- **A model dependency shared across model trees could deadlock the process.** `AnyContext.dependency(for:)` resolves a model dependency while holding its own tree's hierarchy lock, and the resolution copied the dependency model through `MakeInitialDependencyCopyTransformer` → `Model.shallowCopy` → `ModelContext.makeFrozen`, which reads the model's state under **that model's** hierarchy lock. A dependency declared as a `static let` is shared, so the model being copied is routinely anchored in a *different* tree: two threads resolving two such dependencies in opposite order took the two locks in opposite order and deadlocked. Every other thread that then wanted either lock — in a `.modelTesting` run, including the executor the wait verbs drain — queued behind them, so a two-thread deadlock became a whole-process hang in which every `expect` rode to its ceiling and reported a timeout. It was diagnosed from a live `sample` of a hung CI run showing four threads in `__psynch_mutexwait` with none holding-and-running, which is what an AB-BA looks like; it reproduced at roughly 3% of full test-plan runs on 1.0.16 and 1.0.17 alike, and neither the drain queue's QoS nor the number of cores affected it. + + The transformer now resolves genesis state *before* copying rather than after, which removes the foreign-lock acquisition entirely — and the frozen state it used to take that lock for was never used on this path. `Reference.setContext` captures genesis on the very first anchor, so every Reference that has (or ever had) a live context has genesis, which is exactly the case in which `shallowCopy` would freeze under a foreign lock; and `!_hasGenesis` implies the Reference was never anchored, so it has no context and no lock to take. The retained fallback is therefore foreign-lock-free by construction. The one observable side effect of the skipped `shallowCopy` — clearing `modelContext.access` when it freezes an anchored model — is preserved under the identical condition. `DependencyLockInversionTests` is the regression test: two trees, two `@Model`-typed `static let` dependencies resolved concurrently in opposite order, with a progress-based verdict rather than a wall-clock one so a slow machine cannot fail it. + --- ## [1.0.17] — Hot-path performance: index-keyed accessors, lock-free registrar lookups, 10× faster collection reconcile diff --git a/Sources/SwiftModel/Internal/ModelContainer+Internals.swift b/Sources/SwiftModel/Internal/ModelContainer+Internals.swift index 452c6af..067dd18 100644 --- a/Sources/SwiftModel/Internal/ModelContainer+Internals.swift +++ b/Sources/SwiftModel/Internal/ModelContainer+Internals.swift @@ -78,31 +78,56 @@ private struct MakeInitialTransformer: ModelTransformer { private struct MakeInitialDependencyCopyTransformer: ModelTransformer { func transform(_ model: inout M) -> Void { - // Capture the original reference BEFORE shallowCopy may convert an anchored model - // to a frozen snapshot. shallowCopy creates a brand-new Reference with no genesis, - // losing the original's _genesisState. We need the original to recover genesis. - // For pre-anchor models, shallowCopy returns self unchanged, so originalRef === srcRef. - let originalRef = model.modelContext._source.reference - model = model.shallowCopy - // Create a fresh Reference with a new identity and copy state from the frozen copy. - // Without state, Context.init's `hasState` assertion would fire when anchoring this copy. - let srcRef = model.modelContext._source.reference + let src = model.modelContext._source + let originalRef = src.reference + // Prefer genesis state when available — matching reserveOrFork() — so that a fresh // dependency copy always starts from the clean initial state, not from mutations // made by a concurrently-running test that has the same static `testValue` anchored. - // Use originalRef for genesis since shallowCopy may have replaced srcRef with a - // frozen snapshot reference that has no _hasGenesis/_genesisState of its own. - // If state has been cleared post-TTL but genesis was captured, genesis is also used. - // If neither live state nor genesis is available, bail out and leave model.context - // intact; setupModelDependency guards against this residual non-nil context. - let genesisRef = originalRef._hasGenesis ? originalRef : srcRef - guard !srcRef._stateCleared || genesisRef._hasGenesis else { return } - let sourceState = genesisRef._hasGenesis ? genesisRef._genesisState : srcRef.state - let newRef = Context.Reference(modelID: .generate(), state: sourceState) - if genesisRef._hasGenesis { - newRef._genesisState = genesisRef._genesisState + // + // Genesis is resolved BEFORE any `shallowCopy`, and that ordering is load-bearing + // for more than efficiency. `shallowCopy` freezes an anchored model by reading its + // state under **that model's** hierarchy lock (`makeFrozen` → + // `Reference.withHierarchyLockIfLive`). A shared `static let` dependency value is + // routinely anchored in a *different* tree, so that is a foreign lock — and this + // transformer runs inside `AnyContext.dependency(for:)`, which already holds its + // own tree's hierarchy lock. Two threads resolving two such dependencies in + // opposite order took the two locks A→B and B→A and deadlocked the process + // (diagnosed 2026-09-07 from a live `sample` of a hung CI run). + // + // Taking the genesis path first removes that edge entirely, because the frozen + // state was never used in the first place: `Reference.setContext` captures genesis + // on the very first anchor, so *every* Reference that has (or has ever had) a live + // context has genesis — exactly the case in which `shallowCopy` would take a + // foreign hierarchy lock. Conversely `!_hasGenesis` implies the Reference was never + // anchored, so it has no context and `withHierarchyLockIfLive` finds no lock to + // take. The fallback below is therefore foreign-lock-free by construction. + if originalRef._hasGenesis { + let sourceState = originalRef._genesisState + let newRef = Context.Reference(modelID: .generate(), state: sourceState) + newRef._genesisState = sourceState newRef._hasGenesis = true + // `shallowCopy` drops the access when it freezes an anchored model; preserve + // that here, under the same condition, now that we skip the copy. + if !src._isLive, originalRef.context != nil { + model.modelContext.access = nil + } + model.modelContext.setReference(newRef) + return } + + // No genesis — the Reference was never anchored (pre-anchor value, or a + // frozen/lastSeen snapshot). `shallowCopy` cannot reach a live context's hierarchy + // lock from here; it either returns `self` or freezes a snapshot Reference, whose + // `_context` is always nil. + model = model.shallowCopy + // Create a fresh Reference with a new identity and copy state from the frozen copy. + // Without state, Context.init's `hasState` assertion would fire when anchoring this copy. + let srcRef = model.modelContext._source.reference + // If state has been cleared post-TTL and no genesis was captured, bail out and leave + // model.context intact; setupModelDependency guards against this residual non-nil context. + guard !srcRef._stateCleared else { return } + let newRef = Context.Reference(modelID: .generate(), state: srcRef.state) model.modelContext.setReference(newRef) } } diff --git a/Tests/SwiftModelTests/DependencyLockInversionTests.swift b/Tests/SwiftModelTests/DependencyLockInversionTests.swift new file mode 100644 index 0000000..5ec65d2 --- /dev/null +++ b/Tests/SwiftModelTests/DependencyLockInversionTests.swift @@ -0,0 +1,155 @@ +import Testing +import Foundation +@testable import SwiftModel +import SwiftModel +import Dependencies + +// WASI is single-threaded and has neither `Thread` nor `DispatchSemaphore`, so the race +// this reproduces cannot occur there and the test cannot be expressed. +#if !os(WASI) + +/// Reproduction for the AB-BA deadlock between two contexts' hierarchy locks. +/// +/// `AnyContext.dependency(for:)` holds ITS OWN hierarchy lock while resolving a model +/// dependency, and that resolution copies the dependency model — `initialDependencyCopy` +/// → `shallowCopy` → `Reference.lifetime` / `makeFrozen` → **another context's** hierarchy +/// lock. Two threads resolving dependencies that reach into each other's trees acquire the +/// two locks in opposite order and deadlock; a live `sample` of a wedged CI run on +/// 2026-09-07 showed exactly that, with the drive executor queued behind one of them, which +/// is what turns it into a whole-process hang. +/// +/// Each iteration anchors two independent trees and resolves a model dependency on both +/// concurrently. The work runs on detached threads with a wall-clock bound so a regression +/// reports instead of hanging the suite. +@Model private struct DepA: Sendable { + var value = 1 +} +extension DepA: DependencyKey { + static let liveValue = DepA(value: 1) + static let testValue = DepA(value: 1) +} + +@Model private struct DepB: Sendable { + var value = 2 +} +extension DepB: DependencyKey { + static let liveValue = DepB(value: 2) + static let testValue = DepB(value: 2) +} + +@Model private struct LeafOne: Sendable { + @ModelDependency var a: DepA + @ModelDependency var b: DepB + var touched = 0 + func touch() { touched = a.value &+ b.value } +} + +@Model private struct LeafTwo: Sendable { + @ModelDependency var b: DepB + @ModelDependency var a: DepA + var touched = 0 + func touch() { touched = b.value &+ a.value } +} + +private final class Flag: @unchecked Sendable { + private let l = NSLock(); private var v = false + var value: Bool { get { l.lock(); defer { l.unlock() }; return v } set { l.lock(); v = newValue; l.unlock() } } +} + +private final class Counter: @unchecked Sendable { + private let l = NSLock(); private var v = 0 + var value: Int { l.lock(); defer { l.unlock() }; return v } + func increment() { l.lock(); v += 1; l.unlock() } +} + +struct DependencyLockInversionTests { + /// Regression test for the AB-BA fixed by giving `modeLifeTime` its own leaf lock and + /// by resolving genesis state before `shallowCopy` in `MakeInitialDependencyCopyTransformer` + /// — the two places where `dependency(for:)` reached a *foreign* hierarchy lock while + /// holding its own. Deadlocked on the very first iteration before the fix (zero + /// iterations completed); completes 200 iterations in ~0.2 s after it. + /// + /// The verdict is a *stall* detector rather than a total-runtime budget — see the + /// comment in the body — so a regression reports instead of hanging the suite, without + /// the pass/fail line depending on how loaded the machine is. + /// + /// Note for future fixers: removing the `lock { }` from `dependency(for:)` altogether + /// (with a first-wins dependency-cache install) also makes this pass, but crashes the + /// full parallel suite deterministically inside Swift-runtime generic-metadata + /// instantiation. That lock does more than guard the cache check-then-act; the fix has + /// to remove the foreign-lock edges from under it, not remove the lock. + @Test func concurrentDependencyResolutionAcrossTreesDoesNotDeadlock() async { + let iterations = 200 + let done = Flag() + let stop = Flag() + let progress = Counter() + + let worker = Thread { + for _ in 0.. Void] { + Thread { + ready.signal(); go.wait() + model() + finished.signal() + }.start() + } + ready.wait(); ready.wait() + go.signal(); go.signal() + finished.wait(); finished.wait() + progress.increment() + withExtendedLifetime((anchorOne, anchorTwo)) {} + } + done.value = true + } + worker.start() + + // Evidence-based verdict, not a wall-clock budget: an AB-BA parks both resolver + // threads forever, so the iteration counter stops moving and never restarts. A + // merely slow run (TSan is 5-15x, and this suite runs in parallel with 860+ other + // tests) keeps incrementing it. So we fail only on a *stall* — no forward progress + // for `stallBudget` — and treat "still progressing when the overall ceiling is + // reached" as a pass, since sustained forward progress is exactly what the test + // asserts. Both bounds honour SWIFT_MODEL_TIMEOUT_SCALE like the rest of the suite. + let scale = ProcessInfo.processInfo.environment["SWIFT_MODEL_TIMEOUT_SCALE"].flatMap(Double.init) ?? 1 + let stallBudget = 10.0 * scale + let ceiling = Date().addingTimeInterval(120 * scale) + + var lastProgress = progress.value + var lastProgressAt = Date() + var stalled = false + while !done.value && Date() < ceiling { + try? await Task.sleep(nanoseconds: 20_000_000) + let current = progress.value + if current != lastProgress { + lastProgress = current + lastProgressAt = Date() + } else if Date().timeIntervalSince(lastProgressAt) > stallBudget { + stalled = true + break + } + } + stop.value = true + + #expect( + !stalled, + """ + concurrent cross-tree dependency resolution made no progress for \(stallBudget)s \ + after \(lastProgress)/\(iterations) iterations — the two hierarchy locks are \ + deadlocked again + """ + ) + // Let the worker unwind before the test returns so its threads do not outlive it. + while !done.value && !stalled && Date() < ceiling.addingTimeInterval(5 * scale) { + try? await Task.sleep(nanoseconds: 20_000_000) + } + } +} + + +#endif \ No newline at end of file