fix: give the race lane a declared timeout that fits the merge queue - #718
Merged
Merged
Conversation
…ault The macOS race step ran internal/surface/cli into go test's implicit 10-minute per-package timeout and ejected PR #717 from the merge queue with no test hung; the job ceilings around the step are as tight. Refs: iss-2609260319483365 Assisted-by: Claude:claude-opus-5-5
The race lane ran internal/surface/cli under go test's implicit 10m per-package timeout. On the macOS runner that package measured 418s, 472s and 568s passing, and merge_group run 36212607621 crossed 600s with no test hung, ejecting an unrelated pull request from the queue. The lane now runs `go test -race -timeout 20m ./internal/...` in all three places that run it: ci.yml's check job, release.yml's verify job (and the scaffold template that renders it byte-for-byte), and make preflight, so local and CI judge the lane against one budget. 20m is about twice the slowest passing run; a real hang still fails, with a goroutine dump, instead of being masked. Lifting the package timeout alone would move the failure to the job ceiling, which cancels without a dump. The macOS check job already took 19.0, 21.4 and 25.9 minutes of its 30 on passing runs, spending 8.3 minutes before the race step and 9.8 inside it before cli started. So check rises to 45 minutes. release.yml's verify job (ubuntu, uncached toolchain) took 13.2 of its 15 on the last release, with cli starting 4.8 minutes into the step, and the ubuntu race step has grown about two minutes since; verify rises to 35. Managed repos rendered from the bare profile keep 15 and the default timeout: their race leg is not this package. TestRaceLaneBudgetIsDeclaredAndFitsItsJob holds the invariant: the three commands carry one explicit -timeout, and each job's timeout-minutes covers it plus its measured headroom. Considered and not taken: shortening the package. A local -race run of the package alone took 301s, 118s of it in one test, TestHistoryCaptureAcceptsWhatTheHooksAccept, whose 9 MiB fixture is the point of the test (it must exceed the old 8 MiB cap), so trimming it means an injectable cap in production code for a test's sake. That is a CI-cost question, and the direction for CI cost awaits a ruling under iss-2609012313465609; the declared budget is needed either way. zizmor 1.26.1 (the CI pin), --persona regular: no findings. Refs: iss-2609260319483365 Refs: iss-2609012313465609 Assisted-by: Claude:claude-opus-5-5
… timeout Resolves: iss-2609260319483365 Assisted-by: Claude:claude-opus-5-5
…guard reads The race step's new comment said "make preflight", which TestCIRunsTheSkipStepBeforeEachHarness and TestHarnessJobsRunOnHostedRunners read as ci.yml running the local gate (CI runs the gates directly). The comment names the Makefile's preflight recipe instead; no step changes. Refs: iss-2609260319483365 Assisted-by: Claude:claude-opus-5-5
f0994f7 raised ci.yml's check job from 30 to 45 minutes so the race step's -timeout 20m could fire before the job was cancelled. On the event the lane was ejected on, a merge-group run, that ceiling is unreachable: the main ruleset's merge_queue rule fails the group when a required check has not concluded within check_response_timeout_minutes, which is 30 (.abcd/work/rulesets/main-protection.json). The 45 bought nothing there and made the comment, the test and the record claim a budget that is not in force. The check job goes back to 30, equal to the queue's cap, as iss-2609020215079051 deliberately set it. The race step keeps -timeout 20m: it lifts go test's 10m per-package default, which is what ejected PR #717, so a slow package runs on to the job's ceiling instead of failing at ten minutes. On the macOS leg the slowest package starts about 18 minutes in, so a genuine hang there is cancelled at 30 without a goroutine dump; the comments say so. Raising the queue's cap is an admin act on the live ruleset, not taken here and left to the technical facilitator. release.yml's verify job is not a merge-queue job and keeps 35. TestRaceLaneBudgetIsDeclaredAndFitsItsJob now pins what is true: the three -timeout values are equal; ci.yml check's timeout-minutes is at most the mirror's merge_queue check_response_timeout_minutes; release verify's timeout-minutes covers the step timeout plus its measured 10m headroom. The check job's step-plus-20m assertion is dropped, which also retires the review nit on its "about 18m" against the test's 20m. Watched red: the queue-cap assertion against the unfixed ci.yml (45 > 30) in the worktree; on a scratch copy, verify lowered to 29, the mirror's cap lowered to 25, the merge_queue rule removed, and release's -timeout changed to 25m, each failing with its own message. Refs: iss-2609260319483365 Refs: iss-2609020215079051 Assisted-by: Claude:claude-opus-5-5
The resolution said the check job's ceiling rose to 45 minutes so the package timeout fires before the job is cancelled. 2f7b903 holds the check job at the merge queue's 30-minute response cap instead, so the text says what is in force: the step timeout lifts the 10m per-package default, the queue's cap is the outer ceiling on merge-group runs, raising it is an admin act left to the technical facilitator, and only release.yml's verify job leaves room for the package timeout to fire. The grounds no longer count a merge-group cancellation at 30 as a refutation. resolved_by stays f0994f7, the commit that lifted the default; the record has not left this branch. Refs: iss-2609260319483365 Assisted-by: Claude:claude-opus-5-5
Assisted-by: Claude:claude-opus-5-5
Merging main (39ed6fa) into this branch, together with the race-lane budget test this branch adds under internal/core/lint, grew the tree-bounded reading objects. Each window is re-measured by dry run over a clean clone of the merged tip 1dff258 against the rule the entries state, the smallest ten-thousand boundary leaving at least one per cent headroom. Widening measures 1,021,262, past its 1,020,000 declaration, so it moves to 1,040,000 (1.83 per cent); entailment measures 343,062, 2.02 per cent under 350,000, so its declaration stays and only its measured fields move; detection measures 1,030,298, past its 1,030,000 declaration, so it moves to 1,050,000 (1.91 per cent). Comparative is bounded by the widening run it is handed, not by the tree, and is exempt by name. Refs: iss-2609251455354719 Assisted-by: Claude:claude-opus-5-5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With this change, the race-enabled test lane runs under a declared 20-minute timeout instead of go test's default ten minutes, and the release verify job has room for that timeout to fire first.
internal/surface/cliin 418 to 568 seconds on green runs, at the edge of the default 600-second package timeout. A slow runner crossed it and ejected an unrelated pull request from the merge queue (test timed out after 10m0swith no test running).go test -race -timeout 20m ./internal/...is the command in four places: CI's check job, the release workflow's verify job, this repository's branch of the release scaffold template, and the Makefile's preflight recipe.TestRaceLaneBudgetIsDeclaredAndFitsItsJobholds the three timeouts equal, the check job at or below the queue cap in the committed ruleset mirror, and the release verify job above the step timeout plus measured headroom.Review: Fable 5.1, two rounds. Round 1 found the 45-minute check ceiling unreachable under the queue's 30-minute cap; round 2 holds the job to the cap and verified. Reading-corpus change: the new test grew the widening and detection objects, so their windows are recalibrated at the merged tip (widening 1,040,000; detection 1,050,000). Lands before the v0.11.0 cut, on the cutting session's ruling, because the release pull request itself must pass this lane in the merge queue.
Resolves: iss-2609260319483365
Refs: iss-2609012313465609
Refs: iss-2609251455354719
Assisted-by: Claude:claude-opus-5-5