fix: error-handling hardening — outbound retry, CAS claim, delete ordering, grant-GC, queue cap

src/agent-defs.test.ts

@@ -1187,3 +1187,123 @@ describe("AgentDefRegistry — grant-GC reconcile (#96)", () => {
     expect(calls.registered.map((s) => s.name)).toEqual(["uni"]);
   });
 });
+
+// ---------------------------------------------------------------------------
+// FIX 4 (delete ordering: vault-delete first, then deregister) + FIX 5 (grant-GC
+// failure on delete is surfaced, not swallowed) — PR #3.
+// ---------------------------------------------------------------------------
+
+/**
+ * A fetch that serves the def list + by-id GET (so an agent instantiates) and routes
+ * a DELETE to a configurable outcome (`deleteStatus`). Records DELETEs so a test can
+ * assert the note-delete was attempted. Reconcile/PATCH succeed by default.
+ */
+function vaultFetchWithDelete(opts: {
+  defs: Array<{ id: string; content?: string; metadata?: Record<string, unknown> }>;
+  deleteStatus?: number; // the status the DELETE returns (default 204 = success)
+  deletes?: string[]; // record each DELETEd note id
+}): typeof fetch {
+  return (async (url: string | URL | Request, init?: RequestInit) => {
+    const u = String(url);
+    const method = init?.method ?? "GET";
+    if (method === "DELETE") {
+      const id = decodeURIComponent(u.split("/api/notes/")[1]!);
+      opts.deletes?.push(id);
+      const status = opts.deleteStatus ?? 204;
+      return new Response(status >= 400 ? "delete failed" : null, { status });
+    }
+    if (method === "PATCH") return new Response(null, { status: 200 });
+    if (u.includes("/api/notes?") && u.includes("tag=%23agent%2Fdefinition")) {
+      return new Response(JSON.stringify(opts.defs), {
+        status: 200,
+        headers: { "content-type": "application/json" },
+      });
+    }
+    return new Response("[]", { status: 200 });
+  }) as typeof fetch;
+}
+
+describe("AgentDefRegistry — deleteDef ordering + grant-GC surfacing (FIX 4/5, PR #3)", () => {
+  test("FIX 4: a vault-delete failure leaves the def REGISTERED (not orphaned)", async () => {
+    const { deps, calls } = recorderDeps();
+    const deletes: string[] = [];
+    const fetchFn = vaultFetchWithDelete({
+      defs: [{ id: "Agents/uni", content: "role", metadata: { name: "uni" } }],
+      deleteStatus: 502, // the vault note delete 502s
+      deletes,
+    });
+    const reg = new AgentDefRegistry(deps, { bindings: [binding], fetchFn });
+    await reg.loadAll();
+    expect(reg.findLiveByNote("Agents/uni")).not.toBeNull(); // live before delete.
+
+    // The delete throws (the vault note delete failed) BEFORE any deregister.
+    await expect(reg.deleteDef("Agents/uni")).rejects.toThrow(/delete def Agents\/uni failed \(502\)/);
+
+    // FIX 4 invariant: the agent is STILL registered (the in-memory def was NOT torn down
+    // on a failed vault delete) — it re-converges on the next poll rather than orphaning.
+    expect(reg.findLiveByNote("Agents/uni")).not.toBeNull();
+    expect(calls.deregistered).toEqual([]); // nothing was deregistered.
+    expect(deletes).toEqual(["Agents/uni"]); // the delete WAS attempted (and failed).
+  });
+
+  test("FIX 4: a successful vault-delete deregisters cleanly", async () => {
+    const { deps, calls } = recorderDeps();
+    const deletes: string[] = [];
+    const fetchFn = vaultFetchWithDelete({
+      defs: [{ id: "Agents/uni", content: "role", metadata: { name: "uni" } }],
+      deleteStatus: 204,
+      deletes,
+    });
+    const reg = new AgentDefRegistry(deps, { bindings: [binding], fetchFn });
+    await reg.loadAll();
+
+    const removed = await reg.deleteDef("Agents/uni");
+    expect(removed.name).toBe("uni");
+    expect(removed.grantsReconciled).toBe(true); // no grants client → nothing to reconcile = ok.
+    // Now deregistered + removed from the live set.
+    expect(reg.findLiveByNote("Agents/uni")).toBeNull();
+    expect(calls.deregistered).toEqual(["uni"]);
+    expect(deletes).toEqual(["Agents/uni"]);
+  });
+
+  test("FIX 5: a grant-reconcile failure on delete is SURFACED (grantsReconciled:false) — the note-delete still completes", async () => {
+    const { deps, calls } = recorderDeps();
+    const reconciled: Array<{ agent: string; liveConnections: ConnectionSpec[] }> = [];
+    const grants = fakeGrantsClient({ reconciled, reconcileFails: true }); // reconcile POST 500s
+    const deletes: string[] = [];
+    const fetchFn = vaultFetchWithDelete({
+      defs: [{ id: "Agents/uni", content: "role", metadata: { name: "uni" } }],
+      deleteStatus: 204,
+      deletes,
+    });
+    const reg = new AgentDefRegistry(deps, { bindings: [binding], fetchFn, grants });
+    await reg.loadAll();
+    reconciled.length = 0;
+
+    // The delete must NOT throw (grant GC is best-effort) but MUST report the partial
+    // success so the caller doesn't claim a clean full success (orphaned grants).
+    const removed = await reg.deleteDef("Agents/uni");
+    expect(removed.name).toBe("uni");
+    expect(removed.grantsReconciled).toBe(false); // FIX 5: the failure is surfaced, not swallowed.
+    // The note-delete + deregister STILL completed (the def IS gone).
+    expect(reg.findLiveByNote("Agents/uni")).toBeNull();
+    expect(calls.deregistered).toEqual(["uni"]);
+    expect(deletes).toEqual(["Agents/uni"]);
+    // The reconcile WAS attempted (prune-all on the removed agent) — it just failed on the hub.
+    expect(reconciled).toEqual([{ agent: "uni", liveConnections: [] }]);
+  });
+
+  test("FIX 5: a SUCCESSFUL grant-reconcile on delete reports grantsReconciled:true", async () => {
+    const { deps } = recorderDeps();
+    const reconciled: Array<{ agent: string; liveConnections: ConnectionSpec[] }> = [];
+    const grants = fakeGrantsClient({ reconciled }); // reconcile succeeds
+    const fetchFn = vaultFetchWithDelete({
+      defs: [{ id: "Agents/uni", content: "role", metadata: { name: "uni" } }],
+      deleteStatus: 204,
+    });
+    const reg = new AgentDefRegistry(deps, { bindings: [binding], fetchFn, grants });
+    await reg.loadAll();
+    const removed = await reg.deleteDef("Agents/uni");
+    expect(removed.grantsReconciled).toBe(true);
+  });
+});

src/agent-defs.ts

@@ -1081,19 +1081,38 @@ export class AgentDefRegistry {
     this.seenDefs.set(vault, next);
   }
 
-  /** Reconcile a CONFIRMED-removed agent's grants away (prune ALL). Best-effort + no-op
-   *  without a grants client / without a known name. */
-  private async reconcileForRemovedAgent(name: string): Promise<void> {
-    if (!this.grants || !name) return;
+  /**
+   * Reconcile a CONFIRMED-removed agent's grants away (prune ALL). Best-effort + no-op
+   * without a grants client / without a known name.
+   *
+   * FIX 5 (PR #3) — make the failure NON-SILENT. A hub-unreachable reconcile used to be
+   * caught + logged + ignored, ORPHANING the agent's approved grants on the hub (a
+   * re-created same-named agent resurrects them). It's still BEST-EFFORT (we don't block
+   * the note delete on grant cleanup — the def IS gone), but we now (a) `console.warn`
+   * loudly AND (b) RETURN a structured signal so the caller can surface a PARTIAL success
+   * (delete succeeded, grant cleanup didn't) rather than claiming a clean full success.
+   * `skipped` = no grants client / no name (nothing to reconcile — a true no-op).
+   */
+  private async reconcileForRemovedAgent(
+    name: string,
+  ): Promise<{ ok: true; pruned: number } | { ok: false; error: string } | { skipped: true }> {
+    if (!this.grants || !name) return { skipped: true };
     try {
       const { pruned } = await this.grants.reconcileGrants(name, []);
       if (pruned > 0) {
         console.log(`agent-defs: pruned ${pruned} stale grant(s) for removed agent "${name}".`);
       }
+      return { ok: true, pruned };
     } catch (err) {
+      const error = (err as Error).message;
+      // NON-SILENT (FIX 5): a swallowed grant-GC failure orphans approved grants on the
+      // hub. Warn loudly + return the failure so the delete path reports partial success.
       console.warn(
-        `agent-defs: pruning grants for removed agent "${name}" failed (continuing): ${(err as Error).message}`,
+        `agent-defs: pruning grants for removed agent "${name}" FAILED — its approved hub ` +
+          `grants may be ORPHANED (re-creating a same-named agent would resurrect them); ` +
+          `the note delete still completed (best-effort grant cleanup): ${error}`,
       );
+      return { ok: false, error };
     }
   }
 
@@ -1248,11 +1267,21 @@ export class AgentDefRegistry {
 
   /**
    * Delete a live def note, then deregister the agent immediately. The note MUST be a
-   * currently-live def we instantiated. Returns the (vault, name) of what was removed.
-   * Throws {@link AgentDefWriteError} on a miss or a delete failure (the deregister
-   * still runs even if it's already gone).
+   * currently-live def we instantiated. Returns the (vault, name) of what was removed,
+   * plus a `grantsReconciled` flag (FIX 5, PR #3) — `false` when the best-effort grant
+   * cleanup FAILED so the caller can report a PARTIAL success rather than a clean one.
+   *
+   * ORDERING (FIX 4, PR #3) — the VAULT NOTE DELETE happens FIRST; only after it
+   * SUCCEEDS do we deregister the live agent. So a vault-delete 502 throws here BEFORE
+   * any in-memory teardown — the def stays REGISTERED (it reappears coherently on the
+   * next poll), never orphaned (gone from memory but still in the vault, the confusing
+   * half-state). This mirrors the `agent-vaults` removal path's "persist the durable
+   * change first, then tear down in-memory state" discipline (daemon.ts #106). Throws
+   * {@link AgentDefWriteError} on a miss; a vault-delete failure throws (un-torn-down).
    */
-  async deleteDef(noteId: string): Promise<{ vault: string; name: string }> {
+  async deleteDef(
+    noteId: string,
+  ): Promise<{ vault: string; name: string; grantsReconciled: boolean }> {
     const found = this.findLiveByNote(noteId);
     if (!found) {
       throw new AgentDefWriteError(`note ${noteId} is not a live agent definition`, 404);
@@ -1261,11 +1290,15 @@ export class AgentDefRegistry {
     if (!client) {
       throw new AgentDefWriteError(`unknown def-vault "${found.vault}"`, 400);
     }
+    // STEP 1 — delete the vault note FIRST (the durable change). A non-ok (non-404)
+    // response throws out of here, BEFORE any deregister, so the in-memory def is left
+    // intact (FIX 4): no orphan, the next poll re-converges. (404 is fine — gone is gone.)
     await client.deleteNote(noteId);
-    // Tear the agent down + prune grants — the confirmed-removal path (a delete IS a
-    // confirmed removal); reload with "deleted" skips the (now-404) GET.
-    await this.reload(found.vault, noteId, "deleted");
-    return { vault: found.vault, name: found.detail.name };
+    // STEP 2 — the note is gone → tear the agent down + prune grants (the confirmed-
+    // removal path). Capture the grant-reconcile outcome to surface a partial success.
+    const reconcile = await this.confirmedRemoval(found.vault, noteId);
+    const grantsReconciled = !("ok" in reconcile) || reconcile.ok === true;
+    return { vault: found.vault, name: found.detail.name, grantsReconciled };
   }
 
   /**
@@ -1296,15 +1329,22 @@ export class AgentDefRegistry {
    * down AND prune ALL its grants (#96 grant-GC) so a deleted `#agent/definition` note
    * doesn't orphan live approved rows. The seen-set entry is cleared so a later loadAll
    * doesn't re-detect (and re-prune) the same removal. Reconcile is best-effort.
+   *
+   * Returns the grant-reconcile outcome (FIX 5, PR #3) so the API delete path can report
+   * a PARTIAL success when grant cleanup failed (delete done, grants possibly orphaned).
    */
-  private async confirmedRemoval(vault: string, noteId: string): Promise<void> {
+  private async confirmedRemoval(
+    vault: string,
+    noteId: string,
+  ): Promise<{ ok: true; pruned: number } | { ok: false; error: string } | { skipped: true }> {
     // The grant holder name comes from the live record if present, else the last-known
     // name we tracked for this note (a def removed before it ever instantiated).
     const name =
       this.live.get(this.keyOf(vault, noteId))?.name ?? this.seenDefs.get(vault)?.get(noteId);
     await this.deregisterByNote(vault, noteId);
     this.seenDefs.get(vault)?.delete(noteId);
-    if (name) await this.reconcileForRemovedAgent(name);
+    if (!name) return { skipped: true };
+    return this.reconcileForRemovedAgent(name);
   }
 
   /**

src/backends/channel-queue.test.ts

@@ -16,6 +16,7 @@ import {
   type ChannelQueueStore,
 } from "./channel-queue.ts";
 import type { AgentSpec } from "../sandbox/types.ts";
+import { InboundClaimConflictError } from "../transports/vault.ts";
 import type { InboundQueueNote, InboundStatus } from "../transports/vault.ts";
 
 /**
@@ -44,14 +45,30 @@ class FakeStore implements ChannelQueueStore {
       .sort((a, b) => (a.ts < b.ts ? -1 : a.ts > b.ts ? 1 : 0));
   }
 
-  async setInboundStatus(id: string, status: InboundStatus, claimedAt?: string | null): Promise<void> {
+  async setInboundStatus(
+    id: string,
+    status: InboundStatus,
+    claimedAt?: string | null,
+    ifUpdatedAt?: string,
+  ): Promise<void> {
     if (this.throwOnNextSetStatus) {
       const e = this.throwOnNextSetStatus;
       this.throwOnNextSetStatus = null;
       throw e;
     }
     const note = this.notes.get(id);
     if (!note) throw new Error(`fake store: no note ${id}`);
+    // CAS (FIX 3): when a precondition is supplied, the claim only lands if the note's
+    // `updatedAt` still matches what the caller last saw — else the race is lost (the
+    // real vault returns 409 → InboundClaimConflictError). On a successful CAS write we
+    // ADVANCE `updatedAt` (the vault bumps it on every write) so a second concurrent
+    // claimer with the now-stale precondition fails, modelling the real round-trip.
+    if (ifUpdatedAt !== undefined) {
+      if (note.updatedAt !== ifUpdatedAt) {
+        throw new InboundClaimConflictError(id, 409);
+      }
+      note.updatedAt = `${ifUpdatedAt}::bumped`;
+    }
     note.status = status;
     if (claimedAt === null) delete note.claimedAt;
     else if (claimedAt !== undefined) note.claimedAt = claimedAt;
@@ -172,6 +189,63 @@ describe("ChannelQueueRegistry — claimNext (single-claim)", () => {
     await expect(reg.claimNext("laptop")).rejects.toThrow(/vault 500/);
     expect(store.notes.get("a")!.status).toBe("pending"); // not lost — retryable.
   });
+
+  test("FIX 3: a passed-through updatedAt is used as the CAS precondition on the claim", async () => {
+    const reg = new ChannelQueueRegistry();
+    const store = new FakeStore();
+    store.add({ ...inbound("a", "older", "2026-06-18T10:00:00Z"), updatedAt: "rev-1" });
+    reg.register(specFor("laptop"), store);
+    const claimed = await reg.claimNext("laptop");
+    expect(claimed!.id).toBe("a");
+    // CAS landed → the store bumped updatedAt (modelling the vault advancing it on write).
+    expect(store.notes.get("a")!.status).toBe("in-flight");
+    expect(store.notes.get("a")!.updatedAt).toBe("rev-1::bumped");
+  });
+
+  test("FIX 3: a 428/409 conflict on the claim PATCH makes claimNext skip to the NEXT pending (no double-claim)", async () => {
+    const reg = new ChannelQueueRegistry();
+    const store = new FakeStore();
+    // Two pending notes, each with a known revision for the CAS precondition.
+    store.add({ ...inbound("a", "older", "2026-06-18T10:00:00Z"), updatedAt: "rev-a" });
+    store.add({ ...inbound("b", "newer", "2026-06-18T10:05:00Z"), updatedAt: "rev-b" });
+    reg.register(specFor("laptop"), store);
+
+    // Simulate a CONCURRENT winner: between claimNext's list and its PATCH of "a",
+    // another session claims "a" and advances its revision. The next PATCH of "a" with the
+    // now-stale precondition will throw InboundClaimConflictError → re-list → claim "b".
+    const realSet = store.setInboundStatus.bind(store);
+    let firstPatch = true;
+    store.setInboundStatus = (async (id, status, claimedAt, ifUpdatedAt) => {
+      if (firstPatch && id === "a") {
+        firstPatch = false;
+        // The "other session" already claimed "a" (its revision moved on).
+        store.notes.get("a")!.updatedAt = "rev-a-claimed-by-someone-else";
+        store.notes.get("a")!.status = "in-flight";
+      }
+      return realSet(id, status, claimedAt, ifUpdatedAt);
+    }) as typeof store.setInboundStatus;
+
+    const claimed = await reg.claimNext("laptop");
+    // The conflict on "a" was caught + re-listed; we claimed "b" instead — never "a" twice.
+    expect(claimed!.id).toBe("b");
+    expect(store.notes.get("b")!.status).toBe("in-flight");
+  });
+
+  test("FIX 3: a conflict with NO other pending returns null (nothing claimable right now)", async () => {
+    const reg = new ChannelQueueRegistry();
+    const store = new FakeStore();
+    store.add({ ...inbound("a", "only", "2026-06-18T10:00:00Z"), updatedAt: "rev-a" });
+    reg.register(specFor("laptop"), store);
+    // Every CAS on "a" loses (the precondition is always stale → conflict). After the
+    // conflict, "a" is left in-flight (by the simulated winner), so the re-list finds no
+    // pending and returns null — not a double-claim, not an error.
+    store.setInboundStatus = (async (id: string) => {
+      store.notes.get(id)!.status = "in-flight";
+      throw new InboundClaimConflictError(id, 409);
+    }) as typeof store.setInboundStatus;
+    const claimed = await reg.claimNext("laptop");
+    expect(claimed).toBeNull();
+  });
 });
 
 describe("ChannelQueueRegistry — reply (outbound + mark handled)", () => {