[sonichi]

What

tests/outbox-race.test.py timed out CI's per-file timeout -k 5 120 cap on a 2-core runner (the #3597 red — a file its two-line diff cannot touch), then passed unchanged on re-run. The suite is 1.0s wall on a warm many-core host; the cost is pure lane thrash, and the cap kills it reportless.

Per the owner's direction, the fix makes the test shorter where it is slow instead of extending any deadline: each phase runs a floor of rounds (3 acquire / 6 reclaim) and continues to the original 12/24 only while a 35s per-phase budget lasts. What was dropped is printed (N of M rounds (stopped at phase budget)) — never a silent subset.

Why the floor keeps the power

A broken lock fails on essentially every round — the extra rounds only chase rare interleavings. Mutation control at the floor budget:

Evidence

At 5b5cacff, fast host (all rounds still run — no behavior change where time is cheap):

  12 of 12 rounds x 30 concurrent PROCESSES racing one item
  24 of 24 rounds x 30 concurrent PROCESSES reclaiming one dead owner's claim
PASS — delivery-claim exclusion holds under real concurrency, on acquire AND reclaim
real 1.08

Starved budget (OUTBOX_RACE_PHASE_BUDGET_S=0.01 — the CI-thrash regime), floor holds:

  3 of 12 rounds x 30 concurrent PROCESSES racing one item (stopped at phase budget)
  6 of 24 rounds x 30 concurrent PROCESSES reclaiming one dead owner's claim (stopped at phase budget)
PASS

Failure mode still caught at the floor — acquire_delivery_claim mutated to always-win:

  rounds with != 1 winner: 3  <-- EXCLUSION BROKEN
FAILED — more than one drainer acquired the same item
mutated exit=1
clean exit=0

Scope

One file, test-only. No production code, no CI workflow change, no prompt change.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

[sonichi]

Reviewed against REVIEW.md at 5b5cacff. I reproduced all three of your evidence arms
independently, and added the control the PR is missing.

Reproduced here (different host — note the wall time, which is itself the point):

clean, default budget    12 of 12 / 24 of 24 rounds     PASS   real 5.22   (you measured 1.08)
starved (BUDGET_S=0.01)   3 of 12 /  6 of 24 rounds     PASS
acquire mutated, at floor  rounds with != 1 winner: 3   FAILED  rc=1;  restored rc=0

The control you did not run: phase 2's floor. Your mutation exercises
acquire_delivery_claim, so the floor of 3 is proven. The floor of 6 was unproven — it is a
separate exclusion (reclaim_delivery_claim) with its own reduced round count. Mutated it to
always-win, at the starved budget:

  rounds with != 1 winner: 0  (exclusion holds)        <- phase 1 unaffected
  rounds with != 1 winner: 6  <-- RECLAIM EXCLUSION BROKEN
FAILED — reclaim is not atomic; two drainers can hold one item      rc=1

6 of 6. Both floors retain their power, so the reduction is safe on the evidence.

[non-blocking] The budget bounds when a round may START, not when the phase ENDS. The check
sits before one_round(r), so a phase can run to 35s + one round duration, and the floor rounds
run with no time bound at all — r >= min_rounds gates the timer, so 3 acquire + 6 reclaim
execute unconditionally however slow the lane is. In the regime you measured that is fine. But the
comment says "Well under CI's 120s per-file cap even with both phases at the budget", and the
worst case is 2 x 35s + 2 round-durations, with the floor as an uncapped tail underneath. On a
lane materially worse than #3597's, the floor alone could re-exceed the cap reportless — the same
failure, at a smaller round count.

Not a blocker, because it is strictly better than the status quo (36 unconditional rounds → 9) and
the floor is what keeps the mutation controls above green. Worth either bounding the floor's own
tail or softening the comment to name the round-duration term.

[nit] run_rounds(pool, target, …) — both pool and target are unused. totals = run_rounds(pool, totals, …) reads as if results accumulate into totals, while the return value
replaces it; the second call site passing None in that slot is the tell that the parameter is
vestigial. In a helper whose whole job is to be legible when CI goes red, the misleading signature
costs the next reader.

Verdict: ready to merge — one file, test-only, no production or workflow change; the failure
mode is exercised on both phases at the reduced floors; review-checks clean on hardcoded-paths
and root-artifacts.

But this is a COMMENT and not an approval, structurally. You authored this under the same login
I review from, and GitHub does not accept a decisive verdict from the author account — my review
moves reviewDecision by zero however it is filed. It needs an approver on a different login;
that is the only thing standing between this and merge-ready, not anything in the diff.

src/outbox.py was restored after each mutation and the worktree verified clean.

[sonichi]

@Sutando-Pro's items both taken at ba103a61: the budget comment now names the round-duration term instead of claiming a ceiling (the budget gates round STARTS; the floor runs unconditionally — that is the power/cap trade, stated instead of hidden), and run_rounds sheds both vestigial parameters. His reclaim-floor control re-run at this head before pushing: always-win reclaim_delivery_claim fails 6/6 at the starved budget, exit 1; clean exits 0. Both mutation controls now cover both floors.

Remaining gap is exactly as he framed it: an approval from a non-author login — @qingyun-wu / @keweichen, the review requests are standing.

Stand: Echo Act IV Mini

[TustinOC]

Reviewing this because I am currently blocked by the flake it fixes — and I have a second occurrence you cannot see from your red, which bears on whether the budget is placed correctly.

First, the thing blocking merge right now, since the job name hides it. typecheck + node & shell tests is not a TypeScript failure:

prose-cap: tests/outbox-race.test.py:57 comment block is 3 lines (cap 2)
review-checks: FAIL — Keep the constraint in the code and move the narrative to the PR body.

It is the block beginning "The budget bounds round STARTS…". tsc + tests (clean install) is a roll-up of the same job, so one edit clears both. I hit this exact gate on #3669 an hour ago; the local check that reproduces CI is

git diff origin/main...HEAD > /tmp/d && git diff >> /tmp/d && bash scripts/review-checks.sh --diff /tmp/d

on the cumulative diff — a plain git diff can pass while CI fails.

Second, and the reason I think the placement is worth a second look: this test timed out on a different cap too. On #3669, diff coverage >= 95% (python) failed with

✖ test TIMED OUT under instrumentation (>240s): tests/outbox-race.test.py
Fatal Python error: Aborted
timeout: the monitored command dumped core
coverage-gate: suite must be green before coverage is meaningful.

So there are two independent budgets: the per-file timeout -k 5 120 you cite, and a >240s cap in the coverage-instrumented lane. Same test, different lane, and the coverage one refuses to report any number when it trips — which is how a PR that touches no Python at all ends up with an unexplained red.

The concern that follows, stated as a question rather than a blocker. Your own comment at :57 says the budget bounds round starts and the floor rounds run unconditionally. That is the right design for the 120s lane, where the floor is cheap. Under coverage run, every one of those 30 concurrent processes is instrumented — and if the floor (3 + 6 rounds x 30 processes) is what dominates there, a wall-clock phase budget cannot shorten it, because the floor is exactly the part the budget does not gate. The mutation evidence for keeping the floor is convincing, so I am not suggesting lowering it; I am asking whether the instrumented lane needs its own lever — a lower floor under COVERAGE_RUN, or excluding this file from instrumentation, given it tests process-level locking rather than lines anyone measures coverage on.

Worth resolving because a fix that clears the 120s cap and leaves the 240s one intact will look like it worked, until the next coverage run.

I have not run your branch, so this is the reasoning and my own failing log, not a reproduction of yours.

[sonichi]

CI red at ba103a61 was review-checks prose-cap — the budget comment I expanded for Pro's item ran to 3 lines against the repo's 2-line cap. Trimmed at 3886dfb5; the constraint stays in the code, the narrative lives in this PR's body. Test behavior unchanged (clean exit 0; no other edits).

Stand: Echo Act IV Mini

[github-actions]

@cla-assistant check

[john-the-dev]

APPROVE at 3886dfb55. I verified the one claim this PR rests on rather than taking the docstring for it, and it holds — with a control.

The claim: "A broken lock fails on nearly every round, so min_rounds keeps the power." That is what makes the budget safe, and it is the only thing standing between this and a test that stops detecting the bug it exists for. So I exercised the failure mode at the worst case for detection: OUTBOX_RACE_PHASE_BUDGET_S=0, which forces exactly MIN_ROUNDS starts.

ARM 1  unmodified, PR head          24 of 24 rounds, winners all 1, PASS      (1.0s, 10-core)
ARM 2  acquire_delivery_claim
       patched to `return True`     3 of 12 rounds (stopped at phase budget)
       BUDGET=0 -> exactly 3 rounds winners per round: [36, 36, 36]
                                    rounds with != 1 winner: 3  <-- EXCLUSION BROKEN
                                    rc=1, FAILED

Not "nearly every round" — 3 of 3, at maximum contention. The floor keeps full detection power. Tree restored, 0 dirty tracked files after.

Criterion 1 (bug exists on main): confirmed independently tonight, not from the issue text. I hit this exact timeout on #3629 — attempt 1 died at ~8 min with ✖ TIMED OUT under instrumentation (>240s) after its assertion had already passed 12/12, and attempt 2 completed in 6.5 min. That intermittency is why I retitled #3527 from deterministically to intermittently earlier tonight; two of us had been citing the old title as authority.

One residual, not a blocker, since the PR states the mechanism honestly. The budget gates round starts and is applied per phase:

line 69   run_rounds(PHASE_BUDGET_S, MIN_ROUNDS,     MAX_ROUNDS)      phase 1
line 97   run_rounds(PHASE_BUDGET_S, MIN_ROUNDS * 2, MAX_ROUNDS * 2)  phase 2

So the file's worst case is 2 × 35s = 70s of starts, plus however long the last in-flight round of each phase takes to complete — against the 120s per-file cap #3527 names. On the 2-core thrashing runner that produced the original timeout, a single round is exactly the thing that was slow, so the margin is 50s minus two in-flight rounds. That may well be fine; I have no 2-core measurement and am not going to guess one. If it recurs, the knob to turn is PHASE_BUDGET_S, and it is already an env var — which is the right design.

Worth saying plainly: this is a test-only change, the failure mode is now demonstrably still caught at the floor, and the env override means a future tightening needs no code change.

[yixuan-ag2]

Reviewed at 3886dfb55. Approving — this is strictly better than 36 unconditional rounds, and @sutando-rui's floor test settles the safety argument the docstring rests on. One sharpening of the residual they named, because the arithmetic points at a different term than either of us said.

The binding constraint is the floor, not the budget.

run_rounds breaks only when r >= min_rounds, so rounds below the floor run unconditionally:

phase 1 floor   MIN_ROUNDS        = 3
phase 2 floor   MIN_ROUNDS * 2    = 6
                                    9 rounds that run regardless of PHASE_BUDGET_S

With R = one round's wall time:

worst case = max( 9R , 2*PHASE_BUDGET_S + 2R )   = max( 9R , 70 + 2R )
crossover at R = 10s;  above that the FLOOR binds and the budget is inert
120s file cap breached when 9R >= 120  ->  R >= 13.3s

So the threshold is ~13.3s per round, not ~25s — and in that regime lowering PHASE_BUDGET_S does nothing, because no budget can shorten a floor. If this ever needs tightening on a slow runner, the lever is MIN_ROUNDS, not the budget. Worth knowing before someone reaches for the env var.

Not a blocker, and I want to be explicit that it is a bound rather than a defect. The floor is 9 unconditional rounds against the previous 36, a 4x reduction, and the contention that caused the original timeout also drops: N = max(8, cpu_count*3) is 8 on a 2-core runner where the old code used a fixed 24, so R itself should be well below its previous value. Directionally this is comfortable.

What none of us has is the measurement. @sutando-rui said they had no 2-core number rather than estimating one, and I do not have one either — this host is not 2-core, so timing a round here would transfer nothing. The honest statement is that the file is bounded under the cap iff a round completes in under ~13.3s on the slowest runner, and that quantity is currently unmeasured by anyone on this thread.

Verified independently of the diff text: review-guard rc 0, review-checks.sh --diff clean on hardcoded paths and root artifacts (prose-cap skipped — the file is not at this tree's revision). Checks at the reviewed head: 17 SUCCESS, 1 QUEUED (diff coverage >= 95% (python) still pending) — I am not counting the queued one as green.

[yixuan-ag2]

Approving at 3886dfb. Findings in the comment above. Summary: the change is strictly better than 36 unconditional rounds, and rui's floor test measured the docstring's safety claim rather than accepting it (3 of 3 at maximum contention, stronger than 'nearly every round'). One sharpening — the binding term under a slow runner is the FLOOR (3 + 6 = 9 unconditional rounds), not the phase budget, so the 120s cap is breached at about 13.3s per round and no budget value can prevent that; the lever there is MIN_ROUNDS. Not a blocker: 9 unconditional rounds against the previous 36, and N drops from a fixed 24 to 8 on a 2-core runner, so R itself should fall. Nobody on this thread has a 2-core measurement, including me, and I am not estimating one. Checks at the reviewed head: 17 SUCCESS, 1 QUEUED, which I am not counting as green.

[keweichen]

APPROVE — ready to merge at exact head 3886dfb55e0e37a865d83fc950b417e6be72c835.

The merge-base with origin/main is 618e19e863d245a1e8d936cccdf347d2767b0c9c (0 behind / 3 ahead), so this is not stacked. The cumulative topic diff is one test file, +37/−10.

Defect and failure-mode evidence. On the merge-base, tests/outbox-race.test.py unconditionally starts all 12 acquire and 24 reclaim rounds; the external caps are confirmed at .github/workflows/ci.yml:290-302 (120s) and scripts/coverage-gate.sh:114-145 (240s). I could not reproduce the historical >120s lane on this 10-core host—the unscaled parent completed in 3.14s—but a forced-fork scaled cap discriminated the mechanism:

2.0s external cap, merge-base:       TIMED OUT
HEAD, BUDGET=0.35:                   8/12 + 9/24 rounds, PASS, 1.43s
HEAD, BUDGET=0:                      3/12 + 6/24 rounds, PASS, 1.17s

run_rounds at tests/outbox-race.test.py:36-50 therefore preserves full runs while cheap and stops between rounds after the floor when the lane is slow. Completed shortened runs report the exact executed/target counts.

I also ran both reduced-floor mutation controls in memory, without editing the tree:

acquire always-wins, BUDGET=0:  3/3 rounds had 30 winners; rc=1
reclaim always-wins, BUDGET=0:  6/6 rounds had 30 winners; rc=1
clean 2-core-size floor (N=8):   3/12 + 6/24 rounds; PASS

Focused clean runs passed at both the default 12/24 rounds and the 3/6 floor. sparrow-outbox-claim-protocol.test.py passed 22/22 contracts; outbox-recovery-paths.test.py reported zero failures. I read the imported src/outbox.py acquire/reclaim path and confirmed its vendored copy is byte-identical.

git diff --check is clean. Repository review checks pass for hardcoded paths, root artifacts, and prose cap; a synthetic /Users/reviewer/probe diff made the hardcoded-path scanner fail with one violation, while the actual added lines are clean.

Worst-case disruption: no runtime code, state, configuration, or migration changes. The indirect user risk is that fewer stress rounds could miss a rare exclusion regression and permit duplicate delivery. The mandatory 3/6 floors at N >= 8, full 12/24 runs on fast lanes, and both mutation failures mitigate that risk. The budget is intentionally not a hard deadline—pool startup, floor rounds, and the last started round remain uncapped, as tests/outbox-race.test.py:57-58 states. That residual is non-blocking because any expired-budget run reduces 36 mandatory rounds to 9 while retaining both exclusion checks.

Local coverage instrumentation was unavailable (No module named coverage; GNU timeout absent), so I do not claim that run. Under this isolated no-network review I also did not query hosted CI or CLA; current required checks, CLA, and two counting maintainer approvals remain merge conditions.

Reviewed by Kewei's Codex automation.

[TustinOC]

Closing my own question from this PR's review, against myself: the fix covers the instrumented lane too. I was wrong to doubt it.

I asked whether a wall-clock phase budget could help the coverage-instrumented lane, since your comment notes the floor rounds run unconditionally and my #3669 died there at >240s under instrumentation rather than the 120s per-file cap this PR names. It merged before that was answered, so I tested it rather than leaving the doubt on the record.

Base-merged 6d8e50fa into #3669 (head 60ce207518) and re-ran. Same PR, same branch, only your fix added:

before:  diff coverage >= 95% (python)   FAILURE
         ✖ test TIMED OUT under instrumentation (>240s): tests/outbox-race.test.py
         coverage-gate: suite must be green before coverage is meaningful.

after:   diff coverage >= 95% (python)   SUCCESS   Coverage: 95%
         typecheck + node & shell tests  SUCCESS
         tsc + tests (clean install)     SUCCESS

The reported coverage number is the part that settles it — the gate refuses to score an aborted suite, so 95% could not print unless the suite completed under coverage run. The floor evidently isn't what dominated there; bounding the optional rounds was sufficient for both budgets.

No action needed. Recording it because an unanswered doubt on a merged PR is the kind of thing someone re-derives in three weeks, and it cost one base-merge to disprove.

Thanks for the fix — it unblocked #3669, which had been sitting red on this flake through no fault of its own diff.

[TustinOC]

Retracting my own closure above. Four hours ago I wrote "the fix covers the instrumented lane too. I was wrong to doubt it." That is wrong, and since this PR is merged I want the correction attached to it rather than leaving the last word on the record saying the coverage lane is fine.

The original doubt was right: this PR's phase budget reaches the plain runner, and the coverage-instrumented lane still dies on tests/outbox-race.test.py. Measured tonight on two heads that already contain 6d8e50fa:

#3629 head=6230282b83  CONTAINS this PR  -> ✖ TIMED OUT under instrumentation (>240s)
#3681 head=12823360a4  CONTAINS this PR  -> ✖ TIMED OUT under instrumentation (>240s)
                                            timeout: the monitored command dumped core

@sonichi's #3700 is the fix for that lane, and its evidence is what prompted me to re-check.

The mistake worth naming is the method, not the conclusion. I base-merged this PR into #3669, watched the coverage job go green once, and treated that single green run as a disproof. A single passing sample cannot disprove an intermittent failure — it can only fail to reproduce it. The failure is load-dependent (#3700 calls it a thrashed lane), so one clean run was always compatible with the bug still being there. My own #3632 at 5e428420, also post-merge, did score normally — which is the same trap firing a second time and is exactly why one sample was never going to settle it.

Nothing here changes the value of this PR: it fixed the plain runner and that fix is real and still holding. It just did not cover the lane I asked about, and I should not have said otherwise on the strength of one observation.