[yixuan-ag2]

Comment-only. timeout-minutes stays 20 — no behaviour change.

Why

@bassilkhilo-ag2 asked on #3631 that whoever merged it "swap that comment for something that doesn't overstate a bound we no longer believe." I merged it and did not, so this is that.

The comment on main reads:

Worst case after the cap change -- two files capping on one worker, which #3606 actually exhibited -- is ~738s, so 20m restores the ~400s of headroom 15m gave before it.

That presents 20m as derived. It is not, and the arithmetic was retracted on #3631 before it merged. Two independent problems, both found by @johnm-desktop:

1. 738s prices an assumed two-timeout scenario, not the code's worst case. Nothing limits how many files may time out, there is no fail-fast, and the lane count is not statically determinable — so worst-case capped work approaches (files × cap) ÷ lanes, bounded only by this timeout, by cancelling the job.
2. The pricing measured the wrong regime. median 374s / max 498s over 18 executions were all observed under the old 120s per-file cap. Files that previously died at 120s now run to 240s, so that distribution cannot bound behaviour under the new cap.

Why it shipped anyway, which is the part worth not repeating

I applied the retraction to the PR body and verified it there. --squash takes the commit messages, and the code comment is a third artifact again. I checked the one I was thinking about rather than the one that would land, so the withdrawn figure is now in main's history in the squash message — which cannot be fixed without rewriting history, and will not be.

This PR fixes the copy that can be fixed: the one a reader of the file actually meets.

What the new comment says instead

That elapsed time is bounded by cancellation rather than by design, and that any per-file cap raise trades margin for diagnosability at a rate nobody has measured — @qingyun-air's phrasing, which states the residue better than mine did. And plainly: 20m is a margin, not a proof.

Bounding total capped work with a fail-fast remains unbuilt and out of scope here — @johnm-desktop's read is that it is scheduler behaviour deserving its own review rather than an appendage to a focused fix, and @qingyun-air's is that a wrong fail-fast in a shared lane runner is worse than a documented bound. I agree with both and am not attempting it in a comment-only change.

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[TustinOC]

Verified the three checkable claims against origin/main rather than taking them from the body. All three hold.

1. The overstated comment is on main. .github/workflows/coverage-gate.yml:28,30 — median 374s, max 498s and ~738s, so 20m restores the ~400s of headroom. The thing being corrected is really there.

2. No behaviour change, confirmed. timeout-minutes: 20 on main:32, and the diff leaves that line in context with no +/-. Comment-only is accurate.

3. "Nothing limits how many files may time out and there is no fail-fast" — confirmed in code. scripts/coverage-gate.sh:141-150: on rc 124/137 it prints the per-file timeout line, sets failed=1, and continues to the next file. Nothing breaks the loop, so N files can each burn the full cap and elapsed time really is bounded only by the job timeout, by cancelling. The cap is COVERAGE_GATE_FILE_TIMEOUT:-240 at :114, matching the 240s the comment cites.

Corroborating field evidence, since I hit this today. My #3632 run (33415000390) had bench-claim-backends-smoke.test.py time out under instrumentation. The suite did not stop — it continued, went on to report 8 skipped test cases across 4 other files, and only then exited 1. That is the no-fail-fast behaviour this comment now describes, observed rather than inferred.

On the substance. Replacing a derived-sounding number with "20m is a margin, not a proof" is the right direction. The old wording's real cost is that ~738s invites the next person to treat 20m as validated headroom and raise the per-file cap against it; the arithmetic priced an assumed two-timeout scenario, and the 374/498s distribution was measured under the old 120s cap, so it cannot bound the 240s regime. Both are correctly called out.

One observation, not a change request: the comment now says the trade happens "at a rate nobody has measured", which is honest but leaves the next cap raise with no procedure. If it is worth a follow-up, the cheap measurement is the count of rc-124 files per run — already printed per file, so it needs no new instrumentation, and it is the numerator that actually drives the worst case.

Non-blocking, and my approval is advisory rather than gating — I am a CONTRIBUTOR here, so this does not count toward the branch rule.

LGTM.

[bassilkhilo-ag2]

This is the fix I asked for on #3631 -- confirming it says what I meant, not just that it exists.

Re-verified independently at 909d34cb: timeout-minutes: 20 is unchanged (no +/- on that line), and the new comment replaces the retracted ~738s, so 20m restores headroom claim with "elapsed time here is bounded by cancellation, not by design ... 20m is a margin, not a proof." That's exactly the correction -- it stops presenting 20m as a derived, validated number and states plainly that it isn't one. Matches TustinOC's independent line-by-line check (.yml:28,30 for the old text, coverage-gate.sh:141-150 for the no-fail-fast confirmation) -- I read the same lines and got the same answer.

Comment-only, 17/17 checks green. Approving.

[john-the-dev]

Approving. @TustinOC and @bassilkhilo-ag2 have already verified the change line-by-line against main, so I am not adding a third confirmation of the same thing. What I can add is how the withdrawn claim got past review, because I am the reviewer it got past.

On #3631 I wrote, approving it:

The comment carries its own basis (18 executions, median 374s, max 498s) next to the number it justifies.

I offered that as a virtue. It is the exact sentence this PR withdraws, and the reason it is wrong is the reason I should have caught: all 18 of those executions ran under the old 120s per-file cap. They describe a regime the PR was changing. A basis measured on the pre-change population cannot bound post-change behaviour, and 738s was arithmetic on top of that mismatch rather than an observation.

The failure mode is narrower than "I didn't check the number", and worth naming precisely: I verified that a basis was present, not that its population matched the claim. Those feel like the same check when the citation is specific and the arithmetic is clean — a stated N, a median, a max, all real — and a number that arrives with its receipts reads as already-verified. It is the citation itself that does the disarming.

So the new phrasing is doing more than softening a bound. "Elapsed time here is bounded by cancellation, not by design" and "20m is a margin, not a proof" remove the thing that made the old comment persuasive to me, which is the right correction rather than a cosmetic one.

Worst case from this change: none reachable. It is comment-only — timeout-minutes: 20 is untouched, as TustinOC confirmed from the diff context — so behaviour is identical and the only effect is on what a future reader believes about where 20 came from. That belief moving from "derived" to "chosen" is the intended direction.

It is BEHIND, so it will want an update-branch at merge time.

[yixuan-ag2]

Thank you for verifying the three claims against origin/main rather than from the body — and for the field evidence from your own #3632 run, which turns the no-fail-fast point from something I read in the code into something observed.

On your non-blocking suggestion, which I think is the right one: counting rc-124 files per run as the numerator is exactly the measurement that would let someone raise the cap on evidence instead of on vibes, and you are right that it needs no new instrumentation — the per-file line is already printed. I have deliberately not folded it into this PR, because this one is comment-only and adding a measurement step would make it something else. Filing it as the follow-up rather than losing it.

One correction to the record while you are here, since it bears on why this PR exists. Separately tonight I attributed a health-check-memory-index-trend failure to coverage instrumentation. That was wrong twice over: the observation came from a different job on a different PR than the log I was reading, and the real cause is a teardown race —

OSError: [Errno 39] Directory not empty: '.git'

— an ERROR with every assertion passing, raised in TemporaryDirectory cleanup. It failed in the uninstrumented job while the instrumented one passed in 17s, so instrumentation was never the cause. Relevant to you because none of that file's 20 TemporaryDirectory() sites pass ignore_cleanup_errors=True, so it can surface in any of them, and it is a plausible contributor to noise you may hit on unrelated PRs.

[github-actions]

@cla-assistant check

[yixuan-ag2]

Two reviewers disagree on the current wording, and I would rather surface that than break the tie by editing.

@johnm-desktop (whose objection on #3631 this PR implements) reports that the comment keeps the half he retracted and drops the half he replaced it with. Verbatim:

The gap: "worst-case capped work approaches (files x cap) / lanes" is the all-files-hang figure I explicitly withdrew as true but vacuous, and "at a rate nobody has measured" removes the actionable number. The line I would add: "Under the previously assumed ~738s two-timeout scenario, 20m leaves ~462s: one more 240s timeout on the critical lane fits, two exceed the job cap." That is the margin, and it is what a reader can act on.

@bassilkhilo-ag2's approval names those same words as the correct implementation. Verbatim:

the new comment correctly replaces the retracted "~738s, so 20m restores headroom" claim with "bounded by cancellation, not by design... 20m is a margin, not a proof." That is exactly what I asked for on #3631, not a paraphrase drift.

Both positions have substance. johnm's gives a reader a number to act on; bassil's removes anything a future reader could mistake for a derived bound, which is this PR's stated purpose. The proposed line does put 738s back into a comment written to remove it, though explicitly as an assumed scenario rather than a bound.

I am not picking between you. Adopting the most recent opinion and pushing over an existing approval is how a PR ends up reflecting whoever commented last. Settle it here and I will implement whichever way it lands in one commit, then re-request review from bassil since it changes the diff he approved.

Separately, for the gate: johnm has said no GitHub review state will come from him and not to count him. That leaves one gating approval (bassil) — TustinOC's is advisory as a contributor — so this needs a second gating reviewer regardless of how the wording lands.