progress: strategy-wide machine-readable execution progress - #41
Merged
Conversation
Callers (CLI today, the SchemaBot adapter next) need a machine-readable view of a running change: phase, sequence step position, retry attempt, and live pg_stat_progress_create_index counters for concurrent builds. Native operations leave the copy counters empty so copy-and-swap can implement the same contract later.
…s API Split the tracker's one RWMutex into a memory-state lock and a poll lock: concurrent Progress() calls previously shared the reserved pgx connection under RLock (a pgx.Conn is not safe for concurrent use), and a slow poll could gate the executor's own state updates. Budget corroboration now reads the same injected clock that produced its start instant, matching the step report. Adds the missing test coverage for all three *WithProgress entry points, the retry-attempt observer wiring, the server-progress merge branches, and the failing-build-under-polling session handoff.
Kiran01bm
marked this pull request as ready for review
August 17, 2026 10:24
Kiran01bm
requested review from
JashLal,
aparajon,
eeSeeGee,
jayjanssen,
jemiahw and
morgo
as code owners
August 17, 2026 10:24
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
morgo
approved these changes
Aug 17, 2026
morgo
left a comment
Collaborator
There was a problem hiding this comment.
🤖 Approved on Morgan's behalf (agent review, liberal pg-sprite bar).
Verified:
- The progress path cannot interfere with the schema change: the only query it ever issues is a parameterized SELECT against
pg_stat_progress_create_index(stat view, no locks, no transaction), riding the executor's already-reserved verdict session rather than the pool — polling can't starve the build. Optimistic/brief/validate progress is pure in-memory state. - Verdict-session handoff is race-free (checked against head sources):
StopConcurrentBuilddrains in-flight polls afterconn.Execreturns and before either verdict path touches the connection; lock order is consistentlypollMu → mu;Snapshotis a value copy under RLock with a freshWorkalloc per poll — no torn reads. The polled PID belongs to the executor's own live build connection, soWHERE pid = $1can't match a recycled foreign backend. - No leaks: snapshot JSON is phase/step/elapsed/attempt/counters only — no SQL, DSN, or even table/index names. Nil-tracker paths reduce exactly to the old code, and the −16 lines are doc updates plus mechanical body-moves.
Non-blocking nits:
- SAFETY.md says "a slow or hung observation can never gate the executor's own state updates" — true as scoped, but
StopConcurrentBuildtakespollMuto drain an in-flight poll running under the poller's context, so a poller with a non-cancellable ctx on a wedged connection delays the executor's verdict handoff (bounded in practice by the pool's baseline statement_timeout). The drain is the right call; the doc should just say the verdict handoff IS observer-gated. RunSequenceWithProgressseedsStart(len(steps), OperationBrief)— between Start and the first StartStep, polls reportstep: 0/operation: "brief"even when step 1 is a concurrent build, and an admission-time failure leaves afailedsnapshot carrying the placeholder. Consider an "admitting" operation for that window.- Small reset asymmetries in
Tracker:Startdoesn't resetstep, andStartStep/FinishzerobuildPIDbut leavesessionnon-nil. Harmless today (every build path goes throughStopConcurrentBuild), but a reused tracker would briefly show the prior run's step, and the retained session pointer is a footgun for future edits.
Collaborator
Author
|
Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp, Claude Opus 4.5) All three non-blocking nits are fixed in the follow-up commit; the verified sections needed no action.
|
Closes the remaining PR #41 review findings: snapshots carry format_version with a key-pinning contract test and docs page; terminal snapshots freeze elapsed at Finish; the tracker fully resets between runs and drops the build session on step/finish; sequences report an "admitting" operation before step 1; nil-tracker guards return ErrInvariantViolation. pkg/progress is reclassified as core in SAFETY.md — the executors import it — and depguard now mechanically enforces the recorded core dependency list the docs already claimed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
pkg/progress, a strategy-wide machine-readable progress contract, and wires it into the three executor entry points. A caller (the CLI today, an orchestrator adapter next) can now observe a running change — phase, sequence step position, retry attempt, and live server counters for concurrent index builds — instead of staring at a blocking call.What
pkg/progress.Tracker: caller-owned, concurrency-safe, no goroutines, injected clock. Snapshots carry phase, step/total, elapsed, and operation detail.ExecuteNativeWithProgress,RunSequenceWithProgress, andBuildIndexConcurrentlyWithProgressvariants; the existing entry points are unchanged.pg_stat_progress_create_indexon demand — polling lifetime is exactly the caller's context.rows/bytes/blocks/tuples) exist in the contract but stay empty for native operations; copy-and-swap implements the same contract later rather than a second one.Why
Progress reporting is the last adapter-facing surface the native path needs: an orchestrator has to distinguish "still building the index, 60% of blocks scanned" from "stuck" without parsing logs. Making the tracker caller-owned keeps the safety-critical core free of goroutines and timers.
Before / after