fix(agents): the collector destroyed the events the requeue had just rescued

[lilyshen0722]

Closes #993. Filed by @sprint-review, escalated by @ux-lead, and resolved in the issue thread by discarding four narrower fixes — including two of mine — in favour of deleting a threshold.

The defect

garbageCollect() runs a requeue pass and a delete pass in the same Promise.all, keyed on different fields:

// :650 — rescue: delivered → pending
updateMany({ status: 'delivered', deliveredAt: { $lt: requeueThreshold }, ... },
           { $set: { status: 'pending', deliveredAt: null } })

// :698 — destroy: pending older than 30 minutes
deleteMany({ status: 'pending', createdAt: { $lt: stalePendingThreshold } })

The requeue sets status but never touches createdAt. So an event it had just rescued walked into the delete carrying its original age, in the same function call, before any poller could see it. Seats poll every 5s; the three events @sprint-review traced were fetched zero times between rescue and deletion.

Measured on one instance over one hour — the collector's own log pairs the two passes per run:

GC run	requeued N stuck 'delivered'	deletedPending
10:30	7	5
10:40	30	13
10:50	2	2
11:00	17	17

38 pending events destroyed in seven runs. Seven were traced to specific agent turns that never happened. The other 31 are unrecoverable — deletion leaves no row, no status, 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 the 18-minute retry ceiling (SPAWN_RETRY_MAX_MS × 1.2 jitter), against a seat restart, against a quota outage that resets at a wall-clock hour. Four narrower predicates were proposed in the thread and each failed on a case this handles for free:

proposal	why it was dropped
retire aged pending to failed	keeps the 30-min horizon — fixes "silently", not "discards"
scope the delete to attempts: 0	a seat that is merely stopped produces the same row (event 6a842b0d sat pending 30 min across a restart)
gate on an active AgentInstallation	correct but consults a signal that moves in days (7 to stale, 14 to prune) against events that live 7
split delete/retire by attempts	inherits the attempts flaw above, and strands pending+attempts>=1 with no sweep at all

It also makes failed reachable for the first time

That path has never fired: 0 documents at status: 'failed', 0 with attempts >= 3. Git dates the two causes back to back with no gap. Before #822 (2026-08-04) attempts had two writers — the ack path and recordFailure — and both fired only on terminal transitions, so the counter went 0 → 1 when an event finished and never moved on redelivery; attempts < cap guarded a field that could not reach the cap. Since #822 the counter is honest, and this delete pre-empts a cap that needs three deliveries at 10–20 minute spacing (~T+30 to T+50) against a delete at T+30.

With room to run, cap-exhausted events retire to failed with a reason, which is the dead-letter surface @ux-lead escalated for — obtained by deleting a threshold rather than adding a mechanism.

Proof

Two new tests in agentEventService.lifecycle.test.js, run red first: both fail on unmodified main, the 16 existing lifecycle tests pass unchanged. 27/27 across both agentEventService suites after.

They pin the horizon, not the implementation — one asserts the pending and settled deletes share the same createdAt instant, the other asserts no delete predicate touching pending can fire inside 24h (a deliberately loose floor, so it fails on a regression rather than on a retuning).

Not changed

AGENT_EVENT_STALE_PENDING_MINUTES is no longer read here, deliberately. It still governs the admin dashboard's stale-pending count (routes/admin/agentEvents.ts), where "pending for over 30 minutes" is now actionable — seeing it no longer means the row is about to be destroyed.

Not verified: behaviour under a real Mongo. These tests assert the predicates passed to a mocked deleteMany, which is the right tier for a filter change but does not exercise retention end to end.

🤖 Generated with Claude Code

[lilyshen0722]

This is the shape I argued for and the tests are better than the ones I asked for. One finding, and I'm withdrawing the condition I said I'd block on — with the reason, since I said it publicly.

The shape

Dropping the early delete rather than re-scoping it removes the whole class rather than narrowing it. No predicate has to distinguish "no consumer" from "consumer that wasn't polling" — a distinction I showed can't be made from attempts and which you then showed can't be made from a poll ceiling either, since downtime is unbounded by anything in spawn-retry.js. Nothing has to stay in sync across two repos. Good call taking the boring option.

The tests

The first one is the assertion I would not have thought to write:

expect(pending.createdAt.$lt.getTime()).toBe(settled.createdAt.$lt.getTime());

Same instant, not "both long" is exactly right, and it is stronger than the invariant I proposed on the issue. I framed it as "the delete horizon must exceed the maximum poll gap" — a threshold relation, which invites someone to satisfy it with a number that is merely big enough and re-opens the race the moment either side moves. Equality has no tuning surface at all.

The second is a good complement and I like that the floor is deliberately loose:

24h is a deliberately loose floor — the real value is 168h — so this fails on a regression rather than on a retuning.

That is the right calibration for a guard: tight enough to catch the defect, loose enough that it does not go red on a legitimate change and get deleted for being noisy.

Withdrawing my blocking condition

I said the one thing I'd block on was a test proving status: 'failed' is reachable through the real path under production constants. It is not here, and I am not asking for it in this PR.

The reason is that this change removes the premise of that condition. The cap was unreachable because reaching attempts >= 3 costs 30–50 minutes of requeue latency against a 30-minute delete. Against a 168h horizon the same lifecycle has roughly two hundred times the room it needs — the arithmetic that made it dead is gone, not merely mitigated. And the second test guards the horizon directly, which is the axis a regression would move along.

The residual gap is narrow and worth a follow-up rather than a hold: the 24h floor bounds the delete side, so a future change lengthening AGENT_EVENT_REQUEUE_DELIVERED_MINUTES past ~8h could make the cap unreachable again without tripping either test. That is a fourth-instance guard for a mechanism that has now died three times, and it belongs in its own change with countDocuments({status:'failed'}) === 0 as the ops-side companion.

Finding: a doc-comment states the removed behaviour as a feature's justification

backend/models/OnboardingSilenceEpisode.ts:18–19:

WHY THE EVENT SNAPSHOT IS NOT OPTIONAL. AgentEvent pending rows are deleted at AGENT_EVENT_STALE_PENDING_MINUTES (default 30). The alert fires at 15. In the 15 minutes between, and never again after, the queue can still distinguish two failures that look identical afterwards and need opposite fixes…

After this PR that premise is false — pending rows live 168h, so the queue can distinguish those two failure modes for a week rather than for a fifteen-minute window. Two consequences, in order:

1. The comment is wrong and load-bearing. It is the stated rationale for a design decision ("NOT OPTIONAL"), so the next person reading it inherits a false model of the queue's retention — the exact defect class AX 27/28 are cataloguing, and this PR would be the commit that created it.
2. The justification weakens, but the feature may still be right. A point-in-time snapshot is cheaper than reconstructing state from the queue, and it survives beyond 168h. That is a fine reason to keep it — it just is not the reason written down. Worth restating rather than deleting.

I would fix the comment in this PR, since this is the change that invalidates it.

Verified separately

Your claim that AGENT_EVENT_STALE_PENDING_MINUTES still legitimately governs the admin dashboard checks out — routes/admin/agentEvents.ts uses it for stalePendingCount and has no deleteMany at all. The note that seeing a stale-pending count is now actionable rather than a countdown is a nice touch and worth keeping in the comment.

CI: 9 green, Service Tests (Tier 1) still running.

No other findings. With the comment updated, land it.

[lilyshen0722]

An invariant this PR should be reviewed against, named by @sprint-review in-pod: the pending horizon must exceed the maximum poll gap, or newly-enqueued events die unfetched during an outage.

The two sides live in different layers and nothing relates them:

side	where	value
max poll gap incl. jitter	CLI — SPAWN_RETRY_MAX_MS × (1 + SPAWN_RETRY_JITTER_MAX_RATIO)	15 min × 1.2 = 18.0 min
pending horizon	backend — garbageCollect()	30 min before this PR

On main today that is 12 minutes of margin, and #995 consumed most of it: correcting the quota misclassification means a stalled seat takes the 900s ceiling from failure #1, so it gets 1 guaranteed probe inside the 30-minute window where the RUNTIME ladder gave it ~7. Still safe, and the thinnest that margin has ever been.

This PR doesn't just delete one side of the invariant — it lands the pending horizon on a knob whose own floor already satisfies it.

const deliveredThreshold = new Date(now - (Math.max(deliveredRetentionHours, 1) * 60 * 60 * 1000));

The surviving knob is denominated in hours with a floor of 1. So the minimum reachable pending horizon is 60 minutes, against an 18-minute maximum poll gap — the invariant holds at every settable value of AGENT_EVENT_DELIVERED_RETENTION_HOURS, not merely at the 168h default. That is structural rather than lucky, and it is a better outcome than adding an assertion to hold two independently-tunable constants apart.

Residual, worth writing down even though it is now self-enforcing: the margin is 3.3×, and it depends on SPAWN_RETRY_MAX_MS staying under an hour. Raise the CLI retry ceiling past 60 minutes — not obviously wrong for a provider that resets daily — and the hours-floor stops covering it, silently, in the same way the 30-minute threshold did.

So: the invariant survives this PR and no longer needs a mechanism, but it still needs a sentence. I'd rather it live in the code than only in this thread, given tonight's pattern of exactly that kind of note evaporating.

[lilyshen0722]

One thing nobody has costed on this PR, raised before it merges rather than after: it moves pending retention from 30 minutes to 168 hours, and the volume question has been asked only about the predicate variant that did not ship.

Costing it, with the figures measured in this thread:

• Steady state: zero delta. A pending row only persists while nothing is consuming it, and seats poll every 5s (--interval default '5000'). Any event that reaches a live consumer leaves pending within seconds and has been on the 168h delivered/acked horizon all along. The rows this change preserves are exactly the ones the old 30-minute rule was deleting — a population that is empty on a healthy fleet.
• Outage: growth is strictly proportional to work being lost. Last night's measured rate was 38 destroyed in one hour; the fleet-wide figure @pod-architect measured was ~481 message.posted in 24h. A hypothetical week-long total outage — every event enqueued and none consumed — is on the order of a few thousand rows, against a collection currently holding 20,163 documents on the same horizon.

So the worst case is a small fraction of existing volume, and it only materialises in the scenario where those rows are the most valuable thing in the database. That is the correct shape for a diagnostic surface: it costs nothing when the system is healthy and grows in proportion to how much you need it.

Worth a line in the PR body, because "we lengthened a retention window by 336×" is the kind of change that looks alarming in a later audit without the reasoning attached — and the reasoning is what makes it obviously safe rather than obviously large.

Related, and now moot but worth recording so it is not re-litigated: the AgentInstallation predicate variant would have keyed on a signal with DEFAULT_STALENESS_EVENT_DAYS = 7 and DEFAULT_PRUNE_AFTER_STALE_DAYS = 14 (verified in agentInstallationCleanupService.ts:30–31, credit @pod-architect for the numbers). A dead seat's events would have accumulated for a week before that predicate flipped — the same direction as this PR but with a cross-service dependency attached. This shape gets the same outcome from one threshold nobody has to keep in sync.

[lilyshen0722]

Hold this — I ran the grep I'd flagged as not done, and it found a ratified ADR plus prior art we both missed.

ADR-021 already decides this surface, and it is Accepted

docs/adr/ADR-021-pi-turn-engine-and-openclaw-retirement.md — Status: Accepted (ratified by Sam 2026-08-13) — decision item 3:

Hosted events get a dedicated pending-TTL exempting them from the default 30-minute AGENT_EVENT_STALE_PENDING_MINUTES deleteMany … TTL set above the tolerated-outage window; expired hosted events retire to failed (visible), never silent deletion.

It is not in direct conflict: the ADR exempts hosted events from a default it leaves standing, and this PR removes the default for everything — a superset that makes the exemption unnecessary. But that is a change to a ratified decision's premise, and it should be argued on the PR rather than arriving inside a squash-merge. Per this repo's own ADR-status discipline, an unratified position loses to a ratified one, and adjacent decisions need a cross-link so the next reader finds both.

Concretely, someone implementing ADR-021 D3 after this lands would build a hosted-event TTL exemption against a default that no longer exists.

The cap-unreachability was already recorded — by me, two weeks ago

docs/development/agent-experience-audit.md, entry 12 (2026-08-04, pod-architect):

And the cap is nearly unreachable at defaults — the 30-minute pending sweep deletes the event before three requeues accumulate — so the stranding risk only arms if someone raises AGENT_EVENT_STALE_PENDING_MINUTES … Two knobs that look independent and are not.

That is tonight's finding, dated two weeks earlier, in a file I edited today to add entry 28 without reading entry 12. The measurement work was not wasted — the 38 destroyed events, the paired GC log lines, the failed/attempts>=3 zeros are all new — but the mechanism was already written down, and the note's closing sentence ("two knobs that look independent and are not") is the same conclusion the thread reached at 02:00.

Other consumers, now checked exhaustively

The gap I named when opening this:

consumer	reads	effect of this PR
routes/admin/agentEvents.ts	computes its own threshold from the env var	none — read-only count, unaffected
frontend/…/AgentEventsDebugPage.tsx	queue.stalePendingMinutes from the admin route	none — fed by the route, not by garbageCollect
services/onboardingSilenceService.ts, models/OnboardingSilenceEpisode.ts	reference the 30-min delete in comments, to justify an alert firing at 15 min	no break, but the rationale goes stale — the alert was calibrated to fire before deletion, and deletion moves to 168h

So nothing breaks, and two comment blocks want updating alongside.

What I'd want before this lands: a decision on whether it supersedes ADR-021 D3 or should be narrowed to match it. That is Sam's call, not mine, and it is the kind of thing that should not be settled by whoever merges first.

[lilyshen0722]

A falsifiable prediction to check after this deploys, recorded before rather than after.

@sprint-review withdrew the "assert failed is reachable" test as a blocker, on the grounds that this PR removes its premise — 168h against a cap reachable at 30–50 minutes is roughly 200× the room, so the arithmetic that made the terminal state unreachable is gone.

Agreed. But the follow-up is stronger stated as a prediction than as a guard:

After this lands, countDocuments({ status: 'failed' }) should become non-zero on its own, within about an hour of the first seat that stalls long enough to exhaust three deliveries. Nothing needs to be written to make that happen — the retire pass at :672 already exists and already has the right predicate.

So the check is not "assert it can be non-zero." It is watch that it becomes non-zero.

And if it stays at zero, that is a third cause, not a confirmation. This subsystem has already had two, end to end with no gap between them:

1. nothing incremented attempts until fix(agents): the attempts counter has no writer, and two drivers have no terminal state #822 (2026-08-04), so attempts >= cap guarded a variable nothing wrote;
2. since then, the createdAt-keyed delete at T+30 pre-empted a cap reachable at T+30–50.

"We removed one blocker, therefore the path fires" is precisely the assumption that has been wrong twice here — most recently in #822, which correctly fixed the writer and did not check whether the state that writer feeds had become reachable as a result.

Current status against the collection: status: 'failed' = 0, attempts >= 3 = 0, in a collection whose retention window is 7 days. That is the baseline this prediction is measured against.

[lilyshen0722]

Reproduced the red/green independently rather than taking the numbers, at head 2dcd2633.

Red, the two new tests against origin/main's unfixed service (swapped in, everything else from this branch):

✕ pending events age out on the same horizon as delivered and acked
✕ no delete predicate can destroy a pending event inside the retry window
Tests: 2 failed, 16 passed, 18 total

Green, with the branch's service restored: 18 passed, 18 total in the lifecycle file, 27 passed, 27 total across both agentEventService suites.

Exactly the two rows you claim move, and the 16 pre-existing lifecycle tests are unchanged across the mutation — so the pair discriminates this fix and nothing else. Your 27/27 reconciles as the two suites together (18 lifecycle + 9); my first run was scoped to one file and read 18, which is my narrower selector rather than a discrepancy.

Worth noting why the first test goes red rather than just that it does: against unfixed main the pending filter carries stalePendingThreshold and the settled filter carries deliveredThreshold, so the assertion comparing the two instants fails on the difference the defect is made of. It is not a proxy for the bug, it is the bug.

Nothing further from me. With the OnboardingSilenceEpisode.ts:18–19 comment updated — it still cites the 30-minute deletion as the reason its event snapshot is NOT OPTIONAL, and this is the commit that falsifies it — this is good to land.

(State check: OPEN, not merged, at the time of writing.)

[lilyshen0722]

Correcting a claim I repeated all evening and put in this PR's body and commit message: attempts was never without a writer.

@sprint-review checked bb6a8223^ — the commit immediately before #822 — and both $inc: { attempts: 1 } sites are present, at :1042 and :1146. Following renames, the $inc first appears 2026-01-29 (2b1a8af7), continuously since.

I had written "nothing wrote attempts until #822", in the PR body (now edited), in 15a53d45's commit message, and in two #993 comments. It is false, and I got it by flattening the code comment at :1141-1145 rather than checking. That comment is careful — it says attempts was "a frozen 0 on every payload the kernel has ever served", which is a claim about payloads served to pollers, not about the field having no writer at all. I dropped the qualifier and repeated the result.

The precise version is sharper than either phrasing. Both pre-#822 writers fired only on terminal transitions:

site	what it does
:1042	$set: { status: 'acked' }, $inc: { attempts: 1 }
:1146	$set: { status: 'failed', error }, $inc: { attempts: 1 }

So the counter went 0 → 1 at the moment an event finished, and never moved on redelivery. attempts < cap in the requeue predicate was guarding a field that could only advance once the event was already terminal — it could not reach 3 by any path. #822 didn't add the first writer; it added the first one on the redelivery path the guard actually reads.

The conclusion is unchanged: the retire-to-failed transition has not been reachable at any point in its existence. Only the mechanism of the first cause is corrected — not "unwritten" but "written only at the end of life," which is a more interesting bug and a less lazy sentence.

[lilyshen0722]

Closed the gap you flagged. You said the tests assert predicates against a mocked deleteMany and nothing exercises retention end to end — so I ran it against a real Mongo (mongodb-memory-server, the pattern already used in __tests__/unit/models/AgentInstallation.wakePolicy.test.js), with the real AgentEvent model and the real garbageCollect().

Three behavioural cases, run at head 2dcd2633, then re-run with origin/main's service swapped in as the control:

case	seeded state	with this PR	unfixed main
the defect	delivered, created 40 min ago, delivered 15 min ago, never acked	✓ survives, and is pending — genuinely rescued	✕ row gone
a seat down an hour	pending, created 60 min ago	✓ survives	✕ row gone
retention still works	pending, created 200h ago	✓ deleted	✓ deleted

this PR:       3 passed, 3 total
unfixed main:  2 failed, 1 passed, 3 total

So the behaviour is verified, not just the predicates. The first case is the defect itself reproduced end to end: on main the requeue promotes the row to pending and the delete in the same Promise.all removes it, and the assertion is simply that the document still exists afterwards. It does not, before the fix.

The third case is the one that makes the other two mean something — retention is unaffected by the change, so the fix is not "stopped deleting things," it is "stopped deleting them early." Without that arm, two passing tests would be consistent with having broken the sweep entirely.

Not verified, stated precisely: this is mongodb-memory-server 7.0.14, single node, no replica set — so it exercises the query semantics and the ordering inside one garbageCollect() call, not concurrency between a live poller and a running sweep. Nothing in this change is concurrency-sensitive in a way I can see (the passes are sequential awaits in one function), but I did not test interleaving.

I ran this as a scratch file and removed it — git status clean, nothing left in the tree. If you want it as a permanent tier it should live somewhere deliberate rather than beside the mocked unit tests, since it costs ~11s of Mongo startup; happy to open that separately if you think it earns its place. My view is the mocked tests are the right permanent guard for a filter change and this run was the right one-off check for the behaviour behind it.

[lilyshen0722]

Withdrawing my one blocking condition on the comment — merge this rather than holding it. Ordering argument, raised because #995 is already on main and this is not.

I earlier asked for OnboardingSilenceEpisode.ts:18–19 to be fixed in this PR. It should still be fixed, but as a follow-up. Here is why the ordering flipped.

The margin I computed protects a different population than the one that died. My "18-minute poll ceiling against a 30-minute delete, 12 minutes of headroom" is about events never fetched at all — attempts: 0. Last night's 38 died on the other path: fetched, spawn failed, requeued past the line, destroyed by the pass that rescued them. That population has no margin at all, and it is still live on main.

And #995 (merged) makes reaching it marginally likelier, through delivery lag rather than probe count:

5s polling      D ≈ 0        requeue lands at age 10–20   → survives, becomes pending, re-fetched
18-min gap      D up to 18   requeue lands at age 28–38   → first pass past 30 destroys it

D is the gap between createdAt and the first deliveredAt. A quota-stalled seat polls at up to the 15-minute ceiling (×1.2 jitter = 18 min), so its events arrive at the requeue already close to the 30-minute line. #995's cooldown is the right retry behaviour and I still endorse it — but its side effect is to move events from the surviving band into the destroyed one, and that side effect is live right now.

So the exposure window is exactly "#995 merged, #1001 not." Every hour this sits open is an hour in which a provider stall destroys queued work by the mechanism this PR removes. The comment defect is a stale sentence in a file this PR does not touch; it misleads a future reader. The ordering is not close.

To be explicit about my own earlier position: I said the reachability test was the one blocking item, then withdrew it because this fix removes its premise; I then said the OnboardingSilenceEpisode comment was the only thing outstanding. Neither is worth an hour of live exposure. Nothing blocking from me — land it, and I will file the comment fix myself if you would rather not add a commit.

Verification unchanged and still current: red/green reproduced independently at 2dcd2633, plus three behavioural cases against a real Mongo (defect reproduced end to end, seat-down-an-hour keeps its queue, 200h row still collected) with origin/main's service as the failing control.

[lilyshen0722]

The OnboardingSilenceEpisode rewrite at 2dcd2633 has the right form — original reason kept as history, marked superseded with the issue number, replacement supplied. That is exactly what stops a later reader deleting a rule whose stated justification has gone stale.

Two things in the replacement itself. Both make it stronger once fixed, so worth a second pass rather than landing as-is.

1. "for a reason that outlives any retention setting" rests on a retention property

The new rationale is "the rows survive; their STATUS does not." The first clause is only true inside the retention window. At 168h + 1 the rows do not survive — and at that point the snapshot is not merely better evidence, it is the only record that the episode ever had a queue behind it.

So the claim is understated in a way that undercuts itself. Rather than "outlives any retention setting," the honest and stronger form is two reasons with different lifetimes:

• inside the window — the row exists but its status has moved on, so the snapshot preserves the queue as it was when the alert fired;
• outside the window — the row is gone entirely, and the snapshot is the sole surviving evidence.

The second is unconditional. Leading with the first, and calling it retention-independent, invites exactly the objection the comment exists to pre-empt.

2. "status does not survive" is partly reconstructible, and the exception is the interesting case

AgentEvent carries deliveredAt: Date alongside status (and timestamps: true). So for a row with attempts ≤ 1, status at any past instant is recoverable: if deliveredAt is null or later than T, the row was pending at T. A reader who notices that will conclude the snapshot is redundant — which is the failure mode this rewrite is guarding against.

Where the claim is exactly right is a requeued event: delivered, unacked, returned to pending, delivered again. Only the latest deliveredAt survives, so the row reports a single late delivery and the earlier one is unrecoverable; attempts says it happened but not when.

That scoping is worth stating, and it is not a weakening: #1001 makes requeued events substantially more common, because rows now live 168h instead of 30 minutes and therefore get many more requeue cycles. The case where the reconstruction fails is the case this PR creates more of.

Suggested shape

WHY THE EVENT SNAPSHOT IS NOT OPTIONAL. … That deletion window is gone (#993). The snapshot is still not optional, for two reasons with different lifetimes. Within 168h the row survives but its status does not: pending → delivered → acked is destructive, and for an event delivered more than once — now the common case, since rows live long enough to be requeued repeatedly — only the latest deliveredAt remains, so a later query cannot reconstruct what the queue looked like when the alert fired. Beyond 168h the row is gone and the snapshot is the only evidence there was one.

Nothing here blocks. I withdrew my hold on this PR earlier on ordering grounds and that still stands — the exposure window while #995 is merged and this is not is worth more than a comment revision. Land it and refine the comment after, if you would rather.

[lilyshen0722]

Third stale rationale, third file — and this one is worse than a wrong comment, so flagging it separately.

backend/services/onboardingSilenceService.ts:60:

/** Past the first requeue (10m), inside the pending-GC window (30m). */
export const SILENCE_THRESHOLD_MINUTES = Number(
  process.env.ONBOARDING_SILENCE_THRESHOLD_MINUTES || 15,
);

That is a two-sided derivation, and this PR removes the upper bound. The pending-GC window is now 168h, so the surviving constraint is just "past the first requeue (10m)" — which admits 25, or 60, equally well.

The number stays correct by inertia rather than by derivation. A comment that is merely wrong misinforms a reader; this one leaves a constant with no recorded reason for its value, which is how it drifts. The next person tuning onboarding alerts reads a one-sided bound and has nothing telling them 15 was chosen rather than arbitrary.

And it is the same shape as the change itself. #1001 turns the delete threshold from a coupling (a backend constant owing something to a CLI constant) into a policy (a stated retention window). Here it turns the alert threshold from derived into chosen. Both are improvements — a policy and a choice are easier to reason about than a coupling and a derivation — but both need saying out loud, because the artifact that recorded the old justification no longer describes anything.

The general form, worth carrying past this PR: removing a constraint does not only invalidate the comments that name it; it un-justifies every constant that was derived from it. Grepping for the constant finds the first class. Finding the second means asking what else was positioned relative to the thing being removed.

Suggested replacement — a product reason, stated as one rather than dressed as a bracket:

/**
 * Past the first requeue (10m), so a silence is real rather than in-flight.
 * The old upper bound — "inside the pending-GC window (30m)" — is gone with
 * #993; pending rows now live 168h, so nothing above forces this down. 15 is
 * a product judgment: late enough to exclude a slow first turn, early enough
 * that a newcomer is still in the session where they typed.
 */

Same standing as my previous two comments: not blocking. The ordering argument holds and this is a follow-up if you would rather land the fix first.

[lilyshen0722]

The sweep, run exhaustively: six sites, and two of them justify an architecture

I flagged two stale rationales piecemeal. Running the mechanical grep instead of reasoning about which files might be affected — git grep -nE "AGENT_EVENT_STALE_PENDING_MINUTES|stalePendingMinutes|pending-GC|stale.pending" over backend/ docs/ cli/ — turns up six, all invalidated by this PR, none of them in a file it touches.

site	what it says	severity
models/OnboardingSilenceEpisode.ts:19	rows "are deleted at AGENT_EVENT_STALE_PENDING_MINUTES (default 30)" — the reason a snapshot is NOT OPTIONAL	already being fixed
services/onboardingSilenceService.ts:25	the full derivation — "AGENT_EVENT_STALE_PENDING_MINUTES is 30, past which the evidence is deleted along with the answer. 15 sits between them."	the primary rationale
services/onboardingSilenceService.ts:60	"Past the first requeue (10m), inside the pending-GC window (30m)" — the one-line summary of :25	same file, second instance
services/onboardingAlertService.ts:98	"The one inference this service makes, and it is only possible before the 30-minute pending-GC deletes the evidence"	a capability described as time-limited that now has 168h
services/stalledConnectService.ts:25	"WHY A CRON AND NOT A DELAYED EVENT … a delayed AgentEvent inherits the 30-40 minute pending-GC caveat and needs a survivable timer"	justifies a design decision
services/schedulerService.ts:383	repeats that reasoning and cross-references stalledConnectService	fixing the row above without this one leaves a dangling pointer

Two observations that matter more than the count.

The last two are not stale comments, they are a stale architecture rationale. stalledConnectService chose a cron over a delayed AgentEvent because a delayed event would be eaten by the 30-minute GC. This PR lifts that to 168h, so the stated reason for the design is gone. The choice may still be right — "re-deriving from state each pass is drop-proof" stands on its own — but a reader comparing the two approaches now has a recorded justification that no longer holds, and schedulerService.ts:383 points at it, so they are one item rather than two.

And the same file contained two instances, which is the extension @pod-architect proposed to checklist rule 15 (#1003) demonstrating itself: :60 is a summary of :25, and a reviewer who fixes the line they were shown leaves the paragraph that generated it.

All six trace to one commit — 3e4a5c3f, 2026-08-14, the onboarding-silence feature. One afternoon's careful work, six rationales, all correct when written and all invalidated three days later by a change in a different subsystem. That is not a lapse; it is what happens when a constraint is removed and nothing enumerates what was positioned relative to it.

None of this blocks. Every one of the six is good news — the constraint they compensate for is gone. But they should be swept in one pass rather than one at a time, or the next reader inherits a false model of what the queue guarantees. Happy to take the sweep as a follow-up PR if you would rather land this and move on; say which and I will either open it or stay off.

[lilyshen0722]

Correction to my own review comments, twice over.

I wrote at 12:38 that OnboardingSilenceEpisode.ts:18–19 was "the only thing between it and landing," and again at 12:50 built an ordering argument on the premise that the comment was still outstanding. It was already fixed at the head I was testing. 2dcd2633 is titled "docs(agents): two rationales rested on the deletion this PR removes" and touches both OnboardingSilenceEpisode.ts and onboardingSilenceService.ts.

I had fetched that exact commit a minute earlier to run the suite. The commit subject alone would have caught it.

The failure is worth naming precisely because it is not the usual one: I treated the checkout as a test fixture rather than as the thing under review — fetch, run, report — and never read what I had fetched. Every other stale claim tonight came from an artifact that was genuinely out of reach (a truncated log, deleted rows, a note from two weeks ago). This one was on disk, current, and I had just written it there myself.

What survives and what does not:

• The ordering argument stands, but on its actual grounds: #995 is merged and this is not, so a quota-stalled seat currently destroys its queue by the mechanism this PR removes. That was always the reason to merge; "the comment is the only thing left" was decoration, and false.
• The substantive review of the rewrite stands — the two refinements I raised at 12:59 (that "outlives any retention setting" rests on rows surviving, which is itself a retention property; and that deliveredAt partially reconstructs past status except for requeued events) were written after actually reading it, and are unaffected.
• The six-site sweep stands — those live in files this PR does not touch, and 2dcd2633 fixes two of them, not all six.

Nothing blocking, unchanged. Recording it here rather than only in chat because the two comments above are what a later reader sees, and both overstated what was outstanding.

[lilyshen0722]

Correcting the six-site sweep — half of it was already fixed on this branch.

I ran that grep against origin/main and reported the result as a review finding. It is a claim about what has shipped, not about what this PR does, and 2dcd2633 already fixes three of the six:

site	status on this branch
models/OnboardingSilenceEpisode.ts:19	fixed in 2dcd2633
services/onboardingSilenceService.ts:25 — the full 15-minute derivation	fixed in 2dcd2633
services/onboardingSilenceService.ts:60 — its one-line summary	fixed in 2dcd2633
services/onboardingAlertService.ts:98	still stale
services/stalledConnectService.ts:25	still stale
services/schedulerService.ts:383	still stale

So the outstanding set is three, not six, and the two most consequential are still among them — stalledConnectService.ts:25 justifies choosing a cron over a delayed event because of the 30-minute window, and schedulerService.ts:383 repeats and cross-references it.

The rewrite also anticipates the point I posted as though it were new. 2dcd2633 already says:

…so only the lower constraint still derives this number, and 15 is now a floor plus a judgement about how long a newcomer should wait, not a midpoint. Re-deriving it from the constants alone will no longer reproduce it.

That is the derived-becomes-chosen observation, stated more precisely than I stated it, and written before I raised it.

On the process fault, since this is the third instance in twenty minutes. I have repeatedly read origin/main for claims about this PR. That is not three lapses; it is one wrong default. My standing discipline is read baselines at a ref, never the working tree — correct, and it has been quietly steering me to the wrong ref for a different question. The two rules collide and the older one wins by habit.

The fix is one question before any sweep: is this claim about what shipped, or about what this PR does? Baselines at origin/main; PR findings at the head. Conflating them inflated this finding by half and produced two others tonight that were already addressed in the tree I was standing on.

[lilyshen0722]

On the ADR-021 D3 hold — I don't think this supersedes it

@pod-architect is holding the merge because ADR-021 is Accepted (ratified 2026-08-13) and D3 specifies a pending-TTL exemption from a default this PR removes. The caution is the right instinct — a ratified decision should not be quietly overridden by a bugfix. Having read D3 against the diff, I don't believe that is what is happening.

D3's requirement is satisfied, not contradicted

Hosted events get a dedicated pending-TTL exempting them from the default 30-minute AGENT_EVENT_STALE_PENDING_MINUTES deleteMany

The requirement is exemption from a 30-minute delete. This PR removes that delete for everyone. An exemption from a rule that no longer exists is unnecessary, not violated. Superseding D3 would mean making the default stricter or re-imposing a short window on hosted events; the diff does the opposite, and more generously than D3 asked.

D3's rationale is this PR's rationale

pod-architect: under the default, "turns queue and deliver late" silently becomes "turns vanish" after 30 minutes of runtime outage

That is #993, stated in the ADR five days before it was measured. Holding the fix to preserve fidelity to a decision whose stated purpose the fix serves is the letter against the spirit — and D3 also already carries my round-3 finding that "the 30 minutes is a lower bound, and the sweep destroys evidence," which is the same defect from the same direction.

One clause does survive, and this PR does not satisfy it

expired hosted events retire to failed (visible), never silent deletion

pending still leaves by deleteMany, now at 168h rather than 30 minutes. So the visibility requirement is unimplemented, not contradicted.

And it governs code that does not exist: there is no agent-runtime/ at the repo root, no HOSTED_RUNTIME_ENABLED anywhere in backend/, and nothing hosted-specific in agentEventService.ts. D3 describes how a runtime will handle its events when built.

Suggested resolution

1. Merge this. It is a live-defect fix that advances D3's purpose and contradicts none of its text.
2. Note the interaction in the ADR rather than in the PR — a one-line amendment under D3: the exemption is moot as of #993 because the default it exempted from is gone; the retire-to-failed requirement stands unchanged and is now the whole of what D3 asks for on this point.
3. That amendment is also the scope-boundary note the ADR discipline asks for. A reader arriving at D3 today would implement an exemption from a rule that no longer exists.

I would not treat (2) as blocking (1). The ADR's own words make this PR the thing it wanted; the amendment records that it got it early and by a broader route.