progress: detach and bound the build progress read from its observer

[Kiran01bm]

A progress poll whose caller gives up mid-read no longer destroys the session the index build's failure verdict runs on, and a poller no longer waits forever behind a stalled poll.

Why

Tracker.Progress read a concurrent index build's progress on the executor's reserved verdict session under the observer's own context. pgx closes a connection whose query's context ends mid-read, so an orchestrator's poll request timing out killed that session; a build that then failed got an indeterminate InvalidIndexError instead of a classified one, which blocks automated recovery. Every observer also waited on the poll gate without limit, so one stalled poll hung every other poller (LK-2). CancelBuild already avoids both hazards; Progress did not.

What

• The build's progress read runs under a context detached from the observer and bounded by progressReadTimeout (twice dbconn.DefaultStatementTimeout, one minute), so on a pkg/dbconn pool with the default or a lower statement_timeout the server ends a slow read with a query cancellation and the session survives; the client deadline only backstops a network that has stopped answering. A pool configured above that bound, or one with no session statement_timeout, reaches the client deadline first. An observer whose context ends mid-read waits for that read to return, so Progress can outlast its context by up to a minute; the godoc and progress report say so.
• Observers wait for the poll gate only as long as their context lasts. One that gives up, or whose context has already ended when it takes the gate, gets the last-known snapshot and the new progress.ErrProgressNotRead wrapped with its context's error, having read nothing; a deadline error without the sentinel came from the read itself (the reserved session stopped answering). The handoff fences still wait unconditionally, so the executor's verdict never depends on an observer.
• A WorkSource is still read under the observer's context, as its contract says.
• LK-2 text (its own paragraph with an Enforced: pointer to pkg/progress and the tests), SAFETY.md, the progress report's polling semantics, and the low-level design's polling pointer updated (upholds LK-2 and the reserved-session rationale).
• Tests pin an ended context reading nothing, the read deadline outlasting dbconn.DefaultStatementTimeout, and the WorkSource read under the observer's context.

Exported API changes

• progress.ErrProgressNotRead — new sentinel, wrapped together with the caller's context error when Progress issued no read.

Before / after

An orchestrator polls progress with a 1-second request timeout while CREATE UNIQUE INDEX CONCURRENTLY idx_email ON users (email) runs, and the build later fails on duplicate emails. The request times out while its progress read is in flight. Only what happens to the reserved session differs.

Before                                     After

the progress read runs under the           the progress read runs under its own
request's 1-second context                 bounded context, detached from the
                                           request
        │                                          │
        ▼                                          ▼
the request times out mid-read:            the request times out mid-read:
pgx closes the reserved verdict            the read still finishes, and the
session                                    reserved session stays open
        │                                          │
        ▼                                          ▼
the build fails on duplicate emails        the build fails on duplicate emails
        │                                          │
        ▼                                          ▼
the failure verdict runs on a dead         the failure verdict runs on the live
session: an indeterminate                  session: invalid-index-own-leftover,
InvalidIndexError, so automated            so automated recovery can drop and
recovery cannot act                        rebuild the leftover

A second poller arriving while the first poll is stalled: before, it waited forever for the poll gate; after, it returns its own context's error at its deadline, with the last-known snapshot.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-17T06:05:12.509129Z	3c6aa67	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

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

[aparajon]

🤖 Review 1/2 of 0593cf4 (adversarial correctness): 0 blocking, 3 non-blocking.

I read the PR's own change against its merge base (48f317a): Tracker.Progress and its new readState split, the new gate wait, progressReadTimeout, the executor integration test, the two unit tests, and the LK-2 / SAFETY.md / progress-report.md text. The head is the PR's commit with main merged in; the PR's own diff is unchanged by the merge. go build ./... and go test -race ./pkg/progress/ are green, as is the executor integration test (TestProgressReadCancelledMidQueryLeavesTheVerdictSessionUsable, 3.1s) and the TestBuildIndexConcurrently* / TestRunSequence* tests around it on PostgreSQL 16. CI is green at this head.

The fix is correct for the failure it names. The build's progress read now runs under context.WithoutCancel(ctx) with its own deadline (progress.go#L570), so an observer's context ending mid-read no longer makes pgx close the reserved verdict session, and the integration test shows it on a real server: a held progress query, the observer cancelled mid-read, the same backend still answering afterwards. Observers now give up on the poll gate with their context (L551), while the four handoff fences still wait unconditionally (takePollGate); that is the right split, because the fences' wait is now bounded by the read's own deadline instead of by an observer. I looked for a way the detached read could outlive its owner or deadlock a fence and found none: the gate is held for exactly the read, the read is bounded, and defer cancel() releases the timer. LK-2: upholds (extends the "observers must not disturb the verdict session" reasoning from CancelBuild to Progress). Nothing here fails open, and no verdict can be wrongly clean because of this change.

Non-blocking

1. A caller whose context ended before it called Progress still gets a database read about half the time

Progress takes the gate with a two-armed select (takePollGateOrGiveUp). When the context is already done and the gate is free, both arms are ready and Go picks one at random, so about half the calls take the gate and go on to run a full progress read on the reserved session, detached, for a caller that has already given up. CancelBuild closes exactly this window with an explicit ctx.Err() check once it holds the gate (L398, with the comment "this is the last point at which nothing has been sent"); Progress has no equivalent. The docs promise the other behaviour: "one that gives up first gets the last-known snapshot and its context's error, having read nothing" (progress-report.md#L205). The cost is small (one cheap read) but it is a read the observer asked not to wait for, now immune to cancellation, and it makes the "nothing was read" contract depend on a coin flip. Fix: after the gate is taken, check ctx.Err() and return the last-known snapshot with it, as CancelBuild does (four lines; I applied it locally and the new test and the existing package tests pass).

Test that fails on 0593cf4 and passes with the fix

// A caller whose context ended before it called Progress has given up: it
// must read nothing, like a caller that gives up while waiting for the gate.
func TestProgressWithAnEndedContextReadsNothing(t *testing.T) {
	var reads atomic.Int32
	session := fakeSession{query: func(context.Context, string, ...any) pgx.Row {
		reads.Add(1)
		return fakeRow{scan: func(...any) error { return pgx.ErrNoRows }}
	}}
	tracker := runningTrackerWithBuild(t, session)
	ended, cancel := context.WithCancel(t.Context())
	cancel()
	for range 200 {
		_, _ = tracker.Progress(ended)
	}
	assert.Zero(t, reads.Load(), "reads issued for a caller whose context had already ended")
}

--- FAIL: TestProgressWithAnEndedContextReadsNothing on 0593cf4 (Should be zero, but was 102); passes with the ctx.Err() check after the gate is taken.

2. The property the constant exists for, "the client deadline outlasts the server's statement_timeout", is not pinned, and holds only for pools built by dbconn with the default or a lower timeout

progressReadTimeout is 2 * dbconn.DefaultStatementTimeout, and its comment says why the factor matters: a slow read must end at the server with a query cancellation, because a client deadline firing mid-query closes the connection. Nothing tests that. Halving the constant to DefaultStatementTimeout / 2 (so the client deadline fires first and closes the verdict session, the hazard this PR removes) passes the unit tests and the integration test, because the integration test's read lasts two seconds and the unit test only checks that a deadline exists.

Separately, the premise is about the pool, which the tracker cannot see. A dbconn pool built with Config.StatementTimeout above one minute, or a caller-supplied pgxpool with no session statement_timeout (the executors accept any *pgxpool.Pool), gives the reserved session a server bound above the client backstop, and a read that stalls for a minute goes back to closing the session. That is latent: it needs a custom timeout and a read that takes over a minute on a view that takes no locks, and the failure is the one that already existed (an indeterminate, fail-closed verdict), so it does not rise to blocking. The comment already says "a pool configured with a statement_timeout above this bound reaches the deadline first"; the PR body and progress-report.md say "twice the pool's default" without that caveat. Cheap fixes, either of which is enough: pin the relation with the test below, and state the caveat where an importer reads it.

Test that fails when the constant is halved and passes at 0593cf4

// The detached read's deadline must outlast the server-side statement_timeout
// the pool gives the reserved session, so the server ends a slow read and the
// client deadline never closes the connection first.
func TestProgressBuildReadDeadlineOutlastsTheServerStatementTimeout(t *testing.T) {
	var remaining time.Duration
	session := fakeSession{query: func(ctx context.Context, _ string, _ ...any) pgx.Row {
		deadline, ok := ctx.Deadline()
		require.True(t, ok)
		remaining = time.Until(deadline)
		return fakeRow{scan: func(...any) error { return pgx.ErrNoRows }}
	}}
	tracker := runningTrackerWithBuild(t, session)
	_, err := tracker.Progress(t.Context())
	require.NoError(t, err)
	assert.Greater(t, remaining, dbconn.DefaultStatementTimeout)
}

Passes on 0593cf4; --- FAIL: TestProgressBuildReadDeadlineOutlastsTheServerStatementTimeout with the constant set to dbconn.DefaultStatementTimeout / 2.

3. "A WorkSource is still read under the observer's context" is stated in the code, the docs and the PR, and no test pins it

The PR deliberately splits the two poll targets: the build read is detached, the engine's WorkSource keeps the observer's context so a cancelled observer does not keep a source poll alive (the WorkSource contract in work_source.go requires Work to honour ctx, and progress-report.md repeats it at L213). Detaching the source read too, one token at L561 (observeEngineWork(context.WithoutCancel(ctx), …)), passes every test in pkg/progress: the new detached-read test covers only the build path, and the existing source tests never cancel the observer. Because the source read is unbounded by design (its own obligation to bound itself), that mutation would turn every observer timeout into a stalled poll holding the gate for the source's whole read. The test below kills it.

Test that fails when the source read is detached and passes at 0593cf4

// A WorkSource is the engine's own read, not the reserved session, so it is
// read under the observer's context.
func TestProgressReadsTheWorkSourceUnderTheObserversContext(t *testing.T) {
	observer, giveUp := context.WithCancel(t.Context())
	var sawEndedContext atomic.Bool
	tracker := runningTrackerWithSource(t, fakeSource{work: func(ctx context.Context) (progress.Work, error) {
		giveUp()
		sawEndedContext.Store(ctx.Err() != nil)
		return progress.Work{}, nil
	}})
	_, err := tracker.Progress(observer)
	require.NoError(t, err)
	assert.True(t, sawEndedContext.Load(), "the work source must be read under the observer's context")
}

Passes on 0593cf4; --- FAIL: TestProgressReadsTheWorkSourceUnderTheObserversContext with the source read wrapped in context.WithoutCancel.

Verified

• Every production line of the change was perturbed and the PR's own tests run. Unit = go test -race ./pkg/progress/; integration = TestProgressReadCancelledMidQueryLeavesTheVerdictSessionUsable on PostgreSQL 16.

Mutant	Caught by
Build read under the observer's ctx instead of the detached one	TestProgressRunsTheBuildReadDetachedFromTheObserver and the integration test
WithTimeout(ctx, …) (bounded but still cancellable)	the same two
Detached read with no deadline	TestProgressRunsTheBuildReadDetachedFromTheObserver
Observer waits for the gate unconditionally (takePollGate)	TestProgressGivesUpWaitingForThePollGateWhenTheObserverEnds (the second poller reaches the session and the fake panics)
Give-up path returns a nil error	TestProgressGivesUpWaitingForThePollGateWhenTheObserverEnds
Give-up path returns a zero snapshot instead of the last-known state	TestProgressGivesUpWaitingForThePollGateWhenTheObserverEnds
progressReadTimeout halved	survives (unit and integration): finding 2
progressReadTimeout set to one hour	survives (unit only): finding 2
WorkSource read detached from the observer	survives: finding 3

• Both new unit tests and the integration test are real tests of the behaviour: the integration test holds the read executing on the server (wait_event = 'PgSleep') before it cancels the observer, and asserts the backend PID is the same afterwards. A detached read that did not hold the session would fail it.
• The handoff fences wait only on a bounded poll now: build reads end at progressReadTimeout at the latest, source reads under their contract. I found no path where the executor's verdict waits on an observer.
• The docs agree with the code on the three points the PR changes: gate wait bounded by the observer, build read detached and bounded, source read under the observer. I found no remaining "polling lifetime is the caller's context" wording in docs/ or README.md.
• Probes and mutations were run in a scratch worktree and restored; nothing was left behind.

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

[aparajon]

🤖 Review 2/2 of 0593cf4 (OSS adoption and integration ease): 0 blocking, 3 non-blocking.

Through the two lenses this is a good change, and its documentation is better than most: progress-report.md states the new waiting and detachment rules in the importer's terms (an HTTP request timing out, a poller giving up), SAFETY.md and LK-2 carry the same reasoning, and the PR body's before/after traces one concrete poll through both versions. For an orchestrator embedding Tracker.Progress the headline behaviour is the right one: a request timeout no longer costs the build its classified failure verdict. The three notes are about what an importer can and cannot tell from the surface the PR changes. No signature changes; the exported contract moved in two ways (what the error means, and how long a call may take), and both are only half-stated.

Non-blocking

1. Two different "the deadline passed" outcomes from Progress are indistinguishable by errors.Is

Progress can now return a context-deadline error for two unrelated reasons. When the observer gives up waiting for the gate it returns its own context's error bare (progress.go#L551): nothing was read, and the observer's timeout is the cause. When the detached read hits its own deadline (L570), the returned error is the wrapped pgx timeout, and that wraps context.DeadlineExceeded as well: I ran a plain pgx query under a 200ms context against a held pg_sleep and got timeout: context deadline exceeded, errors.Is(err, context.DeadlineExceeded) == true. In the second case the observer's context may still be alive, and the cause is a reserved session that stopped answering for a full minute, which is the case an operator needs to hear about because the build's verdict is about to run on that session. An adapter that maps errors.Is(err, context.DeadlineExceeded) to "my own request timed out, benign" will file a stalled verdict session under the same label. CancelBuild already set the precedent for this seam: ErrCancelNotDispatched is wrapped together with the context error so a caller can tell "never sent" from "may have been sent" (L382). The same shape here (a sentinel wrapped beside the observer's context error for the gate give-up, or a statement in progress-report.md that a caller decides by checking its own ctx.Err() first) lets SchemaBot and other importers route the two outcomes without reading error text. The doc text at progress-report.md#L205 covers the first outcome and says nothing about the second.

2. Progress is no longer bounded by its ctx, and the bound that replaces it is an unexported constant no importer can read

Before, Progress returned when its context ended (at the cost of the session). Now an observer whose context ends mid-read waits for the read: up to the server's statement_timeout (30s on a default dbconn pool), and up to progressReadTimeout (60s) at the latest when the network stops answering. That is a deliberate and sound trade, and the docs say the read "finishes on its own terms" (progress-report.md#L212), but an embedder who fronts Progress with an RPC handler or an HTTP request will see that handler overrun its own deadline by up to a minute with a stalled session, and nothing the importer reads says how long. The godoc names the bound by an unexported identifier (L548, "within progressReadTimeout at the latest"), and the docs give no number. An importer that needs a hard return at its own deadline has to run Progress in its own goroutine, which the tracker's "no goroutines" design leaves to them; one sentence saying so, with the figure ("twice dbconn.DefaultStatementTimeout, one minute"), turns a surprise into a design input. Exporting the constant would be the heavier option; the sentence is enough. low-level-design.md#L852 ("poll Tracker.Progress(ctx)") is where an embedder first reads about polling and could carry the pointer.

3. The new LK-2 text is spliced into the middle of the CIC paragraph and is not in the entry's Enforced: line

The sentence at invariants.md#L487 lands between the caller-owned-build stop path and "VALIDATE CONSTRAINT is different in kind" (L490), so the contrast now reads as "different in kind" from the progress-read sentence instead of from the concurrent-build cancellation discussion it was written against, and one line grew to over 110 columns. More to the point for the registry: LK-2's Enforced: line (L494) lists the executors, Cutover, the copier and the checksum, and does not name pkg/progress Tracker.Progress or the two tests this PR adds, so the new claim has no enforcement pointer for a later reader to check ("is this still where the rule is enforced"). The rule is about the verdict session, which is also LK-5-adjacent territory (the ownership proof the verdict feeds); either home is defensible, but the sentence should either move to the end of the CIC paragraph with its own pointer in Enforced:, or be left in the prose and pointed to from there. The // INV: LK-2 comment at progress.go#L566 is the code-side half and is fine.

Verified

• OSS lens: nothing organization-specific in the diff, code, docs or test names; the example in the PR body and progress-report.md is generic (an HTTP request timing out). The CLI path is untouched. Terminology follows the repo's own docs.
• Integration lens: no exported signature changed; Snapshot/JSON are untouched; the last-known-snapshot-alongside-error contract is preserved on the new give-up path (the existing test and the PR's own pin it). The adapter-visible changes are the two noted above (error meaning, call duration); neither can mis-state a safety verdict.
• Docs and code agree on the three points the PR changes (gate wait bounded by the observer, build read detached and bounded, source read under the observer). SAFETY.md and progress-report.md were updated together, and no stale "polling lifetime is the caller's context" wording remains in docs/ or README.md.
• CI at this head: build, lint, unit and docs guards, PostgreSQL 14 to 18, Supabase, smoke, aws-boundary, Semgrep, zizmor and DCO all pass.

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

[aparajon]

🤖 Stamping with comments: 0 blocking, 6 non-blocking (see the two review comments above).

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

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/199, follow-up commit

6 non-blocking findings across two reviews of 0593cf4: all 6 fixed.

#	Finding	Status	Explanation
C1-F1	A caller whose context had already ended still gets a read about half the time	Fixed	Progress checks ctx.Err() once it holds the gate, as CancelBuild does, and returns the last-known snapshot without reading. Added TestProgressWithAnEndedContextReadsNothing from the review; it fails without the check.
C1-F2	"The client deadline outlasts the server's statement_timeout" is not pinned, and holds only for some pools	Fixed	Added TestProgressBuildReadDeadlineOutlastsTheServerStatementTimeout; it fails with the constant halved. The constant's comment and progress-report.md now give the caveat: the margin holds for a pkg/dbconn pool at or below the default, and a pool configured above it, or one with no session statement_timeout, reaches the client deadline first.
C1-F3	"A WorkSource is still read under the observer's context" is not pinned	Fixed	Added TestProgressReadsTheWorkSourceUnderTheObserversContext; it fails with the source read wrapped in context.WithoutCancel.
C2-F1	Two different "deadline passed" outcomes are indistinguishable by errors.Is	Fixed	Following ErrCancelNotDispatched, both give-up paths return the new progress.ErrProgressNotRead wrapped with the caller's context error. The sentinel's doc and progress-report.md say to test for it first: a deadline error without it came from the read itself, meaning the reserved session stopped answering.
C2-F2	Progress is no longer bounded by its ctx, and the replacement bound is an unexported constant	Fixed	The godoc and progress-report.md now give the figure: within the pool's statement_timeout on a pkg/dbconn pool, and within twice dbconn.DefaultStatementTimeout (one minute) at the latest. They also say a caller needing a hard return calls Progress from its own goroutine. low-level-design.md's polling sentence points there.
C2-F3	The LK-2 sentence is spliced into the CIC paragraph and is missing from Enforced:	Fixed	The CIC paragraph reads as before ("VALIDATE CONSTRAINT is different in kind" follows the cancellation discussion again). The progress-read rule now has its own paragraph with an Enforced: pointer to Tracker.Progress, progressReadTimeout and the three tests that pin it.

Source: 6230643805, 6230644295, 6230491121 (automated run, no review) at head 0593cf4; fixes in the follow-up commit.