Complete raft envelope cutover runtime - #1019
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthrough暗号化DEKのraftエンベロープcutover安全化（バリア、非cutover管理提案の遮断、post-cutoverプロポーザ経路）、起動時DEK自動ローテーション、公開ゲート/HLCリース抑制による起動シーケンス制御、gRPC Unavailableの503マッピング、および外部サーバ起動のprepare/bind/run分割を実装。 ChangesRaft-envelope暗号化とcutover安全化
TLA jar SHA-256検証値の更新
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant Main as main.go
participant RotateTask as encryptionRotateOnStartupTask
participant Admin as EncryptionAdminServer
participant Gate as startupPublicKVGate
Main->>RotateTask: installEncryptionRotateOnStartup
RotateTask->>Admin: RotateDEK(storage)
Admin-->>RotateTask: 提案結果
RotateTask->>Admin: RotateDEK(raft, raftEnvelope経由)
Admin-->>RotateTask: 提案結果
RotateTask-->>Main: 完了通知
Main->>Gate: markReady + rotation完了確認
Gate-->>Main: RawKV/TransactionalKV等の公開解放
sequenceDiagram
participant Client as gRPC Client
participant Admin as EncryptionAdminServer
participant Barrier as raftEnvelopeCutoverBarrier
participant ShardGroup as kv.ShardGroup
Client->>Admin: EnableRaftEnvelope
Admin->>Barrier: ValidateCutoverScope
Admin->>Barrier: Begin
Barrier->>ShardGroup: BeginCutoverBarrier
Admin->>Barrier: WaitDrained
Barrier->>ShardGroup: WaitInflightDrained
Admin->>Barrier: InstallWrap
Admin->>Barrier: End
Barrier->>ShardGroup: EndCutoverBarrier
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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://cdn.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Stage 6E of the data-at-rest encryption design, enabling the Phase-2 raft envelope cutover. It introduces a raftEnvelopeRuntime to coordinate wrap publication across shard groups, routes post-cutover admin mutators through a wrap-aware proposer, and adds several fail-closed startup and API guards. The review feedback highlights a potential deadlock risk in the cutover barrier if a channel is nil, and points out multiple redundant nil checks that can be removed as dead defensive code.
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.
| if r == nil { | ||
| return errors.New("raft envelope runtime: installer is not configured") | ||
| } |
There was a problem hiding this comment.
This nil check on the receiver r is dead defensive code. installFromApply is only registered as a callback in main_encryption_write_wiring.go when w.raftEnvelope is guaranteed to be non-nil. Removing this check keeps the codebase clean and adheres to the repository guidelines against unreachable nil guards.
References
- Avoid adding dead defensive code, such as unreachable nil guards, if upstream checks or callers already guarantee that the variables are non-nil. This keeps the codebase clean and complies with repository guidelines or lints that discourage dead defensive code.
| if r == nil || g == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
This nil check is dead defensive code. attachGroup is only called from attachRaftEnvelopeGroup after verifying that w.raftEnvelope is non-nil, and g is guaranteed to be non-nil as it is built and registered in buildShardGroups. Removing this check keeps the codebase clean and complies with the repository guidelines.
References
- Avoid adding dead defensive code, such as unreachable nil guards, if upstream checks or callers already guarantee that the variables are non-nil. This keeps the codebase clean and complies with repository guidelines or lints that discourage dead defensive code.
| if r == nil || r.cutoverIndex == nil { | ||
| return inertRaftEnvelopeCutoverIndex | ||
| } |
There was a problem hiding this comment.
The receiver r is guaranteed to be non-nil because engineCutoverIndex is only passed as a callback in configureRaftEnvelopeFactory after checking w.raftEnvelope == nil. Additionally, cutoverIndex is guaranteed to be non-nil as newRaftEnvelopeRuntime returns an error if the cell is nil. This nil check is therefore dead defensive code and can be safely removed.
References
- Avoid adding dead defensive code, such as unreachable nil guards, if upstream checks or callers already guarantee that the variables are non-nil. This keeps the codebase clean and complies with repository guidelines or lints that discourage dead defensive code.
| if r == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The receiver r is guaranteed to be non-nil because snapshotGroups is only called on b.runtime, which is initialized from a non-nil receiver in barrier(). This nil check is dead defensive code and can be safely removed.
References
- Avoid adding dead defensive code, such as unreachable nil guards, if upstream checks or callers already guarantee that the variables are non-nil. This keeps the codebase clean and complies with repository guidelines or lints that discourage dead defensive code.
| if r == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
The receiver r is guaranteed to be non-nil because reinstallActiveWrap is only called on b.runtime, which is initialized from a non-nil receiver in barrier(). This nil check is dead defensive code and can be safely removed.
References
- Avoid adding dead defensive code, such as unreachable nil guards, if upstream checks or callers already guarantee that the variables are non-nil. This keeps the codebase clean and complies with repository guidelines or lints that discourage dead defensive code.
|
@codex review |
![chatgpt-codex-connector[bot]](https://cdn.github.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: 01c5fc784e
ℹ️ 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".
| g.SetRaftPayloadWrap(wrap) | ||
| } | ||
| r.mu.Unlock() | ||
| r.cutoverIndex.Store(cutoverIdx) |
There was a problem hiding this comment.
This stores one cutover index in the shared runtime that configureRaftEnvelopeFactory installs into every shard-group engine. In a multi-raft process the groups have independent log indexes; after the marker commits in one group at index N, another group can emit wrapped entries at local indexes <= N, which its apply hook will not unwrap, or can have cleartext entries above N treated as envelopes. The cutover boundary needs to be per group, or the RPC should refuse multi-group deployments until each group has its own marker.
Useful? React with 👍 / 👎.
| var raftEnvelopeWrapEnabled atomic.Bool | ||
|
|
||
| func init() { | ||
| raftEnvelopeWrapEnabled.Store(true) |
There was a problem hiding this comment.
With this gate opened, every normal entry above the cutover must be wrapped, but the leader-local writer registration path still bypasses the wrapper: proposeWriterRegistration calls defaultEngine.ProposeAdmin directly when coordinate.IsLeader(). On a restarted post-cutover leader that needs to register its bumped storage epoch, or on a runtime registration trigger, this appends a cleartext OpRegistration at index > cutover, so the apply hook tries to unwrap it and halts; route the local branch through the wrap-aware ShardGroup.Proposer()/admin path before enabling fresh cutovers.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Lines 893 to 896 in 1422878
Wire the raft cutover source into FSM snapshots
After EnableRaftEnvelope, kvFSM.Snapshot only emits the v2 header when WithCutoverSource supplies a non-zero RaftEnvelopeCutoverIndex, but this constructor still omits that option. Consequently every snapshot taken after the new raft-envelope cutover remains v1 with cutover 0, so a later restore has no snapshot-side boundary for the startup divergence guard to compare against and sidecar/snapshot mismatches can slip through. Pass a raw cutover source from the raft-envelope runtime here (not the engine's inert-sentinel callback) so post-cutover snapshots preserve the boundary.
ℹ️ 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".
| entry := registrationEntry(w.activeRaftDEKID, fullNodeID, w.raftEpoch) | ||
| req := registrationRequest(w.activeRaftDEKID, fullNodeID, w.raftEpoch) |
There was a problem hiding this comment.
This uses w.activeRaftDEKID, which was captured from the sidecar before buildShardGroups opened Raft and replayed any committed bootstrap/raft-rotation entries. On a restart after a crash where WAL replay advances Active.Raft (including the sidecar-missing bootstrap+cutover case), this registers the old DEK or skips registration entirely when the pre-replay value was 0, while the applier may already have installed wrapping for the current raft DEK. Re-read the sidecar/current active raft DEK after replay before deciding what process-start raft writer registration to commit.
Useful? React with 👍 / 👎.
| if err != nil || sc == nil || sc.RaftEnvelopeCutoverIndex == 0 { | ||
| return err |
There was a problem hiding this comment.
This early return skips the pre-nonce-bump snapshot check whenever the sidecar cutover is 0, but a restored post-cutover snapshot can be exactly the divergent case snapshotCutover != 0 && sidecarCutover == 0. The later runtime guard will catch it only after buildEncryptionWriteWiring has already bumped storage/raft local epochs, so a restart loop on this mismatch mutates the sidecar before refusing. Still inspect the newest snapshot when the sidecar exists, and refuse before any epoch bump if it carries a non-zero cutover that differs from the sidecar.
Useful? React with 👍 / 👎.
| if err := installProcessStartRaftRegistration(runCtx, coordinate, defaultGroup, w, raftID); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
This synchronous raft-writer registration runs before startServers registers the raft/admin gRPC listeners. In a multi-node cluster restart after raft-envelope cutover, every node bumps w.raftEpoch and reaches this call before any peer is serving Raft transport or EncryptionAdmin, so no leader/quorum can be reached and the nodes time out and exit instead of forming the cluster. Defer this registration until after the raft listeners are up, or run it asynchronously behind a proposal gate like the storage registration path.
Useful? React with 👍 / 👎.
| // — is the genuine case-4 halt-apply residual; see §3.3 of the | ||
| // design doc.) | ||
| return proposeWriterRegistration(ctx, e.coordinate, e.defaultGroup.Engine, connCache, entry, req) | ||
| return proposeWriterRegistration(ctx, e.coordinate, e.defaultGroup.Proposer(), connCache, entry, req) |
There was a problem hiding this comment.
After raft-envelope cutover, this membership pre-register flow still proposes only the storage activeDEK row built above, then commits the conf-change. A newly added member that later becomes a raft leader will wrap proposals with the active raft DEK, but no (active_raft_dek, newNodeFullID) registry row was inserted for it, so the raft nonce monotonicity guard has no anchor for that member. When RaftEnvelopeCutoverIndex is active, pre-register the active raft DEK as well before allowing the member change.
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: ed695fe580
ℹ️ 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 err != nil { | ||
| return nil, nil, encryptionWriteWiring{}, err | ||
| } | ||
| if err := validateRaftEnvelopeStartupScope(encWiring, groups); err != nil { |
There was a problem hiding this comment.
When a sidecar already has RaftEnvelopeCutoverIndex != 0 and the config has multiple shard groups, this validation returns an error, but it runs after buildEncryptionWriteWiring has already called prepareStorageNonceEpoch and prepareRaftNonceEpoch, both of which durably bump local epochs in the sidecar. In an orchestrator restart loop this refusal path consumes a storage/raft epoch on every failed boot and can exhaust the 16-bit epoch space before the operator fixes the group config; the active-cutover/multi-group check needs to happen before any nonce epoch bump or be made read-only.
Useful? React with 👍 / 👎.
| if err != nil || sc == nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
If the sidecar file is absent, this returns nil before opening the newest FSM snapshot, so a data directory whose v2 snapshot header records a non-zero raft-envelope cutover can boot as if no cutover exists. In that lost/restored-sidecar scenario the engine is configured with an inert cutover index, so committed post-cutover log entries after the snapshot are not unwrapped (or fresh proposals can be admitted cleartext above the snapshot's cutover), defeating the divergence guard this path is meant to enforce. Missing sidecar should fail closed when a restored snapshot may carry a cutover, or the snapshot should still be inspected before skipping.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return w, err | ||
| } | ||
| raftEpoch, err := prepareRaftNonceEpoch(sidecarPath, kekWrapper, keystore) |
There was a problem hiding this comment.
On any bootstrapped encryption-enabled node, this bumps the active raft DEK's LocalEpoch even when RaftEnvelopeCutoverIndex is still zero and the node cannot emit raft-envelope nonces yet. Clusters that restart repeatedly before enabling the raft envelope will consume the finite 16-bit raft epoch space for no nonce-safety benefit and can hit ErrLocalEpochExhausted before the feature is ever cut over. The raft epoch bump should be gated to active raft-envelope use, or deferred until enabling/installing the raft wrapper.
Useful? React with 👍 / 👎.
| r.cutoverIndex.Store(cutoverIdx) | ||
| r.mu.Lock() | ||
| r.activeWrap = wrap | ||
| for _, g := range r.groups { | ||
| g.SetRaftPayloadWrap(wrap) |
There was a problem hiding this comment.
This stores the cutover index before the dynamic proposer wrap is installed on the shard groups. If a follower becomes leader while applying the cutover, or any local proposer races this apply hook, it can observe the still-nil wrap and append a cleartext entry at an index greater than the cutover; once this store has made the strict > unwrap hook active, that entry is treated as an envelope and halts apply. The wrap pointer needs to be published before the cutover index is exposed, or proposals must be blocked until both are installed.
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: a51658b88e
ℹ️ 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".
| // — is the genuine case-4 halt-apply residual; see §3.3 of the | ||
| // design doc.) | ||
| return proposeWriterRegistration(ctx, e.coordinate, e.defaultGroup.Engine, connCache, entry, req) | ||
| return proposeWriterRegistration(ctx, e.coordinate, e.defaultGroup.Proposer(), connCache, entry, req) |
There was a problem hiding this comment.
When an AddVoter/AddLearner pre-register overlaps EnableRaftEnvelope after the cutover marker has been submitted but before the wrap installer publishes, this path calls ShardGroup.Proposer().ProposeAdmin via proposeWriterRegistration. ProposeAdmin is barrier-exempt and, while currentWrap() is still nil, appends the registration entry in cleartext; if it lands at index > raft_envelope_cutover_index, the etcd apply hook will try to unwrap that cleartext entry and halt apply cluster-wide. The pre-register path needs to be serialized with the cutover or otherwise wait/retry until the raft wrap is installed before allowing this admin proposal.
Useful? React with 👍 / 👎.
| if sc.RaftEnvelopeCutoverIndex == 0 || sc.Active.Raft == 0 { | ||
| return 0, false, nil | ||
| } |
There was a problem hiding this comment.
When a node is added after bootstrap but before EnableRaftEnvelope, this guard skips registering the new member for the active raft DEK, even though bootstrap now creates raft registry rows for the initial cohort and the cutover RPC does not batch-register later members. If that new member becomes leader immediately after the raft cutover, its wrap gate has no committed local raft writer row to mark and user proposals fail closed until the async runtime watcher notices and registers; the 7c pre-register step should cover the active raft DEK whenever one exists, not only after the cutover is already active.
Useful? React with 👍 / 👎.
| return nil, errors.New("raft envelope runtime: raft nonce factory is not configured") | ||
| } | ||
| return func(payload []byte) ([]byte, error) { | ||
| if r.registration != nil && !r.registration.Registered(activeRaftDEKID, r.raftEpoch) && !isRaftWriterRegistrationPayload(payload) { |
There was a problem hiding this comment.
Immediately after a fresh raft cutover (and similarly after a raft-DEK rotation), installFromApply publishes a wrap that consults this in-memory gate, but nothing marks the gate for nodes whose writer row is already committed by bootstrap or by the cutover entry itself. Until the 1s runtime watcher happens to re-read the registry and call MarkRegistered, every non-registration proposal on the just-unblocked leader fails with ErrWriterNotRegistered; the apply/cutover path should synchronously seed the gate from the committed row before allowing normal wrapped proposals.
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: 5c9bd2a0b9
ℹ️ 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".
| w.raftNonceFactory = encryption.NewDeterministicNonceFactory(nodeID, raftEpoch) | ||
| w.raftCutoverIndex = &atomic.Uint64{} | ||
| w.raftRegistration = &raftRegistrationGate{} | ||
| runtime, err := newRaftEnvelopeRuntime(cipher, w.raftNonceFactory, cutoverIdx, activeRaftDEKID, w.raftCutoverIndex, w.raftEpoch, w.raftRegistration) |
There was a problem hiding this comment.
When a restored or hand-edited sidecar has RaftEnvelopeCutoverIndex != 0 but Active.Raft == 0, this call returns active cutover requires active raft DEK only after prepareStorageNonceEpoch has already fsynced a bump of the active storage DEK. Startup then aborts before any nonce can be emitted, so a supervisor restart loop burns one storage local_epoch per attempt and can eventually hit ErrLocalEpochExhausted. This invariant should be checked before the storage epoch bump, alongside the other pre-bump cutover validations.
Useful? React with 👍 / 👎.
| var raftEnvelopeWrapEnabled atomic.Bool | ||
|
|
||
| func init() { | ||
| raftEnvelopeWrapEnabled.Store(true) |
There was a problem hiding this comment.
With this default enabled, a rolling upgrade can cut over while some voters or learners still run the previous binary: those peers still report EncryptionCapable=true whenever the sidecar path is configured, and the existing fan-out OK predicate only requires reachable + encryption-capable, not the applier installer/factory wiring added here. The cutover can therefore commit before every replica can unwrap or install the raft envelope, so the first wrapped entry on an old peer, or a later failover to one, can halt apply. Keep this gate closed until the fan-out verifies a 6E-2f-capable feature/build.
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: 7f3d3b8b5c
ℹ️ 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".
| var raftEnvelopeWrapEnabled atomic.Bool | ||
|
|
||
| func init() { | ||
| raftEnvelopeWrapEnabled.Store(false) |
There was a problem hiding this comment.
This still forces the production gate closed, and I found no non-test path that ever stores true into raftEnvelopeWrapEnabled. As a result, any fresh EnableRaftEnvelope call always takes the !raftEnvelopeWrapEnabled.Load() branch and returns FailedPrecondition before fan-out or proposal, so the raft-envelope cutover shipped by this change cannot actually be enabled outside tests.
Useful? React with 👍 / 👎.
| // remains the wrap-aware ShardGroup.Proposer() in production, | ||
| // so post-cutover local registrations still carry the §4.2 | ||
| // raft envelope required by the strict-`>` apply hook. | ||
| if _, err := defaultProposer.ProposeAdmin(attemptCtx, entry); err != nil { |
There was a problem hiding this comment.
When this node is leader, the background/process-start registration path now uses the barrier-exempt ProposeAdmin directly. During EnableRaftEnvelope, there is a window after the raw cutover marker is proposed but before InstallWrap() publishes the wrapper; a concurrent local writer-registration retry in that window can therefore commit cleartext at an index greater than the cutover, and the strict > apply hook will try to unwrap that cleartext entry and halt apply. Route this path through the barriered proposer or serialize it with the cutover semaphore until the wrap is installed.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Rechecked the latest head against the raft-envelope cutover findings. The branch already includes the applied-index callback, startup multi-group cutover refusal, pre-bump divergence guard, serialized non-cutover admin proposals, raft wrap reinstall after raft DEK rotation, and the raft registration gate/wrap checks. Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0d68f022
ℹ️ 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".
| // remains the wrap-aware ShardGroup.Proposer() in production, | ||
| // so post-cutover local registrations still carry the §4.2 | ||
| // raft envelope required by the strict-`>` apply hook. | ||
| if _, err := defaultProposer.ProposeAdmin(attemptCtx, entry); err != nil { |
There was a problem hiding this comment.
When this helper is reached from the runtime storage-registration watcher on the current leader, it bypasses the cutover barrier via ProposeAdmin. During EnableRaftEnvelope, there is a window after the cleartext cutover marker is proposed but before the apply hook publishes the wrap; a local storage registration proposed here can still see currentWrap()==nil and be assigned an index greater than the cutover marker. The strict entry.Index > cutover apply hook will then try to unwrap this cleartext OpRegistration and halt apply, so local runtime registrations need to use the barrier-blocking path or be serialized with the raft cutover semaphore.
Useful? React with 👍 / 👎.
| return nil, errors.New("raft envelope runtime: raft nonce factory is not configured") | ||
| } | ||
| return func(payload []byte) ([]byte, error) { | ||
| if r.registration != nil && !r.registration.Registered(activeRaftDEKID, r.raftEpoch) && !isRaftWriterRegistrationPayload(payload) { |
There was a problem hiding this comment.
When the raft envelope is active but this process has not yet committed its raft-DEK writer row (for example immediately after a restart or raft-DEK rotation), this condition exempts every OpRegistration from the raft registration gate. Storage-DEK registrations from the process-start/runtime storage registration paths also start with OpRegistration, so they can be wrapped and proposed under the current raft DEK before (activeRaftDEKID, raftEpoch) is durably registered; a later stale sidecar restore would then have no registry last_seen record for an epoch that already emitted raft envelopes. Decode the registration and exempt only rows for the active raft DEK.
Useful? React with 👍 / 👎.
|
Fixed the latest raft registration/cutover safety findings. Changes:
Caller audit:
Validation:
Note: full go test ./... -count=1 -timeout=600s timed out in the existing adapter integration package after root/internal/encryption/kv and other packages passed. @codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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 (fd378bb):
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. |
|
Merged latest main and remote raft-envelope branch updates into this PR and resolved the remaining conflict. The Validation:
|
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
main_raft_envelope_wiring.go (1)
116-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
installFromApplyとinstallRotatedRaftDEKはほぼ同一ロジックで重複しています。登録検証（registered読み出し→MarkRegistered）と wrap 配布（activeWrap/activeRaftDEKID更新＋全グループへ SetRaftPayloadWrap）が両メソッドで重複しています。差分は
cutoverIndexの Store 有無と入口ガードのみです。共通の内部ヘルパー（例:publishWrap(activeRaftDEKID)）へ抽出すると保守性が向上します。🤖 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 `@main_raft_envelope_wiring.go` around lines 116 - 175, Both raft DEK install paths duplicate the same registration and wrap-publishing logic. Extract the shared work in installFromApply and installRotatedRaftDEK into a common internal helper (for example, a publishWrap-style method on raftEnvelopeRuntime) that handles wrapFor, registered/MarkRegistered, activeWrap/activeRaftDEKID updates, and SetRaftPayloadWrap on all groups. Keep the differing behavior only in the callers: installFromApply should still validate cutoverIdx and store cutoverIndex, while installRotatedRaftDEK should keep its cutoverIndex guards and return early as today.internal/encryption/applier_test.go (1)
2641-2678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win両テストは機能的に重複しています — テーブル駆動へ統合を推奨。
TestApplyRotation_RaftRotateDEK_CutoverActiveRepublishesInstaller（2641–2678）とTestApplyRotation_RaftDEKRotationAfterCutover_ReinstallsWrapper（2805–2840）は、cutover → raft DEK 回転 → インストーラ呼び出し回数・引数を検証する同一の動作をテストしています。定数のみが異なります（cutoverIdx 600 vs 500、回転 index 601 vs 600）。コーディングガイドラインの「prefer table-driven test cases」に沿って、1つのテーブル駆動テストへ統合することを推奨します。As per coding guidelines,
**/*_test.go: prefer table-driven test cases.♻️ 統合案
+func TestApplyRotation_RaftRotateDEK_CutoverActiveRepublishesInstaller(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + cutoverIdx uint64 + rotateIdx uint64 + }{ + {"idx_600", 600, 601}, + {"idx_500", 500, 600}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, sidecarPath := bootstrappedDir(t) + app, rec := newCutoverApplierWithInstaller(t, sidecarPath) + if err := app.ApplyRotation(tc.cutoverIdx, fsmwire.RotationPayload{ + SubTag: fsmwire.RotateSubEnableRaftEnvelope, + DEKID: cutoverBootstrapRaftDEKID, + Purpose: fsmwire.PurposeRaft, + Wrapped: []byte{}, + ProposerRegistration: fsmwire.RegistrationPayload{ + DEKID: cutoverBootstrapRaftDEKID, FullNodeID: 0xBBBB, LocalEpoch: 1, + }, + }); err != nil { + t.Fatalf("ApplyRotation raft-cutover: %v", err) + } + const newRaftDEKID uint32 = 99 + if err := app.ApplyRotation(tc.rotateIdx, fsmwire.RotationPayload{ + SubTag: fsmwire.RotateSubRotateDEK, + DEKID: newRaftDEKID, + Purpose: fsmwire.PurposeRaft, + Wrapped: []byte("new-raft-dek"), + ProposerRegistration: fsmwire.RegistrationPayload{DEKID: newRaftDEKID, FullNodeID: 0xBBBB, LocalEpoch: 2}, + }); err != nil { + t.Fatalf("ApplyRotation raft RotateDEK: %v", err) + } + if got, want := len(rec.calls), 2; got != want { + t.Fatalf("installer called %d times, want %d (cutover + raft RotateDEK republish)", got, want) + } + last := rec.calls[len(rec.calls)-1] + if last.cutoverIdx != tc.cutoverIdx { + t.Errorf("installer cutoverIdx after RotateDEK = %d, want %d", last.cutoverIdx, tc.cutoverIdx) + } + if last.activeRaftDEKID != newRaftDEKID { + t.Errorf("installer activeRaftDEKID after RotateDEK = %d, want %d", last.activeRaftDEKID, newRaftDEKID) + } + }) + } +}Also applies to: 2805-2840
🤖 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/encryption/applier_test.go` around lines 2641 - 2678, `TestApplyRotation_RaftRotateDEK_CutoverActiveRepublishesInstaller` and `TestApplyRotation_RaftDEKRotationAfterCutover_ReinstallsWrapper` are duplicate coverage for the same cutover-then-rotate behavior in applier_test.go. Consolidate them into one table-driven test around `ApplyRotation`/`newCutoverApplierWithInstaller` that parameterizes the cutover index, rotate index, and expected installer arguments while keeping the assertions on call count, `cutoverIdx`, and `activeRaftDEKID`.Source: Coding guidelines
main_encryption_registration.go (1)
737-754: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
proposeWriterRegistrationBlockingOnCutoverはproposeWriterRegistrationと実装が完全に同一です。両関数ともリーダ時に
defaultProposer.Propose（バリア対応）を用い、フォロワー時にforwardWriterRegistrationToLeaderへフォワードしており、本体が一字一句同一です。名前のBlockingOnCutoverが挙動差を示唆しますが実際には差がなく、追加テスト（TestProposeWriterRegistration_LeaderUsesBarrieredProposeとTestProposeWriterRegistrationBlockingOnCutover_LeaderUsesPropose）も同一挙動を確認しています。将来の乖離リスクと誤解を避けるため、一方を削除して呼び出し側を集約するか、片方をエイリアスにすることを検討してください。♻️ 集約案
-func proposeWriterRegistrationBlockingOnCutover( - ctx context.Context, - coordinate *kv.ShardedCoordinator, - defaultProposer raftengine.Proposer, - connCache *kv.GRPCConnCache, - entry []byte, - req *pb.RegisterEncryptionWriterRequest, -) error { - attemptCtx, cancel := context.WithTimeout(ctx, registrationAttemptTimeout) - defer cancel() - if coordinate.IsLeader() { - if _, err := defaultProposer.Propose(attemptCtx, entry); err != nil { - return errors.Wrap(err, "writer registration: local propose") - } - return nil - } - return forwardWriterRegistrationToLeader(attemptCtx, coordinate, connCache, req) -}呼び出し側（confchange の
PreAddMember、runtimeRaftRegistrationTick）はproposeWriterRegistrationを直接渡すよう変更。🤖 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 `@main_encryption_registration.go` around lines 737 - 754, `proposeWriterRegistrationBlockingOnCutover` and `proposeWriterRegistration` are identical, so keep only one implementation and make the other a thin alias or remove it entirely. Update the call sites that currently reference `proposeWriterRegistrationBlockingOnCutover` (for example in `PreAddMember` and `runtimeRaftRegistrationTick`) to use `proposeWriterRegistration` directly, and keep the leader/follower flow centralized around `defaultProposer.Propose` and `forwardWriterRegistrationToLeader`.main.go (1)
1496-1554: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
//nolint:wrapcheckの代替として.golangci.yaml設定を検討新規追加コードに 9 件の
//nolint:wrapcheckがあります。コーディングガイドラインでは「//nolintディレクティブの使用を避け、リファクタリングを推奨」としています。status.Error(codes.Unavailable, ...)のラップ回避は 503 マッピングのために絶対に必要ですが、パススルーメソッド（IsLeader,VerifyLeader,LinearizableRead等）の//nolint:wrapcheckは.golangci.yamlの wrapcheck 除外設定で代替でき、インライン//nolintを削減できます。As per coding guidelines: "Avoid using
//nolintdirectives; refactor instead in Go code"♻️ Proposed: .golangci.yaml で wrapcheck 除外を設定
# .golangci.yaml の wrapcheck 設定に除外を追加 linters-settings: wrapcheck: ignoreSigs: - "kv.LeaseReadThrough(" - "kv.LeaseReadForKeyThrough(" - "kv.LeaseReadAllGroupsThrough(" # または、パッケージ全体を除外 ignorePackageGlobs: - "elastickv/internal/*"これにより、パススルーメソッドから
//nolint:wrapcheckコメントを削除できます。status.Errorを返す Dispatch / unaryInterceptor の//nolint:wrapcheckは gRPC status 保持のため残す必要があります。Also applies to: 1575-1587
🤖 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 `@main.go` around lines 1496 - 1554, Remove the inline wrapcheck suppressions from the pass-through methods in startupGatedCoordinator by moving those exceptions into the golangci wrapcheck configuration instead. Keep the necessary gRPC status preservation in Dispatch, but delete the unnecessary //nolint:wrapcheck comments from methods like IsLeader, VerifyLeader, LinearizableRead, LeaseRead, LeaseReadForKey, LeaseReadAllGroups, and the related pass-through helpers; use .golangci.yaml ignoreSigs or package-level exclusions to whitelist the coordinator passthrough wrappers.Source: Coding guidelines
kv/sharded_coordinator.go (1)
357-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShardedCoordinator の HLC リース抑制にもテストを追加してください
RunHLCLeaseRenewalの blocker 分岐に対応するテストが見当たりません。Coordinate側と同様に、ブロッカー有効時は提案が止まり、解除後に再開することをkv/lease_warmup_test.goで押さえておくと回帰を防げます。🤖 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/sharded_coordinator.go` around lines 357 - 359, `RunHLCLeaseRenewal` の blocker 分岐をカバーするテストが不足しています。`shardedCoordinator` の `hlcRenewalBlocked` と `RunHLCLeaseRenewal` を使い、ブロッカー有効時は HLC リース提案が発生しないこと、解除後に再開することを `lease_warmup_test` 側で検証するテストを追加してください。`Coordinate` 側の既存テストパターンに合わせて、ブロッカーの切り替えで提案有無が変わることを明確に押さえてください。Source: Coding guidelines
main_admin.go (1)
1019-1089: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueprepare/bind/run分割とライフサイクル管理は適切に実装されている。
start()呼出後にlistenerをnil化し、close()をno-opにする設計は正しい。close()はstart()未呼出時のクリーンアップパスとして機能する。checkAdminConfigがadmin無効時にfalse, nilを返すため、if err != nil || !enabledの分岐も安全である。
close()でlistener.Close()のエラーを_ =で無視しているが、main_sqs.goのcloseSQSListenerOnErrorはslog.Warnでログ出力している。クリーンアップパスでの観測性の一貫性を高めるため、エラーをログ出力することを検討されたい。♻️ 提案: close()でのエラーログ出力
func (p *preparedAdminServer) close() { if p == nil { return } if p.listener != nil { - _ = p.listener.Close() + if err := p.listener.Close(); err != nil { + slog.Warn("admin listener close on startup failure", "err", err) + } p.listener = 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 `@main_admin.go` around lines 1019 - 1089, The close path in preparedAdminServer.close currently discards listener.Close errors, unlike the SQS cleanup flow which logs them. Update preparedAdminServer.close to capture and emit a warning when listener.Close fails, using the existing preparedAdminServer and listener cleanup logic so shutdown failures are observable during the non-started cleanup path.main_s3.go (1)
23-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
runS3ServerとrunSQSServerの二ーゴルーチンシャットダウンパターンが重複している。両関数は同一の
runDoneCtx/runDoneCancelパターン（ctx.Done→Stop、Run完了→cancel、net.ErrClosedをnil扱い）を持つ。Run() errorとStop()を持つインターフェースを導入し、共通ヘルパーに抽出することで重複を解消できる。♻️ 提案: 共通runServerヘルパーの抽出
+type runnableServer interface { + Run() error + Stop() +} + +func runServer(ctx context.Context, eg *errgroup.Group, s runnableServer) { + if s == nil { + return + } + runDoneCtx, runDoneCancel := context.WithCancel(context.Background()) + eg.Go(func() error { + select { + case <-ctx.Done(): + s.Stop() + case <-runDoneCtx.Done(): + } + return nil + }) + eg.Go(func() error { + err := s.Run() + runDoneCancel() + if err == nil || errors.Is(err, net.ErrClosed) { + return nil + } + return errors.WithStack(err) + }) +}これにより
runS3ServerとrunSQSServerは以下のように簡素化できる:-func runS3Server(ctx context.Context, eg *errgroup.Group, s3Server *adapter.S3Server) { - if s3Server == nil { - return - } - runDoneCtx, runDoneCancel := context.WithCancel(context.Background()) - eg.Go(func() error { - select { - case <-ctx.Done(): - s3Server.Stop() - case <-runDoneCtx.Done(): - } - return nil - }) - eg.Go(func() error { - err := s3Server.Run() - runDoneCancel() - if err == nil || errors.Is(err, net.ErrClosed) { - return nil - } - return errors.WithStack(err) - }) -} +func runS3Server(ctx context.Context, eg *errgroup.Group, s3Server *adapter.S3Server) { + runServer(ctx, eg, s3Server) +}🤖 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 `@main_s3.go` around lines 23 - 139, `runS3Server` and `runSQSServer` duplicate the same two-goroutine shutdown logic with `runDoneCtx`, `runDoneCancel`, `ctx.Done() -> Stop()`, and `net.ErrClosed` handling. Extract this pattern into a shared helper that accepts a small interface exposing `Run() error` and `Stop()`, then have both `runS3Server` and `runSQSServer` delegate to it. Keep the existing `Run` completion behavior, cancel flow, and `net.ErrClosed` treatment unchanged while removing the duplicated logic.
🤖 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 `@main_raft_envelope_wiring_test.go`:
- Around line 124-132: `newRaftEnvelopeRuntime` is being called with one
argument too few, causing the test to fail to compile because `fullNodeID` is
missing. Update the call in
`TestRaftEnvelopeRuntime_InstallRotatedRaftDEKRepublishesWrap` to pass a valid
`fullNodeID` after `raftEpoch`, and keep the trailing `nil` for `registration`;
use the `newRaftEnvelopeRuntime` signature to align the argument order
correctly.
---
Nitpick comments:
In `@internal/encryption/applier_test.go`:
- Around line 2641-2678:
`TestApplyRotation_RaftRotateDEK_CutoverActiveRepublishesInstaller` and
`TestApplyRotation_RaftDEKRotationAfterCutover_ReinstallsWrapper` are duplicate
coverage for the same cutover-then-rotate behavior in applier_test.go.
Consolidate them into one table-driven test around
`ApplyRotation`/`newCutoverApplierWithInstaller` that parameterizes the cutover
index, rotate index, and expected installer arguments while keeping the
assertions on call count, `cutoverIdx`, and `activeRaftDEKID`.
In `@kv/sharded_coordinator.go`:
- Around line 357-359: `RunHLCLeaseRenewal` の blocker
分岐をカバーするテストが不足しています。`shardedCoordinator` の `hlcRenewalBlocked` と
`RunHLCLeaseRenewal` を使い、ブロッカー有効時は HLC リース提案が発生しないこと、解除後に再開することを
`lease_warmup_test` 側で検証するテストを追加してください。`Coordinate`
側の既存テストパターンに合わせて、ブロッカーの切り替えで提案有無が変わることを明確に押さえてください。
In `@main_admin.go`:
- Around line 1019-1089: The close path in preparedAdminServer.close currently
discards listener.Close errors, unlike the SQS cleanup flow which logs them.
Update preparedAdminServer.close to capture and emit a warning when
listener.Close fails, using the existing preparedAdminServer and listener
cleanup logic so shutdown failures are observable during the non-started cleanup
path.
In `@main_encryption_registration.go`:
- Around line 737-754: `proposeWriterRegistrationBlockingOnCutover` and
`proposeWriterRegistration` are identical, so keep only one implementation and
make the other a thin alias or remove it entirely. Update the call sites that
currently reference `proposeWriterRegistrationBlockingOnCutover` (for example in
`PreAddMember` and `runtimeRaftRegistrationTick`) to use
`proposeWriterRegistration` directly, and keep the leader/follower flow
centralized around `defaultProposer.Propose` and
`forwardWriterRegistrationToLeader`.
In `@main_raft_envelope_wiring.go`:
- Around line 116-175: Both raft DEK install paths duplicate the same
registration and wrap-publishing logic. Extract the shared work in
installFromApply and installRotatedRaftDEK into a common internal helper (for
example, a publishWrap-style method on raftEnvelopeRuntime) that handles
wrapFor, registered/MarkRegistered, activeWrap/activeRaftDEKID updates, and
SetRaftPayloadWrap on all groups. Keep the differing behavior only in the
callers: installFromApply should still validate cutoverIdx and store
cutoverIndex, while installRotatedRaftDEK should keep its cutoverIndex guards
and return early as today.
In `@main_s3.go`:
- Around line 23-139: `runS3Server` and `runSQSServer` duplicate the same
two-goroutine shutdown logic with `runDoneCtx`, `runDoneCancel`, `ctx.Done() ->
Stop()`, and `net.ErrClosed` handling. Extract this pattern into a shared helper
that accepts a small interface exposing `Run() error` and `Stop()`, then have
both `runS3Server` and `runSQSServer` delegate to it. Keep the existing `Run`
completion behavior, cancel flow, and `net.ErrClosed` treatment unchanged while
removing the duplicated logic.
In `@main.go`:
- Around line 1496-1554: Remove the inline wrapcheck suppressions from the
pass-through methods in startupGatedCoordinator by moving those exceptions into
the golangci wrapcheck configuration instead. Keep the necessary gRPC status
preservation in Dispatch, but delete the unnecessary //nolint:wrapcheck comments
from methods like IsLeader, VerifyLeader, LinearizableRead, LeaseRead,
LeaseReadForKey, LeaseReadAllGroups, and the related pass-through helpers; use
.golangci.yaml ignoreSigs or package-level exclusions to whitelist the
coordinator passthrough wrappers.
🪄 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: 86fdeea8-c4d1-4c9b-a8bf-bf558213002d
📒 Files selected for processing (41)
Makefileadapter/dynamodb.goadapter/dynamodb_errors.goadapter/encryption_admin.goadapter/encryption_admin_test.goadapter/s3.goadapter/sqs_catalog.goadapter/startup_gate_error_test.godocs/design/2026_04_29_partial_data_at_rest_encryption.mddocs/design/2026_05_31_implemented_6e_enable_raft_envelope.mdinternal/encryption/applier.gointernal/encryption/applier_test.gointernal/encryption/errors.gointernal/raftengine/etcd/factory.gointernal/raftengine/etcd/fsm_snapshot_file.gokv/coordinator.gokv/lease_warmup_test.gokv/raft_payload_wrapper.gokv/raft_payload_wrapper_test.gokv/sharded_coordinator.gomain.gomain_admin.gomain_admin_forward.gomain_admin_test.gomain_encryption_admin.gomain_encryption_confchange.gomain_encryption_confchange_test.gomain_encryption_e2e_test.gomain_encryption_registration.gomain_encryption_registration_test.gomain_encryption_rotate_on_startup.gomain_encryption_rotate_on_startup_test.gomain_encryption_startup_guard.gomain_encryption_startup_guard_test.gomain_encryption_write_wiring.gomain_encryption_write_wiring_test.gomain_raft_envelope_wiring.gomain_raft_envelope_wiring_test.gomain_s3.gomain_sqs.gomultiraft_runtime.go
| runtime, err := newRaftEnvelopeRuntime( | ||
| cipher, | ||
| encryption.NewDeterministicNonceFactory(0xCAFE, 7), | ||
| 1234, | ||
| oldKeyID, | ||
| &cutover, | ||
| raftEpoch, | ||
| nil, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
コンパイルエラー: newRaftEnvelopeRuntime の引数が不足しています（fullNodeID 欠落）。
newRaftEnvelopeRuntime のシグネチャは 8 引数 (cipher, nonceFactory, initialCutover, activeRaftDEKID, cell, raftEpoch, fullNodeID, registration) ですが、この呼び出しは 7 引数しか渡していません。raftEpoch の後に fullNodeID uint64 が必要で、末尾の nil は registration に対応します。現状では nil が uint64 の fullNodeID に割り当てられて型エラー兼引数不足となり、TestRaftEnvelopeRuntime_InstallRotatedRaftDEKRepublishesWrap はビルドできません。
🐛 修正案
runtime, err := newRaftEnvelopeRuntime(
cipher,
encryption.NewDeterministicNonceFactory(0xCAFE, 7),
1234,
oldKeyID,
&cutover,
raftEpoch,
+ 0,
nil,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runtime, err := newRaftEnvelopeRuntime( | |
| cipher, | |
| encryption.NewDeterministicNonceFactory(0xCAFE, 7), | |
| 1234, | |
| oldKeyID, | |
| &cutover, | |
| raftEpoch, | |
| nil, | |
| ) | |
| runtime, err := newRaftEnvelopeRuntime( | |
| cipher, | |
| encryption.NewDeterministicNonceFactory(0xCAFE, 7), | |
| 1234, | |
| oldKeyID, | |
| &cutover, | |
| raftEpoch, | |
| 0, | |
| 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 `@main_raft_envelope_wiring_test.go` around lines 124 - 132,
`newRaftEnvelopeRuntime` is being called with one argument too few, causing the
test to fail to compile because `fullNodeID` is missing. Update the call in
`TestRaftEnvelopeRuntime_InstallRotatedRaftDEKRepublishesWrap` to pass a valid
`fullNodeID` after `raftEpoch`, and keep the trailing `nil` for `registration`;
use the `newRaftEnvelopeRuntime` signature to align the argument order
correctly.
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Design docs
Validation
Author: bootjp
Summary by CodeRabbit
New Features
Bug Fixes