From d61bab8a7e5ef5ba3ee3c677817b61d1313935a0 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 6 Aug 2026 14:04:38 -0400 Subject: [PATCH 01/24] oracledb_cdc: track in-flight snapshot batch acks in the publisher --- internal/impl/oracledb/batcher.go | 34 +++++++++++++ internal/impl/oracledb/batcher_test.go | 66 ++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index bfad80612e..7b37921db8 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -34,6 +34,11 @@ type batchPublisher struct { cacheSCN func(ctx context.Context, scn replication.SCN) error schemas *schemaCache + // snapshotAckWG counts published snapshot batches that have not yet been + // acknowledged downstream. The snapshot->streaming handoff blocks on it so + // the post-snapshot SCN is never persisted while snapshot rows are in flight. + snapshotAckWG sync.WaitGroup + log *service.Logger shutSig *shutdown.Signaller } @@ -251,6 +256,9 @@ func (b *batchPublisher) publishBatch(ctx context.Context, batch service.Message msg := asyncMessage{ msg: batch, ackFn: func(ctx context.Context, _ error) error { + if isSnapshotBatch { + defer b.snapshotAckWG.Done() + } scn := resolveFn() if scn == nil || !scn.IsValid() { return nil @@ -263,9 +271,35 @@ func (b *batchPublisher) publishBatch(ctx context.Context, batch service.Message return b.cacheSCN(ctx, *scn) }, } + if isSnapshotBatch { + b.snapshotAckWG.Add(1) + } select { case b.msgChan <- msg: return nil + case <-ctx.Done(): + if isSnapshotBatch { + b.snapshotAckWG.Done() + } + return ctx.Err() + } +} + +// waitSnapshotAcks blocks until every published snapshot batch has been +// acknowledged (or nacked) downstream, or until ctx is cancelled. Nacked +// batches release the gate too: redelivery is owned by auto_replay_nacks, +// and the ctx escape prevents a permanently-failing downstream from +// wedging shutdown. +func (b *batchPublisher) waitSnapshotAcks(ctx context.Context) error { + drained := make(chan struct{}) + go func() { + // May outlive this call if ctx fires first; bounded by process lifetime. + b.snapshotAckWG.Wait() + close(drained) + }() + select { + case <-drained: + return nil case <-ctx.Done(): return ctx.Err() } diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 2352ef5593..9d56fa4efb 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -10,9 +10,11 @@ package oracledb import ( "context" + "errors" "log/slog" "sync" "testing" + "time" "github.com/Jeffail/checkpoint" "github.com/stretchr/testify/require" @@ -82,6 +84,70 @@ func TestPublishBatch(t *testing.T) { }) } +func TestSnapshotAckGate(t *testing.T) { + t.Run("blocks until the snapshot batch is acked", func(t *testing.T) { + ctx := t.Context() + publisher, _ := newTestBatchPublisher(t) + + msg := publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) + + done := make(chan error, 1) + go func() { done <- publisher.waitSnapshotAcks(ctx) }() + + select { + case err := <-done: + t.Fatalf("waitSnapshotAcks returned before the snapshot batch was acked: %v", err) + case <-time.After(100 * time.Millisecond): + } + + require.NoError(t, msg.ackFn(ctx, nil)) + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("waitSnapshotAcks did not return after the snapshot batch was acked") + } + }) + + t.Run("a nack also releases the gate", func(t *testing.T) { + ctx := t.Context() + publisher, _ := newTestBatchPublisher(t) + + msg := publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) + require.NoError(t, msg.ackFn(ctx, errors.New("downstream failure"))) + + require.NoError(t, publisher.waitSnapshotAcks(ctx)) + }) + + t.Run("streaming batches do not hold the gate", func(t *testing.T) { + ctx := t.Context() + publisher, _ := newTestBatchPublisher(t) + + // Published but never acked: must not block the gate. + publishAndReceive(t, ctx, publisher, service.MessageBatch{newStreamingMessage("200")}) + + require.NoError(t, publisher.waitSnapshotAcks(ctx)) + }) + + t.Run("context cancellation escapes the gate", func(t *testing.T) { + publisher, _ := newTestBatchPublisher(t) + + ctx, cancel := context.WithCancel(t.Context()) + publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) + + done := make(chan error, 1) + go func() { done <- publisher.waitSnapshotAcks(ctx) }() + cancel() + + select { + case err := <-done: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(5 * time.Second): + t.Fatal("waitSnapshotAcks did not return after context cancellation") + } + }) +} + func newTestBatchPublisher(t *testing.T) (*batchPublisher, func() []replication.SCN) { t.Helper() From 844162da05eb5ecc2ebc328f3188568344c3453c Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 6 Aug 2026 14:05:56 -0400 Subject: [PATCH 02/24] oracledb_cdc: add flushCurrent to publish partial batches without stopping the publisher --- internal/impl/oracledb/batcher.go | 22 +++++++++---- internal/impl/oracledb/batcher_test.go | 45 ++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 7b37921db8..43f685079a 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -322,15 +322,14 @@ func (b *batchPublisher) msgs() <-chan asyncMessage { return b.msgChan } -// FlushRemaining stops the loop goroutine and then flushes any partial batch -// still held in the batcher, blocking until it is consumed by ReadBatch. -func (b *batchPublisher) FlushRemaining(ctx context.Context) error { +// flushCurrent flushes any partial batch still held by the batcher and +// publishes it, leaving the publisher loop running. Used at the +// snapshot->streaming handoff so every snapshot row is published (and can be +// awaited via waitSnapshotAcks) before the post-snapshot SCN is persisted. +func (b *batchPublisher) flushCurrent(ctx context.Context) error { if b.batcher == nil { return nil } - b.shutSig.TriggerSoftStop() - <-b.shutSig.HasStoppedChan() - b.batcherMu.Lock() remaining, err := b.batcher.Flush(ctx) b.batcherMu.Unlock() @@ -340,6 +339,17 @@ func (b *batchPublisher) FlushRemaining(ctx context.Context) error { return b.publishBatch(ctx, remaining) } +// FlushRemaining stops the loop goroutine and then flushes any partial batch +// still held in the batcher, blocking until it is consumed by ReadBatch. +func (b *batchPublisher) FlushRemaining(ctx context.Context) error { + if b.batcher == nil { + return nil + } + b.shutSig.TriggerSoftStop() + <-b.shutSig.HasStoppedChan() + return b.flushCurrent(ctx) +} + // Close signals the publisher's loop goroutine to stop and waits for it to exit. // TriggerHardStop cancels the HardStopCtx used by publishBatch, unblocking any // send that is waiting on msgChan when no consumer is left. diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 9d56fa4efb..4c34a69ad7 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -148,6 +148,51 @@ func TestSnapshotAckGate(t *testing.T) { }) } +func TestFlushCurrent(t *testing.T) { + ctx := t.Context() + logger := service.NewLoggerFromSlog(slog.Default()) + cp := checkpoint.NewCapped[replication.SCN](100) + + batcher, err := (service.BatchPolicy{Count: 100}).NewBatcher(service.MockResources()) + require.NoError(t, err) + + publisher := newBatchPublisher(batcher, cp, logger) + t.Cleanup(publisher.Close) + publisher.cacheSCN = func(context.Context, replication.SCN) error { return nil } + + publishEvent := func(v int) { + t.Helper() + require.NoError(t, publisher.Publish(ctx, &replication.MessageEvent{ + Schema: "S", + Table: "T", + Operation: replication.MessageOperationRead, + Data: map[string]any{"a": v}, + SCN: replication.SCN(100), + })) + } + receive := func(failMsg string) { + t.Helper() + got := make(chan asyncMessage, 1) + go func() { got <- <-publisher.msgs() }() + require.NoError(t, publisher.flushCurrent(ctx)) + select { + case m := <-got: + require.Len(t, m.msg, 1) + case <-time.After(5 * time.Second): + t.Fatal(failMsg) + } + } + + // Count=100 keeps a single event buffered in the batcher until flushed. + publishEvent(1) + receive("flushCurrent did not publish the buffered partial batch") + + // The loop must still be alive after flushCurrent (unlike FlushRemaining): + // a second publish+flush must work identically. + publishEvent(2) + receive("publisher loop no longer functional after flushCurrent") +} + func newTestBatchPublisher(t *testing.T) (*batchPublisher, func() []replication.SCN) { t.Helper() From 117f4713a6d63f5172fd30452a308c7203892c59 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 6 Aug 2026 14:06:22 -0400 Subject: [PATCH 03/24] oracledb_cdc: gate post-snapshot checkpoint on downstream acks --- internal/impl/oracledb/input_oracledb_cdc.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index d77cdbffda..58371b0ca5 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -617,6 +617,23 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { return } + // Flush the partial snapshot batch still held by the batcher, then + // block until every snapshot batch is acknowledged downstream. + // Persisting the SCN any earlier would let a crash in this window + // skip un-acked snapshot rows on restart. Blocks until acks drain + // or soft-stop (no timeout, by design; see postgres_cdc's + // equivalent barrier). + if err = o.publisher.flushCurrent(softCtx); err != nil { + o.log.Errorf("Failed to flush remaining snapshot batches. Snapshot will re-run on restart (may cause duplicate data): %s", err) + o.stopSig.TriggerHasStopped() + return + } + if err = o.publisher.waitSnapshotAcks(softCtx); err != nil { + o.log.Infof("Interrupted while waiting for snapshot acknowledgements. Snapshot will re-run on restart (may cause duplicate data): %s", err) + o.stopSig.TriggerHasStopped() + return + } + if err = o.cacheSCN(softCtx, startSCN); err != nil { o.log.Errorf("Failed to capture SCN after snapshot completion. Snapshot will re-run on restart (may cause duplicate data): %s", err) o.stopSig.TriggerHasStopped() From 5e04738749dae2194b8106baca7b9525a1505fc6 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 6 Aug 2026 14:11:07 -0400 Subject: [PATCH 04/24] oracledb_cdc: adversarial crash test for the snapshot ack barrier --- internal/impl/oracledb/integration_test.go | 121 +++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/internal/impl/oracledb/integration_test.go b/internal/impl/oracledb/integration_test.go index a5eedd374e..bd2e1f196f 100644 --- a/internal/impl/oracledb/integration_test.go +++ b/internal/impl/oracledb/integration_test.go @@ -560,6 +560,127 @@ oracledb_cdc: } } +// TestIntegrationOracleDBCDCSnapshotAckBarrier verifies that a crash during +// the snapshot->streaming handoff (after snapshot rows are emitted but before +// they are acknowledged) does not lose data: because the post-snapshot SCN is +// only persisted once every snapshot batch is acked, the snapshot must re-run +// on restart. See CON-504. +func TestIntegrationOracleDBCDCSnapshotAckBarrier(t *testing.T) { + integration.CheckSkip(t) + + connStr, db := oracledbtest.SetupTestWithOracleDBVersion(t) + require.NoError(t, db.CreateTableWithSupplementalLoggingIfNotExists(t.Context(), "testdb.ackbarrier", "CREATE TABLE testdb.ackbarrier (id NUMBER GENERATED ALWAYS AS IDENTITY PRIMARY KEY)")) + + const rowCount = 5 + for range rowCount { + db.MustExec("INSERT INTO testdb.ackbarrier (id) VALUES (DEFAULT)") + } + db.MustExec("COMMIT") + + // batching.count == rowCount forces all snapshot rows into a single output + // batch, so the run-1 consumer receives them all at once and can then block + // without acking - reproducing the "emitted but not yet acked" handoff state. + cfg := fmt.Sprintf(` +oracledb_cdc: + connection_string: %s + snapshot_mode: snapshot_and_stream + logminer: + scn_window_size: 20000 + min_scn_window_size: 0 + backoff_interval: 1s + include: ["TESTDB.ACKBARRIER"] + batching: + count: %d + period: 1h`, connStr, rowCount) + + // Run 1: receive the snapshot rows but never acknowledge them, then + // simulate a crash by cancelling the run before the SCN can be persisted. + t.Log("Launching run 1 (blocked consumer, simulated crash)...") + received := make(chan struct{}, 1) + run1Builder := service.NewStreamBuilder() + require.NoError(t, run1Builder.AddInputYAML(cfg)) + require.NoError(t, run1Builder.SetLoggerYAML(`level: INFO`)) + require.NoError(t, run1Builder.AddBatchConsumerFunc(func(ctx context.Context, _ service.MessageBatch) error { + select { + case received <- struct{}{}: + default: + } + // Block without acking until the simulated crash cancels our context. + <-ctx.Done() + return ctx.Err() + })) + run1, err := run1Builder.Build() + require.NoError(t, err) + license.InjectTestService(run1.Resources()) + + run1Ctx, crash := context.WithCancel(t.Context()) + run1Done := make(chan struct{}) + go func() { + defer close(run1Done) + _ = run1.Run(run1Ctx) + }() + + select { + case <-received: + case <-time.After(5 * time.Minute): + t.Fatal("snapshot rows were never delivered to the run-1 output") + } + // Give the input time to reach the ack barrier (and, in the buggy version, + // to persist the post-snapshot SCN) before we crash. + time.Sleep(5 * time.Second) + crash() + select { + case <-run1Done: + case <-time.After(30 * time.Second): + t.Fatal("run 1 did not stop after the simulated crash") + } + + // The barrier must have prevented the post-snapshot SCN from being + // persisted, since the snapshot rows were never acknowledged. This is the + // core guarantee: without it a cached SCN would exist here and the + // snapshot would be skipped on restart, silently losing the un-acked rows. + var checkpoints int + require.NoError(t, db.QueryRowContext(t.Context(), "SELECT COUNT(*) FROM RPCN.CDC_CHECKPOINT_CACHE").Scan(&checkpoints)) + require.Zero(t, checkpoints, "post-snapshot SCN must not be persisted before snapshot rows are acknowledged") + + // Run 2: restart against the same checkpoint cache. Since run 1 never + // acked the snapshot, no SCN was cached, so the snapshot re-runs and every + // row is delivered again. + t.Log("Launching run 2 (verifying the snapshot re-runs)...") + var ( + readsMu sync.Mutex + reads int + ) + run2Builder := service.NewStreamBuilder() + require.NoError(t, run2Builder.AddInputYAML(cfg)) + require.NoError(t, run2Builder.SetLoggerYAML(`level: INFO`)) + require.NoError(t, run2Builder.AddBatchConsumerFunc(func(_ context.Context, mb service.MessageBatch) error { + readsMu.Lock() + defer readsMu.Unlock() + for _, msg := range mb { + if op, _ := msg.MetaGet("operation"); op == "read" { + reads++ + } + } + return nil + })) + run2, err := run2Builder.Build() + require.NoError(t, err) + license.InjectTestService(run2.Resources()) + go func() { + if err := run2.Run(t.Context()); err != nil && !errors.Is(err, context.Canceled) { + t.Error(err) + } + }() + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + readsMu.Lock() + defer readsMu.Unlock() + assert.Equal(c, rowCount, reads, "snapshot should have re-run and re-delivered every row after the crash") + }, 5*time.Minute, 500*time.Millisecond) + require.NoError(t, run2.StopWithin(time.Second*30)) +} + func TestIntegrationOracleDBCDCStreaming(t *testing.T) { integration.CheckSkip(t) connStr, db := oracledbtest.SetupTestWithOracleDBVersion(t) From 05e712822bcfe93276dfad7a9db61f506b8539fc Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 6 Aug 2026 15:52:50 -0400 Subject: [PATCH 05/24] oracledb_cdc: make batch tracking atomic with batch flushing 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 SCN 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 (a data race confirmed by the new stress test under -race). Same fixes as the mssqlserver batcher, which shares this lifted pattern. --- internal/impl/oracledb/batcher.go | 123 ++++++++++++++++++------- internal/impl/oracledb/batcher_test.go | 73 +++++++++++++++ 2 files changed, 163 insertions(+), 33 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 43f685079a..bdf14c7b11 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -73,7 +73,11 @@ func (p *batchPublisher) loop() { return } + // UntilNext reads the batcher's internal state, which concurrent + // Publish calls mutate under batcherMu — take the same lock. + p.batcherMu.Lock() tNext, exists := p.batcher.UntilNext() + p.batcherMu.Unlock() if !exists { if flushBatchTicker != nil { flushBatchTicker.Stop() @@ -100,9 +104,14 @@ func (p *batchPublisher) loop() { adjustTimedFlush() select { case <-flushBatch: - var sendBatch service.MessageBatch - - // Wrap this in a closure to make locking/unlocking easier. + var ( + tracked *trackedBatch + trackErr error + ) + + // Wrap this in a closure to make locking/unlocking easier. Track + // happens under the same lock as the flush so the checkpoint + // sequence matches flush order. func() { p.batcherMu.Lock() defer p.batcherMu.Unlock() @@ -115,13 +124,18 @@ func (p *batchPublisher) loop() { return } + var sendBatch service.MessageBatch if sendBatch, _ = p.batcher.Flush(hardStopCtx); len(sendBatch) == 0 { return } + tracked, trackErr = p.trackBatchLocked(hardStopCtx, sendBatch) }() + if trackErr != nil { + return + } - if len(sendBatch) > 0 { - if err := p.publishBatch(hardStopCtx, sendBatch); err != nil { + if tracked != nil { + if err := p.sendTracked(hardStopCtx, tracked); err != nil { return } } @@ -198,10 +212,17 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven msg.MetaSetImmut("schema", service.ImmutableAny{V: schemaAny}) } - var flushedBatch []*service.Message + // Flush and Track must be atomic: Track order defines the checkpoint + // sequence, so another flusher (the timed-flush loop) must not interleave + // between our flush and our Track. Only the channel send happens outside + // the lock. + var tracked *trackedBatch b.batcherMu.Lock() if b.batcher.Add(msg) { - flushedBatch, err = b.batcher.Flush(ctx) + var flushedBatch []*service.Message + if flushedBatch, err = b.batcher.Flush(ctx); err == nil && len(flushedBatch) > 0 { + tracked, err = b.trackBatchLocked(ctx, flushedBatch) + } } b.batcherMu.Unlock() if err != nil { @@ -209,8 +230,8 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven } // If a batch was flushed, publish it outside the lock - if len(flushedBatch) > 0 { - if err := b.publishBatch(ctx, flushedBatch); err != nil { + if tracked != nil { + if err := b.sendTracked(ctx, tracked); err != nil { return fmt.Errorf("publishing flushed batch: %w", err) } } @@ -218,11 +239,34 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven return nil } +// trackedBatch pairs a ready-to-send asyncMessage with the bookkeeping needed +// to roll back its snapshot-gate slot if the send fails. +type trackedBatch struct { + msg asyncMessage + isSnapshot bool +} + +// publishBatch tracks and sends a batch that was flushed elsewhere. Callers +// that flush the batcher themselves must instead track under the same lock as +// their flush (see Publish/loop/flushCurrent) to keep Track order == flush +// order. func (b *batchPublisher) publishBatch(ctx context.Context, batch service.MessageBatch) error { if len(batch) == 0 { return nil } + b.batcherMu.Lock() + tracked, err := b.trackBatchLocked(ctx, batch) + b.batcherMu.Unlock() + if err != nil { + return err + } + return b.sendTracked(ctx, tracked) +} +// trackBatchLocked registers the batch with the ordered checkpoint tracker and +// builds its ack function. It MUST be called with batcherMu held: Track order +// defines the checkpoint sequence, so it has to match flush order exactly. +func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.MessageBatch) (*trackedBatch, error) { lastMsg := batch[len(batch)-1] // ensure we don't checkpoint snapshot batches @@ -245,40 +289,49 @@ func (b *batchPublisher) publishBatch(ctx context.Context, batch service.Message var parseErr error checkpointSCN, parseErr = replication.ParseSCN(scn) if parseErr != nil { - return fmt.Errorf("parsing checkpoint SCN: %w", parseErr) + return nil, fmt.Errorf("parsing checkpoint SCN: %w", parseErr) } } resolveFn, err := b.checkpoint.Track(ctx, checkpointSCN, int64(len(batch))) if err != nil { - return fmt.Errorf("tracking SCN checkpoint for batch: %w", err) - } - msg := asyncMessage{ - msg: batch, - ackFn: func(ctx context.Context, _ error) error { - if isSnapshotBatch { - defer b.snapshotAckWG.Done() - } - scn := resolveFn() - if scn == nil || !scn.IsValid() { - return nil - } - if isSnapshotBatch && *scn <= checkpointSCN { - // Resolved value is this snapshot batch's own shared SCN (or older) — - // nothing new to persist, and persisting it would be premature. - return nil - } - return b.cacheSCN(ctx, *scn) - }, + return nil, fmt.Errorf("tracking SCN checkpoint for batch: %w", err) } if isSnapshotBatch { b.snapshotAckWG.Add(1) } + return &trackedBatch{ + isSnapshot: isSnapshotBatch, + msg: asyncMessage{ + msg: batch, + ackFn: func(ctx context.Context, _ error) error { + if isSnapshotBatch { + defer b.snapshotAckWG.Done() + } + scn := resolveFn() + if scn == nil || !scn.IsValid() { + return nil + } + if isSnapshotBatch && *scn <= checkpointSCN { + // Resolved value is this snapshot batch's own shared SCN (or older) — + // nothing new to persist, and persisting it would be premature. + return nil + } + return b.cacheSCN(ctx, *scn) + }, + }, + }, nil +} + +// sendTracked hands a tracked batch to ReadBatch. Must be called WITHOUT +// batcherMu held (the send blocks until consumed). A failed send releases the +// batch's snapshot-gate slot. +func (b *batchPublisher) sendTracked(ctx context.Context, tracked *trackedBatch) error { select { - case b.msgChan <- msg: + case b.msgChan <- tracked.msg: return nil case <-ctx.Done(): - if isSnapshotBatch { + if tracked.isSnapshot { b.snapshotAckWG.Done() } return ctx.Err() @@ -330,13 +383,17 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { if b.batcher == nil { return nil } + var tracked *trackedBatch b.batcherMu.Lock() remaining, err := b.batcher.Flush(ctx) + if err == nil && len(remaining) > 0 { + tracked, err = b.trackBatchLocked(ctx, remaining) + } b.batcherMu.Unlock() - if err != nil || len(remaining) == 0 { + if err != nil || tracked == nil { return err } - return b.publishBatch(ctx, remaining) + return b.sendTracked(ctx, tracked) } // FlushRemaining stops the loop goroutine and then flushes any partial batch diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 4c34a69ad7..48caa8dec8 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -193,6 +193,79 @@ func TestFlushCurrent(t *testing.T) { receive("publisher loop no longer functional after flushCurrent") } +// TestTrackOrderUnderConcurrentFlush stresses the two concurrent flushers (the +// count-triggered flush in Publish and the timed-flush loop) and asserts the +// persisted checkpoint never regresses when batches are acked in delivery +// order. Before Track was moved under the batcher mutex, the two flushers +// could interleave between flush and Track, registering batches with the +// ordered tracker in the wrong order and persisting a regressing SCN. Run with +// -race to also catch the underlying data race structurally. +func TestTrackOrderUnderConcurrentFlush(t *testing.T) { + ctx := t.Context() + logger := service.NewLoggerFromSlog(slog.Default()) + cp := checkpoint.NewCapped[replication.SCN](1000) + + // Count 2 + a tiny period keeps both flush paths active concurrently. + batcher, err := (service.BatchPolicy{Count: 2, Period: "1ms"}).NewBatcher(service.MockResources()) + require.NoError(t, err) + + publisher := newBatchPublisher(batcher, cp, logger) + t.Cleanup(publisher.Close) + + var ( + mu sync.Mutex + persisted []replication.SCN + ) + publisher.cacheSCN = func(_ context.Context, scn replication.SCN) error { + mu.Lock() + defer mu.Unlock() + persisted = append(persisted, scn) + return nil + } + + // Consumer: ack every batch immediately, in delivery order. + consumerDone := make(chan struct{}) + consumerCtx, stopConsumer := context.WithCancel(ctx) + go func() { + defer close(consumerDone) + for { + select { + case m := <-publisher.msgs(): + _ = m.ackFn(ctx, nil) + case <-consumerCtx.Done(): + return + } + } + }() + + const events = 500 + for i := range events { + require.NoError(t, publisher.Publish(ctx, &replication.MessageEvent{ + Schema: "S", + Table: "T", + Operation: replication.MessageOperationInsert, + CheckpointSCN: replication.SCN(i + 1), + Data: map[string]any{"i": i}, + })) + } + require.NoError(t, publisher.flushCurrent(ctx)) + + require.Eventually(t, func() bool { + mu.Lock() + defer mu.Unlock() + return len(persisted) > 0 && persisted[len(persisted)-1] == replication.SCN(events) + }, 10*time.Second, 10*time.Millisecond, "final SCN was never persisted") + stopConsumer() + <-consumerDone + + mu.Lock() + defer mu.Unlock() + for i := 1; i < len(persisted); i++ { + require.GreaterOrEqual(t, persisted[i], persisted[i-1], + "persisted checkpoint regressed at index %d: %v", i, persisted) + } +} + func newTestBatchPublisher(t *testing.T) (*batchPublisher, func() []replication.SCN) { t.Helper() From ff3edc07971004a8b5c0607020bc18572ae987e7 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Fri, 7 Aug 2026 10:29:45 -0400 Subject: [PATCH 06/24] oracledb_cdc: fail the snapshot gate on nack, drop orphaned publishBatch 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. --- internal/impl/oracledb/batcher.go | 72 ++++++--- internal/impl/oracledb/batcher_test.go | 155 +++++++++++++------ internal/impl/oracledb/input_oracledb_cdc.go | 3 + 3 files changed, 157 insertions(+), 73 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index bdf14c7b11..6c2a90d9c9 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -38,6 +38,11 @@ type batchPublisher struct { // acknowledged downstream. The snapshot->streaming handoff blocks on it so // the post-snapshot SCN is never persisted while snapshot rows are in flight. snapshotAckWG sync.WaitGroup + // snapshotNackErr records the first snapshot batch nack. auto_replay_nacks + // is user-toggleable, so a nack can be terminal: the gate must fail rather + // than let the post-snapshot SCN persist over undelivered rows. + snapshotNackMu sync.Mutex + snapshotNackErr error log *service.Logger shutSig *shutdown.Signaller @@ -94,7 +99,7 @@ func (p *batchPublisher) loop() { flushBatch = flushBatchTicker.C } - // hardStopCtx survives a soft stop so that an in-flight publishBatch send can + // hardStopCtx survives a soft stop so that an in-flight sendTracked send can // complete before the loop exits. Only a hard stop (triggered by Close) // cancels it, which is the forced-shutdown last resort. hardStopCtx, done := p.shutSig.HardStopCtx(context.Background()) @@ -246,23 +251,6 @@ type trackedBatch struct { isSnapshot bool } -// publishBatch tracks and sends a batch that was flushed elsewhere. Callers -// that flush the batcher themselves must instead track under the same lock as -// their flush (see Publish/loop/flushCurrent) to keep Track order == flush -// order. -func (b *batchPublisher) publishBatch(ctx context.Context, batch service.MessageBatch) error { - if len(batch) == 0 { - return nil - } - b.batcherMu.Lock() - tracked, err := b.trackBatchLocked(ctx, batch) - b.batcherMu.Unlock() - if err != nil { - return err - } - return b.sendTracked(ctx, tracked) -} - // trackBatchLocked registers the batch with the ordered checkpoint tracker and // builds its ack function. It MUST be called with batcherMu held: Track order // defines the checkpoint sequence, so it has to match flush order exactly. @@ -304,10 +292,21 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes isSnapshot: isSnapshotBatch, msg: asyncMessage{ msg: batch, - ackFn: func(ctx context.Context, _ error) error { + ackFn: func(ctx context.Context, err error) error { if isSnapshotBatch { defer b.snapshotAckWG.Done() } + if err != nil { + // auto_replay_nacks is user-toggleable, so a nack can be + // terminal. Never resolve: the checkpoint stays pinned + // before this batch so nothing can be persisted past its + // undelivered rows. Snapshot nacks additionally fail the + // handoff gate so the post-snapshot SCN is not persisted. + if isSnapshotBatch { + b.recordSnapshotNack(err) + } + return err + } scn := resolveFn() if scn == nil || !scn.IsValid() { return nil @@ -323,6 +322,26 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes }, nil } +func (b *batchPublisher) recordSnapshotNack(err error) { + b.snapshotNackMu.Lock() + defer b.snapshotNackMu.Unlock() + if b.snapshotNackErr == nil { + b.snapshotNackErr = err + } +} + +// resetSnapshotGate clears any nack recorded by a previous snapshot attempt so +// the gate reflects only the current run: the publisher outlives reconnects, +// and a stale error would fail every retry even after a clean re-run. The +// WaitGroup is deliberately left untouched — batches from a previous attempt +// that are still in flight can yet be acked or nacked, and both must keep +// counting. +func (b *batchPublisher) resetSnapshotGate() { + b.snapshotNackMu.Lock() + defer b.snapshotNackMu.Unlock() + b.snapshotNackErr = nil +} + // sendTracked hands a tracked batch to ReadBatch. Must be called WITHOUT // batcherMu held (the send blocks until consumed). A failed send releases the // batch's snapshot-gate slot. @@ -339,10 +358,10 @@ func (b *batchPublisher) sendTracked(ctx context.Context, tracked *trackedBatch) } // waitSnapshotAcks blocks until every published snapshot batch has been -// acknowledged (or nacked) downstream, or until ctx is cancelled. Nacked -// batches release the gate too: redelivery is owned by auto_replay_nacks, -// and the ctx escape prevents a permanently-failing downstream from -// wedging shutdown. +// acknowledged or nacked downstream, or until ctx is cancelled (the escape +// prevents a stalled downstream from wedging shutdown). Any nack fails the +// gate: with auto_replay_nacks disabled a nack is terminal, so the +// post-snapshot SCN must not be persisted and the snapshot must re-run. func (b *batchPublisher) waitSnapshotAcks(ctx context.Context) error { drained := make(chan struct{}) go func() { @@ -352,6 +371,11 @@ func (b *batchPublisher) waitSnapshotAcks(ctx context.Context) error { }() select { case <-drained: + b.snapshotNackMu.Lock() + defer b.snapshotNackMu.Unlock() + if b.snapshotNackErr != nil { + return fmt.Errorf("snapshot batch was rejected downstream: %w", b.snapshotNackErr) + } return nil case <-ctx.Done(): return ctx.Err() @@ -408,7 +432,7 @@ func (b *batchPublisher) FlushRemaining(ctx context.Context) error { } // Close signals the publisher's loop goroutine to stop and waits for it to exit. -// TriggerHardStop cancels the HardStopCtx used by publishBatch, unblocking any +// TriggerHardStop cancels the HardStopCtx used by the flush loop, unblocking any // send that is waiting on msgChan when no consumer is left. func (b *batchPublisher) Close() { b.shutSig.TriggerSoftStop() diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 48caa8dec8..9546d8d1c3 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -28,8 +28,8 @@ func TestPublishBatch(t *testing.T) { ctx := t.Context() publisher, cachedSCNs := newTestBatchPublisher(t) - msg := publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) - msg1 := publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) + msg := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) + msg1 := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) require.NoError(t, msg.ackFn(ctx, nil)) require.Empty(t, cachedSCNs(), "cacheSCN must not be called after acking only the first snapshot batch") @@ -42,8 +42,7 @@ func TestPublishBatch(t *testing.T) { ctx := t.Context() publisher, cachedSCNs := newTestBatchPublisher(t) - batch := service.MessageBatch{newStreamingMessage("200")} - msg := publishAndReceive(t, ctx, publisher, batch) + msg := publishAndReceive(t, ctx, publisher, streamingEvent(200)) require.NoError(t, msg.ackFn(ctx, nil)) scns := cachedSCNs() @@ -53,13 +52,21 @@ func TestPublishBatch(t *testing.T) { t.Run("mixed snapshot and streaming batch persists", func(t *testing.T) { ctx := t.Context() - publisher, cachedSCNs := newTestBatchPublisher(t) + // Count=2 groups the snapshot and streaming events into one batch. + publisher, cachedSCNs := newTestBatchPublisherWithCount(t, 2) - batch := service.MessageBatch{ - newSnapshotMessage("100"), - newStreamingMessage("300"), + got := make(chan asyncMessage, 1) + go func() { got <- <-publisher.msgs() }() + require.NoError(t, publisher.Publish(ctx, snapshotEvent(100))) + require.NoError(t, publisher.Publish(ctx, streamingEvent(300))) + + var msg asyncMessage + select { + case msg = <-got: + require.Len(t, msg.msg, 2) + case <-time.After(5 * time.Second): + t.Fatal("mixed batch was never published") } - msg := publishAndReceive(t, ctx, publisher, batch) require.NoError(t, msg.ackFn(ctx, nil)) scns := cachedSCNs() @@ -71,8 +78,8 @@ func TestPublishBatch(t *testing.T) { ctx := t.Context() publisher, cachedSCNs := newTestBatchPublisher(t) - snapshotMsg := publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) - streamingMsg := publishAndReceive(t, ctx, publisher, service.MessageBatch{newStreamingMessage("200")}) + snapshotMsg := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) + streamingMsg := publishAndReceive(t, ctx, publisher, streamingEvent(200)) require.NoError(t, streamingMsg.ackFn(ctx, nil)) require.Empty(t, cachedSCNs(), "cacheSCN must not be called while the snapshot batch is still the unresolved head") @@ -82,6 +89,22 @@ func TestPublishBatch(t *testing.T) { require.Len(t, scns, 1, "expected cacheSCN to be called exactly once when the snapshot batch resolves the streaming SCN") require.Equal(t, replication.SCN(200), scns[0], "expected the streaming batch's SCN to survive the out-of-order snapshot ack") }) + + t.Run("a nacked batch pins the checkpoint", func(t *testing.T) { + ctx := t.Context() + publisher, cachedSCNs := newTestBatchPublisher(t) + + b1 := publishAndReceive(t, ctx, publisher, streamingEvent(200)) + b2 := publishAndReceive(t, ctx, publisher, streamingEvent(300)) + + // Nack b1: with auto_replay_nacks disabled this is terminal, so b2's + // ack must not persist anything past the undelivered b1. + nackErr := errors.New("downstream failure") + require.ErrorIs(t, b1.ackFn(ctx, nackErr), nackErr) + require.NoError(t, b2.ackFn(ctx, nil)) + + require.Empty(t, cachedSCNs(), "a checkpoint must never be persisted past a nacked batch") + }) } func TestSnapshotAckGate(t *testing.T) { @@ -89,7 +112,7 @@ func TestSnapshotAckGate(t *testing.T) { ctx := t.Context() publisher, _ := newTestBatchPublisher(t) - msg := publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) + msg := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) done := make(chan error, 1) go func() { done <- publisher.waitSnapshotAcks(ctx) }() @@ -109,14 +132,20 @@ func TestSnapshotAckGate(t *testing.T) { } }) - t.Run("a nack also releases the gate", func(t *testing.T) { + t.Run("a nack releases the gate but fails it", func(t *testing.T) { ctx := t.Context() - publisher, _ := newTestBatchPublisher(t) + publisher, cachedSCNs := newTestBatchPublisher(t) - msg := publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) - require.NoError(t, msg.ackFn(ctx, errors.New("downstream failure"))) + msg := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) + nackErr := errors.New("downstream failure") + require.ErrorIs(t, msg.ackFn(ctx, nackErr), nackErr) - require.NoError(t, publisher.waitSnapshotAcks(ctx)) + // auto_replay_nacks is user-toggleable, so a nack can be terminal: + // the gate must report it so the post-snapshot SCN is not persisted + // and the snapshot re-runs on restart. + err := publisher.waitSnapshotAcks(ctx) + require.ErrorIs(t, err, nackErr) + require.Empty(t, cachedSCNs()) }) t.Run("streaming batches do not hold the gate", func(t *testing.T) { @@ -124,16 +153,36 @@ func TestSnapshotAckGate(t *testing.T) { publisher, _ := newTestBatchPublisher(t) // Published but never acked: must not block the gate. - publishAndReceive(t, ctx, publisher, service.MessageBatch{newStreamingMessage("200")}) + publishAndReceive(t, ctx, publisher, streamingEvent(200)) require.NoError(t, publisher.waitSnapshotAcks(ctx)) }) + t.Run("a nack fails only the snapshot attempt it belongs to", func(t *testing.T) { + ctx := t.Context() + publisher, cachedSCNs := newTestBatchPublisher(t) + + // Run 1: a snapshot batch is nacked; the gate fails. + msg := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) + nackErr := errors.New("downstream failure") + require.ErrorIs(t, msg.ackFn(ctx, nackErr), nackErr) + require.ErrorIs(t, publisher.waitSnapshotAcks(ctx), nackErr) + + // Run 2 (reconnect reuses the publisher): the gate is reset, the + // re-run snapshot acks cleanly, and the gate must pass — a stale + // run-1 error here would livelock the input re-snapshotting forever. + publisher.resetSnapshotGate() + msg2 := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) + require.NoError(t, msg2.ackFn(ctx, nil)) + require.NoError(t, publisher.waitSnapshotAcks(ctx)) + require.Empty(t, cachedSCNs()) + }) + t.Run("context cancellation escapes the gate", func(t *testing.T) { publisher, _ := newTestBatchPublisher(t) ctx, cancel := context.WithCancel(t.Context()) - publishAndReceive(t, ctx, publisher, service.MessageBatch{newSnapshotMessage("100")}) + publishAndReceive(t, ctx, publisher, snapshotEvent(100)) done := make(chan error, 1) go func() { done <- publisher.waitSnapshotAcks(ctx) }() @@ -150,25 +199,14 @@ func TestSnapshotAckGate(t *testing.T) { func TestFlushCurrent(t *testing.T) { ctx := t.Context() - logger := service.NewLoggerFromSlog(slog.Default()) - cp := checkpoint.NewCapped[replication.SCN](100) - - batcher, err := (service.BatchPolicy{Count: 100}).NewBatcher(service.MockResources()) - require.NoError(t, err) - - publisher := newBatchPublisher(batcher, cp, logger) - t.Cleanup(publisher.Close) - publisher.cacheSCN = func(context.Context, replication.SCN) error { return nil } + // Count=100 keeps published events buffered in the batcher until flushed. + publisher, _ := newTestBatchPublisherWithCount(t, 100) publishEvent := func(v int) { t.Helper() - require.NoError(t, publisher.Publish(ctx, &replication.MessageEvent{ - Schema: "S", - Table: "T", - Operation: replication.MessageOperationRead, - Data: map[string]any{"a": v}, - SCN: replication.SCN(100), - })) + e := snapshotEvent(100) + e.Data = map[string]any{"a": v} + require.NoError(t, publisher.Publish(ctx, e)) } receive := func(failMsg string) { t.Helper() @@ -183,7 +221,6 @@ func TestFlushCurrent(t *testing.T) { } } - // Count=100 keeps a single event buffered in the batcher until flushed. publishEvent(1) receive("flushCurrent did not publish the buffered partial batch") @@ -266,13 +303,24 @@ func TestTrackOrderUnderConcurrentFlush(t *testing.T) { } } +// newTestBatchPublisher builds a publisher whose batcher flushes on every +// published event (count=1), so tests drive the production +// Publish->trackBatchLocked->sendTracked path directly. func newTestBatchPublisher(t *testing.T) (*batchPublisher, func() []replication.SCN) { t.Helper() + return newTestBatchPublisherWithCount(t, 1) +} + +func newTestBatchPublisherWithCount(t *testing.T, count int) (*batchPublisher, func() []replication.SCN) { + t.Helper() logger := service.NewLoggerFromSlog(slog.Default()) cp := checkpoint.NewCapped[replication.SCN](100) - publisher := newBatchPublisher(nil, cp, logger) + batcher, err := (service.BatchPolicy{Count: count}).NewBatcher(service.MockResources()) + require.NoError(t, err) + + publisher := newBatchPublisher(batcher, cp, logger) t.Cleanup(publisher.Close) var ( @@ -295,24 +343,33 @@ func newTestBatchPublisher(t *testing.T) (*batchPublisher, func() []replication. return publisher, cachedSCNsFn } -func newSnapshotMessage(scn string) *service.Message { - msg := service.NewMessage([]byte("{}")) - msg.MetaSet("operation", replication.MessageOperationRead.String()) - msg.MetaSet("scn", scn) - return msg +func snapshotEvent(scn replication.SCN) *replication.MessageEvent { + return &replication.MessageEvent{ + Schema: "S", + Table: "T", + Operation: replication.MessageOperationRead, + SCN: scn, + Data: map[string]any{"a": 1}, + } } -func newStreamingMessage(checkpointSCN string) *service.Message { - msg := service.NewMessage([]byte("{}")) - msg.MetaSet("operation", replication.MessageOperationInsert.String()) - msg.MetaSet("checkpoint_scn", checkpointSCN) - return msg +func streamingEvent(checkpointSCN replication.SCN) *replication.MessageEvent { + return &replication.MessageEvent{ + Schema: "S", + Table: "T", + Operation: replication.MessageOperationInsert, + CheckpointSCN: checkpointSCN, + Data: map[string]any{"a": 1}, + } } -func publishAndReceive(t *testing.T, ctx context.Context, publisher *batchPublisher, batch service.MessageBatch) asyncMessage { +// publishAndReceive publishes a single event through the production Publish +// path (count=1 batcher: every event flushes, tracks, and sends immediately) +// and returns the delivered asyncMessage. +func publishAndReceive(t *testing.T, ctx context.Context, publisher *batchPublisher, event *replication.MessageEvent) asyncMessage { t.Helper() go func() { - _ = publisher.publishBatch(ctx, batch) + _ = publisher.Publish(ctx, event) }() return <-publisher.msgs() } diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index 58371b0ca5..5292644185 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -607,6 +607,9 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { // snapshot if no SCN exists then store checkpoint once complete if snapshotter != nil { + // The publisher outlives reconnects: clear any nack recorded by a + // previous snapshot attempt so the gate judges only this run. + o.publisher.resetSnapshotGate() if startSCN, err = o.processSnapshot(softCtx, snapshotter); err != nil { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { o.log.Infof("Snapshotting stopped: %s", err) From a11a70057703b8953abfa434e12a7d339466d683 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Mon, 10 Aug 2026 10:45:26 -0400 Subject: [PATCH 07/24] oracledb_cdc: log downstream batch rejections 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 SCN, whether it was a snapshot batch, and the pinned-checkpoint consequence so operators can connect a stalled input to the downstream rejection. --- internal/impl/oracledb/batcher.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 6c2a90d9c9..3ca833a082 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -305,6 +305,7 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes if isSnapshotBatch { b.recordSnapshotNack(err) } + b.log.Errorf("Batch rejected downstream (snapshot=%v, checkpoint SCN %d): the checkpoint is now pinned before this batch and the input will stall once checkpoint_limit is reached, unless the batch is redelivered (auto_replay_nacks) or the pipeline restarts: %v", isSnapshotBatch, checkpointSCN, err) return err } scn := resolveFn() From f16d3cda221ceed4260e266e4a1588712adfcc16 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Mon, 10 Aug 2026 16:00:42 -0400 Subject: [PATCH 08/24] oracledb_cdc: log downstream snapshot rejections at error level The snapshot ack gate collapses soft-stop cancellation and downstream rejection into one Info line whose wording only describes the former. A rejection discards the post-snapshot SCN and re-runs the whole snapshot - an unexpected, data-affecting outcome - so it now logs at error level with wording that names it, mirroring the cancellation/error split used by the surrounding branches. --- internal/impl/oracledb/input_oracledb_cdc.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index 5292644185..6bdebf389c 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -632,7 +632,11 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { return } if err = o.publisher.waitSnapshotAcks(softCtx); err != nil { - o.log.Infof("Interrupted while waiting for snapshot acknowledgements. Snapshot will re-run on restart (may cause duplicate data): %s", err) + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + o.log.Infof("Interrupted while waiting for snapshot acknowledgements. Snapshot will re-run on restart (may cause duplicate data): %s", err) + } else { + o.log.Errorf("Snapshot batch was rejected downstream. Snapshot will re-run on restart (may cause duplicate data): %s", err) + } o.stopSig.TriggerHasStopped() return } From 88c7f78f2df49b1fa077bc4bd66afd682049c46e Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Mon, 10 Aug 2026 16:30:53 -0400 Subject: [PATCH 09/24] oracledb_cdc: terminal nacks restart with a fresh tracker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/impl/oracledb/batcher.go | 34 ++++++++++++++- internal/impl/oracledb/batcher_test.go | 39 +++++++++++++++++ internal/impl/oracledb/input_oracledb_cdc.go | 44 ++++++++++++++++---- 3 files changed, 108 insertions(+), 9 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 3ca833a082..b8862f3249 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -14,6 +14,7 @@ import ( "fmt" "strconv" "sync" + "sync/atomic" "time" "github.com/Jeffail/checkpoint" @@ -44,6 +45,15 @@ type batchPublisher struct { snapshotNackMu sync.Mutex snapshotNackErr error + // onTerminalNack, when set, is invoked once a batch is rejected + // downstream: a nack pins the ordered tracker, so the input must restart + // (with a fresh publisher) to resume from the last durable SCN. + onTerminalNack func(error) + // sealed marks a publisher that has been replaced by a reconnect. Late + // acks from its session must not persist checkpoints (they could regress + // the new session's positions) nor trigger restarts. + sealed atomic.Bool + log *service.Logger shutSig *shutdown.Signaller } @@ -301,13 +311,28 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes // terminal. Never resolve: the checkpoint stays pinned // before this batch so nothing can be persisted past its // undelivered rows. Snapshot nacks additionally fail the - // handoff gate so the post-snapshot SCN is not persisted. + // handoff gate so the post-snapshot SCN is not persisted, + // and the input restarts with a fresh tracker to resume + // from the last durable SCN (the pinned slot would + // otherwise wedge checkpointing for the process lifetime). if isSnapshotBatch { b.recordSnapshotNack(err) } - b.log.Errorf("Batch rejected downstream (snapshot=%v, checkpoint SCN %d): the checkpoint is now pinned before this batch and the input will stall once checkpoint_limit is reached, unless the batch is redelivered (auto_replay_nacks) or the pipeline restarts: %v", isSnapshotBatch, checkpointSCN, err) + if b.sealed.Load() { + return err + } + b.log.Errorf("Batch rejected downstream (snapshot=%v, checkpoint SCN %d): restarting to redeliver from the last durable checkpoint: %v", isSnapshotBatch, checkpointSCN, err) + if b.onTerminalNack != nil { + b.onTerminalNack(err) + } return err } + if b.sealed.Load() { + // A late ack from a replaced session: resolving its own + // tracker is harmless, but persisting could regress the + // new session's checkpoints. + return nil + } scn := resolveFn() if scn == nil || !scn.IsValid() { return nil @@ -323,6 +348,11 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes }, nil } +// seal marks the publisher as replaced; see the sealed field. +func (b *batchPublisher) seal() { + b.sealed.Store(true) +} + func (b *batchPublisher) recordSnapshotNack(err error) { b.snapshotNackMu.Lock() defer b.snapshotNackMu.Unlock() diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 9546d8d1c3..7c221eab8c 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -13,6 +13,7 @@ import ( "errors" "log/slog" "sync" + "sync/atomic" "testing" "time" @@ -230,6 +231,44 @@ func TestFlushCurrent(t *testing.T) { receive("publisher loop no longer functional after flushCurrent") } +func TestTerminalNack(t *testing.T) { + t.Run("invokes onTerminalNack so the input can restart", func(t *testing.T) { + ctx := t.Context() + publisher, cachedSCNs := newTestBatchPublisher(t) + + var got atomic.Value + publisher.onTerminalNack = func(err error) { got.Store(err) } + + am := publishAndReceive(t, ctx, publisher, streamingEvent(200)) + nackErr := errors.New("downstream failure") + require.ErrorIs(t, am.ackFn(ctx, nackErr), nackErr) + + stored, _ := got.Load().(error) + require.ErrorIs(t, stored, nackErr) + require.Empty(t, cachedSCNs()) + }) + + t.Run("a sealed publisher neither persists nor restarts", func(t *testing.T) { + ctx := t.Context() + publisher, cachedSCNs := newTestBatchPublisher(t) + + restarted := false + publisher.onTerminalNack = func(error) { restarted = true } + + am1 := publishAndReceive(t, ctx, publisher, streamingEvent(200)) + am2 := publishAndReceive(t, ctx, publisher, streamingEvent(300)) + publisher.seal() + + // Late ack from a replaced session: must not persist. + require.NoError(t, am1.ackFn(ctx, nil)) + require.Empty(t, cachedSCNs(), "a sealed publisher must not persist checkpoints") + + // Late nack: must not trigger a restart of the new session. + require.Error(t, am2.ackFn(ctx, errors.New("late failure"))) + require.False(t, restarted, "a sealed publisher must not trigger restarts") + }) +} + // TestTrackOrderUnderConcurrentFlush stresses the two concurrent flushers (the // count-triggered flush in Publish and the timed-flush loop) and asserts the // persisted checkpoint never regresses when batches are acked in delivery diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index 6bdebf389c..ea75555960 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -261,6 +261,13 @@ type oracleDBCDCInput struct { publisher *batchPublisher metrics *service.Metrics + // batching and checkpointLimit rebuild the publisher (batcher + ordered + // checkpoint tracker) on every Connect: a terminal nack pins a tracker + // slot by design, and only a fresh tracker lets the restart resume from + // the last durable SCN instead of staying wedged behind the stale slot. + batching service.BatchPolicy + checkpointLimit int + stopSig *shutdown.Signaller snapshotOnlyDone atomic.Bool log *service.Logger @@ -403,13 +410,15 @@ func newOracleDBCDCInput(conf *service.ParsedConfig, resources *service.Resource Exclude: tableExcludes, }, }, - lmCfg: lmCfg, - res: resources, - log: logger, - metrics: resources.Metrics(), - stopSig: shutdown.NewSignaller(), - publisher: newBatchPublisher(batcher, cp, logger), - cpCache: cpCache, + lmCfg: lmCfg, + res: resources, + log: logger, + metrics: resources.Metrics(), + stopSig: shutdown.NewSignaller(), + publisher: newBatchPublisher(batcher, cp, logger), + batching: policy, + checkpointLimit: checkpointLimit, + cpCache: cpCache, } defer func() { @@ -531,7 +540,28 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { o.log.Warnf("Failed to pre-fetch schema for %s.%s: %v", t.Schema, t.Name, err) } } + + // Rebuild the publisher (batcher + ordered checkpoint tracker) for this + // connection attempt. A terminal nack pins a tracker slot by design; + // reusing the old tracker would leave every future checkpoint stuck + // behind the stale slot, wedging the input for the process lifetime + // instead of letting this restart resume from the last durable SCN. The + // old publisher is sealed so late acks from the previous session cannot + // persist stale positions. + o.publisher.seal() + o.publisher.Close() + newBatcher, err := o.batching.NewBatcher(o.res) + if err != nil { + return fmt.Errorf("creating batcher: %w", err) + } + o.publisher = newBatchPublisher(newBatcher, checkpoint.NewCapped[replication.SCN](int64(o.checkpointLimit)), o.log) + o.publisher.cacheSCN = o.cacheSCN o.publisher.schemas = schemas + o.publisher.onTerminalNack = func(error) { + // o.stopSig is only replaced while the input is stopped, and sealed + // publishers never invoke this, so the signaller here is current. + o.stopSig.TriggerSoftStop() + } if cachedSCN, err = o.getCachedSCN(ctx); err != nil { if errors.Is(err, service.ErrKeyNotFound) { From c25b42f53ae393800d77050589ad142b75053334 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Tue, 11 Aug 2026 10:00:32 -0400 Subject: [PATCH 10/24] oracledb_cdc: nacks resolve checkpoints (auto_replay_nacks off is an opt-in drop) Unwinds the nack-pinning and terminal-nack-restart changes from the review rounds. Per the framework's documented contract for auto_replay_nacks ("If set to false these messages will instead be deleted"), disabling replay is an explicit opt-in to drop rejected messages - typically because failures are routed to a DLQ, which acks. Pinning the checkpoint (or restarting to force redelivery) contradicted that contract: pinning produced permanent backpressure once checkpoint_limit filled, and the restart variant turned a persistently-failing message into an infinite redelivery loop. The snapshot ack gate still guards the crash window; a nack now simply settles its slot and the stream continues. --- internal/impl/oracledb/batcher.go | 91 +++----------------- internal/impl/oracledb/batcher_test.go | 90 +++---------------- internal/impl/oracledb/input_oracledb_cdc.go | 52 ++--------- 3 files changed, 33 insertions(+), 200 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index b8862f3249..1b92dd5ad5 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -14,7 +14,6 @@ import ( "fmt" "strconv" "sync" - "sync/atomic" "time" "github.com/Jeffail/checkpoint" @@ -39,23 +38,8 @@ type batchPublisher struct { // acknowledged downstream. The snapshot->streaming handoff blocks on it so // the post-snapshot SCN is never persisted while snapshot rows are in flight. snapshotAckWG sync.WaitGroup - // snapshotNackErr records the first snapshot batch nack. auto_replay_nacks - // is user-toggleable, so a nack can be terminal: the gate must fail rather - // than let the post-snapshot SCN persist over undelivered rows. - snapshotNackMu sync.Mutex - snapshotNackErr error - - // onTerminalNack, when set, is invoked once a batch is rejected - // downstream: a nack pins the ordered tracker, so the input must restart - // (with a fresh publisher) to resume from the last durable SCN. - onTerminalNack func(error) - // sealed marks a publisher that has been replaced by a reconnect. Late - // acks from its session must not persist checkpoints (they could regress - // the new session's positions) nor trigger restarts. - sealed atomic.Bool - - log *service.Logger - shutSig *shutdown.Signaller + log *service.Logger + shutSig *shutdown.Signaller } // newBatchPublisher creates an instance of batchPublisher. @@ -302,37 +286,14 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes isSnapshot: isSnapshotBatch, msg: asyncMessage{ msg: batch, - ackFn: func(ctx context.Context, err error) error { + // The ack error is deliberately ignored: nacks are replayed by + // auto_replay_nacks (the default), and disabling that is a + // documented opt-in to DROP rejected messages, so the checkpoint + // must advance past them rather than pin the tracker. + ackFn: func(ctx context.Context, _ error) error { if isSnapshotBatch { defer b.snapshotAckWG.Done() } - if err != nil { - // auto_replay_nacks is user-toggleable, so a nack can be - // terminal. Never resolve: the checkpoint stays pinned - // before this batch so nothing can be persisted past its - // undelivered rows. Snapshot nacks additionally fail the - // handoff gate so the post-snapshot SCN is not persisted, - // and the input restarts with a fresh tracker to resume - // from the last durable SCN (the pinned slot would - // otherwise wedge checkpointing for the process lifetime). - if isSnapshotBatch { - b.recordSnapshotNack(err) - } - if b.sealed.Load() { - return err - } - b.log.Errorf("Batch rejected downstream (snapshot=%v, checkpoint SCN %d): restarting to redeliver from the last durable checkpoint: %v", isSnapshotBatch, checkpointSCN, err) - if b.onTerminalNack != nil { - b.onTerminalNack(err) - } - return err - } - if b.sealed.Load() { - // A late ack from a replaced session: resolving its own - // tracker is harmless, but persisting could regress the - // new session's checkpoints. - return nil - } scn := resolveFn() if scn == nil || !scn.IsValid() { return nil @@ -348,31 +309,6 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes }, nil } -// seal marks the publisher as replaced; see the sealed field. -func (b *batchPublisher) seal() { - b.sealed.Store(true) -} - -func (b *batchPublisher) recordSnapshotNack(err error) { - b.snapshotNackMu.Lock() - defer b.snapshotNackMu.Unlock() - if b.snapshotNackErr == nil { - b.snapshotNackErr = err - } -} - -// resetSnapshotGate clears any nack recorded by a previous snapshot attempt so -// the gate reflects only the current run: the publisher outlives reconnects, -// and a stale error would fail every retry even after a clean re-run. The -// WaitGroup is deliberately left untouched — batches from a previous attempt -// that are still in flight can yet be acked or nacked, and both must keep -// counting. -func (b *batchPublisher) resetSnapshotGate() { - b.snapshotNackMu.Lock() - defer b.snapshotNackMu.Unlock() - b.snapshotNackErr = nil -} - // sendTracked hands a tracked batch to ReadBatch. Must be called WITHOUT // batcherMu held (the send blocks until consumed). A failed send releases the // batch's snapshot-gate slot. @@ -389,10 +325,10 @@ func (b *batchPublisher) sendTracked(ctx context.Context, tracked *trackedBatch) } // waitSnapshotAcks blocks until every published snapshot batch has been -// acknowledged or nacked downstream, or until ctx is cancelled (the escape -// prevents a stalled downstream from wedging shutdown). Any nack fails the -// gate: with auto_replay_nacks disabled a nack is terminal, so the -// post-snapshot SCN must not be persisted and the snapshot must re-run. +// acknowledged (or nacked) downstream, or until ctx is cancelled. Nacked +// batches release the gate too: redelivery is owned by auto_replay_nacks, +// and disabling that is a documented opt-in to drop rejections. The ctx +// escape prevents a permanently-failing downstream from wedging shutdown. func (b *batchPublisher) waitSnapshotAcks(ctx context.Context) error { drained := make(chan struct{}) go func() { @@ -402,11 +338,6 @@ func (b *batchPublisher) waitSnapshotAcks(ctx context.Context) error { }() select { case <-drained: - b.snapshotNackMu.Lock() - defer b.snapshotNackMu.Unlock() - if b.snapshotNackErr != nil { - return fmt.Errorf("snapshot batch was rejected downstream: %w", b.snapshotNackErr) - } return nil case <-ctx.Done(): return ctx.Err() diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 7c221eab8c..c518af48ab 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -13,7 +13,6 @@ import ( "errors" "log/slog" "sync" - "sync/atomic" "testing" "time" @@ -91,20 +90,22 @@ func TestPublishBatch(t *testing.T) { require.Equal(t, replication.SCN(200), scns[0], "expected the streaming batch's SCN to survive the out-of-order snapshot ack") }) - t.Run("a nacked batch pins the checkpoint", func(t *testing.T) { + t.Run("a nack resolves too: auto_replay_nacks off is an opt-in drop", func(t *testing.T) { ctx := t.Context() publisher, cachedSCNs := newTestBatchPublisher(t) b1 := publishAndReceive(t, ctx, publisher, streamingEvent(200)) b2 := publishAndReceive(t, ctx, publisher, streamingEvent(300)) - // Nack b1: with auto_replay_nacks disabled this is terminal, so b2's - // ack must not persist anything past the undelivered b1. - nackErr := errors.New("downstream failure") - require.ErrorIs(t, b1.ackFn(ctx, nackErr), nackErr) + // A nacked batch is deleted per the auto_replay_nacks contract: its + // slot resolves so the stream continues past it instead of pinning + // the tracker and back-pressuring forever. + require.NoError(t, b1.ackFn(ctx, errors.New("downstream failure"))) require.NoError(t, b2.ackFn(ctx, nil)) - require.Empty(t, cachedSCNs(), "a checkpoint must never be persisted past a nacked batch") + scns := cachedSCNs() + require.NotEmpty(t, scns, "the checkpoint must continue advancing past a dropped batch") + require.Equal(t, replication.SCN(300), scns[len(scns)-1]) }) } @@ -133,20 +134,15 @@ func TestSnapshotAckGate(t *testing.T) { } }) - t.Run("a nack releases the gate but fails it", func(t *testing.T) { + t.Run("a nack also releases the gate", func(t *testing.T) { ctx := t.Context() - publisher, cachedSCNs := newTestBatchPublisher(t) + publisher, _ := newTestBatchPublisher(t) msg := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) - nackErr := errors.New("downstream failure") - require.ErrorIs(t, msg.ackFn(ctx, nackErr), nackErr) - - // auto_replay_nacks is user-toggleable, so a nack can be terminal: - // the gate must report it so the post-snapshot SCN is not persisted - // and the snapshot re-runs on restart. - err := publisher.waitSnapshotAcks(ctx) - require.ErrorIs(t, err, nackErr) - require.Empty(t, cachedSCNs()) + // Nacks count as settled: replay is owned by auto_replay_nacks, and + // disabling it is a documented opt-in to drop rejections. + require.NoError(t, msg.ackFn(ctx, errors.New("downstream failure"))) + require.NoError(t, publisher.waitSnapshotAcks(ctx)) }) t.Run("streaming batches do not hold the gate", func(t *testing.T) { @@ -159,26 +155,6 @@ func TestSnapshotAckGate(t *testing.T) { require.NoError(t, publisher.waitSnapshotAcks(ctx)) }) - t.Run("a nack fails only the snapshot attempt it belongs to", func(t *testing.T) { - ctx := t.Context() - publisher, cachedSCNs := newTestBatchPublisher(t) - - // Run 1: a snapshot batch is nacked; the gate fails. - msg := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) - nackErr := errors.New("downstream failure") - require.ErrorIs(t, msg.ackFn(ctx, nackErr), nackErr) - require.ErrorIs(t, publisher.waitSnapshotAcks(ctx), nackErr) - - // Run 2 (reconnect reuses the publisher): the gate is reset, the - // re-run snapshot acks cleanly, and the gate must pass — a stale - // run-1 error here would livelock the input re-snapshotting forever. - publisher.resetSnapshotGate() - msg2 := publishAndReceive(t, ctx, publisher, snapshotEvent(100)) - require.NoError(t, msg2.ackFn(ctx, nil)) - require.NoError(t, publisher.waitSnapshotAcks(ctx)) - require.Empty(t, cachedSCNs()) - }) - t.Run("context cancellation escapes the gate", func(t *testing.T) { publisher, _ := newTestBatchPublisher(t) @@ -231,44 +207,6 @@ func TestFlushCurrent(t *testing.T) { receive("publisher loop no longer functional after flushCurrent") } -func TestTerminalNack(t *testing.T) { - t.Run("invokes onTerminalNack so the input can restart", func(t *testing.T) { - ctx := t.Context() - publisher, cachedSCNs := newTestBatchPublisher(t) - - var got atomic.Value - publisher.onTerminalNack = func(err error) { got.Store(err) } - - am := publishAndReceive(t, ctx, publisher, streamingEvent(200)) - nackErr := errors.New("downstream failure") - require.ErrorIs(t, am.ackFn(ctx, nackErr), nackErr) - - stored, _ := got.Load().(error) - require.ErrorIs(t, stored, nackErr) - require.Empty(t, cachedSCNs()) - }) - - t.Run("a sealed publisher neither persists nor restarts", func(t *testing.T) { - ctx := t.Context() - publisher, cachedSCNs := newTestBatchPublisher(t) - - restarted := false - publisher.onTerminalNack = func(error) { restarted = true } - - am1 := publishAndReceive(t, ctx, publisher, streamingEvent(200)) - am2 := publishAndReceive(t, ctx, publisher, streamingEvent(300)) - publisher.seal() - - // Late ack from a replaced session: must not persist. - require.NoError(t, am1.ackFn(ctx, nil)) - require.Empty(t, cachedSCNs(), "a sealed publisher must not persist checkpoints") - - // Late nack: must not trigger a restart of the new session. - require.Error(t, am2.ackFn(ctx, errors.New("late failure"))) - require.False(t, restarted, "a sealed publisher must not trigger restarts") - }) -} - // TestTrackOrderUnderConcurrentFlush stresses the two concurrent flushers (the // count-triggered flush in Publish and the timed-flush loop) and asserts the // persisted checkpoint never regresses when batches are acked in delivery diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index ea75555960..bdc06ced1a 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -261,13 +261,6 @@ type oracleDBCDCInput struct { publisher *batchPublisher metrics *service.Metrics - // batching and checkpointLimit rebuild the publisher (batcher + ordered - // checkpoint tracker) on every Connect: a terminal nack pins a tracker - // slot by design, and only a fresh tracker lets the restart resume from - // the last durable SCN instead of staying wedged behind the stale slot. - batching service.BatchPolicy - checkpointLimit int - stopSig *shutdown.Signaller snapshotOnlyDone atomic.Bool log *service.Logger @@ -410,15 +403,13 @@ func newOracleDBCDCInput(conf *service.ParsedConfig, resources *service.Resource Exclude: tableExcludes, }, }, - lmCfg: lmCfg, - res: resources, - log: logger, - metrics: resources.Metrics(), - stopSig: shutdown.NewSignaller(), - publisher: newBatchPublisher(batcher, cp, logger), - batching: policy, - checkpointLimit: checkpointLimit, - cpCache: cpCache, + lmCfg: lmCfg, + res: resources, + log: logger, + metrics: resources.Metrics(), + stopSig: shutdown.NewSignaller(), + publisher: newBatchPublisher(batcher, cp, logger), + cpCache: cpCache, } defer func() { @@ -541,27 +532,7 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { } } - // Rebuild the publisher (batcher + ordered checkpoint tracker) for this - // connection attempt. A terminal nack pins a tracker slot by design; - // reusing the old tracker would leave every future checkpoint stuck - // behind the stale slot, wedging the input for the process lifetime - // instead of letting this restart resume from the last durable SCN. The - // old publisher is sealed so late acks from the previous session cannot - // persist stale positions. - o.publisher.seal() - o.publisher.Close() - newBatcher, err := o.batching.NewBatcher(o.res) - if err != nil { - return fmt.Errorf("creating batcher: %w", err) - } - o.publisher = newBatchPublisher(newBatcher, checkpoint.NewCapped[replication.SCN](int64(o.checkpointLimit)), o.log) - o.publisher.cacheSCN = o.cacheSCN o.publisher.schemas = schemas - o.publisher.onTerminalNack = func(error) { - // o.stopSig is only replaced while the input is stopped, and sealed - // publishers never invoke this, so the signaller here is current. - o.stopSig.TriggerSoftStop() - } if cachedSCN, err = o.getCachedSCN(ctx); err != nil { if errors.Is(err, service.ErrKeyNotFound) { @@ -637,9 +608,6 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { // snapshot if no SCN exists then store checkpoint once complete if snapshotter != nil { - // The publisher outlives reconnects: clear any nack recorded by a - // previous snapshot attempt so the gate judges only this run. - o.publisher.resetSnapshotGate() if startSCN, err = o.processSnapshot(softCtx, snapshotter); err != nil { if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { o.log.Infof("Snapshotting stopped: %s", err) @@ -662,11 +630,7 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { return } if err = o.publisher.waitSnapshotAcks(softCtx); err != nil { - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - o.log.Infof("Interrupted while waiting for snapshot acknowledgements. Snapshot will re-run on restart (may cause duplicate data): %s", err) - } else { - o.log.Errorf("Snapshot batch was rejected downstream. Snapshot will re-run on restart (may cause duplicate data): %s", err) - } + o.log.Infof("Interrupted while waiting for snapshot acknowledgements. Snapshot will re-run on restart (may cause duplicate data): %s", err) o.stopSig.TriggerHasStopped() return } From c080bae80e284333edd4a59760b5e1a3d04ef9cd Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Mon, 17 Aug 2026 09:53:47 -0400 Subject: [PATCH 11/24] oracledb_cdc: unblock buffering under backpressure and rebuild the publisher after a failed send Two review findings on the flush/track path: - The flush-order/track-order atomicity fix moved checkpoint.Track 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 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. New blocked-Track buffering test proven red against the old locking. - 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 SCN past the undelivered rows), so the publisher is marked poisoned and Connect rebuilds it with a fresh tracker; the session resumes from the last durable SCN, which is necessarily before the orphaned rows. cacheSCN is now serialized and monotonic so a previous session's late acks can never regress the durable position. Also renames trackedBatch.msg to msgs (review suggestion). Full 16-test integration suite green. --- internal/impl/oracledb/batcher.go | 170 +++++++++++++------ internal/impl/oracledb/batcher_test.go | 86 ++++++++++ internal/impl/oracledb/input_oracledb_cdc.go | 59 ++++++- 3 files changed, 256 insertions(+), 59 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 1b92dd5ad5..d8e4ddf0ca 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -14,6 +14,7 @@ import ( "fmt" "strconv" "sync" + "sync/atomic" "time" "github.com/Jeffail/checkpoint" @@ -26,8 +27,25 @@ import ( // batchPublisher is responsible processing individual events into a batch and flushing // them to the pipeline using service.Batcher. type batchPublisher struct { - batcher *service.Batcher + batcher *service.Batcher + // batcherMu guards only the batcher's buffer (Add/Flush/UntilNext). batcherMu sync.Mutex + // Flush tickets keep the checkpoint sequence exact without a lock held + // across Track: each flush takes a ticket under batcherMu - atomically + // with the Flush, so the user's batching policy stays exact - and + // Track+send admission happens in ticket order. Track can block on + // checkpoint_limit under downstream backpressure; only the admitted + // ticket holder (and flushers queued behind it) waits, while Publish + // calls keep buffering and the timed-flush ticker keeps reading + // UntilNext. + ticketMu sync.Mutex + ticketCond *sync.Cond + nextTicket uint64 // next ticket to hand out; guarded by batcherMu + admitted uint64 // next ticket allowed to Track+send; guarded by ticketMu + // poisoned is set when a tracked batch could not be handed to ReadBatch: + // its checkpoint slot can never resolve, so this publisher can never + // checkpoint past it. Connect rebuilds a poisoned publisher. + poisoned atomic.Bool checkpoint *checkpoint.Capped[replication.SCN] msgChan chan asyncMessage @@ -51,10 +69,38 @@ func newBatchPublisher(batcher *service.Batcher, checkpoint *checkpoint.Capped[r log: logger, shutSig: shutdown.NewSignaller(), } + b.ticketCond = sync.NewCond(&b.ticketMu) go b.loop() return b } +// takeTicketLocked hands out the next flush ticket. MUST be called with +// batcherMu held, atomically with the Flush that produced the batch, so +// ticket order is exactly flush order. +func (b *batchPublisher) takeTicketLocked() uint64 { + t := b.nextTicket + b.nextTicket++ + return t +} + +// admit blocks until it is ticket's turn to Track+send. Pair with release. +func (b *batchPublisher) admit(ticket uint64) { + b.ticketMu.Lock() + for b.admitted != ticket { + b.ticketCond.Wait() + } + b.ticketMu.Unlock() +} + +// release passes the sequence to the next ticket. Every taken ticket must be +// released exactly once, error paths included, or the sequence wedges. +func (b *batchPublisher) release() { + b.ticketMu.Lock() + b.admitted++ + b.ticketCond.Broadcast() + b.ticketMu.Unlock() +} + // loop creates a long-running process that periodically flushes batches by configured interval. // lifted from internal/impl/kafka/franz_reader_ordered.go func (p *batchPublisher) loop() { @@ -103,40 +149,35 @@ func (p *batchPublisher) loop() { adjustTimedFlush() select { case <-flushBatch: - var ( - tracked *trackedBatch - trackErr error - ) - - // Wrap this in a closure to make locking/unlocking easier. Track - // happens under the same lock as the flush so the checkpoint - // sequence matches flush order. - func() { + flushBatch = nil + if err := func() error { p.batcherMu.Lock() - defer p.batcherMu.Unlock() - - flushBatch = nil if tNext, exists := p.batcher.UntilNext(); !exists || tNext > 1 { // This can happen if a pushed message triggered a batch before // the last known flush period. In this case we simply enter the // loop again which readjusts our flush batch timer. - return + p.batcherMu.Unlock() + return nil } - - var sendBatch service.MessageBatch - if sendBatch, _ = p.batcher.Flush(hardStopCtx); len(sendBatch) == 0 { - return + sendBatch, _ := p.batcher.Flush(hardStopCtx) + var ticket uint64 + if len(sendBatch) > 0 { + ticket = p.takeTicketLocked() + } + p.batcherMu.Unlock() + if len(sendBatch) == 0 { + return nil } - tracked, trackErr = p.trackBatchLocked(hardStopCtx, sendBatch) - }() - if trackErr != nil { - return - } - if tracked != nil { - if err := p.sendTracked(hardStopCtx, tracked); err != nil { - return + p.admit(ticket) + defer p.release() + tracked, err := p.trackBatch(hardStopCtx, sendBatch) + if err != nil { + return err } + return p.sendTracked(hardStopCtx, tracked) + }(); err != nil { + return } case <-p.shutSig.SoftStopChan(): return @@ -211,44 +252,55 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven msg.MetaSetImmut("schema", service.ImmutableAny{V: schemaAny}) } - // Flush and Track must be atomic: Track order defines the checkpoint - // sequence, so another flusher (the timed-flush loop) must not interleave - // between our flush and our Track. Only the channel send happens outside - // the lock. - var tracked *trackedBatch + // Add and Flush are atomic under batcherMu so the user's batching policy + // stays exact, and the flush ticket taken in the same critical section + // pins this batch's position in the checkpoint sequence. Track+send then + // run outside batcherMu in ticket order: a Track blocked on + // checkpoint_limit stalls only the ticket queue, never concurrent + // buffering or the timed-flush ticker. + var ( + flushedBatch service.MessageBatch + ticket uint64 + ) b.batcherMu.Lock() if b.batcher.Add(msg) { - var flushedBatch []*service.Message if flushedBatch, err = b.batcher.Flush(ctx); err == nil && len(flushedBatch) > 0 { - tracked, err = b.trackBatchLocked(ctx, flushedBatch) + ticket = b.takeTicketLocked() } } b.batcherMu.Unlock() if err != nil { return fmt.Errorf("flushing batch due to reaching count limit: %w", err) } - - // If a batch was flushed, publish it outside the lock - if tracked != nil { - if err := b.sendTracked(ctx, tracked); err != nil { - return fmt.Errorf("publishing flushed batch: %w", err) - } + if len(flushedBatch) == 0 { + return nil } + b.admit(ticket) + defer b.release() + tracked, err := b.trackBatch(ctx, flushedBatch) + if err != nil { + return err + } + if err := b.sendTracked(ctx, tracked); err != nil { + return fmt.Errorf("publishing flushed batch: %w", err) + } return nil } // trackedBatch pairs a ready-to-send asyncMessage with the bookkeeping needed // to roll back its snapshot-gate slot if the send fails. type trackedBatch struct { - msg asyncMessage + msgs asyncMessage isSnapshot bool } -// trackBatchLocked registers the batch with the ordered checkpoint tracker and -// builds its ack function. It MUST be called with batcherMu held: Track order -// defines the checkpoint sequence, so it has to match flush order exactly. -func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.MessageBatch) (*trackedBatch, error) { +// trackBatch registers the batch with the ordered checkpoint tracker and +// builds its ack function. It MUST be called by the admitted ticket holder: +// Track order defines the checkpoint sequence, so it has to match flush +// (ticket) order exactly. Track may block on checkpoint_limit, which is why +// batcherMu must NOT be held here. +func (b *batchPublisher) trackBatch(ctx context.Context, batch service.MessageBatch) (*trackedBatch, error) { lastMsg := batch[len(batch)-1] // ensure we don't checkpoint snapshot batches @@ -284,7 +336,7 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes } return &trackedBatch{ isSnapshot: isSnapshotBatch, - msg: asyncMessage{ + msgs: asyncMessage{ msg: batch, // The ack error is deliberately ignored: nacks are replayed by // auto_replay_nacks (the default), and disabling that is a @@ -309,17 +361,25 @@ func (b *batchPublisher) trackBatchLocked(ctx context.Context, batch service.Mes }, nil } -// sendTracked hands a tracked batch to ReadBatch. Must be called WITHOUT -// batcherMu held (the send blocks until consumed). A failed send releases the -// batch's snapshot-gate slot. +// sendTracked hands a tracked batch to ReadBatch. Must be called by the +// admitted ticket holder, never under batcherMu: the send blocks until +// consumed. A failed send releases the batch's snapshot-gate slot and poisons +// the publisher. func (b *batchPublisher) sendTracked(ctx context.Context, tracked *trackedBatch) error { select { - case b.msgChan <- tracked.msg: + case b.msgChan <- tracked.msgs: return nil case <-ctx.Done(): if tracked.isSnapshot { b.snapshotAckWG.Done() } + // The batch's checkpoint slot is registered but its ackFn will never + // run, so the tracker is permanently pinned before this batch: mark + // the publisher poisoned so Connect rebuilds it with a fresh tracker. + // Resolving the slot here instead would be unsafe - another flusher + // may already have delivered a later-tracked batch, and its ack would + // then persist an SCN past these undelivered rows. + b.poisoned.Store(true) return ctx.Err() } } @@ -369,14 +429,20 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { if b.batcher == nil { return nil } - var tracked *trackedBatch b.batcherMu.Lock() remaining, err := b.batcher.Flush(ctx) + var ticket uint64 if err == nil && len(remaining) > 0 { - tracked, err = b.trackBatchLocked(ctx, remaining) + ticket = b.takeTicketLocked() } b.batcherMu.Unlock() - if err != nil || tracked == nil { + if err != nil || len(remaining) == 0 { + return err + } + b.admit(ticket) + defer b.release() + tracked, err := b.trackBatch(ctx, remaining) + if err != nil { return err } return b.sendTracked(ctx, tracked) diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index c518af48ab..3755538176 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -283,6 +283,92 @@ func TestTrackOrderUnderConcurrentFlush(t *testing.T) { // newTestBatchPublisher builds a publisher whose batcher flushes on every // published event (count=1), so tests drive the production // Publish->trackBatchLocked->sendTracked path directly. +// TestPublishBuffersWhileTrackBlocked verifies that a flusher blocked in +// checkpoint.Track (checkpoint_limit reached, nothing acked) holds only +// sendMu: other Publish calls must still be able to buffer rows instead of +// freezing on batcherMu behind the blocked Track. +func TestPublishBuffersWhileTrackBlocked(t *testing.T) { + ctx := t.Context() + logger := service.NewLoggerFromSlog(slog.Default()) + // Capacity for exactly one 2-row batch: the second flush blocks in Track. + cp := checkpoint.NewCapped[replication.SCN](2) + + batcher, err := (service.BatchPolicy{Count: 2}).NewBatcher(service.MockResources()) + require.NoError(t, err) + publisher := newBatchPublisher(batcher, cp, logger) + publisher.cacheSCN = func(context.Context, replication.SCN) error { return nil } + t.Cleanup(publisher.Close) + + // Batch 1 fills the tracker to capacity; consume it but do not ack. The + // flushing Publish blocks on the unbuffered channel send until the batch + // is consumed, so it runs on its own goroutine. + require.NoError(t, publisher.Publish(ctx, streamingEvent(100))) + firstPublished := make(chan error, 1) + go func() { firstPublished <- publisher.Publish(ctx, streamingEvent(101)) }() + first := <-publisher.msgs() + require.NoError(t, <-firstPublished) + + // Batch 2 flushes and blocks in Track (capacity exhausted). + blocked := make(chan error, 1) + go func() { + blocked <- func() error { + if err := publisher.Publish(ctx, streamingEvent(200)); err != nil { + return err + } + return publisher.Publish(ctx, streamingEvent(201)) + }() + }() + + // Give the flusher time to reach Track and park there. + time.Sleep(100 * time.Millisecond) + select { + case err := <-blocked: + t.Fatalf("expected the second batch's flusher to block in Track, but it returned: %v", err) + default: + } + + // The key assertion: a concurrent Publish that only buffers (no flush due) + // completes promptly even though a flusher is parked in Track. + buffered := make(chan error, 1) + go func() { buffered <- publisher.Publish(ctx, streamingEvent(300)) }() + select { + case err := <-buffered: + require.NoError(t, err) + case <-time.After(2 * time.Second): + t.Fatal("a buffering Publish froze behind a Track blocked on checkpoint_limit") + } + + // Ack batch 1 to release the blocked flusher and drain. + require.NoError(t, first.ackFn(ctx, nil)) + go func() { + for m := range publisher.msgs() { + _ = m.ackFn(ctx, nil) + } + }() + select { + case err := <-blocked: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("blocked flusher never released after the ack freed tracker capacity") + } +} + +// TestFailedSendPoisonsPublisher verifies that a tracked batch that cannot be +// handed to ReadBatch marks the publisher poisoned: its checkpoint slot can +// never resolve, so Connect must rebuild the publisher rather than reuse a +// permanently pinned tracker. +func TestFailedSendPoisonsPublisher(t *testing.T) { + publisher, _ := newTestBatchPublisher(t) + + sendCtx, cancel := context.WithCancel(t.Context()) + cancel() // nobody consumes msgs(): the send can only fail + + err := publisher.Publish(sendCtx, streamingEvent(100)) + require.ErrorIs(t, err, context.Canceled) + require.True(t, publisher.poisoned.Load(), + "a failed send orphans its tracker slot; the publisher must be marked for rebuild") +} + func newTestBatchPublisher(t *testing.T) (*batchPublisher, func() []replication.SCN) { t.Helper() return newTestBatchPublisherWithCount(t, 1) diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index bdc06ced1a..baa18e7e42 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -16,6 +16,7 @@ import ( "regexp" "strconv" "strings" + "sync" "sync/atomic" "time" @@ -265,6 +266,19 @@ type oracleDBCDCInput struct { snapshotOnlyDone atomic.Bool log *service.Logger cpCache service.Cache + + // batching and checkpointLimit are retained so Connect can rebuild a + // poisoned publisher (see batchPublisher.poisoned). + batching service.BatchPolicy + checkpointLimit int + + // persistMu serializes cacheSCN writes and lastPersistedSCN keeps them + // monotonic: ack functions run on concurrent pipeline goroutines, and + // after a publisher rebuild a previous session's late acks may still + // arrive - without ordering, a stale write could regress the durable + // resume position. + persistMu sync.Mutex + lastPersistedSCN replication.SCN } func newOracleDBCDCInput(conf *service.ParsedConfig, resources *service.Resources) (s service.BatchInput, err error) { @@ -403,13 +417,15 @@ func newOracleDBCDCInput(conf *service.ParsedConfig, resources *service.Resource Exclude: tableExcludes, }, }, - lmCfg: lmCfg, - res: resources, - log: logger, - metrics: resources.Metrics(), - stopSig: shutdown.NewSignaller(), - publisher: newBatchPublisher(batcher, cp, logger), - cpCache: cpCache, + lmCfg: lmCfg, + res: resources, + log: logger, + metrics: resources.Metrics(), + stopSig: shutdown.NewSignaller(), + publisher: newBatchPublisher(batcher, cp, logger), + cpCache: cpCache, + batching: policy, + checkpointLimit: checkpointLimit, } defer func() { @@ -438,6 +454,24 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { err error isCDB bool ) + + // A failed batch send leaves an unresolvable slot in the ordered tracker + // (see sendTracked), so a poisoned publisher can never checkpoint again. + // Rebuild it with a fresh tracker: the new session resumes from the last + // durable SCN, which is necessarily before the orphaned rows, and the old + // session's late acks resolve into the abandoned tracker (cacheSCN's + // monotonic guard turns any stale write into a no-op). + if o.publisher.poisoned.Load() { + o.log.Warn("Rebuilding publisher: a batch could not be handed to the pipeline, so the previous checkpoint tracker is pinned") + o.publisher.Close() + batcher, batcherErr := o.batching.NewBatcher(o.res) + if batcherErr != nil { + return fmt.Errorf("rebuilding batcher: %w", batcherErr) + } + o.publisher = newBatchPublisher(batcher, checkpoint.NewCapped[replication.SCN](int64(o.checkpointLimit)), o.log) + o.publisher.cacheSCN = o.cacheSCN + } + if o.db != nil { _ = o.db.Close() o.db = nil @@ -711,6 +745,16 @@ func (o *oracleDBCDCInput) cacheSCN(ctx context.Context, scn replication.SCN) er return errors.New("SCN for caching is empty") } + // Serialized and monotonic: concurrent acks (and, after a publisher + // rebuild, a previous session's late acks) must never land a stale SCN + // over a newer durable position. SCNs only grow, so skipping + // non-advancing writes is always safe. + o.persistMu.Lock() + defer o.persistMu.Unlock() + if o.lastPersistedSCN.IsValid() && scn <= o.lastPersistedSCN { + return nil + } + // Use internal Oracle-based cache if set (when no external cache configured), // otherwise use external cache resource var cErr error @@ -727,6 +771,7 @@ func (o *oracleDBCDCInput) cacheSCN(ctx context.Context, scn replication.SCN) er if cErr != nil { return fmt.Errorf("persisting checkpoint to cache: %w", cErr) } + o.lastPersistedSCN = scn return nil } From 09a5ebd248d70005a9d223f3f71a719f753258aa Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Mon, 17 Aug 2026 10:54:26 -0400 Subject: [PATCH 12/24] oracledb_cdc: barrier the snapshot handoff behind parked flushers Review finding on the ticket refactor: the snapshot gate Add moved out of the flush critical section (it now runs in trackBatch, after ticket admission and after checkpoint.Track, which can park on checkpoint_limit). The timed-flush loop is an independent flusher, so at the handoff it could already 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 SCN persist ahead of undelivered rows: the exact crash window this PR closes (plus a WaitGroup Add-during-Wait misuse hazard). flushCurrent now takes its ticket unconditionally, so its admission is a sequence barrier: every earlier flush has finished trackBatch+send before it returns, and the gate counts every published snapshot batch. New test encodes the parked-flusher interleaving and is proven red against the unbarriered code. --- internal/impl/oracledb/batcher.go | 16 +++-- internal/impl/oracledb/batcher_test.go | 83 ++++++++++++++++++++++++-- 2 files changed, 87 insertions(+), 12 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index d8e4ddf0ca..aa9d461932 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -431,16 +431,20 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { } b.batcherMu.Lock() remaining, err := b.batcher.Flush(ctx) - var ticket uint64 - if err == nil && len(remaining) > 0 { - ticket = b.takeTicketLocked() - } + // The ticket is taken unconditionally - even when the batcher is empty - + // so that admission below doubles as a sequence barrier: another flusher + // (the timed loop) may already hold the final snapshot rows while parked + // in checkpoint.Track, before it has counted them on the snapshot ack + // gate. Being admitted proves every earlier flush has finished + // trackBatch+send, so once flushCurrent returns the gate counts every + // published snapshot batch and waitSnapshotAcks cannot release early. + ticket := b.takeTicketLocked() b.batcherMu.Unlock() + b.admit(ticket) + defer b.release() if err != nil || len(remaining) == 0 { return err } - b.admit(ticket) - defer b.release() tracked, err := b.trackBatch(ctx, remaining) if err != nil { return err diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 3755538176..0614a088e8 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -280,13 +280,10 @@ func TestTrackOrderUnderConcurrentFlush(t *testing.T) { } } -// newTestBatchPublisher builds a publisher whose batcher flushes on every -// published event (count=1), so tests drive the production -// Publish->trackBatchLocked->sendTracked path directly. // TestPublishBuffersWhileTrackBlocked verifies that a flusher blocked in -// checkpoint.Track (checkpoint_limit reached, nothing acked) holds only -// sendMu: other Publish calls must still be able to buffer rows instead of -// freezing on batcherMu behind the blocked Track. +// checkpoint.Track (checkpoint_limit reached, nothing acked) waits only in +// the ticket queue: other Publish calls must still be able to buffer rows +// instead of freezing on batcherMu behind the blocked Track. func TestPublishBuffersWhileTrackBlocked(t *testing.T) { ctx := t.Context() logger := service.NewLoggerFromSlog(slog.Default()) @@ -353,6 +350,77 @@ func TestPublishBuffersWhileTrackBlocked(t *testing.T) { } } +// TestFlushCurrentBarriersParkedFlusher encodes the snapshot-handoff crash +// window: the timed-flush loop can hold the final snapshot rows while parked +// in checkpoint.Track (before counting them on the snapshot ack gate). The +// handoff's flushCurrent must not return - and waitSnapshotAcks must not +// release - until that parked flusher has registered and delivered its batch, +// otherwise the post-snapshot SCN persists ahead of undelivered rows. +func TestFlushCurrentBarriersParkedFlusher(t *testing.T) { + ctx := t.Context() + logger := service.NewLoggerFromSlog(slog.Default()) + // Capacity for exactly one 2-row batch: the second flush parks in Track. + cp := checkpoint.NewCapped[replication.SCN](2) + + batcher, err := (service.BatchPolicy{Count: 2}).NewBatcher(service.MockResources()) + require.NoError(t, err) + publisher := newBatchPublisher(batcher, cp, logger) + publisher.cacheSCN = func(context.Context, replication.SCN) error { return nil } + t.Cleanup(publisher.Close) + + // Snapshot batch 1 fills the tracker; consume it but do not ack (WG=1). + require.NoError(t, publisher.Publish(ctx, snapshotEvent(100))) + firstPublished := make(chan error, 1) + go func() { firstPublished <- publisher.Publish(ctx, snapshotEvent(100)) }() + first := <-publisher.msgs() + require.NoError(t, <-firstPublished) + + // Snapshot batch 2 flushes, takes its ticket, and parks in Track - the + // exact state the timed-flush loop can be in at the handoff. + parked := make(chan error, 1) + go func() { + parked <- func() error { + if err := publisher.Publish(ctx, snapshotEvent(100)); err != nil { + return err + } + return publisher.Publish(ctx, snapshotEvent(100)) + }() + }() + time.Sleep(100 * time.Millisecond) + + // The handoff: flushCurrent sees an empty batcher but must still barrier + // behind the parked flusher's ticket. + flushed := make(chan error, 1) + go func() { flushed <- publisher.flushCurrent(ctx) }() + select { + case err := <-flushed: + t.Fatalf("flushCurrent returned (%v) while a flusher holding snapshot rows was still parked in Track: the ack gate does not yet count those rows", err) + case <-time.After(300 * time.Millisecond): + } + + // Ack batch 1: the parked flusher tracks, counts, and delivers batch 2. + require.NoError(t, first.ackFn(ctx, nil)) + second := <-publisher.msgs() + require.NoError(t, <-parked) + require.NoError(t, <-flushed) + + // The gate must now hold for batch 2: it is published but un-acked. + gate := make(chan error, 1) + go func() { gate <- publisher.waitSnapshotAcks(ctx) }() + select { + case err := <-gate: + t.Fatalf("waitSnapshotAcks returned (%v) with a published snapshot batch still un-acked", err) + case <-time.After(300 * time.Millisecond): + } + require.NoError(t, second.ackFn(ctx, nil)) + select { + case err := <-gate: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("gate never released after the final ack") + } +} + // TestFailedSendPoisonsPublisher verifies that a tracked batch that cannot be // handed to ReadBatch marks the publisher poisoned: its checkpoint slot can // never resolve, so Connect must rebuild the publisher rather than reuse a @@ -369,6 +437,9 @@ func TestFailedSendPoisonsPublisher(t *testing.T) { "a failed send orphans its tracker slot; the publisher must be marked for rebuild") } +// newTestBatchPublisher builds a publisher whose batcher flushes on every +// published event (count=1), so tests drive the production +// Publish->trackBatch->sendTracked path directly. func newTestBatchPublisher(t *testing.T) (*batchPublisher, func() []replication.SCN) { t.Helper() return newTestBatchPublisherWithCount(t, 1) From efa6b5a4567a8f63e10d46a8fe5b55b4f85c7216 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Tue, 18 Aug 2026 15:19:04 -0400 Subject: [PATCH 13/24] oracledb_cdc: log handoff flush cancellation at info Same finding as mssqlserver (991bcc917), fixed proactively: a graceful stop landing in the snapshot handoff window exits flushCurrent via softCtx cancellation but logged at ERROR. Cancellation without a hard stop now logs at Info like the adjacent handoff branches; genuine flush failures keep the error level. --- internal/impl/oracledb/input_oracledb_cdc.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index baa18e7e42..ce2a44219b 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -659,7 +659,15 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { // or soft-stop (no timeout, by design; see postgres_cdc's // equivalent barrier). if err = o.publisher.flushCurrent(softCtx); err != nil { - o.log.Errorf("Failed to flush remaining snapshot batches. Snapshot will re-run on restart (may cause duplicate data): %s", err) + // A graceful stop lands here whenever shutdown hits the + // handoff window (nothing drains msgChan any more, so the + // blocked send exits via softCtx): normal operation, Info. + // Genuine flush failures keep the error level. + if errors.Is(err, context.Canceled) && !o.stopSig.IsHardStopSignalled() { + o.log.Infof("Interrupted while flushing remaining snapshot batches. Snapshot will re-run on restart (may cause duplicate data): %s", err) + } else { + o.log.Errorf("Failed to flush remaining snapshot batches. Snapshot will re-run on restart (may cause duplicate data): %s", err) + } o.stopSig.TriggerHasStopped() return } From 9382660af5738f1f64ee1db8856b17cf19d6a33e Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Tue, 18 Aug 2026 16:20:03 -0400 Subject: [PATCH 14/24] oracledb_cdc: cancellable ticket admission, batcher teardown under lock Ports the mssqlserver fixes from the same review round, plus the oracle-specific finding that motivated them: the timed-flush loop parks its send under hardStopCtx by design, so a soft stop during the snapshot handoff left flushCurrent(softCtx) wedged in admit behind it - Close burned both shutdown timeouts and logged a spurious error before the publisher's hard stop finally unwound the queue. admit(ctx) now escapes via the caller's context with an abandonment protocol (release skips abandoned tickets, keeping the sequence intact; abandoned batches were never tracked, so their rows re-read from the last durable checkpoint), and the batcher teardown in Close runs under batcherMu with a closed flag guarding the flush paths. New test encodes the exact parked-holder/queued-flusher scenario. --- internal/impl/oracledb/batcher.go | 95 ++++++++++++++++++++++---- internal/impl/oracledb/batcher_test.go | 79 +++++++++++++++++++++ 2 files changed, 159 insertions(+), 15 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index aa9d461932..7247d59249 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -37,11 +37,18 @@ type batchPublisher struct { // checkpoint_limit under downstream backpressure; only the admitted // ticket holder (and flushers queued behind it) waits, while Publish // calls keep buffering and the timed-flush ticker keeps reading - // UntilNext. + // UntilNext. Admission is cancellable: an abandoned ticket is skipped + // when its turn comes, so a graceful stop unwinds queued flushers + // instead of wedging them behind a send parked under hardStopCtx. ticketMu sync.Mutex - ticketCond *sync.Cond - nextTicket uint64 // next ticket to hand out; guarded by batcherMu - admitted uint64 // next ticket allowed to Track+send; guarded by ticketMu + nextTicket uint64 // next ticket to hand out; guarded by batcherMu + admitted uint64 // next ticket allowed to Track+send; guarded by ticketMu + waiters map[uint64]chan struct{} // parked admit calls; guarded by ticketMu + abandoned map[uint64]struct{} // cancelled tickets to skip; guarded by ticketMu + // closed marks the batcher as torn down (guarded by batcherMu): Close's + // batcher.Close races in-flight Publish calls otherwise, and the batcher + // is not goroutine-safe. + closed bool // poisoned is set when a tracked batch could not be handed to ReadBatch: // its checkpoint slot can never resolve, so this publisher can never // checkpoint past it. Connect rebuilds a poisoned publisher. @@ -69,7 +76,8 @@ func newBatchPublisher(batcher *service.Batcher, checkpoint *checkpoint.Capped[r log: logger, shutSig: shutdown.NewSignaller(), } - b.ticketCond = sync.NewCond(&b.ticketMu) + b.waiters = make(map[uint64]chan struct{}) + b.abandoned = make(map[uint64]struct{}) go b.loop() return b } @@ -83,21 +91,58 @@ func (b *batchPublisher) takeTicketLocked() uint64 { return t } -// admit blocks until it is ticket's turn to Track+send. Pair with release. -func (b *batchPublisher) admit(ticket uint64) { +// admit blocks until it is ticket's turn to Track+send, or ctx is cancelled. +// On success, pair with release. On cancellation the ticket is marked +// abandoned - release skips it when its turn comes - and the caller must NOT +// release it; the flushed batch was never tracked, so its rows are re-read +// from the last durable checkpoint by the next session. +func (b *batchPublisher) admit(ctx context.Context, ticket uint64) error { b.ticketMu.Lock() - for b.admitted != ticket { - b.ticketCond.Wait() + if b.admitted == ticket { + b.ticketMu.Unlock() + return nil } + ch := make(chan struct{}) + b.waiters[ticket] = ch b.ticketMu.Unlock() + + select { + case <-ch: + return nil + case <-ctx.Done(): + b.ticketMu.Lock() + select { + case <-ch: + // Admitted between cancellation and the lock: proceed normally, + // the caller owns the release. + b.ticketMu.Unlock() + return nil + default: + } + delete(b.waiters, ticket) + b.abandoned[ticket] = struct{}{} + b.ticketMu.Unlock() + return ctx.Err() + } } -// release passes the sequence to the next ticket. Every taken ticket must be -// released exactly once, error paths included, or the sequence wedges. +// release passes the sequence to the next live ticket, skipping abandoned +// ones. Every ADMITTED ticket must be released exactly once, error paths +// included, or the sequence wedges. func (b *batchPublisher) release() { b.ticketMu.Lock() b.admitted++ - b.ticketCond.Broadcast() + for { + if _, ok := b.abandoned[b.admitted]; !ok { + break + } + delete(b.abandoned, b.admitted) + b.admitted++ + } + if ch, ok := b.waiters[b.admitted]; ok { + close(ch) + delete(b.waiters, b.admitted) + } b.ticketMu.Unlock() } @@ -169,7 +214,9 @@ func (p *batchPublisher) loop() { return nil } - p.admit(ticket) + if err := p.admit(hardStopCtx, ticket); err != nil { + return err + } defer p.release() tracked, err := p.trackBatch(hardStopCtx, sendBatch) if err != nil { @@ -263,6 +310,10 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven ticket uint64 ) b.batcherMu.Lock() + if b.closed { + b.batcherMu.Unlock() + return context.Canceled + } if b.batcher.Add(msg) { if flushedBatch, err = b.batcher.Flush(ctx); err == nil && len(flushedBatch) > 0 { ticket = b.takeTicketLocked() @@ -276,7 +327,9 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven return nil } - b.admit(ticket) + if err := b.admit(ctx, ticket); err != nil { + return err + } defer b.release() tracked, err := b.trackBatch(ctx, flushedBatch) if err != nil { @@ -430,6 +483,10 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { return nil } b.batcherMu.Lock() + if b.closed { + b.batcherMu.Unlock() + return context.Canceled + } remaining, err := b.batcher.Flush(ctx) // The ticket is taken unconditionally - even when the batcher is empty - // so that admission below doubles as a sequence barrier: another flusher @@ -440,7 +497,9 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { // published snapshot batch and waitSnapshotAcks cannot release early. ticket := b.takeTicketLocked() b.batcherMu.Unlock() - b.admit(ticket) + if admitErr := b.admit(ctx, ticket); admitErr != nil { + return admitErr + } defer b.release() if err != nil || len(remaining) == 0 { return err @@ -471,6 +530,12 @@ func (b *batchPublisher) Close() { b.shutSig.TriggerHardStop() <-b.shutSig.HasStoppedChan() if b.batcher != nil { + // The batcher is not goroutine-safe and session goroutines may still + // be inside Publish: close it under batcherMu and mark it closed so + // later flush paths refuse instead of touching a closed batcher. + b.batcherMu.Lock() + b.closed = true _ = b.batcher.Close(context.Background()) + b.batcherMu.Unlock() } } diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 0614a088e8..958683c2f4 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -421,6 +421,85 @@ func TestFlushCurrentBarriersParkedFlusher(t *testing.T) { } } +// TestAdmitEscapesOnContextCancel verifies admission is cancellable: a +// flusher queued in admit behind a ticket whose holder is parked under a +// DIFFERENT, still-live context (the timed loop uses hardStopCtx by design) +// must unwind via its own context - the abandoned ticket is skipped when its +// turn comes, and the sequence stays intact for later flushers. +func TestAdmitEscapesOnContextCancel(t *testing.T) { + ctx := t.Context() + logger := service.NewLoggerFromSlog(slog.Default()) + cp := checkpoint.NewCapped[replication.SCN](100) + + batcher, err := (service.BatchPolicy{Count: 2}).NewBatcher(service.MockResources()) + require.NoError(t, err) + publisher := newBatchPublisher(batcher, cp, logger) + publisher.cacheSCN = func(context.Context, replication.SCN) error { return nil } + t.Cleanup(publisher.Close) + + // Ticket 0's holder parks in sendTracked under a live context (nobody + // consumes msgs()). + holderDone := make(chan error, 1) + go func() { + holderDone <- func() error { + if err := publisher.Publish(ctx, streamingEvent(100)); err != nil { + return err + } + return publisher.Publish(ctx, streamingEvent(101)) + }() + }() + require.Eventually(t, func() bool { + publisher.batcherMu.Lock() + defer publisher.batcherMu.Unlock() + return publisher.nextTicket == 1 + }, 5*time.Second, time.Millisecond) + + // The handoff's flushCurrent queues behind it with a cancellable context + // - the exact soft-stop-during-handoff scenario. + flusherCtx, cancelFlusher := context.WithCancel(ctx) + queued := make(chan error, 1) + go func() { queued <- publisher.flushCurrent(flusherCtx) }() + time.Sleep(100 * time.Millisecond) + select { + case err := <-queued: + t.Fatalf("expected flushCurrent to queue in admit, but it returned: %v", err) + default: + } + + // Cancelling ONLY the flusher's context must unwind it promptly - the + // parked holder's hardStopCtx is untouched. + cancelFlusher() + select { + case err := <-queued: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(2 * time.Second): + t.Fatal("admit did not escape on context cancellation: a soft stop during the handoff would burn the shutdown timeouts") + } + + // The sequence stays intact: drain the parked holder and prove a later + // flusher is still admitted (the abandoned ticket is skipped). + first := <-publisher.msgs() + require.NoError(t, first.ackFn(ctx, nil)) + require.NoError(t, <-holderDone) + + done := make(chan error, 1) + go func() { + done <- func() error { + if err := publisher.Publish(ctx, streamingEvent(200)); err != nil { + return err + } + return publisher.Publish(ctx, streamingEvent(201)) + }() + }() + select { + case m := <-publisher.msgs(): + require.NoError(t, m.ackFn(ctx, nil)) + case <-time.After(5 * time.Second): + t.Fatal("a later flusher was never admitted: the abandoned ticket wedged the sequence") + } + require.NoError(t, <-done) +} + // TestFailedSendPoisonsPublisher verifies that a tracked batch that cannot be // handed to ReadBatch marks the publisher poisoned: its checkpoint slot can // never resolve, so Connect must rebuild the publisher rather than reuse a From 232a28b532b9b997dd725ccb347a05271429c985 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Wed, 19 Aug 2026 10:30:04 -0400 Subject: [PATCH 15/24] oracledb_cdc: seal the flush queue when an abandoned ticket drops rows Review finding on the abandonment protocol: an abandoned ticket's batch had already left the batcher but was never tracked, so nothing pinned the ordered tracker for those rows - a later ticket (the timed loop survives a soft stop under hardStopCtx by design) could still track, deliver, and ack rows after the gap, persisting an SCN past the dropped ones: silent loss on restart, the exact hazard the failed-send path poisons against. The admit doc claimed re-read safety the code did not establish. Admission order is what makes the fix sound: at abandon time nothing after the gap has been tracked yet. An abandon whose ticket owned a non-empty batch now seals the queue - every later admission is refused with errQueueSealed, so nothing can ever be tracked past the gap - and poisons the publisher; Connect rebuilds and the session resumes from the last durable SCN, genuinely re-reading the dropped rows. Empty-ticket abandons (the flushCurrent barrier, mssql's window markers) stay benign. Same change on mssqlserver. New test encodes the exact interleaving: rows-owning abandon behind a parked holder must poison and refuse later flushers. --- internal/impl/oracledb/batcher.go | 66 ++++++++++++++++++++++++-- internal/impl/oracledb/batcher_test.go | 65 +++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 5 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 7247d59249..9a93c45e3a 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -11,6 +11,7 @@ package oracledb import ( "context" "encoding/json" + "errors" "fmt" "strconv" "sync" @@ -45,6 +46,12 @@ type batchPublisher struct { admitted uint64 // next ticket allowed to Track+send; guarded by ticketMu waiters map[uint64]chan struct{} // parked admit calls; guarded by ticketMu abandoned map[uint64]struct{} // cancelled tickets to skip; guarded by ticketMu + // sealed refuses all further admissions (guarded by ticketMu): set when + // an abandoned ticket owned a flushed batch. Admission is strictly + // ordered, so at that moment nothing after the dropped rows has been + // tracked - sealing guarantees nothing ever is, so no ack can persist an + // SCN past them before Connect rebuilds the poisoned publisher. + sealed bool // closed marks the batcher as torn down (guarded by batcherMu): Close's // batcher.Close races in-flight Publish calls otherwise, and the batcher // is not goroutine-safe. @@ -91,13 +98,25 @@ func (b *batchPublisher) takeTicketLocked() uint64 { return t } +// errQueueSealed refuses admission after an abandoned ticket dropped a +// flushed batch: nothing may be tracked past that gap until Connect rebuilds +// the poisoned publisher. +var errQueueSealed = errors.New("publisher flush queue sealed after an abandoned batch; reconnecting rebuilds the publisher") + // admit blocks until it is ticket's turn to Track+send, or ctx is cancelled. // On success, pair with release. On cancellation the ticket is marked // abandoned - release skips it when its turn comes - and the caller must NOT -// release it; the flushed batch was never tracked, so its rows are re-read -// from the last durable checkpoint by the next session. +// release it. A caller whose ticket owned a non-empty flushed batch MUST call +// sealQueue after an abandon: the batch was never tracked, so only sealing +// (no later ticket can ever track) plus the poison rebuild guarantees its +// rows are re-read from the last durable checkpoint rather than silently +// skipped by a later batch's ack. func (b *batchPublisher) admit(ctx context.Context, ticket uint64) error { b.ticketMu.Lock() + if b.sealed { + b.ticketMu.Unlock() + return errQueueSealed + } if b.admitted == ticket { b.ticketMu.Unlock() return nil @@ -106,16 +125,29 @@ func (b *batchPublisher) admit(ctx context.Context, ticket uint64) error { b.waiters[ticket] = ch b.ticketMu.Unlock() + wake := func() error { + b.ticketMu.Lock() + defer b.ticketMu.Unlock() + if b.sealed { + return errQueueSealed + } + return nil + } + select { case <-ch: - return nil + return wake() case <-ctx.Done(): b.ticketMu.Lock() select { case <-ch: - // Admitted between cancellation and the lock: proceed normally, - // the caller owns the release. + // Woken between cancellation and the lock: either admitted + // normally (caller owns the release) or the queue was sealed. + sealed := b.sealed b.ticketMu.Unlock() + if sealed { + return errQueueSealed + } return nil default: } @@ -126,6 +158,21 @@ func (b *batchPublisher) admit(ctx context.Context, ticket uint64) error { } } +// sealQueue permanently refuses further admissions and poisons the publisher: +// called when an abandoned ticket dropped a flushed-but-untracked batch, so +// no later batch can be tracked (and therefore no ack can persist a position) +// past the dropped rows before Connect rebuilds. +func (b *batchPublisher) sealQueue() { + b.ticketMu.Lock() + b.sealed = true + for t, ch := range b.waiters { + close(ch) + delete(b.waiters, t) + } + b.ticketMu.Unlock() + b.poisoned.Store(true) +} + // release passes the sequence to the next live ticket, skipping abandoned // ones. Every ADMITTED ticket must be released exactly once, error paths // included, or the sequence wedges. @@ -215,6 +262,9 @@ func (p *batchPublisher) loop() { } if err := p.admit(hardStopCtx, ticket); err != nil { + if !errors.Is(err, errQueueSealed) && len(sendBatch) > 0 { + p.sealQueue() + } return err } defer p.release() @@ -328,6 +378,9 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven } if err := b.admit(ctx, ticket); err != nil { + if !errors.Is(err, errQueueSealed) && len(flushedBatch) > 0 { + b.sealQueue() + } return err } defer b.release() @@ -498,6 +551,9 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { ticket := b.takeTicketLocked() b.batcherMu.Unlock() if admitErr := b.admit(ctx, ticket); admitErr != nil { + if !errors.Is(admitErr, errQueueSealed) && len(remaining) > 0 { + b.sealQueue() + } return admitErr } defer b.release() diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index 958683c2f4..b12958b15a 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -500,6 +500,71 @@ func TestAdmitEscapesOnContextCancel(t *testing.T) { require.NoError(t, <-done) } +// TestAbandonedBatchSealsQueue encodes the abandonment loss window: a flusher +// whose rows are already out of the batcher abandons its ticket on +// cancellation. Nothing pins the tracker for those rows, so if any LATER +// batch could still be tracked and acked, its resolve would persist an SCN +// past the dropped rows - silent loss on restart. The abandon must therefore +// seal the queue (no later admission can ever track) and poison the +// publisher so Connect rebuilds and re-reads from the last durable SCN. +func TestAbandonedBatchSealsQueue(t *testing.T) { + ctx := t.Context() + logger := service.NewLoggerFromSlog(slog.Default()) + cp := checkpoint.NewCapped[replication.SCN](100) + + batcher, err := (service.BatchPolicy{Count: 2}).NewBatcher(service.MockResources()) + require.NoError(t, err) + publisher := newBatchPublisher(batcher, cp, logger) + publisher.cacheSCN = func(context.Context, replication.SCN) error { return nil } + t.Cleanup(publisher.Close) + + // Ticket 0's holder parks in sendTracked under a live context. + holderDone := make(chan error, 1) + go func() { + holderDone <- func() error { + if err := publisher.Publish(ctx, streamingEvent(100)); err != nil { + return err + } + return publisher.Publish(ctx, streamingEvent(101)) + }() + }() + require.Eventually(t, func() bool { + publisher.batcherMu.Lock() + defer publisher.batcherMu.Unlock() + return publisher.nextTicket == 1 + }, 5*time.Second, time.Millisecond) + + // A flusher with ROWS (ticket 1) queues behind it and is cancelled: its + // batch left the batcher but was never tracked. + abandonCtx, cancelAbandon := context.WithCancel(ctx) + abandoned := make(chan error, 1) + go func() { + abandoned <- func() error { + if err := publisher.Publish(abandonCtx, streamingEvent(200)); err != nil { + return err + } + return publisher.Publish(abandonCtx, streamingEvent(201)) + }() + }() + time.Sleep(100 * time.Millisecond) + cancelAbandon() + require.ErrorIs(t, <-abandoned, context.Canceled) + + // The abandon dropped SCN 200-201 rows: the queue must be sealed and the + // publisher poisoned so nothing can ever be tracked (and persisted) past + // them from this generation. + require.True(t, publisher.poisoned.Load(), + "abandoning a flushed-but-untracked batch must poison the publisher") + laterErr := func() error { + if err := publisher.Publish(ctx, streamingEvent(300)); err != nil { + return err + } + return publisher.Publish(ctx, streamingEvent(301)) + }() + require.ErrorIs(t, laterErr, errQueueSealed, + "a later flusher must be refused: tracking past the dropped rows would let its ack persist an SCN that skips them") +} + // TestFailedSendPoisonsPublisher verifies that a tracked batch that cannot be // handed to ReadBatch marks the publisher poisoned: its checkpoint slot can // never resolve, so Connect must rebuild the publisher rather than reuse a From 6ba286a1dd5d356c27018a087d8b3c44f81369f6 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Wed, 19 Aug 2026 10:53:18 -0400 Subject: [PATCH 16/24] oracledb_cdc: log drops and poisoning, make the publisher pointer atomic Two review findings from the mssqlserver sibling, applied here in the same round: - 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). The ack function now warns with the batch size, snapshot flag, and checkpoint SCN 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. The field is now an atomic.Pointer: readers can never observe a stale pointer, and the session captures its own generation as a local. --- internal/impl/oracledb/batcher.go | 15 ++++--- internal/impl/oracledb/input_oracledb_cdc.go | 44 +++++++++++--------- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 9a93c45e3a..8876cef30b 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -444,14 +444,18 @@ func (b *batchPublisher) trackBatch(ctx context.Context, batch service.MessageBa isSnapshot: isSnapshotBatch, msgs: asyncMessage{ msg: batch, - // The ack error is deliberately ignored: nacks are replayed by - // auto_replay_nacks (the default), and disabling that is a - // documented opt-in to DROP rejected messages, so the checkpoint - // must advance past them rather than pin the tracker. - ackFn: func(ctx context.Context, _ error) error { + // Nacks resolve like acks: they are replayed by auto_replay_nacks + // (the default), and disabling that is a documented opt-in to DROP + // rejected messages, so the checkpoint must advance past them + // rather than pin the tracker. The drop is logged - it is the one + // place rows become unrecoverable by design. + ackFn: func(ctx context.Context, ackErr error) error { if isSnapshotBatch { defer b.snapshotAckWG.Done() } + if ackErr != nil { + b.log.Warnf("Dropping batch of %d messages rejected downstream (snapshot=%v, checkpoint SCN %s): auto_replay_nacks is disabled, so the checkpoint advances past the dropped rows: %v", len(batch), isSnapshotBatch, checkpointSCN, ackErr) + } scn := resolveFn() if scn == nil || !scn.IsValid() { return nil @@ -485,6 +489,7 @@ func (b *batchPublisher) sendTracked(ctx context.Context, tracked *trackedBatch) // Resolving the slot here instead would be unsafe - another flusher // may already have delivered a later-tracked batch, and its ack would // then persist an SCN past these undelivered rows. + b.log.Warnf("Batch of %d messages could not be handed to the pipeline; the publisher is marked for rebuild and its rows re-read from the last durable SCN on reconnect", len(tracked.msgs.msg)) b.poisoned.Store(true) return ctx.Err() } diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index ce2a44219b..72828ee982 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -258,8 +258,11 @@ type oracleDBCDCInput struct { lmCfg *logminer.Config db *sql.DB - res *service.Resources - publisher *batchPublisher + res *service.Resources + // publisher is rebuilt by Connect when poisoned, and read by ReadBatch + // and Close on other goroutines: atomic so those reads can never observe + // a torn or stale pointer and Close always stops the CURRENT publisher. + publisher atomic.Pointer[batchPublisher] metrics *service.Metrics stopSig *shutdown.Signaller @@ -422,20 +425,21 @@ func newOracleDBCDCInput(conf *service.ParsedConfig, resources *service.Resource log: logger, metrics: resources.Metrics(), stopSig: shutdown.NewSignaller(), - publisher: newBatchPublisher(batcher, cp, logger), cpCache: cpCache, batching: policy, checkpointLimit: checkpointLimit, } + pub := newBatchPublisher(batcher, cp, logger) + pub.cacheSCN = o.cacheSCN + o.publisher.Store(pub) + defer func() { if err != nil { - o.publisher.Close() + pub.Close() } }() - o.publisher.cacheSCN = o.cacheSCN - // Has stopped is how we notify that we're not connected. This will get reset at connection time. o.stopSig.TriggerHasStopped() @@ -461,15 +465,17 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { // durable SCN, which is necessarily before the orphaned rows, and the old // session's late acks resolve into the abandoned tracker (cacheSCN's // monotonic guard turns any stale write into a no-op). - if o.publisher.poisoned.Load() { + publisher := o.publisher.Load() + if publisher.poisoned.Load() { o.log.Warn("Rebuilding publisher: a batch could not be handed to the pipeline, so the previous checkpoint tracker is pinned") - o.publisher.Close() + publisher.Close() batcher, batcherErr := o.batching.NewBatcher(o.res) if batcherErr != nil { return fmt.Errorf("rebuilding batcher: %w", batcherErr) } - o.publisher = newBatchPublisher(batcher, checkpoint.NewCapped[replication.SCN](int64(o.checkpointLimit)), o.log) - o.publisher.cacheSCN = o.cacheSCN + publisher = newBatchPublisher(batcher, checkpoint.NewCapped[replication.SCN](int64(o.checkpointLimit)), o.log) + publisher.cacheSCN = o.cacheSCN + o.publisher.Store(publisher) } if o.db != nil { @@ -566,7 +572,7 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { } } - o.publisher.schemas = schemas + publisher.schemas = schemas if cachedSCN, err = o.getCachedSCN(ctx); err != nil { if errors.Is(err, service.ErrKeyNotFound) { @@ -599,7 +605,7 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { // no cached SCN means we're not recovering from a restart if !o.cfg.SnapshotMode.IsSnapshotNone() && cachedSCN == replication.InvalidSCN { - if snapshotter, err = replication.NewSnapshot(ctx, o.cfg.ConnectionString, userTables, o.cfg.SnapshotFilters, o.publisher, o.lmCfg.LOBEnabled, pdbNameForCache, o.log, o.metrics); err != nil { + if snapshotter, err = replication.NewSnapshot(ctx, o.cfg.ConnectionString, userTables, o.cfg.SnapshotFilters, publisher, o.lmCfg.LOBEnabled, pdbNameForCache, o.log, o.metrics); err != nil { return fmt.Errorf("creating database snapshotter: %w", err) } defer func() { @@ -616,7 +622,7 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { if o.lmCfg.TransactionCacheConfig.CacheName != "" { txnCache = logminer.NewConnectCacheResource(o.res, o.lmCfg.TransactionCacheConfig, o.metrics, o.log) } - streaming = logminer.NewMiner(o.db, userTables, o.publisher, o.lmCfg, txnCache, o.metrics, o.log) + streaming = logminer.NewMiner(o.db, userTables, publisher, o.lmCfg, txnCache, o.metrics, o.log) } else { return errors.New("logminer configuration required for streaming") } @@ -658,7 +664,7 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { // skip un-acked snapshot rows on restart. Blocks until acks drain // or soft-stop (no timeout, by design; see postgres_cdc's // equivalent barrier). - if err = o.publisher.flushCurrent(softCtx); err != nil { + if err = publisher.flushCurrent(softCtx); err != nil { // A graceful stop lands here whenever shutdown hits the // handoff window (nothing drains msgChan any more, so the // blocked send exits via softCtx): normal operation, Info. @@ -671,7 +677,7 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { o.stopSig.TriggerHasStopped() return } - if err = o.publisher.waitSnapshotAcks(softCtx); err != nil { + if err = publisher.waitSnapshotAcks(softCtx); err != nil { o.log.Infof("Interrupted while waiting for snapshot acknowledgements. Snapshot will re-run on restart (may cause duplicate data): %s", err) o.stopSig.TriggerHasStopped() return @@ -687,7 +693,7 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { } if o.cfg.SnapshotMode.IsSnapshotOnly() { - if err = o.publisher.FlushRemaining(softCtx); err != nil { + if err = publisher.FlushRemaining(softCtx); err != nil { o.log.Errorf("Failed to flush remaining snapshot events: %s", err) } o.log.Infof("Snapshot-only mode complete, stopping at SCN %s", startSCN) @@ -785,7 +791,7 @@ func (o *oracleDBCDCInput) cacheSCN(ctx context.Context, scn replication.SCN) er func (o *oracleDBCDCInput) ReadBatch(ctx context.Context) (service.MessageBatch, service.AckFunc, error) { select { - case m := <-o.publisher.msgs(): + case m := <-o.publisher.Load().msgs(): return m.msg, m.ackFn, nil case <-o.stopSig.HasStoppedChan(): if o.snapshotOnlyDone.Load() { @@ -837,8 +843,8 @@ func (o *oracleDBCDCInput) Close(ctx context.Context) error { case <-o.stopSig.HasStoppedChan(): } - if o.publisher != nil { - o.publisher.Close() + if pub := o.publisher.Load(); pub != nil { + pub.Close() } // Close both resources and combine errors to avoid resource leaks From acacdb45e9d824c005967609840cc1c5fcd44aad Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Wed, 19 Aug 2026 11:41:07 -0400 Subject: [PATCH 17/24] oracledb_cdc: seal the queue when Track fails after admission Review finding, the abandon fix's sibling: a trackBatch failure after admission (parked in checkpoint.Track when the context cancels) strands rows that already left the batcher with nothing registered in the tracker, while the deferred release advances the queue - a later ticket could track, deliver, and ack rows past the gap, persisting an SCN that skips the dropped ones on restart. All three flush paths now seal (and thereby poison) on a track failure with rows in hand, and flushCurrent also seals when Flush itself errors alongside a non-empty batch. New test parks an admitted flusher in Track at capacity, cancels it, and asserts the poison plus refusal of later flushers. --- internal/impl/oracledb/batcher.go | 21 ++++++++++- internal/impl/oracledb/batcher_test.go | 52 ++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 8876cef30b..64a8a01c0a 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -270,6 +270,10 @@ func (p *batchPublisher) loop() { defer p.release() tracked, err := p.trackBatch(hardStopCtx, sendBatch) if err != nil { + // The rows left the batcher but were never tracked, and + // the deferred release lets later tickets proceed: seal so + // nothing can be tracked (and persisted) past the gap. + p.sealQueue() return err } return p.sendTracked(hardStopCtx, tracked) @@ -386,6 +390,10 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven defer b.release() tracked, err := b.trackBatch(ctx, flushedBatch) if err != nil { + // The rows left the batcher but were never tracked, and the deferred + // release lets later tickets proceed: seal so nothing can be tracked + // (and persisted) past the gap. + b.sealQueue() return err } if err := b.sendTracked(ctx, tracked); err != nil { @@ -562,11 +570,22 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { return admitErr } defer b.release() - if err != nil || len(remaining) == 0 { + if err != nil { + if len(remaining) > 0 { + // Rows came out of the batcher alongside the error: they were + // never tracked, so seal before the deferred release lets later + // tickets persist past them. + b.sealQueue() + } return err } + if len(remaining) == 0 { + return nil + } tracked, err := b.trackBatch(ctx, remaining) if err != nil { + // Same gap as above: flushed but untracked. + b.sealQueue() return err } return b.sendTracked(ctx, tracked) diff --git a/internal/impl/oracledb/batcher_test.go b/internal/impl/oracledb/batcher_test.go index b12958b15a..8b0be107a1 100644 --- a/internal/impl/oracledb/batcher_test.go +++ b/internal/impl/oracledb/batcher_test.go @@ -565,6 +565,58 @@ func TestAbandonedBatchSealsQueue(t *testing.T) { "a later flusher must be refused: tracking past the dropped rows would let its ack persist an SCN that skips them") } +// TestTrackFailureSealsQueue is TestAbandonedBatchSealsQueue's sibling: an +// ADMITTED flusher parked in checkpoint.Track (capacity exhausted) whose +// context cancels returns with its rows flushed but untracked, while the +// deferred release advances the queue - the same unpinned gap. The failure +// must seal and poison so no later ticket can persist past the dropped rows. +func TestTrackFailureSealsQueue(t *testing.T) { + ctx := t.Context() + logger := service.NewLoggerFromSlog(slog.Default()) + // Capacity for exactly one 2-row batch: the next Track parks. + cp := checkpoint.NewCapped[replication.SCN](2) + + batcher, err := (service.BatchPolicy{Count: 2}).NewBatcher(service.MockResources()) + require.NoError(t, err) + publisher := newBatchPublisher(batcher, cp, logger) + publisher.cacheSCN = func(context.Context, replication.SCN) error { return nil } + t.Cleanup(publisher.Close) + + // Batch 1 fills the tracker; consume it but do not ack. + require.NoError(t, publisher.Publish(ctx, streamingEvent(100))) + firstPublished := make(chan error, 1) + go func() { firstPublished <- publisher.Publish(ctx, streamingEvent(101)) }() + <-publisher.msgs() + require.NoError(t, <-firstPublished) + + // Batch 2 is admitted and parks in Track; cancelling its context fails + // the Track with the rows already out of the batcher. + trackCtx, cancelTrack := context.WithCancel(ctx) + parked := make(chan error, 1) + go func() { + parked <- func() error { + if err := publisher.Publish(trackCtx, streamingEvent(200)); err != nil { + return err + } + return publisher.Publish(trackCtx, streamingEvent(201)) + }() + }() + time.Sleep(100 * time.Millisecond) + cancelTrack() + require.Error(t, <-parked) + + require.True(t, publisher.poisoned.Load(), + "a failed Track stranded flushed-but-untracked rows; the publisher must be marked for rebuild") + laterErr := func() error { + if err := publisher.Publish(ctx, streamingEvent(300)); err != nil { + return err + } + return publisher.Publish(ctx, streamingEvent(301)) + }() + require.ErrorIs(t, laterErr, errQueueSealed, + "a later flusher must be refused: tracking past the dropped rows would let its ack persist an SCN that skips them") +} + // TestFailedSendPoisonsPublisher verifies that a tracked batch that cannot be // handed to ReadBatch marks the publisher poisoned: its checkpoint slot can // never resolve, so Connect must rebuild the publisher rather than reuse a From 188bb07a4b5de8d77b2dee8ef536a2cdde449f18 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 20 Aug 2026 14:11:15 -0400 Subject: [PATCH 18/24] oracledb_cdc: seal the queue on a failed Flush in every path Review finding completing the flushed-but-untracked table: flushCurrent sealed on a Flush error but loop() discarded the error entirely (rows drained by the failed Flush vanished with no log, no seal) and Publish returned without sealing - in both cases a concurrent flusher could still track and ack past the dropped rows. Flush runs the user's batching.processors chain, so the error is reachable in any config with batch processors. Both paths now seal (and thereby poison) and the loop surfaces the error at error level instead of discarding it. --- internal/impl/oracledb/batcher.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 64a8a01c0a..522570317f 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -251,12 +251,20 @@ func (p *batchPublisher) loop() { p.batcherMu.Unlock() return nil } - sendBatch, _ := p.batcher.Flush(hardStopCtx) + sendBatch, flushErr := p.batcher.Flush(hardStopCtx) var ticket uint64 - if len(sendBatch) > 0 { + if flushErr == nil && len(sendBatch) > 0 { ticket = p.takeTicketLocked() } p.batcherMu.Unlock() + if flushErr != nil { + // The failed Flush drained rows that were never tracked: + // seal so nothing can be tracked (and persisted) past + // them, and surface the failure instead of discarding it. + p.sealQueue() + p.log.Errorf("Flushing timed batch failed; the publisher is marked for rebuild and its rows re-read from the last durable SCN on reconnect: %v", flushErr) + return flushErr + } if len(sendBatch) == 0 { return nil } @@ -375,6 +383,9 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven } b.batcherMu.Unlock() if err != nil { + // The failed Flush drained rows that were never tracked: seal so + // nothing can be tracked (and persisted) past them. + b.sealQueue() return fmt.Errorf("flushing batch due to reaching count limit: %w", err) } if len(flushedBatch) == 0 { From 9b5ade386c3955561c7e66f6d310e194a740fd2d Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 20 Aug 2026 14:36:54 -0400 Subject: [PATCH 19/24] oracledb_cdc: cover the monotonic guard and poisoned rebuild with tests Review ask from the mssqlserver sibling, applied here too: the rebuild block is extracted into rebuildPublisherIfPoisoned and unit-tested (old generation closed, fresh tracker stored, late ack from the abandoned generation is a no-op on the durable position), and cacheSCN's advance/equal/regress/invalid semantics are locked in directly. --- internal/impl/oracledb/input_oracledb_cdc.go | 38 +++-- .../oracledb/input_oracledb_cdc_unit_test.go | 134 ++++++++++++++++++ 2 files changed, 160 insertions(+), 12 deletions(-) create mode 100644 internal/impl/oracledb/input_oracledb_cdc_unit_test.go diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index 72828ee982..6737e2f6ea 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -451,11 +451,33 @@ func newOracleDBCDCInput(conf *service.ParsedConfig, resources *service.Resource return conf.WrapBatchInputExtractTracingSpanMapping("oracledb_cdc", batchInput) } +// rebuildPublisherIfPoisoned returns the current publisher, replacing it +// first when a failed send or a sealed flush queue poisoned it: the old +// generation is closed (in-flight ack functions keep resolving into the +// abandoned tracker, where cacheSCN's monotonic guard makes any stale +// persist a no-op) and a fresh batcher and tracker take its place, so the +// new session resumes from the last durable SCN. +func (o *oracleDBCDCInput) rebuildPublisherIfPoisoned() (*batchPublisher, error) { + publisher := o.publisher.Load() + if !publisher.poisoned.Load() { + return publisher, nil + } + o.log.Warn("Rebuilding publisher: a batch could not be handed to the pipeline, so the previous checkpoint tracker is pinned") + publisher.Close() + batcher, err := o.batching.NewBatcher(o.res) + if err != nil { + return nil, fmt.Errorf("rebuilding batcher: %w", err) + } + publisher = newBatchPublisher(batcher, checkpoint.NewCapped[replication.SCN](int64(o.checkpointLimit)), o.log) + publisher.cacheSCN = o.cacheSCN + o.publisher.Store(publisher) + return publisher, nil +} + func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { var ( userTables []replication.UserTable cachedSCN replication.SCN - err error isCDB bool ) @@ -465,17 +487,9 @@ func (o *oracleDBCDCInput) Connect(ctx context.Context) (resErr error) { // durable SCN, which is necessarily before the orphaned rows, and the old // session's late acks resolve into the abandoned tracker (cacheSCN's // monotonic guard turns any stale write into a no-op). - publisher := o.publisher.Load() - if publisher.poisoned.Load() { - o.log.Warn("Rebuilding publisher: a batch could not be handed to the pipeline, so the previous checkpoint tracker is pinned") - publisher.Close() - batcher, batcherErr := o.batching.NewBatcher(o.res) - if batcherErr != nil { - return fmt.Errorf("rebuilding batcher: %w", batcherErr) - } - publisher = newBatchPublisher(batcher, checkpoint.NewCapped[replication.SCN](int64(o.checkpointLimit)), o.log) - publisher.cacheSCN = o.cacheSCN - o.publisher.Store(publisher) + publisher, err := o.rebuildPublisherIfPoisoned() + if err != nil { + return err } if o.db != nil { diff --git a/internal/impl/oracledb/input_oracledb_cdc_unit_test.go b/internal/impl/oracledb/input_oracledb_cdc_unit_test.go new file mode 100644 index 0000000000..adaf9e59f6 --- /dev/null +++ b/internal/impl/oracledb/input_oracledb_cdc_unit_test.go @@ -0,0 +1,134 @@ +// Copyright 2026 Redpanda Data, Inc. +// +// Licensed as a Redpanda Enterprise file under the Redpanda Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/redpanda-data/connect/blob/main/licenses/rcl.md + +package oracledb + +import ( + "context" + "log/slog" + "sync" + "testing" + "time" + + "github.com/Jeffail/checkpoint" + "github.com/Jeffail/shutdown" + "github.com/stretchr/testify/require" + + "github.com/redpanda-data/benthos/v4/public/service" + + "github.com/redpanda-data/connect/v4/internal/impl/oracledb/replication" +) + +// recordingCache is a minimal service.Cache capturing Set calls. +type recordingCache struct { + service.Cache + mu sync.Mutex + sets [][]byte +} + +func (c *recordingCache) Set(_ context.Context, _ string, value []byte, _ *time.Duration) error { + c.mu.Lock() + defer c.mu.Unlock() + c.sets = append(c.sets, append([]byte(nil), value...)) + return nil +} + +func (c *recordingCache) recorded() [][]byte { + c.mu.Lock() + defer c.mu.Unlock() + return append([][]byte(nil), c.sets...) +} + +func newTestInput(t *testing.T) (*oracleDBCDCInput, *recordingCache) { + t.Helper() + cache := &recordingCache{} + o := &oracleDBCDCInput{ + cfg: Config{SCNCacheKey: "scn"}, + res: service.MockResources(), + log: service.NewLoggerFromSlog(slog.Default()), + stopSig: shutdown.NewSignaller(), + cpCache: cache, + batching: service.BatchPolicy{Count: 1}, + checkpointLimit: 8, + } + batcher, err := o.batching.NewBatcher(o.res) + require.NoError(t, err) + pub := newBatchPublisher(batcher, checkpoint.NewCapped[replication.SCN](8), o.log) + pub.cacheSCN = o.cacheSCN + o.publisher.Store(pub) + t.Cleanup(func() { o.publisher.Load().Close() }) + return o, cache +} + +// TestCacheSCNMonotonicGuard locks in the persist guard: advancing writes +// land, equal and regressing writes are silently skipped - a stale ack from +// an abandoned publisher generation must never move the durable resume +// position backwards. +func TestCacheSCNMonotonicGuard(t *testing.T) { + o, cache := newTestInput(t) + ctx := t.Context() + + require.NoError(t, o.cacheSCN(ctx, replication.SCN(10))) + require.NoError(t, o.cacheSCN(ctx, replication.SCN(20)), "an advancing SCN must persist") + require.NoError(t, o.cacheSCN(ctx, replication.SCN(20)), "an equal SCN is a no-op, not an error") + require.NoError(t, o.cacheSCN(ctx, replication.SCN(15)), "a regressing SCN is a no-op, not an error") + + got := cache.recorded() + require.Len(t, got, 2, "only the two advancing writes may reach the cache") + + require.Error(t, o.cacheSCN(ctx, replication.InvalidSCN), "an invalid SCN is rejected") +} + +// TestRebuildPublisherIfPoisoned proves the rebuild actually swaps +// generations: the old publisher is closed, the new one is distinct with a +// fresh tracker wired to cacheSCN, and a late ack from the OLD generation +// cannot regress the durable position past the guard. +func TestRebuildPublisherIfPoisoned(t *testing.T) { + o, cache := newTestInput(t) + ctx := t.Context() + + old := o.publisher.Load() + + // Not poisoned: same generation back. + same, err := o.rebuildPublisherIfPoisoned() + require.NoError(t, err) + require.Same(t, old, same) + + // Deliver a batch on the old generation but hold its ack (late ack). The + // flushing Publish blocks on the unbuffered channel until consumed. + oldPublished := make(chan error, 1) + go func() { oldPublished <- old.Publish(ctx, streamingEvent(10)) }() + oldMsg := <-old.msgs() + require.NoError(t, <-oldPublished) + + // Poison and rebuild. + old.poisoned.Store(true) + rebuilt, err := o.rebuildPublisherIfPoisoned() + require.NoError(t, err) + require.NotSame(t, old, rebuilt, "a poisoned publisher must be replaced") + require.Same(t, rebuilt, o.publisher.Load(), "the stored pointer must be the new generation") + select { + case <-old.shutSig.HasStoppedChan(): + default: + t.Fatal("the old generation's flush loop must be stopped by the rebuild") + } + + // The new generation persists progress normally. + newPublished := make(chan error, 1) + go func() { newPublished <- rebuilt.Publish(ctx, streamingEvent(30)) }() + newMsg := <-rebuilt.msgs() + require.NoError(t, <-newPublished) + require.NoError(t, newMsg.ackFn(ctx, nil)) + + // The old generation's late ack resolves into its abandoned tracker and + // must be a no-op on the durable position (monotonic guard). + require.NoError(t, oldMsg.ackFn(ctx, nil)) + got := cache.recorded() + require.NotEmpty(t, got) + require.Equal(t, replication.SCN(30).Bytes(), got[len(got)-1], "a late ack from the abandoned generation must not regress the cache") +} From 1c62591d3a82dc4531d5bced9b162b627b5823ed Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 20 Aug 2026 20:19:38 -0400 Subject: [PATCH 20/24] oracledb_cdc: make abandon-seal atomic and seal Flush errors under batcherMu Two review findings on the seal placement, both TOCTOU windows: - Abandon and seal were separate steps (admit marked abandoned under ticketMu, the caller sealed afterwards): the moment abandoned[ticket] is visible, a release from the previous holder can skip it and admit the next ticket, which can track, deliver, and ack past the dropped rows before the caller's seal lands. admit now takes ownsRows and seals+poisons in the same ticketMu critical section that records the abandonment; row-less abandons stay benign. - The Flush-error seal ran after batcherMu was released: in that gap another flusher could flush, take the next ticket, and be admitted past the discarded rows. All three paths now seal before the unlock (batcherMu before ticketMu is the established order, so the nesting is safe), and flushCurrent seals on any Flush error for consistency. --- internal/impl/oracledb/batcher.go | 89 ++++++++++++++++++------------- 1 file changed, 52 insertions(+), 37 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 522570317f..43db27c99e 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -106,12 +106,14 @@ var errQueueSealed = errors.New("publisher flush queue sealed after an abandoned // admit blocks until it is ticket's turn to Track+send, or ctx is cancelled. // On success, pair with release. On cancellation the ticket is marked // abandoned - release skips it when its turn comes - and the caller must NOT -// release it. A caller whose ticket owned a non-empty flushed batch MUST call -// sealQueue after an abandon: the batch was never tracked, so only sealing -// (no later ticket can ever track) plus the poison rebuild guarantees its -// rows are re-read from the last durable checkpoint rather than silently -// skipped by a later batch's ack. -func (b *batchPublisher) admit(ctx context.Context, ticket uint64) error { +// release it. ownsRows declares whether the ticket holds a non-empty flushed +// batch: such an abandon seals and poisons IN THE SAME critical section that +// records the abandonment, because the moment abandoned[ticket] is visible, +// a release from the previous holder may skip it and admit the next ticket - +// sealing any later would let that ticket track, deliver, and ack past the +// dropped rows before the seal lands. Row-less abandons (barrier tickets, +// window markers) skip benignly. +func (b *batchPublisher) admit(ctx context.Context, ticket uint64, ownsRows bool) error { b.ticketMu.Lock() if b.sealed { b.ticketMu.Unlock() @@ -153,22 +155,36 @@ func (b *batchPublisher) admit(ctx context.Context, ticket uint64) error { } delete(b.waiters, ticket) b.abandoned[ticket] = struct{}{} + if ownsRows { + b.sealLocked() + } b.ticketMu.Unlock() + if ownsRows { + b.poisoned.Store(true) + } return ctx.Err() } } -// sealQueue permanently refuses further admissions and poisons the publisher: -// called when an abandoned ticket dropped a flushed-but-untracked batch, so -// no later batch can be tracked (and therefore no ack can persist a position) -// past the dropped rows before Connect rebuilds. -func (b *batchPublisher) sealQueue() { - b.ticketMu.Lock() +// sealLocked marks the queue sealed and wakes every waiter (they observe the +// seal and refuse). Caller must hold ticketMu. +func (b *batchPublisher) sealLocked() { b.sealed = true for t, ch := range b.waiters { close(ch) delete(b.waiters, t) } +} + +// sealQueue permanently refuses further admissions and poisons the publisher: +// called when flushed-but-untracked rows were dropped (a failed Flush or +// trackBatch), so no later batch can be tracked (and therefore no ack can +// persist a position) past the dropped rows before Connect rebuilds. Safe to +// call while holding batcherMu: the established order is batcherMu before +// ticketMu, never the reverse. +func (b *batchPublisher) sealQueue() { + b.ticketMu.Lock() + b.sealLocked() b.ticketMu.Unlock() b.poisoned.Store(true) } @@ -256,12 +272,15 @@ func (p *batchPublisher) loop() { if flushErr == nil && len(sendBatch) > 0 { ticket = p.takeTicketLocked() } - p.batcherMu.Unlock() if flushErr != nil { - // The failed Flush drained rows that were never tracked: - // seal so nothing can be tracked (and persisted) past - // them, and surface the failure instead of discarding it. + // The failed Flush drained rows that were never tracked. + // Seal BEFORE releasing batcherMu: in the gap after the + // unlock another flusher could flush, take the next + // ticket, and be admitted past the dropped rows. p.sealQueue() + } + p.batcherMu.Unlock() + if flushErr != nil { p.log.Errorf("Flushing timed batch failed; the publisher is marked for rebuild and its rows re-read from the last durable SCN on reconnect: %v", flushErr) return flushErr } @@ -269,10 +288,7 @@ func (p *batchPublisher) loop() { return nil } - if err := p.admit(hardStopCtx, ticket); err != nil { - if !errors.Is(err, errQueueSealed) && len(sendBatch) > 0 { - p.sealQueue() - } + if err := p.admit(hardStopCtx, ticket, true); err != nil { return err } defer p.release() @@ -381,21 +397,22 @@ func (b *batchPublisher) Publish(ctx context.Context, m *replication.MessageEven ticket = b.takeTicketLocked() } } - b.batcherMu.Unlock() if err != nil { - // The failed Flush drained rows that were never tracked: seal so - // nothing can be tracked (and persisted) past them. + // The failed Flush drained rows that were never tracked. Seal BEFORE + // releasing batcherMu: in the gap after the unlock another flusher + // could flush, take the next ticket, and be admitted past the + // dropped rows. b.sealQueue() + } + b.batcherMu.Unlock() + if err != nil { return fmt.Errorf("flushing batch due to reaching count limit: %w", err) } if len(flushedBatch) == 0 { return nil } - if err := b.admit(ctx, ticket); err != nil { - if !errors.Is(err, errQueueSealed) && len(flushedBatch) > 0 { - b.sealQueue() - } + if err := b.admit(ctx, ticket, true); err != nil { return err } defer b.release() @@ -573,21 +590,19 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { // trackBatch+send, so once flushCurrent returns the gate counts every // published snapshot batch and waitSnapshotAcks cannot release early. ticket := b.takeTicketLocked() + if err != nil { + // The failed Flush may have drained rows that were never tracked. + // Seal BEFORE releasing batcherMu: in the gap after the unlock + // another flusher could flush, take the next ticket, and be admitted + // past the dropped rows. + b.sealQueue() + } b.batcherMu.Unlock() - if admitErr := b.admit(ctx, ticket); admitErr != nil { - if !errors.Is(admitErr, errQueueSealed) && len(remaining) > 0 { - b.sealQueue() - } + if admitErr := b.admit(ctx, ticket, len(remaining) > 0); admitErr != nil { return admitErr } defer b.release() if err != nil { - if len(remaining) > 0 { - // Rows came out of the batcher alongside the error: they were - // never tracked, so seal before the deferred release lets later - // tickets persist past them. - b.sealQueue() - } return err } if len(remaining) == 0 { From c491bee89b8999197c1bd375ea33f17fc408b3b0 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Thu, 20 Aug 2026 20:59:19 -0400 Subject: [PATCH 21/24] oracledb_cdc: surface the real flush error in flushCurrent Review finding: after the seal moved under batcherMu, admit always refuses with errQueueSealed when Flush has failed, so flushCurrent's post-admit error check was dead and the operator-facing log described a sealed queue instead of the actual batching.processors failure. The flush error now returns before admit (the seal is already applied, so the barrier is unaffected), matching how Publish and loop() surface it. --- internal/impl/oracledb/batcher.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 43db27c99e..c935c21be1 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -598,13 +598,16 @@ func (b *batchPublisher) flushCurrent(ctx context.Context) error { b.sealQueue() } b.batcherMu.Unlock() + if err != nil { + // The seal is already applied under batcherMu; return the real flush + // error rather than letting admit's sealed refusal mask it (the + // operator needs the batching.processors failure, not the seal). + return err + } if admitErr := b.admit(ctx, ticket, len(remaining) > 0); admitErr != nil { return admitErr } defer b.release() - if err != nil { - return err - } if len(remaining) == 0 { return nil } From 2e964495a935e5542c79bb614395cf4f98daa2c2 Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Fri, 21 Aug 2026 09:17:32 -0400 Subject: [PATCH 22/24] oracledb_cdc: document that the Flush-error seals are contract-defensive Investigating the review's test ask showed the branch is unreachable through the current public API: service.Batcher.Flush never assigns its error return (the internal policy batcher returns only a batch, and processor failures surface as errored messages) - verified empirically with a registered hard-failing BatchProcessor and confirmed in the wrapper source. The seals stay because the signature declares the error and sealing is the correct handling if a future benthos version does fail here; the timed-loop branch now documents the unreachability. --- internal/impl/oracledb/batcher.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index c935c21be1..2fdd6000e7 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -273,10 +273,14 @@ func (p *batchPublisher) loop() { ticket = p.takeTicketLocked() } if flushErr != nil { - // The failed Flush drained rows that were never tracked. - // Seal BEFORE releasing batcherMu: in the gap after the - // unlock another flusher could flush, take the next - // ticket, and be admitted past the dropped rows. + // Defensive: the current benthos Batcher.Flush never + // assigns its error return (processor failures surface as + // errored messages), so this branch is unreachable today - + // but the signature declares the error, and if a future + // version does fail here the drained rows were never + // tracked. Seal BEFORE releasing batcherMu: in the gap + // after the unlock another flusher could take the next + // ticket and be admitted past the dropped rows. p.sealQueue() } p.batcherMu.Unlock() From 6640238c800b8ab0d28678445b7047edfa583c3b Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Fri, 21 Aug 2026 09:21:57 -0400 Subject: [PATCH 23/24] oracledb_cdc: log undelivered-at-shutdown batches at debug Ports the mssqlserver log-level split: sendTracked's cancellation under a signalled soft stop is the expected graceful-shutdown path and logs at debug; the warning stays for a send that fails while the publisher is meant to be live. --- internal/impl/oracledb/batcher.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 2fdd6000e7..7a23eec135 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -529,7 +529,13 @@ func (b *batchPublisher) sendTracked(ctx context.Context, tracked *trackedBatch) // Resolving the slot here instead would be unsafe - another flusher // may already have delivered a later-tracked batch, and its ack would // then persist an SCN past these undelivered rows. - b.log.Warnf("Batch of %d messages could not be handed to the pipeline; the publisher is marked for rebuild and its rows re-read from the last durable SCN on reconnect", len(tracked.msgs.msg)) + if b.shutSig.IsSoftStopSignalled() { + // Expected on a graceful stop: nothing drains msgChan once + // ReadBatch stops, and Close cancels this send. Not a fault. + b.log.Debugf("Batch of %d messages undelivered at shutdown; its rows re-read from the last durable SCN on the next run", len(tracked.msgs.msg)) + } else { + b.log.Warnf("Batch of %d messages could not be handed to the pipeline; the publisher is marked for rebuild and its rows re-read from the last durable SCN on reconnect", len(tracked.msgs.msg)) + } b.poisoned.Store(true) return ctx.Err() } From 24655483f58ef33af8946b32941b1ed39be9559e Mon Sep 17 00:00:00 2001 From: Jonathan Chaput Date: Fri, 21 Aug 2026 09:38:16 -0400 Subject: [PATCH 24/24] oracledb_cdc: set the stopping flag before shutdown cancellation propagates Review finding on the log-level split: the check read the publisher's own shutSig, which on the streaming path is only triggered by pub.Close() AFTER the session contexts have already been cancelled - so the graceful unwind still warned. Close now sets an explicit stopping flag on the publisher before triggering the input's soft stop, and sendTracked checks it alongside shutSig. --- internal/impl/oracledb/batcher.go | 8 +++++++- internal/impl/oracledb/input_oracledb_cdc.go | 6 ++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/internal/impl/oracledb/batcher.go b/internal/impl/oracledb/batcher.go index 7a23eec135..647366c43c 100644 --- a/internal/impl/oracledb/batcher.go +++ b/internal/impl/oracledb/batcher.go @@ -56,6 +56,12 @@ type batchPublisher struct { // batcher.Close races in-flight Publish calls otherwise, and the batcher // is not goroutine-safe. closed bool + // stopping is set by the input's Close BEFORE any cancellation + // propagates, so sendTracked can distinguish the expected + // graceful-shutdown unwind (debug) from a send that fails while the + // pipeline is meant to be live (warn) - the publisher's own shutSig is + // triggered too late on the streaming path to make that call. + stopping atomic.Bool // poisoned is set when a tracked batch could not be handed to ReadBatch: // its checkpoint slot can never resolve, so this publisher can never // checkpoint past it. Connect rebuilds a poisoned publisher. @@ -529,7 +535,7 @@ func (b *batchPublisher) sendTracked(ctx context.Context, tracked *trackedBatch) // Resolving the slot here instead would be unsafe - another flusher // may already have delivered a later-tracked batch, and its ack would // then persist an SCN past these undelivered rows. - if b.shutSig.IsSoftStopSignalled() { + if b.stopping.Load() || b.shutSig.IsSoftStopSignalled() { // Expected on a graceful stop: nothing drains msgChan once // ReadBatch stops, and Close cancels this send. Not a fault. b.log.Debugf("Batch of %d messages undelivered at shutdown; its rows re-read from the last durable SCN on the next run", len(tracked.msgs.msg)) diff --git a/internal/impl/oracledb/input_oracledb_cdc.go b/internal/impl/oracledb/input_oracledb_cdc.go index 6737e2f6ea..db5a3450ef 100644 --- a/internal/impl/oracledb/input_oracledb_cdc.go +++ b/internal/impl/oracledb/input_oracledb_cdc.go @@ -842,6 +842,12 @@ func (o *oracleDBCDCInput) Close(ctx context.Context) error { if o.stopSig == nil { return nil // Never connected } + // Mark the publisher as stopping BEFORE any cancellation propagates: the + // session's contexts unwind off stopSig, and sendTracked needs the flag + // already visible to log the graceful unwind at debug rather than warn. + if pub := o.publisher.Load(); pub != nil { + pub.stopping.Store(true) + } o.stopSig.TriggerSoftStop() select { case <-ctx.Done():