feat(executor): prove and recover abandoned invalid indexes

[Kiran01bm]

Adds RebuildAbandonedIndex, a proven recovery path for invalid indexes left behind by a dead CREATE INDEX CONCURRENTLY, and fixes the classifier so an unobservable builder is never reported as abandoned.

Why

When a concurrent index build dies mid-flight (executor crash, connection loss, statement cancel), PostgreSQL leaves the index in the catalog marked invalid. Until now the executor refused every retry of that statement and pointed the operator at a manual DROP INDEX. That is the wrong end state for an engine that is meant to run unattended behind an orchestrator: the retry loop stalls on a leftover only pg-sprite created, and a human has to run destructive DDL by hand to unblock it.

Recovery has to be proven, not assumed. pg_stat_progress_create_index is the natural "is anyone still building this?" oracle, but it nulls relid/index_relid for another role's command unless the reader has pg_read_all_stats, and it records nothing at all under track_activities = off. A classifier that reads "no visible builder" as "no builder" would drop an index that is actively being built. The definitive proof is the lock: every concurrent index command holds SHARE UPDATE EXCLUSIVE on the table for its whole life, so acquiring that lock ourselves excludes any builder, visible or hidden. And "no builder" is still not "debris": a partitioned table's ON ONLY index is invalid by design with nobody building it, so the proof also has to establish that the entry is one a failed concurrent build can leave and the server will drop concurrently.

What

• RebuildAbandonedIndex(ctx, pool, sql, budget) — inspect and classify → refuse unless the code is recoverable → bounded transaction (lock_timeout 5s) taking LOCK TABLE … IN SHARE UPDATE EXCLUSIVE MODE as the abandonment proof → re-verify by OID (name, validity, table by OID, schema and the table name the statement gave, droppability, no visible builder) → quarantine via ALTER INDEX … RENAME TO pgsprite_abandoned_<oid> → commit → DROP INDEX CONCURRENTLY each quarantined entry on a budgeted session with a 5s per-lock bound and pre/post OID verification → buildIndexConcurrently. The proof lock, every drop, and the build share one overall budget. Returns an IndexRecoveryReport (Dropped, Skipped, whole-recovery Duration, build report). A drop stopped by the budget, by an operator's pg_cancel_backend, or by the caller's own context is reported as that cause (budget-statement-exceeded, cancelled-externally, cancelled-by-caller) — never as a verdict on the entry, which a cancelled drop leaves exactly as quarantined for the next sweep. A table renamed, dropped, or moved since the observation — or one whose inspected name no longer matches the statement's — is ErrTargetIdentityChanged; any other disagreement is ErrAbandonmentUnproven. Needs a pool one connection larger than the build and refuses a smaller one with ErrPoolTooSmall.
• Classifier reads builder visibility alongside the builder PID and whether the entry is droppable concurrently (relkind = 'i', not relispartition, no pg_constraint.conindid). Order of proof strength: visible builder → other table → not droppable → unobservable → abandoned. New codes invalid-index-builder-unobservable (recoverable — the lock proof decides) and invalid-index-not-droppable (refused: a partitioned table's index, an index partition, or a constraint's index is never debris). invalid-index-preexisting is removed. catalogVerdict reads indrelid, so another table's debris after a name collision is reported as invalid-index-other-table with that table's name, not as this build's own leftover.
• Quarantine sweep drops only entries whose name equals the quarantine name derived from their own OID, refuses on a visible builder, and skips (reports, leaves for an operator) an entry the server will not drop concurrently, so one such entry never wedges the table. A plain BuildIndexConcurrently refuses over droppable quarantine debris on the target table, so debris never goes quiet after a recovery that died between rename and drop.
• Code.Permanent() classifies each outcome code: permanent means the outcome is decided by the statement, the caller's configuration, or the standing catalog — retrying unchanged reproduces it and no executor entry point changes it. The execution-model table gains a Permanent column that a docs test pins to the method.
• Tests: unit classifier ordering and the abandonment predicate; Permanent() classified for every code in Codes(); droppableColumn checked against the server's own DROP INDEX CONCURRENTLY verdict (plain, primary-key, unique-constraint, exclusion, FK-referenced, partitioned parent, attached partition); internal tests on a real database that alter the catalog between the observation and the lock (table renamed before or after the inspection / moved schema behind a same-named decoy → ErrTargetIdentityChanged; index renamed / made valid in place → ErrAbandonmentUnproven; replaced by a concurrent reindex → nothing to do; the same for the pre-drop check) and a shared-budget arithmetic test; integration coverage for own-leftover removed and rebuilt, no debris, in-flight refusal, other-table refusal, lock budget at the proof and at the drop, quarantine sweep, sweep refusing a quarantine-named entry whose builder is visible, skipped undroppable entry, never dropping a valid index, partitioned parent refusal, plain build refusing quarantine debris, pool-size refusal, admission guards, hidden builder via a role without pg_read_all_stats, and track_activities = off; the sweep cancelled by its own caller while a drop waits on the table lock reports cancelled-by-caller with the debris still quarantined; a caller-owned build that succeeds and is cancelled the instant its statement returns still reports a verified valid index; every cancellation cause routed through the sweep's bounded-outcome check; isStatementCancellation over chains that carry another server error beside the client's context error. The blocked-build helper registers a t.Cleanup so a failed wait cannot leak a live build.
• Docs: invalid-index-recovery.md rewritten around the seven-code table with the not-droppable state, the pool requirement, the lock bounds, the shared budget, and the report fields; new invariant LK-5 (an index is dropped only by proven identity, under the lock that excludes its builder) with // INV: LK-5 tags at both re-verifications; schemabot-integration.md gains a routing table for the invalid-index outcome codes and the opt-in recovery; execution-model, capabilities, limitations, tcb-model updated. Recovery is library-only for now (no CLI verb).

Consumer note: an adapter that enumerates executor.Codes() exhaustively must map the two new codes and no longer reference invalid-index-preexisting. The replacement codes are not one retry class: two of the five (invalid-index-other-table, invalid-index-not-droppable) are permanent and belong with the refusals, the other three wait on a recovery, a builder, or a re-taken proof — Code.Permanent() says which, so a total switch over Codes() on the consumer side needs that split rather than a mechanical paste of the removed code's arm. An adapter that retries invalid-index codes as transient errors will loop, because the refusal is deterministic until RebuildAbandonedIndex runs. cancelled-by-caller is also new in Codes(): it is the caller's own context ending (a lease lapsing, a deadline expiring) and nothing about the target needs to change before a retry, so it belongs with the operational outcomes beside cancelled-externally, not with the refusals.

Before / after

Before
  CIC dies ──▶ invalid index in catalog
                    │
      retry ────────┤
                    ▼
          inspect: any invalid index on this name?
                    │
        ┌───────────┴────────────┐
        │ builder PID visible    │ no PID visible
        ▼                        ▼
   build-in-flight          "preexisting" ─▶ REFUSE
                             (manual DROP INDEX required;
                              hidden builder indistinguishable
                              from abandoned)

After
  CIC dies ──▶ invalid index in catalog (or quarantine debris on the table)
                    │
      retry ────────┤
                    ▼
          classify (strongest proof first)
                    │
    ┌───────┬───────┼───────────┬──────────────┬──────────────┐
    ▼       ▼       ▼           ▼              ▼              ▼
 in-flight other  not-       builder-      abandoned    own-leftover
 (refuse)  table  droppable  unobservable
           (refuse) (refuse)    │              │              │
                                └──────┬───────┴──────────────┘
                                       ▼
                 RebuildAbandonedIndex                 ── one shared overall budget ──
                   LOCK TABLE … SHARE UPDATE EXCLUSIVE (5s)  ◀ proof: any live concurrent
                     │ 55P03 ─▶ BudgetError(CauseLock)          index command holds it
                     │ 42P01 ─▶ ErrTargetIdentityChanged
                     ▼
                   re-verify by OID (name, invalid, table+schema+statement's table, droppable, no builder)
                     │ mismatch ─▶ ErrAbandonmentUnproven / ErrTargetIdentityChanged
                     ▼
                   RENAME TO pgsprite_abandoned_<oid> ─▶ commit
                     ▼
                   per quarantined entry: droppable? ──no──▶ Skipped (reported)
                     │ yes: re-verify by OID ─▶ DROP INDEX CONCURRENTLY (lock 5s) ─▶ OID gone?
                     ▼
                   CREATE INDEX CONCURRENTLY (remaining budget, verified)

---

Authored with Amp (Claude Opus 4.5).

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review — 8ec0ecc7 (+2380/-211, 19 files)

The load-bearing claim here is the one in the file header — that SHARE UPDATE EXCLUSIVE on the table excludes every concurrent index command for its whole life — so I checked that before anything else rather than taking it from the docs. It holds, and for a better reason than the docs give: DefineIndex promotes its heap and index locks to session-level locks (LockRelationIdForSession) before the internal commits, so they survive the multi-transaction structure; DROP/REINDEX ... CONCURRENTLY do the same. Taking that lock really does exclude all three, including a builder this role cannot see. The proof is sound.

I also chased the one step that looked like it could block inside the proof transaction and found it can't:

BEGIN; ALTER INDEX lk_v RENAME TO lk_v2;
SELECT relation::regclass, mode FROM pg_locks WHERE pid = pg_backend_pid();
 lk_v2 | ShareUpdateExclusiveLock      -- and nothing at all on the table

ALTER INDEX ... RENAME takes SUE on the index and no lock on the heap, so it never queues behind DML on an indisready invalid index — which is exactly the shape that would have turned the rename into a silent lock_timeout mapped to an untyped error. It doesn't arise.

So the findings are narrow. One is a real behavioural gap, one is where the suite stops pinning the proof, and one is about the consumer this whole file exists for.

#	Sev	Where	What
1	low-med	pkg/executor/recover.go:249	The proof locks the table under the name the inspection saw, not the one the statement gave — so a rename landing between resolveTarget and inspectInvalidIndex is followed silently, contradicting the comment four lines above
2	low	recover.go, native.go, tests	Three reachable safety predicates survive mutation; one of them has a test named for exactly its scenario that passes by a different route
3	low	docs/schemabot-integration.md + consumer	The new routing table is right, and the live consumer is on the wrong side of it — the compile break lands somewhere the obvious paste-in fix reintroduces the retry-forever case the doc warns about

---

1 — the target's identity is proven by OID, but never against the name the statement gave (low-med)

// LOCK TABLE takes a name, not an identity: a table renamed or dropped
// since the resolution is no longer the table the statement names, ...
table := pgx.Identifier{target.schema, existing.table}

The comment states the rule; the line below it doesn't implement it for one window. existing.table is the table name as of inspectInvalidIndex, and indexTarget carries only {tableOID, schema} — the statement's own table name (build.table) is never compared to anything after resolveTarget consumed it. So a rename landing in the gap between those two calls is not detected: the lock is taken on the new name, the under-lock re-check compares *tableName != existing.table (new against new, so it passes), and the OID matches because a rename doesn't change it.

Measured, on the internal fixture — resolve, rename, inspect, prove:

quarantine err = <nil>
entry name now = "pgsprite_abandoned_23710"

The rename went through on a table the statement never names. Downstream the sweep then drops that entry, and step 3's buildIndexConcurrently re-resolves and fails ErrTableNotFound — so the caller does get an error, but only after the recovery has removed an index from a table it was not pointed at.

Worth being precise about the blast radius, because it is smaller than the shape suggests: the entry removed is genuinely invalid debris on the same physical relation (same OID), so nothing valuable is destroyed, and LK-5 as written is not violated — it says "on the target table by OID and schema", and both hold. What's violated is the narrower promise in this comment, and the window is two adjacent catalog round-trips on one connection, so this is a race no operator arranges deliberately.

It's also a one-line close, which is most of why I'd take it: resolveTarget already reads pg_class and could return relname alongside the OID, or the caller can compare existing.table != build.table right after the inspection (both are in scope at recover.go:194) and fail ErrTargetIdentityChanged. Either way the guard then matches its own comment, and the Table field on the returned error starts naming a table the caller asked about.

Two adjacent things I checked that are fine and shouldn't be changed: locking existing.table rather than build.table is the right instinct for the later window — a rename between the inspection and the lock grant correctly fails 42P01 → ErrTargetIdentityChanged — and the schema half of the under-lock check is load-bearing for the moved-out-plus-decoy case (see finding 2). The fix is an addition, not a swap.

2 — where the suite stops pinning the proof (low)

Ten mutations against the safety predicates; four killed, three survived reachably (three more survived but are unreachable by construction — detail in the second comment).

Mutation	Result
isAbandonmentCandidate drops && f.droppable	killed
classifyInvalidIndex drops the unobservable-builder case	killed
quarantine list matches by prefix instead of name == prefix‖oid	killed
build stops refusing over quarantined debris	killed
droppableColumn stops excluding constraint-backed indexes	survives
sweep drops its visible-builder refusal (recover.go:374)	survives
proof drops the schema half of the table identity (recover.go:295)	survives

The last one is the one worth acting on, because a test already carries its name:

t.Run("table moved to another schema", func(t *testing.T) {
	f.exec(t, "ALTER TABLE %s.t SET SCHEMA %s", f.schema, other)
	...
	require.ErrorIs(t, err, ErrTargetIdentityChanged)

That passes without the check, because LOCK TABLE <schema>.t raises 42P01 once the table has left the schema — the 42P01 arm, not the identity arm. The scenario the check is actually for is the one its own comment describes: the table moves out and a same-named table is left behind, so the lock resolves and the OID re-read still answers "t". Adding three lines to the existing fixture reaches it, and it fails without the check:

Error: Target error should be in err chain:
  expected: "the target table was dropped or replaced during the build ..."
  in chain: "... quarantine index t_81912_1.idx_left: ERROR: relation ... does not exist (SQLSTATE 42P01)"

(Note the consequence there is a typed-outcome regression, not a wrong drop — the rename still fails loudly, just as an opaque "could not be proven" instead of ErrTargetIdentityChanged. The check earns its place by producing the right verdict.)

The other two are plainer gaps. droppableColumn has three terms and two of them have a dedicated test each — RefusesPartitionedParentIndex for relkind, SkipsQuarantinedEntryTheServerWillNotDrop for relispartition — while the pg_constraint.conindid term has none. And the sweep's visible-builder refusal at recover.go:374 is a different code path from the one RefusesVisibleInFlightBuild covers: that test exercises the entry under the requested name, this branch is the entry under a quarantine name, which is the state an operator reaches by trying to REINDEX INDEX CONCURRENTLY the pgsprite_abandoned_* entry they just found. The harness for both is already in the file.

3 — the routing table is right; the consumer is on the wrong side of it (low)

This isn't a defect in the diff — docs/schemabot-integration.md calls the hazard exactly:

The refusal is deterministic: retrying the build unchanged reproduces it, so an adapter that classifies these codes as transient operational errors retries forever.

What I'd add is where that lands, since it's concrete and the PR body frames the consumer work as "must be updated" without saying that the mechanical version of the update is wrong. In block/schemabot, on pg-sprite v0.2.0, the removal of CodeInvalidIndexPreexisting breaks compilation at pkg/engine/postgres/apply.go:271 and :448 — loudly, which is the good failure. But :448 is this:

case executor.CodeBudgetLockExceeded, executor.CodeCancelledExternally,
	executor.CodeInvalidIndexOwnLeftover, executor.CodeInvalidIndexPreexisting,
	executor.CodeInvalidIndexUnproven, executor.CodePoolTooSmall,
	executor.CodeExecutionFailed:
	// Operational outcomes: ... A retry can succeed once
	// conditions change, so none is a permanent refusal.
	return nil, true

Pasting the five successors where the one used to sit compiles, passes the exhaustiveness test at apply_test.go:232, and puts invalid-index-other-table and invalid-index-not-droppable — both Recoverable() == false, both permanent until an operator acts — into the retry-forever group. The grouping is defensible today, because the old code really did mean "may be another actor's build, wait and retry"; the split is what makes it wrong, and the compile break is the only thing that forces anyone to look.

Two cheap things would make the doc's warning self-enforcing rather than advisory:

• Give the codes their retryability in the type, not only in a table a consumer has to find: a func (c Code) Permanent() bool (or a PermanentCodes() set) beside Codes() turns the routing decision into something the consumer reads from the package instead of re-deriving. Recoverable() already does this for the error; the code is the surface an adapter switches on.
• Say it in the PR body, one line, in the same place it says the adapter must be updated: two of the five replacements are permanent, so the enumeration change is not mechanical. The body is what the consumer's author will read; the integration doc is what they'll read second.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Verified — the runs, the mutations, and five attacks that dissolved

Local. go build ./..., go vet ./... and gofmt -l clean. Full suite against a real PostgreSQL 16.14 (make db-up, PG_DSN=… go test -race -count=1 ./...): 17 packages, 0 failures, pkg/executor at 40.0s. CI is green on all five majors (14–18) plus build, lint, semgrep, zizmor, aws-boundary and smoke.

The lock claim, checked at the source rather than the docs. The file header rests on "every concurrent index command holds SHARE UPDATE EXCLUSIVE on the table for its whole life." That is true for a non-obvious reason worth having in the record: DefineIndex promotes the heap and index locks to session-level (LockRelationIdForSession) before its internal commits, so the multi-transaction structure doesn't drop them; index_drop and ReindexRelationConcurrently do the same. So the proof lock excludes CIC, DIC and RIC alike, including a builder whose progress row this role cannot read. The design's central bet is good.

Mutation testing — ten mutations, pkg/executor each time.

Mutation	Result
isAbandonmentCandidate drops && f.droppable	killed (TestIndexFactsIsAbandonmentCandidate)
classifyInvalidIndex drops the unobservable case	killed (4 tests, incl. both hidden-builder integration tests)
quarantine list matches by prefix, not name == prefix‖oid	killed (SweepsQuarantinedDebris)
build stops refusing over quarantined debris	killed (BuildIndexConcurrentlyRefusesQuarantinedDebris)
droppableColumn stops excluding constraint-backed indexes	survives
sweep drops its visible-builder refusal	survives
proof drops the schema half of the table identity	survives (see finding 2)
post-drop "the OID is gone" check removed	survives — unreachable
post-rename "the OID carries the quarantine name" check removed	survives — unreachable
catalogVerdict's NULL identity-facts guard removed	survives — unreachable

The last three are worth separating from the other survivors rather than counted against the diff. Each is a post-condition on a statement the server has already reported successful, and there is no way to make PostgreSQL report a successful DROP INDEX CONCURRENTLY that leaves the OID, or a successful ALTER INDEX ... RENAME that renames something else, or a pg_index⋈pg_class row with NULL identity columns. They are correct fail-closed code that no test at this layer can reach, and the comment on the NULL guard says as much in so many words. Noting them so the coverage number isn't read as three missing tests.

---

Attack that dissolved: a lock_timeout firing inside the proof transaction. The transaction sets lock_timeout = 5000 and then runs a SELECT and an ALTER INDEX ... RENAME; only the LOCK TABLE maps 55P03 to a *BudgetError, so a lock timeout at the rename would surface as an untyped InvalidIndexError. It can't happen: measured above, ALTER INDEX ... RENAME takes ShareUpdateExclusiveLock on the index and nothing on the heap. Everything that could hold a conflicting lock on that index (CIC/DIC/RIC, plain REINDEX, VACUUM, ANALYZE) also needs a table lock this transaction already holds; ordinary DML takes RowExclusive on the index, which doesn't conflict. The rename cannot queue.

Attack that dissolved: the SET lock_timeout = 5000 in dropQuarantinedIndex leaking to the next borrower. It runs on a pooled connection, and acquireBudgetedSession deliberately sets lock_timeout = 0 for the CONCURRENTLY exception policy — so an un-reset override would hand a 5s lock bound to whatever borrows the connection next, which for a build is precisely the setting that creates the invalid index this package exists to prevent. release() runs RESET lock_timeout; RESET statement_timeout and hijacks and closes the connection if the reset can't be proven. Fully closed, and the asymmetry it protects is right: a drop cancelled mid-wait leaves the entry for the next sweep, a build cancelled mid-wait leaves new debris.

Attack that dissolved: the progress view's hidden-row accounting. builderVisibilityColumns treats p.relid IS NULL as "a command whose target is withheld from this session". That matches the server: pg_stat_get_progress_info returns pid and datid to everyone and nulls st_progress_command_target and every param without pg_read_all_stats, and pgstat_progress_start_command is always called with a valid heap OID for CIC and RIC — so a visible row never has a NULL relid, and the count means what the comment says. The database-wide scope is over-broad by necessity, and the comment says so honestly rather than claiming precision it can't have. current_setting('track_activities') reads this session, not the builder's; if the builder has it off and we have it on, the classification says "abandoned" — but the recovery's table lock then catches the live builder anyway, so the weaker fact only ever costs a lock-budget error, never a wrong drop.

Attack that dissolved: remainingAfter dropping budget fields. It reconstructs ConcurrentBudget{Overall: left} rather than copying, which would silently drop any other field. ConcurrentBudget has exactly Overall and CallerOwned, and CallerOwned returns early, so the reconstruction is total today — worth knowing it becomes lossy the moment a third field is added.

Attack that dissolved: recoveryMinConns sizing. Peak concurrent sessions is 3 — the recovery's own session (held from recover.go:184 through the build) plus the build's build+verdict pair; the per-drop session is released before the build starts, so the drops peak at 2. recoveryMinConns = buildMinConns + 1 = 3 is exactly right, and the refusal is at admission with a message naming all three roles.

---

Leak check: clean. Nothing in the 19 files or the body names anything internal — no company names, hostnames, ticket ids or deployment topology. The schemabot-integration.md additions describe a public repo's public API surface and stay at that level. The pgsprite_abandoned_ prefix is a good choice for a name that lands in a customer's catalog: it says who put it there and that it's debris.

Two notes, neither a finding. The branch is 1 behind origin/main (the pg-sprite#78 merge) and 3 ahead; no conflict, and CI is green as-is. And LK-5's registry row is a genuinely good entry — it states the rule, names the enforcement precisely enough to check, and its "Source:" line points at the PostgreSQL behaviour the whole proof depends on, which is the thing a future reader would otherwise have to rediscover.

Both probe files were moved out of the tree and every mutation restored from backup; the worktree is clean at 8ec0ecc7.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving. The abandonment proof is sound — I verified the session-level SUE lock claim at the source and the rename's lock level empirically, and four of the safety predicates kill their mutations. Findings are in the review comment above: one narrow rename window in the proof (low-med, one-line close), three reachable mutation survivors, and a note on the consumer's retryability grouping.

This stamp was left by Claude Code (claude-opus-5).

[aparajon]

🤖 Re-review — 8ec0ecc7 → 253e7443 (rebased; real delta 20 files, +863/−124)

The three-way cancellation typing is the right decomposition, and the part I'd have argued for is the part that's already here: a 57014 at or past the overall budget stays a *BudgetError even when the caller's context ended in the same instant, because an orchestrator's deadline commonly sits just outside the budget it configured and the escalation signal must not be lost to that coincidence. That precedence is the whole design, it's stated in the doc comment, and TestAsConcurrentBudgetError pins it. Replacing BuildPID() with CancelBuild is also a real improvement rather than a rename — handing out a PID hands out something that stops being true the moment the build's session returns to the pool, and issuing the signal inside the tracker under pollMu makes StopConcurrentBuild an actual fence.

One finding, in the same shape as the change itself: the taxonomy grew by one member, and one of the two places that route on it didn't.

#	Sev	Where	What
1	med	pkg/executor/recover.go:552	isBoundedOutcome doesn't know ErrCancelledByCaller, so the recovery sweep reports its own caller's cancellation as invalid-index-unproven — the fail-closed "an operator must inspect the catalog" verdict
2	low	pkg/executor/native.go:556	Detaching the success-path verdict is a stated fix with no test; reverting it to ctx survives the whole suite
3	low	pkg/executor/native.go:1174	isStatementCancellation short-circuits on any non-57014 PgError, diverging from the `

---

1 — the recovery sweep can't report cancelled-by-caller (med)

isBoundedOutcome gained no case for the new sentinel, and its own doc says what that costs: these are the outcomes "that callers branch on directly and that must therefore not be wrapped in a verdict about the entry." So the wrap happens, and fail() only sets Cleanup — for which InvalidIndexError.Code() has no matching case, so it falls to default.

DROP INDEX CONCURRENTLY fails in dropQuarantinedIndex
                  │
   asConcurrentBudgetError(ctx, …)
                  │
   ┌──────────────┼──────────────────┬─────────────────────┐
   ▼              ▼                  ▼                     ▼
*BudgetError   ErrCancelled       ErrCancelled          (other)
               Externally         ByCaller
   │              │                  │                     │
   ▼              ▼                  ▼                     ▼
isBoundedOutcome: true           FALSE  ← the gap       false
   │              │                  │                     │
   ▼              ▼                  ▼                     ▼
propagated     propagated       fail(): wrapped in     fail()
   │              │             InvalidIndexError          │
   ▼              ▼                  ▼                     ▼
budget-        cancelled-       invalid-index-        invalid-index-
statement-     externally       UNPROVEN              unproven
exceeded

Measured, driving each cause through asConcurrentBudgetError → isBoundedOutcome → fail → OutcomeCode exactly as dropQuarantinedIndex does:

caller cancel, bare context.Canceled    bounded=false  CODE=invalid-index-unproven
caller cancel, server 57014 arrived     bounded=false  CODE=invalid-index-unproven
operator pg_cancel_backend, live ctx    bounded=true   CODE=cancelled-externally
budget exhausted                        bounded=true   CODE=budget-statement-exceeded

So the sweep can cleanly report a third party's cancel and its own budget, but not the one cause this PR added a type for. ErrCancelledByCaller's own doc says an orchestrator "reads this error as its own lease lapsing" — and a lapsing lease is the most ordinary way a recovery sweep ends. What it gets instead is invalid-index-unproven, which in the consumer means the catalog state could not be verified, inspect pg_index.indisvalid by hand before any recovery.

That contradicts this function's own reasoning. The comment above dropQuarantinedIndex says a drop cancelled mid-wait "leaves the entry exactly as it was for the next sweep" — the state is proven, benignly unchanged, and the next sweep handles it. unproven is the one thing it isn't.

Worth separating from the build path, where the same wrap is correct: there InvalidIndexError reports a leftover the build really did create, the outer code is invalid-index-own-leftover, and the cause is recovered from invalidErr.Build — which is exactly what your new TestBuildIndexConcurrentlyCallerOwnedCallerCancel asserts. That two-level design is right. The recovery path has no Build field to carry the cause, and isBoundedOutcome exists precisely to keep the wrap from happening there.

One line closes it:

func isBoundedOutcome(err error) bool {
	var budgetErr *BudgetError
	return errors.As(err, &budgetErr) ||
		errors.Is(err, ErrCancelledExternally) ||
		errors.Is(err, ErrCancelledByCaller)
}

Verified — with that applied, all four causes classify correctly and the full suite stays green:

caller cancel, bare context.Canceled    bounded=true   CODE=cancelled-by-caller
caller cancel, server 57014 arrived     bounded=true   CODE=cancelled-by-caller
operator pg_cancel_backend, live ctx    bounded=true   CODE=cancelled-externally
budget exhausted                        bounded=true   CODE=budget-statement-exceeded

The suite passing both before and after is the part worth acting on beyond the one line: no test pins either behaviour, so this is uncovered rather than deliberate.

On the guard for it. TestCodesEnumerateEveryDeclaredCode is a genuinely good addition — I checked it bites rather than assuming, by declaring a Code constant and leaving it out of Codes(), and it fails with the missing member named. But it guards the vocabulary, not the routing: Codes() is now complete by construction while isBoundedOutcome and InvalidIndexError.Code() are still hand-maintained switches over the same taxonomy, and the new member slipped through one of them in the same commit that added the completeness test. A table test over the three cancellation sentinels asserting each one's isBoundedOutcome verdict and OutcomeCode would sit naturally beside it.

2 — the success-path detach has no test (low)

Moving verdictCtx above the buildErr == nil branch fixes something the old code got wrong, and the new comment states the claim: "a build that succeeded at the finish line must not report as unproven because the caller cancelled a moment later." But reverting just that one call to the caller's context —

return verifiedBuildReport(ctx, conn, build, target, elapsed)

— survives ./pkg/executor/ and ./pkg/progress/ in full. The failure-path detach is covered; the success path isn't, and it's the half where the consequence is a successful build reported as unverified. blockedCallerOwnedBuild already builds the fixture this needs — a build that completes, with the context cancelled immediately after the statement returns.

3 — the two cancellation classifiers have different shapes (low)

isStatementCancellation is an if/else on errors.As:

if errors.As(err, &pgErr) {
	return pgErr.Code == sqlstateQueryCanceled
}
return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)

so any PgError anywhere in the chain that isn't 57014 returns false without ever reaching the context check. corroborateValidateCancel expresses the same three-way typing as an OR (serverCancelled || clientCancelled), which has no such short-circuit. Two implementations of one rule, and only one of them can be reached past a PgError.

I tried to reach it and couldn't, so this is a structural note rather than a live bug. Synthetic chains do show it — 57P01 + context.Canceled, 08006 + context.Canceled, context.Canceled + 57P01 and deadline + 08006 all classify as not-a-cancellation and come out execution-failed. But against a real server, both realistic shapes return a bare context.Canceled with no PgError in the chain at all:

A caller cancels a live CREATE INDEX CONCURRENTLY   err=context canceled  chain -> *errors.errorString  CODE=cancelled-by-caller
B pg_terminate_backend, then caller cancels         err=context canceled  chain -> *errors.errorString  CODE=cancelled-by-caller

So pgx doesn't produce the hole today. Matching the sibling's || shape costs nothing and removes the dependence on that:

if errors.As(err, &pgErr) && pgErr.Code == sqlstateQueryCanceled {
	return true
}

I confirmed that change is behaviour-preserving against the current suite.

---

Verified — the precedence, the tracker rework, the mutation ledger, and two attacks that dissolved

The budget-first precedence is right and protected. Dropping the !b.CallerOwned term from the budget branch is killed by three tests (TestAsConcurrentBudgetError, TestBuildIndexConcurrentlyCallerOwnedOperatorCancelViaTracker, TestBuildIndexConcurrentlyCancelBuildResistsCatalogShadowing). The native_internal_test.go table is the strongest part of the diff: it asserts both ErrorIs and NotErrorIs on each sentinel for every case, which is what makes a three-way typing actually pinned rather than just exercised.

CancelBuild is a better primitive than BuildPID, for the reason given. A PID handed to a caller stops being the build's the moment the session returns to the pool; issuing the signal inside the tracker under pollMu, with StopConcurrentBuild taking the same mutex before the session can be released, is what makes "the PID it signals still belongs to the build" true rather than hoped. Reading the backend's state and signalling it in one statement rules out the common lost-cancel, and the describeState / classifyBuildBackend split means a hidden or untracked backend is reported as unobservable instead of assumed either way — treating NULL as idle is killed by TestCancelBuildReportsAnUnobservableBackend. pg_catalog-qualifying pg_cancel_backend against a search_path impostor is the kind of thing that only gets written by someone who tried the attack, and TestBuildIndexConcurrentlyCancelBuildResistsCatalogShadowing proves the signal reaches the real backend through it.

Attack that dissolved — lock-order inversion. A new method taking both pollMu and mu is the classic way to introduce one, and Progress releases mu mid-function which is where an inversion usually hides. Checked every acquisition: CancelBuild, StopConcurrentBuild and Progress all take pollMu then mu; Start, StartStep, SetAttempt, SetConcurrentBuild and Finish take mu alone. No path takes them in the other order, and the doc comment added at Tracker explains why the resets deliberately stay under mu only. CancelBuild can hold pollMu for up to cancelSignalTimeout and StopConcurrentBuild waits behind it, which is the intended fence and is bounded.

Attack that dissolved — the double StopConcurrentBuild. The new defer sits alongside an explicit call, which is normally where an idempotency bug lives. It's safe: the method only zeroes session and buildPID under both locks, so the second call is a no-op. Dropping the defer survives the suite, but only via the panic path it exists for, so I'd keep it — that's defensive depth, not missing coverage.

LK-2's correction is the honest direction. Replacing "the statement remains bounded by construction" with the distinction between the bounded client call and the unbounded server statement — "a client that dies without cancelling leaves it running until Tracker.CancelBuild or an operator's pg_cancel_backend stops it" — makes the entry weaker and true instead of stronger and not. Narrowing CodeCancelledExternally's doc to "not by its caller, not by its budget" keeps the vocabulary mutually exclusive now that there are three causes.

Mutation ledger — 7 mutations, 3 killed, 2 reachable survivors, 2 unreachable by construction. Killed: !b.CallerOwned dropped from the budget branch; classifyBuildBackend treating NULL as idle; a declared Code left out of Codes(). Reachable survivors are findings 2 and 3 above. Unreachable: the elapsed >= b.Overall boundary relaxed to > (a nanosecond-wide window the doc already calls approximate), and dropping the defer StopConcurrentBuild (panic-only). Also survived but equivalent: removing the CASE WHEN state = 'active' guard from cancelBuildSQL, because classifyBuildBackend independently decides the return value — the SQL guard only avoids emitting a signal PostgreSQL would drop, so it's belt-and-braces rather than load-bearing.

Suite and hygiene. Full suite green against real PostgreSQL 16.14 (make db-up, PG_DSN=… go test -count=1 ./..., 17 packages, pkg/executor 34s), and green again with the finding-1 fix applied. BuildPID has no remaining reference anywhere in the repo, so the removal is clean. Probes removed, worktree clean at 253e7443.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Re-review, part 2 — carry-overs from round 1, and what the new code does downstream

Still open from round 1

pkg/executor/recover.go moved by two lines in this delta (threading ctx into asConcurrentBudgetError), so neither round-1 item was touched. Restating rather than re-arguing, since both were low and neither blocks:

• The target's identity is proven by OID but never against the name the statement gave. build.table still appears nowhere in recover.go — it's dropped after resolveTarget, and the under-lock check compares the inspection's name against itself, so a rename landing in the resolve→inspect window is followed silently. LK-5 as written is not violated (it says by OID and schema) and the blast radius stays small, since the same OID is the same physical relation; what's wrong is the comment four lines above. One line: compare existing.table != build.table before quarantining.
• Code.Permanent() beside Codes(). Finding 1 in my other comment is the argument for it. The taxonomy now has three hand-maintained switches over it — sentinelCode, InvalidIndexError.Code() and isBoundedOutcome — and Codes() plus its new completeness test only guarantees the vocabulary is whole, not that each member is routed. A Permanent() method (or any predicate the consumer can read off the package instead of re-deriving) turns the routing into something a completeness test can cover too.

The three reachable mutation survivors from round 1 are also unchanged, since no recovery test moved: droppableColumn's pg_constraint.conindid term, the sweep's visible-builder refusal at recover.go:374, and the schema half of the under-lock table identity.

Downstream: the new code will stop the consumer's build, as designed

block/schemabot pins pg-sprite v0.2.0 and imports pkg/executor, where TestRefusalForOutcomeTotalOverExecutorCodes iterates executor.Codes() and asserts every member has an explicit disposition. CodeCancelledByCaller is in Codes(), so that test fails on the bump until it's routed — which is the mechanism working exactly as intended, and worth naming in the release notes so it reads as expected rather than as breakage.

The routing I'd suggest: group it with the operational set alongside CodeCancelledExternally, not with the refusals. A caller's context ending is a lease lapsing or a deadline expiring; nothing about the target needs to change before a retry, so a permanent refusal would be wrong. Concretely, adding it to the existing case at pkg/engine/postgres/apply.go:448 is the whole change.

Two smaller notes on the same surface:

• invalidIndexDetail is where finding 1 becomes operator-facing. Its default branch renders an unrecognised verdict as "may be invalid but its catalog state could not be verified; inspect pg_index.indisvalid on the target before any recovery". That is the correct fail-safe wording for a genuinely unproven entry, and it is what a recovery sweep cancelled by its own caller currently produces. Fixing isBoundedOutcome keeps this branch for the cases that deserve it.
• Removing BuildPID() costs nothing downstream. pkg/progress isn't imported by the consumer at all — it takes dbconn, diffplan, executor, plan, planner, preflight, router, schemadiff and statement — so dropping a public method from the tracker breaks no build outside this repo. Worth stating because removing an exported method is otherwise the kind of thing that wants a deprecation window, and here it doesn't.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving at 253e7443. The three-way cancellation typing is the right decomposition and the budget-first precedence — a 57014 at or past the overall budget stays a *BudgetError even when the caller's context ended in the same instant — is the part that makes it usable by an orchestrator. CancelBuild replacing BuildPID() is a genuine safety improvement, not a rename.

One medium finding to pick up: isBoundedOutcome (recover.go:552) has no case for the new ErrCancelledByCaller, so the recovery sweep reports its own caller's cancellation as invalid-index-unproven — the fail-closed verdict — which contradicts dropQuarantinedIndex's own reasoning that a cancelled drop leaves the entry unchanged for the next sweep. Measured, one-line fix, suite green with it applied. Details plus two low findings in the comments above.

This stamp was left by Claude Code (claude-opus-5).

[morgo]

🤖 Automated review on Morgan's behalf. Approving.

This drops indexes automatically, so I read the proof chain rather than the description. It holds up, and the central design call is the right one: pg_stat_progress_create_index genuinely cannot answer "is anyone building this?" — it nulls relid/index_relid without pg_read_all_stats and records nothing under track_activities = off — so treating "no visible builder" as "no builder" would drop an index mid-build. Using the SHARE UPDATE EXCLUSIVE lock as the oracle is correct, because a concurrent index command holds it for its entire life, so holding it yourself excludes a builder you cannot see.

What I verified in the code:

• A valid index can never be quarantined or dropped. isAbandonmentCandidate requires !f.valid alongside the exact OID, exact name, indrelid == target.tableOID, and droppability, and it gates both the under-lock quarantine and the pre-drop re-verify.
• The sweep is self-keying, which is the property that makes automatic dropping safe. listQuarantinedIndexes matches c.relname = ($2 || c.oid::text) — an entry qualifies only if its name is literally pgsprite_abandoned_ followed by its own OID. An operator's index can't collide with that by accident, so the sweep can only ever see debris this code renamed.
• Identity is re-established under the lock, not carried over. LOCK TABLE necessarily takes a name, and the re-check then requires target.tableOID to still carry that name and schema — so a table renamed away with an impostor taking its name locks the impostor and immediately bails with ErrTargetIdentityChanged rather than acting. Treating the schema as part of table identity is a subtle case to have gotten right.
• The rename is verified before it commits: the proof's OID must now carry the quarantine name, or the ALTER INDEX touched something the proof never examined.
• A reported drop is not trusted. Counting pg_class for the OID afterwards catches a DROP INDEX CONCURRENTLY that failed midway and left the entry in place.
• Both lock waits are bounded at 5s, 42P01 and 55P03 are separated into distinct verdicts, and pgx.ErrNoRows at the quarantine stage correctly no-ops rather than erroring.
• Every identifier goes through pgx.Identifier{...}.Sanitize(), quarantine names are derived from a uint32, and the catalog predicates use OPERATOR(pg_catalog.=) so a hostile search_path can't shadow the operators.

Also worth crediting: skipping rather than refusing an undroppable quarantine entry, so one such entry can't wedge the table forever, and having a plain BuildIndexConcurrently refuse over droppable quarantine debris so a recovery that died between rename and drop can't go quiet.

Two observations, neither blocking:

1. The drop path uses session-level SET lock_timeout rather than SET LOCAL — unavoidable, since DROP INDEX CONCURRENTLY can't run inside a transaction. The comment says the release resets it with the rest of the session settings, which is the right contract; just noting it's the one setting here whose cleanup depends on the pool release doing its job rather than on transaction scope.
2. I considered OID reuse — an index quarantined as pgsprite_abandoned_12345, dropped externally, with a later index receiving OID 12345 and being named to match. It needs deliberate adversarial naming, and NOT indisvalid plus the indrelid and droppability predicates would still have to line up, so it isn't a practical concern. Mentioning it only so it's on the record as considered.

The consumer note about Codes() exhaustiveness and the removal of invalid-index-preexisting is the kind of thing that bites a downstream adapter silently, so it's good that it's called out in the description — worth carrying into the schemabot bump when it happens.

CI is clean and the PR is CLEAN to merge.

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/79, follow-up commit

All findings from review comments 5562518278, 5562518558 (round 2) and 5564254652, 5564255058 (round 3) are fixed in one follow-up commit; nothing deferred, nothing rejected. Every mutation the reviews named as surviving was reintroduced and confirmed to fail its new test. The two downstream notes in 5564255058 are answered inline; they are consumer-side work at the pin bump, not changes to this PR.

#	Concern	Status
R3-1 (med)	isBoundedOutcome lacked ErrCancelledByCaller, so a recovery sweep cancelled by its own caller mid-drop was wrapped in InvalidIndexError and reported invalid-index-unproven — the "inspect pg_index.indisvalid by hand" verdict — for an entry a cancelled drop leaves exactly as quarantined	fixed — one-line routing plus the two tests the review asked for: TestIsBoundedOutcomeRoutesEveryCancellationCause drives all five causes through asConcurrentBudgetError → isBoundedOutcome → wrap → OutcomeCode exactly as dropQuarantinedIndex does (caller cancel bare / with 57014 → cancelled-by-caller; live-context 57014 → cancelled-externally; at budget → budget-statement-exceeded; anything else → invalid-index-unproven); TestRebuildAbandonedIndexReportsItsCallerCancellingTheDrop holds SUE on the table, waits until pg_stat_activity shows the DROP INDEX CONCURRENTLY blocked on the lock, cancels, and asserts ErrCancelledByCaller, not InvalidIndexError/BudgetError, debris still quarantined, index never built. Reverting the line fails both
R2-1 (low)	Abandonment proof compared OID + schema under the lock but never the table name the statement gave; a rename in the resolve→inspect window was followed silently, and the comment above claimed otherwise	fixed — indexTarget carries the statement's table; quarantineAbandonedIndex refuses ErrTargetIdentityChanged before the lock when existing.table != target.table, locks target.table, and re-checks under the lock. LK-5 now reads "by OID, schema and the table name the statement gave". Killing test: "table renamed before the inspection" subtest of TestQuarantineAbandonedIndexFailsClosedOnStaleObservation
R2-3 / R3-p2 (low)	Removing invalid-index-preexisting breaks a consumer's total switch over Codes(), and the five replacement codes are not one retry class — asked for Code.Permanent() so routing, not just vocabulary, is testable	fixed — func (c Code) Permanent() bool: permanent = decided by the statement, the caller's configuration, or the standing catalog; retrying unchanged reproduces it and no executor entry point changes it. Two of five invalid-index codes are permanent (other-table, not-droppable); own-leftover, abandoned, builder-unobservable, build-in-flight, unproven are not; nor are budget-* or cancelled-*. TestCodePermanentClassifiesEveryCode pins the case set to Codes(); execution-model gains a Permanent column that TestDocPermanentColumnMatchesCodePermanent pins to the method; schemabot-integration tells adapters to start from Permanent(). Documented as a floor — an adapter may still refuse a non-permanent code
R3-2 (low)	Success-path verdictCtx detach (native.go) had no test; reverting that one call to ctx survived the suite	fixed — TestBuildIndexConcurrentlyVerifiesASuccessfulBuildAfterTheCallerCancels uses a cancelOnSecondRead clock: the tracker's second Now() read is the elapsedSince immediately after Exec returns, so the caller's context is cancelled at the instant the successful statement completes; expects NoError and a verified valid index. Reverting to ctx fails it
R3-3 (low)	isStatementCancellation short-circuited on any non-57014 PgError, diverging from corroborateValidateCancel's `
R2-2a (low)	Surviving mutation: schema half of the under-lock identity check	fixed — "table moved to another schema behind a same-named decoy" subtest: target moved out, same-named table created in its place, so OID/name match but schema does not; expects ErrTargetIdentityChanged, nothing dropped
R2-2b (low)	Surviving mutation: sweep's visible-builder refusal on a quarantine-named entry	fixed — TestRebuildAbandonedIndexRefusesSweepWhileQuarantinedEntryIsBeingBuilt: a CIC parked in "waiting for old snapshots" holds no lock on its own index, so the entry is renamed to pgsprite_abandoned_<oid> under it while pg_stat_progress_create_index.index_relid still points at it; the sweep refuses ErrInvalidIndexBuildInFlight with Index = quarantine name and BuilderPID > 0, drops nothing. The suggested "REINDEX CONCURRENTLY visibly in flight" shape is not constructible on PG 16: from phase 2 on index_relid reports the new _ccnew OID, and a unique reindex over duplicates fails in the build pass before the snapshot wait; that path is covered by the reindex session's SUE lock hitting the drop's lock budget (TestRebuildAbandonedIndexReportsLockBudgetWhenDropIsBlocked)
R2-2c (low)	Surviving mutation: droppableColumn's pg_constraint.conindid term	fixed — TestDroppableColumnMatchesTheServer checks the predicate against the server's own DROP INDEX CONCURRENTLY verdict: plain index droppable; PK, unique constraint, exclusion constraint, FK-referenced unique → 2BP01; partitioned parent ON ONLY → 0A000; attached partition → 2BP01. Deleting the term makes the constraint cases claim droppable
R3-p2 note	Consumer's TestRefusalForOutcomeTotalOverExecutorCodes will fail on the pin bump until CodeCancelledByCaller is routed; suggested operational group beside CodeCancelledExternally	reply — agreed on both the mechanism and the routing. No change here (the core must not depend on the orchestrator); the routing lands in the consumer's pin-bump PR as an addition to the existing CodeCancelledExternally arm, and the v0.3.0 release notes name the break and the intended group so it reads as the guard working. PR body's consumer note now carries the same line
R3-p2 note	invalidIndexDetail's default wording is the correct fail-safe for a genuinely unproven entry; fixing R3-1 keeps it for those cases. Removing BuildPID() costs nothing downstream	reply — agreed; both are consequences of R3-1 and the CancelBuild design, nothing further to do

Verification on the follow-up: gofmt -l clean; go vet ./... clean; golangci-lint run ./pkg/... ./cmd/... 0 issues; go test -race -count=1 ./pkg/executor/... ok (165s, PostgreSQL 16 via testcontainers); make test-unit ok. Mutation checks: isBoundedOutcome without ErrCancelledByCaller → 2 tests fail; isStatementCancellation back to if/else → 3 subtests fail; success-path verify under ctx → 1 test fails; each of the four round-2 mutations → its test fails; one flipped Permanent cell → docs test fails.

Adapter-side note (not a change to this PR): the consumer's refusalForOutcome on its main today treats budget-statement-exceeded as a refusal and pool-too-small as operational; under Permanent() the former is not permanent and the latter is. Both are legitimate adapter policy; the pin-bump PR should decide them explicitly rather than inherit.

Source: #79, review comments 5562518278 and 5562518558 at head 8ec0ecc7; 5564254652 and 5564255058 at head 253e7443