test: close active dev-up stub connections on signal - #2548
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Chris0Jeky
left a comment
There was a problem hiding this comment.
Independent coordinator review at exact head 6c10850e688a3c51d67107e2bdfdad1ea2934cd2: no CRITICAL/HIGH correctness, security, or data-loss findings.
The changed helper ordering is safe for the Node 24.13.1 engine contract: it stops new accepts with server.close() and then clears active HTTP connections with closeAllConnections(). The cross-platform source assertion passes, and the POSIX-only live test is appropriately isolated from Windows native-signal behavior. The focused test is green. No fix requested.
The full local launcher suite's Bash failures are the pre-existing Windows/Git-Bash timeout cohort tracked under #2378/#2159/#2161, not failures in this diff; the corresponding PowerShell cases passed.
|
Review (agent half of the ADR-0066 gate; Codex credits exhausted, SC-9): one fresh-context reviewer confirmed the stub teardown cannot hang (server.close then closeAllConnections; Node 24 pinned) or double-resolve (single process.exit path), the Linux/macOS regression is non-vacuous (an accepted-but-silent socket would keep close pending under the old helper) and cleans up in finally, and the launcher scripts are untouched. Verdict SHIP. MEDIUM worth a one-line change or follow-up: the readiness poll at ~1063 uses canBind, which LISTENS on the port every 50 ms while the child is racing to bind it — in success mode the helper rethrows EADDRINUSE, so a required-gate flake vector with no diagnostics (stdio ignore); poll with net.createConnection retrying on ECONNREFUSED instead. LOWs: the source-string ordering assertion is self-referential; the message overstates an ordering invariant; 10 s budget is the tightest in the file; stderr discarded. Overlap with parked #2522: helperSource stop path (~142-159) and the insertion point before the platform loop (~1033-1092). CI-control region: parked for the maintainer's review per ADR-0066 / SC-10. |
Summary
server.close()in the synthetic Node frontend/API servers.Refs #2135 #2111 #2112
Verification
node --check scripts/ci/dev-up.test.mjsnode --test --test-concurrency=1 --test-name-pattern="launchers encode" scripts/ci/dev-up.test.mjs— 1 passedgit diff --checkThe full local launcher suite reached 75 passed before the existing Windows/Git-Bash instability forced the suite watchdog: 3 unrelated tests failed and 4 were cancelled on Bash timeout-shaped failures (the same #2378 cohort); all exercised PowerShell cases passed. The new POSIX-only live-socket test is not runnable on this Windows host and is intended for the Linux/macOS lanes.
NOT verified