You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Sun-GLiang (via GitHub)" <gi...@apache.org> on 2026/09/03 13:01:27 UTC
[PR] fix(runtime-host): defer resource drain outside admission [maka]
Sun-GLiang opened a new pull request, #4661:
URL: https://github.com/apache/maka/pull/4661
## 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
- [x] 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
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): defer resource drain outside admission [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #4661:
URL: https://github.com/apache/maka/pull/4661#discussion_r3927084518
##########
packages/runtime-host/src/server/runtime-resource-coordinator.ts:
##########
@@ -373,6 +375,12 @@ export class HostRuntimeResourceCoordinator
}
},
);
+ if (canonicalReadFailure !== undefined) {
+ console.error(
+ `[runtime-host] canonical Runtime Resource read failed: ${boundedFailureDiagnostic(canonicalReadFailure.error)}`,
+ );
+ this.#requestDrain();
Review 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.
- `#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.
##########
packages/runtime/src/session-manager.ts:
##########
@@ -3604,7 +3606,8 @@ export class SessionManager {
} catch (error) {
childLookupError = error;
}
- await ownStop;
+ const ownStopResult = await ownStop;
+ if (ownStopResult.status === 'rejected') throw ownStopResult.reason;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): defer resource drain outside admission [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #4661:
URL: https://github.com/apache/maka/pull/4661#issuecomment-5573528984
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?
```ts
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.
<details>
<summary>中文</summary>
谢谢这轮修复。我重新检查了复杂度，之前称它为“最小完整形状”过早了。实际消融支持先复用 composition 的 `sessionAdmission.detach` 接缝，避免关闭操作继承请求的 admission 上下文，而不是新增全局上下文跟踪和释放回调协调。
真实 composition / SQLite / Graph / SessionManager 的故障回归在加上这层 detach 时通过；只移除 detach 就复现原来的 admission 重入错误，恢复后再次通过。同一 Session 的停止仍由原有队列等待当前 admission 释放。相关四个包构建也通过。
建议保留有实际必要性的修复：统一经 kernel 发起关闭、立即观察停止 Promise 的拒绝、共享停止逻辑，以及原始错误的有界诊断；尝试删除额外的全链释放协调及绑定该机制的测试。如果确实必须等待所有继承的 admission 释放，请给出 detach 方案会破坏的具体生产路径和不变量。
这次只是针对真实 composition 路径的正负消融，尚未验证真实 kernel 子进程或全部关闭场景，不代表候选已经可以直接合并。最初资源读取失败的原因仍未知，也应与本次防止故障升级的修复区分开。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): defer resource drain outside admission [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #4661:
URL: https://github.com/apache/maka/pull/4661#discussion_r3950897191
##########
packages/runtime/src/__tests__/session-manager.test.ts:
##########
@@ -3290,6 +3290,95 @@ describe('SessionManager child-session runtime primitive', () => {
assert.strictEqual(childTwoResult.status, 'cancelled');
});
+ test('observes a rejected hosted stop while child lookup is pending', async () => {
+ const store = new MemorySessionStore();
+ const listStarted = makeGate();
+ const releaseList = makeGate();
+ const childLookupError = new Error('child lookup failed');
+ store.list = async () => {
+ listStarted.release();
+ await releaseList.promise;
+ throw childLookupError;
+ };
+ const runStore = new MemoryAgentRunStore();
+ const authority = hostedRootAuthority();
+ const ownStopError = new Error('hosted stop rejected');
+ authority.stopSession = async () => {
+ throw ownStopError;
+ };
+ const manager = new SessionManager({
+ store,
+ runStore,
+ runtimeEventStore: runStore,
+ backends: new BackendRegistry(),
+ messageAuthority: authority,
+ newId: nextId(),
+ now: nextNow(350),
+ });
+ const unhandled: unknown[] = [];
+ const onUnhandledRejection = (reason: unknown) => {
+ if (reason === ownStopError) unhandled.push(reason);
+ };
+ process.on('unhandledRejection', onUnhandledRejection);
+
+ const stopping = manager.stopSession('session-1', { source: 'stop_button' });
+ const stopRejection = assert.rejects(stopping, (error: unknown) => error === ownStopError);
+ try {
+ await listStarted.promise;
+ await new Promise<void>((resolve) => setImmediate(resolve));
+ assert.deepStrictEqual(unhandled, []);
+ } finally {
+ releaseList.release();
+ await stopRejection;
+ process.off('unhandledRejection', onUnhandledRejection);
+ }
+ });
+
+ test('observes a rejected direct stop while hosted child lookup is pending', async () => {
Review Comment:
**[P3] Share the stop-rejection test body while keeping both entry paths**
This case and the preceding one repeat the blocked child lookup, stores, rejection listener, event-loop wait, error-precedence assertion and cleanup. The meaningful differences are the injected own-stop implementation and whether the test calls `stopSession` or `deliverHostedRootStop`. Could these be two named cases in a small local table sharing that test body? No new reusable fixture framework or production test seam is needed.
Please keep both public calls: testing only one because they now share a private helper would miss a future caller bypass—the hosted entry was an actual gap earlier in this review. Keep the pending-lookup timing, exact own-stop error and listener cleanup as well. This is P3 maintenance duplication in tests, not a runtime defect.
AI-assisted review by Codex; checked at `70b8a3d2`.
<details>
<summary>中文</summary>
相邻两例重复了阻塞 child lookup、store、rejection listener、事件循环等待、错误优先级断言和 cleanup；真正不同的是注入哪个 own-stop，以及调用哪个公开入口。建议用本文件内的小型双案例表共享测试主体，不新增通用夹具框架。两个公开调用都必须保留：之前实际漏修过其中一个入口，不能只测私有 helper 或只留一条。此项是 P3 测试维护建议，不是运行时缺陷。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): detach composition drain from admission [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #4661:
URL: https://github.com/apache/maka/pull/4661
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): detach composition drain from admission [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #4661:
URL: https://github.com/apache/maka/pull/4661#issuecomment-5578057895
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): defer resource drain outside admission [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #4661:
URL: https://github.com/apache/maka/pull/4661#issuecomment-5536771959
Addressed the remaining review notes in b9b0ecd4c:
- 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): defer resource drain outside admission [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #4661:
URL: https://github.com/apache/maka/pull/4661#discussion_r3950582271
##########
packages/runtime-host/src/__tests__/runtime-resource-coordinator.test.ts:
##########
@@ -257,6 +261,66 @@ describe('Host Runtime Resource coordinator', () => {
assert.equal(harness.terminateCount, 0);
});
+ test('requests a canonical state drain only after leaving Session admission', async (t) => {
+ t.mock.method(console, 'error', () => {});
+ let drainAdmission: Promise<void> | undefined;
+ let harness!: ReturnType<typeof createHarness>;
+ harness = createHarness({
+ requestDrain: () => {
+ runAfterCurrentSessionAdmission(() => {
Review Comment:
**[P3] Remove the fixture-owned deferral test**
Thanks for moving drain deferral to the kernel. This test supplies a fake `requestDrain` that calls `runAfterCurrentSessionAdmission` itself, so its successful outside-admission stop does not exercise the production kernel's responsibility. The preceding test already proves that a canonical resource-read failure requests drain, and `host-kernel.test.ts` exercises the real kernel deferral. Could we remove this 26-line case and its now-unused setup/import, retaining those owner-level checks and the composition regression? This is optional test simplification, not a claim that the production failure path is broken. The scenario is a supported resource-read failure; the excess is in how the test proves it.
<details><summary>中文</summary>
感谢把 drain 延迟集中到 kernel。本用例在假 `requestDrain` 中自己调用延迟 helper，因此“离开 admission 后成功停止”证明的是夹具配置，没有经过 kernel 的实际职责。前一用例已证明读取失败会请求 drain，host-kernel 测试已覆盖真实 kernel 的延迟。建议删除这 26 行及因此无用的 setup/import，保留权威处测试和真实 composition 回归。这是可选 P3 测试精简，不是生产故障；场景是真实读取失败，多余的是证明方式。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime-host): defer resource drain outside admission [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on code in PR #4661:
URL: https://github.com/apache/maka/pull/4661#discussion_r3931559770
##########
packages/runtime/src/session-manager.ts:
##########
@@ -3604,7 +3606,8 @@ export class SessionManager {
} catch (error) {
childLookupError = error;
}
- await ownStop;
+ const ownStopResult = await ownStop;
+ if (ownStopResult.status === 'rejected') throw ownStopResult.reason;
Review Comment:
Fixed in b9b0ecd4c. stopSession and deliverHostedRootStop now share #stopSessionTree, which wraps the own stop in observeSettlement before starting child lookup. The direct runtime-kernel path now has the same immediate rejection observation and error precedence as the hosted-authority path, with a regression that reproduced the prior unhandled rejection window.
##########
packages/runtime-host/src/server/runtime-resource-coordinator.ts:
##########
@@ -373,6 +375,12 @@ export class HostRuntimeResourceCoordinator
}
},
);
+ if (canonicalReadFailure !== undefined) {
+ console.error(
+ `[runtime-host] canonical Runtime Resource read failed: ${boundedFailureDiagnostic(canonicalReadFailure.error)}`,
+ );
+ this.#requestDrain();
Review Comment:
Fixed at the RuntimeHostKernel owner in b9b0ecd4c. The kernel latches shutdown synchronously, then runAfterCurrentSessionAdmission begins composition drain and commits shutdown after every active run, runMany, or externally-started runAdmitted context releases. Release hooks execute before the next queued admission resumes. This covers #mutableSessionFailure, #resourceFailure, and the other current or future requestDrain call sites without per-site state; the query-local deferral was removed. Regression tests cover synchronous outside-admission drain, deferred in-admission drain, queued ordering, and external-chain lease reuse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org