fix(runtime-host): detach composition drain from admission - #4661
Conversation
Observe hosted stop failures immediately so delayed child lookup cannot expose an unhandled rejection. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head 30078b30e6366b1a0bf584f86d7bd4ff2a02cdc5, CI green. The diagnosis matches the source: SessionAdmissionGate.run rejects a nested admission (session-admission-gate.ts:49-60), execution-composition.ts:343 builds one shared gate for both the resource coordinator and RootTurnCoordinator, so the reentry in #4656 is real. Deferring the drain to after await this.#sessionAdmission.run(...) is the right place: the gate wraps only operation(lease) in its AsyncLocalStorage run, so the continuation is genuinely outside the admission context. observeSettlement is the general half of the fix and I agree with it.
Two gaps, both the same defect left in place next to the one you fixed. Details inline.
P3: the drain now runs one microtask after the lease is released, so another admission already queued on that Session (a runtime.resource.start, say) can be admitted and launch a shell before #draining is set. The Host is tearing down anyway, so this only matters if you want the ordering stated somewhere.
P3: operation-dispatcher.ts:58 puts the ./failure-diagnostic.js import in the middle of the ../protocol/* group. Biome's assist is off so nothing flags it.
One question worth answering in the PR body rather than in code: requestDrain() has around 90 call sites across 30 coordinators and a good number of them run inside an admission. The owner is a single method, RuntimeHostKernel.#requestDrain (host-kernel.ts:339). Having that owner leave the current admission context before #beginCompositionDrain() fixes every call site at once and needs no per-site local state. If the per-call-site deferral is the deliberate choice, say why, because otherwise this pattern has to be repeated for every future site.
No conflict with #4626: it only touches interaction-coordinator.ts and its test, zero file or line overlap with this PR.
Generated-by: Codex
|
Addressed the remaining review notes in b9b0ecd:
Local verification: npm test (1674 tests; 1662 passed, 0 failed, 12 skipped), typecheck, lint, format check, ASF headers, and diff check. |
Preserve detached admission context semantics while resolving the SessionAdmissionGate test conflict. Generated-by: Codex
Preserve admission-aware drain deferral alongside Host resource snapshots. Generated-by: Codex
Preserve admission-aware drain deferral alongside the upstream runOrJoin lease context. Generated-by: Codex
75a61f8 to
960eac0
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
P2 — the interaction fail-stop path still starts graph drain inside Session admission (reachable failure path).
Thanks for consolidating the stop handling and moving drain deferral into the kernel. The previous unhandled-rejection window is fixed, but one production caller bypasses the new boundary: execution-composition.ts:674–679 calls local beginDrain() in onPoison before calling context.requestDrain().
I reproduced this at 960eac05482428415fd7d03394c1aa2a087f2e42 using the real execution composition, SQLite graph topology and SessionManager, with a backend continuation-apply failure injected during interaction.answer while a graph operator exists. The local drain reaches SessionManager.stopSession before the answer admission releases and rejects with Cannot enter Session admission from an active admission; reuse its lease instead. Graph close then retains that failed drain task. The later deferred kernel call cannot restart it because the composition/domain drain is already marked begun.
Please keep the immediate fail-stop/shutdown latch, but route domain drain through the admission-release boundary instead of starting it locally first. A regression should enter through interaction answer and the production composition with a provisioned graph operator, then verify that stop is reached outside admission. The current gate/kernel tests alone cannot detect this caller bypass.
This is P2 for degraded failure-path shutdown, not a new P1 process crash: the reproduction observed zero unhandled rejections. I am not attributing the poisoned composition's overall close timeout or all terminal failures to this ordering issue.
AI disclosure: Codex and independent reviewers assisted with source tracing, fault injection and this review.
中文摘要
P2:interaction 的 onPoison 在调用 kernel requestDrain 之前,仍同步启动本地 domain drain,绕过了本次统一的 admission-release 边界。真实 composition、SQLite graph topology 和 SessionManager 的故障注入复现了 stop 重入 admission;后续 drain 因已启动而不会重试。请保留立即 fail-stop 的状态锁定,把实际 domain drain 交给释放 admission 后的统一入口,并覆盖该生产调用方。原先的 unhandled rejection 已修复,本次复现为零;不把 poisoned composition 的整体关闭超时升格为新 P1。
Interaction poison and OAuth fatal callbacks started domain drain before the kernel could defer it past Session admission. Keep the immediate fail-stop latch and let the kernel own domain drain ordering. Cover interaction.answer continuation failure with the production composition, SQLite graph operator provisioning, and real SessionManager stop. Verify stop runs outside admission and the original failure remains canonical. Generated-by: Codex
Wait for each still-active inherited admission without a fan-in counter or deduplication set. Observe the original stop promise immediately and await it after child lookup, removing the single-use settlement wrapper and stop factory. Ablation checks: removing kernel deferral fails the kernel regression; removing rejection observation fails both stop paths; keeping only the newest admission fails overlapping detached publication with admission reentry. Preserve these safeguards and cover both ancestor/child release orders. Generated-by: Codex
Preserve the upstream safe-boundary handoff test controls alongside the interaction fail-stop regression. Reuse the existing backend factory seam and retain admission-aware kernel drain ordering. Validation: full npm test, typecheck, Biome check, format check, ASF headers, protocol epoch guard, and Desktop/UI knip passed. The staged Biome stdin checker rewrites upstream warning glyphs in its console output; verified the index equals the worktree and ran Biome directly before bypassing that broken hook entry for this merge. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks. A focused size/complexity follow-up is attached inline. This is AI-assisted source review, cross-checked by the coordinating reviewer; no new full-suite run.
中文
感谢更新,行内补充一条规模与复杂度复核意见。本次为 AI 辅助源码审查,经主审交叉核对,未重跑全仓测试。
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for preserving the distinct admission and stop paths. One additional optional P3 test-maintenance suggestion is inline: share the duplicated stop-test setup while retaining both public entry cases. Together with the earlier fixture-owned deferral comment, this reduces test scaffolding without dropping concurrency coverage. AI-assisted review by Codex.
中文
补充一项 P3:两条公开停止路径都保留,但共享重复测试搭架;与前面的自证 deferral 测试意见一起,可减小测试规模而不丢并发保护。
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks, re-reviewed exact head 4fb4577c94da5a617dd4547279af7cf3ce18afdb. I found no remaining P0–P3 issues. The delta from 70b8a3d2 is limited to two test files (+65/−117): the fixture-owned deferral test is removed, and both public stop entry paths now share the test body while preserving the pending child lookup, rejection observation, error precedence and listener cleanup.
The production contracts remain intact: shutdown latches synchronously, drain waits for the inherited admission chain to release, and both stop entries observe rejection before child lookup. I resolved the four older inline threads after checking their fixes. The current head's hosted test check passed and GitHub reports no merge conflict.
AI-assisted independent review plus primary source cross-check; no new local suite run or human acceptance is claimed.
中文
感谢更新。复核当前 head 后没有剩余 P0–P3。相对 70b8a3d 只改两个测试文件,净减 52 行:删除夹具自行延迟的测试,双公开停止入口共享主体,关键并发保护和清理仍保留。生产的同步 shutdown 锁存、等待继承 admission 链释放、提前观察停止 rejection 三项契约均保持。四条旧行内串已核实并解决;当前 CI test 通过,无合并冲突。本轮为独立 AI 辅助审查和主审源码交叉核对,未重跑本地套件,不代表人工验收。|
Thanks for the follow-up fixes. I revisited my earlier complexity assessment at The demonstrated failure in #4656 establishes two requirements: shutdown must not reacquire admission while inheriting an active admission context, and a started stop promise must have a rejection observer before another asynchronous lookup can delay its handling. The initial resource-read exception remains unknown; this fixes the escalation from that failure, rather than identifying its original cause. Could we reduce the fix by detaching at the composition's existing drain entry point? beginDrain: () => sessionAdmission.detach(beginDrain),For the experiment, I restored the kernel and admission gate to their pre-PR implementations and applied this wrapper. I retained the fixes that have a demonstrated purpose: routing poison-triggered shutdown through The evidence so far:
This suggests we can remove the new global admission-context tracking, after-release registration/rearming, and tests whose purpose is to enforce that additional mechanism. The issue requires avoiding reentrant acquisition; waiting for every inherited admission to release is a stronger contract that needs its own production justification. Please try this reduction, or show a reachable production sequence where detaching at this boundary breaks a required invariant. That would give us a concrete reason to retain any additional coordination. This was a focused composition-level ablation, not a real kernel-subprocess or full shutdown-suite validation, so the smaller candidate still needs the relevant integration checks before merging. 中文谢谢这轮修复。我重新检查了复杂度,之前称它为“最小完整形状”过早了。实际消融支持先复用 composition 的 真实 composition / SQLite / Graph / SessionManager 的故障回归在加上这层 detach 时通过;只移除 detach 就复现原来的 admission 重入错误,恢复后再次通过。同一 Session 的停止仍由原有队列等待当前 admission 释放。相关四个包构建也通过。 建议保留有实际必要性的修复:统一经 kernel 发起关闭、立即观察停止 Promise 的拒绝、共享停止逻辑,以及原始错误的有界诊断;尝试删除额外的全链释放协调及绑定该机制的测试。如果确实必须等待所有继承的 admission 释放,请给出 detach 方案会破坏的具体生产路径和不变量。 这次只是针对真实 composition 路径的正负消融,尚未验证真实 kernel 子进程或全部关闭场景,不代表候选已经可以直接合并。最初资源读取失败的原因仍未知,也应与本次防止故障升级的修复区分开。 |
Reuse the existing Session admission detach boundary for drain. Keep per-Session stop serialization and verify real kernel dispatch without imposing an all-ancestor release contract. Generated-by: Codex
Preserve upstream ledger transcript assertions alongside the kernel drain regression. Generated-by: Codex
|
Automated update from Codex on behalf of the PR author. Applied the reduction in The additional evidence supports this shape:
The regression explicitly prepares its held-open Turn with a fixture residency. A stronger probe that also starts that Turn over UDS reached the shutdown deadline on both The PR description and local historical ablation conclusions have been corrected: deleting pieces of the old coordination did not demonstrate that the complete detach alternative required them. |
Preserve residency-kind forwarding and the drain regression context in the composition test fixture. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for updating the branch. I refreshed to 99693664; the merge conflict is resolved and the focused drain/Stop changes remain intact against the merged main. I found no P0–P3 issues in the PR's fix. Detaching admission at the existing composition drain entry preserves the kernel's synchronous shutdown gate, and the shared stop-tree helper observes rejection immediately without introducing another lifecycle owner.
Both targeted production-composition checks passed again on this new SHA. The earlier ablation that removed only the detach reproduced admission reentry over the actual UDS path. This validates the failure cascade fix; it does not claim the original resource-read exception has been identified. CI passes and GitHub reports MERGEABLE.
AI-assisted review with two focused reviewer passes and primary-agent reconciliation. This is a scoped merge follow-up, not a new full-suite or Desktop acceptance run.
简体中文
感谢更新分支。已刷新到 99693664,冲突解决,合入 main 后原 drain/Stop 修复保持完整。本 PR 修复范围内未发现 P0–P3。在已有 composition drain 入口 detach admission,保留 kernel 同步关门语义;共用 stop-tree helper 及时观察 rejection,没有新增生命周期 owner。
新 SHA 的两个生产 composition 定向检查再次通过。此前仅去掉 detach 的消融在真实 UDS 路径重现 admission reentry。验证的是故障级联修复,不声称原始 resource-read 异常已定位。CI 通过、MERGEABLE。
AI 辅助评审,两路 reviewer 后主审复核。本次是范围限定的 merge 补核,不是重新跑全套测试或 Desktop 验收。
Summary
A Runtime Resource read failure could start graph drain inside an active Session admission. Stopping graph operators then re-entered the gate, and a stop rejection left unobserved during child lookup could terminate the Runtime Host.
beginDrainentry point. The kernel still requests shutdown synchronously, while each Session stop uses the existing Session queue. No kernel deferral or global admission tracking is added.context.requestDrain(), preserving immediate poison and process-retention state.The initial resource-read failure remains unidentified. This prevents admission reentry and the unhandled-rejection window; it does not eliminate all poisoned-composition close errors or shutdown timeouts.
Fixes #4656
Verification
Verified on the final tree at
5648e4e99414c79814c9f32554dbbf4d9c36aabf(merged maine60e85d77):npm test: 11,256 tests; 11,219 passed, 37 skipped, 0 failed. Runtime Host: 1,801 passed, 12 skipped, 0 failed.npm run build,npm run typecheck,npm run lint,npm run format:check.testpassed for this head.detachis removed, and passes when restored.The kernel/UDS regression provisions a real graph operator and checks its actual SessionManager stop plus ownership release after an injected interaction-answer failure. Its held-open Turn is prepared with a fixture residency; it does not claim full poisoned-root lifecycle coverage. Removing only the detach wrapper reproduces admission reentry; restoring it passes. Other regressions cover same-Session stop serialization, queued resource-start rejection, and both immediate stop-rejection observers.
AI use
Tool(s) and scope: Codex implemented the fix and regressions, ran differential experiments and a drain-side-effect audit, resolved the main-branch import conflict, and performed validation. Commits include Generated-by trailers.
Checklist
Does this PR entail a change in behavior?