Repository navigation
fix(agents): stop deleteSubAgent from being reversed by a stale sub-agent socket - #2024
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: 55df841 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}
| const routed = await this._cf_resolveSubAgentConnection(connection, { | ||
| create: false | ||
| }); | ||
| if (routed.status === "no-match") return false; | ||
| if (routed.status === "dropped") return true; |
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.
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
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.
Main moved sub-agent routing into the dynamic-agents capability, so the #2003 fix is re-applied in DynamicAgentsInternal: message/close forwarding resolves only existing children, and delete/destroyDescendant close sockets targeting the deleted path (facets via a root RPC). Adds a nested-facet delete regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
| const row = this.registry.row(className, name); | ||
| if (!row) return null; |
There was a problem hiding this comment.
🔴 Deleted socket marker lost on state updates
If a pending setState runs after deletion, CF_SUB_AGENT_DELETED_KEY disappears because it is not registered as an internal key. A late frame can then reach a same-name replacement.
Learn more
Connection flags share a state wrapper that preserves only keys registered through registerInternalConnectionKeys. A facet can queue connection operations through routeConnectionOperation, and setConnectionState calls connection.setState(state) on the root socket. If that queued operation completes after the delete marks the socket, the unregistered marker is omitted from the new attachment. The close handler then treats a late event as an ordinary frame if a child with the same name exists.
Example: A child queues connection.setState({ ready: true }); its parent deletes the child and recreates it before the queued root operation runs. That operation replaces the socket state without _cf_subAgentDeleted, so a late frame can enter the replacement.
Recommended fix: Register CF_SUB_AGENT_DELETED_KEY alongside the other sub-agent flags in registerInternalConnectionKeys. Add a regression test that sets state on a marked socket and forwards a late frame to a replacement.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 8aba973. closeConnectionsForPrefix now tags each socket it closes with a _cf_subAgentDeleted connection flag (stored in the attachment, so it survives hibernation), and #resolveConnection drops message/close events from a tagged socket before resolving. New sockets are untagged, so connections after recreation work normally. This covers both deleteSubAgent and destroyDescendant, since both close through the root's closeConnectionsForPrefix. New test: "a late frame from a deleted sub-agent's socket does not reach a same-name replacement" (the replacement received the frame before this change).
|
✅ agents import sizes: no significant changes ( |
…ey can't reach a same-name replacement (#2003) Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
This PR fixes
deleteSubAgentnot sticking while a client is still connected directly to the sub-agent. Fixes #2003.Why
A client connected via
/sub/{class}/{name}surviveddeleteSubAgent(): nothing closed its WebSocket, and its nextmessageorcloseevent was forwarded through the same create-on-access resolver used for new connections (DynamicAgentsInternal.resolve). That silently recreated the deleted sub-agent, with a fresh registry row and empty state.What changed (all in
dynamic-agents.ts)deleteSubAgent()anddestroyDescendant()(used by a sub-agent's owndestroy()) close every client socket targeting the deleted path or a descendant, with1001/"Sub-agent deleted", before tearing the facet down. Nested deletes ask the root to do this, because only the root owns real sockets._cf_subAgentDeletedconnection flag, which lives in the socket attachment and so survives hibernation. Latemessage/closeevents from a tagged socket are dropped before routing, so they can't reach a same-name replacement created after the delete.message/closeforwarding resolves through#resolveExisting, which only attaches to a child that still has a registry row and never writes one. A child deleted mid-resolution is treated as dropped.Tests
sub-agent.test.ts:1001.