Skip to content

Commit df2407a

Browse files
committed
backup: reject Add after WriteTo on snapshotBuilder (claude review)
claude review on PR #825 found a silent-data-loss footgun: after WriteTo set b.written, Add still passed its checks and appended to b.entries, but those records could never be flushed (the second WriteTo fails closed before re-sorting). For the M2-M5 adapter feed loops this would silently drop records on builder reuse. Add now guards on b.written at the top and returns the same ErrSnapshotBuilderReused sentinel WriteTo uses, so a caller reusing an exhausted builder gets one consistent signal regardless of method. Caller audit: the only Add callers are tests; the production callers (per-adapter encoders) land in M2-M5, so no existing call site relies on post-WriteTo Add. Test: TestSnapshotBuilderRejectsAddAfterWriteTo.
1 parent d83d398 commit df2407a

2 files changed

Lines changed: 30 additions & 0 deletions

File tree

internal/backup/encode.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,16 @@ func newSnapshotBuilder(commitTS uint64) *snapshotBuilder {
150150
// order-dependent `.fsm`. userKey/userValue are copied — callers may
151151
// reuse their buffers after Add returns.
152152
func (b *snapshotBuilder) Add(userKey, userValue []byte, expireAt uint64) error {
153+
// A builder is single-use. Once WriteTo has flushed, any further
154+
// Add would stage entries that can never be written (the second
155+
// WriteTo fails closed before re-sorting), silently dropping
156+
// records. Reject with the same sentinel WriteTo uses so a caller
157+
// reusing an exhausted builder gets one consistent signal
158+
// regardless of which method it calls (claude review on PR #825 —
159+
// a silent-data-loss footgun for the M2-M5 adapter feed loops).
160+
if b.written {
161+
return errors.WithStack(ErrSnapshotBuilderReused)
162+
}
153163
// Size-check from the user-buffer lengths before allocating the
154164
// framed buffers — encKey = userKey + snapshotTSSize, encVal =
155165
// snapshotValueHeaderSize + userValue — so an oversize record

internal/backup/encode_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,26 @@ func TestSnapshotBuilderRejectsReuse(t *testing.T) {
269269
}
270270
}
271271

272+
// TestSnapshotBuilderRejectsAddAfterWriteTo pins that Add fails closed
273+
// after WriteTo rather than silently staging records that can never be
274+
// flushed (the second WriteTo would reject before re-sorting). Guards
275+
// the M2-M5 adapter feed loops against a silent-data-loss reuse bug.
276+
func TestSnapshotBuilderRejectsAddAfterWriteTo(t *testing.T) {
277+
t.Parallel()
278+
b := newSnapshotBuilder(encodeFixtureTS)
279+
if err := b.Add([]byte("!redis|str|first"), []byte("v"), 0); err != nil {
280+
t.Fatalf("Add: %v", err)
281+
}
282+
var buf bytes.Buffer
283+
if _, err := b.WriteTo(&buf); err != nil {
284+
t.Fatalf("WriteTo: %v", err)
285+
}
286+
err := b.Add([]byte("!redis|str|second"), []byte("v"), 0)
287+
if !errors.Is(err, ErrSnapshotBuilderReused) {
288+
t.Fatalf("Add-after-WriteTo err = %v, want ErrSnapshotBuilderReused", err)
289+
}
290+
}
291+
272292
// TestResolveCommitTS pins the --last-commit-ts override semantics
273293
// (design §"MVCC re-encoding"): no override yields the manifest value;
274294
// an override >= manifest is accepted; an override < manifest fails

0 commit comments

Comments
 (0)