fix(config): stop billing provider-command fixtures to the 5s timeout - #810
Conversation
Both background-child tests fail intermittently on the Windows runner, and have since #690. #800 and #802 each relaxed an assertion, which is why neither held: the deadline itself was the problem. The tests drove LoadProviderCommand, so the fixture's own startup was billed to the production 5s budget. On a loaded runner the outer timer won the select in runProviderCommand, and both symptoms follow from that. The nonzero-exit test saw a timeout instead of the exit status it asserts, because the timer preempted the shell's exit. The other test lost its PID file, because the timer's Terminate killed the background child before it could record a PID, putting the file permanently out of reach however long the assertion polled. Neither test is a claim about the 5s budget. They assert which error each cmd.Wait path yields and that the leftover descendant is killed, so they now call runProviderCommand directly with a budget of their own and the production deadline stops competing with process startup. Making the child outlive that budget lets both drop their elapsed-versus-lifetime bounds as well, so no wall-clock assertion is left in either test. That is only worth doing if the two timeout paths can be told apart, and they could not: the WaitDelay path and the outer-timer path both returned the same bare sentinel, so asserting on it would have been satisfied by the very timeout the tests exist to rule out. The WaitDelay path now wraps rather than replaces. Callers asking errors.Is for the sentinel are unaffected, so no user-visible message changes. The Windows fixture also waited for its ready marker by launching a second PowerShell. That cold start is pure overhead added after the PID is already written, so the wait now runs in cmd itself and is bounded. Verified by looping both tests under CPU contention: 7 failures in 16 iterations before, none in 32 after, with per-iteration time going from a spread of 2.3s to 66s down to a steady 2.1s. Removing the wrap makes the WaitDelay assertion fail, so it is load-bearing rather than decorative.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughProvider command timeout handling now wraps ChangesProvider command timeout handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/config/command_test.go (1)
95-143: 🩺 Stability & Availability | 🔵 TrivialSound fix for the startup-vs-timeout coupling.
Switching to
runProviderCommand(command, providerCommandTestBudget)with a budget decoupled fromproviderCommandTimeoutcorrectly stops fixture cold-start from competing with the production 5s deadline, and assertingerrors.Is(err, errProviderCommandTimeout) && errors.Is(err, exec.ErrWaitDelay)(line 139) precisely targets the WaitDelay path rather than the outer-timer path, matching the wrapping added incommand.go.Since this test directly exercises the
done/timerselectand thecmd.Wait()goroutine inrunProviderCommand, worth running it (and its sibling below) with-racegiven they're new coverage over that concurrent path.As per coding guidelines,
**/*_test.go: "add regression tests for behavior changes, and run affected concurrent code under the race detector."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/command_test.go` around lines 95 - 143, Run TestLoadProviderCommandTerminatesBackgroundChild and its sibling concurrent command test with the race detector, using the test-specific providerCommandTestBudget path. Fix any race reports in runProviderCommand or related test helpers while preserving the assertions for WaitDelay error wrapping and background-child termination.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/config/command_test.go`:
- Around line 95-143: Run TestLoadProviderCommandTerminatesBackgroundChild and
its sibling concurrent command test with the race detector, using the
test-specific providerCommandTestBudget path. Fix any race reports in
runProviderCommand or related test helpers while preserving the assertions for
WaitDelay error wrapping and background-child termination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 138e26aa-afc9-4885-aa94-426aa7427f63
📒 Files selected for processing (2)
internal/config/command.gointernal/config/command_test.go
2 participants
Both background-child tests fail intermittently on the Windows smoke job, and have since #690. #800 and #802 each relaxed an assertion; neither held, because the deadline itself was the problem.
Why they flake
The tests drove
LoadProviderCommand, so the fixture's own process startup was billed to the production 5s budget. On a loaded runner the outer timer wins the select inrunProviderCommand, and both symptoms fall out of that:...OnFailureseesprovider command timed out after 5sinstead of the exit status it asserts, because the timer preempts the shell'sexit /b 7....TerminatesBackgroundChildloses its PID file, because the timer'sTerminatekills the background child before it records a PID. test(config): wait for background PID file #802 added a 3s poll for that file, but the writer is already dead, so no amount of polling helps.Reproduced locally by looping both under CPU contention, including the exact CI messages:
What this does
Neither test is a claim about the 5s budget; they assert which error each
cmd.Waitpath yields and that the leftover descendant is killed. They now callrunProviderCommanddirectly with a budget of their own, so the production deadline stops competing with process startup. Making the child outlive that budget lets both drop their elapsed-versus-lifetime bounds too, so no wall-clock assertion is left in either test.That is only worth doing if the two timeout paths can be told apart, and they could not: the
WaitDelaypath and the outer-timer path both returned the same bare sentinel, so asserting on it would have been satisfied by the very timeout these tests exist to rule out. Hence the one product line here, wrapping instead of replacing on theWaitDelaypath. Callers askingerrors.Isfor the sentinel are unaffected,runProviderCommandhas exactly one non-test caller, and no user-visible message changes.The Windows fixture also waited for its ready marker by launching a second PowerShell. That cold start is pure overhead added after the PID is written, so the wait now runs in cmd itself and is bounded.
Checks
gofmt,go vet,go build ./...clean.Under the CPU contention that reproduced the failures: 7 failures in 16 iterations before, 0 in 32 after, with per-iteration time going from a spread of 2.3s to 66s down to a steady 2.1s.
The new
ErrWaitDelayassertion is load-bearing rather than decorative: dropping the wrap makes it fail.Separately: the timeout is not actually a bound
Not fixed here, and I will open an issue rather than widen this PR. While reproducing the above I saw
LoadProviderCommandtake 19.7s and later 106s against a 5s timeout. Two causes, both incommand.go:startCommandProcessruns before the timer is armed, so process creation is unbounded, and<-doneafterTerminate()on the timer path is unbounded too. The magnitudes came from synthetic load, but the ordering is wrong regardless of load. A user whose provider command is slow on a cold or AV-heavy Windows box can block startup well past the 5s the error text promises, and gets no diagnostics, since the timeout path discards stderr.Summary by CodeRabbit
Bug Fixes
Tests