feat(executor): caller-owned deadline, lockers, and backend PID for concurrent builds

[Kiran01bm]

BuildIndexConcurrently gains a caller-owned deadline mode, the progress tracker exposes the build backend's PID, and progress snapshots report the lockers a concurrent build is waiting on.

Why

A concurrent index build on a large table can run for hours. Today the only bound the executor accepts is a fixed server-side statement_timeout (ConcurrentBudget.Overall), which fits a synchronous attempt but not an orchestrator that keeps a build alive for as long as it can renew a lease. The same orchestrator needs to stop a running build from a second connection, which requires the build backend's PID, and its operators need to see why a build in the "waiting for old snapshots" phase is not moving, which requires the lockers columns of pg_stat_progress_create_index.

What

• ConcurrentBudget.CallerOwned: the session runs with statement_timeout = 0 and the caller's cancellable context is the statement's only bound. Overall must be zero, and a context that cannot be cancelled is refused with ErrCallerOwnedNeedsCancellableContext before any session is acquired, so the statement remains bounded by construction (LK-2). In this mode SQLSTATE 57014 is always ErrCancelledExternally, never a *BudgetError. The bounded mode and every existing caller are unchanged.
• progress.Tracker.BuildPID() returns the active build's backend PID (0 when idle) for pg_cancel_backend.
• dbconn.ConcurrentIndexProgress reads lockers_total, lockers_done, current_locker_pid; the snapshot carries them as work.lockers_total / work.lockers_done and detail.current_locker_pid. format_version bumps to 2 and docs/progress-report.md documents the new fields.
• LK-2 in docs/invariants.md records the caller-owned exception; capability, execution-model and TCB docs no longer describe statement_timeout as the only bound.
• Integration tests on a real server: a caller-owned build completes once its blocker releases; a build cancelled via tracker.BuildPID() returns ErrCancelledExternally with its invalid leftover reported; a blocked build publishes lockers_total ≥ 1 and the blocker's PID as current_locker_pid.

Before / after

Before
  caller ── ConcurrentBudget{Overall: 30m} ──▶ SET statement_timeout = 30m ──▶ CREATE INDEX CONCURRENTLY
           (no way to say "for as long as my lease holds")
  tracker.Progress() ──▶ phase, blocks, tuples            (no lockers, no build PID)

After
  caller ── ConcurrentBudget{CallerOwned: true} + cancellable ctx
        ├─ ctx.Done() == nil ──▶ ErrCallerOwnedNeedsCancellableContext (refused)
        └─ SET statement_timeout = 0 ──▶ CREATE INDEX CONCURRENTLY
             ├─ ctx cancelled        ──▶ context error (+ catalog verdict)
             └─ pg_cancel_backend    ──▶ ErrCancelledExternally (+ catalog verdict)
  tracker.BuildPID() ──▶ backend PID for pg_cancel_backend from a second connection
  tracker.Progress() ──▶ phase, blocks, tuples, lockers_total/done, current_locker_pid

[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, requested by Armand and performed by his agent. Reviewed at head 52091384.

Verdict: caller-owned mode is bounded by construction and the new refusals hold — but the mode's own primary exit path is the one thing not covered: a build cancelled by its caller can report ErrCancelledExternally, which is exactly the distinction a lease-driven orchestrator needs. Four of five mutations died honestly; the fifth is pinned by a nil-pointer panic rather than its assertion. Nothing blocks, and finding 1 is the one worth an answer before a real orchestrator builds on it.

#	Finding	Severity
1	Caller-owned 57014 is unconditionally external, even when the caller cancelled	medium
2	BuildPID() hands out a PID with no validity window, and its doc prescribes the unsafe use	medium
3	The Overall != 0 refusal is pinned only by a nil-pointer panic	low (test)
4	PR body says format_version bumps to 2; it bumps to 3	nit

Findings

1. In caller-owned mode every SQLSTATE 57014 becomes ErrCancelledExternally, including the caller's own cancellation. In bounded mode elapsed >= b.Overall was the discriminator between statement_timeout and a third party's pg_cancel_backend. Caller-owned mode has no discriminator, and asConcurrentBudgetError never receives ctx — so it cannot consult the one signal that is free and exact: ctx.Err() != nil means the caller did this. The common path is fine (pgx usually surfaces context.Canceled, which falls through to the operational wrap), but the server's ErrorResponse can beat pgx's local deadline, and on that side of the race the build reports an external cancellation to the very orchestrator that cancelled it. That inverts the motivation in the PR body — an orchestrator "that keeps a build alive for as long as it can renew a lease" cancels its own context when the lease lapses, and would read the outcome as an operator's intervention. None of the three new integration tests exercises it: they cover caller-owned success, pg_cancel_backend via tracker.BuildPID(), and lockers reporting, so the mode's primary exit is untested as well as ambiguous. Two smaller things ride along: the wrapped message says "caller-owned deadline" where the mode's whole point is that there is no deadline, and ErrCancelledExternally's own doc comment still defines itself as "cancelled before its overall budget elapsed", which no longer describes this case.

2. Tracker.BuildPID() returns a bare PID whose safety window the caller cannot observe, and the doc comment prescribes exactly that use. The clearing is right — Start, StartStep, StopConcurrentBuild and Finish all zero buildPID, and TestBuildPIDLifecycle pins three of the four. But a PID a caller has already read stays live in the caller's hand after the build returns and the connection goes back to the pool; pg_cancel_backend on it then cancels whatever the pool next runs on that backend. The window is narrow and, for a library consumer, unmitigable: there is no generation or epoch to re-check against, and the comment reads as an instruction ("An orchestrator passes this PID to pg_cancel_backend from a second connection") with no mention of the boundary.

3. (test) Deleting the Overall != 0 arm of validate leaves TestBuildIndexConcurrentlyRejectsCallerOwnedOverallBudget red for the wrong reason. The mutant fails via pgxpool dereferencing the nil pool at native.go:320, not via the assertion, because require.Error(t, err) accepts any error at all. The sibling one function down does it properly — require.ErrorIs(..., ErrCallerOwnedNeedsCancellableContext) — and dies honestly under the same treatment. (RejectsUnboundedBudget has the same shape on main, so this is precedent rather than a regression.)

4. (nit) The PR body says format_version "bumps to 2"; the bump is 2 → 3. Code, docs/progress-report.md, and both JSON-shape tests agree on 3 — only the body is stale.

Action items

1. (Finding 1) Pass ctx into asConcurrentBudgetError and, in the CallerOwned arm, return the context error (or a distinct ErrCancelledByCaller) when ctx.Err() != nil, reserving ErrCancelledExternally for a still-live context. Add the missing integration case — caller-owned build, caller cancels its own context, assert the caller-side error and the catalog verdict. While there, drop "deadline" from the caller-owned message and extend ErrCancelledExternally's doc comment to cover the mode.
2. (Finding 2) Either return (uint32, bool) keyed to a build generation the caller can re-check, or expose Tracker.CancelBuild(ctx) that issues the cancel under the same lock that guards buildPID — the tracker is the only place that knows the build is still live. Failing that, say in the comment that the PID is valid only while the build is active and that a stale one can cancel an unrelated statement on a pooled backend.
3. (Finding 3) require.ErrorContains(t, err, "Overall to be zero"). Worth the same treatment on RejectsUnboundedBudget while you are in there (optional).
4. (Finding 4) Fix the "bumps to 2" line in the body.
5. (optional) TestBuildPIDLifecycle covers StopConcurrentBuild, Finish and Start but not StartStep, which also clears the PID — one more line pins the step-boundary case the comment on StartStep promises.

Verified (tried to break these, couldn't)

Mutation results: deleting the ctx.Done() == nil guard is caught by ErrorIs on ErrCallerOwnedNeedsCancellableContext; deleting the CallerOwned arm of asConcurrentBudgetError is caught, and that arm is load-bearing rather than cosmetic — without it elapsed >= b.Overall with Overall == 0 is always true, so every caller-owned cancellation would have surfaced as a nonsense BudgetError{Budget: 0}; deleting the Overall != 0 arm is caught only incidentally (finding 3). The refusal ordering is right: validate() and the ctx.Done() check both run before any session is acquired, so a misconfigured caller-owned build never touches the pool — ctx.Done() is also correctly nil for the shapes that matter (Background, TODO, WithoutCancel, WithValue over a non-cancellable parent), so the guard is not vacuous. buildPID is cleared on all four transitions, so the stale-PID-from-the-tracker attack dissolves and only the caller-held-PID window in finding 2 survives. The statement_timeout = 0 / lock_timeout = 0 pair keeps the CONCURRENTLY exception intact, and the partial-SET recovery path is untouched. The version bump is consistent everywhere it matters: FormatVersion = 3, the doc's "current version is 3" with per-version provenance, the doc's worked example, and both JSON-shape tests; the format_version: 2 examples in cli-output-examples.md are plan reports on their own contract and correctly left alone, and no second progress example exists to drift. current_locker_pid scans through a *int32 and lands as omitempty, so "omitted when none is published" holds against a 0 from the view. go build ./... clean at head; ./pkg/progress/ green (0.37s) and the non-container ./pkg/executor/ unit tests green. I could not run the three new integration tests locally — no Docker on this machine — so their evidence is CI's: all fourteen checks pass, including test (PostgreSQL 14…18), aws-boundary, and the built-artifact smoke test. No prior review findings to fold in; the Codex reviewer left only a usage-limit notice.

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

[aparajon]

🤖 Two-lens product review (adoption + integration), same head 52091384. Shorter than the correctness review above and deliberately separate from it — nothing here is a correctness claim.

Lens 1 — OSS adoption ease

The best thing in this PR is a refusal message. ErrCallerOwnedNeedsCancellableContext says what is wrong and why the tool cares — "with statement_timeout disabled the context is the statement's only bound" — which is the difference between a newcomer fixing their call in thirty seconds and filing an issue. More of the error surface should read like that.

current_locker_pid answers the single most common "why is my build stuck?" question, and a standalone user still can't see it. pkg/progress has no consumer outside pkg/executor — no CLI command renders a snapshot — so the progress report is an adapter contract with no first-party reader. Someone who found this repo, ran a concurrent index build against their own database, and watched it sit in "waiting for old snapshots" has the blocker's PID available in a contract they'd have to write Go to reach. That is a fine place to be for a library, but it means this change improves the integration surface rather than first use, and docs/progress-report.md is currently the whole story. If a pg-sprite progress/status surface is on the roadmap, the lockers columns are the strongest argument for it yet.

The capabilities.md positioning sentence got harder to read, not easier. "the safest known online pattern — bounded server timeouts, or an explicit caller-owned cancellable context for concurrent index builds — or refuses with a typed reason" now has two ors doing different jobs in one sentence, and the line runs well past the file's wrap. The one-paragraph pitch is the highest-traffic prose in the repo; the caller-owned exception is probably better as a clause in the bullet below it than as a qualifier in the headline claim. Same for tcb-model.md, where "caller-owned cancellable contexts where a server statement timeout is explicitly disabled" reads as a loophole in "put a limit on everything" rather than as the different-but-equal bound it actually is — worth one clause saying the refusal is what keeps the rule intact.

Lens 2 — SchemaBot integration

The seam is right to design now, because there is no consumer yet. SchemaBot's pkg/engine/postgres imports pkg/dbconn and the plan report, and does not import pkg/progress at all — so BuildPID() and caller-owned mode have zero callers today. That is the cheapest possible moment to fix the two seams in the correctness review: a generation-keyed PID or a CancelBuild method, and a caller-vs-external cancellation distinction. Both become compatibility problems the moment an orchestrator ships against them.

The version bump is correctly scoped and does not gate anything downstream. SchemaBot's strict-version gate is formatVersion != pgplan.FormatVersion — the plan contract — so bumping the progress contract to 3 cannot trip it. The docs' claim that the report contracts move independently holds in practice, not just on paper; no landing-order coupling here.

What integration will actually need next is a reason it can classify, not just an error it can wrap. Caller-owned mode exists for an orchestrator that renews a lease, and that orchestrator's decision tree is: my lease lapsed (retry later, same plan), an operator stopped it (do not retry, tell the human), the server killed it (retry with a different bound). Today the first two collapse into ErrCancelledExternally and the third is a *BudgetError — so two of the three branches are reachable and one is not distinguishable. Typed sentinels are the right shape and no string parsing is needed anywhere, which is the important part; it is the partition that needs one more member.

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

[aparajon]

🤖 Approving on Armand's behalf after the adversarial correctness review above. The findings there are yours to pick up as follow-ups — flagging them, not gating on them.

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