From 90b99bc42a289e0378d2c1136dc26ea39153e8be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5ns=20Bernhardt?= Date: Fri, 25 Sep 2026 22:05:50 +0200 Subject: [PATCH] Don't run a memoize's producer on a model torn down mid re-evaluation Teardown cuts parent links before it destructs a model and cancels its memoizes; the parent-link change wakes memoizes that read ancestors. A withObservationTracking re-evaluation already past its cancellation check blocked on the context lock until the cascade finished, then ran produce() on a model with no ancestors. A producer like `node.memoize { node.member(in: .ancestors) ?? Fallback() }` then called memoize on the never-anchored fallback, reporting 'unanchored model node' from the background queue, attributed to no test (xcodebuild exits nonzero; 57 per downstream ParallelEditorTests run). The update closure now checks the model's lifetime under the context lock and serves the last produced value once it has been destructed. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 6 ++ Sources/SwiftModel/Model+Changes.swift | 25 ++++++++- .../MemoizeDuringTeardownTests.swift | 56 +++++++++++++++++++ 3 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 Tests/SwiftModelTests/MemoizeDuringTeardownTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 4861216..07ce9d8 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 memoize re-evaluated during its model's teardown ran its producer on the torn-down model, recording unattributed "memoize(...) on an unanchored model node" issues.** A teardown cuts a model's parent links before it destructs the model and cancels its memoizes, and the parent-link change wakes memoizes that read ancestors. On the `withObservationTracking` path the re-evaluation is scheduled on the background queue; one already past its cancellation check blocked on the context lock until the cascade finished, then ran `produce()` on a model with no ancestors. A producer like `node.memoize { node.member(in: .ancestors) ?? TimelineModel() }` then fell back to a fresh, never-anchored model, and the memoize it called on that model reported. The report was raised on the background queue after the test had finished, so it was attributed to no test and made `xcodebuild` exit nonzero despite `TEST SUCCEEDED` (57 per `ParallelEditorTests` run downstream). + - Once inside the context lock, the memoize's update closure now checks whether the model has been destructed, and if so serves its last produced value instead of running the producer. The result would have been discarded anyway, because teardown clears the cache. + - `MemoizeDuringTeardownTests` reproduces the downstream shape. Without the fix, 200 iterations ran the producer on a torn-down model 293 times and recorded 186 unattributed issues; with it, zero and zero. + --- ## [1.1.0] — Signals (`onSignal` / `signal` / `onTeardown`) + `TestPredicate` `==` no longer leaks into app code diff --git a/Sources/SwiftModel/Model+Changes.swift b/Sources/SwiftModel/Model+Changes.swift index a30b756..12d152d 100644 --- a/Sources/SwiftModel/Model+Changes.swift +++ b/Sources/SwiftModel/Model+Changes.swift @@ -652,6 +652,10 @@ private extension ModelNode { let useWithObservationTracking = context.hasObservationRegistrar && useCoalescing let usesAsyncTracking = useWithObservationTracking // Capture for cache entry + // The last value `produce()` returned, served instead of re-producing once + // this model has been torn down (see the destructed check below). + let lastProduced = LockIsolated(nil) + return update( initial: true, isSame: nil, @@ -684,14 +688,29 @@ private extension ModelNode { // performUpdate's access() call. This avoids spurious extra computes from // other access() call sites (forceObserver setup, initial registration, // dirty-path sync reads). - let (entry, version): (AnyContext.MemoizeCacheEntry?, UInt64) = context.lock { + let (entry, version, isTornDown): (AnyContext.MemoizeCacheEntry?, UInt64, Bool) = context.lock { let entry = context._memoizeCache[key] - return (entry, entry?.dirtyVersion ?? 0) + return (entry, entry?.dirtyVersion ?? 0, context.unprotectedIsDestructed) + } + // Torn down: never run the producer on a removed model. A teardown + // cuts parent links before it destructs a model and cancels its + // memoizes, and the parent-link change wakes memoizes that read + // ancestors. A re-evaluation already scheduled past the cancellation + // check blocks on `context.lock` above until the cascade finishes, + // and would then run `produce()` on a model with no ancestors — e.g. + // falling back to a fresh, never-anchored model and calling memoize on + // it, reporting "unanchored model node" unattributed to any test. The + // result would be discarded anyway (the cache is gone), so serve the + // last produced value. + if isTornDown, let last = lastProduced.value { + return (last, version) } if !threadLocals.isInsideAsyncPerformUpdate, let entry = entry, !entry.isDirty { return (entry.value as! T, version) } - return (produce(), version) + let value = produce() + lastProduced.setValue(value) + return (value, version) } onUpdate: { @Sendable (produced: (value: T, version: UInt64)) in let value = produced.value var postLockCallbacks: [() -> Void] = [] diff --git a/Tests/SwiftModelTests/MemoizeDuringTeardownTests.swift b/Tests/SwiftModelTests/MemoizeDuringTeardownTests.swift new file mode 100644 index 0000000..0c7ab27 --- /dev/null +++ b/Tests/SwiftModelTests/MemoizeDuringTeardownTests.swift @@ -0,0 +1,56 @@ +import Testing +@testable import SwiftModel +import ConcurrencyExtras +import IssueReporting + +// A memoize whose producer resolves an ancestor, falling back to a fresh (never-anchored) +// model when there is none, then calls a memoize on the result — the shape of a +// real-world `timelineModel` accessor (`node.memoize { node.member(in: .ancestors) ?? +// TimelineModel() }`). Tearing the tree down cuts the child's parent link first; that +// wakes the child's memoizes, and a re-evaluation racing the teardown cascade ran the +// producer on a model being torn down: the ancestor was gone, the fallback was used, and +// memoize on the never-anchored fallback reported "memoize(...) on an unanchored model +// node is not allowed" — unattributed to any test, after the test had finished. +struct MemoizeDuringTeardownTests { + @Test func reevaluationRacingTeardownDoesNotRunTheProducer() async { + // The reports are raised on the background queue, outside any task-local issue + // reporter (they surface as unattributed issues), so assert on their cause: the + // producer running on a torn-down model, i.e. the ancestor fallback being taken. + let fallbacks = LockIsolated(0) + for _ in 0..<60 { + await withModelTesting(exhaustivity: .off) { + let root = Timeline(segments: (0..<8).map { _ in Segment(fallbacks: fallbacks) }).withAnchor() + for segment in root.segments { _ = segment.visibleCount } + } + } + await backgroundCall.waitUntilIdle(deadline: _monotonicNs() + 5_000_000_000) + #expect(fallbacks.value == 0) + } +} + +@Model private struct Timeline { + var media: [Int] = [1, 2, 3] + var segments: [Segment] + + var mediaCount: Int { + node.memoize { media.count } + } +} + +@Model private struct Segment { + let fallbacks: LockIsolated + + var timeline: Timeline { + node.memoize { + if let timeline = node.mapHierarchy(for: .ancestors, transform: { $0 as? Timeline }).first { + return timeline + } + fallbacks.withValue { $0 += 1 } + return Timeline(segments: []) + } + } + + var visibleCount: Int { + node.memoize { timeline.mediaCount } + } +}