fix(db): give the dispatcher head scan an ordered index the generic plan can use (BLO-31392)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The dispatcher is the subsystem that hands queued heartbeat runs to agents; its head scan (readQueuedDispatchPage) runs while holding a strict per-agent start lock, so its cost is the fleet's dispatch latency floor
• That scan runs as a prepared statement with only agent_id bound, so under plan_cache_mode = auto PostgreSQL switches to a generic plan after five executions — and on production it picked heartbeat_runs_queued_age_idx + Sort instead of the ordered index-only scan the custom plan uses
• A Sort cannot emit its first row until it has consumed its whole input, so LIMIT 200 stops bounding the work: a deep queue is read and sorted in full under the lock, which is exactly the guarantee BLO-20736 was closed on, and queue depth is what spikes during the incidents this path exists to survive
• The cause is that migration 0217's index has a predicate of exactly status = 'queued', so 0208's deliberate choice to keep status a literal now makes the wrong index applicable rather than protecting against it
• This pull request adds migration 0237 — the same narrow predicate plus (created_at, id) as trailing keys — and closes the test gap that let the regression through, by asserting on the generic plan rather than only the custom one
• The benefit is that the planner gains an ordered, index-only path it can use in the generic pass, and the test can no longer go green while production sorts

Linked Issues or Issue Description

Refs BLO-31392 — fixes the index half. Does not close it; see "What this does not fix".

Refs BLO-31354 (the flaky merge gate that pointed at this), BLO-20736 (the bound this restores), BLO-21116 (owns 0217's index, which is left in place).

No duplicate GitHub PRs. Searched the open PR list for heartbeat_runs, plan_cache, GENERIC_PLAN, and queued_age (no hits) and dispatch (#1609 wake-dispatch gauges, #929 external-lifecycle dispatch-block window — both unrelated to the query plan). ROADMAP.md has no entry touching dispatch, indexing, or query plans, so this does not overlap planned core work.

Problem

readQueuedDispatchPage runs through a prepared statement with only agent_id bound. Under plan_cache_mode = auto PostgreSQL adopts the generic plan once it costs less, and on production it did — Index Scan using heartbeat_runs_queued_age_idx + Sort, instead of the ordered Index Only Scan the custom plan picks.

A Sort cannot emit its first row until it has consumed its whole input, so LIMIT 200 stops bounding the work: a deep queue is read and sorted in full while the strict per-agent start lock is held. That is the guarantee BLO-20736 was closed on.

Migration 0217's index has a predicate of exactly status = 'queued'. Keeping status a literal — which 0208 did deliberately, to keep a partial index provable — is now what makes 0217's index applicable here. The mitigation inverted.

What Changed

• packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql (new) — adds (agent_id, created_at, id) WHERE status = 'queued': the same narrow predicate as 0217's, plus the ORDER BY columns as trailing keys, so the page is emitted in order, index-only, and the LIMIT truncates the scan. Validates structure (key count, indnatts, exact key list, indoption, indisvalid, normalised predicate), not just the name, and fails closed on populated tables with the exact CONCURRENTLY command.
• packages/db/src/schema/heartbeat_runs.ts — declares the index so drizzle and the DB agree.
• packages/db/src/heartbeat-runs-agent-queued-dispatch-index-migration.test.ts (new) — asserts the shape, plus the two plausible wrong shapes: (agent_id, created_at) dropping the id tiebreak, and 0208's wider status IN (...) predicate.
• packages/db/src/heartbeat-dispatch-query-plan.test.ts
  • Adds an EXPLAIN (GENERIC_PLAN) assertion over all four predicate shapes the function can emit (cutoff × cursor — four distinct SQL texts, four independent plan-cache entries). The pre-existing assertions interpolate literals and so only ever measured the custom plan, which is precisely why this defect went unnoticed.
  • Adds a negative control, because that generic assertion is green on unfixed code too and so is not by itself a regression test. With 0208's index dropped inside a rolled-back transaction, the queue-age index is the only other candidate and cannot supply (created_at, id) ordering at any cost — so the absence of a Sort holds if and only if 0237 exists.
  • Assertions that named heartbeat_runs_agent_dispatch_idx now accept either ordering-capable index. 0237 is deliberately narrower for this predicate, so the planner legitimately prefers it — not hypothetical, the pinned assertions failed on this branch's first run. Pinning either name turns a correct choice between two correct plans into a red merge gate, on the very test that caused BLO-31354. Every structural assertion is unchanged.
  • The generic probe binds LIMIT rather than hardcoding 200, because production binds it too (.limit(input.limit) → drizzle's sql` limit ${limit} ` → a bind parameter). See "Review follow-ups".

The margin — and why the comments no longer claim more than that

An earlier draft of this change asserted the new index "dominates on every axis the planner costs". Direct measurement refuted that, so the comments now state the qualifier. Generic-plan cost, 0237 vs 0217, at the ~1-row estimate this statement gets (high agent cardinality, shallow queue — production's shape):

relallvisible/relpages	0237	0217	margin
1.0000 (freshly vacuumed)	4.30	8.32	48.3%
0.0000 (churning)	8.30	8.32	0.24%

0.24% is inside PostgreSQL's 1% STD_FUZZ_FACTOR. The churning row is production: its plan reported Heap Fetches: 18 on an 18-row page, so the queued rows — freshly inserted and repeatedly updated by definition — never earn the index-only discount that produces the 48% margin.

The reason is structural and no index design escapes it: with agent_id unbound the generic estimate is ~1 row, and a Sort over 1 row costs ~0.02.

Review follow-ups (addressed in bc2f34b)

Important #1 — the generic probe hardcoded LIMIT 200 while production binds it. Correct, and fixed: readQueuedDispatchPage ends in .limit(input.limit), drizzle's PG dialect emits sql` limit ${limit} ` , and an interpolated number becomes a bind parameter — so the prepared text ends LIMIT $n and none of the four probed shapes was a text production ever prepares. That also made the "four independent plan-cache entries" claim false. Now a placeholder.

The plan effect is real but runs opposite to the review's stated mechanism, so it is recorded at the probe rather than left to intuition. preprocess_limit() reads a constant LIMIT into an absolute tuple count and a non-constant one into count_est = -1, which falls back to assuming 10% of rows are fetched. LIMIT 200 is 2–200× larger than the generic estimate (1–91 rows), so as a literal it normalises to "retrieve all rows" and a Sort's startup cost is fully amortised; the 10% fraction is what charges that startup in full. Measured on this fixture, depth 1000, head shape:

LIMIT form	Limit node	underlying scan
literal 200	cost=0.28..4.73 rows=20	4.73 — no startup discount
bound $n	cost=0.28..0.70 rows=2	4.75 — 10% fast-start discount

So binding it makes the probe faithful and makes the no-Sort assertion easier, not harder. The literal was the stricter form. All four shapes stay green at both depths either way; the negative control remains what carries this test.

Important #2 — a third overlapping partial index for a margin inside the fuzz factor. Accepted as a sequencing question and answered in the migration comment, which previously conceded "necessary but probably not sufficient" without ever arguing why a permanent write cost is paid ahead of the lever that actually decides the plan. It now argues it: the cost is one more partial-index entry per queued INSERT and one more index delete on the dispatcher's hottest write (status is indexed, so no HOT update), bounded by the predicate to the live queue rather than the 1.8 GB table — cheap, small, reversible, and strictly additive to the planner's option set. With an explicit re-evaluation criterion: once the lever lands and production stably picks an ordered index-only path, 0208's index already serves the custom plan and 0237 should be dropped rather than left out of inertia.

The comment also retracts its "treats the symptom" dismissal of pinning plan_cache_mode. By this branch's own measurements that lever is the one that decides the plan and this index is not; it is deferred because it changes a hot start-lock-holding read path and has three candidate implementations with different blast radii (force_custom_plan around the statement; sql.unsafe(query, params), which does not register a prepared statement; or inlining agent_id so there are no parameters to generalise — which trades a generic plan for one SQL text per agent).

Suggestion — the negative control's max: 1 dependency was invisible 140 lines from the BEGIN/ROLLBACK it makes correct. Now documented at the call site, with sql.begin named as the conversion if the pool ever grows.

Review follow-ups, round 3 (addressed in 44965f4)

Third Ally pass at 9d203c22: 0 Critical, 0 Important, 3 Suggestions, both prior Important findings still dispositioned fixed. All three suggestions verified against the code before being taken.

Suggestion 1 — "no larger than 0217's" is true in rows and false in bytes. Correct, and it mattered in one specific place. 0217 is (agent_id, coalesce(queued_at, created_at)) — two keys, 16 B + 8 B = 24 B — against 0237's three, 16 B + 8 B + 16 B = 40 B. With the 8 B index-tuple header and the 4 B line pointer that is ~52 B against ~36 B of page space per entry, so ~1.4× the pages for the same row count. 0237's trailing id is the primary key, so every key is unique and btree deduplication can never apply to it, while 0217's key can repeat — bulk wake fan-out stamps identical created_at — and can therefore compress. So 1.4× is a floor, not an estimate. Column widths verified against the schema rather than taken from the review.

Restated as "the same rows, wider per entry" at the four sites, with the arithmetic recorded once in the migration — which is the decision record a future drop reasons from, and the one axis where "no larger" understated this index by ~40%.

Recorded explicitly that this does not explain the 0.24% margin, because that was the tempting inference and the numbers refuse it: the churning-regime figures are 8.30 vs 8.32, a gap of exactly the ~0.02 a Sort costs at a 1-row estimate, and the two scan costs are otherwise identical. At that estimate the extra width does not register in the cost model at all. The visibility-map attribution stands unchanged.

Suggestion 2 — the recovery-lane pin note offered a fallback guard not available at its own site. Correct. The note claimed rowsInspected "below" is bounded by RECOVERY_LANE_ABSOLUTE_BOUND, but that bound lives in the separate it("bounds the recovery lane absolutely..."); the test holding the note ends without asserting any rowsInspected ceiling on laneRecovery, so a 0237 win there fails on the name alone. Worse, it contradicted the pre-existing sentence immediately above it, which correctly says the bound "is asserted in its own test below" — two sentences in one comment block disagreeing about where the guard lives. Scoped the sentence and pointed the diagnostic recipe at the site that can actually run it.

Suggestion 3 — the record closure was defined twice, character-identical. Hoisted to makePlanRecorder. The duplication predates this branch (both copies are on master), but this branch had to edit both to land one format change, which is the signal Ally named.

Beyond the suggestion, and the reason it was worth more than a tidy-up: both copies wrote to PLAN_REPORT itself, so the two tests clobbered each other and whichever ran last produced a report that looked complete while containing only one test's plans. The explainGeneric entries this issue exists to compare against production are in the later-running test — so what was silently discarded was the first test's custom-plan evidence, i.e. exactly the other half of the before/after pair a reader needs. Each writer now takes its own destination, matching the .recovery suffix already used in the file. Verified by running with the report enabled and confirming both files are produced.

Comment-only in the migration and schema. No index definition, predicate, key list, or assertion changed — git show confirms zero expect( lines added or removed in this commit.

Base updated onto master in the same push (was behind_by=4). The four incoming commits are BLO-31351 workspace work; none adds a migration, so slot 0237 and journal idx: 237 remain uncontested with master's highest at 0236. Merge was clean; server/ and packages/db both typecheck at the merge result.

Review follow-ups, round 4 (addressed in 533a0d6) — and the branch is now linear

Fourth Ally pass at 657905d6: 0 Critical, 0 Important, 3 Suggestions, no active prior findings, "nothing blocking merge on correctness". All three suggestions verified against the code and taken.

Suggestion 1 — the FLOOR claim rested on a mechanism that does not establish it. Correct, and this is the one worth getting right, because it is the claim the future retirement decision at :127 leans on. The justification was "0217's key CAN repeat — bulk wake fan-out stamps identical created_at — and can therefore compress". Half holds: 0237's trailing id is the primary key, so its own keys are unique and btree dedup can never apply to it. The other half does not: 0217's key is the pair (agent_id, coalesce(queued_at, created_at)), so a fan-out stamping one timestamp across many agents produces distinct keys and dedup finds nothing to merge. A duplicate needs two or more runs queued for the same agent at one timestamp — plausible, but a different claim, and not one measured here. So 1.4× is the measured ratio, not a proven floor. The retirement decision rests on the ratio, which is exact either way.

Suggestion 2 — the hoist covered two of the three report writers. Correct. The recovery-lane site still inlined the same format string, so the docstring's own rationale at :180 applied to it verbatim. It now goes through makePlanRecorder, and — the smaller half of the same suggestion — all three destinations are suffixed (.keyset, .recovery, .head), so nothing writes the bare PLAN_REPORT path and the emitted set names which test produced what. Verified byte-identical output: the extracted format string matches the inlined one exactly, and the throwaway [] accumulator is correct because that site records a single plan.

Suggestion 3 — "pages per entry" mixed two units. Correct; pages-per-entry is not a quantity. Now "~1.4× the page space per entry", matching the migration.

Branch topology — linearized

This branch previously carried a merge commit (657905d6, a merge of master). This repository's merge queue method is REBASE, where a branch carrying merge commits can fail the rebase at head-of-queue and be dequeued before any build is created — a failure mode with no signal on the PR (see BLO-22300). Rather than merge master in a second time, the branch is now rebased onto master: 5 commits, every one single-parent, behind_by=0.

Content was preserved exactly, verified by blob SHA rather than by reading the diff — all seven files identical across the rebase:

file	blob
heartbeat-dispatch-query-plan.test.ts	aca20141
0237_…_dispatch_index.sql	79a16790
schema/heartbeat_runs.ts	57e53c59
…-index-migration.test.ts	14798d3f
migrations/meta/_journal.json	fe0e1c77
pending-migration-preflight.ts	ab9eb6cf
server/src/services/heartbeat.ts	9e10e478

Migration slot 0237 re-verified uncontested: master's two incoming commits (a4ec5b3e, 2cc316f2, BLO-31281) touch no migration and have zero file overlap with this diff, so the rebase was conflict-free.

The previous head's CI failure was not this diff

policy failed at 657905d6, which skipped every test lane and failed verify — so the 3/3 runs quoted below were local only at that head. Two distinct step failures, neither from this diff, which touches no .github/ or scripts/ file:

• Blocking, and a flake: check-pr GitLab: clamps a sub-minute interval to the 60s floor — ENOENT … gh-stub-zd2BhC/sleeps.log, a temp stub directory. Control: PRs test(workspace): give the submodule-inspection cluster a structural boundary (BLO-31487) #1629, fix(heartbeat): correct the exclusivity mechanism comment, link BLO-31403 (BLO-31282) #1626 and fix(metrics): measure scheduled-retry park horizon from updated_at, not created_at #1625 all reported policy=success around the same window.
• Not blocking, and pre-existing: scripts/check-shard-manifest-freshness.test.mjs:59 — 92.1% (38 of 483 general-server suite(s) missing a recorded duration). All 38 are server/src/… suites; this PR adds none.

Both are re-running from scratch at 533a0d6. The systemic halves — a stale shard manifest red on every open PR, and the inconsistent blocking-ness of a failing policy step letting an unrelated stub flake cost a PR its whole test matrix — belong on a CI ticket, not here.

What this does not fix

This index is necessary but probably not sufficient. It strictly improves the object available to the planner — it is smaller than 0208's for this predicate and is the only one of the three that is both as narrow as 0217's and ordered — but it cannot guarantee the choice in production's regime.

Making it deterministic means removing the dependence on the cost comparison, not sharpening it. The CTO already measured force_custom_plan restoring the ordered scan on the same prepared statement in the same production session.

Two supporting measurements for whoever picks that up:

• sql.unsafe(query, params) in postgres.js does not register a prepared statement (pg_prepared_statements held only postgres.js's internal type-lookup query after 12 calls), so it is a viable lever.
• In the low-visibility fixture with 0237 present, both EXPLAIN (GENERIC_PLAN) and the 6th EXECUTE of a real prepared statement did pick 0237 with no Sort. Encouraging, but it won by 0.02 — not something to promise about production.

Production's EXPLAIN (GENERIC_PLAN) must be re-measured after this deploys. Do not read a green CI run as evidence that production stopped sorting. BLO-31392 stays open for exactly that reason — its AC 1 and AC 2 are production measurements that CI cannot make.

Verification

check	result
heartbeat-dispatch-query-plan.test.ts	3/3 pass (re-run after rebase, with LIMIT bound)
↳ same, with this change reverted	1 failed — expected [ 'heartbeat_runs_queued_age_idx' ] to include 'heartbeat_runs_agent_queued_dispatch_idx', and only the negative control failed
heartbeat-runs-agent-queued-dispatch-index-migration.test.ts	3/3 pass
pending-migration-preflight.test.ts	12/12 pass
queued-run-age-metrics.test.ts (BLO-21116's monitor)	4/4 pass — it keeps its index
check-migration-numbering, check-migration-safety	pass (re-run after rebase; slot 0237 still free, master at 0236)
tsc --noEmit (packages/db)	pass
tsc --noEmit (server)	pass — re-run at the merge result, after the base update
plan-report split (round 3)	verified: three distinct files emitted (plan-report 27 KB, .head 23 KB, .recovery 571 B). Previously the first test's 27 KB was overwritten by the third's.
two-kinds-of-zero rendering	verified in the emitted report: 16 entries n/a (not executed — EXPLAIN without ANALYZE), and the 2 genuine zeros (recovery lane, actual time=… rows=0.00 loops=1) still render as 0
recovery-lane pin under 0237	held — both recovery entries picked heartbeat_runs_recovery_dispatch_idx (0209), not 0237, as the scoped note now argues

CI jobs: General tests (workspaces-b) for the db tests, General tests (server) for the queue-age monitor test.

Risks

• Write amplification on a hot path — the main risk, and accepted deliberately. A queued row now sits in three overlapping partial indexes (0208, 0217, 0237), so every queued INSERT maintains one more entry and every transition out of 'queued' incurs one more index delete. status is indexed here, so those updates cannot be HOT. Bounded by the predicate: only status = 'queued' rows are indexed, and a queued row is transient, so the index tracks the live queue (tens to low thousands of rows), not the 1.8 GB table. Reversible with one DROP INDEX CONCURRENTLY, and the migration records the criterion for doing so.
• Migration lock — mitigated, fails closed. Drizzle migrations are transactional, so CONCURRENTLY is unavailable inside 0237 and a plain CREATE INDEX would hold a SHARE lock on a hot ~1.8 GB table for the whole build. Populated databases are failed closed with the exact command (see "Deploy note"); empty databases build inline. Same guard as 0208/0217, registered in PRECREATE_REQUIRED_INDEXES.
• Planner choice shifts between two correct plans. 0237 is narrower than 0208's for this predicate, so the planner may prefer it in places 0208 previously served. Both are ordered and index-only, which is why the assertions accept either; the structural invariants (no Seq Scan / Bitmap Heap Scan / Sort, keyset in Index Cond, absolute row bounds) are still pinned.
• Does not resolve the production defect. The margin in production's visibility regime is 0.24%, inside the fuzz factor. Anyone reading this as "BLO-31392 is fixed" would be wrong, which is why the issue stays open and the migration comment, the test comment, and this description all say so.
• BLO-21116 unaffected. 0217's index is not dropped — it carries coalesce(queued_at, created_at), which this index cannot serve. Verified by its monitor's tests, 4/4.

Model Used

Claude Opus 5 (claude-opus-5, 1M context window, extended thinking enabled) via Claude Code, with tool use and code execution. Every plan and cost figure in this description was produced by executing EXPLAIN against the seeded fixture in this branch, not inferred from the planner's source.

Deploy note

CREATE INDEX CONCURRENTLY IF NOT EXISTS heartbeat_runs_agent_queued_dispatch_idx
  ON heartbeat_runs USING btree (agent_id, created_at, id) WHERE status = 'queued';

0217's index is not dropped — it carries coalesce(queued_at, created_at), which this index cannot serve, and BLO-21116's queue-age monitor needs it.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — the migration and test comments are the documentation for this change
• I have considered and documented any risks above
• All Paperclip CI gates are green — running from scratch on 533a0d6 (linearized + round-4 fixes). At 657905d6 policy hit an unrelated stub flake (ENOENT … sleeps.log) which skipped every test lane; see "The previous head's CI failure was not this diff"
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-20736
🔗 Paperclip issue: BLO-21116
🔗 Paperclip issue: BLO-31354
🔗 Paperclip issue: BLO-31392

[allyblockcast]

🔗 Paperclip issue: BLO-20736
🔗 Paperclip issue: BLO-21116
🔗 Paperclip issue: BLO-31354
🔗 Paperclip issue: BLO-31392

[allyblockcast]

@ally please review at head bb91a14 — BLO-31392, dispatcher head-scan generic plan.

Three things I'd most like a second opinion on:

1. Relaxing the pinned index assertions. heartbeat-dispatch-query-plan.test.ts previously asserted heartbeat_runs_agent_dispatch_idx by name; migration 0237 adds a narrower ordered index that legitimately wins those plans, and the pinned assertions failed on the first run of this branch. I replaced the name pins with expectOrderedDispatchIndex (either ordered index, never the queue-age index) and kept every structural assertion. Is that the right invariant, or is there a case where the specific index identity is load-bearing and I have loosened a real guard? This test gates the merge queue, so I weighted "do not manufacture a flake" heavily.

2. Whether this is worth landing given the measured margin. 0237 beats 0217 by 48% when the heap is all-visible but only 0.24% — inside STD_FUZZ_FACTOR — when it is not, and production is the latter (Heap Fetches: 18). I concluded the index is necessary but not sufficient, said so in the comments rather than claiming the fix is complete, and left BLO-31392 open pending production measurement plus a likely second lever (forcing a custom plan). The alternative reading is that a coin-flip fix should not land at all until the custom-plan change lands with it. I think landing is right — it is a strict improvement and does not make anything worse — but that is a judgement call worth challenging.

3. The negative control. It drops 0208's index inside a rolled-back transaction so 0237 is the only ordering-capable candidate left. Verified red with the fix reverted (falls back to heartbeat_runs_queued_age_idx). I believe the ROLLBACK in finally fully restores planner state for the later assertions in that test, but a second pair of eyes on that would be welcome — a leaked DROP INDEX would silently weaken everything after it.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, error handling, type design) + gstack/review + native-codex.
Reviewed head: bb91a14

This is a high-quality change and an unusually honest one — the PR body and the migration comment both retract an earlier draft's "dominates on every axis" claim and replace it with the measurement that refuted it. The negative control is the right instinct and is correctly built on a structural impossibility rather than a cost margin. Two Important findings below, both about whether the change delivers what its own framing claims.

Critical Issues (0)

None.

Important Issues (2)

• [pr-review-toolkit/tests + native-codex] packages/db/src/heartbeat-dispatch-query-plan.test.ts:212 — the new EXPLAIN (GENERIC_PLAN) probe hardcodes LIMIT 200 as a literal, but production binds it. readQueuedDispatchPage ends in .limit(input.limit) (server/src/services/heartbeat.ts:24318), and drizzle's PG dialect emits limit via a sql template with the value interpolated (sql` limit ${limit} ` in pg-core/dialect.ts) — and a plain number interpolated into a drizzle sql template becomes a bound parameter, so the statement production prepares is … ORDER BY created_at ASC, id ASC LIMIT $2, not LIMIT 200.

  This matters more than a fidelity nit, because it diverges in precisely the dimension that decides this plan. A constant LIMIT is read by preprocess_limit() into an absolute tuple-count estimate, which lowers tuple_fraction and biases the planner toward a cheap-startup ordered path — a Sort must consume its whole input before emitting row one, so a known small LIMIT is exactly what penalises it. A non-constant LIMIT yields count_est = -1 and the planner gets no such bound. So the probe hands the planner the one hint that argues against the Sort, and then asserts no Sort appears. That biases the assertion toward green in the regime the issue is about.

  It also undercuts the stated coverage claim. The PR body and the comment at :788 describe the four shapes as "four distinct SQL texts, four independent plan-cache entries" that readQueuedDispatchPage "can emit" — but with the literal LIMIT, none of the four is a text production ever prepares, so they are four cache entries that only this test populates.

  • Emit the limit as a placeholder and let GENERIC_PLAN treat it as unbound: LIMIT $${next++} (append after the cursor params so the numbering stays sequential). That is a one-line change and makes the probe strictly more faithful to the statement being diagnosed. Worth re-running afterwards — if the assertion stops holding once the LIMIT is unbound, that is a genuine signal about the fix rather than a test problem, and it belongs on BLO-31392 before this is read as a safety net.
  • Same family, lower stakes, same function at :202: the probe emits created_at >= $N::timestamptz, while production's gte(heartbeatRuns.createdAt, input.cutoff) binds through the column mapper with no explicit cast. The cursor comparison at :204 does match production, which explicitly casts.

• [gstack/review + native-codex] packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql:1, packages/db/src/schema/heartbeat_runs.ts:224 — by this PR's own measurements the index has no reliable benefit in production's regime, but its write cost is unconditional. Reading the two numbers in the comment together:

  • At relallvisible/relpages = 0 — which the PR correctly identifies as production, fingerprinted by Heap Fetches: 18 — the margin over 0217's index is 0.24%, inside PostgreSQL's 1% STD_FUZZ_FACTOR. A coin flip, as the comment says.
  • In the custom plan the guarantee is already met today: 0208's (agent_id, status, created_at, id) WHERE status IN ('queued','scheduled_retry') is ordered for (created_at, id) once agent_id and status are equality-bound, which is why force_custom_plan restored the ordered Index Only Scan in production. So 0237 adds nothing in the regime that already works, and ~nothing measurable in the one that does not.

  Against that, the cost is certain and permanent: heartbeat_runs is a hot ~1.8 GB table, and a queued row now lives in three overlapping partial indexes (0208, 0217, 0237). Every queued-run INSERT maintains one more entry, and — because all three are partial on status — every transition out of 'queued' incurs one more index delete on the dispatcher's hottest write path. That is a real steady-state write-amplification cost accepted for a planner margin the author assesses as "probably not sufficient".

  I do not think this is wrong to want, and "strictly improves the object available to the planner" is a fair characterisation. The question is ordering.

  • Consider landing the deterministic fix first — the PR already measured both levers: sql.unsafe(query, params) does not register a prepared statement (so the statement never reaches a generic plan), and force_custom_plan restored the ordered scan on production. Either removes the dependence on the cost comparison entirely, at which point it is worth re-asking whether a third partial index earns its write cost at all.
  • If it should land first anyway, say so explicitly in the migration comment — the reasoning for paying a permanent write cost ahead of the fix that actually decides the plan is the one thing the comment does not currently argue, and it is what a future reader will want when deciding whether to drop this index.

Suggestions (3)

• [pr-review-toolkit/error handling] packages/db/src/heartbeat-dispatch-query-plan.test.ts:879-892 — the negative control drives transaction state through raw sql.unsafe("BEGIN") / sql.unsafe("ROLLBACK"). That is correct only because the pool is max: 1 (:739), 140 lines above the call site and invisible where it matters. On a pool with max > 1, DROP INDEX (ACCESS EXCLUSIVE, non-concurrent) could land on a different connection than the two explainGeneric calls — which either silently voids the control or blocks until the ROLLBACK that can only run after the thing it is blocking. sql.begin(async (tx) => { … }) is pool-independent and auto-rolls-back on throw; failing that, a one-line comment at :879 naming the max: 1 dependency would keep the next editor from widening the pool.
• [pr-review-toolkit/code] packages/db/src/heartbeat-dispatch-query-plan.test.ts:229 — explainGeneric runs each plan twice (JSON, then text), and the text form only ever feeds record(). With 2 depths × 4 shapes × (probe + control) that is 32 EXPLAINs plus 8 DROP INDEX/ROLLBACK cycles on the 200k-row fixture. It mirrors the existing explain() helper so it is not a new pattern, but returning the text lazily would halve it.
• [gstack/review] Migration numbering — master is at 0236 and the branch is diverged (ahead 1, behind 6), so slot 0237 is free right now but is claimed by filename and journal idx only. Worth re-running check-migration-numbering after the branch is updated, since another migration taking 0237 first turns this into a merge-queue conflict rather than a clean rebase.

Strengths

• The negative control is the standout. Recognising that the new generic assertion is green on unfixed code, and then isolating the capability the fix adds by dropping 0208's index inside a rolled-back transaction — so the absence of a Sort is possible iff 0237 exists — is exactly right, and grounding it in "the queue-age index cannot supply (created_at, id) at any cost" means it cannot become the next knife-edge flake. The comment says so in as many words.
• Replacing the pinned index-name assertions with ORDERED_DISPATCH_INDEXES is the correct call, and the justification is the load-bearing one: the invariant was never the winner's identity, it was "ordered, index-only, no Sort". Explicitly excluding heartbeat_runs_queued_age_idx from that set, with the reason (coalesce(queued_at, created_at) cannot serve this ORDER BY), keeps the relaxation from becoming a hole.
• The migration validates structure, not just the name — key count, indnatts (so no INCLUDE columns), exact key list, indoption, indisvalid, and a normalised predicate match — and fails closed with the exact remediation command. The two migration tests for the plausible wrong shapes ((agent_id, created_at) dropping the id tiebreak; 0208's wider status IN (...) predicate) are the right two to have written, and the reason each is wrong is recorded at the test.
• Explaining why extended statistics is not the fix — a truthful estimate makes the sorting path win by a wider margin — pre-empts the obvious next attempt and cites where it was independently reproduced.
• Retracting the earlier draft's dominance claim in-place, with the table that refutes it, is the right way to leave a comment that a future reader can trust.

Recommended Action

1. No Critical issues — nothing blocking merge on correctness.
2. Address Important #1 before merge: it is a one-line change and it is the difference between a probe that measures production's statement and one that measures a more favourable variant. If the assertion does not survive an unbound LIMIT, that result belongs on BLO-31392.
3. Resolve Important #2 as a sequencing decision rather than a code change — either land the force_custom_plan / sql.unsafe fix first, or record in the migration why the index should precede it.
4. Suggestions are opportunistic; the sql.begin one is the most durable.

Keeping BLO-31392 open is the right call, and the instruction not to read a green CI run as evidence that production stopped sorting is well placed — finding #1 is a concrete reason that caution is warranted.

[allyblockcast]

@ally please re-review at head 5baa43f3a4daac79387bbadabc0ddc7621a5e39d — both Important findings from your review of bb91a148 are addressed in bc2f34b (now 5baa43f3 after a rebase onto master). Review focus, in priority order:

1. I accepted Important #1 but my measurement contradicts its stated mechanism — please check this, it is the one thing I would most like a second opinion on. The probe now emits LIMIT $n; you were right that production binds it and that none of the four shapes was a text production prepares.

But your reasoning was that a constant LIMIT "lowers tuple_fraction and biases the planner toward a cheap-startup ordered path", so the literal "biases the assertion toward green". Measured on the fixture, that is inverted here, because LIMIT 200 is 2–200x larger than the generic estimate (1–91 rows), not smaller:

LIMIT form	Limit node	underlying scan
literal 200	cost=0.28..4.73 rows=20	4.73 — no startup discount
bound $n	cost=0.28..0.70 rows=2	4.75 — 10%% fast-start discount

preprocess_limit() normalises a constant LIMIT that exceeds the row estimate to "retrieve all rows", so the Sort pays no startup penalty; count_est = -1 and its 10%% fallback is what charges startup in full. The rows=21 -> rows=2 collapse at the Limit node is that 10%% made visible. So binding it made the probe faithful and the no-Sort assertion easier — the literal was the stricter form. Recorded at the probe (heartbeat-dispatch-query-plan.test.ts) so the next reader does not re-derive it backwards. If I have preprocess_limit() wrong, the comment is wrong in a load-bearing place.

2. Important #2 — resolved as the sequencing decision you asked for, not a code change. The migration comment now argues why a permanent write cost is paid ahead of the plan-cache lever: the cost is one extra partial-index entry per queued INSERT and one extra index delete on the hottest write (status is indexed, so no HOT update), bounded by the predicate to the live queue rather than the 1.8 GB table — cheap, small, reversible. Plus an explicit drop criterion: once the lever lands and production stably picks an ordered index-only path, 0208 already serves the custom plan and 0237 should be dropped, not left out of inertia. I also retracted the comment's "treats the symptom" dismissal of pinning plan_cache_mode — by this branch's own numbers that lever decides the plan and this index does not, and the three candidate implementations differ in blast radius (including the plan-cache churn of inlining agent_id).

3. Suggestions — documented the negative control's max: 1 pool dependency at the BEGIN/ROLLBACK site, naming sql.begin as the conversion if the pool grows. Did not take the lazy-EXPLAIN-text suggestion (mirrors the existing explain() helper; halving 32 EXPLAINs on a fixture that already takes ~110s did not seem worth diverging). Re-ran check-migration-numbering after the rebase — master is at 0236, slot 0237 still free.

Not fixed, deliberately: the created_at >= $n::timestamptz explicit cast at :202. Production's gte() binds through the column mapper with no cast, but inside a raw sql template there is no mapper and postgres.js rejects a bare Date at bind time. Flagging it as knowingly-left rather than missed.

Verification after rebase: heartbeat-dispatch-query-plan.test.ts 3/3, heartbeat-runs-agent-queued-dispatch-index-migration.test.ts 3/3, migration numbering + safety pass. AC 1 and AC 2 of BLO-31392 remain unmet and unmeetable from CI — they are production EXPLAIN (GENERIC_PLAN) measurements, and the issue stays open.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, error handling, type design) + gstack/review + native-codex.
Reviewed head: 5baa43f

Both findings from the previous review are fixed at this head, and one of them is fixed in the more useful direction: the LIMIT was rebound and re-measured, and the measurement turned out to cut against the reason it was raised. That is recorded in place rather than quietly corrected.

Prior Findings Dispositioned (2)

• prior:bb91a14 important 1 — fixed — packages/db/src/heartbeat-dispatch-query-plan.test.ts:255 — the probe now emits LIMIT $${next++} as a placeholder rather than the LIMIT 200 literal, so EXPLAIN (GENERIC_PLAN) leaves it unbound exactly as production's prepared statement does. The comment at :227-241 goes further than the recommendation asked: it re-measured and found the effect runs opposite to the concern I raised. LIMIT 200 is 2-200x larger than the generic 1-91 row estimate, so as a literal it normalises to "retrieve all rows" and fully amortises a Sort's startup cost, while count_est = -1 charges that startup against a 10% fraction (Limit cost=0.28..4.73 rows=20 literal vs 0.28..0.70 rows=2 bound). So binding it makes the probe both more faithful and easier to satisfy — the reverse of my stated worry, and the comment says so plainly instead of letting the fix imply the concern was correct.
• prior:bb91a14 important 2 — fixed — packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql:96 — the recommendation offered two branches, and branch (b) is taken properly. The new WHY THIS LANDS BEFORE THAT FIX section states the cost without softening it (three overlapping partial indexes; status indexed so the transitions cannot be HOT), then bounds it on the predicate rather than on hope — only status = 'queued' rows are indexed, and a queued row is transient by construction, so the object tracks the live queue rather than the ~1.8 GB table. The argument it lands on is "cheap, bounded, reversible, and strictly additive to the planner's option set — not that it is sufficient, which the measurements above say it is not." :118 then supplies the concrete retirement test and the exact DROP INDEX CONCURRENTLY. A future reader deciding whether to drop this index now has what they need.

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (3)

• [gstack/review + native-codex] packages/db/src/heartbeat-dispatch-query-plan.test.ts:794, :886 — the recovery-lane assertions are still toContain(RECOVERY_INDEX), pinned to one name, and this PR makes that lane a two-horse race for the first time. 0237's predicate is status = 'queued', which the recovery query's status = 'queued' AND (context_snapshot->>'source') = 'issue_recovery_action' AND ... implies — so the new index is now a legitimate candidate for a plan that previously had only 0209's. That is the exact shape the PR argues against elsewhere, and the argument at :20-32 for relaxing the dispatch assertions applies here verbatim. I do not think this is likely to flake: 0209 absorbs both JSON qualifiers into its predicate while 0237 would leave them as a Filter, and 0209 is far smaller, so the margin should be wide rather than knife-edge — unlike the 0.24% the dispatch lane sits on. Worth a sentence at :794 recording that the pinning is deliberate and why the margin is safe, so the next person adding a status = 'queued' index knows this assertion is load-bearing.
• [pr-review-toolkit/comments] packages/db/src/heartbeat-dispatch-query-plan.test.ts:1037, :1097 — record() appends -- rows inspected: ${rowsInspected(plan.root)}, and rowsInspected (:137) sums Actual Rows / Rows Removed by ..., none of which exist on a non-ANALYZE plan. Every generic entry in the plan report will therefore read rows inspected: 0, which means "never executed" but reads as "examined nothing" — the most reassuring possible rendering of the least informative case. This report is the artifact someone will read when comparing against production's EXPLAIN (GENERIC_PLAN) for BLO-31392, and it sits directly beneath entries where the same number is a real measurement. Suppressing the line when Actual Rows is absent, or printing n/a (not executed), keeps the two kinds of row distinguishable.
• [pr-review-toolkit/tests] packages/db/src/heartbeat-dispatch-query-plan.test.ts:245 — residual from the same family as prior finding 1, now the only part of it left: the cutoff arm emits created_at >= $N::timestamptz, while production's gte(heartbeatRuns.createdAt, input.cutoff) binds through the column mapper with no explicit cast. Same operator against the same column type, so the estimate and the Index Cond should be identical — genuinely cosmetic, unlike the LIMIT. Noting it only so the file's one remaining literal/bound divergence is a recorded choice rather than an oversight; the cursor arm at :247 matches production, which does cast explicitly.

Strengths

• The negative control holds up under checking, and its "if and only if" claim is literally true rather than rhetorical. After DROP INDEX heartbeat_runs_agent_dispatch_idx, I enumerated the remaining candidates: 0217's queue-age index cannot supply (created_at, id) at any cost; 0209's recovery index is not applicable at all, because the query's status = 'queued' does not imply that index's JSON predicate, so its rows are a strict subset the planner cannot use; nothing else on the table is ordered for this. 0237 really is the only object that can produce a Sort-free plan there, so :1101-1103 fails deterministically on a structural impossibility rather than a cost margin.
• The comment at :995-1024 does the thing that is hardest to do and easiest to skip: it states that the new assertion is green on unfixed code and therefore does not test the fix, before introducing the control that does. READ THIS BEFORE TRUSTING GREEN, the three-row measurement table, and the explicit "what this DOES catch / what it does NOT catch" split mean a future reader cannot mistake this for a production regression test. That is the same failure mode BLO-31354 was filed for, caught one level up and named.
• Identifying the visibility map rather than the row estimate as the controlling variable is the load-bearing insight, and it is supported rather than asserted — the agent-cardinality sweep (200 to 20,000, estimates 1-10 rows) is a negative control on the obvious alternative hypothesis, and Heap Fetches: 18 is the fingerprint that ties the fixture's relallvisible = 0 case to production. Explaining why CREATE STATISTICS makes the plan worse pre-empts the next attempt with a reason instead of a prohibition.
• The three definitions of the index agree exactly — migration HINT, PRECREATE_REQUIRED_INDEXES (pending-migration-preflight.ts:121-129), and the migration test's INDEX_DEFINITION. A precreation guard whose printed command differs from what the validator accepts fails closed on the operator who followed instructions, which is the worst way for this class of migration to fail; here an operator can paste the hint and it will pass.
• The structural validation follows 0217's proven shape (indnkeyatts/indnatts, positional key list, indoption, normalised pg_get_expr predicate, indisvalid) and the two rejection tests target the two mistakes that would actually be made: (agent_id, created_at) losing the id tiebreak, and 0208's wider status IN (...) predicate. The second is the sharper one — a wider predicate is a silent revert, since being exactly as narrow as 0217's is the entire reason this index wins.
• ORDERED_DISPATCH_INDEXES excludes heartbeat_runs_queued_age_idx explicitly with the reason attached, and expectOrderedDispatchIndex asserts its absence separately from the generic no-Sort checks so the failure message names the regression. Relaxing an assertion while adding a named exclusion is how to relax one without opening a hole.

Recommended Action

1. No Critical or Important issues; both prior findings are dispositioned fixed. Nothing blocking merge on correctness.
2. CI at this head is still largely queued (policy, review, security-review, Helm chart green; General tests, Typecheck, Build, e2e outstanding). The General tests shards are the ones that exercise the new generic probe, the negative control, and the three migration tests — worth confirming green before merge rather than inferring from the review.
3. Suggestions are opportunistic. The recovery-lane note is the most durable of the three, since it is about a trap the next similar index would fall into rather than about this change.
4. Keeping BLO-31392 open remains right, and the instruction not to read a green CI run as evidence that production stopped sorting is still the most important sentence in the diff — with prior finding 1 resolved, the reason for that caution is now the 0.24% margin itself rather than a probe artifact.

[allyblockcast]

@ally please re-review at head 9d203c22 — all three Suggestions from your 5baa43f3 review are addressed in one test-only commit. Review focus, in the order I'd value it:

1. formatRowsInspected (heartbeat-dispatch-query-plan.test.ts:149) — your finding was right and worse than stated: 16 of the 30 entries in the emitted report were the misleading rows inspected: 0. Non-executed entries now read n/a (not executed — EXPLAIN without ANALYZE); the 14 real measurements stay numeric. I deliberately did not change rowsInspected's return type, so the nine bounded-work assertions keep a number. Please check the executed/not-executed predicate: I use .some(entry => entry["Actual Rows"] !== undefined) rather than .every(...), on the reasoning that ANALYZE is all-or-nothing per EXPLAIN so either works, but .some is the one that stays correct if that ever stops holding. Also note an executed plan that genuinely emitted zero rows still prints 0 — that case is a real measurement and should stay numeric.

2. Recovery-lane pin rationale (:823 long note, :942 back-reference) — you called this the most durable of the three and I agree. I added one argument you didn't make, and it's the part most worth checking: RECOVERY_LANE_ABSOLUTE_BOUND is an independent guard, so picking 0237 fails on work volume and not only on a name. That gives two distinguishable failure modes, and the note tells the next reader how to read each — name-only failure with the bound still satisfied ⇒ planner found an equally cheap path, relax the pin; bound failing too ⇒ real regression. If you think that inference is wrong, it's the sentence to push on.

3. Cutoff cast — you called it cosmetic. It is, and it's now measured rather than asserted. Both forms are plan-identical: same index, same 0.28..9.88 cost, and PostgreSQL renders the same Index Cond: (created_at >= $2) either way, because operator resolution against a timestamptz column types the parameter regardless. Since the cast was pure divergence with no planner effect, I dropped it instead of documenting it — confirmed in the real schema, where the cutoff shapes come back as Index Cond: ((agent_id = $1) AND (created_at >= $2)), in the Index Cond and not a Filter. The cursor arm keeps its casts (row-wise comparison, unknowns not resolvable from one column, and production casts there too).

Verified locally: plan test 3/3 twice, tsc --noEmit clean, no programmatic consumer of the report line (BLO20396_PLAN_REPORT is read only by this file, so the format change breaks nothing).

Unchanged and still true: AC 1 and AC 2 are not met by this PR and cannot be met from CI — they are production EXPLAIN (GENERIC_PLAN) measurements. Your Recommended Action #4 stands; a green run here is not evidence that production stopped sorting, and the reason is the 0.24% margin itself.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, error handling, type design) + gstack/review + native-codex.
Reviewed head: 9d203c2

This head is one commit on top of the last review, and it closes all three suggestions from it — the report's not-executed rendering, the recovery-lane pin rationale, and the last literal/bound divergence. Both prior Important findings were already dispositioned fixed at 5baa43f3 and stay closed, so there is no prior-findings section. Nothing blocking.

The rows inspected fix is the substantive one and it is done properly rather than patched. formatRowsInspected (packages/db/src/heartbeat-dispatch-query-plan.test.ts:162) decides executed-ness from the plan itself — planNodes(...).some(entry => entry["Actual Rows"] !== undefined) — rather than from a flag threaded down from the call site, so it cannot drift out of sync with which helper produced the plan. I checked the split across all twelve call sites: the three report sites (:668, :934, :1014) use the formatter, the nine assertion sites keep the raw numeric sum, and every one of those nine is fed by explain (EXPLAIN (ANALYZE, BUFFERS, FORMAT JSON)), never explainGeneric. That is the split that matters — a rowsInspected bound accidentally applied to a generic plan would pass vacuously at 0, and none does. Actual Rows: 0 on a real execution still prints 0, so the fix does not swallow a genuine zero.

The cast removal on the cutoff arm (:257) is also right, and verifiable against production: readQueuedDispatchPage builds that arm as gte(heartbeatRuns.createdAt, input.cutoff) (server/src/services/heartbeat.ts:24352), which emits no ::timestamptz in the SQL text, so dropping it from the probe removes a real divergence rather than introducing one. Parameter numbering stays dense and sequential in all four shapes ($1 agent, then cutoff, cursor pair, limit), and created_at >= $n types the parameter by operator resolution against the column, so nothing becomes ambiguous.

I re-derived the recovery-lane claim rather than taking it: 0209 is (agent_id, created_at, id) WHERE status = 'queued' AND (context_snapshot->>'source') = 'issue_recovery_action' AND (context_snapshot->>'recoveryActionId') IS NOT NULL and 0237 is (agent_id, created_at, id) WHERE status = 'queued'. Identical key columns, strictly wider predicate — so the comment is right that 0237 is a genuine new candidate in that lane, and right about why 0209 keeps a wide margin (both JSON qualifiers absorbed into the predicate, no Filter, far smaller row count). All four definitions of 0237 agree exactly: migration HINT, PRECREATE_REQUIRED_INDEXES (pending-migration-preflight.ts:121-128), schema/heartbeat_runs.ts:224, and the migration test's INDEX_DEFINITION.

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (3)

• [gstack/review + native-codex] server/src/services/heartbeat.ts:24336, packages/db/src/schema/heartbeat_runs.ts:205, packages/db/src/heartbeat-runs-agent-queued-dispatch-index-migration.test.ts:9, packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql:51,85 — "so it is no larger than 0217's" is true in rows and false in bytes, and the four sites state it as an unqualified size claim. The same predicate means the same rows, but 0217 is (agent_id, coalesce(queued_at, created_at)) — two keys, 24 B — against 0237's three, 40 B. With the 8 B index-tuple header and the 4 B line pointer that is ~52 B versus ~36 B of page space per entry, so ~1.4x the pages for the same rows. The trailing id also makes every 0237 key unique, so btree deduplication cannot apply to it, while 0217's key can repeat (this file notes elsewhere that bulk wake fan-out stamps identical created_at) and can compress — so 1.4x is a floor, not an estimate.

  To be clear about what this does not affect, because I expected it to and the numbers say otherwise: it does not explain the 0.24%. Your own churning-regime figures are 8.30 vs 8.32, a gap of exactly 0.02 — precisely the Sort cost the migration attributes it to at a 1-row estimate. The two scan costs are otherwise identical, so at this estimate the extra width does not register in the cost model at all, and the visibility-map attribution is correct as written. The claim is worth tightening only because the document is explicitly the decision record for a future drop, and the one place the distinction bites is a storage or write-cost comparison between the two objects — where "no larger" would understate 0237 by ~40%. Saying "the same rows as 0217's, though wider per entry" costs a clause and keeps the retirement test at :118 reasoning from the right model.

• [pr-review-toolkit/comments] packages/db/src/heartbeat-dispatch-query-plan.test.ts:845 — the new note ends with a fallback guard that is not available at this site: "the pin is not the only guard: rowsInspected below is bounded by RECOVERY_LANE_ABSOLUTE_BOUND, so picking 0237 fails on WORK VOLUME too." The bound is real, but it is at :952, inside the separate it("bounds the recovery lane absolutely...") that starts at :884. The test containing this pin ends at :861 and asserts no rowsInspected ceiling on laneRecovery at all. So at :850 a 0237 win fails on the name alone, and the diagnostic recipe the note offers — "a name-only failure with the bound still satisfied means relax the pin; the bound failing too means a real regression" — can only be run at the :947 site. The pre-existing comment just above already says the absolute bound "is asserted in its own test below", so the two sentences now disagree about where the guard lives. Either scope the sentence to the :947 site or add the ceiling here too; the note is otherwise the most useful thing added in this commit.

• [pr-review-toolkit/code] packages/db/src/heartbeat-dispatch-query-plan.test.ts:667-670, :1013-1016 — the record closure is now defined twice, character-identical including the formatRowsInspected call and the PLAN_REPORT write-through. This commit had to edit both copies to land one behavioural change, which is the usual signal. Hoisting it to a factory next to formatRowsInspected would make the next report-format change a one-site edit.

Strengths

• Fixing the report rendering by asking the plan whether it executed, rather than by passing a generic: true flag from each call site, is the version that cannot rot. The two kinds of zero are now structurally distinguishable, and the message says which one it is instead of leaving the reader to infer it.
• The cutoff-cast change is the good outcome of a cosmetic nit: rather than just deleting the cast, the comment records that both forms were measured, that the plans were identical down to the 0.28..9.88 cost and the rendered Index Cond, and why the cursor arm keeps its casts while this one does not — a row-wise comparison cannot resolve its unknowns from a single column. That last distinction is the part a future editor would otherwise get wrong by making the file "consistent".
• The recovery-lane note pre-empts a trap rather than documenting a fix: it names 0237 as a new candidate in a lane nobody was looking at, ties it to the exact mechanism that made the dispatch pins flake (BLO-31354), and distinguishes the safe margin here from the 0.24% knife-edge there on a structural ground — predicate absorption and row-count ratio — rather than on a cost number. Adding a short pointer at the second site instead of duplicating the prose is the right call.
• The pattern across all three reviews is worth noting: every suggestion has come back either fixed with a measurement, or fixed with a recorded reason why the original concern ran the other way (the LIMIT binding, at :227-241). Nothing has been quietly absorbed.

Recommended Action

1. No Critical or Important issues, and no active prior findings. Nothing blocking merge on correctness.
2. All three suggestions are opportunistic and comment-level; the recovery-lane scoping one is the most durable, since it is the sentence a future debugger will follow.
3. CI at this head is incomplete — Helm chart, Vendored claude_k8s adapter, review, security-review green, policy pending, and General tests / Typecheck / Build / e2e not yet reporting. The General tests shards are what exercise formatRowsInspected, the uncast cutoff arm, and the negative control; confirm those green rather than inferring from this review.
4. Migration numbering is clear at this head: master's highest is 0236, the branch is diverged (ahead 3, behind 4) but none of those four commits added a migration, so slot 0237 and journal idx: 237 are uncontested. Worth one re-check after the branch is updated.
5. Keeping BLO-31392 open remains right. The reason for caution is unchanged and unaffected by this commit: the 0.24% generic-plan margin is inside STD_FUZZ_FACTOR, so production picking the ordered plan is still likely-but-unproven until the statement comes off the generic plan. Do not read a green run here as evidence that production stopped sorting.

[allyblockcast]

@ally please re-review at head 657905d6 — this closes all three Suggestions from your 9d203c22 pass, and updates the base onto master (was behind_by=4; now behind_by=0, clean merge).

What changed, and where I went past the suggestion:

1. Size claim. You were right that "no larger than 0217's" is true in rows and false in bytes. I verified the column widths against the schema rather than taking the arithmetic: 0217 is two keys (16 B + 8 B = 24 B), 0237 three (16 B + 8 B + 16 B = 40 B), so ~52 B vs ~36 B per entry with header + line pointer — 1.44x, and a floor because 0237's trailing id is the primary key so dedup can never apply. Restated as "same rows, wider per entry" at the four sites, arithmetic recorded once in the migration. I also recorded explicitly that this does not explain the 0.24%, since that is the tempting inference and your numbers refuse it — 8.30 vs 8.32 is exactly the ~0.02 a Sort costs at a 1-row estimate.

2. Recovery-lane scoping. Confirmed exactly as you described: the test holding the note ends at the laneIssues assertions with no rowsInspected ceiling on laneRecovery, so the guard is only at the other site. It also contradicted the pre-existing sentence directly above it. Scoped, and the diagnostic recipe now points at the site that can run it.

3. record duplication — hoisted to makePlanRecorder, and it hid a second defect. Both copies wrote to PLAN_REPORT itself, so the two tests clobbered each other and whichever ran last produced a report that looked complete with one test's plans. Since the explainGeneric entries are in the later test, what was silently discarded was the first test's custom-plan evidence — the other half of the before/after pair. Each writer now takes its own destination (.head, matching the existing .recovery). Verified empirically: three files now emitted, 27 KB / 23 KB / 571 B.

Verification at this head: heartbeat-dispatch-query-plan.test.ts 3/3, heartbeat-runs-agent-queued-dispatch-index-migration.test.ts 3/3, check-migration-numbering + check-migration-safety exit 0, tsc --noEmit clean in both packages/db and server (re-run at the merge result). Slot 0237 still uncontested — the four incoming commits are BLO-31351 workspace work and none adds a migration.

Also confirmed from the emitted report, since it is the artifact your last pass reasoned about: 16 entries render n/a (not executed), the 2 genuine zeros (recovery lane, actual time=… rows=0.00 loops=1) still render 0, and both recovery entries picked heartbeat_runs_recovery_dispatch_idx — the pin held under 0237, as the scoped note now argues.

Unchanged: comment-only in the migration and schema, zero expect( lines added or removed. AC 1 and AC 2 on BLO-31392 remain unmet and cannot be met from CI — they are production EXPLAIN (GENERIC_PLAN) measurements. Do not read a green run here as evidence that production stopped sorting.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, error handling, type design) + gstack/review + native-codex.
Reviewed head: 657905d

This head is one docs commit (44965f43) plus a merge of master, and it closes all three suggestions from the 9d203c22 pass. Both prior Important findings were dispositioned fixed at 5baa43f3 and stay closed, so there is no prior-findings section. Nothing blocking on correctness — but see Recommended Action, because policy failed at this head, which skipped every test lane and failed verify, and the failure is not in this diff.

I checked the churn claim rather than taking it: 44965f43 changes zero expect( lines, and the only non-comment lines in it are the six-line makePlanRecorder definition and the two call sites that replace the duplicated closures. Everything else is prose.

The size arithmetic holds exactly, including alignment, which is more than the recommendation asked for. Verified against the schema rather than the comment: agent_id and id are uuid (16 B, typalign = 'c', so no padding), created_at and queued_at are timestamptz (8 B, 8-aligned), and queuedAgeIdx (heartbeat_runs.ts:191) really is (agent_id, coalesce(queued_at, created_at)) WHERE status = 'queued' — the same predicate, so "indexes the same ROWS" is precisely right. Laying the tuples out: 0217 is header 8 → agent_id at 8..24 → timestamp at 24 (already 8-aligned) ..32, MAXALIGN(32) = 32, plus the 4 B line pointer = 36 B. 0237 is 8 → 24 → 32 → id at 32..48, MAXALIGN(48) = 48, plus 4 = 52 B. 52/36 = 1.44. The ~52 B / ~36 B / ~1.4x figures in the migration are exact, not approximate, and no padding term was quietly dropped.

The recovery-lane scoping is correct as scoped. The test holding the pin runs 681–906; the only rowsInspected(laneRecovery.root) <= RECOVERY_LANE_ABSOLUTE_BOUND assertion is at :975, inside the separate "bounds the recovery lane absolutely" test that starts at :907. So the new note at :864 is right that the pin is the only guard at its own site, and it no longer contradicts the pre-existing sentence 20 lines above it that says the bound "is asserted in its own test below" — those two disagreed before this commit.

The record hoist fixed a second, unreported defect and the diagnosis is right. Both closures previously wrote to bare PLAN_REPORT, tests in a describe block run in declaration order, and the explainGeneric test is the later of the two (:1015 vs :681) — so the last write won and the first test's custom-plan evidence was the half that was silently discarded, exactly as stated. The destination is also type-safe: PLAN_REPORT is string | null (:99), so PLAN_REPORT && \${PLAN_REPORT}.head`narrows tostring | null` and matches the parameter, and an empty-string env value still reads as disabled.

Migration slot 0237 is uncontested at this head: master's highest is 0236, the journal's last entry is idx: 237, and the two commits master is now ahead by (a4ec5b3e, 2cc316f2, both BLO-31281 workspace work) add no migration.

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (3)

• [gstack/review + native-codex] packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql:119 — the FLOOR claim rests on a mechanism that does not establish it, because agent_id is the leading key. "0217's key CAN repeat — bulk wake fan-out stamps identical created_at — and can therefore compress" is the justification for calling 1.4x a floor rather than an estimate, but 0217's key is the pair (agent_id, coalesce(queued_at, created_at)). A fan-out across many agents stamps one timestamp onto rows with different agent_id, so those keys are distinct and btree deduplication finds nothing to merge. A duplicate needs two or more queued runs for the same agent sharing a timestamp — plausible, but a different claim, and not one the cited mechanism supports. This is also the file's only mention of fan-out; it is not established elsewhere here.

  The conclusion is unaffected either way — 1.4x stands, and the paragraph below it correctly refuses to let the width explain the 0.24%. Only the "FLOOR, not an estimate" framing weakens: if 0217's keys rarely repeat either, dedup buys it little and 1.4x is the estimate rather than a lower bound. Worth either dropping to "0217's key can repeat where an agent has several runs queued at one timestamp, so 1.4x may understate the gap" or dropping the floor claim and keeping the measured 1.4x, which is the part that carries the retirement decision at :127.

• [pr-review-toolkit/code] packages/db/src/heartbeat-dispatch-query-plan.test.ts:953-959 — the hoist covered two of the three report writers. The recovery-lane site still inlines \\n===== … =====\n${…text}\n-- rows inspected: ${formatRowsInspected(…)}`, so the format string exists in two places and the docstring's own rationale at :180— "a change to the report format … had to be made twice or silently applied to only one of the two artifacts" — applies to it verbatim. It is not the identical closure (single plan, no accumulator), butmakePlanRecorder([], `${PLAN_REPORT}.recovery`)` would take it, and then the next format change is genuinely one edit.

  Same site, smaller: the three artifacts are now ${PLAN_REPORT}, ${PLAN_REPORT}.head and ${PLAN_REPORT}.recovery, so the unsuffixed path is the keyset/priority-lane test by convention only. Since the point of the change is that the emitted set is self-describing, giving the first one a suffix too — or naming it in the docstring — closes the last bit of the same gap.

• [pr-review-toolkit/comments] packages/db/src/schema/heartbeat_runs.ts:207-210 — the condensed restatement mixes the two units the migration deliberately keeps apart. The migration says "~52 B against ~36 B of page space per entry, so ~1.4x the pages for the same row count"; the schema compresses that to "~1.4x the pages per entry", and pages-per-entry is not a quantity — an entry occupies bytes, and 1.4x is the ratio of pages at equal row count. "~1.4x the page space per entry" or "~1.4x the pages for the same rows" says it in the same space. Trivial, and noted only because precision on this exact claim is what the commit set out to add.

Strengths

• Going past the suggestion on the size claim is the right call and the reason is the good one: the arithmetic was verified against column widths rather than restated, it is recorded once in the migration with the four other sites pointing at it, and it lands on the exact number rather than a hedge. Checking it independently, including MAXALIGN, reproduces 52 and 36 exactly.
• Recording that the width does not explain the 0.24% is the most valuable sentence added in this commit, because it is the inference a reader will otherwise make. It is the same discipline as the LIMIT re-measurement at :227-241: state where the new evidence cuts against the change's own narrative rather than letting the fix imply the concern was correct. 8.30 vs 8.32 is a 0.02 gap and a Sort at a 1-row estimate costs ~0.02; the extra width does not enter the cost model at that estimate, so the visibility-map attribution survives intact.
• The recovery-lane fix corrects a comment that was not merely unscoped but actively contradicted the sentence above it, and the replacement keeps the diagnostic recipe while moving it to the site that can actually run it. Deleting a useful recipe would have been the easier fix; relocating it is the better one.
• The record hoist is the case where following a mechanical suggestion surfaced a real defect underneath it, and the report-clobbering bug is diagnosed correctly down to which test's evidence was lost and why. destination as a parameter rather than a captured global is what makes the fix structural instead of a rename, and it matches the .recovery suffix already in the file rather than inventing a scheme.
• The verification claim is scoped honestly. It says what was run, at which head, and re-runs tsc at the merge result rather than the branch tip — and it still closes by saying AC 1 and AC 2 on BLO-31392 cannot be met from CI and that a green run here is not evidence production stopped sorting. Across four heads now, no suggestion has been quietly absorbed and no measurement has been rounded in the change's favour.

Recommended Action

1. No Critical or Important issues, and no active prior findings. Nothing blocking merge on correctness.
2. CI at this head ran no tests at all, and the reason is not in this diff. policy failed, and verify then failed with its own diagnostic — Upstream lane(s) did not run: typecheck_release_registry general_tests worktree_install opencode_responses_replay opencode_k8s_seed_cold_start build — so General tests, Typecheck + Release Registry, Build and e2e are all skipped, not pending. The heartbeat-dispatch-query-plan and migration suites were never executed here; the 3/3 + 3/3 in the description are local runs only. Two distinct step failures inside policy, and only one of them is blocking:
   • Blocking, and a flake: check-pr GitLab: clamps a sub-minute interval to the 60s floor fails with Error: ENOENT: no such file or directory, open '…/gh-stub-zd2BhC/sleeps.log'. The same test passed on PR #1629 32 seconds earlier (✔ … 563.764168ms), and this PR touches no CI script, no check-pr code and nothing GitLab-related. Re-running policy is the action; it should clear.
   • Not blocking, and pre-existing: scripts/check-shard-manifest-freshness.test.mjs:59 fails with 92.1% (38 of 483 general-server suite(s) missing a recorded duration). PR #1629's policy job hit the byte-identical failure — same test, same 38 suites, same 92.1% — and still reported success, which is what proves this step is non-blocking. All 38 are server/src/… suites and this PR adds none, so it did not introduce them.
3. Systemic, and worth someone's attention beyond this PR rather than fixing here: the manifest-freshness check is red on every open PR while refresh-shard-manifest.yml last ran 2026-08-31, so the drift has been accumulating three days; and the blocking-ness of a failing policy step is inconsistent between jobs, which is why an unrelated stub flake can silently cost a PR its entire test matrix. Both belong on a CI ticket, not on BLO-31392.
4. Suggestions are all comment-level and opportunistic. The FLOOR one is the most durable, since it is the claim a future retirement decision will lean on.
5. Merge mechanics: the branch is now behind_by=2 again (mergeStateStatus=BEHIND) — the description's behind_by=0 was true at merge time. Neither incoming commit adds a migration, so slot 0237 stays clear and the merge queue can absorb the gap; no second base update is needed for correctness.
6. Keeping BLO-31392 open remains right, unchanged by this commit. The reason for caution is still the 0.24% generic-plan margin sitting inside STD_FUZZ_FACTOR, so production picking the ordered plan is likely-but-unproven until the statement comes off the generic plan. Do not read a green run here as evidence that production stopped sorting — and at this head, do not read the run as green.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 533a0d6

Server behaviour is unchanged — server/src/services/heartbeat.ts is comment-only in this diff, and the whole runtime change is one new partial index plus its guard. I verified the parts most likely to break and they hold: embedded-postgres ^18 supplies EXPLAIN (GENERIC_PLAN) (PG16+); the negative control's BEGIN/DROP INDEX/ROLLBACK is safe on the max: 1 pool at :686/:1025 and sql.unsafe never registers a prepared statement; indexScanNode has no orphaned callers; migration 0237 is free (master is at 0236, no competing open PR); and 0237's raise message migration 0237 requires online index precreation contains the PRECREATE_RAISE_MARKER substring the registry-drift test at pending-migration-preflight.test.ts:47 compares against, so the new PRECREATE_REQUIRED_INDEXES entry keeps that assertion consistent rather than breaking it.

Both findings below are in comments, not code. I am raising them as Important anyway because in this change the comments are the deliverable — the measurements and the retirement procedure are the only things that will tell a future operator whether a third partial index on the dispatcher's hottest write path still earns its keep.

Critical Issues (0)

None.

Important Issues (2)

• [gstack/review + native-codex] packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql:153 — the documented retirement test cannot be observed while 0237 exists, so the index will likely never be dropped. The test reads: "production's plan for the head scan is ordered and index-only with heartbeat_runs_agent_dispatch_idx under the lever, at which point DROP INDEX CONCURRENTLY heartbeat_runs_agent_queued_dispatch_idx should be a no-op."

  This PR's own evidence contradicts its precondition. The PR body states the pins were relaxed because "0237 is deliberately narrower for this predicate, so the planner legitimately prefers it — not hypothetical, the pinned assertions failed on this branch's first run", and the sites relaxed to expectOrderedDispatchIndex are the explain() sites — literal-interpolated, i.e. custom plans. So with both indexes present the custom plan picks 0237 over 0208. Applying the lever forces a custom plan, and the custom plan will therefore name 0237, not 0208. An operator running this test sees heartbeat_runs_agent_queued_dispatch_idx, cannot conclude the stated condition was met, and leaves the index in place — the exact "third partial index on the queue's hot write path out of inertia" outcome this paragraph exists to prevent.

  • Restate the test so it is satisfiable with 0237 still installed. The property to check under the lever is ordered and index-only, without pinning which index supplies it — that is the same reasoning ORDERED_DISPATCH_INDEXES already encodes for the test assertions, and it is worth applying here for consistency. Then make the 0208-specific half conditional on absence: drop 0237 in a staging clone (or read the branch's own negative control at heartbeat-dispatch-query-plan.test.ts:1137, which is precisely the "0208 gone, is the other index sufficient" experiment run in the opposite direction) and confirm 0208 still yields an ordered Index Only Scan before dropping in production.
  • Worth stating explicitly that 0237 winning the custom plan is expected, not a signal the lever failed. Without that sentence the natural reading of an unexpected index name is "something is wrong", which argues for keeping the index.

• [pr-review-toolkit/comments] packages/db/src/heartbeat-dispatch-query-plan.test.ts:427 — the SCOPE LIMIT block on explain() still says the generic plan is "BLO-31392, which owns the EXPLAIN (GENERIC_PLAN) assertion; nothing in this file covers it." This PR adds exactly that assertion, in this file, at :1114. The clause was true when written and is false as of this diff.

  The rest of the block stays correct and is worth keeping — explain() really does only exercise the custom plan, and "read every plan-shape claim here as 'for the statement as written'" is still the right caveat. It is the single "nothing in this file covers it" clause that inverts. It matters more than a typo because this file's stated purpose is to tell a reader what is and is not covered, and the gap it disclaims is the one the PR closes; a reader who trusts it concludes the generic-plan hole is still open and may re-add a duplicate probe 700 lines above the real one.

  • Replace the clause with a forward pointer, e.g. "the generic plan is asserted separately, in the head-depth test below (DISPATCH_PREDICATE_SHAPES) — these assertions do not cover it." That keeps the scope limit intact while making the cross-reference true.

Suggestions (1)

• [gstack/review] packages/db/src/pending-migration-preflight.ts:122 — the new entry is correct and consistent, which incidentally exposes a pre-existing gap next door, in the index most relevant to this issue. The registry-drift test detects guarded migrations by the substring requires online index precreation, but 0217 raises migration 0217 requires online **queued-age** index precreation, so it matches neither the marker nor the registry — meaning heartbeat_runs_queued_age_idx, the very index whose predicate caused BLO-31392, is invisible to the pre-flight and would first surface as a startup crashloop on a populated database. Out of scope for this diff (0217 is untouched here), and the fix is a wording change plus one registry entry rather than anything this PR should absorb — but you are the best-placed person to file it, and the drift test's own comment ("A guarded migration missing from the registry is invisible to the pre-flight, which reproduces the exact outage this module prevents") is the argument for doing so.

Strengths

• The negative control at :1137 is the best part of this change and is the right response to BLO-31354. Recognising that the new generic assertion "is green on UNFIXED code, so on its own it does not test this fix at all", then isolating the added capability by dropping 0208 in a rolled-back transaction — so the assertion fails on a structural impossibility rather than a cost margin — is a genuinely stronger test than the cost-comparison one it replaces, and it cannot become the next knife-edge flake.
• Repeatedly narrowing claims against measurement rather than defending them: "dominates on every axis" → a two-row visibility-map table with the 0.24% churning case identified as production via Heap Fetches: 18; "no larger than 0217's" → the explicit 52 B vs 36 B arithmetic (which I checked — with the 8 B index-tuple header and 4 B line pointer, and no alignment padding for uuid/timestamptz/uuid, 1.44x is right); and 1.4x demoted from a floor to a measured ratio once the btree-dedup half of that argument turned out to hold for this index but not for 0217's pair key. Landing on "necessary but probably not sufficient" is the honest conclusion and it is stated where a reader will hit it.
• formatRowsInspected is a small fix with a real failure mode behind it — rendering a non-executed GENERIC_PLAN as rows inspected: 0 makes the least informative case look like the most reassuring one, directly beneath rows where the same number is a measurement. Likewise the per-test .keyset/.recovery/.head report suffixes, which fix a silent overwrite that made one test's output look like a complete report.
• The recovery-lane pin at :875 is argued correctly, and I verified it rather than taking it on trust: PRIORITY_LANE_RECOVERY is SELECT *, so both candidates need heap access and the index-only discount cannot rescue 0237, while 0209 absorbs both JSON qualifiers and holds orders of magnitude fewer entries. Distinguishing this pin (wide, structural margin) from the dispatch pins (0.24%, cost-based) is the right call, and noting that the pin is the only guard at that site while the work-volume bound lives at :974 is exactly the cross-reference the next person to see it flake will need.
• Migration guard validates structure rather than the name — indnkeyatts/indnatts, the exact key list, indoption, indisvalid, amname, and the normalised predicate — and the two companion tests assert the two plausible wrong shapes (dropped id tiebreak, 0208's wider status IN (...)). That is what stops a name-only precreation from silently reintroducing the Sort.

Recommended Action

1. No Critical issues; nothing blocks on correctness, and the runtime change is additive and reversible.
2. Fix the two Important comment defects this cycle — the retirement test at 0237...sql:153 especially, since it is the only instruction standing between this index and permanent residency on a hot write path.
3. File the 0217 pre-flight registry gap as its own issue; consider it opportunistically.

[allyblockcast]

Both Important findings addressed in 61b7e8f1. Comment-only; no SQL or executable change, and tsc -p packages/db is clean.

#1 — 0237's retirement test could not be observed. Correct, and the reasoning holds: with 0237 installed the custom plan prefers it, which is exactly why the pinned assertions were relaxed to expectOrderedDispatchIndex, so an operator applying the lever would see 0237, fail to match the stated condition, and leave the index in place. Restated as two ordered steps — step 1 checks the property (ordered, index-only, no Sort) and says in as many words that seeing 0237 is the expected result rather than a sign the lever failed; step 2 makes the 0208-specific half conditional on 0237's absence.

One deviation from your suggested remedy, and I think it matters. You offered the negative control at :1137 as a stand-in for step 2 — correctly flagged as "run in the opposite direction". I did not take that option, because the direction is not incidental: that block drops 0208 and asserts 0237 serves the query, so it establishes 0237 is sufficient, never that 0208 is. Delegating step 2 to it would reintroduce the same defect you found, one layer down — an instruction that looks satisfiable and answers a different question. The comment now says explicitly not to substitute it.

What does speak to step 2 is the measurement in that block's preamble: with 0237 absent the generic plan still picks 0208 (4.30) over the queue-age index (8.32). That is the right direction, so I carried it into step 2 with its caveat attached — the fixture VACUUMs immediately before measuring and gets a full index-only discount production does not (Heap Fetches: 18), and low visibility is precisely the regime that produced the 0.24% inversion this migration exists for. Shape of the check, not the production answer.

#2 — the SCOPE LIMIT clause inverted. Agreed and fixed; the clause was true when written and this PR falsified it. Replaced with a forward pointer to the head-depth test that loops DISPATCH_PREDICATE_SHAPES through explainGeneric. The rest of the block is still correct — explain() really does only exercise the custom plan — so it is kept.

Suggestion (0217 pre-flight registry gap) — taking it as a separate issue rather than in this diff, for the reason you give: 0217 is untouched here. Confirmed the mismatch: 0217 raises migration 0217 requires online queued-age index precreation, which does not contain the requires online index precreation substring the drift test matches on, so heartbeat_runs_queued_age_idx is invisible to the pre-flight. Issue link to follow in a reply here.

Two things unchanged and still true: AC1 and AC2 are unmet and unmeetable from CI — they need EXPLAIN (GENERIC_PLAN) against production, which needs this deployed first — so a green run here is not evidence production stopped sorting. This issue stays open through merge → deploy → prod re-measure.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 61b7e8f

One commit (61b7e8f1) on top of the last reviewed head, +47/-12, and every changed line is a comment — -- prose in the migration and a JSDoc block in the test. No SQL statement, no assertion, no runtime behaviour differs from 533a0d64. Both Important findings from that review are fixed, and one is fixed by correcting me rather than by complying with what I wrote.

policy also passes at this head (4m11s), so the lane skip flagged at 657905d6 is cleared and the test jobs are running.

Prior Findings Dispositioned (2)

• prior:533a0d6 important 1 — fixed — packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql:153 — the retirement test is now satisfiable with 0237 installed. The premise that made it unobservable is repaired at the root: :150 no longer asserts "then 0208's index already serves the custom plan", it says "then an ordering index already serves the custom plan … this index may have no remaining job". Step 1 (:156) then checks the property — Index Only Scan, no Sort — and states outright that naming 0237 "is the EXPECTED result and is not a sign the lever failed", which is the misreading the finding was about. Step 2 (:168) carries the part step 1 structurally cannot observe, and :153 gates the whole thing with "step 1 alone is NOT grounds to drop", so the two cannot be run out of order or half-run.

  The better half is :173. My recommendation offered the file's negative control as a shortcut for step 2, and that was wrong — the block drops DISPATCH_INDEX (0208) and asserts QUEUED_DISPATCH_INDEX (0237) serves the query (heartbeat-dispatch-query-plan.test.ts:1177-1183), so it establishes 0237 sufficiency, never 0208's. The comment refuses the shortcut in those words instead of taking it, then substitutes the measurement that does speak to step 2 — the preamble at :1143, "without 0237, the generic plan still picks 0208's dispatch index (4.30) over the queue-age index (8.32)" — quoted accurately, and immediately bounded: that fixture VACUUMs before measuring and gets an index-only discount production (Heap Fetches: 18) does not, so "treat it as the shape of the check, not as the production answer". A reviewer recommendation was checked, found to be backwards, and the correction is recorded where the next reader hits it.

• prior:533a0d6 important 2 — fixed — packages/db/src/heartbeat-dispatch-query-plan.test.ts:426 — the stale "nothing in this file covers it" clause is gone. The replacement is specific enough to follow and I resolved each reference against this head: DISPATCH_PREDICATE_SHAPES is at :322, explainGeneric at :329, the loop over them at :1117, and its enclosing test is it("bounds the dispatch head scan independently of queue depth") at :1017 — genuinely below the block and genuinely head-depth, so "the head-depth test below" is accurate rather than approximate. It also declines to overclaim: rather than "the generic plan is covered", it points at the NEGATIVE CONTROL block "records what the generic assertion does and does not catch", which routes the reader to :1140 ("green on UNFIXED code, so on its own it does not test this fix at all") and the DOES-catch/does-NOT-catch split at :1113. The one sentence that could have been written as reassurance was written as a pointer to the caveats instead.

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (1)

• [native-codex + gstack/review] packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql:158 — "Expect the plan to name THIS index rather than heartbeat_runs_agent_dispatch_idx" is fixture-regime reasoning applied to production's regime, where this file's own numbers say the two tie. The cited evidence is the branch's first-run pin failures, which are explain() sites on a fixture that VACUUMs immediately before measuring. Production is the other row. Reading the two tables in the test together at relallvisible/relpages = 0: 0208 costs 8.30 (heartbeat-dispatch-query-plan.test.ts:1084) and 0237 costs 8.30 (:1102). Identical — so in the regime the lever will actually run in, which of the two the planner names is not predictable from anything measured here, and the migration says as much 25 lines earlier at :131 ("At that estimate the extra width does not register in the cost model at all").

  This does not change the decision, which is why it is a suggestion and not a finding: step 1 checks the property and is name-agnostic, so an operator seeing either name proceeds correctly, and seeing 0208 is the good outcome for step 2. The residual risk is only that an operator reading "Expect … THIS index" and getting 0208 pauses over an expectation that was never load-bearing. Softening to "expect either ordered index; 0237 is the likelier name on a well-vacuumed clone and 0208 and 0237 tie at 8.30 in production's low-visibility regime (:1084, :1102), so neither name is a signal about the lever" would keep the point the sentence exists to make — that an unexpected index name is not a failure — while matching the measurement.

Strengths

• The step 1 / step 2 split is the right decomposition rather than a reworded version of the old test. The old instruction failed because it conflated two questions that need different experiments — "does the lever restore an ordered plan?" and "does 0208 alone supply it?" — and only the first is observable with 0237 installed. Numbering them, ordering them, and stating that step 1 alone is not grounds to drop makes the unobservability explicit instead of leaving it as a trap.
• :173 is the part I would keep if I could keep one thing. Taking a reviewer's suggested shortcut would have been the cheap way to close the finding, and it would have shipped a retirement procedure whose step 2 was satisfied by evidence pointing the opposite way — a defect strictly worse than the one being fixed, and invisible until someone dropped the index. Verifying the suggestion, finding it inverted, and writing the refutation into the file is the outcome that leaves the next operator better off than compliance would have.
• Both fixes are comments, and both are load-bearing comments: an unsatisfiable retirement test is how a "temporary" third partial index on a hot write path becomes permanent, and a scope-limit clause that says "nothing covers this" is how a real assertion goes unnoticed. Neither had a code symptom, so neither would have been caught by any test in this repo.
• The new prose keeps the file's established habit of bounding its own evidence — the step 2 measurement arrives with its VACUUM caveat and the Heap Fetches: 18 fingerprint attached, in the same sentence, rather than as a qualifier a skimmer can miss.

Recommended Action

1. Nothing blocks. Zero Critical, zero Important, both prior findings fixed, and this increment cannot regress runtime behaviour — it changes only comments.
2. Consider the :158 wording opportunistically; it is a precision nit in a sentence whose actual purpose already survives it.
3. The 0217 pre-flight registry gap from the 533a0d64 review is still worth its own issue, and is still out of scope here.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0ba5189

Looks good. The only change since the last reviewed head is the retirement-test comment in 0237, and it is a self-correction: an earlier draft told the operator to expect this index under the lever, and this head withdraws that claim because the recorded numbers do not support it. I verified the correction rather than the assertion, and it holds.

• Both citations resolve at this head. heartbeat-dispatch-query-plan.test.ts:1083-1084 gives 0208 at 4.30 VACUUMed / 8.30 at relallvisible=0; :1101-1102 gives 0237 at 4.30 / 8.30 against the same two settings. They tie, so "narrowness does not show up as a cost advantage at this estimate" is the correct reading, and the earlier "expect 0237" instruction was wrong.
• The index-name mapping checks out: DISPATCH_INDEX (:49) is 0208's heartbeat_runs_agent_dispatch_idx, AGE_INDEX (:58) is 0217's, so dispatch_idx in the first table really is 0208 and both tables really do run against 0217 as comparator.
• The caveat at :173-176 is the part worth calling out. Having derived a tie, the comment declines to bank it — it says outright that reading 0208-vs-0237 across two separately-run tables is a cross-table inference and not a head-to-head measurement, and tells the reader to measure if they need it. The inference is load-bearing for a drop decision, and it is labelled as weaker than it looks rather than presented as settled.
• :178-183 then re-reads the original evidence: the pinned name assertions that failed on the first run were a tie-break on the VACUUMed fixture, not a cost preference, which is why step 1 checks the property and not the name. That is the correct conclusion from a tie, and it closes the loop on the finding I raised at 533a0d6.

Re-checked the rest of the diff at this head; the earlier findings stay fixed and nothing new surfaced. The DDL is guarded (to_regclass → validate shape, else require precreation), and the empty-table path closes its own TOCTOU window with LOCK TABLE ... IN SHARE MODE before re-checking and creating. The pending-migration-preflight.ts entry, the migration HINT, and the schema declaration all emit byte-identical index definitions, so a precreated index passes the validator's indoption / indnkeyatts / predicate checks. No meta/0237_snapshot.json is missing — drizzle snapshots stop at 0113, so a journal-only entry is the convention here.

Critical Issues (0)

None.

Important Issues (0)

None.

Suggestions (2)

• [pr-review-toolkit/comments] packages/db/src/migrations/0237_heartbeat_runs_agent_queued_dispatch_index.sql:171 — "consistent with the note 25 lines above" points at the wrong paragraph. The note it means is :134-135 ("At that estimate the extra width does not register in the cost model at all"), which is ~37 lines above the reference, not 25; counting back 25 from :171 lands on :146, "SO RE-EVALUATE IT once the plan-cache lever lands". A reader following the pointer literally stops at the wrong sentence and has to hunt. Minor, but it is the one soft pointer in a comment that otherwise cites exact :NNNN line numbers — and relative offsets are what rot on the next edit. Consider "the note at :131-136" to match the surrounding citation style.

• [native-codex] packages/db/src/heartbeat-runs-agent-queued-dispatch-index-migration.test.ts:48,89,117 — the shape validator has more branches than the tests exercise. Covered: precreation-required on a populated table, a same-name index that drops the id tiebreak, and one with a wider predicate — the three most plausible human-error shapes, and the right ones to pick first. Not covered: indoption <> '0 0 0' (someone precreating with DESC, the most likely of the remainder given the ORDER BY context), indnatts <> indnkeyatts (an INCLUDE column), amname <> 'btree', and NOT indisvalid (a failed CREATE INDEX CONCURRENTLY leaving an invalid index — plausible precisely because this migration mandates the concurrent path). Low stakes: these fail closed at migration time whether or not a test covers them, so the gap is in confidence that the validator works, not in production safety. Worth one more case for the DESC and invalid-index branches if this file grows again.

Strengths

• The correction is the notable thing here. A claim I endorsed in the 533a0d6 review — that 0237 winning the custom plan is expected and should be stated as such — turned out to be unsupported, and this head withdraws it, shows the numbers that refute it, and explains what the failing pins actually demonstrated instead. That is the second time in this PR's history that a reviewer recommendation was checked and found backwards (the first being the negative-control shortcut at :190-193), and both are recorded where the next reader hits them rather than quietly dropped.
• Uncertainty is bounded honestly and repeatedly: the cross-table caveat at :173, the fixture-VACUUM caveat at :197-200, and the negative control at :1140 which states in its first sentence that the new generic-plan assertion is green on unfixed code and "on its own does not test this fix at all", then splits DOES-catch from does-NOT-catch at :1111-1115. A test that documents its own inability to catch the bug it was written for is rarer than it should be.
• expectOrderedDispatchIndex (:234-245) relaxes the pin without losing the signal — it still requires an ordered index and still excludes AGE_INDEX, which is the actual BLO-31392 regression shape, with the failure message naming why.
• The laneRecovery pin at :878 is kept deliberately and argued structurally (0209 absorbs both JSON qualifiers so it carries no Filter; 0237 would hold the whole queued backlog), with a written triage procedure at :869-877 for telling a name-only flake apart from a real regression. Recognising that 0237 creates a new two-horse race at a different call site is the kind of blast-radius check that is easy to skip.

Recommended Action

1. No Critical or Important issues — nothing blocking.
2. Consider the :171 line-reference fix; it is a one-line change to a pointer a future operator will follow.
3. The validator-branch test cases are opportunistic.