backup: snapshot dispatcher (Phase 0a routing layer)#806
Conversation
Wires the four adapter encoders (DynamoDB, S3, Redis, SQS) under
a single DecodeSnapshot entrypoint that consumes a .fsm-format
stream and routes each entry by leading prefix into the encoder
that owns it.
- internal/backup/decode.go
- DecodeOptions + DecodeResult + DecodeCounters
- dispatcher with prefix-length-sorted routing table (longer
prefixes win so !hs|meta|d| beats !hs|meta|)
- tombstones dropped before adapter dispatch; disabled adapters
surface as Internal (not Unknown) so --adapter <subset> keeps
counts in the right bucket
- !txn|, !s3route|, !s3|gc|upload| routed to internal-drop
- internal/backup/snapshot_reader.go
- new ReadSnapshotWithHeader so header-only snapshots still
surface LastCommitTS to the dispatcher (the existing
callback-only ReadSnapshot never fires fn on an empty body,
leaving MANIFEST.last_commit_ts unpopulated)
- internal/backup/decode_test.go pins:
empty OutRoot rejected, header-only snapshot finalizes cleanly,
Redis string round-trip end-to-end (one entry in, file out),
tombstone skip, disabled-adapter -> Internal counter,
unknown prefix -> Unknown counter, !txn| -> Internal counter,
prefix routing table sorted by descending length, and
!hs|meta|d| matches before !hs|meta| (routing ambiguity canary)
cmd/elastickv-snapshot-decode binary + MANIFEST/CHECKSUMS
emission follow in a separate PR.
Self-review:
1. Data loss: tombstones dropped on purpose (Phase 0a reflects
current state, not MVCC history); disabled-adapter entries
counted but not written (operator opted out); routing covers
every prefix the live adapters emit.
2. Concurrency: dispatcher is single-threaded over ReadSnapshot
callback; no shared state.
3. Performance: prefix table walks about 30 entries per record;
acceptable for offline decode (throughput dominated by
per-adapter file I/O, not the dispatch lookup).
4. Data consistency: header carried through; encrypted entries
propagate ErrSnapshotEncryptedEntry from ReadSnapshot.
5. Test coverage: eight tests cover the routing surface; per-
adapter encoders are already tested in their own files.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 a dispatcher to orchestrate the routing of snapshot entries to specific adapter encoders (DynamoDB, S3, Redis, and SQS) based on key prefixes. It also refactors the snapshot reader to ensure headers are accessible for empty snapshots and adds unit tests for routing logic. Feedback focuses on preventing data loss by ensuring the S3 scratch directory does not overlap with the output root and improving the robustness of the finalization process to ensure all adapters are cleaned up even when errors occur.
| scratch = opts.OutRoot | ||
| } | ||
| d.s3 = NewS3Encoder(opts.OutRoot, scratch). |
There was a problem hiding this comment.
Defaulting ScratchRoot to opts.OutRoot causes the S3Encoder to use the same base directory (s3/) for both temporary scratch chunks and final assembled objects. Since S3Encoder.Finalize performs an os.RemoveAll on its scratchRoot at the end of its execution, this default behavior will result in the entire S3 dump being deleted immediately after it is written. Using a dedicated .scratch subdirectory prevents this collision.
if scratch == "" {
scratch = filepath.Join(opts.OutRoot, ".scratch")
}| import ( | ||
| "bytes" | ||
| "io" | ||
| "sort" | ||
|
|
||
| "github.com/cockroachdb/errors" | ||
| ) |
There was a problem hiding this comment.
The path/filepath package should be imported to support safe path manipulation when setting up the default scratch directory for the S3 encoder.
| import ( | |
| "bytes" | |
| "io" | |
| "sort" | |
| "github.com/cockroachdb/errors" | |
| ) | |
| import ( | |
| "bytes" | |
| "io" | |
| "path/filepath" | |
| "sort" | |
| "github.com/cockroachdb/errors" | |
| ) |
| hdr, err := ReadSnapshotWithHeader(r, func(_ SnapshotHeader, e SnapshotEntry) error { | ||
| return d.handleEntry(e) | ||
| }) | ||
| if err != nil { | ||
| return DecodeResult{}, err | ||
| } | ||
| if err := d.finalize(); err != nil { | ||
| return DecodeResult{}, err | ||
| } |
There was a problem hiding this comment.
If ReadSnapshotWithHeader fails (e.g., due to a truncated stream), d.finalize() is currently skipped. This prevents encoders from cleaning up temporary resources, such as the S3 scratch directory. The dispatcher should ensure that finalize is called even on read errors to perform necessary cleanup, while still prioritizing and returning the original read error.
| hdr, err := ReadSnapshotWithHeader(r, func(_ SnapshotHeader, e SnapshotEntry) error { | |
| return d.handleEntry(e) | |
| }) | |
| if err != nil { | |
| return DecodeResult{}, err | |
| } | |
| if err := d.finalize(); err != nil { | |
| return DecodeResult{}, err | |
| } | |
| hdr, err := ReadSnapshotWithHeader(r, func(_ SnapshotHeader, e SnapshotEntry) error { | |
| return d.handleEntry(e) | |
| }) | |
| fErr := d.finalize() | |
| if err == nil { | |
| err = fErr | |
| } | |
| if err != nil { | |
| return DecodeResult{}, err | |
| } |
References
- Do not silently ignore errors from cleanup methods on resources. Ensure errors are visible to prevent resource leaks or hidden failures.
| func (d *dispatcher) finalize() error { | ||
| if d.ddb != nil { | ||
| if err := d.ddb.Finalize(); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if d.s3 != nil { | ||
| if err := d.s3.Finalize(); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if d.redis != nil { | ||
| if err := d.redis.Finalize(); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| if d.sqs != nil { | ||
| if err := d.sqs.Finalize(); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The current finalize implementation returns immediately on the first error, which can leave subsequent encoders unfinalized and their resources (like scratch directories) leaked. The dispatcher should attempt to finalize all active encoders and return the first error encountered to ensure visibility into cleanup failures.
func (d *dispatcher) finalize() error {
var firstErr error
if d.ddb != nil {
if err := d.ddb.Finalize(); err != nil && firstErr == nil {
firstErr = err
}
}
if d.s3 != nil {
if err := d.s3.Finalize(); err != nil && firstErr == nil {
firstErr = err
}
}
if d.redis != nil {
if err := d.redis.Finalize(); err != nil && firstErr == nil {
firstErr = err
}
}
if d.sqs != nil {
if err := d.sqs.Finalize(); err != nil && firstErr == nil {
firstErr = err
}
}
return firstErr
}References
- Do not silently ignore errors from cleanup methods on resources. Ensure errors are visible to prevent resource leaks or hidden failures.
![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: 3e0fade764
ℹ️ 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".
| // Internal-only: transactional intents / locks / resolver | ||
| // records. Phase 0a drops these; they belong to the running | ||
| // cluster, not the data. | ||
| {[]byte("!txn|"), routeInternalDrop}, |
There was a problem hiding this comment.
The dispatcher treats any non-matched key as Unknown, but the route table only hard-drops !txn| (plus S3 internals) and omits other non-user namespaces that are persisted in snapshots, such as distribution catalog keys (!dist|meta|, !dist|route| in distribution/catalog.go). Because snapshot writing iterates all Pebble keys, these valid control-plane records will be counted as Unknown instead of Internal, creating false corruption/format-skew signals and skewing counters on real clustered snapshots.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return DecodeResult{}, err | ||
| } |
There was a problem hiding this comment.
If ReadSnapshotWithHeader returns an error after some entries were already processed, DecodeSnapshot returns immediately and skips d.finalize(). This bypasses adapter cleanup paths (notably S3 Finalize, which closes buffered JSONL writers and removes scratch files), so a malformed/truncated tail can leak temporary files/handles and leave partial on-disk state in long-running processes that call the decoder repeatedly.
Useful? React with 👍 / 👎.
…ys runs Addresses gemini r1 review on PR #806 (1 high + 3 mediums, all on internal/backup/decode.go). ## security-high: default ScratchRoot wipes final S3 output Pre-fix, `newDispatcher` defaulted `scratch := opts.OutRoot` when `opts.ScratchRoot` was empty. `NewS3Encoder(outRoot, scratch)` then computed `scratchRoot = filepath.Join(scratch, "s3")` = the SAME directory the encoder writes final assembled bodies to. `S3Encoder.Finalize` runs `defer os.RemoveAll(s.scratchRoot)` unconditionally, so the entire S3 portion of the dump was wiped immediately after it landed. Fix: default scratch to `filepath.Join(opts.OutRoot, ".scratch")` so the scratch tree is a disjoint sibling of the final `s3/` output tree. Operators who still want scratch outside OutRoot keep their existing override via `opts.ScratchRoot`. Regression test: `TestDecodeSnapshot_DefaultScratchDoesNotWipeFinalS3Output` pre-creates a marker file at `<OutRoot>/s3/preexisting.txt`, runs a header-only DecodeSnapshot with S3 enabled and empty ScratchRoot, and asserts the marker survives. Verified the test fails against the pre-fix code (`scratch = opts.OutRoot`) — it reports "marker file was wiped by Finalize". ## medium: finalize() skipped on ReadSnapshotWithHeader error Pre-fix, a truncated stream returned `err` from `ReadSnapshotWithHeader` and skipped `d.finalize()` — leaving scratch trees on disk after a failed decode. Fix: call `d.finalize()` unconditionally. The original read error wins over a finalize error when both occur (the read failure is the root cause; finalize errors are usually downstream symptoms). Regression test: `TestDecodeSnapshot_FinalizeRunsOnTruncatedStream` feeds a half-header, asserts DecodeSnapshot errors, AND asserts the `.scratch` tree is absent. ## medium: finalize() short-circuits on first encoder error Pre-fix, `dispatcher.finalize()` returned on the first encoder error — later encoders never got to clean up. Fix: best-effort iteration, first error wins. Each encoder's Finalize still runs even if an earlier one errored, so all scratch trees get reclaimed. ## medium: add `path/filepath` import Required by the high-priority fix above. ## Caller audit (semantic-change rule) `DecodeSnapshot` has no non-test callers yet (Phase 0a — the routing layer ships separately from any CLI wrapper). The only direct caller of `dispatcher.finalize()` is `DecodeSnapshot` itself, which is updated in lock-step with the best-effort change. The fail-closed semantics are preserved: a Finalize error still surfaces as a non-nil return; the change only affects WHICH error is returned when multiple encoders fail. ## Verification - `go test -race -count=1 ./internal/backup/...` passes (existing tests + 2 new regression tests). - `golangci-lint` clean. - Regression test verified to catch the pre-fix bug.
|
@claude review Round 1 (commit security-high ( medium ( medium ( medium ( Regression tests:
Caller audit:
|
|
Claude finished @bootjp's task in 2m 53s —— View job Review of Round 1 fixes (commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9f23c75b3
ℹ️ 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".
| // Internal-only: transactional intents / locks / resolver | ||
| // records. Phase 0a drops these; they belong to the running | ||
| // cluster, not the data. | ||
| {[]byte("!txn|"), routeInternalDrop}, |
There was a problem hiding this comment.
The dispatch table only hard-drops !txn| (plus S3 internals), so persisted control-plane keys like !dist|meta|... / !dist|route|... fall through to Unknown even though they are expected in healthy snapshots. This produces false format-skew/corruption signals and noisy counters for real cluster backups. Fresh evidence: distribution/catalog.go defines and persists these prefixes, and store/snapshot_pebble.go snapshots all Pebble keys, so they will reach this router.
Useful? React with 👍 / 👎.
…then test Addresses round-2 review on PR #806: - Codex r1 P2 (!dist| missing from route table) - claude-bot r1 (stale ScratchRoot doc + vacuous truncated-stream test) ## P2: !dist| dispatched as Internal, not Unknown The distribution catalog persists `!dist|meta|*` and `!dist|route|*` keys through the default Raft group's Pebble (distribution/catalog.go). Snapshot writes iterate every Pebble key, so these records appear in every real clustered snapshot. The prefix router only knows `!txn|` as an internal-drop today, so distribution keys land in Counters.Unknown — a false corruption signal on production dumps. The Redis adapter already treats this as a known internal namespace (adapter/redis_compat_types.go's []byte("!dist|") skip), confirming the prefix is well-defined cluster-wide. Add `{[]byte("!dist|"), routeInternalDrop}` to buildPrefixRoutes() alongside the existing !txn| entry. The descending-length sort preserves longest-match semantics so no existing route is shadowed. Caller audit: routeInternalDrop is a no-op that increments Counters.Internal. Moving distribution-catalog entries from Unknown → Internal preserves the operator-facing semantics: - Counters.Unknown remains the "corruption / version-skew" alarm - Counters.Internal remains the "intentional drop" bucket Existing Counters.Unknown / Counters.Internal consumers (none in production code yet — Phase 0a is the routing layer) interpret both buckets correctly under the new routing. Regression test: TestDecodeSnapshot_DistPrefixDroppedAsInternal feeds one `!dist|meta|version` + one `!dist|route|abc` and asserts Counters.Internal == 2 / Counters.Unknown == 0. ## Stale ScratchRoot doc (claude-bot r1 must-fix) DecodeOptions.ScratchRoot comment said "Defaults to OutRoot when empty" — directly contradicted the security-high fix in r1 (scratch now defaults to `<OutRoot>/.scratch`). Updated to match the actual behavior, with an explicit NEVER-default-to-OutRoot note explaining why. ## Strengthened truncated-stream test (claude-bot r1 follow-up) TestDecodeSnapshot_FinalizeRunsOnTruncatedStream was vacuously true: the truncated stream errored before any S3 record was processed, so `.scratch` was never created and the absent-check passed regardless of whether finalize() ran. Strengthen: pre-create `<root>/.scratch/s3/` manually so it exists going in. Now finalize-skip leaves the dir intact; the fix's unconditional finalize() reclaims it via the deferred os.RemoveAll. Verified the strengthened test FAILS against a hand-rolled regression of the pre-fix code (finalize skipped on read error): "`.scratch/s3/ survived truncated-stream decode — finalize did not run`". ## Verification - go test -race ./internal/backup/... passes (existing tests + 1 new + 1 strengthened). - golangci-lint clean.
|
@claude review Round 2 (commit Codex r1 P2 ( claude-bot r1 must-fix (stale ScratchRoot doc): claude-bot r1 follow-up (vacuous truncated-stream test): New regression test: Caller audit (semantic-change rule):
|
|
Claude finished @bootjp's task in 3m 33s —— View job Review of Round 2 fixes (commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67cce29e59
ℹ️ 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".
| // Internal-only: transactional intents / locks / resolver | ||
| // records. Phase 0a drops these; they belong to the running | ||
| // cluster, not the data. | ||
| {[]byte("!txn|"), routeInternalDrop}, |
There was a problem hiding this comment.
The dispatch table drops !txn|/!dist| as internal but leaves out the encryption writer-registry namespace, so !encryption|writers|... records fall through to Unknown. Those keys are legitimately persisted in Pebble by the encryption applier/store glue and are included in snapshots because writePebbleSnapshotEntries iterates all DB keys, so snapshots from clusters that have encryption registry rows will report false format-skew/corruption signals. Add an internal-drop route for the writer-registry prefix in this section.
Useful? React with 👍 / 👎.
PR #806 round 3 — addresses Codex r3 P2 on the !encryption| writer-registry namespace. `!encryption|writers|<be4 dek_id>|<be2 uint16 node_id>` rows (internal/encryption/registry.go's WriterRegistryPrefix) are persisted through the default Raft group's Pebble by the §4.1 encryption applier on every Bootstrap / Rotation / Registration apply. They ride snapshots like any other 0x03 entry — so every clustered snapshot from an encrypted-storage cluster carries them. Pre-fix, these rows hit no prefix route and landed in Counters.Unknown — a false corruption signal on production dumps the moment storage envelope encryption rolls out. Fix: add `{[]byte("!encryption|"), routeInternalDrop}` to buildPrefixRoutes(), mirroring the !txn| and !dist| entries. The broader `!encryption|` prefix (not just `!encryption|writers|`) covers any future encryption metadata under the same namespace without needing a second route addition. The descending-length sort preserves longest-match semantics so no existing route is shadowed. ## Caller audit (semantic-change rule) routeInternalDrop is a no-op that increments Counters.Internal. The behavior change moves writer-registry entries from Counters.Unknown → Counters.Internal. As with the !dist| addition in r2, this preserves operator-facing semantics: Counters.Unknown remains the corruption / version-skew alarm bucket; Counters.Internal remains the intentional-drop bucket. No production consumers of either counter exist yet (Phase 0a). ## Regression test TestDecodeSnapshot_EncryptionPrefixDroppedAsInternal feeds one `!encryption|writers|<be4 1>|<be2 0x42>` row and asserts Counters.Internal == 1 / Counters.Unknown == 0. ## Verification - go test -race ./internal/backup/... passes. - golangci-lint clean.
|
@claude review Round 3 (commit Route Used the broader Caller audit (semantic-change rule): Regression test:
|
|
Claude finished @bootjp's task in 2m 23s —— View job Review of Round 3 fixes (commit
|
| Prefix | Source | Covered since |
|---|---|---|
| `!txn | ` | kv/txn_keys.go:13 |
| `!s3route | ` | S3 internal |
| `!s3 | gc | upload |
| `!dist | ` | distribution/catalog.go:15–16 |
| `!encryption | ` | internal/encryption/registry.go:21 |
| `!ddb | route | table |
| `!sqs | route | ` |
HLC ceiling updates call hlc.SetPhysicalCeiling on the in-memory *HLC struct (kv/fsm.go:342); no Pebble key is written. No other persistent internal namespaces found.
Minor (carry-over from r2, still non-blocking)
decode.go:104–106 lists !txn|, !s3route| as illustrative Internal examples; the list now lags by two entries (!dist|, !encryption|). The comment is labeled e.g. and is intentionally non-exhaustive, so this doesn't affect correctness or operator behavior. Worth a one-liner update before Phase 1 if the example list grows further.
Summary
Round 3 is clean. All three rounds of findings are correctly resolved, the namespace coverage is complete against all known Pebble-persisted internal keys, and the regression test is genuine. Ready to merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53469c33a8
ℹ️ 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".
| scratch := opts.ScratchRoot | ||
| if scratch == "" { |
There was a problem hiding this comment.
When opts.ScratchRoot is provided, the dispatcher uses it verbatim for S3 without checking whether it aliases OutRoot; if a caller sets ScratchRoot == OutRoot, NewS3Encoder turns that into <out>/s3 and S3Encoder.Finalize later runs os.RemoveAll on that path, deleting the decoded S3 output tree after a successful run. This is a silent data-loss path triggered by a plausible CLI misconfiguration (--scratch-root equal to output), so DecodeSnapshot should fail fast on overlapping roots.
Useful? React with 👍 / 👎.
PR #806 round 4 — addresses Codex r3 P1. The r1 fix only protected the *default* path (empty ScratchRoot → `<OutRoot>/.scratch`). An explicit `--scratch-root=$OUT_ROOT` CLI invocation would still set scratchRoot to outRoot verbatim; NewS3Encoder then turns that into `<outRoot>/s3` and S3Encoder.Finalize's deferred os.RemoveAll wipes the final output tree after a successful decode — a plausible misconfiguration with no surfaceable failure mode. Fix: in newDispatcher, after resolving the scratch path, fail fast with ErrDecodeOptionsInvalid if `filepath.Clean(scratch) == filepath.Clean(opts.OutRoot)`. Clean normalises trailing slashes / `./` noise, so the two paths must be cleaned-equal to trip the check (a sibling scratch dir like `<OutRoot>/scratch` is fine; only true equality is rejected). This is a fail-closed validation change at the DecodeSnapshot boundary. Caller audit: DecodeSnapshot has no non-test callers yet (Phase 0a — no CLI wrapper); future callers receive ErrDecodeOptionsInvalid the same way they would for missing OutRoot, so the contract is uniform. The ScratchRoot field doc-comment now mentions the failure mode so operators reading the API can spot the constraint without running into the error at decode time. ## Regression test TestDecodeSnapshot_RejectsScratchEqualsOutRoot covers two cases: - Exact equality (ScratchRoot == OutRoot) - Noisy equality (ScratchRoot == OutRoot + "/./") — confirms filepath.Clean normalises both before comparison Both assert errors.Is(err, ErrDecodeOptionsInvalid). ## Verification - go test -race ./internal/backup/... passes. - golangci-lint clean.
|
@claude review Round 4 (commit Reject
Caller audit: This is a fail-closed validation change at the Doc: Regression test:
|
|
Claude finished @bootjp's task in 1m 38s —— View job Review of Round 4 (commit
|
Adds the Phase 0a operator-facing binary on top of the dispatcher landed in PR #806. The CLI reads a .fsm snapshot, dispatches it through DecodeSnapshot, and emits MANIFEST.json + CHECKSUMS into the chosen output root. - cmd/elastickv-snapshot-decode/main.go - flag surface: --input, --output, --adapter, --cluster-id, --include-incomplete-uploads, --include-orphans, --preserve-sqs-visibility, --include-sqs-side-records, --rename-collisions, --dynamodb-bundle-mode - testable run() entrypoint (argv slice in, error out; no os.Exit) so main_test.go can drive the binary directly - structured slog warning sink wired into the encoders - MANIFEST.json stamped with snapshot_index parsed from filename, last_commit_ts from the dispatcher header, exclusion flags from CLI, and a non-nil pointer to Adapter{} for every enabled adapter so the on-disk shape is already correct when per-scope enumeration lands - cmd/elastickv-snapshot-decode/main_test.go - end-to-end synthetic Redis-string snapshot round-trips through run(), produces the expected blob file, manifest, and CHECKSUMS, and passes VerifyChecksums - flag-validation paths: missing --input, unknown adapter, zero adapter list, unparseable .fsm filename - parseAdapterSet table test - internal/backup/checksums.go - WriteChecksums(root): walks root, computes sha256, writes a sha256sum(1)-compatible CHECKSUMS file (two-space separator, forward-slash paths, lowercase hex, lexicographic order, excludes CHECKSUMS itself) - VerifyChecksums(root): re-reads the file and surfaces the first mismatch as ErrChecksumMismatch (typed sentinel for the Phase 0b encoder self-test path) - internal/backup/source.go - SnapshotIndexFromPath: parses the <uint64>.fsm filename convention the live writer emits and the CLI stamps into MANIFEST.snapshot_index; soft-fails for off-convention names via ErrSnapshotIndexUnparseable - internal/backup/{checksums,source}_test.go - basic listing, self-exclusion, determinism across runs, happy-path verify, tamper detection - SnapshotIndexFromPath table test covering live-writer layout, hand-rolled names, max-uint64, missing suffix, empty stem, non-numeric, negative, hex stems Self-review: 1. Data loss: no — CHECKSUMS is written last so a crash mid-dump leaves an unchecked but otherwise intact tree the operator can re-checksum; the MkdirAll on outputRoot is idempotent so an operator-supplied pre-existing directory is preserved. 2. Concurrency: single-threaded; main is sequential. 3. Performance: sha256 over each output file streams via io.Copy so multi-GB dump artifacts pass without buffering. CHECKSUMS walk is O(files). 4. Data consistency: MANIFEST.last_commit_ts comes from the dispatcher header (now surfaced via ReadSnapshotWithHeader so empty-snapshot dumps still populate it); snapshot_index parsed from the filename per the documented live-writer convention; encryption-state failures propagate from DecodeSnapshot as ErrSnapshotEncryptedEntry. 5. Test coverage: five CLI tests (golden path + four flag- validation surfaces); seven backup-package tests covering the new helpers; existing backup-package tests still pass.
…ning)
Two reviewer-flagged issues on the CHECKSUMS path. The Codex P1
("default ScratchRoot aliased OutRoot") was already fixed on main
by PR #806 follow-ups (c9f23c7 + b9fa4a7 — default to
<OutRoot>/.scratch + reject ScratchRoot==OutRoot), so this commit
ships only the remaining two findings.
1. (coderabbit critical, path traversal) VerifyChecksums joined
CHECKSUMS relative paths to root without validating them, so a
malicious CHECKSUMS line like `<hex> ../../etc/shadow` would
direct the verifier to fingerprint files outside the dump root.
Fix: validateChecksumRelPath rejects absolute paths and any
path whose filepath.Clean'd form starts with `..` or equals
`..`/`.`/empty. Returns ErrChecksumsPathTraversal so callers
can branch on errors.Is.
Test: TestVerifyChecksums_RejectsTraversalPath — table-driven
over five attack shapes (parent-traversal, deep-traversal,
absolute, current-dir, empty); each is rejected before any
sha256 is computed.
2. (gemini security-high + high, OOM vector) VerifyChecksums used
os.ReadFile + strings.Split, materialising the whole CHECKSUMS
in memory. A multi-million-file dump or an adversarial
newline-free CHECKSUMS could OOM the verifier.
Fix: switched to bufio.Scanner with a fixed 8 KiB per-line
budget. Streaming throughout — even with a TB of dump
artefacts the verifier holds at most one line plus the
in-flight sha256 hash state.
Test: TestVerifyChecksums_StreamsLargeFile exercises the
scanner path with 64 files; CI cost of a literal-large
fixture is not justified vs. the assertion that the streaming
API works at all.
Caller audit (CLAUDE.md "semantic-change → grep all callers"):
- VerifyChecksums: zero production callers. Test-only consumers.
Path-validation is rejection-side tightening; honest CHECKSUMS
unaffected. Streaming semantics unobservable to callers.
- WriteChecksums (unchanged in this commit) has one caller
(cmd/elastickv-snapshot-decode/main.go:252) — still passes
through.
Self-review:
1. Data loss — no regression (S3 scratch fix already on main).
2. Concurrency — none.
3. Performance — streaming verify is strictly better.
4. Data consistency — path traversal closed.
5. Test coverage — two regression tests, one per finding.
## Summary Adds the Phase 0a operator-facing binary on top of the dispatcher in #806. Plus the CHECKSUMS writer / verifier and the `<uint64>.fsm` filename → snapshot-index parser the CLI needs to stamp MANIFEST.json. Stacked on `backup/snapshot-dispatcher` — once #806 merges, this PR's diff collapses to just the CLI + helpers and I'll retarget to `main`. ## What lands | File | Purpose | |---|---| | `cmd/elastickv-snapshot-decode/main.go` | Flag parsing, `DecodeSnapshot` invocation, `MANIFEST.json` + `CHECKSUMS` emission | | `cmd/elastickv-snapshot-decode/main_test.go` | End-to-end synthetic-snapshot golden path + four flag-validation paths + `parseAdapterSet` table test | | `internal/backup/checksums.go` | `WriteChecksums(root)` / `VerifyChecksums(root)` — sha256sum(1)-compatible | | `internal/backup/source.go` | `SnapshotIndexFromPath` — `<uint64>.fsm` filename parser used to stamp `MANIFEST.snapshot_index` | | `internal/backup/checksums_test.go`, `source_test.go` | Pin file shape, deterministic ordering, tamper detection, parse table | ## CLI surface ``` elastickv-snapshot-decode \ --input <fsm-file> (required) --output <directory-root> (required) [--adapter dynamodb,s3,redis,sqs|all] [--cluster-id <id>] [--include-incomplete-uploads] [--include-orphans] [--preserve-sqs-visibility] [--include-sqs-side-records] [--rename-collisions] [--dynamodb-bundle-mode per-item|jsonl] ``` Matches the design's §"Tooling" decoder section, minus the deferred `--input-snap-dir`, `--scope`, and `--dynamodb-bundle-size` flags (called out in the design as later additions). ## Output shape `MANIFEST.json` carries `phase = "phase0-snapshot-decode"`, the filename-derived `snapshot_index`, the dispatcher-surfaced `last_commit_ts`, exclusion flags from the CLI, and a non-nil `Adapter{}` pointer for every enabled adapter (per-scope enumeration is a follow-up). `CHECKSUMS` is the standard `sha256sum --binary` shape — verifiable with `sha256sum -c CHECKSUMS` from the dump root, no elastickv binary involved. ## Test plan - [x] `go test -race ./internal/backup/ ./cmd/elastickv-snapshot-decode/` (passes) - [x] `golangci-lint run ./...` over the touched packages (clean — 0 issues) - [x] `go build ./...` (clean) - [x] Golden-path test: synthetic `!redis|str|greeting` → `redis/db_0/strings/greeting.bin` + `MANIFEST.json` + `CHECKSUMS` + `VerifyChecksums` round-trip ## Self-review (CLAUDE.md §"Self-review of code changes") 1. **Data loss** — CHECKSUMS is written last so a crash mid-dump leaves an intact-but-unchecked tree; `MkdirAll(outputRoot)` is idempotent. 2. **Concurrency** — single-threaded; no shared state across goroutines. 3. **Performance** — sha256 streams via `io.Copy`; multi-GB files pass without buffering. CHECKSUMS walk is O(files). 4. **Data consistency** — `MANIFEST.last_commit_ts` comes from the dispatcher header (now surfaced via `ReadSnapshotWithHeader` so empty-snapshot dumps still populate it); `snapshot_index` parsed from the filename per the documented live-writer convention; encryption-state failures propagate from `DecodeSnapshot` as `ErrSnapshotEncryptedEntry`. 5. **Test coverage** — five CLI tests (golden path + four flag-validation paths); seven backup-package tests for the new helpers; existing backup-package tests still pass. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * New CLI to decode local snapshots into vendor-independent directory trees with adapter selection, output options, and manifest/CHECKSUMS generation * CHECKSUMS written in sha256sum-compatible format for integrity verification; manifest includes version, cluster metadata and parsed snapshot index when available * **Bug Fixes / Security** * CHECKSUMS verification rejects path-traversal and symlink escape attempts * **Tests** * Comprehensive tests covering decoding, manifesting, checksums, parsing, and security regressions <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/bootjp/elastickv/pull/810?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/bootjp/elastickv/pull/810?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
1 participant
Summary
DecodeSnapshot(io.Reader, DecodeOptions) (DecodeResult, error)ininternal/backup/decode.gothat wires the four adapter encoders (DynamoDB, S3, Redis, SQS) onto a single prefix-routed dispatcher.ReadSnapshotWithHeaderso the header is surfaced even for snapshots with zero entries —MANIFEST.last_commit_tsneeds to populate even on an empty dump.cmd/elastickv-snapshot-decodebinary,MANIFEST.jsonemission, andCHECKSUMSwriter live in a separate follow-up PR on top of this.Design reference
docs/design/2026_04_29_proposed_snapshot_logical_decoder.md§"Pipeline" — the routing table here implements the prefix list spelled out there. Longer prefixes win (!hs|meta|d|beats!hs|meta|);!txn|/!s3route|/!s3|gc|upload|are dropped as internal-only.Behavior
DecodeOptions.Adaptersis counted asInternal(intentional drop), notUnknown— so--adapter <subset>keeps per-class counts in the right bucket.Unknown— the format-skew / corruption signal a Phase 0a operator wants to surface, not swallow.Test plan
go test -race ./internal/backup/(passes — 1.4s including the new tests)golangci-lint run ./internal/backup/...(clean — 0 issues)!txn|Internal counter, prefix-length descending ordering invariant, and the!hs|meta|d|vs!hs|meta|ambiguity canary.Self-review (CLAUDE.md §"Self-review of code changes")
ReadSnapshotcallback; no shared state across goroutines.last_commit_tscarried throughReadSnapshotWithHeader; encrypted entries propagateErrSnapshotEncryptedEntryfromReadSnapshot(the underlying reader fails closed onencState=0b01).redis_*_test.go,dynamodb_test.go,s3_test.go,sqs_test.go).