fix(engine): key PostgreSQL progress to every apply, not one slot - #1130
Open
aparajon wants to merge 1 commit into
Open
fix(engine): key PostgreSQL progress to every apply, not one slot#1130aparajon wants to merge 1 commit into
aparajon wants to merge 1 commit into
Conversation
aparajon
marked this pull request as ready for review
August 26, 2026 02:26
aparajon
requested review from
JashLal,
Kiran01bm,
eeSeeGee,
jayjanssen,
jemiahw and
morgo
as code owners
August 26, 2026 02:26
One PostgreSQL engine serves a target for its whole lifetime, and it held a single progress slot that each accepted apply took over. A second apply on the same target — a lease handover re-driving work while the previous goroutine still runs — evicted the first one's state, so the running apply's own driver read the idle sentinel and was told its work no longer exists. Track every accepted apply under its own identity instead, so a poller always reads its own schema change. Accepting an apply retires the entries that already reached a terminal state, which bounds the map without ever dropping an in-flight change: a terminal entry is kept only so its driver can read the outcome, and a driver that has accepted another apply on this target has moved past it.
aparajon
force-pushed
the
armand/pg-progress-keying
branch
from
August 28, 2026 12:04
648908d to
d942449
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes PostgreSQL engine progress tracking so concurrent applies on the same long-lived engine instance don’t overwrite each other’s in-memory progress, preventing a running apply from incorrectly observing the idle sentinel (“No active schema change”) when another apply is accepted.
Changes:
- Replace the single progress “slot” with a map keyed by apply identity (
ResumeState.MigrationContext) so each apply reads its own progress. - Retire already-terminal progress entries on acceptance of a new apply to bound memory usage without evicting in-flight entries.
- Expand unit tests to cover concurrent applies, terminal-entry retirement, discard behavior for untracked writes, and drain semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/engine/postgres/postgres.go | Changes the engine’s in-memory progress storage from a single value to a per-apply map and updates drain semantics accordingly. |
| pkg/engine/postgres/apply.go | Keys Progress, claimProgress, and publishProgress by apply identity; adds terminal-entry retirement and updates discard behavior. |
| pkg/engine/postgres/apply_test.go | Adds/updates tests to prove per-apply isolation, bounded retirement, discard-on-untracked, and drain behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
One PostgreSQL engine serves a target for its whole lifetime, but it held a single progress slot that each accepted apply took over. A second apply on the same target — a lease handover re-driving work while the previous goroutine is still executing — evicted the first one's state, so the running apply's own driver read the idle sentinel and was told its work no longer exists.
Progress is now tracked per apply identity, so a poller always reads its own schema change. This mirrors how the sharded engine already isolates progress per operation, rather than leaving PostgreSQL as the one engine where a sibling can answer for you.
Accepting an apply retires the entries that already reached a terminal state, which bounds the map without ever dropping an in-flight change: a terminal entry is kept only so its own driver can read the outcome, and a driver that has accepted another apply on this target has moved past it. Running entries are never retired.
Drainstill stops tracking everything, and a lookup miss still returns the verbatim idle message that stale-task recovery compares against.With the eviction gone, the engine can declare
SynchronousWorkRegistration: it starts tracking an accepted apply under the engine mutex beforeApplyreturns and executes the statement in a goroutine of this process, so a pending report about work a driver believes is in flight is conclusive rather than a setup phase to wait out. From then on the only writer that stops tracking a running apply isDrain, which means the drive that accepted the work has given it up.Stacked on #1113, which adds the capability interface.
This PR was written by an agent (Claude Opus 5) on Armand's behalf.