sequenceDiagram
  participant Snapshot as Snapshot Data
  participant Intake as Intake Handlers
  participant State as per-ZSET State
  participant TTL as TTL Router
  participant Emit as JSON Emission
  participant Output as zsets/key.json
  
  Snapshot->>Intake: !zs|meta|... (member count)
  Intake->>State: Store declaredLen, metaSeen
  
  Snapshot->>Intake: !zs|mem|... (member name + score)
  Intake->>State: Decode IEEE-754 score, store member
  
  Snapshot->>TTL: !redis|ttl|... (for ZSET key)
  TTL->>State: Inlined into hasTTL + expireAtMs
  
  Snapshot->>Intake: !zs|score|... or !zs|meta|d|...
  Intake->>Intake: Silently discard (no-op)
  
  Snapshot->>Emit: Finalize() called
  Emit->>State: Collect all ZSET states
  State->>Emit: Check declaredLen vs observedLen
  Emit->>Emit: Warn if mismatch
  Emit->>Emit: Sort members by binary name
  Emit->>Emit: Encode scores (+Inf/-Inf as strings)
  Emit->>Emit: Include expire_at_ms if TTL set
  Emit->>Output: Atomic write with format_version

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@claude review

Claude finished @bootjp's task in 3m 31s —— View job

Code Review: PR #790 — Redis zset encoder (Phase 0a)

Tasks

• Gather context
• Read internal/backup/redis_zset.go
• Read internal/backup/redis_zset_test.go
• Read internal/backup/redis_string.go (modified)
• Read internal/backup/redis_hash.go (modified)
• Analyze correctness, edge cases, conventions
• Post review feedback

Wire format

The encoder constants and decoding arithmetic exactly mirror store/zset_helpers.go:

• !zs|meta| / !zs|mem| / !zs|scr| / !zs|meta|d| prefixes match the live constants.
• redisZSetScoreSize = 8 is the correct mirror of zsetMetaSizeBytes.
• math.Float64frombits(binary.BigEndian.Uint64(value)) correctly inverts MarshalZSetScore (IEEE 754 big-endian).
• The uint64 > math.MaxInt64 overflow guard in HandleZSetMeta (line 104) correctly mirrors the pattern in the hash/list/set encoders.

Issues

Medium — TTL scan-order gap not tested (functional risk)

!redis|ttl| keys sort lexicographically before !zs|... keys (r=0x72 < z=0x7A). In a single sorted Pebble scan, TTL records for zset keys arrive before any !zs|meta| or !zs|mem| record registers the key in kindByKey. That means HandleTTL hits the redisKindUnknown branch and increments orphanTTLCount — the TTL is not inlined, and the JSON expire_at_ms stays null.

The same ordering issue exists for the set encoder (s=0x73 > r=0x72), so this is a pre-existing design decision rather than a new regression. But:

1. TestRedisDB_ZSetTTLInlinedFromScanIndex only exercises the cooperative order (meta → member → TTL). There is no test for the inverse (TTL → meta → member).
2. If the real dispatcher does a single prefix-sorted scan, TTL inlining for zsets (and sets) will silently fall through to orphanTTLCount on every expiring key.

It would be valuable to add a test that documents the current (Phase 0a) behavior under the real scan order, even if that behavior is "TTL becomes an orphan and is reported but not inlined." Otherwise the existing test creates a false sense that TTL inlining works end-to-end.

If the dispatcher is known to do per-prefix scans in a controlled order, a brief note in the encoder's top-of-file comment would be enough.

Fix/document this →

Low — marshalRedisZSetScore: no unreachable guard for NaN

redis_zset.go:254-256 documents that NaN is unreachable because HandleZSetMember rejects it at intake. However, marshalRedisZSetScore has no explicit guard. If NaN somehow enters st.members (e.g., via a future refactor that bypasses the handler), json.Marshal(nan) returns a standard-library error — which surfaces correctly. This is not a silent corruption bug, but it surfaces as an opaque json: unsupported value error rather than the domain-specific ErrRedisInvalidZSetMember. A small defensive check would make the failure mode obvious:

if math.IsNaN(score) {
    // unreachable: HandleZSetMember rejects NaN at intake
    return nil, cockroachdberr.New("backup: internal error: NaN score reached marshalling (should have been rejected at intake)")
}

Confirmed correct

• sort.Strings byte order — sort.Strings compares by byte content in Go; the "raw byte order" claim is accurate.
• //nolint:dupl cross-reference — redis_hash.go:244 now points to redis_zset.go:marshalZSetJSON. The rationale (can't collapse parallel wrappers without breaking JSON field-order determinism) is sound.
• Empty zset emit — Len=0 with metaSeen=true still writes a file; TYPE key → zset is observable to clients. Correct.
• Members-without-meta — metaSeen=false guard in flushZSets:212 prevents a spurious length-mismatch warning. Test covers this.
• HandleZSetScore / HandleZSetMetaDelta as named no-ops — The discards are explicit functions rather than routing to an orphan sink, so a future grep -n 'every !zs| prefix has a handler' audit finds them. The design rationale is documented.
• Delta double-guard — parseZSetMetaKey independently rejects delta-prefixed keys (line 177), so a dispatcher that bypasses HandleZSetMeta's entry guard still surfaces a parse error rather than corrupt state. Well-motivated.
• HandleTTL case redisKindZSet — Additive-only change. r.zsetState(userKey) inside the case is a no-op re-registration of the already-registered key; correct and idempotent.
• Test coverage — 17 cases covering round-trip, empty zset, TTL inlining, binary member base64 envelope, delta-key skip (both entry and parser levels), score-index discard, malformed meta length, overflow guard, MaxInt64 boundary, malformed member value, NaN rejection, ±Inf string form, members-without-meta, duplicate-member latest-wins.

Summary

Solid implementation that faithfully mirrors the hash/list/set encoders and handles all the wire-format corner cases well. The one functional concern is the TTL scan-order edge case (which the existing set encoder shares); whether that's a problem in practice depends on the dispatcher's scan strategy. Adding a test that documents the behavior under inverted order would close the coverage gap without requiring a design change.

@claude review

Claude finished @bootjp's task in 2m 38s —— View job

Code Review: PR #790 — Redis zset encoder (Phase 0a)

Tasks

• Gather context from PR description and prior reviews
• Read internal/backup/redis_zset.go
• Read internal/backup/redis_zset_test.go
• Read internal/backup/redis_string.go (modified)
• Read internal/backup/redis_set.go (modified)
• Cross-check against store/zset_helpers.go
• Analyze and post review

The PR landed in two commits. The second commit (63e54d9) directly addresses the Codex P1 TTL scan-order finding that the previous Claude review and Codex both flagged, so I'm reviewing the whole PR as-shipped (including both commits).

P1 Fix — pendingTTL buffering (commit 2)

The fix is architecturally correct and well-tested. The essential flow:

• HandleTTL parks unknown-kind expiries into pendingTTL (a new map[string]uint64 on RedisDB)
• zsetState() and setState() call claimPendingTTL() on first registration of a user key, draining the parked expiry into the zset/set's expireAtMs
• Finalize() counts len(r.pendingTTL) as orphans after all flush steps, so only keys that truly never appeared as typed records are warned

Key correctness properties verified:

1. No double-set: when TTL arrives after the key is registered, HandleTTL hits case redisKindZSet: → r.zsetState(userKey) fast-paths via if st, ok := r.zsets[uk]; ok { return st } — claimPendingTTL is never re-called. Safe.
2. No double-drain: claimPendingTTL does delete(r.pendingTTL, uk) atomically. A second call for the same key returns (0, false).
3. Finalize ordering: flushSets and flushZSets both run before orphanTTLCount is incremented, so no typed record can claim a TTL after the count fires.
4. hashState/listState correctness: Those state-init functions don't call claimPendingTTL because !hs|/!lst| lex-sort before !redis|ttl| — the typed record always arrives first. pendingTTL is always empty for those keys when their state-init fires. ✓

New tests TestRedisDB_ZSetTTLArrivesBeforeRows, TestRedisDB_SetTTLArrivesBeforeRows, and TestRedisDB_OrphanTTLCountsTrulyUnmatchedKeys all pin this behavior correctly.

Wire format and codec

All field sizes and decoding arithmetic match store/zset_helpers.go:

• MarshalZSetScore → math.Float64bits(score) / binary.BigEndian.PutUint64 matches math.Float64frombits(binary.BigEndian.Uint64(value)) in HandleZSetMember:133. ✓
• ZSetMetaKey layout matches parseZSetMetaKey. ✓
• Delta key !zs|meta|d| double-guard (entry check + parseZSetMetaKey rejection) is the correct defense-in-depth pattern from the hash encoder. ✓
• uint64 > math.MaxInt64 overflow guard mirrors hash/list/set encoders exactly. ✓

Confirmed correct — design decisions

• Sort by member-name bytes, not score — sort.Strings in Go sorts by byte value of the string content; the claim in the docstring is accurate.
• //nolint:dupl — The marshalZSetJSON / marshalHashJSON structural parallel cannot be collapsed without either losing JSON field-order determinism (map) or needing per-type generics (distinct JSON tags). The //nolint is the right call here.
• NaN fail-closed at intake — HandleZSetMember:134 returns ErrRedisInvalidZSetMember for NaN; this prevents silent re-corruption and surfaces at the source record. ✓
• Empty-zset emit — metaSeen=true with Len=0 still writes zsets/<key>.json; TYPE k → zset is observable. ✓
• Members-without-meta — flushZSets:223 checks st.metaSeen before firing the length-mismatch warning, so a members-only run doesn't emit a spurious warning. ✓
• HandleZSetScore / HandleZSetMetaDelta — Named no-op functions rather than routing to the orphan sink. Future audits via grep 'every !zs| prefix has a handler' will find them. ✓

Issues

Low — Base64 test assertion is shallow (redis_zset_test.go:314-316)

TestRedisDB_ZSetBinaryMemberUsesBase64Envelope only verifies the envelope is non-empty:

if envelope["base64"] == "" {
    t.Fatalf("base64 envelope missing payload: %v", envelope)
}

This passes even if the encoded bytes are wrong. The CodeRabbit suggestion is sound — decode and compare against the expected bytes {0x80, 0xff, 0x01}:

payload, _ := envelope["base64"].(string)
decoded, err := base64.StdEncoding.DecodeString(payload)
if err != nil { t.Fatalf(...) }
if !bytes.Equal(decoded, []byte{0x80, 0xff, 0x01}) {
    t.Fatalf("decoded member = %v, want [0x80 0xff 0x01]", decoded)
}

Fix this →

Nit — //nolint:gosec in test key-builder helpers

Per CLAUDE.md convention (Avoid //nolint — refactor instead), the five //nolint:gosec suppressions in encodeZSetMetaValue, zsetMetaKey, zsetMemberKey, zsetScoreKey, and zsetMetaDeltaKey could be removed by adding an explicit bounds panic (e.g., if memberCount < 0 { panic("test bug") } before the conversion). Given these are test helpers with fully-controlled inputs this is very low impact, but worth tracking to stay consistent with the project convention.

Nit — Per-record []byte(prefix) allocations (CodeRabbit)

bytes.HasPrefix(key, []byte(RedisZSetMetaDeltaPrefix)) at lines 87, 177 and similar at 180, 192 create a []byte from a string constant on every call. During a real snapshot scan these fire per record. Hoisting to package-level var redisZSetMetaDeltaPrefixBytes = []byte(RedisZSetMetaDeltaPrefix) (and analogues) eliminates 4 allocations per dispatched record. In the same vein the set and hash encoders have the same pattern — fixing zset here would be consistent.

Future work (not blocking)

• Stream encoder — pendingTTL's doc comment already notes that streamState must call claimPendingTTL once PR backup: Redis stream encoder (Phase 0a) #791 lands. The infrastructure is in place.
• store.UnmarshalZSetMeta missing overflow guard — The live decoder does int64(binary.BigEndian.Uint64(b)) without the > math.MaxInt64 check. The backup encoder is more defensive here. Worth a separate follow-up to align (or document that the live side trusts the write path to never produce > MaxInt64).

Summary

Solid implementation. The Codex P1 scan-order bug was correctly identified and fixed in commit 2 with proper pendingTTL infrastructure and three new tests. The zset encoder itself faithfully mirrors the hash/list/set encoders across wire format, delta handling, NaN rejection, ±Inf serialization, binary member safety, and empty-key emit rules. The only functional gap remaining is the weak base64 assertion in the binary-member test; the rest are style nits.

@claude review

Claude finished @bootjp's task in 4m 37s —— View job

Code Review: PR #790 — Redis zset encoder (Phase 0a) — Round 3

Tasks

• Gather context from PR description and prior reviews
• Read internal/backup/redis_zset.go (all 3 commits)
• Read internal/backup/redis_zset_test.go
• Read internal/backup/redis_string.go (HandleTTL routing)
• Cross-check legacy !redis|zset| handler against live encoder
• Check outstanding issues from prior reviews
• Post comprehensive review

The third commit (5446190) addresses the Codex P1 finding on round 2: legacy !redis|zset|<userKey> blobs were silently dropped from backup output. This review covers the full three-commit PR as shipped.

Commit 3: HandleZSetLegacyBlob — correctness

Magic prefix match: redisZSetLegacyProtoPrefix = []byte{0x00, 'R', 'Z', 0x01} exactly matches adapter/redis_storage_codec.go:15 storedRedisZSetProtoPrefix. A live-side rename would surface as ErrRedisInvalidZSetLegacyBlob on any real cluster dump — the "canary" pattern documented in the constant's docstring is a good operational safeguard.

!redis|zset| < !zs|... ordering (mid-migration merge): comparing at the first diverging byte — r (0x72) vs z (0x7A) — !redis|... sorts before !zs|.... TestRedisDB_ZSetLegacyBlobThenWideColumnMerges tests the correct order. ✓

decodeZSetLegacyBlobValue: rejects payloads shorter than 4 bytes, rejects mismatched magic, rejects NaN scores — all fail-closed at intake. ✓

zsetState() + claimPendingTTL(): HandleZSetLegacyBlob calls r.zsetState(userKey) which calls claimPendingTTL on first registration. This means the pendingTTL drain works for legacy blobs as well, regardless of scan order. ✓

Medium — Incorrect scan-order claim; wrong test order for legacy-blob TTL

HandleZSetLegacyBlob's docstring (lines 202–210 in the current file) states:

!redis|zset| ALSO sorts BEFORE !redis|ttl|, so an inline TTL on the same user key will reach HandleTTL after this handler has already registered redisKindZSet.

This is incorrect. Comparing at the first diverging byte (|t... vs |z...): t = 0x74 < z = 0x7A, so !redis|ttl|k sorts before !redis|zset|k in Pebble's lexicographic order. The real snapshot sequence for a TTL'd legacy zset is:

!redis|ttl|k    → HandleTTL → kindByKey["k"] == redisKindUnknown → pendingTTL["k"] = ms
!redis|zset|k   → HandleZSetLegacyBlob → zsetState() → claimPendingTTL() → st.expireAtMs = ms

The pendingTTL mechanism handles this correctly (since zsetState() always calls claimPendingTTL()), so the behavior is correct — but the comment documents the wrong ordering and the wrong code path.

TestRedisDB_ZSetLegacyBlobWithInlineTTL tests blob-first-then-TTL, which is the non-real-world order (taking the case redisKindZSet: shortcut, not the pendingTTL path). The real-world TTL-first ordering for legacy blobs is not tested.

Two fixes needed:

1. Correct the docstring: !redis|ttl| sorts before !redis|zset| so the TTL arrives first; the pendingTTL drain in zsetState() handles it.
2. Add TestRedisDB_ZSetLegacyBlobTTLArrivesBeforeBlob that calls HandleTTL first then HandleZSetLegacyBlob, and asserts expire_at_ms is inlined. This pins the real-world path through pendingTTL.

Fix these →

Low — TestRedisDB_ZSetLegacyBlobRejectsMalformedKey has contradictory comments and tests the wrong boundary

The test docstring says "key with no trailing user-key bytes — parser must still accept it (empty user key is technically valid Redis)" and then the body comment says "Use a key that doesn't have the prefix to trigger the parse failure." The test uses []byte("not-the-right-prefix|k") — a key that doesn't have the !redis|zset| prefix at all.

Inspecting parseZSetLegacyBlobKey: it returns (rest, true) whenever TrimPrefix removes anything, including when rest is empty (prefix-only key). So HandleZSetLegacyBlob([]byte("!redis|zset|"), value) succeeds with an empty user key. The test never touches this boundary.

This isn't a functional bug — the test does exercise the prefix-missing error path — but the test name (RejectsMalformedKey) and first comment are misleading. Better: rename to TestRedisDB_ZSetLegacyBlobRejectsMissingPrefix and remove the contradictory "no trailing user-key bytes" comment.

Outstanding from prior reviews (still unaddressed)

Both prior Claude reviews flagged these. They remain open:

Low — TestRedisDB_ZSetBinaryMemberUsesBase64Envelope (redis_zset_test.go:346): only checks envelope["base64"] == "" (non-empty presence). A wrong base64 payload still passes. Fully decode and compare against []byte{0x80, 0xff, 0x01}:

payload, ok := envelope["base64"].(string)
if !ok || payload == "" { t.Fatalf(...) }
decoded, err := base64.StdEncoding.DecodeString(payload)
if err != nil { t.Fatalf("invalid base64: %v", err) }
if !bytes.Equal(decoded, []byte{0x80, 0xff, 0x01}) {
    t.Fatalf("decoded member = %v, want [0x80 0xff 0x01]", decoded)
}

Nit — Five //nolint:gosec suppressions remain in test helpers (encodeZSetMetaValue, zsetMetaKey, zsetMemberKey, zsetScoreKey, zsetMetaDeltaKey). Per CLAUDE.md: "Avoid //nolint — refactor instead." Adding a bounds panic before the cast makes the conversion obviously safe and removes the suppressions.

Nit — []byte(RedisZSetMetaDeltaPrefix), []byte(RedisZSetMetaPrefix), []byte(RedisZSetMemberPrefix) are allocated per dispatched record in parseZSetMetaKey, parseZSetMemberKey, and HandleZSetMeta. Hoisting these to package-level var (as CodeRabbit suggested) eliminates the per-record overhead.

Confirmed correct in commit 3

• Magic prefix {0x00, 'R', 'Z', 0x01} matches the live storedRedisZSetProtoPrefix exactly ✓
• decodeZSetLegacyBlobValue rejects payloads shorter than 4 bytes (empty protobuf body is 0 bytes; 0 < 4 → error) ✓
• NaN fail-closed in decodeZSetLegacyBlobValue matches HandleZSetMember's contract ✓
• Mid-migration merge (!redis|zset| arrives before !zs|...): ordering claim and test are correct ✓
• zsetState() → claimPendingTTL() chain means pendingTTL works for legacy blobs even though the comment misstates the reason ✓
• encodeZSetLegacyBlobValue test helper mirrors adapter/redis_storage_codec.go::marshalZSetValue exactly ✓
• zsetLegacyBlobKey mirrors adapter/redis_compat_types.go:177 ZSetKey exactly ✓
• 6 new legacy-blob tests cover round-trip, mid-migration merge, NaN rejection, missing-magic rejection ✓

Summary

The Codex P1 data-loss fix is architecturally sound: HandleZSetLegacyBlob decodes the protobuf blob, registers the same per-key state HandleZSetMeta + HandleZSetMember would, and the zsetState() → claimPendingTTL() chain means TTL inlining works via pendingTTL for the real snapshot order. The one material issue is that the code comments state the wrong scan order for !redis|ttl| vs !redis|zset|, and the TTL test exercises the non-real-world path (blob-first), leaving the actual pendingTTL path for legacy blobs untested. The base64 shallow assertion and //nolint:gosec suppressions carry over from prior reviews.

@claude review

Claude finished @bootjp's task in 4m 12s —— View job

Code Review: PR #790 — Redis zset encoder (Phase 0a) — Round 4

Tasks

• Gather context from PR description and prior reviews
• Read internal/backup/redis_zset.go (all 4 commits)
• Read internal/backup/redis_zset_test.go
• Read internal/backup/redis_string.go and redis_set.go
• Cross-check scan ordering and pendingTTL logic
• Check all outstanding issues from Round 3
• Analyze commit 4 (ab88f3e): wide-column rows supersede legacy blob
• Post comprehensive review

Commit 4 (ab88f3e) — Wide-column supersedes legacy blob

This addresses the Codex P1 round-3 finding correctly. The sawWide flag + markZSetWide() mechanism mirrors the live read path (adapter/redis_compat_helpers.go:610-620) precisely:

markZSetWide() logic traced across all orderings:

Key invariants confirmed:

• markZSetWide() is called from both HandleZSetMeta (line 160) and HandleZSetMember (line 194), covering every wide-column entry point ✓
• HandleZSetScore and HandleZSetMetaDelta are no-ops; neither creates state nor sets sawWide — correct, these are secondary-index / delta records ✓
• Three new tests (TestRedisDB_ZSetWideColumnSupersedesLegacyBlob, TestRedisDB_ZSetMetaAloneEvictsLegacyBlob, TestRedisDB_ZSetLegacyBlobAfterWideRowsIsDropped) cover all key orderings ✓
• The markZSetWide in-place map-clear (for k := range st.members { delete(st.members, k) }) avoids changing the pointer — consistent with the nil-check-free pattern used throughout ✓

No new issues found in commit 4.

Issues — Remaining from prior rounds (none addressed in commit 4)

Medium — HandleZSetLegacyBlob docstring states the wrong scan order for TTL (lines 252–257)

The comment claims:

!redis|zset| ALSO sorts BEFORE !redis|ttl|, so an inline TTL … will reach HandleTTL after this handler has already registered redisKindZSet.

This is incorrect. Comparing at the first diverging byte after the shared !redis| prefix: t = 0x74 < z = 0x7A, so !redis|ttl| sorts BEFORE !redis|zset|. The real sequence for a TTL'd legacy zset is:

!redis|ttl|k   → HandleTTL → redisKindUnknown → pendingTTL["k"] = ms
!redis|zset|k  → HandleZSetLegacyBlob → zsetState() → claimPendingTTL() → st.expireAtMs = ms

The code is correct — the zsetState() → claimPendingTTL() chain handles this. Only the comment documents the wrong path. The first paragraph at line 246–250 also incorrectly says !redis|zset| sorts before !redis|ttl|.

Fix this →

Low — TestRedisDB_ZSetLegacyBlobWithInlineTTL tests the non-real-world scan order

TestRedisDB_ZSetLegacyBlobWithInlineTTL (line 783) calls HandleZSetLegacyBlob first, then HandleTTL. In a real Pebble scan the opposite is true (!redis|ttl| < !redis|zset| as shown above), so the test exercises the HandleTTL → redisKindZSet → zsetState(k) shortcut path — not the actual pendingTTL → claimPendingTTL() path that fires in production.

A companion test is needed:

func TestRedisDB_ZSetLegacyBlobTTLArrivesBeforeBlob(t *testing.T) {
    db, root := newRedisDB(t)
    // Real-world order: TTL arrives first (!redis|ttl| < !redis|zset|).
    if err := db.HandleTTL([]byte("k"), encodeTTLValue(fixedExpireMs)); err != nil {
        t.Fatal(err)
    }
    value := encodeZSetLegacyBlobValue(t, []zsetLegacyEntry{{member: "m", score: 1}})
    if err := db.HandleZSetLegacyBlob(zsetLegacyBlobKey("k"), value); err != nil {
        t.Fatal(err)
    }
    if err := db.Finalize(); err != nil { t.Fatal(err) }
    got := readZSetJSON(t, filepath.Join(root, "redis", "db_0", "zsets", "k.json"))
    if zsetFloat(t, got, "expire_at_ms") != float64(fixedExpireMs) {
        t.Fatalf("expire_at_ms = %v want %d — pendingTTL path for legacy blob failed",
            got["expire_at_ms"], fixedExpireMs)
    }
}

Fix this →

Low — Base64 assertion is non-functional (redis_zset_test.go:346-348, carried from rounds 2–3)

if envelope["base64"] == "" {
    t.Fatalf("base64 envelope missing payload: %v", envelope)
}

envelope["base64"] is a any typed map value; comparing it to "" compares the interface value to a string. This check passes even if the payload is corrupt. Fully decode and compare:

payload, ok := envelope["base64"].(string)
if !ok || payload == "" {
    t.Fatalf("base64 envelope missing payload: %v", envelope)
}
decoded, err := base64.StdEncoding.DecodeString(payload)
if err != nil {
    t.Fatalf("invalid base64 payload %q: %v", payload, err)
}
if !bytes.Equal(decoded, []byte{0x80, 0xff, 0x01}) {
    t.Fatalf("decoded member = %v, want [0x80 0xff 0x01]", decoded)
}

Fix this →

Nit — //nolint:gosec suppressions in test helpers (lines 50, 67, 77, 90, 102; carried from rounds 2–3)

Per CLAUDE.md: Avoid //nolint — refactor instead. The five integer-to-uint conversions in encodeZSetMetaValue, zsetMetaKey, zsetMemberKey, zsetScoreKey, and zsetMetaDeltaKey can be made obviously safe by adding an explicit bounds panic before the cast. CodeRabbit's proposed putUserKeyLen helper applies here.

Nit — TestRedisDB_ZSetLegacyBlobRejectsMalformedKey has contradictory comments (line 838–841; carried from round 3)

The test comment block says "Key has the prefix but no trailing user-key bytes — parser must still accept it (empty user key is technically valid Redis)" but the actual test passes a key that lacks the !redis|zset| prefix entirely ("not-the-right-prefix|k"). The test exercises the missing-prefix path, not the empty-user-key path. Rename to TestRedisDB_ZSetLegacyBlobRejectsMissingPrefix and remove the contradictory comment.

Confirmed correct across all 4 commits

• sawWide mechanism mirrors adapter/redis_compat_helpers.go:610-620 exactly: wide-column rows are authoritative, legacy blob is a fallback consulted only when no wide rows exist ✓
• markZSetWide is guarded by if st.sawWide { return } making it idempotent across multiple wide member records ✓
• redisZSetLegacyProtoPrefix = []byte{0x00, 'R', 'Z', 0x01} matches adapter/redis_storage_codec.go:15 storedRedisZSetProtoPrefix ✓
• !redis|zset| < !zs|... ordering is correct for the mid-migration merge case (at the first diverging byte: r=0x72 < z=0x7A) ✓
• NaN fail-closed in decodeZSetLegacyBlobValue matches HandleZSetMember contract ✓
• pendingTTL drain in zsetState() correctly handles the legacy-blob+TTL scenario at the code level (comment is wrong, code is right) ✓
• encodeZSetLegacyBlobValue and zsetLegacyBlobKey test helpers match the live encoder/key builder exactly ✓
• All six legacy-blob tests in commit 3 + three new wide-supersedes-legacy tests in commit 4 are well-constructed ✓

Summary

Commit 4 is architecturally correct: sawWide + markZSetWide() faithfully replicates the live read path's source-of-truth preference, and the three new tests pin all ordering variants. The one functional gap is that the real-world scan order for TTL'd legacy blobs (TTL arrives first, blob second) is not exercised by a test — the existing TestRedisDB_ZSetLegacyBlobWithInlineTTL tests the synthetic reverse order. The docstring at lines 252–257 documents the wrong path for the same reason. Both can be fixed together with a comment correction and a single new test.

@claude review

Claude finished @bootjp's task in 3m 5s —— View job

Code Review: PR #790 — Redis zset encoder (Phase 0a) — Round 5

Tasks

• Gather context from PR description and prior reviews (rounds 1–4)
• Read internal/backup/redis_zset.go (all 5 commits)
• Read internal/backup/redis_zset_test.go
• Read internal/backup/redis_string.go
• Review commit 5 (174456a): bound pendingTTL to prevent OOM
• Check all outstanding issues from Round 4
• Post comprehensive review

Commit 5 (174456a) — pendingTTL OOM bound

The fix is architecturally correct and complete:

• defaultPendingTTLCap = 1 << 20 (~64 MiB at ~64 B/entry) with clear documentation in the constant's comment ✓
• WithPendingTTLCap(0) disables buffering entirely (reverts to pre-pendingTTL immediate-orphan behavior); negative coerced to 0 ✓
• parkUnknownTTL caps at pendingTTLCap entries; overflow increments both pendingTTLOverflow and orphanTTLCount immediately ✓
• Finalize surfaces pending_ttl_buffer_overflow and pending_ttl_buffer_cap in the warning so operators can distinguish "exceeded cap" from "truly unmatched TTLs" ✓
• Three tests (TestRedisDB_PendingTTLBoundedByCap, TestRedisDB_WithPendingTTLCapZeroDisablesBuffering, TestRedisDB_WithPendingTTLCapNegativeCoercedToZero) correctly pin all behavioral variants ✓

TestRedisDB_PendingTTLBoundedByCap arithmetic verified:
cap=8, 16 writes → intake: len(pendingTTL)=8, orphanTTLCount=8, pendingTTLOverflow=8 → after Finalize: orphanTTLCount=16. Correct.

Low — parkUnknownTTL: duplicate-key at-cap edge case

When the buffer is exactly at pendingTTLCap and a TTL record arrives for a key already in pendingTTL, the code takes the overflow branch (incrementing both pendingTTLOverflow and orphanTTLCount) rather than updating the existing entry. If the key is later claimed by a typed record, orphanTTLCount will have been incremented by 1 for a key that was actually buffered — a phantom orphan.

In practice Pebble snapshots don't emit duplicate !redis|ttl| records for the same key, so this is effectively unreachable today. But a one-line pre-check makes the edge case explicit and prevents overcounting in a corrupt snapshot:

func (r *RedisDB) parkUnknownTTL(userKey []byte, expireAtMs uint64) {
    uk := string(userKey)
    if _, alreadyBuffered := r.pendingTTL[uk]; alreadyBuffered {
        r.pendingTTL[uk] = expireAtMs // latest-wins update; no size change
        return
    }
    if len(r.pendingTTL) >= r.pendingTTLCap {
        r.pendingTTLOverflow++
        r.orphanTTLCount++
        return
    }
    r.pendingTTL[uk] = expireAtMs
}

Fix this →

Issues — Carried from prior rounds (still unaddressed in commit 5)

Medium — HandleZSetLegacyBlob docstring states wrong scan order for TTL

redis_zset.go:246–257 says:

!redis|zset| ALSO sorts BEFORE !redis|ttl| …

This is incorrect. Comparing the first diverging byte after the shared !redis| prefix: t = 0x74 < z = 0x7A, so !redis|ttl| sorts before !redis|zset|. The real-world scan sequence for a TTL'd legacy zset is:

!redis|ttl|k   → HandleTTL → redisKindUnknown → pendingTTL["k"] = ms
!redis|zset|k  → HandleZSetLegacyBlob → zsetState() → claimPendingTTL() → st.expireAtMs = ms

The code is correct — the zsetState() → claimPendingTTL() chain handles this properly. But the docstring at lines 246–250 also gets the mid-migration order claim half-wrong (it says !redis|zset| sorts before !redis|ttl|; the !redis|zset| < !zs|... part at line 246 is correct).

Fix this →

Low — TestRedisDB_ZSetLegacyBlobWithInlineTTL tests non-real-world scan order

The test docstring (line 779) repeats the wrong claim (!redis|zset| sorts before !redis|ttl|) and the test exercises the blob-first → TTL-second path (i.e., the HandleTTL redisKindZSet fast-path). The real-world ordering is TTL-first → blob-second (the pendingTTL path) — which is currently untested for legacy blobs.

A companion test is still needed:

func TestRedisDB_ZSetLegacyBlobTTLArrivesBeforeBlob(t *testing.T) {
    t.Parallel()
    db, root := newRedisDB(t)
    // Real-world order: !redis|ttl|k sorts before !redis|zset|k (t < z).
    if err := db.HandleTTL([]byte("k"), encodeTTLValue(fixedExpireMs)); err != nil {
        t.Fatal(err)
    }
    value := encodeZSetLegacyBlobValue(t, []zsetLegacyEntry{{member: "m", score: 1}})
    if err := db.HandleZSetLegacyBlob(zsetLegacyBlobKey("k"), value); err != nil {
        t.Fatal(err)
    }
    if err := db.Finalize(); err != nil { t.Fatal(err) }
    got := readZSetJSON(t, filepath.Join(root, "redis", "db_0", "zsets", "k.json"))
    if zsetFloat(t, got, "expire_at_ms") != float64(fixedExpireMs) {
        t.Fatalf("expire_at_ms = %v want %d — pendingTTL path for legacy blob failed",
            got["expire_at_ms"], fixedExpireMs)
    }
}

Fix this →

Low — Base64 assertion is non-functional (redis_zset_test.go:346–348)

envelope["base64"] == "" compares an any-typed map value to the string "". A wrong-but-non-empty base64 payload still passes. Need a full round-trip decode (unchanged from Rounds 2–4):

payload, ok := envelope["base64"].(string)
if !ok || payload == "" {
    t.Fatalf("base64 envelope missing payload: %v", envelope)
}
decoded, err := base64.StdEncoding.DecodeString(payload)
if err != nil {
    t.Fatalf("invalid base64 payload %q: %v", payload, err)
}
if !bytes.Equal(decoded, []byte{0x80, 0xff, 0x01}) {
    t.Fatalf("decoded member = %v, want [0x80 0xff 0x01]", decoded)
}

Fix this →

Nit — //nolint:gosec in test helpers (lines 50, 67, 77, 90, 102)

Per CLAUDE.md: "Avoid //nolint — refactor instead." The five integer conversions in encodeZSetMetaValue, zsetMetaKey, zsetMemberKey, zsetScoreKey, and zsetMetaDeltaKey can be made obviously safe with a bounds panic (or putUserKeyLen helper as CodeRabbit suggested), eliminating the suppressions.

Nit — TestRedisDB_ZSetLegacyBlobRejectsMalformedKey has contradictory comments

The outer docstring (line 831) says "key with no trailing user-key bytes fails parse" but the inner comment (lines 837–840) says "empty user key is technically valid Redis … use a key that doesn't have the prefix." The test actually exercises the missing-prefix case, not an empty-user-key case. Rename to TestRedisDB_ZSetLegacyBlobRejectsMissingPrefix and remove the contradictory comment.

Confirmed correct across all 5 commits

• pendingTTLCap default (1M entries, ~64 MiB) is documented and correctly applied ✓
• WithPendingTTLCap(0) disables buffering; negative coercion to 0 is clean ✓
• Finalize double-counts nothing: overflow path increments orphanTTLCount at intake; orphanTTLCount += len(pendingTTL) at Finalize adds only the buffered-but-unclaimed entries ✓
• sawWide + markZSetWide() faithfully mirrors adapter/redis_compat_helpers.go:610-620 ✓
• redisZSetLegacyProtoPrefix matches adapter/redis_storage_codec.go:15 ✓
• NaN fail-closed in both HandleZSetMember and decodeZSetLegacyBlobValue ✓
• !redis|zset| < !zs|... ordering correct (mid-migration merge case) ✓
• zsetState() → claimPendingTTL() chain correctly handles TTL-before-zset ordering at the code level ✓

Summary

Commit 5's pendingTTL OOM bound is correct: the 1 << 20 cap, the WithPendingTTLCap override, and the three new tests are all well-constructed. The one behavioral gap is the duplicate-key-at-cap edge case in parkUnknownTTL (phantom orphan count on corrupt input, not reachable from a healthy Pebble snapshot).

From prior rounds, the docstring scan-order error (!redis|zset| sorts after, not before, !redis|ttl|) and the missing TestRedisDB_ZSetLegacyBlobTTLArrivesBeforeBlob test remain open — the code handles the real-world TTL-first ordering correctly via pendingTTL, but the comment documents the wrong path and the test exercises the non-production order.

@claude review

Claude finished @bootjp's task in 4m 11s —— View job

Code Review: PR #790 — Redis zset encoder (Phase 0a) — Round 6

Tasks

• Gather context from PR description and prior reviews (rounds 1–5)
• Read commit 6 (800cc8c): fail closed on pendingTTL overflow
• Read internal/backup/redis_string.go (parkUnknownTTL changes)
• Read internal/backup/redis_zset.go (docstring / scan-order)
• Read internal/backup/redis_zset_test.go (new tests, base64 assertion)
• Check all outstanding issues from Round 5
• Post comprehensive review

Commit 6 (800cc8c) — fail-closed on pendingTTL overflow

The Codex P1 finding from Round 5 is correctly addressed. The three-mode design is sound:

Key correctness properties verified:

• ErrPendingTTLBufferFull is well-documented (lines 75–91) with recovery guidance pointing to WithPendingTTLCap.
• pendingTTLOverflow is still tracked so Finalize's redis_orphan_ttl warning can surface both the count and the cap context (pending_ttl_buffer_overflow / pending_ttl_buffer_cap fields), letting operators distinguish corruption from an undersized cap.
• HandleTTL's redisKindUnknown case propagates parkUnknownTTL's error to the caller. All test call sites already check HandleTTL errors.
• TestRedisDB_PendingTTLFailsClosedAtCap: correctly asserts orphanTTLCount == 0 at intake (caller must handle the error, not Finalize).
• TestRedisDB_WithPendingTTLCapZeroOpts: counter-only mode, no error, immediate increment.
• TestRedisDB_WithPendingTTLCapNegativeCoercedToZero: input-sanitisation guard.

Issues

Low — parkUnknownTTL: false error on duplicate-key-at-cap (carried from Round 5, new shape)

The Round-5 concern changes shape under the fail-closed semantics. When the buffer is exactly at cap and a TTL record arrives for a key already in pendingTTL:

if len(r.pendingTTL) >= r.pendingTTLCap {  // true — key exists, len == cap
    r.pendingTTLOverflow++
    return ErrPendingTTLBufferFull          // ← wrong: key IS already buffered
}

The key is in the buffer (with the previous expiry) and will be claimed by claimPendingTTL when the typed record arrives. But the caller sees ErrPendingTTLBufferFull — a false error that may cause the snapshot decoder to abort the decode for a key that was actually handled correctly.

A healthy Pebble snapshot never emits duplicate !redis|ttl| records for the same key, so this only triggers on corrupt snapshots. Still, returning an error for something that succeeded is a correctness violation, and the fix is a one-liner:

func (r *RedisDB) parkUnknownTTL(userKey []byte, expireAtMs uint64) error {
    if r.pendingTTLCap == 0 {
        r.orphanTTLCount++
        return nil
    }
    uk := string(userKey)
    if _, alreadyBuffered := r.pendingTTL[uk]; alreadyBuffered {
        r.pendingTTL[uk] = expireAtMs // latest-wins; no size change
        return nil
    }
    if len(r.pendingTTL) >= r.pendingTTLCap {
        r.pendingTTLOverflow++
        return cockroachdberr.Wrapf(ErrPendingTTLBufferFull, ...)
    }
    r.pendingTTL[uk] = expireAtMs
    return nil
}

Fix this →

Medium — HandleZSetLegacyBlob docstring states wrong scan order for !redis|ttl| (carried from Round 3)

Lines 246–257 state (twice) that !redis|zset| sorts before !redis|ttl|. This is incorrect. Comparing the first diverging byte after the shared !redis| prefix: t = 0x74 < z = 0x7A, so !redis|ttl|k sorts before !redis|zset|k. The real snapshot sequence for a TTL'd legacy zset is:

!redis|ttl|k   → HandleTTL → redisKindUnknown → pendingTTL["k"] = ms
!redis|zset|k  → HandleZSetLegacyBlob → zsetState() → claimPendingTTL() → st.expireAtMs = ms

The code is correct — zsetState() → claimPendingTTL() handles this. But the comment documents the wrong path and creates false confidence that the redisKindZSet HandleTTL shortcut is what fires in production (it isn't — that only fires if a wide-column row arrived before the TTL, which would also be non-standard Pebble order).

Line 248 also gets the !redis|zset| < !zs|... ordering correct — only the !redis|ttl| ordering claim is wrong.

The TestRedisDB_ZSetLegacyBlobWithInlineTTL docstring (line 779–782) makes the same wrong claim, saying !redis|zset|<k> "sorts before !redis|ttl|."

Fix this →

Low — Missing test for real-world TTL-first ordering on legacy blobs (carried from Round 3)

TestRedisDB_ZSetLegacyBlobWithInlineTTL exercises blob-first-then-TTL (the redisKindZSet HandleTTL fast path). The actual Pebble ordering — !redis|ttl|k arrives before !redis|zset|k — goes through pendingTTL → claimPendingTTL(), which is untested for legacy blobs. A companion test would pin the real path:

func TestRedisDB_ZSetLegacyBlobTTLArrivesBeforeBlob(t *testing.T) {
    t.Parallel()
    db, root := newRedisDB(t)
    // Real-world Pebble order: !redis|ttl|k < !redis|zset|k (t < z).
    if err := db.HandleTTL([]byte("k"), encodeTTLValue(fixedExpireMs)); err != nil {
        t.Fatal(err)
    }
    value := encodeZSetLegacyBlobValue(t, []zsetLegacyEntry{{member: "m", score: 1}})
    if err := db.HandleZSetLegacyBlob(zsetLegacyBlobKey("k"), value); err != nil {
        t.Fatal(err)
    }
    if err := db.Finalize(); err != nil { t.Fatal(err) }
    got := readZSetJSON(t, filepath.Join(root, "redis", "db_0", "zsets", "k.json"))
    if zsetFloat(t, got, "expire_at_ms") != float64(fixedExpireMs) {
        t.Fatalf("expire_at_ms = %v want %d — pendingTTL path for legacy blob failed",
            got["expire_at_ms"], fixedExpireMs)
    }
}

Fix this →

Low — Base64 assertion is non-functional (redis_zset_test.go:346–348, carried from Round 2)

if envelope["base64"] == "" {

envelope["base64"] is typed as any. Comparing an any to a string literal "" with == compares the interface value to the string — this evaluates to false whenever the map value is non-nil (even if the string payload is "garbage"), so a wrong base64 payload still passes. The assertion should be:

payload, ok := envelope["base64"].(string)
if !ok || payload == "" {
    t.Fatalf("base64 envelope missing payload: %v", envelope)
}
decoded, err := base64.StdEncoding.DecodeString(payload)
if err != nil {
    t.Fatalf("invalid base64 payload %q: %v", payload, err)
}
if !bytes.Equal(decoded, []byte{0x80, 0xff, 0x01}) {
    t.Fatalf("decoded member = %v, want [0x80 0xff 0x01]", decoded)
}

Add "bytes" and "encoding/base64" to the import block.

Fix this →

Nits (carried from prior rounds, still unaddressed)

//nolint:gosec suppressions in test helpers (lines 50, 67, 77, 90, 102) — five integer-to-uint casts in encodeZSetMetaValue, zsetMetaKey, zsetMemberKey, zsetScoreKey, zsetMetaDeltaKey. Per CLAUDE.md: "Avoid //nolint — refactor instead." A putUserKeyLen helper (as CodeRabbit proposed) or a bounds-panic before each cast removes all five suppressions cleanly.

TestRedisDB_ZSetLegacyBlobRejectsMalformedKey contradictory comment (lines 837–840) — the comment block says "key with no trailing user-key bytes — parser must still accept it (empty user key is technically valid Redis)" then immediately says "Use a key that doesn't have the prefix to trigger the parse failure." These contradict each other. Rename to TestRedisDB_ZSetLegacyBlobRejectsMissingPrefix and remove the contradictory first sentence.

Per-record []byte() allocations — []byte(RedisZSetMetaDeltaPrefix) / []byte(RedisZSetMetaPrefix) / []byte(RedisZSetMemberPrefix) at lines 132, 361, 364, 376 create a new allocation on every dispatched record. Hoisting to package-level var eliminates ~4 allocations per record in the snapshot hot path.

Confirmed correct in commit 6

• Three-mode parkUnknownTTL is correct: cap==0 (counter-only), not-full (buffer), full (fail-closed) ✓
• pendingTTLOverflow incremented before the error return so Finalize warning carries the count even if the caller swallows ErrPendingTTLBufferFull ✓
• TestRedisDB_PendingTTLFailsClosedAtCap correctly asserts orphanTTLCount == 0 at intake ✓
• TestRedisDB_WithPendingTTLCapZeroOpts correctly asserts no error, immediate orphan count ✓
• Rename from *_BoundedByCap / *_DisablesBuffering → *_FailsClosedAtCap / *_CapZeroOpts is accurate ✓
• All HandleTTL call sites already check the returned error ✓

Summary

Commit 6's fail-closed semantics are architecturally correct. The remaining open items — all carried since earlier rounds — are: a false error in parkUnknownTTL when a duplicate TTL arrives at cap (the entry IS buffered; returning an error is wrong), the wrong scan-order claim in the HandleZSetLegacyBlob docstring and test comment, the missing real-world TTL-first ordering test for legacy blobs, and the non-functional base64 assertion. The //nolint:gosec suppressions and contradictory test comment are style nits.

@claude review

Claude finished @bootjp's task in 3m 44s —— View job

Code Review: PR #790 — Redis zset encoder (Phase 0a) — Round 7

Tasks

• Gather context from PR description and prior reviews (rounds 1–6)
• Read internal/backup/redis_string.go (commit 7: byte-bound pendingTTL)
• Read internal/backup/redis_zset.go (docstring, outstanding items)
• Read internal/backup/redis_zset_test.go (base64 assertion, legacy TTL test)
• Read internal/backup/redis_string_test.go (new byte-cap tests)
• Check all outstanding issues from Round 6
• Post review

Commit 7 (8eca5cd) — byte-budget pendingTTL

Architecturally correct and well-tested. Key properties verified:

• pendingTTLBytes tracks cumulative byte cost; pendingTTLBytesCap (default 64 MiB = 64 << 20) is the budget
• parkUnknownTTL:478 charges cost := len(userKey) + pendingTTLEntryOverheadBytes (8 bytes for the uint64 payload) — bounds the map regardless of key size, fixing the entry-count-cap OOM bypass on 1 MiB keys
• claimPendingTTL:522 correctly decrements pendingTTLBytes -= len(uk) + pendingTTLEntryOverheadBytes, so the budget is reclaimed when an entry is drained
• Finalize:562 reports pending_ttl_buffer_bytes_cap (renamed from pending_ttl_buffer_cap) in the orphan warning ✓
• TestRedisDB_PendingTTLFailsClosedAtByteCap arithmetic verified: 8 "orphan-N" keys × 16 bytes each = 128 = byteCap; 9th entry fails closed ✓
• TestRedisDB_PendingTTLByteCapBoundedByLargeKey: 100-byte key + 8 overhead = 108 > 64-byte cap ✓
• TestRedisDB_PendingTTLByteBudgetReclaimedOnClaim: drain k0 frees 10 bytes → subsequent entry fits ✓

Issues

Low — parkUnknownTTL: false error on duplicate-key-at-cap (carried from R5/R6)

When the byte budget is exactly at capacity and a TTL record arrives for a key already in pendingTTL, the code reaches the overflow branch:

// redis_string.go:479
if r.pendingTTLBytes+cost > r.pendingTTLBytesCap {
    r.pendingTTLOverflow++
    return cockroachdberr.Wrapf(ErrPendingTTLBufferFull, ...)
}

Since the key is already in the map (pendingTTLBytes == cap and cost == prior entry's cost), this returns a false error — the key IS buffered and will be claimed correctly by claimPendingTTL. A healthy Pebble snapshot never emits duplicate !redis|ttl| records for the same key, so this is unreachable in practice. But returning an error for a successfully-handled key is a correctness violation that a one-line pre-check eliminates:

func (r *RedisDB) parkUnknownTTL(userKey []byte, expireAtMs uint64) error {
    if r.pendingTTLBytesCap == 0 {
        r.orphanTTLCount++
        return nil
    }
    uk := string(userKey)
    if _, alreadyBuffered := r.pendingTTL[uk]; alreadyBuffered {
        r.pendingTTL[uk] = expireAtMs // latest-wins; no byte change
        return nil
    }
    cost := len(userKey) + pendingTTLEntryOverheadBytes
    if r.pendingTTLBytes+cost > r.pendingTTLBytesCap {
        r.pendingTTLOverflow++
        return cockroachdberr.Wrapf(ErrPendingTTLBufferFull, ...)
    }
    r.pendingTTL[uk] = expireAtMs
    r.pendingTTLBytes += cost
    return nil
}

Fix this →

Medium — HandleZSetLegacyBlob docstring states wrong scan order for TTL (carried from R3/R4/R5/R6)

redis_zset.go:246–257 states twice that !redis|zset| sorts before !redis|ttl|:

// The legacy prefix `!redis|zset|` lex-sorts BEFORE `!zs|...` and
// BEFORE `!redis|ttl|`, …
//
// `!redis|zset|` ALSO sorts BEFORE `!redis|ttl|`, so an inline TTL …

This is incorrect. Comparing the first diverging byte after the shared !redis| prefix: t = 0x74 < z = 0x7A, so !redis|ttl|k sorts BEFORE !redis|zset|k. The real snapshot sequence for a TTL'd legacy zset is:

!redis|ttl|k   → HandleTTL → redisKindUnknown → pendingTTL["k"] = ms
!redis|zset|k  → HandleZSetLegacyBlob → zsetState() → claimPendingTTL() → st.expireAtMs = ms

The code is correct — zsetState() → claimPendingTTL() handles the real ordering. But the docstring documents the wrong path and creates false confidence that the redisKindZSet HandleTTL shortcut fires in production (it only fires when a wide-column row arrived before the TTL, which itself is non-standard Pebble order for zsets).

The !redis|zset| sorts before !zs|... claim at line 246 is correct (r < z); only the !redis|ttl| ordering claim is wrong.

Fix this →

Low — Missing test for real-world TTL-first ordering on legacy blobs (carried from R3/R4/R5/R6)

TestRedisDB_ZSetLegacyBlobWithInlineTTL exercises blob-first → TTL-second (the redisKindZSet HandleTTL shortcut). In a real Pebble scan !redis|ttl|k arrives before !redis|zset|k (since t < z), which goes through the pendingTTL → claimPendingTTL() path. That path is exercised by TestRedisDB_ZSetTTLArrivesBeforeRows for wide-column records but not for legacy blobs. A companion test:

func TestRedisDB_ZSetLegacyBlobTTLArrivesBeforeBlob(t *testing.T) {
    t.Parallel()
    db, root := newRedisDB(t)
    // Real Pebble order: !redis|ttl|k < !redis|zset|k (t < z).
    if err := db.HandleTTL([]byte("k"), encodeTTLValue(fixedExpireMs)); err != nil {
        t.Fatal(err)
    }
    value := encodeZSetLegacyBlobValue(t, []zsetLegacyEntry{{member: "m", score: 1}})
    if err := db.HandleZSetLegacyBlob(zsetLegacyBlobKey("k"), value); err != nil {
        t.Fatal(err)
    }
    if err := db.Finalize(); err != nil { t.Fatal(err) }
    got := readZSetJSON(t, filepath.Join(root, "redis", "db_0", "zsets", "k.json"))
    if zsetFloat(t, got, "expire_at_ms") != float64(fixedExpireMs) {
        t.Fatalf("expire_at_ms = %v want %d — pendingTTL path for legacy blob failed",
            got["expire_at_ms"], fixedExpireMs)
    }
}

Fix this →

Low — Base64 assertion is non-functional (redis_zset_test.go:346, carried from R2/R3/R4/R5/R6)

if envelope["base64"] == "" {
    t.Fatalf("base64 envelope missing payload: %v", envelope)
}

envelope["base64"] is typed as any. The == comparison against the string "" compares the interface directly to a string literal — this evaluates to false (not equal) whenever the map value is a non-nil string, so a corrupt base64 payload still passes. A full round-trip decode:

payload, ok := envelope["base64"].(string)
if !ok || payload == "" {
    t.Fatalf("base64 envelope missing payload: %v", envelope)
}
decoded, err := base64.StdEncoding.DecodeString(payload)
if err != nil {
    t.Fatalf("invalid base64 payload %q: %v", payload, err)
}
if !bytes.Equal(decoded, []byte{0x80, 0xff, 0x01}) {
    t.Fatalf("decoded member = %v, want [0x80 0xff 0x01]", decoded)
}

Add "bytes" and "encoding/base64" to the import block.

Fix this →

Nits (carried from prior rounds, still unaddressed)

//nolint:gosec suppressions (lines 50, 67, 77, 90, 102 in redis_zset_test.go) — per CLAUDE.md: "Avoid //nolint — refactor instead." The five uint32(len(userKey)) casts in encodeZSetMetaValue, zsetMetaKey, zsetMemberKey, zsetScoreKey, zsetMetaDeltaKey can be made obviously safe with a bounds panic (or a putUserKeyLen helper as CodeRabbit proposed), eliminating all five suppressions.

TestRedisDB_ZSetLegacyBlobRejectsMalformedKey contradictory comments (lines 837–840) — the test outer docstring says "key with no trailing user-key bytes fails parse" but the inner comment says "empty user key is technically valid Redis — use a key without the prefix to trigger failure." The test uses "not-the-right-prefix|k" (no prefix match), not an empty user-key. Rename to TestRedisDB_ZSetLegacyBlobRejectsMissingPrefix and remove the contradictory sentence.

Per-record []byte() allocations — []byte(RedisZSetMetaDeltaPrefix), []byte(RedisZSetMetaPrefix), []byte(RedisZSetMemberPrefix) at lines 132, 361, 364, 376 create one allocation per dispatched record in the snapshot hot path. Package-level var hoists (as CodeRabbit proposed) eliminate all four.

Confirmed correct across all 7 commits

• Byte-budget accounting is exact: parkUnknownTTL charges and claimPendingTTL refunds the same len(key) + 8 cost ✓
• pendingTTLBytes drains to 0 after Finalize processes all typed records (verified by TestRedisDB_PendingTTLByteBudgetReclaimedOnClaim) ✓
• WithPendingTTLByteCap(0) → counter-only, no error; negative → coerced to 0 ✓
• Finalize warning carries pending_ttl_buffer_bytes_cap and pending_ttl_buffer_overflow when overflow occurred ✓
• sawWide + markZSetWide() mirror adapter/redis_compat_helpers.go:610-620 exactly ✓
• NaN fail-closed in both HandleZSetMember and decodeZSetLegacyBlobValue ✓
• redisZSetLegacyProtoPrefix = []byte{0x00, 'R', 'Z', 0x01} matches adapter/redis_storage_codec.go:15 ✓

Summary

Commit 7's byte-budget approach is architecturally correct and well-tested — it closes the OOM vector that an entry-count cap left open for snapshots with large keys. The five open issues all carry from prior rounds: the most impactful remaining gap is the wrong scan-order claim in HandleZSetLegacyBlob's docstring (and the matching test comment), plus the missing test for the real-world TTL-first ordering on legacy blobs. The duplicate-key-at-cap false error in parkUnknownTTL is unreachable in healthy snapshots but worth fixing for correctness. The base64 assertion and the //nolint nits are straightforward one-line fixes.