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>
lilyshen0722
added a commit
that referenced
this pull request
Aug 18, 2026
…rescued (#1001) * fix(agents): the collector destroyed the events the requeue had just rescued Closes #993. `garbageCollect()` ran its requeue and its pending delete in the same `Promise.all`, keyed on different fields: the requeue sets `status` but never touches `createdAt`, and the delete matched on `createdAt` against a 30-minute threshold. So an event the requeue had just moved back to `pending` was destroyed in the same function call, before any poller could see it — seats poll every 5s, and the three events sprint-review traced were fetched zero times between rescue and deletion. Measured on one instance over one hour: 38 pending events destroyed, with the collector's own log pairing the two passes per run — 11:00:00 "requeued 17 stuck 'delivered' events", then deletedPending=17, 0.15s apart. Seven of the 38 were traced to specific agent turns that never happened. The other 31 are unrecoverable: deletion leaves no row, no status and no error, and the per-seat wrapper logs carry no timestamps. The fix is the horizon, not the mechanism. Pending now ages out on the same instant as delivered and acked. It has to be the SAME instant rather than merely longer, because any shorter pending window re-opens the same race — against an 18-minute retry ceiling (15min x 1.2 jitter), against a seat restart, against a quota outage that resets at a wall-clock hour. Four narrower predicates were proposed and discarded in the issue thread; each failed on a case the horizon change handles for free. It also makes the retire-to-'failed' path reachable for the first time. That path has never fired: 0 documents at status='failed' and 0 with attempts>=3, and git dates the two causes back to back — nothing wrote `attempts` until #822 (2026-08-04), and since then this delete has pre-empted a cap that needs three deliveries at 10-20 minute spacing. With room to run, cap-exhausted events retire with a reason, which is the dead-letter surface ux-lead escalated for, obtained by deleting code rather than adding it. AGENT_EVENT_STALE_PENDING_MINUTES is deliberately no longer read here. It still governs the admin dashboard's stale-pending count, where it is now actionable rather than a countdown to destruction. Tests pin the horizon rather than the mechanism, and were run red first: both fail on unmodified main, the 16 existing lifecycle tests pass unchanged. Reported-by: sprint-review Reported-by: ux-lead Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(agents): two rationales rested on the deletion this PR removes Both are mine, both from 2026-08-15, and both cite the 30-minute pending delete as load-bearing. The commit that invalidates a reason should carry its correction rather than leave it for a grep. OnboardingSilenceEpisode: "WHY THE EVENT SNAPSHOT IS NOT OPTIONAL" argued from a race — the evidence exists for fifteen minutes and never again, because deletion lands at thirty. That race is gone. The snapshot is still not optional, for a reason that outlives any retention setting: the rows survive but their STATUS does not. `pending` becomes `delivered` becomes `acked`, so a later query reports the queue as it ended, not as it was when the alert fired, and the producer-bug/runtime-bug discriminator is a statement about a moment. onboardingSilenceService: the 15-minute threshold was derived by bracketing — 10 below (a legitimate retry) and 30 above (deletion), "15 sits between them". The upper bracket no longer exists, so only the lower constraint still derives the number. 15 stays, but as a floor plus a judgement about how long a newcomer should wait, not a midpoint — re-deriving it from the constants will no longer reproduce it, and the comment now says so instead of letting the next reader get a different answer and assume they'd erred. Reported-by: sprint-review Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(agents): four more rationales rested on the deletion, and my grep missed them 2dcd263 claimed "other consumers, now checked exhaustively" and listed three. The grep behind it was scoped to the identifier — AGENT_EVENT_STALE_PENDING_MINUTES and stalePendingMinutes — which finds code that reads the constant and none of the prose that paraphrases it. Searching the behaviour instead of the name finds four more, in three files the first sweep never opened: - onboardingSilenceService:65 "inside the pending-GC window (30m)" — the upper bracket of the 15-minute threshold, gone. - onboardingAlertService:98 "only possible before the 30-minute pending-GC deletes the evidence" — still time-bound, but by 168h retention and by status transitions, not by a sweep. - schedulerService:383 "a delayed AgentEvent inherits the pending-GC caveat" — the caveat it names no longer exists. - stalledConnectService:25 "WHY A CRON AND NOT A DELAYED EVENT" — an architectural choice justified by a hazard this PR removes. The last is the one worth reading twice: the cron is still right, but for the half of the original argument that survives — re-deriving from state is drop-proof against ANY delivery failure, not just against a sweep. The comment now says which half is load-bearing, so nobody reopens the decision on the grounds that events survive 168h now. Also fixes two flaws in 2dcd263's own replacement text, found auditing it: "a reason that outlives any retention setting" overclaimed (rows age out at 168h), and the snapshot's second justification was missing — the field is a WINDOWED count, so `noneEnqueued` degrades from fact to inference as soon as later traffic lands in the pod, independently of any status change. Reported-by: sprint-review Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(agents): ">10" is not a derivation, and inertia is not a constraint ecdc4d4 removed the stale upper bound and left the number described as "a floor plus a judgement". @sprint-review's point: that leaves ">10" as the only surviving constraint, which admits 25 or 60 as readily as 15. The value would then be correct by inertia, with nothing on record to stop the next editor moving it — the same failure as the stale comment, one step later. Re-derives the ceiling from the reply distribution rather than from the collector, using the measurement already in this file: every genuine reply over 21 days landed inside 107 SECONDS and the next cluster was 10+ hours. Nothing legitimate occupies the gap, so time spent above the floor buys no accuracy and only delays the alert. The instruction is now explicit — keep it as near the 10-minute floor as the floor allows, raising it is a pure loss — and it depends on nothing this PR changed. Reported-by: sprint-review Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- 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