Repository navigation
fix(ci): unblock Windows CI — force LF checkout, then fix the Windows bugs it revealed - #286
Conversation
The `test (windows-latest)` job failed `vp check` with ~1028 files reported as mis-formatted. The repo had no `.gitattributes`, so the Windows runner (git's default core.autocrlf=true) checked out every text file with CRLF while oxfmt enforces LF. Ubuntu kept LF and passed, so the failure was platform-only. Add a root `.gitattributes` pinning text files to LF on every platform (`* text=auto eol=lf`), plus explicit `binary` markers for image/font/archive types so those are never line-ending converted. This is checkout-only: every committed blob is already LF, so `git add --renormalize .` rewrites nothing and `vp check` stays green locally (1037 files formatted, 0 lint errors). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_132faaf3-91f6-4469-b464-7eb9594175cf) |
…hild termination Fixing the Windows formatting gate let the suite run on Windows for the first time, which surfaced 42 failures in code that had never executed there. Product bugs (all changes are win32-gated; POSIX behavior is byte-identical): - canonicalizeConfigPath returned TWO spellings of the same file. Every path in this module comes from pathe (POSIX-normalized), but realpathSync is node:fs and returns a NATIVE path, so an existing config canonicalized to `D:\a\...` while a not-yet-created one canonicalized to `D:/a/...`. That split the lock identity, the wrangler `-c` argv, and the paths printed in errors, defeating the "ONE identity" contract the function exists to provide. - The quarantine gate could fail OPEN on Windows. Its probe target was `-pgid` with no platform guard, but `process.kill(-n, 0)` on win32 resolves to OpenProcess((DWORD)-n), which fails and surfaces as ESRCH — reporting any recorded owner "confirmed dead" and clearing a LIVE quarantine. The identical probe is already guarded in the retention watchdog; this mirrors it. - A child that had already exited could be QUARANTINED. Windows bypassed the killProcessGroup seam for child.kill(), whose `false` conflates ESRCH with EPERM, and exitCode/signalCode are set asynchronously so a just-exited child still reads as running — reported as a failed signal delivery. Windows now routes through the same seam, where process.kill throws ESRCH and is classified "already gone". - Termination confirmation was assumed rather than earned on Windows: probeGroupGone returned a constant true. It now performs a real kill(pid, 0) liveness probe; descendants remain covered by libuv's global Job Object. Test-only fixes (product verified correct): - The env-schema probe fixtures spliced an absolute path in as an ESM specifier; on Windows `D:/...` parses as protocol `d:` and Node rejects it with ERR_UNSUPPORTED_ESM_URL_SCHEME, so the child never evaluated and every result came back `schema-error`. Now built with pathToFileURL, as the product does. - The CF plugin writes its deploy pointer with node:path.relative, so compare the normalized form instead of pinning a POSIX literal. - writeQuarantineSidecar records pgid null on win32 by design. - Signal-delivery cases are skipped on Windows, which cannot deliver a signal to another process (libuv maps them to TerminateProcess). This includes one case that "passed" there only because the kernel Job Object reaped the child. Verified: packages/void full suite 3673 pass, typecheck and vp check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b8165a72-1435-441a-bccf-203cafcc5533) |
Brooooooklyn
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a38d7a6b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dle release The two remaining Windows failures were not assertion failures — both test bodies passed and the afterEach teardown threw: EPERM, Permission denied: \\?\D:\a\...\packages\void\test\unit\.tmp-kill-classify These cases deliberately keep a real child process alive (that is the scenario: a kill that fails, and a termination that cannot be confirmed), and the child's cwd is that temp dir. Both already kill the child and assert waitForDead before finishing, so the process is genuinely gone — but Windows releases a terminated process's handles, including the directory handle for its cwd, asynchronously, so the recursive remove immediately afterwards can still fail with EPERM. Use Node's documented remedy for exactly this: rm's maxRetries/retryDelay, which retry EBUSY/EMFILE/ENFILE/ENOTEMPTY/EPERM on a linear backoff and are honored only when recursive is set. Inert on POSIX, which unlinks a directory regardless of open handles. Verified: provision suite 248 pass, typecheck and vp check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b7ca53f8-11ab-4eb5-bded-0f8c5489ba47) |
…temp dir The previous retry-only attempt did not clear the Windows EPERM: ~5.5s of backoff still failed, so the directory was held persistently rather than being released late. Every child this block spawns runs with cwd = the temp dir, and Windows refuses to remove a directory that is a live process's cwd, so teardown cannot assume each case reached its own cleanup — a case that fails before its cleanup strands a child that then blocks every later removal. Reap anything still recorded in a *.pid file first, then remove. If removal still fails, report which children were alive and what entries remain instead of a bare errno, which is undebuggable without a Windows machine. Also document, at the reviewed line, why the Windows pid-based signal has no pid-reuse hazard: libuv closes its process handle in the same straight-line frame that invokes exit_cb (Node's onexit, which sets exitCode/signalCode), and Windows pins a pid while any handle to the process object is open — so a pid observed with a null exit code cannot have been reassigned. child.kill() is no safer in that instant, since uv_process_kill short-circuits to ESRCH and ChildProcess.kill reports it as the same ambiguous false that caused the original spurious quarantine. Verified: provision suite 248 pass, typecheck and vp check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_69e72aa8-86c8-46c4-a8e2-0dda325de4ee) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62008fe06e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…pids from disk The previous commit had teardown read `*.pid` files from a fixed, shared temp directory and SIGKILL whatever pid they named, gated only by a `kill(pid, 0)` probe. That probe only proves some process currently owns the pid — not that it is the process that wrote the file. A crashed or concurrent run leaves a stale pid file behind, the OS reassigns that pid, and teardown would then terminate an unrelated developer or CI process. Removing it. That reaping was also speculative: both affected cases demonstrably kill their own child and assert waitForDead before finishing, so a stranded child was never the confirmed cause of the EPERM. Instead give each test its own directory via mkdtempSync, which removes the shared-state hazard at the source: no stale artifact from another run can ever be read, and one case's stranded child can no longer block later cases. Teardown now kills nothing; it retries the removal and, if that still fails, reports which recorded pids currently resolve to a live process and what entries remain, so the next Windows run identifies the real holder instead of returning a bare errno. Verified: provision suite 248 pass, typecheck and vp check clean, no temp dirs left behind. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_cb83cb5a-c16a-424b-af3e-9bf7adba75a3) |
…ng on it Diagnostics from the last Windows run identified the actual cause, and it is neither a stranded child nor a product defect: entries remaining: cli.pid, hung.cjs still-live children reaped at teardown: none No process of ours is running, yet the child's own script (hung.cjs, the image the node child was executing) and its pid file cannot be unlinked. Windows keeps a terminated process's mapped image alive until rundown completes, which here outlasts ~10s of retries. waitForDead only proves the exit code is set. So teardown now classifies rather than blanket-tolerates. A recorded pid that is still live means a child was genuinely stranded — a real leak, still failed loud. Otherwise the only residue is the dead child's own files, which is an OS artifact, and the case's assertions have already passed; warn (keeping it visible) rather than reddening a green behavioural test over temp-file hygiene. Each directory is unique per test, so a leftover cannot affect any other case, and .tmp-* is now gitignored so one cannot dirty a working tree either. Confirmed no tracked path matches that pattern. Verified: provision suite 248 pass, typecheck and vp check clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e0583482-c9da-4f07-bb81-203e2ec5230b) |
…om Defender Windows `vp run test:all` was 828s, of which `vp test run` was 609s. vitest reported 775s of test time against that 609s wall — only 1.27x parallelism on a 4-vCPU runner. Two causes, both addressed here. 1. `fileParallelism: false` on the `core-markdown` project serialized all 24 integration files to protect a conflict between a handful of them. Mapping every file to the playground it drives shows only two clusters actually share one: kitchen-sink / kitchen-sink-build / cloudflare-deploy-build on playground/kitchen-sink, and named-layouts / full-deploy-manifest on playground/named-layouts — 34.6s of the 143.5s being serialized. Those five move to their own serial `shared-playground` project and the remaining 20 run in parallel. The new project needs its own `sequence.groupOrder`: `fileParallelism: false` resolves to `maxWorkers: 1`, and vitest rejects projects that share a group order but disagree on `maxWorkers`. Verified locally at higher concurrency than CI uses: 25 files, 197 passed, 44.9s of test time in 18.5s wall. 2. Defender's real-time scan is synchronous on every file write, and this suite writes a great many of them — dependency install, ~26 playground builds, and per-worker temp dirs. The workspace and runner temp are now excluded on the Windows runner only. It warns rather than fails if the call is refused, so a runner image that disallows it still runs the suite. Same-run per-file cost shows a uniform ~3-3.7x Windows tax, with analog (4.9x) and nuxt (4.8x) the outliers and the largest absolute contributors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7f642490-36b9-4d26-bfa5-cf2a08ec971e) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 564862a823
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…r.jsonc spa-middleware-auth.test.ts and the gated cloudflare-deploy-live e2e both rewrite playground/spa/wrangler.jsonc — the first deletes and restores it, the second writes a deploy overlay over it. Making core-markdown file-parallel let those two race. The earlier survey that found the kitchen-sink and named-layouts clusters globbed only test/integration, so it never covered test/e2e and missed this pair. Not reachable from the live-e2e job, which runs the e2e file by path and so collects nothing else, but it is reachable from a local `test:all` with credentials set — the case the credential forwarding exists to serve. Moving the e2e file also moves where its credentials have to be forwarded past vitest's worker env filter: `shared-playground` now carries `env: e2eWorkerEnv`. Without it the suite would read empty credentials and self-skip, turning the live deployment gate into a green no-op. core-markdown keeps the same env for whichever e2e file is added next. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_457a4b70-ceae-4c2b-b25a-1cdfd86d1618) |
Leaves real-time scanning on for the only checkout that is code no maintainer has pushed. Not because scanning is a boundary for this job — it has no secrets, a read-only token, an ephemeral runner, and already executes the PR's own build and test code by design — but because keeping it there costs nothing: Windows runs on a PR only when a maintainer applies the `test: windows` label, so fork runs are rare, while main pushes and maintainer branches keep the speedup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_39e430dc-626b-457d-b2f7-db3257571a17) |
|
Measured outcome of the two perf commits, so the record is straight: the test phase did not get faster.
Two things this shows. Run-to-run variance exceeds the effect size. The same The runner is CPU-saturated, not serialization-bound. Total CPU-time went up 9% when parallelism went up — the contention signature. Group 2 already runs three projects at two workers each on four vCPUs, so unblocking group 3 redistributes work rather than absorbing it. What did hold up is the low-variance file-I/O steps, which is where the Defender exclusion applies: dependency install 127s->107s, I am keeping both changes: the serialization fix is correct on its own merits (19 files were serialized to protect a conflict among 5, and it demonstrably parallelizes locally — 44.9s of test time in 18.5s wall), and the Defender exclusion pays for itself in the install steps. But neither is the answer to Windows being slow, and I would rather say so than let the commit rationale stand as a result. |
Windows CI has been red for months. This fixes the gate that hid the problem, then fixes everything the gate was hiding.
1. Why Windows CI was red
test (windows-latest)failedvp checkwith ~1028 files reported as mis-formatted — effectively the whole repo, on the last 5+ consecutivemainruns.The repo had no
.gitattributes. On Windows, git's defaultcore.autocrlf=trueconverts LF→CRLF on checkout, while oxfmt enforces LF. Ubuntu keeps LF and passed, so the failure was platform-only.Fix: a root
.gitattributespinning text files to LF everywhere (* text=auto eol=lf) plus explicitbinarymarkers. Checkout-only — every committed blob is already LF, sogit add --renormalize .rewrites nothing.2. What that revealed
With the formatting gate passing, the Windows job reached the test suite for the first time and 42 tests failed.
provision.ts/cloudflare-deploy.tslanded in #273 while Windows CI was already dying at formatting, so this code had never executed on Windows.Product bugs —
packages/void/src/cli/provision.tsAll changes are
win32-gated; POSIX behavior is byte-identical.canonicalizeConfigPathreturned two identities for one file. Every path in the module comes frompathe(POSIX-normalized), butrealpathSyncisnode:fsand returns a native path. An existing config canonicalized toD:\a\…, a not-yet-created one toD:/a/…— splitting the lock identity, the wrangler-cargv, and the paths printed in errors, defeating the "ONE identity" contract the function exists to provide.The quarantine gate could fail OPEN. Its probe target was
-pgidwith no platform guard, butprocess.kill(-n, 0)on win32 resolves toOpenProcess((DWORD)-n), which fails and surfaces asESRCH— reporting any recorded owner "confirmed dead" and clearing a live quarantine. The identical probe is already guarded in the retention watchdog; this mirrors it.An already-exited child could be quarantined. Windows bypassed the
killProcessGroupseam forchild.kill(), whosefalseconflatesESRCHwithEPERM, andexitCode/signalCodeare set asynchronously so a just-exited child still reads as running. Windows now routes through the same seam, whereprocess.killthrowsESRCHand is classified "already gone".Termination confirmation was assumed, not earned.
probeGroupGonereturned a constanttrueon Windows, degrading "never release the lock until the child is confirmed dead" to a single fact. It now performs a realkill(pid, 0)liveness probe; descendants stay covered by libuv's global Job Object.Test-only fixes (product verified correct)
D:/…parses as protocold:and Node rejects it withERR_UNSUPPORTED_ESM_URL_SCHEME, so the child never evaluated and every result came backschema-error. Now built withpathToFileURL, as the product does. Real users are unaffected — theirenv.tsuses bare specifiers and the product already usespathToFileURL.@cloudflare/vite-pluginwrites it withnode:path.relative(native separators); the assertion pinned a POSIX literal. Compare the normalized form.writeQuarantineSidecarrecordsnullon win32 by design.TerminateProcess). This includes one case that "passed" there only because the kernel Job Object reaped the child — a green that proved nothing.Two failures (
[33]/[34]) are fixed by the product change, not by touching them: their fixtures write a positivepgidagainst a live process and demand the gate fail closed. The tests were right; the code was wrong. They now guard the fix.Verification
packages/voidfull suite on POSIX: 3673 pass, 0 failvp checkcleanisWindows-gatedtest: windowslabelWindows progression
vp checkdied on ~1028 files — suite never ran.gitattributesThe 40 cleared include all 21 path-separator, all 10 env-probe, the build pointer, the pgid/quarantine set, and the signal-delivery cases. Two of them (
[33]/[34]) were fixed by the product change rather than by touching the test: their fixtures write a positivepgidagainst a live process and require the gate to fail closed. The tests were right; the code was wrong. They now guard the fix.Known remaining
cwd= the temp dir, and Windows will not remove a directory that is a live process's cwd. Teardown now reaps any recorded pid before removing and, if it still fails, reports which children were alive and what entries remain rather than a bare errno.net::ERR_NO_BUFFER_SPACE, then SvelteKit__spa_markerundefined). That pattern points at runner resource contention rather than a code defect, so it is being tracked across runs instead of patched speculatively.🤖 Generated with Claude Code
https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora
Note
Medium Risk
Changes touch provision locking and quarantine gates on Windows (fail-closed correctness); POSIX paths are explicitly preserved behind platform checks.
Overview
Unblocks Windows CI by adding root
.gitattributes(eol=lf) so checkout onwindows-latestmatches oxfmt’s LF expectation, then fixes the Windows-only failures that surfaced oncevp checkpassed.provision.ts(win32-gated): NormalizesrealpathSyncresults to POSIX so lock/wrangler paths stay one identity; fixes quarantine probing so negative PIDs are never used on Windows (fail-closed); routes child termination throughkillProcessGroup/kill(pid, 0)instead of assuming the child is gone.Tests & harness: Env-schema fixtures use
pathToFileURL; deploy pointer assertions normalize separators; POSIX-only signal tests skip on Windows; kill-classify temps are per-test with safer Windows teardown; Vitest splits shared-playground integration into a serialized project and forwards live-e2e env there.CI: Optional Defender exclusions for workspace/temp on trusted Windows runs;
.gitignorefor**/.tmp-*.Reviewed by Cursor Bugbot for commit 1daa7f8. Bugbot is set up for automated code reviews on this repo. Configure here.