test(tools): stop internal/tools failing on stock Windows - #814
Conversation
This package fails on every run on a Windows box without PowerShell 7, which is what a stock install is: PowerShell 7 is a separate download. CI never sees any of it, because the runners have 7. Three distinct causes, all surfaced by the move to PowerShell in #804. The stop-intent test waited a fixed second for a session to die and asserted on whatever was true by then. Under cmd.exe that was enough; under PowerShell the tree is slower to stand up and tear down, and the FIRST stop in a fresh process pays the interpreter's cold start, so it routinely returned with termination under way but unfinished. The assertion read that as a failed stop. It now re-sends the stop until the session reports it has exited. Repeating is safe, since Terminate is a no-op against an already-dead tree, and it keeps the interrupt flag that a poll with empty chars would drop. Separately, and pre-existing, every test that starts a session hit a teardown race: a terminated process does not release its handles the instant the session reports exited, and Windows refuses to remove a directory with an open handle where POSIX does not. t.TempDir removes once and fails the test on the sharing violation, so ordinary teardown timing was reported as a broken test. Those roots now clean up with a bounded retry, and give up quietly rather than failing: the process is already terminated, so the worst case is a stale directory under the OS temp root. Last, the MSYS guard test asserted a windows_shell_syntax block using a command containing `&&`. On PowerShell 5.1 the windows_powershell_version preflight added by #804 fires first and shadows it. The `&&` was incidental to what that case is about, so it is gone. This one is worth noting as a shape rather than a typo: a new preflight silently changed which block an older assertion got, and only on the shell CI does not run. Verified by running the package repeatedly rather than once: previously failing on every run, now passing 3 of 3, and the stop-intent test passing 8 of 8 alone and 6 of 6 under CPU contention. Disabling the terminate path still fails it, so the assertion continues to bind.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesExecution test stability
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}
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. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at 20032342e55e, base 81a5e6c, re-confirmed live before posting. Up to date with main.
Test-only, and I checked that: git diff origin/main --name-only returns nothing outside _test.go. No production behaviour changes, which sets the bar for this correctly — the question is whether it fixes the flakes without weakening what the tests assert.
It does, and the two bounds are the reason. My concern with a change of this shape is that a Windows-motivated relaxation quietly relaxes the POSIX assertions too. Neither does.
The stop retry is bounded at 30s and ends in t.Fatalf naming the input, the meta and the output. It re-sends until the session reports exited rather than sleeping a guessed duration, but a session that genuinely never stops still fails the test. That is the distinction between fixing a race and deleting an assertion, and it lands on the right side.
The temp-root cleanup is bounded at 10s and then gives up with t.Logf. That is a real relaxation, and I went looking for what it costs on POSIX. Nothing, as far as I can tell: os.RemoveAll succeeds against open handles on POSIX unlink semantics, so t.TempDir would not have caught a leaked handle there either. The retry loop exits on the first attempt on darwin and Linux, so behaviour is unchanged outside Windows. No coverage is lost.
execTestRoot as one cross-platform helper rather than a Windows-tagged variant matches what SKILL.md §2 asks for, and fixing it as a class rather than per test is the right instinct — you are correct that chasing them individually would keep producing new names, and the description says which three surfaced and why the visible one varied.
On the correction in the description. You reported this to @anandh8x on #804 as "exit is broken while Ctrl+C still works", then corrected it there once you found the failing input alternates because what matters is which stop runs first. Recording that in the PR rather than quietly shipping the fix is the useful thing to have done — it stops the wrong diagnosis propagating into #804's history.
Verification. On macOS (darwin/arm64): ./internal/tools/ passing, and passing at -count=3 on the three named tests, so the retry has not introduced flakiness on the platform where the wait was previously sufficient. gofmt -l and git diff --check clean.
Limitations, and they are the whole point of the change. I have no Windows host and no PowerShell 5.1-only box, so the failures this fixes are unreproducible for me. Every claim about PowerShell cold start, handle release and the t.TempDir sharing violation rests on your measurements and on reading. The yield 1s / 3s / 6s table is the kind of evidence that makes that acceptable, but a Windows reviewer confirming the flakes are gone would close it properly.
Process. No parent issue. You are a collaborator so this is not the community gate, and a test-only Windows fix is a reasonable thing to carry without one — but it is worth an issue if only so the PowerShell 7 assumption is recorded somewhere findable.
Merge is kevin's call per the program gate.
The retry loop added to survive a slow PowerShell teardown also made the test unfalsifiable. The helper sleeps 5s, well inside the 30s deadline, so a terminate that did nothing at all would let the sleep end on its own and every existing assertion would still hold: the session reports exited, it reports an exit code, and the interrupted flag echoes the request rather than the outcome. Neutering KillProcessTree confirmed it, the test passed. Assert instead that the helper's completion line never appears, which distinguishes a killed session from a finished one without depending on how long the loop took. Output is accumulated across every poll rather than read off the last result, because each continuation returns only what the session produced since the previous collection.
aa7ce57
|
Went back over this one before asking anyone else to look at it, and the retry loop I added turned out to defeat the test it was meant to stabilise. The helper sleeps 5s and the deadline I gave the loop is 30s. So if termination did nothing at all, the loop would just spin until the sleep ended by itself, and every assertion still passed: the session reports exited, it reports an exit code, and Before the retry loop the single 1s yield made that impossible, so this was a regression the flake fix introduced, not something pre-existing. Fixed by asserting the helper's completion line never appears, which separates a killed session from a finished one without caring how long the loop took. Output is accumulated over every poll instead of read off the last result, since each continuation only returns what arrived since the previous collection and the marker can land in an earlier one. Rest of the PR held up. The |
|
@kevincodex1 ready to merge from my side. All nine checks green on One thing to square first, and it is my doing. @gnanam1990 approved this at The push was worth making. Reviewing my own work before handing it over, the retry loop I had added to fix the flake turned out to make the test unfalsifiable: the helper self exits after 5s and the loop waits up to 30s, so a terminate that did nothing at all would let the sleep end on its own and still satisfy every assertion, gnanam, the delta since your approval is 18 added lines in |
|
@gnanam1990 sorry to bounce this back to you. Your approval at To save you re-reading the whole thing, the delta is If you want to confirm the new assertion has teeth rather than take my word for it: Before the new line that fails with |
3 participants
internal/toolsfails on every run on a Windows box without PowerShell 7, which is what a stock install is. PowerShell 7 is a separate download; the GitHub runners have it, so CI has never seen any of this. Anyone cloning Zero on Windows hits an intermittent, and in one case constant, failure with no obvious cause.Three distinct problems, all surfaced by the move to PowerShell in #804.
1. The stop-intent test raced the shell's teardown
TestWriteStdinStopIntentTerminatesNonTTYSessionwaited a fixed second for a session to die, then asserted on whatever happened to be true. Under cmd.exe a second was plenty. Under PowerShell the tree is slower to stand up and tear down, and the FIRST stop in a fresh process pays the interpreter's cold start, so it routinely returned with termination under way but not finished. The assertion read that as "the stop did not work".This is the one I originally reported to @anandh8x on #804 as "
exitis broken while Ctrl+C still works". That was wrong, and I corrected it there: the failing input alternates, because what matters is which stop runs first, not which input it is. Measured, varying only the wait:It now re-sends the stop until the session reports it has exited, rather than guessing a duration. Repeating is safe because
Terminateis a no-op against an already-dead tree, and it keeps the interrupt flag that a poll with empty chars would drop.2. Every session test raced Windows teardown, pre-existing
A terminated process does not release its handles the instant the session reports exited, and Windows refuses to remove a directory with an open handle where POSIX does not.
t.TempDirremoves once and fails the test on the sharing violation, so ordinary teardown timing was reported as a broken test. This affectedTestExecSessionSnapshotsAndStopAllandTestWriteStdinRejectsInputForNonTTYSessionas well, and which one surfaced varied by whichever held a handle when cleanup ran.Those roots now clean up with a bounded retry and give up quietly rather than failing. The process is already terminated, so the worst case is a stale directory under the OS temp root, which is not worth failing a test over.
I fixed this as a class rather than per test, because chasing them one at a time was going to keep producing new names.
3. A new preflight silently shadowed an older assertion
TestBashToolRequireEscalatedMsysGuardasserts awindows_shell_syntaxblock, using a command containing&&. On PowerShell 5.1 thewindows_powershell_versionpreflight added by #804 fires first and returns a differentshell_issue, so the assertion fails. On PowerShell 7 that check does not fire at all, which is why CI is green.The
&&was incidental to what the case is about, so it is gone. Worth recording as a shape rather than a typo: adding a preflight changed which block an older assertion received, and only on the shell CI does not run.Verification
Run repeatedly rather than once, since a single green run proves nothing here:
gofmt,go vet, and builds for linux, darwin and windows are clean.Test-only apart from nothing; no production code is touched.
Summary by CodeRabbit