Decide test quiescence from SwiftModel's own work registry, not from executor idleness - #74
mansbernhardt wants to merge 11 commits into
Conversation
…strumentation Implements steps 1–2 of Docs/test-quiescence-redesign.md §10 (spike + compute-but-don't-use), leaving every existing verdict path untouched. * `ModelWorkUnit` — per-`TaskCancellable` running/parked state, stored as a signed counter of non-parked activities (start 1, park decrements, running iff > 0) so nested parks compose. Replaces the `LockIsolated<Bool>` `hasStartedRunning` box, so no extra allocation per task. * `withModelParked` — public primitive that finds the current work unit via a task-local set once per task body (so it propagates into child tasks) and marks it parked for the duration of `body`. No-op passthrough outside a model task. * Hook 1: `node.forEach` / `node.onChange` park around their own `next()` only, never around the body — this makes any AsyncSequence (incl. swift-async-algorithms `debounce`/`throttle`) park with zero adoption. `_DedupBox` parks around its upstream wait for the same reason. * `AnyContext.semanticQuiescence` — the new answer (no running unit in the tree + both call queues idle), computed in `_driveToStableFixpoint` beside the existing one. Disagreements are tallied, and traced per-check with `SWIFT_MODEL_QUIESCENCE_TRACE=1` (/tmp/swift-model-quiescence-trace.log, summary at exit). The existing answer still decides every verdict. * `SemanticQuiescenceTests` — foreign-clock parked/unparked, `Task.yield()` loop is running (the PR #70 blind spot), forEach parked-vs-delivering, await-free compute loop running, passthrough outside a model task, nesting. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Makes the disagreement inventory classifiable: `newQuiescentOldNot` lines now say which of the existing answer's parts was busy (executor / bg / main / pendingStart), and `oldQuiescentNewNot` lines de-duplicate identical call sites and cap the list at five with a total count (a wide hierarchy produced 80 identical entries on one line). `atexit` summary is skipped on WASI. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Closes the two gaps the first dual-run inventory measured
(`Docs/test-quiescence-redesign.md` §4a/§5). Verdicts are unchanged
throughout: `idleNow` still decides every wait; only the semantic answer
computed beside it moves.
Gap 1 — tier 1 source-side parking (§5). The prototype only implemented
hook 1 (`node.forEach` parking around its own `next()`), so a
hand-written `node.task { for await v in Observed { … } }` read as
*running* forever — 23 of the 33 decisive disagreements. The park mark
now lives where the design says it belongs: the input source. A new
`_eraseToParkedWaitStream()` wraps every stream SwiftModel produces
(`Observed`, all eight `node.event(…)` overloads, `observeModifications`)
so the consumer's wait for the next yield parks the calling work unit,
`forEach` or raw loop alike. For events this REPLACES an
`eraseToStream()` that already built an unfolding stream, so it costs
nothing; `Observed` and `observeModifications` gain one layer.
Gap 2 — the epilogue windows (category (c), the dangerous one). Two
places unregistered a task's work unit while the task could still write
to the model:
* `TaskCancellable`'s `defer { onDone() }` sat inside the innermost
task-local scope, i.e. it fired BEFORE the `catch` handler — and
`node.task(catch:)` / `node.onChange(catch:)` handlers routinely
write model state. Hoisted to the closure's outermost scope.
* `Cancellations` drops an entry from `registered` before the body
unwinds (`cancelAll()` empties the dictionary, then calls
`onCancel()`), so every teardown / `task(id:)` replacement /
`cancelPrevious` swap read as quiescent while cancelled bodies ran
their `defer`s. Work units now live in a separate `liveWorkUnits`
map retired by exactly one thing — the body's outermost `defer`.
Together these took the "job on the executor owning no registered unit"
bucket from 31/46 of direction B to 2/42.
Categories (a) test-drive machinery and (d) excluded housekeeping are
now enumerated in `ModelWorkUnit.swift` with a per-site justification at
`Context.swift`'s TTL cleanup and `ObservedModel.swift`'s priming task.
Diagnostics: `SWIFT_MODEL_QUIESCENCE_JOB_TRACE=1` symbolicates the stack
of whatever made each still-ready executor job runnable (expensive —
targeted runs only); the disagreement trace now also reports the
work-unit census and whether any unit parked/unparked since the previous
check (§5 property 2), which is what identifies the one remaining
direction-B category.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Records the two constraints that picked the mechanism — the public `AsyncStream` / `AsyncStream.Iterator` return types stay identical, and `for await` keeps the stdlib's SE-0431 `next(isolation:)` rather than an availability-gated witness of our own — plus the one residual cost (a consumer iterating from an actor now hops off it once per element to run `produce`, which every `node.event(…)` stream already paid because `eraseToStream()` is itself an unfolding wrapper). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Increment 2 left direction B (new says quiescent, old says busy) flat at
42-62 per suite run, with the census showing `registered == parked` and a
job ready to run. Job stacks attributed 62 % of it to an `AsyncStream`
consumer resumed through `next()`'s CANCELLATION handler
(`_Storage.finish()`) and 20 % to `Continuation.yield`: in both, the
continuation has been resumed but the resumed task has not reached its
`defer { unpark() }`, so the unit still reads parked while model-writing
work is inbound.
Design §5 said that window was contained, on two grounds that measurement
killed. (a) "The yield is itself an activity signal" is true for yield and
false for cancellation — `_Storage.finish()` from the cancel handler emits
no signal at all, and the body it resumes then runs model-writing `defer`s.
(b) The prescribed park-generation double-check does not see it: increment
2 measured `parkGeneration` and it had not changed in any sample.
So the unpark becomes eager. `ModelWorkUnit.noteResumeInFlight()` marks the
unit running at the moment the continuation is resumed; `park()` now hands
back a `_ParkTicket` stamped with a park EPOCH that `noteResumeInFlight()`
bumps, so the resumed side's `defer` is a no-op — the idempotence the shape
needs, verified rather than assumed. One force invalidates every open park
scope, which is what lets an eager unpark escape `forEach`'s own enclosing
hook-1 park.
Two things in the prescribed shape needed correcting:
* The park had to move INWARDS. `node.event(ofType:)` is
`events().compactMap { … }`; an event failing the filter resumes the
consumer, which filters it and loops back into the SAME open park scope.
An eager unpark wrapped around the filter would leave the unit reading
running until the next matching event — possibly never. The mark now sits
on the raw source (`AnyContext.events()`, `Observed`, `observeModifications`),
entered and left exactly once per upstream element, so filtering re-parks
by construction. The 8 `node.event(…)` overloads go back to plain
`eraseToStream()`.
* Resume-in-flight is not the only way a park can be wrong. A value yielded
while the consumer was RUNNING sits in the stream's buffer, so the
consumer's next `park()` marks parked for a delivery that is already
queued an executor hop away. `_ParkSource` counts undelivered yields and
refuses to park while any are outstanding — and, because `forEach` wraps
its own park around the whole call, forces the unit running rather than
merely declining.
`_ParkSource` is the producer/consumer rendezvous; `_ParkedYield` is an
`AsyncStream.Continuation` spelled identically that tells the source about
every resume first, so source sites change only where the stream is built.
The cancellation hook is `onTermination`, which the stdlib documents as
running before `next()` is resumed with nil.
Verdicts are unchanged: `idleNow` still decides everything.
Full-suite dual run, 3-4 runs each side:
direction B 43/56/76 -> 7-31 (mean 55 -> 15)
park-resume-in-flight 52/run -> 7/run
job owns no unit 5.7/run -> 2.5/run
direction A 7-10 -> 9-12 (same population: an unadopted clock, bare
Task.sleep in the timeout-message tests, cancelled bodies
unwinding through model-writing defers)
Job-stack attribution: the cancellation-resume family goes 160/256 -> 0/28.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`_ParkSource` covers streams SwiftModel produces. `node.forEach` over a THIRD-PARTY `AsyncSequence` (design §6b hook 1) and a clock that adopted `withModelParked` (hook 3) park through `_withCurrentWorkUnitParked`, and their deliveries are not ours to mark — the job-stack census after the first commit showed the residual concentrated exactly there (`ClockTests.testImmediateClock`, whose `ImmediateClock` timer resumes its consumer from `swift-clocks` internals, and `forEach` over test-owned raw `AsyncStream`s). But SwiftModel does own their **cancellation**: `Cancellations.cancelAll` -> `TaskCancellable.onCancel` -> `Task.cancel()` runs a cancellation handler synchronously, before the parked task is resumed. That is also the half that matters most — no activity signal behind it, and the resumed body goes straight into model-writing `defer`s, which is the premature-pass shape increment 2 fixed on the registry side and this fixes on the park side. So `_withCurrentWorkUnitParked` installs one, forcing the unit running from inside `Task.cancel()`. The residual left behind is a foreign source DELIVERING a value: inherent to hook 1's "park any AsyncSequence with no adoption at all", and §6b hook 2 (`parkedInModelTasks()`) is the opt-in that would close it. Named in the doc comment rather than papered over. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…Undeclared)
Three increments in, the work-unit running/parked state was accurate enough to
use but had never decided anything -- `idleNow` still owned every verdict. This
switches `_driveToStableFixpoint` to the combined rule from
`Docs/test-quiescence-redesign.md` SS3/SS6:
quiescent := oldSaysDone AND (newSaysDone OR runningWorkLooksUndeclared)
`oldSaysDone` stays a REQUIRED conjunct, and that is what makes the switch safe
on its own. The residual window the eager unpark could not close -- a foreign
source delivering a value we cannot observe (hook 1's known price) -- can only
make the semantic answer say "done" a moment early, and an early "done" is
exactly what a required old conjunct refuses to act on. So design SS6b hook 2
is not needed for this increment. The only verdicts that change are the ones
where the old answer passed and the new one says work is still running: we now
WAIT where we used to conclude.
`runningWorkLooksUndeclared` is the fallback, and the reason the design is
adoptable incrementally rather than all at once. Work SwiftModel cannot see
inside -- an unadopted clock's sleep, a bare `Task.sleep`, a compute loop -- is
correctly reported as *running*, and believing that would hang waits that
return today. The discriminator is structural, not a timeout: a unit that has
NEVER parked (so the framework has never been told where it suspends) and has
produced no transition for a settle grace. Only there do we defer to the old
answer, reproducing today's behaviour exactly.
The anti-swallow guarantee is the other half. A unit that has parked even once
is one whose suspensions we can see, so any later window in which it reads
running is a resumption, a yield hop or a starved job -- mid-flight, and the
correctness this whole effort exists for. `_hasEverParked` is never reset, so
such a unit can never look undeclared again however long it stays quiet, and
`allRunningLookUndeclared` is an ALL-quantifier so one mid-flight unit anywhere
in the subtree vetoes the fallback for the whole wait.
No new wall clock: the per-unit quiet threshold is `_settleGraceNs`, and the
keep-waiting path is bounded by the same `hangDeadlineNs` as every other path.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every time the combined rule concludes through `runningWorkLooksUndeclared`,
SwiftModel has just guessed rather than known: it deferred to the scheduler
because it could not see where a task suspends. That is correct behaviour, not
a failure -- but it is a place `withModelParked` adoption would help, and the
user-facing value is that the site can be *found*.
Three surfaces, none of them noisy:
* `settleDiagnostics()` gains an `undeclared async work` section listing the
sites, but only when the fallback actually fired in that test. A timeout is
the moment someone is reading this output, and an undeclared suspension is
the one thing about the test's timing the framework had to guess at. When
the fallback never fired the message is byte-for-byte what it was, so the
existing output snapshots are untouched.
* `SWIFT_MODEL_QUIESCENCE_TRACE=1` logs every occurrence with the running
units' model name, task name and call site.
* The exit summary gains `undeclaredFallbacks=N` plus a per-site adoption
worklist (`site: N x in M test(s)`), which is how the frequency across a
whole suite gets measured.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ine hook `withModelParked` existed but nothing used it, so every `clock.sleep` in the suite went through the undeclared-work fallback -- the path where quiescence is a guess. SwiftModel defines no clock (`node.continuousClock` is the user's own dependency) and the suite's clocks come from swift-clocks, so adoption here means wrapping a clock we do not own: `ParkedClock` forwards everything and wraps only its own `sleep` in `withModelParked`. That is the whole shape of the adoption story, and it is worth that the fixture is three lines: the adoption point is the clock IMPLEMENTATION, not its call sites. Injecting `ParkedClock(TestClock())` in `ClockTests` and `OnChangeTests` changes no model and no assertion, and moves `OnChangeCancelPreviousModel`'s `clock.sleep` off the fallback and onto the parked path -- measured: the per-run fallback count drops from 4 to 2, and the only sites left are two snapshot fixtures whose `Task.sleep(100s)` is deliberately an active task. `Docs/Testing.md` gains the corresponding section under Time control, making the point that `node.forEach` over any AsyncSequence (swift-async-algorithms included) already parks with no adoption at all, so a bare clock sleep is the one shape that needs the wrap. Also adds the combined rule's classification tests: that a never-parked unit needs a full quiet window before it looks undeclared, that a unit which has parked can NEVER look undeclared again (the anti-swallow guarantee, asserted with a zero-length quiet window and a far-future clock so it is structural rather than timing-dependent), and that one mid-flight unit vetoes the fallback for a whole subtree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Found by probing the rule rather than trusting it. `node.forEach` /
`node.onChange` run the user's per-element closure in the SAME work unit as the
loop that parks around `next()` -- the body is deliberately outside the park,
because running the closure is real model work. So `_hasEverParked`, read as a
property of the unit, says "we can see inside this" about a stretch we
demonstrably cannot:
node.forEach(stream) { value in
try await someUnadoptedClock.sleep(for: .seconds(1)) // invisible
}
Measured on that exact shape: `settle()` returns in 22 ms before this series and
ran to the trait cap after the combined rule landed. Nothing in this suite has
the shape, which is precisely why it needed probing -- a downstream consumer
with an unadopted clock inside a `forEach` body would have hit a hang, not a
diagnostic.
`_withUserBodyRegion` marks those stretches, and inside one the park history is
ignored so only the quiet window decides. This does not give back the guarantee
the fallback exists to protect, because the window it protects lies OUTSIDE the
region: the producer's eager unpark, the scheduling gap before the resumed
consumer returns from `next()`, and the return into the loop are all framework
stretches where `_hasEverParked` still applies in full. A starved resumption is
still waited for; only user code that has gone quiet for a whole grace inside
its own body may fall back -- exactly the set the framework has no claim to know
about.
Residual, stated rather than papered over: a HAND-WRITTEN `for await` loop.
SwiftModel owns the stream (so the wait parks) but not the loop, so there is no
body to mark and the unit's park history applies to its body too. Adoption --
`withModelParked` at the clock -- closes it, and the test suite now uses that
shape deliberately as its stable "running, parked before, arbitrarily quiet,
still mid-flight" fixture.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t assertion The three eager-unpark tests asserted `hasRunningWorkUnit == true` immediately after waking the consumer, which is unobservable: the eager unpark, the body and the re-park can all complete before any check, so they failed roughly 1 in 20. The yield path is now deterministic — the consumer body parks at an opt-in gate after each delivery, so "running" is a durable state. That also makes it a real test of the property rather than a race: without the eager unpark the unit would still read parked at that point, because the lazy defer cannot run while the body is held. The cancellation path has no equivalent hold (its resumed body only unwinds defers), so those two tests assert the durable consequence and the comment now says plainly that the eager unpark there rests on the job-stack measurement rather than on an assertion. An earlier version of that comment claimed a determinism that did not exist. 25 consecutive runs of the quiescence suite clean, from 1-2 failures per 20. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
425d58b to
448c637
Compare
|
Measured downstream and it is a large performance loss. Not merging this as it stands. Paired, interleaved, both arms prebuilt at the same base and differing by exactly the quiescence change (A = 1.0.18 @
4× to 9× slower. The box was not quiet, so the magnitudes are soft, but both pairs agree on direction and the effect is an order of magnitude beyond the noise the paired design controls for. It is also exactly consistent with what this PR says it does: the new rule waits more, in five places per suite run in this repo's own suite and evidently far more in a large downstream one, and the cheaper quiescence check does not come close to paying for that. The other motivation is also gone. The flakiness this was meant to address was the cross-tree dependency deadlock, fixed in 1.0.18 — so there is no flake rate left for a more truthful Staying open as a draft for the record, not as a queued change. What is worth keeping from it, independent of the rule:
If this is ever revived, the cost is the thing to attack first, and the suspect is the extra waiting rather than the accounting: each additional wait pays at least a grace window, and that is what a settle-heavy suite multiplies. |
|
Closing: measured 4–9× slower downstream and the flake it targeted was the cross-tree deadlock fixed in 1.0.18. The two epilogue-ordering windows found on the way ( |
Replaces the way
.modelTesting's wait verbs decide "is the model done?" — from observing the scheduler to asking SwiftModel's own registry of work. Design doc and its full reasoning are in #72; this is the implementation, built in four measured increments and landing behind a rule that cannot break anything that works today.Why
expect/settle/waitUntilanswered that question by counting outstanding executor jobs and checking queue idleness. That accounting is only as complete as our visibility into the Swift runtime, and the runtime exposes neither task state nor which executor a resumption travels through. So there is an open-ended set of ways work can be pending but invisible, and we have found and patched three separately: a task parked on aTestClock, a continuation hopping through the global executor after a yield, and a job sitting at background QoS. An open set cannot be closed by enumerating it, which is why this kept recurring.SwiftModel already registers every
node.task/forEach/onChangefrom creation to return. The drive just asked that registry the wrong question — "has it started" rather than "is it still running".The rule
oldSaysDoneis today's predicate, unchanged and still required. That means the one residual case where the new answer could be a moment early — a third-party sequence delivering a value we cannot observe — can never cause a premature pass.newSaysDone— no registered work unit is running.runningWorkLooksUndeclared— every running unit has never parked and has shown no transition for the grace window. That is a compute loop or a sleep on a clock we cannot see into; the wait defers to the old answer, so behaviour is exactly what it is today, and a diagnostic names the task and its call site so the adoption is discoverable.Net effect measured across a suite run: the drive waits in 5 places where it used to conclude, the fallback fires twice, and everything passes.
What parks, and what adoption costs
node.forEachparks around its ownnext(), so anyAsyncSequence— includingswift-async-algorithmsdebounce/throttle— parks with no user action. SwiftModel's own streams (Observed, events,observeModifications) park at the source, so a hand-writtenfor awaitbehaves identically toforEach. For a bareclock.sleep, the clock wraps its own suspension inwithModelParked { }— the adoption point is the clock implementation, not the call sites, which for a typical custom clock is a handful of edits.Docs/Testing.mddocuments it. Not adopting costs nothing: you get today's behaviour and a diagnostic.Two production bugs found on the way
catchhandlers ran — and those write model state.Cancellationsdropped entries before bodies unwound, so every teardown,task(id:)replacement andcancelPreviousswap looked quiescent while cancelled bodies were still running model-writingdefers.Corrections the design needed once it met real code
Recorded because each was found by probing rather than by argument: the running counter must start at registration, not body entry (which makes
hasPendingStartTaskfall out as a special case of the same rule); the park mark belongs to the input source, not toforEach's loop, since users write the loop by hand; the unpark must be done by the resumer, not the resumed task; the design's park-generation double-check was measured and is insufficient; and the eager unpark must sit at the raw source, because a filtered event resumes the consumer back into the same open park scope.The last commit is the one to scrutinise, because it is not in the design:
forEachruns the user's closure in the same unit as the loop that parked, so the rule as specified hung onnode.forEach(stream) { await unadoptedClock.sleep(...) }— 22 ms to the trait cap. Inside a user closure the park history is now ignored and only the quiet window decides. Every framework stretch keeps the full guarantee. Nothing in this suite has that shape, which is why it needed probing rather than testing.Gate
897 tests passing parallel and serial; 20 stress iterations clean; TSan per
ci.ymlclean with 896 tests and zero warnings; every suite log grepped for host crashes rather than trusting the summary line; release build warning-free; benchmarks flat (the ~3% hierarchy-mutation regression flagged mid-way was re-measured over 10 interleaved pairs and is noise). The quiescence suite itself went from 1–2 failures per 20 runs to 25 consecutive clean, after replacing three assertions of an unobservable transient state with a gated body that makes it durable.Still open, deliberately
The foreign-delivery window (a third-party sequence resuming a consumer through internals we do not hold) is ~4 checks per run and needs the opt-in sequence wrapper from the design's hook 2. It cannot cause an early pass while
oldSaysDoneremains a required conjunct, which is why it does not block this. Roughly 4 checks per run are executor jobs owning no work unit, down from 31; the remainder looks like registered-but-not-yet-enqueued task creation.🤖 Generated with Claude Code