Skip to content

fix(runtime-host): defer resource drain outside admission - #4661

Open
Sun-GLiang wants to merge 1 commit into
apache:mainfrom
Sun-GLiang:fix/runtime-host-reentrant-drain
Open

fix(runtime-host): defer resource drain outside admission#4661
Sun-GLiang wants to merge 1 commit into
apache:mainfrom
Sun-GLiang:fix/runtime-host-reentrant-drain

Conversation

@Sun-GLiang

Copy link
Copy Markdown
Contributor

Summary

  • Defer Runtime Resource drain requests until the active Session admission has exited, preventing drain-triggered stop paths from reentering the admission gate.
  • Preserve the original canonical read failure in a bounded, redacted Runtime Host diagnostic before draining.
  • Observe hosted Session stop rejection immediately while child lookup is pending, preventing an unhandled rejection without changing error precedence.

Fixes #4656

Verification

  • npm test — all workspace tests passed
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • node scripts/asf-license-headers.mjs check

AI use

Select exactly one:

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

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

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

}
await ownStop;
const ownStopResult = await ownStop;
if (ownStopResult.status === 'rejected') throw ownStopResult.reason;

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.

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();

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

  • #mutableSessionFailure at :762, reached inside sessionAdmission.run from :410 (#start), :525 (#acquire), :611 (#control), :718 (#stop)
  • #resourceFailure at :776, reached inside sessionAdmission.run from :580, :666, :735 (the :507 call 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 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