fix(engine): bound never-dispatched pending invocations by absolute age - #358
Merged
Conversation
The 0032 handler-unreachable TTL fails an open invocation only after a DISPATCHED handler connection is observed unreachable for the TTL. A `pending` row that was never dispatched to any node (`dispatch_attempts = 0`) has no handler to observe, so the clock never starts and it can sit pending indefinitely — a 7,527-row backlog on the `relaycast-cloud` D1 (oldest 2026-06-26) is the direct measurement. The failure mode is not inert. When a matching node eventually returns and the queue drains, week-old spawn briefs come back to life as fresh agents. One landed as `chief` on a second node, and because agent rows carry one `token_hash` per name it evicted the live resident's token — every DM to `chief` routed to the impostor while the resident sat silently deaf. `sweepTimedOutInvocations` now runs a second pass that fails `status='pending' AND dispatch_attempts=0 AND created_at older than PENDING_INVOCATION_MAX_AGE_MS (default 72h)` with the distinguishing error `never_dispatched_expired`. 72h rides out a weekend-scale node outage with a day of buffer, and is far shorter than the observed multi-month backlog. The existing handler-unreachable TTL runs first and is unchanged; the new bound only touches the shape that guard cannot see. Tests: - must-fire: an old, never-dispatched pending row IS failed with `never_dispatched_expired`. - must-not-fire: a recent pending row is untouched. - must-not-fire: a dispatched-then-unreachable row inside the 0032 TTL is untouched (has `dispatch_attempts > 0`). - control: reset the same row to the never-dispatched shape and the age bound DOES fail it, proving the must-not-fire is guarded by the query filter rather than by accident. Both must-not-fire cases were verified to turn red when their guarded condition was removed from the sweep query. Fixes #357 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Session-Id: 10847086-04c3-455f-9fba-c975ab69ab62 Session-Id: 10847086-04c3-455f-9fba-c975ab69ab62 Session-Id: 10847086-04c3-455f-9fba-c975ab69ab62
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…weep Two review findings on the age-bound sweep: 1) D1 100-bound-parameter cap: failNeverDispatchedExpiredInvocations was passing every stale id for a workspace into one inArray, so any workspace with >~100 aged rows threw "too many bound parameters" that the per- workspace catch swallowed — leaving the exact backlog the fix targets unable to drain. Chunk by D1_SAFE_IN_QUERY_CHUNK_SIZE with a per-chunk try/catch and per-chunk completion emission. 2) SELECT/UPDATE race: a row selected as (pending, dispatch_attempts=0, past cutoff) could be concurrently dispatched before failOpenInvocationRows ran; that UPDATE only re-checks status IN OPEN_INVOCATION_STATUSES, so it would fail a row that had just been dispatched. Introduce failNeverDispatchedInvocationRows whose UPDATE re-checks status='pending', dispatch_attempts=0, and created_at<=cutoff atomically, so a concurrently dispatched row is skipped. Never-dispatched rows have no spawn reservation to release, so the held-rows lookup is unnecessary here. Test: pendingInvocationAgeBound now includes an isolation case for a pending row with dispatch_attempts>0 past the cutoff — must NOT be failed with never_dispatched_expired. Proves the dispatch_attempts=0 filter is load- bearing on its own (the existing must-not-fire uses a dispatched row, so it alone couldn't isolate the two filters). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Merge #355 (`fix: make action invocation retries idempotent`) into this branch to clear the CONFLICTING/DIRTY state left by its landing on main. Only textual conflicts (both changelogs, both entries under `[Unreleased - Patch] > Fixed`); no logical conflict — #355's dispatch reordering and this branch's sweep both coexist. Also narrow the cubic-flagged race on the age-bound sweep. The dispatch flow's send→record ordering (send frame, then `dispatchNodeAttempt` increments `dispatchAttempts`) is load-bearing for #355's "wait for durable dispatch outcome" idempotent-replay semantics — a claim that materializes before the send would let a replay observe a success that hasn't happened. Reordering to claim-then-send breaks the keyed idempotency suite. Instead, sleep `NEVER_DISPATCHED_SWEEP_GRACE_MS` (default 5s) between the sweep's candidate SELECT and its atomic UPDATE, so any concurrent dispatcher's `dispatchAttempts` UPDATE lands before ours fires. Combined with the existing `dispatch_attempts = 0` re-check in the UPDATE WHERE, an in-flight dispatch is reliably invisible to the sweep instead of racing with it. Only sleep when candidates were found. Override with `neverDispatchedSweepGraceMs: 0` for tests. A fully race-free fix requires reordering dispatch to claim-then-send with revert-on-send-failure — that touches #355's atomic-outcome contract and is out of this PR's scope; tracked as a follow-up. Co-Authored-By: Claude Opus 4.7 <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
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.
Summary
Fixes #357.
The 0032 handler-unreachable TTL fails an open invocation only after a dispatched handler connection is observed unreachable for the TTL. A
pendingrow that was never dispatched (dispatch_attempts = 0) has no handler to observe, so the clock never starts and the row can sitpendingindefinitely. Therelaycast-cloudD1 measurement in #357 shows 7,527 such rows, oldest 2026-06-26.The failure is not inert. When a matching node eventually returns and the queue drains, week-old spawn briefs come back to life as fresh agents. In the incident that motivated this fix, one landed as
chiefon a second node and — because agent rows carry onetoken_hashper name — evicted the live resident's token, so every DM tochiefrouted to the impostor while the resident sat silently deaf.The fix:
sweepTimedOutInvocationsnow runs a second pass that failswith the distinguishing error
never_dispatched_expired.Choice of N
PENDING_INVOCATION_MAX_AGE_MS= 72 hours. Justification:Exposed as
pendingInvocationMaxAgeMsonSweepTimedOutInvocationsOptionsso operators can tune per environment.What is NOT changed
dispatch_attempts = 0) never touches a row the existing guard would touch (which requiresdispatch_attempts > 0).Test plan
pendingrow IS failed withnever_dispatched_expired.pendingrow is untouched.dispatch_attempts > 0excludes it from the new query).dispatch_attempts=0removed → dispatched-then-unreachable turns red;createdAt<cutoffremoved → recent-untouched turns red). Guards restored before push.npx turbo test lint build --filter=@relaycast/engine --force— all 684 tests green.Notes for reviewer
chief) is currently deaf per the incident described in A never-dispatched invocation has no path to failure: 7,527 pending spawns, oldest 2026-06-26 #357; this PR is for eyes-on review, not lockstep merge.🤖 Generated with Claude Code