fix(ingest): make the code clause of the vm guard load-bearing, and guard the teardown properly - #649
Open
josephismikhail wants to merge 6 commits into
Open
fix(ingest): make the code clause of the vm guard load-bearing, and guard the teardown properly#649josephismikhail wants to merge 6 commits into
josephismikhail wants to merge 6 commits into
Conversation
…uard the teardown properly Review of #621, which merged at its first commit. Six findings, all real. MEDIUM -- the `code` clause I added had ZERO coverage. The real vm error carries both a matching code and a matching message, so `||` short-circuits on the message and the code branch is never reached: replacing the whole predicate with the message-only test left the file 4/4 green. That is the pre-#613 form, so a later "simplification" back to it would ship green, and then Node rewords the message -- the exact hazard the code check exists for -- and the fallback silently stops firing. The test file claimed the gate was protected "in either direction"; it was protected in one. There is now a case only the code clause can satisfy: the real error with its real code and the message replaced by something that does not match. Derived from a genuine failure, like the others. Both clauses are mutation-validated now -- deleting either one fails a different test. LOW -- the `backend?.` optional chain added last round was inert. The scenario it names is a `beforeEach` that throws before assigning `backend`, but the unguarded `backend.unknownPaths` dereference is two lines ABOVE it and throws the TypeError first. Guarded where it actually runs. LOW -- `home` and `repo` were not guarded the way `backend` was, and `force: true` does not cover it: it suppresses ENOENT, not the argument type check, so `rmSync(undefined, ...)` throws ERR_INVALID_ARG_TYPE. On a first `beforeEach` that fails inside `mkdtempSync` that raised a second, unrelated error and skipped the `repo` removal entirely -- leaking the tree in precisely the case cleanup exists for. LOW -- the decoupling comment claimed protection against a `stop()` that "outruns the hook timeout". It does not: on a hook timeout the promise is rejected from outside while the `await` is still pending, so no `finally` runs at all. The comment now claims only what it delivers, and the real fix is in `stop()`, which can no longer hang: it calls `closeAllConnections()` before `close()` and races a 2s deadline, because `close()` waits for every open socket and one stray keep-alive was enough to leave the hook pending. LOW -- the loader's JSDoc had drifted onto the wrong function. The new block was inserted after the existing one rather than replacing it, so both attached to the predicate and `importModule` was left undocumented. LOW -- `(await realVmFailure()) as Error` then `original.message` would crash with an opaque TypeError in the scenario the file explicitly anticipates (a host that gains a dynamic-import hook returns null). Both sites assert it first. 16 tests pass across the two files; full suite 1736 passed with the 3 read-containment symlink failures that need Windows Developer Mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…eak audible Review of #649. Three findings; the first is a hole in the fix I added last round. MEDIUM -- the three teardown guards were inert for every test after the first. `home`, `repo` and `backend` are describe-scoped and `afterEach` never reset them, so from test 2 onward they held the PREVIOUS test's values -- which makes the guards decorative in precisely the scenario their own comments name. A `beforeEach` that throws inside `mkdtempSync` left `backend !== undefined` passing against the previous test's already-stopped fake, `stop()` closing an already-closed server, and both `rmSync` calls aimed at already-deleted paths, while the directory this test had just created leaked. They are cleared at the top of `beforeEach` now, and the fake is constructed before anything that can throw, so each test's teardown can only ever see its own state. That rather than `string | undefined`, which would have meant a non-null assertion at forty use sites for no extra safety. Proven rather than argued, with a throwaway probe of the same shape (since removed). What the failing test's `afterEach` observes: with the reset { home: "home-2", repo: "" } guard skips without it { home: "home-2", repo: "repo-1" } stale, acts anyway LOW -- when `stop()`'s 2s deadline won the race, the leak it was added to prevent happened silently: the server stayed listening, which is the open handle that keeps a vitest worker from exiting, and every later `afterEach` also paid 2s. Trading a hang for a leak is still right, but it has to be audible or it surfaces as a mysteriously slow file rather than a message. It now reports which branch won, and clears the timer when `close()` wins. LOW -- the new code-clause test assumed the real error's `code` matches without asserting it, in a file where every other premise is asserted. Test 2 in that same file exists because a module runner rethrows this failure under its own code, so the day that happens at the `realVmFailure()` level this would have failed as a bare "expected false to be true" pointing at the predicate rather than at the input that changed. 16 tests across the two files; full suite 1792 passed, the 3 failures being the read-containment symlink tests that need Windows Developer Mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…t the predicate reads Round-2 review of #649. Three low findings, all about comments claiming more than the code does -- which is the defect this PR keeps being about. The `if (home)` / `if (repo)` guards are inert, and the comment said they were the mechanism. They are not: the same commit clears both to `""` at the top of `beforeEach`, so neither is ever undefined by teardown, and `rmSync("")` is a silent no-op on Node 26 anyway -- checked, alongside `rmSync(undefined)`, which throws ERR_INVALID_ARG_TYPE. So the scenario the comment named could not occur and deleting the guards would change nothing. That mattered because the rationale was on the wrong half. A reader who trusted it would remove the "redundant" reset, leaving a stale path in the `rmSync` call, which no-ops under `force: true` while the directory the failed `beforeEach` just created leaks -- with nothing red. The reset now carries the reasoning; the guards say only what they are, which is a cheap explicit statement of intent that keeps the reset from looking redundant. `backend` is the same shape with the opposite asymmetry: it has no sentinel, so it is only correct because it is constructed before anything that can throw. Nothing said so, and a statement inserted above it would silently leave the teardown asserting against the previous test's fake and passing. That invariant is now written down where it can be violated. And the assertion meant to prove the message clause cannot carry the code-clause test read `reworded.message`, while the predicate reads `String(err)` -- they differ by the error's name prefix. A fixture that ever carried the phrase in its NAME would satisfy the predicate's message branch while that assertion still passed, and the test would go green without the code clause, reopening the exact gap it exists to close. It delegates to `String(reworded)` now, the same expression the predicate uses. Both clauses re-checked after the change and still load-bearing: dropping either one fails a different test. 16 tests across the two files; full suite 1792 passed with the 3 read-containment symlink failures that need Windows Developer Mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…wn guard stops being inert Review of #649 found this PR's own bug reproduced one level up, in the hook that fixes it. `afterEach` guarded its assertion with `if (backend !== undefined)`, and the comment justified keeping it by naming the failure it catches: "a throw introduced above the assignment ... the assertion would run against the previous test's fake and pass." In that exact scenario the guard does nothing. `backend` is declared `let backend: FakeBackend`, describe-scoped, and is only ever assigned a fresh fake -- never reset. So a statement inserted above the assignment that throws leaves `backend` holding the PREVIOUS test's fake, `!== undefined` is true, and the assertion runs against the wrong object and passes. The guard discriminates only on the very first test in the file. Executed rather than reasoned about, in a five-test file mirroring the hook structure, with test 2's `beforeEach` throwing above the assignment: OLD ran against fake-1, unknownPaths=[] OLD ran against fake-1, unknownPaths=[] <- test 2, stale fake, passes NEW ran against fresh-1, unknownPaths=[] NEW skipped <- test 2 The old guard asserts against `fake-1` twice. That is the silent pass, and it also confirms vitest runs `afterEach` after a `beforeEach` throw, which the whole guard depends on. `home` and `repo` already carry a sentinel -- the `""` reset at the top of `beforeEach` -- and `backend` was the one that did not. It gets the equivalent now: `backendIsCurrent`, cleared first and set immediately after the construction, so the window where the flag is false is exactly the window where `backend` is stale. `backend` itself cannot use `undefined` as its sentinel: it is read as a plain `FakeBackend` at ~35 call sites in the tests below, and widening the type would put a narrowing burden on every one of them for no gain in the hook. The stop is guarded on the same flag, replacing `await backend?.stop()`. Two comments went with it: The optional chain was justified by "a `beforeEach` that throws in `mkdtempSync` leaves it unassigned" -- false since `fde7e65` moved the construction ABOVE both `mkdtempSync` calls, and contradicted by the comment twenty lines up in the same hook. A reader trusting it would conclude the construction order is not load-bearing and move it back down, reintroducing the vacuous teardown with nothing red. And `String.fromCharCode(10)` is now `"\n"`, matching the literal in `fixture()` in this same file and 20+ writes in `ingest.ts`. It was an artifact of how the line was written, not a constraint, and reading as one sends the next person looking for a rule that does not exist. 16 tests in the two touched files, full suite 1792, typecheck and lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
…away from Second review round on #649. Five findings, and the sharpest one is that the sentinel I added last round is dormant: nothing between the reset and the assignment can throw, so swapping the guard for a literal `true` leaves the suite green. The commit message claiming it "stops being inert" was wrong and this one says so. What the flag buys is not coverage but correctness WHEN it fires, which is where it differs from the `!== undefined` it replaced -- that one was dormant AND could not have worked, because the stale fake is never undefined. The comment now states the dormancy outright rather than implying a test covers it. The same finding named a hole that IS reachable: the flag was never cleared on the way out, so it read `true` from the first test onward. A file- or project-level `beforeEach` added later that throws before this describe's own would find it set and assert against the stale fake -- the exact bug, through the one door the flag did not cover. It is cleared at the end of `afterEach` now, so it means "constructed for the test now running". And a real leak, fixed rather than re-documented. `realpathSync(mkdtempSync())` leaves `home` empty when the OUTER call throws -- the directory exists by then, so the `if (home)` cleanup skips a tree already on disk. The assignment is split, so the variable always names whatever was created: OLD home-dirs before=24 after=25 LEAKED=1 NEW home-dirs before=25 after=25 LEAKED=0 That is the two forms with `realpathSync` forced to throw after a successful `mkdtempSync`, which is the only way the failure is reachable. The guarded `rmSync` comment had credited itself with preventing a leak that could not happen, while this one sat two lines above it. Two comments stated things that are not true. The stop guard said a throw in `mkdtempSync` "leaves this test's own fake live and needing the stop" -- but `start()` runs BELOW those calls, so the fake never listened and `stop()` returns at its own `if (!this.server)`. And `FakeBackend.stop`'s timeout message said it was "leaving it open": `close()` releases the listening handle synchronously, so the port is free and what can still hold the worker is a socket opened after `closeAllConnections()`. The branch is also effectively unreachable -- that call destroys every socket first -- which the comment now says instead of implying it is a live hazard. Finally, `ingestion-loader.test.ts`'s premise assertion checked `String(code)` where the predicate requires `typeof code === "string"` before `startsWith`. A non-string that stringifies the same way -- the shape the NUMERIC-code test exists for -- passed the premise and failed two lines later as a bare `expected false to be true` against the predicate, which is the confusion the premise assertion was added to prevent. It pins `typeof` too. 16 tests in the touched files; full suite 1791 with the 3 known Windows symlink failures; typecheck and lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
Contributor
Author
The red check is a known flake, not this PR
This PR does not touch That test waits a flat 120ms for a worker Fixed in #651, which waits on the pool's own signal that |
…other Third review round on #649. The medium is the same principle the outer `finally` in this hook already argues for -- "the steps do not share fate" -- not applied to the inner one. Three statements sat in that block: remove `home`, remove `repo`, clear `backendIsCurrent`. `rmSync` with `force` suppresses ENOENT and nothing else, so one EBUSY or EPERM on `home` -- a handle under `IX_LOCK_DIR`, which is `home/locks`, or a `.git` handle, both ordinary on Windows -- threw and skipped the other two. That leaks the `repo` tree this block exists to remove, and leaves the flag `true`, reopening the exact door the reset was added last round to close. Measured, each arm with its own TEMP so the count is only what that run left: old shape (shared fate) + EBUSY on home repo dirs leaked: 11, tests 11 FAILED new shape (independent) + EBUSY on home repo dirs leaked: 0, tests 11 passed The second column is the half I had not thought about. In the old shape the teardown throw propagates and fails every test in the file, replacing whatever they were actually asserting -- including the `unknownPaths` failure this hook exists to report. So the shared fate did not merely leak; it destroyed the diagnosis too. `backendIsCurrent = false` now runs first, with nothing above it that can throw, and each removal goes through a `removeTree` helper that catches per directory and reports the leak on stderr rather than raising. Raising there would put the teardown's problem in front of the test's. Two smaller ones: `FakeBackend.stop`'s timeout branch was justified by "a socket opened AFTER `closeAllConnections()`". That cannot happen: `Promise.race` builds its array eagerly, so `server.close()` runs in the same synchronous tick and the listening handle is gone before any later turn of the loop. I invented that scenario last round. The branch stays as a backstop against `close()` never calling back at all, and now says so -- and stays a message rather than a throw, because the failure it replaces is a hook that never settles, which takes the hook timeout and runs NEITHER `finally`, leaking both trees and leaving the env pointed at a deleted home. `closeAllConnections?.()` loses its `?.`: `engines.node` is >=22 and the method landed in 18.2. And an attribution: the comment said a throw "inside `mkdtempSync`" leaked the directory the test had just created. A `mkdtempSync` throw creates nothing. The leak that was reachable came from `realpathSync` throwing after the directory existed, which is what the split assignment fixes -- the mechanism was right and that one sentence pointed at the wrong call. 16 tests in the touched files; full suite 1792; typecheck and lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #621, which merged at its first commit — these six review findings are not on
main.The one that matters: my
codeclause had zero coverage#621 widened the vm guard to
code || message, with tests. But the real vm error carries both a matching code and a matching message, so||short-circuits on the message and the code branch is never reached.Mutation-verified: replacing the whole predicate with the message-only test left the file 4/4 green.
That is exactly the pre-#613 form. So a later "simplification" back to it ships green — and then Node rewords the message, which is the precise hazard the code check was added for, and the fallback silently stops firing with nothing pointing at the guard. The test file claimed the gate was protected "in either direction". It was protected in one.
There is now a case only the code clause can satisfy: the real error, its real code, and the message replaced with something that does not match. Derived from a genuine failure like the others. Both clauses are mutation-validated now — deleting either fails a different test.
The teardown guards, which were in the wrong places
backend?.optional chain was inertbeforeEachthat throws before assigningbackend— but the unguardedbackend.unknownPathsdereference is two lines above it and throws firsthome/repounguardedforce: truesuppresses ENOENT, not the argument-type check:rmSync(undefined, …)throwsERR_INVALID_ARG_TYPE, raising a second unrelated error and skipping thereporemoval entirely — leaking the tree in exactly the case cleanup exists forstop()that "outruns the hook timeout". It does not — on a hook timeout the promise is rejected from outside while theawaitis still pending, so nofinallyruns at allThe last one is now fixed where it has to be:
stop()can no longer hang. It callscloseAllConnections()beforeclose()and races a 2s deadline, becauseclose()waits for every open socket and a single stray keep-alive was enough to leave the hook pending. The comment now claims only what it delivers.Two smaller ones
importModulewas left undocumented.(await realVmFailure()) as Errorthenoriginal.messagewould crash with an opaqueTypeErrorin the scenario the file explicitly anticipates (a host that gains a dynamic-import hook returnsnull). Both sites assert it first.Verification
ix-clisuite: 1736 passed; the 3 failures are theread-containmentsymlink tests that need Windows Developer Mode locally and pass on CI'swindows-2022runnertsc --noEmitandeslintclean🤖 Generated with Claude Code
https://claude.ai/code/session_01B5bkc5oUL4SapwDE13UgHt