mssqlserver_cdc: ack-gate snapshot checkpoint and fix transaction-boundary resume (CON-504) - #4677
Open
squiidz wants to merge 27 commits into
Open
mssqlserver_cdc: ack-gate snapshot checkpoint and fix transaction-boundary resume (CON-504)#4677squiidz wants to merge 27 commits into
squiidz wants to merge 27 commits into
Conversation
Track order defines the ordered checkpoint sequence, but Track was called after releasing the batcher mutex, so the count-triggered flush (Publish) and the timed-flush loop could register batches out of order and persist a regressing LSN on ack. Track now happens under the same lock as the flush. Also guards the loop's UntilNext call, which read batcher state concurrently mutated by Publish (caught by the new stress test under -race).
All rows of a transaction share one __$start_lsn and resume is exclusive (> lsn), so persisting the last row's own LSN while its transaction was only partially delivered skipped the transaction's remaining rows on restart. Each row now carries checkpoint_lsn - the start LSN of the most recent transaction whose rows are all published - and only that value is persisted. Partially-delivered transactions replay in full on restart (duplicates, not loss). The final transaction of a burst is checkpointed once a later transaction is observed; until then a restart re-delivers it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…p orphaned publishBatch, carry checkpoint LSN out-of-band - A nacked batch no longer resolves its checkpoint slot, and a nacked snapshot batch fails waitSnapshotAcks: auto_replay_nacks is user-toggleable, so a nack can be terminal and the post-snapshot LSN must not be persisted over undelivered rows (the snapshot re-runs on restart instead). - publishBatch had no production callers left after the flush/track refactor; deleted, and the batcher tests now drive the production Publish/flushCurrent paths (exercising the Track-under-mutex contract). - checkpoint_lsn is no longer message metadata: it is internal plumbing, now carried on the publisher (pendingCheckpointLSN, guarded by batcherMu) instead of an undocumented user-visible key.
…lling window Transaction-boundary checkpointing left the final transaction of a burst un-checkpointed until a later transaction appeared, so a graceful stop on an idle stream re-delivered it on every restart (caught by CI: TestIntegration_MicrosoftSQLServerCDC_ResumesFromCheckpoint). When a polling window drains, every transaction <= lastLSN is fully published, so the stream now registers an empty marker slot carrying the window's end LSN with the ordered tracker. Once all of the window's batches are acked the exact position persists — no trailing-transaction lag on graceful stop or steady state, while a crash mid-window still replays from the last safe boundary (duplicates, never loss).
snapshotNackErr was sticky for the publisher's lifetime, but the publisher is reused across Connect retries: after one nack, every re-run's waitSnapshotAcks returned the stale error even when the retried snapshot acked cleanly, livelocking the input into re-emitting the full snapshot on every reconnect. The gate error is now cleared at the start of each snapshot attempt; the WaitGroup is deliberately untouched since a previous attempt's in-flight batches can still ack or nack.
squiidz
added a commit
that referenced
this pull request
Aug 10, 2026
Mirrors the mssqlserver review fixes (#4677): a nacked batch no longer resolves its checkpoint slot, and a nacked snapshot batch fails waitSnapshotAcks so the post-snapshot SCN is not persisted over undelivered rows (auto_replay_nacks is user-toggleable, so a nack can be terminal). publishBatch had no production callers left after the flush/track refactor; deleted, with the batcher tests rewritten to drive the production Publish/flushCurrent paths.
A terminal nack (auto_replay_nacks disabled) deliberately pins the checkpoint and eventually stalls the input behind checkpoint_limit, but that consequence was invisible: nothing was logged anywhere on the nack path. Emit an error identifying the batch's checkpoint LSN, whether it was a snapshot batch, and the pinned-checkpoint consequence so operators can connect a stalled input to the downstream rejection.
This was referenced Aug 10, 2026
Merged
… tracker, batching policy preserved - A terminal nack (auto_replay_nacks disabled) pinned the ordered checkpoint tracker permanently: the publisher and tracker were built once and reused across Connect retries, so after one nack no LSN could ever be persisted again and the input eventually wedged behind checkpoint_limit. A nack now triggers a restart, and Connect rebuilds the publisher (batcher + tracker) per attempt - sealing the old one so late acks from the previous session cannot persist stale positions - letting the restart resume from the last durable LSN and redeliver. - CheckpointWindow force-flushed the partial batch at every drained polling window, silently overriding the user's batching policy during steady-state streaming. It now defers the window checkpoint onto the buffered batch (the window-end LSN rides as its checkpoint payload) and only registers a marker when the batcher is empty. - The snapshot gate's downstream-rejection failure is logged at error level with wording that names it; soft-stop cancellation keeps Info. Full integration suite (10 tests) green after the changes.
squiidz
added a commit
that referenced
this pull request
Aug 10, 2026
Aligns with the mssqlserver review outcome (#4677): a terminal nack (auto_replay_nacks disabled) pinned the ordered checkpoint tracker permanently — the publisher and tracker were built once and reused across Connect retries, so after one nack no SCN could ever be persisted again and the input eventually wedged behind checkpoint_limit. A nack now triggers a restart, and Connect rebuilds the publisher (batcher + tracker) per attempt, sealing the old one so late acks from the previous session can neither persist stale positions nor trigger spurious restarts. The restart resumes from the last durable SCN and redelivers.
Contributor
Author
|
The nack-handling changes from the earlier review rounds have been unwound in the latest commit. The framework's documented contract for Unwound here (c3f2ff9): the nack gate failure and per-connect publisher rebuild. Kept: the snapshot ack gate, transaction-boundary checkpointing (checkpoint_lsn carried out-of-band + CheckpointWindow markers), and the Flush→Track atomicity fix. SnapshotAckBarrier, TransactionSplitAcrossBatches, and ResumesFromCheckpoint integration tests re-verified green. |
…gression CheckpointWindow (stream goroutine) and batch ack functions (pipeline goroutines) each ran resolve+cacheLSN with no shared ordering, so two persists could land out of order and overwrite a newer resume position with an older one - bounded replay after restart, not loss. A persistMu critical section around each resolve+persist pair keeps the cache writes in tracker order. New concurrency test proven red against the unfixed code (regression reproduced under -race within one run).
… publisher after a failed send Ports the oracledb_cdc fixes from the same review round: - checkpoint.Track ran under batcherMu, so a Track blocked on checkpoint_limit (slow downstream during bulk snapshot load) froze every concurrent Publish and the timed-flush ticker instead of just its own flusher. Each flush (and CheckpointWindow marker) now takes an order ticket under batcherMu - atomically with the Flush, keeping the user's batching policy exact - and Track+send admission happens in ticket order outside batcherMu, so the checkpoint sequence still matches flush order exactly while a blocked Track stalls only the ticket queue. Proven by a new blocked-Track buffering test and the ConcurrentSnapshot integration test (which failed against an earlier draft that sacrificed policy exactness for lock granularity). - A failed batch send rolled back the snapshot gate but never resolved the checkpoint slot, permanently pinning the constructor-lifetime tracker. Resolving the slot instead would be unsafe (another flusher may already have delivered a later-tracked batch, whose ack would then persist an LSN past the undelivered rows), so the publisher is marked poisoned and Connect rebuilds it with a fresh tracker. cacheLSN is now monotonic across publisher generations so a previous session's late acks can never regress the durable position. Also renames trackedBatch.msg to msgs, mirroring the oracledb review suggestion. Full 10-test integration suite green.
Ports the oracledb_cdc fix from the same review round: the ticket refactor moved the snapshot gate Add out of the flush critical section (it runs in trackBatch, after ticket admission and after checkpoint.Track, which can park on checkpoint_limit), so at the handoff the timed-flush loop could hold the final snapshot rows - flushed, ticketed, but not yet counted on the gate - while flushCurrent saw an empty batcher and returned with no barrier. waitSnapshotAcks could then release early and the post-snapshot LSN persist ahead of undelivered rows (plus a WaitGroup Add-during-Wait misuse hazard). flushCurrent now takes its ticket unconditionally so its admission is a sequence barrier. New test encodes the parked-flusher interleaving (proven red against the unbarriered code on oracledb).
…in drains Review finding on the ticket refactor: admit() waits on a sync.Cond with no shutdown escape, and the timed-flush loop runs under the publisher's OWN signaller, which the input's Close never triggered (unlike oracledb, whose Close already stops its publisher). With a batching period set, a flush parked in sendTracked after ReadBatch stops would hold its ticket forever, wedging every later flusher in admit - the stream goroutine never returns, TriggerHasStopped never fires, and Close burns its timeouts leaking the session goroutines. Close now soft-stops the publisher alongside the session: the loop's context cancels, its ticket releases, and the chain drains - each later ticket holder's Track/send escapes via its stopSig-derived context. New test wedges the exact chain (loop parked in send holding a ticket, flusher waiting in admit) and asserts shutdown unwinds it - proven red against Close without the publisher stop.
Review finding: a graceful stop landing in the snapshot handoff window exits flushCurrent via softCtx cancellation (nothing drains msgChan once ReadBatch stops - a path the Close fix made routine), but logged at ERROR with 'Failed to flush'. Cancellation without a hard stop now logs at Info like the two adjacent handoff branches; genuine flush failures keep the error level.
… lock Review finding on the Close fix: triggering the publisher's soft stop from Close made loop()'s deferred batcher.Close reachable while session goroutines can still be inside Publish - unsynchronised access to the non-goroutine-safe batcher. The teardown now runs under batcherMu and marks the batcher closed; flush paths refuse on the closed flag instead of touching it. Alongside it, admission gains the context escape the ticket system was missing: admit(ctx) parks on a per-ticket channel and a cancelled waiter marks its ticket abandoned - release skips abandoned tickets, so the sequence stays intact - letting a graceful stop unwind flushers queued behind a send parked under a different, still-live context. Abandoned batches were never tracked, so their rows re-read from the last durable checkpoint. New test parks ticket 0's holder under a live context, queues a flusher behind it, and asserts its own cancellation unwinds it promptly with the sequence intact afterwards.
…rows Ports the oracledb fix from the same review round: an abandoned ticket's flushed-but-untracked batch left nothing pinning the tracker, so a later tracked batch's ack could persist an LSN past the dropped rows. Because admission is strictly ordered, nothing after the gap is tracked at abandon time: sealing the queue (errQueueSealed for all later admissions) plus the poison rebuild guarantees the dropped rows are re-read from the last durable LSN. CheckpointWindow markers and empty flushCurrent barriers own no rows and abandon benignly. New test encodes the rows-owning abandon.
…atomic Two review findings: - A downstream rejection with auto_replay_nacks disabled dropped rows and advanced the checkpoint past them with no log anywhere (the drop logging was removed wholesale with the nack-pinning unwind, leaving the publisher's logger entirely unused). The ack function now warns with the batch size, snapshot flag, and checkpoint LSN when it advances past rejected rows, and the failed-send poison path logs the rebuild it schedules. - Connect's poisoned rebuild made the publisher field mutable while ReadBatch and Close read it unguarded on other goroutines - a Close racing a rebuild could soft-stop the OLD publisher and leave the new one wedged past the shutdown timeout. The field is now an atomic.Pointer: readers can never observe a stale pointer, and the session captures its own generation as a local. Same pair applied to oracledb.
Ports the oracledb fix from the same review round: a trackBatch failure after admission strands flushed-but-untracked rows while the deferred release advances the queue, letting a later ticket's ack persist an LSN past the gap. All three flush paths seal on a track failure with rows in hand; flushCurrent also seals when Flush errors alongside a non-empty batch. Same new test as oracledb.
Ports the oracledb fix: loop() discarded the Flush error (drained rows vanished silently) and Publish returned without sealing; both now seal (and thereby poison) like flushCurrent already did, and the loop surfaces the error at error level.
…tests Review ask: nothing exercised cacheLSN's monotonic guard (a wrong comparison direction would silently stop checkpoint writes) or proved the poisoned rebuild swaps generations. The rebuild block is extracted into rebuildPublisherIfPoisoned and unit-tested - old generation closed, fresh tracker stored, and a late ack from the abandoned generation is a no-op on the durable position - and cacheLSN's advance/equal/regress/ empty semantics are locked in directly. Same pair added to oracledb.
… batcherMu Ports the oracledb fixes: admit(ownsRows) seals+poisons in the same ticketMu critical section that records an abandonment (window markers and barrier tickets stay benign), and all Flush-error seals run before batcherMu is released so no flusher can slip a ticket past the discarded rows in the unlock gap.
Ports the oracledb fix: the flush error returns before admit so the sealed refusal cannot mask the actual batching.processors failure; the dead post-admit check is gone.
…ensive Same annotation as oracledb: service.Batcher.Flush never assigns its error return through the current public API, so the seal branches are defensive handling of the declared contract rather than a reachable path; documented at the timed-loop branch.
Review finding, same class as the handoff log-level fix: Close's publisher soft stop makes sendTracked's cancellation the EXPECTED path on every graceful shutdown with a batch in flight, but it warned as if the pipeline had faulted. Cancellation under a signalled soft stop now logs at debug; the warning stays for a send that fails while the publisher is meant to be live. Poisoning is kept on both paths (moot after Close, required otherwise).
…ropagates Ports the oracledb fix: the shutSig-based check was racy (Close triggers the input's soft stop, whose cancellation can unwind a parked send before the publisher's own soft stop lands one statement later). Close now sets an explicit stopping flag first, and sendTracked checks it alongside shutSig.
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
2 participants
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.
Part of CON-504 (CDC at-least-once / ack-gated progress).
Fixes three at-least-once gaps found in the CON-504 audit, plus one data race the new tests surfaced:
1. Post-snapshot LSN persisted at read time. After the snapshot read loop finished,
cacheLSN(maxLSN)ran while snapshot batches were still un-acked downstream (and a partial batch could still sit unflushed in the batcher). A crash in that window meant the restart saw a cached LSN, skipped the snapshot, and silently lost the un-delivered rows. The handoff now flushes the remaining partial batch, blocks until every snapshot batch is acknowledged (escapable by soft-stop), and only then persists the LSN — the same barrier as postgres (#4584) and oracledb (#4675).2. Transaction tail skipped on resume (tie-group). All rows of a transaction share one
__$start_lsnand resume is exclusive (> lsn), but the checkpoint used the last row's own LSN. Acking a batch that ended mid-transaction persisted that transaction's LSN; a crash then skipped every remaining row of the same transaction on restart. Rows now carrycheckpoint_lsn— the start LSN of the most recent transaction whose rows are all published, computed in the globally LSN-ordered stream loop — and only that value is ever persisted. Partially-delivered transactions replay in full (duplicates, not loss). Known limitation: the final transaction of a burst is checkpointed once a later transaction is observed; until then a restart re-delivers it.3. Out-of-order checkpoint tracking.
checkpoint.Trackran outside the batcher mutex, so the count-triggered flush (Publish) and the timed-flush loop could register batches with the ordered tracker in the wrong order and persist a regressing LSN on ack. Track now happens under the same lock as the flush.4. Data race on batcher state (found by the new stress test under
-race). The timed-flush loop readbatcher.UntilNext()unlocked while Publish mutated the batcher under the mutex. Now locked.Proof of Work
flushCurrent, checkpoint selection (checkpoint_lsnpreferred, row LSN never persisted, first-transaction batches persist nothing), transaction-boundary tracker, and a concurrent-flush ordering stress test (race-clean, checkpoint never regresses).TestIntegration_MicrosoftSQLServerCDC_SnapshotAckBarrier: blocked consumer + simulated crash → no checkpoint persisted → restart re-runs the snapshot. Fails on pre-fix code:post-snapshot LSN must not be persisted before snapshot rows are acknowledged: Should be zero, but was 1.TestIntegration_MicrosoftSQLServerCDC_TransactionSplitAcrossBatches(the ticket's commit-order ≠ batch-order criterion): a 4-row transaction split across count-2 batches, first batch acked, crash, restart. Fails on pre-fix code — every tail row reportednever redelivered (checkpoint advanced past a partially-delivered transaction); passes with the fix.-race; existing resume/ordering integration tests unaffected.Note: the Track-outside-mutex and unguarded-
UntilNextraces also exist in the oracledb batcher (same lifted pattern) — flagged as a follow-up for #4675.