feat(encryption): Stage 6E-2c — dynamic raft envelope wrap on ShardGroup - #922
Conversation
Warning Review limit reached
More reviews will be available in 42 minutes and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces a runtime-swappable raft payload envelope wrapping system. It adds an atomic-backed mechanism to dynamically apply envelope wrapping to raft proposals, integrates this into ChangesDynamic Raft Envelope Wrapping Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 (26b939f):
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.
Code Review
This pull request implements a dynamic, runtime-swappable Raft payload wrapping mechanism (dynamicWrappedProposer) to support the Stage 6E-2c encryption cutover pipeline, allowing the wrap closure to be updated atomically on ShardGroup without stalling proposals. It also introduces startup warnings if a cutover is active but the wrap closure is missing. The reviewer feedback suggests adding defensive nil checks for the ShardGroup parameter in NewLeaderProxyForShardGroup and the inner proposer in newDynamicWrappedProposer to prevent potential nil pointer dereferences.
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.
| func NewLeaderProxyForShardGroup(g *ShardGroup, opts ...TransactionOption) *LeaderProxy { | ||
| // Prepend the wrap-installation option so a caller-provided |
There was a problem hiding this comment.
To prevent potential nil pointer dereferences, add a defensive nil check for the g parameter before accessing its fields.
| funcNewLeaderProxyForShardGroup(g*ShardGroup, opts...TransactionOption) *LeaderProxy { | |
| // Prepend the wrap-installation option so a caller-provided | |
| funcNewLeaderProxyForShardGroup(g*ShardGroup, opts...TransactionOption) *LeaderProxy { | |
| ifg==nil { | |
| returnnil | |
| } | |
| // Prepend the wrap-installation option so a caller-provided |
| func newDynamicWrappedProposer(inner raftengine.Proposer, wrapPtr *atomic.Pointer[RaftPayloadWrapper]) raftengine.Proposer { | ||
| if wrapPtr == nil { | ||
| // Defensive: passing a nil pointer would NPE on the first |
There was a problem hiding this comment.
Add a defensive nil check for the inner proposer to prevent potential nil pointer dereferences when wrapping it.
| funcnewDynamicWrappedProposer(inner raftengine.Proposer, wrapPtr*atomic.Pointer[RaftPayloadWrapper]) raftengine.Proposer { | |
| ifwrapPtr==nil { | |
| // Defensive: passing a nil pointer would NPE on the first | |
| funcnewDynamicWrappedProposer(inner raftengine.Proposer, wrapPtr*atomic.Pointer[RaftPayloadWrapper]) raftengine.Proposer { | |
| ifinner==nil { | |
| returnnil | |
| } | |
| ifwrapPtr==nil { | |
| // Defensive: passing a nil pointer would NPE on the first |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:26b939fc59
ℹ️ 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".
| slog.Warn("encryption: sidecar reports raft envelope cutover active but Stage 6E-2c does not install the wrap closure on shard groups; Stage 6E-2e/2f wires the cipher. Until then, fresh writes that land above the cutover would halt the apply loop on §6.3 strict-> unwrap. Operator action required.", | ||
| slog.Uint64("cutover_index", sc.RaftEnvelopeCutoverIndex), | ||
| slog.String("sidecar_path", sidecarPath)) |
There was a problem hiding this comment.
When this binary starts with a sidecar whose RaftEnvelopeCutoverIndex is already non-zero, this helper only logs and returns, and buildShardGroupsWithEncryptionWiring still returns success. In the exact scenario described by the new comment (restored/hand-edited sidecar with a cutover but no wrap-closure factory), the node will proceed to serve traffic with nil shard wrap pointers, so fresh proposals can land cleartext above the cutover and trip the strict unwrap path. Please make this path refuse startup (or otherwise return an error to the caller) rather than just warning.
Useful? React with 👍 / 👎.
| Store: st, | ||
| Txn: kv.NewLeaderProxyWithEngine(runtime.engine, kv.WithProposalObserver(observerForGroup(proposalObserverForGroup, g.id))), | ||
| } | ||
| sg.Txn = kv.NewLeaderProxyForShardGroup(sg, kv.WithProposalObserver(observerForGroup(proposalObserverForGroup, g.id))) |
There was a problem hiding this comment.
This only installs the dynamic wrapper on sg.Txn, but the same shard group still exposes Engine and has production paths that propose directly; for example ShardedCoordinator.RunHLCLeaseRenewal calls group.Engine.Propose(...) on the default-group leader every renewal interval. Once SetRaftPayloadWrap is populated after cutover, those direct proposals bypass the atomic wrap pointer and can append cleartext entries at index > raftEnvelopeCutoverIndex, causing the strict unwrap hook to halt. Please route shard-owned direct proposals such as HLC renewals through the same dynamic wrapper or add an explicit cutover-safe path for them.
Useful? React with 👍 / 👎.
bootjp
commented
Jun 3, 2026
@claude review Round-1 round-up:
The Head: d290026. |
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 (d290026):
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. |
Claude finished @bootjp's task in 4m 53s —— View job TLA+ Spec Divergence ReviewTasks:
TLA+ Spec Divergence AssessmentFiles changed that carry spec anchors: HLC subsystem ( |
| Invariant | Verdict | Note |
|---|---|---|
| HLC-4(iii) | Gap | RunHLCLeaseRenewal:1829 calls group.Engine.Propose() directly; post-6E-2d HLC renewals submit cleartext at index > cutover, causing GCM mismatch and apply-loop halt. Track for 6E-2 full milestone. |
| HLC-1 / HLC-2 / HLC-3 | No divergence | Issuance, ceiling, and leader-only paths unchanged. |
| OCC-1..OCC-5 | No divergence | transaction.go changes are wrapping-layer only; OCC commit-ts, conflict, lock-map semantics untouched. |
| MVCC-1..MVCC-4 | Not anchored | Not changed. |
| Routes-1..Routes-4 | Not anchored | Not changed. |
| Codex P1 | Deferred | Reachable only in downgrade scenarios; flag as 6E-3 pre-req for the authoritative startup refusal. |
| Codex P2 | Confirmed (= HLC-4(iii) gap) | Same as HLC gap above. |
The 6E-2c scope itself — dynamic wrap infrastructure only, no cipher factory, raftEnvelopeWrapEnabled still false — is correct and introduces no reachable safety violation in its current deployment window.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d29002621b
ℹ️ 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".
| Store: st, | ||
| Txn: kv.NewLeaderProxyWithEngine(runtime.engine, kv.WithProposalObserver(observerForGroup(proposalObserverForGroup, g.id))), | ||
| } | ||
| sg.Txn = kv.NewLeaderProxyForShardGroup(sg, kv.WithProposalObserver(observerForGroup(proposalObserverForGroup, g.id))) |
There was a problem hiding this comment.
When a follower handles a client write after SetRaftPayloadWrap is active, this sg.Txn wrapper is bypassed: LeaderProxy.forwardWithRetry sends the request to the leader's Internal.Forward, and startRaftServers still constructs that Internal service with a separate kv.NewTransactionWithProposer(rt.engine, ...) at main.go:1553. In that forwarded-write scenario the leader commits through the raw engine instead of the shard group's dynamic wrapper, so entries above raftEnvelopeCutoverIndex can land cleartext and trip the strict unwrap path even though local leader writes are wrapped.
Useful? React with 👍 / 👎.
bootjp
commented
Jun 3, 2026
@claude review Round-1 round-up:
Caller audit (per /loop): all production Head: 9a33e67. |
…LC renewal Codex round-1 surfaced two findings on the 6E-2c plumbing: **P1: fail closed on restored raft cutovers** The round-1 noteRaftEnvelopeCutoverStartup only logged a WARN; if a sidecar restored from a post-cutover deployment hit a binary without the wrap closure factory, the node would proceed to serve traffic with nil shard wrap pointers and fresh writes would land cleartext above the cutover, halting the apply loop on the §6.3 strict-> unwrap path. Renamed to refuseStartupOnActiveRaftCutover and now returns a fail-closed error so buildShardGroupsWithEncryption Wiring refuses startup before any shard storage opens. Operator sees the refusal at boot, before any traffic is admitted. The helper now runs BEFORE buildShardGroups (not after), so a refusal does not produce a half-built runtime set that the caller would need to tear down. **P2: route direct shard proposals through the wrapper** ShardedCoordinator.RunHLCLeaseRenewal called group.Engine.Propose directly on every renewal tick. Once SetRaftPayloadWrap is populated after cutover, those direct proposals would bypass the dynamicWrapped Proposer pointer and append cleartext entries at index > cutover, also tripping the strict-> unwrap halt. Fix: cache the dynamicWrappedProposer on the ShardGroup itself (g.proposer) when NewLeaderProxyForShardGroup constructs it, and publish it via the new ShardGroup.Proposer() accessor with an Engine fallback for legacy / test-fixture paths that bypass the constructor. RunHLCLeaseRenewal now calls group.Proposer().Propose so the HLC lease entry takes the same wrap path as user data. Withdrew the unused withDynamicRaftPayloadWrap option that the round-1 design used; NewLeaderProxyForShardGroup now constructs the proposer directly and shares it across both the TransactionManager path and the direct-shard path. Tests (4 renamed + 1 strengthened): - TestRefuseStartupOnActiveRaftCutover_NoSidecarPath / _MissingSidecar / _ZeroCutoverPasses — confirm the helper returns nil on the input branches that should pass. - TestRefuseStartupOnActiveRaftCutover_NonZeroCutoverRefuses — pins the codex P1 fail-closed property: non-zero cutover returns a non-nil error and the sidecar is unchanged on disk (helper is read-only). Caller audit (per /loop semantic-change rule): The P2 fix narrows the "direct Engine.Propose on a wrap-aware shard group" pattern. Audited every production .Engine.Propose call site: - kv/sharded_coordinator.go:1864 (RunHLCLeaseRenewal): migrated to group.Proposer().Propose this PR. ✓ - kv/coordinator.go:637 (Coordinate.ProposeHLCLease): single- group Coordinator. Production usage is via adapter/test_util.go's NewCoordinatorWithEngine, which is test-only. Not migrated; documented in Proposer() doc that legacy / test-fixture paths fall through to Engine. - main_encryption_registration.go:503 (defaultEngine.ProposeAdmin): deliberately raw-engine for the cutover marker / writer-registration path; the wrap-bypass is intentional and documented in the call site comment (the cutover marker MUST be cleartext, and the registration's post-cutover gap is tracked alongside 6E-2c/2e wiring). P1 fix is fail-closed, not fail-open — no caller silently degrades. Audited the single call site in buildShardGroupsWithEncryptionWiring; the new error propagates through run()'s existing chainEncryptionStartupGuard composition unchanged. Refs: codex P1+P2 round-1 on PR #922 (chatgpt-codex-connector, 2026-06-03T17:33:27Z, comment IDs 3350616197 + 3350616215)
Claude finished @bootjp's task in 4m 29s —— View job Round-2 ReviewTasks:
Verified: Round-1 FixesP1 — |
| Invariant | Status | Note |
|---|---|---|
| HLC-4(iii) — RunHLCLeaseRenewal | Closed | Migrated to group.Proposer().Propose in 9a33e67. |
| HLC-4(iii) — Internal.Forward path | Open | main.go:1553 still uses rt.engine; forwarded writes bypass wrap post-6E-2d. Pre-req for 6E-2d. |
| HLC-1 / HLC-2 / HLC-3 | No divergence | Unchanged. |
| OCC-1..OCC-5 | No divergence | transaction.go only removes withDynamicRaftPayloadWrap; OCC paths untouched. |
The 6E-2c scope is safe today (wrap pointer nil, raftEnvelopeWrapEnabled = false). The Internal-forward gap must be closed before or in 6E-2d.
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 (9a33e67):
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:9a33e678ef
ℹ️ 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".
| // deployment where 6E-2f flipped the gate, or hand-edited — | ||
| // both cases the operator must intervene before serving | ||
| // traffic. | ||
| if cutoverErr := refuseStartupOnActiveRaftCutover(sidecarPath); cutoverErr != nil { |
There was a problem hiding this comment.
When --encryption-enabled is on and the restored sidecar has both an active storage DEK and a non-zero RaftEnvelopeCutoverIndex, this refusal runs only after buildEncryptionWriteWiring has called prepareStorageNonceEpoch, which persists a BumpLocalEpoch to the sidecar. Since the process then refuses startup, an orchestrator restart loop can consume one local epoch per failed boot and eventually hit ErrLocalEpochExhausted before the operator has a chance to recover. Please perform the active-raft-cutover refusal before the write-path wiring mutates the sidecar.
Useful? React with 👍 / 👎.
bootjp
commented
Jun 3, 2026
@claude review Round-1 r3 round-up:
Caller audit (per /loop): every production Head: 83e7b3f. |
…utover Codex r2 P1: the Internal.Forward path on each leader's gRPC server is the receive side of follower-forwarded writes (LeaderProxy.forwardWithRetry sends them there). With round-1's fix in place, LOCAL leader writes now route through the ShardGroup's wrap-aware proposer (via NewLeaderProxyForShardGroup), but startRaftServers built the Internal service's TransactionManager from rt.engine directly — so a follower-forwarded write would commit through the raw engine and post-cutover entries could land cleartext above raftEnvelopeCutoverIndex, halting the apply loop on §6.3 strict-> unwrap. Plumbing: - serversInput.shardGroups (new field): threaded from run() into runtimeServerRunner.shardGroups, then into startRaftServers as a new positional argument. - proposerForGroup helper: centralises the per-runtime lookup so the Internal.Forward construction site and any future wrap-aware site share the same shape (sg.Proposer() with a raw engine fallback for test fixtures that pass a partial map). - startRaftServers's Internal.Forward TransactionManager now routes through proposerForGroup(rt, shardGroups), closing the forwarded-write gap. The fallback is deliberate: test fixtures that bypass buildShardGroups can pass an empty / partial shardGroups map and the Internal service still works on the raw engine. Production wires every runtime's shard group via buildShardGroups, so the fallback never fires. Caller audit (per /loop semantic-change rule): The new proposerForGroup helper has a single caller (the Internal.Forward trx construction inside startRaftServers). The LeaderProxy.Commit local path already routes through NewLeaderProxyForShardGroup. The Stage 6E-2c HLC lease renewal in ShardedCoordinator (PR #922 round-2) already routes through group.Proposer(). With this fix, every production .Engine.Propose audit-trail entry is either: - wrap-aware (LeaderProxy.Commit, Internal.Forward, RunHLCLeaseRenewal), or - explicitly raw-engine with a documented reason (adapter/encryption_admin.go's proposeEncryptionEntry uses the raw raftengine.Proposer because the cutover marker MUST be cleartext at index == cutover; main_encryption_registration.go has the same shape for ConfChange-time RegisterEncryptionWriter, where the post-cutover gap is tracked alongside 6E-2c/2e). Refs: codex P1 round-1 r2 on PR #922 (chatgpt-codex-connector, 2026-06-03T17:40:50Z, comment ID 3350658784)
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 (83e7b3f):
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. |
Claude finished @bootjp's task in 2m 54s —— View job Round-1 r3 ReviewTasks:
Verified: Round-1 r3 FixCodex r2 P1 — Internal.Forward forwarded-write bypass: ✓ ClosedThe funcproposerForGroup(rt*raftGroupRuntime, shardGroupsmap[uint64]*kv.ShardGroup) raftengine.Proposer {
ifsg, ok:=shardGroups[rt.spec.id]; ok&&sg!=nil {
returnsg.Proposer()
}
returnrt.engine
}The TLA+ HLC-4(iii) — CLOSED
Caller Audit (all production |
| Finding | Status |
|---|---|
| Round-1 P1 (fail-closed startup refusal) | ✓ Closed (9a33e67) |
| Round-1 P2 / TLA+ HLC-4(iii) (HLC renewal bypassed wrap) | ✓ Closed (9a33e67) |
| Round-2 P1 (Internal.Forward bypassed wrap) | ✓ Closed (83e7b3f) |
| Round-2 P2 (epoch bump before cutover refusal) | Open — 2-line reorder needed |
Minor: proposerForGroup no unit test | Open (low urgency) |
The PR is in good shape for 6E-2c's scope. The remaining open item (epoch-before-refusal ordering) is a clean fix. Recommend addressing it before merge to avoid the epoch drain in the (unlikely, but possible) downgrade-sidecar restart scenario.
To use Codex here, create a Codex account and connect to github. |
bootjp
commented
Jun 3, 2026
@claude review Round-1 r4 round-up:
Note re Claude round-2 review (4615200467): the "Internal.Forward bypass" item it flagged on commit 9a33e67 was already addressed in 83e7b3f (the r3 commit Claude hadn't seen yet) via Head: ad8b214. |
…car mutation Codex r3 P2: refuseStartupOnActiveRaftCutover ran AFTER buildEncryptionWriteWiring, which internally calls prepareStorageNonceEpoch → BumpLocalEpoch → durable sidecar write. On a sidecar with both an active storage DEK and a non-zero RaftEnvelopeCutoverIndex, an orchestrator restart loop would consume one local_epoch per failed boot and could hit ErrLocalEpochExhausted before the operator notices — turning a "recover by restoring a zero-cutover sidecar" remediation into "DEK unusable, full rotation required." Move the refusal BEFORE buildEncryptionWriteWiring so the sidecar stays untouched on the refusal path. The structural order is now: refuse → epoch-bump → buildShardGroups. The buildEncryptionWriteWiring step is unchanged in body; only its caller-side ordering shifted. Test: - TestRefuseStartupOnActiveRaftCutover_NonZeroCutoverDoesNotBumpEpoch writes a sidecar with storage LocalEpoch=7 and non-zero cutover, invokes the helper, then re-reads the sidecar and asserts LocalEpoch is still 7. Catches any future refactor that swaps the refusal order back behind the epoch-bump step. Refs: codex P2 r3 on PR #922 (chatgpt-codex-connector, 2026-06-03T17:48:32Z, comment ID 3350705536)
Claude finished @bootjp's task in 2m 55s —— View job Round-1 r4 ReviewTasks:
P2 r3 Fix — Ordering Invariant ✓
Minor framing note: the test exercises Internal.Forward bypass (r3 commit 83e7b3f) ✓
The second argument to TLA+ HLC-4(iii) ✓
Caller Audit — All Production |
| Site | Status |
|---|---|
main.go:906 — return rt.engine in proposerForGroup fallback | Only fires when shardGroups is empty/partial (test fixtures). Never in production where buildShardGroups wires every runtime. ✓ |
main_encryption_registration.go:503 — defaultEngine.ProposeAdmin | Deliberately raw-engine: the cutover marker at index == cutover must be cleartext so the strict-> hook leaves it alone. Documented exemption at the call site. ✓ |
rt.engine in adapter.NewInternalWithEngine | Passed as LeaderView (read-only), not a proposer. ✓ |
TLA+ Spec Divergence Summary (Final)
| Invariant | Status |
|---|---|
HLC-4(iii) — RunHLCLeaseRenewal direct engine call | Closed (r2, 9a33e67) |
HLC-4(iii) — Internal.Forward path bypass | Closed (r3, 83e7b3f) |
| HLC-4(iii) — sidecar epoch consumed before cutover refusal | Closed (r4, ad8b214) |
| HLC-1 / HLC-2 / HLC-3 | No divergence — unchanged |
| OCC-1..OCC-5 | No divergence — wrap layer only |
No open spec divergence items remain. The 6E-2c scope is correct: wrap infrastructure installed, pointer nil by default, raftEnvelopeWrapEnabled = false, no reachable safety violation in the current deployment window.
No further action required for this PR.
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 (ad8b214):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ad8b2147f2
ℹ️ 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".
| --concurrency ${{ inputs.concurrency || '8' }} \ | ||
| --key-count ${{ inputs.key-count || '16' }} \ | ||
| --max-writes-per-key ${{ inputs.max-writes-per-key || '250' }} \ | ||
| --max-txn-length ${{ inputs.max-txn-length || '4' }} \ |
There was a problem hiding this comment.
With the default max-txn-length of 4, this new DynamoDB step will generate multi-op transactions; the Jepsen client routes those through TransactWriteItems for any txn with more than one micro-op, while the gated DynamoDB dedup code only applies in retryItemWriteWithGeneration for single-item UpdateItem/PutItem/DeleteItem and transactWriteItemsWithRetry still uses the legacy retry loop. That means this scheduled "dedup mode" job can fail on duplicate anomalies from an intentionally unsupported multi-item path, or spend much of the run not exercising the new gate. Please force this step to --max-txn-length 1 (or split out a separate non-dedup multi-op workflow) if it is meant to validate the partial DynamoDB one-phase dedup path.
Useful? React with 👍 / 👎.
Install the Stage 6E-2 wrap-on-propose plumbing without yet activating any cipher: every shard group gets an atomic.Pointer storage for a RaftPayloadWrapper closure, the proposer chain consults that pointer on every Propose / ProposeAdmin call, and a startup hook surfaces a WARN if the sidecar reports a cutover already installed but the binary does not (yet) ship the cipher factory. Stage 6E-2d will use the hot-swap surface (ShardGroup.SetRaftPayloadWrap) to publish the active wrap closure when EnableRaftEnvelope commits; 6E-2e/2f will wire the cipher-aware factory. Pieces: - dynamicWrappedProposer (kv/raft_payload_wrapper.go): wraps a raftengine.Proposer with an atomic.Pointer load on every call. Mirrors the static wrappedProposer's Propose / ProposeAdmin routing and wrap semantics — the wrap layer is NOT a barrier exemption; Propose and ProposeAdmin apply the same wrap. The cutover marker stays cleartext via the raw-engine reference at the call site, not at the method level. - ShardGroup.raftPayloadWrap + SetRaftPayloadWrap + RaftPayloadWrap (kv/sharded_coordinator.go): the published hot-swap surface. A nil load means the wrap is inactive (the Stage 3 default); a non-nil load applies the closure to every payload before it reaches the engine. - NewLeaderProxyForShardGroup (kv/sharded_coordinator.go) + withDynamicRaftPayloadWrap option (kv/transaction.go): the production wiring path. main.go now constructs every shard group's LeaderProxy via this helper so the dynamic wrap chain is universal; a future "I forgot to wire wrap on this code path" regression would surface as a wrap install no-op rather than a silent dropped envelope. - noteRaftEnvelopeCutoverStartup (main_encryption_write_wiring.go): process-start sidecar read. Stays silent in the universal pre-cutover case (RaftEnvelopeCutoverIndex == 0) and on any sidecar read error (the Stage 6C-1 startup guard is the authoritative refuser). Logs a WARN if the cutover index is non-zero — the binary's wrap pointer stays nil because the cipher factory ships in 6E-2e/2f, so post-cutover writes proposed under this binary WOULD halt the apply loop on §6.3 strict-> unwrap. In a healthy 6E-2c-only deployment this never fires (raftEnvelopeWrapEnabled is still false in adapter/encryption_admin.go); the WARN fires only on a sidecar restored from a future deployment or hand-edited. Tests: - TestDynamicWrappedProposer_NilPointerReturnsInnerVerbatim: defensive degradation when newDynamicWrappedProposer receives a nil *atomic.Pointer. - TestDynamicWrappedProposer_NilStoredIsPassThrough: Stage 3 default (Propose and ProposeAdmin route verbatim through the inner proposer's respective method when the pointer holds nil). - TestDynamicWrappedProposer_LoadsCurrentWrapEveryCall: hot-swap contract — pre-install / install / clear all observed by the next proposal without rebuilding the proposer. - TestDynamicWrappedProposer_ProposeAdminAppliesCurrentWrap: admin-path mirror, confirms ProposeAdmin still routes through inner.ProposeAdmin (not Propose) when the wrap is active. - TestShardGroup_SetRaftPayloadWrap_RoundTrip: ShardGroup's Set/Get round-trip including the nil-clears semantics. - TestNoteRaftEnvelopeCutoverStartup_* (×4): startup-hook input matrix — empty path, missing file, zero cutover, non-zero cutover (read-only, sidecar unchanged). Five-lens self-review: - Data loss: the dynamic wrap proposer is a pass-through when the pointer holds nil (the Stage 6E-2c default). Production behaviour is unchanged today — every proposal still lands cleartext via the engine's existing path. No new error swallow, no new fall-through. - Concurrency / distributed: the hot-swap surface uses atomic.Pointer.Store / Load; reads are lock-free and observe a fully-published closure (either nil or non-nil) on every call. Two concurrent Store calls observe last-writer-wins; no torn read. - Performance: one extra atomic.Pointer.Load per Propose / ProposeAdmin call on the dynamic path. No allocation, no contention. The wrap closure invocation cost is paid only when a non-nil closure is installed; that path matches wrappedProposer's existing cost profile. - Data consistency: the wrap pointer's lifetime is owned by the ShardGroup that owns the engine; the proposer holds a *atomic.Pointer reference back into ShardGroup. No cross-shard mixing of wrap state — each shard has its own pointer, so a future per-shard rotation policy works without changes here. - Test coverage: 8 new tests cover the wrap-proposer mechanics (nil pointer defensive, nil/non-nil stored, hot-swap, ProposeAdmin mirror), the ShardGroup hot-swap surface (round-trip), and the startup hook (4 input branches). Caller audit (per /loop semantic-change rule): - kv.NewLeaderProxyWithEngine (kept; non-wrap-aware): no behaviour change. Test fixtures and any shard group that opts out of encryption continue to use it. - kv.NewLeaderProxyForShardGroup (new; wrap-aware): used by main.go's buildShardGroups for every production shard group. Audit: the only construction site for production ShardGroup instances now routes through this helper. - ShardGroup struct literal in main.go: switched from a one-shot literal with Txn set inline to a two-step build (struct literal, then Txn = NewLeaderProxyForShardGroup(sg, opts...)). Necessary because NewLeaderProxyForShardGroup needs the &sg.raftPayloadWrap pointer, which is only addressable after the struct exists. - SetRaftPayloadWrap callers: none in this PR. The hot-swap surface is published for Stage 6E-2d's EnableRaftEnvelope handler. Refs: docs/design/2026_05_31_partial_6e_enable_raft_envelope.md
… wrap constructors Gemini round-1 ×2 medium asked for defensive nil checks on newDynamicWrappedProposer(inner ...) and NewLeaderProxyForShardGroup(g ...). Rejecting both per CLAUDE.md's "don't validate for scenarios that can't happen at internal boundaries" — silently returning nil would defer the panic to the first call site (Propose / Commit) with worse diagnostics. Documented the contract on both constructors so the rejection rationale lives in-code rather than review history. The wrapPtr-is-nil degradation on newDynamicWrappedProposer is preserved: that one matches newWrappedProposer(inner, nil) returning inner verbatim, and the wiring-side test fixture TestDynamicWrappedProposer_NilPointerReturnsInnerVerbatim pins the shape against regressions.
…LC renewal Codex round-1 surfaced two findings on the 6E-2c plumbing: **P1: fail closed on restored raft cutovers** The round-1 noteRaftEnvelopeCutoverStartup only logged a WARN; if a sidecar restored from a post-cutover deployment hit a binary without the wrap closure factory, the node would proceed to serve traffic with nil shard wrap pointers and fresh writes would land cleartext above the cutover, halting the apply loop on the §6.3 strict-> unwrap path. Renamed to refuseStartupOnActiveRaftCutover and now returns a fail-closed error so buildShardGroupsWithEncryption Wiring refuses startup before any shard storage opens. Operator sees the refusal at boot, before any traffic is admitted. The helper now runs BEFORE buildShardGroups (not after), so a refusal does not produce a half-built runtime set that the caller would need to tear down. **P2: route direct shard proposals through the wrapper** ShardedCoordinator.RunHLCLeaseRenewal called group.Engine.Propose directly on every renewal tick. Once SetRaftPayloadWrap is populated after cutover, those direct proposals would bypass the dynamicWrapped Proposer pointer and append cleartext entries at index > cutover, also tripping the strict-> unwrap halt. Fix: cache the dynamicWrappedProposer on the ShardGroup itself (g.proposer) when NewLeaderProxyForShardGroup constructs it, and publish it via the new ShardGroup.Proposer() accessor with an Engine fallback for legacy / test-fixture paths that bypass the constructor. RunHLCLeaseRenewal now calls group.Proposer().Propose so the HLC lease entry takes the same wrap path as user data. Withdrew the unused withDynamicRaftPayloadWrap option that the round-1 design used; NewLeaderProxyForShardGroup now constructs the proposer directly and shares it across both the TransactionManager path and the direct-shard path. Tests (4 renamed + 1 strengthened): - TestRefuseStartupOnActiveRaftCutover_NoSidecarPath / _MissingSidecar / _ZeroCutoverPasses — confirm the helper returns nil on the input branches that should pass. - TestRefuseStartupOnActiveRaftCutover_NonZeroCutoverRefuses — pins the codex P1 fail-closed property: non-zero cutover returns a non-nil error and the sidecar is unchanged on disk (helper is read-only). Caller audit (per /loop semantic-change rule): The P2 fix narrows the "direct Engine.Propose on a wrap-aware shard group" pattern. Audited every production .Engine.Propose call site: - kv/sharded_coordinator.go:1864 (RunHLCLeaseRenewal): migrated to group.Proposer().Propose this PR. ✓ - kv/coordinator.go:637 (Coordinate.ProposeHLCLease): single- group Coordinator. Production usage is via adapter/test_util.go's NewCoordinatorWithEngine, which is test-only. Not migrated; documented in Proposer() doc that legacy / test-fixture paths fall through to Engine. - main_encryption_registration.go:503 (defaultEngine.ProposeAdmin): deliberately raw-engine for the cutover marker / writer-registration path; the wrap-bypass is intentional and documented in the call site comment (the cutover marker MUST be cleartext, and the registration's post-cutover gap is tracked alongside 6E-2c/2e wiring). P1 fix is fail-closed, not fail-open — no caller silently degrades. Audited the single call site in buildShardGroupsWithEncryptionWiring; the new error propagates through run()'s existing chainEncryptionStartupGuard composition unchanged. Refs: codex P1+P2 round-1 on PR #922 (chatgpt-codex-connector, 2026-06-03T17:33:27Z, comment IDs 3350616197 + 3350616215)
…utover Codex r2 P1: the Internal.Forward path on each leader's gRPC server is the receive side of follower-forwarded writes (LeaderProxy.forwardWithRetry sends them there). With round-1's fix in place, LOCAL leader writes now route through the ShardGroup's wrap-aware proposer (via NewLeaderProxyForShardGroup), but startRaftServers built the Internal service's TransactionManager from rt.engine directly — so a follower-forwarded write would commit through the raw engine and post-cutover entries could land cleartext above raftEnvelopeCutoverIndex, halting the apply loop on §6.3 strict-> unwrap. Plumbing: - serversInput.shardGroups (new field): threaded from run() into runtimeServerRunner.shardGroups, then into startRaftServers as a new positional argument. - proposerForGroup helper: centralises the per-runtime lookup so the Internal.Forward construction site and any future wrap-aware site share the same shape (sg.Proposer() with a raw engine fallback for test fixtures that pass a partial map). - startRaftServers's Internal.Forward TransactionManager now routes through proposerForGroup(rt, shardGroups), closing the forwarded-write gap. The fallback is deliberate: test fixtures that bypass buildShardGroups can pass an empty / partial shardGroups map and the Internal service still works on the raw engine. Production wires every runtime's shard group via buildShardGroups, so the fallback never fires. Caller audit (per /loop semantic-change rule): The new proposerForGroup helper has a single caller (the Internal.Forward trx construction inside startRaftServers). The LeaderProxy.Commit local path already routes through NewLeaderProxyForShardGroup. The Stage 6E-2c HLC lease renewal in ShardedCoordinator (PR #922 round-2) already routes through group.Proposer(). With this fix, every production .Engine.Propose audit-trail entry is either: - wrap-aware (LeaderProxy.Commit, Internal.Forward, RunHLCLeaseRenewal), or - explicitly raw-engine with a documented reason (adapter/encryption_admin.go's proposeEncryptionEntry uses the raw raftengine.Proposer because the cutover marker MUST be cleartext at index == cutover; main_encryption_registration.go has the same shape for ConfChange-time RegisterEncryptionWriter, where the post-cutover gap is tracked alongside 6E-2c/2e). Refs: codex P1 round-1 r2 on PR #922 (chatgpt-codex-connector, 2026-06-03T17:40:50Z, comment ID 3350658784)
…car mutation Codex r3 P2: refuseStartupOnActiveRaftCutover ran AFTER buildEncryptionWriteWiring, which internally calls prepareStorageNonceEpoch → BumpLocalEpoch → durable sidecar write. On a sidecar with both an active storage DEK and a non-zero RaftEnvelopeCutoverIndex, an orchestrator restart loop would consume one local_epoch per failed boot and could hit ErrLocalEpochExhausted before the operator notices — turning a "recover by restoring a zero-cutover sidecar" remediation into "DEK unusable, full rotation required." Move the refusal BEFORE buildEncryptionWriteWiring so the sidecar stays untouched on the refusal path. The structural order is now: refuse → epoch-bump → buildShardGroups. The buildEncryptionWriteWiring step is unchanged in body; only its caller-side ordering shifted. Test: - TestRefuseStartupOnActiveRaftCutover_NonZeroCutoverDoesNotBumpEpoch writes a sidecar with storage LocalEpoch=7 and non-zero cutover, invokes the helper, then re-reads the sidecar and asserts LocalEpoch is still 7. Catches any future refactor that swaps the refusal order back behind the epoch-bump step. Refs: codex P2 r3 on PR #922 (chatgpt-codex-connector, 2026-06-03T17:48:32Z, comment ID 3350705536)
bootjp
commented
Jun 3, 2026
Force-push: rebased onto origin/main to drop an unrelated commit ( New head: 9fe4125. This makes the Codex P2 r4 finding on @claude review |
ad8b214 to
9fe4125CompareClaude finished @bootjp's task in 2m 5s —— View job Round-1 r5 ReviewTasks:
Rebase Verification ✓The force-push is clean. The 5 Stage 6E-2c commits are intact with the expected SHAs: The unrelated All Previous Findings — Still Resolved
No new issues introduced by the rebase. The code content at the new head is identical to the TLA+ Spec Divergence (Final)
No open spec divergence items. The 6E-2c scope is correct and the branch is ready to merge. |
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 (9fe4125):
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.
🧹 Nitpick comments (2)
main.go (1)
1573-1573: ⚡ Quick winAdd a regression test for the forwarded-write proposer selection.
This
NewTransactionWithProposer(...)call is what keepsInternal.Forwardon the wrap-aware path instead of falling back tort.engine. The added tests cover the wrapper types themselves, but nothing in the supplied coverage pins thismain.gowiring, so a later refactor here can silently reopen the cleartext-bypass path without breaking tests.🤖 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` at line 1573, Add a regression test that asserts forwarded-write proposer selection uses the wrap-aware path by ensuring NewTransactionWithProposer is invoked with proposerForGroup (and the proposal observer via observerForGroup/proposalObserverForGroup) so Internal.Forward does not fall back to rt.engine; in the test, exercise the forwarding code path for the wrapper types and verify behavior/signature that only the proposer returned by proposerForGroup is used (mock or spy proposer/observer to detect invocation) to pin the wiring around NewTransactionWithProposer, proposerForGroup, observerForGroup, proposalObserverForGroup, Internal.Forward and rt.engine.main_encryption_write_wiring_test.go (1)
215-224: ⚡ Quick winCover the non-ENOENT sidecar read failure branch too.
refuseStartupOnActiveRaftCutoveronly returns nil forIsNotExist; malformed or otherwise unreadable sidecars are supposed to abort startup. Right now this file pins the missing-file branch but not the fail-closed read-error branch, so a future refactor could accidentally widen this helper into fail-open behavior.🤖 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_write_wiring_test.go` around lines 215 - 224, Add a complementary test that exercises the non-ENOENT read-failure branch so malformed/unreadable sidecars cause startup refusal: create a new test (e.g., TestRefuseStartupOnActiveRaftCutover_BadSidecar) which writes a malformed or permission-denied sidecar file into a temp dir and asserts that refuseStartupOnActiveRaftCutover(...) returns a non-nil error; reference the refuseStartupOnActiveRaftCutover function and mirror the style of TestRefuseStartupOnActiveRaftCutover_MissingSidecar to ensure the fail-closed behavior is covered.
🤖 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.
Nitpick comments:
In `@main_encryption_write_wiring_test.go`:
- Around line 215-224: Add a complementary test that exercises the non-ENOENT
read-failure branch so malformed/unreadable sidecars cause startup refusal:
create a new test (e.g., TestRefuseStartupOnActiveRaftCutover_BadSidecar) which
writes a malformed or permission-denied sidecar file into a temp dir and asserts
that refuseStartupOnActiveRaftCutover(...) returns a non-nil error; reference
the refuseStartupOnActiveRaftCutover function and mirror the style of
TestRefuseStartupOnActiveRaftCutover_MissingSidecar to ensure the fail-closed
behavior is covered.
In `@main.go`:
- Line 1573: Add a regression test that asserts forwarded-write proposer
selection uses the wrap-aware path by ensuring NewTransactionWithProposer is
invoked with proposerForGroup (and the proposal observer via
observerForGroup/proposalObserverForGroup) so Internal.Forward does not fall
back to rt.engine; in the test, exercise the forwarding code path for the
wrapper types and verify behavior/signature that only the proposer returned by
proposerForGroup is used (mock or spy proposer/observer to detect invocation) to
pin the wiring around NewTransactionWithProposer, proposerForGroup,
observerForGroup, proposalObserverForGroup, Internal.Forward and rt.engine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40d5a036-a22d-4334-891a-ccc858bd7f06
📒 Files selected for processing (6)
kv/raft_payload_wrapper.gokv/raft_payload_wrapper_test.gokv/sharded_coordinator.gomain.gomain_encryption_write_wiring.gomain_encryption_write_wiring_test.go
…proposer + fail-closed read Coderabbit round-1 r5 surfaced two defense-in-depth nitpicks; both add now to prevent silent reopening of cleartext-bypass paths in a future refactor: - main.go:1573 (Internal.Forward proposer selection): a future refactor that swaps proposerForGroup back to rt.engine would silently bypass the wrap chain for follower-forwarded writes after the EnableRaftEnvelope cutover. Added a focused unit-test file main_proposer_for_group_test.go pinning all three branches: (1) shardGroups has entry → sg.Proposer() (wrap-aware) AND not the raw engine; (2) shardGroups missing entry → rt.engine fallback; (3) shardGroups has explicit nil entry → rt.engine fallback (the partial-map invariant for both shapes). - main_encryption_write_wiring.go:refuseStartupOnActiveRaftCutover (non-ENOENT read failures): the existing test suite covered the IsNotExist branch (pre-bootstrap) but not the propagate-non-NotExist-errors branch. A future refactor that silently widens the nil-return to "any read failure" would bypass the cutover guard. Added TestRefuseStartupOnActiveRaftCutover_BadSidecarPropagates that writes a malformed (non-JSON) sidecar and asserts the helper returns a non-nil error. No production code changed; both helpers behave as before. The tests pin the documented contract so the next refactor that wants to relax these checks must visibly remove the assertion.
bootjp
commented
Jun 3, 2026
@claude review Round-1 r5 round-up:
Both are defense-in-depth regression tests; no production code changed. Head: 47bb81a. |
Claude finished @bootjp's task in 2m 46s —— View job Round-1 r5 ReviewTasks:
Verified: r5 Test Coverage
|
| Test | Branch | Assertion |
|---|---|---|
TestProposerForGroup_UsesShardGroupProposerWhenPresent | sg != nil → sg.Proposer() | got != raw engine AND got == sg.Proposer() |
TestProposerForGroup_FallsBackToEngineWhenShardGroupMissing | key absent in map | got == rt.engine |
TestProposerForGroup_FallsBackToEngineWhenShardGroupNil | shardGroups[id] = nil | got == rt.engine |
Correctness trace for the wrap-aware case:NewLeaderProxyForShardGroup(sg) sets g.proposer = newDynamicWrappedProposer(g.Engine, &g.raftPayloadWrap). &g.raftPayloadWrap is non-nil (address of a struct field), so newDynamicWrappedProposer returns a *dynamicWrappedProposer — not inner verbatim. sg.Proposer() returns g.proposer (the cached *dynamicWrappedProposer). The comparison got == raftengine.Proposer(engine) is false (different dynamic types), and got == sg.Proposer() is true (same pointer). ✓
stubEngineForProposerTest embeds a nil raftengine.Engine interface value, which is fine here — proposerForGroup only stores/returns the reference; it never calls Propose or ProposeAdmin in these tests. The nil-embedded shape satisfies the type constraint without requiring a full Engine surface area.
TestRefuseStartupOnActiveRaftCutover_BadSidecarPropagates ✓
Writes "not a valid sidecar" (non-JSON) to a temp path; asserts non-nil error. Production code path: ReadSidecar fails JSON decode → error is non-NotExist → pkgerrors.Wrap(err, "encryption: refuse startup on active raft cutover (read sidecar)") propagates. The test correctly asserts err != nil without pinning the error chain type. ✓
Minor: Stale comment in TestRefuseStartupOnActiveRaftCutover_MissingSidecar (lines 220–221)
The existing MissingSidecar test comment says:
"silently returns nil on any ReadSidecar error"
This was accurate before r4 but is now stale. Since the r4 Codex P1 fix, refuseStartupOnActiveRaftCutover propagates non-NotExist read errors rather than silently returning nil. The BadSidecarPropagates test (added in r5) documents the correct narrow contract — but the MissingSidecar comment contradicts it.
The test itself is correct; only the comment is misleading. Not a blocker for merge, but worth fixing in a follow-up to keep the test docblock accurate.
All Prior Findings — Still Resolved at 47bb81a
No production code changed in r5; the unchanged kv/sharded_coordinator.go and main.go carry the same semantics as reviewed in r4.
| Finding | Status |
|---|---|
| R1 P1 — fail-closed startup refusal | ✓ Closed (eca4cb5) |
R1 P2 / TLA+ HLC-4(iii) — RunHLCLeaseRenewal direct engine call | ✓ Closed (eca4cb5) |
R2 P1 — Internal.Forward bypass | ✓ Closed (5c79b38) |
| R3 P2 — epoch bump before cutover refusal | ✓ Closed (9fe4125) |
CodeRabbit nitpick — proposerForGroup unverified | ✓ Closed (47bb81a) |
| CodeRabbit nitpick — non-NotExist branch uncovered | ✓ Closed (47bb81a) |
TLA+ Spec Divergence Summary (Final)
| Invariant | Status |
|---|---|
| HLC-4(iii) — all identified direct-engine proposal sites | Closed |
| HLC-1 / HLC-2 / HLC-3 | No divergence — unchanged |
| OCC-1..OCC-5 | No divergence — wrap layer only |
| MVCC-1..MVCC-4 / Routes-1..Routes-4 | Not anchored / not changed |
No open spec divergence items. The 6E-2c scope is correct and the branch is ready to merge. The one minor item (stale comment in TestRefuseStartupOnActiveRaftCutover_MissingSidecar) can be addressed in a cleanup commit or follow-up PR.
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 (47bb81a):
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. |
Claude r6 caught a stale comment that predated the codex P1 r2 narrowing of the nil-return branch. The MissingSidecar test was written when refuseStartupOnActiveRaftCutover silently returned nil on any read error; the fail-closed fix narrowed that to encryption.IsNotExist only. The test itself was already correct (it asserts the missing-file case returns nil) — only the docblock contradicted the actual contract. Rewrote the comment to point at the BadSidecarPropagates sibling test as the canonical reference for the non-NotExist branch.
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 (b452b0c):
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. |
bootjp
commented
Jun 4, 2026
@claude review please verify TLA+ spec divergence per the checklist above. |
Claude finished @bootjp's task in 1m 59s —— View job TLA+ Spec Divergence Review — head b452b0cTasks:
What changed in b452b0cSingle doc-only commit: The
The revised comment accurately describes the code at TLA+ Spec Divergence — Final StatusAll findings identified across rounds 1–5 remain resolved and unchanged at b452b0c:
The Deliberate raw-engine exemptions (carry-over audit)
No open spec divergence items. The 6E-2c scope (dynamic wrap infrastructure, nil wrap pointer by default, |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Stage 6E-2c: install the Stage 6E-2 wrap-on-propose plumbing on every shard group without yet activating any cipher. Every
ShardGroupgets anatomic.Pointer[RaftPayloadWrapper]cell; the proposer chain consults that pointer on everyPropose/ProposeAdmincall;SetRaftPayloadWrapis the hot-swap surface Stage 6E-2d will use whenEnableRaftEnvelopecommits. The wrap closure factory itself ships in Stage 6E-2e/2f.A startup hook reads the sidecar and logs a
WARNifRaftEnvelopeCutoverIndex != 0— operator-visible refusal of a deployment that crosses the cutover line under a binary without the cipher factory wired.Pieces
dynamicWrappedProposer(kv/raft_payload_wrapper.go): wraps araftengine.Proposerwith anatomic.Pointer.Loadon every call. MirrorswrappedProposer's Propose / ProposeAdmin routing and wrap semantics — the wrap layer is NOT a barrier exemption; Propose and ProposeAdmin apply the same wrap. The cutover marker stays cleartext via the raw-engine reference at the call site, not at the method level.ShardGroup.raftPayloadWrap+SetRaftPayloadWrap+RaftPayloadWrap(kv/sharded_coordinator.go): the published hot-swap surface.NewLeaderProxyForShardGroup+withDynamicRaftPayloadWrap(kv/sharded_coordinator.go, kv/transaction.go): the production wiring path.main.go'sbuildShardGroupsnow constructs every shard group'sLeaderProxyvia this helper so the dynamic wrap chain is universal.noteRaftEnvelopeCutoverStartup(main_encryption_write_wiring.go): process-start sidecar read. Silent on the universal pre-cutover case and on any read error (the Stage 6C-1 startup guard is the authoritative refuser). LogsWARNonly when the cutover index is non-zero but the binary does not (yet) ship the cipher factory.Tests (8 new)
TestDynamicWrappedProposer_NilPointerReturnsInnerVerbatim— defensive degradation for nil*atomic.Pointer.TestDynamicWrappedProposer_NilStoredIsPassThrough— Stage 3 default (verbatim routing for Propose and ProposeAdmin separately).TestDynamicWrappedProposer_LoadsCurrentWrapEveryCall— hot-swap contract (install / clear observed by the next proposal).TestDynamicWrappedProposer_ProposeAdminAppliesCurrentWrap— admin-path mirror; confirmsinner.ProposeAdmin(notPropose) is reached when wrap is active.TestShardGroup_SetRaftPayloadWrap_RoundTrip— Set/Get round-trip including the nil-clears semantics.TestNoteRaftEnvelopeCutoverStartup_*(×4) — empty path / missing file / zero cutover / non-zero cutover (asserts the hook is read-only).Five-lens self-review
atomic.Pointer.Store/Load; reads are lock-free and observe a fully-published closure (either nil or non-nil) on every call. Last-writer-wins on concurrent stores; no torn read.atomic.Pointer.Loadper Propose / ProposeAdmin on the dynamic path; no allocation, no contention. Wrap closure invocation cost is paid only when non-nil; matches the existingwrappedProposerprofile.ShardGroupthat owns the engine; the proposer holds an*atomic.Pointerreference back intoShardGroup. No cross-shard mixing — each shard has its own pointer, so a future per-shard rotation policy works without changes here.Caller audit (per /loop semantic-change rule)
kv.NewLeaderProxyWithEngine(kept; non-wrap-aware): no behaviour change. Test fixtures and any shard group that opts out of encryption continue to use it.kv.NewLeaderProxyForShardGroup(new; wrap-aware): used bymain.go'sbuildShardGroupsfor every production shard group. Audit: the only construction site for productionShardGroupinstances now routes through this helper.main.go: switched from one-shot literal withTxnset inline to two-step build (struct literal, thenTxn = NewLeaderProxyForShardGroup(sg, opts...)). Necessary becauseNewLeaderProxyForShardGroupneeds the&sg.raftPayloadWrappointer, which is only addressable after the struct exists.SetRaftPayloadWrapcallers: none in this PR. The surface is published for Stage 6E-2d'sEnableRaftEnvelopehandler.Test plan
go build ./...cleango vet ./...cleangolangci-lint runclean (pre-commit hook ran)go test -race -run 'DynamicWrappedProposer|ShardGroup_Set|WrappedProposer|NoteRaftEnvelopeCutover' .passesgo test -race ./kv/scoped tests passRefs:
docs/design/2026_05_31_partial_6e_enable_raft_envelope.mdSummary by CodeRabbit
Release Notes
New Features
Chores
Tests