Skip to content

ci(windows): fix the six remaining failures, then make the Windows leg permanent - #104

Merged
hristo2612 merged 2 commits into
hristo2612:mainfrom
e1010101:fix/windows-ci
Jul 29, 2026
Merged

ci(windows): fix the six remaining failures, then make the Windows leg permanent#104
hristo2612 merged 2 commits into
hristo2612:mainfrom
e1010101:fix/windows-ci

Conversation

@e1010101

Copy link
Copy Markdown
Contributor

The Windows leg is opt-in behind a windows-ci label because the repo could not be built on Windows at all. #97 fixed the build, #101 the ACL checks, and run 30317780975 narrowed what remained to six failures in four files. Those are fixed here, in the same change that makes the leg unconditional — so it arrives green rather than as a red box everyone learns to ignore, per your ordering.

The plan job is gone and the matrix is inlined, which is what its own comment prescribed once #97 and #101 landed.


Two suites held an open SQLite handle while deleting their temp home. browser-operator-authorization and workflow-provenance both initDb() and neither closed it. POSIX unlinks an open file; Windows refuses with EPERM, so rmSync threw out of afterAll and failed the suite for a reason unrelated to anything it tested.

Both now close the database first — that is the actual bug, not the removal. A shared removeTempDir() then adds Node's documented retry/backoff (maxRetries/retryDelay, which cover EBUSY/EMFILE/ENFILE/ENOTEMPTY/EPERM) for handles a test cannot close: a child process the OS has not finished reaping, for instance. The global teardown gets the same treatment — it was throwing the identical EPERM after every test had already passed, which is how a fully green run reports as failed.

activity/performance is skipped on Windows, measured rather than assumed. I raised the hook budget and ran it to find out what was actually there:

  • seeding the 100k-row corpus takes ~80s against a 60s hook budget, and
  • once past that, the filtered query lands at 343ms against a 300ms budget ({"firstMs":57.8,"middleMs":5.5,"filterMs":343.1,"searchMs":59.8}).

Both are the platform — sqlite durability behaviour plus Defender on the temp directory — not a regression. Relaxing wall-clock budgets to accommodate a slower machine leaves a guard that can no longer catch the regressions it exists for, and the numbers would be tuned to whichever machine happened to run it rather than to the runner. It stays enforced on ubuntu where it is calibrated, and the correctness assertions it also makes (totals, page sizes, the indexed-search hit) are platform-independent and covered there.

The three codex-rollout cases in engine-limits are skipped pending #103. Their stubs are #!/usr/bin/env node files made executable with chmod, and Windows has neither a shebang nor an executable bit, so spawn(bin, …) never starts them. As you noted in #103, fixing the stub alone would make them pass without touching the product bug underneath — so I would rather they come back with real Windows coverage via a .cmd stub, which only becomes possible once #103 is fixed. The skip names the issue.

The root postinstall no longer errors on every Windows install. Taken as tidiness, not as the prerequisite we both thought it was — you were right that the runner disproved that twice.

It is a behaviour-preserving port of the shell one-liner rather than a switch to packages/jinn/scripts/fix-node-pty-permissions.mjs: that script is darwin-only by design (process.platform !== "darwin" → exit 0), so pointing the root script at it would have silently stopped chmodding the Linux prebuild. Every prebuild directory is still visited, missing files and chmod failures are still non-fatal, and Windows exits early.


Verification. All four files pass or skip cleanly on Windows 11 / Node 24, and both packages typecheck.

One caveat worth stating plainly: my Windows box is a noisier oracle than the runner — a different subset of unrelated suites fails per full-suite run here (pty-stream's 30s timeout, callback-concurrent-init's "readonly database"), and neither has ever failed on windows-latest across the three runs so far. So the runner's verdict on this PR is the one that counts, and since this change makes the leg unconditional it will run on this PR itself. If anything is still red I will fix it before this merges — it should not land red.

e1010101 added 2 commits July 28, 2026 22:21
The leg was opt-in because the repo could not be built on Windows at all. hristo2612#97
fixed the build, hristo2612#101 the ACL checks, and run 30317780975 narrowed what was
left to six failures in four files. Those are fixed here, in the same change
that makes the leg unconditional, so it arrives green rather than as a red box
everyone learns to ignore.

Two suites deleted their temp home while still holding an open SQLite handle.
POSIX unlinks an open file; Windows refuses with EPERM, so the removal threw
out of afterAll and failed the suite for a reason unrelated to what it tested.
Both now close the database first, and a shared removeTempDir() adds Node's
documented retry/backoff for handles that outlive the code that owned them --
a child process the OS has not finished reaping, for instance. The global
teardown gets the same treatment; it was throwing the identical EPERM after
every test had already passed.

activity/performance is skipped on Windows, measured rather than assumed:
seeding its 100k-row corpus takes ~80s against a 60s hook budget, and once
that is raised the filtered query lands at 343ms against a 300ms budget.
Relaxing wall-clock budgets to fit a slower machine leaves a guard that can no
longer catch the regressions it exists for, and the numbers would be tuned to
whichever machine happened to run it. It stays enforced on ubuntu, where it is
calibrated; the correctness assertions it also makes are platform-independent
and covered there.

The three codex-rollout cases in engine-limits are skipped pending hristo2612#103. Their
stubs are '#!/usr/bin/env node' files made executable with chmod, and Windows
has neither a shebang nor an executable bit. The fix for hristo2612#103 is what makes a
.cmd stub possible, so they should come back with real coverage rather than a
weakened assertion.

The root postinstall no longer errors on every Windows install. It is
deliberately a behaviour-preserving port of the shell one-liner rather than a
switch to packages/jinn's fix-node-pty-permissions.mjs, which is darwin-only
and would silently drop the Linux chmod.
The last Windows-only failure. `callback-concurrent-init` starts 16 processes
against one registry and one of them died with "attempt to write a readonly
database" — roughly one run in five here, and once on the runner.

The stack puts it at `journal_mode = WAL` in initDb, which is already wrapped in
runSqliteBusyRetry. Setting WAL takes a brief exclusive lock to rewrite the
header, and when a peer holds the file at that instant SQLite reports
SQLITE_READONLY rather than SQLITE_BUSY, which the wrapper did not recognise.

So the retry now covers both classes, with a longer backoff on Windows, where
locks are released less promptly and the race is at process start where a few
hundred milliseconds cost nothing. A database that is genuinely read-only takes
the same bounded wait and then throws the identical error, so nothing is masked.

Not only a test concern: the gateway, the CLI and session workers all open this
database, and a Windows operator running any two at once was exposed to the same
race.

Measured on Windows 11 / Node 24: 1 failure in 5 runs before, 0 in 12 after.
@e1010101

Copy link
Copy Markdown
Contributor Author

Green, including the Windows leg, on run 30368772169: unit-tests (windows-latest) pass in 6m14s, alongside ubuntu, typecheck and build.

Since this PR makes the leg unconditional, that result came from the leg this PR turns on, running on this PR — no label, no approval. Green first, then flipped, in the order you asked for.

One of the six was not a test problem

The last failure took three iterations to reach and turned out to be a product bug. callback-concurrent-init starts 16 processes against one registry and one died with attempt to write a readonly database — about one run in five locally, and once on the runner.

The stack put it at journal_mode = WAL in initDb, which is already wrapped in runSqliteBusyRetry. Setting WAL takes a brief exclusive lock to rewrite the header, and when a peer holds the file at that instant SQLite reports SQLITE_READONLY rather than SQLITE_BUSY — a class the wrapper did not recognise. So the retry now covers both, with a longer backoff on Windows.

That is not confined to the test: the gateway, the CLI and session workers all open this database, so a Windows operator running any two at once was exposed to the same race. Retrying a database that is genuinely read-only takes the same bounded wait and throws the identical error, so nothing is masked. Measured 1 failure in 5 runs before, 0 in 12 after.

The other five

  • Two EPERM teardowns — a real bug in both suites, not a Windows quirk to tolerate: each opened a SQLite registry and never closed it before deleting its directory. POSIX unlinks an open file; Windows refuses. They close the database first now, and removeTempDir() adds Node's documented retry for handles a test cannot close, such as a child process the OS has not finished reaping. The global teardown had the same defect — it was throwing after every test had already passed, which is how a green run reports as failed.
  • activity/performance — skipped on Windows, measured rather than assumed. Seeding its 100k-row corpus takes ~80s against a 60s hook budget, and once that is raised the filtered query lands at 343ms against a 300ms budget. Relaxing wall-clock budgets to fit a slower machine leaves a guard that can no longer catch what it exists for, and the number would be tuned to whichever machine ran it. It stays enforced on ubuntu where it is calibrated.
  • The three engine-limits cases — skipped pending [Windows] Engine usage limits are unavailable: bare spawn/execFile of a .cmd shim #103, deliberately not "fixed" by making the stub cross-platform, since you pointed out that would paper over the product bug. fix(windows): spawn engine binaries that are .cmd shims #105 now gives them real Windows coverage instead: their stub becomes a .cmd, the shape npm actually installs, so they exercise the spawn path rather than asserting around it.

postinstall

Swapped, as tidiness only — you were right that the runner disproved it as a blocker, twice. It is a behaviour-preserving port rather than a switch to packages/jinn/scripts/fix-node-pty-permissions.mjs, because that script is darwin-only by design and pointing the root script at it would have silently stopped chmodding the Linux prebuild.

Ordering with #105

Both touch engine-limits.test.ts. If this merges first, #105 drops the skip on rebase; if #105 merges first, the skip here becomes a no-op to remove. Either order works. Worth noting #105 cannot be verified on Windows by CI until this lands, since it does not change the workflow and so only gets the ubuntu leg.

@hristo2612
hristo2612 merged commit d7c090a into hristo2612:main Jul 29, 2026
4 checks passed
e1010101 added a commit to e1010101/jinn that referenced this pull request Jul 29, 2026
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.
hristo2612 pushed a commit to e1010101/jinn that referenced this pull request Aug 3, 2026
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.
hristo2612 pushed a commit that referenced this pull request Aug 3, 2026
The fixed retry ladder spent 1.76s on Windows against measured contention of 3.5s, so concurrent processes racing initDb() gave up while the WAL header lock was still held. The ladder becomes a jittered exponential backoff under a time budget: 5s on POSIX, 15s on Windows. Jitter matters because peers that collide once would otherwise back off by identical amounts and collide again on every subsequent attempt.

The budget cannot truncate the previous ladder on either platform, so the SQLITE_READONLY handling added in #104 is unaffected: worst case moves from 260ms to 5s on POSIX and 1.76s to 15s on Windows, and even with every jitter draw at 0.5x the first six sleeps total ~315ms. A genuinely read-only database still throws the identical error, unwrapped, after a bounded wait.

The budget is measured with performance.now() rather than a wall clock, because this runs at process start and Atomics.wait blocks the thread: a backward clock step during the wait would extend a synchronous block by the size of the step, unbounded and unlogged.

This raises a ceiling rather than removing one, as the author states plainly. The fix without a ceiling is cross-process serialization of schema init, tracked in #121.

Thanks to @e1010101.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants