Skip to content

Commit 75bd87c

Browse files
Brooooooklynclaude
andauthored
fix(ci): unblock Windows CI — force LF checkout, then fix the Windows bugs it revealed (#286)
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)` failed `vp check` with ~1028 files reported as mis-formatted — effectively the whole repo, on the last 5+ consecutive `main` runs. The repo had **no `.gitattributes`**. On Windows, git's default `core.autocrlf=true` converts LF→CRLF on checkout, while oxfmt enforces LF. Ubuntu keeps LF and passed, so the failure was platform-only. Fix: a root `.gitattributes` pinning text files to LF everywhere (`* text=auto eol=lf`) plus explicit `binary` markers. Checkout-only — every committed blob is already LF, so `git 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.ts` landed 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.ts` All changes are `win32`-gated; POSIX behavior is byte-identical. - **`canonicalizeConfigPath` returned two identities for one file.** Every path in the module comes from `pathe` (POSIX-normalized), but `realpathSync` is `node:fs` and returns a **native** path. An existing config canonicalized to `D:\a\…`, a not-yet-created one to `D:/a/…` — splitting 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.** 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. - **An already-exited child 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. Windows now routes through the same seam, where `process.kill` throws `ESRCH` and is classified "already gone". - **Termination confirmation was assumed, not earned.** `probeGroupGone` returned a constant `true` on Windows, degrading "never release the lock until the child is confirmed dead" to a single fact. It now performs a real `kill(pid, 0)` liveness probe; descendants stay covered by libuv's global Job Object. ### Test-only fixes (product verified correct) - **env-schema probe (10 failures).** The 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. Real users are unaffected — their `env.ts` uses bare specifiers and the product already uses `pathToFileURL`. - **Deploy pointer.** `@cloudflare/vite-plugin` writes it with `node:path.relative` (native separators); the assertion pinned a POSIX literal. Compare the normalized form. - **pgid assertion.** `writeQuarantineSidecar` records `null` on win32 by design. - **Signal-delivery cases skipped on Windows**, which cannot deliver a signal to another process (libuv maps SIGTERM/SIGINT/SIGKILL to `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 positive `pgid` against a live process and demand the gate fail closed. The tests were right; the code was wrong. They now guard the fix. ## Verification - `packages/void` full suite on POSIX: **3673 pass**, 0 fail - typecheck clean, `vp check` clean - POSIX unchanged — every product change is `isWindows`-gated - Windows verified by CI on this PR via the `test: windows` label ### Windows progression | Run | Windows result | |---|---| | baseline (before this PR) | `vp check` died on ~1028 files — suite never ran | | after `.gitattributes` | gate passes; **42** test failures surfaced | | after the provision/probe fixes | **2** failures (both teardown-only; all 40 behavioural failures cleared) | | after the teardown fix | pending | The 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 positive `pgid` against 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 - **Teardown EPERM** in the kill-classify block: every child there runs with `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. - **Browser-test flakiness on Windows**: one metaframework browser case has failed per run, but a *different* one each time (Solid `net::ERR_NO_BUFFER_SPACE`, then SvelteKit `__spa_marker` undefined). 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.com/claude-code) https://claude.ai/code/session_019wnwcxjavdb2xys8XGZora <!-- CURSOR_SUMMARY --> --- > [!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 on `windows-latest` matches oxfmt’s LF expectation, then fixes the Windows-only failures that surfaced once `vp check` passed. > > **`provision.ts` (win32-gated):** Normalizes `realpathSync` results 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 through `killProcessGroup` / `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; `.gitignore` for `**/.tmp-*`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1daa7f8. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 870b8c1 commit 75bd87c

8 files changed

Lines changed: 283 additions & 37 deletions

File tree

‎.gitattributes‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# Enforce LF line endings for every text file on all platforms.
2+
#
3+
# Without this, the Windows CI runner (git's default `core.autocrlf=true`) checks out
4+
# text files with CRLF, and oxfmt — which `vp check` / `vp fmt` run and which enforces
5+
# LF — then flags every file as mis-formatted (the `test (windows-latest)` job). Every
6+
# committed blob is already LF, so this only changes checkout behavior on Windows; it
7+
# triggers no renormalization of existing files.
8+
* text=auto eol=lf
9+
10+
# Binary assets — never apply any line-ending conversion.
11+
*.png binary
12+
*.jpg binary
13+
*.jpeg binary
14+
*.gif binary
15+
*.ico binary
16+
*.webp binary
17+
*.avif binary
18+
*.woff binary
19+
*.woff2 binary
20+
*.ttf binary
21+
*.otf binary
22+
*.eot binary
23+
*.pdf binary
24+
*.wasm binary
25+
*.mp4 binary
26+
*.zip binary
27+
*.gz binary

‎.github/workflows/ci.yml‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,34 @@ jobs:
3333
- uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
3434
with:
3535
persist-credentials: false
36+
# Defender's real-time scan is synchronous on every file write, and this suite writes a lot of
37+
# them (dependency install, ~26 playground builds, per-worker temp dirs). Excluding the paths
38+
# we generate is purely a speed measure: it never changes what runs.
39+
#
40+
# Skipped for fork pull requests, the only case where the checkout is code no maintainer has
41+
# pushed. Not because scanning is a boundary for this job — it has no secrets, a read-only
42+
# token, an ephemeral runner, and already executes the PR's own build and test code by design
43+
# — but because leaving the scan on for untrusted checkouts costs us nothing: Windows runs on
44+
# a PR only when a maintainer applies the `test: windows` label, so this is rare, while main
45+
# pushes and maintainer branches (the everyday path) keep the speedup.
46+
#
47+
# Warns instead of failing, so a runner image that refuses the call still runs the suite.
48+
- name: Exclude build paths from Windows Defender
49+
if: >-
50+
${{ contains(matrix.os, 'windows')
51+
&& (github.event_name == 'push'
52+
|| github.event.pull_request.head.repo.full_name == github.repository) }}
53+
shell: pwsh
54+
run: |
55+
foreach ($path in @('${{ github.workspace }}', '${{ runner.temp }}')) {
56+
try {
57+
Add-MpPreference -ExclusionPath $path -ErrorAction Stop
58+
Write-Host "Defender exclusion added: $path"
59+
} catch {
60+
Write-Host "::warning::could not add Defender exclusion for ${path}: $_"
61+
}
62+
}
63+
3664
- uses: voidzero-dev/setup-vp@2dec1e33f4ab2c6d5bce1b0c4607961bb1a3f7a1 # v1.12.0
3765
with:
3866
node-version-file: .node-version

‎.gitignore‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,7 @@ playground-local
2121
.worktrees
2222
.svelte-kit/
2323
docs/superpowers/
24+
25+
# Test temp dirs (unique per test; a Windows run may leave one behind when the OS has not
26+
# yet unmapped a terminated child process image — see provision.test.ts teardown).
27+
**/.tmp-*

‎packages/void/src/cli/provision.ts‎

Lines changed: 69 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import {
1717
import { createRequire } from 'node:module';
1818
import { types as utilTypes } from 'node:util';
1919
import { parse, type ParseError } from 'jsonc-parser';
20-
import { basename, dirname, join, resolve } from 'pathe';
20+
import { basename, dirname, join, normalize, resolve } from 'pathe';
2121
import lockfile from 'proper-lockfile';
2222
import { voidLog, voidWarn } from '../log.ts';
2323
import { scanWranglerDotenvKeys } from '../shared/dotenv.ts';
@@ -1424,6 +1424,32 @@ export function createDefaultRunWrangler(
14241424
killProcessGroup(-child.pid, signal); // negative pid → the whole group
14251425
return 'signaled';
14261426
}
1427+
if (isWindows && typeof child.pid === 'number') {
1428+
// Windows has NO process groups, so the signal target is the child ITSELF.
1429+
// Consult Node's own bookkeeping FIRST: once the exit has been observed there is
1430+
// nothing left to signal. This ORDERING is also what makes the pid-based call below
1431+
// free of any pid-reuse hazard. Windows destroys a pid only when the process OBJECT
1432+
// is destroyed, which requires the process to have exited AND every handle to it to
1433+
// be closed; libuv closes its handle (`CloseHandle(handle->process_handle)` in
1434+
// `uv__process_proc_exit`, win/process.c) in the SAME straight-line frame that then
1435+
// invokes `exit_cb` — Node's `onexit`, whose first statements set
1436+
// `exitCode`/`signalCode`. No JavaScript runs in between, so a pid observed here
1437+
// with a null exit code is still pinned by libuv's open handle and cannot have been
1438+
// reassigned. (`child.kill()` is no safer in that instant: libuv sets
1439+
// UV_HANDLE_ESRCH on the very next line and `uv_process_kill` then short-circuits to
1440+
// ESRCH, which `ChildProcess.kill` reports as the same ambiguous `false`.)
1441+
if (child.exitCode !== null || child.signalCode !== null) {
1442+
return 'gone';
1443+
}
1444+
// Routed through the SAME seam POSIX uses, because `process.kill` THROWS ESRCH
1445+
// (classified "already gone" below), whereas `child.kill()` collapses ESRCH and
1446+
// EPERM into one silent `false`. Node sets `exitCode`/`signalCode` asynchronously
1447+
// in its `exit` handler, so a child that died a moment ago still reads as
1448+
// "running" here — and the ambiguous `false` would then be reported as a FAILED
1449+
// delivery, quarantining the lock over a child that had simply already exited.
1450+
killProcessGroup(child.pid, signal);
1451+
return 'signaled';
1452+
}
14271453
if (child.kill(signal)) {
14281454
return 'signaled';
14291455
}
@@ -1447,11 +1473,31 @@ export function createDefaultRunWrangler(
14471473
if (groupGone) {
14481474
return true;
14491475
}
1450-
if (isWindows || typeof child.pid !== 'number') {
1451-
// No process-group concept — `close` is the only (and sufficient) confirmation.
1476+
if (typeof child.pid !== 'number') {
14521477
groupGone = true;
14531478
return true;
14541479
}
1480+
if (isWindows) {
1481+
// No process GROUP to probe, but the child ITSELF is still confirmable: libuv
1482+
// implements `kill(pid, 0)` on Windows as a real liveness check
1483+
// (GetExitCodeProcess + WaitForSingleObject). Descendants are covered separately —
1484+
// libuv assigns every non-`detached` child to a global Job Object with
1485+
// KILL_ON_JOB_CLOSE, so they die with this process. Prefer Node's own bookkeeping
1486+
// (immune to pid reuse) and fall back to the signal-0 probe while the exit has not
1487+
// been observed yet — so confirmation stays EARNED rather than assumed.
1488+
if (child.exitCode !== null || child.signalCode !== null) {
1489+
groupGone = true;
1490+
return true;
1491+
}
1492+
try {
1493+
process.kill(child.pid, 0);
1494+
} catch (error) {
1495+
if ((error as NodeJS.ErrnoException).code === 'ESRCH') {
1496+
groupGone = true;
1497+
}
1498+
}
1499+
return groupGone;
1500+
}
14551501
try {
14561502
process.kill(-child.pid, 0);
14571503
} catch (error) {
@@ -2259,7 +2305,11 @@ function assertQuarantineClearedOrClear(lockTarget: string, onBeforeClear?: () =
22592305
typeof recorded?.quarantinedAt === 'string' ? recorded.quarantinedAt : undefined;
22602306
// Prefer the recorded GROUP (negative-pid probe); fall back to the plain pid where no
22612307
// group was recorded (Windows). No probe target at all → cannot verify → fail closed.
2262-
const probeTarget = pgid !== undefined ? -pgid : pid;
2308+
// A NEGATIVE target is POSIX-ONLY: on win32 `process.kill(-n, 0)` resolves to
2309+
// OpenProcess((DWORD)-n), which fails with ERROR_INVALID_PARAMETER and surfaces as ESRCH —
2310+
// i.e. it would report ANY recorded owner "confirmed dead" and clear a LIVE quarantine,
2311+
// inverting this gate from fail-closed to fail-open. Mirrors the watchdog's guard above.
2312+
const probeTarget = pgid !== undefined && process.platform !== 'win32' ? -pgid : (pid ?? pgid);
22632313
let confirmedDead = false;
22642314
if (probeTarget !== undefined) {
22652315
try {
@@ -2354,6 +2404,20 @@ function loadProperLockfileInternals(): ProperLockfileInternals | undefined {
23542404
}
23552405
}
23562406

2407+
/**
2408+
* `realpathSync` is `node:fs`, so it returns a NATIVE path — backslashes on Windows —
2409+
* while every other path in this module comes from `pathe` and is POSIX-normalized,
2410+
* including the not-yet-created branch below (whose `join()` already normalizes). Without
2411+
* this, `canonicalizeConfigPath` hands back TWO different spellings of the same file
2412+
* depending only on whether it happens to exist yet, defeating the "ONE identity" contract
2413+
* the function exists to provide — and splitting the lock identity, the wrangler `-c` argv,
2414+
* and the paths printed in error messages. No-op off Windows, where `\` is a legal filename
2415+
* character that must survive byte-for-byte.
2416+
*/
2417+
function toPosixConfigPath(nativePath: string): string {
2418+
return process.platform === 'win32' ? normalize(nativePath) : nativePath;
2419+
}
2420+
23572421
/**
23582422
* Canonicalize an already-absolute config path via `realpath` so the lock identity
23592423
* and every read/write/`-c` key on the REAL file, not a lexical symlink alias. When the
@@ -2366,7 +2430,7 @@ function loadProperLockfileInternals(): ProperLockfileInternals | undefined {
23662430
*/
23672431
function canonicalizeConfigPath(absoluteConfigPath: string, depth = 0): string {
23682432
try {
2369-
return realpathSync(absoluteConfigPath);
2433+
return toPosixConfigPath(realpathSync(absoluteConfigPath));
23702434
} catch (error) {
23712435
// ONLY ENOENT (a path component does not exist yet) is the benign "not created yet" case
23722436
// we handle below. Any OTHER realpath error (EACCES, ELOOP for a real symlink cycle,

‎packages/void/test/integration/cloudflare-deploy-build.test.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
22
import spawn from 'cross-spawn';
3-
import { join } from 'pathe';
3+
import { join, normalize } from 'pathe';
44
import { afterAll, beforeAll, describe, expect, it } from 'vite-plus/test';
55
import {
66
computeDesiredCloudflareResources,
@@ -167,7 +167,11 @@ describe('playground/kitchen-sink --backend cloudflare real-build deploy guard',
167167
const pointer = JSON.parse(
168168
readFileSync(join(PG, '.wrangler', 'deploy', 'config.json'), 'utf-8'),
169169
) as { configPath?: string };
170-
expect(pointer.configPath).toBe('../../dist/ssr/wrangler.json');
170+
// The CF plugin writes this with node:path.relative, so the separator is PLATFORM-NATIVE
171+
// (`..\..\dist\ssr\wrangler.json` on Windows). The guard that consumes it resolves through
172+
// pathe, which normalizes backslashes, so compare the normalized form rather than pinning
173+
// a POSIX literal that only holds on one platform.
174+
expect(normalize(pointer.configPath ?? '')).toBe('../../dist/ssr/wrangler.json');
171175
});
172176

173177
it('merges the pinned account, worker name, and real binding ids into the emitted artifact', () => {

‎packages/void/test/unit/cloudflare-deploy.test.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
2+
import { pathToFileURL } from 'node:url';
23
import { log } from '@clack/prompts';
34
import { dirname, join } from 'pathe';
45
import { afterEach, describe, expect, it } from 'vite-plus/test';
@@ -103,9 +104,18 @@ function emittedVars(vars: Record<string, unknown> = {}): EmittedWorkerVarsResul
103104
return { ok: true, vars };
104105
}
105106

106-
/** Absolute src paths for runtime imports inside spawned-child env.ts fixtures. */
107-
const srcEnvPublic = join(import.meta.dirname, '..', '..', 'src', 'runtime', 'env-public.ts');
108-
const srcEnvHelpers = join(import.meta.dirname, '..', '..', 'src', 'runtime', 'env-helpers.ts');
107+
/**
108+
* `file://` URLs for the runtime imports inside spawned-child env.ts fixtures. These are ESM
109+
* SPECIFIERS, not filesystem paths: an absolute Windows path (`D:/…`, as `pathe` spells it)
110+
* parses as a URL whose protocol is `d:`, so Node rejects it with
111+
* ERR_UNSUPPORTED_ESM_URL_SCHEME and the child's env.ts never evaluates — the probe then
112+
* reports `schema-error` for reasons that have nothing to do with the schema.
113+
* `pathToFileURL` is the same form `loadUserEnvFile` uses for real user env files.
114+
*/
115+
const srcFileUrl = (...segments: Array<string>): string =>
116+
pathToFileURL(join(import.meta.dirname, '..', '..', 'src', ...segments)).href;
117+
const srcEnvPublic = srcFileUrl('runtime', 'env-public.ts');
118+
const srcEnvHelpers = srcFileUrl('runtime', 'env-helpers.ts');
109119

110120
/**
111121
* Wrangler's FULL credential env-var set, canonical names + deprecated `CF_*` aliases.

0 commit comments

Comments
 (0)