This repository was archived by the owner on Aug 10, 2026. It is now read-only.
fix: error-handling hardening — outbound retry, CAS claim, delete ordering, grant-GC, queue cap - #116
Merged
Conversation
…aths (PR #3) Six post-audit point-fixes in the load-bearing reply-delivery, claim, and def-lifecycle paths. The audit flagged silent reply-loss as the scariest failure mode for an agent module; each fix has a regression test. FIX 1 (registry.ts) - vault 5xx/network during the outbound write no longer silently loses the reply. The outbound write now gets a BOUNDED retry (2 retries, linear backoff) on a TRANSIENT error (5xx or no-status network), but NOT on a 4xx (a real rejection). On a persistent failure the live view resolves to error (not done) AND the #agent/thread note is RE-RECORDED as status:error carrying the undelivered reply text - so the durable record never falsely claims a clean "ok". The claude -p turn is never re-run (no fork/quota burn); only the idempotent outbound WRITE retries. Backoff base is injectable for fast tests. FIX 2 (programmatic.ts) - mid-turn vault-token expiry: ASSESSED + DEFERRED (documented, no code). The token is minted FRESH per turn at the hub default ~90d TTL, so it cannot expire during a minutes-long turn; and the vault writes happen inside the opaque claude -p subprocess via the token baked into its .mcp.json, so the backend has no in-process seam to observe a 401 and re-mint. A re-mint-on-401 is infeasible at this layer + unnecessary; the real fix (if a long/short-TTL turn ever makes it real) is MCP-client refresh-on-401, flagged as a follow-up. FIX 3 (channel-queue.ts, vault.ts) - claimNext double-claim race. The claim PATCH is now a COMPARE-AND-SWAP: it carries if_updated_at (the note's last-seen updated_at); the vault returns 409 (stale precondition) / 428 (precondition required) when the race is lost, surfaced as a typed InboundClaimConflictError, on which claimNext re-lists and tries the next pending message instead of double-claiming. updated_at is threaded through InboundQueueNote. release/handled/sweep keep last-write-wins (force). FIX 4 (agent-defs.ts) - deleteDef now deletes the vault note FIRST, then deregisters the in-memory agent (mirrors the agent-vaults removal ordering): a vault-delete 502 throws before any teardown, leaving the def REGISTERED (it re-converges on the next poll) instead of orphaned in the confusing gone-from-memory-but-still-in-vault half-state. FIX 5 (agent-defs.ts, daemon.ts) - a grant-reconcile failure on def-delete is no longer silently swallowed. It is still best-effort (does not block the delete), but now warns loudly AND returns a grantsReconciled:false partial- success signal so the delete path/route reports that approved hub grants may be orphaned rather than claiming a clean full success. FIX 6 (vault.ts) - listInboundQueue no longer lets accumulated handled notes crowd pending out of the query cap. handled notes are excluded client-side (only pending + in-flight are the actionable queue) and the vault query is requested newest-first so a hard cap drops the oldest handled notes, never a recent pending. status is not indexable per-vault, so a server-side filter is a future scale optimization. Gates: typecheck 0 errors; bun test 970 pass / 0 fail; vitest 87 pass / 0 fail. Closes #101 Closes #96 Closes #103 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ount) PR #3 reviewer (af47de6b, LGTM-with-nits) caught the one structural nit in FIX 1: the outbound-failure path called recordThread a SECOND time for the same turn, so single-threaded re-read turn_count=N and wrote N+1 (the turn counted twice), and multi-threaded minted a SECOND per-fire note (fresh uuid) for one turn. Fix: thread a stable per-TURN id (turnThreadId) through every recordThread for the turn, and a `sameTurn` flag on the failure re-record. writeThread then reuses that leaf for multi-threaded (one note, not a duplicate) and, for single-threaded with sameTurn, keeps the existing turn_count instead of incrementing. The note still ends on status:error with the undelivered reply — just counted once. Tests: single-threaded same-turn re-record keeps turn_count==1 + flips to error; multi-threaded re-record reuses the threadId leaf (both writes hit one path). Declined the 3 cosmetic nits (dead camelCase fallback, two comment-only items). Gate: bun run test:all -> typecheck 0, bun 972/0, vitest 87/0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
unforced
added a commit
that referenced
this pull request
Jun 22, 2026
…he CAS single-claim (#128) The production adapter wiring the pull-queue to a VaultTransport (channelQueueStoreFor, daemon.ts) declared `setInboundStatus` as a 3-param arrow `(id, status, claimedAt) => vt.setInboundStatus(id, status, claimedAt)` — silently DROPPING the 4th `ifUpdatedAt` arg. VaultTransport.setInboundStatus does a CAS (`if_updated_at`) only when that arg is present, else falls back to `force:true` (last-write-wins). So in production every claim was force:true → the compare-and-set single-claim guard (agent#101, the whole point of PR #116) was SILENTLY DISABLED, and the double-claim race (two connected sessions grabbing the same inbound) was re-opened for channel-backend agents. The bug was invisible to the type checker (a narrower arrow is assignable to the wider interface slot) and to the tests (they inject a FAKE ChannelQueueStore that honors all four args — only the live daemon adapter was lossy). Fix: forward all four args. New test exercises the REAL adapter against a VaultTransport (records the PATCH body): with ifUpdatedAt → `if_updated_at` CAS (not force); without it → `force:true` (the release/handled/sweep path, unchanged). Found by the agent-module deep audit. Gate: typecheck clean; `bun test ./src` 1022 pass / 0 fail. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Error-handling + robustness hardening (post-audit, PR #3)
Six robustness fixes in the load-bearing turn-delivery / claim / def-lifecycle paths — led by the audit's scariest class (silent reply loss). Closes #101, #96, #103. No version bump.
registry.ts: a transient outbound failure now gets a bounded retry (isTransientOutboundError: 5xx/network retry, 4xx give up); on persistent failure the live view resolves toerrorAND the thread note is re-recordedstatus:errorcarrying the undelivered reply (no false "ok"); theclaude -pturn is never re-run. Reviewer-caught nit folded: the re-record now reuses a stable per-turnthreadId+ asameTurnflag so it updates the same note instead of double-countingturn_count(single) / minting a duplicate (multi) — with two new transport tests.setInboundStatusdoes CAS (if_updated_at) + throwsInboundClaimConflictErroron 409/428;claimNextre-lists + skips to the next pending on conflict (bounded). Verified the cross-repo seam: the vault returns 409 on staleif_updated_at, 428 on missing precondition.grantsReconciled:false+ the daemon warns; delete still completes (best-effort).listInboundQueueexcludeshandled+ requestssort=descso the cap drops oldest-settled, never recent-pending.claude -psubprocess → no in-process re-mint seam). Documented, flagged as a follow-up.Review + gates
Independent reviewer: LGTM-with-nits; the one structural nit (FIX 1 double-count) is fixed + tested in this PR; the 3 cosmetic nits declined.
bun run test:all→ typecheck 0, bun 972/0, vitest 87/0. (These failure paths are hard to E2E without inducing the exact faults; the unit tests exercise the real failure windows + I verified the FIX-3 cross-repo seam against the vault source.)🤖 Generated with Claude Code