From dc7ab6db98f07f2bad7b439a78f9b4e1585360e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 11:13:15 +0000 Subject: [PATCH] backup: allow pebble's sstable reader in the import boundary The boundary test from #1277 forbids every github.com/cockroachdb/pebble package, but snapshot_sst.go from #1276 reads EKVSSTI1 snapshot ingest bundles with pebble/v2/sstable, so go test ./internal/backup failed on main. The snapshot decoder design names that reader as the one storage library the package links, because the bundle's files are Pebble sstables. Allow that exact package path; the Pebble DB and every other Pebble package stay forbidden. Record the exception in the production-commonization as-built doc. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015KERH6Po1wqFPHTfp9WMgk --- ...26_10_16_implemented_production_commonization.md | 5 +++-- internal/backup/import_boundary_test.go | 13 +++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/docs/design/2026_10_16_implemented_production_commonization.md b/docs/design/2026_10_16_implemented_production_commonization.md index 61986c9c0..e2edc3793 100644 --- a/docs/design/2026_10_16_implemented_production_commonization.md +++ b/docs/design/2026_10_16_implemented_production_commonization.md @@ -111,6 +111,7 @@ The 2026-10-16 code audit (see `2026_10_16_proposed_test_suite_cleanup.md` for t **Design.** - `internal/backup` must not depend on `store`, `adapter`, `kv`, `distribution`, `internal/encryption` or Pebble. `proto` and the leaf key packages (`internal/s3keys`) remain allowed. + - Exception: Pebble's file-format reader `github.com/cockroachdb/pebble/v2/sstable`. `snapshot_sst.go` (PR #1276, merged independently of this change) reads the SST files of an `EKVSSTI1` snapshot ingest bundle with it; the snapshot decoder design names it as the one storage library the package links. The boundary test allows that exact package path; the Pebble DB and every other Pebble package stay forbidden. - A test in `internal/backup` walks the package's first-party import closure with `go/build` and fails on any forbidden package. - `redis_stream.go` gets a local copy of `store.StreamMetaTrimBinarySize`, pinned against `store` by a cross-check in `internal/backup`'s own tests (the existing `snapshot_reader_live_test.go` pattern). - Adapter-side cross-check tests (package `adapter`, which already imports `internal/backup`) build the same logical object with the live adapter builders and with the backup encoder. They decode the backup output with `backup.DecodeLiveEntries` and assert byte-identical keys and values for: @@ -184,7 +185,7 @@ 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 DB or `internal/encryption` (they link Pebble's `sstable` reader for SST ingest bundles). `elastickv-snapshot-prepare-restore` still does, through `internal/raftengine/etcd`, which the restore path needs. **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 +197,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, which allows only Pebble's `sstable` reader for SST ingest bundles, and every backup mirror of an adapter encoder has an adapter-side byte-for-byte cross-check (M4). - Shared server wiring lives in `internal/serverwiring`; the demo stays a separate binary (M5). diff --git a/internal/backup/import_boundary_test.go b/internal/backup/import_boundary_test.go index 06e3f41c6..b7ce0a5ff 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,15 @@ var backupForbiddenDeps = []string{ "github.com/cockroachdb/pebble", } +// backupAllowedDeps are exact package paths exempt from +// backupForbiddenDeps. Snapshot SST ingest bundles (EKVSSTI1) carry +// Pebble sstable files, so snapshot_sst.go reads them with Pebble's +// file-format reader; the live Pebble DB stays forbidden +// (docs/design/2026_04_29_implemented_snapshot_logical_decoder.md). +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. @@ -59,6 +69,9 @@ func TestBackupLinksNoLiveClusterCode(t *testing.T) { } func forbiddenBackupDep(pkgPath string) string { + if slices.Contains(backupAllowedDeps, pkgPath) { + return "" + } for _, forbidden := range backupForbiddenDeps { if pkgPath == forbidden || strings.HasPrefix(pkgPath, forbidden+"/") { return forbidden