backup: decode the .fsm files a running node persists by bootjp · Pull Request #1276 · bootjp/elastickv

bootjp · 2026-10-16T07:15:53Z

Summary

elastickv-snapshot-decode could only read snapshots the encoder produced. Review of #1275 found that it could not read the fsm-snap/<index>.fsm file a running node writes, and that on a raw store stream with MVCC history it wrote a wrong dump. This PR closes the four gaps Codex reported plus one more found while reproducing them. The design doc addendum (first commit) records each one:

Gap	Before	After
Framing	The `EKVTHLC1`/`EKVTHLC2` FSM header in front of the store payload was rejected as a bad magic (`got "EKVTHLC1"`), and the engine's 4-byte CRC32C footer was never checked	`backup.ReadSnapshotFile` accepts a persisted file, a raw store payload (encoder output), and a raw payload followed by the footer. With a header the footer is required and verified. The verified value is written to MANIFEST `source.fsm_crc32c`, a field the format defined and the CLI never filled
MVCC history	Every physical version was routed, so older values overwrote newer ones and a deleted key came back from the version under its tombstone	Only each key's newest version is kept, and the key is dropped when that version is a tombstone. Versions of one key need not be adjacent (a key that extends another can sort between them), so the filter tracks the open prefix keys. It fails closed (`ErrSnapshotUnsorted`) on out-of-order input. `DecodeCounters.Superseded` counts the hidden versions
SST ingest bundle	`EKVSSTI1` (the payload when `ELASTICKV_PEBBLE_SST_INGEST_SNAPSHOT=true`) was rejected	The manifest SHA-256 and each file's SHA-256 are verified, file key ranges must be ordered and disjoint, only `Set` entries are accepted, and SSTs are read with Pebble's `sstable` reader. The entries match the legacy stream
Key budget	The reader capped keys at the 1 MiB logical size, but the store streams stored keys up to `MaxSnapshotStoredKeySize` (1 MiB + 32-byte envelope)	The reader accepts the stored budget, pinned to the store's constant by a test. The encoder keeps the logical cap for keys it writes
File names (not in the Codex report)	The engine writes `%016x.fsm`, but `SnapshotIndexFromPath` parsed the stem as decimal, so `0000000000000040.fsm` gave index 40	A stem of exactly 16 lowercase hex digits parses as hex (64); any other stem still parses as decimal

Commits (made through the GitHub API, test-first per CLAUDE.md):

3aeede92 docs: addendum in 2026_04_29_implemented_snapshot_logical_decoder.md
32198b50 failing tests only (compiles on main, 8 tests red)
cd94fcfa reader: stored-key budget, shared entry path
3f097df6 framing reader, SST bundle reader, newest-version filter
bfd7ff9c DecodeSnapshot wired through them, plus the hex .fsm index
c5b0889c CLI log fields and source.fsm_crc32c, plus unit and property tests

Behavior change

DecodeSnapshot and the CLI now accept persisted .fsm files and SST bundles. Encoder-produced streams decode exactly as before (one version per key, so Superseded = 0).
New public API in internal/backup: ReadSnapshotFile, SnapshotFraming, DecodeResult.Framing, DecodeCounters.Superseded, MaxSnapshotEncodedKeySize, and the errors ErrSnapshotFSMHeaderUnknown, ErrSnapshotFSMHeaderInvalid, ErrSnapshotFooterMismatch, ErrSnapshotTrailingBytes, ErrSnapshotSSTInvalid, ErrSnapshotUnsorted. The invariant is now Total = Superseded + Tombstone + DynamoDB + S3 + Redis + SQS + Internal + Unknown.
A .fsm whose stem is exactly 16 lowercase hex digits now yields a hex snapshot_index. A hand-named 16-digit decimal stem would change value, but the live writer never produces one.
The CLI log gains superseded, fsm_header, store_format, and footer_verified.

Risk

Low. Only the offline decoder (internal/backup, cmd/elastickv-snapshot-decode) changes. No server, Raft, FSM, store, or adapter code changes. internal/backup now imports github.com/cockroachdb/pebble/v2/sstable, which is already a module dependency through store/. The footer is optional on a headerless payload: it is recognised only when the last four bytes equal the CRC32C of everything before them. On a footer-less payload where that happens by chance, the stream reads as truncated, never as different data.

Test evidence

Red on 32198b50 (tests only): TestDecodeSnapshot_LiveStoreKeepsNewestVersion, TestDecodeSnapshot_SSTIngestSnapshot, TestMaxSnapshotEncodedKeySize_MatchesStore, TestReadSnapshot_AcceptsStoredKeyBudget, TestSnapshotIndexFromPath, TestRun_DecodesOnDiskFSMFile, TestRun_DecodesLivePebbleSnapshot, TestRun_DecodesSimpleRedisStringSnapshot.
Green on c5b0889c: go test -race -count=1 ./internal/backup/... ./cmd/elastickv-snapshot-decode/... ./cmd/elastickv-snapshot-encode/... ./cmd/elastickv-snapshot-prepare-restore/... all ok.
go build ./... and go vet pass on every commit of the branch.
golangci-lint v2.9.0 (.golangci.yaml) on ./internal/backup/... ./cmd/... reports 0 issues. No new //nolint except two os.ReadFile test-path suppressions that follow the package's existing test idiom.
TestRun_DecodesOnDiskFSMFile persists a real .fsm through the engine's own writer (kv.NewKvFSMWithHLC + PreparePhysicalSnapshotRestore) and checks snapshot_index 64 and source.fsm_crc32c. This pins the duplicated header/footer constants to the live writer.
Jepsen: not run. No replication, MVCC, OCC, or Redis adapter code path changes; the decoder runs offline.

Self-review (five lenses)

Data loss: Fixes silent loss in dumps: an overwritten value or a deleted key could previously reach the dump. A truncated or corrupt persisted file now fails on the required footer instead of producing a partial dump, and finalize() still runs on error, so S3 scratch is cleaned up. The live write path is untouched.
Concurrency / distributed failures: Not applicable to the code change: single-threaded offline reader, no shared state, no live-cluster calls. Pointing the tool at a half-written .fsm fails the footer check rather than decoding a prefix. go test -race passes.
Performance: Server hot paths are untouched. The decoder adds one CRC32C pass (Castagnoli, hardware-accelerated) and one 64 KiB-chunk copy through the footer reader. The version filter re-encodes each key once and scans an open set bounded by prefix-nesting depth, with no per-key map. SST bundles are buffered one file at a time (store target 64 MiB, hard cap 1 GiB per file).
Data consistency: The dump shows each key's newest version in the snapshot, i.e. the state at last_commit_ts; tombstone-newest keys are dropped. The order check fails closed, so a non-store-ordered input can never pick a stale version. SST bundle bounds and digests mirror store/'s own validation. Encoder output round-trips unchanged.
Test coverage: Failing tests landed first (32198b50) for every gap. Unit tests cover all framings, each through whole and one-byte readers, plus header, footer, magic, and SST corruption cases with typed errors. A rapid property test (TestLatestVersionFilter_KeepsNewestPerKey) runs over keys with shared prefixes and 0xFF bytes, alongside a deterministic interleaving case, the counter invariant, and an end-to-end CLI test on an engine-written .fsm.

Known limitations (unchanged by this PR)

Encrypted entries are still rejected (ErrSnapshotEncryptedEntry); the decoder links no keyring.
!txn| records are dropped as internal. A committed 2PC transaction whose secondary locks were still unresolved at snapshot time is not applied to the dump.
Expiry handling is unchanged.

Follow-up

#1275 (positioning roadmap) records this gap as open in CONTEXT.md, roadmap §3/§4/§6.1, and the audit. Once this lands, those lines can say the decoder reads persisted .fsm files.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

Generated by Claude Code

Summary by CodeRabbit

新機能
- スナップショットデコーダーが、FSMファイル、エンコーダー出力、SST ingest形式の読み込みに対応しました。
- デコード結果に、スナップショットのヘッダー情報やフッターの検証結果が記録されます。
バグ修正
- 更新履歴から各キーの最新値だけを復元し、削除済みのキーが古い値で復活しないようになりました。
- スナップショットの破損や不正な形式を検出し、デコード時にエラーを返します。
- 16桁の16進数形式のファイル名から、スナップショットインデックスを正しく読み取ります。

The decoder as first shipped read only the raw store stream. A fsm-snap/<index>.fsm carries the FSM snapshot header in front and the engine's CRC32C footer behind; the store streams every MVCC version, so routing each one wrote stale values and resurrected deleted keys; the SST ingest format and the store's stored-key budget were refused; and the engine's hex file names were parsed as decimal. Add a dated addendum to the decoder design doc recording each gap and the fix the following commits implement, and point the Background section's framing description at it. Found in review on #1275. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

Each test fails on main: - TestRun_DecodesOnDiskFSMFile persists a real KV FSM snapshot through the Raft engine's file writer (FSM snapshot header, store stream, CRC32C footer) and decodes it with the CLI; main rejects the file on its EKVTHLC1 magic. - TestDecodeSnapshot_LiveStoreKeepsNewestVersion decodes a live store with an overwrite and a delete; main writes the stale "old" value and resurrects the deleted key. - TestDecodeSnapshot_SSTIngestSnapshot decodes the EKVSSTI1 bundle a store emits with SST ingest snapshots on; main rejects its magic. - TestMaxSnapshotEncodedKeySize_MatchesStore and TestReadSnapshot_AcceptsStoredKeyBudget pin the reader's key budget to the store's stored-key budget; main refuses a staged maximum-size key. - TestSnapshotIndexFromPath and the two CLI golden tests use the engine's %016x file names; main parses them as decimal (index 64 is 0000000000000040.fsm and read back as 40). Found in review on #1275. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

The reader capped encoded keys at the 1 MiB logical size plus the timestamp, while a live store streams stored keys up to MaxSnapshotStoredKeySize (1 MiB plus a 32-byte internal envelope, such as a migration-staged key) plus the timestamp; raise MaxSnapshotEncodedKeySize to match. The encoder writes user keys, so it keeps the logical cap under its own constant. Split ReadSnapshotWithHeader into readPebbleStream and emitRawEntry so the framed-file and SST-ingest readers that follow can reuse the EKVPBBL1 loop and the per-entry decode (metadata-key skip included). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

…west version ReadSnapshotFile accepts a store payload with or without the FSM snapshot header (EKVTHLC1/EKVTHLC2) a node writes in front of it, and withholds and verifies the engine's CRC32C footer. It dispatches the payload to the EKVPBBL1 stream reader or to the new EKVSSTI1 bundle reader, which checks the manifest and per-file SHA-256 before parsing each SST. latestVersionFilter keeps the newest version of each user key from a store that streams every physical MVCC version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

DecodeSnapshot now reads through ReadSnapshotFile, so it accepts the fsm-snap/<index>.fsm file a node persists and the SST ingest bundle, and filters versions through latestVersionFilter: a key's older MVCC versions count as Superseded instead of overwriting the newest one. DecodeResult carries the input's framing, including the CRC32C footer the reader verified. SnapshotIndexFromPath parses the live writer's %016x stem as hex; any other stem still parses as decimal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

…footer The CLI logs the superseded count and how the input was framed, and stamps source.fsm_crc32c in MANIFEST.json from the CRC32C footer the reader verified. Adds unit and property tests for the framing reader, the SST ingest bundle's integrity checks, the newest-version filter, and the Superseded counter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

coderabbitai · 2026-10-16T07:16:03Z

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 83070fe7-0681-4ad9-8635-7087addf2c74

📥 Commits

Reviewing files that changed from the base of the PR and between 95889d1 and c5b0889.

📒 Files selected for processing: 15

cmd/elastickv-snapshot-decode/fsm_file_test.go
cmd/elastickv-snapshot-decode/main.go
cmd/elastickv-snapshot-decode/main_test.go
docs/design/2026_04_29_implemented_snapshot_logical_decoder.md
internal/backup/decode.go
internal/backup/encode.go
internal/backup/latest_version.go
internal/backup/latest_version_test.go
internal/backup/snapshot_file.go
internal/backup/snapshot_file_test.go
internal/backup/snapshot_live_store_test.go
internal/backup/snapshot_reader.go
internal/backup/snapshot_sst.go
internal/backup/source.go
internal/backup/source_test.go

+----------------------------+
| Bunny-grade bug detection. |
+----------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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

chatgpt-codex-connector · 2026-10-16T08:34:24Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-16T08:37:20.538598Z	`c5b0889`	Draft marked ready
🔒 Security Review	✅ Completed 2026-10-16T08:39:37.269605Z	`c5b0889`	Draft marked ready

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

The decoder now reads the fsm-snap/<index>.fsm file a node persists, in either store snapshot format, as well as encoder output. CONTEXT.md and the roadmap's section 3 inventory, section 4 row, and section 6.1 README note say so. The section 4 row keeps open what remains: encrypted entries, and committed transactions whose secondary locks are still unresolved when the snapshot is taken. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

The A0 paragraph now says #1267 removed the first failure and #1276 closed the rest: the decoder reads an on-disk .fsm, keeps only the newest version of each key, and accepts the SST-ingest format and the store's stored-key budget. Encrypted entries are still rejected, and the roadmap's section 4 row records that limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz

Design: docs/design/2026_09_26_proposed_positioning_and_roadmap.md ## Summary Follow-up to #1267 (merged as `95889d1f`) and #1276 (merged as `184351ae`). The docs merged with #1256 said `elastickv-snapshot-decode` could not read a live-store snapshot, and named #1267's metadata-key fix as the cure. Review on this PR showed #1267 removed only the first failure, so the decoder fix went to #1276. With #1276 merged, this change records what the two PRs fixed and what remains. It scopes the decoder to one group's DynamoDB, S3, Redis, and SQS keyspaces, and says a dump is not a complete restore source. - **`CONTEXT.md` "Backup and snapshots":** a scope statement only, pointing at the roadmap's §4 row. It says: - the decoder reads the `fsm-snap/<index>.fsm` file a node persists as well as encoder output, and dumps that one group's four adapter keyspaces; - encrypted entries are not decodable offline; - a dump can omit committed data: an unresolved secondary lock's write, a value staged by an in-flight range migration, and keyspaces outside the four adapters such as gRPC RawKV and filesystem keys; - a dump ignores storage-level expiry; - a dump reflects the snapshot's applied index, not the Raft log after it; - dumps of several groups do not share a point in time. - **Roadmap §3 inventory:** the same scope, pointing at §4. - **Roadmap §4 row, what each PR fixed:** - #1267 skips the reserved metadata keys. - #1276 strips the FSM header and verifies the CRC32C footer. - #1276 keeps the newest version of each key from MVCC history. - #1276 accepts `EKVSSTI1` and the stored-key budget, and parses hex `%016x.fsm` names. - `TestRun_DecodesOnDiskFSMFile` pins the on-disk path. - **Roadmap §4 row, open limits, each with its code path:** - encrypted entries (`ErrSnapshotEncryptedEntry`); - unresolved secondary locks (`!txn` records dropped); - staged migration values (`getAtWithStagedVisibility` versus the `!dist` drop); - the gRPC and filesystem keyspaces (`dispatcher.route` counts them as `Unknown`); - storage-level expiry (`dispatcher.handleEntry` drops `ExpireAt`); - the Raft log tail and a cross-group point in time, both non-goals of the decoder design and Phase 1 logical backup's job. - **Roadmap §6.1:** the README backup section may describe the decoder for one group's four adapter keyspaces. It must name the limits the §4 row keeps open and say a dump is not a complete restore source. - **Audit A0 status:** the decoder-rejection defect is closed for cleartext stores. #1267 and #1276 are credited, and the paragraph points at the §4 row. Besides the docs, there is one test-only hunk ported from #1236 in 5d39cc3 to unblock CI: `internal/backup/live_producer_test.go` raises `TestRunLiveBackupRenewalFailureCancelsStreamAndReleasesPin`'s TTL from 9 ms to 300 ms. It becomes a no-op once #1236 lands. a83e602 merges `main` into this branch, so the reviewed tree carries #1276's code. ## Behavior change None. The change is documentation plus one test fixture value. ## Risk None beyond a stale statement, which this change removes. The ported hunk is test-only and identical to #1236's. Every symbol and test the §4 row cites exists on `main` at `184351ae` and in this branch's tree. ## Test evidence - Docs are Markdown only. `git diff -U0 | grep '^+[+-] '` shows no stray diff markers. - Every row in the roadmap's §4 table has three cells. Names that contain a pipe (`!txn|`, `!dist|`, `!fs|`) are written without it, so the row does not split. - Each commit was fetched back and diffed against the local worktree: identical. - Checked on `main` at `184351ae`: - `ReadSnapshotFile`, `latestVersionFilter`, `DecodeSnapshot`, and `SnapshotIndexFromPath` (hex parse at `internal/backup/source.go:49`). - `TestRun_DecodesOnDiskFSMFile`, `ErrSnapshotEncryptedEntry`, and `TestReadSnapshot_RejectsEncryptedEntry`. - The `!txn|` and `!dist|` internal-drop routes, and `dispatcher.route`'s `Unknown` path. - `dispatcher.handleEntry` dropping `SnapshotEntry.ExpireAt` (`internal/backup/snapshot_reader.go:529`, `internal/backup/decode.go:297`), with the store's `PutWithTTLAt` / `ExpireAt` and `kv/fsm_migration_import.go:127` as the writers. - `getAtWithStagedVisibility` and `distribution.MigrationStagedDataKey` in `kv/shard_store.go`. - The unprefixed gRPC raw keys (`adapter/grpc.go`) and the `!fs|` prefixes (`internal/fskeys`). - The decoder design's non-goals: the Raft log tail and the cross-group point in time. - The unresolved-secondary limit, against `kv/lock_resolver_test.go`. - Ported test hunk, run as `go test -race -count=400` under CPU load: - 1 failure before the change and 0 after; - `go test -race ./internal/backup/` passes; - lint reports 0 issues. - No Jepsen runs apply. ## Review rounds - **Codex P2 (cleartext-only):** addressed in bd7d7df / b445803, then superseded by e97663f / f8968bd and 68cfb1d / 4d5c8ae. Resolved. - **CodeRabbit (`CONTEXT.md` glossary-only):** fixed in 1d8c950. Two later Minors, which asked to remove the scope statement, were answered as scope rather than mechanism; CodeRabbit withdrew both and recorded learnings. Resolved. - **Codex P1 / P2 (MVCC versions, SST-ingest, FSM header and CRC footer, stored-key budget):** recorded as open in e97663f / f8968bd, fixed in code by #1276, and recorded as closed in 68cfb1d / 4d5c8ae. Resolved. - **Codex P1 (unresolved secondary locks in the summaries):** fixed in 563b01d. Resolved. - **Codex P2 (`SnapshotIndexFromPath` still parses decimal):** the branch predated #1276; a83e602 merges `main`. Resolved. - **Codex P1 ×3 (staged migration values, unsupported keyspaces, cross-group point in time):** fixed in ba6b729. Resolved. - **Codex P2 ×2 (storage-level expiry, Raft log tail):** fixed in 0ccc0d4. §6.1 now says "the limits" rather than "every limit". Resolved. - **Reviews on 0ccc0d4:** Codex found nothing in its code and security reviews; CodeRabbit generated no actionable comments. ## CI On ba6b729, `test (ubuntu-latest)` failed in two tests this PR does not touch: - The backup renewal test: #1236's fix is ported in 5d39cc3. - `TestRedis_LuaXAddRecreatesTTLExpiredString`: `NOTLEADER` during leader loss in its 3-node cluster. It passed on the rerun. On 0ccc0d4, `test (encryption=on)` failed once in the filesystem Jepsen setup with `reg-0 (File exists)`. `RawFileSystem.Create` returns `EEXIST` for a non-exclusive `O_CREAT` that races another mount; that is existing FUSE behaviour outside this docs PR, so no fix is carried here. It passed on the rerun. 0ccc0d4 is green on every job: build, lint, three test jobs, and CodeQL. ## Self-review lenses 1. **Data loss:** no production code change. The docs list the ways a dump can be incomplete or wrong without an error, so an operator does not treat a dump as a full restore source. 2. **Concurrency / distributed failures:** no production code change. The docs now name the 2PC secondary-resolution window, the staged-visibility window, the log tail, and cross-group consistency. The ported test change only widens a fixture TTL. 3. **Performance:** no production code change. The ported test runs about 100 ms instead of a few milliseconds. 4. **Data consistency:** `CONTEXT.md`, the roadmap (§3, §4, §6.1), and the audit's A0 paragraph agree with `main` at `184351ae`. 5. **Test coverage:** the §4 row names the tests that pin the on-disk path and the encrypted-entry limit. The ported hunk makes an existing test deterministic under load. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01S5GoEpeK4aYNkUxWxutJTz  ## Summary by CodeRabbit * **ドキュメント** * 保存済みスナップショットから、単一RaftグループのDynamoDB、S3、Redis、SQSのデータをダンプできることを明記しました。 * 暗号化されたエントリーはオフラインでデコードできないこと、未解決のロックや移行中データなど一部の内容はダンプに含まれないことを説明しました。 * ダンプは適用済みインデックス時点の内容であり、複数グループ間で同一時点のデータを示すものではないことを追記しました。

TestBackupLinksNoLiveClusterCode fails on main since two merges met: 3f097df (#1276) taught internal/backup to read snapshot SST ingest bundles with github.com/cockroachdb/pebble/v2/sstable, and 5678081 (#1277) forbade every github.com/cockroachdb/pebble package there. Allow exactly the sstable package. The bundles are files in pebble's sstable format, and the reader is a file-format dependency, not the live store the boundary keeps out; the rest of pebble stays forbidden. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC

) Design: docs/design/2026_10_16_implemented_production_commonization.md Design: docs/design/2026_04_29_implemented_snapshot_logical_decoder.md ## Summary The Test workflow (`test (ubuntu-latest)`) has been red on `main` since 927e4bf ([run 38914301223](https://github.com/bootjp/elastickv/actions/runs/38914301223)). Its only failure: ``` --- FAIL: TestBackupLinksNoLiveClusterCode import_boundary_test.go:51: internal/backup links github.com/cockroachdb/pebble: github.com/bootjp/elastickv/internal/backup -> github.com/cockroachdb/pebble/v2/sstable ``` Two PRs merged on the same day with rules that conflict. Each was green on its own branch. - #1276 (`3f097df6`) added `internal/backup/snapshot_sst.go`. It reads `EKVSSTI1` SST ingest bundles with `github.com/cockroachdb/pebble/v2/sstable`. - #1277 (`56780818`) added the import-boundary test. The test forbids every package under `github.com/cockroachdb/pebble`. ## Which design is intended Allow the sstable reader and keep the Pebble engine out (option 1). - **The decoder design chose this package on purpose.** Its #1276 addendum says: "This is the one place the package links a storage library, because the files are in Pebble's format." - **The boundary is meant to keep live-cluster code out.** That is the purpose stated in the test's comment, in the encoder design ("`internal/backup` links no live-cluster machinery") and in M4. Pebble was on the list because `store` brought in the Pebble engine and `internal/encryption`. - **The sstable reader is not the engine.** `github.com/cockroachdb/pebble/v2` imports `sstable`, so the reverse import would be a cycle. `go list -deps` confirms that `elastickv-backup`, `-snapshot-decode`, `-snapshot-encode` and `-snapshot-archive` link no engine root, no `store` and no `internal/encryption`. - **Option 2 (moving the reader out of `internal/backup`) would need a new injection seam.** The decode CLI and the encoder's self-test both reach the reader through `ReadSnapshotFile` / `DecodeSnapshot`, so those would need an injection point. That is an API change made only to shrink what gets linked, and the decode CLI would still link the same reader. **Cost.** I built each CLI with and without the import (`go build -overlay`, go1.26.6, linux/amd64). The import adds 5.1 to 5.2 MiB per binary, about 1,900 Pebble symbols. `elastickv-backup` and `elastickv-snapshot-archive` carry it without ever calling it. If that size matters, putting the reader behind such a seam is a possible follow-up. This PR does not do it. ## Changes 1. `docs:` records the decision. - Decoder addendum: the boundary test allows exactly this package. - Commonization doc, M4 "As built": corrects the claim that the four CLIs link no Pebble. They link no Pebble *engine*, and they do link the sstable reader. - Commonization doc, `## Decisions`: adds the exception. 2. `backup:` changes `import_boundary_test.go`. - `backupAllowedDeps` exempts exactly `github.com/cockroachdb/pebble/v2/sstable`. It is matched exactly, not by prefix. - The walk fails if an exemption goes unused, so the exemption cannot outlive the import. - The new table test `TestForbiddenBackupDep` checks that the exemption is exact. `pebble/v2`, `pebble/v2/sstable/block` and v1 `pebble/sstable` stay forbidden. ## Behavior change None. The changes are test and docs only. No production code changes, and binaries are the same as on `main`, which has linked `sstable` since #1276. ## Risk Low. The boundary is a little looser, by exactly one package. That package cannot bring in the engine, `store` or `internal/encryption`. ## Test evidence - `go test -race -count=1 ./internal/backup/... ./cmd/elastickv-backup/... ./cmd/elastickv-snapshot-decode/... ./cmd/elastickv-snapshot-encode/... ./cmd/elastickv-snapshot-archive/...`: all pass. - `golangci-lint` v2.9.0 (the CI pin) with `--config=.golangci.yaml ./...`: 0 issues. - On unmodified `main`, `TestBackupLinksNoLiveClusterCode` fails with the error above. - Mutation checks. Each mutant is caught: - no allowlist (`main`'s behaviour): both tests fail; - prefix-matched allowlist: `sstable/block` row fails; - engine root added to the allowlist: `pebble/v2` row fails, and the unused-exemption check fails; - reader without the sstable import, run in a scratch worktree because `go/build` reads files from disk: the unused-exemption check fails. - Every other package in the failing `main` run already passed. This change touches no other package. - Jepsen: not applicable. No replication, MVCC, OCC or adapter path changes. ## Self-review (five lenses) 1. **Data loss:** no write, apply, snapshot or restore path changes. Test and docs only. 2. **Concurrency / distributed failures:** none. The test is a static walk over the package graph, and it passes under `-race`. 3. **Performance:** no runtime change. The link cost of `sstable` predates this PR, and is measured and recorded above and in the doc. 4. **Data consistency:** no MVCC, OCC, HLC or routing change. The decoder still reads SST bundles with the Pebble version the store writes with. 5. **Test coverage:** the exemption has a table test for its matching rules and a check that it is still used. Both were mutation-tested. --- _Generated by [Claude Code](https://claude.ai/code/session_01Gz1MesaSCqfXkJa9zb8Ysx)_  ## Summary by CodeRabbit - **ドキュメント** - バックアップ関連 CLI の依存範囲と、SST バンドルの読み込みに関する設計上の境界を明確化しました。 - **テスト** - 依存関係の検証を更新し、許可された SST 読み込み依存のみが対象となることや、必要な CLI で実際に利用されることを確認するようにしました。

…cally led shard (#1278) ## Summary The Redis delta compactor's TTL-inline migration (`adapter/redis_ttl_inline_migrator.go`) has done no work in the server since a36dac7 ("Limit TTL migration scans to local leaders", 2026-07-22). This PR makes it run on every data group a node leads, makes the original failure mode fail safe, and adds a behavioural guard against coordinator wrappers that drop an optional capability. There were two bugs, one behind the other: 1. **The capability never reached the migrator.** The adapters hold `WithKeyVizLabel(startupGatedCoordinator{ShardedCoordinator})`. The migrator asks that value which data groups this node leads (`LocalLeaderGroupIDs`) and scans only those. The label wrapper answered `nil` when its inner lacked the method, and the startup gate never forwarded it, so the migrator planned zero scans. - **main has since fixed the forwarding** in #1277: both wrappers now embed a shared `kv.CoordinatorForwarder`. This branch is merged with main and takes that forwarder. - What this PR adds on top is the `known` contract (below). With it, a coordinator without the capability falls back to whole-prefix scans instead of planning none. 2. **The per-group scan refused every shard but the first.** Codex review found this. `ShardStore.ScanGroupAt` resolves the requested range through the route catalog, and a bare anchor prefix such as `!redis|str|` resolves to the first route only. Every other group got `ErrExplicitGroupRouteOwnerMismatch`. So on a split catalog, a node migrated at most the first route's anchors and failed every tick on the rest. This PR adds `ShardStore.ScanGroupOwnedAt` for it. ### Commits | Commit | Change | |---|---| | `main: reproduce the TTL migration losing local leader groups` | Failing regression test on the real chain: a `ShardedCoordinator` leading groups 1 and 2 and following group 3, wrapped as `main.go` wraps it. | | `kv, main, adapter: keep local leader groups through the coordinator wrappers` | The capability becomes `kv.LocalLeaderGroupsCoordinator` with `LocalLeaderGroupIDs() (ids []uint64, known bool)`. Decorators forward through `kv.LocalLeaderGroupIDsThrough`, which reports a coordinator without it as unknown. Unknown falls back to the migrator's existing whole-prefix `ScanAt` and logs once. A known empty list still plans nothing. | | `main: guard every ShardedCoordinator capability through the startup wrappers` | Behavioural guard test (see the wrapper section). | | `main: forward LinearizableReadForKey through the startup gate` | Superseded by main's forwarder in the merge. | | `adapter: reproduce TTL migration refusing every shard but the first` | Failing regression test over a **real** `ShardStore` split at `m` across two groups. | | `kv, adapter: scan each local leader group's own share of the TTL anchors` | New `ShardStore.ScanGroupOwnedAt`, used by the migrator's per-group plans. | | `kv, adapter: reproduce the group-owned scan's staged, bound and legacy gaps` / `kv, adapter: bound the group-owned scan, defer staged routes, serve legacy rows` | Codex round 2: one physical page per call, staged routes deferred, legacy-route rows served. | | `Merge main into feature/focused-pascal-dbr58v` | Takes main's `CoordinatorForwarder`. Moves the `known` contract onto it. Updates main's chain and plan tests to the new signature. Reworks the behavioural guard to use main's `shardedMethodsNotForwarded` list. | | `kv, adapter: reproduce legacy-route rows served against the owner's state` / `kv: serve legacy-route rows only while the logical owner holds none` | Codex round 3: legacy stream metadata. Also fixes a key the owner also holds being served by two groups. | | `adapter: reproduce TTL-inline ticks starving later scan plans` / `adapter: rotate TTL-inline scan plans across ticks` | Codex round 3: plans rotate across ticks that run out of budget. | | `kv: reproduce the group-owned scan re-reading every wide-column row` / `kv: skip the point-read pass for rows the group-owned scan serves` | Self-review: the scan no longer re-reads each served wide-column row through the point-read path (see below). | | `backup: allow pebble's sstable reader through the import boundary` | CI fix, ported from main's breakage. `TestBackupLinksNoLiveClusterCode` fails on main since 3f097df (#1276) added a pebble `sstable` reader to `internal/backup`, while 5678081 (#1277) forbade all of pebble there. The commit allows exactly the `sstable` package. | ### `ShardStore.ScanGroupOwnedAt` `ScanGroupOwnedAt(ctx, groupID, start, end, limit, ts) (rows, resume, err)`: - **One physical page per call.** Reads up to `limit` rows from the group's own store, fenced like the other explicit-group reads. `resume` is the last key the page read when it was full, `nil` once the range is exhausted. The migrator moves its cursor to `resume`, so a page with no served rows still advances. Each tick stays within one page per plan. - **Served rows only.** - **Owned rows:** keys whose route the group owns, judged by the same `routeAndGroupForKey` that routes writes. - **Legacy-route rows:** hash, set, zset and stream rows still stored at the route of their raw key. These are served only while the logical owner holds no version of the key at `ts`, as checked by this node as the owner's leader. That is exactly when a point read falls back to the legacy route (`routeMissStopsPointFallback`), so the row is the value a reader sees. The legacy route candidate is `legacyPointRouteKey`, the one point reads use. - **Skipped:** copies of ranges the group no longer owns. No key is rewritten from a stale value. - **No point-read pass.** Other scans re-read every hash, set and zset row through the point-read path. That pass now runs once in `scanRouteAtForwardPage`, after whichever page path ran, so those scans are unchanged. `ScanGroupOwnedAt` skips it, because every row it serves already carries the point-read value. This removes: - one point read per owned wide-column row; - a proxied read per legacy-route row the scan then dropped; - a page failure when such a row's owner had no known leader. - **Staged routes deferred.** A key whose route is under staged visibility is skipped and picked up on a later pass. An older live copy must not feed a rewrite while a newer staged copy waits for promotion. - **Meant for the group's own leader.** If leadership moves between planning and the scan, the forwarded request is served as `ScanGroupAt` and may be refused for one tick. The next tick replans. **Known limit, handled in the separate design PR #1280.** A legacy row in group L whose user key belongs to group P, where a different node leads P, is not migrated here. - **Why:** only P's leader may write the key (the HLC rule), and only P's leader can tell whether P holds it. Per-group plans never let P's leader read L. - **Plan:** #1280 proposes measuring the backlog first. L's leader then asks P's leader whether P holds the key and forwards the rewrite, with its timestamp issued by P's leader (A0b) or the TSO (Phase D). Finally L's shadowed copies are evacuated. - **History:** main has not migrated these rows since a36dac7. Before it, every node's whole-prefix scan reached them. - **Meanwhile:** reads stay correct through the dual-route point-read fallback and the legacy TTL fallback. Phase 3 (disabling the fallback) must wait until this case is covered or shown empty. ### Plan rotation Every plan (one per handler and local leader group) shares the compactor's per-tick budget. `migrateTTLInlineOnce` now flattens the plans into one list per tick and starts at the plan after the last one the previous tick ran, as `compactBackgroundHandlers` already does for its handlers. A tick that runs out of budget hands the rest of the rotation to the next tick, so no handler or group starves. ### Design choices - **`known` on main's forwarder.** `CoordinatorForwarder.LocalLeaderGroupIDs` returns `LocalLeaderGroupIDsThrough(Inner)`. Without `known`, any forwarder over an inner that lacks the capability turns "my inner doesn't say" into "leads no group", the failure mode of bug (1). This matches main's own fallbacks for single-group coordinators, such as `IsLeaderForGroup` falling back to the inner's leadership. - **Presence check over canonicalization for legacy rows.** With the owner-holds-nothing rule, the raw row is already the canonical value. Streams therefore need no value-reading canonicalization, and a key the owner also holds is migrated once, by the owner's plan. ### Behavior change - **Production:** each node migrates the TTL anchors of every data group it leads, in rotation. Each plan scans at most 512 physical rows per tick (30s) and rewrites at most 32 keys per proposal, under the existing 5s tick budget and 1m timeout backoff. Expect migration writes after rollout in proportion to the legacy backlog. - **API:** - `ShardedCoordinator.LocalLeaderGroupIDs` and `CoordinatorForwarder.LocalLeaderGroupIDs` now return `([]uint64, bool)`. - New `ShardStore.ScanGroupOwnedAt`. - Existing `ShardStore` behaviour is unchanged. - **Fail-safe:** if a coordinator does not report its groups, the migrator logs `ttl inline migrator: coordinator does not report local leader groups; falling back to whole-prefix scans` once, then runs one `ScanAt` per prefix. That path is not new and is not reached in the server. ## Wrapper capability guard Since #1277, both wrappers embed `kv.CoordinatorForwarder`, which closed every gap in this PR's original audit: - `LinearizableReadForKey` on the gate; - `EnsureMutationsWriteAllowed`, `ObserveForwardedRequests`, `RaftMembers*`, `PhaseD*` and `ValidateDurableTimestamp` on the label. The two guard tests check different things: - **main's `TestAdapterCoordinatorWrappersForwardShardedCapabilities`** checks that every `ShardedCoordinator` method not listed in `shardedMethodsNotForwarded` exists on both wrappers with the same signature. - **This PR's `TestStartupWrappersForwardShardedCoordinatorCapabilities`** uses the same list. It also calls each forwarded method through `startupGatedCoordinator` and through `WithKeyVizLabel(startupGatedCoordinator)` against a spy, and requires the call to reach the spy and return the spy's result and error: 30 methods × 2 wrappers. That second check catches the bug-(1) class, a method that exists but probes its inner through an interface that no longer matches. In a mutation test, I put the forwarder's `LocalLeaderGroupIDs` probe back on the old `[]uint64` signature. This test, the chain test and the wiring test all failed; the signature test passed. ## Do nodes need a one-off migration catch-up? No. The regular loop is the catch-up. - **Cursors reset on restart.** Migration cursors live only in memory, so a restart with this fix starts every per-group cursor at the top of each prefix. The rotation then cycles through every handler and locally led group. - **Reads stay correct meanwhile.** The stock server never disables the legacy `!redis|ttl|` read fallback; `WithRedisLegacyTTLReadFallback` is not wired to any flag or env var. - **What stalled and now resumes:** - rewriting legacy anchors into the inline format; - repairing the `!redis|ttl|` index; - physically cleaning up expired TTL-indexed legacy collections. This one affects storage, not correctness. - **Caveats:** - Time elapsed since 2026-07-22 says nothing about migration progress. Count from this rollout before considering Phase 3 (disabling the fallback). - On split catalogs, the whole-prefix `ScanAt` reaches only the first route for string, HLL, TTL-index and list anchors. So anchors outside the first route may never have been migrated at all, not just since 2026-07-22. This PR covers them. - The different-leader legacy case above stays unmigrated, and Phase 3 must wait for #1280. - Nothing signals that a full pass has finished; I suggest adding that as a follow-up. ## Test evidence - **Each fix was preceded by a failing test:** - The wrapper chain loses the groups: `expected []uint64{0x1, 0x2}`, `actual []uint64(nil)`. - The split store refuses group 2: `explicit group read does not own the requested key range`, for all eight prefixes. - The staged-route key was returned. - A scan read 7 rows for `limit=2`. - A legacy-route hash row was dropped. - A legacy stream meta row was dropped, and the compactor left its TTL out (`ExpireAt:0x0`). - A hash key the owner also held was served by both groups. - 24 budget-limited ticks scanned the first of 24 plans 24 times. - Three owned hash rows cost three point reads. - A legacy hash row whose owner had no leader failed the page with `leader not found`. - **Mutation checks:** each of these fails its targeted tests: - dropping the owner presence check, or narrowing the legacy candidate back to wide-column keys; - ignoring "cannot answer"; - always starting the rotation at plan 0; - putting the forwarder's `LocalLeaderGroupIDs` probe back on the old signature. - **Race suites:** `go test -race -count=1 ./adapter/... ./kv/... .` on `a73a7c0a`: adapter ok (233s), kv ok (14s), root ok (46s). - **Lint:** golangci-lint v2.9.0, CI's version, with main's config (now including `cyclop`): `0 issues`. - **End to end:** one process hosting groups 1 and 2 split at `m`, with a legacy string plus a legacy `!redis|ttl|` anchor seeded on each side. | Binary | Group 1 key | Group 2 key | "does not own" errors | compactor pass failures | |---|---|---|---|---| | PR at `35f698e` (before the split-store fix) | migrated | still legacy after 100s | 4 | 3 | | PR at `a73a7c0a` (merged with main) | migrated | migrated (both within 30s) | 0 | 0 | `TTL` and `GET` were correct throughout. An earlier single-group run (3 nodes) showed base `184351ae` still legacy after 100s, while this PR migrated within 30s. - **Jepsen:** on a 3-node cluster running `a73a7c0a`, the same workloads as CI with `--time-limit 60 --rate 10 --concurrency 10`: - `elastickv.redis-workload`: `{:valid? true}`, 603 ok / 0 fail / 2 info. - `elastickv.redis-zset-safety-workload`: `{:valid? true}`, 586 ok / 0 fail / 2 info. - Zero migrator, ownership or panic lines in the node logs. - These workloads write only inline-format data, so the migrator scans each group concurrently with them but has nothing to rewrite. The end-to-end runs cover the rewrite. ## Self-review (five lenses) 1. **Data loss:** - Migration writes are OCC transactions at the scan's `readTS`, with a commit timestamp from `kv.NextTimestampAfterThrough` on the dispatching group leader (`dispatchCompaction`). A user write after `readTS` conflicts rather than being overwritten. - The scan hands the migrator only rows whose value is the one a reader sees: owned rows, and legacy-route rows the owner holds no version of. Never stale copies of moved ranges, never live copies of a route under staged visibility, never a legacy copy the owner has superseded or deleted. - Rotation changes only the order of plans. - No change to Raft apply, snapshots, Pebble sync or retention. 2. **Concurrency / distributed:** - `LocalLeaderGroupIDs` is a point-in-time snapshot. Each key is still checked with `IsLeaderForKey` before dispatch. - The owner presence check runs only on the owner's leader, behind the same lease fence point reads use. Elsewhere the row is skipped. - A leadership change between the check and the dispatch is caught by `IsLeaderForKey` and OCC. - The rotation cursor is guarded by `stateMu`. Concurrent `SyncOnce` calls only shift where a tick starts. - The one-time warning uses `sync.Once`. Race tests pass. 3. **Performance:** - One physical page per scan call, plus one in-memory route lookup per row. A legacy-route row adds one local version probe. - No per-row point reads; the shared page path keeps its point-read pass for every other caller. - Rotation adds no reads; it builds handlers × local-groups plan structs per tick. - Followers scan nothing. The Redis request path is unchanged. 4. **Data consistency:** - The page and the presence check both read at `readTS`. The presence rule mirrors the point-read fallback exactly, including tombstones. - Ownership uses the same resolution as writes. - The new code issues no timestamps and does not touch the route catalog. - Keeping known-empty distinct from unknown keeps followers from scanning. - TLA+ anchors (`kv/coordinator.go`, `kv/sharded_coordinator.go`): the change adds a capability interface and helper and changes `LocalLeaderGroupIDs`' return. Nothing in the HLC-4, renewal-cadence, group-selection, fail-closed or OCC checklist changed, so no spec update is needed. 5. **Test coverage:** - Regression tests for every bug and review finding: wrapper chain, real split `ShardStore`, staged routes, physical bound, legacy-route rows (hash and stream, three owner states, owner led elsewhere), legacy stream migration end to end in the compactor, plan rotation, no point reads for served rows. - The behavioural guard test and mutation checks. - End-to-end runs on real binaries and the Redis Jepsen workloads. ## Risk Low to moderate. This turns back on a background writer that has been dormant in production for about three months, and on split catalogs it reaches parts of the keyspace it never reached before. Its writes are OCC-guarded and rate-bounded. Watch Raft proposal load on group leaders during the first pass after rollout. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC

bootjp and others added 6 commits October 16, 2026 15:58

bootjp marked this pull request as ready for review October 16, 2026 08:34

bootjp merged commit 184351a into main Oct 16, 2026
9 of 10 checks passed

bootjp deleted the fix/snapshot-decoder-live-fsm branch October 16, 2026 08:38

bootjp mentioned this pull request Oct 16, 2026

docs: record the snapshot decoder fixes from #1267 and #1276 #1275

Merged

bootjp mentioned this pull request Oct 16, 2026

kv, main, adapter: restore the Redis TTL-inline migration on every locally led shard #1278

Merged

This was referenced Oct 17, 2026

TTL-inline: implement cross-leader legacy-row migration #1280

Merged

backup: allow Pebble's sstable reader through the import boundary #1281

Merged

backup: allow pebble's sstable reader in the import boundary #1282

Closed

Navigation Menu

Repository navigation

backup: decode the .fsm files a running node persists - #1276

backup: decode the .fsm files a running node persists#1276
bootjp merged 6 commits into
mainfrom
fix/snapshot-decoder-live-fsm

bootjp commented Oct 16, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Oct 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

bootjp commented Oct 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Behavior change

Risk

Test evidence

Self-review (five lenses)

Known limitations (unchanged by this PR)

Follow-up

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bootjp commented Oct 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Oct 16, 2026 •

edited

Loading