Skip to content

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

Open
ggbdpq wants to merge 2 commits into
apache:mainfrom
ggbdpq:fix/host-lifecycle-test-bounds
Open

test(runtime-host): bound owned-Host lifecycle tests to kernel contracts#4814
ggbdpq wants to merge 2 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 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