Add snapshot offload substrate#1151
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@codex review |
📝 WalkthroughWalkthrough物理スナップショットのオブジェクトストア公開・復元基盤とCLI、HLCリース回復、Redis接続プール設定、XREAD型検証、FSMスナップショット復元のキャンセル対応を追加した。 Changes物理スナップショットのオブジェクトオフロード
HLCリース回復
Redis接続プール設定とストリーム応答
FSMスナップショット処理
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TimestampAllocator
participant Coordinator
participant ShardedCoordinator
participant RaftLeaseGroup
TimestampAllocator->>Coordinator: ceiling期限切れ時にRecoverHLCLease
Coordinator->>ShardedCoordinator: 回復対象グループを選択
ShardedCoordinator->>RaftLeaseGroup: HLC leaseを提案
RaftLeaseGroup-->>TimestampAllocator: 回復後のfenced timestamp
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![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: 8c493c1199
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return "", errors.Wrap(ErrInvalidOptions, "object store is required") | ||
| } | ||
| normalized := normalizeObjectKey(key) | ||
| if normalized == "" || normalized == "." || strings.HasPrefix(normalized, "../") { |
There was a problem hiding this comment.
For keys that normalize exactly to .. (for example .. or a/../..), this check does not reject the key, so filepath.Join(s.root, "..") resolves outside the local store root. With a nested root whose parent does not already exist, PutObject can create the hard link at that parent path, and a crafted manifest can make restore read a parent-path file if it exists and matches the advertised checksum. Please reject normalized == ".." in addition to ../....
Useful? React with 👍 / 👎.
| var buf bytes.Buffer | ||
| if _, err := io.Copy(&buf, contextReader{ctx: ctx, reader: body}); err != nil { |
There was a problem hiding this comment.
If an operator passes a payload key by mistake, or the object store returns a corrupt manifest object with snapshot-sized contents, this copies the entire object into memory before JSON decoding. Physical snapshot payloads can be many GiB, so restore can OOM before it has a chance to reject the object as not being a manifest. Please cap the manifest body with a small maximum size while loading it.
Useful? React with 👍 / 👎.
| } | ||
| }() | ||
| hash := sha256.New() | ||
| n, err := export.WriteTo(io.MultiWriter(tmp, hash)) |
There was a problem hiding this comment.
When ctx is cancelled during publication, this still streams the entire persisted snapshot to the temporary file before checking ctx.Err(). For multi-GiB snapshots, shutdown or leadership-loss cancellation can therefore spend minutes doing local I/O and hashing even though the publish is already abandoned. Please make the writer/context participate in the WriteTo copy so cancellation interrupts the spool promptly.
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: 5
🧹 Nitpick comments (1)
docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winステータス更新はライフサイクル規約の
partial状態と一致するが、ファイル名はproposedのまま。新しい
Statusは「M0 実装済み、M1 部分実装」であり、これはガイドラインが定義する*_partial_*.md（一部マイルストーン実装済み、全体未完了）に該当する。しかし本ファイルは末尾の記述（165-169行目、既存）で意図的に M1-M3 完了までproposedのファイル名/ヘッダを維持すると明記しており、ガイドラインの3状態運用から外れている。意図的な運用上の判断であれば問題ないが、確認しておきたい。
As per coding guidelines, "Design docs follow lifecycle markers:*_proposed_*.md(design accepted, not yet implemented),*_partial_*.md(some milestones shipped, full proposal not complete),*_implemented_*.md(all milestones shipped, preserved as as-built record)".🤖 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_07_19_proposed_physical_snapshot_object_offload.md` around lines 3 - 6, Update the document’s lifecycle marker to match its current “M0 implemented, M1 partial” status: rename the file from the proposed marker to the partial marker and update any corresponding header or references. Remove or revise the existing statement near the document’s lifecycle guidance that intentionally preserves the proposed name until M1–M3 completion.Source: Coding guidelines
🤖 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 `@internal/snapshotoffload/manifest.go`:
- Around line 70-101: Update DecodeManifest validation to require a non-empty
ManifestSHA256 and compare it with the hash recomputed from MarshalCanonical.
Reject manifests when the field is missing or does not match the canonical hash,
while preserving the existing validateManifest checks and error handling.
In `@internal/snapshotoffload/publish.go`:
- Around line 110-124: putManifest に、putPayload
と同様の既存オブジェクト整合性チェックを追加してください。store.HeadObject で manifest.ManifestKey の既存
size/SHA256 を確認し、一致しない場合は ErrIntegrity を返して PutObject
に進まないようにします。一致または未存在時のみ現在の公開処理を継続し、必要に応じて既存の hexSHA256Bytes(data)
を比較値として再利用してください。
In `@internal/snapshotoffload/restore.go`:
- Around line 47-79: Update RestorePhysicalSnapshot to create the parent
directory of the cleaned opts.DataDir with os.MkdirAll before calling
os.MkdirTemp, preserving the temporary directory’s placement on the same
filesystem. Also add startup cleanup for leftover .snapshot-offload-restore-*
directories in that parent directory, while retaining the existing deferred
RemoveAll cleanup.
In `@internal/snapshotoffload/store.go`:
- Around line 66-104: Update objectInfoForPath and the GetObject/HeadObject
paths to avoid reading the entire object solely to compute SHA256. Build
ObjectInfo from os.Stat-based metadata, preserving not-found handling, and leave
content integrity verification to the existing streaming/commit-time logic.
Ensure GetObject opens the file only for the returned reader and HeadObject
remains a lightweight existence/metadata check.
- Around line 106-115: Update the traversal validation in LocalStore.pathForKey
to reject the normalized key ".." in addition to the existing invalid values,
while preserving the current handling for "../" descendants and valid keys.
---
Nitpick comments:
In `@docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md`:
- Around line 3-6: Update the document’s lifecycle marker to match its current
“M0 implemented, M1 partial” status: rename the file from the proposed marker to
the partial marker and update any corresponding header or references. Remove or
revise the existing statement near the document’s lifecycle guidance that
intentionally preserves the proposed name until M1–M3 completion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddd348fb-534b-402d-9413-ac18231ef534
📒 Files selected for processing (6)
docs/design/2026_07_19_proposed_physical_snapshot_object_offload.mdinternal/snapshotoffload/manifest.gointernal/snapshotoffload/offload_test.gointernal/snapshotoffload/publish.gointernal/snapshotoffload/restore.gointernal/snapshotoffload/store.go
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b96976e8aa
ℹ️ 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".
| } | ||
|
|
||
| func spoolExport(ctx context.Context, export *etcdraftengine.PersistedSnapshotExport) (*os.File, string, int64, error) { | ||
| tmp, err := os.CreateTemp("", "elastickv-snapshot-offload-*.fsm") |
There was a problem hiding this comment.
For large persisted snapshots, this writes the entire FSM payload to the process default temp directory before upload. Snapshot offload is meant for multi-TB shards, and /tmp is often a small root partition or tmpfs, so publishing can fail or exhaust host storage even when the raft data volume and object store have capacity. Use a configured/data-dir-local spool location or avoid requiring a full copy in the default temp area.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dc76d41a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if opts.Manifest != nil { | ||
| return *opts.Manifest, nil |
There was a problem hiding this comment.
When callers pass RestoreOptions.Manifest instead of ManifestKey, this path returns the struct without the self-hash verification that LoadManifest/DecodeManifest applies. A cached or tampered manifest whose payload descriptor is still valid can therefore seed a restore with altered snapshot index/term or payload key, bypassing the manifest commit-marker integrity check; please run the same self-hash validation before accepting inline manifests.
Useful? React with 👍 / 👎.
| if err := downloadVerifiedPayload(ctx, opts.Store, manifest, payloadPath); err != nil { | ||
| return nil, err | ||
| } | ||
| result, err := etcdraftengine.PreparePhysicalSnapshotRestore(etcdraftengine.PhysicalSnapshotRestoreOptions{ |
There was a problem hiding this comment.
If Peers contains a duplicate/zero node ID, invalid suffrage, or only learners, the full object payload is still downloaded and checksummed here before PreparePhysicalSnapshotRestore rejects the peer set. For the multi-TB snapshots this path is intended to handle, an operator typo can waste hours of network and disk I/O; validate the complete peer set before starting downloadVerifiedPayload.
Useful? React with 👍 / 👎.
| if err := syncDir(filepath.Dir(finalPath)); err != nil { | ||
| return ObjectInfo{}, err |
There was a problem hiding this comment.
If the directory fsync fails after os.Link succeeds, PutObject returns an error but leaves finalPath visible. For manifest writes this means PublishPersistedSnapshot can report failure while a restorable commit marker remains in the local store, so remove or otherwise quarantine the linked object before returning the sync error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd92cd78f6
ℹ️ 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".
| defaultRedisBlockWaitFallback = 250 * time.Millisecond | ||
| // Keep normal producer wakeups event-driven while reducing idle fallback scans | ||
| // from blocked consumers waiting on empty collections. | ||
| defaultRedisBlockWaitFallback = time.Second |
There was a problem hiding this comment.
With short XREAD BLOCK windows below one second, wrong-type writes that do not signal streamWaiters (for example a SET on the blocked stream key) now rely on this fallback timer but it is capped to the remaining BLOCK deadline. When that timer fires at the deadline, xreadBusyPoll takes the iterTimeout <= 0 branch and returns null before xreadOnce can run keyTypeAtExpect and surface WRONGTYPE, so Redis clients can miss mid-block type changes for common sub-second waits; keep the default below those windows or perform one final full type check before returning null.
Useful? React with 👍 / 👎.
| result, err := etcdraftengine.PreparePhysicalSnapshotRestore(etcdraftengine.PhysicalSnapshotRestoreOptions{ | ||
| InputFSMPath: payloadPath, | ||
| DataDir: opts.DataDir, | ||
| Index: manifest.SnapshotIndex, | ||
| Term: manifest.SnapshotTerm, | ||
| Peers: opts.Peers, | ||
| ExpectedPayloadSHA256: manifest.Payload.SHA256, | ||
| }) |
There was a problem hiding this comment.
If ctx is canceled after the payload has been downloaded, this call still rewrites and fsyncs the full payload through PreparePhysicalSnapshotRestore because the context is no longer checked or passed into that copy path. For multi-TB offloaded snapshots, an operator abort or shutdown at this point can keep consuming local I/O and disk for a long time; check cancellation before this step and make the prepare/copy path cancellable.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (958d56d):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
kv/lease_read_test.go (1)
191-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
//nolint:gosecの使用について。境界チェック（例:
ceilingMs >= 0相当の検証）を追加すれば//nolintなしでも成立する可能性があります。テスト専用ヘルパーであり実害は低いため任意対応で構いません。As per coding guidelines, "Avoid using
//nolintdirectives; refactor instead in Go code."🤖 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 `@kv/lease_read_test.go` around lines 191 - 200, Remove the gosec suppression in applyHLCLeaseEntryToClock by validating that the decoded ceilingMs is non-negative before calling clock.SetPhysicalCeiling. Preserve the existing lease-entry length/type checks and return early for invalid decoded values.Source: Coding guidelines
kv/tso.go (1)
176-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
nextFencedWithRecoveryとnextBatchFencedWithRecoveryのロジック重複。両関数は「フェッチ→
ErrCeilingExpired判定→回復→再フェッチ」という同一の骨格を持っており、単発/バッチの違いのみです。ジェネリクスで共通化するとメンテナンス性が上がります。♻️ 共通化の一例
func fencedWithRecovery[T any](ctx context.Context, recoverer hlcLeaseRecoverer, label string, fetch func() (T, error)) (T, error) { v, err := fetch() if err == nil { return v, nil } var zero T if !errors.Is(err, ErrCeilingExpired) || recoverer == nil { return zero, errors.Wrap(err, label) } if recoverErr := recoverer.RecoverHLCLease(ctx); recoverErr != nil { return zero, errors.Wrapf(err, "%s: on-demand HLC lease renewal failed: %v", label, recoverErr) } v, err = fetch() if err != nil { return zero, errors.Wrap(err, label) } return v, nil }🤖 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 `@kv/tso.go` around lines 176 - 210, 共通の「取得→ErrCeilingExpired判定→リース回復→再取得」処理を新しいジェネリック関数 fencedWithRecovery に抽出し、fetch コールバックで単発取得とバッチ取得を差し替える構成に変更してください。nextFencedWithRecovery は clock.NextFenced、nextBatchFencedWithRecovery は clock.NextBatchFenced を渡して共通関数を利用し、既存のエラー処理と戻り値を維持してください。
🤖 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 `@adapter/redis_peer_limiter_test.go`:
- Around line 10-18: Update TestRedisPeerLimiterDefaultMatchesProxyPool to
compare limiter.limit with the actual proxy default from
proxy.DefaultElasticKVBackendOptions().PoolSize instead of the independent
defaultElasticKVProxyPoolSizeForTest constant. Remove the copied test constant
and preserve the test’s verification that newDefaultRedisPeerLimiter uses the
proxy’s current default.
In `@internal/raftengine/etcd/engine.go`:
- Around line 3368-3369: Update releaseProtectedReceivedFSMSnapshotsUpTo so a
failed snapshotMu.TryLock does not abandon the release request: requeue or
otherwise schedule the requested release for retry, preserving the existing
cleanup once the lock is acquired. Ensure drainReady’s single Ready processing
signal eventually removes the target entries from protectedReceivedFSMSnaps even
when the initial lock attempt fails.
In `@internal/snapshotoffload/publish.go`:
- Around line 122-130: Update the ObjectStore contract and publish flow so
immutable objects use create-only or conditional PUT semantics instead of
unconditional overwrites. In internal/snapshotoffload/publish.go:122-130, make
manifest creation conditional and, on a conflict, fetch and fully validate the
existing manifest before reusing it; in
internal/snapshotoffload/publish.go:256-264, apply the same create-only behavior
to payloads and re-hash the existing object on conflict for validation. Add a
concurrent publish test covering competing writers.
In `@kv/sharded_coordinator.go`:
- Around line 2416-2441: Update ShardedCoordinator.RecoverHLCLease to attempt
proposal recovery for every target instead of returning immediately on the first
proposeHLCLeaseForGroup failure. Track proposal errors, then recheck the shared
HLC ceiling and return success when any earlier proposal restored it; only
return an error if recovery remains unavailable, preserving useful failure
context. Extend TestShardedCoordinator_RecoverHLCLease_ProposesToEveryLedGroup
in lease_warmup_test.go to cover a later-target failure after an earlier target
succeeds.
---
Nitpick comments:
In `@kv/lease_read_test.go`:
- Around line 191-200: Remove the gosec suppression in applyHLCLeaseEntryToClock
by validating that the decoded ceilingMs is non-negative before calling
clock.SetPhysicalCeiling. Preserve the existing lease-entry length/type checks
and return early for invalid decoded values.
In `@kv/tso.go`:
- Around line 176-210: 共通の「取得→ErrCeilingExpired判定→リース回復→再取得」処理を新しいジェネリック関数
fencedWithRecovery に抽出し、fetch
コールバックで単発取得とバッチ取得を差し替える構成に変更してください。nextFencedWithRecovery は
clock.NextFenced、nextBatchFencedWithRecovery は clock.NextBatchFenced
を渡して共通関数を利用し、既存のエラー処理と戻り値を維持してください。
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05598c9a-d317-4933-a62b-67c2a478b490
📒 Files selected for processing (24)
adapter/redis.goadapter/redis_peer_limiter.goadapter/redis_peer_limiter_test.gocmd/redis-proxy/main_test.godeploy/redis-proxy/docker-compose.ha.ymldocs/design/2026_06_12_proposed_scaling_roadmap.mddocs/design/2026_07_19_partial_physical_snapshot_object_offload.mddocs/redis-proxy-deployment.mdinternal/raftengine/etcd/engine.gointernal/raftengine/etcd/engine_applied_index_test.gointernal/snapshotoffload/manifest.gointernal/snapshotoffload/offload_test.gointernal/snapshotoffload/publish.gointernal/snapshotoffload/restore.gointernal/snapshotoffload/store.gokv/coordinator.gokv/keyviz_label.gokv/lease_read_test.gokv/lease_warmup_test.gokv/sharded_coordinator.gokv/tso.gokv/tso_test.goproxy/backend.goproxy/proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/snapshotoffload/manifest.go
- internal/snapshotoffload/restore.go
- internal/snapshotoffload/store.go
| if exists, err := verifyExistingManifest(ctx, store, manifest, int64(len(data)), objectSHA, reuseExistingCreatedAt); err != nil { | ||
| return err | ||
| } else if exists { | ||
| if manifest.ManifestSHA256 == "" { | ||
| manifest.ManifestSHA256 = manifestSHA | ||
| } | ||
| return nil | ||
| } | ||
| info, err := store.PutObject(ctx, manifest.ManifestKey, bytes.NewReader(data), PutOptions{ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
不変オブジェクトの公開を原子的にしてください。
HeadObject と無条件 PutObject の間で別 publisher が同じキーを書き込めます。特に index/term 由来の manifest key では、異なる ConfState 等の manifest が後続書き込みで上書きされ、公開済みスナップショットの復元契約が変わります。ObjectStore に create-only/条件付き PUT を追加し、既存時は取得・完全検証してから再利用してください。並行 publish の競合テストも追加してください。
internal/snapshotoffload/publish.go#L122-L130: manifest を条件付き作成し、競合時は既存 manifest を検証して一致時のみ再利用する。internal/snapshotoffload/publish.go#L256-L264: payload も同じ create-only 契約で保存し、競合時は既存オブジェクトを再ハッシュして検証する。
📍 Affects 1 file
internal/snapshotoffload/publish.go#L122-L130(this comment)internal/snapshotoffload/publish.go#L256-L264
🤖 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 `@internal/snapshotoffload/publish.go` around lines 122 - 130, Update the
ObjectStore contract and publish flow so immutable objects use create-only or
conditional PUT semantics instead of unconditional overwrites. In
internal/snapshotoffload/publish.go:122-130, make manifest creation conditional
and, on a conflict, fetch and fully validate the existing manifest before
reusing it; in internal/snapshotoffload/publish.go:256-264, apply the same
create-only behavior to payloads and re-hash the existing object on conflict for
validation. Add a concurrent publish test covering competing writers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 958d56da27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if recoverErr := recoverer.RecoverHLCLease(ctx); recoverErr != nil { | ||
| return 0, errors.Wrapf(err, "%s: on-demand HLC lease renewal failed: %v", label, recoverErr) |
There was a problem hiding this comment.
When RecoverHLCLease fails because the caller's context is canceled or its deadline expires while the renewal proposal is waiting, this returns an error wrapping only the original ErrCeilingExpired; the recovery error is formatted with %v and is no longer discoverable via errors.Is. Callers that classify cancellations/deadlines with errors.Is(err, context.Canceled/DeadlineExceeded) will report a timed-out on-demand renewal as a clock-fence failure instead of a client abort/timeout. Please wrap or join recoverErr for context failures while preserving the fail-closed ceiling signal where needed.
Useful? React with 👍 / 👎.
| if !e.snapshotMu.TryLock() { | ||
| return |
There was a problem hiding this comment.
If snapshotMu is held during the first Ready after a received FSM snapshot has been applied, this early return leaves that snapshot index in protectedReceivedFSMSnaps even though it is now safe to release. On an idle follower there may be no later Ready to retry, so future snapshot cleanup treats the old received .fsm as protected and can retain a large snapshot file indefinitely. Please schedule a retry or perform this release from a path that is guaranteed to run after the lock becomes available.
Useful? React with 👍 / 👎.
| for _, target := range targets { | ||
| if err := c.proposeHLCLeaseForGroup(rctx, target.group, ceilingMs); err != nil { | ||
| return errors.Wrapf(err, "recover hlc lease group %d", target.gid) |
There was a problem hiding this comment.
When this node leads multiple shard groups and one earlier target stalls or lacks quorum, the expired-ceiling recovery returns immediately before attempting later groups that may still be accepting writes, so timestamp allocation stays failed for keys routed to otherwise healthy groups until a background renewal eventually succeeds. Please run these recovery proposals independently, or continue after a per-group failure when another target can advance the shared HLC safely.
Useful? React with 👍 / 👎.
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (4d895c1):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@adapter/redis_compat_commands_stream_test.go`:
- Around line 221-222:
テスト内の固定待機でXREADのBLOCK開始を推測せず、readerの待機者登録完了を検知できるテスト用フックまたは同期手段を使ってからrdbWriter.Setを実行してください。これにより、初回XREADが確実に待機状態へ入った後にキーを書き込み、期限直前の最終チェック検証を維持してください。
In `@adapter/redis_stream_cmds.go`:
- Around line 1283-1287: Update the final heavy-pool check in the BLOCK
expiration paths surrounding xreadOnce so slot exhaustion returns false instead
of writing errRedisHeavyCommandPoolFull and returning true. Preserve the
existing NULL response behavior by letting the caller handle the false result.
In `@cmd/elastickv-snapshot-offload/main.go`:
- Around line 121-123: Update both FlagSet parsing paths in the command’s
argument-parsing functions to reject positional arguments: after each successful
fs.Parse call, check fs.NArg() and return an error when it is nonzero. Preserve
the existing wrapped parse-error handling and apply the same validation to both
parser locations.
- Around line 191-193: Update validateStoreFlags to require --s3-sse to equal
aws:kms whenever --s3-kms-key-id is provided, rejecting AES256 or any other
value; preserve the existing missing-SSE validation and add focused parser unit
tests covering valid aws:kms and invalid non-KMS combinations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4338c64-914d-492a-96f6-230b2894a698
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
adapter/redis_compat_commands_stream_test.goadapter/redis_stream_cmds.gocmd/elastickv-snapshot-offload/main.gocmd/elastickv-snapshot-offload/main_test.godocs/design/2026_07_19_partial_physical_snapshot_object_offload.mdgo.modinternal/raftengine/etcd/external_snapshot_restore.gointernal/raftengine/etcd/external_snapshot_restore_test.gointernal/raftengine/etcd/wal_store.gointernal/snapshotoffload/restore.gointernal/snapshotoffload/s3_store.gointernal/snapshotoffload/s3_store_test.gointernal/snapshotoffload/store.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/snapshotoffload/store.go
- internal/snapshotoffload/restore.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d895c1894
ℹ️ 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".
| } | ||
|
|
||
| func (r *RedisServer) xreadFinalCheck(conn redcon.Conn, req xreadRequest) bool { | ||
| ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout) |
There was a problem hiding this comment.
When the BLOCK deadline has already expired, this final check starts a fresh xreadOnce under the full redisDispatchTimeout (10s) and a current read timestamp. For short BLOCKs on a slow backend, or when an XADD lands just after the deadline but before this read observes the stream, the command can wait far beyond the requested BLOCK window or return an entry instead of the timeout null reply. Bound this last read by the original deadline, or skip it once the deadline has passed.
Useful? React with 👍 / 👎.
| if info.Size == opts.Size && (info.SHA256 == "" || info.SHA256 == opts.SHA256) { | ||
| if info.SHA256 == "" { | ||
| info.SHA256 = opts.SHA256 | ||
| } | ||
| return info, nil |
There was a problem hiding this comment.
If the conditional S3 put races with an already-created object that has the same length but no SHA metadata/checksum (for example a manual upload or an S3-compatible service that drops those fields), this branch accepts it as identical and even fills in the expected SHA without reading the object. That can publish a manifest pointing at different bytes under the content-addressed key; the local-store path hashes missing-SHA objects, so the S3 conflict path should also fetch/hash or reject when info.SHA256 is empty.
Useful? React with 👍 / 👎.
| const ( | ||
| redisPerPeerLimitEnv = "ELASTICKV_REDIS_PER_PEER_CONNECTIONS" | ||
| defaultRedisPerPeerConnectionCap = 8 | ||
| defaultRedisPerPeerConnectionCap = 64 |
There was a problem hiding this comment.
With the new proxy default pool size of 64, this cap only covers the normal command pool, but the proxy also opens PubSub/shadow-PubSub connections through NewPubSub as dedicated connections outside that pool. In a default dual-write deployment, a saturated ElasticKV backend pool plus even one subscribe/shadow-subscribe connection from the same proxy host can exceed the server-side cap and start returning ERR max connections per client exceeded; leave headroom for those dedicated connections or lower the proxy default accordingly.
Useful? React with 👍 / 👎.
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (3325028):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33250285fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if info.Size == opts.Size && (info.SHA256 == "" || info.SHA256 == opts.SHA256) { | ||
| if info.SHA256 == "" { | ||
| info.SHA256 = opts.SHA256 | ||
| } | ||
| return info, nil |
There was a problem hiding this comment.
When a conditional S3 PutObject loses to an existing object whose HeadObject response has no SHA metadata/checksum (for example an object uploaded outside this tool, or a racing writer without metadata), this branch accepts any same-sized object and then stamps it with the expected SHA without reading it. That breaks the content-addressed snapshot guarantee: publish can succeed with a manifest pointing at an unverified/corrupt payload, leaving restore to fail later. Please hash the existing object when info.SHA256 is empty, as the preflight verifier does.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func (r *RedisServer) xreadFinalCheck(conn redcon.Conn, req xreadRequest) bool { | ||
| ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout) |
There was a problem hiding this comment.
When the BLOCK deadline has already expired, this final check starts a fresh redisDispatchTimeout context, so a storage/raft read that stalls here can make XREAD BLOCK 1 or other tight timeouts wait up to 10 seconds instead of returning the Redis timeout null. The earlier loop carefully bounded each xreadOnce by the remaining BLOCK budget; the post-deadline wrong-type check should not get a new full dispatch budget.
Useful? React with 👍 / 👎.
| const ( | ||
| defaultPoolSize = 128 | ||
| defaultElasticKVPoolSize = 4 | ||
| defaultElasticKVPoolSize = 64 |
There was a problem hiding this comment.
With this ElasticKV proxy pool default raised to the same 64 connections as the Redis adapter's default per-peer cap, a warmed command pool already consumes the entire server allowance from this proxy host. I checked the bundled go-redis v9.21.0 implementation: NewPubSub uses a separate pubsub pool sized from the same PoolSize, and the proxy opens shadow PubSub connections to the secondary in pubsub modes, so those sessions start getting rejected by ElasticKV unless operators raise ELASTICKV_REDIS_PER_PEER_CONNECTIONS above the documented default. Please either keep client defaults below the server cap or raise/document the server cap with headroom for pubsub connections.
Useful? React with 👍 / 👎.
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (a48f7ac):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/snapshotoffload/publish.go (1)
117-175: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
MarshalCanonical()の失敗にエラー文脈をつけてください。この経路は already
github.com/cockroachdb/errorsを使っていますが、manifest.MarshalCanonical()の失敗をそのまま返すとput snapshot manifestからmarshal snapshot manifestまで失敗原因が埋まりません。errors.Wrap(err, "marshal snapshot manifest")にしてください。🤖 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 `@internal/snapshotoffload/publish.go` around lines 117 - 175, Wrap errors returned by manifest.MarshalCanonical in the publish flow with the context "marshal snapshot manifest" before returning them, using the existing cockroachdb/errors package. Leave the successful serialization and subsequent manifest verification logic unchanged.Source: Coding guidelines
🧹 Nitpick comments (1)
cmd/elastickv-snapshot-offload/main_test.go (1)
74-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueパーサー別ケースをテーブル駆動テストにまとめてください。
publish/restore の入力・期待エラーが同型なので、
run func([]string) errorを持つケーステーブルにすると追加ケースを保守しやすくなります。As per coding guidelines, 「prefer table-driven test cases」に従ってください。
🤖 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 `@cmd/elastickv-snapshot-offload/main_test.go` around lines 74 - 93, TestSnapshotOffloadCLIRejectsPositionalArgsを、publish/restoreそれぞれの引数と期待エラーを持つケーステーブルへ統合してください。各ケースにrun func([]string) errorを定義して対応するparsePublishFlagsまたはparseRestoreFlagsを呼び出し、共通のループでunexpected positional argumentを検証してください。Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@internal/snapshotoffload/publish.go`:
- Around line 117-175: Wrap errors returned by manifest.MarshalCanonical in the
publish flow with the context "marshal snapshot manifest" before returning them,
using the existing cockroachdb/errors package. Leave the successful
serialization and subsequent manifest verification logic unchanged.
---
Nitpick comments:
In `@cmd/elastickv-snapshot-offload/main_test.go`:
- Around line 74-93:
TestSnapshotOffloadCLIRejectsPositionalArgsを、publish/restoreそれぞれの引数と期待エラーを持つケーステーブルへ統合してください。各ケースにrun
func([]string)
errorを定義して対応するparsePublishFlagsまたはparseRestoreFlagsを呼び出し、共通のループでunexpected
positional argumentを検証してください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e92acb11-5786-4c39-a4a5-2b8207a5685e
📒 Files selected for processing (20)
adapter/redis.goadapter/redis_compat_commands_stream_test.goadapter/redis_peer_limiter.goadapter/redis_peer_limiter_test.goadapter/redis_stream_cmds.gocmd/elastickv-snapshot-offload/main.gocmd/elastickv-snapshot-offload/main_test.godocs/redis-proxy-deployment.mdinternal/raftengine/etcd/engine.gointernal/raftengine/etcd/engine_applied_index_test.gointernal/snapshotoffload/offload_test.gointernal/snapshotoffload/publish.gointernal/snapshotoffload/restore.gointernal/snapshotoffload/s3_store.gointernal/snapshotoffload/s3_store_test.gokv/lease_warmup_test.gokv/sharded_coordinator.gokv/tso.gokv/tso_test.goproxy/backend.go
🚧 Files skipped from review as they are similar to previous changes (12)
- proxy/backend.go
- adapter/redis_compat_commands_stream_test.go
- docs/redis-proxy-deployment.md
- kv/tso.go
- adapter/redis.go
- kv/tso_test.go
- internal/snapshotoffload/s3_store_test.go
- kv/sharded_coordinator.go
- cmd/elastickv-snapshot-offload/main.go
- internal/snapshotoffload/offload_test.go
- internal/snapshotoffload/s3_store.go
- internal/snapshotoffload/restore.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a48f7acfe6
ℹ️ 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".
| func (c *ShardedCoordinator) recoverHLCLeaseTargets(ctx context.Context, targets []hlcLeaseRecoveryTarget, ceilingMs int64) error { | ||
| var recoveryErr error | ||
| for _, target := range targets { | ||
| if err := c.proposeHLCLeaseForGroup(ctx, target.group, ceilingMs); err != nil { |
There was a problem hiding this comment.
When a multi-shard node leads multiple timestamp-bridge groups, a quorum-stalled first target can consume the entire shared hlcRecoveryContext budget inside this serial Propose; the subsequent healthy groups are then invoked with an already-canceled context and never get a chance to advance the shared HLC ceiling. Since recovery only needs one target to apply a fresh ceiling, a single bad led shard can keep all timestamp allocations failing with ErrCeilingExpired even though another led shard could have recovered it; give each target its own bounded attempt or run them in parallel.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return ObjectInfo{}, err | ||
| } | ||
| if _, err := s.client.PutObject(ctx, input); err != nil { |
There was a problem hiding this comment.
When publishing real S3-backed physical snapshots larger than 5 GB, this single PutObject call will fail because AWS documents single PUT uploads as capped at 5 GB and requires multipart upload for larger SDK/REST uploads (https://docs.aws.amazon.com/AmazonS3/latest/userguide/upload-objects.html). These snapshots are the disaster-recovery path for large/TB-scale shards, so the S3 store needs a multipart uploader (or an explicit size rejection before manifest publication) rather than relying on one PutObject request.
Useful? React with 👍 / 👎.
| if info.Size != opts.Size || (info.SHA256 != "" && info.SHA256 != opts.SHA256) { | ||
| return ObjectInfo{}, errors.Wrapf(ErrIntegrity, "s3 object %s remote integrity mismatch", key) | ||
| } | ||
| if info.SHA256 == "" { |
There was a problem hiding this comment.
When checksum headers are disabled or a compatible store does not return ChecksumSHA256, HeadObject still supplies the user metadata written by this client, so info.SHA256 is non-empty and this branch skips verifyS3ObjectBytes. That lets a corrupted or pre-existing object with matching size and elastickv-sha256 metadata be accepted and later referenced by a committed manifest, with the corruption only discovered during restore; distinguish server-verified checksums from metadata, or hash the object before reusing/committing when only metadata is present.
Useful? React with 👍 / 👎.
1 participant
Summary
Tests
Summary by CodeRabbit