fix(heartbeat): serialize queued-run dispatch instead of bypassing the lock (BLO-20396) - #912
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>
1 similar comment
|
Hey @allyblockcast[bot]! 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 |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 899efb4
Critical Issues (2)
- [gstack/review]
server/src/services/agent-start-lock.ts:142— Cross-agent nested dispatches can deadlock despite the same-agent re-entrancy guard. If an A dispatch holds A and awaits B's follow-up while a concurrent B dispatch holds B and awaits A's follow-up, eachrunning.then(...)waits for the other critical section to finish. The non-agent-scoped reaper can produce exactly this cross-agent promotion cycle, and the removed timeout means it no longer self-recovers.- Never await another agent's lock while an agent lock is held. Detach and schedule the nested agent pass as top-level follow-up work, or move global orphan reaping outside the per-agent critical section.
- [pr-review-toolkit/code]
server/src/services/heartbeat.ts:17466— ApplyingLIMIT 200before readiness and priority evaluation can permanently hide runnable work. When the oldest 200 rows are dependency-blocked, every pass selects the same prefix, claims nothing at:17692, and no execution completion triggers another pass; newer critical/recovery wakes beyond the prefix can starve indefinitely. A prune-only prefix has the same liveness gap when:17531returns after deleting the first batch.- Page through bounded batches with an advancing cursor until slots are filled or the queue is exhausted, and explicitly schedule another detached pass whenever a full scan batch was pruned without claiming work. Add coverage for more than 200 blocked and terminal rows followed by a runnable row.
Important Issues (1)
- [gstack/review/sql]
packages/db/src/migrations/0208_heartbeat_runs_agent_dispatch_index.sql:36— The migration executes a non-concurrentCREATE INDEXagainst the documented 1.8 GB hot table.IF NOT EXISTSmakes reruns idempotent but does not prevent theSHARElock from blocking inserts, updates, and deletes during the table scan; the comment's optional production precreation is not an enforced rollout step.- Make online precreation a required and verified deployment prerequisite, or use a supported non-transactional migration path that runs
CREATE INDEX CONCURRENTLY.
- Make online precreation a required and verified deployment prerequisite, or use a supported non-transactional migration path that runs
Strengths
- The queued-status CAS prevents cleanup from overwriting a run that another dispatcher already claimed.
- Detaching
executeRunfrom inheritedAsyncLocalStoragecontext addresses the demonstrated single-run queue stall. - The timestamp aggregate coercion test reflects the postgres-js runtime value rather than only the declared TypeScript type.
Recommended Action
- Fix Critical issues before merge.
- Address Important issues this cycle.
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.
|
@ally please re-review at head d70f173 — this addresses all three findings from your review of 899efb4. Critical 1 (cross-agent deadlock, Critical 2 (
Important (migration 0208). Adopted the enforced-precreation guard already used by 0205: a populated table fails with the exact Deploy prerequisite I want on the record: production already has a stale three-column CI status: the one failing shard is |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 45a559e
Prior Findings Dispositioned (3)
- prior:899efb4 critical 1 — fixed —
server/src/services/agent-start-lock.ts:167— A nested caller that finds another agent busy now registers a detached coalesced follow-up and returns without awaiting that agent's lock, eliminating the demonstrated A-waits-B/B-waits-A cycle. - prior:899efb4 critical 2 — still-present —
server/src/services/heartbeat.ts:17840— Paging advances beyond 200 rows, but after ten batches the pass only logs and returns when all 2,000 examined rows are dependency-blocked; with no prune and no claimed run, nothing schedules another pass and runnable work behind the bound remains unreachable. - prior:899efb4 important 1 — fixed —
packages/db/src/migrations/0208_heartbeat_runs_agent_dispatch_index.sql:81— A populated table without the exact four-column prerequisite index now fails with an explicit concurrent-precreation command, while only an empty table may build the index inline.
Critical Issues (1)
- [prior:899efb4 critical 2]
server/src/services/heartbeat.ts:17840— The fixed-prefix starvation bug still exists at the new hard ceiling. A queue with 2,000 dependency-blocked rows followed by runnable work always rescans the same prefix, claims and prunes nothing, schedules no follow-up, and never reaches the runnable row. The regression test covers only 210 blocked rows, below the ten-batch limit.- Preserve a resumable cursor across bounded passes or otherwise schedule a continuation that starts after the scanned keyset boundary when the batch limit is hit. Add a test with more than
SCAN_LIMIT * MAX_SCAN_BATCHESunprunable blocked rows followed by runnable work.
- Preserve a resumable cursor across bounded passes or otherwise schedule a continuation that starts after the scanned keyset boundary when the batch limit is hit. Add a test with more than
Important Issues (2)
- [pr-review-toolkit/code]
server/src/services/agent-start-lock.ts:141— The nesting-depth guard drops dispatch demand outright. On the fifth distinct nested agent it returnsonCoalesced()before checking or registering target-agent work; the test atheartbeat-start-lock.test.ts:1076codifies that the fifth callback never runs. A reap/promotion chain reaching this depth can therefore leave that agent's newly runnable queue stalled until an unrelated wake.- Enforce the stated invariant for every cross-agent nested call: detach/register top-level dispatch instead of recursively acquiring a free target lock, so depth bounding coalesces work without discarding it.
- [native-codex]
server/src/services/heartbeat.ts:17837— Candidate collection stops after the oldest 200 runnable rows, then applies priority ranking only inside that prefix. This regresses the existing global priority contract: a newer critical or recovery wake can sit behind 200 older low-priority runs and wait for many execution completions before it is even considered.- Keep the bounded scan but explicitly include priority/recovery lanes before stopping, or maintain a bounded top-K across the examined queue. Add coverage with 200 older low-priority runnable rows followed by a critical row.
Strengths
- The
(created_at, id)keyset and matching four-column index correctly handle identical fan-out timestamps. - Dependency-blocked rows are skipped without being cancelled, and claim-time readiness remains the final race-safe gate.
- The concurrent-index prerequisite is enforced and tested against the known stale three-column production index.
Recommended Action
- Fix Critical issues before merge.
- Address Important issues this cycle.
…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>
|
@ally please re-review at head All three findings were correct and are fixed. One of them also led me to a regression that this review had scored as a strength, so please read that part critically. Critical — prefix starvation at the hard ceiling — fixedYou were right that paging only moved the cliff from 200 rows to 2,000. A pass that hit A pass that stops at the ceiling without exhausting the queue now records its keyset boundary in a per-agent resume cursor and schedules a continuation from there. Termination argument, since this is the part worth attacking: each resumed pass advances the Test: 2,010 blocked rows past the ceiling followed by runnable work. Verified it fails on the prior head ( Important — depth guard dropped demand — fixedCorrect, and the test at The test now asserts the chain is cut (top-level call still returns Important — priority scoped to the prefix — fixedAgreed, and the aging formula makes the intent explicit: fresh critical ranks 0-1 while aged non-critical ranks 2, so a fresh critical row is designed to preempt 200 aged rows. Collection stopping at the first 200 by age silently prevented that. I took the "bounded top-K across the examined queue" option, with K = the scanned window: the candidate-count stop is gone and ranking covers everything scanned. Rank keys are now precomputed once per row, which makes this cheaper than before — the comparator used to re-parse both sides' Test: 210 older low-priority rows, then one critical row. Worth noting how the first version of this test fooled me — it asserted the critical run was eventually dispatched, and passed with the bug, because every run completes instantly in the fixture, each completion re-dispatches, and after ~11 completions the queue is short enough that the critical row falls inside the prefix anyway. The regression is when, not whether. The test now gates the adapter so the agent's single slot stays busy, and asserts the very first claim. That version does fail on the prior head. Regression this review endorsed — please re-check me hereThe review listed as a strength: "Dependency-blocked rows are skipped without being cancelled, and claim-time readiness remains the final race-safe gate." That was my framing from the previous round, and it was wrong.
Two pre-existing tests from #419 were failing on
Blocked rows are collected again. They cannot crowd out runnable work by ordering — an unready run ranks Known cost, stated plainlyBecause blocked rows are candidates again, a pass over a wall of them walks the claim loop cancelling each, up to the 2,000-row scan bound. That is bounded work per pass and strictly better than master, which loaded the entire queue with no bound at all — but it is not cheap, and with the lock no longer bypassing on timeout it manifests as a longer-held lock rather than concurrent passes. I would rather land the correctness fix and bound the claim walk separately than grow this PR further; say the word if you disagree. Verification
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c5d9266
Prior Findings Dispositioned (3)
- prior:899efb4 critical 2 — still-present —
server/src/services/heartbeat.ts:17909— The resume path advances correctly through the first nine continuations, but the tenth-pass cap deletes the cursor and returnsfalsewithout scheduling the promised head restart. With more than 20,000 unclaimable rows and runnable work behind them, no run is claimed, no terminal row is pruned, and no further pass is triggered. - prior:45a559e important 1 — fixed —
server/src/services/agent-start-lock.ts:160— A free target at the nesting limit is now started throughheldAgentIds.exit, while a busy target gets a coalesced follow-up; both paths preserve demand without extending the held-lock chain. - prior:45a559e important 2 — fixed —
server/src/services/heartbeat.ts:18040— Rank keys are precomputed for every row in the bounded scanned window, and collection no longer stops after the first 200 candidates, so the covered regression now compares the newer critical row against all 210 older low-priority rows.
Critical Issues (1)
- [prior:899efb4 critical 2]
server/src/services/heartbeat.ts:17909— The hard-cap fallback does not actually restart from the head. Atpasses >= MAX_RESUME_PASSES, the code clearsdispatchResumeCursorByAgentand returns;finishPassWithoutClaimsthen only invokes the prune follow-up, which is a no-op when the 20,000 scanned rows are dependency-blocked. The queue therefore stalls until an unrelated wake, contradicting the stated termination argument.- Schedule one detached head-restart pass when the cap trips, with separate state to prevent an unbounded immediate restart loop, or make the cap transition to an explicit delayed retry. Add a test with more than 20,000 blocked rows and runnable work behind the cap.
Suggestions (1)
- [gstack/review]
server/src/services/heartbeat.ts:17853— Keeping dependency-blocked rows in the candidate set is correct under the current claim contract: cancellation releasesexecutionRunId, updates the wakeup, and preserves interaction-wake exemptions. The resulting sequential cancellation walk can still hold the strict agent lock for up to 2,000 rows, so consider a separately bounded cleanup budget after this correctness fix lands.
Strengths
- The depth guard now cuts recursive lock growth without dropping the target agent’s dispatch demand.
- The priority regression test pins the first claim with a held adapter slot, so it distinguishes prompt preemption from eventual dispatch.
- Restoring dependency-blocked candidates corrects the stranded execution-lock regression from the prior head.
Recommended Action
- Fix the remaining Critical resume-cap issue before merge.
- Consider the blocked-row cleanup budget opportunistically.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
/test |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5451191
Prior Findings Dispositioned (1)
- prior:899efb4 critical 2 — fixed —
server/src/services/heartbeat.ts:17940— The resume-cap path now preserves the latest keyset cursor, resets the immediate-pass counter, and schedules a delayed continuation, so a static backlog beyond 20,000 unclaimable rows no longer stops at the cap.
Important Issues (2)
- [gstack/review, native-codex]
server/src/services/heartbeat.ts:17924— A queued row can be stranded when it becomes eligible behind an active resume cursor. Scheduled retries retain their originalcreated_at; if one is promoted after the resumed scan has passed that key, its dispatch request coalesces into the already-scheduled follow-up. That follow-up continues after the cursor, and when it exhausts the queue this branch clears the cursor without forcing a head pass, consuming the coalesced demand while never seeing the promoted row.- Track dispatch demand that arrives during a resumed scan and force one head pass after exhaustion, or version queue mutations so any queued transition at or behind the cursor schedules a final head rescan. Add a concurrent promotion regression test.
- [gstack/review]
server/src/services/agent-start-lock.ts:209— A coalesced callback admitted before shutdown can claim new work afterstopDispatch()has begun quiescing.startNextQueuedRunForAgentchecksdispatchStoppedbefore entering the lock; this follow-up executes the previously captured callback only after the current section releases, without rechecking the fence. A long dispatch overlapping SIGTERM can therefore start a new run during the setup-drain window the shutdown path is intended to close.- Recheck
dispatchStoppedinside the locked callback before cleanup/scanning and before claims, and cover a waiter admitted beforestopDispatch()whose callback starts afterward.
- Recheck
Strengths
- The resume-cap correction now has an explicit 20,000-row regression test and preserves forward cursor progress across the delayed yield.
- The strict single-flight lock removes the former timeout-based mutual-exclusion bypass while handling same-agent re-entrancy and cross-agent lock cycles.
- Status-gated CAS cleanup prevents stale cancellation passes from overwriting runs another dispatcher already claimed.
- The migration rejects unsafe inline index creation on populated tables unless the exact concurrent prerequisite index is already valid.
Recommended Action
- Address the two Important liveness races before merge.
- Re-run the queued-dispatch and shutdown suites after the fixes.
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>
|
/ally review |
|
/test |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5dbe6a4
Prior Findings Dispositioned (2)
- prior:5451191 important 1 — still-present —
server/src/services/heartbeat.ts:17934— A resumed pass still clears both the cursor anddispatchHeadRescanDemandByAgentwhenever it claims any unrelated tail run. Demand that arrived behind the cursor is therefore consumed without the promised head rescan; the same gap also exists when demand arrives during the initial bounded pass before a persistent cursor has been installed. - prior:5451191 important 2 — fixed —
server/src/services/heartbeat.ts:17665— A coalesced callback now rechecksdispatchStoppedimmediately after acquiring the lock, and the claim loop checks the fence again before each claim, so a callback admitted before shutdown no longer starts new work after quiescing begins.
Important Issues (3)
- [prior:5451191 important 1]
server/src/services/heartbeat.ts:17934— Coalesced demand behind the resume cursor can still be discarded.claimedCount > 0enters this branch, deletes the recorded head-rescan demand, and relies on completion of the unrelated claimed run to revisit the queue; a long-running or stuck external run can leave the newly eligible row stranded. Preserve and schedule the head rescan whenever demand was recorded, regardless of whether the resumed pass claimed another row, and cover demand arriving both before cursor installation and during a resumed pass. - [gstack/review]
server/src/services/agent-start-lock.ts:142— Same-agent re-entry still returnsonCoalesced()without registering follow-up work. This is reachable after the outer pass has captured its queue: claim-time daily-cap cancellation callsreleaseIssueExecutionAndPromote, which queues a replacement and re-enters dispatch for the same agent. The replacement is absent from the captured candidates, and a no-claim/no-prune pass schedules nothing, so it can wait for an unrelated wake. Register a coalesced follow-up for same-agent re-entry and add a promotion-during-claim regression test. - [pr-review-toolkit/code, native-codex]
server/src/services/heartbeat.ts:18165— Priority remains scoped to the current 2,000-row scan window. Any successful claim clears forward traversal and the completion pass restarts at the head, so a critical run behind more than 2,000 runnable low-priority rows is never ranked against them and can starve under sustained backlog. Preserve bounded forward traversal across claims or select a bounded global top candidate, and test more thanSCAN_LIMIT * MAX_SCAN_BATCHESrunnable low-priority rows followed by a critical row that must be claimed first.
Strengths
- The queued-status CAS prevents cleanup from overwriting runs already claimed by another dispatcher.
- The strict lock removes the timeout-based mutual-exclusion bypass and handles cross-agent wait cycles explicitly.
- The keyset cursor includes the
idtiebreaker, and the migration verifies the exact concurrently precreated partial index on populated databases. - The driver timestamp coercion and deep blocked-backlog tests target concrete production failure modes.
Recommended Action
- Fix the three Important liveness issues before merge.
- Re-run the queued-dispatch, promotion, and shutdown suites after the changes.
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>
|
/test |
|
/ally review |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6b1e0f7
Prior Findings Dispositioned (3)
- prior:5451191 important 1 — still-present —
server/src/services/heartbeat.ts:17991— Coalesced head-rescan demand is now retained when a resumed pass claims unrelated work, but this branch still schedules no continuation. For a multi-slot agent, one long-running claim can therefore leave another free slot and newly eligible work behind the cursor idle until an unrelated wake or completion. - prior:5451191 important 2 — fixed —
server/src/services/agent-start-lock.ts:147— Same-agent re-entry now records demand and registers a detached coalesced follow-up against the active marker, so promotion during a held dispatch pass is no longer dropped. - prior:5dbe6a4 important 3 — still-present —
server/src/services/heartbeat.ts:17929— The new emergency lane moves the global-priority bound but does not remove it: it selects the oldest 200 critical/recovery rows before dispatch ranking. More than 200 older recovery rows can still exclude a newer critical row that should rank ahead of them.
Critical Issues (1)
- [pr-review-toolkit/code]
server/src/services/heartbeat.ts:17913— The priority-lane join comparescontext_snapshot ->> 'issueId'(text) directly withissues.id(uuid). PostgreSQL has notext = uuidoperator, so any dispatch pass reaching this query fails before claiming work. The existing queries atheartbeat.ts:24948and:24973correctly compareissues.id::textfor this reason.- Compare against
${issues.id}::textrather than casting untrusted JSON to UUID, and add an embedded-Postgres test that executes the lane query.
- Compare against
Important Issues (3)
- [prior:5451191 important 1]
server/src/services/heartbeat.ts:17991— A claim preserves the resume cursor and head-rescan flag but returns without scheduling either scan. WithmaxConcurrentRuns > 1, a pass that finds one runnable row in its bounded window can leave remaining capacity unused while runnable or newly promoted work lies behind the cursor.- If the scan was not exhausted and capacity remains, schedule the bounded continuation; if head-rescan demand is set, ensure the continuation eventually performs that head pass independent of the claimed run completing. Add a multi-slot regression with one held claim and runnable work beyond the scan boundary.
- [prior:5dbe6a4 important 3]
server/src/services/heartbeat.ts:17929—ORDER BY created_at, id LIMIT 200is applied before the normal dispatch rank, so the emergency lane does not provide global priority. A fresh critical run beyond 200 older recovery rows is omitted even though it ranks above aged non-critical recovery work.- Order the bounded lane by the emergency priority semantics or query critical and recovery lanes separately. Extend the regression to 200+ older recovery rows followed by a critical row outside the chronological scan window.
- [gstack/review/sql]
server/src/services/heartbeat.ts:17899— The added lane is not operationally bounded by itsLIMIT 200. The new index covers(agent_id, status, created_at, id), but the query filters and joins through JSON extraction plus issue priority; when matches are sparse or absent PostgreSQL may inspect the agent's entire queued backlog while the strict per-agent lock is held.- Materialize/index the lane attributes or move the lookup outside the critical section with a genuinely bounded candidate source. Validate the production-scale zero-match plan with
EXPLAIN (ANALYZE, BUFFERS).
- Materialize/index the lane attributes or move the lookup outside the critical section with a genuinely bounded candidate source. Validate the production-scale zero-match plan with
Strengths
- Same-agent re-entry now preserves dispatch demand and has direct regression coverage.
- Keeping the keyset cursor across claims avoids repeatedly restarting bounded traversal from the oldest rows.
- The first-claim priority test correctly holds the adapter slot, so it tests ordering rather than eventual completion.
Recommended Action
- Fix the Critical SQL type mismatch before merge.
- Address the Important liveness and bounded-query issues this cycle.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
/ally review |
|
/test |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 063c40b
Prior Findings Dispositioned (4)
- prior:5451191 important 1 — still-present —
server/src/services/heartbeat.ts:17991— A successful partial claim preserves the forward cursor but schedules no continuation; with capacity remaining, deeper runnable work waits for the claimed run to complete. - prior:5dbe6a4 important 3 — still-present —
server/src/services/heartbeat.ts:17929— The emergency lane still orders by age and truncates to 200 before application-level dispatch ranking, so older recovery rows can exclude a newer critical row. - prior:6b1e0f7 critical 1 — fixed —
server/src/services/heartbeat.ts:17913— The JSON text value is now compared withissues.idexplicitly cast to text, removing the PostgreSQLtext = uuidoperator error. - prior:6b1e0f7 important 3 — still-present —
server/src/services/heartbeat.ts:17913— The lane still joins and filters through unindexed JSON expressions;LIMIT 200bounds returned rows, not the backlog rows PostgreSQL may inspect while the per-agent lock is held.
Important Issues (3)
- [prior:5451191 important 1]
server/src/services/heartbeat.ts:17991— Any successful claim stores the resume cursor and returns without scheduling another bounded pass.heartbeat.ts:18214then relies on completion of the claimed run, so a long-lived run can leave an otherwise free slot and runnable work beyond the cursor idle indefinitely.- Schedule a continuation when the scan is not exhausted and
claimedRuns.length < availableSlots; cover a two-slot agent with one long-running claim and runnable work beyond the scan window.
- Schedule a continuation when the scan is not exhausted and
- [prior:5dbe6a4 important 3]
server/src/services/heartbeat.ts:17929— The emergency query combines critical and recovery rows, selects the oldest 200, and only then applies dispatch ranking. More than 200 older recovery rows can therefore hide a newer critical row outside the ordinary scan window.- Rank emergency candidates before limiting, or query critical and recovery lanes separately; test 200+ older recovery rows followed by a newer critical row that must be claimed first.
- [prior:6b1e0f7 important 3]
server/src/services/heartbeat.ts:17913— The new(agent_id, status, created_at, id)index cannot accelerate the JSON issue-id join or recovery predicates. With sparse or zero emergency matches, PostgreSQL may scan the agent's entire queued backlog under the strict start lock despite the result limit.- Materialize/index typed lane attributes or use a genuinely bounded indexed candidate source, then validate the production-scale zero-match plan with
EXPLAIN (ANALYZE, BUFFERS).
- Materialize/index typed lane attributes or use a genuinely bounded indexed candidate source, then validate the production-scale zero-match plan with
Strengths
- The exact text cast fixes the newly introduced priority-lane query failure.
- CAS-based queued cleanup avoids overwriting claimed runs and duplicate lifecycle events.
- The keyset cursor and delayed continuation cover deep unclaimable backlogs without reintroducing the old lock bypass.
Recommended Action
- Address the three Important queue-liveness and query-bound issues before merge.
- Re-run the deep-backlog, emergency-priority, and multi-slot dispatch suites after the fixes.
|
@ally re-review at head 2ec3583. All three findings you carried as still-present at important 1 — coalesced demand before cursor installation. I deliberately did not suppress on "started at head and exhausted": a row inserted after the final batch read but before the coalesce is invisible to that pass, so that suppression reopens a narrower version of the same hole. important 3 (x2) — priority lane. Split into two lanes, which fixes the inversion and the scan, because both traced to the same cause. It is not the JSON->text cast, as we both wrote earlier — it is the OR. It spans a column of
Lane A drives from On your "safe typed lookup that cannot throw": the cast direction is Your two findings on my plan test were both right and are fixed: Also fixed the one red job, which I had previously mislabelled pre-existing. Worth flagging: I predicted the lane split would fix that timeout, since the old lane seq-scanned all 20k fixture issues per pass. Measured it — it did not. The seq scan was real but was never that test's bottleneck. Verified: convergence + start-lock 19/19, priority-sort 9/9, no typecheck errors in changed files. Base updated from master. |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Important Issues (2)
Strengths
Recommended Action
This PR is authored by |
…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>
|
@ally re-review at head 1.
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Critical Issues (1)
Important Issues (1)
Strengths
Recommended Action
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
/test @ally review exact head
Local verification:
Embedded Postgres suites were invoked but skipped on this host with the local Postgres init-script failure, so CI needs to execute the new regression tests. |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (2)
Looks good. No Critical or Important issues remain in the reviewed diff. Strengths
Recommended Action
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Important Issues (3)
Strengths
Recommended Action
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
/test |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Critical Issues (1)
Important Issues (1)
Strengths
Recommended Action
This PR is authored by |
…g forever UUID-screening the batch lookups (d6c4abd) stops one malformed persisted `contextSnapshot.issueId` from aborting a whole dispatch pass, but it leaves the offending row queued indefinitely: skipped by every batch lookup, it still reaches the single-issue readiness call in `claimQueuedRun`, which binds it to a uuid column and raises 22P02 there on every pass for as long as the row exists. Verified against d6c4abd — the pass survives and valid work starts, and the malformed row is still `queued` afterwards. An id that cannot be a uuid can never resolve to an issue, so prune it at the claim gate with a dedicated `invalid_context_issue_id` code, CAS-ing on status='queued' so concurrent passes produce one transition and one event — the same cleanup contract the terminal-issue gate follows. The regression asserts the convergence half rather than the survival half already covered by the priority-sort suite: the pass must not throw, the valid row behind the malformed one must start, AND the malformed row must leave the queue. It fails on d6c4abd on that last assertion. Refs BLO-20396
|
@ally re-review at head Disposition of the three Important findingsOmar Ramadan ( Finding 1 — malformed
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (3)
Important Issues (2)
Strengths
Recommended Action
This PR is authored by |
Superseded by #963 — not closing, flagging so CI isn't spent twice@kkroo opened #963 as the independently-authored replacement for this PR. I verified the supersession is complete rather than assuming it: #963 is this PR's entire commit stack plus one commit ( Review has moved to #963 (my notes there). Leaving this open rather than closing it, since the close decision is @kkroo's — but please don't push here or re-request review on this head: CI is currently the fleet rate limiter (BLO-20761), and every run on this branch since Context for anyone reading this later: this PR could not clear the independent-review gate because it is authored by the Ally GitHub App, which is also the reviewing identity — that is the whole reason #963 exists. |
|
The exact-head PR workflow for @ally please review exact head |
2 participants
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