backup: Redis simple-type encoders (strings, HLL, TTL routing)#713
backup: Redis simple-type encoders (strings, HLL, TTL routing)#713bootjp wants to merge 14 commits intofeat/backup-phase0a-keymap-manifestfrom
Conversation
Builds on PR #712 (KEYMAP/MANIFEST). Adds the first per-adapter encoder of the Phase 0 logical-backup decoder. Covers the three "simple" Redis prefixes that map ONE snapshot record to one user key — strings, HLLs, and the TTL scan index. Wide-column types (hash/list/set/zset/stream) ship in follow-up PRs. Snapshot key prefixes handled: - !redis|str|<userKey> -> strings/<encoded>.bin (+ inline TTL stripped into strings_ttl.jsonl). New magic-prefix format and legacy raw bytes both supported. - !redis|hll|<userKey> -> hll/<encoded>.bin (raw HLL sketch bytes). - !redis|ttl|<userKey> -> routed by prior type observation: redisKindString -> strings_ttl.jsonl (legacy strings) redisKindHLL -> hll_ttl.jsonl redisKindUnknown -> buffered for wide-column types (warning at Finalize until those encoders land). Routing relies on Pebble's lex order: !redis|hll|* < !redis|str|* < !redis|ttl|* (h < s < t), so by the time HandleTTL is called the type tracker already has the answer. Filenames go through EncodeSegment for safety; values are written atomically (tmp + rename) and writeFileAtomic refuses to overwrite a symlink so a malicious or careless link-in-the-output-tree cannot redirect bytes. Tests cover: new-format with/without inline TTL, legacy format treated as raw, HLL raw round-trip, TTL routing for both kinds, orphan-TTL warning emission, truncated header / TTL section rejection, malformed TTL value rejection, EncodeSegment integration, symlink-overwrite refusal, HasInlineTTL helper. The package keeps these constants local rather than importing from adapter/redis_compat_types.go because the backup decoder is intentionally adapter-independent (it must run as an offline tool with no live cluster). A doc comment in redis_string.go points back to the live source so the next renamer is reminded to update both.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![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 Redis simple-type encoders for strings, HyperLogLogs, and TTL records, featuring atomic file operations and sidecar file management. The review feedback identifies several improvement opportunities: implementing bounds on memory usage for pending TTL records to prevent OOM, parameterizing hardcoded database indices to align with documentation, optimizing directory creation to reduce redundant system calls, and expanding retriable error checks to include disk-full scenarios.
| r.pendingWideColumnTTL = append(r.pendingWideColumnTTL, redisTTLPending{ | ||
| UserKey: bytes.Clone(userKey), | ||
| ExpireAtMs: expireAtMs, | ||
| }) |
There was a problem hiding this comment.
The pendingWideColumnTTL slice grows unbounded as it accumulates TTL records for keys not yet claimed by string or HLL handlers. To prevent potential OOM issues, apply a fixed bound to this collection and reject new requests when the bound is reached, as per the repository's memory management guidelines.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests. Reject new requests when the bound is reached.
|
|
||
| func (r *RedisDB) writeBlob(subdir string, userKey, value []byte) error { | ||
| encoded := EncodeSegment(userKey) | ||
| dir := filepath.Join(r.outRoot, "redis", "db_0", subdir) |
There was a problem hiding this comment.
The database index db_0 is hardcoded here and in appendTTL (line 230). This contradicts the documentation stating that RedisDB encodes one logical database and that the caller is responsible for choosing the index. This hardcoding will cause collisions if multiple RedisDB instances are used for different databases within the same outRoot. Consider passing the database index as a parameter to NewRedisDB and using it to construct the path.
| func (r *RedisDB) writeBlob(subdir string, userKey, value []byte) error { | ||
| encoded := EncodeSegment(userKey) | ||
| dir := filepath.Join(r.outRoot, "redis", "db_0", subdir) | ||
| if err := os.MkdirAll(dir, 0o755); err != nil { //nolint:mnd // 0755 == standard dir mode |
There was a problem hiding this comment.
Calling os.MkdirAll for every blob write is inefficient as it performs multiple system calls to verify the directory hierarchy. Since the subdirectories ('strings', 'hll') are fixed for a given RedisDB instance, consider caching the fact that they have been created (e.g., using a map[string]struct{}) to avoid redundant I/O on the hot path.
| } | ||
| // errors.Is handles wrapped paths; both sentinel checks are stable | ||
| // for now because we never wrap them ourselves. | ||
| return errors.Is(err, io.ErrShortWrite) |
There was a problem hiding this comment.
Consider including syscall.ENOSPC (disk full) in the check for retriable errors. While io.ErrShortWrite is checked, os.File.Write typically returns a concrete system error like ENOSPC when the disk is full, which is a common scenario for backup operations that might be retriable after freeing up space.
…est' into feat/backup-phase0a-redis-simple
All four Gemini findings addressed. #189 -- pendingWideColumnTTL slice bounded. The orphan-TTL buffer is now capped at maxPendingWideColumnTTL = 1,000,000 entries (~50 MiB). Records past the cap are dropped and the warn sink reports the count at Finalize. Real production state (where wide-column type encoders eventually claim every TTL) is far under the cap; the bound only protects against malformed or adversarial snapshots. #217 -- db_0 hardcoding fixed. NewRedisDB now takes a dbIndex parameter; the per-encoder root "<outRoot>/redis/db_<idx>/" is computed from it. Two encoders with the same outRoot but different indices no longer collide. TestRedisDB_PerDBIndexRoutesIntoOwnDirectory locks in the distinction. #218 -- MkdirAll cached. Added dirsCreated map[string]struct{} on RedisDB. ensureDir() checks the map before MkdirAll so repeated writes (one per blob record) collapse to a map lookup. For a 10M-key dump this saves ~10M stat+mkdir(EEXIST) round-trips. TestRedisDB_DirsCreatedCachesMkdirAll asserts the cache is populated exactly once per directory. #382 -- ENOSPC handling. Added IsBlobAtomicWriteOutOfSpace as the explicit ENOSPC probe. IsBlobAtomicWriteRetriable continues to report only io.ErrShortWrite as retriable -- ENOSPC is intentionally NOT retriable (a backup against a full disk should surface to the operator rather than spin). The two-function split lets the master pipeline render the right alarm message.
|
All four Gemini MEDIUM items addressed in #189 — #217 — Hardcoded #218 — #382 — ENOSPC @claude review |
|
Claude finished @bootjp's task in 3m 18s —— View job Review of PR #713 (after
|
| Item | Verdict |
|---|---|
| #189 pendingWideColumnTTL OOM | ✅ Cap implemented (maxPendingWideColumnTTL = 1_000_000); documented rationale is solid |
#217 hardcoded db_0 |
✅ NewRedisDB(outRoot, dbIndex int) + per-index path + collision test |
| #218 MkdirAll per blob | ✅ dirsCreated map[string]struct{} + ensureDir short-circuit + cache test |
| #382 ENOSPC | ✅ Separated into IsBlobAtomicWriteOutOfSpace (not retriable, explicit probe) — the two-function split and the "why not retriable" comment are well-reasoned |
The constant cross-check against adapter/redis_compat_types.go also confirms the re-defined magic bytes are byte-for-byte identical to the live encoder.
Additional Findings
Medium
1. Dropped-TTL count in Finalize warning is inaccurate when the cap is hit (redis_string.go:242)
r.warn("redis_orphan_ttl",
"count", len(r.pendingWideColumnTTL), // always ≤ 1_000_000
...When 2M orphan TTLs arrive, the warning reports count: 1000000, silently hiding that another 1M were dropped. An operator reading that log would assume exactly 1M orphans are pending — not 2M. A droppedWideColumnTTL int counter incremented inside the else branch of HandleTTL's cap check would let Finalize report "dropped", r.droppedWideColumnTTL separately.
2. TestRedisDB_PendingWideColumnTTLBounded doesn't exercise the cap boundary (redis_string_test.go:312)
The test drives 10,000 entries (1% of the 1M cap) and asserts they all landed, then explicitly skips testing the drop behavior: "a 1M-record stress test in CI would be wasteful." But the critical path — the if len < maxPendingWideColumnTTL branch that drops entries — is never executed by any test. This means the cap check is untested dead code from coverage's perspective.
A straightforward fix: add a package-internal helper (or export a WithMaxPendingTTL(n int) option) that lets tests inject a lower cap, then verify that entry n+1 is dropped and len(db.pendingWideColumnTTL) == n. Alternatively, the test could drive exactly maxPendingWideColumnTTL + 1 entries in a goroutine-parallel fashion if 1M+1 iterations are too slow for CI.
Low / Nits
3. writeFileAtomic lacks fsync (redis_string.go:430)
tmp.Write → tmp.Close() → os.Rename() is not crash-durable on POSIX without an fsync between write and close. On a power failure after the rename but before the kernel flushes the page, the renamed file can contain zeros. For an offline rerunnable backup tool this may be an acceptable tradeoff (simply rerun), but it should be a deliberate, documented choice, not accidental. A //nolint:wrapcheck comment noting "no fsync by design — backup is idempotent; rerun on crash" costs one line.
4. Second closeJSONL error silently swallowed in Finalize (redis_string.go:236)
if err := closeJSONL(r.stringsTTL); err != nil && firstErr == nil {
firstErr = err
}
if err := closeJSONL(r.hllTTL); err != nil && firstErr == nil {
firstErr = err
}If both flush/close fail (possible on NFS under heavy I/O), only the strings error is returned. The HLL sidecar error is silently swallowed and the caller has no way to know its TTL sidecar is corrupt/incomplete. Consider errors.Join or at minimum a second warn-sink call for the secondary error.
5. Ordering assumption undocumented in HandleTTL (redis_string.go:203)
The PR description explains that Pebble lex order guarantees hll < str < ttl, so HandleTTL is always called after the corresponding HandleString/HandleHLL. But this invariant is not stated in the doc comment for HandleTTL itself. A caller who doesn't read the PR would have no warning that out-of-order calls silently promote the TTL to pendingWideColumnTTL. Worth one sentence in the doc comment: "Callers must have already dispatched the corresponding Handle{String,HLL} record — guaranteed by Pebble key order; see PR description."
6. Double-clamping of dbIndex (redis_string.go:148 and redis_string.go:258)
NewRedisDB already clamps dbIndex < 0 to 0, and redisDBSegment also clamps it again. The second clamp is unreachable for any caller going through NewRedisDB. It's harmless but creates a confusing impression that dbIndex might escape the clamp somewhere.
Pass Summaries
- Data loss — Atomic write (tmp+rename) is sound; symlink guard is correct; deferred tmp cleanup is correct. The missing
fsyncis the one unresolved gap (see Update docker/setup-qemu-action action to v3 #3 above). - Concurrency — Package is intentionally single-threaded; documented and appropriate.
- Performance —
ensureDircaching eliminates hot-pathstatcalls;kindByKeymap allocation is proportional and bounded;intToDecimalavoidsstrconvimport on the hot path. No regressions. - Data consistency — Magic-byte constants verified byte-for-byte against
adapter/redis_compat_types.go;math.MaxInt64clamp mirrors the live decoder; TTL sidecar writes mirror the snapshot record ordering contract. - Test coverage — 15 tests, all passing per CI. Coverage gap: the cap-boundary drop path in
HandleTTLis exercised by zero tests (see Update dependency go to v1.21.4 #2 above).
Overall: The four Gemini items are cleanly resolved; the code is well-structured and safe for its stated scope. Items #1 and #2 are worth a follow-up — the dropped-TTL count accuracy and the untested cap-boundary path are the two gaps that could mislead an operator or hide a future regression in the cap logic.
| Branch
…est' into feat/backup-phase0a-redis-simple
![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: 84a1c5a3e0
ℹ️ 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 redisKindString: | ||
| return r.appendTTL(&r.stringsTTL, redisStringsTTLFile, userKey, expireAtMs) |
There was a problem hiding this comment.
This redisKindString TTL routing appends every !redis|ttl| record to strings_ttl.jsonl, but HandleString already extracts inline TTL from new-format string values and writes that same key to the same sidecar. In current Redis writes, string commits emit both encoded string TTL and a !redis|ttl| scan-index entry (see adapter/redis_lua_context.go stringCommitElems), so expiring string keys will be duplicated in the sidecar, violating one-record-per-key assumptions and creating ambiguous restore input.
Useful? React with 👍 / 👎.
| if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { //nolint:mnd // 0755 == standard dir mode | ||
| return nil, cockroachdberr.WithStack(err) | ||
| } | ||
| f, err := os.Create(path) //nolint:gosec // path is composed from output-root + fixed file name |
There was a problem hiding this comment.
Creating TTL sidecars with os.Create(path) follows symlinks, so if strings_ttl.jsonl or hll_ttl.jsonl is a symlink in the output tree, backup generation will truncate and overwrite the symlink target outside the intended directory. Blob writes already defend against this (writeFileAtomic refuses symlink overwrites), so sidecar creation currently bypasses the same safety boundary.
Useful? React with 👍 / 👎.
Codex round 5 raised two correctness issues: 1. P1: !redis|ttl| scan-index records duplicated TTL for new-format strings. The live Redis encoder writes both !redis|str|<k> (with TTL embedded inline in the magic-prefix header) and the scan-index !redis|ttl|<k> for every expiring string (see adapter/redis_lua_context.go stringCommitElems). HandleString wrote strings_ttl.jsonl from the inline TTL; HandleTTL then routed the redundant !redis|ttl| record to the same sidecar, violating the one-record-per-key contract sidecar consumers rely on. Track inline-emitted keys in a new inlineTTLEmitted set and drop the scan-index TTL when the same key was already covered by the inline header. Legacy (no-magic-prefix) strings still flow through the !redis|ttl| path because their TTL only lives there. 2. P2: openJSONL followed symlinks. writeFileAtomic already refuses to clobber a symlink at a blob path; openJSONL did not. A strings_ttl.jsonl or hll_ttl.jsonl symlink in the output tree would otherwise be followed by os.Create, truncating a target outside the dump tree. Mirror the writeFileAtomic Lstat guard. Tests: TestRedisDB_NewFormatStringTTLNotDuplicatedByScanIndex, TestRedisDB_OpenJSONLRefusesSymlinkOverwrite.
|
@claude review Round 2 fixes for Codex round 5 on commit 84a1c5a:
Tests: |
|
Claude finished @bootjp's task in 3m 0s —— View job Review of PR #713 — Round 2 (commit
|
| Case | inlineTTLEmitted set? |
HandleTTL behaviour |
|---|---|---|
| New-format string WITH inline TTL | ✅ Yes, in HandleString |
Skips appendTTL, drops redundant scan-index record |
| New-format string WITHOUT TTL | No (early return at expireAtMs==0) |
No !redis|ttl| record exists in snapshot — nothing to call |
| Legacy string (no magic prefix) | No (decodeRedisStringValue returns expireAtMs=0) |
Falls through to appendTTL — correct, TTL lives only in the scan-index |
The one-record-per-key invariant is preserved for all three cases. TestRedisDB_NewFormatStringTTLNotDuplicatedByScanIndex asserts exactly 1 record in the sidecar after a HandleString + HandleTTL sequence. ✓
P2 — openJSONL now refuses symlinks (redis_string.go:408)
The Lstat-before-Create guard mirrors writeFileAtomic exactly. Both have the same TOCTOU window, but for an offline backup tool running in a controlled output directory, this is an accepted and consistent tradeoff. TestRedisDB_OpenJSONLRefusesSymlinkOverwrite verifies the bait file is untouched and the error message is correct. ✓
Pass Summaries
-
Data loss — Atomic write semantics unchanged; the new
inlineTTLEmittedmap is write-only within a key's lifetime and does not affect flush/close paths. No new data-loss vectors. The prior noted issues (missingfsync, secondcloseJSONLerror swallowed inFinalize) remain but are pre-existing nits. -
Concurrency — Single-threaded by design and documented;
inlineTTLEmittedaccess is sequential. No issues. -
Performance —
inlineTTLEmittedgrows proportional to expiring strings, which is inherent and bounded by the actual keyspace. The map lookup inHandleTTLis O(1) on the already-hot path. No regressions. -
Data consistency — The deduplication logic is correct for all three string formats. Legacy strings correctly bypass
inlineTTLEmittedsincedecodeRedisStringValuereturnsexpireAtMs=0for non-magic-prefix values, leavinginlineTTLEmittedunpopulated. ✓ -
Test coverage — 17 tests now. Both regression tests exercise the correct boundary: P1 tests the sidecar count after a real duplicate sequence; P2 confirms the bait file is untouched and the error message matches.
Residual Items (Carried from Round 1 — Not Blocking)
Two medium items from the previous review pass remain open:
1. Dropped-TTL count understated when cap is hit (redis_string.go:250)
When the pendingWideColumnTTL cap (1_000_000) is hit, Finalize warns with "count", len(r.pendingWideColumnTTL) — which is always ≤ 1_000_000. If 2M orphan TTLs arrived, the operator sees count: 1000000 with no indication that another 1M were silently dropped. A droppedWideColumnTTL int counter (incremented in the else branch of the cap check) would let Finalize report "dropped", r.droppedWideColumnTTL separately.
2. TestRedisDB_PendingWideColumnTTLBounded does not exercise the drop path (redis_string_test.go)
The test drives 10K entries (1% of the 1M cap) and verifies they all land. The else branch (the actual drop) is executed by zero tests, so it's untested dead code from coverage's perspective. Adding a package-level var maxPendingWideColumnTTL = 1_000_000 instead of a const (or a withMaxPendingTTL option on RedisDB) would let tests inject a tiny cap and verify entry n+1 is dropped and len(db.pendingWideColumnTTL) == n.
Overall: The two Codex round-5 issues are cleanly resolved with correct logic and matching regression tests. Ready to merge pending the two medium carry-overs above (or deferring them to a follow-up alongside the wide-column encoders, when the cap becomes load-bearing).
| Branch
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c699c15ca5
ℹ️ 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".
| if info, err := os.Lstat(path); err == nil && info.Mode()&os.ModeSymlink != 0 { | ||
| return nil, cockroachdberr.WithStack(cockroachdberr.Newf("backup: refusing to overwrite symlink at %s", path)) | ||
| } | ||
| f, err := os.Create(path) //nolint:gosec // path is composed from output-root + fixed file name |
There was a problem hiding this comment.
The os.Lstat check is separated from os.Create, so a process that can write the output directory can swap strings_ttl.jsonl/hll_ttl.jsonl to a symlink between those calls and still get the symlink target truncated. This reintroduces the overwrite primitive the guard is trying to prevent in concurrent or adversarial local environments; the open needs to be atomic with no-follow semantics (or written via temp+rename) to actually enforce the boundary.
Useful? React with 👍 / 👎.
| // the warning sink at Finalize. | ||
| if len(r.pendingWideColumnTTL) < maxPendingWideColumnTTL { | ||
| r.pendingWideColumnTTL = append(r.pendingWideColumnTTL, redisTTLPending{ | ||
| UserKey: bytes.Clone(userKey), |
There was a problem hiding this comment.
HandleTTL clones every unknown-key TTL into pendingWideColumnTTL, but Finalize only reads len(...) for warning output and never consumes the stored key/expiry payloads. On snapshots with many unsupported wide-column TTLs (especially with large user keys), this can allocate large, unnecessary memory and defeat the intended OOM protection despite the record-count cap.
Useful? React with 👍 / 👎.
…est' into feat/backup-phase0a-redis-simple
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c70e83da4
ℹ️ 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".
| } | ||
|
|
||
| func (r *RedisDB) writeBlob(subdir string, userKey, value []byte) error { | ||
| encoded := EncodeSegment(userKey) |
There was a problem hiding this comment.
writeBlob stores filenames from EncodeSegment(userKey), but this code never records KEYMAP entries when EncodeSegment takes its SHA-fallback path for long keys. For Redis keys whose encoded segment is non-reversible (>240-byte or promoted fallback cases), restore tooling cannot recover the original key bytes from *.bin/TTL metadata alone, causing irreversible key-name loss for affected records.
Useful? React with 👍 / 👎.
| if !isNewRedisStrFormat(value) { | ||
| return value, 0, nil | ||
| } |
There was a problem hiding this comment.
The new-format probe only checks the first two bytes (0xFF,0x01), so any legacy raw string value that happens to start with that prefix is parsed as encoded format and has bytes stripped (or can trigger a false TTL parse). This silently corrupts restored values for binary-safe legacy Redis strings matching that prefix pattern; the decoder needs a stricter discriminator than two-byte magic alone.
Useful? React with 👍 / 👎.
Codex round 6 raised two issues on commit 1c70e83: 1. P1: openJSONL had a TOCTOU window between Lstat and os.Create. A process that can write the output directory could swap the sidecar path to a symlink between the two syscalls and still get the target truncated, defeating the round-5 guard. Replace the two-step pattern with a single os.OpenFile call carrying O_WRONLY|O_CREATE|O_TRUNC|O_NOFOLLOW so the open itself refuses symlinks atomically; on Linux/macOS/BSD this returns ELOOP, on Windows O_NOFOLLOW is 0 (matches the different filesystem-permission model). Wrap the syscall error with the same "refusing to overwrite symlink" message so the existing test's substring assertion still holds. 2. P2: HandleTTL was buffering each orphan TTL record's full user key into pendingWideColumnTTL, but Finalize only ever read len(...) for the warning sink. With user keys up to 1 MiB each (store/mvcc_store.go maxSnapshotKeySize), this could allocate gigabytes for no benefit. Replace the slice with a plain `orphanTTLCount int`. The wide-column-encoder PR will re-introduce per-key tracking when those encoders actually consume the keys. Tests: - TestRedisDB_OpenJSONLRefusesSymlinkOverwrite — comment updated to reflect the atomic open semantics. - TestRedisDB_OrphanTTLCountedNotBuffered — replaces TestRedisDB_PendingWideColumnTTLBounded; asserts the count field instead of slice length.
|
@claude review Round 3 fixes for Codex round 6 on commit 1c70e83:
|
|
Claude finished @bootjp's task in 4m 1s —— View job Review of PR #713 — Round 3 (commit
|
…nd 4) Codex P1 round 7 (commit d965b65): `writeBlob` and `appendTTL` were storing user keys as `EncodeSegment(userKey)` filenames / JSONL row keys, but never recording a KEYMAP.jsonl entry when EncodeSegment took its SHA-fallback path. For Redis keys longer than the 240-byte ceiling (or those whose percent-encoded form was promoted to the fallback to disambiguate from the binary `b64.` prefix or the SHA-fallback shape), the encoded segment is non-reversible — so without a KEYMAP record, the original Redis user key bytes were lost in the backup output. Wire a lazy-opened, atomic-no-follow `KEYMAP.jsonl` writer into `RedisDB`. Both `writeBlob` and `appendTTL` now call `recordIfFallback(encoded, userKey)` which writes a `KindSHAFallback` record on first encounter. `closeKeymap` (called from Finalize) flushes + closes the file; if no fallback records were ever emitted, the empty file is removed so dumps without any non-reversible keys carry no spurious file (matches the s3 encoder's policy). Tests: - TestRedisDB_SHAFallbackKeymapped — drives a 300-byte key, asserts KEYMAP.jsonl exists and round-trips back to the original bytes. - TestRedisDB_NoKeymapWhenAllReversible — short-key dump produces no KEYMAP file.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d965b6598c
ℹ️ 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".
| // kernel supports it (Linux, macOS, BSD) this returns ELOOP for | ||
| // a symlink path. On Windows syscall.O_NOFOLLOW is 0 (no-op), | ||
| // matching Windows's different filesystem-permission model. | ||
| const flag = os.O_WRONLY | os.O_CREATE | os.O_TRUNC | syscall.O_NOFOLLOW |
There was a problem hiding this comment.
openJSONL is compiled for all targets but unconditionally references syscall.O_NOFOLLOW; this symbol is not defined on Windows, so the package no longer builds there (reproduced with GOOS=windows GOARCH=amd64 go test ./internal/backup -run TestDoesNotExist, which fails at this line). This introduces a cross-platform build regression and conflicts with the surrounding comment that implies Windows compatibility.
Useful? React with 👍 / 👎.
|
@claude review Round 4 fixes for Codex round 7 on commit d965b65:
|
|
Claude finished @bootjp's task in 4m 21s —— View job Review of PR #713 — Round 4 (commit
|
| Path | recordIfFallback called |
KEYMAP record emitted |
|---|---|---|
| Short key, no TTL | ✓ (IsShaFallback → false, returns nil) | No |
| Short key, with TTL | ✓ (IsShaFallback → false, returns nil) | No |
| Long key, no TTL | ✓ from writeBlob only |
1 record |
| Long key, with inline TTL | ✓ from writeBlob + appendTTL (via HandleString) |
2 records (same mapping) |
| HLL long key + TTL | ✓ from writeBlob + appendTTL (via HandleTTL) |
2 records (same mapping) |
The lazy-open with O_NOFOLLOW mirrors openJSONL exactly. closeKeymap's empty-file removal is correct — r.keymap.Count() is only 0 when every WriteOriginal call failed (which means the caller got an error and aborted before Finalize). LoadKeymap's "last record wins" semantics are verified: out[rec.Encoded] = rec — duplicate records with the same key produce the same mapping. ✓
P1 — FF01 ambiguity (deferred): ✓ Rationale is sound
The argument holds: the backup decoder must produce bit-for-bit identical output to what the live cluster returns for a given stored value. Changing the two-byte discriminator in the backup decoder without changing the live encoder would cause backup→restore round-trips to diverge from the live cluster's behavior — exactly the wrong invariant. A coordinated wire-format change is the right path, and a follow-up design doc is the right vehicle.
P2 round 7 — O_NOFOLLOW Windows build (not addressed in Round 4)
Codex raised that syscall.O_NOFOLLOW may not be defined on Windows and that openJSONL would break Windows builds. The Round 3 review accepted the existing openJSONL pattern based on the comment "On Windows syscall.O_NOFOLLOW is 0 (no-op)." Round 4 adds recordIfFallback with the identical pattern. This comment needs verification — if syscall.O_NOFOLLOW is genuinely absent on Windows rather than just 0, both openJSONL and recordIfFallback would fail to compile there. A one-line cross-compile check (GOOS=windows GOARCH=amd64 go build ./internal/backup/) would settle it. If the symbol is absent, the standard fix is a build-tagged shim (backup_nofollow_unix.go + backup_nofollow_other.go) or using golang.org/x/sys/unix.O_NOFOLLOW behind a build tag.
Additional Findings
Low
1. Duplicate KEYMAP records for expiring SHA-fallback keys (redis_string.go:317, redis_string.go:340)
For any expiring SHA-fallback string key, HandleString calls writeBlob → recordIfFallback (record #1) then appendTTL → recordIfFallback (record #2). Both records carry identical encoded and original bytes, so LoadKeymap sees them as the same entry. Correctness is not affected, but KEYMAP.jsonl is 2× larger than necessary for such keys. For a dump with 1M expiring long-key strings this doubles the KEYMAP file size.
A seenFallbacks map[string]struct{} on RedisDB (checked inside recordIfFallback before calling WriteOriginal) would eliminate the duplicates. Not blocking — just a space inefficiency.
2. closeKeymap swallows the keymapFile.Close() error when flush also fails (redis_string.go:393)
flushErr := r.keymap.Close()
closeErr := r.keymapFile.Close()
if flushErr == nil && closeErr != nil {
flushErr = closeErr
}If both flush and close fail (e.g., NFS under heavy I/O), only flushErr is returned. This is consistent with the pre-existing closeJSONL first-error pattern carried through the whole file, so it's not a new regression — just noting it for completeness.
Pass Summaries
- Data loss —
recordIfFallbackcorrectly emits KEYMAP records for both blob and TTL paths.O_TRUNCon lazy open is correct (fresh dump semantics).closeKeymap's empty-file removal is safe. No new data-loss vectors. - Concurrency — Single-threaded by design.
recordIfFallbacklazy-init ofr.keymapis sequential. No issues. - Performance —
IsShaFallbackis O(34) string prefix check on every blob/TTL write — negligible. The duplicate-record concern (finding Configure Renovate #1) is a space, not time, cost. - Data consistency —
LoadKeymaplast-record-wins confirmed atkeymap.go:238. Duplicate records for expiring fallback keys produce the same mapping.O_TRUNCensures a clean KEYMAP for each dump session. ✓ - Test coverage — 19 tests total.
TestRedisDB_SHAFallbackKeymappedexercises bothwriteBlobandappendTTLpaths becauseHandleStringwith inline TTL calls both. The round-trip viaLoadKeymap+rec.Original()is thorough.TestRedisDB_NoKeymapWhenAllReversiblecovers the no-fallback path. ✓
Overall: The SHA-fallback KEYMAP fix is cleanly implemented and its tests are solid. The FF01 deferral is well-argued. The only open item from Codex round 7 is the Windows build question — a quick GOOS=windows go build verifies or refutes it with one command. If it compiles, close the item; if not, a two-file build-tag shim resolves it in minutes.
| Branch
…d 5) Codex P2 round 7 (commit d965b65): `syscall.O_NOFOLLOW` is not defined on Windows, so the round-3 fix that used it directly broke the cross-platform build (`GOOS=windows GOARCH=amd64 go build ./internal/backup` failed at `undefined: syscall.O_NOFOLLOW`). Extract the platform-specific open into a small build-tagged helper: - open_nofollow_unix.go (`//go:build unix`) keeps the atomic O_NOFOLLOW open with ELOOP wrapping. - open_nofollow_windows.go (`//go:build windows`) uses Lstat then OpenFile. The residual TOCTOU window is acceptable on Windows because mounting a successful attack on the dump tree there already requires write access to the output directory plus SeCreateSymbolicLinkPrivilege — a much higher bar than the unix case the round-3 fix was hardening against. Both `openJSONL` and the keymap-creation path in `recordIfFallback` now go through `openSidecarFile`, eliminating the duplicate inline OpenFile blocks. Verified: `go test -race` passes on Linux/macOS; `GOOS=windows GOARCH=amd64 go build ./internal/backup/...` now succeeds (was the Codex repro).
|
@claude review Round 5 fix for Codex P2 round 7 on commit d965b65 (commit
Extracted the per-platform open into a build-tagged helper
Both |
|
Claude finished @bootjp's task in 3m 26s —— View job Review of PR #713 — Round 5 (commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f35446404
ℹ️ 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 flag = os.O_WRONLY | os.O_CREATE | os.O_TRUNC | syscall.O_NOFOLLOW | ||
| f, err := os.OpenFile(path, flag, 0o600) //nolint:gosec,mnd // path is composed from output-root + fixed file name; 0600 is the standard owner-only mode |
There was a problem hiding this comment.
openSidecarFile now blocks symlinks, but it still opens existing files with O_TRUNC, which will truncate the underlying inode even when the path is a hard link. In a local-adversary scenario where another process can write the dump directory, it can pre-create strings_ttl.jsonl/hll_ttl.jsonl as a hard link to another writable file outside the backup tree, and this call will clobber that external file despite the symlink guard.
Useful? React with 👍 / 👎.
…est' into feat/backup-phase0a-redis-simple
Codex P2 round 9 (commit 87971ae): The round-5 unix sidecar opener used O_NOFOLLOW so the kernel refuses symlinks atomically, but it kept O_TRUNC on the same call. A local adversary who can write the output directory could pre-create strings_ttl.jsonl / hll_ttl.jsonl / KEYMAP.jsonl as a hard link to a writable file outside the dump tree (e.g. /etc/passwd) and the open's O_TRUNC would zero that inode despite the symlink guard. Restructure the unix open: 1. Open with O_WRONLY|O_CREATE|O_NOFOLLOW (no O_TRUNC). 2. fstat() the descriptor. 3. If sysStat.Nlink > 1, refuse with a wrapped error and close. 4. Otherwise call f.Truncate(0) — atomic with the file we just verified is single-linked. The Windows build (open_nofollow_windows.go) is unchanged because its symlink/hard-link attack model is materially different (SeCreateSymbolicLinkPrivilege required, ACLs gate the directory) and the platform's link-count semantics under fstat are nontrivial. Test: - TestRedisDB_OpenJSONLRefusesHardLinkClobber — pre-creates strings_ttl.jsonl as a hard link to a bait file, exercises HandleString (which lazy-opens the sidecar), asserts the "refusing to overwrite hard-linked file" error and that the bait file is untouched.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@claude review Round 6 fix for Codex P2 round 9 on commit 87971ae (commit The round-5 unix opener kept Restructured the unix open:
The Windows build is unchanged because the equivalent attack model is materially different there ( Test:
|
|
Claude finished @bootjp's task in 3m 20s —— View job Review of PR #713 — Round 6 (commit
|
| Step | What it does | Why |
|---|---|---|
O_WRONLY|O_CREATE|O_NOFOLLOW (no O_TRUNC) |
Opens the file descriptor without truncating | Preserves old content until we've verified the link count |
f.Stat() → *syscall.Stat_t |
fstat(2) on the already-open fd |
Path-independent — cannot be raced by a rename |
sysStat.Nlink > 1 → refuse + close |
Rejects adversarially hard-linked targets | An inode with nlink > 1 exists outside our dump tree |
f.Truncate(0) |
Empties the verified-single-link file | Equivalent to O_TRUNC but only after the nlink check |
Why writeFileAtomic doesn't need the same treatment: rename(2) atomically replaces the target directory entry without truncating the old inode. If path is a hard link to /etc/passwd, the Rename merely detaches the hard link from path — the original inode stays intact and nlink decreases by one. The blob write path is therefore safe from hard-link attacks by construction; the sidecar open is the only path that truncates via O_TRUNC-on-fd (before this fix).
File-offset correctness: os.OpenFile without O_APPEND starts the file offset at 0. Truncate(0) does not change the offset. The bufio writer wrapping the returned fd will write from offset 0 into the now-empty file. Correct for both new files (O_CREATE on a non-existent path, offset starts at 0) and pre-existing files from a previous run (offset 0, truncated to 0 before any write).
Test: TestRedisDB_OpenJSONLRefusesHardLinkClobber pre-creates strings_ttl.jsonl as a hard link to a bait file, drives HandleString (which lazy-opens the sidecar via appendTTL → openJSONL → openSidecarFile), asserts the error message, and asserts the bait file is untouched. ✓
Additional Findings
Medium (Carried from Round 5 — Not Yet Addressed)
1. open_nofollow_windows.go returns a bare OS error (open_nofollow_windows.go:25)
// Windows path — no cockroachdberr.WithStack wrapping
return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600)The Unix counterpart wraps every OS error with cockroachdberr.WithStack; the Windows variant returns os.OpenFile's error bare. The wrapcheck linter enforces wrapping at all OS/package boundaries in this repo — the Windows file currently escapes lint only because CI runs on Linux. If a Windows CI step or a local GOOS=windows golangci-lint run is ever added, this line will be flagged.
Low / Nits
2. Sys() type-assertion failure silently skips the Nlink check (open_nofollow_unix.go:48)
if sysStat, ok := info.Sys().(*syscall.Stat_t); ok && sysStat.Nlink > 1 {On all realistic //go:build unix targets (Linux, Darwin, BSD), os.File.Stat().Sys() reliably returns *syscall.Stat_t. The two-value form is defensive coding for hypothetical future platforms — but the silently-pass-through semantics (skip the check if the assertion fails) would defeat the guard on such a platform. A brief inline comment (// Sys() always returns *syscall.Stat_t on realistic Unix targets; the ok-assert is defensive) would make the intent explicit and prevent a future reader from "simplifying" the check into a direct assertion.
3. TestRedisDB_OpenJSONLRefusesHardLinkClobber has no t.Skip for non-Unix (redis_string_test.go:205)
The test calls os.Link (hard-link creation), which is unavailable or fails silently on some filesystems (overlay, NFS, FAT). On a tmpfs-mounted t.TempDir() (the norm in Linux CI), this is fine. But a short if os.Getenv("GOOS") == "windows" guard (or checking os.Link's error and skipping on ENOSYS / ErrInvalid) would make the intent explicit and prevent a false failure if CI ever mounts temp dirs on a no-hardlink filesystem.
Pass Summaries
- Data loss — Three-step open (no O_TRUNC, fstat, Truncate) correctly prevents hard-link clobber. File offset after Truncate(0) is at 0 — no stale data can appear in the output.
writeFileAtomicis unaffected (Rename atomically replaces the directory entry without truncating the old inode). No new data-loss vectors. - Concurrency —
fstatoperates on the open file descriptor (path-independent), so a concurrent rename cannot race the Nlink check. The remaining TOCTOU window between Nlink check and Truncate is not exploitable for data capture (Truncate only zeroes the file, no backup data is written through it). Single-threaded by design. - Performance — One extra
fstatsyscall per sidecar open; sidecars are opened once per db encoder. Negligible. - Data consistency — Magic-byte constants unchanged. Deduplication logic via
inlineTTLEmitted, KEYMAP emission viarecordIfFallback, and orphan-TTL counting all carry forward correctly. - Test coverage — 20 tests total.
TestRedisDB_OpenJSONLRefusesHardLinkClobbercovers the new Nlink-check path end-to-end. Thef.Stat()failure path (extreme kernel error) is the only untested path; it's not a realistic gap.
Overall: The hard-link clobber fix is correct and well-targeted. The openSidecarFile refactor is the minimum necessary change. The one remaining medium item is the unwrapped Windows error (carried from Round 5) — a one-line fix. The two low nits are cosmetic. Ready to merge pending the Windows wrap or explicit deferral.
| Branch
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87971aef96
ℹ️ 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".
| if _, ok := r.dirsCreated[dir]; ok { | ||
| return nil | ||
| } | ||
| if err := os.MkdirAll(dir, 0o755); err != nil { //nolint:mnd // 0755 == standard dir mode |
There was a problem hiding this comment.
The path hardening only checks the final file (os.Lstat(path) in writeFileAtomic and O_NOFOLLOW in openSidecarFile), but it never verifies parent directories, so a pre-existing symlink like redis/db_0/strings -> /tmp/outside causes HandleString to write blobs outside outRoot. This bypasses the intended “don’t follow links” safety boundary for both blob files and JSONL sidecars whenever an attacker (or accidental setup) can place directory symlinks in the output tree.
Useful? React with 👍 / 👎.
| if err == nil { | ||
| return false | ||
| } | ||
| return errors.Is(err, syscall.ENOSPC) |
There was a problem hiding this comment.
IsBlobAtomicWriteOutOfSpace only checks errors.Is(err, syscall.ENOSPC), which works on POSIX but misses Windows disk-full write failures (typically Win32 errno 112 / ERROR_DISK_FULL). On Windows this means real out-of-space errors can be reported as generic failures, so retry/alarm logic that depends on this helper won’t classify them correctly.
Useful? React with 👍 / 👎.
…d 7) Codex round 9 raised two issues on commit 87971ae: 1. P1: O_NOFOLLOW only blocks the trailing component. A pre-existing directory symlink anywhere up the path (e.g. `<outRoot>/redis/db_0/strings -> /tmp/outside`) let MkdirAll silently honor it, then writeFileAtomic / openSidecarFile wrote outside outRoot. ensureDir now Lstat-walks each ancestor under outRoot via assertNoSymlinkAncestors and refuses if any is a symlink. This is best-effort against TOCTOU (an adversary can race us at any boundary), but closes the much more common case of a stale or misconfigured symlink in the dump tree. Full race-free traversal would need os.Root / openat which is out of scope at this layer. 2. P2: IsBlobAtomicWriteOutOfSpace only checked syscall.ENOSPC, missing Windows disk-full failures (ERROR_DISK_FULL = 112, ERROR_HANDLE_DISK_FULL = 39). Retry/alarm logic that keys off this helper would silently misclassify Windows disk exhaustion. Move the platform-specific check into disk_full_unix.go and disk_full_windows.go; the latter accepts both Win32 codes plus ENOSPC for the Go-translated case. Tests: - TestRedisDB_RefusesSymlinkedAncestor: pre-places a `redis/db_0/strings -> /tmp/...` symlink before HandleString and asserts the ancestor refusal + that the bait dir is untouched. Verified: - go test -race ./internal/backup/... clean. - GOOS=windows GOARCH=amd64 go build ./internal/backup/... clean. - golangci-lint clean.
|
@claude review Round 7 fixes for Codex round 9 on commit 87971ae (commit
|
|
Claude finished @bootjp's task in 3m 9s —— View job Review of PR #713 — Round 7 (commit
|
| Step | What it does |
|---|---|
filepath.Rel(cleanRoot, cleanTarget) |
Computes relative path; escape-to-parent detected and refused |
lstatRefuseSymlink(cleanRoot) |
Checks the root itself first |
Loop over strings.Split(rel, sep) |
Walks each component, Lstat-checking each joined path |
if seg == "" { continue } |
Handles degenerate splits from filepath.Clean'd paths |
lstatRefuseSymlink's "return nil for non-existent path" is correct because ensureDir calls MkdirAll immediately before the check — any missing directory is impossible unless MkdirAll itself failed (and that failure is caught). The TOCTOU disclosure in the comment is the right acknowledgement.
TestRedisDB_RefusesSymlinkedAncestor pre-places <root>/redis/db_0/strings → bait-tree, drives HandleString, asserts the error message, and verifies the bait is untouched. ✓
P2 — Platform-specific isDiskFullError (disk_full_unix.go, disk_full_windows.go)
- Unix:
errors.Is(err, syscall.ENOSPC)— correct. - Windows: covers
windows.ERROR_DISK_FULL(112),windows.ERROR_HANDLE_DISK_FULL(39), andsyscall.ENOSPC(for Go's translated case). The three-caseswitchis clean and exhaustive. golang.org/x/sys v0.43.0is confirmed ingo.mod— no new dependency. ✓
TestRedisDB_IsBlobAtomicWriteOutOfSpace verifies that both a raw and errors.Join-wrapped ENOSPC round-trip through IsBlobAtomicWriteOutOfSpace. ✓
Residual Item (Carried from Round 5 — Not Yet Addressed)
Medium: open_nofollow_windows.go returns a bare OS error (open_nofollow_windows.go:25)
// Windows — the os.OpenFile error is not wrapped:
return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600)The repo's wrapcheck linter enforces cockroachdberr.WithStack at all OS/package boundaries. This file escapes the check only because Linux CI does not cross-compile for GOOS=windows. Any future GOOS=windows golangci-lint step will flag this. One-line fix:
Additional Findings
Low / Nits
1. TestRedisDB_RefusesSymlinkedAncestor tests leaf-level only (redis_string_test.go:209)
The test places the symlink trap at <root>/redis/db_0/strings (one level below dbDir()). The assertNoSymlinkAncestors loop covers all levels including <root>/redis and <root>/redis/db_0, but these higher-level cases are not exercised by the test. The logic is the same for any level (the same lstatRefuseSymlink call), so this is a coverage note rather than a bug.
2. openJSONL bypasses ensureDir for the JSONL parent directory (redis_string.go:530)
func openJSONL(path string) (*jsonlFile, error) {
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { // direct MkdirAll — no ancestor check
...
}openJSONL calls os.MkdirAll directly rather than routing through ensureDir. In practice this is safe because HandleString/HandleHLL always call writeBlob → ensureDir before any appendTTL invocation — the ancestor check for the JSONL parent directory (dbDir()) is implicitly covered by the blob ensureDir call for the sibling subdir (dbDir()/strings, dbDir()/hll). But this ordering invariant is implicit and undocumented in openJSONL. A one-line comment noting the dependency — or threading openJSONL through ensureDir for its parent — would make the contract explicit for a future refactor.
3. disk_full_windows.go Windows error codes are untestable from Linux CI (disk_full_windows.go)
windows.ERROR_DISK_FULL and windows.ERROR_HANDLE_DISK_FULL are not exercised by any test in this PR (inherent constraint: //go:build windows code is excluded from Linux CI). This is the same limitation as open_nofollow_windows.go. Acceptable for a backup tool targeting server environments; worth noting for completeness.
Pass Summaries
-
Data loss —
assertNoSymlinkAncestorsis called afterMkdirAllsucceeds, beforedirsCreatedis populated. Pre-existing ancestor symlinks are caught; newly swapped ones require the TOCTOU window (documented).openJSONL's directMkdirAllbypasses the ancestor check but is safe due to call ordering (see finding Update dependency go to v1.21.4 #2). No new data-loss vectors. -
Concurrency — Single-threaded by design.
assertNoSymlinkAncestorsandlstatRefuseSymlinkare pure functions with no shared state. No issues. -
Performance —
assertNoSymlinkAncestorsis called once per unique directory (gated bydirsCreatedcache), O(path depth) Lstat calls per directory. Negligible for a backup tool.isDiskFullErroris O(1). No regressions. -
Data consistency — Magic-byte constants, deduplication logic, KEYMAP emission, and orphan-TTL counting are all unchanged.
golang.org/x/sysconfirmed ingo.mod. ✓ -
Test coverage — 21 tests.
TestRedisDB_RefusesSymlinkedAncestorexercises the new ancestor-check path end-to-end.TestRedisDB_IsBlobAtomicWriteOutOfSpacecovers ENOSPC and wrapped ENOSPC. Windows disk-full codes are untested (inherent platform gap, acceptable).
Overall: The two Round 7 Codex items are correctly implemented and each has a dedicated regression test. The single remaining open item is the unwrapped Windows OS error from Round 5 — a one-line fix or explicit deferral is all that stands between this and merge-ready.
| Branch |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e061b6d088
ℹ️ 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".
| // mounting a successful attack on the dump tree there already | ||
| // requires write access plus SeCreateSymbolicLinkPrivilege. | ||
| // Codex P1 round 6 (atomic open) + P2 round 7 (Windows build). | ||
| f, err := openSidecarFile(path) |
There was a problem hiding this comment.
This change introduces calls to openSidecarFile and isDiskFullError, but both helpers are only implemented in *_unix.go and *_windows.go; targets that are neither (for example GOOS=js and GOOS=wasip1) now fail to compile with undefined symbol errors. I verified this by running GOOS=js GOARCH=wasm go test -c ./internal/backup and GOOS=wasip1 GOARCH=wasm go test -c ./internal/backup, both of which fail at these call sites, whereas the parent commit compiles for those targets.
Useful? React with 👍 / 👎.
Codex P2 round 10 (commit e061b6d): Round 5 + round 7 split openSidecarFile and isDiskFullError into unix and windows build-tagged files, but the union doesn't cover every Go target. GOOS=js GOARCH=wasm and GOOS=wasip1 GOARCH=wasm now fail with `undefined: openSidecarFile` / `undefined: isDiskFullError` at compile time, which Codex reproduced via `GOOS=js go test -c ./internal/backup`. Add `_other.go` files behind `//go:build !unix && !windows`: - openSidecarFile falls back to Lstat-then-OpenFile. Those targets either lack syscall.O_NOFOLLOW (wasm runtimes) or have a different filesystem model (plan9), and the local-adversary threat that motivated the unix nlink+atomic-open guard does not apply to offline / sandboxed dump tooling. - isDiskFullError returns false. wasm has no disk; plan9 uses a different error vocabulary. The helper's contract treats unrecognised errors as non-retriable, so false is the safe default. Verified: - GOOS=js GOARCH=wasm go build ./internal/backup/... clean. - GOOS=wasip1 GOARCH=wasm go build ./internal/backup/... clean. - GOOS=windows GOARCH=amd64 go build ./internal/backup/... clean. - go test -race ./internal/backup/... clean.
|
@claude review Round 8 fix for Codex P2 round 10 on commit e061b6d (commit The unix/windows split for Added
Verified: |
|
Claude finished @bootjp's task in 1m 54s —— View job Review of PR #713 — Round 8 (commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20236ac43b
ℹ️ 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 flag = os.O_WRONLY | os.O_CREATE | syscall.O_NOFOLLOW | ||
| f, err := os.OpenFile(path, flag, 0o600) //nolint:gosec,mnd // path is composed from output-root + fixed file name; 0600 is the standard owner-only mode |
There was a problem hiding this comment.
openSidecarFile opens the TTL sidecar with os.O_WRONLY|os.O_CREATE before checking what kind of file it is, so a pre-existing FIFO at strings_ttl.jsonl/hll_ttl.jsonl will block the backup indefinitely when no reader is attached. The current symlink and hard-link checks do not catch this case (mkfifo produces nlink=1 and is not a symlink), so a local adversary or stale artifact in the output directory can hang the first TTL write.
Useful? React with 👍 / 👎.
Codex P2 round 11 (commit 20236ac): `openSidecarFile` opened the TTL sidecar with `O_WRONLY|O_CREATE|O_NOFOLLOW` and only checked the resulting descriptor's link count. A pre-existing FIFO at strings_ttl.jsonl / hll_ttl.jsonl would block the first write indefinitely (POSIX: opening a reader-less FIFO with O_WRONLY hangs until a reader attaches). The symlink and hard-link guards do not catch this case: `mkfifo` produces nlink=1 and is not a symlink. Add O_NONBLOCK to the unix open so a reader-less FIFO surfaces ENXIO immediately instead of hanging, then enforce info.Mode().IsRegular() after fstat. Reader-attached FIFOs, sockets, and character/block devices are all refused with a stable "non-regular file" error. The Windows path is unchanged because Windows has no FIFO concept. Test: TestRedisDB_OpenJSONLRefusesFIFO lives in redis_string_unix_test.go behind `//go:build unix` because syscall.Mkfifo is undefined on Windows and js/wasm. Verified clean: linux race tests, GOOS=windows GOARCH=amd64 build, GOOS=js GOARCH=wasm build, golangci-lint.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@claude review Round 9 fix for Codex P2 round 11 on commit 20236ac (commit
Added Test: Verified clean: |
|
Claude finished @bootjp's task in 3m 47s —— View job Review of PR #713 — Round 9 (commit
|
| Step | What it catches |
|---|---|
| ELOOP on open | Symlinks |
| ENXIO on open | Reader-less FIFOs |
!IsRegular() after fstat |
Reader-attached FIFOs, sockets, devices |
Nlink > 1 |
Hard links to files outside dump tree |
Truncate(0) |
Only reached for verified single-link regular files |
All four threat vectors are now addressed with no gap between them. ✓
O_NONBLOCK effect on write path: O_NONBLOCK is retained on the returned *os.File fd, meaning subsequent buffered write(2) calls are technically non-blocking. For regular files on local block devices (ext4, XFS, Btrfs), the kernel never returns EAGAIN on write(2) — regular-file writes always go to the page cache. This is a no-op in practice and the IsRegular() check ensures only regular files are returned. ✓
Test: TestRedisDB_OpenJSONLRefusesFIFO (redis_string_unix_test.go:24)
The test creates a real FIFO via syscall.Mkfifo at the exact path openJSONL would open (redisStringsTTLFile), then drives HandleString with an expiring string (which triggers appendTTL → openJSONL). The error-message check accepts both "FIFO at" and "non-regular file" — correctly handling both the ENXIO path (reader-less) and the IsRegular path (reader-attached). The t.Skipf guard for platforms where mkfifo isn't available is the right defensive pattern. ✓
Residual Item (Carried from Round 5 — Still Open)
Medium: open_nofollow_windows.go and open_nofollow_other.go return bare os.OpenFile errors
Both files return the os.OpenFile result unwrapped:
// open_nofollow_windows.go:25 and open_nofollow_other.go:24
return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) //nolint:gosec,mndThe //nolint:gosec,mnd comment suppresses gosec and mnd but not wrapcheck. The .golangci.yaml (line 27) enables wrapcheck without a cross-platform build-tag exemption. Linux CI avoids the issue only because these files are excluded by their build tags. Any GOOS=windows golangci-lint run (local dev on Windows, future CI step) would flag both lines.
The unix counterpart correctly wraps on every error path with cockroachdberr.WithStack.
Fix open_nofollow_windows.go →
Pass Summaries
-
Data loss — The four-gate open sequence (ELOOP / ENXIO / IsRegular / Nlink) is exhaustive for the known threat vectors against sidecar files.
Truncate(0)is only reached for verified single-link regular files.writeFileAtomic(blob path) is unchanged. No new data-loss vectors. -
Concurrency — Single-threaded by design.
O_NONBLOCK+IsRegular()are syscall-level primitives with no race window between them (the fstat is on the open fd, not a re-stat of the path). The TOCTOU between the IsRegular check and Truncate is not exploitable by swapping the path (the fd is already open against the inode). No issues. -
Performance — One extra fstat-on-fd and one mode flag check per sidecar open; sidecars are opened once per db encoder. Negligible.
-
Data consistency — Magic-byte constants, deduplication, KEYMAP emission, and orphan-TTL counting are unchanged. The fix is purely in the sidecar file-open layer. ✓
-
Test coverage — 22 tests (21 in
redis_string_test.go+ 1 inredis_string_unix_test.go).TestRedisDB_OpenJSONLRefusesFIFOcovers the ENXIO path end-to-end. TheIsRegularpath for a reader-attached FIFO is not separately tested (would require a goroutine to hold the read end open), but the code is a two-line guard that directly follows the ENXIO block — the gap is minimal.
Overall: Round 9 is clean. The FIFO guard is correct, well-commented, and tested. The single remaining open item — the unwrapped error in windows.go / other.go — is a one-line fix in each file, and it's the same carry-over from Round 5. Either fix it now or defer with an explicit note; it's the last gap before this PR is clean across the board.
| Branch |
1 participant
Summary
Stacked on top of #712. Adds the first per-adapter encoder of the Phase 0 logical-backup decoder — covers the three Redis prefixes that always map ONE snapshot record to one user key:
!redis|str|<userKey>→strings/<encoded>.bin(+ inline-TTL stripped intostrings_ttl.jsonl). Both new magic-prefix format and legacy raw bytes accepted.!redis|hll|<userKey>→hll/<encoded>.bin(raw HLL sketch).!redis|ttl|<userKey>→ routed by prior type observation:redisKindString→strings_ttl.jsonl(legacy strings)redisKindHLL→hll_ttl.jsonlredisKindUnknown→ buffered for wide-column types; orphan warning atFinalize.Wide-column types (hash / list / set / zset / stream) ship in follow-up PRs.
How TTL routing works
Pebble lex order guarantees
!redis|hll|*<!redis|str|*<!redis|ttl|*(h < s < t). So whenHandleTTLis called, the type tracker already knows what kind of key the user-key belongs to. No two-pass needed.Safety
EncodeSegment(the encoding contract from backup: filename encoding (Phase 0a foundation) #711).writeFileAtomicrefuses to overwrite a symlink so a malicious or accidental link cannot redirect output bytes.ErrRedisInvalidStringValue,ErrRedisInvalidTTLValue) rather than silent data loss.0xFF 0x01magic constants local (not imported fromadapter/redis_compat_types.go) because the decoder is intentionally adapter-independent — must run as an offline tool with no live cluster. A doc comment points back to the live source so a future renamer updates both.Test plan
go test -race ./internal/backup/...— pass.golangci-lint run ./internal/backup/...— clean.HasInlineTTLhelper.Stacking
Base:
feat/backup-phase0a-keymap-manifest(PR #712).