test(config): remove Windows startup timing assumptions - #800
Merged
Conversation
Cold PowerShell startup under concurrent Windows CI can consume most of the provider timeout, so an unsynchronized PID file may never be written and a fixed four-second bound can fail despite correct process-tree cleanup. Treat an absent PID as a helper that never reached startup, retain strict liveness checks for synchronized children, and bound completion by the child's natural lifetime instead.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughProvider command termination tests now tolerate missing PID files and use a shared 10-second child lifetime for background-process timing assertions. ChangesProvider command termination tests
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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. |
kevincodex1
approved these changes
Jul 24, 2026
Collaborator
Author
|
@kevincodex1 could you please merge this when ready? The same Windows provider-command timing failure is repeatedly blocking unrelated PRs, including #801, even though their Windows-specific changes pass. Landing #800 will stop this known test flake from affecting other PRs. |
Contributor
|
good to merge |
This was referenced Jul 26, 2026
kevincodex1
pushed a commit
that referenced
this pull request
Jul 27, 2026
…#810) 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.
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
2 participants
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.
Summary
Why
Windows smoke runs repeatedly failed even though provider-command cleanup was working:
WaitDelaypath exceed a fixed four-second assertionThe actual safety contract is that execution returns before the 10-second child finishes naturally and that every child known to have started is dead afterward. These tests now assert that behavior directly.
Verification
make fmt-checkgo vet ./...go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokego run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...git diff origin/main...HEAD --checkSummary by CodeRabbit