You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "CuSO41108 (via GitHub)" <gi...@apache.org> on 2026/08/23 11:18:07 UTC
[PR] fix(runtime-host): fence State Root before service retirement [maka]
CuSO41108 opened a new pull request, #3602:
URL: https://github.com/apache/maka/pull/3602
## 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
- [x] 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
- [x] 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?
- [x] Yes — managed Runtime Host retirement now fails closed when Host identity
or State Root ownership cannot be safely fenced.
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): fence State Root before service retirement [maka]
Posted by "CuSO41108 (via GitHub)" <gi...@apache.org>.
CuSO41108 commented on code in PR #3602:
URL: https://github.com/apache/maka/pull/3602#discussion_r3838386551
##########
packages/cli/src/runtime-host-service-manager.ts:
##########
@@ -934,15 +940,32 @@ async function verifyRuntimeHostRootReleased(
async function acquireRuntimeHostRootRetirementFence(
root: StorageRootCapability<'interactive'>,
+ expectedPid?: number,
+ readStatus?: () => Promise<RuntimeHostServiceBackendStatus>,
): Promise<InteractiveRootOwner> {
+ const deadline = Date.now() + RUNTIME_HOST_RETIREMENT_FENCE_TIMEOUT_MS;
try {
- const owner = await tryAcquireInteractiveRootOwner(root);
- if (!owner) throw new Error('The State Root writer changed while retirement was starting');
- return owner;
+ while (true) {
+ const owner = await tryAcquireInteractiveRootOwner(root);
+ if (owner) return owner;
+ if (expectedPid !== undefined && readStatus !== undefined) {
+ const status = await readStatus();
+ if (status.pid !== expectedPid) {
+ throw new Error('The Runtime Host process identity changed while draining');
+ }
+ }
Review Comment:
Thanks for catching this. I agree that the fence acquisition path must
revalidate the managed service identity after acquiring the fence.
I will add the post-acquisition check and a regression test, while preserving
the valid case where the service is already stably stopped with pid=null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): fence State Root before service retirement [maka]
Posted by "CuSO41108 (via GitHub)" <gi...@apache.org>.
CuSO41108 commented on PR #3602:
URL: https://github.com/apache/maka/pull/3602#issuecomment-5385935245
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): fence State Root before service retirement [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter merged PR #3602:
URL: https://github.com/apache/maka/pull/3602
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): fence State Root before service retirement [maka]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3602:
URL: https://github.com/apache/maka/pull/3602#discussion_r3838372835
##########
packages/cli/src/runtime-host-service-manager.ts:
##########
@@ -934,15 +940,32 @@ async function verifyRuntimeHostRootReleased(
async function acquireRuntimeHostRootRetirementFence(
root: StorageRootCapability<'interactive'>,
+ expectedPid?: number,
+ readStatus?: () => Promise<RuntimeHostServiceBackendStatus>,
): Promise<InteractiveRootOwner> {
+ const deadline = Date.now() + RUNTIME_HOST_RETIREMENT_FENCE_TIMEOUT_MS;
try {
- const owner = await tryAcquireInteractiveRootOwner(root);
- if (!owner) throw new Error('The State Root writer changed while retirement was starting');
- return owner;
+ while (true) {
+ const owner = await tryAcquireInteractiveRootOwner(root);
+ if (owner) return owner;
+ if (expectedPid !== undefined && readStatus !== undefined) {
+ const status = await readStatus();
+ if (status.pid !== expectedPid) {
+ throw new Error('The Runtime Host process identity changed while draining');
+ }
+ }
Review Comment:
`acquireRuntimeHostRootRetirementFence` only checks `status.pid` when the fence is unavailable. If the service PID changes right before the fence becomes available (e.g., old host exits and a successor starts but hasn’t acquired the State Root yet), this function can acquire the fence and return without noticing the PID mismatch, and the caller will proceed to `backend.stop()`—potentially stopping the successor despite `expectedPid` being different. To match the “fail closed when Host identity changes” intent, re-check the PID after successfully acquiring the fence and abort (releasing the fence) on mismatch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): fence State Root before service retirement [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter commented on PR #3602:
URL: https://github.com/apache/maka/pull/3602#issuecomment-5386234263
@Sun-GLiang Thank you for comparing the two implementations. I agree with the first finding against the previous head, but the rebased head `1724d4511` 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.
---
感谢你对两份实现进行比较。第一点对旧 head 成立，但 rebase 后的 `1724d4511` 已从结构上解决：
- 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 是更小的设计。
AI disclosure: Codex drafted and posted this response under maintainer direction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): fence State Root before service retirement [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #3602:
URL: https://github.com/apache/maka/pull/3602#issuecomment-5386173094
## 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:
```ts
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()` 现在即使没有传入 `expectedPid` 和 `readStatus`，也会轮询最长 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 失败时，当前使用以下方式清理：
```ts
await ownerForCleanup?.close().catch(() => undefined);
```
如果身份验证和 fence cleanup 同时失败，cleanup failure 会被丢弃，只报告 identity error。随后 owner 引用也会丢失，但 fence 是否成功释放并未得到确认。
是否可以同时保留两个错误，例如通过 `AggregateError` 一并报告 validation error 和 cleanup error？
我已经为这些失败路径准备了确定性测试，很乐意将它们分享或调整后用于当前 PR。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org