[bootjp]

Design: docs/design/2026_10_16_partial_test_suite_cleanup.md

Summary

A survey of every proposed/partial design doc found two kinds of unfinished work:

1. Docs whose implementation is already on main but still named proposed. They are promoted to implemented in a dedicated commit: SIGTERM leadership transfer (main: transfer Raft leadership before shutdown #1291), DCO sign-off (ci: enforce sign-offs with the Probot DCO app #1306), the filesystem/SQLite Jepsen history budgets (Bound scheduled filesystem Jepsen rename histories #1321, Bound scheduled SQLite Jepsen histories #1333), Jepsen artifact archives, and the review-approval-label series (ci: label current HEAD after a zero-finding code review #1328, ci: grant permission to label pull requests #1332, ci: revoke approval for findings mapped to current HEAD #1336, ci: distinguish resolved findings from earlier revisions #1340). Their last "verify live" milestones are met by successful runs on main (Jepsen Test 40657220651, review label updater 40666978702).
2. The test-suite cleanup design. WP-0, WP-1, WP-2 and WP-8 had already landed. This PR implements WP-3 to WP-7 and the WP-9 to WP-12 migrations onto the shared test-support packages, and promotes the doc to partial.

The doc asks for one PR per WP. They are bundled here as one commit per WP (raftengine,encryption:, adapter: ×2, main,cmd,proxy:, test:), so each can be reviewed or split out separately.

The larger proposed designs (serializable isolation audit, typed error boundaries, migration ownership partitions, apply-time readiness evidence, scaling/positioning roadmaps) touch replication, OCC and routing. This PR does not attempt them.

WP-9 to WP-12 migrations

All four directory sets were migrated together, one commit per pattern. The before/after counts and every intentional exception are recorded in the design doc under "WP-9 to WP-12 results":

• Receive-or-fail select blocks: 276 candidates; 154 replaced by testutil.Recv / NoRecv / Closed. A bare receive becomes Closed only when the file closes the channel and Recv only when it sends on it. 122 remain, where the choice was ambiguous, the body uses break/goto, the failure is non-fatal, or hoisting a variable could shadow another name.
• Legacy list delta key copies: 5 → 0 (store.LegacyListMetaDeltaKey, keeping each package's sequence number).
• Encryption fixtures: deleted the production-exported test helper store/encryption_test_helpers.go; 37 encryptiontest call sites.
• gRPC loopbacks: 14 kv servers on grpctest.Serve, so they now use the production server options.
• Fake engines and coordinators: duplicates and pure forwarders moved onto raftenginetest / kvtest. The in-memory apply coordinators, including fstest.MemoryDispatcher, now dispatch through kvtest.LocalCoordinator, which gains a Clock option so a wrapper can share its HLC. Fakes whose behaviour is the subject of the test stay local (rule 4).

Intentional behaviour changes from the LocalCoordinator move:

• dispatches are serialized and elements are copied;
• ctx is checked before applying;
• the response carries CommitTS/CommitIndex;
• unknown ops return ErrUnknownOp;
• an invalid commit-timestamp patch surfaces kv's validation error instead of filesystem.ErrInvalid.

Behaviour change

None in production code. Production edits only delete functions that the retargets leave without callers, and fix comments that named them:
persistConfigSnapshot, encryptionAdminWiringForGroup, startPprofServer, startMetricsServer, startS3Server, prepareS3Server, readWriterRegistryLastSeen, extractS3Signature, bucketStore.invalidateQueue/bumpQueueEpoch, persistHashTxn, dispatchExpireForType, dispatchCollectionExpire, dispatchHLLExpire, dispatchStringExpire, and the key-only Redis read-fence chain plus its discarded nil-server branch. A repository-wide grep confirms none of them has a remaining caller.

Deviations from the plan (also recorded in the doc)

1. WP-5 retarget 1 can't be written as specified. queryLeasePrefix is only reached with an empty IndexName, so the hash-key-mismatch branch is unreachable. I deleted the duplicate test and added removing the dead branch as a follow-up (it is a production change).
2. WP-6 retarget 3: the live prepare functions moved to internal/serverwiring, which already tests them, so I deleted the root wrappers and their tests instead of retargeting them.
3. WP-4 retarget 6: redisDispatchTimeout is a 30 s constant. Instead of blocking, the test records the stream-deletion scan's deadline. The doc's mutation (context.Background() for the EXEC dispatch) survives because txnContext.dispatchContext bounds the context a second time. Breaking that second bound fails the test.

Mutation table

Each mutation was applied only through go test -overlay to a temporary copy.

WP	Mutation	Detecting test	Result
3	Remove the up-to-date fast path in persistConfigState	TestPersistConfigStateSkipsSerializationWhenNewerSnapshotExists	FAIL
3	Delete count > maxBootstrapBatchCount	TestBootstrap_RejectsOverCapBatchCount	FAIL (nil error)
6	Decorate the KEK unwrapper after the startup guards	TestTimedKEKUnwrapperCoversStartupPreflightUnwraps	FAIL ("0" not positive)
6	Never close the orphan in installOrAttach	TestFanoutClientForOrphanedDialClosed	FAIL (state IDLE)
6	Drop the virtual-hosted check in newS3Server	TestNewS3ServerRejectsVirtualHostedStyleConfig	FAIL
6	registryLastSeen returns 0	TestRegistryLastSeen	FAIL
6	Skip address validation in listenMonitoring	TestPrepareMonitoringServers	FAIL
5	Break the = delimiter in findSigV4Param	TestExtractSigV4Signature	FAIL
5	Skip the epoch bump in beginQueueReset	SQS throttle tests	FAIL
4	Zero ExpireAt in persistHashReadTimestampTxn	TestRedisHDelLegacyRewritePreservesLegacyTTL	FAIL
4	Zero the TTL in dispatchCollectionExpireReadTimestamp	TestRedisCollectionExpire*	FAIL
4	Encode the HLL without its expiry	TestRedisDispatchHLLExpireWritesInlineAnchorAndScanIndex	FAIL
4	redisLegacyBareReadFenceAllowed always true	TestRedisExecReadFenceUsesRedisStorageKeys	FAIL
4	Pass the uppercased name to the unsupported observer	TestRedisMetricsUnsupportedCommandRawInvalidUTF8Bytes	FAIL
4	hasHigherPriorityStringEncoding returns false	TestRedis_FastPathGuardDualEncodingReportsWrongType	FAIL
4	txnContext.dispatchContext returns an unbounded context	TestRedis_StreamMultiExecDelHonoursDispatchTimeout (both modes)	FAIL
4	context.Background() for the EXEC dispatch (plan's mutation)	same	PASS: re-bounded by dispatchContext (deviation 3)
9..12	kvtest.NewLocalCoordinator ignores LocalOptions.Clock	TestLocalCoordinatorSharesInjectedClock	FAIL

Test evidence

• go test -race ./internal/raftengine/... ./internal/encryption/...: pass, except TestFinalizeAsFSMFile_RenameFailureCleansUpSpool. It fails identically on a clean checkout because the container runs as root, so chmod 0o500 doesn't block writes. It is unrelated to this PR.
• go test -race . ./cmd/... ./proxy/... ./internal/serverwiring/...: pass.
• go test -race ./internal/admin/... ./distribution/... ./keyviz/...: pass.
• go test -race ./adapter/... .: root passes. The adapter package passes except TestMigrationQuorumPartitionAcceptance, which also fails on a clean origin/main checkout in this container and failed once in CI on this branch (see the PR comment). TestMigrationQuorumCommitFollowsForcedLeadershipTransfer fails intermittently under load and passed 3/3 in isolation. Neither test touches a file changed here.
• WP-9..12: go test -race ./adapter/... ./kv/... ./store/... ./internal/filesystem/... ./internal/raftengine/... ./internal/admin/... . pass, apart from the pre-existing failures above. TestKernelMountReaddirPagesLargeDirectory failed once in a five-package parallel run and then passed 3 full-package runs, 2 more parallel runs and 20 targeted repetitions under load.
• golangci-lint v2.15.0 --new-from-rev=origin/main (the CI configuration): 0 issues.
• Jepsen: not run. No runtime path changes; only unreferenced code is removed.

Self-review (five lenses)

1. Data loss: No persistence path changes. The deleted functions were unreferenced copies of live paths, and those live paths now have the tests (HDEL TTL preservation, persistConfigState ordering).
2. Concurrency / distributed failures: No production concurrency changes. New tests clean up their goroutines (Redis Run(), recorder mutex), and the throttle tests use the production reset sequence. The LocalCoordinator move serializes test dispatches, as fstest.MemoryDispatcher already did.
3. Performance: No hot-path change. Only test code and dead functions changed.
4. Data consistency: The read-fence, EXPIRE and HDEL retargets now pin the live OCC/ReadTimestamp paths. The bare-key !sqs| exclusion is mutation-checked. The migrated coordinators keep their timestamp policies (store+1 / StartTS+1 clamp for filesystem; HLC for adapter), and the adapter wrapper shares one HLC with the dispatcher.
5. Test coverage: Every deletion has a named survivor, and incident/review references were moved onto the survivors. Every retarget has a mutation row. Deviations and kept fakes are listed in the doc.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Fm5mogKjCk6edosPUsnMWZ

[coderabbitai]

Important

Review skipped

Too many files!

This PR contains 168 files, which is 68 over the limit of 100.

To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch.

Upgrade to a paid plan to raise the limit.

This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.

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: 4f89031c-e683-453c-9206-e7d677481bed

Commits

Reviewing files that changed from the base of the PR and between 30ec3e9 and 2134e7b.

Files selected for processing: 168

• adapter/admin_backup_renew_race_test.go
• adapter/admin_backup_test.go
• adapter/admin_grpc_test.go
• adapter/background_reservation_shutdown_test.go
• adapter/distribution_server_test.go
• adapter/dynamodb_lease_validation_test.go
• adapter/dynamodb_migration_test.go
• adapter/dynamodb_read_fence_test.go
• adapter/dynamodb_retention_test.go
• adapter/dynamodb_table_cleanup_shutdown_test.go
• adapter/dynamodb_test.go
• adapter/encryption_admin_test.go
• adapter/http_shutdown_test.go
• adapter/internal_admin_proposal_security_test.go
• adapter/internal_admin_proposal_test.go
• adapter/internal_migration_test.go
• adapter/redis_apply_observer_test.go
• adapter/redis_bzpopmin_wake_test.go
• adapter/redis_collection_fastpath_test.go
• adapter/redis_collection_ttl.go
• adapter/redis_collection_ttl_test.go
• adapter/redis_command_test.go
• adapter/redis_compat_commands_stream_test.go
• adapter/redis_delta_compactor_test.go
• adapter/redis_expire_cmds.go
• adapter/redis_hash_cmds.go
• adapter/redis_hello_test.go
• adapter/redis_hll_ttl_test.go
• adapter/redis_info_test.go
• adapter/redis_key_waiters_test.go
• adapter/redis_keys_pattern_test.go
• adapter/redis_list_dedup_test.go
• adapter/redis_list_pop_benchmark_test.go
• adapter/redis_lua_collection_fastpath_test.go
• adapter/redis_lua_pool_test.go
• adapter/redis_metrics_test.go
• adapter/redis_retry_test.go
• adapter/redis_server_setup_test.go
• adapter/redis_shutdown_test.go
• adapter/redis_txn.go
• adapter/redis_txn_test.go
• adapter/s3_admin_delete_shutdown_test.go
• adapter/s3_auth.go
• adapter/s3_cleanup_shutdown_test.go
• adapter/s3_test.go
• adapter/sigv4_test.go
• adapter/sqs_health_test.go
• adapter/sqs_messages.go
• adapter/sqs_shutdown_test.go
• adapter/sqs_test.go
• adapter/sqs_throttle.go
• adapter/sqs_throttle_test.go
• cmd/elastickv-admin/encryption_test.go
• cmd/elastickv-admin/main_test.go
• cmd/elastickv-snapshot-prepare-restore/main_fifo_unix_test.go
• distribution/migrator_export_plan_test.go
• distribution/watcher_test.go
• docs/design/2026_04_26_implemented_sqs_per_queue_throttling.md
• docs/design/2026_10_16_implemented_production_commonization.md
• docs/design/2026_10_16_partial_test_suite_cleanup.md
• docs/design/2026_10_22_implemented_sigterm_leadership_transfer.md
• docs/design/2026_10_24_implemented_dco_signoff.md
• docs/design/2026_10_24_implemented_filesystem_jepsen_history_budget.md
• docs/design/2026_10_24_implemented_review_approval_label.md
• docs/design/2026_10_25_implemented_jepsen_artifact_archives.md
• docs/design/2026_10_25_implemented_review_label_inline_findings.md
• docs/design/2026_10_25_implemented_review_label_permissions.md
• docs/design/2026_10_25_implemented_sqlite_jepsen_history_budget.md
• docs/design/2026_10_26_implemented_review_label_resolved_findings.md
• internal/admin/keyviz_fanout_test.go
• internal/admin/server_test.go
• internal/encryption/envelope_test.go
• internal/encryption/fsmwire/wire_test.go
• internal/encryption/nonce_factory.go
• internal/filesystem/fstest/memory.go
• internal/filesystem/fuseadapter/abandoned_mount_test.go
• internal/filesystem/fuseadapter/ack_test.go
• internal/filesystem/fuseadapter/adapter_test.go
• internal/filesystem/fuseadapter/barrier_test.go
• internal/filesystem/fuseadapter/helper_lookup_snapshot_test.go
• internal/filesystem/fuseadapter/server_ops_test.go
• internal/filesystem/fuseadapter/unmount_lazy_cancel_test.go
• internal/filesystem/fuseadapter/unmount_shutdown_join_test.go
• internal/filesystem/fuseadapter/wait_mount_cancel_test.go
• internal/filesystem/operation_journal_multigroup_test.go
• internal/filesystem/service_test.go
• internal/memwatch/memwatch_test.go
• internal/raftengine/etcd/dispatch_report_test.go
• internal/raftengine/etcd/encryption_test.go
• internal/raftengine/etcd/engine.go
• internal/raftengine/etcd/engine_applied_index_test.go
• internal/raftengine/etcd/engine_test.go
• internal/raftengine/etcd/grpc_transport_test.go
• internal/raftengine/etcd/removal_notification_test.go
• internal/snapshotoffload/scheduler_test.go
• keyviz/flusher_test.go
• keyviz/labels_test.go
• kv/coordinator_dispatch_test.go
• kv/coordinator_keyviz_label_test.go
• kv/coordinator_retry_test.go
• kv/fakes_test.go
• kv/fsm_dynamodb_timestamp_collision_test.go
• kv/fsm_restore_origin_encrypted_test.go
• kv/grpc_conn_cache_test.go
• kv/kvtest/coordinator.go
• kv/kvtest/coordinator_test.go
• kv/leader_admin_proposer_test.go
• kv/leader_proxy_test.go
• kv/leader_routed_store_test.go
• kv/lease_group_keys_test.go
• kv/lease_warmup_test.go
• kv/lock_resolver_test.go
• kv/migration_frontier_admission_test.go
• kv/raft_engine_test.go
• kv/raft_payload_wrapper_test.go
• kv/shard_key_test.go
• kv/shard_store_exact_version_test.go
• kv/shard_store_test.go
• kv/sharded_coordinator_leader_test.go
• kv/sharded_coordinator_registration_gate_test.go
• kv/sharded_coordinator_txn_test.go
• kv/sharded_membership_test.go
• kv/tso_floor_test.go
• kv/tso_test.go
• main.go
• main_admin_forward_test.go
• main_admin_lifecycle_test.go
• main_admin_shutdown_test.go
• main_encryption_admin.go
• main_encryption_admin_test.go
• main_encryption_fanout_test.go
• main_encryption_metrics_wiring_test.go
• main_encryption_registration.go
• main_encryption_registration_test.go
• main_encryption_rotate_on_startup_test.go
• main_encryption_startup_guard_test.go
• main_encryption_write_wiring_test.go
• main_filesystem_test.go
• main_grpc_stop_test.go
• main_metrics_test.go
• main_pprof_test.go
• main_s3.go
• main_s3_test.go
• main_shutdown_balance_test.go
• main_shutdown_handoff_test.go
• main_shutdown_signal_log_test.go
• main_shutdown_signal_test.go
• main_shutdown_test.go
• main_sqs.go
• monitoring/http_shutdown_test.go
• proxy/proxy_test.go
• proxy/pubsub_test.go
• store/encryption_compression_test.go
• store/encryption_glue.go
• store/encryption_test_helpers.go
• store/list_helpers_test.go
• store/lsm_store_encryption_metrics_test.go
• store/lsm_store_encryption_prop_test.go
• store/lsm_store_encryption_test.go
• store/lsm_store_registration_gate_test.go
• store/lsm_store_sync_mode_benchmark_test.go
• store/lsm_store_test.go
• store/migration_ownership_partition_boundary_test.go
• store/migration_ownership_partition_namespace_test.go
• store/migration_ownership_partition_snapshot_test.go
• store/snapshot_external_encryption_test.go
• store/snapshot_external_user_keys_test.go
• store/snapshot_pebble_sst_test.go

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 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

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

[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 (5e320c4):

• distribution/migrator_export_plan_test.go
• distribution/watcher_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 (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). 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. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
• kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
• store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
• 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.

[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 (2851afd):

• distribution/migrator_export_plan_test.go
• distribution/watcher_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 (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). 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. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
• kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
• store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
• 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]

CI status on this branch:

• golangci-lint (2851afd): three gci import-order findings from the testutil migration. Fixed in 8e7ef73, which reorders imports only. golangci-lint v2.15.0 --new-from-rev=origin/main now reports 0 issues locally.
• test (ubuntu-latest) (5e320c4): TestMigrationQuorumPartitionAcceptance failed at migration_quorum_partition_acceptance_test.go:700, a 5 s Eventually waiting for all four members to persist a snapshot covering the ACKed write. This PR doesn't modify the test or the migration/snapshot code it exercises. The test also fails twice in a row on a clean origin/main checkout in my sandbox; the Test workflow on main passed. There's no fix to port yet. The push of 8e7ef73 re-runs the job.

---

Generated by Claude Code

[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 (8e7ef73):

• distribution/migrator_export_plan_test.go
• distribution/watcher_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 (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). 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. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
• kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
• store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
• 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.

[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 (944b6ef):

• distribution/migrator_export_plan_test.go
• distribution/watcher_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 (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). 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. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
• kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
• store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
• 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.

[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 (3c50453):

• distribution/migrator_export_plan_test.go
• distribution/watcher_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 (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). 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. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
• kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
• store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
• 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]

@claude review
@codex review

Please review the current head (3c50453). The design spec is docs/design/2026_10_16_partial_test_suite_cleanup.md; the PR description has the mutation table and the deviations from the plan.

---

Generated by Claude Code

[claude]

Claude finished @bootjp's task in 26s —— View job

---

Review of 3c50453: no blocking findings

I reviewed the full diff against origin/main at 3c50453a2211ff058c12beb8ae4ff824125de047. I did not run the test suite or go vet in this job, because the sandbox denied the go command. The test results below come from the PR description, not from me.

What I checked

• Dead-code deletions: I searched all *.go files for invalidateQueue(, bumpQueueEpoch, persistConfigSnapshot(, dispatchCollectionExpire( and persistHashTxn(. None has a remaining reference, so the "no callers" claim holds for those. I read the surviving *ReadTimestamp paths and the queuedCommandReadFenceGroupTargets / commandReadFenceTargets paths, and they cover what was deleted. I also checked the nil-server branch that was removed, and it was only reachable through the deleted key-only chain.
• Comment fixes: The doc comment for invalidateQueueBuckets now attaches to the right function after invalidateQueue was removed. The dispatchStringExpireReadTimestamp comment is updated correctly.
• kvtest.LocalCoordinator Clock option (kv/kvtest/coordinator.go:92-98): the injected clock is set through the same sync.Once that lazily initialises the default clock, so it can't be overwritten later. The mutation test TestLocalCoordinatorSharesInjectedClock covers it.
• fstest.MemoryDispatcher move onto LocalCoordinator: the timestamp policy (store+1, StartTS+1 clamp for transactions), read-key validation, commit-timestamp patching and DelPrefix rejection are all kept by the options. The behaviour changes the PR lists are all expected in test-only code: serialized dispatches, the ctx check, the response carrying CommitTS, ErrUnknownOp, and kv's validation error replacing filesystem.ErrInvalid.
• Production edits: Only unreferenced functions and comments change, which matches "no production behaviour change". The doc renames follow the partial / implemented lifecycle in CLAUDE.md, and the Design: line is present.

Non-blocking notes

1. TestMigrationQuorumPartitionAcceptance still fails in CI, and by your account on a clean main in your sandbox as well. That makes it a separate flake, but it will block merge if the check is required. Consider filing it so it doesn't get lost.
2. internal/encryption/nonce_factory.go: the new comment, "Tests construct it directly", is accurate. It might be worth adding one line saying why store.CounterNonceFactory is gone, for future readers.
3. The 122 receive-or-fail select blocks left unmigrated are recorded in the doc. A follow-up count in the doc once they are cleaned up would keep the WP-9..12 results table honest.
4. The follow-up to remove the dead hash-key-mismatch branch (WP-5 deviation 1) is a production change. It should get its own PR with its own test, per the design-doc workflow.

TLA+ divergence checklist: the only anchored files touched are distribution/migrator_export_plan_test.go and distribution/watcher_test.go. Both are test-only, with small helper swaps. No HLC, coordinator, FSM, OCC or MVCC production code changed, so there is no drift from the spec.

I found nothing blocking.

[chatgpt-codex-connector]

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-29T00:11:51.336822Z	2134e7b	Draft marked ready
🔒 Security Review	✅ Completed 2026-10-29T00:12:35.048720Z	2134e7b	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 (2134e7b):

• distribution/migrator_export_plan_test.go
• distribution/watcher_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 (strategy (c) Observe(MaxAppliedHLC)), and the commit-time ceiling fence (fail-closed when wall_now >= physicalCeiling). Any change to the bit layout (48/16), the CAS loop, or the ceiling getter/setter is in scope.
• kv/coordinator.go, kv/sharded_coordinator.go — RunHLCLeaseRenewal, hlcRenewalInterval, hlcPhysicalWindowMs constants, and the new-term detection that calls Observe(fsm.MaxAppliedHLC()) (strategy (c)). 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. (M2 spec will land OCC-1..OCC-5; until then the spec doc §5.2 is the contract.)
• kv/fsm.go — FSM apply of HLC lease entries (SetPhysicalCeiling), and any future MaxAppliedHLC() accessor that strategy (c) needs.
• store/mvcc_store.go — version visibility, snapshot install, and the MVCC-1..MVCC-4 invariants (M3 scope).
• 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.