[bootjp]

Design: docs/design/2026_10_16_partial_test_suite_cleanup.md

Summary

Implements the "Production dead code that is only tested" follow-up from the test-suite cleanup design. Before each deletion I re-checked that the code has no production caller on current main. Where a test exercised live behaviour through the dead code, the test was ported to the live path first. One commit per area:

Commit	Removed	Live-relevant tests
monitoring:	observeLeaderChange (duplicate of the inline Inc in setLeaderMetric)	The metric test now drives two leader changes through setLeaderMetric. Mutation: dropping the inline Inc fails it.
fskeys:	ChunkRouteHomeFromKey	The chunk-prefix rejection moved into TestChunkRoutePartsFromKey.
adapter: (SQS)	throttleAttributesPresent plus its name slice; sqsMsgByAgePrefixesForQueue	None. setQueueAttributes never gated on the helper. The reaper walks legacy and per-partition prefixes itself, and the comment now names those functions.
backup:	ResolveCommitTS / ErrLastCommitTSRegression, peekMsgDataKey, HasInlineTTL, IsBlobAtomicWrite* plus the disk_full_*.go files, IsBinarySegment, effectivePartitionCount, WriteChecksumsWithVirtualFile	The --last-commit-ts floor is enforced and tested in the encode CLI's resolveLastCommitTS.
adapter: (Redis)	redis_transcord.go and its property test (CLAUDE.md citation updated); selectXReadEntries; unmarshalStreamValue / redisStreamValueFromProto and their benchmarks	loadStreamAt deliberately discards legacy blobs (PR #620). marshalStreamValue moved to a test file, where it remains the legacy-blob fixture for the cleanup tests.
autosplit:	ObserveSnapshot, ReadCommittedWindows and their three types	The four reset tests now drive CommittedWindowsFromColumns + Evaluate, as the scheduler does, with unchanged assertions.
encryption:	CheckLocalEpochRollback	Its tests pinned the old single-shot rule, which contradicts the live checkLocalEpochRollbackBeforeBump / AfterBump guard; that guard has its own rollback, equal-epoch, missing-row and foreign-row tests. Comments now name the live checks.
etcd:	MigrateFSMStore	The two root node-ID-collision tests seed persisted peers through PreparePhysicalSnapshotRestore. Mutation: making the guard ignore persisted peers fails both. finalizeMigrationDir / errMigrationDestinationExists move to external_snapshot_restore.go, which uses them.
etcd:	loadOrCreateState, bootstrapState, saveSplitState, rewriteEntriesFile, removeFileIfExists	TestOpenRestoresLegacySnapshotState covers the live legacy migration (loadLegacyOrSplitState). The pre-WAL writers that tests use to seed legacy directories moved to persistence_legacy_writers_test.go.
docs:	—	The follow-ups list records the removals.

Not removed.

• distribution.IsCatalogDeltaKey: it has a production caller in kv/raft_bootstrap_guard.go.
• The audit's "unused options": the doc never recorded the list. A scan finds about 70 With* options set only by tests, mostly clock, interval, logger and timeout seams, so each needs a separate decision. This is left open in the doc.

Behaviour change

None at run time; every removed symbol was unreachable from production code. Net change is −1,400 lines.

Test evidence

• go build ./...: passes.
• GOOS=windows go build ./internal/backup/: passes (the deleted disk_full_windows.go was the only Windows-specific file there). The full Windows build fails in the vendored go-fuse, as on main.
• go test -race passes on monitoring, fskeys, backup, autosplit, encryption and the snapshot-encode CLI.
• go test passes on etcd, the root startup-guard tests, and the adapter SQS and Redis subsets. The one exception is TestFinalizeAsFSMFile_RenameFailureCleansUpSpool, which fails identically on main in a root container (chmod does not block writes for root).
• Mutation checks: the two listed in the table above.
• golangci-lint v2.15.0 --new-from-rev=origin/main ./...: 0 issues.
• Jepsen: not run; nothing reachable changes.

Self-review (five lenses)

1. Data loss: no write, apply or persistence path changes. The legacy pre-WAL reader and migration stay; only the unused writers and old entry point go.
2. Concurrency / distributed failures: no reachable code changes. The encryption startup guard is untouched, and its live tests still run.
3. Performance: none. RedisServer loses one unused field.
4. Data consistency: the live paths that replaced the dead helpers (resolveLastCommitTS, the reaper's own prefix walks, CommittedWindowsFromColumns + Evaluate, PreparePhysicalSnapshotRestore) are what the ported tests now exercise.
5. Test coverage: every deleted test was either testing only the removed code or was ported to the live path. Two mutation checks confirm the ported tests still bite.

---

Generated by Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 32 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 862cb80c-bb23-4fae-829e-39eae3e1aa4a

Commits

Reviewing files that changed from the base of the PR and between d3407d1 and 55bf5a3.

Files selected for processing: 44

• CLAUDE.md
• adapter/redis.go
• adapter/redis_compat_types_benchmark_test.go
• adapter/redis_storage_codec.go
• adapter/redis_storage_codec_test.go
• adapter/redis_stream_cmds.go
• adapter/redis_transcoder_prop_test.go
• adapter/redis_transcord.go
• adapter/sqs_keys.go
• adapter/sqs_keys_test.go
• adapter/sqs_throttle.go
• adapter/sqs_throttle_test.go
• distribution/autosplit/sampler_reader.go
• distribution/autosplit/sampler_reader_test.go
• docs/design/2026_10_16_partial_test_suite_cleanup.md
• internal/backup/checksums.go
• internal/backup/disk_full_other.go
• internal/backup/disk_full_unix.go
• internal/backup/disk_full_windows.go
• internal/backup/encode.go
• internal/backup/encode_sqs.go
• internal/backup/encode_sqs_partitioned_test.go
• internal/backup/encode_test.go
• internal/backup/filename.go
• internal/backup/filename_test.go
• internal/backup/redis_string.go
• internal/backup/redis_string_test.go
• internal/backup/sqs.go
• internal/backup/sqs_test.go
• internal/encryption/errors.go
• internal/encryption/local_epoch_rollback.go
• internal/encryption/local_epoch_rollback_test.go
• internal/fskeys/keys.go
• internal/fskeys/keys_test.go
• internal/raftengine/etcd/external_snapshot_restore.go
• internal/raftengine/etcd/migrate.go
• internal/raftengine/etcd/migrate_test.go
• internal/raftengine/etcd/persistence.go
• internal/raftengine/etcd/persistence_legacy_writers_test.go
• internal/raftengine/etcd/persistence_test.go
• main_encryption_registration.go
• main_encryption_startup_guard_test.go
• monitoring/raft.go
• monitoring/raft_test.go

• 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

• X
• Mastodon
• Reddit
• LinkedIn

[github-actions]

TLA+ spec divergence review (auto-triggered)

This PR touches files that the TLA+ safety spec has an anchor on (per
docs/design/2026_05_28_implemented_tla_safety_spec.md §3),
so an AI review is requested below to verify the implementation has not drifted
from the model.

Anchored files changed in this PR head (55bf5a3):

• distribution/autosplit/sampler_reader.go
• distribution/autosplit/sampler_reader_test.go

What to check, by subsystem:

• kv/hlc*.go — Next() must respect the HLC-4 preconditions (i)/(ii)/(iii) from the design doc: bounded skew, logical-counter handoff on leader change (the TSO allocation floor; legacy modes have none and are pinned as known gaps), and the NextFenced ceiling fence (fail-closed when wall_now >= physicalCeiling or when the next value's physical half exceeds it). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/tso*.go — RaftTSOAllocator must Observe the committed allocation floor before reserving and commit the new floor before handing a window out (HLC.tla IssuanceMode = "tso").
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and which node stamps StartTS / CommitTS (HLC-3; legacy ShardedCoordinator stamping is the known gap G10). Any change to renewal cadence, group selection, or fail-closed behaviour is in scope.
• kv/transaction.go, kv/lock_resolver.go — OCC commit-ts assignment, lock-map encoding (key, lock_ts) -> start_ts, and the LockResolver action OCC-3 depends on (OCC.tla; G0 / G1 are pinned as known gaps).
• kv/fsm.go, kv/fsm_migration_serving.go — FSM apply of HLC lease entries (SetPhysicalCeiling), the one-phase / PREPARE / COMMIT validation OCC.tla models, verifyComposed1, and the migration serving guard Composed.tla relies on for unpinned writes.
• store/mvcc_store.go, store/lsm_store.go — version visibility, resumable compaction, the commit-ts apply fence, and the MVCC-1 / MVCC-4 / MVCC-5 invariants.
• adapter/ts.go — the snapshot start timestamp OCC.tla assumes (applied watermark; MaxUint64 on an empty store).
• distribution/** — route catalog versioning, SplitRange atomicity, and CatalogWatcher async fan-out (M4 scope).

If the change is correct but requires a spec update, edit tla/hlc/HLC.tla (or the corresponding M2..M5 module once landed) and the design doc in the same PR. The tla-check workflow runs the TLC model check on the same paths.

---

@claude review please verify TLA+ spec divergence per the checklist above.

@codex review please verify TLA+ spec divergence per the checklist above.

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-09T17:53:31.899939Z	55bf5a3	Draft marked ready
🔒 Security Review	✅ Completed 2026-11-09T17:55:11.497208Z	55bf5a3	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.

[github-actions]

TLA+ spec divergence review (auto-triggered)

This PR touches files that the TLA+ safety spec has an anchor on (per
docs/design/2026_05_28_implemented_tla_safety_spec.md §3),
so an AI review is requested below to verify the implementation has not drifted
from the model.

Anchored files changed in this PR head (55bf5a3):

• distribution/autosplit/sampler_reader.go
• distribution/autosplit/sampler_reader_test.go

What to check, by subsystem:

• kv/hlc*.go — Next() must respect the HLC-4 preconditions (i)/(ii)/(iii) from the design doc: bounded skew, logical-counter handoff on leader change (the TSO allocation floor; legacy modes have none and are pinned as known gaps), and the NextFenced ceiling fence (fail-closed when wall_now >= physicalCeiling or when the next value's physical half exceeds it). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/tso*.go — RaftTSOAllocator must Observe the committed allocation floor before reserving and commit the new floor before handing a window out (HLC.tla IssuanceMode = "tso").
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and which node stamps StartTS / CommitTS (HLC-3; legacy ShardedCoordinator stamping is the known gap G10). Any change to renewal cadence, group selection, or fail-closed behaviour is in scope.
• kv/transaction.go, kv/lock_resolver.go — OCC commit-ts assignment, lock-map encoding (key, lock_ts) -> start_ts, and the LockResolver action OCC-3 depends on (OCC.tla; G0 / G1 are pinned as known gaps).
• kv/fsm.go, kv/fsm_migration_serving.go — FSM apply of HLC lease entries (SetPhysicalCeiling), the one-phase / PREPARE / COMMIT validation OCC.tla models, verifyComposed1, and the migration serving guard Composed.tla relies on for unpinned writes.
• store/mvcc_store.go, store/lsm_store.go — version visibility, resumable compaction, the commit-ts apply fence, and the MVCC-1 / MVCC-4 / MVCC-5 invariants.
• adapter/ts.go — the snapshot start timestamp OCC.tla assumes (applied watermark; MaxUint64 on an empty store).
• distribution/** — route catalog versioning, SplitRange atomicity, and CatalogWatcher async fan-out (M4 scope).

If the change is correct but requires a spec update, edit tla/hlc/HLC.tla (or the corresponding M2..M5 module once landed) and the design doc in the same PR. The tla-check workflow runs the TLC model check on the same paths.

---

@claude review please verify TLA+ spec divergence per the checklist above.

@codex review please verify TLA+ spec divergence per the checklist above.

[bootjp]

test shard (rest) failure is not caused by this PR. The failing test is kv.TestLeaderRoutedStore_GlobalLastCommitTS_ProxiesToLeader: it expected 99 and got 0 (job).

Why it is not this PR's: this PR does not touch kv/. The same test is green on main's latest Test run. Locally it passes 30/30 with -race.

Root cause: GlobalLastCommitTS gives the proxy RPC globalLastCommitTSTimeout (200 ms). On error or timeout it falls back to the local store's LastCommitTS, which is 0 in this test. The test makes the first call on a cold connection, so the gRPC dial and the RPC share that 200 ms budget. In the failing run the test took 0.43 s; under the parallel -race shard the budget can run out, and the call returns the local 0. This became more likely once kv tests started running in parallel (1c8089e6).

Proposed fix (test-only): connect before measuring, so the timeout covers only the RPC. Verified locally with -race, ×20. I have kept it out of this PR to avoid widening it; it can go in its own PR.

@@ func TestLeaderRoutedStore_GlobalLastCommitTS_ProxiesToLeader(t *testing.T) {
 	s := NewLeaderRoutedStore(store.NewMVCCStore(), coord)
 	t.Cleanup(func() { _ = s.Close() })
 
+	// GlobalLastCommitTS gives the proxy RPC globalLastCommitTSTimeout and
+	// falls back to the local value on timeout. Connect first so the dial
+	// does not spend that budget under a loaded parallel -race run.
+	conn, err := s.connCache.ConnFor(addr)
+	require.NoError(t, err)
+	conn.Connect()
+	require.Eventually(t, func() bool { return conn.GetState() == connectivity.Ready }, 5*time.Second, 10*time.Millisecond)
+
 	ts := s.GlobalLastCommitTS(context.Background())

The fix also needs the time and google.golang.org/grpc/connectivity imports. I am re-running the failed job once.

---

Generated by Claude Code