Skip to content

decode, checkpoint, applier: slot lag ceiling and orphan-slot reaper - #156

Merged
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/cs7-lag-reaper
Oct 9, 2026
Merged

Kiran01bm merged 3 commits into
mainfrom
kiran01bm/cs7-lag-reaper

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Why

A replication slot is cluster state: it retains WAL on the volume until it is confirmed or dropped. ST-3 names three guards — a recognizable name prefix, a reaper for slots a run left behind, and a hard ceiling on what a slot may retain. Only the first existed. A run that died after CreateSlot left a slot pinning WAL forever, and a run that stayed alive but fell behind (a long-open transaction on the source, a stalled catch-up) had no bound on how much WAL its slot could keep.

This PR lands the other two guards and the typed lost state ST-4 needs to tell "the server gave up on the slot" from a stream failure.

Before / after

before                                              after
──────                                              ─────
run crashes after CreateSlot                        run crashes after CreateSlot
   │                                                   │
   ▼                                                   ▼
slot pgsprite_xxxx stays, pins WAL                  next engine start → Store.ReapOrphanSlots
   (nothing ever drops it)                             │ 1. list pg_replication_slots
                                                       │    (this database only, engine prefix)
                                                       │ 2. read checkpoint rows for those names
                                                       │    (later statement → newer snapshot)
                                                       ├─ prefix, wrong shape → Foreign (kept)
                                                       ├─ live row            → Live   (kept)
                                                       ├─ listed as active    → Active (kept)
                                                       └─ otherwise
                                                            ├─ re-read row → live now? → Live
                                                            └─ DropIdleSlot (no WAIT)
                                                                 ├─ 55006 held → Active (kept)
                                                                 └─ dropped    → Dropped ✓

Catchup cycle                                       Catchup cycle
  gather → drain → flush → confirm                    gather → drain → flush → confirm
                                                        → InspectSlot(stream.Slot())
  (slot may retain any amount of WAL;                   ├─ not found / lost  → SlotLostError
   nothing checks)                                      ├─ unknown measure   → ErrSlotLagUnknown
                                                        ├─ > SlotLagCeiling  → SlotLagExceededError
                                                        └─ else              → next cycle
                                                      Run ends; lock held, slot in place

What

  • pkg/decode/slot_lag.go (new) — DefaultSlotLagCeiling (1 GiB, D11); SlotStatus.Lost(); SlotStatus.WithinLagCeiling(ceiling): lost → *SlotLostError before any measure, unknown retained → ErrSlotLagUnknown (never read as zero), over → *SlotLagExceededError{Slot, Retained, Ceiling}, ceiling < 1 → ErrInvariantViolation. Sentinels ErrSlotLagCeiling, ErrSlotLost, ErrSlotLagUnknown.
  • pkg/decode/slot_status.go — InspectSlot reads on a server in recovery: pg_wal_lsn_diff(pg_current_wal_lsn(), …) is wrapped in CASE WHEN pg_is_in_recovery() THEN NULL, so a demoted writer reports retained WAL as unknown instead of failing the whole read.
  • pkg/decode/slot.go — SlotNamePrefix, IsEngineSlotName. pkg/decode/stream.go — Stream.Slot().
  • pkg/checkpoint/reap.go (new) — Store.ReapOrphanSlots(ctx, cfg) (Reaped, error). Two reads, in order: pg_replication_slots filtered to database = current_database(), slot_type = 'logical', slot_name LIKE 'pgsprite\_%'; then pgsprite_checkpoint WHERE slot_name = ANY($1) AND phase NOT IN (Done, Failed). pg_replication_slots reads shared memory, not a snapshot, so a single joined statement could list a slot while the row committed before it was still invisible; reading the rows second makes their snapshot newer than the slot list. Each candidate lands in exactly one of Dropped / Live / Active / Foreign. An orphan's row is re-read right before its drop (a run that claimed the slot since the pass began keeps it), then it goes through decode.DropIdleSlot; a 55006 object_in_use refusal files it as Active. Drops stop at the first failure and report what landed. Missing checkpoint table → ErrTableMissing, slots or not.
  • pkg/decode/slot_drop.go — DropIdleSlot: DropSlot without WAIT. A held slot is ErrSlotActive and left in place, publication and all; the verdict is the server's at the drop, not a catalog read before it. DropSlot keeps its waiting behaviour for the route's own cleanup.
  • SlotLagExceededError.Error() renders 1.2 GiB of WAL, over the 1 GiB ceiling (1288490188 > 1073741824 bytes) and names the usual cause (a long-open transaction on the source) and when the run can resume.
  • pkg/applier/catchup.go — CatchupOptions.SlotLagCeiling (0 → default; < 1 refused, so the ceiling is always in force — "cannot be set below one byte", not "cannot be switched off", since math.MaxInt64 is accepted). cycle ends with checkSlot: InspectSlot on the pool by stream.Slot(); not found → SlotLostError{Found: false}; then WithinLagCeiling.
  • Tests: slot_lag_test.go (unit, including the rendered error text — the renderer exception); slot_drop_integration_test.go — DropIdleSlot refused with ErrSlotActive while a walsender holds the slot, context intact, slot and publication left, then dropping both once released; catchup_slot_integration_test.go — an uncommitted 4 MiB insert pins restart_lsn under a 1 MiB ceiling, asserting ErrSlotLagCeiling, the ErrorAs fields, not ErrSlotLost, not ErrTableLockLost, lock intact, slot still present; reap_integration_test.go — orphan dropped and live kept, live row turning Done reaped on the next pass, clean database reaps nothing, held slot reported Active, another database's slot never read, pgsprite_backup reported Foreign, no checkpoint table refused.
  • Docs: docs/invariants.md ST-3 / ST-4 Enforced, plus an ST-3 Open: sentence — a ceiling abort leaves the slot under a non-terminal row the reaper reads as Live, so whoever handles ErrSlotLagCeiling must move the row to a terminal phase (or resume and drain) before the reaper can reclaim it; D11 names ReapOrphanSlots and the pg_drop_replication_slot / DROP PUBLICATION fallback for an operator reclaiming a slot by hand; SAFETY.md applier, checkpoint, slot-lifecycle rows; docs/copy-and-swap-design.md D11 / D12 Where enforced and the pkg/decode / pkg/applier / pkg/checkpoint package-map rows; docs/architecture.md and docs/low-level-design.md move the ceiling off the planned pkg/throttler row.

Decisions to veto

  • Ceiling enforced in applier.Catchup, not deferred to the orchestrator. The catch-up loop is the one live consumer of the slot today and already reads pg_current_wal_lsn() on the pool every cycle; judging the slot there costs one more catalog row read per cycle and means the bound holds without an orchestrator. The orchestrator will call ReapOrphanSlots at startup when it exists; the reaper is a Store method with no caller in this PR.
  • Reaper lives in pkg/checkpoint. "Orphan" is defined by the checkpoint row, and checkpoint already imports decode; the reverse edge would cycle.
  • Active slots are left, not waited out — judged at the drop. A holder is the judge of its slot; waiting under the reaper's context and then dropping would race a run that is about to write its row. The listed active flag is advisory; the server's 55006 at the no-wait drop is the verdict, so a slot that becomes held between the list and the drop is also left.
  • Two reapers on one database are not coordinated yet. The re-read before each drop narrows the window to the drop call itself; closing it fully needs an advisory lock the reaper takes around its drops and a run takes before committing its row, which belongs with the orchestrator that will call the reaper. Until then the doc says to run one reaper per database at a time.
  • Over-the-ceiling leaves the slot and the lock in place. Run ends with the typed error; dropping the slot is a lifecycle decision for the caller (the run may be resumable once the pinning transaction ends). The error carries Retained and Ceiling so an operator can read what happened.
  • Standby branch of InspectSlot has no kept test. The harness has no standby fixture. It was validated by hand against a throwaway pg_basebackup -R standby on PostgreSQL 16: the previous expression fails with recovery is in progress; the new one reads the slot row with retained NULL on the standby and a byte count on the primary. The live "lost mid-stream" transition (ST-4 reconcile) is a separate follow-up.

Verification

  • make lint — 0 issues.
  • SKIP_INTEGRATION=1 go test ./... — ok.
  • go test -count=1 ./pkg/decode/ ./pkg/applier/ ./pkg/checkpoint/ — ok (PostgreSQL 16 testcontainers).
  • scripts/test-flaky.sh TestCatchupEndsWhenTheSlotRetainsMoreThanTheCeiling 8 ./pkg/applier/ — 8/8.
  • scripts/test-flaky.sh TestReapOrphanSlotsLeavesHeldSlotsAndOtherDatabasesAlone 8 ./pkg/checkpoint/ — 8/8.
  • scripts/test-flaky.sh TestDropIdleSlotRefusesAHeldSlotWithoutWaiting 8 ./pkg/decode/ — 8/8.
  • Manual standby check as described above; containers removed afterwards.

A replication slot is cluster state that retains WAL on the volume until
it is confirmed or dropped. Two of ST-3's three guards were still planned:
nothing bounded what a slot could retain, and nothing cleaned up a slot a
run left behind.

pkg/decode judges a slot: SlotStatus.WithinLagCeiling returns nil under
the ceiling, SlotLostError for a slot the server reports lost (before any
measure is read), ErrSlotLagUnknown for a retained amount the server could
not measure (never read as nothing retained), and SlotLagExceededError
carrying both figures over it. A ceiling below one byte is an invariant
violation. DefaultSlotLagCeiling is one GiB (D11). InspectSlot now reads
on a server in recovery, reporting retained WAL unknown instead of failing
on pg_current_wal_lsn(). SlotNamePrefix and IsEngineSlotName tell the
engine's name shape from a name merely wearing its prefix; Stream.Slot
names the slot a stream decodes.

pkg/checkpoint owns the reaper, because the checkpoint row is what makes a
slot owned: Store.ReapOrphanSlots reads the logical, engine-prefixed slots
of current_database() in one statement with the non-terminal rows that
name them, drops each slot no live row names and no process holds through
decode.DropSlot, publication and all, and reports the rest as Live, Active,
or Foreign without touching them. Slots of other databases are never read.
A database without the checkpoint table is refused with ErrTableMissing
rather than read as all orphans.

pkg/applier enforces the ceiling where the slot is consumed: Catchup ends
every cycle by inspecting the stream's slot on the pool and judging it
against CatchupOptions.SlotLagCeiling (zero takes the default; it cannot be
switched off). A slot that vanished is SlotLostError; over the ceiling,
Run ends with SlotLagExceededError, the table lock intact and the slot in
place for the operator to read.

Tests: WithinLagCeiling unit cases; a catch-up integration test pins the
slot with an uncommitted multi-MiB transaction and asserts the typed abort
with the lock held and the slot present; reaper integration tests cover
orphan versus live row, a row turning terminal, a held slot, another
database's slot, a prefix-wearing foreign slot, and a missing table.
@Kiran01bm
Kiran01bm marked this pull request as ready for review October 9, 2026 04:02
@chatgpt-codex-connector

Copy link
Copy Markdown

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

@aparajon

aparajon commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 551d1bd.

Verdict: the lag ceiling and the typed lost state are correct and fail closed; the reaper has one gap between its read and its drop worth closing before the orchestrator calls it. I attacked every branch of WithinLagCeiling, checkSlot, InspectSlot's recovery path and the reaper's classification against the helpers they call. Nothing here can corrupt a table. Both findings are in ReapOrphanSlots, which has no caller yet, and in each case the worst outcome is a failed or stalled run, never silent data loss.

Findings

1. The reaper waits out, then drops, a slot that becomes active after its read. ReapOrphanSlots reads active once in candidateSlotsSQL, then calls decode.DropSlot, which issues DROP_REPLICATION_SLOT … WAIT (slot_drop.go, Wait: true). A slot a process acquires between the read and the drop is waited on until its holder lets go and is then dropped. That is the behavior the "Active slots are left, not waited out" decision rules out. A holder that never lets go holds the pass for the whole of ctx. Because drops stop at the first failure and candidates come back in name order, every orphan sorted after that slot then waits behind it on every pass.

2. Two reapers running at once can drop a slot a new run has just created. Slot names are derived per target (DecodingName), so a fresh run for a table re-creates the same name. Two engines starting on one database both read pgsprite_x as an orphan. A drops it; a new run for that table commits its live row and creates pgsprite_x again; then B's drop, working from its stale read, removes it. If the new slot is held, the WAIT from Finding 1 means B waits for it and then removes it. DropSlot's own checks (engine shape, own database) all pass. It fails closed, since checkSlot later reports SlotLostError, but the run loses its copy.

Action items

  1. (Finding 1) Drop from the reaper without waiting: give DropSlot a no-wait mode, or add a reaper-only drop. Classify 55006 object_in_use as Active rather than an error. Pin it with a test that holds the slot over a replication connection and calls the no-wait drop.
  2. (Finding 1) Update the ST-3 Enforced text to match the new behavior, so "no process holds" says it is judged at drop time.
  3. (Finding 2) Before each drop, re-read whether a non-terminal row now names the slot, and skip it if one does. That narrows the gap to the drop call itself. To close it fully, take an advisory lock around the reaper's drops that run startup also takes before committing its row.
  4. (optional) Note in ReapOrphanSlots's doc that the orchestrator should run one reaper per database at a time until item 3 lands.

Verified (tried to break, couldn't)

WithinLagCeiling checks lost before any measure and never reads an unknown retained as zero; a ceiling below one byte is refused both there and in CatchupOptions.validate, and zero takes the default, so the ceiling cannot be switched off. checkSlot turns a vanished slot into SlotLostError{Found: false} rather than a successful read. InspectSlot returns NULL retained only under pg_is_in_recovery() and still reads the rest of the row. The reaper's LEFT JOIN … phase NOT IN (done, failed) cannot duplicate a candidate, because (schema_name, table_name) is the primary key and names are derived per target; likePrefix escapes _; the current_database() and logical filters keep other databases' slots unread; a missing checkpoint table is ErrTableMissing rather than "every slot is an orphan". CreateSlot documents that the caller commits the row before creating the slot, which the reaper relies on. go build, go vet, and the new decode, checkpoint and applier tests pass locally at head, and CI is green (16/16).

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

@aparajon

aparajon commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

🤖 Product review, the second of two: adoption by a standalone user, and fit for the engine that consumes pg-sprite.

OSS adoption ease

  • A leaked slot still has no way out for a standalone user. ReapOrphanSlots is a Store method with no caller, so someone who finds a pgsprite_xxxxxxxx slot pinning WAL after a crashed run has no command to clear it. A small reap subcommand, or a line in the docs naming the method and the SQL fallback, would let them recover without reading the code.
  • The over-ceiling error says what happened but not what to do next. slot pgsprite_x retains 1288490188 bytes of WAL, over the ceiling of 1073741824 is accurate. A reader still has to work out that the usual cause is a long-open transaction on the source, and that the run can resume once it ends. Human-readable sizes (1.2 GiB over a 1 GiB ceiling) plus one sentence on the next step, in the error or the docs, would make this self-serve.
  • The ceiling is only reachable through CatchupOptions. That is fine for library use. When there is a CLI path to catch-up, exposing the ceiling there would match how the other bounds are set.

Integration with the engine that consumes it

  • The typed errors are the right seam. SlotLagExceededError{Retained, Ceiling}, SlotLostError{Found, WALStatus} and ErrSlotLagUnknown can each be classified with errors.As / errors.Is and rendered as a fixed operator line without parsing message text. The catch-up wraps them with %w, so they survive.
  • Keep that wrapping intact through the orchestrator. The consuming engine reaches the library through the executor today, not through pkg/applier directly. When the orchestrator lands, each of these should still unwrap at the executor boundary, so the consumer can tell "over the ceiling, resumable" from "slot lost, start over".
  • Reaped is useful as returned. Its four lists give the consumer what it needs to log each slot's outcome. The Active and Live lists in particular let an operator see why a leaked-looking slot was kept.

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

@aparajon aparajon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving on Armand's behalf after the adversarial correctness review above. The two reaper findings there are yours to pick up, ideally before the orchestrator starts calling ReapOrphanSlots. I'm flagging them, not gating on them.

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

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated adversarial review, posted on Morgan Tocker's behalf.

Approving. The error taxonomy is right, the fail-closed directions are all set the way round they should be, and the "Decisions to veto" section made it easy to go straight at the choices rather than rediscover them. Two notes: the reaper's safety argument rests on a consistency guarantee the catalog does not provide, and the two new ST-3 guards do not compose — the ceiling abort creates exactly the state the reaper is written to refuse.

Non-blocking

The reaper's one-statement join does not give the consistency its comment claims, because pg_replication_slots is not MVCC.

candidateSlotsSQL's comment says: "The checkpoint table is read in the same statement, so a row saved before the slot it names is created is seen whenever the slot is." That inference holds for two heap tables. It does not hold here, because the two sides of the join are read under different rules. pg_replication_slots is a view over pg_get_replication_slots(), a set-returning function that walks the slot array in shared memory under ReplicationSlotControlLock; it reports what exists at the moment the executor reaches it. pgsprite_checkpoint is a heap read under the statement's snapshot, taken at statement start.

So a slot can be visible while the row that owns it is not:

  1. The reaper's statement begins; snapshot S is taken.
  2. Run R commits its checkpoint row at T1 > S.
  3. R creates its slot at T2, then closes the replication connection, leaving the slot inactive at T3.
  4. The reaper's function scan executes at Tf > T3, sees R's slot, and joins it against snapshot S, where R's row does not exist.
  5. live is false, active is false, the name has the engine's shape — so R's slot falls to default: and is dropped, publication and all, under a live run.

The active flag is the only thing standing between this and a dropped slot, and it is false for the whole window between CreateSlot closing its connection and the stream opening — which spans the initial copy, the longest phase of a route. The ordering guarantee the design relies on (row committed before slot created) is real; it just is not sufficient when the slot list is newer than the snapshot the rows are read under.

The window is narrow in practice: Tf - S is one statement's execution, so hitting it needs the reaper's statement to outlast R's commit-to-slot-close sequence, and CREATE_REPLICATION_SLOT waiting for a consistent point makes that sequence slow. And ReapOrphanSlots has no caller in this PR, so nothing can hit it today. That is what makes it cheap to close now.

Reversing the reads fixes it without a lock: list the slots first, then read the checkpoint rows in a second statement. The second statement's snapshot is then strictly newer than the slot list, so any row committed before a slot it names was created is guaranteed visible — which is exactly the invariant the current comment asserts. It also costs one extra round trip on a path that runs once at startup. Requiring two consecutive passes to agree before dropping would also work, at the cost of leaving a genuine orphan for one more pass.

The ceiling abort leaves a slot the reaper is specifically written not to reclaim, so the two ST-3 guards do not compose.

Crossing the ceiling ends Run with the lock held and the slot in place, and the invariants entry says so plainly. I agree with the decision on its own terms: the run may be resumable once a pinning transaction ends, dropping is irreversible, keeping is not, and an operator gets Retained and Ceiling to read.

The gap is what happens next. After the abort, nothing is advancing confirmed_flush_lsn any more, so the slot keeps accruing WAL — the guard's response to the bound being crossed removes the only thing that was holding retention down. And the reaper will not take it: the route's checkpoint row is in a non-terminal phase, so the slot classifies as Live and is "reported and left", which is correct for a resumable run and wrong for an abandoned one. The two guards meet only if something moves the row to Done or Failed, and Catchup.Run does not — it returns the error and leaves the phase alone.

So ST-3 as worded, "a hard ceiling on what a slot may retain", is not what the ceiling enforces: it bounds how long a run keeps confirming, not what the slot retains, and retention after the abort is bounded only by whatever eventually marks the route failed. This repo already has the convention for saying that — the *Open:* sentences elsewhere in docs/invariants.md — and one of them here would keep the ST-3 entry from reading as fully enforced. The concrete composition point is worth naming too: whichever caller handles ErrSlotLagCeiling has to move the checkpoint row to a terminal phase, or the reaper will never collect the slot the ceiling just stranded.

Smaller, same entry: the doc says the ceiling "cannot be switched off", and validate does refuse anything below one byte, but math.MaxInt64 passes and disables it in every practical sense. "Cannot be set below one byte" is the claim the code actually makes.

What I tried to break and could not

likePrefix double-escaping the prefix. strings.NewReplacer with \→\\, %→\%, _→\_ is the one correct way to write this: a Replacer makes a single left-to-right pass and never re-examines what it just emitted, so the backslash it inserts for _ is not itself escaped. The obvious sequential alternative — three chained strings.Replace calls in that order — would turn pgsprite_ into pgsprite\\_ and match nothing. Since the pattern reaches the server as a parameter rather than a literal, LIKE's default backslash escape applies and pgsprite\_% matches the literal underscore, which is what the body's example shows.

A foreign name being dropped. The classification switch puts !IsEngineSlotName first, so a name wearing the prefix without the shape is Foreign before live or active are consulted — it can never fall through to the drop regardless of its row or its holder. Ordering the cheap boolean checks first would have been the natural way to write it and would have been wrong.

An empty checkpoint table reading as all-orphans. The two ways to lose the rows are handled differently and both correctly: a missing table is ErrTableMissing from missingTable(err) on both the Query and the CollectRows error, refusing the whole pass; a present-but-empty table legitimately means every engine slot is an orphan. Checking missingTable on the CollectRows error too is the part that is easy to omit, since the relation-missing error can surface at either point depending on the plan.

WithinLagCeiling reading an unknown measure as zero. RetainedWAL.Known is a separate field rather than a sentinel in Bytes, the check is !s.Retained.Known before any comparison, and TestWithinLagCeilingFailsClosedOnAnUnknownMeasure asserts the sentinel is neither loss nor the ceiling. The standby change is what makes this reachable rather than theoretical: wrapping the measure in CASE WHEN pg_is_in_recovery() THEN NULL converts a read that failed outright into a row with an unmeasured figure, and the Known flag is what keeps that from becoming "retains nothing".

Loss being inferred from a measure. Lost() reads WALStatus == WALStatusLost only, and WithinLagCeiling checks it before Known and before the comparison, so a lost slot that still reports a small retained figure is the lost state. TestWithinLagCeilingReportsALostSlotBeforeMeasuring constructs exactly that — WALStatusLost with Retained{Bytes: 1, Known: true} — and asserts NotErrorIs(ErrSlotLagCeiling). Testing the negative is what makes it an ordering test rather than a happy-path one.

The boundary being off by one. Retained.Bytes > ceiling with TestWithinLagCeilingAbortsOnlyAboveTheCeiling asserting both that exactly the ceiling is within it and that one byte over is not, against the same fixture. The ceiling < 1 refusal is checked before the slot is read at all, so a zero ceiling is a programming error rather than an abort of every slot.

Zero meaning "off" rather than "default". withDefaults maps zero to DefaultSlotLagCeiling and validate then refuses anything below one, so zero is the only value that takes the default and a negative is a refusal naming zero as the way to get it. That matches the MaxChanges shape already in the file, so the two options cannot drift in how they read their zero value.

The vanished-slot case being folded into the ceiling check. checkSlot handles found == false with its own &SlotLostError{Slot: ...} before WithinLagCeiling is reached, and leaves Found false so the error reads "no longer exists" rather than quoting an empty wal_status. The two spellings of loss stay distinguishable through one sentinel, which is what the ST-4 entry needs.

* origin/main:
  dbconn: have the replication connection ask for warnings itself (#154)

Amp-Thread-ID: https://ampcode.com/threads/T-01a0dbfe-ca87-726b-a2cb-406197a863c6

# Conflicts:
#	SAFETY.md
#	docs/copy-and-swap-design.md
@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

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

The reaper now lists slots first and reads their owning rows in a later statement, re-reads each orphan's row right before dropping it, and drops without WAIT through the new decode.DropIdleSlot (55006 → Active); the ceiling error reads as an operator message; ST-3 gains the Open: sentence for the ceiling-abort / reaper gap.

# Finding Status Explanation
C1-F1 Reaper waits out, then drops, a slot that becomes active after its read fixed decode.DropIdleSlot issues DROP_REPLICATION_SLOT without WAIT; SQLSTATE 55006 object_in_use is ErrSlotActive, and the reaper files that slot as Active and moves on. TestDropIdleSlotRefusesAHeldSlotWithoutWaiting holds the slot over a replication connection, asserts the refusal is the server's (context intact), slot and publication left, then both dropped once released. The listed active flag is now documented as advisory.
C1-F1 (action 2) ST-3 Enforced text should say "held" is judged at drop time fixed docs/invariants.md ST-3, SAFETY.md checkpoint and slot-lifecycle rows, docs/copy-and-swap-design.md D11 and package map, and the ReapOrphanSlots doc comment all say the drop itself judges whether a slot is held.
C1-F2 Two reapers can drop a slot a new run just created fixed (narrowed) / deferred (lock) Each orphan's row is re-read in its own statement right before the drop; a row in a non-terminal phase now files it as Live. That narrows the window to the drop call itself. The advisory lock that closes it fully has to be taken by a run before it commits its row, which is the orchestrator's startup path and lands with it; tracked as an internal follow-up. The ReapOrphanSlots doc says to run one reaper per database at a time until then.
C1 (optional) Note "one reaper per database at a time" in the doc fixed In the ReapOrphanSlots doc comment, ST-3 Enforced, and the package-map row.
R2-F1 One-statement join is not consistent because pg_replication_slots is not MVCC fixed The join is gone. candidateSlotsSQL lists pg_replication_slots only; liveRowSlotsSQL reads pgsprite_checkpoint WHERE slot_name = ANY($1) AND phase NOT IN (done, failed) in a later statement, so its snapshot is strictly newer than the slot list and a row committed before its slot is seen whenever the slot is. The second statement always runs, so a database without the table is still ErrTableMissing whether or not it has slots. The comment that claimed same-statement consistency is replaced with why the two reads are ordered.
R2-F2 Ceiling abort leaves a Live slot the reaper refuses to reclaim; the two ST-3 guards do not compose fixed (doc) ST-3 in docs/invariants.md gains an Open: sentence stating exactly that, and names the composition point: whoever handles ErrSlotLagCeiling must move the checkpoint row to a terminal phase — or resume and drain the slot — before the reaper can collect it, and that handler is the orchestrator's. Catchup.Run still leaves the lock held and the slot in place; the decision stands, now with its gap written down.
R2-F2 (smaller) "cannot be switched off" overstates what validate enforces fixed docs/invariants.md, docs/copy-and-swap-design.md (two places), SAFETY.md, and the CatchupOptions.SlotLagCeiling comment now say the ceiling cannot be set below one byte / is always in force.
R2 (verified) "What I tried to break and could not" no action Agree with the reading of likePrefix, the switch ordering, missingTable on both errors, Known, Lost() ordering, the boundary, zero-as-default, and the vanished-slot branch. All of those are unchanged; missingTable now wraps both errors of the row read instead of the join.
C2-F1 A leaked slot has no way out for a standalone user fixed (doc) / deferred (CLI) D11 in docs/copy-and-swap-design.md names checkpoint.Store.ReapOrphanSlots as the pass to run and the SELECT pg_drop_replication_slot('<name>') + DROP PUBLICATION fallback, with the check that no non-terminal row still names the slot. A reap subcommand belongs with the CLI surface the orchestrator work adds; tracked as an internal follow-up.
C2-F2 Over-ceiling error says what happened but not what to do fixed SlotLagExceededError.Error() now reads slot pgsprite_x retains 1.2 GiB of WAL, over the 1 GiB ceiling (1288490188 > 1073741824 bytes); the usual cause is a long-open transaction on the source, and the run can resume once it ends. Unit-tested as rendered text (the renderer exception).
C2-F3 Ceiling only reachable through CatchupOptions deferred There is no CLI path to catch-up yet; the flag lands with it, alongside the other bounds. Tracked as an internal follow-up.
C2-F4 Typed errors are the right seam no action Agreed; unchanged.
C2-F5 Keep %w wrapping intact through the orchestrator / executor boundary deferred A requirement on code that does not exist yet; noted against the orchestrator work so errors.As on SlotLagExceededError / SlotLostError still works at the executor boundary. Tracked as an internal follow-up.
C2-F6 Reaped is useful as returned no action Unchanged; Active now means "held when the drop came round" and its doc says so.

Reviewed head 551d1bd. Comments: 6074196335, 6074197274. Reviews: #pullrequestreview-5465668017, #pullrequestreview-5465818261.

@Kiran01bm
Kiran01bm enabled auto-merge (squash) October 9, 2026 09:56
@Kiran01bm
Kiran01bm merged commit ca9a8f3 into main Oct 9, 2026
16 checks passed
@Kiran01bm
Kiran01bm deleted the kiran01bm/cs7-lag-reaper branch October 9, 2026 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants