backup: Redis zset encoder (Phase 0a)#790
Conversation
Decodes !zs|meta|/!zs|mem|/!zs|scr|/!zs|meta|d| snapshot records into
the per-zset `zsets/<key>.json` shape defined by the Phase 0 design
(docs/design/2026_04_29_proposed_snapshot_logical_decoder.md line
334-335). Mirrors the hash/list/set encoder shape:
- !zs|meta| → 8-byte BE declared member count
- !zs|mem|<m> → member m with 8-byte IEEE 754 score
- !zs|scr| → secondary score index; silently discarded
(!zs|mem| is source of truth at backup time)
- !zs|meta|d| → delta records; silently skipped (set/hash policy)
Output is sorted by raw member-name bytes so `diff -r` of two dumps
with the same logical contents but mutated scores stays line-stable.
Scores serialize as JSON numbers for finite values and the
ZADD-conventional `"+inf"`/`"-inf"` strings for non-finite ones
(json.Marshal rejects ±Inf so the score field uses json.RawMessage).
NaN scores fail closed at HandleZSetMember: Redis's ZADD rejects NaN
at the wire level, so a NaN in storage indicates corruption that we
refuse to silently propagate into the dump.
TTL routing: !redis|ttl|<userKey> for a registered zset key folds
into the zset JSON's expire_at_ms (matching the set/list/hash
inlining), so a restorer replays ZADD + EXPIRE in one shot without
chasing a separate sidecar.
Self-review:
1. Data loss — !zs|mem| is the source of truth; !zs|scr| and the
delta family are intentional silent skips with caller-audit notes.
2. Concurrency — RedisDB is sequential per scope (matches the
existing per-DB encoder contract); no shared state across DBs.
3. Performance — per-zset state buffered in a map; flushed at
Finalize. Bounded by maxWideColumnItems on the live side. Sort
is O(n log n) on member-name bytes; identical cost shape to
the hash/set encoders that shipped in #725/#758.
4. Consistency — JSON field-order determinism preserved (struct
tags, not map). Inf score uses string form via json.RawMessage
so the same `score` key accepts both shapes.
5. Coverage — 17 table-driven tests:
- round-trip basic / empty zset / TTL inlining
- binary member via base64 envelope
- delta-key skip (both HandleZSetMeta entry + parseZSetMetaKey guard)
- HandleZSetMetaDelta explicit entry point
- HandleZSetScore silent discard
- malformed meta length / overflow / MaxInt64 boundary
- malformed member-value length / NaN rejection
- ±Inf string-form serialization
- members-without-meta still emits file
- duplicate-members latest-wins (ZADD semantics)
Caller audit for semantics-changing edit (new `case redisKindZSet:`
in HandleTTL, redis_string.go:310): purely additive. The new branch
fires only when zsetState() has previously registered the key; no
existing handler maps to redisKindZSet, so no prior call site
changes behavior. Verified via
`grep -n 'redisKindZSet' internal/backup/`.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds complete Redis sorted-set (ZSET) backup support by introducing ZSET snapshot record handlers, per-key state accumulation with TTL inlining, and deterministic JSON emission. The implementation extends the existing RedisDB infrastructure with a new ChangesZSET Backup Feature
Sequence DiagramsequenceDiagram
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
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@claude review |
|
Claude finished @bootjp's task in 3m 31s —— View job Code Review: PR #790 — Redis zset encoder (Phase 0a)Tasks
Wire formatThe encoder constants and decoding arithmetic exactly mirror
IssuesMedium — TTL scan-order gap not tested (functional risk)
The same ordering issue exists for the set encoder (
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. Low —
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e323cc539
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case redisKindZSet: | ||
| // Same per-record TTL inlining: ZADD + EXPIRE replay in | ||
| // one shot from the per-zset JSON, no separate sidecar. | ||
| st := r.zsetState(userKey) | ||
| st.expireAtMs = expireAtMs | ||
| st.hasTTL = true |
There was a problem hiding this comment.
For expiring sorted sets in real snapshot order, this redisKindZSet branch is not reached: Pebble snapshots are emitted by iterator order (store/snapshot_pebble.go uses iter.First(); iter.Next()), and the TTL key prefix !redis|ttl| sorts before all zset prefixes !zs|.... That means HandleTTL sees redisKindUnknown, increments orphanTTLCount, and discards the expiry before zsetState is created, so every restored zset with a TTL becomes persistent. The zset TTL needs to be buffered/rerouted when the zset row is observed rather than relying on prior type observation.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Redis Sorted Sets (ZSets) in the backup system, adding a new encoder that translates raw snapshot records into versioned JSON files. The implementation handles member-score mappings, TTL inlining, and ensures deterministic output by sorting members by name. Review feedback suggests refactoring the marshaling logic for ZSets and Hashes into a shared helper to reduce code duplication, although the developer previously noted that duplication was intentional to maintain JSON field order.
| // Reviewers comparing the two functions should diff | ||
| // (hashFieldRecord, "fields") against (zsetMemberRecord, "members") — | ||
| // the only intentional divergence. | ||
| func marshalZSetJSON(st *redisZSetState) ([]byte, error) { //nolint:dupl // see comment above |
There was a problem hiding this comment.
While deterministic field order is required for stable backups, repository guidelines (Rule 9) require centralizing common logic to avoid duplication. Instead of using nolint:dupl to suppress the warning, consider refactoring marshalZSetJSON and marshalHashJSON to use a shared helper function that preserves the necessary field order.
References
- Centralize common complex logic, such as retry/backoff mechanisms, into shared helper functions to avoid code duplication and improve maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/backup/redis_zset_test.go (1)
16-19: ⚡ Quick winRemove avoidable
//nolint:gosecsuppressions in test key/value builders.Line 18 and Lines 35, 45, 58, and 70 can avoid suppression with explicit bounds-safe conversions. Keep
//nolintonly where truly unavoidable.Proposed refactor
func encodeZSetMetaValue(memberCount int64) []byte { + if memberCount < 0 { + panic("test bug: negative memberCount") + } v := make([]byte, 8) - binary.BigEndian.PutUint64(v, uint64(memberCount)) //nolint:gosec + binary.BigEndian.PutUint64(v, uint64(memberCount)) return v } + +func putUserKeyLen(l *[4]byte, userKey string) { + if len(userKey) > math.MaxUint32 { + panic("test bug: userKey too large") + } + binary.BigEndian.PutUint32(l[:], uint32(len(userKey))) +}func zsetMetaKey(userKey string) []byte { out := []byte(RedisZSetMetaPrefix) var l [4]byte - binary.BigEndian.PutUint32(l[:], uint32(len(userKey))) //nolint:gosec + putUserKeyLen(&l, userKey) out = append(out, l[:]...) return append(out, userKey...) }(Apply the same
putUserKeyLencall pattern inzsetMemberKey,zsetScoreKey, andzsetMetaDeltaKey.)As per coding guidelines,
Use gofmt formatting and pass linters configured in .golangci.yaml ... Avoid //nolint annotations — refactor instead.Also applies to: 35-35, 45-45, 58-58, 70-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/backup/redis_zset_test.go` around lines 16 - 19, The test helpers use //nolint:gosec to silence integer-to-uint64 conversions; replace those suppressions by doing explicit, bounds-safe casts or using the existing putUserKeyLen pattern so the conversion is obviously safe: in encodeZSetMetaValue replace the nolint line with binary.BigEndian.PutUint64(v, uint64(memberCount)) using an explicit checked/constrained conversion (e.g., ensure memberCount >= 0 then cast) or mirror putUserKeyLen’s approach; apply the same refactor to zsetMemberKey, zsetScoreKey, and zsetMetaDeltaKey so no //nolint is required and the conversions are clearly safe.internal/backup/redis_zset.go (1)
87-93: ⚡ Quick winHoist []byte prefix conversions to package-level variables to avoid per-record allocations in snapshot processing.
Lines 87, 177, 180, and 192 repeatedly convert string constants to bytes at call sites during hot-path snapshot scanning. Moving these to package-level
[]bytevariables eliminates four allocations per record processed.♻️ Suggested refactor
const ( RedisZSetMetaPrefix = "!zs|meta|" RedisZSetMemberPrefix = "!zs|mem|" RedisZSetScorePrefix = "!zs|scr|" RedisZSetMetaDeltaPrefix = "!zs|meta|d|" redisZSetScoreSize = 8 ) + +var ( + redisZSetMetaPrefixBytes = []byte(RedisZSetMetaPrefix) + redisZSetMemberPrefixBytes = []byte(RedisZSetMemberPrefix) + redisZSetMetaDeltaPrefixBytes = []byte(RedisZSetMetaDeltaPrefix) +)Then replace:
- Line 87:
[]byte(RedisZSetMetaDeltaPrefix)→redisZSetMetaDeltaPrefixBytes- Line 177:
[]byte(RedisZSetMetaDeltaPrefix)→redisZSetMetaDeltaPrefixBytes- Line 180:
[]byte(RedisZSetMetaPrefix)→redisZSetMetaPrefixBytes- Line 192:
[]byte(RedisZSetMemberPrefix)→redisZSetMemberPrefixBytes🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/backup/redis_zset.go` around lines 87 - 93, Hot-path code repeatedly converts string prefix constants to []byte per record; define package-level byte-slice variables (e.g., redisZSetMetaDeltaPrefixBytes, redisZSetMetaPrefixBytes, redisZSetMemberPrefixBytes) initialized from the existing string constants (RedisZSetMetaDeltaPrefix, RedisZSetMetaPrefix, RedisZSetMemberPrefix) and replace all call-site conversions such as bytes.HasPrefix(key, []byte(RedisZSetMetaDeltaPrefix)) and other occurrences (`[]byte(RedisZSetMetaPrefix)`, `[]byte(RedisZSetMemberPrefix)`) with the new package-level variables; keep the semantic checks (e.g., parseZSetMetaKey usage) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/backup/redis_zset_test.go`:
- Around line 310-316: The test currently only checks envelope["base64"] is
non-empty; instead decode the base64 string from envelope["base64"] (the
variable envelope in redis_zset_test.go) back to bytes and assert the decoded
bytes equal the original payload expected in the test (compare against the known
original member payload used earlier in the test), failing the test if decoding
errors or the bytes differ to guarantee true round-trip behavior.
---
Nitpick comments:
In `@internal/backup/redis_zset_test.go`:
- Around line 16-19: The test helpers use //nolint:gosec to silence
integer-to-uint64 conversions; replace those suppressions by doing explicit,
bounds-safe casts or using the existing putUserKeyLen pattern so the conversion
is obviously safe: in encodeZSetMetaValue replace the nolint line with
binary.BigEndian.PutUint64(v, uint64(memberCount)) using an explicit
checked/constrained conversion (e.g., ensure memberCount >= 0 then cast) or
mirror putUserKeyLen’s approach; apply the same refactor to zsetMemberKey,
zsetScoreKey, and zsetMetaDeltaKey so no //nolint is required and the
conversions are clearly safe.
In `@internal/backup/redis_zset.go`:
- Around line 87-93: Hot-path code repeatedly converts string prefix constants
to []byte per record; define package-level byte-slice variables (e.g.,
redisZSetMetaDeltaPrefixBytes, redisZSetMetaPrefixBytes,
redisZSetMemberPrefixBytes) initialized from the existing string constants
(RedisZSetMetaDeltaPrefix, RedisZSetMetaPrefix, RedisZSetMemberPrefix) and
replace all call-site conversions such as bytes.HasPrefix(key,
[]byte(RedisZSetMetaDeltaPrefix)) and other occurrences
(`[]byte(RedisZSetMetaPrefix)`, `[]byte(RedisZSetMemberPrefix)`) with the new
package-level variables; keep the semantic checks (e.g., parseZSetMetaKey usage)
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a09d53a0-f475-408c-b3ba-4d2181e38e17
📒 Files selected for processing (4)
internal/backup/redis_hash.gointernal/backup/redis_string.gointernal/backup/redis_zset.gointernal/backup/redis_zset_test.go
| envelope, ok := rec["member"].(map[string]any) | ||
| if !ok { | ||
| t.Fatalf("expected base64 envelope on member, got %T(%v)", rec["member"], rec["member"]) | ||
| } | ||
| if envelope["base64"] == "" { | ||
| t.Fatalf("base64 envelope missing payload: %v", envelope) | ||
| } |
There was a problem hiding this comment.
Assert base64 payload correctness, not only presence.
Line 314-Line 316 only verifies non-empty payload, so a wrong payload could still pass. Decode and compare bytes to pin true round-trip behavior.
Proposed test hardening
import (
+ "bytes"
+ "encoding/base64"
"encoding/binary"
"encoding/json"
"math"
@@
envelope, ok := rec["member"].(map[string]any)
if !ok {
t.Fatalf("expected base64 envelope on member, got %T(%v)", rec["member"], rec["member"])
}
- if envelope["base64"] == "" {
- t.Fatalf("base64 envelope missing payload: %v", envelope)
- }
+ 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 %v", decoded, []byte{0x80, 0xff, 0x01})
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| envelope, ok := rec["member"].(map[string]any) | |
| if !ok { | |
| t.Fatalf("expected base64 envelope on member, got %T(%v)", rec["member"], rec["member"]) | |
| } | |
| if envelope["base64"] == "" { | |
| t.Fatalf("base64 envelope missing payload: %v", envelope) | |
| } | |
| envelope, ok := rec["member"].(map[string]any) | |
| if !ok { | |
| t.Fatalf("expected base64 envelope on member, got %T(%v)", rec["member"], rec["member"]) | |
| } | |
| 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 %v", decoded, []byte{0x80, 0xff, 0x01}) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/backup/redis_zset_test.go` around lines 310 - 316, The test
currently only checks envelope["base64"] is non-empty; instead decode the base64
string from envelope["base64"] (the variable envelope in redis_zset_test.go)
back to bytes and assert the decoded bytes equal the original payload expected
in the test (compare against the known original member payload used earlier in
the test), failing the test if decoding errors or the bytes differ to guarantee
true round-trip behavior.
…d records Pebble snapshots are emitted in encoded-key order (store/snapshot_pebble.go::iter.First()+Next()), and `!redis|ttl|` lex-sorts BEFORE every wide-column prefix where the type letter is >= 's' (`!st|` set, `!stream|`, `!zs|` zset — because `r` < `s`/`z`). The original HandleTTL routed any unknown-kind expiry straight into orphanTTLCount, so for sets, streams, and zsets the TTL was DROPPED before zsetState/setState/streamState could claim it. Restored sets/zsets/streams with TTL became permanent. Codex P1 finding on PR #790. The same bug exists in the already- merged set encoder (PR #758); this commit fixes both retroactively. Stream encoder (PR #791) inherits the fix once rebased. Fix: HandleTTL parks unknown-kind expiries in a new pendingTTL map. Each wide-column state-init that may face the bad ordering (setState, zsetState — and streamState once PR #791 lands) drains the entry on first user-key registration via claimPendingTTL(). Finalize counts whatever remains in pendingTTL as truly-unmatched orphans (a TTL whose user key never appeared in any typed record — indicates store corruption or an unknown type prefix). Semantic-change caller audit (per /loop standing instruction): - HandleTTL's redisKindUnknown branch: previously incremented orphanTTLCount immediately; now buffers and lets Finalize count. All callers: only the per-record dispatcher in cmd/elastickv-snapshot-decode (not yet built — Phase 0a follow- up). No external caller mutates orphanTTLCount today. - TestRedisDB_OrphanTTLCountedNotBuffered: updated to assert intake-time orphanTTLCount==0 + pendingTTL grows, then post- Finalize orphanTTLCount==n. - New caller claimPendingTTL: called only by zsetState and setState in this PR. hashState/listState don't call it because their type prefixes (`!hs|`/`!lst|`) lex-sort BEFORE `!redis|ttl|` so the typed record arrives first; pendingTTL is always empty for them. Verified via `grep -n 'claimPendingTTL' internal/backup/`. New tests: - TestRedisDB_ZSetTTLArrivesBeforeRows — pins the fix for zsets. - TestRedisDB_SetTTLArrivesBeforeRows — retroactive coverage for PR #758's set encoder. - TestRedisDB_OrphanTTLCountsTrulyUnmatchedKeys — pins the new Finalize-time orphan semantics. Self-review: 1. Data loss — the original code DROPPED real TTL'd sets/zsets/streams on every backup. This fix recovers them. No new data-loss surface introduced. 2. Concurrency — pendingTTL is added to RedisDB which is already sequential-per-scope; no new locking required. 3. Performance — pendingTTL holds (string-userKey, uint64-expireAt) pairs. The string allocation cost matches kindByKey's, which we already pay for every typed record. The original P2 round 6 concern (don't buffer arbitrarily-large value payloads) is preserved: we still only buffer 8-byte expiry, not value bytes. 4. Consistency — drain happens at FIRST state registration so a later HandleTTL re-arrival (which would route through the redisKindSet/ZSet case in HandleTTL) is a no-op (st.expireAtMs gets overwritten with the same value). 5. Coverage — 3 new tests + 1 updated test. All 64 redis tests pass.
|
@claude review |
|
Claude finished @bootjp's task in 2m 38s —— View job Code Review: PR #790 — Redis zset encoder (Phase 0a)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63e54d91a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ( | ||
| RedisZSetMetaPrefix = "!zs|meta|" | ||
| RedisZSetMemberPrefix = "!zs|mem|" | ||
| RedisZSetScorePrefix = "!zs|scr|" | ||
| RedisZSetMetaDeltaPrefix = "!zs|meta|d|" |
There was a problem hiding this comment.
For zsets stored only as the legacy single-blob key, this new encoder never claims the record because it only recognizes the wide-column !zs|... prefixes. That format is still part of the live data model: adapter/redis_compat_types.go:82 defines !redis|zset|, adapter/redis_compat_commands.go:3495-3508 still writes that key for non-empty persisted zset updates, and adapter/redis_compat_helpers.go:610-631 reads it as the fallback when no wide-column members exist. A snapshot containing such a zset will omit it from zsets/ entirely (and any TTL remains orphaned), so backups can lose user-visible sorted sets; add a handler for !redis|zset|<key> or migrate those blobs during encoding.
Useful? React with 👍 / 👎.
…ield preservation Two P1 findings from chatgpt-codex on PR #791: P1a: Buffer stream TTLs that arrive before stream rows Pebble snapshots emit records in encoded-key order (store/snapshot_pebble.go::iter.First()+Next()), and `!redis|ttl|` lex-sorts BEFORE `!stream|...` because `r` < `s`. In real snapshot order the TTL arrives FIRST, kindByKey is still redisKindUnknown when HandleTTL fires, and the original code counted the TTL as an orphan and dropped it — every TTL'd stream restored as permanent. Same root cause as the set encoder's latent bug in PR #758. This commit adds a pendingTTL infrastructure (matching the parallel fix on PR #790) so the expiry parks during the redisKindUnknown window and drains when streamState first registers the user key. The set encoder gets the same retroactive drain. P1b: Preserve duplicate stream fields instead of map-collapsing XADD permits duplicate field names within one entry (e.g. `XADD s * f v1 f v2`). The protobuf entry stores the interleaved slice verbatim, but my marshalStreamJSONL collapsed pairs into `map[string]string`, silently dropping every duplicate. A restore of such an entry would lose the second (and later) pair. Fix: emit `fields` as a JSON ARRAY of `{name, value}` records (streamFieldJSON). Order is the protobuf's interleaved order so a restore can replay the original XADD argv exactly. The design example at docs/design/2026_04_29_proposed_snapshot_logical_decoder.md:338 showed object form. That representation was unsafe for streams (though fine for hashes where the wire-level encoder normalises field names earlier). The format is owned by Phase 0 — adjusted in this PR before the format ships any consumers. Caller audit (per /loop standing instruction): - HandleTTL's redisKindUnknown branch: same semantic change as PR #790's r1 — previously incremented orphanTTLCount on intake; now buffers in pendingTTL and lets Finalize count at end. Same audit conclusion: no external callers of orphanTTLCount; TestRedisDB_OrphanTTLCountedNotBuffered updated to assert the new intake/Finalize split. - streamEntryJSON.Fields type change `map → slice`: only marshalled by encoding/json; the only reader is the test suite, which is updated in this commit. No on-disk format compatibility concerns because Phase 0 has not shipped a consumer yet. - New caller claimPendingTTL: called by setState (retroactive) and streamState (new) in this PR. hashState/listState don't call it because their type prefixes lex-sort BEFORE `!redis|ttl|`. Verified via `grep -n 'claimPendingTTL' internal/backup/`. New tests: - TestRedisDB_StreamDuplicateFieldsPreserved — pins P1b fix. - TestRedisDB_StreamTTLArrivesBeforeRows — pins P1a fix for streams. - TestRedisDB_SetTTLArrivesBeforeRows — retroactive coverage for PR #758's set encoder (same root cause as the stream bug). - TestRedisDB_StreamFieldsDecodedToArray (renamed from ToObject) — updated to match the array shape. Self-review: 1. Data loss — the original code DROPPED real TTL'd streams on every backup AND dropped duplicate-field entries' later pairs. This fix recovers both. No new data-loss surface introduced. 2. Concurrency — pendingTTL added to RedisDB which is already sequential-per-scope; no new locking required. 3. Performance — pendingTTL holds (string-userKey, uint64-expireAt) pairs; same allocation shape as kindByKey. Fields slice replaces a map of the same logical size — slightly cheaper actually (no hash overhead). 4. Consistency — drain happens at FIRST state registration. The array form preserves insertion order from the protobuf so the restored XADD argv matches. 5. Coverage — 4 new tests + 2 updated. All 78 redis tests pass.
…lob layout
Codex flagged that the wide-column zset encoder skips the legacy
consolidated single-key blob layout the live store still writes.
A zset stored only as `!redis|zset|<userKey>` (with the magic-
prefixed pb.RedisZSetValue body) is silently dropped from backup
output and any inline TTL becomes an orphan — user-visible
sorted-set data loss.
Live-side references (adapter, not changed by this commit):
- adapter/redis_compat_types.go:82 — redisZSetPrefix
- adapter/redis_compat_commands.go:3495-3508 — writes the blob for
non-empty persisted zset updates
- adapter/redis_compat_helpers.go:610-631 — reads it as the
fallback when no wide-column members exist
Fix: new public RedisDB.HandleZSetLegacyBlob method that decodes
the magic-prefixed pb.RedisZSetValue and registers the same per-
member state HandleZSetMember would. The wide-column merge case
(mid-migration snapshot containing BOTH layouts for the same user
key) works because `!redis|zset|` sorts BEFORE `!zs|...` so the
blob arrives first and wide-column rows then update / add members
via the latest-wins map.
Inline TTL: `!redis|zset|<k>` sorts BEFORE `!redis|ttl|<k>`, so
HandleTTL after this handler sees redisKindZSet already and
folds via the case-redisKindZSet branch. No pendingTTL detour
needed for this ordering.
Fail-closed contract (matches existing wide-column path):
- Missing magic prefix → ErrRedisInvalidZSetLegacyBlob
- Unmarshal error → ErrRedisInvalidZSetLegacyBlob
- NaN score → ErrRedisInvalidZSetLegacyBlob (Redis ZADD
rejects NaN at wire level)
Caller audit (per /loop standing instruction): new public method
HandleZSetLegacyBlob has no external callers. Verified via
'grep -rn HandleZSetLegacyBlob --include=*.go' — all matches inside
the test file in this PR. The cmd/elastickv-snapshot-decode
dispatcher (Phase 0a follow-up, not yet built) will route the
`!redis|zset|` prefix to this handler.
Parallel bug class: the SAME issue exists for `!redis|hash|`,
`!redis|set|`, and `!redis|stream|` legacy blob prefixes. Those
encoders shipped in earlier PRs (#725, #758, #791). Each needs
its own legacy-blob handler in a follow-up PR; this commit fixes
only the zset case codex flagged on PR #790.
New tests:
- TestRedisDB_ZSetLegacyBlobRoundTrip — basic round-trip
- TestRedisDB_ZSetLegacyBlobThenWideColumnMerges — mid-migration
- TestRedisDB_ZSetLegacyBlobWithInlineTTL — TTL ordering
- TestRedisDB_ZSetLegacyBlobRejectsMissingMagic — fail-closed
- TestRedisDB_ZSetLegacyBlobRejectsNaNScore — fail-closed
- TestRedisDB_ZSetLegacyBlobRejectsMalformedKey — fail-closed
Self-review:
1. Data loss — exact opposite: this commit RECOVERS zsets that
were silently dropped. New fail-closed guards prevent silently
importing a corrupt blob.
2. Concurrency — no new shared state; per-DB sequential as before.
3. Performance — one protobuf Unmarshal per legacy zset key
(same as the live read path). Member map shares the same
latest-wins behavior as wide-column intake.
4. Consistency — merge order (blob first, wide-column second) is
determined by snapshot lex order; tested explicitly.
5. Coverage — 6 new tests. All 84 redis tests pass.
|
@claude review |
|
Claude finished @bootjp's task in 4m 37s —— View job Code Review: PR #790 — Redis zset encoder (Phase 0a) — Round 3Tasks
The third commit ( Commit 3:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5446190f78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| st := r.zsetState(userKey) | ||
| for _, e := range entries { | ||
| st.members[e.member] = e.score | ||
| } |
There was a problem hiding this comment.
When a snapshot contains both !redis|zset|<key> and !zs|mem|... rows for the same key, this merge makes backup output include members from the legacy blob that are not user-visible anymore. I checked the live read path in adapter/redis_compat_helpers.go:610-620: as soon as any wide-column member exists, loadZSetAt returns only loadZSetMembersAt and uses the legacy blob only as a fallback when no wide rows exist. A dump taken after a partially migrated/stale legacy blob remains alongside current wide rows would therefore restore deleted or outdated members; the encoder should skip or replace legacy state once wide-column rows are observed for that key.
Useful? React with 👍 / 👎.
Codex flagged that the r2 fix (legacy-blob handler in #790 #2) mis-handles the mid-migration / stale-leftover case: when a snapshot contains BOTH `!redis|zset|<k>` and `!zs|mem|<k>...` for the same user key, my code merged both, surfacing members that are no longer visible to readers. Live read path (adapter/redis_compat_helpers.go:610-620 RedisServer.loadZSetAt): when ANY wide-column row exists, loadZSetMembersAt is used and the legacy blob is IGNORED. A stale `!redis|zset|` blob left over after a partial migration would be invisible to clients but my encoder was surfacing its entries in the dump — silent backup corruption. Fix: add a sawWide flag to redisZSetState that flips on every wide-column observation (HandleZSetMeta + HandleZSetMember). markZSetWide() clears any legacy entries previously deposited by HandleZSetLegacyBlob, and HandleZSetLegacyBlob short-circuits if sawWide is already set. The result matches the live read path's source-of-truth selection: - No wide rows → legacy blob is the source of truth. - Any wide row exists → wide rows are authoritative; legacy entries are ignored (whether they arrived before or after the wide row). Caller audit (per /loop standing instruction): the new markZSetWide method is called only from HandleZSetMeta line 160 and HandleZSetMember line 194. HandleZSetLegacyBlob (line 268) short-circuits on the same flag. No other call sites — verified via 'grep -rn markZSetWide|sawWide internal/backup/'. The semantic change (legacy entries cleared on first wide observation) is contained to redisZSetState; no public API or external caller visible. Test changes: - TestRedisDB_ZSetLegacyBlobThenWideColumnMerges renamed to TestRedisDB_ZSetWideColumnSupersedesLegacyBlob, expectations updated (bob-stale no longer survives). - New TestRedisDB_ZSetMetaAloneEvictsLegacyBlob — meta-only wide-column row evicts legacy blob. - New TestRedisDB_ZSetLegacyBlobAfterWideRowsIsDropped — reverse- order case (wide first, blob later). Self-review: 1. Data loss — opposite: this fix PREVENTS surfacing stale members that the live read path hides. Legacy blob is still applied when no wide rows exist (preserves the original P1 round 2 fix for legacy-only zsets). 2. Concurrency — no new shared state. 3. Performance — sawWide is a single bool per state; markZSetWide short-circuits on subsequent calls so the clear-loop runs at most once per state. 4. Consistency — encoder output now matches `loadZSetAt`'s read- side source-of-truth selection. Tested with both orderings (legacy-first-wide-later, wide-first-legacy-later, meta-alone). 5. Coverage — 1 renamed test + 2 new tests. All 90 redis tests pass.
|
@claude review |
|
Claude finished @bootjp's task in 4m 12s —— View job Code Review: PR #790 — Redis zset encoder (Phase 0a) — Round 4Tasks
Commit 4 (
|
| Order | Trace | Result |
|---|---|---|
| legacy blob → wide member | HandleZSetLegacyBlob adds {alice:1, bob-stale:2}; HandleZSetMember("alice", 99) → markZSetWide clears map, adds alice:99 |
{alice:99} ✓ |
| wide member → legacy blob | HandleZSetMember → sawWide=true; HandleZSetLegacyBlob → st.sawWide check → early return |
wide-only ✓ |
| legacy blob → meta(0) | HandleZSetLegacyBlob adds {stale:1}; HandleZSetMeta(0) → markZSetWide clears map |
{} ✓ |
| legacy blob alone | sawWide never set; entries stay in map through Finalize | legacy entries ✓ |
Key invariants confirmed:
markZSetWide()is called from bothHandleZSetMeta(line 160) andHandleZSetMember(line 194), covering every wide-column entry point ✓HandleZSetScoreandHandleZSetMetaDeltaare 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
markZSetWidein-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|.
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)
}
}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)
}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
sawWidemechanism mirrorsadapter/redis_compat_helpers.go:610-620exactly: wide-column rows are authoritative, legacy blob is a fallback consulted only when no wide rows exist ✓markZSetWideis guarded byif st.sawWide { return }making it idempotent across multiple wide member records ✓redisZSetLegacyProtoPrefix = []byte{0x00, 'R', 'Z', 0x01}matchesadapter/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
decodeZSetLegacyBlobValuematchesHandleZSetMembercontract ✓ - pendingTTL drain in
zsetState()correctly handles the legacy-blob+TTL scenario at the code level (comment is wrong, code is right) ✓ encodeZSetLegacyBlobValueandzsetLegacyBlobKeytest 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab88f3e096
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // this state is stale; drop it. The live read path | ||
| // (adapter/redis_compat_helpers.go:610-620) makes the same | ||
| // choice on read. | ||
| r.markZSetWide(st) |
There was a problem hiding this comment.
When a snapshot has a legacy !redis|zset|<k> blob plus a !zs|meta|<k> row but no !zs|mem| rows, this call clears the legacy members and the dump restores an empty zset. The live read path in adapter/redis_compat_helpers.go:610-623 only switches away from the legacy blob when the member-prefix scan returns rows; meta alone is not used as the value source. This can happen with stale/partial wide metadata alongside a still-visible legacy blob, so the backup should only evict legacy entries once a member row is observed, not on meta alone.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds the Redis sorted-set (zset) encoder for the Phase 0 logical
snapshot decoder (
docs/design/2026_04_29_proposed_snapshot_logical_decoder.md,lines 334-335). Mirrors the hash/list/set encoders shipped in
#725/#755/#758. After this lands, Phase 0a's remaining Redis work
is the stream encoder + HLL TTL sidecar; the per-adapter encoders
themselves will be complete.
Output shape per the design:
{"format_version": 1, "members": [{"member": "alice", "score": 100}], "expire_at_ms": null}Members sorted by raw byte order (not by score) so
diff -rbetweentwo dumps with the same logical contents stays line-stable across
score-only mutations. Scores emit as JSON numbers for finite values,
ZADD-conventional
"+inf"/"-inf"strings for ±Inf (json.Marshalrejects non-finite floats). NaN scores fail closed at intake — Redis's
ZADD rejects NaN at the wire level, so a NaN at backup time indicates
corruption.
Wire layout (mirrors
store/zset_helpers.go):!zs|meta|<userKeyLen(4)><userKey>→ 8-byte BE Len!zs|mem|<userKeyLen(4)><userKey><member>→ 8-byte IEEE 754 score!zs|scr|<userKeyLen(4)><userKey><sortableScore(8)><member>→silently discarded (secondary index;
!zs|mem|is the source oftruth)
!zs|meta|d|<userKeyLen(4)><userKey><commitTS(8)><seqInTxn(4)>→silently skipped (delta family; same policy as hash/set encoders)
TTL routing:
!redis|ttl|<userKey>for a registered zset folds intothe JSON
expire_at_msfield, matching the set/list/hash inlining,so a restorer replays ZADD + EXPIRE in one shot.
Self-review (5 lenses)
!zs|mem|is the source of truth;!zs|scr|anddelta records are intentional silent skips with audit notes.
Empty zsets (Len=0 but meta seen) still emit a file because
TYPE k → zsetis observable to clients.RedisDBis sequential per scope(matches the existing per-DB encoder contract); no shared state.
Bounded by
maxWideColumnItemson the live side; sort is O(n log n)on member-name bytes. Identical cost shape to hash/set encoders.
(not map). Inf score uses
json.RawMessageso the samescorekey emits as either a number or a string. NaN fails closed.
internal/backup/redis_zset_test.go:HandleZSetMetaentry +parseZSetMetaKeyguard)HandleZSetMetaDeltaexplicit entryHandleZSetScoresilent-discardCaller audit (per
/loopstanding instruction)Semantics-changing edit: new
case redisKindZSet:branch inHandleTTL(redis_string.go:310). Purely additive — the new branchfires only when
zsetState()has previously registered the key.No existing handler maps to
redisKindZSet, so no prior call sitechanges behavior. Verified:
Three references, all new in this PR. No legacy caller assumes the
prior behavior.
Test plan
go test -race ./internal/backup/→ okgolangci-lint run ./internal/backup/...→ 0 issuesgo build ./...→ okgo vet ./internal/backup/...→ okSummary by CodeRabbit
New Features
Tests