fix(runtime-host): claim Daily Review archives before reading and widen the owned settle bound

[Astro-Han]

Summary

Two Runtime Host tests failed in CI on 2026-09-03 without a product change behind either run. This PR fixes both.

Daily Review generated one archive per Client. two Clients share Daily Review config, generation, and restart recovery failed on main in https://github.com/apache/maka/actions/runs/33775485723: the two Clients received archives with the same id but generatedAt values 6 ms apart. HostDailyReviewCoordinator.#run consulted its in-flight map only after awaiting the summary, the existing-archive read, and the config read. Under load the first request could finish and publish while the second was between its existence check and its in-flight check, so the second generated and published its own archive. The claim now happens before the first await: the archive id is derived synchronously from the clock and the requested day, and a second request for the same archive joins the running generation or conflicts on different options. The join test compares the requested modelKeyOverride and trigger rather than the resolved model key, because the resolved key is not known before the config read and reading it later is the same race. A replace that arrives while a non-replacing run holds the claim waits for it and then claims the archive itself, so it never inherits an archive that run merely found; non-replacing runs join anything compatible and a replace joins a replace.

The owned settle assertion was too tight for CI. owned Host exits promptly after its first connection closes failed on #4660 (a website-only change) in https://github.com/apache/maka/actions/runs/33778890390 (attempt 1): settle(500) returned false. The Host is starved, not stuck. After the only Client closes, the owned launch's idleGraceMs: 0 fires immediately and the shutdown sequence runs; on a 4-vCPU runner with three test files in parallel it lands past 500 ms while still exiting cleanly. The bound is now 5 s with a comment saying what it separates: the owned grace of 0 versus the 30 s default. #3190 / #3221 widened the connection half of the same test for the same reason.

Refs #3190

Root cause evidence

Daily Review: the new coordinator test fires two identical runs and holds the second request's session read until the first has published. Without the fix the model is called twice; with it, once.

Owned settle: a script that repeats the test's steps and times close() to exited on macOS (18 cores), with this workspace's test:dist suites as background load:

Load	connect (ms)	settle (ms)	exit
idle, 8 runs	546–634	28–35	code 0 every run
one suite in parallel, 25 runs	546–2414	29–364	code 0 every run
three suites in parallel, 15 runs	2696–4634	395–878 (13 of 15 over 500)	code 0 every run

The failing CI run connected in about 1.8 s, in the range where the loaded settle time crosses 500 ms. Measuring from a close acknowledgement would not help: RuntimeHostConnection.close() is a local transport abort with no reply, and the slow part is the shutdown work after the Host notices the close.

Verification

• npm --workspace @maka/runtime-host run test:dist: 1670 tests, 1658 pass, 12 skipped, 0 fail
• New test Daily Review joins an in-flight generation even when its own reads land later fails on main (modelCalls 2, expected 1) and passes with the fix
• New tests for both replaceExisting orderings against an existing archive; the replace-second ordering failed on the previous head of this PR (replace received the stored ok archive) and passes now
• npm run format and npm run lint: clean
• The settle measurement script is not committed.

AI use

Select exactly one:

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

Tool(s) and scope: Claude Code diagnosed both failures, measured the settle timing under load, wrote the coordinator fix, the regression test, the test-bound change, and this PR body. Commits carry a Generated-by: Claude Code trailer.

Checklist

• Tests cover the change and fail without it (the Daily Review test; the settle bound is a timing budget in an existing test and has no deterministic failing seam)
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[hqhq1025]

Review of head 1a1fc79842c8f4c7e3dafdff48fae717af2eac0a.

The problem definition is supported by the failing CI evidence. Moving the Daily Review claim before the first await is the direct, minimal fix for duplicate publication, and widening the owned-host settle assertion is justified: the focused test took about 883 ms locally while the process still exited cleanly, so the old 500 ms bound can report a failure without a lifecycle regression.

One merge-blocking behavioral issue remains in the new claim compatibility rule: replaceExisting is omitted, so a force-replacement request can join a non-replacing request and receive the existing archive without any regeneration. The inline finding includes a deterministic reproduction.

Production code deletion: none identified. The extracted dayRange helper is justified because generation must freeze the archive identity before asynchronous reads.

Test deletion: none identified. The new lagging-read regression test exercises the original race. It should be complemented by an existing-archive test that covers both replaceExisting orderings.

Deeper refactor: not required. Keep the early single-flight ownership, but define compatibility over every option that changes the observable result.

Verification performed:

• node --test dist/__tests__/daily-review-coordinator.test.js dist/__tests__/daily-review-two-client-uds.test.js: 6 passed.
• Focused owned-host test: 1 passed in about 883 ms.
• npx biome check on all three changed files: clean.
• git diff --check: clean.
• Full build:test reached and built @maka/runtime-host, then failed in unrelated packages/ui type errors on the current branch.
• PR CI is still running, so a final full-suite result is not yet available.

Verdict: not ready to merge until the replaceExisting concurrency contract is fixed and covered. After that, the architecture is consistent with first principles and is close to the simplest correct solution.

[me2seeks]

LGTM — approving. Verified locally, not just from the diff:

Race fix (daily-review-coordinator.ts)

• Checked out the PR head: tsc --build clean; daily-review-coordinator.test.js (5 tests) and owned-candidate.test.js (11 tests) all pass.
• Mutation check: reverting only daily-review-coordinator.ts to main while keeping the new test makes it fail with actual: 2, expected: 1 (the model runs twice), so the regression test genuinely pins the bug.
• This race is currently failing main: run 33775485723 failed two Clients share Daily Review config, generation, and restart recovery with two archives of the same id (2026-09-01-1d) whose generatedAt values are 6 ms apart (...063 vs ...069) — the exact scenario fixed here.
• Claim-before-await is sound: #run reaches #inFlight.set with no intervening await, so the claim is atomic within the event-loop turn. dayRange preserves the clamping semantics of the code it replaces, and capturing now at request entry also keeps archiveId and summary consistent across a midnight boundary.
• The join-key change (resolved modelKey → raw modelKeyOverride) only trades join for conflict when two concurrent requests spell the same model differently; conflict is the safe direction, and the reasoning (the resolved key is only knowable after the config read — the same race again) holds.

Settle bound (owned-candidate.test.ts)

• 500 ms → 5 s preserves the semantic claim: idleGraceMs of 0 vs the 30 s default grace still has 6x separation, and the bound matches the settle budgets already used elsewhere in this file. Not a quality loss.

Two non-blocking nits:

1. The in-code comment covers claim-before-await but not why the join key is the override rather than the resolved key — that rationale currently lives only in the PR description; worth one line of code comment.
2. In the new test, Promise.race([first, second]).then(release) — if the leader ever rejects, the laggard hangs until the test timeout instead of failing cleanly; .finally would release either way.

[Astro-Han]

Thanks for the mutation check. Both nits taken in ba702be: the claim comment now says requests match on the requested override because resolving the key needs the config read, which would put the claim back after an await; and the lagging-read test releases its gate on either settlement of the leader, so a rejected leader no longer leaves close() waiting on the laggard.

[Astro-Han]

Reviewed current head ba702bed6caa2ba6da8e00cd8b7c4e20c74d3f0f (OPEN, MERGEABLE, hosted test green). One P2 below; no P0/P1/P3. The P2 reported on the previous head is fixed.

P2 — a waiting replace can still generate/publish after close() has returned

packages/runtime-host/src/server/daily-review-coordinator.ts:158-162, 296-306

When #run meets a non-replacing leader for the same archive, the replace request first awaits the in-flight promise and then recursively re-enters #run(input). But close() only waits for a snapshot of the #inFlight map taken after beginDrain(); a replace that is still waiting is not in that snapshot. The recursive entry does not re-check #draining, and the #mutate guard only runs before the wait (:203-205).

Reproduced on this head: with an archive and usage preset, the leader's summary read suspended, a replace waiter started, then close() triggered drain; after releasing the leader, the waiter entered a second #run while draining and called the model. Observed modelCalls=1 with closeResolved=true, and both mutations eventually succeeded — so close claims completion while generation/publish may still touch closing storage, and the new AbortController will not be cancelled by the finished drain.

Suggested fix: after the leader resolves and before the recursive re-entry, fail closed on #draining (return host_draining for the original request), or track the whole replace-wait/retry chain in the in-flight lifecycle that close() waits on; plus an ordering test where drain happens before the leader completes.

Fixed since the previous head

The earlier replace-second concern is resolved: #inFlight now records replaceExisting, :296-306 retries the replace-second in order, and the two opposite-order tests at :230-276 both pass on this head.

What was checked on this head

@maka/runtime-host build passes; daily-review-coordinator.test.js 7/7 and owned-candidate.test.js 11/11 pass; git diff --check clean. What I could not judge: whether the suggested host_draining return value is already part of the public host contract or needs a contract update alongside the fix.

---

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.

简体中文

本条结论全部来自 @未开智选手 的审查。我自己没有读这份 diff；我核的是当前 head 有没有漂移、以及 exact-head 的 CI 状态。当前 head 是 ba702be，检查为 test 通过，PR 为可合并未关闭。P2 是关闭与等待中替换的竞态，位置在 daily-review-coordinator.ts:158-162 与 296-306，上一轮的替换顺序问题已修好。

[jackwener]

Approving at exact head ba702bed6caa2ba6da8e00cd8b7c4e20c74d3f0f. No P0 or P1. The blocking issue raised earlier is fixed, and the fix is better than the one suggested.

The claim now covers replaceExisting, and it does not settle it by refusing. A replace request that meets a non-replacing leader waits for that leader and then claims for itself, rather than being rejected outright or inheriting an archive the leader merely found. Requests still conflict on a different modelKeyOverride or trigger. I looked at whether return this.#run(input) can spin: each pass waits on a generation that actually completes and the leader clears itself from the in-flight map, so it is not a busy loop. A steady stream of non-replacing requests could starve a replace, but that is liveness, not correctness, and it is not worth holding this.

On the two original changes, which are unchanged since I last read them:

The claim really is before the first await — now, the day range, the archive id and the trimmed override are all derived synchronously, and every read moved into #generateArchive. That is what closes the window where two Clients each published their own archive.

Widening settle(500) to settle(5_000) is a bound change, not a masked failure. What "prompt" means here is the owned launch's idleGraceMs of 0 as against the 30 s default, so 5 s still separates those two cases by a wide margin, and a Host that genuinely fails to exit still fails the assertion. The property under test survives.

The delta since the previous head is two comments plus one test correction: Promise.race(...).then(release, release) so that a rejected leader also releases the lagging read, instead of leaving close() waiting forever. That is a real improvement to the test rather than a loosening of it.

test is green on this head.

Evidence boundary: this is a source-level review. I did not run the Runtime Host suite, and the local timing figures quoted in this thread are not mine.

简体中文

在 ba702bed6caa2ba6da8e00cd8b7c4e20c74d3f0f 上批准。没有 P0/P1。先前那条阻塞问题已修,而且修法比建议的更好。

claim 现在覆盖了 replaceExisting,而且不是靠「拒绝」来解决的。 一个要求强制重新生成的请求,遇到不替换的 leader 时会等它跑完、再自己重新 claim,而不是被直接拒绝、也不会继承一份 leader「只是找到的」旧 archive。modelKeyOverride 或 trigger 不同仍然冲突。我看了 return this.#run(input) 会不会空转:每一轮等的都是一次真实完成的生成,leader 完成后会把自己从 in-flight 里摘掉,所以不是忙等。持续涌入的非替换请求理论上可能把 replace 饿着,但那是活性不是正确性,不值得为此拦下。

原来那两处改动自上次阅读以来未变:

claim 确实落在第一个 await 之前——now、日期区间、archive id 和 trim 过的 override 全部同步导出,所有读操作都移进了 #generateArchive。这才是真正关掉了「两个 Client 各自发布一份 archive」的那个窗口。

把 settle(500) 放宽到 settle(5_000) 是边界调整,不是把失败盖住。这里「prompt」的判据是 owned launch 的 idleGraceMs = 0,对比默认的 30 秒;5 秒仍然把这两种情况分得很开,而一个真正退不出去的 Host 照样会让断言失败。被检测的性质保住了。

相对上一个 head 的增量是两处注释加一处测试修正:Promise.race(...).then(release, release),让被拒绝的 leader 也释放那个滞后的读,否则 close() 会永远等下去。这是把测试改对了,不是把它放松了。

这个 head 上 test 是绿的。

证据边界:这是源码层面的审查。我没有运行 Runtime Host 套件,本讨论串中引用的本地耗时数字也不是我测的。

---

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.

[MicroGery]

LGTM