Skip to content

fix(runtime-host): detach composition drain from admission - #4661

Merged
Astro-Han merged 12 commits into
apache:mainfrom
Sun-GLiang:fix/runtime-host-reentrant-drain
Sep 9, 2026
Merged

fix(runtime-host): detach composition drain from admission#4661
Astro-Han merged 12 commits into
apache:mainfrom
Sun-GLiang:fix/runtime-host-reentrant-drain

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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.

  • Detach at the execution composition's existing beginDrain entry 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.
  • Route interaction poison and OAuth fatal handling through context.requestDrain(), preserving immediate poison and process-retention state.
  • Share direct and hosted Session stop handling, observe stop rejection immediately, and preserve bounded, redacted diagnostics for the original resource-read error.

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 main e60e85d77):

  • 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.
  • Desktop/UI knip, ASF source headers, protocol epoch guard, and diff whitespace checks.
  • GitHub Actions test passed for this head.
  • The final kernel regression fails with admission reentry when only detach is 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

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

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

  • 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 — described under Summary above
  • No

Observe hosted stop failures immediately so delayed child lookup cannot expose an unhandled rejection.

Generated-by: Codex
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 3, 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 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.

Comment thread packages/runtime/src/session-manager.ts Outdated
Comment thread packages/runtime-host/src/server/runtime-resource-coordinator.ts Outdated
@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Addressed the remaining review notes in b9b0ecd:

  • Moved the failure-diagnostic import out of the protocol import group.
  • Centralized drain deferral at RuntimeHostKernel instead of retaining per-call-site state. Shutdown latches synchronously; composition drain runs after all active Session admissions release and before the next queued admission resumes, so the ordering is explicit and shared by all requestDrain callers.
  • Consolidated the two Session stop paths so both immediately observe own-stop rejection.
  • Added regressions for direct-stop rejection, mutation failure drains, synchronous outside-admission drain, queued ordering, and external-chain runAdmitted lease reuse.

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
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 5, 2026
Preserve admission-aware drain deferral alongside the upstream runOrJoin lease context.

Generated-by: Codex
@Sun-GLiang
Sun-GLiang force-pushed the fix/runtime-host-reentrant-drain branch from 75a61f8 to 960eac0 Compare September 6, 2026 01:09

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

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

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 辅助源码审查,经主审交叉核对,未重跑全仓测试。

Comment thread packages/runtime-host/src/__tests__/runtime-resource-coordinator.test.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.

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 测试意见一起,可减小测试规模而不丢并发保护。

Comment thread packages/runtime/src/__tests__/session-manager.test.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.

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 辅助审查和主审源码交叉核对,未重跑本地套件,不代表人工验收。

@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for the follow-up fixes. I revisited my earlier complexity assessment at 4fb4577c: calling this the smallest complete shape was premature. A focused ablation now supports a smaller approach using the existing admission boundary.

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 context.requestDrain(), immediately observing ownStop in the shared stop implementation, and preserving bounded failure diagnostics.

The evidence so far:

  • The core, storage, runtime and runtime-host builds passed.
  • The existing interaction fail-stop regression, using the real composition, SQLite, Graph and SessionManager, passed with the wrapper. Its requestDrain harness directly invoked the composition entry point, matching the restored synchronous kernel behavior; assertions checked the real operator stop rather than the new after-release helper.
  • Removing only the detach wrapper made that same regression fail with Cannot enter Session admission from an active admission; reuse its lease instead. Restoring it made the test pass again.
  • A separate probe against the existing gate confirmed that a detached stop for the same session still waits for the current admission through the existing session queue.

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 的 sessionAdmission.detach 接缝,避免关闭操作继承请求的 admission 上下文,而不是新增全局上下文跟踪和释放回调协调。

真实 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
@Sun-GLiang Sun-GLiang changed the title fix(runtime-host): defer resource drain outside admission fix(runtime-host): detach composition drain from admission Sep 8, 2026
@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Automated update from Codex on behalf of the PR author.

Applied the reduction in af12ded17, then merged current main in 5648e4e99. Kernel and admission-gate production code now match main; drain detaches at the composition entry point. The poison-routing, immediate stop-rejection observation, shared stop implementation, and bounded diagnostics remain.

The additional evidence supports this shape:

  • The regression now sends the failing interaction answer over real UDS through RuntimeHostKernel, SQLite-backed composition, Graph, and SessionManager. It checks actual operator stop and ownership release. Removing only the detach wrapper reproduces admission reentry; restoring it passes, including after the main merge.
  • Same-Session stop serialization and rejection of queued resource startup are covered. A targeted audit of all 22 drain callbacks found no production requirement for waiting on the complete inherited admission chain; the relevant reservation, accepted-mutation, cancellation, and process-startup checks passed.
  • Final local validation: 11,256 tests, 11,219 passed, 37 skipped, 0 failed; full build, typecheck, lint, format, Desktop/UI knip, ASF headers, and protocol guard passed.

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 4fb4577c and the reduced candidate before the main merge. This does not establish a detach regression, and the PR does not claim to fix all poisoned-root close errors or shutdown timeouts. The initial resource-read cause also remains unknown.

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

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 验收。

@Astro-Han
Astro-Han merged commit 7129b19 into apache:main Sep 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

runtime-host: resource query failure can crash the host during reentrant session admission

2 participants