Skip to content

fix(runtime-host): fence State Root before service retirement - #3602

Merged
M4n5ter merged 1 commit into
apache:mainfrom
CuSO41108:codex/test-runtime-host-retirement-race
Aug 23, 2026
Merged

fix(runtime-host): fence State Root before service retirement#3602
M4n5ter merged 1 commit into
apache:mainfrom
CuSO41108:codex/test-runtime-host-retirement-race

Conversation

@CuSO41108

Copy link
Copy Markdown
Contributor

Summary

host.upgrade.prepare only starts the Runtime Host's asynchronous drain. Before
this change, the PID-known retirement path called backend.stop() immediately
after preparation.

If the prepared Host exited unexpectedly, systemd could restart the same service
unit with a successor Host. Stopping the unit at that point could stop the
successor instead of the prepared Host.

This change:

  • waits for the prepared Host to release the State Root;
  • acquires a retirement fence before calling backend.stop();
  • rechecks the service PID while the fence is unavailable;
  • fails closed when the Host identity changes or the fence deadline expires;
  • keeps the post-stop State Root check as a cleanup verification, rather than
    treating it as proof of continuous Host identity.

Fixes #3594

Verification

  • npm run build:test — passed.
  • npm --workspace @maka/core run build — passed.
  • npm --workspace maka-agent run build — passed.
  • npm --workspace maka-agent run typecheck — passed.
  • npx biome check packages/cli/src/runtime-host-service-manager.ts packages/cli/src/__tests__/runtime-host-service-manager.test.ts — passed.
  • git diff --check — passed.
  • Regression test for successor replacement — passed.
  • Regression test for normal Host draining and State Root fencing — passed.
  • The complete service-manager test file still has unrelated Windows fsync
    and symlink EPERM failures.
  • Linux/systemd runtime verification has not been performed yet.

Review focus

The important safety property is that backend.stop() is not called until the
retirement process owns the State Root fence. If a successor has already
acquired the State Root, retirement fails without stopping the service unit.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Codex was used for repository analysis, implementation design, production code,
regression tests, verification, and PR description drafting.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — managed Runtime Host retirement now fails closed when Host identity
    or State Root ownership cannot be safely fenced.
  • No

Copilot AI lite review requested due to automatic review settings August 23, 2026 11:18

Copilot AI 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.

Pull request overview

This PR hardens the managed Runtime Host “PID-known” retirement flow so it won’t stop a successor host (and misreport retirement) when systemd restarts the service unit between host.upgrade.prepare and backend.stop(). It does this by waiting to fence the State Root before issuing the service stop, and by failing closed when the observed service identity changes.

Changes:

  • Acquire and hold a State Root retirement fence (with PID re-checking) before calling backend.stop() in the PID-known retirement path.
  • Convert the fence acquisition into a bounded polling loop with an explicit deadline and clearer failure modes.
  • Add regression tests for successor replacement during the drain window and for “fence held across stop” behavior.

Required Conclusion (per review skill)

  1. Is the current solution optimal for the actual problem?
    Mostly, but there is a remaining race where a PID change may not be detected if it happens right as the fence becomes available; this can still allow stopping a successor under some timing conditions (see stored comment).
  2. What production code can be deleted?
    none identified.
  3. What low-quality tests can be deleted or replaced?
    none identified.
  4. Is a deeper refactor required, and what should the final structure be?
    No deeper refactor is required for this fix; tightening the fence/PID verification logic inside acquireRuntimeHostRootRetirementFence should be sufficient.
  5. Is the reviewed revision ready to merge?
    No.
  6. What residual risks or verification gaps remain?
    The safety property hinges on correctly treating PID changes as fatal across the entire fence acquisition boundary; the current implementation appears to miss a narrow race at the “fence becomes available” transition. Additionally, the PR notes Linux/systemd runtime verification has not been performed yet.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/cli/src/runtime-host-service-manager.ts Adds bounded, polling State Root fencing (with PID re-checking) and uses it before service stop in the PID-known retirement path.
packages/cli/src/tests/runtime-host-service-manager.test.ts Adds regressions covering successor replacement during retirement and verifying the fence is held when stopping the service.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/cli/src/runtime-host-service-manager.ts Outdated

@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 at exact head 52357a78fe70ab272a1ed231d49eb4f445cabc2c (2 commits, 2 files, +305/-4: 257 lines of tests and ~48 lines of production code). No [P0][P2] findings — the fence holds from all four directions I checked. Notes below, plus one bounded observation and a CI caveat.

What the fence actually fences

The fence is the existing interactive-writer lock on the State Root, held from a successful prepare until the service confirms stopped. That matters because it means there is no second state machine: the PR reuses the one existing authority over root writes rather than inventing a parallel notion of "retiring".

The ordering is what makes it sound. prepareRetirement cross-checks diagnostics.pid against expectedPid, then host.upgrade.prepare(expectedHostEpoch) puts the host into drain, and #requestDrain#closeResources stops new admissions, waits for in-flight operations, closes composition, waits for residency, and only then calls owner.close() to release the writer. So by the time the CLI's poll loop acquires the lock, in-flight work has already landed. The lock acquisition is itself the drain proof, not a separate signal that could disagree with reality.

Ordering and atomicity

The window between fencing and backend.stop() is guarded twice: isExpectedRuntimeHostRetirementStatus runs on every poll iteration and once more immediately after the lock is acquired. A successor process is rejected because its pid differs; the only other accepted state is a fully stopped host (pid === null && !active && state === 'stopped'), which cannot be a live successor.

CLI crash between the two steps is recoverable rather than a half-retired hang: the fd lock dies with the process, and the host's shutdown is self-driven from the drain, not dependent on the CLI issuing the stop.

Failure direction

Every failure mode I traced — prepare failure, connect failure, pid mismatch, fence timeout, identity change — throws retirement_failed before backend.stop(). The new tests pin this with stops === 0, which is the right assertion: it proves the stop never happened, not merely that an error surfaced. The one path that throws after stop() is the post-stop stable-state check, where retirement has already taken effect; that is a loud failure rather than a fail-open, and a retry converges through the idempotent 'stopped' branch.

[P3] Point-in-time recheck leaves a small unguarded gap

Between the final identity recheck and backend.stop() there is no continuous guard. If the old host dies in that gap and systemd starts a successor, the successor is locked out of the writer by the fence the CLI still holds, then gets hit by the stop — and the returned retired result still reports the old pid and hostEpoch.

The consequence is bounded (no writes occur, the root ends up idle), and converting a point check into continuous monitoring is not worth the cost here. Recording it for the record rather than asking for a change.

Validation and CI

Red-green run locally: reverting the production change while keeping the new tests turns exactly the 3 new tests red; restoring the PR gives 19/19 green. Typecheck clean.

CI cannot be called green at this head. This is a fork PR, check-runs is empty, and the only workflow run (32636922940) is action_required — never approved to execute. That is a fork permission gate rather than anything about the code, but it means there is no hosted exact-head evidence, and a maintainer should approve the workflow run before this is treated as gated.

@CuSO41108

Copy link
Copy Markdown
Contributor Author

Hi @Astro-Han, could you please approve the pending GitHub Actions workflow run for this fork PR? The required test check is currently waiting for maintainer approval, so CI has not started yet. Thank you!

@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.

Approving at exact head 52357a78fe70ab272a1ed231d49eb4f445cabc2c.

Hosted CI has now run on this head — the earlier action_required state was a fork permission gate, and with the run executed, test is completed/success. That closes the one thing my previous comment held open. The findings position is unchanged: no [P0][P2], and the single [P3] (the unguarded gap between the final identity re-check and backend.stop()) is recorded rather than requested.

One sequencing note for whoever merges. #3591 touches this same file and carries a different, weaker version of acquireRuntimeHostRootRetirementFence — the single-attempt form, not wired into the retire path at all. Landing this PR first makes that resolution additive; landing #3591 first risks the polling-plus-identity-recheck hardening being silently dropped when the conflict is resolved. Merging this one ahead of #3591 is the safer order.

@Sun-GLiang

Copy link
Copy Markdown
Contributor

English

Thanks for putting this together. I reviewed the current head 52357a78fe70ab272a1ed231d49eb4f445cabc2c against the acceptance criteria of #3594 and compared it with the implementation I developed independently during the same period.

The core direction is aligned: wait for the prepared Host to release the State Root, acquire the fence before backend.stop(), and fail closed when the service identity changes. The primary successor race is covered well.

While reviewing the surrounding lifecycle and failure paths, I found two edge cases that may be worth tightening before merge. I’m sharing them so we can converge on one implementation rather than maintain competing PRs.

[P2] Preserve immediate fail-closed behavior for PID-less starting retirement

acquireRuntimeHostRootRetirementFence() now polls for up to 45 seconds even when it is called without expectedPid and readStatus. This means the existing PID-less starting path can wait across possible service-generation changes without identity validation.

If the current writer releases the root after the managed service identity has changed, the manager could acquire the fence and proceed to backend.stop() without knowing which service generation it is stopping.

Could we keep the PID-less path as a one-shot, immediate fail-closed acquisition, and use bounded identity-aware polling only for PID-known prepared retirement?

[P2] Preserve cleanup failures after post-acquisition identity validation

When a newly acquired fence fails the post-acquisition service identity check, cleanup currently uses:

await ownerForCleanup?.close().catch(() => undefined);

If identity validation and fence cleanup both fail, the cleanup failure is discarded and only the identity error is reported. The owner reference is then lost even though fence release was not confirmed.

Would it make sense to preserve both failures, for example by reporting the validation and cleanup errors together through an AggregateError?

I already have deterministic tests for these failure paths and would be happy to share or adapt them for this PR.

中文

感谢你完成这份实现。我基于 #3594 的验收标准审查了当前 HEAD 52357a78fe70ab272a1ed231d49eb4f445cabc2c,并与我在同一时期独立完成的实现进行了对照。

我们的核心方向是一致的:等待 prepared Host 释放 State Root,在调用 backend.stop() 前取得 fence,并在 service identity 发生变化时 fail closed。当前 PR 对主要的 successor race 已经覆盖得很好。

在继续检查相关生命周期和失败路径时,我发现两个可能值得在合并前进一步收紧的边界情况。我提出这些问题是希望我们能够收敛到同一份实现,而不是维护相互竞争的 PR。

[P2] 保持 PID-less starting 退役路径的即时 fail-closed 行为

acquireRuntimeHostRootRetirementFence() 现在即使没有传入 expectedPidreadStatus,也会轮询最长 45 秒。这意味着现有 PID-less starting 路径可能在没有身份校验的情况下跨越 service generation 变化。

如果 managed service identity 发生变化后,当前 writer 才释放 State Root,manager 可能取得 fence,并在不知道正在停止哪个 service generation 的情况下继续调用 backend.stop()

是否可以保持 PID-less 路径的一次性即时 acquisition,失败后立即 fail closed;只有 PID-known prepared retirement 才使用有界、身份感知的轮询?

[P2] 保留 acquisition 后身份验证失败时的 cleanup failure

新取得的 fence 在 post-acquisition service identity check 失败时,当前使用以下方式清理:

await ownerForCleanup?.close().catch(() => undefined);

如果身份验证和 fence cleanup 同时失败,cleanup failure 会被丢弃,只报告 identity error。随后 owner 引用也会丢失,但 fence 是否成功释放并未得到确认。

是否可以同时保留两个错误,例如通过 AggregateError 一并报告 validation error 和 cleanup error?

我已经为这些失败路径准备了确定性测试,很乐意将它们分享或调整后用于当前 PR。

Treat only the prepared PID or a fully stopped service as the same retirement generation. This keeps a PID-less active successor from being stopped during the State Root handoff.

Generated-by: Codex
@M4n5ter
M4n5ter force-pushed the codex/test-runtime-host-retirement-race branch from 52357a7 to 1724d45 Compare August 23, 2026 13:12
@M4n5ter

M4n5ter commented Aug 23, 2026

Copy link
Copy Markdown
Member

@Sun-GLiang

English

Thank you for comparing the two implementations. After #3600 was merged, #3602 conflicted with main; I helped rebase it and reduce the overlapping implementation to the current head 1724d4511. I agree with the first finding against the previous head, but the rebased result now resolves it structurally:

  • the PID-less starting path retains the existing one-shot acquireRuntimeHostRootRetirementFence() behavior from main;
  • only the PID-known prepared path uses bounded polling and service-generation validation.

The new head is a minimal follow-up to #3600 rather than a second fence implementation. It additionally rejects a PID-less service unless it is fully inactive and stopped.

I do not think the second point should block this change. StateRootOwner.close() marks the lease inactive before releasing the OS handle, caches the close promise, and cannot be retried through the same owner after a failure. The production caller is a short-lived CLI process, so process exit remains the authoritative recovery for the underlying advisory lock. Aggregating the identity error with the extremely narrow handle-close failure would preserve extra diagnostics, but would not improve recovery or authority correctness; keeping the primary identity failure is the smaller design.

中文

感谢你对两份实现进行比较。#3600 合并后,#3602main 发生了冲突;我协助完成了 rebase,并将重叠实现收敛为当前的 1724d4511。第一点对旧 head 成立,但 rebase 后的结果已从结构上解决:

  • PID-less starting 路径保留 main 中一次性获取 fence、失败即关闭的行为;
  • 只有 PID-known prepared 路径进行有界轮询和 service generation 校验。

当前 head 是 #3600 上的最小增量,不再维护第二套 fence 实现;它还会拒绝 PID-less 且未完全进入 inactive + stopped 的 service。

第二点不应阻塞本次变更。StateRootOwner.close() 会先使 lease 失效,并缓存 close promise;失败后无法通过同一 owner 重试。生产调用方是短生命周期 CLI,因此进程退出仍是底层 advisory lock 的最终恢复机制。把极窄的 handle-close failure 与 identity error 聚合,只会增加诊断复杂度,不会改善恢复或 authority correctness;保留主要的 identity failure 是更小的设计。

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Runtime Host PID-known retirement can stop a successor and report the old epoch as retired

5 participants