Skip to content

Pin the executor drive's drain queue to .userInitiated QoS: a job resumed from a background thread could starve for minutes and wedge settle() under a saturated parallel run - #70

Closed
mansbernhardt wants to merge 5 commits into
mainfrom
fix/drive-queue-qos
Closed

mansbernhardt wants to merge 5 commits into
mainfrom
fix/drive-queue-qos

Conversation

@mansbernhardt

Copy link
Copy Markdown
Collaborator

Root cause of the rare full-plan wedge the parallel-apple simulator session measured on both 1.0.16 and 1.0.17 (1 in ~30 full-plan runs, 1500 s absolute-ceiling reports across ~79 tests, never in isolation), and of the smooth 4×–250× slowdown of settle()-heavy tests under that plan (1.4 s isolated → 5–343 s).

Mechanism. The drive's _sharedDrainQueue was a process-wide GCD concurrent queue with no QoS. A queue without a QoS runs each block at the QoS of the submitting thread, and a drive job is submitted by whichever thread resumes the task. A resumption from a DispatchQueue.global(qos: .background) callback, a .background Task or a low-QoS test double therefore produced a background-QoS job. Under a saturated machine such a block can stay unscheduled for minutes; while pending it counts as outstanding, so the executor reports itself busy (the inactivity watchdog never fires), settle() cannot reach its fixpoint, and eventually the absolute ceiling fires with its "almost certainly a deadlock" wording — with no lock involved. It is load-dependent (smooth), full-plan-only, and predates every recent change, which is the profile that was measured. No lock cycle was ever present; every candidate lock order was audited and is correct.

Fix. The drain queue is created with qos: .userInitiated and every job is submitted with .enforceQoS at that level. Once scheduled, the runtime runs the job at its task's own priority, which is the intended behaviour; the submitter's QoS can no longer drag it down. This makes the drive's documented "non-starvable" contract true. No wait budget, deadline or timeout changed.

Evidence. DriveJobQoSTests parks a node.task on a continuation resumed from a background GCD thread and reads qos_class_self() in the resumed job. On main it reads 9 (QOS_CLASS_BACKGROUND); with the fix it reads the task's own priority (default, 21). A standalone GCD probe confirmed the propagation rule for unspecified-QoS versus pinned queues independently. The 79 collateral reports in the downstream hangs are explained by the global-quiescence fail-gate: one pending job holds every other unmet expect in the process to its ceiling (1,045 tests passed after the wedge started in the kept log).

Gate: scripts/test 869 passed (parallel and serial); regression test red on main, green here. 10× loop and TSan per ci.yml running on this commit and will be posted below. Darwin-only test (qos_class_self); the fix itself is plain GCD and compiles everywhere Dispatch does.

🤖 Generated with Claude Code

mansbernhardt and others added 2 commits September 7, 2026 09:19
…sumed from a background thread ran at background QoS and could starve for minutes, wedging settle() under a saturated parallel run

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…onfiguration

Forcing every drive job to .userInitiated changed how drive jobs are scheduled
against work the drive cannot see: Task.yield() inside a task that prefers the
drive resumes on the global pool, not on the drive (verified with a counting
executor: one enqueue for a six-yield task), and on CI's 3-core runners a
settle() fixpoint check then outran a yielding child it could not count
(ExecutorDrainSettleTests.settleIsLoadIndependentAcrossChildTasks). A queue
QoS without .enforceQoS raises low-QoS submissions to the floor and leaves
higher ones as they were, which is all the starvation fix needs.

The regression test now asserts the queue's configured QoS. The behavioural
version resumed a task from a background GCD block, which the TSan job's
saturated runner could not schedule inside the test budget.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mansbernhardt

Copy link
Copy Markdown
Collaborator Author

Second push: the queue QoS is now a floor (no .enforceQoS), and the regression test asserts the configuration instead of racing a background block.

Why: the first push failed CI's macOS parallel jobs on ExecutorDrainSettleTests.settleIsLoadIndependentAcrossChildTasks (repeatedly) and my own behavioural QoS test timed out under TSan. Two findings behind that:

  • Task.yield() inside a task that prefers the drive executor does not re-enqueue on it — a counting executor sees one enqueue for a six-yield task — so a yielding child is invisible to the fixpoint during its yields and covered only by the grace window. That hole predates this PR. Forcing every drive job to .userInitiated let settle's fixpoint checks outrun a starved yielding child on the 3-core runner. A floor raises low-QoS submissions (the starvation fix) and leaves higher-QoS submissions exactly as before, so that balance is untouched.
  • The behavioural test's own trigger was a DispatchQueue.global(qos: .background) block, which the saturated TSan runner did not schedule within the budget — the same starvation the PR fixes, in the test's trigger. It now pins the configured QoS; the before/after measurement (9 → task priority) is recorded in the changelog.

Locally neither failure reproduces even under a 10-thread userInteractive burner (15/15 on both commits), so CI is the arbiter. scripts/test 869 passed in both modes on this commit.

…e drain-settle shape (to be removed before merge)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… declaring quiescence

A yielded task is invisible to the executor's outstanding count while the
runtime hops its continuation through the global executor back to enqueue.
On CI's 3-core runner the DIAG arms measured that hop at 773 ms with the drive
idle: children's jobs ended at 0.7 ms, their yielded continuations ran at
774 ms, settle fired at 807 ms — inside the next hop. No grace window bounds
a starved runner. Settle's task now yields from inside the fixpoint check; it
queues behind every pending yielded child in the same global executor, so on
return those children are re-enqueued or have run, and quiescence is declared
only if the system is still idle and quiet — an ordering signal, not a clock.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mansbernhardt

Copy link
Copy Markdown
Collaborator Author

Third push (3677a8d): the drive's fixpoint now requires idleness to survive a Task.yield() round-trip of settle's own task before declaring quiescence.

What the two diagnostic arms showed (DriveQoSDiagnosticsTests on this branch vs the same test on main's queue in draft #71, both on the 3-core macOS runner):

  • Main's queue: children's jobs at QoS 21; each yielded continuation took ~1–11 s to come back; settle waited correctly (0/40 premature) only because its own resumptions were equally starved.
  • Floor queue (this PR): children's jobs at QoS 25, ~15× faster overall, but in the premature iteration all four children's jobs ended at 0.7 ms, their yielded continuations ran at 774 ms, and settle declared the fixpoint at 807 ms — inside the next hop. During that hop outstanding is 0: a Task.yield() re-enqueues on the drive (verified: 8 enqueues for 3 yields with a proper Task(executorPreference:)), but only after the runtime hops the continuation through the global executor, and on a starved runner that hop is hundreds of ms. No grace window bounds it.

The fix: when the drive looks idle and quiet, settle's task yields; it queues behind every pending yielded child in the same global executor, so on return those children are re-enqueued or have run. Quiescence is declared only if still idle and quiet after that round trip — an ordering signal rather than a wall-clock one, consistent with the no-timeout-hunting rule. The diagnostic test stays in for this CI run and will be removed before merge; #71 is closed.

Locally: 870 passed in both modes; 0/40 premature under load.

)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mansbernhardt

Copy link
Copy Markdown
Collaborator Author

All 7 jobs green on 3677a8d (macOS parallel and TSan included — the two that failed on the first two pushes). Fourth push removes the temporary DriveQoSDiagnosticsTests; no other change. Ready for review.

@mansbernhardt
mansbernhardt marked this pull request as draft September 7, 2026 11:58
@mansbernhardt

Copy link
Copy Markdown
Collaborator Author

Converted to draft — do not merge in this form. The parallel-apple after-arm (5 full-plan runs on 8347727 vs 4 on 1.0.17) shows: the settle-heavy onboardingSlice test inflates 2.3–9× (273–1056 s vs ≤116 s) while still passing — the yield round-trip adds one global-executor hop per settle(), seconds on a starved runner, so it is overhead rather than newly-waited work (the main-queue DIAG arm already showed settle waiting correctly there); slicePaths did not collapse; and the 1500 s wedge still occurred (1/5), so the QoS floor did not remove it — a job outstanding for 1500 s at ≥ userInitiated is running and blocked, not unscheduled. Net: the floor alone makes settle prematurely wrong under starvation, the round-trip makes it right but slow, and neither addresses the wedge. Needs a local reproduction of the starvation shape and a stack from the wedge before another attempt.

@mansbernhardt

Copy link
Copy Markdown
Collaborator Author

The evidence this PR was withdrawn on has been retracted, so its status is now open rather than rejected.

The downstream after-arm that reported this branch regressing a settle-heavy test 2.3–9× never built this branch. swift package edit inside the package directory does not affect what an xcodebuild -project build resolves — Xcode uses the xcodeproj's own Package.resolved and SourcePackages checkout, which was verified afterwards to be stock 1.0.17 in both arms. So both arms ran identical code and the difference was machine conditions.

What that changes, precisely:

  • The 2.3–9× regression claim is withdrawn. My own mechanism analysis (one hop through a starved global executor per settle()) may still be right, but it is now unmeasured rather than confirmed, and I should measure it here before treating it as a cost.
  • "The wedge survived the QoS floor" is withdrawn as evidence about the floor. That wedge was the cross-tree dependency deadlock fixed in 1.0.18, sampled from a stock 1.0.17 process. It says nothing about the floor.

What still stands on this branch's own evidence, independent of that arm:

  • The QoS finding is directly measured here: a drive job resumed from a background-QoS thread ran at QOS_CLASS_BACKGROUND (9) without the floor and at its task's priority with it.
  • The reason the yield round-trip exists is this repo's own CI, not the downstream arm: the floor-only pushes failed ExecutorDrainSettleTests on macOS parallel twice, and the round-trip made all seven jobs green. The floor alone provably causes a premature settle on a 3-core runner.

Staying in draft for now, because the semantic-quiescence work in progress replaces the fixpoint rule that both halves of this PR are working around — if that lands, the round-trip is unnecessary by construction and the floor can be judged on its own. Revisit once that is settled.

mansbernhardt added a commit that referenced this pull request Sep 9, 2026
…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>
@mansbernhardt

Copy link
Copy Markdown
Collaborator Author

Closing after measuring the round-trip's cost on 1.0.20 (the measurement the last comment said was owed): 12 interleaved paired full-suite --parallel runs, this branch rebased on 1.0.20 vs 1.0.20. Wall time indistinguishable (~25 s both), but summed per-test in-flight time +14 % median (473 → 527 s), +20 % mean, slower in 10/12 pairs, with a heavy tail (two runs at +40–90 %, no baseline run near them) — spread uniformly across every test, i.e. one global-executor hop per settle() under a saturated pool, which a downstream settle-heavy suite multiplies per settle. The floor alone is unsafe per this PR's own CI history, the wedge it chased was the 1.0.18 deadlock, and the QoS gap has no current victim. Finding + measurement + what to do if a victim appears recorded as Update 28 in Docs/test-determinism-executor-drain.md (#78). Branch kept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant