Repository navigation
fix(ci): size the Windows launcher-compile test budget from measurement - #17116
brennanb2025 wants to merge 1 commit into
Conversation
Greptile SummaryRaises the timeout for two Windows-only launcher compilation integration tests from 15 seconds to a measured 60-second budget.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/main/ssh/ssh-remote-cli-launcher.test.ts | Replaces the Windows-only 15-second per-test timeout with a documented 60-second measured allowance; no eligible blocking issue was identified. |
Reviews (5): Last reviewed commit: "fix(ci): size the Windows launcher-compi..." | Re-trigger Greptile
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthrough
Merge Risk: ⚪ Minimal · up to This localized CI change raises the Windows launcher test budget from 15 seconds to a measured 60 seconds without changing production behavior or the shared test default, preventing false failures from slow hosted runners. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is comprehensive and covers the required explanation, change, rationale, testing, visual proof, scope, and checklist items. The Linked Issue section uses N/A despite the template requiring an issue link, but it explains that this is a CI flake found during unrelated work.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
961a3e6 to
ea6d75e
Compare
brennanb2025
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1311ce1f-1b11-4991-8c74-986ee05a7f2a
📥 Commits
Reviewing files that changed from the base of the PR and between 87995fb15ccc1cddca255f68d6d1e011aef5db9d and 961a3e6ae1ca4d81ae4d03b44fddd66d5c163737.
📒 Files selected for processing (1)
src/main/ssh/ssh-remote-cli-launcher.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
`native-smoke (windows-latest)` fails ~4.5% of runs on `preserves a multiline argument through the compiled remote launcher` with "Test timed out in 15000ms" — on unrelated PRs, for reasons that have nothing to do with them. Across 176 sampled attempts it is the only red that job produced, and it hit seven different PRs in two days: #16900, #16904, #16915, #16955 (twice), #16979, #17014, #17085. The test is six process creations: powershell.exe forks csc.exe, then the freshly compiled orca.exe forks node.exe, twice. Hosted Windows runners periodically slow process creation down, and this test amplifies that far harder than anything else in the job. Comparing the 80 attempts where it ran under 3s against the 12 where it ran over 12s, its own median goes 2198ms -> 15917ms (7.2x) while the same file's powershell-only test moves 556 -> 686ms (1.2x), the cmd.exe and Git Bash process tests in the neighbouring file move 1.4x, and the other 35 files put together move 1.5x. Measured across those 176 attempts: 1881ms to 35438ms, p50 4264ms, correlation +0.881 with the job's total Vitest duration. 8 of 176 (4.5%) exceeded the 15s cap; 2 of 176 (1.1%) also exceeded the shared 30s testTimeout, so deleting the override and inheriting the config is not enough on its own. 60s clears all 176 with 1.7x headroom on the worst. This is slow, not hung. Every body here is synchronous spawnSync, so Vitest cannot interrupt one — the timer fires only after the body returns and the reported duration is real elapsed time. That is why a failure reads `× ... 22464ms` under `Test timed out in 15000ms`. The work finished; the stopwatch was short. Seven reruns at one identical head measured 2053 / 4680 / 5551 / 8732 / 13506 / 14868 / 21937ms — the last of those would have been red on code that had not changed. The 15s came from #8897, which raised this test off Vitest's built-in 5s default because the job then ran bare `pnpm vitest run`. #8909 landed 3h27m later and pointed the job at config/vitest.config.ts, which is the real fix for that. The constant stayed behind and has been the binding budget ever since.
4c7c9bd to
148e965
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This branch has not been deployed
1 participant
ELI5
One test in the
native-smoke (windows-latest)CI job builds a tinyorca.exewith the real Windows C# compiler and then runs it. Depending on how busy the CI machine is, that takes anywhere from 1.9 seconds to 35 seconds. It was given a 15-second stopwatch. About one run in twenty-two the machine is busy, the test runs long, and an unrelated pull request goes red.Nobody ever measured this test before picking 15 seconds. This PR measures it — 176 CI runs — and sets the budget from the data.
What Changed
One file, one hunk.
src/main/ssh/ssh-remote-cli-launcher.test.tsraises the timeout on its two Windows-only integration tests from15_000to a namedWINDOWS_LAUNCHER_TIMEOUT_MS = 60_000, with a comment recording the measurement. That is a deliberate exception to the sharedconfig/vitest.config.tsdefault oftestTimeout: 30_000— 30 s was measured and is insufficient; see Documented exception to the shared 30 s default below.config/vitest.config.ts, the workflow, the test bodies, and the file's three non-Windows tests are all untouched.Why
The mechanism
The test performs six process creations:
powershell.exeforkscsc.exe, then the freshly compiledorca.exeforksnode.exe— twice. Hosted Windows runners periodically slow process creation down, and this test amplifies that far harder than anything else in the job.Comparing the 80 sampled attempts where it ran under 3 s against the 12 where it ran over 12 s (medians):
windows-shell-preflight-runtime.windows.test.ts› cmd.exewindows-shell-preflight-runtime.windows.test.ts› Git BashThe job gets ~1.5× slower; this test gets 7.2× slower. Compiling a binary and then executing a brand-new one is the part that amplifies. Its own sibling in the same file — which starts
powershell.exeand stops as soon as compiler discovery fails — barely moves, which rules out powershell startup as the variable.It is slow, not hung
Every body in this file is synchronous (
spawnSyncthroughout). Vitest cannot interrupt a synchronous body — the timer fires only after the body returns, so the reported duration is real elapsed time. That is why a failure reads:22464 > 15000. The work finished; the stopwatch was short. All eight observed failures completed, at 15088 / 15862 / 15972 / 17327 / 19506 / 22464 / 31503 / 35438 ms. There is no wait that never arrives, and no assertion ever failed.Measured: 176
native-smoke (windows-latest)attemptsParsed from the job logs' per-test durations, over every attempt of 172 runs — not just the latest, because a run that was re-run to green hides its failed first attempt from
conclusion.config/vitest.config.tsdefault)Documented exception to the shared 30 s default
config/vitest.config.tssetstestTimeout: 30_000. This file'sitWindowshelper overrides it with60 s, and that override is deliberate. This section is the record of why, so it is not "tidied" back
to the default later.
30 s was my first patch — delete the override entirely and inherit the shared value. The data
killed it. Replayed against the 176 measured attempts, 30 s still leaves 2 red (1.1 %): 31503 ms
and 35438 ms. So 30 s is not a stricter budget for this test — it is a known-insufficient one.
60 s exceeds 0 / 176 attempts, with 1.7× headroom over the worst run observed.
The measurement behind it, so nobody has to re-derive it: 1881–35438 ms over 176 attempts of
native-smoke (windows-latest), p50 4264 ms, correlating +0.881 with the job's total duration.A 21937 ms attempt on byte-identical code passed under the new budget and would have been red under
the old 15 s cap.
The shared config is not changed. One slow Windows test is not a reason to move everyone's budget.
Is 30 s a repo-wide contract? No. It appears exactly once in the repo —
config/vitest.config.ts:37— and its own adjacent comment frames it as raising Vitest's 5 s default for the slowest integration
cases. No doc,
CONTRIBUTING, or lint rule states it as a ceiling. A per-test budget above it, carryinga comment that explains the cost, is the established pattern here:
src/main/ssh/ssh-relay-live-connect.test.ts(same directory)src/main/native-chat/agent-session-wire/structured-agent-session-refusal-retry.test.tssrc/main/daemon/slow-daemon-session-verification.test.tssrc/main/runtime/rpc/terminal-stream-byte-length.test.tsThis PR follows that pattern: it names the constant and writes the measurement next to it.
Same head, different colour
Eight reruns of this PR's own
native-smoke (windows-latest)job at one identical commit measured 2053 / 4680 / 5551 / 5989 / 8732 / 13506 / 14868 / 21937 ms — all green. On byte-identical code, the 21937 ms attempt would have been red under the old 15 s cap, and one more cleared it by 132 ms. The colour was never a property of the code.Four further reruns on the head this PR actually ships measured 4977 / 5673 / 6240 / 9431 ms, also green. Twelve post-change attempts, zero red.
Not Windows-vs-others
There is nothing cross-platform to compare: both integration tests are
it.skipoff Windows.native-smoke (ubuntu-22.04), same commit:✓ ssh-remote-cli-launcher.test.ts (5 tests | 2 skipped) 6msTests 3 passed | 2 skipped (5), 9 ms of test timeWindows does 100 % of the work. Skipping it there is not on the table — this is the SSH launcher.
Where the 15 s came from
Two commits landed on 2026-07-15:
20:26:4245a772cb42— test(windows): allow native launcher compilation time (#8897){ timeout: 15_000 }, commented "cold csc.exe startup exceeds Vitest's 5s unit budget"23:53:29d3fedb9ba9— ci(computer-e2e): run native-smoke vitest with the shared config (#8909)--config config/vitest.config.ts, commented "without --config, bare vitest ignores config/vitest.config.ts … and falls back to the 5s default timeout … so the real csc.exe launcher-compile tests time out"Both were aimed at the same problem, three and a half hours apart. #8909 is the real fix for it;
config/vitest.config.tshas carriedtestTimeout: 30_000since well before either landed. #8897's constant stayed behind and has been the binding budget ever since — never measured, and since23:53that day, below the project default rather than above Vitest's.Why this is its own PR
native-smokeis gatedif: github.event_name == 'pull_request', somainnever runs it (the nightly scheduled run shows it skipped) and it cannot be reproduced there. It is not specific to any PR either: the job's Vitest file list is hardcoded in the workflow YAML, and neither #16955 nor #17085 changes a single file in it. So it lands againstmainon its own, alongside #17086 and #17088.What this is not
config/vitest.config.tsis untouched, and so are the three non-Windows tests in this file.Linked Issue
N/A — CI flake found while reviewing unrelated PRs. Third in a batch with #17086 (a leaked real timer poisoning a later fake-timer test) and #17088 (a Windows cleanup permission error raised after every assertion had passed).
Visual Proof
N/A— CI-only change; no UI or user-visible behavior.Before / after
Before — an unrelated pull request could go red for a reason that had nothing to do with it. It hit seven PRs in the two days I sampled, each costing a rerun. Worse than the rerun: a red check that is not about your change teaches everyone to re-run first and read second, which is exactly how a real regression slips past a glance.
After — the budget is 1.7× the worst of 176 measured runs. Replayed against those 176, 0 would have failed; all eight real failures, up to 35438 ms, pass.
Testing
node config/scripts/run-typecheck-projects-in-parallel.mjs(all projects) — passoxlinton the changed file — passoxlint --config config/oxlint-code-quality-native-plugins.json src/main/ssh config tests --deny-warnings— passoxlint --type-aware --config config/oxlint-code-quality-type-aware.json src/main/ssh --deny-warnings— passnode config/scripts/check-changed-code-quality.mjs— 0 new findings (native, type-aware, React Doctor)npx oxfmt --writeon the changed file only;git diff --checkcleanvitest run --config config/vitest.config.ts src/main/ssh/ssh-remote-cli-launcher.test.tson macOS — 3 passed, 2 skippednative-smoke (windows-latest)job — the exact lane that was failing — plus eight reruns of it collected while measuring.Platforms: macOS locally; Linux and Windows via this PR's CI. SSH: this is the SSH remote-CLI launcher's own coverage, and it still runs unchanged on Windows.
No new test: this is the test, and its assertions are untouched. What changed is the budget it runs under.
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Security, mobile, performance, backwards compatibility: unaffected — this is a test-runner budget. Cross-platform: the two affected tests are Windows-only by construction and stay that way; Linux and macOS still skip them. Remote SSH: coverage of the Windows remote CLI launcher is unchanged — same work, same assertions.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)