add drop-only abandoned index recovery

[Kiran01bm]

Add a library recovery entry point that safely removes an abandoned invalid index without rebuilding it.

Why

Callers cleaning up a cancelled apply need the existing proof, quarantine, and OID-verified concurrent drop guarantees without resuming the requested index build.

What

• Add executor.DropAbandonedIndex with the same admission and fail-closed recovery semantics as RebuildAbandonedIndex.
• Share proof, quarantine, and drop sequencing between drop-only and rebuild recovery.
• Leave IndexRecoveryReport.Build zero for drop-only recovery and document the API.
• Cover successful drop-only cleanup, no-debris behavior, and refusal cases with integration tests.

Before / after

Before: CREATE statement -> prove -> quarantine -> DROP CONCURRENTLY -> rebuild
After:  CREATE statement -> prove -> quarantine -> DROP CONCURRENTLY -> [optional rebuild]

[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 review, part 1 of 2 — contracts (39869637, 4 files, +168/−22 over fd4bcb1)

The refactor itself is the right shape and I could not break it. Extracting recoverAbandonedIndex with an after hook keeps a single copy of the proof, the quarantine, and the sweep, so the drop-only entry point inherits the fail-closed verdicts by construction rather than by a parallel implementation that could drift. Every mutation I aimed at the split died (details in part 2), including the ones that would have let the drop-only path build, let the hook run before the sweep, or let the hook's error be swallowed.

What I do want to flag is that the pool-size contract is now stated in four places and they do not agree, and that the invariant registry no longer points at where the proof lives.

LK-5's enforcement pointer names a function that no longer performs the enforcement (med-low)

docs/invariants.md:209 reads:

Enforced: pkg/executor recovery (RebuildAbandonedIndex): locked re-verification and rename, pre-/post-drop OID checks, droppability predicate, shared-budget accounting…

After this PR none of those mechanisms are in RebuildAbandonedIndex. It is a four-line wrapper; the locked re-verification, the OID checks, the droppability predicate and the budget accounting are all in recoverAbandonedIndex and its callees, and there is now a second public entry point into them. The pointer still resolves to a real symbol, which is the version of this that goes unnoticed — nothing about the diff looks registry-relevant, so nobody opens the registry.

The same paragraph a few lines up carries the consequence for a reader rather than a maintainer:

The plan completes only through this invariant's proven removal — RebuildAbandonedIndex — or through an operator following the runbook

That enumeration is now incomplete, and the omission is exactly the case the new entry point exists for: a caller that must clear the occupied name without completing the plan. A reviewer asking "is DropAbandonedIndex covered by LK-5?" finds no answer in the registry, even though the answer is yes and the tests are here.

*Enforced:* naming the shared helper and both entry points, plus DropAbandonedIndex in that enumeration, is a two-line change and belongs in this PR — the registry's own header frames "where each rule is enforced" as part of what it is for.

The exported ErrPoolTooSmall contract is now false for the new caller (med-low)

recover.go:179 passes buildMinConns. The number is right — the recovery session is held across the drops and each drop takes one session sequentially, so two is exactly enough, and I confirmed nothing in the sweep acquires a third. But four texts now describe that two, and three of them describe a different two:

Where	What it says the connections are
native.go:182-184 (buildMinConns)	"the build session and the verdict session reserved beside it"
native.go:136-139 (ErrPoolTooSmall)	"for a build … buildMinConns; for a recovery, its own session on top of those (recoveryMinConns)"
recover.go:174-175 (DropAbandonedIndex)	"the recovery session and a drop session"
docs/invalid-index-recovery.md	"its own session and one drop session"

The last two are correct. The first names two sessions the drop-only path never opens. The second is now simply wrong about the sentinel error it documents: a recovery can need buildMinConns, so "for a recovery, its own session on top of those" no longer holds, and that comment is the contract an embedder reads when they catch ErrPoolTooSmall and decide how to resize.

This is a naming and documentation problem rather than a live coupling, and I want to credit why: TestDropAbandonedIndexRunsOnPoolSizedForTheBuild hardcodes MaxConns: 2 as a literal rather than deriving it from the constant, so raising buildMinConns would fail that test rather than silently widening the drop-only requirement. I mutated exactly that (buildMinConns → recoveryMinConns at recover.go:179) and the test caught it. The fix is a dropRecoveryMinConns = 2 with its own comment naming the held recovery session and the drop session, and one clause added to the ErrPoolTooSmall doc.

The refusal message lost its enumeration precisely where the count became ambiguous (low-med)

Before, the recovery had one caller and a fixed requirement:

index recovery needs 3 connections (recovery session, build session,
and reserved verdict session), pool holds 2

Now it is shared, so the enumeration went away:

index recovery needs %d connections, pool holds %d

and the same sentence is emitted for both entry points with a count that varies. An operator reading "index recovery needs 2 connections, pool holds 1" cannot tell which operation refused, and downstream in block/schemabot this maps to a refusal telling them to raise the pool size on the target DSN — actionable on the number, silent on the why. Its sibling guard for the plain build kept its enumeration (native.go:469: "build session and reserved verdict session"), so the file now has one enumerated pool refusal and one bare one, and the bare one is the one whose requirement is not constant.

Passing a short description alongside minConns, or having each wrapper format its own message, restores it without reintroducing the duplication the refactor removed.

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

[aparajon]

🤖 Adversarial review, part 2 of 2 — evidence and notes (39869637)

Baseline, against a live PostgreSQL 16 via PG_DSN (so the integration tests run rather than skip):

go build ./...                                          clean
go test -run 'AbandonedIndex' ./pkg/executor/           ok  17.193s
go test ./pkg/executor/                                 ok  30.311s
gh pr checks 90                                         14/14 pass

Fourteen mutations against recover.go, aimed at the extraction boundary — the after hook, the minConns parameter, the guards it now shares, and the Duration assignment that moved out of the helper. Thirteen killed by tests, one survived.

drop passes recoveryMinConns not buildMinConns   killed  RunsOnPoolSizedForTheBuild
rebuild passes buildMinConns not recoveryMinConns killed  (by the 900s test timeout — see below)
drop also runs the build                          killed  RemovesOwnLeftoverWithoutRebuilding,
                                                          WithoutDebrisDoesNotBuild
the after hook never runs                         killed  RemovesOwnLeftoverAndRebuilds,
                                                          WithoutDebrisIsJustTheBuild,
                                                          SweepsQuarantinedDebris,
                                                          ProofsResistCatalogShadowing
the after hook runs before the sweep              killed  SweepsQuarantinedDebris, +3
the hook's build report is discarded              killed  RemovesOwnLeftoverAndRebuilds, +3
the hook's error is discarded                     killed  NeverDropsValidIndex
caller-owned cancellable-context guard removed    killed  AdmissionGuardsPrecedeSessionUse
pool guard < -> <=                                killed  RunsOnPoolSizedForTheBuild
drop reports Duration 0                           killed  RemovesOwnLeftoverWithoutRebuilding,
                                                          WithoutDebrisDoesNotBuild
rebuild reports Duration 0                        killed  SkipsQuarantinedEntryTheServerWillNotDrop
drop's Duration assignment dropped (compile)      killed (compile — not a test kill)
rebuild's Duration assignment dropped (compile)   killed (compile — not a test kill)
drop's Duration set only when err == nil          SURVIVED

Worktree clean afterwards. The two compile kills are honest zeros, not evidence — removing the assignment orphans start, so the compiler catches it before any test does; the behavioral versions of both (assign time.Since(start) * 0) are the two Duration 0 rows above, and those are real test kills.

That is a tight suite for a refactor of this shape. The five new tests do the load-bearing work: assert.Zero(t, rep.Build) and assert.False(t, exists, "the requested index is not rebuilt") are what kill the "drop also builds" mutant, and the MaxConns: 2 literal is what makes the pool number falsifiable.

Duration is now populated on refusals, and nothing pins that (low)

Moving start := time.Now() out of the helper into both wrappers is more than plumbing. Before, start was taken after the admission guards and Duration was assigned on the final line only, so every non-success return reported Duration: 0 — including the refusals where the elapsed time is the interesting part, like a *BudgetError from the proof lock after waiting out recoveryLockTimeout. Now every path reports what it spent.

That is the better behavior, and Duration's doc at recover.go:98-102 frames the field for exactly this reader ("a caller that sized the budget as a lease can see what the recovery actually spent") — but it then describes the value as "proof and drops, plus the build when requested", which reads as a success-path accounting and does not tell that caller they will now get a real number on a refusal too. Restoring the old semantics (if err == nil { rep.Duration = … }) survives the whole suite, so a future edit can take it back silently. One assertion on a refusal path — RefusesOtherTableLeftover already has the report in hand — plus a clause in the field's doc closes both halves.

Two notes

The pool guard's own justification is now demonstrated rather than asserted. Making the rebuild demand buildMinConns does not fail an assertion — it makes TestRebuildAbandonedIndexRefusesPoolWithoutRoomForItsSession stop refusing and walk into the recovery holding one connection while the build waits for two from a pool of two, which is precisely what the guard's comment predicts ("would not fail but wait on itself for as long as the caller's context allows"). It killed the mutant by burning the 900-second test timeout. Nothing to change — the guard is correct and fail-closed, and it is worth knowing that the test pinning it degrades to a hang rather than a failure, since that is what a CI run would show if the constant ever drifted.

The narrowed LK-2 comment is correct, and I checked rather than assumed. recover.go:193-195 went from "the drops and the build run in caller-owned mode" to "the drops run in caller-owned mode", which reads at first like a clause lost in the extraction, since the shared guard still fires ahead of the rebuild's build. But buildIndexConcurrently carries an identical guard of its own (native.go:456-459), so the build is bounded by construction whichever way it is reached, and "the drops" is the accurate description of what this guard alone is responsible for.

One piece of context rather than a finding: this PR ships the drop-only path with no caller, so the entry point's only exercise is its own tests until the consumer lands. The four verdict cases and the pool sizing are covered; the shared-budget accounting over multiple quarantined entries and the not-droppable skip are covered only through the rebuild, which is fine while the code is one function, and worth remembering if the paths ever diverge.

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

[aparajon]

🤖 Approving. The extraction keeps one copy of the proof, so the drop-only entry point inherits the fail-closed verdicts rather than reimplementing them, and thirteen of fourteen mutations at the new boundary died on tests. My findings are contract and documentation ones — LK-5's enforcement pointer, the exported ErrPoolTooSmall doc, and the de-enumerated refusal message — none of which block the merge.

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

[morgo]

🤖 Review posted by Morgan's AI agent.

Approving at 39869637. The extraction is honest — recoverAbandonedIndex takes the build as an after callback at exactly the position the old inline call occupied, so the rebuild path's ordering, its held recovery session, and its error propagation are unchanged. Nothing about RebuildAbandonedIndex moves except where the code lives.

The part worth actually checking is the pool minimum, since lowering an admission guard is the one way this could hurt: admit on too few connections and the recovery waits on itself for the caller's whole budget instead of failing fast. So I traced where the sessions actually go.

recoverAbandonedIndex holds its own session across the drops (pool.Acquire at recover.go:213, released by defer), and dropQuarantinedIndex takes one more via acquireBudgetedSession. Critically, its post-drop confirmation — the count(*) on the OID that makes a reported success trustworthy — runs on that same drop connection, not on a reserved third. So the drop-only peak really is two, and recoveryMinConns = buildMinConns + 1 exists because buildIndexConcurrently needs a build session plus a separate verdict session beside it. Dropping the requirement to 2 for a path with no build is correct, not a relaxation.

TestDropAbandonedIndexRunsOnPoolSizedForTheBuild pins exactly that, and pins it the right way: same pool, same statement, rebuild refused with ErrPoolTooSmall and drop succeeding on MaxConns: 2. That's a differential assertion rather than a "it works" assertion, so a future change that quietly re-couples the two minimums fails it.

Also good: adding "drop" to the existing partitioned-parent refusal table means the new entry point inherits the refusal cases by construction instead of by a copy that can drift, and every new test asserts quarantinedIndexes is empty — so a drop-only recovery is held to the same "no debris survives" standard as the rebuild.

One nit, no action needed now. DropAbandonedIndex passes buildMinConns as its minimum. The number is right, but the constant's own doc comment says it means "the build session and the verdict session reserved beside it" — neither of which is a session this path uses. It works today because both quantities are 2 for unrelated reasons. If a build ever needs a third session, buildMinConns becomes 3 and the drop path silently starts demanding a connection it has no use for. A dropRecoveryMinConns = 2 with its own one-line rationale would decouple them.

Related, smaller: the admission error lost its role breakdown. It used to name the three sessions ("recovery session, build session, and reserved verdict session"); it now reads "index recovery needs %d connections". Since the whole point is telling an operator how to size the pool, the roles were the useful half of that message — and they now differ per entry point, so they'd have to come from the caller.

14/14 checks green. mergeStateStatus is BEHIND, so it needs a main update before merge — nothing to do with the change itself.

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — block/pg-sprite pull/90, follow-up commit.

Verdict: every finding across the two adversarial review comments and the approving review's nit is addressed in one follow-up commit; the notes and context items needed no change. Nothing rejected or deferred.

Part 1 — contracts (comment)

#	Concern	Status	Explanation
1 (med-low)	LK-5's Enforced: pointer in docs/invariants.md names RebuildAbandonedIndex, which is now a four-line wrapper; the mechanisms live in recoverAbandonedIndex, and the "plan completes only through … RebuildAbandonedIndex or the runbook" enumeration omits the new entry point.	fixed	Enforced: now reads recoverAbandonedIndex, shared by both entry points RebuildAbandonedIndex and DropAbandonedIndex. The completion enumeration adds DropAbandonedIndex "when the name must be cleared without completing the plan" — the case the entry point exists for.
2 (med-low)	DropAbandonedIndex passes buildMinConns, whose doc names sessions the drop path never opens; ErrPoolTooSmall's doc says a recovery needs "its own session on top of those", now false for the drop path. Four texts describe the same 2, three of them differently.	fixed	New dropRecoveryMinConns = 2 in pkg/executor/native.go with its own comment (recovery session held across the drops, drop session beside it; its own constant because it counts different sessions from buildMinConns and only happens to equal it). recoveryMinConns's comment now says "rebuild recovery". ErrPoolTooSmall's doc lists all three: build (buildMinConns), rebuild recovery (recoveryMinConns), drop-only recovery (dropRecoveryMinConns). DropAbandonedIndex passes dropRecoveryMinConns. The docs/invalid-index-recovery.md and DropAbandonedIndex doc-comment wording was already the correct one and is unchanged.
3 (low-med)	The shared guard's refusal lost its session enumeration exactly where the count became variable; "index recovery needs 2 connections, pool holds 1" does not say which operation refused.	fixed	recoverAbandonedIndex now takes a recoveryPool{minConns, sessions}; each entry point supplies its own — rebuild: recovery session, build session, and reserved verdict session; drop: recovery session and drop session — and the refusal reads index recovery needs %d connections (%s), pool holds %d, matching the plain build's enumerated refusal. One guard, one message format, no duplicated logic.

Part 2 — evidence and notes (comment)

#	Concern	Status	Explanation
1 (low)	Duration is now populated on refusals (the better behaviour), but the field's doc reads as success-path accounting and nothing pins it; if err == nil { rep.Duration = … } survives the suite.	fixed	Duration's doc: "set on every return, a refusal included, so a refusal that waited out a lock bound reports the wait"; docs/invalid-index-recovery.md says the same. TestDropAbandonedIndexRefusesOtherTableLeftover and TestRebuildAbandonedIndexRefusesOtherTableLeftover now assert rep.Duration is positive on the refusal. Mutation check: restoring the success-only assignment fails the drop test with "a refusal reports what it spent".
—	Note: the pool guard's justification is demonstrated — buildMinConns on the rebuild degrades TestRebuildAbandonedIndexRefusesPoolWithoutRoomForItsSession to a hang, not a failure.	no action	Confirmed the same shape on the drop side while verifying #2 above: a floor of 1 makes TestDropAbandonedIndexRefusesSingleConnectionPool self-wait until the test timeout. The guard is fail-closed; the pinning tests hang on drift, as noted.
—	Note: the narrowed LK-2 comment ("the drops run in caller-owned mode") is accurate because buildIndexConcurrently carries its own guard.	no action	Agreed; unchanged.
—	Context: the drop-only path ships with no caller; shared-budget accounting over multiple quarantined entries and the not-droppable skip are covered only through the rebuild.	no action	Acknowledged. Both paths are one function today; the consumer PR in block/schemabot is the first caller.

Approving review (review)

#	Concern	Status	Explanation
1 (nit)	DropAbandonedIndex passes buildMinConns; a dropRecoveryMinConns = 2 with its own rationale would decouple the two floors.	fixed	Same change as part 1 #2.
2 (smaller)	The admission error lost its role breakdown; the roles differ per entry point so they must come from the caller.	fixed	Same change as part 1 #3 — each entry point supplies its sessions.
—	mergeStateStatus is BEHIND; needs a main update before merge.	pending	Branch is still BEHIND; a git merge origin/main (no rebase — a human has approved) is the remaining step before merge.