Repository navigation
Conversation
The Redis delta compactor's TTL-inline migrator does no work in the production server. main.go hands it a startupGatedCoordinator, and NewDeltaCompactor wraps that in WithKeyVizLabel. The label wrapper always exposes LocalLeaderGroupIDs and answers nil when its inner value lacks the method, and the startup gate never forwarded it. The migrator then takes its per-group branch with zero groups, plans no scans, and never reaches the full-keyspace fallback. Pin the real chain: a ShardedCoordinator leading groups 1 and 2 and following group 3, wrapped exactly as main.go wraps it. The test fails here because LocalLeaderGroupIDs comes back nil through the chain and SyncOnce issues no ScanGroupAt at all. The fix follows in the next commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
…rappers
The TTL-inline migrator asks its coordinator which data groups this node
leads so it scans only those. In the server that coordinator is
WithKeyVizLabel(startupGatedCoordinator{ShardedCoordinator}). The label
wrapper always exposed LocalLeaderGroupIDs and answered nil when its
inner value lacked the method, and the startup gate never forwarded it.
The migrator read nil as "this node leads no group", planned no scans,
and never reached its full-keyspace fallback. The migration has done
nothing in production since a36dac7.
Forward LocalLeaderGroupIDs through the startup gate, and make the answer
carry whether it is known. kv.LocalLeaderGroupsCoordinator now returns
(ids, known); ShardedCoordinator always knows, and decorators forward
through kv.LocalLeaderGroupIDsThrough, which reports a coordinator
without the capability as unknown. A decorator that drops the forward
can no longer turn "unknown" into "no groups": the migrator falls back
to the full-keyspace scan it already had, filtered per key by
IsLeaderForKey, and logs once that it did. A known empty list still
plans nothing, since this node then leads no group.
The regression test from the previous commit now passes; only its two
probe lines change for the new signature. Adapter and kv tests pin the
unknown and known-empty cases; both fail against the old "nil means no
groups" behaviour.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
…rappers Capabilities are optional interfaces found by type assertion, so a wrapper that does not forward one fails silently: the probe misses and the consumer takes a fallback, or the outer wrapper answers on the inner's behalf. That has now cost the Redis read fence its group routing, Internal.Forward its route-floor gate, and the Redis TTL-inline migration all of its work, each time with green unit tests that used an unwrapped coordinator. Classify every exported ShardedCoordinator method by the surface its consumers probe: the adapters' WithKeyVizLabel(startupGatedCoordinator), the startupGatedCoordinator main probes itself (internal gRPC server options, S3 blob peer discovery), or the raw coordinator main drives directly (lifecycle, HLC lease renewal, autosplit, the legacy TSO bridge). A new method fails the test until it is classified. Each forwarded capability is called through its surface against a spy and must reach the spy and return its result and error. The test fails on one gap: the startup gate does not forward LinearizableReadForKey, so WithKeyVizLabel serves it from the default group's LinearizableRead and a key in another group would be fenced against the wrong group. Nothing in the server calls it through the adapters today, so the gap is latent. The next commit closes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
WithKeyVizLabel exposes LinearizableReadForKey to the adapters and serves it from the plain LinearizableRead when its inner coordinator lacks it. The startup gate in between did not forward it, so through the adapters' coordinator a key owned by a non-default group would get a read fence on the default group. Nothing in the server calls it that way today, so the gap was latent; the capability guard from the previous commit flagged it. Forward it, falling back to LinearizableRead for an inner coordinator without a key-routed fence -- one that owns a single group -- as WithKeyVizLabel already does. The guard now passes, and a unit test pins the fallback branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 64.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 16 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check タイトルは、ローカルリーダーの各シャードでRedis TTL-inline移行を復旧するという主要変更を正確かつ具体的に示しています。
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 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.
Comment @coderabbitai help to get the list of available commands.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (35f698e):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35f698ee9d
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review on #1278 found that the per-group TTL-inline scans only work on a single-route store. The migrator scans each locally led group with ShardStore.ScanGroupAt over a bare anchor prefix such as "!redis|str|". ScanGroupAt resolves the requested range through the route catalog, and a bare prefix resolves to the first route only, so every other group is refused with ErrExplicitGroupRouteOwnerMismatch. A node leading several shards migrates at most the first route's anchors and fails every tick on the rest. The existing tests hand the compactor a store whose ScanGroupAt ignores routing, so they could not see it. Run the migrator over a real ShardStore split at "m" across two groups and require legacy string and hash anchors on both sides to migrate. It fails here on group 2's scans for all eight anchor prefixes. The fix follows in the next commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
The TTL-inline migrator scanned each locally led group with ShardStore.ScanGroupAt over a bare anchor prefix. ScanGroupAt resolves the requested range through the route catalog first, and a bare prefix such as "!redis|str|" resolves to the first route only, so every other group was refused with ErrExplicitGroupRouteOwnerMismatch. On a store with more than one route, a node migrated at most the first route's anchors and failed every tick on the rest. Add ShardStore.ScanGroupOwnedAt. It scans the raw range on the group's own store, fenced like the other explicit-group reads, and keeps only keys whose route the group currently owns, judged by the same routeAndGroupForKey that routes writes. It keeps paging past skipped keys, so fewer than limit results still means the range is exhausted. A group's store can still hold copies of ranges it no longer owns, and ScanGroupAt returned those to the first route's group; acting on one could rewrite a key from a stale value, so skipping them matters. Each call reads about one page, the same per-tick cost a36dac7 intended, instead of one walk per owned route. The migrator uses it for its per-group plans. The split-store regression test from the previous commit now passes. kv tests cover both anchor encodings (user key directly after the prefix, and length prefixed), a stale copy, and paging past a page of skipped keys. Also correct the fallback wording: when the groups are unknown, the migrator runs one ScanAt over the whole prefix, which on a split catalog reaches only the first route for the string, HLL, TTL and list anchors. That path is not new and is not reached in the server. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
bootjp
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 301aaa060c
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…y gaps Review of 301aaa0 found three gaps in ShardStore.ScanGroupOwnedAt: - It scans with a bare group route, which carries no staged-visibility flag, so a route being imported is read from its live rows only. An older live copy could feed a rewrite while a newer staged copy waits for promotion. - It reads physical pages until it has limit owned rows. A group holding a large backlog of rows of ranges it no longer owns turns one call into a walk over the whole backlog, which overruns the tick budget and never advances the cursor. - It keeps a key only when its logical owner is the scanned group. Hash, set and zset rows written before wide-column routing still sit at the route of their raw key, and reads fall back to them there, so dropping them leaves those anchors unmigrated. Pin each one: the staged route's key must be deferred, one call must read at most one page, a legacy-route row must be served with the value a point read sees, a tick must stop after one page of stale rows and resume on the next, and the migrator must rewrite a legacy-route hash meta into its user key's group. All five fail here; the fix follows in the next commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
…egacy rows Close the three gaps the previous commit reproduced in ShardStore.ScanGroupOwnedAt: - One physical page per call. It now reads a single page and returns the last key that page read when it was full, nil once the range is exhausted. The migrator moves its cursor to that key instead of to the last row it kept, so a page holding no row the group serves still advances the cursor and a tick stays within one page per plan. - Staged routes are deferred. A key whose route is under staged visibility is skipped: the page reads live rows only, and an older live copy must not feed a rewrite while a newer staged copy waits for promotion. Such keys are picked up on a later pass. - Legacy-route rows are served. Hash, set and zset rows still stored at the route of their raw key are kept when that route is the scanned group. The page has already re-read them through the point-read path, so they carry the value a reader sees, not a shadowed copy. The migrator still writes one only when this node leads the user key's group, as the HLC rule requires. The earlier stale-copy test used a hash key, which this commit now correctly serves at its legacy route; it uses string keys, which have no legacy route, instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50494e865a
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
main (#1277) added kv.CoordinatorForwarder, which both coordinator wrappers now embed, and fixed the same wrapper bug this branch fixed by hand-forwarding. Resolution: - Take main's startupGatedCoordinator and keyVizLabeledCoordinator. The forwards this branch added to them (LocalLeaderGroupIDs on both, LinearizableReadForKey on the gate) are served by the forwarder. - Keep this branch's known contract on the forwarder itself: CoordinatorForwarder.LocalLeaderGroupIDs returns ([]uint64, bool) through kv.LocalLeaderGroupIDsThrough, so an inner coordinator without the capability stays unknown and the TTL-inline migration falls back to whole-prefix scans instead of planning none. - Update main's chain and plan tests to the ([]uint64, bool) signature and to ScanGroupOwnedAt. - The behavioural capability guard now classifies methods with main's shardedMethodsNotForwarded list and calls every forwarded method through both wrappers. Drop the gate's LinearizableReadForKey fallback test: the gate no longer has its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
…tate ScanGroupOwnedAt serves a hash, set or zset row stored at the route of its raw key whenever the scan canonicalized it, and never a stream row: - A legacy stream meta row whose user key belongs to another group is dropped, so the stream TTL-inline handler never migrates it, even when this node leads both groups (Codex review). - A wide-column key the logical owner also holds is served by both groups. The migrator then rewrites it from both plans in one tick, and the second dispatch fails its OCC check. Point reads fall back to the legacy route only when the logical owner holds no version of the key, so that is when the legacy group should serve it. These tests fail until the scan follows that rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
ScanGroupOwnedAt now serves a row stored at the route of its raw key only when this node leads the key's logical owner and that group holds no version of the key at the read timestamp: exactly when a point read falls back to the legacy route, so the row is the value a reader sees. - Stream metadata is covered: the legacy route candidate is now legacyPointRouteKey, the one point reads use, which includes streams. Streams are not canonicalized by the scan, and they need not be: when the owner holds nothing, the raw row is the canonical value. - A wide-column key the owner also holds is served only by the owner, so the migrator rewrites it once per tick instead of from two plans whose second dispatch failed its OCC check. - When another node leads the owner, or this node does not host it, the row is left out; only the owner's leader may rewrite the key. Cost: one local version probe per legacy-route row, through the same lease-fenced check point reads already use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
Every TTL-inline scan plan (one per handler and locally led group) shares the compactor's per-tick budget, and every tick starts from the first handler's first group. When the earlier plans use up the budget, the later handlers and groups are never reached (Codex review). With three local leader groups, 24 budget-limited ticks scan the first plan 24 times and no other. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
Each tick now flattens every handler's scan plans (one per handler and locally led group) into one list and starts at the plan after the last one the previous tick ran. A tick that runs out of budget hands the rest of the rotation to the next tick instead of starting over at the first handler's first group, so no handler or group is starved while the earlier plans keep using up the budget. The background delta handlers already rotate this way. Their two cursor helpers now take the cursor they rotate, and both loops share them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
ScanGroupOwnedAt reads its page through scanRouteAtForwardPage, which re-reads every hash, set and zset row through the point-read path. Under the owner-holds-nothing rule every served row already carries the value a point read returns, so those reads are redundant, and some do harm: - one extra point read per owned wide-column row; - a proxied read to the owner's leader for every legacy-route row the scan then drops; - a page that fails outright with "leader not found" when a legacy row's owner has no known leader, so the plan's cursor cannot advance. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
The three forward page paths (local, leader, proxy) each re-read the page's hash, set and zset rows through the point-read path. That pass now runs once, in scanRouteAtForwardPage, after whichever path read the page, so its callers behave as before. ScanGroupOwnedAt reads the page through the new scanRouteAtForwardStoredPage, without the pass: every row it serves is owned or held by no one at the owner, so it already carries the value a point read returns. This drops one point read per owned wide-column row, a proxied read per legacy-route row the scan then drops, and the page failure when such a row's owner has no known leader. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019P5kzueGgfe3F5iSe8TMtC
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (a73a7c0):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a73a7c0a59
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
The failing test is Two commits that landed on main independently collide:
This PR doesn't touch There is no fix PR yet. Choosing between the two designs is a maintainer call. The narrowest change I can see allows the sstable reader explicitly, since it is a file-format reader rather than a live store. I checked this patch on a scratch checkout of main, where --- a/internal/backup/import_boundary_test.go
+++ b/internal/backup/import_boundary_test.go
@@ -24,6 +24,13 @@ var backupForbiddenDeps = []string{
"github.com/cockroachdb/pebble",
}
+// backupAllowedDeps are exceptions inside a forbidden prefix. Snapshot SST
+// ingest bundles (snapshot_sst.go) are files in pebble's sstable format, and
+// reading them takes pebble's sstable reader, not a live store.
+var backupAllowedDeps = []string{
+ "github.com/cockroachdb/pebble/v2/sstable",
+}
+
@@ -59,6 +66,11 @@ func TestBackupLinksNoLiveClusterCode(t *testing.T) {
func forbiddenBackupDep(pkgPath string) string {
+ for _, allowed := range backupAllowedDeps {
+ if pkgPath == allowed {
+ return ""
+ }
+ }
for _, forbidden := range backupForbiddenDeps {The other option is to move the SST reading out of Generated by Claude Code |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
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
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (5ac9319):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ac93191a9
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return plans | ||
| c.warnTTLInlineLocalLeaderGroupsUnknown() | ||
| } | ||
| return []ttlInlineMigrationScanPlan{{cursorName: h.typeName}} |
There was a problem hiding this comment.
When c.st is a split *kv.ShardStore but a coordinator decorator does not forward LocalLeaderGroupIDs, this fallback creates a plain ScanAt plan. For bare string, HLL, TTL-index, and list prefixes, routesForInternalScanWithVersion resolves only the prefix's catalog route, so IsLeaderForKey can filter returned rows but cannot discover anchors on the other shards. Thus the advertised fail-safe still silently leaves most locally led shards unmigrated in exactly the missing-capability scenario; the fallback needs a way to enumerate/scan every relevant group rather than treating ScanAt as a whole-prefix physical scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The limit is real: on a split catalog, the unknown-groups fallback's ScanAt reaches only the first route for the bare string, HLL, TTL-index and list prefixes. This was already the behaviour before a36dac7. I'm not adding code for it here, because no caller can reach that combination:
main.go:413passesstartupGatedCoordinator{ShardedCoordinator}, wrapped later inWithKeyVizLabel. Both wrappers embedkv.CoordinatorForwarder, so the groups are always known.cmd/server/demo.go:589passes a single-groupCoordinatewith a plain store. That store is not a group scanner, so the fallback runs oneScanAtover a single route, which covers the whole prefix.- A future decorator that drops the capability fails
TestStartupWrappersForwardShardedCoordinatorCapabilitiesand the wiring test before it ships. Even if it did ship, the fallback logs once instead of silently planning nothing.
Making the fallback cover every shard would mean having each node scan every group, proxied to remote leaders. Those are the whole-keyspace scans a36dac7 removed, built only for a configuration the tests already reject. I've put this call to the maintainer and am leaving the thread open in case they want it anyway.
Generated by Claude Code
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ac93191a9
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if g, ok := s.groupForID(owner.GroupID); !ok || g == nil || g.Store == nil { | ||
| return false, nil | ||
| } | ||
| exists, answered, err := s.routeHasVersionAtOrBefore(ctx, owner, key, ts) |
There was a problem hiding this comment.
When the scanned legacy group and logical-owner group are co-led but the owner has a committed-yet-unapplied metadata write, this direct presence probe can report the owner key absent because routeHasVersionAtOrBefore only verifies leadership and does not perform the owner group's lease/read-index fence. If another local group has already advanced readTS beyond that write's commit timestamp, the migrator then derives a rewrite from the stale legacy row; after the pending owner write applies, the migration's OCC check sees its timestamp as <= readTS and overwrites it rather than conflicting. Fence the owner group before probing it, and cover this committed-but-unapplied cross-group sequence.
AGENTS.md reference: AGENTS.md:L22-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I traced this. The interleaving is real, but it is not specific to this probe, and a probe-local fence would not close it.
The probe is already fenced on the owner group. routeHasVersionAtOrBefore returns "cannot answer" unless isLinearizableRaftLeader(ctx, ownerEngine) passes. That check runs leaseReadEngineCtx on the owner group's engine: it uses the lease when the lease is fresh, and otherwise a LinearizableRead that waits for apply. This is the same fence, and the same call, that GetAtWithReadFence uses through routeMissStopsPointFallback to decide whether a point read falls back to the legacy route.
What remains is audit gap G0 across groups. That gap is open in docs/design/2026_09_26_proposed_serializable_isolation_audit.md §2 ("Multi-group deployments"), reproduced by TestApplyOrder_GlobalSnapshotMissesLaggingGroupWrite. The compactor's readTS comes from snapshotTS, the maximum applied watermark over all groups. A write to owner group P whose commit timestamp is below that maximum, but which has not yet applied on P, is missed by any read at readTS, and OCC ignores it because c1 ≤ startTS. A barrier on P closes only the committed-but-unapplied half: a write still in flight when the barrier runs lands later and is missed the same way.
The same exposure exists today, independent of this PR:
- the point-read fallback, which returns the legacy value under that same interleaving;
- this migrator's owned-row path, and
buildElems' reads of P; - every adapter transaction started at the global snapshot.
The fix is the audit's A0: advance a lagging group's watermark to the snapshot before reading it, plus the apply-order fence. That is a per-group Raft entry on the read path, outside this PR's scope. Adding a LinearizableRead here would cost a read-index round trip per legacy row and still leave the in-flight half open.
I'm leaving this thread open for the maintainer to decide.
Generated by Claude Code
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
2 participants
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:
WithKeyVizLabel(startupGatedCoordinator{ShardedCoordinator}). The migrator asks that value which data groups this node leads (LocalLeaderGroupIDs) and scans only those. The label wrapper answerednilwhen its inner lacked the method, and the startup gate never forwarded it, so the migrator planned zero scans.kv.CoordinatorForwarder. This branch is merged with main and takes that forwarder.knowncontract (below). With it, a coordinator without the capability falls back to whole-prefix scans instead of planning none.ShardStore.ScanGroupAtresolves 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 gotErrExplicitGroupRouteOwnerMismatch. So on a split catalog, a node migrated at most the first route's anchors and failed every tick on the rest. This PR addsShardStore.ScanGroupOwnedAtfor it.Commits
main: reproduce the TTL migration losing local leader groupsShardedCoordinatorleading groups 1 and 2 and following group 3, wrapped asmain.gowraps it.kv, main, adapter: keep local leader groups through the coordinator wrapperskv.LocalLeaderGroupsCoordinatorwithLocalLeaderGroupIDs() (ids []uint64, known bool). Decorators forward throughkv.LocalLeaderGroupIDsThrough, which reports a coordinator without it as unknown. Unknown falls back to the migrator's existing whole-prefixScanAtand logs once. A known empty list still plans nothing.main: guard every ShardedCoordinator capability through the startup wrappersmain: forward LinearizableReadForKey through the startup gateadapter: reproduce TTL migration refusing every shard but the firstShardStoresplit atmacross two groups.kv, adapter: scan each local leader group's own share of the TTL anchorsShardStore.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 rowsMerge main into feature/focused-pascal-dbr58vCoordinatorForwarder. Moves theknowncontract onto it. Updates main's chain and plan tests to the new signature. Reworks the behavioural guard to use main'sshardedMethodsNotForwardedlist.kv, adapter: reproduce legacy-route rows served against the owner's state/kv: serve legacy-route rows only while the logical owner holds noneadapter: reproduce TTL-inline ticks starving later scan plans/adapter: rotate TTL-inline scan plans across tickskv: reproduce the group-owned scan re-reading every wide-column row/kv: skip the point-read pass for rows the group-owned scan servesbackup: allow pebble's sstable reader through the import boundaryTestBackupLinksNoLiveClusterCodefails on main since 3f097df (#1276) added a pebblesstablereader tointernal/backup, while 5678081 (#1277) forbade all of pebble there. The commit allows exactly thesstablepackage.ShardStore.ScanGroupOwnedAtScanGroupOwnedAt(ctx, groupID, start, end, limit, ts) (rows, resume, err):limitrows from the group's own store, fenced like the other explicit-group reads.resumeis the last key the page read when it was full,nilonce the range is exhausted. The migrator moves its cursor toresume, so a page with no served rows still advances. Each tick stays within one page per plan.routeAndGroupForKeythat routes writes.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 islegacyPointRouteKey, the one point reads use.scanRouteAtForwardPage, after whichever page path ran, so those scans are unchanged.ScanGroupOwnedAtskips it, because every row it serves already carries the point-read value. This removes:ScanGroupAtand 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.
Plan rotation
Every plan (one per handler and local leader group) shares the compactor's per-tick budget.
migrateTTLInlineOncenow flattens the plans into one list per tick and starts at the plan after the last one the previous tick ran, ascompactBackgroundHandlersalready 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
knownon main's forwarder.CoordinatorForwarder.LocalLeaderGroupIDsreturnsLocalLeaderGroupIDsThrough(Inner). Withoutknown, 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 asIsLeaderForGroupfalling back to the inner's leadership.Behavior change
ShardedCoordinator.LocalLeaderGroupIDsandCoordinatorForwarder.LocalLeaderGroupIDsnow return([]uint64, bool).ShardStore.ScanGroupOwnedAt.ShardStorebehaviour is unchanged.ttl inline migrator: coordinator does not report local leader groups; falling back to whole-prefix scansonce, then runs oneScanAtper 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:LinearizableReadForKeyon the gate;EnsureMutationsWriteAllowed,ObserveForwardedRequests,RaftMembers*,PhaseD*andValidateDurableTimestampon the label.The two guard tests check different things:
TestAdapterCoordinatorWrappersForwardShardedCapabilitieschecks that everyShardedCoordinatormethod not listed inshardedMethodsNotForwardedexists on both wrappers with the same signature.TestStartupWrappersForwardShardedCoordinatorCapabilitiesuses the same list. It also calls each forwarded method throughstartupGatedCoordinatorand throughWithKeyVizLabel(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
LocalLeaderGroupIDsprobe back on the old[]uint64signature. 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.
!redis|ttl|read fallback;WithRedisLegacyTTLReadFallbackis not wired to any flag or env var.!redis|ttl|index;ScanAtreaches 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.Test evidence
Each fix was preceded by a failing test:
expected []uint64{0x1, 0x2},actual []uint64(nil).explicit group read does not own the requested key range, for all eight prefixes.limit=2.ExpireAt:0x0).leader not found.Mutation checks: each of these fails its targeted tests:
LocalLeaderGroupIDsprobe back on the old signature.Race suites:
go test -race -count=1 ./adapter/... ./kv/... .ona73a7c0a: 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.35f698e(before the split-store fix)a73a7c0a(merged with main)TTLandGETwere correct throughout. An earlier single-group run (3 nodes) showed base184351aestill 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.Self-review (five lenses)
readTS, with a commit timestamp fromkv.NextTimestampAfterThroughon the dispatching group leader (dispatchCompaction). A user write afterreadTSconflicts rather than being overwritten.LocalLeaderGroupIDsis a point-in-time snapshot. Each key is still checked withIsLeaderForKeybefore dispatch.IsLeaderForKeyand OCC.stateMu. ConcurrentSyncOncecalls only shift where a tick starts.sync.Once. Race tests pass.readTS. The presence rule mirrors the point-read fallback exactly, including tombstones.kv/coordinator.go,kv/sharded_coordinator.go): the change adds a capability interface and helper and changesLocalLeaderGroupIDs' return. Nothing in the HLC-4, renewal-cadence, group-selection, fail-closed or OCC checklist changed, so no spec update is needed.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.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.ai/code/session_019P5kzueGgfe3F5iSe8TMtC