[theluckystrike]

Description

A snapshot that never records completion leaves its row in pgstream.snapshot_requests as requested or in progress. That happens after a kill, an OOM or a lost store connection. With repeatable_snapshots on, every later run of the same snapshot then fails on the insert with duplicate key value violates unique constraint "schema_table_status_hash_unique_index". The only way out was to delete the row by hand.

This change handles that conflict in the snapshot recorder.

• With repeatable_snapshots on, the recorder logs a warning and reuses the leftover row. It marks the row in progress and the run goes on as normal.
• With it off, the error now names pgstream.snapshot_requests and the schema, and says how to clear the row. The original error is still wrapped.

There's no migration and no index change. The mode gap in the unique index (point 3 in the issue) isn't part of this PR, since it needs its own change.

Related issue

Fixes #1222. The issue proposes three fixes, and this PR does the first two.

Type of change

Bug fix (non-breaking change that fixes an issue)

Changes made

• pkg/snapshot/generator/snapshot_generator_recorder.go: markSnapshotInProgress now handles ErrConstraintViolation from CreateSnapshotRequest.
• pkg/snapshot/generator/snapshot_generator_recorder_test.go: two new tests. One gives the recorder a store that returns the duplicate key error on create, with repeatable snapshots on. It checks that the snapshot runs and the row goes to in progress and then completed. The other checks the error text with repeatable snapshots off.

Testing

• Unit tests added/updated
• Integration tests added/updated
• Manual testing performed
• All existing tests pass

Both new tests fail on main and pass here.

go test -count=1 -run 'TestSnapshotRecorder_CreateSnapshot_(repeatableReusesStaleRequest|nonRepeatableConflictIsActionable)' -v ./pkg/snapshot/generator/

main:
  Error: Received unexpected error:
         error creating snapshot request: constraint violation: duplicate key value violates unique constraint "schema_table_status_hash_unique_index"
  --- FAIL: TestSnapshotRecorder_CreateSnapshot_repeatableReusesStaleRequest (0.00s)
  Error: Error "error creating snapshot request: constraint violation: duplicate key value violates unique constraint \"schema_table_status_hash_unique_index\"" does not contain "pgstream.snapshot_requests"
  --- FAIL: TestSnapshotRecorder_CreateSnapshot_nonRepeatableConflictIsActionable (0.00s)

this branch:
  --- PASS: TestSnapshotRecorder_CreateSnapshot_nonRepeatableConflictIsActionable (0.00s)
  --- PASS: TestSnapshotRecorder_CreateSnapshot_repeatableReusesStaleRequest (0.00s)
  PASS
  ok  	github.com/xataio/pgstream/pkg/snapshot/generator	1.683s

I also followed the steps in the issue by hand, with real pgstream snapshot runs against a local PostgreSQL 16.13 server. The setup was pgstream snapshot with repeatable_snapshots: true, mode data, and public.t1 with 3,000,000 rows. Run 1 was killed with kill -9 once its request row was in progress and the target had rows. Run 2 was the same snapshot into the truncated target.

binary built from main:
  run 1 killed, row public {t1} "in progress", target 3000 rows
  run 2 exit 1
  Error: error creating snapshot request: constraint violation: duplicate key value violates unique constraint "schema_table_status_hash_unique_index"
  row stays "in progress", target 0 rows

binary built from this branch:
  run 1 killed, row public {t1} "in progress", target 21000 rows
  run 2 exit 0
  WRN reusing non-completed snapshot request left by a previous run mode=data schema=public
  row ends "completed" with no errors, target 3000000 rows

A leftover requested row behaves the same way. A repeatable run after that adds a new completed row, as before.

Other checks on this commit: TZ=UTC make test ok for all 56 packages, make generate with no diff, golangci-lint run (v2.10.1) 0 issues, and the license header check 495 ok. I didn't run make integration-test, because Docker isn't available on my machine.

Checklist

• Code follows project style guidelines
• Self-review completed
• Code is well-commented
• Documentation updated where necessary

Additional notes

This touches pkg/snapshot/, so the approval agent policy sends it to a human reviewer.

Update after review (1fdc11b)

• The recorder holds a session advisory lock per request for the whole run (Store.LockSnapshotRequests), keyed like the unique index (schema name and table names hash, no mode). A second run on the same scope fails with ErrSnapshotRequestLocked and does not touch the row or the target. The server frees the lock when the holder's connection ends, so only a request whose run has ended is reused.
• The reuse is Store.ResumeSnapshotRequest. It matches the index key whatever the mode, writes the new mode, and reports whether a row matched. If no row matched (the old row completed after the failed insert), the recorder inserts again.
• Only a unique violation on schema_table_status_hash_unique_index is treated as a stale request (ErrNonCompletedRequestExists). Other constraint errors are returned as before.
• New integration tests in pkg/snapshot/store/postgres/integration/pg_snapshot_store_recovery_integration_test.go: lock exclusivity across modes, lock freed after the holder's backend ends, reuse across modes with the completion update, and two retries started together while a slow run copies, then recovery of a dead run. They pass with -race -count=3 against a local PostgreSQL 16. TZ=UTC make test ok for 56 packages, golangci-lint v2.10.1 0 issues.

[github-actions]

Coverage

Total: 63.3% (±0.0% vs main)

Coverage in packages changed by this PR:

Package	Coverage	Δ
pkg/snapshot/generator	95.6%	+0.1%

[AasheeshLikePanner]

a duplicate row does not prove the old writer stopped so what happens if the old run is just slow or two retries start together they can reuse the same row and both keep writing then one can mark it completed while the other is still copying and a late completion can update a newer request too

there is also a gap between the failed insert and the update where the old row could complete and the update would touch nothing but still report success the mode mismatch has the same problem since the index ignores mode but the update requires it and we are catching every constraint error as if it was this duplicate

manual test kills the old process and truncates the target so it does not cover these cases or retrying without clearing the target first can we check that the old writer cannot interfere before reusing the row and test slow writers two retries together and late completion against real postgres

[theluckystrike]

Thanks @AasheeshLikePanner, you were right on all three, and I reproduced each one against PostgreSQL 16 on the previous commit f4dc1db. A retry started while a slow run was still copying went ahead, and its completion marked the shared row completed before the slow run finished. A leftover schema-only row made a data run report success while the row stayed `requested`, because `UpdateSnapshotRequest` keys on mode and matched nothing. Any class 23 error was also taken for the duplicate. 1fdc11b changes this. For the old writer question, the recorder now holds a session advisory lock for the whole run, keyed like the unique index on schema and table names hash without mode. A second run on the same scope fails with `ErrSnapshotRequestLocked` before it touches the row or the target. The server frees that lock when the holder's connection ends, so only a run that has ended can be taken over, and two retries can't both get it. A late completion can't land on a newer request either, since the newer run can't start while the old one holds the lock. On the gap and the mode mismatch, the take over is now `ResumeSnapshotRequest`, which matches the index key whatever the mode, writes the new mode and reports whether a row matched. If none matched because the old row completed in between, the recorder inserts again. Only a unique violation on `schema_table_status_hash_unique_index` counts as the stale row now, and other constraint errors are returned as before. The new integration tests in `pg_snapshot_store_recovery_integration_test.go` cover lock exclusivity across modes, the lock freed after `pg_terminate_backend` on its holder, take over across modes plus the completion update, and two retries started together while a slow run copies. Both retries are refused without copying, the slow run's row stays in progress until it completes, and a dead run is then recovered. They pass with `-race -count=3` against a local PostgreSQL 16, and `make test` passes for all 56 packages with golangci-lint at 0 issues. Two limits remain. If the lock connection drops while the writer keeps copying, the server frees the lock early, and a binary from before this change doesn't take the lock at all. Fencing writes that a Kafka or webhook target already accepted is the larger design you sketched in #1222, and it stays out of this PR. Thank you, Michael Lip Founder of https://zovo.one, 108 Chrome extensions with 150,000+ users Also building https://makeroll.com and https://ml0x.com, open-source guardrails for Claude Code agents Open source has to win, and small fixes like this one are my part of it GitHub Sponsors keeps these fixes coming, https://github.com/sponsors/theluckystrike