[codex] land deterministic queued-run dispatch - #933
Closed
kkroo wants to merge 16 commits into
Closed
Conversation
…e lock (BLO-20396)
The per-agent start lock stopped serializing after 30s and let waiters run
concurrently, so overlapping dispatch passes scanned and mutated the same
queue. Observed on the workers pod: 28 lock bypasses in 40 minutes (20 of them
for one agent), 229 queued runs for Ally with the oldest at 20h37m, 240 for CTO
with the oldest at 5 days, and 21 rows still queued against already-terminal
issues.
Five changes:
1. agent-start-lock is now a coalescing single-flight dispatcher with no
timeout bypass. Callers arriving while the lock is held share ONE follow-up
pass instead of each queueing their own. `startedAtMs` no longer charges a
caller's own wait against its execution budget, because there is no budget:
a timeout must never downgrade mutual exclusion.
The old bypass was load-bearing, so this could not be a naive mutex.
startNextQueuedRunForAgent calls reapOrphanedRuns inside the lock; the
reaper is not agent-scoped and reaches releaseIssueExecutionAndPromote ->
startNextQueuedRunForAgent, which for the same agent is a self-deadlock
(measured: 30,035ms stall, then bypass). suppressDispatchAfterReap does not
close this - it only suppresses the direct call. Re-entrancy is now detected
via an AsyncLocalStorage set of held agent ids and coalesced, with a nesting
depth cap so a cleanup pass cannot amplify dispatch.
Corollary: executeRun is launched inside the critical section but outlives
it, and ALS propagates into it. Left attached, the dispatch it triggers on
completion looks re-entrant and is swallowed, stalling the queue after the
first run. It is now launched via runDetachedFromAgentStartLock.
2. The queue read is bounded (LIMIT 200, oldest first, truncation logged) and
issue state is resolved before prioritization rather than after.
3. Terminal-issue rows are pruned eagerly for the whole scanned set instead of
lazily for rows the priority walk happens to reach. This reuses the existing
evaluateQueuedRunStaleness / cancelQueuedRunForStaleIssue gate rather than
adding a parallel rule, so its exemptions (resumeIntent, wakeCommentId,
source_scoped_recovery_action) and its issue_terminal_status error code
still apply.
4. Queued-run cleanup is CAS-safe. setRunStatus is a blind by-id update that
always reports success, so overlapping passes each believed they had
cancelled the same row and each logged it, and cleanup could stomp a row
another pass had already claimed to running. Cleanup now goes through a new
setRunStatusIfQueued (generalized from setRunStatusIfRunning) and callers
only log when they won.
5. New partial index (agent_id, status, created_at) WHERE status IN
('queued','scheduled_retry'). Only ~850 of 219k rows are dispatchable, so
the index is 64 kB against a 1.8 GB table.
Also fixes the pr-review-queue 500. `sql<Date | null>` sets only the
compile-time generic; drizzle's postgres-js mapper runs for column references,
not raw expressions, so min(created_at) arrives as a string and .getTime()
threw. The value is coerced at the boundary, and the test stub now returns what
the driver actually returns instead of what the annotation claimed - which is
why this was invisible in CI.
Co-Authored-By: Claude <noreply@anthropic.com>
Addresses Ally's review of #912. Critical 1 — cross-agent deadlock in the start lock. The same-agent re-entrancy guard did not cover cycles: a pass holding agent A's lock that nests into agent B awaited B's follow-up, while a concurrent pass holding B awaited A's. Each follow-up could only start once the waiting section finished, and removing the 30s bypass removed the only thing that broke the cycle. The reaper is not agent-scoped, so reap -> promote -> dispatch reaches exactly this shape. Enforce one invariant instead: a caller holding any agent lock never awaits another agent's lock. Such a caller now registers the coalesced follow-up (so the work still happens, detached and at top level) and returns onCoalesced() immediately. Only lock-free callers block, and a waiter holding nothing cannot be a node in a wait cycle. Pinned by a test that deadlocks in 5s without the guard. Critical 2 — LIMIT 200 could permanently hide runnable work. A fixed prefix traded one liveness bug for another: when the oldest 200 rows are all dependency-blocked, every pass ranks the same unclaimable prefix, claims nothing, and nothing completes to trigger another pass. Blocked rows cannot be pruned out of the way either, since they are valid work. Page forward with a keyset cursor on (created_at, id) instead, skipping dependency-blocked rows rather than accumulating them (claimQueuedRun re-checks readiness and refuses them, so they can never be claimed and only crowd the candidate pool). Bounded by a candidate limit and a batch limit, with a loud warn when the batch bound is hit. A pass that pruned rows but claimed nothing now schedules an explicit detached follow-up; that terminates because pruning is monotone. The id tiebreak is required for correctness, not just planner shape: bulk wake fan-out stamps identical created_at values, so paging on created_at alone would skip or repeat rows at a batch boundary. The index therefore gains id as a fourth key column. Important — migration 0208 built its index inline. A plain CREATE INDEX holds a SHARE lock on a ~1.8 GB hot table for the whole build; IF NOT EXISTS makes reruns idempotent but does nothing about that lock. Adopt the enforced-precreation guard already used by 0205: a populated database fails with the exact CREATE INDEX CONCURRENTLY to run, and a precreated index that does not match this definition is rejected rather than silently accepted. Empty databases still build it inline. NOTE: production has a stale three-column heartbeat_runs_agent_dispatch_idx precreated from the earlier revision. The new guard will correctly reject it, so it must be dropped and recreated concurrently with the id column before this deploys. Tests: cross-agent deadlock; >200 unclaimable rows followed by a runnable row (starved without paging, started with it); three migration-guard cases including the stale three-column index. Both new failure modes were verified to fail without their fix.
…pping nested dispatch (BLO-20396) Addresses Ally's second review on #912, plus a regression that review's "Strengths" section had mistaken for correct behavior. Critical — prefix starvation reappeared at the hard scan ceiling. Paging fixed the 200-row prefix but a pass still gave up after MAX_SCAN_BATCHES. With 2,000 dependency-blocked rows it pruned nothing, claimed nothing and scheduled nothing, so the next pass rescanned the identical prefix forever and runnable work behind it was unreachable. A pass that stops at the ceiling without exhausting the queue now records its keyset boundary and schedules a continuation from there. The chain is self-terminating: each resumed pass advances strictly forward, and exhausting the scan, claiming a run, or tripping MAX_RESUME_PASSES all clear the cursor so the next pass restarts at the head. Important — the nesting-depth guard discarded dispatch demand. At the depth bound it returned onCoalesced() before consulting runningByAgent, so when the target agent's own lock was free there was no pass to fold into and its queue stalled until an unrelated wake. It now detaches the pass to top level instead: the call stack still stops growing, but the work is registered rather than dropped. This is the shape the deadlock guard already used for the busy-lock case; only the free-lock case leaked. Important — priority was scoped to the scanned prefix. Collection stopped at the first 200 runnable rows by age and ranked only those, so a fresh critical or recovery wake behind 200 older low-priority rows was never considered. That contradicts the dispatch formula, which deliberately ranks fresh critical (0-1) ahead of aged non-critical (2). Ranking now covers the whole scanned window. Rank keys are precomputed once per row, which makes this cheaper than the old code: the comparator used to re-parse both sides' contextSnapshot on every comparison, O(N log N) JSON parses, now O(N). Self-caught regression — the previous commit skipped dependency-blocked rows at collection, on the premise that claimQueuedRun would only refuse them. The claim gate does more than refuse: it cancels the run with issue_dependencies_blocked, marks the wakeup skipped, and releases the issue's execution lock. Skipping stranded all three, leaving issue locks held by runs that would never start, and bypassed the gate's interaction-wake exemption. Blocked rows are collected again; they cannot crowd out runnable work because an unready run ranks 12+, below everything runnable, and collection no longer stops at a candidate count. This restores the behavior pinned by #419 and its two dependency-scheduling tests, which were failing on the previous head. Tests, each verified to fail against the prior code: - 2,010 blocked rows past the ceiling followed by runnable work. - 210 older low-priority rows followed by one critical row; the adapter is gated so the assertion pins the *first* claim rather than eventual dispatch (without the gate every run completes instantly and the critical row starts anyway after ~11 completions, so the test passed with the bug). - The depth-cap test asserted the fifth callback never ran, i.e. it codified the bug; it now asserts the chain is cut but the work still runs. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Track queued-run dispatch demand that folds into a resumed scan, force one head pass when that cursor exhausts, and re-check the shutdown fence inside locked dispatch callbacks before scanning or claiming work. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Register same-agent re-entrant dispatches as coalesced follow-up work, keep bounded-scan cursors across claims, and include critical/recovery queued runs outside the scan prefix so priority is not limited to the oldest 2,000 rows. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…ions `runExclusively` published the agent's lock into `runningByAgent` only after invoking `fn`, but the wrapper runs `fn`'s synchronous prefix immediately. A call that re-entered dispatch inside that prefix therefore saw an empty `runningByAgent`, and the re-entrancy guard's `runExclusively` fallback started a SECOND critical section for the same agent, concurrently with the one it was nested inside — the exact property this module exists to guarantee. Publish the marker before `fn` is invoked so the window cannot exist, and drop the same-agent fallback: re-entrancy means a section is by definition already running, so there is always something to fold into. The depth guard keeps its fallback, which is for a *different* agent and so is not a mutual-exclusion violation. Also adds regression coverage for the three liveness fixes in this PR: - start-lock: a re-entrant call made before the outer section's first `await` must not run inline and must not run concurrently; repeated re-entry must collapse into one shared follow-up (2 calls became 2 passes before this). - convergence: a `critical` row behind 2,000 older claimable rows is claimed first, proving priority is ranked globally rather than within the scan window. - convergence: after a claim, the next pass resumes from the scan boundary instead of restarting at the head (4,100-row fixture, all rows equal-ranked and issue-less so queue position is the only variable). - convergence: a row behind the cursor is still reached while the runs already claimed are long-running and will never complete to trigger a pass. Both convergence tests fail against 5dbe6a4 and pass on 063c40b; both start-lock tests fail without the fix above. Co-Authored-By: Claude <noreply@anthropic.com>
…e (BLO-20396) The acceptance criteria ask for query-plan or integration evidence that the dispatch query uses the new index. Migration 0208 already has a test, but it only proves the index exists and is defined correctly — not that the planner chooses it, which is the property the ticket is actually about. Seeds a production-shaped fixture (200k heartbeat_runs, 20k issues, one agent holding a 350-row queued backlog) and asserts on EXPLAIN (ANALYZE, BUFFERS): - the head scan and the keyset-cursor scan both use heartbeat_runs_agent_dispatch_idx, with the (created_at, id) row comparison pushed into the Index Cond rather than applied as a filter; - no Sort node, so the index supplies the ordering over ~1.4 kB rows; - rows inspected stay within the LIMIT. It also measures the priority lane in its zero-match worst case, which review asked to validate. The heartbeat_runs side is bounded by the new index, but `context_snapshot ->> 'issueId' = cast(issues.id as text)` is not sargable on issues.id, so the issues side is a full company seq scan on every dispatch pass, held under the per-agent start lock. Rows inspected scale with the company's issue count rather than the LIMIT: 14,034 at 20k issues, 67,367 at 100k. For comparison the test also plans the same lane joined on context_issue_id — the STORED generated column that has held `context_snapshot ->> 'issueId'` since migration 0079. That keeps the issues side on issues_pkey and rows inspected flat at 350 across both scales. Recorded as executable evidence so the tradeoff can be judged from a plan rather than from an argument; the lane itself is deliberately left unchanged here. Set BLO20396_ISSUES to change the issue-table scale and BLO20396_PLAN_REPORT to dump the full plans to a file.
…dispatch demand (BLO-20396) Answers the three findings Ally carried as still-present at f2c3454. Priority lane (important 3, twice over). The lane was one query joining issues on `context_snapshot ->> 'issueId' = cast(issues.id as text)` with `priority = 'critical' OR <recovery predicate>` under a single `ORDER BY created_at, id LIMIT 200`. Both defects trace to the OR: - it spans a column of issues and a JSON field of heartbeat_runs, so no index satisfies it and the existing issues_company_priority_idx went unused -- the company's whole issue table was seq-scanned on EVERY dispatch pass, while holding the strict per-agent start lock. Measured on the zero-match worst case: 14,034 rows / 21.4 ms at 20k issues, 67,367 / 100.9 ms at 100k. Linear in company size, not queue depth. - both row kinds competed for the same 200 slots, applied BEFORE dispatch ranking, so >200 older recovery rows could push a newer runnable critical row out of the lane entirely -- the exact priority inversion the lane exists to prevent. Splitting the lanes fixes both, and each predicate can now reach an index: lane A drives from issues_company_priority_idx and joins on the STORED generated column context_issue_id; lane B needs no join at all (recovery-ness is a property of the run) and resolves issues by primary key. Coverage is unchanged -- a critical row is still found at any queue depth. before (one query, OR) 14,384 rows 21.3 ms 435 buffers critical lane 1 row 0.205 ms 9 buffers recovery lane + pkey lookup 350 rows 0.96 ms 29 buffers The cast direction is deliberate: uuid -> text is total. The inverse, issues.id = context_issue_id::uuid, parses untrusted text per outer row, so one malformed issueId takes dispatch down for that agent; a `~` guard in WHERE does not fix it because nothing orders it before the join condition. Lane B screens ids through a UUID pattern in JS instead, so a malformed value is a skipped row rather than a query error. No migration needed. Coalesced demand (important 1). onCoalescedDemand recorded a head rescan only when a resume cursor already existed, but the cursor is installed at the END of a pass -- so every wake folding into the first bounded pass was dropped, and the resume chain it scheduled started past the newly eligible row and ran to exhaustion. Demand is now recorded unconditionally. Deliberately NOT suppressed when the in-flight pass started at the head and exhausted: a row inserted after that pass read its final batch but before the coalesce is invisible to it, so that suppression would reopen a narrower version of the same hole. Tests: the behind-cursor regression Ally asked for. It is built so the pass that exhausts the scan prunes nothing -- an earlier version put terminal rows in the tail and passed against the bug, because the prune follow-up rescued the row for an unrelated reason. Verified: heartbeat-queued-backlog-convergence + heartbeat-start-lock 19/19 green.
…st fits its budget (BLO-20396)
`heartbeat-dispatch-priority-sort > continues after the resume cap` was the
sole red job on this PR (1 failed / 1610 passed), timing out at 180s. The test
is added BY this PR, so it is a merge blocker rather than pre-existing noise.
The cost was the fixture, not the dispatch logic. Reaching the resume cap
takes `scanLimit * maxScanBatches * maxResumePasses` queued rows, and with the
production constants (200 * 10 * 10) that is 20,000 runs plus their issues,
wakes and dependency rows — roughly 80k inserts before the assertion can even
begin. All three bounds were module-level consts, so the test had no way to
ask for a smaller world.
They are now overridable through HeartbeatServiceOptions, alongside the
existing test-only hooks. Production passes nothing and is unaffected.
The test takes a scoped service at 20 * 2 * 2 and keeps `blockedCount` equal
to the cap product, so the geometry is identical — the runnable row still sits
immediately past the boundary the chain yields at — at 1/250th the size. The
cap is still genuinely reached, which the run proves:
startNextQueuedRunForAgent: queued backlog still unclaimable after the
resume cap; delaying bounded scan continuation
{passes: 2, maxResumePasses: 2, scannedRows: 80}
Shrinking the world rather than raising the timeout keeps the cap arithmetic
under test; a longer budget would have kept the slowest job in CI slow and
proved nothing extra.
Verified: heartbeat-dispatch-priority-sort 9/9 green in 93s (was 1 failed,
>300s). No typecheck errors in either changed file.
I first predicted the lane split in 120e562 would fix this, on the theory
that the old priority lane seq-scanned all 20,000 fixture issues on every
pass. Measured it instead of assuming: still 300s. The seq scan was real
(21.4ms -> 0.2ms) but was never this test's bottleneck.
…lane (BLO-20396) Review follow-up on the two remaining Important findings. 1. A partially filled dispatch pass abandoned its continuation. advanceOrClearResumeCursor stored the forward cursor and returned false whenever anything was claimed, and the caller scheduled nothing, on the reasoning that a claim re-triggers dispatch when it completes. That only covers the slot the claim occupied. A pass with two free slots that claims one long-running row and refuses the rest of its window stops with a slot free — not out of capacity, but out of candidates in that window. Nothing then looks past the cursor: no completion fires for a slot that never started, and a pass that pruned nothing does not schedule the prune follow-up either. The runnable row waits for the claim to finish, which on this fleet means a ~50-minute review. It now schedules a cursor continuation when the scan is not exhausted and claimedCount < availableSlots. The chain terminates on two independent bounds: the cursor only advances forward through a finite queue, and each claim CAS-flips a row to running, so successive passes recompute a strictly smaller availableSlots and dead-end at the availableSlots <= 0 return. Regression test asserts exactly that geometry, with the first claim held open for the duration so the continuation is the only thing that could dispatch the third run. Verified as a true regression: reverting only the four-line fix fails it (dispatched Array(1), missing the row beyond the cursor). 2. The recovery lane's predicate was unindexed. Lane B filters two jsonb expressions, so 0208's index supplied the agent's queued rows in dispatch order and the executor filtered them one at a time. With no recovery work — the common case — nothing lets the LIMIT stop early, so PostgreSQL walked the agent's entire queued set to return zero rows, while the strict per-agent start lock was held. Migration 0209 adds a partial index on the lane's own predicate, so the zero-match case is an empty index range rather than a filtered walk. Measured on a 5,000-row non-recovery backlog (25x SCAN_LIMIT): the lane now uses heartbeat_runs_recovery_dispatch_idx with Index Cond on agent_id alone, and inspects 0 rows in 0.9 ms. The new test asserts a FIXED ceiling independent of queue depth, which is the property that distinguishes "bounded" from "bounded by queue depth"; other agents' recovery rows are seeded so the index is not globally empty, since any plan is cheap against an empty index. 0209 follows 0208's online-precreation guard. Its predicate check is structural rather than an exact pretty-printed string: the real rendering was measured, and it parenthesizes the second AND operand but not the first, which is the kind of detail a hand-written expected string gets wrong — and getting it wrong fails the migration for an operator who precreated correctly. Four guard tests cover it, including that the exact command the hint prints is accepted, and that a predicate degenerated to status = 'queued' is rejected. Also flushes the plan report on every record, so the plans survive a failing assertion instead of being written only after the asserts pass. Co-Authored-By: Claude <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-20396 |
1 similar comment
|
🔗 Paperclip issue: BLO-20396 |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
kkroo
marked this pull request as ready for review
August 2, 2026 02:12
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Closing this in favour of #912. The premise for opening a second, independently-authored PR at the same head does not hold, and keeping both actively slows the landing. 1. An approving review is not a merge gate on this repo. 2. Ally never submits 3. What is actually blocking both PRs is CI capacity, and this PR made it worse. I have cancelled the duplicate run No criticism of the work: the branch is byte-identical and the description is compliant. It is simply solving a gate that is not the one we have. Reopen if someone has evidence that contradicts points 1-3. |
13 tasks
13 tasks
13 tasks
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
2 participants
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.
Thinking Path
Linked Issues or Issue Description
Paperclip issue: BLO-20396
Bug report fields:
startNextQueuedRunForAgentstopped serializing after the lock waited 30s, so concurrent dispatch passes could scan and mutate the same agent queue.Related work searched: open Paperclip PRs touching heartbeat queue dispatch, stale queued-run cleanup, and recovery wake fan-out. Known upstream arrival-rate work remains BLO-20074; this PR focuses on queue draining correctness, not wake fan-out reduction.
What Changed
agent-start-lock.tsinto a strict coalescing single-flight lock with re-entrancy detection and no 30s bypass.0208_heartbeat_runs_agent_dispatch_index.sqlfor the dispatchable queue scan.pr-review-queuedate coercion for rawmin(created_at)values returned by the driver.Verification
heartbeat-start-lock,heartbeat-queued-backlog-convergence,heartbeat-dispatch-priority-sort,heartbeat-stale-queue-invalidation, andagent-live-run-routes.pnpm typecheckandpackages/db check:migrationspassing.Risks
LIMIT 200oldest-first could delay a brand-new critical wake if a single agent has more than 200 older dispatchable rows.heartbeat_runstable.Model Used
Claude Code assisted with the original implementation and PR description. The exact model identifier was not recorded in the original PR body; the work used tool-assisted code editing and test execution.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template