Skip to content
This repository was archived by the owner on Aug 10, 2026. It is now read-only.

fix(agent): poll convergence tears down a deleted def's live agent (orphan fix) - #125

Merged
unforced merged 2 commits into
mainfrom
ag-fix-orphan-poll-teardown
Jun 22, 2026
Merged

fix(agent): poll convergence tears down a deleted def's live agent (orphan fix)#125
unforced merged 2 commits into
mainfrom
ag-fix-orphan-poll-teardown

Conversation

@unforced

Copy link
Copy Markdown
Contributor

What

Deleting an #agent/definition note out-of-band (anything other than the admin-SPA delete button — a direct vault API/MCP delete, an Obsidian sync, another surface) left the live agent + wake channel running until the next daemon restart. The orphaned agent kept answering messages after its def was gone from the vault.

Root cause

There is no vault deleted trigger. The hub's connection engine maps only note.created/note.updated to vault-trigger verbs (parachute-hub admin-connections eventToVaultEventsnote.deleted has no mapping and throws). So a delete never fires the reactive reload(...,"deleted") teardown.

That leaves the 60s poll as the only automatic convergence path for a deletion — but loadAllpruneRemovedDefs only pruned the removed def's grants (reconcileGrants(agent, [])). It never called deregisterByNote, so the live programmatic registration + the wake channel survived. Only the admin-SPA delete button worked, because deleteDef does the teardown in-process; every other delete path orphaned the agent.

Fix

  • pruneRemovedDefs now does the full teardown the reactive confirmedRemoval already does: deregisterByNote (drop the programmatic registration + wake channel) then prune grants.
  • Truncation guard (because the diff is now destructive): a def list at the page cap (DEF_LIST_LIMIT = 500) is treated as possibly-partial — not a confident set — so a truncated read can't be mistaken for deletions and tear down live agents. Sits alongside the existing list-failure guard. This also hardens the pre-existing grant-prune, which had the same latent exposure.
  • Corrected the loadAll/pruneRemovedDefs/daemon-boot comments that wrongly claimed the poll already converged deletes.

How it was found

Live end-to-end verification of the agent-to-agent callback feature (#123). While cleaning up two throwaway test agents, deleting their def notes did not deregister them (/health still listed them 72s later, past the 60s poll). The reactive deleted webhook never fired → traced to the missing vault delete-trigger → the poll's grant-only convergence. (The callback feature itself verified clean: worker→callback→orchestrator chain with the full metadata contract, no loop.)

Tests

  • loadAll tears down a removed def — deregister + removeChannel via the poll (the no-delete-trigger regression).
  • loadAll skips the removed-def teardown on a truncated (page-cap) read — no spurious deregister.
  • The existing agent-grants: grant rows orphaned when an #agent/definition note is deleted #96 removal test now also asserts teardown.
  • Gates: tsc --noEmit clean; bun test ./src 1019 pass / 0 fail; SPA (vitest) 110 / 110.

Note for the merger

origin/ag-unforced-dev currently carries two unmerged commits with no open PR (8aca779, 9db9689 — a "robustness hardening (PR #3)" series, looks like another session's WIP). To avoid clobbering/entangling that branch, this fix is isolated on a one-off branch off main. It is independent: that series leaves pruneRemovedDefs unchanged (still grant-only), so there's no semantic overlap — at most a trivial textual conflict in agent-defs.ts whichever lands second.

Per RC versioning, no version bump in this PR (bump + tag on ship).

unforced and others added 2 commits June 20, 2026 01:30
…rphan fix)

Deleting an `#agent/definition` note out-of-band left the live agent + wake
channel running until the next daemon restart — an orphan that kept answering
messages after its def was gone.

Root cause: there is no vault `deleted` trigger. The hub's connection engine
maps only `note.created`/`note.updated` to vault-trigger verbs
(`admin-connections` `eventToVaultEvents`), so a delete never fires the reactive
`reload(...,"deleted")` teardown. The 60s poll is the ONLY automatic
convergence path for a deletion — but `loadAll`→`pruneRemovedDefs` only pruned
the removed def's GRANTS; it never deregistered the agent or removed the
channel. Only the admin-SPA delete button (which calls `deleteDef` in-process)
tore down correctly; any other delete path orphaned the agent.

Fix: the poll's removed-def diff now does the SAME full teardown the reactive
`confirmedRemoval` does — `deregisterByNote` (drop the programmatic registration
+ wake channel) THEN prune grants.

Because that diff is now destructive, add a TRUNCATION GUARD: a def list at the
page cap (`DEF_LIST_LIMIT`, 500) is treated as possibly-partial — NOT a
confident set — so a truncated read can't be mistaken for deletions and tear
down live agents. (This also hardens the pre-existing grant-prune, which had the
same latent exposure.) Sits alongside the existing list-failure guard.

Tests: a removed def is deregistered + channel-removed via the poll (the
no-delete-trigger regression); a truncated page-cap read skips the diff (no
spurious teardown); the existing #96 removal test now also asserts teardown.
Gates: typecheck clean; `bun test ./src` 1019 pass / 0 fail; SPA 110/110.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mment

Reviewer nits (PR #125, non-blocking): (1) the truncation test now does a third
CONFIDENT pass proving the seen-set survived the truncated pass (a real deletion
is still caught), making it a complete proof of the invariant; (2) an inline note
at the `< DEF_LIST_LIMIT` guard boundary so the off-by-one reads unambiguously.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@unforced

Copy link
Copy Markdown
Contributor Author

Folded the two diff-relevant reviewer nits inline (non-blocking):

  • Nit 1 — the truncation test now does a third confident pass proving the seen-set survived the truncated pass (researcher is still caught as a real removal afterward), so it's a complete proof of the invariant rather than a partial one.
  • Nit 2 — added an inline note at the notes.length < DEF_LIST_LIMIT guard boundary so the off-by-one reads unambiguously.

Nit 3 (seen-set clearing relies on rebuildSeenDefs running right after) is already acknowledged in the pruneRemovedDefs comment — left as-is since adding a redundant .delete() would just duplicate work the rebuild does.

Gates after folding: typecheck clean; bun test ./src/agent-defs.test.ts 70 pass / 0 fail (207 assertions).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant