test: pay Windows PowerShell's cold start outside the hook budgets - #3
Merged
Conversation
TestRunUserPromptSubmit_CollectsStdout failed on test (windows-latest) about two runs in three and never on a developer machine. The number was measured before it was changed. On four GitHub windows-latest runners the first `powershell -Command "exit 0"` in a job took 4.33s, 4.66s, 4.63s and 4.87s; every later one took 0.16-0.19s. A bare cmd.exe CreateProcess at the same instant took 15-38ms, so the machine was not busy. Attaching stdin cost nothing (170ms with none, 175ms with it, 180ms running the full hook script), and internal/hooks cost nothing (184ms with the tree-cancel wiring, 194ms end to end through Runner). The cost is Windows PowerShell's own start-up, paid once per machine, and it landed entirely on whichever test spawned first -- whose 5s budget sat 0.1-0.7s above a 4.6s constant. That is a coin flip, not a test. So: spawn the interpreter once in TestMain, before any budget is running, and let the budgets scale through internal/testwait like every other deadline in the suite. The tests still run the real hook path through the real interpreter; pwsh was measured as an alternative and is slower warm (265-285ms vs 165-185ms), so it would have been a regression. TestRunPreToolUse_MatcherCanBlock had less headroom than its 2 suggests -- 1.0-1.7s against two seconds, because Write-Error builds a full ErrorRecord -- and survived only because the test above it absorbed the cold start. TestRunPostToolUse_TruncatesStdoutAndStderr has already failed once on CI for the same reason. Both scale now. TestRunPreToolUse_TimeoutMessage deliberately does not: there the timeout is the subject rather than the scaffolding, and scaling the three-second ceiling past the five-second sleep would stop it telling a cancelled hook from a completed one. Measured at 1.02-1.05s, it does not need the slack. internal/statusline and internal/jobs spawn the same interpreter with the same budgets and were surviving only because internal/hooks happens to run earlier and absorb the cost -- a dependency on package scheduling that nothing stated. Their budgets scale too. testwait.Seconds is new: Timeout in whole seconds for the timeout_sec config fields, rounding up so a scaled budget is never quietly shortened by truncation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
packetloss404
force-pushed
the
fix/windows-hook-cold-start
branch
from
September 5, 2026 16:26
ce1887a to
61d8540
Compare
packetloss404
marked this pull request as ready for review
September 5, 2026 17:06
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. |
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
1 participant
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.
Fixes the flaky
TestRunUserPromptSubmit_CollectsStdoutontest (windows-latest).Diagnosis (measured, not assumed)
Instrumented on four GitHub
windows-latestrunners:cmd.exeCreateProcessThe first
powershell -Command "exit 0"on a machine costs ~4.6 s; every laterone costs ~0.18 s. A bare CreateProcess at the same instant costs 15-38 ms, so
the machine is not contended.
Ruled out by the same measurement:
running the full hook script.
internal/hooksitself — 184 ms with theConfigureTreeCancelwiring,194 ms end to end through
Runner.pwshas a cheaper interpreter — slower warm, 265-285 ms vs 165-185 ms.So the whole ~4.6 s landed on whichever test spawned first, against a 5 s
budget. The margin was 0.1-0.7 s, which is why it failed ~2 runs in 3 on CI and
never on a developer machine.
Fix
internal/hooksspawns the interpreter once inTestMain, before any test'sbudget is running.
internal/testwait(PACKETCODE_TEST_TIMEOUT_SCALE),as every other deadline in the suite already does.
is skipped on Windows.
Other tests in the file
TestRunPreToolUse_MatcherCanBlock(TimeoutSec: 2) measured 1.0-1.7 sagainst two seconds —
Write-Errorbuilds a fullErrorRecord, so it costsseveral times a bare spawn. It survived only because the test above it
absorbed the cold start. Scaled.
TestRunPostToolUse_TruncatesStdoutAndStderrhas already failed once on CIfor the same reason (job 99385812448). Scaled.
TestRunPreToolUse_TimeoutMessagedeliberately not scaled: there thetimeout is the subject, and scaling the 3 s ceiling past the 5 s sleep would
stop it distinguishing a cancelled hook from a completed one. Measured at
1.02-1.05 s; it does not need the slack.
internal/statuslineandinternal/jobsspawn the same interpreter with thesame budgets, and were surviving only because
internal/hookshappens to runearlier and absorb the cost — a dependency on package scheduling that nothing
stated. Their budgets scale too.
🤖 Generated with Claude Code