fix(agents): stop deleteSubAgent from being reversed by a stale sub-agent socket - #2024
Open
cjol wants to merge 6 commits into
Open
fix(agents): stop deleteSubAgent from being reversed by a stale sub-agent socket#2024cjol wants to merge 6 commits into
cjol wants to merge 6 commits into
Conversation
…gent socket Fixes #2003. ## Why - A client connected directly to a sub-agent via `/sub/{class}/{name}` survives `deleteSubAgent()` on the parent: nothing closes its WebSocket. Its next `message` or `close` event still gets forwarded by the parent's WebSocket interceptor. - That forwarding path (`_cf_resolveSubAgentConnection` -> `_cf_resolveSubAgent`) is the same create-on-access resolver used for brand-new connections. It unconditionally calls `ctx.facets.get(...)` and re-inserts the `cf_agents_sub_agents` registry row if missing, so the stale frame silently recreates the sub-agent the caller just deleted, with fresh (empty) state. - A `hasSubAgent()` precheck in the interceptor doesn't close this: `_cf_resolveSubAgent()` itself awaits (identity hashing, the child's own `_cf_initAsFacet` RPC), so `deleteSubAgent()` can interleave between a passing precheck and the resolver's own `ctx.facets.get()`/registry-write. - Fix has two parts, and both are needed: - `deleteSubAgent()` now proactively closes any client socket targeting the deleted sub-agent (or a descendant), with code `1001` and reason `"Sub-agent deleted"`, before any other teardown. This ends the stale connection instead of leaving it open and pointed at nothing. - Closing the socket alone isn't sufficient: an invocation that had already started resolving before the delete began isn't cancelled by a later `close()`, and other entry points into the same creating resolver (plain HTTP `/sub/...`, RPC delegation) aren't affected by closing one WebSocket at all. So `message`/`close` forwarding now resolves through a new non-creating path, `_cf_resolveExistingSubAgent`, which only attaches to a sub-agent that still has a live registry row. It never calls `_recordSubAgent()` and never bootstraps a facet that doesn't already have one; a missing row resolves to "dropped", which the caller treats as consumed rather than falling through to the parent's own `onMessage`/`onClose`. - `connect` forwarding is intentionally left create-on-access: a brand-new connection legitimately may wake or create its target, same as before. - `deleteSubAgent()` also now performs the facet-delete + registry-clear synchronously before its awaited schedule/fiber- lease cleanup, so a concurrent resolver observes "gone" as early as possible rather than after an extra await boundary. - Same-name recreation after delete is unaffected and remains supported: sub-agent identity is a deterministic hash of the logical path (`_cf_subAgentIdentity`), so deleting and recreating a child under the same class+name always resolves to the same Durable Object id with wiped storage, exactly like the existing `destroy()`-then-reaccess and abort-then-reaccess behavior. Nothing here introduces a new identity/generation concept. - Deliberately out of scope, to keep this fix minimal and consistent with existing behavior: - `abortSubAgent()` / self-`destroy()` are untouched. They have an opposite, documented contract to `deleteSubAgent()`: the facet is expected to restart on next access, the registry row is never removed, and `hasSubAgent()` stays `true`. The create-on-access resolver's behavior there is correct, not a bug. - A connect racing a concurrent delete for the same name (a new client attaching to a facet mid-teardown) is a distinct, pre-existing race in the same family as any two concurrent mutations targeting one key. It isn't force-reproducible through the public WebSocket API without adding a test-only synchronization hook, which was explicitly decided against to avoid coupling tests to internal timing. ## Code Changes - `packages/agents/src/index.ts` - `_cf_resolveSubAgentConnection` now returns a tagged union (`"ok" | "no-match" | "dropped"`) instead of `T | null`, and takes an explicit `create` flag alongside the existing `request`/`gate` options. `connect` forwarding passes `create: true` (unchanged behavior); `message`/`close` forwarding now pass `create: false`. - New private `_cf_resolveExistingSubAgent(className, name)`: the non-creating counterpart to `_cf_resolveSubAgent`. Looks up the `cf_agents_sub_agents` registry row directly, resolves the same deterministic identity, and calls the child's `_cf_initAsFacet` RPC without ever writing a registry row. Returns `null` if the row is missing (target deleted or never existed). - New `_cf_closeSubAgentConnectionsForPrefix(prefix, code, reason)` on the root RPC surface (`RootFacetRpcSurface`): closes every root-owned connection whose `/sub/...` target path equals or descends from `prefix`, reusing the existing `_cf_subAgentTargetPath` / `_isSameAgentPathPrefix` helpers already used for schedule-prefix cleanup. - `deleteSubAgent()` reordered: resolve the root stub once, close matching sockets, then synchronously delete the facet and clear the registry row, then run the existing awaited `_cf_cleanupFacetPrefix` (schedules, fiber-recovery leases) last, since that bookkeeping isn't read by anything that decides whether the child still exists. - `packages/agents/src/tests/sub-agent.test.ts` - Added a `waitForClose` helper alongside the existing `waitForJsonMessage` helper. - Three regression tests under "parentPath and registry": a stale send after delete can't resurrect the sub-agent even if the client's own close throws first; `deleteSubAgent` closes a live socket with the documented code/reason; a stale client-initiated close after delete can't resurrect the sub-agent either. - `.changeset/tricky-buckets-relate.md`: patch changeset for `agents`. ## Verification - `pnpm run build` (full monorepo) - `packages/agents` workers test suite: 90 files, 1744 tests passing - `pnpm run test` (all 19 projects via Nx) - `pnpm exec nx affected -t test --base=99cbb514` - `pnpm run check` (sherif, export checks, oxfmt, oxlint, typecheck across 118 projects) - Each new test independently confirmed red on the pre-fix code and green after, including one iteration where an initial test attempt turned out to be non-deterministic/vacuous under close inspection and was rewritten before being accepted.
🦋 Changeset detectedLatest commit: f92c264 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+7369
to
+7373
| const routed = await this._cf_resolveSubAgentConnection(connection, { | ||
| create: false | ||
| }); | ||
| if (routed.status === "no-match") return false; | ||
| if (routed.status === "dropped") return true; |
Contributor
There was a problem hiding this comment.
🟡 Clients stay connected but silently ignored after a sub-agent shuts itself down
Frames from a client attached to a sub-agent that shut itself down are now silently discarded (status === "dropped" at packages/agents/src/index.ts:7373) instead of reaching a restarted sub-agent, so the client keeps an open connection that never responds and is never told anything is wrong.
Impact: A user chatting with a sub-agent that tears itself down sees the conversation freeze with no error and no disconnect, where previously it kept working.
Why self-teardown also clears the registry row that the new non-creating resolver depends on
The PR's rationale states self-destroy() never removes the cf_agents_sub_agents row, so the new non-creating path is safe there. That isn't true: a facet's destroy() delegates to _cf_destroyDescendantFacet, and the immediate parent calls this._forgetSubAgent(target.className, target.name) (packages/agents/src/index.ts:6295) right after ctx.facets.delete(...). So after a self-destroy hasSubAgent() is false and the registry row is gone.
With the row gone, _cf_resolveExistingSubAgent returns null (packages/agents/src/index.ts:10830-10831), so _cf_forwardSubAgentWebSocketMessage / _cf_forwardSubAgentWebSocketClose return "dropped" and consume the event (packages/agents/src/index.ts:7372-7373 and 7392-7393). Unlike deleteSubAgent(), the destroy path never calls _cf_closeSubAgentConnectionsForPrefix, so the client socket is left open and every subsequent message vanishes. Before this PR the same frame went through the create-on-access resolver and restarted the child.
A fix would be to close matching sockets in _cf_destroyDescendantFacet the same way deleteSubAgent() now does (the root already exposes _cf_closeSubAgentConnectionsForPrefix).
Prompt for agents
A facet that calls this.destroy() ends up delegating to _cf_destroyDescendantFacet on the root, and the immediate parent there runs ctx.facets.delete(...) followed by this._forgetSubAgent(target.className, target.name) — so the cf_agents_sub_agents registry row IS removed on self-destroy, contrary to the assumption that only deleteSubAgent removes it. Because message/close forwarding now resolves through the non-creating _cf_resolveExistingSubAgent, any still-open client WebSocket pointed at a self-destroyed sub-agent will have all of its frames resolved to "dropped" and silently consumed, with no close sent to the client and no fallthrough to the parent. Previously those frames recreated the child and kept working. Consider making _cf_destroyDescendantFacet perform the same socket teardown deleteSubAgent now does (close root-owned connections whose /sub/... target path matches or descends from the destroyed path, via _cf_closeSubAgentConnectionsForPrefix), so the client gets an explicit close rather than a silent black hole. Also update the PR/design notes that claim destroy() leaves the registry row intact.
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
There was a problem hiding this comment.
Good catch, confirmed and fixed in c68b6ef. The claim that self-destroy leaves the registry row intact was wrong — _cf_destroyDescendantFacet's immediate-parent teardown branch does call _forgetSubAgent right after ctx.facets.delete(...), same as deleteSubAgent. Added a regression test that reproduced the silent-drop first (confirmed red), then applied the same _cf_closeSubAgentConnectionsForPrefix(targetPath, 1001, "Sub-agent deleted") call that deleteSubAgent() already uses to that teardown branch. abortSubAgent() is unaffected since it never removes the registry row.
agents
@cloudflare/ai-chat
@cloudflare/codemode
create-think
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
Addresses a Devin Review finding on PR #2024 (#2024 (comment)). The PR's original rationale claimed self-destroy() never removes the `cf_agents_sub_agents` registry row, and was therefore safe to leave untouched by the socket-close fix. That claim was wrong: a facet's `destroy()` delegates to `_cf_destroyDescendantFacet`, and the immediate parent's teardown branch calls `this._forgetSubAgent(target.className, target.name)` right after `ctx.facets.delete(...)` — the same registry removal `deleteSubAgent` performs. Because message/close forwarding now resolves through the non-creating `_cf_resolveExistingSubAgent` (gated purely on registry- row existence), a client socket connected to a facet that self-destroys had every subsequent frame silently resolved to "dropped" and consumed, with no close sent and no fallthrough. Before this PR that frame went through the creating resolver and restarted the child, so this was a regression introduced by the original fix, not a pre-existing gap. Fix: the immediate-parent teardown branch of `_cf_destroyDescendantFacet` now calls the same `_cf_closeSubAgentConnectionsForPrefix(targetPath, 1001, "Sub-agent deleted")` that `deleteSubAgent()` already calls, before deleting the facet. `abortSubAgent()` is unaffected and correctly untouched — abort never removes the registry row, so the existing restart-on-next-access behavior there is unchanged. Added a regression test (TDD: confirmed red before the fix, green after) mirroring the existing `deleteSubAgent` close-with-code/reason test, but driving teardown via a sub-agent's own `destroy()` instead. Verification: packages/agents workers suite (90 files, 1745 tests), pnpm run check (118 projects typecheck clean), pnpm exec nx affected -t test.
Addresses a Devin Review finding on PR #2024 (packages/agents/src/index.ts:7219-7224). _cf_closeSubAgentConnectionsForPrefix called connection.close() in a loop with no try/catch. If any single socket's close() throws (e.g. one already closing/closed after racing a client-initiated close), the exception would propagate out of deleteSubAgent() before ctx.facets.delete() and _forgetSubAgent() run, leaving the sub-agent undeleted — and would also stop closing the remaining matched sockets partway through the loop. Wrap each close() call in its own try/catch, matching the existing defensive pattern a few lines below in the same method (ctx.facets.delete() is already guarded for the same reason: cleanup work must not be able to abort the state mutation). Not test-driven: reproducing this deterministically would require mocking connection.close() to throw, which couples the test to an internal collaborator rather than observable behavior through the public API — the anti-pattern the tdd skill explicitly flags. Applied as defensive hardening consistent with the adjacent pattern instead. Verification: packages/agents workers suite (90 files, 1745 tests), pnpm run check (118 projects typecheck clean).
…stingSubAgent Follow-up to a sense-check on PR #2024 that flagged duplicated validation logic drifting between _cf_resolveSubAgent (throws) and _cf_resolveExistingSubAgent (silently returned null for the same conditions) - the latter was produced by copying the former and blanket-converting every failure exit to a sentinel, which had already caused one real bug (config/runtime failures silently swallowed as if the sub-agent had been deleted). Traced reachability precisely instead of throwing for the mismatched conditions: - ctx.facets/ctx.exports unavailable, and the child class missing from ctx.exports: the one call site only reaches this function with a className already validated against ctx.exports by _parseSubAgentPath on the same synchronous turn, so these are unreachable given the current call site. - The root namespace lookup (renamed/un-exported root class, minified class names): per the facets-never-own-real-sockets invariant (#1677), this function is only reached via a real WebSocket, which means the agent instance is always the root DO, so rootClassName is always this DO's own class identifier - and if that identifier didn't resolve via ctx.exports, the very first subAgent()/connect for this root would already have thrown before any registry row or client socket could exist. Removed all three defensive checks rather than throwing for any of them, since none are reachable through the current single call site. Replaced the loose Partial<FacetCapableCtx> cast with the non-partial type (justified by the same reachability argument) instead of using non-null assertions, which have no other precedent in this file. Caught during this change: `pnpm run build` alone did not surface a resulting type error (rootNs possibly undefined) that the repo's actual typecheck script (`pnpm exec tsc --noEmit`, run via `pnpm run check`) did catch - fixed before landing. No test changes: this is a pure internal simplification of dead defensive branches, not a change to any externally observable behavior for the reachable path (the missing-registry-row case, which is what issue #2003's fix depends on, is unchanged and still covered by the existing regression tests). Verification: packages/agents workers suite (90 files, 1745 tests), pnpm run check (118 projects typecheck clean, including tsc --noEmit).
Clarify that existing-only WebSocket resolution also runs on intermediate facets for nested routes, where the earlier connect traversal provides the required runtime guarantees. Also document that connection teardown is shared by deleteSubAgent and self-destroy paths.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes
deleteSubAgentnot sticking while a client is still connected directly to the sub-agent. Fixes #2003.Why
/sub/{class}/{name}survivesdeleteSubAgent()on the parent: nothing closes its WebSocket. Its nextmessageorcloseevent still gets forwarded by the parent's WebSocket interceptor._cf_resolveSubAgentConnection->_cf_resolveSubAgent) is the same create-on-access resolver used for brand-new connections. It unconditionally callsctx.facets.get(...)and re-inserts thecf_agents_sub_agentsregistry row if missing, so the stale frame silently recreates the sub-agent that was just deleted, with fresh (empty) state.hasSubAgent()precheck in the interceptor doesn't close this:_cf_resolveSubAgent()itself awaits (identity hashing, the child's own_cf_initAsFacetRPC), sodeleteSubAgent()can interleave between a passing precheck and the resolver's ownctx.facets.get()/registry-write.deleteSubAgent()now proactively closes any client socket targeting the deleted sub-agent (or a descendant), with code1001and reason"Sub-agent deleted", before any other teardown. This ends the stale connection instead of leaving it open and pointed at nothing.close(), and other entry points into the same creating resolver (plain HTTP/sub/..., RPC delegation) aren't affected by closing one WebSocket at all. Somessage/closeforwarding now resolves through a new non-creating path,_cf_resolveExistingSubAgent, which only attaches to a sub-agent that still has a live registry row. It never calls_recordSubAgent()and never bootstraps a facet that doesn't already have one; a missing row resolves to "dropped", which the caller treats as consumed rather than falling through to the parent's ownonMessage/onClose.connectforwarding is intentionally left create-on-access: a brand-new connection legitimately may wake or create its target, same as before.hasSubAgent()precheck inside the existing creating resolver, but rejected it: the precheck and the resolver's own writes straddle multipleawaitpoints, so a concurrentdeleteSubAgent()can still land in between and recreate the row._cf_subAgentIdentity), so deleting and recreating a child under the same class+name always resolves to the same Durable Object id with wiped storage, exactly like the existingdestroy()-then-reaccess and abort-then-reaccess behavior. Nothing here introduces a new identity/generation concept.destroy()delegates to_cf_destroyDescendantFacet, which removes the samecf_agents_sub_agentsregistry row thatdeleteSubAgent()does. Because message/close forwarding now gates purely on that row's existence, a socket connected to a self-destroying sub-agent would otherwise have every frame silently dropped with no close and no fallthrough, which is worse than the pre-fix behavior (which at least restarted the child). So_cf_destroyDescendantFacet's teardown branch now closes matching sockets the same waydeleteSubAgent()does. This was caught by review after the initial version of this PR incorrectly claimed self-destroy()was untouched; see the discussion on the review thread.abortSubAgent()is untouched. It has an opposite, documented contract todeleteSubAgent()/destroy(): the facet is expected to restart on next access, the registry row is never removed, andhasSubAgent()staystrue. The create-on-access resolver's behavior there is correct, not a bug.Code Changes
packages/agents/src/index.ts_cf_resolveSubAgentConnectionnow returns a tagged union ("ok" | "no-match" | "dropped") instead ofT | null, and takes an explicitcreateflag alongside the existingrequest/gateoptions.connectforwarding passescreate: true(unchanged behavior);message/closeforwarding now passcreate: false._cf_resolveExistingSubAgent(className, name): the non-creating counterpart to_cf_resolveSubAgent. Looks up thecf_agents_sub_agentsregistry row directly, resolves the same deterministic identity, and calls the child's_cf_initAsFacetRPC without ever writing a registry row. Returnsnullif the row is missing._cf_closeSubAgentConnectionsForPrefix(prefix, code, reason)on the root RPC surface (RootFacetRpcSurface): closes every root-owned connection whose/sub/...target path equals or descends fromprefix, reusing the existing_cf_subAgentTargetPath/_isSameAgentPathPrefixhelpers already used for schedule-prefix cleanup.deleteSubAgent()reordered: resolve the root stub once, close matching sockets, then synchronously delete the facet and clear the registry row, then run the existing awaited_cf_cleanupFacetPrefix(schedules, fiber-recovery leases) last, since that bookkeeping isn't read by anything that decides whether the child still exists._cf_destroyDescendantFacet's immediate-parent teardown branch also now calls_cf_closeSubAgentConnectionsForPrefixbefore deleting the facet, for the same reasondeleteSubAgent()needs it.packages/agents/src/tests/sub-agent.test.tswaitForClosehelper alongside the existingwaitForJsonMessagehelper.deleteSubAgentcloses a live socket with the documented code/reason; a stale client-initiated close after delete can't resurrect the sub-agent either; a sub-agent's owndestroy()closes a still-open socket connected to it.Compatibility
deleteSubAgent(), and a sub-agent's owndestroy(), now close any client WebSocket connected directly to the deleted sub-agent (code1001, reason"Sub-agent deleted"), where previously the socket stayed open. Clients that relied on the socket staying open after deletion (arguably already a bug, per deleteSubAgent doesn't stick while a client is still connected #2003) will now see an explicit close.