Repository navigation
test(system-one): verify resident PID teardown and preserve Windows evaluator subprocess - #302
Conversation
VerificationCI run #801 completed 12/12 successful on commit Relevant evidence for #284:
The fix does not skip Windows tests and keeps RS12 as the evaluator CLI/process-boundary coverage while RS13 directly exercises the acceptance/evaluator contract. |
dyphn1
left a comment
There was a problem hiding this comment.
Strict re-review: I would not merge this yet.
P1 — RS13 currently avoids the crashing boundary instead of bounding/fixing the supported-runtime crash
#284 reproduces specifically on Windows + Node 24.15.0 with exit code 3221226505 / 0xC0000409, while CI already passed before this PR. There is now strong upstream evidence that Windows Node 24.15.0 has native/libuv failures with the same 0xC0000409 signature around network/process teardown:
- nodejs/node#63620 — Windows Node 24.15.0, 0xC0000409 stack-buffer-overrun in the TCP connect path; 24.16.0 appears clean.
- nodejs/node#56645 — Windows libuv shutdown assertion with exit code 3221226505.
- nodejs/node#64322 — Windows Node 24.x process-exit libuv assertion.
This repository declares Node >=22, so 24.15.0 is currently inside the supported engine range. Changing RS13 from a real evaluator child process to an in-process call can make the test green without proving that the original supported-runtime failure is gone. RS12 exercises one evaluator CLI invocation, but it does not reproduce RS13's repeated acceptance/evaluator scenario or prove the 24.15.0 crash boundary is safe.
Before closing #284, do one of:
- reproduce the original RS13 path on Windows Node 24.15.0 and demonstrate the real fix, or
- explicitly classify Windows Node 24.15.0 (or the confirmed affected range) as a known-bad runtime with a visible guard/version requirement, while keeping the subprocess coverage intact.
Do not use the current floating node-version: 24 CI result as proof: the issue already stated that CI passed, and that lane now resolves to a newer 24.x runtime.
P2 — cleanup waits for state-file removal, not resident-process termination
The new cleanup waits until the state file disappears and then retries rmSync. In the Python server, the state file is unlinked during teardown before the process has necessarily fully exited. Therefore the test can pass once filesystem handles are released even if the detached resident process is still alive briefly (or leaked).
For a Windows-stability fix, capture the resident PID before shutdown and verify the process is actually gone (Windows-specific PID-existence check is sufficient; keep the existing POSIX zombie caveat in mind), then remove the directory. Add a focused teardown regression/stress case that proves repeated start/stop leaves neither live resident PIDs nor undeletable fixture directories.
P2 — current CI does not distinguish old behavior from the fix
CI run #801 is green, but the original #284 explicitly says Windows CI was already green on the failing main revision. The PR needs evidence that would have failed before the change and passes after it: either an exact-version Windows 24.15.0 reproduction/guard test for RS13 and a deterministic/stress teardown test for the EPERM race.
Merge hygiene
The branch is also currently 1 commit behind main (0.30.8 release commit). Rebase/update after the blockers above are fixed and rerun the final matrix against current main.
#243 cleanup was separately re-audited and looks correctly closed: Phase 1–3 plus the parser regression are on main, and the remaining live behavioral evidence is still tracked by #253.
Refined review finding — previous upstream comparison correctedAfter checking Node's actual fixes, the RS13 evidence is clearer. Correction: nodejs/node#63620 is not the primary match; it is a separate TCP-connect stack-corruption issue. The close match is nodejs/node#56645 / #64322, fixed by nodejs/node#61999. That patch prevents The #284 machine was on Node 24.15.0, so it definitely predates that upstream fix. This makes the current RS13 change (subprocess -> in-process Requested changes
We cannot prove from the original #284 evidence alone that its native frame is exactly |
Verification follow-up (head
|
a55a449 to
56c6cc8
Compare
Resolution / final verification for #284Both review blockers are addressed on PR head
The exact native frame of the original v24.15.0 crash was not retained, so this closes the practical reproducibility/compatibility gap without claiming the original native stack was proven. PR is left open for normal merge/review. |
Remaining runtime-policy scopeOne residual compatibility question: upstream nodejs/node#56645 reports the same Windows libuv assertion on Node 23.x and lists reproductions on 23.0.0, 23.4.0, and 23.6.0. The new The repo currently declares |
Node 23 runtime-policy gap — resolved in
|
Summary
Fixes #284's test-harness teardown race while retaining the RS13 child-process boundary.
evaluator.run().engines.node >=22.main(release 0.30.8).Validation
Latest CI: https://github.com/dyphn1/Harness-everything/actions/runs/37722476831 — run #804 completed SUCCESS, 15/15 jobs green on head
75ef983.Node 23.x is covered by explicit Windows-only guard without changing package-wide
>=22engines. The original crash's precise native stack frame is not confirmed.Resolves #284 when the complete matrix is green.