test(runtime-host): bound owned-Host lifecycle tests to kernel contracts - #4814
test(runtime-host): bound owned-Host lifecycle tests to kernel contracts#4814ggbdpq wants to merge 2 commits into
Conversation
Fixes the intermittent failures reported in apache#4776. The test-side analysis and fix shape are @UncertaintyDeterminesYou4ndMe's (proposed in apache#4784 and donated in the issue after that PR was withdrawn); this commit implements it so the findings are not lost. - The launch-owner exit test gated its assertion on the Client's own `connection.closed`, which the Client aborts after a 2 s unanswered liveness probe, so a merely busy Host resolved it while still running. The assertion now waits on the process itself with a 20 s bound derived from the kernel's `shutdownGraceMs` contract, below the launcher's new 60 s idle grace. - The owned launch fixture's `idleGraceMs` (10 s) could expire mid-test and let an idle exit masquerade as an owner-loss exit; it now sits at 60 s with an explicit `initialConnectionTimeoutMs`. - "Exits promptly" now asserts shutdown start (the kernel's published `draining` registration) separately from shutdown completion, and the settle bound (15 s) sits above the kernel's own 10 s grace instead of inside it, so a starved-but-clean Host no longer reports an unclean exit. Fixes apache#4776 Generated-by: GLM-5.3-Flash (ZCode)
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head bc509b754bf9d2aa229c02c466e6e0fbcf83f80d (OPEN). Technical GO — no P0–P3. Test-only change; no product code touched. It fixes a test that passed without proving anything.
What was wrong with the old assertion
The old test killed the launcher process, then awaited connected.connection.closed as proof that "the candidate exited with its launch owner". That proof does not hold: the closed event is the client's own transport, which the client aborts itself after two seconds of unanswered liveness probes. So a merely busy but perfectly alive host would also fulfill the promise — the assertion could go green with the host never exited.
Why the fix is right
The order is inverted: first waitForProcessExit(launchedPid, 20_000) waits for the process to actually be gone — the only signal that proves it — and only then asserts the connection closure, with the failure text rewritten to match what is now really verified. The 20s bound is derived, not picked: DEFAULT_SHUTDOWN_GRACE_MS = 10_000 (host-kernel.ts:101, default confirmed at :248), so 10s is the legitimate-exit ceiling and 20s keeps a 2× margin. The fixture changes are the配套 adjustment, not a casual timeout bump.
What I could not judge
The fixed tests were not actually run here, so "no longer flaky" is inferred from timing, not observed; and whether 60s/20s/10s are wide enough on slow CI machines cannot be judged without CI timing data.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Opus-Qronos-AstroHan 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移、以及 exact-head 的 CI 状态。当前 head 是 bc509b7,未关闭。修的是一个通过了但没验到东西的断言,技术上无阻断问题。
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head e778e0e931a0cc811929f1b283d7d6940935106f (OPEN). Technical GO — no P0–P2, one P3 plus one maintainability note below. Test-only change (3 files, no production code), required test check green on this head.
What it fixes
The old test "proved" a Candidate exited with its launching owner by waiting on connected.connection.closed after killing the launcher. That proof is invalid: connection.closed is the client's own transport, which the client aborts after ~2 s of failed liveness probes — a merely busy but alive host would also satisfy it. The fix inverts the order: first waitForProcessExit(launchedPid, 20_000) on the real signal (process gone), then assert the connection closed, with the failure message rewritten to match what is actually verified. The 20 s bound traces to a real constant: DEFAULT_SHUTDOWN_GRACE_MS = 10_000 (host-kernel.ts:101, default applied at :248), so 20 s is 2× the legitimate-exit ceiling and below the 60 s idle grace — a genuine margin, not a guess.
P3 — the new "registry gone" assertion can pass for the wrong reason
undefined has two sources: the registry record truly deleted (the intended signal), or any read throwing mid-flight (permissions, torn file, JSON parse failure). The second case lets the assertion pass for the wrong reason — and torn reads are most likely exactly on loaded machines, the scenario this test hardens. Suggest distinguishing "file not exists" (ENOENT-class) from "read failed": only the former counts as shutdown-in-progress; other errors should keep polling or fail outright.
Maintainability note — the 20 s / 60 s coupling lives only in comments
The 20 s bound has discriminating power only because the fixture's idle grace is 60 s — it must sit above legitimate exit and below idle exit. If someone later trims the fixture to 10 s for speed, host-kernel.test.ts will silently lose its power without failing. Consider making the coupling code (fixture exports the constant, the kernel test derives its bound and asserts), not prose.
What I could not judge
Not run on a loaded machine, so "this removes the flakiness" is unproven — what is proven is that the assertion now verifies the right object with bounds traceable to real constants. True validation needs repeated CI runs.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Opus-Qronos-AstroHan 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移。当前 head 是 e778e0e,未关闭。修的是会骗人的测试断言,技术上无阻断问题,另有一条小的诚实性建议和一条数字耦合提醒。
jackwener
left a comment
There was a problem hiding this comment.
Reviewed exact head e778e0e931a0cc811929f1b283d7d6940935106f. One P2 remains.
P2 — the 20-second exit deadline excludes a legitimate pre-bind recovery phase
The new comment says 20 seconds is above every legitimate exit because shutdown has a 10-second deadline. That deadline does not start when this test kills the launcher.
runExecutionCandidateEntry() binds the launch-owner guard only after startExecutionRuntimeHostCandidate() returns (candidate-entry.ts:70-105). Meanwhile, the kernel publishes a listening recovering Host and accepts the launch-owner Client before composition.recover() settles (host-kernel.ts:362-395). Therefore retryConnect() can succeed and the test can kill the launcher before the guard has a closeHost callback. The guard records the lost owner, but it cannot call host.close() until candidate startup eventually returns and bind() runs. Only that later host.close() starts the kernel's 10-second shutdown deadline.
The actual bound from launcher death is consequently:
remaining composition startup/recovery (no deadline) + shutdownGraceMs (10 s)
I verified this with a mutation through the existing composition dependency seam: delaying a valid recovery by 25 seconds makes this exact test fail after 20.87 seconds with process ... did not exit. After restoring the exact head, both changed lifecycle tests passed in three consecutive runs. This means the patch improves the common case but can still report a false failure when recovery is slow—the loaded-machine condition it is intended to harden—and the PR body's contract claim is not true.
Please synchronize the test on a deterministic candidate-won / guard-bound signal before killing the launcher and starting the exit clock. If the pre-bind owner-loss case itself must be tested, control that startup phase explicitly and release it before starting the shutdown assertion. Merely increasing 20 seconds would preserve the same unsupported assumption.
The separate registration-read issue reported in the preceding review also remains: broad .catch(() => undefined) must not turn arbitrary read failures into proof that shutdown started.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
Fixes #4776 (intermittent owned-Host lifecycle failures). The root-cause analysis and the fix shape are @UncertaintyDeterminesYou4ndMe's, proposed in #4784 and donated to the issue after that PR was withdrawn; this commit implements the test-side fix so the findings are not lost, with small adaptations to the current tree.
Three waits were bounded by numbers the kernel does not promise:
connection.closed— the Client aborts that transport after a 2 s unanswered liveness probe, so a merely busy Host resolved it while still running, and the exit budget started at a moment unrelated to shutdown. The assertion now waits on the process directly (waitForProcessExit(pid, 20_000)), a bound derived from the kernel contract: owner loss cannot close a composition before startup settles, and the following shutdown is bounded byshutdownGraceMs(10 s). 20 s sits above every legitimate exit and below the launcher's new 60 s idle grace, so an idle exit cannot satisfy it.idleGraceMs: 10_000, which could expire mid-test and let an idle exit masquerade as an owner-loss exit. It now runs at 60 s with an explicitinitialConnectionTimeoutMs: 10_000so a Candidate no Client ever reaches still exits on its own.settlebound sat inside the kernel's own 10 sshutdownGraceMs— a starved-but-clean Host was force-terminated and reportedfalse !== true. The test now asserts shutdown start separately (polling the control directory until the registration disappears or reportsdraining, the state the kernel publishes before any shutdown work) and then settles at 15 s.Verification
node --test --test-name-pattern=…on both dist tests (fresh build)node --test owned-candidate.test.jsnode --test host-kernel.test.jsanswers an admitted bootstrap with draining after shutdown commits(EPIPE)npm run format:checkHonest limits: the full Runtime Host suite was also started on this Windows machine and stalled after ~350 tests on a file unrelated to this diff (an environment issue worth its own investigation; the UDS-inspect tests adjacent to the stall are Windows-skipped). The authoritative full-suite result will come from CI on Linux/macOS. The flake itself was reported on macOS arm64; per the issue's data (2/10 and 1/10 focused failures) the tightened bounds remove both timing holes the analysis identified.
AI use
Implemented with ZCode (GLM-5.3-Flash) from @UncertaintyDeterminesYou4ndMe's donated analysis and #4784 patch, adapted to the current tree and re-verified. The commit carries the
Generated-bytrailer.Checklist
shutdownGraceMs, idle grace), not magic numbers