Skip to content

fix(agents): the attempts counter has no writer, and two drivers have no terminal state - #822

Merged
lilyshen0722 merged 2 commits into
mainfrom
fix/agent-event-lifecycle-terminal-states
Aug 4, 2026
Merged

fix(agents): the attempts counter has no writer, and two drivers have no terminal state#822
lilyshen0722 merged 2 commits into
mainfrom
fix/agent-event-lifecycle-terminal-states

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

attempts is a frozen 0 on 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 attempts incremented."

Nothing increments it in the pending↔delivered cycle. attempts moves only at acknowledge() (:1030) and recordFailure() (: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:

garbageCollect()  attempts < 3        ← reads as a poison-event bound
write sites       acked | failed      ← both terminal, both outside the cycle

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. attempts counts deliveries: first claim 1, requeued redelivery 2. acknowledge() and recordFailure() stop incrementing — with the claim counting, double-writing made a normally-handled event read attempts: 2 and 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 to list() — 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 >= cap to terminal failed. The two are disjoint on attempts alone (< cap vs >= cap), so no document can be touched by both in one run.

Two driver classes had no terminal state at all

driver before consequence
native created 'delivered'; nativeRuntimeService has zero ack/recordFailure calls structurally unackable — every native event entered the pending queue ~10-20 min later and could be handed to an external poller that cannot run it
webhook 'delivered' after a successful POST; requeue has no delivery exclusion the endpoint was called again ~10-20 min later, indefinitely — duplicate delivery across the whole ADR-006 driver class

Both now settle. Native acks on completion and records failure on error, via a two-argument .then rather 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 no ackedAt field. Not in IAgentEvent, 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

21 pass (12 new + 9 existing)   ·   tsc --noEmit  0 errors

M1  drop $inc at the claim                    → 1 fail
M2  expire pass $gte → $gt (off-by-one)       → 2 fail
M3  webhook back to 'delivered'               → 1 fail
M4  native drops attempts: 1                  → 1 fail

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. schedulerService runs this job on */10 and the threshold is also 10 min, so period P and threshold T give [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 agentEventService suite; not the full backend run.

🤖 Generated with Claude Code

lilyshen0722 and others added 2 commits August 4, 2026 06:10
… 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
lilyshen0722 merged commit bb6a822 into main Aug 4, 2026
11 checks passed
@lilyshen0722
lilyshen0722 deleted the fix/agent-event-lifecycle-terminal-states branch August 4, 2026 20:22
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant