fix(runtime-host): stop a slow PowerShell start from refusing the Windows endpoint - #3235
Conversation
|
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 (2)
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review. 📝 WalkthroughWhat problem this solvesWindows PowerShell cold starts can exceed the previous 10-second ACL timeout. This PR raises the timeout to 30 seconds and prevents valid Windows endpoints from being rejected too early. The endpoint remains fail-closed. Timeout, spawn, exit-code, and ACL failures still reject startup. The error now includes capped PowerShell diagnostics, exit codes, and errno details. Timeout failures state that the restriction was not confirmed. Source of truth and solution scopeThe PR extends the existing Windows named-pipe ACL path. It does not create a parallel startup or retry path. The change is the smallest coherent solution identified in the diff. It changes the existing timeout, adds one failure-formatting function, and passes existing process output into that formatter. The added complexity is required to preserve fail-closed behavior while making failures diagnosable. The exported Deletion or simplification opportunitiesNo deletion is evident without weakening regression coverage. The four tests cover distinct failure mappings: command failure, timeout termination, spawn failure, and oversized output. The output limits and newline normalization also require explicit coverage. Validation and concrete risksThe affected test suite, build, typecheck, formatting, and lint reportedly pass locally. Repository-wide tests and Windows-specific tests were not run locally. Windows behavior therefore remains dependent on CI validation. The main risks are:
Complexity delta
Total maintenance complexity increases slightly. The increase is justified by required diagnostics and regression coverage. No evidence shows an unnecessary parallel implementation or redundant configuration. Review-relevant risksThe current diff changes user-visible startup failure timing and error messages. Material changes in user-visible behavior require independent human review under repository policy. The diff changes Windows endpoint security behavior only by extending the existing ACL wait period. The endpoint remains fail-closed, and no retry is added. Material security changes require independent human review under repository policy. The diff adds an exported function for testability. Material public-contract changes require independent human review under repository policy. The final status of required checks is unverified from direct evidence here. The person performing the merge must review the final diff, and a maintainer makes the final determination. WalkthroughWindows named-pipe ACL execution now uses a timeout and bounded diagnostics. Failure messages include execution status, signals, exit codes, spawn errors, and normalized PowerShell output. Tests cover these cases on Windows. ChangesWindows ACL diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change extends the Windows startup timeout and improves failure diagnostics while preserving fail-closed endpoint behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…dows endpoint The Windows Local IPC endpoint is restricted by an ACL applied through PowerShell, under a 10s timeout. That budget killed a healthy run: in run 32182673038 the trust-boundary step took 32s and failed, and the rerun of the identical commit took 7s and passed. A genuine ACL error fails in under a second, so the failure was the timeout firing on a slow Windows PowerShell 5.1 cold start, not a broken ACL. The budget is not a statement about how long the work should take. This ACL is the entire trust boundary of the endpoint -- every accepted connection is granted Local Owner authority with no further per-connection check -- so the call must succeed and a timeout must refuse the endpoint, which makes a refusal a startup failure for the user. Raise the ceiling to 30s so it only fires on a genuinely stuck process, well clear of a slow start. Nothing in the log allowed that call to be made at the time: the callback discarded the execFile error entirely, collapsing a timeout kill, a real ACL failure and a failure to spawn PowerShell into one opaque message, so the only way to classify the failure was to rerun the job. Map the failure onto the thrown error instead -- the length-capped PowerShell diagnostic, the exit code or errno, and a timeout reported as a restriction that was never confirmed rather than one that failed. Every branch still rejects. The ACL call only runs on win32, so the mapping is exported and covered directly; the raised budget is asserted only through the message that carries it. Generated-by: Claude Code
9b36d26 to
0a1f7a6
Compare
Astro-Han
changed the title
PR Summary by QodoTolerate slow Windows pipe ACL setup and improve diagnostics
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
|
Code review by qodo was updated up to the latest commit 0a1f7a6 |
6b54ce7 to
0a1f7a6
Compare
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
This is close, but one timeout still blocks the intended fix.
windows-runtime-host-local-ipc-trust.ps1 waits only 10 seconds for fixture readiness, while the ACL helper may now legitimately take up to 30 seconds. If PowerShell takes 10–30 seconds, the outer fixture still times out and terminates the process first.
Please raise the fixture readiness deadline above the ACL budget with some startup margin, for example 45 seconds.
The startup transport retention noted by Qodo can be simplified later, but I don’t consider it blocking for this PR.
中文
这个 PR 已经很接近了，但还有一个超时边界会阻塞预期修复。
windows-runtime-host-local-ipc-trust.ps1 只等待 fixture 10 秒，而 ACL helper 现在允许合法运行最多 30 秒。当 PowerShell 耗时 10–30 秒时，外层 fixture 仍会先超时并终止进程。
请将 fixture readiness deadline 提高到 ACL 预算之上并预留启动余量，例如 45 秒。
Qodo 提到的启动期 transport 保留可以后续简化，但我不认为它会阻塞本 PR。
The Windows trust fixture prints readiness only once the endpoint ACL has been applied, and the harness waited 10s for that line. Raising the ACL budget to 30s therefore moved the bottleneck rather than removing it: an ACL taking 10-30s now trips the harness first, and because it throws while the fixture is still alive, the fixture's stderr is never read -- the failure reports less than it did before. Raise the readiness deadline to 45s, the budget the client already uses to wait for a Runtime Host, and record at both ends that the waiters have to outlast the ACL budget. Reported in review on #3235. Generated-by: Claude Code
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
LGTM — the fixture deadline now safely outlasts the ACL budget, and the relevant Windows checks pass.
EnglishThanks — fixed in 1f91603. Raised the readiness deadline to 45s, matching the budget Agreed on the Qodo point — leaving the startup transport retention alone here. 中文谢谢——已在 1f91603 修复。 readiness deadline 提到了 45s，与 Qodo 那条同意——启动期 transport 保留这次不动。 |
维护者式预审 · PR #3235 @
|
3 participants
Summary
secureWindowsNamedPipe()applies the Windows Local IPC pipe ACL through PowerShell under a 10s timeout, and that budget killed a healthy run: in run 32182673038 the trust-boundary step failed after 32s, and the rerun of the identical commit passed in 7s. A real ACL error fails in under a second, so this was the timeout firing on a slow PowerShell 5.1 cold start.The budget is a question of how long we are willing to wait, not how long the work should take. This ACL is the endpoint's entire trust boundary —
packages/runtime-host/src/server/local-ipc-listener.tsgrants Local Owner authority to every accepted connection with no further per-connection check — so the call must succeed, and a timeout must refuse the endpoint, which makes a refusal a user-facing startup failure. Raised to 30s, well clear of the ~7s a healthy step takes. Fail-closed is unchanged, and no retry is added: the observed mechanism is a slow start, not a random hang.Endpoint readiness waits on that budget, so the waiters have to outlast it.
scripts/windows-runtime-host-local-ipc-trust.ps1allowed 10s, which would have made it the new bottleneck — and because it throws while the fixture is still alive, its stderr never gets read, so that failure would report less than before. Raised to 45s, the budget the client already uses inclient/wait-for-ready.ts, and recorded at both ends.Nothing in the log allowed any of this diagnosis at the time. The callback discarded the
execFileerror, collapsing a timeout kill, a real ACL failure (the pipe may be world-accessible) and a failure to spawn PowerShell into one opaque message. The thrown error now carries the capped PowerShell diagnostic plus the exit code, the errno, or — for a timeout — a restriction that was never confirmed rather than one that failed. Every branch still rejects.Refs #3225
Review focus
Runtime Host is a protected area under
AGENTS.md, so this needs independent human review and is not a self-merge fast-path candidate.windowsPipeAclFailure()is exported so the mapping can be tested off-Windows. The branch sits behindplatform === 'win32', where no injected-exec seam would reach it either;control/endpoint.tsis not in the packageexportsmap.RuntimeHostEndpointError['code']is deliberately unchanged — a timeout is a refusal like any other, and no caller branches oncode.error.message, which repeats the whole command, is a capped last resort.Verification
@maka/runtime-hostbuild, typecheck,npm run format:checkand biome lint — clean.node --test packages/runtime-host/dist/__tests__/control-endpoint.test.js— 9/9 pass (4 new, cross-platform).Not run: repository-wide tests (left to CI), and anything on Windows — no Windows host here, so
windows_recoveryis only exercised by CI, and the.ps1edit (a comment and one constant) was not parsed locally, as nopwshis available. The new tests cover the failure mapping; the raised budgets are asserted only through the message that interpolates the ACL constant, so nothing here proves either value reaches its caller. Those lines are unreachable off-Windows and I did not add a test that pretends otherwise.AI use
Select exactly one:
Tool(s) and scope: Claude Code (Opus) diagnosed the root cause from the CI attempt timings, wrote the change, the tests and this description, from a human-written problem statement identifying the failing run. The human contributor of record reviews the final diff and owns the merge decision.
Checklist
Does this PR entail a change in behavior?