cockroachdb / cockroachdb/cockroach

physical replication: silent data loss in span config ingestion when retry loop exhausts

Open
#166,640 1 comment 0 reactions 0 assignees View on GitHub
branch-master C-bug O-agent T-testeng
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

## Summary

`flushEvents` in `pkg/crosscluster/physical/ingest_span_configs.go` silently drops buffered span configuration updates when its retry loop exhausts all attempts. The function returns `nil` and clears the buffer even though no data was persisted, causing the physical replication stream to lose span config updates without any error signal. This can lead to the destination tenant operating with stale or incorrect span configurations.

## Affected code

- `pkg/crosscluster/physical/ingest_span_configs.go:259-297` -- `flushEvents` method
- Specifically lines 294-296: after the retry loop exits (due to exhaustion), `resetBuffer()` is called unconditionally and `nil` is returned.

## Reproduction

This bug requires internal/unit-test-level access to trigger. A SQL reproduction is not feasible.

**Test sketch (pseudocode):**

```
// Setup: create a spanConfigIngestor with a mock KVAccessor that
// always returns commitTimestampOutOfBoundsError from UpdateSpanConfigRecords.

ingestor := spanConfigIngestor{
accessor: mockAccessorAlwaysReturnsLeaseExpired,
session: validSession,
// ... other fields
}

// Populate the buffer with some span config records
ingestor.bufferedUpdates = []spanconfig.Record{someRecord}

// Call flushEvents
err := ingestor.flushEvents(ctx)

// ASSERT: err should be non-nil (currently returns nil -- BUG)
require.Error(t, err)
// ASSERT: buffer should NOT be cleared if flush failed
require.NotEmpty(t, ingestor.bufferedUpdates)
```

## Secondary issue: testing knob called on error path

At line 279-281, `RightAfterSpanConfigFlush` is invoked before the error from `UpdateSpanConfigRecords` is checked. This means the knob fires even on failed flushes in the non-full-scan path, which is inconsistent with the full-scan path (line 339) where the knob only fires on success.

## Suggested fix direction

After the retry loop exits, check whether the loop completed successfully or fell through due to retry exhaustion. One approach:

```go
var success bool
for retrier := retry.StartWithCtx(ctx, retryOpts); retrier.Next(); {
// ... existing logic ...
if err != nil {
if spanconfig.IsCommitTimestampOutOfBoundsError(err) {
continue
}
return err
}
success = true
break
}
if !success {
return errors.New("failed to flush span config events: retries exhausted due to repeated lease expiration")
}
sc.resetBuffer()
return nil
```

Additionally, move the testing knob invocation (lines 279-281) to after the error check to avoid calling it on failed flushes.

_This issue was found via automated deep static analysis._

Jira issue: CRDB-62046

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.