[Kiran01bm]

The drain no longer holds a landed old key's delete back for a moved image whose own chunk is still in flight, so a primary-key move that keeps a unique value can land while the copy is running.

Why

A row's primary key moves to a fresh value: the stream delivers an image under the new key paired with a delete under the old one. The drain's pair rule deferred both halves whenever either was inside an in-flight chunk, so that the flush never deleted the old key's row while the image still needed it to complete an unchanged out-of-line column. When the moved row also carries a unique value and the move goes from a landed key to a key in flight, that rule deadlocks against the copier: the chunk's insert of the row under the new key collides with the shadow row still under the old key, the copier runs the chunk again and waits for the flush to remove that row, and the flush is waiting for the chunk to land before it will delete it. The copier's collision attempts run out and the catch-up fails. docs/invariants.md CO-4 recorded this as open and the key-move convergence test started its load only after the copy had landed.

What

• Buffer.Drain's pair rule is one-directional: a moved image waits while its old key's entry is in flight or invisible, and an old key's entry waits while its image is invisible, but a landed old key's delete flushes ahead of an image whose chunk is in flight. The deferred image stays buffered with its marker; when its chunk lands, CompleteFirst names it, the flush finds the old key's shadow row gone, and completes the image from the source row under its own key instead — the fallback that already served a reused old key, held until the stream passes the read.
• Drain unit tests: the pair waits for an in-flight old key; a landed old key flushes alone as a delete marker with its image deferred and flushed by the next drain; deferral spreading through a shared old key is covered for an invisible image and for an in-flight image separately.
• TestCatchupFlushesALandedOldKeyAheadOfItsInFlightImage holds the new key's chunk in flight by hand, moves a landed row's primary key into it, asserts the old key's shadow row is deleted while the image stays deferred, then lands the chunk and asserts convergence; without the drain change both halves stay deferred until the timeout.
• CO-4 in docs/invariants.md describes the one-directional rule, records the primary-key-move case as enforced, and states the bound that remains: the chunk-level retry still runs out when unique values keep moving into one chunk's range faster than one move per catch-up cycle, each attempt colliding with a different row.

Before / after

seats(id PK, uniq UNIQUE). Row (7, 'A') is copied into the shadow under the landed key 7. The source then moves it: UPDATE seats SET id = 1600 WHERE id = 7, and key 1600 is inside the chunk the copier is reading. The only thing that changes is whether the delete under key 7 waits for chunk 1600; what the flush does with the image once chunk 1600 has landed — complete it from a row, then upsert it — is unchanged.

Before                                             After
──────                                             ─────
stream: image (1600,'A') ← old key 7               stream: image (1600,'A') ← old key 7
        delete marker at 7                                 delete marker at 7

drain:  1600 in flight ──▶ both halves deferred     drain:  1600 in flight ──▶ image 1600 deferred
        (pair waits in either direction)                    7 landed ────────▶ delete 7 flushes now

flush:  nothing written                             flush:  DELETE shadow WHERE id = 7
                                                            (shadow no longer holds uniq 'A')

copier: INSERT (1600,'A') into shadow               copier: INSERT (1600,'A') into shadow
        ✗ 23505: uniq 'A' still held by row 7                ✓ chunk 1600 lands
        wait, run the chunk again …
        ✗ 23505 … × CollisionAttempts               drain:  1600 landed ──▶ image 1600 flushes,
                                                            CompleteFirst names it
        the delete waits for the chunk,
        the chunk waits for the delete              flush:  shadow row 7 is gone → complete the
                                                            image from source row 1600 (D13),
result: "collided with a shadow row on each                 upsert (1600,'A') — shadow converges
        of 10 attempts" — catch-up fails

The other direction is unchanged: when the old key is the one in flight, the image still waits for it, because the chunk that will copy the old key's row has not written anything the delete could collide with, and the flush must not complete the image from a row that chunk has yet to copy.

🤖 Generated with Amp (Claude Opus 4.6)

[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-28T23:03:33.208710Z We couldn’t complete this review. Comment @codex review to try again.	45d0c05	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.

[JashLal]

🤖 Reviewed by Jash's AI agent at 45d0c05f3282d8f28b38088b7c847e2429eb89fa (the full diff against kiran01bm/cs8-copier-unique-retry). No concrete correctness blocker found in the changed code.

The directional dependency in pkg/applier/drain.go:209 addresses the stated wait cycle: an image still waits for its old key, while an image waiting only on its own chunk no longer retains the landed old-key entry. Passing transaction invisibility separately preserves the stronger rule for an invisible image. The changed tests cover both directions, retaining the deferred image's marker/CompleteFirst membership, and the difference between invisibility and a chunk wait when images share an old key.

Non-blocking test suggestion at pkg/applier/catchup_stall_integration_test.go:109: the new test deterministically proves that the old row is deleted before the simulated chunk lands and that the image subsequently converges. A companion test with a real copier insert encountering the unique collision, then retrying after that deletion, would also exercise the cross-component handoff that motivates this change. It would protect the interaction with collision retry and source completion; the current position-switch test does not itself execute that insert/retry. The documented remaining retry bound is appropriately explicit.

Validation: all 16 reported CI checks pass, including PostgreSQL 14–18. I did not run a local build, tests, or a database reproduction. The exact-head contents API failed, so I could not independently inspect unchanged completion/hold helpers, the fixture setup, or repository-specific instructions outside the supplied diff. Those are limits of this review, not verified behavior.

[JashLal]

🤖 Approving on Jash's behalf after the full-diff review at 45d0c05. No concrete blocker found; the optional real-copier collision/retry test and surrounding-source verification limits are recorded in the review. No local build or tests were run. Generated by Codex.