From 33142b7917d80eaf423e2e08f9364263a3cc2a09 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 07:04:20 +0000 Subject: [PATCH 1/2] docs: record the sstable exception to the backup import boundary #1276 made the decoder read SST ingest bundles with Pebble's sstable package, while #1277's boundary test forbids every Pebble package. Both merged, and main has been red since. The decoder design already chose the sstable reader for EKVSSTI1 bundles, whose files are Pebble sstables. Record in both docs that the boundary keeps the Pebble engine out but allows exactly that package, and correct the as-built claim that the backup CLIs link no Pebble. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Gz1MesaSCqfXkJa9zb8Ysx --- .../2026_04_29_implemented_snapshot_logical_decoder.md | 3 +++ .../2026_10_16_implemented_production_commonization.md | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/design/2026_04_29_implemented_snapshot_logical_decoder.md b/docs/design/2026_04_29_implemented_snapshot_logical_decoder.md index 1d3570a98..0beaf15db 100644 --- a/docs/design/2026_04_29_implemented_snapshot_logical_decoder.md +++ b/docs/design/2026_04_29_implemented_snapshot_logical_decoder.md @@ -772,6 +772,9 @@ decoder's answer: disjoint, and reads each SST with Pebble's sstable reader, yielding the same entries as the legacy stream. This is the one place the package links a storage library, because the files are in Pebble's format. + The package's import-boundary test allows exactly + `github.com/cockroachdb/pebble/v2/sstable`; importing the Pebble + engine or any other Pebble package fails it. - **Key budget.** The reader capped keys at the 1 MiB logical size, while the store streams stored keys up to `MaxSnapshotStoredKeySize`, 1 MiB plus a 32-byte internal envelope such as migration staging. The reader diff --git a/docs/design/2026_10_16_implemented_production_commonization.md b/docs/design/2026_10_16_implemented_production_commonization.md index 61986c9c0..799cc1761 100644 --- a/docs/design/2026_10_16_implemented_production_commonization.md +++ b/docs/design/2026_10_16_implemented_production_commonization.md @@ -184,7 +184,11 @@ All five milestones shipped in PR #1277. Each defect fix landed as a failing-tes - the FIFO dedup send timestamp sharing the record's millisecond; - no-TTL HLLs restoring as the legacy raw payload. - Mutation checks on both the backup copy and the live counterpart fail every family's test. -- `elastickv-backup`, `elastickv-snapshot-decode`, `elastickv-snapshot-encode` and `elastickv-snapshot-archive` no longer link `store`, Pebble or `internal/encryption`. `elastickv-snapshot-prepare-restore` still does, through `internal/raftengine/etcd`, which the restore path needs. +- `elastickv-backup`, `elastickv-snapshot-decode`, `elastickv-snapshot-encode` and `elastickv-snapshot-archive` no longer link `store`, the Pebble engine or `internal/encryption`. `elastickv-snapshot-prepare-restore` still does, through `internal/raftengine/etcd`, which the restore path needs. +- #1276 merged while PR #1277 was open. It made the decoder read SST ingest bundles with `github.com/cockroachdb/pebble/v2/sstable` (`internal/backup/snapshot_sst.go`), as the decoder design records. With both on `main`, the boundary test failed on that import. + - The test now exempts exactly that package (`backupAllowedDeps`), and fails if the exemption goes unused. + - The Pebble engine (`github.com/cockroachdb/pebble/v2`) imports `sstable`, so it cannot come in through it. Importing the engine or any other Pebble package still fails the test. + - The four CLIs above therefore link the sstable reader and the Pebble packages it depends on, about 5 MiB per linux/amd64 binary. Only `elastickv-snapshot-decode` and the self-test of `elastickv-snapshot-encode` reach it; `elastickv-backup` and `elastickv-snapshot-archive` carry it unused. **M5.** - The autosplit splitter converts SplitRange children with `distribution.RouteDescriptorFromProto`, which rejects a nil child, an unspecified state or invalid bounds. Such a response now fails the split call instead of becoming a write-fenced placeholder. @@ -196,5 +200,5 @@ All five milestones shipped in PR #1277. Each defect fix landed as a failing-tes - Coordinator wrappers share one forwarding base, and a capability-parity test is the guard against future drift (M1). - Admin writes and sensitive reads require both the JWT role and the live role (M2). - Admin S3 uploads go through the same PUT pipeline as SigV4 uploads, including admission control and blob offload; admin keeps its own size cap (M3). -- `internal/backup`'s import boundary is enforced by a test, and every backup mirror of an adapter encoder has an adapter-side byte-for-byte cross-check (M4). +- `internal/backup`'s import boundary is enforced by a test, and every backup mirror of an adapter encoder has an adapter-side byte-for-byte cross-check (M4). The boundary keeps the Pebble engine out but allows exactly Pebble's `sstable` reader, because SST ingest bundles are Pebble sstable files (#1276). - Shared server wiring lives in `internal/serverwiring`; the demo stays a separate binary (M5). From 9b1d187c9de1693a285a9056a475f27d4b31a0f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 07:04:20 +0000 Subject: [PATCH 2/2] backup: allow the sstable reader through the import boundary test TestBackupLinksNoLiveClusterCode has failed on main since 927e4bf3: snapshot_sst.go (#1276) imports github.com/cockroachdb/pebble/v2/sstable and the boundary (#1277) forbids the whole Pebble module. Exempt exactly that package. It cannot reach the Pebble engine, which imports it, and importing the engine or any other Pebble package still fails the test. The test also fails when an exemption goes unused, and TestForbiddenBackupDep pins the matching rules: exact for allowed paths, prefix for forbidden ones. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Gz1MesaSCqfXkJa9zb8Ysx --- internal/backup/import_boundary_test.go | 45 +++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/internal/backup/import_boundary_test.go b/internal/backup/import_boundary_test.go index 06e3f41c6..b8e112074 100644 --- a/internal/backup/import_boundary_test.go +++ b/internal/backup/import_boundary_test.go @@ -4,6 +4,7 @@ import ( "go/build" "os" "path/filepath" + "slices" "strings" "testing" ) @@ -24,6 +25,16 @@ var backupForbiddenDeps = []string{ "github.com/cockroachdb/pebble", } +// backupAllowedDeps are exact package paths exempt from +// backupForbiddenDeps. Snapshot SST ingest bundles are Pebble sstable +// files, so the decoder reads them with Pebble's sstable reader +// (snapshot_sst.go, docs/design/2026_04_29_implemented_snapshot_logical_decoder.md). +// That package cannot reach the Pebble engine, which imports it, and +// importing the engine or any other Pebble package still fails the test. +var backupAllowedDeps = []string{ + "github.com/cockroachdb/pebble/v2/sstable", +} + // TestBackupLinksNoLiveClusterCode walks the non-test import closure of // internal/backup through first-party packages and fails on any // forbidden package, naming the import chain that reaches it. @@ -56,9 +67,43 @@ func TestBackupLinksNoLiveClusterCode(t *testing.T) { } } } + for _, allowed := range backupAllowedDeps { + if _, seen := via[allowed]; !seen { + t.Errorf("internal/backup no longer links %s; remove it from backupAllowedDeps", allowed) + } + } +} + +// TestForbiddenBackupDep pins the boundary's matching rules: a forbidden +// entry covers its subpackages, and an allowed entry exempts only its +// exact path. +func TestForbiddenBackupDep(t *testing.T) { + t.Parallel() + + const pebble = "github.com/cockroachdb/pebble" + cases := []struct { + pkgPath string + want string + }{ + {pkgPath: pebble + "/v2/sstable", want: ""}, + {pkgPath: pebble + "/v2", want: pebble}, + {pkgPath: pebble + "/v2/sstable/block", want: pebble}, + {pkgPath: pebble + "/sstable", want: pebble}, + {pkgPath: elastickvModule + "/store", want: elastickvModule + "/store"}, + {pkgPath: elastickvModule + "/internal/encryption/kek", want: elastickvModule + "/internal/encryption"}, + {pkgPath: elastickvModule + "/internal/s3keys", want: ""}, + } + for _, tc := range cases { + if got := forbiddenBackupDep(tc.pkgPath); got != tc.want { + t.Errorf("forbiddenBackupDep(%q) = %q, want %q", tc.pkgPath, got, tc.want) + } + } } func forbiddenBackupDep(pkgPath string) string { + if slices.Contains(backupAllowedDeps, pkgPath) { + return "" + } for _, forbidden := range backupForbiddenDeps { if pkgPath == forbidden || strings.HasPrefix(pkgPath, forbidden+"/") { return forbidden