Repository navigation
test(delete): the #1198 queue test reads the gate, not the accepted count - #6458
Conversation
…ount DeleteCommitQueuedBehindAMovingLaneTest failed in the whole-project run with "Expected 00:00:01.8188890 to be greater than 00:00:04". A pool counts a leaf as waiting from the moment it is accepted, one ThreadPool hop before it reaches the gate, and the gate is first-come-first-served among leaves that reached it. The precondition CurrentlyWaiting >= 11 was therefore true while nine of twelve holders had no place in the queue; the delete's leaf was served after three. The load is now read as IoPoolAdmission.BehindCap, the queue is kept at depth until the delete's leaf has asked for its slot, and the budget is compared with the leaf's own wait as measured by the store adapter. The sibling test's clock starts where the leaf is admitted rather than at the request. Refs #1198 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The holder replacement loop still permits the target queue depth to decay under ThreadPool saturation.
2 open findings
What changed in this PR
Corrects recursive-delete queue tests to measure actual gate ordering and per-leaf wait time.
Changes:
- Instruments delete-lane admission timing.
- Revises moving/stuck lane tests.
- Documents the queue-order finding.
| File | Description |
|---|---|
DeleteDrainCompletionTest.cs |
Adds lane timing instrumentation. |
DeleteCommitQueuedBehindAMovingLaneTest.cs |
Revises queue-depth and timeout assertions. |
RecursiveDeleteDrain.md |
Documents accepted-versus-gated behavior. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…cements reached the gate A replacement holder is only accepted until it has had a ThreadPool thread, so keeping the accepted count at depth did not guarantee who was ahead of the delete's leaf. The instant the leaf asks - before the pool has heard of it - the test counts the holders at the gate and stops the lane draining; it lets the lane move again once nothing accepted is still before the gate, i.e. the leaf has joined the queue behind exactly the holders counted. The architecture page no longer quotes a test count. Refs #1198 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results 6 files 6 suites 18m 2s ⏱️ Results for commit 8b5f5b1. ♻️ This comment has been updated with latest results. |
The doc sentence and the test's class comment still described twelve writers and about 7 s of queue. The test guarantees at least QueuedAhead holders at the gate when the leaf asks (5.4 s against the 4 s budget); say that. Refs #1198 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| .Subscribe( | ||
| _ => { }, | ||
| _ => holders.Remove(holder), | ||
| () => |
There was a problem hiding this comment.
Not changing this one. A holder is Observable.Timer(...) behind the lane; the only fault it can raise is the pool refusing or cancelling admission, which in this test means the mesh is being torn down. If that ever happened mid-test the failure is not misleading: the precondition prints the pool's own reading (LOADED: ... in flight, ... waiting ... before the gate, behind the cap) before it asserts, so a lane that lost its holders shows as exactly that. A second signal path for a fault that has no producer in this test would be more machinery than the case warrants.
| var askedAt = Stopwatch.GetTimestamp(); | ||
| Interlocked.Increment(ref _laneAsked); | ||
| var admitted = lane.InvokeObservable(ct => | ||
| { | ||
| var grantedAt = Stopwatch.GetTimestamp(); | ||
| Interlocked.CompareExchange(ref _laneAdmittedAt, grantedAt, 0); | ||
| var waited = Stopwatch.GetElapsedTime(askedAt, grantedAt).Ticks; | ||
| long seen; | ||
| do | ||
| { | ||
| seen = Interlocked.Read(ref _longestLaneWaitTicks); | ||
| } | ||
| while (waited > seen | ||
| && Interlocked.CompareExchange(ref _longestLaneWaitTicks, waited, seen) != seen); | ||
| return served; | ||
| }); | ||
| return Observable.Create<T>(observer => | ||
| { | ||
| // Before the pool has heard of this leaf: whatever is at the gate now is AHEAD of it. | ||
| OnLaneAsking?.Invoke(); | ||
| var subscription = admitted.Subscribe(observer); | ||
| // The pool has accepted the leaf (it counts as waiting), and it may not have reached the gate yet. | ||
| OnLaneAccepted?.Invoke(); | ||
| return subscription; | ||
| }); |
There was a problem hiding this comment.
The gap is narrower than it reads. WithLane is only called from inside Slow's Observable.Defer, so it runs at subscription, and Defer subscribes the observable it returns synchronously on the same thread: the count, the timestamp, OnLaneAsking and admitted.Subscribe are consecutive statements with no scheduler hop between them.
For that window to matter, the asking thread would have to be descheduled between two adjacent statements for several lane rotations: the lane holds about fifteen holders at the gate and the assertion needs nine, so roughly 3.6 s of preemption at that exact point, against a 600 ms rotation. The earlier failure was a different thing, a ThreadPool hop (work queued behind other work), not a running thread losing its core for seconds. And if it did happen, the test fails on aheadAtAsk >= QueuedAhead with the measured count, not silently.
Moving the three statements inside Observable.Create would be tidier and I agree with the direction, but it does not change what the test can observe, so I am not spending another review round on it.
| /// <summary>Completed when the leaf has reached the gate; pinned holders let go on it.</summary> | ||
| private readonly AsyncSubject<Unit> released = new(); |
There was a problem hiding this comment.
That pattern is for a worker that is PARKED, i.e. a thread the test deliberately blocks (a leaf ignoring its token, a wedged action block), where a failed assertion would leave a pool thread held. Nothing is parked here: a pinned holder is an unfinished observable (Timer(...).SelectMany(_ => released...)), it occupies the lane's slot and no thread, so polling a flag under SpinWait would introduce the blocked worker the rule exists to release.
Stranding is covered by ownership instead: every holder and the watch that completes released live in using var holders, so any exit from the test, a throwing assertion included, disposes them and frees the lane. released itself is the producer-completes-an-AsyncSubject<Unit> shape the same guard text prescribes (the watch is the producer), and HandWovenGateRatchetGuard's scan does not flag it.
Refs #1198
What failed
DeleteCommitQueuedBehindAMovingLaneTest.ADeleteQueuedBehindAMovingWriteLane_IsNotFailedForWaitingItsTurn(added by #6403) failed in four consecutive local Release runs of the wholeMeshWeaver.Graph.Testproject on macOS arm64, and passes alone and in CI:The delete SUCCEEDED, in 1.8 s. The watchdog was never involved, so this is a defect in the test, not in the queue credit.
Mechanism
A pool counts a leaf as waiting from the moment it is ACCEPTED, which is one ThreadPool hop before the leaf reaches the gate (
IoPoolAdmission.BeforeGate, #5057), and the gate is first-come-first-served among the leaves that have REACHED it. The test subscribed twelve 600 ms holders, waited forCurrentlyWaiting >= 11and started the delete. That precondition is true the instant the holders are accepted. With the ThreadPool busy, the delete's leaf reached the gate behind three of them and was served after 3 x 600 ms.Reproduced with no load, deterministically, by parking nine holder prologues before the gate (
IoPool.OnLeafPrologueStarting) in the ORIGINAL test:The change (test and doc only)
IoPoolAdmission.BehindCap >= 9(9 x 600 ms = 5.4 s ahead of the leaf against the 4 s budget).LatentDeleteStorageAdapterbetween asking the lane and being granted it, not with a wall clock started at the request.ALeafAdmittedAndHungInItsLane_...) had the same two shapes: its precondition read the accepted count, and its< 2 budgetsclock started at the request, charging the watchdog for the pre-commit stages and for the time before the leaf was admitted (during which the unrelated lane may rightly credit it). Its clock now runs from the leaf's admission to the failure's arrival. It was not observed failing.RecursiveDeleteDrain.mdrecords the finding.No product code changes. The credit's rule is unaffected by the before-gate window: a lane whose work has not reached the gate reads as busy-and-not-advancing, which denies credit.
Verification
11 waiting ... before the gate 9, behind the cap 2,Expected 2 to be greater than or equal to 9QueueWaitCreditdisabled in the product (negative control on what the test discriminates)[DeleteNode:commit] ... made no progress for 4s - 0 of 1 planned path(s) removed; loaded readingbefore the gate 0, behind the cap 11FullyQualifiedName~DeleteCommit(4 tests)MeshWeaver.Graph.Test, Release, one process (the condition it failed under)dotnet build -c Release -warnaserror:MeshWeaver.Graph.TestandMeshWeaver.Documentation, 0 warnings, 0 errors each.Not established: why CI's runners never lost this race (not investigated; the precondition was vacuous there too).
Pairs-with: none — test and documentation only; no public surface changes.
🤖 Generated with Claude Code