fix(runtime-host): defer resource drain outside admission - #4661
fix(runtime-host): defer resource drain outside admission#4661Sun-GLiang wants to merge 1 commit into
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.
| } | ||
| await ownStop; | ||
| const ownStopResult = await ownStop; | ||
| if (ownStopResult.status === 'rejected') throw ownStopResult.reason; |
There was a problem hiding this comment.
P1 (reach: reasonable failure path): deliverHostedRootStop, 8 lines below this one at session-manager.ts:3618-3639, still has the exact shape you are fixing here. const ownStop = this.runtimeKernel.stopSession(sessionId, input) is created, then await this.listChildSessions(sessionId) runs, and only after that does await ownStop attach a handler. If runtimeKernel.stopSession rejects during that gap, Node reports an unhandled rejection and the Host exits, which is #4656 again.
It is on the same call chain: RootTurnCoordinator.stopSession:988 -> deliverRuntimeStopIntent -> root-turn-coordinator.ts:2654 -> deliverHostedRootStop.
Smallest fix: wrap that ownStop in observeSettlement too. Better, the two methods are near duplicates that differ only in which authority performs the own stop, so folding them into one body would leave a single place that can regress.
| console.error( | ||
| `[runtime-host] canonical Runtime Resource read failed: ${boundedFailureDiagnostic(canonicalReadFailure.error)}`, | ||
| ); | ||
| this.#requestDrain(); |
There was a problem hiding this comment.
P2 (reach: reasonable failure path): this file still calls #requestDrain() from inside an admission in two other places, so the query path is the only one that got the fix.
#mutableSessionFailureat:762, reached insidesessionAdmission.runfrom:410(#start),:525(#acquire),:611(#control),:718(#stop)#resourceFailureat:776, reached insidesessionAdmission.runfrom:580,:666,:735(the:507call site is outside the run and is fine)
With observeSettlement in place these no longer take the process down, but the nested admission still rejects. stream-graph-coordinator.ts:650-675 collects that into failures, so the drain never stops the graph's operator sessions and close() ends with Failed to close one or more agent graph coordinators. The drain is degraded on exactly the paths it exists for.
Smallest fix: give these two the same treatment as the query path, or move the deferral into RuntimeHostKernel.#requestDrain so every site is covered at once.
Summary
Fixes #4656
Verification
AI use
Select exactly one:
Tool(s) and scope: Codex diagnosed the failure path and authored the implementation and regression tests. The commit includes the required Generated-by trailer.
Checklist
Does this PR entail a change in behavior?