Capability cleanup + three admission/revocation fixes for the host-request dispatcher - #1357
Capability cleanup + three admission/revocation fixes for the host-request dispatcher#1357snimu wants to merge 62 commits into
Conversation
…o HostRequestHandler The two exported names described the same binary (payload, context) shape; keep the single public name and update the test fixtures that imported the alias.
…RequestHandler The options argument had exactly one legal value at all 17 call sites; the factory + WeakSet is the provenance authority and the TypeScript signature already forces the binary shape, so the marker was pure ceremony. Delete HostRequestHandlerOptions, contextAwareHostRequestHandler, and the test that only exercised the marker.
requestId and generation had zero production consumers (only tests echoed them back); the abort signal is the only authority handlers act on. Also delete the KernelManager generation counter that existed solely to feed the deleted field.
The WeakSet is the real provenance check; the symbol was copyable (the old test literally copied it) and rejected nothing the WeakSet did not already reject. With the brand gone the capability intersection type is vacuous, so the factory and assertion now speak plain HostRequestHandler.
…uthority isCurrent() reduced to !signal.aborted (the internal flag only ever flipped inside the abort listener), so callers ended up checking the same bit under two spellings. Delete isCurrent() and check the signal everywhere. A revoked context also stays registered in the dispatcher WeakSet: it is still genuine, just no longer current. The factory wrapper now rejects aborted contexts explicitly at dispatch, which is what the WeakSet removal was previously (ab)used for.
…ation The delete handler ignored its request context: a revoked request (comm closed mid-flight) still deleted the child and only the reply was suppressed. The handler now rejects an already-aborted context, and deleteRlmSubagent threads the request signal through so authority is re-checked after each async resolution step, immediately before the mutating delete. Regression test: revoke during the async selector listing; the child survives, stays listed, and no reply is sent.
… update The abort listener and the post-registration authority assert ran only after the run had entered _activeRlmChildRuns and emitChildUpdate had already reached subscribers. A subscriber that revoked authority in reaction to that queued update (or a revocation landing in that window) left the run entry queued forever: the throw at the old assert bypassed every cleanup path. The listener now attaches immediately after registration and before the first emit, and an already-revoked authority is routed through the same cancel + deregister cleanup as the late admission path, including removing the just-created child session dir like the neighbouring session-name failure path does. Regression tests: a subscriber revoking on the queued update, and a revocation landing just before registration, both reject the spawn and leave _activeRlmChildRuns empty and no orphaned sub-* dir on disk.
…e commit After run.publication.promise resolved, the admission commit re-checked only request authority, not run.status. A _cancelRlmChildRun landing in that window (e.g. a concurrent delete or parent abort) still handed the caller a successful spawn handle for a child that would never run. The commit now also throws when the run was cancelled, taking the same revoke-and-cleanup path as a revoked authority. Regression test: cancel the run between publication resolve and admission; the spawn rejects and no run entry is left behind.
jonaowen
left a comment
jonaowen
left a comment
There was a problem hiding this comment.
The signal-only simplification is sound, but the pre-admission spawn fix is incomplete.
Revocation on/after the first queued update calls _cancelRlmChildRun, yet execution continues into detached _createRlmSubagentRuntime. Its cancellation catch can inject deliverTerminalMessageToParent even though admission never committed, and runtime disposal/deletion does not remove childSessionDir. The only new rmSync(childSessionDir) is in the earlier pre-emit catch. Thus a revoked, never-admitted request can mutate the parent transcript and leave a durable orphan sub-* artifact directory.
The queued-update regression asserts only the active-run map/list. Please gate absence of parent cancellation injection and removal of the artifact directory, including a runtime host that publishes/settles late after revocation. Also add exact coalesced-delete and late-publication cleanup ownership coverage so one revocation/current waiter cannot double-clean or inherit the wrong request's authority.
Follow-up to the queued-run leak fix, prompted by the #1357 review: making the cancellation catch reachable for never-admitted spawns exposed three latent gaps in that path. - The detached task still created a full child runtime (potentially kernel startup) for an already-cancelled run; it now checks cancellation before awaiting runtime creation. - The catch delivered a cancelled/failure notice into the parent transcript for a spawn whose caller already received the outcome as the admission exception; notices are now gated on committed admission. Admitted runs keep their notices. - No cleanup path removed the child session dir; a never-admitted cancelled run now removes it after the dispose/release paths, so a late-created runtime is disposed before its dir is deleted. Regression tests: the queued-update revocation test now also asserts no runtime is created, no custom message lands in the parent transcript, and no sub-* dir remains; a new variant publishes the child session after revocation and asserts the same plus child disposal.
|
@jonaowen Verified all three findings and fixed them in e83a5a2 — thanks, the analysis was exactly right. For context: the notice/orphan-dir behavior is latent in #1243's cancellation catch (admitted-then-cancelled runs behave the same way there by design); our fix B exposed it by making the catch reachable for never-admitted spawns, so the never-admitted case now owns its full postcondition:
Tests: the queued-update regression now also asserts no runtime creation, no parent-transcript injection, and no orphan dir; a new variant publishes the child session late (after revocation) and asserts the same plus child disposal. Each assertion was verified to fail on the previous head (75df2d8). Src delta is +9/−1. On the last paragraph (coalesced-delete and late-publication cleanup ownership coverage): the late-publication disposal path is now covered as above; broader double-clean/authority-inheritance coverage for the coalesced-delete path is deliberately not in this PR — that machinery predates it and is better owned by #1243 itself. Flagging it for @sethkarten. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ed up Cursor Bugbot flagged that the never-admitted rmSync ran unconditionally after the release/delete hooks: when a runtime host had already created and published the child before revocation, a daemon's release path records a deleted registry entry whose sessionFile still points into the artifact tree the rm then destroyed — violating the daemon contract that deletion keeps the transcript and artifact tree on disk. The rm is now gated on nothing durable referencing the dir: it fires only when there is no runtime host at all, or the host never produced a runtime or session. Any host-involved path — including a failed release hook, whose retry state may still reference the dir — skips the rm and leaves persistence to the host. A dir referenced by a deleted registry entry is not an orphan; a dir nothing references still is, and is still removed. Tests: the late-publication variant now uses a daemon-like host with a release hook and asserts the dir survives and release is called with "cancelled"; a new inline (no-host) variant asserts the dir is still removed; the subscriber-revocation orphan assertions are unchanged.
Second Cursor Bugbot finding: the round-4 gate keyed on host presence, but the default in-process AgentSessionRuntime installs itself as every session's host, so the no-host arm was effectively dead in real usage and a spawn revoked after in-process runtime creation left an unreferenced orphan dir again. The discriminator is not whether a host exists but whether it persists. The cleanup catch already branches on exactly that: only the release-hook path records durable deletion (the daemon implements it; core writes no registry, and AgentSessionRuntime's deleteRlmSubagentRuntime is dispose-only). Gate the rm on releaseRlmSubagentRuntime presence instead. The round-4 decision stands for a release hook that was present but failed: retry state may still reference the dir, so it is kept. Tests: the in-process variant now mirrors the dispose-only self-host shape (no release hook) and asserts the dir is removed; the daemon-like release hook variant keeps asserting survival.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Published append-only checkpoint It adds exact passive-resident deletion authority and retains typed precommit release failures behind a private exact-incarnation retry lease. Exact typecheck, changed-file Biome, focused daemon/lifecycle tests, diff checks, and two initial exact-tip reviews passed before publication. A later review then identified an additional public attach-snapshot visibility path for quarantined resident children. A local append-only correction exists and is being re-reviewed; it is not yet published. We also reproduced an independent deletion-latency gap: deleting a child stuck in a tool call can wait for physical teardown for several minutes rather than acknowledging durable logical deletion promptly. Accordingly this PR is not ready. No merge performed. |
This reverts commit 5a1d330.
|
Core lifecycle checkpoint update (local, not yet pushed): bounded deletion now acknowledges after the durable tombstone, keeps cleanup-pending state private, retries the full close pipeline, and passes strict typecheck plus focused lifecycle/privacy coverage. Fresh review found a directly related unscoped cron-cancel privacy path; the minimal guard/regression is being finalized. Unrelated pre-existing/environment-only failures are explicitly deferred. The destructive checkpoint will be fast-forward published only after two fresh exact-tip reviews pass. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 915faf9. Configure here.
| return { deletionDurability }; | ||
| } | ||
| if (deletionDurability === "absent") authority?.assertCurrent(); | ||
| if (state) await this.closeSession(state, "killed", false); |
There was a problem hiding this comment.
🟠 High daemon/daemon-mode.ts:2574
deleteRlmSubagentRuntime leaves a supplied session running when no resident state exists: it tombstones the registry entry but only closes the session inside the if (state) branch. This passive/stale-runtime path no longer disposes the supplied session, so its resources and background work leak; restore the session?.disposeAsync() fallback.
-\t\t\t\t\t\tif (state) await this.closeSession(state, "killed", false);\n+\t\t\t\t\t\tif (state) {\n+\t\t\t\t\t\t\tawait this.closeSession(state, "killed", false);\n+\t\t\t\t\t\t} else {\n+\t\t\t\t\t\t\tawait session?.disposeAsync();\n+\t\t\t\t\t\t}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/daemon/daemon-mode.ts around line 2574:
`deleteRlmSubagentRuntime` leaves a supplied `session` running when no resident `state` exists: it tombstones the registry entry but only closes the session inside the `if (state)` branch. This passive/stale-runtime path no longer disposes the supplied session, so its resources and background work leak; restore the `session?.disposeAsync()` fallback.
Evidence trail:
packages/coding-agent/src/modes/daemon/daemon-mode.ts:2515-2582 @ 4e7bdf04e0f3f07d608b9b8ec00edd9b33b62d31; packages/coding-agent/src/core/agent-session.ts:9742-9757 @ 4e7bdf04e0f3f07d608b9b8ec00edd9b33b62d31; packages/coding-agent/src/core/agent-session.ts:9622-9634 @ 4e7bdf04e0f3f07d608b9b8ec00edd9b33b62d31; git diff MERGE_BASE REVIEWED_COMMIT -- packages/coding-agent/src/modes/daemon/daemon-mode.ts
3 participants
Capability cleanup + three admission/revocation fixes for the host-request dispatcher
Follow-up to #1243, based on its current head (77b188b). Targets
core02-host-request-dispatcherso it can be reviewed in isolation and merged into the PR branch.PR #1243 turned kernel→host callbacks into factory-minted capability handlers with per-request
abortable contexts. This branch removes the migration scaffolding that shipped with that change
and fixes three bugs in the revocation/admission paths.
Cleanup commits (behavior-preserving; suite green after each)
37189b7 — collapse
HostRequestHandlerImplementationintoHostRequestHandlerTwo exported names for the same binary
(payload, context) => Promise<...>shape. Keep one.d877040 — drop the context-aware marker from
createHostRequestHandlerThe
optionsparameter (HostRequestHandlerOptions/contextAwareHostRequestHandler) hadexactly one legal value at all 17 call sites. The factory + WeakSet is the provenance
authority and the TS signature already forces the binary shape; the marker was ceremony.
Also deletes the test that only exercised the marker.
9d45f21 — shrink
HostRequestContextto the signalrequestIdandgenerationhad zero production consumers (tests just echoed them). TheKernelManager.hostRequestGenerationcounter existed solely to feed the deleted field.d69df06 — drop the copyable handler brand symbol
The symbol was copyable (the old test literally copied it onto a forgery) and rejected
nothing the WeakSet did not already reject. With it gone, the
HostRequestHandlerCapabilityintersection type is vacuous and is deleted too.7e19130 — make the abort signal the single revocation authority
isCurrent()reduced to!signal.aborted(the internal flag only flipped inside the abortlistener), so call sites like rlm-runtime checked the same bit under two spellings. Delete
isCurrent()everywhere; checksignal.aborted. Revocation semantics change deliberately:a revoked context stays in the dispatcher WeakSet — it is still genuine, just no longer
current.
assertGenuineHostRequestContextnow means only "minted by this dispatcher"; thefactory wrapper rejects aborted contexts explicitly at dispatch (
"host request authority was revoked"instead of the misleading"host request context is invalid"for replayedrevoked contexts).
a6a8f9d — drop migration-era wording from test names/comments ("staged", marker
mechanics, "business-unit fixtures").
Fixes (one commit each, each with a regression test that fails on the base)
A. 06ae279 —
rlm.delete_subagentignored its authority contextRepro logic: open the delete comm, close it (revoking authority) while the async selector
listing is in flight → the child was still deleted; only the reply was suppressed.
Fix:
deleteRlmSubagent(target, signal)threads the request signal through and re-assertsauthority immediately before each mutating delete (the factory wrapper already rejects an
aborted context at dispatch). Test: revoke mid-listing → child not deleted, still listed,
no reply sent.
B. 220fca4 — queued-run leak on pre-revoked spawn
Repro logic: the run entered
_activeRlmChildRunsandemitChildUpdate()reached subscribersbefore the abort listener attached and
assertRequestCurrent()ran; a subscriber revoking inreaction to the queued update made the assert throw outside any cleanup path, leaking the run
as "queued" forever (an abort listener attached to an already-aborted signal never fires).
Fix: attach the listener + assert immediately after registration and before the first emit;
route the early throw through the same cancel + deregister cleanup as the late path, and
remove the just-created child session dir like the neighbouring session-name failure path.
Tests: subscriber revokes on the queued update; revocation lands just before registration —
both reject the spawn, leave
_activeRlmChildRunsempty, and leave no orphaned sub-* dir.C. 75df2d8 — stale spawn handle for a cancelled child
Repro logic: after
run.publication.promiseresolves, admission re-checked only authority;a concurrent
_cancelRlmChildRunin that window still returned a "successful" handle.Fix: the admission commit also throws when
run.status === "cancelled".Test: cancel between publication resolve and admission → spawn rejects, no run entry remains.
Validation
npx tsgo --noEmitclean at every commit.biome checkclean on all touched files.acp-kernel-features,agent-session-bus,agent-session-concurrent,agent-session-recursion,host-request-contract,kernel-abort,kernel-agent-message-skill,kernel-agent-observe-skill,kernel-attach-image-skill,kernel-bootstrap,kernel-fork-server,kernel-goal-skill,kernel-rlm-heartbeat-skill,kernel-startup,kernel-state-roundtrip,kernel-state-snapshot,mcp-manager—224 passed, 15 skipped (skips are environment-gated kernel-python tests, skipped on base too).
MERGE-ORDER NOTE
When this merges into
core02-host-request-dispatcherand propagates up tov080/core-split-c3-managed-catalog(PR #1333 and successors), the stackedagent-messages.tscall sites still pass the deletedcontextAwareHostRequestHandlerconstant (import at line 3, call sites ~541/~614 on
pr-1333-review). Resolution is atrivial argument + import deletion; no behavioral conflict.
Note
High Risk
Changes span host-request revocation, RLM spawn/delete coordination, daemon registry tombstoning, session lease ownership, and close/archive ordering—core orchestration paths where races or partial failure could strand children or delete the wrong incarnation.
Overview
Host-request dispatcher now treats
AbortSignalrevocation as the only per-request authority:HostRequestContextdropsrequestId,generation, andisCurrent(), andcreateHostRequestHandlerno longer needs the context-aware marker. Revoked requests fail withhost request authority was revokedat dispatch.rlm.delete_subagentandrlm.runthread the signal through; spawn admission arms abort before the first queued update and rejects cancelled children after publication.RLM subagent lifecycle is reworked around deletion durability (
absent/tombstoned/unknown),RlmSubagentDeletionAuthority(generation + session incarnation fences), and_coordinateRlmSubagentDeletionso explicit deletes, compaction reapers, and quarantine retries cannot race or commit after revocation. Failed precommit releases land in private quarantine (hidden from listings); the daemon persistssessionIdin the registry, validates rows strictly before destructive cleanup, tombstones before close, retries failed tombstoned closes with backoff, and hides terminalizing / quarantined sessions from public daemon APIs. Runtime teardown adds lease deferral to the daemon and dispose retry after one-shot failures.Reviewed by Cursor Bugbot for commit 4e7bdf0. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Replace capability branding with
AbortSignal-based revocation in the host-request dispatcherHostRequestContextto carry only anAbortSignal, removingrequestId,generation, andisCurrent()from the interface and all call sites.createHostRequestHandlernow rejects dispatched requests whose signal is already aborted, throwing'host request authority was revoked'without requiring an external context-aware marker.AgentSession.deleteRlmSubagentand_startRlmChildRunnow check the signal before and during admission, aborting spawns and deletions cleanly if authority is revoked mid-flight.assertHostRequestHandleris simplified to aWeakSetidentity check, removing symbol-brand forgery concerns.HostRequestContextafter revocation will now receive'host request authority was revoked'instead of a context-aware marker rejection.Changes since #1357 opened
AgentSession.rlmchild spawn task [e83a5a2]AgentSessioncleanup logic to conditionally delete never-admitted cancelled child artifact directories based on subagent runtime host presence [3110923]AgentSession rlm recursiontest suite to validate artifact directory cleanup behavior for revoked spawns [3110923]AgentSession[aa1908a]AgentSessionRLM child spawn task to track whether the host durably retained a cancelled child's artifacts and conditionally delete the child session directory based on this flag [2a26824]AgentDaemon.createSubagentRuntimeHost.releaseRlmSubagentRuntimehandler to compute and return aretainedflag for cancelled children and suppress exceptions on registry write failures [2a26824]SubagentRuntimeHost.releaseRlmSubagentRuntimeinterface method signature to return an object with aretainedboolean property [2a26824]AgentDaemon.recordRlmSubagentDeletionutility to return a boolean indicating whether a durable deletion tombstone exists for the child [2a26824]releaseRlmSubagentRuntimereturn type andrecordRlmSubagentDeletionboolean return, and added test coverage for durable retention scenarios [2a26824]AgentSession.deleteRlmSubagentandAgentSession._resolveRlmChildDeletionQuarantineto enforce deletion authority and durability guarantees before completing quarantined RLM subagent deletions [03ff589]AgentDaemon.SubagentRuntimeHost.deleteRlmSubagentRuntimeto require exact authority matching for passive RLM subagent deletions [43ca8dd]AgentSessionRLM subagent release finalizer for failed precommit deletions [43ca8dd]agent-session-recursionanddaemon-modetest files to reflect quarantine semantics and stricter authority validation [43ca8dd]AgentSessionRuntimeand daemon with deferral mechanism [b01731e]AgentDaemon.closeSessionOnceto continue teardown despite cleanup failures and coordinate lease release with runtime [b01731e]AgentDaemon.closeSessionOnceto establish archive boundary before teardown, creating terminalizing records on archive failure [915faf9]FailedTombstonedRlmCloseinterface with terminalizing flag and optional fields for archive-failure session retention [915faf9]AgentDaemon.shutdownandAgentDaemon.closeTerminalizingSessionsForShutdown[915faf9]AgentDaemon.runTombstonedCloseAttemptto differentiate terminalizing records and honor original close reasons [915faf9]daemon-mode.test.tsto validate terminalizing behavior, archive-failure retention, and shutdown sequences [915faf9]AgentDaemon[4e7bdf0]📊 Macroscope summarized 8c97153. 3 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.