Skip to content

Commit 903a5f7

Browse files
committed
test(raft): rename cap + add Bytes() round-trip (PR #746 r2)
Round-2 review on commit 070d5e6 from claude bot: - Issue A (gocritic builtinShadow): renamed local const `cap` to `spoolCap` in TestSnapshotSpool_OverrideViaEnv. `cap` is a Go built-in identifier and shadows trip golangci-lint's gocritic rule. - Issue B (refactor coverage gap): Bytes() switched from io.ReadAll to make([]byte, s.size) + io.ReadFull at PR-round-1. TestSnapshotSpool_DefaultCapAcceptsRealisticFSM exercised Write at 1.5 GiB but never called Bytes() — a regression where the pre-allocation drifts out of sync with the on-disk size would not surface. Added a Bytes() call after the write loop with: - len(got) == target (catches short reads / size drift) - first/last byte == 0xAB (catches catastrophic corruption without paying the memory cost of full byte-equality on a 1.5 GiB slice) No semantic changes. The OOM concern from the Codex P1 reviewer is acknowledged in the PR body and tracked as a follow-up (streaming apply); the immediate-incident risk is bounded by real FSM size (~1.35 GiB today), not by the new 16 GiB ceiling. Test: go test -race -count=1 -short ./internal/raftengine/etcd -- 11.6s, all green. go test -race -count=1 \ -run TestSnapshotSpool_DefaultCapAcceptsRealisticFSM \ ./internal/raftengine/etcd -- 3.0s, green (1.5 GiB Write + Bytes round-trip succeeds).
1 parent 070d5e6 commit 903a5f7

1 file changed

Lines changed: 17 additions & 4 deletions

File tree

internal/raftengine/etcd/snapshot_spool_test.go

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,23 +45,36 @@ func TestSnapshotSpool_DefaultCapAcceptsRealisticFSM(t *testing.T) {
4545
written += int64(n)
4646
}
4747
require.Equal(t, target, spool.size)
48+
49+
// Round-trip through the materialization path (the io.ReadAll →
50+
// io.ReadFull refactor) to lock down behaviour at 1.5 GiB. The
51+
// returned slice MUST match s.size exactly: a short read here would
52+
// indicate the pre-allocation drifted out of sync with what Write
53+
// actually persisted to the spool file.
54+
got, err := spool.Bytes()
55+
require.NoError(t, err)
56+
require.Equal(t, int(target), len(got), "Bytes() returned %d, want %d", len(got), target)
57+
// Spot-check first/last bytes match the 0xAB fill; full byte-equality
58+
// would double the test's memory cost without adding signal.
59+
require.Equal(t, byte(0xAB), got[0])
60+
require.Equal(t, byte(0xAB), got[len(got)-1])
4861
}
4962

5063
// TestSnapshotSpool_OverrideViaEnv confirms the env knob actually moves the
5164
// cap. Tests deliberately *lower* it (cheap to write past) instead of
5265
// raising — the upper-bound test above already proves a generous cap works.
5366
func TestSnapshotSpool_OverrideViaEnv(t *testing.T) {
54-
const cap = int64(4096)
55-
t.Setenv(maxSnapshotPayloadBytesEnvVar, strconv.FormatInt(cap, 10))
67+
const spoolCap = int64(4096)
68+
t.Setenv(maxSnapshotPayloadBytesEnvVar, strconv.FormatInt(spoolCap, 10))
5669

5770
spool, err := newSnapshotSpool(t.TempDir())
5871
require.NoError(t, err)
5972
t.Cleanup(func() { _ = spool.Close() })
6073

61-
require.Equal(t, cap, spool.maxSize)
74+
require.Equal(t, spoolCap, spool.maxSize)
6275

6376
// Write up to the cap — succeeds.
64-
_, err = spool.Write(bytes.Repeat([]byte{0x01}, int(cap)))
77+
_, err = spool.Write(bytes.Repeat([]byte{0x01}, int(spoolCap)))
6578
require.NoError(t, err)
6679

6780
// One byte past — fails with the documented sentinel so callers can

0 commit comments

Comments
 (0)