perf(heartbeat): keep deferred runs out of dispatch probe SQL (BLO-20736) - #1069
Open
kkroo wants to merge 2 commits into
Open
perf(heartbeat): keep deferred runs out of dispatch probe SQL (BLO-20736)#1069kkroo wants to merge 2 commits into
kkroo wants to merge 2 commits into
Conversation
1 similar comment
|
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. |
…depth (BLO-20736) The dispatcher's keyset scan filters agent_id = $1 AND status = 'queued' and orders by (created_at, id) with LIMIT 200. PostgreSQL estimates the two equality qualifiers independently and multiplies their selectivities, but they are almost perfectly correlated: a backlogged agent's rows are overwhelmingly queued, an idle agent has none. Measured on a 200k-row interleaved fixture the estimate is 250x low at queue depth 1000 (rows=4 vs 1000) and 42x low at depth 5000 (rows=118 vs 5000). On an estimate that small the planner stops treating LIMIT as a reason to preserve index order and picks Bitmap Heap Scan + top-N Sort. A sort cannot emit until it has consumed its whole input, so the dispatcher read and sorted the agent's ENTIRE queue to return 200 rows, under the strict per-agent start lock, getting worse as the backlog deepened. Read the page in two phases instead. Phase 1 projects ONLY (created_at, id) -- both live in heartbeat_runs_agent_dispatch_idx behind the (agent_id, status) prefix, so the page comes from an ordered Index Only Scan with no heap access that stops after SCAN_LIMIT entries. Phase 2 hydrates exactly that page by primary key. Measured, at both depths: 1000 -> 200 and 5000 -> 200 rows actually read. The projection is what makes the plan insensitive to the estimate rather than dependent on fixing it: with no heap fetches to pay for, the ordered path's LIMIT-scaled cost stays ~10 whether the planner believes 118 rows or 5167, while the bitmap alternative must still materialize the whole match set before its sort emits. The plan stays correct at a still-42x wrong estimate. Deliberately NOT fixed with extended statistics: CREATE STATISTICS ON (agent_id, status) does correct the estimate and then makes the plan worse -- the planner switches to a BitmapAnd of two other indexes and still sorts. Measured at both depths, and inconsistent between them. Phase 2 takes the id list as its only SQL predicate and re-checks status in JS. Adding AND status = 'queued' there makes the dispatch index attractive again and PostgreSQL drives the fetch from it instead of the primary key, reintroducing the unbounded shape phase 1 exists to remove. Callers advance the cursor from the probe rather than from the hydrated rows, so a page thinned by a row leaving the queue between the two statements is not mistaken for the end of the backlog. The new plan test asserts both properties at two depths against one shared ceiling, on a fixture whose queued rows are interleaved one per heap page rather than packed at the tail -- against tight clustering bitmap+sort genuinely is the better plan, so the packed fixture would be asserting the wrong thing. Co-Authored-By: Claude <noreply@anthropic.com>
kkroo
force-pushed
the
codex/blo-20736-dispatch-probe-deferred
branch
from
August 6, 2026 02:33
1237347 to
f53da69
Compare
![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: f53da69
Critical Issues (0)
Important Issues (1)
- [gstack/review]
server/src/services/heartbeat.ts:18028— The new two-phase reader removes deferred IDs from phase-1 SQL for the ordinary and critical scans, but the recovery lane is not migrated and still appends the accumulatingdeferredRunIdsasNOT IN (...)to its ordered SQL probe (server/src/services/heartbeat.ts:18653). A refused recovery candidate is added to this set before another recovery continuation runs, so repeated admission refusals can grow the exclusion prefix and make PostgreSQL walk every deferred recovery row to return the next page. That preserves the same queue-depth-dependent work under the per-agent start lock that this PR is intended to remove.- Apply the same raw-page/cursor-first rule to the recovery lane: bound its indexed recovery probe without
NOT IN, capture the raw cursor, filter deferred IDs afterward, and base exhaustion on the raw page. Add a regression with several fully deferred recovery pages.
- Apply the same raw-page/cursor-first rule to the recovery lane: bound its indexed recovery probe without
Suggestions (2)
- [tests]
packages/db/src/heartbeat-dispatch-query-plan.test.ts:669— Exercise the production paging helper or a service-level dispatch path for the all-deferred-page case. The current test manually advances a SQL cursor, so it cannot catch regressions inprobed, raw-page cursor selection, post-probe filtering, or hydration/status-race handling. - [tests]
packages/db/src/heartbeat-dispatch-query-plan.test.ts:356— Add a plan case with partially cleared visibility-map bits and production-style bound parameters. The current immediateVACUUM ANALYZEfixture validates the favorable index-only state but not the churn window explicitly discussed in the implementation comments.
Strengths
- The raw probe owns cursor and exhaustion state, so fully filtered or concurrently transitioned pages still advance.
- The
(created_at, id)keyset preserves timestamp precision and deterministic boundaries. - The plan test uses fixed ceilings at two materially different backlog depths and verifies that the cursor predicate remains an index condition.
Recommended Action
- Address the recovery-lane exclusion path before merge.
- Add direct deferred-page behavior coverage; consider the visibility-map case as follow-up hardening.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Linked Issues or Issue Description
Refs: https://paperclip.blockcast.net/BLO/issues/BLO-20736
Refs: https://paperclip.blockcast.net/BLO/issues/BLO-20396
Supersedes: #1056
Bug/perf gap: The new dispatch probe was bounded for the ordinary queue, but
excludeRunIdscould put many deferred emergency-admission refusals intoNOT IN (...)inside the SQL predicate. Returning 200 non-deferred rows, or proving none remain, could therefore inspect a deferred backlog instead of a fixed raw page.What Changed
excludeRunIdsfrom the phase-1 dispatch probe predicate.probedand cursor from the raw page so an all-deferred page still advances.Verification
PATH=/opt/homebrew/bin:$PATH pnpm install --frozen-lockfilePATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server typecheckPATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/db typecheckPATH=/opt/homebrew/bin:$PATH pnpm exec vitest run packages/db/src/heartbeat-dispatch-query-plan.test.ts --testNamePattern "bounds the dispatch head scan independently of queue depth" --no-file-parallelism --maxWorkers=1imported successfully but skipped locally because embedded Postgres support is unavailable on this host (3 skipped). CI remains the execution signal for this DB-gated plan suite.Risks
Moderate, but scoped to dispatch paging. The intended behavior shift is that a page containing only deferred run ids now advances the dispatch cursor and returns no hydrated rows, rather than trying to find replacements inside the same SQL query. That can require additional continuation passes, but each pass is bounded and liveness is preserved by advancing from the raw cursor.
Model Used
Codex GPT-5 coding agent with shell, GitHub CLI, and local typecheck/test execution.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template