fix(windows): spawn engine binaries that are .cmd shims - #105
Conversation
|
Worth flagging before this is reviewed: CI cannot verify the Windows half of this PR yet. This branch does not touch the workflow, so it gets the label-gated Two ways to close that, whichever you prefer:
I would rather not have this merged on my machine's word alone, given that is exactly how the |
|
#104 is merged, so the Windows leg is now unconditional on Taking your ordering point as decided: I'd rather this waited for the runner than merged on one machine's word, for exactly the reason you gave. The On rebase, #104's skip on the three codex-rollout cases comes off, since this PR replaces it with the Two things I'll flag now rather than after the run, both from reading the diff: The
|
Fixes hristo2612#103. Codex usage limits were unavailable on Windows against a working install, silently: both call sites swallow the error and resolve undefined, so there was no log line and no user-visible reason. There were two bugs, not one. Resolution never found the binary. findOnPath joined dir + bare name, but an npm-installed CLI on Windows is `codex.cmd`, so nothing matched and resolveBin fell through to the bare name -- which CreateProcess cannot start either, because it tries .exe and .com but never .cmd. It now walks PATHEXT, most specific first, so a real .exe still wins over a shim. isExecutableFile also stopped consulting X_OK there, where it succeeds for any readable file and means nothing. Spawning then refused the shim. Node has rejected .cmd and .bat without a shell since 18.20.2 (CVE-2024-27980), so both engine-limits call sites threw EINVAL. They now route through cmd.exe explicitly rather than shell: true -- same interpreter, but the quoting stays visible and reviewable, cmd.exe is resolved from System32 rather than PATH, and /d keeps a registry AutoRun out of every engine spawn. Arguments are quoted per MSVCRT rules with the trailing backslash run doubled, the hazard cli/skills.ts documents. windowsVerbatimArguments is required with it: without that Node re-quotes the line and cmd.exe reports the whole thing as an unrecognised command. Interposing cmd.exe makes the engine a grandchild, so killing the handle would reap only the interpreter and leave a long-lived app-server running on every timeout. killProcessTree uses taskkill /T for that, best-effort, since the caller is already on an error path. The three codex-rollout cases now run on Windows with real coverage rather than skipped: their stub is written as a .cmd, which is the shape npm actually installs, so they exercise this path instead of asserting around it. Note for review: resolveBin now hands every other engine a resolved .cmd path where it previously handed them a bare name. That is not a regression -- a .cmd-only install could not be spawned either way -- but making the PTY engines launch a shim is separate work, and I have not attempted it here.
hristo2612#104 skipped these on Windows because it had to make the leg green without this fix. The .cmd stub replaces that skip with real coverage: they exercise the shim spawn path and fail without the change in this PR.
4302861 to
d6254e2
Compare
|
Rebased onto The Windows leg should now run on this PR for real, since #104 made it unconditional. That is the run I have been waiting for. PTY follow-up filed as #107. It ended up worth more than a one-liner, because a PTY is not just a
One thing I flagged there that is worth your attention: the engine case is more severe than the limits gap #103 covered. That one degraded a display; this one means an npm-installed engine cannot be launched at all on Windows. Anyone with Agreed on keeping the |
|
Gated this on top of One blocker, and it is one character.
const root = process.env.SystemRoot || process.env.windir || "C:\Windows";
It only bites when both Rather than fixing the literal, I'd pull the root resolution into one helper that both On the CI red you may have seen: one of my four gate runs failed on Fix the literal and I'll merge. Everything else here is done. Separately: #107 is a good split, and I've read it. Your severity read is right and worth restating plainly, because it is worse than #103 was: that one degraded a display, this one means an npm-installed engine cannot be launched at all on Windows. I verified the surface — 11 sites across Take your step 1 first and report before building anything: whether |
killProcessTree wrote its fallback as "C:\Windows". \W is not an escape
sequence, so JavaScript dropped the backslash and the literal was the
drive-relative "C:Windows" -- a directory that does not exist. taskkill.exe
beneath it could not be found, execFileSync threw, and the catch fell back to
child.kill("SIGTERM"), degrading silently into precisely the grandchild leak
the function exists to prevent. Reachable whenever neither SystemRoot nor
windir is set, as in a stripped service environment.
Fixed as review suggested, by removing the duplication rather than the
character: one windowsRoot() and one system32(), used by both cmdExePath and
killProcessTree. Two copies of the same fallback in one 90-line file is what
let them drift, and hristo2612#107 will add a third caller.
windowsRoot is exported so a test pins the behaviour instead of proofreading
catching it next time: precedence between the two variables, and that the
fallback is ROOTED -- asserted against path.win32.isAbsolute, with the
drive-relative form the bug produced asserted false alongside it so the
expectation cannot later be adjusted to match whatever the code emits.
Mutation-checked: reintroducing the single backslash fails two of these tests.
|
Fixed at Verified the mechanism before touching it — Taken your approach rather than the one-character fix: one
Mutation-checked: reintroducing the single backslash fails two of those tests. It would not have been caught otherwise, since the fallback only runs when both variables are unset. One process note, since it is the second time on this branch that my own escaping has bitten me: I generated that literal through a shell heredoc, which silently ate a backslash. The test above now covers the class rather than the instance. On the Separately, and it changes #107 materially: I have answered your step 1. node-pty launches a So the |
|
The Windows red on this PR is not this PR. It is I have not re-run it to green. Raised as #110 instead, with the investigation:
Two things I want to be straight about rather than let the PR description imply otherwise: I cannot claim #110 eliminates it. I instrumented the give-up path at six times CI's concurrency and got My first instinct was wrong and the measurement caught it. I assumed a longer budget would show up as a lower failure rate under stress, and it did not — 1/12 against Happy to rebase this onto #110 once it lands so the leg here is green for the right reason. It should not merge on a re-run. Everything else on this PR is unchanged and green: |
|
Field evidence, from a real Windows install rather than a fixture. I upgraded my own instance to the released 0.29.0 today and the boot log carries this unprompted: Volta installs both shims side by side: And Node's
That is this PR's argument reproduced end to end, and it shows both halves are load-bearing:
So the Worth noting the failure mode matches what #103 described: the warning is one line in a boot log nobody reads, the collector resolves Same evidence applies to #107 — the engines resolve through the identical path, so an npm- or Volta-installed engine on Windows hits |
|
Merging. The blocker is resolved the way I asked, and I chased the red Windows leg to a named test rather than re-running until green. The The test you pinned it with is better than the fix. Asserting the fallback is rooted via The Windows failure is not yours. Run 30541919096: 1 failed / 3578 passed / 12 skipped. The single failure is Four lines of evidence, and I want to be honest about which ones actually carry weight:
Point 4 is the weakest and I am not leaning on it: four green runs is what a 20%-rate flake produces about 41% of the time, so "main is green, the PR is red" would not settle ownership on its own. Point 2 is what settles it. Your diagnosis in #110 matches: On sequencing. The tidy order is #110 first, then rebase this and take a green leg. I am not doing that, because #110 by your own measurement makes the leg probably green rather than reliably green, and holding a finished PR behind a probabilistic gate trades a real merge for a cosmetic one. The ownership evidence above is stronger than a re-run would be, so it goes in the record instead. The cross-process init lock you flagged is the actual fix and is worth its own issue. macOS gate on top of Closes #103. Thank you — that is six merged now. |
The Windows leg failed on hristo2612#105 with "attempt to write a readonly database" from one of 16 processes initializing the registry -- the same class hristo2612#104 addressed at journal_mode, now at the schema-init transaction one frame along. The retry was engaging correctly: the error code is SQLITE_READONLY, which the predicate matches, and runSqliteBusyRetry is in the stack. It simply ran out. The ladder [10, 50, 200, 500, 1000] spends 1.76s, and the worker that died had been contending for 3.5s. An attempt count is the wrong unit for this. What is being waited out is a window of contention whose length has nothing to do with how many times we have asked, so this is now a time budget: 15s on Windows, 5s elsewhere, matching the busy_timeout already set on the connection. Backoff is exponential and jittered -- without jitter, peers that collide once back off by the same amount and collide again, which is how a ladder that looks generous still exhausts itself. Measured rather than assumed. Instrumenting the give-up path at six times CI's concurrency produced `RETRY-GAVEUP elapsed=15015ms code=SQLITE_READONLY`: the loop engages, backs off, and exhausts the whole budget. So this raises the ceiling from 1.76s to 15s against observed contention of 3.5s, and it is not a guarantee -- no bounded wait can be one. The change that would remove the ceiling is serializing initialization across processes, which is larger and deserves its own review. At CI-equivalent load (16 processes) this is 0 failures in 10 local runs. A comparison against main at 96 processes is within noise, because at that concurrency both exhaust whatever budget they are given.
2 participants
Fixes #103. Codex usage limits were unavailable on Windows against a perfectly working install, and silently — both call sites swallow the error and resolve
undefined, so there is no log line and no user-visible reason. The feature simply appears never to have data.There were two bugs, not one. The second is the one #103 describes; the first is why it was never even reached.
Resolution never found the binary.
findOnPathjoineddir+ bare name, but an npm-installed CLI on Windows iscodex.cmd, so nothing matched andresolveBinfell through to returning the bare name — whichCreateProcesscannot start either, since it tries.exeand.combut never.cmd. Both halves had to know aboutPATHEXT.It now walks
PATHEXT, most-specific first, so a real.exestill wins over a shim when both exist (pinned by a test).isExecutableFilealso stops consultingX_OKon Windows, where it succeeds for any readable file and therefore means nothing.Spawning then refused the shim. Node has rejected
.cmd/.batwithout a shell since 18.20.2 (the CVE-2024-27980 fix), so both sites threwEINVAL.They now route through
cmd.exeexplicitly, rather thanshell: true. Both end up running cmd.exe, but doing it here keeps the quoting visible and reviewable instead of delegating it, resolves cmd.exe fromSystem32rather than PATH, and passes/dso a registry AutoRun cannot inject work into every engine spawn. Arguments are quoted per MSVCRT rules with the trailing backslash run doubled — the same hazardcli/skills.tsdocuments.windowsVerbatimArgumentsis required alongside it, and this took a moment to find: without it Node applies its own quoting on top of the line built here, and cmd.exe reports the whole thing as an unrecognised command. It is whatshell: truesets internally.Interposing cmd.exe makes the engine a grandchild, so
child.kill()would reap only the interpreter and leave a long-livedapp-serverrunning — one leaked process per collection, every time the read times out.killProcessTreeusestaskkill /Tfor that, best-effort, since the caller is already on an error path.The three codex-rollout cases now run on Windows with real coverage. You noted in #103 that fixing the stub alone would make them pass without touching the product bug, so I did the opposite: their stub is written as a
.cmd— the shape npm actually installs — so they exercise this spawn path rather than asserting around it. They fail without this change and pass with it, which is the coverage the issue asked for.Also tested:
PATHEXTresolution and its ordering, the cmd.exe rewrite and its quoting, and a real.cmdexecuted end-to-end after asserting that a barespawnSyncof it givesEINVAL— so the regression itself is pinned, not just the fix.Stacking: #104 skips those three cases on Windows, since it had to make the leg green without this fix. If #104 lands first this PR should drop that skip on rebase; if this lands first, #104's skip becomes a no-op to remove. Either order works, they just both touch
engine-limits.test.ts.One thing for review.
resolveBinnow hands every engine a resolved.cmdpath where it previously handed them a bare name. I do not believe that is a regression — a.cmd-only install could not be spawned either way — but making the PTY engines actually launch a shim is separate work with different constraints (interposing cmd.exe inside a PTY changes the process tree the CLI renders into), and I have not attempted it here. Happy to file it if you agree it is worth tracking.Verified on Windows 11 / Node 24:
shared+engines+clisuites pass (738 tests), typecheck clean on both packages.