docs(design): propose hotspot split M2 — migration plane#945
Conversation
Parent partial doc 2026_02_18_partial_hotspot_shard_split.md tracks M1 as shipped; M2-M4 are open. This proposal scopes M2 — the migration plane that moves a split child to a different Raft group. Scope: - SplitJob durable record under !dist|job|<id> with phase/cursor/ts fields so a leader flap resumes mid-flight. - BACKFILL / FENCE / DELTA_COPY / CUTOVER / CLEANUP state machine driven by a single migrator goroutine on the default-group leader, with per-phase recovery semantics enumerated in the matrix. - proto/distribution.proto extension: target_group_id on SplitRangeRequest; GetSplitJob / ListSplitJobs / AbandonSplitJob. - proto/internal.proto: ExportRangeVersions (server-streaming) + ImportRangeVersions (idempotent via persisted import_cursor). - store/MVCCStore.ExportVersions / ImportVersions with internal-key family dispatch and routeKey() equivalence assertion (covers !txn|, !lst|, !ddb|, !redis|, !sqs|, !s3|). - ErrRouteWriteFenced at both coordinator and FSM (defense in depth on the gRPC mesh + Raft apply path); reuses PR #927 / #932 Composed-1 alignment at CUTOVER. - 8-PR landing plan with five-lens self-review per PR. Non-goals (M3+): hotspot detection, auto-split scheduler, online merge, per-tenant policy. 7 open questions called out at the end for reviewer input — notably grace-period sizing, abandon-after-cutover policy, and backfill-vs-prepare-window semantics. Per the design-doc-first workflow in CLAUDE.md, this PR is the doc only; M2-PR1..PR7 follow once the proposal is accepted.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDesign doc adding a Milestone‑2 migration plane: durable SplitJob state machine and phases, cross-group route/staging semantics, streaming MVCC Export/Import contracts and RPCs, CUTOVER/promote semantics, coordinator/FSM fencing and read-fence, migrator runtime, recovery matrix, test plan, and rollout gating. ChangesMilestone 2 Migration Design
Sequence Diagram(s)sequenceDiagram
participant Coordinator
participant SourceStore
participant TargetStore
participant Catalog
Coordinator->>SourceStore: ExportRangeVersions(job_id, start_cursor, KeyFilter)
SourceStore->>Coordinator: Stream(versions[], cursor)
Coordinator->>TargetStore: ImportRangeVersions(job_id, cursor, KeyFilter)
TargetStore->>TargetStore: persist staged MVCC pages (idempotent by job_id+cursor)
TargetStore->>Catalog: stage shadow keyspace (not bound in catalog until CUTOVER)
Coordinator->>Catalog: CUTOVER (atomic catalog rebind, commit route_version)
Catalog->>Coordinator: ack CUTOVER (route_version increment)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request proposes a design document for Milestone 2 of the Hotspot Shard Split feature, focusing on the migration plane. It outlines the data model additions (such as SplitJob), state machine transitions, RPC changes, MVCC range export/import, and coordinator/FSM integration. The review feedback highlights three key areas for improvement: defining a retention or garbage collection policy for completed SplitJob records to prevent unbounded storage growth, decoupling the low-level store package from high-level routing logic by using an interceptor or delegate pattern instead of directly referencing routeKey, and addressing rolling upgrade risks where older M1 nodes might not enforce the WriteFenced state by detailing mitigation strategies such as capability gates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ### 3.1 SplitJob | ||
|
|
||
| A split job is durable state for the migration. Persist under reserved keys in the default group, alongside the route catalog. | ||
|
|
||
| Key layout: | ||
|
|
||
| - `!dist|meta|next_job_id` — uint64, allocator for `job_id`. | ||
| - `!dist|job|<job_id>` — encoded `SplitJob` record. |
There was a problem hiding this comment.
The proposed design persists SplitJob records under !dist|job|<job_id> but does not specify a limit or a garbage collection/retention policy for completed (DONE, FAILED, or abandoned) jobs. Over time, as the cluster performs many hotspot splits, this collection can grow unboundedly. This poses a risk of unbounded storage growth and potential OOM issues when listing or scanning jobs (e.g., via ListSplitJobs).
To prevent unbounded memory and storage growth, please define a fixed bound or a retention/cleanup policy to automatically garbage collect or archive historical job records after a certain period or count.
References
- To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests, such as pending configuration changes. Reject new requests when the bound is reached.
| ### 6.3 Internal-key coverage | ||
|
|
||
| §9 of the parent doc enumerates the key families. The exporter scans the raw storage range `[start, end)` filtered by **routeKey** equivalence to the moving range, so internal keys (`!txn|...`, `!lst|...`, redis hash/list/etc.) land in the same shard as their logical owner. The importer dispatches by `key_family` into the matching store helper to preserve any per-family invariant (e.g., list head pointer updates). | ||
|
|
||
| A simple safety check: every exported key must map back to the moving range under `routeKey()`. The exporter asserts this on every row before adding to the chunk; an assertion failure aborts the job and surfaces in `last_error`. This protects against a future internal-key family being added that the exporter forgot to teach. |
There was a problem hiding this comment.
The proposed design has the low-level store package (store/mvcc_store.go) directly referencing and calling routeKey() to filter keys and map them back to the moving range. This introduces tight coupling between the low-level generic storage package and high-level routing/domain logic.
Per the general rules, to prevent tight coupling between low-level generic packages and high-level domain packages, you should use an interceptor or delegate pattern with interfaces (or function values) defined in the low-level package. For example, ExportVersions can accept an optional, nil-safe key-filter function or delegate interface (e.g., func(key []byte) bool) injected from the high-level package, keeping the store package generic and decoupled.
References
- To prevent tight coupling between low-level generic packages and high-level domain packages, use an interceptor or delegate pattern with interfaces defined in the low-level package. Ensure the interceptor is optional (nil-safe) so that builds or environments with the high-level feature disabled can run without modification.
| | Lens | M2-specific risk | | ||
| |---|---| | ||
| | Data loss | CUTOVER atomicity; ImportRangeVersions idempotency; the BACKFILL/DELTA_COPY commit-ts windows have no gap. | | ||
| | Concurrency | Migrator goroutine vs leadership election; FENCE visibility timing on followers; AbandonJob race with phase advance. | | ||
| | Performance | Chunk size vs gRPC frame limit; pacing default vs prod throughput; cursor lookup cost per import. | | ||
| | Data consistency | Composed-1 alignment at CUTOVER; reader at `≤ fence_ts` on source vs reader at `≥ catalog_version+1` on target; internal-key route mapping. | | ||
| | Test coverage | Property tests over chunk-then-restart sequences; FSM unit for FENCE; Jepsen split + nemesis. | |
There was a problem hiding this comment.
The design introduces a significant operational risk during rolling upgrades. If a migration job is initiated while some nodes in the cluster are still running the older M1 version, those M1 nodes will not enforce the WriteFenced state (since the FENCE rejection logic is introduced in M2-PR5). This could allow writes to be accepted on the source group during the FENCE/DELTA_COPY phases, leading to silent write loss or inconsistency.
Per the general rules, when a design document identifies a significant operational risk like rolling upgrade incompatibility, it must detail potential mitigation strategies. Please update the design (e.g., in the self-review or as a new section) to include layered mitigations, such as using admin RPC capability gates to prevent starting any SplitRange migration jobs until all nodes have upgraded and advertised M2 capability.
References
- When a design document identifies a significant operational risk, such as the inability to perform rolling upgrades, it must also detail potential mitigation strategies, like implementing a temporary "bridge" or "proxy" mode.
There was a problem hiding this comment.
Already addressed in §11.1 ("Rolling-upgrade compatibility") landed in commit 93de9b2: cluster-readiness gate at the SplitRange entry RPC keyed on the cap_migration_v2 capability bit in the existing distribution heartbeat, with M2-PR5 as the bit-advertise commit so the gate semantics are safe across rollout. Could you confirm this resolves the finding, or point at what's still uncovered?
![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: 3aca90c833
ℹ️ 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".
| ├─ BACKFILL: right child created with state=MigratingTarget on target group; | ||
| │ source remains Active; writes still flow to source. | ||
| ├─ FENCE: source.right_child → WriteFenced; writes on the moving range |
There was a problem hiding this comment.
When a cross-group job enters BACKFILL, this plan creates the target right child while leaving the full source route Active, so the catalog snapshot contains overlapping ranges for the moving interval. The current engine does not ignore non-Active states when applying snapshots: routesFromCatalog converts every descriptor and validateRouteOrder rejects any prev.End > curr.Start with ErrEngineSnapshotRouteOverlap (distribution/engine.go:468-523). As written, watchers would fail to apply the first migration catalog snapshot, so the migration cannot progress through the proposed visible route states unless the design keeps the target out of the route snapshot or changes the overlap invariant.
Useful? React with 👍 / 👎.
|
|
||
| - `kv/fsm.go` already routes through `verifyComposed1` at apply. Extend `verifyOwnerFromSnapshot` to also reject when the resolved route's state is `WriteFenced` — even if a coordinator with a stale engine forwarded a write, the FSM closes the door. The error wraps `ErrRouteWriteFenced` so M4 retry logic differentiates from `ErrComposed1Violation`. | ||
|
|
||
| Reads are **not** fenced: `ShardStore.GetAt` continues to serve from source until CUTOVER. Snapshot reads at any `commit_ts ≤ fence_ts` remain consistent because the source's MVCC history is unchanged through DELTA_COPY. |
There was a problem hiding this comment.
If a coordinator or source-group leader has not applied the CUTOVER catalog yet, leaving reads unfenced lets it keep routing the moved key to the source while new writes are already accepted on the target. The current read path resolves the group from the local engine and then serves/proxies the read without carrying an observed catalog version (kv/shard_store.go:38-53, kv/shard_store.go:1471-1477), while the Composed-1 guard cited here only checks mutations in verifyOwnerFromSnapshot (kv/fsm.go:753-773). During the CLEANUP grace window this can return the pre-cutover source value after a successful post-cutover target write, so the design needs a read-side route-version fence/redirect or another mechanism that stops stale source reads for the moved range.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md (2)
108-108: ⚖️ Poor tradeoffConfirm operational acceptance of one-way CUTOVER.
After CUTOVER completes, the migration cannot be rolled back without implementing a full reverse migration (explicitly out of scope for M2). This means if post-CUTOVER issues arise (e.g., target group instability, data corruption discovered later), the only recovery path is forward through CLEANUP or manual intervention.
Is this operational trade-off acceptable for M2, or should §12 Open Questions include a specific item about post-CUTOVER failure scenarios and operator runbooks?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md` at line 108, The document currently states that CUTOVER is one-way and rollback requires a reverse migration; update the design doc to explicitly capture operational acceptance by either (A) adding a clear sentence in the CUTOVER paragraph that the team accepts the one-way behavior for M2 and lists recovery paths (CLEANUP, manual intervention, reverse migration out-of-scope), or (B) add a new item under §12 Open Questions titled "Post-CUTOVER failure scenarios and operator runbooks" that requests acceptance criteria, required runbooks, and escalation steps for issues like target-group instability or latent data corruption; reference the terms CUTOVER, PLANNED → BACKFILL, AbandonJob, and CLEANUP so reviewers can locate and verify the change.
258-261: ⚖️ Poor tradeoffVerify sufficiency of routeKey() assertion failure handling.
Lines 260-261 state that if an exported key fails the
routeKey()equivalence check, "an assertion failure aborts the job and surfaces inlast_error." Per §4 line 106, this puts the job intoFAILEDstate requiring manual operator intervention.This safety check protects against a future internal-key family being added without updating the exporter. However, silent job abortion may not provide sufficient visibility for this class of bug.
Consider:
- Is
last_errorsurfaced prominently enough (e.g., via metrics/alerts) to catch this during development/testing?- Should dev/test environments panic on this assertion to fail-fast rather than require operator log inspection?
- Should this be elevated in §12 Open Questions as a code-maintenance concern?
The defensive check is excellent; the question is whether the failure mode provides adequate feedback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md` around lines 258 - 261, The routeKey() assertion currently aborts the exporter job and writes to last_error leaving the job in FAILED state; enhance visibility and fail-fast behavior by (1) emitting a dedicated metric/counter and structured error log whenever routeKey() mismatches occur (include routeKey, key_family, and chunk id) so monitoring/alerts can catch it, (2) adding a configurable dev-only mode on the exporter (e.g., Exporter devFailFast flag or ENV) that panics/throws immediately instead of only setting last_error to ensure fast feedback in testing, and (3) document this operational choice in the design notes (add to §12 Open Questions) so maintainers know to update alerting/ops runbooks; refer to routeKey(), exporter, last_error and the FAILED job state when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md`:
- Around line 76-86: Add a language specifier to the fenced code block that
contains the migration state diagram (the block starting with "source.Active"
and the listed states BACKFILL/FENCE/DELTA_COPY/CUTOVER/CLEANUP) to satisfy
markdownlint MD040; update the opening fence from ``` to something like ```text
(or ```nohighlight) so the block is explicitly marked as plain text.
---
Nitpick comments:
In `@docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md`:
- Line 108: The document currently states that CUTOVER is one-way and rollback
requires a reverse migration; update the design doc to explicitly capture
operational acceptance by either (A) adding a clear sentence in the CUTOVER
paragraph that the team accepts the one-way behavior for M2 and lists recovery
paths (CLEANUP, manual intervention, reverse migration out-of-scope), or (B) add
a new item under §12 Open Questions titled "Post-CUTOVER failure scenarios and
operator runbooks" that requests acceptance criteria, required runbooks, and
escalation steps for issues like target-group instability or latent data
corruption; reference the terms CUTOVER, PLANNED → BACKFILL, AbandonJob, and
CLEANUP so reviewers can locate and verify the change.
- Around line 258-261: The routeKey() assertion currently aborts the exporter
job and writes to last_error leaving the job in FAILED state; enhance visibility
and fail-fast behavior by (1) emitting a dedicated metric/counter and structured
error log whenever routeKey() mismatches occur (include routeKey, key_family,
and chunk id) so monitoring/alerts can catch it, (2) adding a configurable
dev-only mode on the exporter (e.g., Exporter devFailFast flag or ENV) that
panics/throws immediately instead of only setting last_error to ensure fast
feedback in testing, and (3) document this operational choice in the design
notes (add to §12 Open Questions) so maintainers know to update alerting/ops
runbooks; refer to routeKey(), exporter, last_error and the FAILED job state
when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70d218ee-479f-4859-864a-0bdc51656929
📒 Files selected for processing (1)
docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md
… minor)
Codex P1 (line 80) — overlapping migration routes
Rewrote §3.2 state machine: catalog snapshots are kept strictly
non-overlapping throughout the job. BACKFILL/DELTA_COPY land in a
shadow keyspace (!dist|migstage|<job_id>|<raw_key>) on the target
group's MVCC store, outside the catalog. The catalog gets a NEW
WriteFenced route at FENCE-time (split out from source) and the
group-rebind happens atomically at CUTOVER. routesFromCatalog +
validateRouteOrder (distribution/engine.go:496-527) stay green.
RouteState.MigratingSource/MigratingTarget are reserved but unused
in M2.
Codex P1 (line 277) — post-CUTOVER read fence
Added §7.2: read-path now carries read_route_version (parallel to
ObservedRouteVersion on writes). New verifyReadOwner gate next to
verifyOwnerFromSnapshot returns ErrRouteMoved{new_route_version,
new_group_id} when a stale coordinator routes a read to source
after CUTOVER. Coordinator does WatcherRefresh + single retry.
CLEANUP grace window readFenceGrace=30s bounds the corner case.
The write-side Composed-1 gate is unchanged so PR #932's Jepsen
trust holds.
Gemini medium (line 51) — SplitJob unbounded growth
Added §3.1.1 + §4.2: live cap maxLiveJobs=64 enforced by
SplitRange; on terminal transition jobs move to
!dist|jobhist|<terminal_at_ms_be>|<id> with retention by
maxJobHistory=1k OR historyTTL=7d (whichever tighter); once-per-
minute sweep on default-group leader. ListSplitJobs supports
since_terminal_at_ms / phase filters and cursors.
Gemini medium (line 260) — store/routeKey tight coupling
Added store.KeyFilter delegate; ExportVersions takes the filter as
a nil-safe argument so the store package never imports kv or
knows about routing keys. routeKey() closure built in
kv/migrator_filter.go and passed in by the migrator. §6.3
rewritten with the explicit decoupling seam.
Gemini medium (line 346) — rolling-upgrade incompatibility
Added §11.1: node_capability_bitfield in the existing distribution
heartbeat; cap_migration_v2 bit gates cross-group SplitRange at
the entry RPC with ErrClusterNotReadyForMigration; in-flight
job-pause path on observed downgrade; bit is advertised only in
M2-PR5 (the commit that lands the FENCE rejection enforcement)
so the gate is safe across the rollout. Same-group split (M1)
remains unconditionally available.
CodeRabbit minor (line 86) — MD040 fenced-code-block language
All fenced blocks now carry a language tag (text/proto/go).
Replaced the bare ``` with ```text on the state-machine diagram
and consolidated to a single language across the diagram and
evolution table.
PR-plan note (§11): the rolling-upgrade gate dictates a hard
ordering between M2-PR4 (state machine) and M2-PR5 (FENCE
enforcement) — the capability bit only goes live in PR5.
|
@claude review — addressed all 6 findings from the gemini / codex / coderabbit reviews in commit 93de9b2: Codex P1 (line 80, overlapping routes) — §3.2 rewritten so catalog snapshots stay strictly non-overlapping. BACKFILL/DELTA_COPY land in a target-side shadow keyspace ( Codex P1 (line 277, post-CUTOVER read fence) — added §7.2 with Gemini medium (line 51, SplitJob unbounded growth) — added §3.1.1 / §4.2: Gemini medium (line 260, store/routeKey coupling) — Gemini medium (line 346, rolling upgrade) — added §11.1: CodeRabbit minor (line 86, MD040) — all fenced blocks now carry a language tag. |
|
Claude finished @bootjp's task in 3m 26s —— View job Review of commit 93de9b2 — addressing 6 prior findingsTodo list
Verified: all 6 prior findings are addressed
New observations on the updated designP1 — CUTOVER two-step ordering undefined (data-visibility gap)§3.2 (line 139) says:
But the CUTOVER step as described consists of two independent Raft proposals on different groups:
The design does not specify the ordering of these two. If (1) commits before (2), reads routed to the target during the rename window return empty results: the catalog says the target owns the range, but the target's MVCC only has shadow keys. The read fence in §7.2 only redirects stale source reads; it doesn't compensate for the target not yet having live data. The safe ordering is: (2) commits on the target first, then (1) executes on the default group — by the time routing switches, all data is live on the target. But:
Suggest: add a §7.5 (or a sub-bullet in §4's CUTOVER row) that specifies: "Target bulk-rename commits first ( Medium — Grace window inconsistency (§7.2.4 vs §4 vs Open Question 3)Three places cite the CLEANUP grace default and they disagree:
Pick one and make them consistent. Whichever wins should also answer Open Question 3 (or at least note the reasoning). For reference: Medium — "Wait until applied" in §7.2.2 under-specified
How is this wait implemented? Options include an applied-index condition variable, a short retry loop with a timeout, or a channel signalled by the FSM apply goroutine. The choice affects:
A one-sentence note (e.g., "implemented as a condition-variable wait on the FSM's appliedIndex, timedout by Minor —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93de9b2d2e
ℹ️ 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".
| // ImportVersions writes the given versions idempotently. A second call | ||
| // with the same (job_id, cursor) MUST be a no-op past the recorded | ||
| // cursor — the importer records last-applied cursor under | ||
| // !dist|job|<id>|import_cursor so a network retry doesn't double-write. | ||
| ImportVersions(ctx context.Context, jobID uint64, versions []MVCCVersion, | ||
| cursor []byte) ([]byte, error) |
There was a problem hiding this comment.
When the source range contains commit timestamps higher than the target group's current LastCommitTS/HLC, this import contract only writes versions and an idempotency cursor, but does not require the target to observe the max imported commit_ts. Since the existing FSM observes commit timestamps only after normal pb.Request applies, a migration import path can leave the target clock below the staged data; after CUTOVER the first target write can receive a lower timestamp than an imported older value, causing MVCC time travel where later reads resurrect the pre-cutover value. The import apply needs to atomically advance metaLastCommitTS and the target HLC (or otherwise fence new writes) to at least the max imported timestamp before the route becomes active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 17b0719 — added §6.2.1 "Target HLC advancement": ImportVersions now atomically advances both metaLastCommitTS and the HLC ceiling (hlc.SetPhysicalCeiling) to max(batch.commit_ts) under the same FSM apply lock as the MVCC writes. SplitJob carries a monotone max_imported_ts witness, and CUTOVER is gated on a final SetPhysicalCeiling(max_imported_ts) proposal on the TARGET group before the catalog CAS — so post-CUTOVER Next() is provably greater than every imported ts. Reuses the existing Composed-1 monotone-ceiling primitive (PR #927) — no new fence.
| | `snapshot_ts` | uint64 | HLC commit-ts pinned at BACKFILL entry. | | ||
| | `fence_ts` | uint64 | HLC commit-ts pinned at FENCE → DELTA_COPY entry. | | ||
| | `cutover_version` | uint64 | Catalog version produced by the CUTOVER write; populated at CUTOVER, used by §7.2 read-fence and §4.2 GC. | | ||
| | `cursor` | bytes | Next-key cursor for BACKFILL / DELTA_COPY resume. | |
There was a problem hiding this comment.
The job cursor is defined as a next-key cursor, but an MVCC export can contain many committed versions for the same raw key. If a hot key's history exceeds chunkBytes, resuming by key alone either has to keep that entire key history in one unbounded chunk or risks skipping/duplicating the remaining versions after a restart; the API should make the cursor opaque over the encoded MVCC position (raw key plus commit timestamp) or explicitly require chunks never split a key's full version history.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 17b0719 — added §6.1.1 "Cursor granularity": the cursor now encodes (raw_key, commit_ts) and the exporter iterates (raw_key ASC, commit_ts DESC) with exclusive-resume semantics. Chunks may contain only versions of one raw key without an all-or-nothing constraint on the history — the next chunk continues at the next-older commit_ts of the same key. The SplitJob row was updated accordingly. Opaque encoding so the store-side type stays []byte and the exporter alone owns the codec (preserves the §6.1 store/kv decoupling seam).
…x P1/P2) Codex P1 on PR #945 (line 323) — Advance target HLC when importing Source range can contain commit_ts > target group's current HLC / metaLastCommitTS. Import-only versions-and-cursor writes leave the target clock behind; after CUTOVER the first target write at Next() can collide with imported ts and resurrect pre-cutover values on a snapshot read. Fix: ImportVersions atomically advances metaLastCommitTS AND calls hlc.SetPhysicalCeiling(batchMax) under the same FSM apply lock as the MVCC writes; SplitJob carries a monotone max_imported_ts witness; CUTOVER is gated on a final SetPhysicalCeiling(max_imported_ts) proposal on the TARGET group before the catalog CAS. Reuses the existing Composed-1 / HLC monotone-ceiling primitive — no new fence. New unit + property tests pin (a) per-batch advance, (b) CUTOVER gap-closing proposal, (c) strict Next() > max_imported_ts post-CUTOVER. Codex P2 on PR #945 (line 66) — Cursor must address MVCC versions A "next-key" cursor cannot resume safely on a hot key whose version history exceeds chunkBytes. New §6.1.1 spec: opaque cursor encodes (raw_key, commit_ts), iterated in (raw_key ASC, commit_ts DESC) order with exclusive-resume semantics. Chunks may contain only versions of one raw key; the next chunk continues with the next-older version of the same key — no all-or-nothing constraint on a key's history. Idempotent ack at the importer dedups any one resume overlap. SplitJob row (§3.1) gains `max_imported_ts uint64` and the cursor field description is updated. §3.2 state-machine diagram now shows the SetPhysicalCeiling proposal as a precondition of CUTOVER. No caller-visible wire change beyond the existing M2 proto deltas.
|
@claude review — round 2:
|
|
Claude finished @bootjp's task in 2m 51s —— View job Review — Round 2Todo list
Round-2 items — all three verified
Remaining items from prior reviewP1 — CUTOVER bulk-rename ordering still implicit (unresolved)The prior review flagged that the CUTOVER step on two different Raft groups must be sequenced: target bulk-rename first, then catalog CAS — otherwise there is a window where the catalog says the target owns the range but the target's MVCC only has shadow keys, making reads to the target return empty results. The current doc has moved closer but the ordering is still not stated explicitly in the §3.2 CUTOVER description. §6.2.1 (line 367) says It does not list The safe and intended ordering is:
Steps 2 and 3 are specified, step 1 is not. Without step 1 appearing in the CUTOVER sequence, an implementer working from §3.2 alone could issue the catalog CAS before the rename is applied, creating the data-availability gap. The leader-flap resume case (new migrator checking whether rename already landed before re-proposing the catalog write) also needs to be specced. Suggested fix: add a Medium — Grace window inconsistency (still unresolved)Three locations cite the CLEANUP grace default and they still disagree after commit 17b0719:
§7.2.4 was updated with a rationale ("comfortably exceed both lease TTL and watcher tick × 2") but the value there (30 s) still disagrees with the other two (60 s). Pick one and update all three; if 30 s wins, close Open Question 3 with the chosen rationale. Minor —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17b0719055
ℹ️ 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".
|
|
||
| FSM-side defense in depth: | ||
|
|
||
| - `kv/fsm.go` already routes through `verifyComposed1` at apply. Extend `verifyOwnerFromSnapshot` to also reject when the resolved route's state is `WriteFenced` — even if a coordinator with a stale engine forwarded a write, the FSM closes the door. The error wraps `ErrRouteWriteFenced` so M4 retry logic differentiates from `ErrComposed1Violation`. |
There was a problem hiding this comment.
This FSM-side fence is described as an extension of verifyOwnerFromSnapshot, but today verifyComposed1 returns before that helper when ObservedRouteVersion == 0. There are still intentional zero-version write paths (for example read/write txns with ReadKeys, caller-supplied StartTS, or resolver-claimed keys in kv/sharded_coordinator.go:694-754), so a stale coordinator in those contexts can forward a write to the source during FENCE and the proposed FSM defense will not run. The fence check needs to be on an unconditional apply path, or the design needs to require all mutation flows to carry a nonzero observed route version before cross-group split is enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit c16e1c4. §7.1 now introduces verifyRouteNotFenced as a SEPARATE, unconditional pre-gate ahead of verifyComposed1 — not an extension of verifyOwnerFromSnapshot. It scans mutations with routeKey() against the current catalog snapshot and rejects ErrRouteWriteFenced for any key whose route is WriteFenced, with the same Phase_ABORT bypass and NO dependency on ObservedRouteVersion. So the zero-version write flows in kv/sharded_coordinator.go:694-754 (ReadKeys, caller-supplied StartTS, resolver-claimed keys) are covered — the gate runs on every applied write request unconditionally. The two gates stay independent so a future change to one cannot regress the other.
| [ target.Active(right) ] | ||
| ``` | ||
|
|
||
| Key invariant: **catalog snapshots never contain overlapping ranges**. The target shadow keyspace lives outside the catalog (BACKFILL / DELTA_COPY land into private `!dist|migstage|<job_id>|<raw_key>` keys on the target group's MVCC store; CUTOVER promotes the staged data to live keys via a single bulk-rename FSM apply). This keeps `routesFromCatalog` + `validateRouteOrder` (`distribution/engine.go:496-527`) green throughout the migration — addresses codex P1 on overlapping routes (PR #945 review). RouteState `MigratingSource` / `MigratingTarget` are reserved but unused in M2; they remain available for a future merge / multi-stage migration design. |
There was a problem hiding this comment.
Staging every imported row under !dist|migstage|... and then promoting the whole range via a single FSM apply makes CUTOVER proportional to the entire migrated range. For any split larger than the Raft proposal/apply budget, this defeats the chunked BACKFILL/DELTA_COPY design and can block or fail exactly when the catalog must switch atomically. The design should avoid a per-key bulk move at cutover, or make the staged-to-live promotion incremental while preserving the visibility fence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit c16e1c4. New §6.4 "Incremental staged-to-live promotion": CUTOVER is O(1) — just a catalog version bump + route swap + set staged_visibility_active on the new target route, NO per-key work. Reads use a staged-fallback through the per-route bit so imported data is visible the instant CUTOVER lands. A leader-local promoter on the target group walks !dist|migstage|<job_id>|* in cursor-resumable chunks (same chunkBytes / pacing as BACKFILL) and proposes one bounded PromoteStaged Raft entry per batch — never a single oversized apply. CLEANUP → DONE waits for the promoter to drain AND the grace window AND the staged_visibility_active flag clear. SplitJob picks up promote_cursor + promotion_completed_ts for resumability and audit.
…945) Codex P1 on PR #945 (line 407) — FENCE must run on unconditional path verifyComposed1 returns before verifyOwnerFromSnapshot when ObservedRouteVersion == 0, and several intentional zero-version write paths exist today (read/write txns with ReadKeys, caller- supplied StartTS, resolver-claimed keys in kv/sharded_coordinator.go:694-754). The naïve "extend verifyOwnerFromSnapshot to reject WriteFenced" placement would silently skip those paths — a stale coordinator could forward a write to the source during FENCE without the FSM defense ever running. Fix in §7.1: introduce verifyRouteNotFenced as a separate, unconditional pre-gate ahead of verifyComposed1. It scans mutations with routeKey() against the current catalog snapshot and rejects ErrRouteWriteFenced for any key whose route is WriteFenced, with the same Phase_ABORT bypass and no dependency on ObservedRouteVersion. Two independent gates, write side unchanged Composed-1 trust preserved. Codex P2 on PR #945 (line 147) — CUTOVER bulk rename is unbounded Staging every imported row under !dist|migstage|... and promoting via a single FSM apply makes CUTOVER O(migrated range) — defeats chunked BACKFILL/DELTA_COPY exactly when the catalog must atomically switch. New §6.4 ships incremental promotion: 1. CUTOVER apply is O(1): catalog version bump + route swap + set staged_visibility_active on the new target route. 2. Reads use a staged-fallback through the per-route bit so imported data is visible the instant CUTOVER lands. 3. A leader-local promoter on the target group walks the staged prefix in cursor-resumable chunks, copying each version to its live position with one bounded PromoteStaged apply per batch. 4. CLEANUP → DONE precondition extends to "promoter empty AND grace window elapsed AND staged_visibility_active cleared", so route is indistinguishable from any Active route once promotion finishes. SplitJob row picks up promote_cursor (opaque) and promotion_completed_ts (diagnostic). §3.2 state-machine diagram updated: CUTOVER no longer says "bulk-rename"; the staged-to- live work is explicitly background and incremental. Both fixes preserve the §6.2.1 HLC ceiling advance and §7.2 read fence guarantees — incremental promotion is a strict superset of the chunked BACKFILL/DELTA_COPY semantics applied at a different phase.
|
@claude review — round 3 (2 codex findings on commit 17b0719):
|
|
Claude finished @bootjp's task in 3m 56s —— View job Review — Round 3Todo list
Round-3 items — both verified
Prior round-2 P1 — CUTOVER two-step ordering: resolved by §6.4The round-2 P1 flagged that "target bulk-rename must commit before the catalog CAS" to avoid a data-availability gap. §6.4 eliminates the bulk-rename entirely: CUTOVER sets The CUTOVER sequence (§3.2, lines 120–134) now correctly specifies: New P1 — §6.4 step 3 "no-op when a later live write has shadowed it" can lose MVCC history§6.4 step 3 (line 411) says the Scenario:
The "shadow" optimization is safe only for a last-write-wins (non-MVCC) store. In an MVCC store with snapshot read semantics, every version of a key must be preserved across its valid range. The correct guard is idempotency: "no-op if the staged version has already been copied to the live namespace (checked by the presence of a live version at the same commit_ts)." The fix is two parts:
The MVCC compaction GC ( Remaining from round 2 (three items still open)Medium — Grace window inconsistency (unresolved, 3rd flag)Three locations still disagree after commit c16e1c4:
§7.2.4 has a clear rationale for 30 s. If 30 s is the chosen value: update §4 line 168 to 30 s and close Open Question 3 with the chosen value + rationale. If 60 s is the chosen value: update §7.2.4 to 60 s and reconcile the rationale. Either way, one pass to make all three locations consistent and close OQ3. Minor —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c16e1c4335
ℹ️ 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".
| // ExportVersions iterates committed versions in the half-open range | ||
| // [start, end) whose commit_ts is in (minCommitTS, maxCommitTS], |
There was a problem hiding this comment.
For splits whose logical range is a normal user-key interval such as [foo, bar), this raw [start, end) MVCC iterator never visits keys like !redis|str|foo, !lst|...foo, or !txn|lock|foo, because those prefixes sort outside the user-key range before the routeKey() filter can accept them. That means the proposed migration can cut over without copying internal state that routes to the moved key, causing data loss or broken adapter invariants; the export needs to scan the relevant internal-family prefixes or use a route-key-indexed traversal rather than relying on a raw-range scan plus filter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 231e9a5. New §6.3.1 "Internal-family brackets": migration now runs MULTIPLE parallel exports per phase, one per relevant raw-key family (user-key, !txn|lock|, !txn|cmt|, !txn|rb|, !txn|meta|, !txn|int|, !lst|meta|, !lst|itm|, !redis|, !ddb|item|, !ddb|gsi|, !sqs|, !s3|), each driven by the same KeyFilter so routeKey() remains the single source of truth. PlanExportBrackets() returns the bracket list; per-bracket opaque cursors + done flags on the SplitJob allow independent resumability. Bracket count is small (~12 today) and fan-out parameter caps parallelism. A future adapter MUST add its bracket at the same PR that teaches routeKey() its family.
|
|
||
| #### 7.2.2 Source FSM rejects reads after CUTOVER | ||
|
|
||
| When a source-group FSM applies a lease read whose `read_route_version` is **strictly less than** the FSM's last-applied catalog version, and the requested key's `routeKey()` maps to a route the FSM no longer owns, the FSM returns `ErrRouteMoved{new_route_version, new_group_id}` instead of the value. The error is mapped to a retryable status (gRPC `codes.FailedPrecondition` + detail) and the coordinator retries after a `RouteEngine` refresh. |
There was a problem hiding this comment.
This condition only rejects when the request version is older than the source FSM's local catalog version, so it misses the common post-CUTOVER case where both the coordinator and the source group have only applied the pre-CUTOVER/FENCE version. In that window another coordinator that has refreshed can already write the key on the target, while this stale coordinator routes a read to the stale source and the source serves the old value because read_route_version == local_version; the read fence needs an authoritative cutover/ownership check that also catches equal-stale source reads during the grace window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 231e9a5. §6.4 step 2 rewritten: the staged area now participates in MVCC MERGE — newest commit_ts wins. The read takes the newest live version with commit_ts ≤ read_ts AND the newest staged version with commit_ts ≤ read_ts and returns whichever has the greater commit_ts. Four read_ts cases enumerated to prove correctness (live_write.ts > max_imported_ts > staged.ts; merge correctly surfaces live for read_ts ≥ live.ts and staged for read_ts ∈ (max_imported_ts, live.ts)). Per-family adapter invariants preserved via the key_family dispatch.
|
|
||
| No iteration over the migrated key range, no bulk rename, no per-key proposals. The Raft proposal carrying this apply is `O(1)` in the migrated data size — bounded by a handful of catalog descriptors and the SplitJob record. | ||
|
|
||
| 2. **Staged keyspace stays visible through a per-route flag.** After CUTOVER the target FSM's read path checks the route's `staged_visibility_active` bit. When set, a read for key `K` in the moved range first looks up `!dist|migstage|<job_id>|<K>` and falls back to the live key only if the staged entry is absent. (Writes always land in the live keyspace — the CUTOVER bump already routed writers to the target group via the catalog, and §6.2.1's HLC ceiling advance guarantees their `commit_ts` is strictly greater than every imported `commit_ts`, so a live write shadowing a staged version is the *correct* MVCC visibility.) This gives reads access to the imported data the instant CUTOVER lands, with **no promotion work blocking CUTOVER itself**. |
There was a problem hiding this comment.
Looking in the staged keyspace first and falling back to live only when staged is absent makes post-CUTOVER reads return stale imported versions after any live write to the same key, until promotion deletes the staged entry. For a read timestamp after the live write's commit timestamp, both staged and live versions exist and the newest live version must win, so the read path needs to merge/compare the staged and live MVCC candidates by commit timestamp instead of treating staged presence as authoritative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 231e9a5. §7.2.2 replaces the strict-less-than version check with an authoritative ownership check. When source has applied CUTOVER → unconditional ErrRouteMoved. When source has NOT applied CUTOVER but the heartbeat-piggybacked max_cutover_version_seen indicates a possible stale read → run GetRouteOwnership(routeKey(K), max_cutover_version_seen) against the default-group leader (cached one second). New §7.2.2a walks through the exact 7-step equal-stale scenario (t=0..6) showing the watermark + ownership-query path catches the case the original rule missed.
Codex P1 round-4 on PR #945 — three fundamental design issues in the §6.3 / §6.4 / §7.2 designs landed in c16e1c4: P1 line 310 — Scan internal families outside raw route bounds A single raw [start, end) iterator + routeKey filter misses every internal-family raw key (!txn|lock|foo, !lst|meta|foo, !redis|str|foo, !ddb|item|<tbl>|foo, !sqs|..., !s3|...) because those prefixes sort outside the user-key band — the filter rejects nothing because the iterator never visits those bytes. Without the fix, CUTOVER would leave intent locks, list metadata, and adapter-private state behind on the source. Fix in new §6.3.1 "Internal-family brackets": migration runs multiple parallel exports per phase, one per relevant raw-key family, driven by the same KeyFilter (routeKey() remains the single source of truth). The migrator computes the bracket list from the moving range's logical scope via PlanExportBrackets; each bracket gets its own opaque cursor and done flag on the SplitJob. Bracket count is small (~12 today) and bounded; the fan-out parameter caps parallelism. A future adapter MUST add its bracket at the same PR that teaches routeKey() its family — both are required for end-to-end coverage. P1 line 408 — Merge staged and live MVCC versions on reads The §6.4 step 2 "staged-first, fall back to live when staged is absent" form returned stale imported versions after any live write to the same key. After the §6.2.1 HLC advance, live writes have commit_ts > max_imported_ts; for read_ts ≥ live_write.ts the live version is the most-recent committed, but the fallback rule would still return the older staged version because the staged entry exists until promotion. Fix in §6.4 step 2: the staged area participates in MVCC merge — newest commit_ts wins. The read takes the newest live version with commit_ts ≤ read_ts AND the newest staged version with commit_ts ≤ read_ts and returns whichever has the greater commit_ts. This is the standard MVCC visibility rule applied to a second column-family-like source, not stage-shadows-live. Four read-ts cases enumerated to prove correctness; per-family adapter invariants preserved via the key_family dispatch (§6.3.1). P1 line 459 — Reject reads when both caller and source are stale The strict-less-than 'read_route_version < source_local_version' rule misses the equal-stale window: both coordinator and source pinned at the pre-CUTOVER version, while a different refreshed coordinator has already written to the target. Source's local snapshot says it still owns the key, so the read fence passes and serves the stale value. Fix in §7.2.2: replace version comparison with an authoritative ownership check scoped on whichever snapshot exists. When source has applied CUTOVER → unconditional ErrRouteMoved. When source has NOT applied CUTOVER but the heartbeat-piggybacked max_cutover_version_seen indicates a possible stale read → run GetRouteOwnership(routeKey(K), max_cutover_version_seen) against the default-group leader, cache one second. New §7.2.2a walks through the exact 7-step equal-stale scenario to demonstrate the fix catches the case the original rule missed. All three are fundamentally correctness fixes — the original wording would have produced silent data loss (310), MVCC visibility regression (408), or stale reads (459) in production. The fixes preserve the catalog-stays-non-overlapping invariant (§3.2), the §6.2.1 HLC advance, and the §7.2 read-fence scaffolding; only the under-spec'd details are tightened.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 231e9a5729
ℹ️ 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".
|
|
||
| 1. **Per-batch advance.** On every apply of an `ImportVersions` batch, the target FSM computes `batchMax = max(versions[i].commit_ts)` and **atomically** (under the same FSM apply lock that mutates the MVCC store): | ||
| - sets `metaLastCommitTS = max(metaLastCommitTS, batchMax)`, and | ||
| - calls `hlc.SetPhysicalCeiling(batchMax)` (the same Raft-agreed primitive used by the M1 HLC renewal proposer, `kv/coordinator.go:644-669`). `SetPhysicalCeiling` is already monotone — a lower argument is a no-op — so duplicate / out-of-order batches are safe. |
There was a problem hiding this comment.
This passes batchMax/max_imported_ts as a full HLC commit timestamp into hlc.SetPhysicalCeiling, but the current HLC stores that field as Unix milliseconds and Next() later shifts it as the physical part (kv/hlc.go). For any imported timestamp with logical bits set or even a normal ms<<16 encoding, this advances the target ceiling to an artificial far-future millisecond, causing subsequent writes to jump to roughly commit_ts<<16 and weakening the lease-ceiling fence until wall time catches up. The import path should either Observe the full HLC timestamp or pass only its physical millisecond component to SetPhysicalCeiling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 44e6125. §6.2.1 step 1 now passes hlcPhysicalMs(batchMax) = int64(batchMax >> 16) to SetPhysicalCeiling (the same shift HLC.Next() uses to extract the physical half, kv/hlc.go:128-145) AND calls hlc.Observe(batchMax) for the local clock's last-issued value. Step 3 (CUTOVER precondition) uses the same conversion. Both functions are monotone so duplicates/out-of-order are safe.
|
|
||
| The migrator runs the brackets **in parallel up to `--migrationExportFanout` (default 4)** within a single phase (BACKFILL or DELTA_COPY): each bracket gets its own opaque cursor (the §6.1.1 codec is extended to key on `(family, raw_key, commit_ts)`), so a bracket can be paused/resumed independently and the §9 resumability matrix applies per bracket. The migrator records per-bracket `cursor` and `done` flags on the SplitJob; the phase advances only when **every bracket** reports `done = true`. Cost is at most `fanout`× the gRPC frame budget but is bounded by the total bracket count (~12 today), not by data volume. | ||
|
|
||
| A bracket whose family doesn't actually intersect the moving range (e.g. an SQS migration on a Redis-only deployment) returns its first chunk empty and `done=true` immediately — no wasted scan work. The `keys per bracket` distribution is a §7.3 / §11.x metric so operators can spot a runaway family scan. |
There was a problem hiding this comment.
This claims an internal-family bracket with no matching keys returns immediately, but the proposed brackets are family-wide and the only narrowing happens after iteration through routeKey(). In a cluster with a large SQS/Redis/etc. prefix outside the moving range, an export for a range with no accepted keys must scan that whole raw prefix to prove done=true because rejected rows do not count toward chunkBytes, making a single chunk unbounded in CPU/I/O and defeating the migration pacing guarantees. The exporter needs a route-key-indexed traversal or a separate scan budget that advances the cursor over rejected rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 44e6125. §6.3.1 ships two layers: (1) Scan-budget pacing — each ExportVersions call carries a separate maxScannedBytes (default 4× chunkBytes) that counts both accepted and rejected rows; iterator returns at the budget with cursor advanced over the rejected tail. (2) Per-family EncodeBracketStart hook tightens family-wide bounds to the moving-slice band where the family layout allows it. Layer (1) alone is sufficient for correctness; (2) is the optimization per adapter PR. New metrics: keys per bracket + rejected_rows per bracket.
|
@claude review — round 4 (3 codex P1 on commit c16e1c4):
|
|
Claude finished @bootjp's task in 5m 5s —— View job Claude finished @bootjp's task in 3m — View job Review — Round 4Todo list
Round-4 items — all three verified
New P1 —
|
| Location | Line | Value |
|---|---|---|
| §4 table, CLEANUP row | 168 | 60 s |
| §7.2.4 | 566 | 30 s (with rationale: "comfortably exceed both lease TTL and watcher tick × 2") |
| §12 Open Question 3 | 681 | 60 s |
§7.2.4 has the rationale for 30 s. Pick one value, update all three locations, and close Open Question 3.
Minor — !dist| prefix on target-group import cursor
Line 349 stores the import cursor as !dist|job|<id>|import_cursor in the target group's MVCC store. !dist| is the default-group catalog namespace throughout the codebase. Future operator tools and distribution/catalog.go's key parsers would treat this as a catalog entry. Rename to a target-scoped prefix: !migstage|cursor|<id>.
Medium — "Wait until applied" still a one-liner
Line 544: "the FSM treats read_route_version > source_local_version as a 'wait until applied' (bounded by lease TTL) before answering" — still unspecified. The implementation mechanism (condition variable on appliedIndex signalled by the apply goroutine? short retry loop?) and the timeout error type (does the timeout surface as ErrRouteMoved or a distinct ErrTimeout?) remain unspecified. A one-sentence elaboration would close this: e.g., "condition-variable wait on the FSM's appliedIndex signalled by the apply goroutine, timedout by min(remaining_lease_TTL, 200ms); on timeout, re-evaluate ownership — if still not owned, return ErrRouteMoved."
Summary
All three round-4 items (codex P1s on commit c16e1c4) are properly verified as resolved. One new P1 emerges from the latest codex review on commit 231e9a5: SetPhysicalCeiling(batchMax) receives a full HLC timestamp instead of Unix ms, confirmed by reading kv/hlc.go — passing a full HLC value would set the ceiling ~3.5 billion years ahead and corrupt every subsequent Next() call. The correct call is SetPhysicalCeiling(int64(batchMax >> hlcLogicalBits)).
The round-3 P1 (PromoteStaged "no-op when shadowed" drops MVCC history) remains unresolved for the fourth consecutive round — the "shadowed" clause on line 477 still exists and is still wrong. Three editorial items from round 2 (grace window inconsistency, !dist| prefix, wait-until-applied spec) and one new P2 (sparse family bracket scan unbounded) round out the remaining work before implementation can start cleanly.
…dex round-5) Codex P1 round-5 on PR #945 (line 367) — HLC SetPhysicalCeiling units hlc.SetPhysicalCeiling takes int64 Unix milliseconds (kv/hlc.go:222), but the previous spec passed the full HLC encoded commit_ts (ms<<16 | logical) as the argument. With logical bits set or any normal ms<<16 encoding that would advance the ceiling by 65 536× the intended milliseconds — weakening the lease fence until wall time catches up. Fix §6.2.1 steps 1 and 3: pass hlcPhysicalMs(batchMax) = int64(batchMax >> 16) (the same shift HLC.Next() uses to extract the physical half) AND call hlc.Observe(batchMax) so the local clock's last-issued value tracks the full HLC ts. Both calls are monotone so duplicates and out-of-order batches are safe. Codex P2 round-5 on PR #945 (line 445) — sparse internal-family scans The earlier 'returns its first chunk empty and done=true immediately' claim only holds when the iterator can short-circuit; under the §6.1 contract accept rejections do NOT count against chunkBytes, so a family-wide raw range can scan an arbitrarily large prefix to prove done=true for a moving range with no intersection. Fix §6.3.1: two layers. (1) Scan-budget pacing — each ExportVersions call carries a separate maxScannedBytes (default 4× chunkBytes) that counts both accepted and rejected rows; when the budget hits the iterator returns whatever it has accepted + the next cursor (over the rejected tail position). (2) Route-key sub-prefix indexing where the family layout allows it — per-family EncodeBracketStart() hook tightens bracket bounds from family-wide to moving-slice-within-family; adapters without the hook keep the scan-budget pacing alone, which is sufficient for correctness. New metrics: keys per bracket + rejected_rows per bracket.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44e6125daf
ℹ️ 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".
| | FENCE → DELTA_COPY Copy (snapshot_ts, fence_ts] from | ||
| | source to target shadow space. |
There was a problem hiding this comment.
When the catalog enters WriteFenced, the source FSM only learns that through its local route engine, which is refreshed asynchronously by the catalog watcher (distribution/watcher.go:11, distribution/watcher.go:87-98), and the proposed FSM fence checks the current local snapshot. If the migrator records fence_ts and starts copying (snapshot_ts, fence_ts] immediately, a stale source leader can still accept writes for the moved keys after fence_ts but before its watcher applies the fenced route; those versions are above the DELTA_COPY upper bound and will be left on the source at CUTOVER. The phase needs a source-side barrier/ack that the fenced route is applied, or fence_ts must be chosen after writes are actually rejected on the source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 9fa5931. New §3.2a 'Source-side fence-apply barrier': migrator advances FENCE → DELTA_COPY only after every voter of the source-group Raft membership has reported catalog_version_applied >= fence_catalog_version (via the cap_migration_v2 heartbeat or a per-node GetCatalogVersion RPC). Only after the unanimous voter-ACK does the migrator query source.LastCommitTS() and pin fence_ts. Unanimous (not quorum) is required for fence safety — any voter could become the leader later and re-serve writes for the moved range in the pre-FENCE state if its catalog hadn't applied. fence_ack_cursor is durable in the SplitJob for restart-safety. State machine diagram (§3.2) updated.
|
@claude review — round 5 (1 P1 + 1 P2 on commit 231e9a5):
|
|
Claude finished @bootjp's task in 4m 52s —— View job Review — Round 5Todo list
Round-5 items — both verified
New P1 —
|
| Location | Line | Value |
|---|---|---|
| §4 table, CLEANUP row | 168 | 60 s |
| §7.2.4 | 574 | 30 s (with rationale: "comfortably exceed both lease TTL and watcher tick × 2") |
| §12 Open Question 3 | 689 | 60 s |
§7.2.4 has the rationale for 30 s. Picking 30 s: update lines 168 and 689 to match, and close Open Question 3 with the §7.2.4 rationale. Five rounds of flagging — this is a one-line fix.
Minor — !dist| prefix on target-group import cursor (5th flag)
Line 350, inside the ImportVersions doc comment: !dist|job|<id>|import_cursor is stored in the target group's MVCC store. !dist| is the default-group catalog namespace throughout the codebase (distribution/catalog.go). An operator tool or the catalog's key parsers scanning the default-group catalog would misparse these as catalog entries. Rename to a target-scoped prefix such as !migstage|cursor|<id> — one-line change.
Medium — "Wait until applied" spec (4th flag, slightly improved)
Line 552 (§7.2.2 sub-case 2): "the FSM treats this as 'wait until applied' (bounded by lease TTL) before answering, then re-evaluates ownership; if still not owned, returns ErrRouteMoved."
The round-4 addition of "then re-evaluates ownership; if still not owned, returns ErrRouteMoved" is an improvement. What remains unspecified: the implementation mechanism for "wait until applied" (condition variable on appliedIndex signalled by the apply goroutine? a retry loop with time.Sleep? a channel?) and the timeout value within the lease TTL (200 ms? 500 ms?). Given the level of implementation detail elsewhere in the doc, one sentence would close this: e.g., "condition-variable wait on the FSM's appliedIndex signalled by the apply goroutine, timedout by min(remaining_lease_TTL, 200ms); on timeout, re-evaluate ownership — if still not owned, return ErrRouteMoved."
Summary
Both round-5 items are verified and properly resolved. hlcPhysicalMs(ts) = int64(ts >> 16) is correct per kv/hlc.go (confirmed hlcLogicalBits = 16, SetPhysicalCeiling takes Unix ms, Observe takes a full HLC value). The scan-budget pacing correctly bounds per-call cost for sparse family brackets.
One new P1 emerges from the codex review of commit 44e6125: fence_ts is recorded before the source-group FSM has applied the WriteFenced route — writes accepted in the watcher-lag window after fence_ts are outside DELTA_COPY's range and would be lost at CUTOVER. The fix is a source-group catalog-version poll before DELTA_COPY begins.
The round-3 P1 (PromoteStaged "no-op when shadowed") has now been flagged five consecutive rounds without resolution; it is a correctness bug that causes MVCC history versions to vanish. Three editorial items from round 2 remain (grace window, !dist| prefix, wait-until-applied).
…und-6 P1) Codex P1 round-6 on PR #945 (line 115) — fence_ts pinned before source FSM actually rejects writes The FENCE catalog write commits on the default group, but the source FSM only learns about WriteFenced through the watcher's async catalog snapshot apply (distribution/watcher.go:11, 87-98). If the migrator records fence_ts = source.LastCommitTS() and starts DELTA_COPY immediately, a stale source leader can still accept writes for the moved keys for one watcher-tick worth of time after the catalog commit. Those writes get commit_ts > fence_ts, fall outside DELTA_COPY's (snapshot_ts, fence_ts] window, and are silently left on the source at CUTOVER — write loss after CUTOVER hands the routing to the target. Fix: new §3.2a 'Source-side fence-apply barrier'. The migrator advances FENCE → DELTA_COPY only after every voter of the source-group Raft membership has reported catalog_version_applied >= fence_catalog_version (via the cap_migration_v2 heartbeat's existing catalog_version_applied field, or a per-node GetCatalogVersion RPC if the heartbeat is stale). Only then does the migrator query source.LastCommitTS() and pin that as fence_ts. Crucially: this requires UNANIMOUS voter-ACK, not quorum. Any voter could become the leader at any future moment; without local WriteFenced applied, it would re-serve writes for the moved range in the pre-FENCE state. Quorum is necessary for catalog-write durability; unanimous voter-ACK is necessary for fence safety. State machine diagram (§3.2) updated: FENCE → DELTA_COPY now explicitly waits for the source-side fence ACK before picking fence_ts. SplitJob gains a fence_ack_cursor for restart-safety during the wait. A non-responsive voter blocks the phase; the migrator logs the pending voter list and either operator intervenes or Raft removes the voter via the leader-loss path. CUTOVER is structurally guaranteed by a different mechanism (the §7.2 read fence's authoritative ownership check), so no parallel 'every voter applies CUTOVER' gate is needed — readers are the consumers there.
|
@claude review — round 6 (1 P1 on commit 44e6125):
|
|
@codex review Addressed the fresh PR #945 Codex findings in 414c1f8 (doc-only).
Validation: git diff --check; GOCACHE=$(pwd)/.cache GOTMPDIR=$(pwd)/.cache/tmp go test ./... -run "^$". Caller audit for the fail-closed design change: rg over GetRouteOwnership/GetIntersectingRoutes/ErrRouteOwnershipUnknown/ErrRouteCutoverPending/ImportVersions. GetRouteOwnership and ErrRouteOwnershipUnknown are design-new symbols in this doc; existing GetIntersectingRoutes callers are distribution.Engine tests and kv/shard_store scan routing. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 414c1f8a63
ℹ️ 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".
| {Start: ddbItemPrefix(), End: ddbItemPrefixEnd(), Family: familyDdbItem}, | ||
| {Start: ddbGSIPrefix(), End: ddbGSIPrefixEnd(), Family: familyDdbGSI}, |
There was a problem hiding this comment.
When the moving route is a DynamoDB table, this plan only exports the item and GSI raw prefixes. The existing router also maps !ddb|meta|table| and !ddb|meta|gen| into the same table route (kv/shard_key.go:96-99), and the adapter reads those keys to load the table schema and generation (adapter/dynamodb.go:8163 and adapter/dynamodb.go:8179). Because those raw prefixes sort outside both the route-key bracket and the item/GSI brackets, they would never be copied, so after cutover the target can have the table's items but no schema/generation and DynamoDB operations will treat the table as missing or use generation 0. Add export brackets for the DynamoDB table metadata and generation prefixes alongside item/GSI.
Useful? React with 👍 / 👎.
| rKey := routeKey(rawKey) | ||
| // half-open [rangeStart, rangeEnd) in the routing-key namespace. | ||
| if bytes.Compare(rKey, rangeStart) < 0 { return false } | ||
| if rangeEnd != nil && bytes.Compare(rKey, rangeEnd) >= 0 { return false } |
There was a problem hiding this comment.
For an open-ended route, the wire contract above says route_end is empty for +infinity, but this predicate only special-cases nil. If an empty-but-non-nil slice reaches RouteKeyFilter, bytes.Compare(rKey, rangeEnd) >= 0 is true for every non-empty routing key, so BACKFILL/DELTA_COPY and the shared lock/prepare scans reject the entire rightmost range and can cut over without its data. Normalize empty ends to nil at the boundary or check len(rangeEnd) > 0 here.
Useful? React with 👍 / 👎.
|
@codex review Addressed the fresh PR #945 Codex findings in 8a1a301 (doc-only).
Validation: git diff --check; GOCACHE=$(pwd)/.cache GOTMPDIR=$(pwd)/.cache/tmp go test ./... -run "^$". Caller/semantic audit: rg over |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a1a301af5
ℹ️ 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".
| message MVCCVersion { | ||
| bytes key = 1; // raw storage key (NOT routeKey-normalized) | ||
| uint64 commit_ts = 2; | ||
| bool tombstone = 3; | ||
| bytes value = 4; // omitted when tombstone == true |
There was a problem hiding this comment.
Values written through PutWithTTLAt / ExpireAt have an expireAt timestamp in the MVCC value header, and the read path suppresses them once ExpireAt <= read_ts. This wire record only carries value/tombstone, so the importer has no way to preserve an existing TTL; a not-yet-expired key migrated before CUTOVER would be written on the target as non-expiring (or with an invented expiry), causing post-CUTOVER reads after the original expiry to resurrect data. Add expire_at to the exported version, or carry the raw stored-value header through import.
Useful? React with 👍 / 👎.
| // Visibility rules MUST match the read path's snapshot at maxCommitTS; | ||
| // in particular, intent locks (!txn|lock|...) are excluded — those | ||
| // belong to in-flight txns the importer must not observe as commits. |
There was a problem hiding this comment.
This contract says export visibility must match the read path at maxCommitTS, but the read path hides tombstones. In a DELTA_COPY where BACKFILL copied K@10 and the source deletes K at commit_ts=20 <= fence_ts, a read-visible export at fence_ts emits nothing for K, so the target keeps K@10 and the delete is lost after CUTOVER. The exporter needs to iterate raw committed MVCC versions in (minCommitTS, maxCommitTS], including tombstones, rather than only rows visible to a snapshot read.
Useful? React with 👍 / 👎.
| 3. **Background incremental promoter on the target group.** Immediately after CUTOVER the target group's FSM starts a **leader-local promoter goroutine** that walks the `!dist|migstage|<job_id>|*` prefix in cursor-resumable chunks (the same `chunkBytes` / pacing knobs §6.1 and the migrator already use), and for each staged version proposes one `PromoteStaged` Raft entry that: | ||
| - copies the staged key into its live position with the original imported `commit_ts` — **always**, regardless of whether a newer live version at a higher `commit_ts` exists. The MVCC store naturally holds both `K@commit_ts=3` (the staged version being promoted) and `K@commit_ts=10` (a later live write) at different commit timestamps; a snapshot read at any `read_ts ∈ [3, 10)` correctly sees the imported `K@3`, and a read at `read_ts ≥ 10` correctly sees `K@10`. The idempotency guard for this copy is "**no-op if a live version at the same `commit_ts` already exists**" — that means this exact staged version was already promoted in a prior batch before a leader flap re-tried it. It is **not** "no-op when a newer live version exists," which would silently drop the imported version from MVCC history and make snapshot reads at older `read_ts` return wrong results (closes claude round-3 P1, 5-round flag). History pruning belongs to `kv/compactor.go`, not the promoter, | ||
| - deletes the staged key, | ||
| - advances the per-job `promote_cursor` on the SplitJob. |
There was a problem hiding this comment.
PromoteStaged is described as a target-group Raft entry, but promote_cursor is stored in the default-group SplitJob and the later staged_visibility_active flag is default-group catalog state. In the CLEANUP path this cannot be atomic: a crash between separate target/default writes can resume from a cursor that skips unpromoted staged rows, or clear the merge flag before target promotion is durable. Keep promotion progress in target-local Raft state and clear catalog visibility only via an ordered default-group CAS after the target reports completion.
Useful? React with 👍 / 👎.
|
@codex review Addressed the fresh PR #945 Codex findings in 701845a (doc-only).
Validation: Caller/semantic audit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 701845acec
ℹ️ 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".
| - read the **newest live MVCC version of `K` with `commit_ts ≤ read_ts`** (the standard snapshot read against `!ddb|...K`, `!redis|...K`, the user-key band, etc. — whichever family `K` belongs to); | ||
| - read the **newest staged MVCC version of `K` with `commit_ts ≤ read_ts`** under `!dist|migstage|<job_id>|<K>` (the §6.1.1 staged area, iterated by the same `(raw_key ASC, commit_ts DESC)` order the snapshot iterator already uses); | ||
| - return whichever has the **greater `commit_ts`** (newest wins); if both are absent return "not found". |
There was a problem hiding this comment.
When promotion is only partially complete, using the standard snapshot read here hides tombstones/expired versions as “absent”, which can resurrect older staged data. For example, if staged has K@10 and a delete tombstone K@20, the promoter can copy/delete the K@20 staged row into live first; the live snapshot read then returns absent while the remaining staged K@10 is visible, so reads at read_ts >= 20 return the deleted value until the older staged row is promoted. The merge needs to compare raw MVCC versions including tombstone/expire_at and treat a winning hidden version as not found.
Useful? React with 👍 / 👎.
| 3. **CUTOVER precondition.** Before the migrator issues the CUTOVER catalog write, it ensures the target group's HLC ceiling is at least `hlcPhysicalMs(max_imported_ts)` by issuing a final `SetPhysicalCeiling(hlcPhysicalMs(max_imported_ts))` + `Observe(max_imported_ts)` proposal on the **target group** (not the default group). The proposal is gated by `cap_migration_v2` (§11.1) and is the **last** target-side write before CUTOVER. If the target HLC is already above this value (e.g. due to per-batch advancement), the `SetPhysicalCeiling` call is the trivial no-op the ceiling already enforces; if a follower flap dropped one of the per-batch advances, this proposal closes the gap. | ||
| 4. **Post-CUTOVER write-side invariant.** A target-group `Next()` after CUTOVER is bounded below by `metaLastCommitTS` AND by the HLC ceiling (the existing `HLC.Next()` semantics in `kv/hlc.go`), so it is provably strictly greater than every `commit_ts` ever imported under this job. Reads at any post-CUTOVER `Next()` therefore see the imported version as historical (older than the read ts) and see the new write as the most-recent committed version — no resurrection, no time travel. |
There was a problem hiding this comment.
This invariant does not survive a target restart/snapshot because HLC.last is in-memory: the existing snapshot restore path reapplies only the physical ceiling, and HLC.Next() does not consult metaLastCommitTS. If max_imported_ts has the same physical millisecond as the restored ceiling but a non-zero logical half, a restarted target can issue ms<<16|0, which is below an imported version despite metaLastCommitTS being higher. The design needs to durably re-observe the full max_imported_ts on target startup/apply, or advance the physical ceiling past that timestamp before serving post-CUTOVER writes.
Useful? React with 👍 / 👎.
|
@codex review Addressed the fresh PR #945 Codex findings in 41b8aba (doc-only).
Validation: Semantic/caller audit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41b8abaa6b
ℹ️ 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".
| {Start: txnIntPrefix(), End: txnIntPrefixEnd(), Family: familyTxnInternal}, | ||
| {Start: lstMetaPrefix(), End: lstMetaPrefixEnd(), Family: familyListMeta}, | ||
| {Start: lstItmPrefix(), End: lstItmPrefixEnd(), Family: familyListItem}, | ||
| {Start: redisPrefix(), End: redisPrefixEnd(), Family: familyRedis}, |
There was a problem hiding this comment.
This redisPrefix() bracket only covers !redis|... keys, but current Redis collection state is also stored under non-!redis prefixes such as !hs|, !st|, !zs|, and !stream| (see adapter/redis_compat_types.go's knownInternalPrefixes and the store/*_helpers.go layouts). A cross-group split of a route containing a Redis hash/set/zset/stream key would never export those rows (and needs matching routeKey decoders), so after CUTOVER the target would serve empty or partial collections. Please include these existing Redis wide-column prefixes in the bracket plan and tests.
Useful? React with 👍 / 👎.
|
@codex review 対応しました (b799e94): fresh Codex P1 の Redis wide-column migration bracket 漏れを doc 側で修正しました。
Validation:
Audit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b799e94b29
ℹ️ 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".
|
|
||
| M2 fixes this with a **three-step gate** on the FENCE → DELTA_COPY transition (step 0 added per claude round-12 P1 on PR #945 — prepared-txn drain — see §7.1's no-`Phase_COMMIT`-bypass rationale): | ||
|
|
||
| 0. **Prepared-txn drain (resolves OQ-6).** Before transitioning `BACKFILL → FENCE` (yes, before — the FENCE catalog write itself does not run until this drain returns), the migrator waits until **no in-flight transaction intent locks remain in the moving range**. "No intents remaining" is observed via the **route-faithful lock drain** defined in §3.2a.0a below — a `!txn|lock|` family scan filtered by `routeKey(lockedKey) ∈ [routeStart, routeEnd)`, returning empty. The wait is bounded by the txn timeout — a long-lived prepared txn either commits or is aborted by the lock resolver (`kv/lock_resolver.go`) within its TTL, so the scan converges in at worst one TTL window. While the drain is pending the migrator logs `last_error = "fence drain pending: <N> intent locks in moving range"`. **Why this is required:** §7.1's `verifyRouteNotFenced` rejects every write request in the WriteFenced range and has **no `Phase_COMMIT` bypass** because the matching intent lock is excluded from BACKFILL (§6.1) and absent on the target after CUTOVER — a `Phase_COMMIT` reaching the FSM after FENCE has nothing to commit against. Letting a prepared txn into the FENCE phase would therefore strand its commit indefinitely. The drain guarantees every commit that reaches the FSM during/after FENCE is for a txn whose intent has already resolved (committed via the pre-FENCE accept window, or aborted by the lock resolver). After CUTOVER the target has no historical intent for a moved-range key, so a coordinator that retries a Phase_COMMIT against the target would also fail; the drain is the only safe ordering. |
There was a problem hiding this comment.
While this BACKFILL→FENCE drain is waiting, the route is still Active and nothing prevents new prepares from landing in the moving range. Under a steady transactional workload, each lock may resolve within its TTL but another lock can arrive before the scan is empty, so the job is not bounded by one TTL and can starve before it ever writes the WriteFenced route. The migration needs a draining gate or Phase_COMMIT-safe fence that stops new prepares before relying on an empty lock scan.
Useful? React with 👍 / 👎.
|
|
||
| Because the source is still `Active` during BACKFILL and FENCE-apply, new prepares with arbitrarily low caller-supplied / lagging-clock `commit_ts` can keep appearing on **either** the primary or a secondary key in the moving range; the running minimum must capture every one. To avoid an inter-tick gap — a prepare that both lands and resolves between two ticks would never appear in a *live-lock* sample — the running minimum is taken over the following sources, all route-faithful by `routeKey()` against the moving range (`routeKey(key) ∈ [routeStart, routeEnd)`): | ||
| - the live `!txn|lock|` family (in-flight prepares): each accepted lock is in the moving range by its **own** `routeKey(lockedKey)` regardless of whether the locked key is the txn's primary or a secondary, so a secondary lock in the moving range counts here. For each accepted lock the migrator records `(routeKey(lockedKey), PrimaryKey, StartTS)` from the lock value (`txnLock` carries `PrimaryKey` + `StartTS`, `kv/txn_codec.go:223-245`; the `commit_ts` itself is added by the M2-PR0 wire change). Timestamp priority is explicit: first use the non-zero `commit_ts` in the lock value; for legacy locks where that field is absent or zero, fall back to a **direct point lookup** of the commit record at `!txn|cmt|<PrimaryKey>|<startTS>`. If the direct lookup is absent because the legacy txn is still prepared, the migrator treats that lock as unresolved and keeps the relevant scan/drain pending until the lock resolves or the resolver aborts it. The migrator already holds the exact primary key, so it does not need the commit record to route into the moving range. The primary-keyed commit record is therefore consulted by **direct address from the secondary lock**, never by a route-faithful commit-record-family *scan* (which, per the paragraph above, cannot find a moved-secondary's primary-keyed record), AND | ||
| - the **committed MVCC versions in the moving range itself** — a route-faithful scan of committed versions over `[routeStart, routeEnd)` (the same §6.3.1 brackets + `routeKey()` filter the data export uses) with `commit_ts ≤ snapshot_ts`. This is the backstop for the same-tick land-and-resolve case where the secondary's intent lock was never sampled: the txn's committed secondary version `(secondaryKey, commit_ts)` is written into the moving-range MVCC space and routes into `[routeStart, routeEnd)` by construction, so this scan observes its `commit_ts` directly without any dependence on the primary-keyed commit record. (It is sufficient on its own for correctness; the live-lock + direct-commit-record path above is the cheap fast path that lets the migrator floor `delta_floor` below a still-prepared secondary before its version even commits, sparing DELTA_COPY a re-export.) |
There was a problem hiding this comment.
This final scan includes every committed MVCC version in the range with commit_ts <= snapshot_ts, so any old retained version that was already copied by BACKFILL lowers snapshot_min_prepared_ts to the oldest history, not just to late prepares that BACKFILL missed. That makes delta_floor near zero and forces DELTA_COPY to re-export almost the entire range while writes are fenced, turning the intended short cutover window into another full backfill for large ranges. The design needs a way to distinguish newly resolved prepares from ordinary historical versions before using them to set the floor.
Useful? React with 👍 / 👎.
| | CUTOVER → CLEANUP Source group's leader deletes | ||
| v the moved key range from its | ||
| [ source.Active(left) ] MVCC store after the read-fence | ||
| [ target.Active(right) ] grace window (§7.2). |
There was a problem hiding this comment.
If the source is the default group and the moving right child intersects !dist|, this cleanup deletes the authoritative catalog/SplitJob keys that §3.1 stores under !dist|meta|, !dist|route|, and !dist|job|. A split such as one starting at or before !dist| would migrate and then remove the control-plane records needed to route and resume the job. Cross-group migration should explicitly reject or carve out the default-group reserved catalog prefix before applying generic source-range deletion.
Useful? React with 👍 / 👎.
|
@codex review 対応しました (a603f17): fresh Codex findings at b799e94.
Validation:
Semantic/caller audit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a603f175c3
ℹ️ 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".
| {Start: lstMetaPrefix(), End: lstMetaPrefixEnd(), Family: familyListMeta}, | ||
| {Start: lstItmPrefix(), End: lstItmPrefixEnd(), Family: familyListItem}, |
There was a problem hiding this comment.
When a Redis list has un-compacted metadata deltas or LPOP/RPOP claim keys at migration time, this bracket plan only exports !lst|meta| and !lst|itm|. The existing store also persists live list state under store.ListMetaDeltaPrefix (!lst|meta|d|) and store.ListClaimPrefix (!lst|claim|), which Redis reads/compaction consume via the corresponding scan prefixes, while current routeKey() only calls store.ExtractListUserKey for meta/item keys. Without adding both brackets and matching route-key decoders, the target misses these rows after CUTOVER, so list length/head state can be wrong and pop claim protection can be lost.
Useful? React with 👍 / 👎.
| - **Branch 1 — source has applied CUTOVER (`source_local_version >= cutover_version`).** The FSM calls `GetIntersectingRoutes(start, end)` against its **own** current catalog snapshot. If **every** intersecting route is owned by this group, the scan proceeds. If **any** intersecting route is owned by a different group, the FSM returns `ErrRouteMoved{new_route_version, new_group_id, sub_range_start, sub_range_end}` for the **first** foreign-owned sub-range encountered (lexicographically smallest). | ||
| - **Branch 2 — source has NOT applied CUTOVER but `max_cutover_version_seen > source_local_version` (equal-stale, watcher lag).** The local snapshot still says `source.Active(full: [A, Z))` — a single route, no foreign owner visible — so Branch 1 alone would silently pass a cross-boundary scan. The gate therefore additionally calls `GetIntersectingRoutes([start, end), catalog_version = max_cutover_version_seen)` as a single deadline-bounded RPC to the **default-group leader** (mirroring exactly §7.2.2's point-key extension — same watermark trigger, same RPC target, same 1 s cache TTL with cache key `(start, end, max_cutover_version_seen)`). The default-group leader answers from its current catalog snapshot. If any returned route is owned by another group, the source FSM returns `ErrRouteMoved{max_cutover_version_seen, foreign_group_id, sub_range_start, sub_range_end}` for the first foreign-owned sub-range. If every returned route is still source-owned (the elevated watermark covers an *unrelated* migration that did not touch this scan range), the scan proceeds normally. If the RPC times out, the default-group leader is unreachable, or the route set cannot be verified at `max_cutover_version_seen`, the FSM returns `ErrRouteOwnershipUnknown{route_version=max_cutover_version_seen, sub_range_start=start, sub_range_end=end, retry_after_ms}` and **must not scan source MVCC**. |
There was a problem hiding this comment.
When the stale read is an adapter/internal scan, both branches here compare the raw scan bounds against catalog routes. For families whose raw scan prefix sorts outside the logical route key acknowledged in §6.3.1 (for example Redis wide-column !hs|... / !st|... / !zs|... or list delta/claim prefixes), raw [start,end) can appear source-owned even though the rows' routeKey() values are in the moved right half. In that case the source FSM will pass the fence and scan source MVCC, returning stale collection data after target.Active; the scan fence needs per-family route-key interval mapping or fail-closed filtering instead of raw GetIntersectingRoutes(start,end).
Useful? React with 👍 / 👎.
|
@codex review 対応しました (646fe7e): fresh Codex P1 findings at a603f17.
Validation:
Semantic/caller audit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 646fe7ee86
ℹ️ 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".
| M2 therefore makes target visibility depend on a synchronous source-side read barrier: | ||
|
|
||
| 1. After DELTA_COPY completes and after the target full-HLC floor proposal in §6.2.1, but **before** the default-group CUTOVER catalog CAS, the migrator proposes `ArmCutoverReadFence(job_id, route_start, route_end, expected_cutover_version, target_group_id)` to the **source group**. `expected_cutover_version` is the catalog version the pending CAS will create (`current_catalog_version + 1` under the CAS input). | ||
| 2. The source FSM applies this as source-group Raft state: a pending-fence entry keyed by `(job_id, route_start, route_end, expected_cutover_version)`. After the source-group apply ACK, the migrator records `cutover_read_fence_armed = true` on the default-group SplitJob as restart progress. From that point until the source watcher applies CUTOVER and normal §7.2.2 Branch 1 takes over, any point read or scan whose **route-key-normalized interval** intersects the moving range and whose `read_route_version < expected_cutover_version` **must not read source MVCC**. If a raw internal scan cannot be normalized to route-key intervals, the source fails closed with `ErrRouteOwnershipUnknown` / retry rather than proving ownership from raw bytes. If the source has already learned that the default-group CAS committed (via watcher, heartbeat watermark, or the ownership RPC), it returns `ErrRouteMoved{expected_cutover_version, target_group_id}`. Otherwise it returns `ErrRouteCutoverPending{retry_after_ms}`; coordinators treat it like `ErrRouteMoved` with a short wait + `WatcherRefresh` retry. A transient retry is acceptable; serving stale source data is not. |
There was a problem hiding this comment.
If the default-group leader crashes after the source-group ACKs ArmCutoverReadFence but before this SplitJob flag is written, the source has a durable pending fence while recovery has no witness that it must complete the CAS or clear the fence. In that window, reads intersecting the moving range keep returning ErrRouteCutoverPending (including after an abandon/rollback path that sees cutover_read_fence_armed=false) because no migrator is required to clear the source-group state. Persist an arming intent before the source proposal, or make recovery/abandon probe and clear source fences even when this flag is false.
Useful? React with 👍 / 👎.
## Summary Doc-only PR. Proposes **multi-node multi-group bootstrap** — the prerequisite for true distributed multi-Raft scaling. Today a real multi-node deployment can only run **single-group**: `resolveBootstrapServers` rejects `--raftBootstrapMembers` when `len(groups) != 1` (`main.go:744-748`), `groupSpec` carries one address per group (`shard_config.go:14-17`), and `buildRuntimeForGroup` threads the same (nil-in-multi-group) `bootstrapServers` into every group (`multiraft_runtime.go:234-254`), so each group bootstraps single-member and the factory never even builds a transport (`factory.go:49-52`). This is exactly the PR0 / OQ-9 "option (a)" gap the leader-balance design (#953) identified. New file: `docs/design/2026_06_12_proposed_multinode_multigroup_bootstrap.md` (Status: Proposed / Author: bootjp / Date: 2026-06-12). Every current-state claim is cited at file:line, verified against `main`. ## Key design points - **Flag surface:** new companion `--raftGroupPeers "1=n1@host1:5051,n2@host2:5051,...;2=..."` (per-group `raftID@host:port` voter lists), lifting the `len(groups)==1` guard. `--raftGroups`/`--raftBootstrapMembers`/`--raftBootstrap` and the `cmd/server/demo.go` demo are unchanged; mutually exclusive with `--raftBootstrapMembers`; full validation rules (local-node-present, addr-match, homogeneity). - **Bootstrap semantics:** all-nodes-same-list (the etcd model `cmd/server/demo.go:204-219` already uses for single-group), NOT a designated proposer. Idempotent restart and divergent-config fail-fast come free from the existing persisted-peers + `errClusterMismatch` + `raft-engine` marker machinery (already per-group). - **Transport/addressing:** transport is already per-group and address-map driven; keep one listener per group (the `5005{1..6}` convention). No transport change — only feeding per-group lists in. - **Alternative considered:** AddVoter-composition stays the **live-expansion** path, not the bootstrap path (per #953 OQ-9). - **Cross-doc impact:** unblocks leader-balance PR2/PR3 (#953) and hotspot-split M2 cross-group migration testing (#945). ## Open questions OQ-1 flag spelling (companion flag vs. extend `--raftGroups`); OQ-2 `--raftBootstrap` required vs. implied; OQ-3 per-group carrier (map vs. func); OQ-4 heterogeneous membership (hard error vs. warning in v1); OQ-5 learner-start interop; OQ-6 single multiplexed raft listener for very high M. ## Test plan Doc-only — no code, no tests run. The doc itself proposes the test strategy: unit (flag-parse table tests, per-group resolution, restart idempotency), integration (in-process 3-node × 2-group harness asserting multi-voter `Configuration()` + inter-node `TransferLeadershipToServer`), and a later Jepsen milestone (true multi-node multi-group runner). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added comprehensive design proposal for multi-node multi-group bootstrap capabilities, including CLI flag specifications, backward compatibility guidelines, bootstrap semantics, and integration testing roadmap. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
@codex review 対応しました (ad2f7bb): fresh Codex P1 finding at 646fe7e.
Validation:
Semantic/caller audit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad2f7bb534
ℹ️ 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 tracker is keyed by the **locked key**, not the primary, so a secondary lock in the moving range counts even when the txn's primary routes elsewhere. Verified against `kv/fsm.go:1136-1142` and `kv/txn_keys.go:50-58`: the durable `!txn|cmt|<primaryKey>|<startTS>` commit record is written **only when committing the primary** (`if committingPrimary`) and is keyed by `primaryKey`, so for such a txn the commit record's `routeKey()` falls **outside** `[routeStart, routeEnd)` — a commit-record family scan filtered by `routeKey()` never sees it. The migrator may still use the exact `PrimaryKey` + `StartTS` decoded from a live lock to do a **direct point lookup** for backward-compat locks where the M2-PR0 `commit_ts` field is absent or zero, but it never relies on a route-faithful commit-record-family scan for moved-secondaries. | ||
|
|
||
| The minimum is only **final** once §3.2a step 1's unanimous voter-ACK has made every source voter reject new prepares (`verifyRouteNotFenced`) **and** step 0b's post-fence drain has returned empty — at that point no future prepare can land on any key in the moving range, and every migration-window prepare was either recorded by the tracker or was a pre-tracker lock sampled by the drain path. Tracker rows are retained until `delta_floor` is finalised, then deleted by the same cleanup that clears the import ack state for the job. | ||
| 3. **Finalise the boundary after the post-fence drain.** When step 0b's drain returns empty (`post_fence_drain_completed = true`), the migrator reads the prepare tracker and live-lock scan **one last time**; the lock side returns empty by construction (no in-flight prepare survives the drain), and the tracker yields the final running minimum over every migration-window prepare — primary or secondary — that resolved into the moving range. The DELTA_COPY lower bound is then fixed as `delta_floor = min(snapshot_ts, snapshot_min_prepared_ts - 1)` and DELTA_COPY exports the window **`(delta_floor, fence_ts]`** (not `(snapshot_ts, fence_ts]`). Any post-scan prepare — on the primary or a moved secondary — that committed with `commit_ts ≤ snapshot_ts` was recorded by the step-2 tracker, so `delta_floor ≤ snapshot_min_prepared_ts - 1 < commit_ts`, i.e. `commit_ts > delta_floor` and the version falls **inside** DELTA_COPY's window — it is no longer dropped. `delta_floor` is persisted on the SplitJob alongside `snapshot_min_prepared_ts` so a DELTA_COPY resume uses the same lower bound. |
There was a problem hiding this comment.
This boundary only accounts for prepared transactions, but the existing non-prepare paths can also apply preallocated timestamps: handleRawRequest writes at r.Ts, and handleOnePhaseTxnRequest writes at meta.CommitTS. While the source route remains Active during BACKFILL, a raw/one-phase request can allocate or carry commit_ts <= snapshot_ts, apply after the exporter has already passed its key, and create no prepare-tracker row or live lock; with no prepares lowering the floor, DELTA_COPY's (delta_floor, fence_ts] still starts at snapshot_ts and excludes that committed version, so CUTOVER can lose it. The fence/drain logic needs to cover all admitted low-timestamp writes, not only prepares, before finalizing this floor.
Useful? React with 👍 / 👎.
|
@codex review 対応しました (cbdc0fc): fresh Codex P1 finding at ad2f7bb.
Validation:
Semantic/caller audit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbdc0fc576
ℹ️ 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".
|
|
||
| 1. **Arm the migration write tracker, then choose the provisional snapshot for BACKFILL.** Before opening BACKFILL, the migrator proposes `ArmMigrationWriteTracker(job_id, route_start, route_end)` to the source group and waits for it to apply; only then may it set `write_tracker_armed=true` on the default-group SplitJob and choose `snapshot_ts`. The tracker is source-group Raft state. From that apply onward: | ||
|
|
||
| - `handleRawRequest` appends a tracker row for each mutation whose `routeKey(mut.Key) ∈ [routeStart, routeEnd)`, using the request's `commit_ts = r.Ts`, in the same apply that writes the MVCC version. |
There was a problem hiding this comment.
This records raw writes only by the point key routeKey(mut.Key), but handleRawRequest special-cases Op_DEL_PREFIX into DeletePrefixAt, which writes tombstones for every live key under the prefix (and nil prefix means all keys). If such a prefix delete is admitted after BACKFILL has passed moved keys with r.Ts <= snapshot_ts, it can create low-ts tombstones without lowering snapshot_min_admitted_ts, so DELTA_COPY may exclude those tombstones and the target resurrects values copied by BACKFILL. Prefix deletes need to be recorded as range-intersecting migration-window writes, not as a single mutation key.
Useful? React with 👍 / 👎.
| FSM-side defense in depth — **must run on an unconditional apply path** (closes codex round-3 P1 on PR #945): | ||
|
|
||
| - The naïve placement — extend `verifyOwnerFromSnapshot` so it also rejects routes in `WriteFenced` — does NOT work. `verifyComposed1` short-circuits when `ObservedRouteVersion == 0` (`kv/fsm.go:608-611`), and there are several **intentional zero-version write paths today**: read/write txns with `ReadKeys`, caller-supplied `StartTS`, and resolver-claimed keys all dispatch with `ObservedRouteVersion = 0` (`kv/sharded_coordinator.go:694-754`, M3 sibling §3.5 + auto-pin policy). A stale coordinator in any of those flows would forward a write to the source during FENCE and the FSM defense would never run. The fence must be enforced **before** any version-conditional early return. | ||
| - M2 therefore introduces a **separate, unconditional pre-gate** `verifyRouteNotFenced(mutations)` in `kv/fsm.go` that the apply path consults **before** `verifyComposed1`. It scans the mutations exactly as `verifyOwnerFromSnapshot` does — `routeKey(mut.Key)` against the **current** catalog snapshot — and rejects with `ErrRouteWriteFenced` when **any** key resolves to a route whose `RouteState == WriteFenced`. No dependency on `ObservedRouteVersion`, no early return on `0`; the gate runs on every applied write request unconditionally. The gate has the same `Phase_ABORT` bypass as the Composed-1 gate (for the same lock-release reason in §3.5 of the parent partial doc). `Phase_COMMIT` is **not** a blanket bypass: it is allowed only while the SplitJob is still in the post-fence drain window (`post_fence_drain_completed=false`) and only if every committed mutation resolves a matching pre-existing `!txn|lock|` row for the same `(primary_key, start_ts, commit_ts)` in the moving range. That narrow lane lets already-prepared txns resolve after the fence has stopped new prepares (closing codex round-18 P1's starvation issue) without letting a stale coordinator create new state. Once the post-fence drain returns empty, the lane closes and any later `Phase_COMMIT` for the moved range is rejected; after CUTOVER the target has no historical intent for that key, by construction. |
There was a problem hiding this comment.
The proposed fence gate checks each mutation as a point lookup via routeKey(mut.Key), but Op_DEL_PREFIX is a range write: an empty prefix is broadcast to every group, and a non-empty prefix can overlap the moving right child even when the prefix key itself is outside it. During FENCE/DELTA_COPY such a request would bypass verifyRouteNotFenced, write source tombstones after the point where the source is supposed to be write-fenced, and those tombstones are outside the already-fixed copy window, so deleted data can reappear after CUTOVER. The fence path needs explicit DEL_PREFIX intersection handling.
Useful? React with 👍 / 👎.
|
@codex review 対応しました (c0a678f): fresh Codex P1 review 4492933289.
Validation:
Semantic/caller audit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0a678f1bf
ℹ️ 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".
|
|
||
| | PR | Scope | Tests | | ||
| |---|---|---| | ||
| | M2-PR0 | Add `commit_ts` to the `txnLock` wire value on the Raft-applied lock-write path; decoder backward-compat for existing lock values without the field (zero → migrator fallback to direct commit-record lookup / pending drain) | `kv/txn_codec_test.go` round-trip + backward-compat case | |
There was a problem hiding this comment.
PR0 only calls out adding commit_ts to the txnLock value, but in the current multi-shard path the PREPARE request has no actual commit timestamp (prewriteTxn builds txnMetaMutation(..., 0) and sends Request.Ts = startTS; the real commit_ts is sent later on COMMIT). If the implementation follows this touchpoint literally, handlePrepare still cannot populate the new lock field or append the admission-time tracker row with the transaction's commit_ts, so a low-ts prepare that lands and resolves between migrator polls can be absent from snapshot_min_admitted_ts and the DELTA_COPY floor can again exclude the committed version. Please include the coordinator/request/meta plumbing and a prepare-path test here, not just the lock codec round-trip.
Useful? React with 👍 / 👎.
1 participant
Summary
docs/design/.Key design points
!dist|job|<id>so a default-group leader flap resumes mid-migration without operator action.routeKey()equivalence check on every exported row, so!txn|,!lst|,!ddb|,!redis|,!sqs|,!s3|families all land in the right shard.target_group_id == 0or equal to source short-circuits through the existing fast path.Open questions for reviewers
7 questions called out at
§12— notably: grace-period sizing, abandon-after-cutover policy, backfill-vs-prepare semantics, andMVCCVersion.key_familystrict-enum vs numeric.Test plan
*_proposed_*→*_partial_*once PR1 lands.Summary by CodeRabbit