Repository navigation
[test] TestCancelledRunMakesNoFurtherRequest fails under the repeated run #295
Description
Context
make test-repeat (the -count=2 step of the Build + Lint + Test job) failed on PR #294's first
run with:
--- FAIL: TestCancelledRunMakesNoFurtherRequest (0.00s)
retry_http_test.go:266: OnRetry fired 1 times, want 0: a cancelled run has no next attempt to announce
The PR changes nothing in agent/retry*; go test ./agent -run TestCancelledRunMakesNoFurtherRequest -count=20 passes on this machine, and no master run in the last twelve shows the same failure. It is
a race that a loaded runner can win, not a regression.
Why it can fire
The test cancels the context after the server has answered 503, then asserts three things: the error
is either the cancellation or the 503 (a race the test already documents, issue #193), requests == 1,
and retries == 0. The last one is stricter than the property the test is named for: if the cancel lands
after the retry decision but before the backoff's sleep observes it, OnRetry announces the attempt
that the sleep then abandons — and no second request is made, which is what "makes no further
request" means. retries is also a plain int written from the retry path and read in the test
goroutine, so the assertion has no ordering guarantee behind it.
What to build
Decide which is true and make the test say it:
- Announcing a cancelled retry is allowed — then assert
requests == 1plusretries <= 1, read
retriesundersync/atomic, and state in the comment that the announcement is a decision the cancel
can overtake, not a request; or - It is not allowed — then the retry path must check cancellation before announcing, and the test
stays as it is.
Acceptance
go test ./agent -run TestCancelledRunMakesNoFurtherRequest -count=50 -racepasses, and the counter
it reads is atomic.- The assertion names the property, not a proxy: "no second request" is checked directly; any leftover
count assertion says why it holds under both orderings. - The chosen reading is in the test's comment, so the next flake report can tell whether the test or the
code is wrong.
Capture recipe
When it fails again, save the run and the ordering:
make test-repeat > /tmp/repeat.txt 2>&1 # the gate that caught it
go test ./agent -run TestCancelledRunMakesNoFurtherRequest -count=200 -race > /tmp/retry-race.txt 2>&1/tmp/repeat.txt's --- FAIL block names the assertion; /tmp/retry-race.txt either reproduces it or
shows the data race on retries. Acceptance for a recurrence is attribution to one of the two readings
above, with that ordering recorded — not "could not reproduce".
Where it was found
PR #294, run
40155562754, step Test repeated in one process.
Activity
yusiwen commentedon Oct 25, 2026
Found while landing #294 (run
40155562754, step
Test repeated in one process). The commit touchesagent/memory_local.goand the tool path fence, notretry*; a re-run ofthe same head passed, and
go test ./agent -run TestCancelledRunMakesNoFurtherRequest -count=20passeshere. Not reproduced — so this issue stays open until the ordering is attributed, per its own acceptance.
test(agent): make the cancelled-retry assertion match the property
yusiwen commentedon Oct 31, 2026
Reproduced, then fixed as this issue's first reading: announcing a retry that the cancel then abandons
is allowed, and the property is "no second request".
The second occurrence was master's post-merge run for
8d1ccb8(Test repeated in one process):OnRetry fired 1 times, want 0. The ordering it caught is the one described above — the cancel landedafter the retry decision,
OnRetryreported the attempt the backoff's sleep then abandoned, andrequestsstayed 1.#313, merged as
8a6b557:retriesis anint32read withatomic(the retry path and the test goroutine both touch it);retries <= 1, with the two orderings and the reason in the comment, so the next flakereport can tell whether the test or the code is wrong;
requests == 1keeps its stricter check — a run that actually retried still fails.Verified at
-count=50for the test,-count=3for the package (the gate that failed), and the racedetector across the repository. Master's next run (
41168560103) is green.One correction to the analysis above: the "forbid the announcement" reading would need a cancellation
check before announcing, which does not remove the race — it moves it one line earlier. The announcement
is the honest report of a decision the cancel then overtakes.