test(runtime-host): bound owned-Host lifecycle tests to kernel contracts - #4814
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.
|
Merged current The exit assertion passes on the merged head locally (Windows), alongside the remaining changed lifecycle tests. Marking this P2 resolved by the upstream rework rather than by a bound change — happy to re-derive the bound if a maintainer prefers it anchored to the new mechanism explicitly.
中文:已并入当前 main(含上游 startup/shutdown 重构)并在合并后的 head 上重放了你的 25s recovery 延迟 mutation:Candidate 在 launcher 被杀后约 1s 退出,远在 20s bound 内,pre-bind 假失败不再复现,20s bound 维持原样。此 P2 由上游重构解决而非改 bound,如维护者希望 bound 显式锚定新机制可以再推导。 |
…gistration read errors Follow-up to the P2 review on apache#4814. Two changes: - The owner-loss exit test kills the launcher before the launch-owner guard binds, so the remaining recovery time was part of its exit bound with no kernel deadline. A test-only entry now delays composition creation by a fixed 5 s, the kill lands inside that pinned window, and the deadline becomes delay + shutdownGraceMs + margin. The comment derives the bound instead of asserting it. - waitForHostShutdownStart swallowed every readHostRegistration error, masquerading I/O or decode failures as "shutdown started"; readHostRegistration already maps a missing file to undefined, so the unguarded await surfaces real errors. Generated-by: GLM-5.3-Flash (ZCode)
|
Follow-up, correcting my earlier reply: the "upstream rework eliminated the pre-bind window" conclusion does not hold. A control run with the same three-process shape but no Maka code shows the same abrupt grandchild disappearance on Windows, so the fast local exit was not evidence about the kernel at all; the source still binds the guard only after startup returns ( The test now makes the scenario deterministic instead of relying on that artifact: a test-only entry ( Pushed as 中文:更正上一条回复——"上游重构消除了 pre-bind 窗口"不成立:无 Maka 代码的三进程对照在 Windows 上同样瞬间消失,本地快速退出不是内核行为证据(guard 仍在启动返回后才绑定)。测试改为确定性方案:test-only 入口把恢复窗口钉在 5s,kill 落在窗口内(仍是 pre-bind 场景),deadline=延迟+shutdownGraceMs+余量;另删掉 owned-candidate.test.ts 里吞注册读取错误的 catch(ENOENT 在 readHostRegistration 内部已映射为 undefined)。新 head a60a2df 本地(Windows)通过;跨平台结论仍以 CI 为准,欢迎在新 head 上复审。 |
format:check on CI wants waitForProcessExit on a single line; the previous round verified build and tests but skipped the format gate. Generated-by: GLM-5.3-Flash (ZCode)
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for correcting the Windows control result and preserving registration-read errors. The exit observations now follow the production owner, but the fixed delay does not make the pre-bind interleaving deterministic. The remaining P2 is inline. #4566 and #4661 do not introduce a demonstrated contract conflict with these observations.
AI-assisted review at 2ee447b0f: focused runs and a removal/control experiment were executed by a fresh reviewer; the coordinating Codex agent checked the described production ordering.
… signal The P2 follow-up on apache#4814: a fixed 5 s sleep cannot decide whether the kill lands before the launch-owner guard binds — a test-side pause longer than the sleep silently turns the scenario post-bind, and the sleep is additive to the real composition startup rather than a cap on it. Replace the fixed delay with an explicit gate: the test-only entry now parks composition creation behind a release file and writes a stall marker when it reaches the gated window. The test waits for that marker (so the kill provably lands pre-bind), kills the launcher, releases the gate, and asserts the OS process exits within shutdownGraceMs + margin — observing the real PID rather than the kernel's `closed` promise. The registration-read error handling from the previous commit is kept. Generated-by: GLM-5.3-Flash (ZCode)
…ine form Generated-by: GLM-5.3-Flash (ZCode)
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for replacing the fixed delay with an explicit gate and removing the swallowed registration errors. Those earlier requests are addressed at a0c4dba.
One P2 remains in the timing contract (inline): the test begins its process-exit budget before real startup/recovery finishes. Please move the synchronization boundary so the assertion measures the lifecycle phase it claims to bound. This is a test-oracle issue, not a request to add another Host lifecycle authority or to fix the separate TLS investigation from #4776 here.
AI-assisted review, independently reconciled against the production startup/owner-guard chain. CI is green; the slow-start counterexample is source-derived, not a locally executed test.
中文
感谢用显式 gate 替换固定延迟,并移除吞掉 registration 错误的处理;旧意见已解决。仍有一个 P2:真实启动/恢复结束之前就开始计算退出预算,请把同步点放到所断言的生命周期阶段边界。这是测试判据问题,不需要新增 Host 生命周期权威,也不要求在这里修 #4776 的独立 TLS 调查。已独立对照生产链路复核;CI 绿色,慢启动反例来自源码推导,未在本地运行。
…d boundary Review follow-up on apache#4814 (Astro-Han P2): the gated-recovery fixture parks composition creation behind the release marker, so real composition creation and recovery run after the release — inside the 20-second exit budget — while the launch-owner guard only binds once startup returns. A legitimately slow startup (25 s) would exhaust the assertion before the owner-loss shutdown it claims to bound even begins; the kernel's 10 s shutdown grace does not constrain that startup phase. The fixture's `onWon` hook now writes a bound marker: `candidate-entry` fires it immediately after `launchOwnerGuard.bind()`, so the marker is an explicit guard-bound boundary rather than a Client-transport signal (a liveness-aborted `connection.closed` can resolve while a busy Host still runs). The test releases the gate, waits for that marker, and only then starts the 20-second real-PID exit budget; the shutdown now has the full grace-plus-margin the deadline documents. The stall/kill pre-bind window is unchanged, so the regression this test pins still holds. Generated-by: GLM-5.3-Flash (ZCode)
|
Pushed c10d35c: the exit budget now starts at an explicit guard-bound boundary. The gated-recovery entry's One local-run caveat kept honest in the comment: on Windows the Candidate can be terminated abruptly the moment its launcher dies — no JS exit event, so no bind and no marker — so the wait races the marker against |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the update. At c10d35c48, the previous timing finding is addressed: onWon follows the real owner-guard bind, and the process-exit budget now starts after that boundary. The real PID assertion remains, registration errors are no longer swallowed, and the fixture still runs the production startup/recovery path. No remaining P0–P2 findings.
One optional P3: the comments claim deterministic pre-bind disconnect coverage, but observing the launcher's exit does not itself prove the Candidate has processed its disconnect callback. Please narrow that claim unless it is separately observed; the current test still usefully verifies owner-loss exit.
AI-assisted independent source review and primary reconciliation. Current-head CI is green; I did not rerun the Host subprocess suite locally.
中文
感谢更新。c10d35c48 已修复旧计时问题:onWon 在真实 owner guard 绑定后触发,退出预算从该边界后开始;真实 PID 断言、registration 错误传播和生产启动/恢复链均保留,无剩余 P0–P2。
可选 P3:注释把 pre-bind disconnect 覆盖说得过强,launcher 退出本身不证明 Candidate 已处理 disconnect 回调。若没有单独观察,建议收窄这句说明;当前测试仍有效验证 owner 丢失后退出。
经 AI 独立源码评审及主审裁定。当前 CI 绿色;未在本地重跑 Host 子进程套件。
…cts (apache#4814) Make owned-Host lifecycle tests observe the lifecycle phase they assert, instead of treating a client transport timeout as process shutdown. Use an explicit startup gate and guard-bound marker before the real-PID exit budget, separate shutdown start from completion, and keep idle expiry outside the owner-loss assertion window. Test-only changes reuse the production candidate and recovery path. Current-head CI passed. The test-side analysis and original fix shape were contributed by @UncertaintyDeterminesYou4ndMe in apache#4784 for apache#4776. Refs apache#4776 Generated-by: GLM-5.3-Flash (ZCode)
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