fix 2003 subagent delete impl - #2067
Closed
cjol wants to merge 6 commits into
Closed
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.
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.
🦋 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://cdn.statically.io/img/avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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.
Uh oh!
There was an error while loading. Please reload this page.