fix(agents): the attempts counter has no writer, and two drivers have no terminal state - #822
Merged
Merged
Conversation
… no terminal state
ADR-004 §Event model: "Unacked events stay in the queue and re-deliver on
next poll, with `attempts` incremented." Nothing increments it in the
pending↔delivered cycle. `attempts` moves only at acknowledge() and
recordFailure(), both terminal, so every payload the kernel has ever served
carries `attempts: 0` — the one field CAP obliges drivers to dedup with.
That also froze the requeue's own guard. `attempts < 3` in garbageCollect()
reads as a poison-event bound; it is a predicate on a variable no write path
touches, so it has never once fired. The guard's condition and the variable's
write sites were in complementary states, which is why it looked fine.
Four changes, one lifecycle:
- **list()** increments on the pending → delivered claim. The claim IS the
delivery, so it is the only site that can honour ADR-004:73, and `{new:
true}` means the driver receives the post-increment value. `attempts` now
counts deliveries: first claim 1, requeued redelivery 2.
- **acknowledge() / recordFailure()** stop incrementing. With the claim
counting, a normally-handled event read `attempts: 2` and the field meant
"deliveries plus terminal transitions" — a number no driver can use.
- **garbageCollect()** gains a cap-exhaustion pass. Turning the counter on
without one is worse than leaving it off: a capped event fails the requeue
predicate and a 'delivered' row is invisible to list(), so it would sit
stuck for the full 168h retention — the exact Task #67 symptom the requeue
exists to fix, recreated by its fix. The two passes are disjoint on
`attempts` alone (< cap vs >= cap).
- **Two drivers reach a terminal state.** Native events were created
'delivered' and nativeRuntimeService has no ack or recordFailure call at
all, so they were structurally unackable; they now settle on the run's
outcome. Webhook events were left 'delivered' after a successful POST, and
the requeue has no `delivery` exclusion — so every handled webhook event
was re-POSTed ~10-20 min later, indefinitely, across the whole ADR-006
driver class.
Also drops `ackedAt: {$in: [null, undefined]}` from the requeue predicate.
There is no `ackedAt` field on AgentEvent — not in IAgentEvent, not in the
schema, never written — so Mongoose stripped it and the clause matched every
document while reading as a live narrowing. `status: 'delivered'` already
excludes acked events.
Not fixed here, and now documented at the site: effective redelivery latency
is 10-20 min, not 10. schedulerService runs this job on `*/10` and the
threshold is also 10 min, so period P and threshold T give [T, T+P). ADR-004
says "next poll"; drivers are told 3-10s. That gap is a design question, not
a bug fix, and it wants its own PR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lilyshen0722
added a commit
that referenced
this pull request
Aug 4, 2026
… never existed (#825) ADR-004 was frozen 2026-04-14 and has not been read against the code since. Four seats found three separate divergences in one afternoon, independently, while working on unrelated PRs. Three findings in one day is a fact about the document, not about the code. C1 — `createdBy` is not a field. ADR-004 named it three times (Auth, invariant 5, install lifecycle step 2) and ADR-006 six more times including its own audit claim. `grep -c createdBy models/AgentRegistry.ts` → 0. The field is `installedBy`. Not a typo: a term of art that spread between documents while never existing in the schema. The naming is the small half. On agent-initiated installs the value is the AGENT's User id (agentsRuntime.ts:2593/:2650/:2740, agentAutoJoinService.ts:80), so "every agent action traces back to a human" is false there and invariant 5 does not hold. And the obvious fix is wrong, which is the part worth recording. `installedBy` is also a live authorization predicate — reactionController.ts:50-55 gates agent reactions on `findOne({podId, installedBy: req.agentUser._id})`, which only matches rows where the field IS the calling agent. Rewriting the four write sites to store a human would silently drop every agent to its Pod.members fallback. The field carries two incompatible meanings and one gate depends on the second; restoring the invariant needs a separate field. Filed, not fixed. C2 — `attempts` was a frozen 0 until today. Fixed in #822; recorded here with the new semantics (counts deliveries, incremented at the claim) and the fact that invariant 8's "re-delivers" is now bounded by a 3-attempt cap. C3 — "re-deliver on next poll" is a 10-20 minute server-side sweep. Cron `*/10` against a 10-min threshold gives [T, T+P); against a spec that guides drivers to 3-10s that is a 60-400x divergence. Still open, wants a lease design rather than a quiet behaviour edit. Markers are INLINE at each divergent bullet, not only in the section. A conformance block at the bottom is invisible to a reader who jumps to `### Auth` or greps for `attempts` — which is exactly how ADR-012's rolled-back heartbeat cue survived three months with its correction already written forty lines below it (PR #818). Co-authored-by: Claude Opus 5 <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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
attemptsis a frozen0on every payload the kernel has ever served, and the guard that reads it has never fired. Fixing that alone would have made things worse, so this is the whole lifecycle in one PR — at @pod-architect's suggestion, since the driver fixes live in the same thirty lines.The counter
ADR-004 §Event model: "Unacked events stay in the queue and re-deliver on next poll, with
attemptsincremented."Nothing increments it in the pending↔delivered cycle.
attemptsmoves only atacknowledge()(:1030) andrecordFailure()(:1134) — both terminal. So the field CAP obliges every driver to dedup with is constant, and a driver has no way to tell a redelivery from a first delivery.That also froze the requeue's own guard:
The guard's condition and the variable's write sites are in complementary states, which is exactly why it looks fine in review. It has never once fired.
list()now increments on the pending → delivered claim. The claim is the delivery, so it's the only site that can honour:73, and{new: true}means the value the driver receives is the post-increment one.attemptscounts deliveries: first claim1, requeued redelivery2.acknowledge()andrecordFailure()stop incrementing — with the claim counting, double-writing made a normally-handled event readattempts: 2and left the field meaning "deliveries plus terminal transitions," which is not a number anyone can dedup on.Why the counter needed a cap in the same PR
Turning it on without a cap-exhaustion pass is worse than leaving it off. A capped event fails the requeue predicate, and a
'delivered'row is invisible tolist()— so it sits stuck until the 168h retention delete. That is precisely the Task #67 symptom the requeue exists to fix, recreated by its own fix.Second pass retires
attempts >= capto terminalfailed. The two are disjoint onattemptsalone (< capvs>= cap), so no document can be touched by both in one run.Two driver classes had no terminal state at all
'delivered';nativeRuntimeServicehas zero ack/recordFailure calls'delivered'after a successful POST; requeue has nodeliveryexclusionBoth now settle. Native acks on completion and records failure on error, via a two-argument
.thenrather than.then().catch()— a chained catch would also catch a rejection from the success handler and mark a run that actually succeeded as failed. There's a test for that inversion specifically.The phantom predicate
ackedAt: { $in: [null, undefined] }— there is noackedAtfield. Not inIAgentEvent, not in the schema, never written; Mongoose strips it, so the clause matched every document while reading as a live narrowing. Dropped rather than "fixed":status: 'delivered'already excludes acked events. Caught by @pod-architect.Verification
Assertions are on query shape, not on observed counts — this is a mocked-model suite, and a test that only checked "an update happened" would pass against all four bugs.
Not fixed here, documented at the site
Effective redelivery latency is 10-20 min, not 10.
schedulerServiceruns this job on*/10and the threshold is also 10 min, so periodPand thresholdTgive[T, T+P)— uniform, mean ~15. ADR-004 says "next poll" and tells drivers 3-10s. Closing that gap means a lease or a short-TTL claim, which is a design question and wants its own PR against ADR-004.Not verified: no DB or cluster read — I can show these paths are reachable and cannot show how many rows are in each state today. The webhook duplicate-delivery claim in particular is derived from the predicate, not observed against a live endpoint. Suites run: this one plus the existing
agentEventServicesuite; not the full backend run.🤖 Generated with Claude Code