From 72e2b75a61e20c22d838e047318f0cac7103c0f6 Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Sat, 2 May 2026 01:24:15 +0900 Subject: [PATCH 1/4] backup: add Redis hash encoder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First of the Phase 0a wide-column encoders (after the redis_string.go strings/HLL/TTL work in PR #713). Hashes follow a meta+per-field key shape on the live store (`!hs|meta|` → `[Len(8)]`, `!hs|fld|` → field bytes; see store/hash_helpers.go), so the encoder buffers each user key's fields in memory and flushes one `hashes/.json` file per hash at Finalize. Output schema matches the design (docs/design/2026_04_29_proposed_snapshot_logical_decoder.md:327): { "format_version": 1, "fields": {"name": "alice"}, "expire_at_ms": null } Cross-cutting decisions: - TTL is folded INLINE into expire_at_ms (not a separate sidecar) so a restorer can replay the hash in one shot. HandleTTL routes redisKindHash records into the hash state rather than the strings_ttl.jsonl path. - Field VALUES use the same UTF-8-or-base64 envelope introduced for the SQS message body in PR #714: valid UTF-8 → plain JSON string; non-UTF-8 → `{"base64":"..."}`. Field NAMES that are not valid UTF-8 are percent-encoded (EncodeSegment) for the JSON object key — JSON object keys must be strings. - Empty hashes (HLEN==0) still emit a file because their existence is observable to clients (HEXISTS, HLEN). Mismatched declared-vs-observed length surfaces as `redis_hash_length_mismatch` warning. - SHA-fallback user keys feed back through the existing recordIfFallback path so KEYMAP.jsonl still reverses non-reversible filenames. - Output is sorted by user key + field name for deterministic byte-identical dumps across runs (matches the round-9 sort pattern from #716 schemaToPublic). Tests cover: round-trip, empty hash, inline TTL routing, length mismatch warning, binary value envelope, malformed meta, truncated field key, SHA-fallback keymap. --- internal/backup/redis_hash.go | 255 +++++++++++++++++++++++++++ internal/backup/redis_hash_test.go | 272 +++++++++++++++++++++++++++++ internal/backup/redis_string.go | 38 +++- 3 files changed, 556 insertions(+), 9 deletions(-) create mode 100644 internal/backup/redis_hash.go create mode 100644 internal/backup/redis_hash_test.go diff --git a/internal/backup/redis_hash.go b/internal/backup/redis_hash.go new file mode 100644 index 00000000..3c44b5fc --- /dev/null +++ b/internal/backup/redis_hash.go @@ -0,0 +1,255 @@ +package backup + +import ( + "bytes" + "encoding/base64" + "encoding/binary" + "encoding/json" + "path/filepath" + "sort" + "unicode/utf8" + + cockroachdberr "github.com/cockroachdb/errors" +) + +// Snapshot key prefixes the hash encoder dispatches on. Mirror the live +// store/hash_helpers.go constants — a renamed prefix on the live side +// surfaces here at compile time via the dispatch tests. +const ( + RedisHashMetaPrefix = "!hs|meta|" + RedisHashFieldPrefix = "!hs|fld|" + RedisHashMetaDeltaPrefix = "!hs|meta|d|" + + // hashUserKeyLenSize is the fixed BE-uint32 width of the + // per-key length prefix used by every wide-column key shape. + // Mirrors store/wideColKeyLenSize. + hashUserKeyLenSize = 4 +) + +// ErrRedisInvalidHashMeta is returned when the !hs|meta| value is not +// the expected 8-byte big-endian field count. +var ErrRedisInvalidHashMeta = cockroachdberr.New("backup: invalid !hs|meta| value") + +// ErrRedisInvalidHashKey is returned when an !hs| key cannot be parsed +// for its userKeyLen+userKey segment (truncated, malformed, etc). +var ErrRedisInvalidHashKey = cockroachdberr.New("backup: malformed !hs| key") + +// redisHashState buffers the per-userKey hash being assembled. The +// encoder accumulates fields as they arrive and flushes a single JSON +// record at Finalize time. We deliberately buffer per key (rather than +// stream) because the design's per-hash JSON shape requires the full +// field map up-front and Redis hashes are typically small. +type redisHashState struct { + declaredLen int64 + metaSeen bool + fields map[string][]byte // field-name → field-value bytes + expireAtMs uint64 + hasTTL bool +} + +// HandleHashMeta processes one !hs|meta| record. The value is +// the 8-byte BE field count. We park the state for finalize-time flush +// and register the user key so a later !redis|ttl| record +// routes back to this hash state. +func (r *RedisDB) HandleHashMeta(key, value []byte) error { + userKey, ok := parseHashMetaKey(key) + if !ok { + return cockroachdberr.Wrapf(ErrRedisInvalidHashKey, "meta key: %q", key) + } + if len(value) != redisUint64Bytes { + return cockroachdberr.Wrapf(ErrRedisInvalidHashMeta, + "length %d != %d", len(value), redisUint64Bytes) + } + st := r.hashState(userKey) + st.declaredLen = int64(binary.BigEndian.Uint64(value)) //nolint:gosec // signed int64 by design + st.metaSeen = true + r.kindByKey[string(userKey)] = redisKindHash + return nil +} + +// HandleHashField processes one !hs|fld| record. +// The value is the raw field-value bytes (binary-safe). +func (r *RedisDB) HandleHashField(key, value []byte) error { + userKey, fieldName, ok := parseHashFieldKey(key) + if !ok { + return cockroachdberr.Wrapf(ErrRedisInvalidHashKey, "field key: %q", key) + } + if len(fieldName) == 0 { + return cockroachdberr.Wrapf(ErrRedisInvalidHashKey, + "empty field name in key %q", key) + } + st := r.hashState(userKey) + st.fields[string(fieldName)] = bytes.Clone(value) + r.kindByKey[string(userKey)] = redisKindHash + return nil +} + +// hashState lazily creates per-key state. +func (r *RedisDB) hashState(userKey []byte) *redisHashState { + if st, ok := r.hashes[string(userKey)]; ok { + return st + } + st := &redisHashState{fields: make(map[string][]byte)} + r.hashes[string(userKey)] = st + return st +} + +// parseHashMetaKey strips !hs|meta| and the 4-byte BE userKeyLen prefix. +// Returns (userKey, true) on success. +func parseHashMetaKey(key []byte) ([]byte, bool) { + rest := bytes.TrimPrefix(key, []byte(RedisHashMetaPrefix)) + if len(rest) == len(key) { + return nil, false + } + return parseUserKeyLenPrefix(rest) +} + +// parseHashFieldKey strips !hs|fld|, the 4-byte userKeyLen prefix, and +// returns (userKey, fieldName, true). +func parseHashFieldKey(key []byte) ([]byte, []byte, bool) { + rest := bytes.TrimPrefix(key, []byte(RedisHashFieldPrefix)) + if len(rest) == len(key) { + return nil, nil, false + } + userKey, ok := parseUserKeyLenPrefix(rest) + if !ok { + return nil, nil, false + } + fieldName := rest[hashUserKeyLenSize+len(userKey):] + return userKey, fieldName, true +} + +// parseUserKeyLenPrefix decodes the shared shape used +// by every wide-column !hs|/!st|/!zs| key. Returns the userKey slice +// (aliasing the input) plus a presence flag. +func parseUserKeyLenPrefix(b []byte) ([]byte, bool) { + if len(b) < hashUserKeyLenSize { + return nil, false + } + ukLen := int(binary.BigEndian.Uint32(b[:hashUserKeyLenSize])) //nolint:gosec // bounded by len(b) + if len(b) < hashUserKeyLenSize+ukLen { + return nil, false + } + return b[hashUserKeyLenSize : hashUserKeyLenSize+ukLen], true +} + +// flushHashes writes one JSON file per accumulated hash to +// hashes/.json. Empty hashes (Len=0, no fields) still emit a +// file because their existence is observable to clients (HEXISTS, +// HLEN). Mismatched declared-vs-observed length surfaces an +// `redis_hash_length_mismatch` warning. +func (r *RedisDB) flushHashes() error { + if len(r.hashes) == 0 { + return nil + } + dir := filepath.Join(r.dbDir(), "hashes") + if err := r.ensureDir(dir); err != nil { + return err + } + // Stable order across runs (Codex pattern from #716): sort by user + // key before flushing so identical snapshots produce identical + // dump output regardless of Go's randomised map iteration. + userKeys := make([]string, 0, len(r.hashes)) + for k := range r.hashes { + userKeys = append(userKeys, k) + } + sort.Strings(userKeys) + for _, uk := range userKeys { + st := r.hashes[uk] + if r.warn != nil && st.metaSeen && int64(len(st.fields)) != st.declaredLen { + r.warn("redis_hash_length_mismatch", + "user_key_len", len(uk), + "declared_len", st.declaredLen, + "observed_fields", len(st.fields), + "hint", "meta record's Len does not match the count of !hs|fld| keys for this user key") + } + if err := r.writeHashJSON(dir, []byte(uk), st); err != nil { + return err + } + } + return nil +} + +func (r *RedisDB) writeHashJSON(dir string, userKey []byte, st *redisHashState) error { + encoded := EncodeSegment(userKey) + if err := r.recordIfFallback(encoded, userKey); err != nil { + return err + } + path := filepath.Join(dir, encoded+".json") + body, err := marshalHashJSON(st) + if err != nil { + return err + } + if err := writeFileAtomic(path, body); err != nil { + return cockroachdberr.WithStack(err) + } + return nil +} + +// hashJSONFields marshals the per-field map. Field NAMES are emitted +// as JSON object keys, which forces them into UTF-8 string territory: +// when a name is not valid UTF-8 we percent-encode it via +// EncodeSegment so the output is reversible without changing JSON +// shape. Field VALUES preserve full binary fidelity via the same +// `{"base64":...}` envelope used for SQS bodies (see sqsMessageBody). +type hashJSONFields map[string]json.RawMessage + +func marshalHashJSON(st *redisHashState) ([]byte, error) { + fields := make(hashJSONFields, len(st.fields)) + // Sort for deterministic output across runs. + names := make([]string, 0, len(st.fields)) + for name := range st.fields { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + jsonName := name + if !utf8.ValidString(name) { + jsonName = EncodeSegment([]byte(name)) + } + valueJSON, err := marshalRedisBinaryValue(st.fields[name]) + if err != nil { + return nil, err + } + fields[jsonName] = valueJSON + } + type out struct { + FormatVersion uint32 `json:"format_version"` + Fields hashJSONFields `json:"fields"` + ExpireAtMs *uint64 `json:"expire_at_ms"` + } + rec := out{FormatVersion: 1, Fields: fields} + if st.hasTTL { + ms := st.expireAtMs + rec.ExpireAtMs = &ms + } + body, err := json.MarshalIndent(rec, "", " ") + if err != nil { + return nil, cockroachdberr.WithStack(err) + } + return body, nil +} + +// marshalRedisBinaryValue is the shared "binary-safe text or base64 +// envelope" projection used by every Redis wide-column type for value +// bytes that may or may not be valid UTF-8. Mirrors the SQS body +// projection for restore-roundtrip determinism: a UTF-8 bytestring +// emits as a plain JSON string; non-UTF-8 emits as +// `{"base64":""}`. +func marshalRedisBinaryValue(b []byte) (json.RawMessage, error) { + if utf8.Valid(b) { + out, err := json.Marshal(string(b)) + if err != nil { + return nil, cockroachdberr.WithStack(err) + } + return out, nil + } + envelope := struct { + Base64 string `json:"base64"` + }{Base64: base64.RawURLEncoding.EncodeToString(b)} + out, err := json.Marshal(envelope) + if err != nil { + return nil, cockroachdberr.WithStack(err) + } + return out, nil +} diff --git a/internal/backup/redis_hash_test.go b/internal/backup/redis_hash_test.go new file mode 100644 index 00000000..4285b048 --- /dev/null +++ b/internal/backup/redis_hash_test.go @@ -0,0 +1,272 @@ +package backup + +import ( + "encoding/binary" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/cockroachdb/errors" +) + +// encodeHashMetaValue builds the 8-byte BE field-count value used by +// the live store/hash_helpers.go. +func encodeHashMetaValue(fieldCount int64) []byte { + v := make([]byte, 8) + binary.BigEndian.PutUint64(v, uint64(fieldCount)) //nolint:gosec + return v +} + +// hashMetaKey is the test-side key constructor for !hs|meta|. +// Mirrors store.HashMetaKey. +func hashMetaKey(userKey string) []byte { + out := []byte(RedisHashMetaPrefix) + var l [4]byte + binary.BigEndian.PutUint32(l[:], uint32(len(userKey))) //nolint:gosec + out = append(out, l[:]...) + return append(out, userKey...) +} + +// hashFieldKey mirrors store.HashFieldKey: +// !hs|fld|. +func hashFieldKey(userKey, fieldName string) []byte { + out := []byte(RedisHashFieldPrefix) + var l [4]byte + binary.BigEndian.PutUint32(l[:], uint32(len(userKey))) //nolint:gosec + out = append(out, l[:]...) + out = append(out, userKey...) + return append(out, fieldName...) +} + +func readHashJSON(t *testing.T, path string) map[string]any { + t.Helper() + b, err := os.ReadFile(path) //nolint:gosec // test path + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + var m map[string]any + if err := json.Unmarshal(b, &m); err != nil { + t.Fatalf("unmarshal: %v", err) + } + return m +} + +// hashFloat / hashSubMap are local type-assertion-checking helpers +// that satisfy the forcetypeassert linter on JSON-decoded test +// fixtures. JSON unmarshals all numbers to float64 and all objects +// to map[string]any; these wrappers fail loudly with the field name +// rather than panicking on a bare assertion. +func hashFloat(t *testing.T, m map[string]any, key string) float64 { + t.Helper() + v, ok := m[key] + if !ok { + t.Fatalf("field %q missing in %+v", key, m) + } + f, ok := v.(float64) + if !ok { + t.Fatalf("field %q = %T(%v), want float64", key, v, v) + } + return f +} + +func hashSubMap(t *testing.T, m map[string]any, key string) map[string]any { + t.Helper() + v, ok := m[key] + if !ok { + t.Fatalf("field %q missing in %+v", key, m) + } + sub, ok := v.(map[string]any) + if !ok { + t.Fatalf("field %q = %T(%v), want map[string]any", key, v, v) + } + return sub +} + +func TestRedisDB_HashRoundTripBasic(t *testing.T) { + t.Parallel() + db, root := newRedisDB(t) + if err := db.HandleHashMeta(hashMetaKey("user:42"), encodeHashMetaValue(2)); err != nil { + t.Fatalf("HandleHashMeta: %v", err) + } + if err := db.HandleHashField(hashFieldKey("user:42", "name"), []byte("alice")); err != nil { + t.Fatalf("HandleHashField: %v", err) + } + if err := db.HandleHashField(hashFieldKey("user:42", "age"), []byte("30")); err != nil { + t.Fatalf("HandleHashField: %v", err) + } + if err := db.Finalize(); err != nil { + t.Fatalf("Finalize: %v", err) + } + got := readHashJSON(t, filepath.Join(root, "redis", "db_0", "hashes", "user%3A42.json")) + if hashFloat(t, got, "format_version") != 1 { + t.Fatalf("format_version = %v", got["format_version"]) + } + if got["expire_at_ms"] != nil { + t.Fatalf("expire_at_ms must be nil without TTL, got %v", got["expire_at_ms"]) + } + fields := hashSubMap(t, got, "fields") + if fields["name"] != "alice" { + t.Fatalf("name = %v want alice", fields["name"]) + } + if fields["age"] != "30" { + t.Fatalf("age = %v want 30", fields["age"]) + } +} + +func TestRedisDB_HashEmptyHashStillEmitsFile(t *testing.T) { + t.Parallel() + db, root := newRedisDB(t) + // HLEN==0 hashes are observable to clients; the dump must + // preserve their existence. + if err := db.HandleHashMeta(hashMetaKey("empty"), encodeHashMetaValue(0)); err != nil { + t.Fatal(err) + } + if err := db.Finalize(); err != nil { + t.Fatal(err) + } + got := readHashJSON(t, filepath.Join(root, "redis", "db_0", "hashes", "empty.json")) + fields := hashSubMap(t, got, "fields") + if len(fields) != 0 { + t.Fatalf("empty hash should emit empty fields object, got %v", fields) + } +} + +func TestRedisDB_HashTTLInlinedFromScanIndex(t *testing.T) { + t.Parallel() + db, root := newRedisDB(t) + if err := db.HandleHashMeta(hashMetaKey("k"), encodeHashMetaValue(1)); err != nil { + t.Fatal(err) + } + if err := db.HandleHashField(hashFieldKey("k", "f"), []byte("v")); err != nil { + t.Fatal(err) + } + // TTL records arrive on the !redis|ttl| scan index. The hash + // encoder must fold this into the JSON file's expire_at_ms + // field, NOT route it to a separate sidecar (that's the + // strings/HLL pattern only). + if err := db.HandleTTL([]byte("k"), encodeTTLValue(fixedExpireMs)); err != nil { + t.Fatal(err) + } + if err := db.Finalize(); err != nil { + t.Fatal(err) + } + got := readHashJSON(t, filepath.Join(root, "redis", "db_0", "hashes", "k.json")) + if hashFloat(t, got, "expire_at_ms") != float64(fixedExpireMs) { + t.Fatalf("expire_at_ms = %v want %d", got["expire_at_ms"], fixedExpireMs) + } + // And no hashes_ttl.jsonl should ever be emitted. + if _, err := os.Stat(filepath.Join(root, "redis", "db_0", "hashes_ttl.jsonl")); !os.IsNotExist(err) { + t.Fatalf("unexpected hash TTL sidecar: stat err=%v", err) + } +} + +func TestRedisDB_HashLengthMismatchWarns(t *testing.T) { + t.Parallel() + db, _ := newRedisDB(t) + var events []string + db.WithWarnSink(func(event string, _ ...any) { events = append(events, event) }) + // Meta declares 5 fields but only 1 field key arrives — surface + // as a structured warning so operators can correlate dump shape + // with mid-write snapshot timing. + if err := db.HandleHashMeta(hashMetaKey("h"), encodeHashMetaValue(5)); err != nil { + t.Fatal(err) + } + if err := db.HandleHashField(hashFieldKey("h", "only"), []byte("x")); err != nil { + t.Fatal(err) + } + if err := db.Finalize(); err != nil { + t.Fatal(err) + } + wantEvent := "redis_hash_length_mismatch" + found := false + for _, e := range events { + if e == wantEvent { + found = true + break + } + } + if !found { + t.Fatalf("expected %q warning, got %v", wantEvent, events) + } +} + +func TestRedisDB_HashBinaryFieldValueUsesBase64Envelope(t *testing.T) { + t.Parallel() + db, root := newRedisDB(t) + if err := db.HandleHashMeta(hashMetaKey("k"), encodeHashMetaValue(1)); err != nil { + t.Fatal(err) + } + // 0x80 0xff is invalid UTF-8 — emit must use the typed + // `{"base64":"..."}` envelope (Codex pattern from PR #714 SQS body). + if err := db.HandleHashField(hashFieldKey("k", "blob"), []byte{0x80, 0xff, 0x01}); err != nil { + t.Fatal(err) + } + if err := db.Finalize(); err != nil { + t.Fatal(err) + } + got := readHashJSON(t, filepath.Join(root, "redis", "db_0", "hashes", "k.json")) + fields := hashSubMap(t, got, "fields") + envelope, ok := fields["blob"].(map[string]any) + if !ok { + t.Fatalf("expected base64 envelope for binary value, got %T(%v)", fields["blob"], fields["blob"]) + } + if envelope["base64"] == "" { + t.Fatalf("base64 envelope missing payload: %v", envelope) + } +} + +func TestRedisDB_HashRejectsMalformedMetaValueLength(t *testing.T) { + t.Parallel() + db, _ := newRedisDB(t) + err := db.HandleHashMeta(hashMetaKey("k"), []byte{0x00}) + if !errors.Is(err, ErrRedisInvalidHashMeta) { + t.Fatalf("err=%v want ErrRedisInvalidHashMeta", err) + } +} + +func TestRedisDB_HashRejectsTruncatedFieldKey(t *testing.T) { + t.Parallel() + db, _ := newRedisDB(t) + // Field key with userKeyLen=10 but only 3 bytes of userKey: + // must not silently route bytes into the wrong hash. + bad := []byte(RedisHashFieldPrefix) + bad = append(bad, 0x00, 0x00, 0x00, 0x0a) // ukLen = 10 + bad = append(bad, []byte("abc")...) + err := db.HandleHashField(bad, []byte("v")) + if !errors.Is(err, ErrRedisInvalidHashKey) { + t.Fatalf("err=%v want ErrRedisInvalidHashKey", err) + } +} + +func TestRedisDB_HashSHAFallbackKeymapped(t *testing.T) { + t.Parallel() + db, root := newRedisDB(t) + // 300-byte key forces EncodeSegment's SHA-fallback. The encoder + // must record a KEYMAP entry so restore can recover the + // original key bytes from a non-reversible filename. + long := string(bytesRepeat('%', 300)) + if err := db.HandleHashMeta(hashMetaKey(long), encodeHashMetaValue(1)); err != nil { + t.Fatal(err) + } + if err := db.HandleHashField(hashFieldKey(long, "f"), []byte("v")); err != nil { + t.Fatal(err) + } + if err := db.Finalize(); err != nil { + t.Fatal(err) + } + keymapPath := filepath.Join(root, "redis", "db_0", "KEYMAP.jsonl") + if _, err := os.Stat(keymapPath); err != nil { + t.Fatalf("KEYMAP.jsonl missing for SHA-fallback hash key: %v", err) + } +} + +// bytesRepeat is a small helper that mirrors bytes.Repeat without +// importing bytes everywhere — keeps test imports tight. +func bytesRepeat(b byte, n int) []byte { + out := make([]byte, n) + for i := range out { + out[i] = b + } + return out +} diff --git a/internal/backup/redis_string.go b/internal/backup/redis_string.go index 35323a88..df0c9f38 100644 --- a/internal/backup/redis_string.go +++ b/internal/backup/redis_string.go @@ -70,6 +70,7 @@ const ( redisKindUnknown redisKeyKind = iota redisKindString redisKindHLL + redisKindHash ) // RedisDB encodes one logical Redis database (`redis/db_/`). All @@ -147,6 +148,15 @@ type RedisDB struct { keymap *KeymapWriter keymapFile *os.File keymapDir string + + // hashes buffers per-userKey hash state (declared length + the + // in-flight field map + inline TTL). The Phase 0a hash design + // emits one JSON file per hash at Finalize, which requires the + // full field set up front; we accumulate in memory because + // real-world Redis hashes are small (10s–100s of fields) and + // each meta record arriving without a key set must still emit + // the empty-hash file (HLEN==0, observable to clients). + hashes map[string]*redisHashState } // NewRedisDB constructs a RedisDB rooted at /redis/db_/. @@ -163,6 +173,7 @@ func NewRedisDB(outRoot string, dbIndex int) *RedisDB { kindByKey: make(map[string]redisKeyKind), dirsCreated: make(map[string]struct{}), inlineTTLEmitted: make(map[string]struct{}), + hashes: make(map[string]*redisHashState), } } @@ -233,6 +244,14 @@ func (r *RedisDB) HandleTTL(userKey, value []byte) error { return nil } return r.appendTTL(&r.stringsTTL, redisStringsTTLFile, userKey, expireAtMs) + case redisKindHash: + // Wide-column types fold TTL into the per-hash JSON record + // (`expire_at_ms` field) so a restorer can replay the hash + // in one shot rather than chasing a separate sidecar. + st := r.hashState(userKey) + st.expireAtMs = expireAtMs + st.hasTTL = true + return nil case redisKindUnknown: // Track orphan TTL counts only — keys are unused before the // wide-column encoders land, and buffering them allocates @@ -250,19 +269,20 @@ func (r *RedisDB) HandleTTL(userKey, value []byte) error { // dispatched. func (r *RedisDB) Finalize() error { var firstErr error - if err := closeJSONL(r.stringsTTL); err != nil && firstErr == nil { - firstErr = err - } - if err := closeJSONL(r.hllTTL); err != nil && firstErr == nil { - firstErr = err - } - if err := r.closeKeymap(); err != nil && firstErr == nil { - firstErr = err + for _, step := range []func() error{ + r.flushHashes, + func() error { return closeJSONL(r.stringsTTL) }, + func() error { return closeJSONL(r.hllTTL) }, + r.closeKeymap, + } { + if err := step(); err != nil && firstErr == nil { + firstErr = err + } } if r.warn != nil && r.orphanTTLCount > 0 { r.warn("redis_orphan_ttl", "count", r.orphanTTLCount, - "hint", "wide-column type encoders (hash/list/set/zset/stream) have not landed yet") + "hint", "wide-column type encoders (list/set/zset/stream) have not landed yet") } return firstErr } From cacd4489c55d8569c47e29a6ae901d7d014e7a4d Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Sat, 2 May 2026 19:28:29 +0900 Subject: [PATCH 2/4] backup: hash field array shape + 32-bit safety + kindByKey centralisation (PR #725, round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 12 raised two P1 findings on commit 72e2b75a; Gemini raised one high and three mediums alongside. P1: Hash field name encoding via JSON object key was lossy. - Distinct fields could collapse to the same JSON key: a UTF-8 literal `%FF` and a single binary byte `0xFF` both ended up as `%FF` after the EncodeSegment-on-non-UTF-8 fallback, silently dropping data on hashes with both forms. - Long binary field names (>240 bytes) hit EncodeSegment's SHA fallback which is irreversible at this layer (no per-field KEYMAP), so restore could not reconstruct the original bytes. Fix: change `fields` to a JSON array of typed `{name, value}` records. Both name and value go through the same UTF-8-or-base64 envelope (marshalRedisBinaryValue, mirrors PR #714's SQS body), so arbitrary binary bytes round-trip without collisions and without SHA fallback. High (Gemini): parseUserKeyLenPrefix used `int(uint32)` which wraps to negative on 32-bit when the high bit is set, bypassing the bounds check and causing a slice panic. Compare in `uint64` space before converting. Medium (Gemini, ×3): centralise `kindByKey` registration in the hashState helper so meta/field/TTL paths agree on the kind in one place. Audited callers — HandleHashMeta, HandleHashField, and HandleTTL's `case redisKindHash:` arm — all reach hashState via the same userKey identity, and the TTL caller already knows the kind so the now-implicit kindByKey write is idempotent. No other callers per `grep -n "r\.hashState(" internal/backup/`. Tests: - TestRedisDB_HashRoundTripBasic updated to assert the array shape via new hashFieldArray / hashFieldByName helpers. - TestRedisDB_HashEmptyHashStillEmitsFile, _BinaryFieldValue updated likewise. - NEW TestRedisDB_HashBinaryFieldNameRoundTripsViaArray drives the exact Codex collision pair (UTF-8 `%FF` + binary 0xFF) and asserts both names survive distinctly with their expected types. Verified: linux race tests, GOOS=windows, GOOS=js wasm, lint — all clean. --- internal/backup/redis_hash.go | 78 +++++++++++------ internal/backup/redis_hash_test.go | 133 +++++++++++++++++++++++++---- 2 files changed, 170 insertions(+), 41 deletions(-) diff --git a/internal/backup/redis_hash.go b/internal/backup/redis_hash.go index 3c44b5fc..351d3411 100644 --- a/internal/backup/redis_hash.go +++ b/internal/backup/redis_hash.go @@ -63,7 +63,6 @@ func (r *RedisDB) HandleHashMeta(key, value []byte) error { st := r.hashState(userKey) st.declaredLen = int64(binary.BigEndian.Uint64(value)) //nolint:gosec // signed int64 by design st.metaSeen = true - r.kindByKey[string(userKey)] = redisKindHash return nil } @@ -80,17 +79,31 @@ func (r *RedisDB) HandleHashField(key, value []byte) error { } st := r.hashState(userKey) st.fields[string(fieldName)] = bytes.Clone(value) - r.kindByKey[string(userKey)] = redisKindHash return nil } -// hashState lazily creates per-key state. +// hashState lazily creates per-key state. The `kindByKey` registration +// lives here (Gemini medium PR #725 #1/#3) so every code path that +// touches a hash state — meta, field, and the TTL-routing back-edge +// from HandleTTL — agrees on the kind. Caller audit (per the loop's +// "audit semantics-changing edits" rule): +// +// - HandleHashMeta and HandleHashField both want kindByKey set; +// centralising here means the explicit assignment is no longer +// needed at the call site. +// - HandleTTL only ever calls hashState() inside the +// `case redisKindHash:` branch, where kindByKey == redisKindHash +// already holds; the assignment here is idempotent for that path. +// - No other caller exists; verified via +// `grep -n "r\.hashState(" internal/backup/`. func (r *RedisDB) hashState(userKey []byte) *redisHashState { - if st, ok := r.hashes[string(userKey)]; ok { + uk := string(userKey) + if st, ok := r.hashes[uk]; ok { return st } st := &redisHashState{fields: make(map[string][]byte)} - r.hashes[string(userKey)] = st + r.hashes[uk] = st + r.kindByKey[uk] = redisKindHash return st } @@ -122,15 +135,20 @@ func parseHashFieldKey(key []byte) ([]byte, []byte, bool) { // parseUserKeyLenPrefix decodes the shared shape used // by every wide-column !hs|/!st|/!zs| key. Returns the userKey slice // (aliasing the input) plus a presence flag. +// +// The length comparison is done in uint64 space because on 32-bit +// architectures `int(uint32)` can wrap to a negative value when the +// high bit is set, bypassing the bounds check and causing a slice +// panic. Gemini high finding (PR #725 round 1). func parseUserKeyLenPrefix(b []byte) ([]byte, bool) { if len(b) < hashUserKeyLenSize { return nil, false } - ukLen := int(binary.BigEndian.Uint32(b[:hashUserKeyLenSize])) //nolint:gosec // bounded by len(b) - if len(b) < hashUserKeyLenSize+ukLen { + ukLen := binary.BigEndian.Uint32(b[:hashUserKeyLenSize]) + if uint64(len(b)) < uint64(hashUserKeyLenSize)+uint64(ukLen) { return nil, false } - return b[hashUserKeyLenSize : hashUserKeyLenSize+ukLen], true + return b[hashUserKeyLenSize : hashUserKeyLenSize+int(ukLen)], true //nolint:gosec // bounded above } // flushHashes writes one JSON file per accumulated hash to @@ -186,37 +204,49 @@ func (r *RedisDB) writeHashJSON(dir string, userKey []byte, st *redisHashState) return nil } -// hashJSONFields marshals the per-field map. Field NAMES are emitted -// as JSON object keys, which forces them into UTF-8 string territory: -// when a name is not valid UTF-8 we percent-encode it via -// EncodeSegment so the output is reversible without changing JSON -// shape. Field VALUES preserve full binary fidelity via the same -// `{"base64":...}` envelope used for SQS bodies (see sqsMessageBody). -type hashJSONFields map[string]json.RawMessage +// hashFieldRecord is the dump-format projection of one Redis hash +// field. Both name and value go through the same UTF-8-or-base64 +// envelope (json.RawMessage produced by marshalRedisBinaryValue) so +// arbitrary binary bytes round-trip without lossy rewrites. +// +// We deliberately emit `fields` as an ARRAY of records rather than a +// JSON object keyed on the field name, because Redis hash field +// names are binary-safe and JSON object keys are not. With a map +// shape, two distinct fields could collapse to the same JSON key +// (Codex P1 round 12 #725: a UTF-8 literal `%FF` and a single byte +// `0xFF` both percent-encoded to `%FF` would overwrite one +// another), and a >240-byte non-UTF-8 field name would route +// through EncodeSegment's SHA fallback which is non-reversible at +// this layer (no per-field KEYMAP). The array form keeps every +// (name, value) pair distinct and binary-safe. +type hashFieldRecord struct { + Name json.RawMessage `json:"name"` + Value json.RawMessage `json:"value"` +} func marshalHashJSON(st *redisHashState) ([]byte, error) { - fields := make(hashJSONFields, len(st.fields)) - // Sort for deterministic output across runs. + // Sort by raw byte order for deterministic output across runs. names := make([]string, 0, len(st.fields)) for name := range st.fields { names = append(names, name) } sort.Strings(names) + fields := make([]hashFieldRecord, 0, len(names)) for _, name := range names { - jsonName := name - if !utf8.ValidString(name) { - jsonName = EncodeSegment([]byte(name)) + nameJSON, err := marshalRedisBinaryValue([]byte(name)) + if err != nil { + return nil, err } valueJSON, err := marshalRedisBinaryValue(st.fields[name]) if err != nil { return nil, err } - fields[jsonName] = valueJSON + fields = append(fields, hashFieldRecord{Name: nameJSON, Value: valueJSON}) } type out struct { - FormatVersion uint32 `json:"format_version"` - Fields hashJSONFields `json:"fields"` - ExpireAtMs *uint64 `json:"expire_at_ms"` + FormatVersion uint32 `json:"format_version"` + Fields []hashFieldRecord `json:"fields"` + ExpireAtMs *uint64 `json:"expire_at_ms"` } rec := out{FormatVersion: 1, Fields: fields} if st.hasTTL { diff --git a/internal/backup/redis_hash_test.go b/internal/backup/redis_hash_test.go index 4285b048..939a0134 100644 --- a/internal/backup/redis_hash_test.go +++ b/internal/backup/redis_hash_test.go @@ -70,17 +70,43 @@ func hashFloat(t *testing.T, m map[string]any, key string) float64 { return f } -func hashSubMap(t *testing.T, m map[string]any, key string) map[string]any { +// hashFieldArray fetches the `fields` JSON array from a decoded hash +// JSON file and returns it as a slice of {name, value} maps. The +// dump-format projection emits fields as an ARRAY of records (not a +// JSON object) so binary-safe Redis field NAMES round-trip without +// collision — see the comment on hashFieldRecord in redis_hash.go. +func hashFieldArray(t *testing.T, m map[string]any) []map[string]any { t.Helper() - v, ok := m[key] + v, ok := m["fields"] if !ok { - t.Fatalf("field %q missing in %+v", key, m) + t.Fatalf("fields missing in %+v", m) } - sub, ok := v.(map[string]any) + raw, ok := v.([]any) if !ok { - t.Fatalf("field %q = %T(%v), want map[string]any", key, v, v) + t.Fatalf("fields = %T(%v), want []any", v, v) } - return sub + out := make([]map[string]any, 0, len(raw)) + for i, r := range raw { + rm, ok := r.(map[string]any) + if !ok { + t.Fatalf("fields[%d] = %T(%v), want map[string]any", i, r, r) + } + out = append(out, rm) + } + return out +} + +// hashFieldByName looks up a single field in the array shape and +// returns its `value` element (UTF-8 string or base64 envelope). +func hashFieldByName(t *testing.T, m map[string]any, name string) any { + t.Helper() + for _, f := range hashFieldArray(t, m) { + if f["name"] == name { + return f["value"] + } + } + t.Fatalf("field %q not found in %+v", name, m["fields"]) + return nil } func TestRedisDB_HashRoundTripBasic(t *testing.T) { @@ -105,12 +131,11 @@ func TestRedisDB_HashRoundTripBasic(t *testing.T) { if got["expire_at_ms"] != nil { t.Fatalf("expire_at_ms must be nil without TTL, got %v", got["expire_at_ms"]) } - fields := hashSubMap(t, got, "fields") - if fields["name"] != "alice" { - t.Fatalf("name = %v want alice", fields["name"]) + if v := hashFieldByName(t, got, "name"); v != "alice" { + t.Fatalf("name = %v want alice", v) } - if fields["age"] != "30" { - t.Fatalf("age = %v want 30", fields["age"]) + if v := hashFieldByName(t, got, "age"); v != "30" { + t.Fatalf("age = %v want 30", v) } } @@ -126,9 +151,8 @@ func TestRedisDB_HashEmptyHashStillEmitsFile(t *testing.T) { t.Fatal(err) } got := readHashJSON(t, filepath.Join(root, "redis", "db_0", "hashes", "empty.json")) - fields := hashSubMap(t, got, "fields") - if len(fields) != 0 { - t.Fatalf("empty hash should emit empty fields object, got %v", fields) + if fields := hashFieldArray(t, got); len(fields) != 0 { + t.Fatalf("empty hash should emit empty fields array, got %v", fields) } } @@ -206,16 +230,91 @@ func TestRedisDB_HashBinaryFieldValueUsesBase64Envelope(t *testing.T) { t.Fatal(err) } got := readHashJSON(t, filepath.Join(root, "redis", "db_0", "hashes", "k.json")) - fields := hashSubMap(t, got, "fields") - envelope, ok := fields["blob"].(map[string]any) + v := hashFieldByName(t, got, "blob") + envelope, ok := v.(map[string]any) if !ok { - t.Fatalf("expected base64 envelope for binary value, got %T(%v)", fields["blob"], fields["blob"]) + t.Fatalf("expected base64 envelope for binary value, got %T(%v)", v, v) } if envelope["base64"] == "" { t.Fatalf("base64 envelope missing payload: %v", envelope) } } +// TestRedisDB_HashBinaryFieldNameRoundTripsViaArray covers the Codex +// P1 round-12 scenario: a UTF-8-literal field name `%FF` and a +// 1-byte binary field name `0xFF` would collide if `fields` were a +// JSON object, because EncodeSegment percent-encodes 0xFF to `%FF`. +// With the array-of-records shape both names round-trip +// independently — the binary name uses the typed base64 envelope +// while the UTF-8 literal stays as a plain JSON string. +func TestRedisDB_HashBinaryFieldNameRoundTripsViaArray(t *testing.T) { + t.Parallel() + db, root := newRedisDB(t) + if err := db.HandleHashMeta(hashMetaKey("k"), encodeHashMetaValue(2)); err != nil { + t.Fatal(err) + } + if err := db.HandleHashField(hashFieldKey("k", "%FF"), []byte("literal-percent-ff")); err != nil { + t.Fatal(err) + } + if err := db.HandleHashField(append(hashFieldKey("k", ""), 0xff), []byte("binary-byte")); err != nil { + t.Fatal(err) + } + if err := db.Finalize(); err != nil { + t.Fatal(err) + } + got := readHashJSON(t, filepath.Join(root, "redis", "db_0", "hashes", "k.json")) + fields := hashFieldArray(t, got) + if len(fields) != 2 { + t.Fatalf("len(fields) = %d, want 2 (binary and literal must be distinct)", len(fields)) + } + literalSeen, binarySeen := classifyHashFieldShapes(t, fields) + if !literalSeen { + t.Fatalf("literal %%FF field missing from %+v", fields) + } + if !binarySeen { + t.Fatalf("binary 0xFF field missing from %+v", fields) + } +} + +// classifyHashFieldShapes folds the binary-vs-literal field-shape +// assertions out of the parent test so cyclomatic complexity stays +// under the project linter's ceiling. Returns (sawUTF8Literal, +// sawBase64Envelope). +func classifyHashFieldShapes(t *testing.T, fields []map[string]any) (bool, bool) { + t.Helper() + var literalSeen, binarySeen bool + for _, f := range fields { + switch n := f["name"].(type) { + case string: + if n == "%FF" { + literalSeen = true + if f["value"] != "literal-percent-ff" { + t.Fatalf("literal value = %v", f["value"]) + } + } + case map[string]any: + if n["base64"] == base64URLEncode(0xff) { + binarySeen = true + if f["value"] != "binary-byte" { + t.Fatalf("binary value = %v", f["value"]) + } + } + default: + t.Fatalf("unexpected name type %T(%v)", f["name"], f["name"]) + } + } + return literalSeen, binarySeen +} + +func base64URLEncode(b byte) string { + // base64.RawURLEncoding of a single byte; matches what + // marshalRedisBinaryValue emits for non-UTF-8 content. + const alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_" + hi := alphabet[b>>2] + lo := alphabet[(b&0x3)<<4] + return string([]byte{hi, lo}) +} + func TestRedisDB_HashRejectsMalformedMetaValueLength(t *testing.T) { t.Parallel() db, _ := newRedisDB(t) From f44a4e7b8ee3795ebb657f33179a9e2a0d9d2901 Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Sat, 2 May 2026 19:43:28 +0900 Subject: [PATCH 3/4] backup: accept empty Redis hash field names (PR #725, round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 round 13 (commit 4646b9f7): HandleHashField was rejecting zero-length field names as ErrRedisInvalidHashKey, but Redis hash field names are binary-safe and `HSET k "" v` is a valid command. The live store emits a key shaped exactly `!hs|fld|` with no trailing field bytes for that case, so backup decoding would error out on real data created via HSET with an empty field name. Remove the explicit zero-length check; the array-of-records JSON shape introduced in round 1 already handles empty names without collision (an entry like {"name":"", "value":"..."} sorts and round-trips distinctly). Caller audit per loop's audit-semantics rule (fail-closed → fail-open behavior change for empty fieldName): - HandleHashField has only test-code callers; no production pipeline consumes it yet (per `grep -n "HandleHashField" internal/backup/`). - The truncated-key test (TestRedisDB_HashRejectsTruncatedFieldKey) drives a malformed binary key whose parseHashFieldKey returns ok=false; that branch still surfaces ErrRedisInvalidHashKey, independent of the empty-name check we removed. Test: TestRedisDB_HashAcceptsEmptyFieldName drives one empty- name field and one named field, asserts both survive distinct records in the output JSON. --- internal/backup/redis_hash.go | 11 +++++--- internal/backup/redis_hash_test.go | 41 ++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/internal/backup/redis_hash.go b/internal/backup/redis_hash.go index 351d3411..8333ac88 100644 --- a/internal/backup/redis_hash.go +++ b/internal/backup/redis_hash.go @@ -68,15 +68,18 @@ func (r *RedisDB) HandleHashMeta(key, value []byte) error { // HandleHashField processes one !hs|fld| record. // The value is the raw field-value bytes (binary-safe). +// +// Note: Redis hash field names are binary-safe and may legitimately +// be empty — `HSET k "" v` is a valid command and the live store +// emits a key shaped exactly `!hs|fld|` with no +// trailing field bytes. We deliberately do NOT reject zero-length +// field names here so backup decoding succeeds on real data created +// via HSET with empty names. Codex P1 round 13 (PR #725). func (r *RedisDB) HandleHashField(key, value []byte) error { userKey, fieldName, ok := parseHashFieldKey(key) if !ok { return cockroachdberr.Wrapf(ErrRedisInvalidHashKey, "field key: %q", key) } - if len(fieldName) == 0 { - return cockroachdberr.Wrapf(ErrRedisInvalidHashKey, - "empty field name in key %q", key) - } st := r.hashState(userKey) st.fields[string(fieldName)] = bytes.Clone(value) return nil diff --git a/internal/backup/redis_hash_test.go b/internal/backup/redis_hash_test.go index 939a0134..554974bf 100644 --- a/internal/backup/redis_hash_test.go +++ b/internal/backup/redis_hash_test.go @@ -315,6 +315,47 @@ func base64URLEncode(b byte) string { return string([]byte{hi, lo}) } +// TestRedisDB_HashAcceptsEmptyFieldName is the regression for Codex +// P1 round 13 (PR #725): Redis hash field names are binary-safe and +// `HSET k "" v` is a valid command, so the live store emits a key +// shaped exactly `!hs|fld|` with no trailing field +// bytes. The previous explicit zero-length rejection in +// HandleHashField caused backup decoding to error out on real data. +// Now an empty field name is accepted and emitted as one of the +// hash record's `fields` entries with an empty-string `name`. +func TestRedisDB_HashAcceptsEmptyFieldName(t *testing.T) { + t.Parallel() + db, root := newRedisDB(t) + if err := db.HandleHashMeta(hashMetaKey("k"), encodeHashMetaValue(2)); err != nil { + t.Fatal(err) + } + if err := db.HandleHashField(hashFieldKey("k", ""), []byte("empty-name-value")); err != nil { + t.Fatalf("empty field name must be accepted: %v", err) + } + if err := db.HandleHashField(hashFieldKey("k", "named"), []byte("named-value")); err != nil { + t.Fatal(err) + } + if err := db.Finalize(); err != nil { + t.Fatal(err) + } + got := readHashJSON(t, filepath.Join(root, "redis", "db_0", "hashes", "k.json")) + fields := hashFieldArray(t, got) + if len(fields) != 2 { + t.Fatalf("len(fields) = %d, want 2", len(fields)) + } + // The empty-string name sorts before "named"; the array form + // preserves both as distinct records. + if fields[0]["name"] != "" { + t.Fatalf("fields[0].name = %v, want empty string", fields[0]["name"]) + } + if fields[0]["value"] != "empty-name-value" { + t.Fatalf("empty-name field value = %v", fields[0]["value"]) + } + if fields[1]["name"] != "named" { + t.Fatalf("fields[1].name = %v", fields[1]["name"]) + } +} + func TestRedisDB_HashRejectsMalformedMetaValueLength(t *testing.T) { t.Parallel() db, _ := newRedisDB(t) From b757dadf9bf93c057feadc85479a84590f402a54 Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Sat, 2 May 2026 20:50:41 +0900 Subject: [PATCH 4/4] backup: skip hash meta-delta keys in HandleHashMeta (PR #725, round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 round 14 (commit 6abf62f5): The delta prefix `!hs|meta|d|` shares the base meta prefix `!hs|meta|` (the live store/hash_helpers.go documents the same nesting). A snapshot dispatcher that routes records by "starts with RedisHashMetaPrefix" lands delta records in HandleHashMeta, where parseHashMetaKey trimmed the meta prefix and tried to parse `d|...` as `` — failing with ErrRedisInvalidHashKey on real snapshots that contain unresolved deltas. Phase 0a's hash output emits an array of OBSERVED fields (`!hs|fld|...` records), so the delta arithmetic the live system uses to recompute Len is not needed at backup time. Skip delta keys silently in HandleHashMeta. Defense-in-depth: parseHashMetaKey also returns `!ok` for delta-prefixed keys so a future refactor that bypasses the HandleHashMeta short-circuit still surfaces a parse failure rather than silent state corruption. Caller audit (per loop's audit-semantics rule for fail-closed → fail-open behavior change): - HandleHashMeta has only test-code callers; no production pipeline consumes it yet. - parseHashMetaKey is called only from HandleHashMeta. The !ok return is mapped to ErrRedisInvalidHashKey in HandleHashMeta's existing path; the delta short-circuit runs first now so the parser path never sees a delta key in normal flow. Test: TestRedisDB_HashHandleHashMetaSkipsDeltaKey synthesises a realistic delta key and asserts both that HandleHashMeta returns nil and that parseHashMetaKey returns !ok directly. --- internal/backup/redis_hash.go | 22 ++++++++++++++++- internal/backup/redis_hash_test.go | 38 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/internal/backup/redis_hash.go b/internal/backup/redis_hash.go index 8333ac88..38a9b410 100644 --- a/internal/backup/redis_hash.go +++ b/internal/backup/redis_hash.go @@ -51,7 +51,18 @@ type redisHashState struct { // the 8-byte BE field count. We park the state for finalize-time flush // and register the user key so a later !redis|ttl| record // routes back to this hash state. +// +// Delta keys (!hs|meta|d|...) share the !hs|meta| string prefix, so a +// snapshot dispatcher that routes by "starts with RedisHashMetaPrefix" +// will land delta records here too. Phase 0a's output (an array of +// observed fields) doesn't need to apply the delta arithmetic — the +// !hs|fld|... records are the source of truth — so we silently skip +// delta keys instead of returning ErrRedisInvalidHashKey. Codex P1 +// round 14 (PR #725 #13). func (r *RedisDB) HandleHashMeta(key, value []byte) error { + if bytes.HasPrefix(key, []byte(RedisHashMetaDeltaPrefix)) { + return nil + } userKey, ok := parseHashMetaKey(key) if !ok { return cockroachdberr.Wrapf(ErrRedisInvalidHashKey, "meta key: %q", key) @@ -111,8 +122,17 @@ func (r *RedisDB) hashState(userKey []byte) *redisHashState { } // parseHashMetaKey strips !hs|meta| and the 4-byte BE userKeyLen prefix. -// Returns (userKey, true) on success. +// Returns (userKey, true) on success. Delta keys (!hs|meta|d|...) +// share the meta string prefix and would otherwise be parsed as +// base-meta with a garbage userKeyLen — refuse them at the boundary +// so a misrouted delta surfaces a parse error rather than silent +// state corruption. Callers that want delta-tolerant behavior +// (HandleHashMeta) should detect the delta prefix BEFORE calling +// this function. Codex P1 round 14 (PR #725 #13). func parseHashMetaKey(key []byte) ([]byte, bool) { + if bytes.HasPrefix(key, []byte(RedisHashMetaDeltaPrefix)) { + return nil, false + } rest := bytes.TrimPrefix(key, []byte(RedisHashMetaPrefix)) if len(rest) == len(key) { return nil, false diff --git a/internal/backup/redis_hash_test.go b/internal/backup/redis_hash_test.go index 554974bf..b72e8df9 100644 --- a/internal/backup/redis_hash_test.go +++ b/internal/backup/redis_hash_test.go @@ -365,6 +365,44 @@ func TestRedisDB_HashRejectsMalformedMetaValueLength(t *testing.T) { } } +// TestRedisDB_HashHandleHashMetaSkipsDeltaKey is the regression for +// Codex P1 round 14 (PR #725 #13): the delta prefix `!hs|meta|d|` +// shares the base meta prefix `!hs|meta|`, so a snapshot dispatcher +// that routes by "starts with RedisHashMetaPrefix" would land delta +// records in HandleHashMeta. Phase 0a's output (an array of +// observed fields) doesn't need to apply delta arithmetic — the +// !hs|fld| records are the source of truth — so deltas are silently +// skipped. The earlier behavior aborted with ErrRedisInvalidHashKey, +// breaking valid snapshots that contain unresolved deltas. +func TestRedisDB_HashHandleHashMetaSkipsDeltaKey(t *testing.T) { + t.Parallel() + db, _ := newRedisDB(t) + // Synthesise a !hs|meta|d| + // key shape: prefix + 4-byte len + userKey + 8B commitTS + 4B + // seqInTxn trailer. + deltaKey := []byte(RedisHashMetaDeltaPrefix) + var l [4]byte + binary.BigEndian.PutUint32(l[:], uint32(len("k"))) //nolint:gosec + deltaKey = append(deltaKey, l[:]...) + deltaKey = append(deltaKey, "k"...) + deltaKey = append(deltaKey, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01) + deltaKey = append(deltaKey, 0x00, 0x00, 0x00, 0x00) + // LenDelta value is a single signed int64; -1 here, encoded + // as the two's-complement uint64 0xFFFFFFFFFFFFFFFF. + deltaValue := make([]byte, 8) + binary.BigEndian.PutUint64(deltaValue, ^uint64(0)) + if err := db.HandleHashMeta(deltaKey, deltaValue); err != nil { + t.Fatalf("delta key must not error: %v", err) + } + // And the parser-level guard returns !ok for delta keys so a + // later refactor that bypasses HandleHashMeta's prefix check + // still surfaces a parse failure rather than silent + // state-corruption. + if _, ok := parseHashMetaKey(deltaKey); ok { + t.Fatalf("parseHashMetaKey must reject delta-prefixed keys") + } +} + func TestRedisDB_HashRejectsTruncatedFieldKey(t *testing.T) { t.Parallel() db, _ := newRedisDB(t)