Skip to content

Commit 070d5e6

Browse files
committed
refactor(raft): address PR #746 round-1 review
- gemini medium (snapshot_spool.go:75): Switch the cap check from `int64(len(p))+s.size > s.maxSize` to `int64(len(p)) > s.maxSize-s.size`. With the new env override an operator can set maxSize near math.MaxInt64; the previous form would overflow there and silently let the write through. Subtraction stays in [0, maxSize] and rejects the same payloads correctly. Mathematically equivalent for all sane configurations, no caller-visible semantic change. - gemini medium (snapshot_spool.go:90): Replace `io.ReadAll(s.file)` with `make([]byte, s.size)` + `io.ReadFull`. Write only increments s.size on successful os.File.Write returns, so it is the truth-of- record for what's on disk. Pre-allocating skips ~30 reallocs and intermediate copies that ReadAll would do via power-of-two doublings on a 1.35 GiB receive. - codex P1 (snapshot_spool.go:28): "Keep snapshot cap within in-memory apply limit" — a 16 GiB receive cap admits payloads larger than the in-memory materialization can survive on a memory-constrained node. Acknowledged: this PR alone is the on-disk-cap fix; the in-memory hazard is closed by stacked PR #747, which routes receive into the existing token-format streaming path so Snapshot.Data is a 17-byte token (not the payload) and apply reads from disk via io.Reader. #747 keeps the receive cap aligned with disk space (where 16 GiB is realistic) rather than RAM (where it is not). See PR #747 for the streaming-receive change and self-review. No semantic changes here: Write's reject/accept set is unchanged; Bytes() returns the same content via a faster path. Tests still green: go test -race -count=1 -short ./internal/raftengine/etcd -- 11.6s, all green.
1 parent 2bcd177 commit 070d5e6

1 file changed

Lines changed: 16 additions & 7 deletions

File tree

internal/raftengine/etcd/snapshot_spool.go

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,13 @@ func newSnapshotSpool(dir string) (*snapshotSpool, error) {
7171
}
7272

7373
func (s *snapshotSpool) Write(p []byte) (int, error) {
74-
if int64(len(p))+s.size > s.maxSize {
75-
return 0, errors.Wrapf(errSnapshotPayloadTooLarge, "%d > %d", int64(len(p))+s.size, s.maxSize)
74+
// Subtraction-based comparison so the cap check stays correct even when
75+
// s.maxSize is set to a value near math.MaxInt64 via the env override:
76+
// `int64(len(p))+s.size > s.maxSize` would overflow into a negative number
77+
// at large maxSize and let the write through. `int64(len(p)) > s.maxSize-s.size`
78+
// stays in [0, maxSize] and rejects the same payloads correctly.
79+
if int64(len(p)) > s.maxSize-s.size {
80+
return 0, errors.Wrapf(errSnapshotPayloadTooLarge, "adding %d bytes to current %d would exceed limit %d", len(p), s.size, s.maxSize)
7681
}
7782
n, err := s.file.Write(p)
7883
s.size += int64(n)
@@ -86,13 +91,17 @@ func (s *snapshotSpool) Bytes() ([]byte, error) {
8691
if _, err := s.file.Seek(0, io.SeekStart); err != nil {
8792
return nil, errors.WithStack(err)
8893
}
89-
// Read incrementally instead of sizing a buffer from s.size so malformed
90-
// inputs stay bounded by s.maxSize and file-backed I/O.
91-
data, err := io.ReadAll(s.file)
92-
if err != nil {
94+
// Pre-allocate from the bytes we have already accepted past Write's
95+
// per-call cap check, instead of letting io.ReadAll grow the buffer
96+
// through several power-of-two doublings (a 1.35 GiB receive would
97+
// trigger ~30 reallocs and copy the running total each time). s.size
98+
// is the truth-of-record for what's on disk because Write only
99+
// increments it on successful os.File.Write returns.
100+
buf := make([]byte, s.size)
101+
if _, err := io.ReadFull(s.file, buf); err != nil {
93102
return nil, errors.WithStack(err)
94103
}
95-
return data, nil
104+
return buf, nil
96105
}
97106

98107
func (s *snapshotSpool) Reader() (io.Reader, error) {

0 commit comments

Comments
 (0)