test: stop the stall-guard tests blaming the guard for working - #13
Conversation
TestStallGuard_TickKeepsAlive was flaky, and TestStallGuard_ConcurrentTicks had the same defect waiting. Both assert a negative: that the guard does not fire while Ticks keep arriving. That premise holds only while the ticking goroutine is actually scheduled inside the guard's window, and the windows were 30ms and 40ms. A loaded machine starves a ticker past 30ms easily, at which point the guard fires -- correctly -- and the test reports it as a bug in the guard. Under six concurrent suite runs this produced 3 failures in a batch of 25, on a tree with no other changes. Two changes, and the second is the one that matters. The windows scale with testwait.Factor. Not testwait.Timeout: this is a real-time window the guard itself measures, and Timeout's five-second floor would stretch a millisecond-scale test into a ten-second one. What the test needs from the scale is the ratio, because scheduling jitter is roughly constant in absolute terms, so a wider window makes a starved ticker proportionally rarer. But rarer is not correct, so the tests now verify their own premise instead of assuming it. Each records the widest gap between its own Ticks. A cancellation is a failure only when every Tick demonstrably landed inside the window; when the ticker was itself starved past it, the test says so and skips, because firing was the right thing to do. The test can now only fail when the guard actually misbehaves. Verified in both directions. With Tick stubbed out to do nothing, both tests fail and name the evidence: "cancelled although every Tick landed inside the 300ms window (widest gap 147ms)". Under a deliberately impossible 1.5ms window they skip rather than lie. And under six concurrent full-suite runs, 900 executions pass with no skips at all -- the widened window alone was enough, with the premise check as the net. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84ef12ec22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defer g.Stop() | ||
|
|
||
| // Tick several times at sub-timeout intervals so the guard never fires. | ||
| rec := newTickRecorder() |
There was a problem hiding this comment.
If the test goroutine is descheduled after NewStallGuard arms its timer but before this recorder is created, the guard can correctly expire while that initial starvation interval is never recorded. On resumption, last starts at the current time and subsequent ticks can all show sub-window gaps, so explainCancellation reports a failure instead of skipping—the exact load-induced false failure this change is intended to prevent. The concurrent test has the same ordering; initialize the recorder timestamp before arming the guard (or otherwise include the arm-to-first-tick interval).
Useful? React with 👍 / 👎.
1 participant
Fixes the
TestStallGuard_TickKeepsAliveflake found while verifying the shutdown-budget work, plus the same defect waiting inTestStallGuard_ConcurrentTicks.What was wrong
Both tests assert a negative: that the guard does not fire while Ticks keep arriving. That premise holds only while the ticking goroutine is actually scheduled inside the guard's window — and the windows were 30 ms and 40 ms.
A loaded machine starves a ticker past 30 ms easily. The guard then fires, correctly, and the test reports it as a bug in the guard. Reproduced at 3 failures in a batch of 25 under six concurrent suite runs, on a tree with no other changes.
Two changes, and the second is the one that matters
1. The windows scale with
testwait.Factor. Nottestwait.Timeout— this is a real-time window the guard itself measures, andTimeout's five-second floor would stretch a millisecond-scale test into a ten-second one. What the test needs from the scale is the ratio: scheduling jitter is roughly constant in absolute terms, so a wider window makes a starved ticker proportionally rarer.2. But rarer is not correct, so the tests now verify their own premise instead of assuming it. Each records the widest gap between its own Ticks. A cancellation is a failure only when every Tick demonstrably landed inside the window; when the ticker was itself starved past it, the test says so and skips — because firing was the right thing to do.
The test can now only fail when the guard actually misbehaves.
Verified in both directions
Tick()stubbed to do nothing (guard genuinely broken)cancelled although every Tick landed inside the 300ms window (widest gap 147ms)PACKETCODE_TEST_TIMEOUT_SCALE=0.05→ impossible 1.5 ms windowwidest gap between Ticks was 2ms, past the 1.5ms guard window, so cancelling was the correct behaviourAlso
-raceclean, and the concurrent test still callsTickoff-lock so the race detector still sees it hit from eight goroutines at once.Checks
go test ./...clean;go test -race -run TestStallGuardclean.golangci-lint(repo config, truncation disabled) on an LF checkout of this commit underGOOS=linux,darwin,windows: 0 issues each. (Linting the CRLF working tree directly is meaningless here — every file reports agofmtdiff.)🤖 Generated with Claude Code