[codex] bound serialized dispatcher critical scans - #963
Merged
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>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…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
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
🔗 Paperclip issue: BLO-20396 |
kkroo
marked this pull request as ready for review
August 2, 2026 11:27
Author
|
@ally please review exact head |
|
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. |
Author
|
Correction: @ally please review exact head |
|
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 |
![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: e86298c
Important Issues (3)
- [gstack/review]
packages/db/src/heartbeat-dispatch-query-plan.test.ts:188— The central plan test explains hand-written literal SQL viasql.unsafe, not the parameterized statements emitted by Drizzle inheartbeat.ts. In production,status = "queued"is a bound value (heartbeat.ts:17955andheartbeat.ts:18199), and the postgres.js client uses prepared statements by default. A generic prepared plan cannot prove that a status parameter implies either partial-index predicate, so this test can stay green while the hot path stops using the new indexes and resumes scanning under the per-agent lock. Exercise the real generated statement through the production driver, including a forced generic-plan case, or emit the partial-predicate constants as SQL literals. - [pr-review-toolkit/tests]
server/src/__tests__/heartbeat-dispatch-priority-sort.test.ts:1802— The new critical-lane regression test recordsonQueuedDispatchScheduledForTest, which fires before the continuation attempts to acquire the lock, then only asserts that the suppression option was requested at lines 1935-1941. It never holds the original pass long enough to forceresume_critical_laneto coalesce, so it can pass without exercising the race this fix targets or proving that coalescence does not re-arm the head-rescan marker. Add a deterministic barrier around lock acquisition and assert the coalesced-demand/cursor transition itself. - [native-codex/tests]
tests/e2e/mcp-user-stories.spec.ts:433— US-9 explicitly requires that “The Review link does not lose the pending card,” but the test now approves and completes the action before visiting/review. That removes the only observation of the pending state, so a regression that drops the pending card will pass. Keep the pre-approval navigation/assertion (and add a post-completion check separately if needed).
Strengths
- The dispatcher changes consistently use keyset cursors with an ID tiebreak, CAS queued-run mutations, and explicit continuation bookkeeping.
- The migration guards fail closed on populated databases and validate precreated index shape before accepting it.
- The new tests cover deep critical queues, malformed issue IDs, pruning convergence, and index-plan bounds across the main paths.
Recommended Action
- Address the three Important issues this cycle, then request a fresh review on the new exact head.
CTO review notes at
|
Author
|
@ally please review exact head This addresses both Important findings from the
Dispatcher-owned continuations suppress manufactured head-rescan demand for both emergency lanes. Current Post-merge 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: a1c467a
Prior Findings Dispositioned (2)
- prior:24abda8 important 1 — fixed —
server/src/services/heartbeat.ts:17943— External wakes now record critical-lane head-rescan demand without deleting the saved cursor before lock acquisition; the lock-owning continuation retains forward progress and consumes the marker only after the lane makes progress or exhausts. - prior:24abda8 important 2 — fixed —
server/src/services/heartbeat.ts:18381— The recovery lane now has its own keyset cursor and bounded page loop, continuing past malformed, deleted, terminal, and dependency-blocked rows before ordinary work is ranked.
Important Issues (2)
- [native-codex/concurrency]
server/src/services/heartbeat.ts:18345— Both emergency lanes stop paging as soon as they find a dependency-ready row, but dependency readiness is weaker than claimability. In particular,claimQueuedRunreturnsnullwithout dequeuing a future-dated Kubernetes isolation retry (server/src/services/heartbeat.ts:14643). The lane then clears its keyset cursor, repeatedly rediscovers that deferred row from the head, and can let ordinary work claim the slot while runnable critical or recovery work remains deeper than the bounded window. Treat isolation-deferred rows as not ready for emergency-lane stopping, or preserve/advance the emergency cursor when the selected candidate is refused without a state transition. - [pr-review-toolkit/tests]
server/src/__tests__/heartbeat-dispatch-priority-sort.test.ts:1532— The resume-cap regression makes the only runnable row beyond the capcritical. The independent critical lane can discover and dispatch that row regardless of whetherscheduleDelayedResumeCapRetryworks, so this test remains green if delayed main-scan continuation is removed or broken. Make the target non-critical and assert that the delayed cap path is reached before dispatch.
Strengths
- The two requested remediations use separate emergency-lane cursors and preserve deferred head-rescan demand without destroying continuation state.
- Recovery query predicates align with the dedicated partial index, and the generic-plan coverage guards the prepared-statement path.
- The new recovery fixture covers malformed, deleted, terminal, and dependency-blocked rows across multiple pages.
Recommended Action
- Keep emergency-lane paging past dependency-ready candidates that are still claim-deferred.
- Make the resume-cap test exercise the ordinary lane rather than the critical fast path.
- Re-run the focused dispatcher and backlog convergence suites before requesting review again.
Signed-off-by: Omar Ramadan <omar@blockcast.net>
Author
|
@ally please re-review exact head This addresses both Important findings from the
The critical and recovery paging regressions now each include two future-dated isolation-deferred rows filling an entire page ahead of the runnable target. The recovery targets are non-critical, keeping that test isolated from the critical lane. Verification:
The branch remains integrated with current |
Author
|
Correction: @ally please re-review canonical exact head |
Author
|
@ally please review exact head |
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: 97688f3
Prior Findings Dispositioned (2)
- prior:a1c467a important 1 — still-present —
server/src/services/heartbeat.ts:18369— The new checks correctly page past future-dated isolation retries, but both emergency cursors are still deleted beforeclaimQueuedRunsucceeds.claimRunWithExternalRuntimeSlotPoolcan return no claim atserver/src/services/heartbeat.ts:14756/:14783while leaving the selected dependency-ready row queued, so a deeper critical or recovery row can still lose its continuation and be hidden behind the same refused head candidate. - prior:a1c467a important 2 — fixed —
server/src/__tests__/heartbeat-dispatch-priority-sort.test.ts:1536— The post-cap target is now medium priority, and lines 1587-1590 assert thatresume_bounded_scan_after_capoccurs before its dispatch, so the critical lane can no longer make the regression pass accidentally.
Important Issues (1)
- [prior:a1c467a important 1]
server/src/services/heartbeat.ts:18369— Emergency-lane progress is cleared before claim success. Preserve or advance the critical/recovery cursor until the selected candidate is claimed or terminally mutated; when claim admission refuses it without changing the queued row, continue from the saved boundary instead of restarting at the same head candidate.
Strengths
- Isolation-deferred rows are now excluded from the emergency-lane stopping condition with deterministic critical and recovery coverage.
- The resume-cap regression now directly observes the ordinary delayed continuation before dispatch.
- The incremental fix is narrow and retains the bounded indexed query shapes.
Recommended Action
- Retain emergency-lane continuation state across claim refusals that leave the selected row queued, then add a focused external-slot-admission regression.
Author
|
@ally please review exact head This addresses the remaining Important finding from Added a focused external-runtime slot admission regression covering both critical and recovery lanes with a stale active slot reservation that forces Local verification:
|
Author
|
@ally review exact head |
Author
|
@ally please review exact head |
Author
|
@ally please review exact head |
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: 490e33a
Prior Findings Dispositioned (1)
- prior:a1c467a important 1 — fixed —
server/src/services/heartbeat.ts:18792— A refused emergency candidate is now re-read after admission; when it remains queued, lines 18805-18820 install main and emergency continuation cursors and schedule the matching detached lane, so the tested refused-head case no longer clears all continuation state before claim success.
Important Issues (2)
- [native-codex/concurrency]
server/src/services/heartbeat.ts:18144— Every keyset cursor round-tripscreated_atthrough a JavaScriptDateandtoISOString(), which preserves only milliseconds while PostgreSQL timestamps can retain microseconds. Runs inserted without an explicitcreatedAtuse the database timestamp, so a boundary row at.123456is read back as.123, and the next>predicate still includes every row at.123456; a full page at that timestamp can repeat indefinitely instead of advancing. The critical and recovery cursors repeat the same conversion at lines 18309 and 18422. Carry a precision-safe database cursor value (for example raw timestamp text or epoch microseconds) and add an embedded-Postgres regression using default-generated sub-millisecond timestamps. - [pr-review-toolkit/concurrency]
server/src/services/heartbeat.ts:18805— Refusal recovery advances the main cursor to the refused run and each emergency cursor to that run's own keyset position, but candidates are attempted in dispatch-rank order rather than(created_at, id)order. A newercriticalin-progress run can therefore be attempted and refused before an oldercriticaltodo run already collected from the same page; installing the newer run's cursor permanently skips the older candidate on the detached continuation. The new regression usesscanLimit: 1, so the refused row is necessarily keyset-oldest and cannot expose this ordering. Preserve the pre-page boundary or every unattempted keyset predecessor, and add a multi-row test where priority order differs from cursor order.
Strengths
- The exact-head change now verifies that admission refusal actually left the candidate queued before scheduling emergency continuation work.
- Critical and recovery refusal behavior is covered symmetrically, with a real external-runtime reservation conflict.
- CI is green across build, typecheck, server shards, serialized suites, and end-to-end coverage on this head.
Recommended Action
- Make all dispatcher keysets precision-safe for PostgreSQL timestamps.
- Preserve keyset-older candidates when a newer, higher-ranked emergency run is refused.
- Re-run the focused dispatcher, convergence, and query-plan suites before requesting review again.
Author
|
@ally please review exact head This addresses both Important findings from the
Regression coverage uses default-generated sub-millisecond PostgreSQL timestamps and a multi-row page where a newer in-progress critical run ranks ahead of an older todo critical run, for both critical and recovery refusal paths. Verification:
The branch remains integrated with current |
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: 3de4e56
Prior Findings Dispositioned (2)
- prior:490e33a important 1 — fixed —
server/src/services/heartbeat.ts:17871— Dispatcher scans now selectcreated_at::textand carry that exact PostgreSQL representation through the main, critical, and recovery keysets (:18164,:18338,:18457), avoiding the lossyDate/toISOString()round-trip. - prior:490e33a important 2 — fixed —
server/src/services/heartbeat.ts:18370— Emergency candidates now retain the cursor from before their containing page; refusal recovery restores that boundary at:18844-18860and excludes only the refused run, so rank ordering cannot skip keyset-older candidates.
Important Issues (2)
- [native-codex/concurrency]
server/src/services/heartbeat.ts:18923— A lone emergency run refused by atomic admission can remain queued indefinitely. The detached continuation excludes it throughdispatchDeferredRunIdsByAgent; when no other candidate exists, this branch clears that exclusion and returns without scheduling another pass. Releasing the conflicting external slot therefore does not retry the refused row unless an unrelated wake happens. Schedule a bounded retry after exhausting an exclusion-only pass, or retain an explicit retry path for the refused run; add a regression with no second candidate/completion to trigger dispatch. - [gstack/review/error-handling]
server/src/services/heartbeat.ts:18821— The new status re-read can throw after an earlier candidate has already been CAS-claimed but beforelaunchClaimedRuns()executes. With multiple slots, a transient database failure while examining a later refused emergency row rejects the lock callback and leaves the earlier run markedrunningwithout ever invoking its adapter. Ensure accumulated claims launch in a guaranteed cleanup path, or handle this re-read failure without abandoning already-claimed runs.
Strengths
- The precision-safe cursor representation closes the PostgreSQL microsecond boundary defect consistently across all three scans.
- Saving the pre-page emergency boundary correctly separates dispatch-rank order from keyset order.
- The new exact-head tests exercise real sub-millisecond database timestamps and multi-row refusal ordering.
Recommended Action
- Preserve retry liveness when the refused row is the only remaining candidate.
- Guarantee already-claimed runs launch if refusal bookkeeping fails.
- Add focused regressions for both paths before requesting another review.
Author
|
@ally please review exact head This addresses both Important findings from the
Focused regressions cover a lone critical run with no completion or second candidate to trigger dispatch, and an injected refusal status-read failure after an earlier run has already been claimed. Verification:
The branch remains integrated with current |
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: c6b24b1
Prior Findings Dispositioned (2)
- prior:3de4e56 important 1 — fixed —
server/src/services/heartbeat.ts:17949— Exhausting an exclusion-only pass now arms one deduplicated delayed admission retry; its callback clears the refused-run exclusions and dispatches from the head, and the no-claim exits at lines 18682-18685 and 18957-18960 invoke it. - prior:3de4e56 important 2 — fixed —
server/src/services/heartbeat.ts:18947— The claim loop now launches all accumulated CAS claims in its catch path before rethrowing a later refusal-bookkeeping error, withlaunchClaimedRunsguarded against duplicate invocation at lines 18897-18916.
Important Issues (1)
- [pr-review-toolkit/tests]
server/src/__tests__/heartbeat-dispatch-priority-sort.test.ts:2666— The new refusal-read regression awaitsresumeQueuedRuns()as a successful call, but the injected hook throws at line 2572 and the production catch deliberately rethrows atheartbeat.ts:18949. The test therefore rejects before reaching the assertions at lines 2669-2671, so it cannot verify this fix;toContainwould also allow the first run to launch more than once. Assert the expected rejection, then wait for the claimed run to settle and assert thatfirstRunIdwas dispatched exactly once.
Strengths
- The delayed admission retry is per-agent and deduplicated, while clearing only temporary refusal exclusions before rescanning.
- Claimed-run launching is centralized behind an explicit exactly-once guard.
- The lone-refusal regression exercises the important no-completion and no-second-candidate liveness geometry.
Recommended Action
- Correct the failure-injection regression so it reaches and strengthens its launch assertions.
- Re-run the focused dispatcher test before requesting review again.
Author
|
@ally please review exact head |
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: 7211db1
Prior Findings Dispositioned (1)
- prior:c6b24b1 important 1 — fixed —
server/src/__tests__/heartbeat-dispatch-priority-sort.test.ts:2666— The regression now asserts that the injected refusal status read rejects, waits for the earlier CAS-claimed run to settle at line 2669, and requires exactly one dispatch of that run at line 2672. The two-slotcodex_localfixture at line 2589 ensures both ranked candidates are processed in the same pass.
Looks good. No Critical or Important issues remain on this head.
Strengths
- The incremental change is narrowly limited to making the failure-injection regression exercise the production rethrow and exact-once launch behavior.
- The assertion now detects both a missing launch and duplicate dispatch of the earlier claim.
Recommended Action
- Merge after the required checks and approvals are satisfied.
allyblockcast
approved these changes
Aug 3, 2026
allyblockcast
left a comment
allyblockcast
left a comment
Collaborator
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: 7211db1
Prior Findings Dispositioned (1)
- prior:c6b24b1 important 1 — fixed —
server/src/__tests__/heartbeat-dispatch-priority-sort.test.ts:2666— The regression now asserts that the injected refusal status read rejects, waits for the earlier CAS-claimed run to settle at line 2669, and requires exactly one dispatch of that run at line 2672. The two-slotcodex_localfixture at line 2589 ensures both ranked candidates are processed in the same pass.
Looks good. No Critical or Important issues remain on this head.
Strengths
- The incremental change is narrowly limited to making the failure-injection regression exercise the production rethrow and exact-once launch behavior.
- The assertion now detects both a missing launch and duplicate dispatch of the earlier claim.
Recommended Action
- Merge after the required checks and approvals are satisfied.
13 tasks
13 tasks
kkroo
pushed a commit
that referenced
this pull request
Aug 5, 2026
…en no branch tier is available (BLO-21312) github_pr_review_requested arrives via issue_comment, whose payload carries no pull_request.head.ref, so the BLO-20886 case-insensitive branch tier is structurally unreachable on that path -- a PR naming its owner only via Issue:/Paperclip task:/Paperclip issue:/Paperclip QA task: (real shapes on #931, #963, #976, #916) still failed closed to no_owning_reference. Add a fourth, lowest-priority tier to resolveOwningPaperclipIdentifiers for these house labels. Ranked below both the closing-keyword and branch tiers so pull_request-sourced resolution is unchanged; it only activates when title, closing keyword, and branch (when available) are all empty. Co-Authored-By: Claude Sonnet 5 <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
3 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
The queued-run dispatcher needs to preserve strict per-agent serialization while bounding database work under that lock. The prior PR cannot pass the independent review gate because Ally authored it.
What Changed
Verification
pnpm --filter @paperclipai/server typecheckheartbeat-dispatch-priority-sortcritical paging/convergence testpnpm exec vitest run packages/db/src/heartbeat-dispatch-query-plan.test.tsgit diff --checkThe query-plan test asserts candidate scans use
heartbeat_runs_agent_dispatch_idx, issue resolution usesissues_pkey, and both statements inspect work bounded by the configured scan limit rather than total queue depth.Risks
Model Used
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template