Skip to content

test(runtime-host): bound owned-Host lifecycle tests to kernel contracts - #4814

Merged
Astro-Han merged 10 commits into
apache:mainfrom
ggbdpq:fix/host-lifecycle-test-bounds
Sep 8, 2026
Merged

test(runtime-host): bound owned-Host lifecycle tests to kernel contracts#4814
Astro-Han merged 10 commits into
apache:mainfrom
ggbdpq:fix/host-lifecycle-test-bounds

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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:

  1. Launch-owner exit test gated its assertion on the Client's own 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 by shutdownGraceMs (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.
  2. The owned-launch fixture ran with 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 explicit initialConnectionTimeoutMs: 10_000 so a Candidate no Client ever reaches still exits on its own.
  3. "Owned Host exits promptly" conflated shutdown start with shutdown duration, and its 5 s settle bound sat inside the kernel's own 10 s shutdownGraceMs — a starved-but-clean Host was force-terminated and reported false !== true. The test now asserts shutdown start separately (polling the control directory until the registration disappears or reports draining, the state the kernel publishes before any shutdown work) and then settles at 15 s.

Verification

Claim Command Result
Focused tests pass with the fix node --test --test-name-pattern=… on both dist tests (fresh build) both pass
owned-candidate file complete node --test owned-candidate.test.js 11 tests, 11 pass, 0 fail
host-kernel file complete node --test host-kernel.test.js 70 tests: 64 pass, 5 skipped, 1 fail — answers an admitted bootstrap with draining after shutdown commits (EPIPE)
That failure is pre-existing on Windows, not from this diff control run with the three changed files stashed, rebuilt, same focused test fails identically on the clean tree
Format npm run format:check exit 0

Honest 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-by trailer.

Checklist

  • Test-only diff; no production code changed
  • Analysis credit to the original finder in commit message and PR
  • New bounds derived from kernel contracts (shutdownGraceMs, idle grace), not magic numbers
  • Pre-existing Windows failures disclosed with control-experiment evidence

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)
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 4, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ggbdpq

ggbdpq commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged current main into this branch (upstream moved ~110 commits since your review, including the Runtime Host startup/shutdown rework around #4958) and re-ran your mutation on the merged head: a 25 s delay injected immediately before composition.recover() in host-kernel.ts — the same seam your review used — no longer reproduces the false failure. The Candidate exits ~1 s after the launcher is killed, well inside the 20 s bound, with the launch-owner Client still admitted before the guard binds. So the pre-bind lingering window no longer exists on this tree, and the 20 s bound stands unchanged.

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.

Automated findings above were re-verified against head 0a2dad08bf1f598aa02914b1dbf71178f8c5e754.


中文:已并入当前 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)
@ggbdpq

ggbdpq commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

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 (candidate-entry.ts:70-108, candidate-launch-owner-guard.ts:78-80).

The test now makes the scenario deterministic instead of relying on that artifact: a test-only entry (test-only/owned-candidate-delayed-recovery-main.ts) delays composition creation by 5 s, the launcher is killed inside that pinned window (owner loss pre-bind, the same scenario as before), and the exit deadline is OWNED_CANDIDATE_RECOVERY_DELAY_MS + shutdownGraceMs + margin — derived from contracts rather than asserted. Also removed a .catch(() => undefined) in owned-candidate.test.ts's waitForHostShutdownStart that masked real registration read errors (ENOENT is already mapped to undefined inside readHostRegistration).

Pushed as a60a2dfbd380a4fab8f598e2e23b9cceb77a3d4e; the rewritten test passes locally (Windows). As noted in the test comment, local Windows runs terminate such grandchildren abruptly without a JS exit event, so only CI verdicts count as cross-platform evidence for this assertion — re-review welcome on the new head.


中文:更正上一条回复——"上游重构消除了 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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

中文

退出观察与读取错误处理已修正;剩余问题是固定延迟并未锁定 pre-bind 顺序。未发现与 #4566/#4661 的已证契约冲突。

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.

Comment thread packages/runtime-host/src/test-only/owned-candidate-delayed-recovery-main.ts Outdated
… 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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 绿色,慢启动反例来自源码推导,未在本地运行。

Comment thread packages/runtime-host/src/__tests__/host-kernel.test.ts
@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Sep 8, 2026
…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)
@ggbdpq

ggbdpq commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Pushed c10d35c: the exit budget now starts at an explicit guard-bound boundary. The gated-recovery entry's onWon hook — which candidate-entry fires immediately after launchOwnerGuard.bind() — writes a bound marker, and the test releases the gate, waits for that marker, and only then starts the 20-second real-PID budget. A legitimately slow startup (composition creation and recovery included) can no longer eat the deadline, and the shutdown keeps its documented grace-plus-margin.

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 isProcessAlive; a live process without a marker past the deadline still fails, and cross-platform evidence stays with CI.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 子进程套件。

@Astro-Han
Astro-Han merged commit 5f87ca2 into apache:main Sep 8, 2026
1 check passed
testikun pushed a commit to testikun/maka that referenced this pull request Sep 9, 2026
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flaky(runtime-host): owned Host lifecycle tests intermittently fail to observe process exit

3 participants