Repository navigation
Fix lost Redis appends during Raft leader handoff - #1286
Conversation
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 (fd25bda):
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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 43 minutes. View limit details
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 (2f5e5a8):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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 (2f5e5a8):
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. |
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 (afbb73d):
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f5e5a84b2
ℹ️ 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 mut.Op != pb.Op_PUT { | ||
| continue |
There was a problem hiding this comment.
When an option-2 retry contains a delete and another transaction committed any version of its primary key at the colliding PrevCommitTS, this loop skips the delete entirely; for a delete-only write, or when all PUTs happen to match, the function therefore returns true without verifying that a tombstone exists at that timestamp. The FSM then no-ops the whole Redis EXEC or DynamoDB retry and can acknowledge a deletion that never occurred. Compare the exact operation/tombstone state for DEL mutations before declaring the prior attempt a match.
Useful? React with 👍 / 👎.
| value, err := f.store.GetAt(ctx, key, prevTS) | ||
| if err != nil { | ||
| return false, errors.WithStack(err) |
There was a problem hiding this comment.
When PrevCommitTS has fallen below a replica's local retention watermark but the exact version is still physically present, CommittedVersionAt deliberately reports the version so retry dedup remains deterministic, but this subsequent GetAt returns ErrReadTSCompacted. A genuine landed retry that previously no-op'ed therefore becomes an error, and replicas with different local retention floors can produce different apply results for the same Raft entry. Use an exact-value probe that bypasses the retention watermark, matching the existence probe's contract.
Useful? React with 👍 / 👎.
| return s.stageMutationsInRaftEntry(ctx, scope, mutations, readKeys, startTS, commitTS, appliedIndex) | ||
| } | ||
| return s.applyMutationsWithOpts(ctx, mutations, readKeys, startTS, commitTS, s.raftApplyWriteOpts(), false, appliedIndex, true) | ||
| return s.applyMutationsWithOpts(ctx, mutations, readKeys, startTS, commitTS, s.raftApplyWriteOpts(), false, appliedIndex, true, false) |
There was a problem hiding this comment.
The raw FSM path still calls ApplyMutationsRaftAt, and this false disables the new commit-timestamp fence for those live entries. If concurrent requests allocate timestamps in one order but reach Raft in the opposite order, a raw PUT/DELETE with commitTS <= LastCommitTS()—especially an equal timestamp on a different key—passes because raw writes use startTS == commitTS; it can therefore add a version after a reader has already used that timestamp as an applied snapshot. Route live raw mutations through a fenced path while retaining the unfenced method only for PREPARE/2PC semantics and verified replay.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (01e8424):
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. |
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 (aa51b35):
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: aa51b35c4f
ℹ️ 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".
| value, err := f.store.GetAt(ctx, key, prevTS) | ||
| if err != nil { | ||
| return false, errors.WithStack(err) |
There was a problem hiding this comment.
When a PUT retry's PrevCommitTS collides with another transaction's delete on the same key during a leader handoff, CommittedVersionAt reports the tombstone but this GetAt returns ErrKeyNotFound. The FSM consequently rejects the retry even though its prior PUT never landed, potentially surfacing a Redis or DynamoDB write failure; inspect the exact version's operation and treat a tombstone as a mismatch for PUT.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
| (locking setup-done | ||
| (when-not @setup-done | ||
| (with-retries max-retries #(local-setup! mount-dir keys)) | ||
| (reset! setup-done true))) |
There was a problem hiding this comment.
After one local setup succeeds, this persistent guard prevents every subsequent setup! call on the same workload client from running local-setup! again. That leaves values written by a previous invocation in place and deterministically breaks the existing local-client-setup-is-idempotent-and-resets-values test, which writes 3, invokes setup again, and expects the initial value; scope the once-only coordination to concurrent initialization rather than permanently suppressing later setup calls.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (6d874a3):
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: 6d874a3744
ℹ️ 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".
| matched = bytes.Equal(value, mut.Value) | ||
| break |
There was a problem hiding this comment.
When staged visibility is active and both the live key and a staged alias have versions at PrevCommitTS, this unconditional break stops after the live candidate even if its value belongs to a colliding transaction. A matching staged copy of the actual prior attempt is therefore ignored, so a genuine landed retry falls through to normal application and may conflict or be recomputed. Continue through the remaining aliases when bytes.Equal is false, and stop only after finding a match.
Useful? React with 👍 / 👎.
| // ApplyMutationsRaftAtFenced rejects a live one-phase write whose | ||
| // commit timestamp is at or below the already-applied watermark. A write | ||
| // below that watermark could appear after a reader took that snapshot. | ||
| // PREPARE and 2PC COMMIT use the unfenced method above. | ||
| ApplyMutationsRaftAtFenced(ctx context.Context, mutations []*KVPairMutation, readKeys [][]byte, startTS, commitTS, appliedIndex uint64) error |
There was a problem hiding this comment.
When a multi-shard transaction receives commit timestamp c1 and another entry advances the primary shard's watermark to c2 > c1 before PREPARE arrives, a reader can successfully take snapshot c2 before the intent exists; handleCommitRequest then uses the unfenced ApplyMutationsRaftAt and later publishes the primary version at c1, changing that already-served snapshot. Subsequent validation also misses the write because c1 <= c2. The first primary COMMIT needs the timestamp fence, while idempotent replays and secondary COMMITs can retain their existing exemptions.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (78cab1b):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
@codex review |
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 (78cab1b):
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 17s —— View job TLA+ spec divergence review (head
|
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Related acceptance evidence from #1290: encrypted DynamoDB list-append with follower ingress/no HTTP map, snapshot interval 20, prior append -> fresh table reset -> six verified leader transfers failed on 8db0155 (G-single-item-realtime/G1c-realtime; acknowledged append 37 to key 16 was absent from a later read including 46). A separate diagnostic build combining 8db0155 with this PR at 78cab1b passed one identical Jepsen run with six transfers; this suggests overlap but is not a causal proof or a completed stress campaign. No duplicate Raft implementation was added to #1290. That combined run then exposed a separate encrypted restart guard trying to scan a compacted Raft interval, so it is not counted as snapshot/restart success. |
|
Follow-up controlled matrix for #1290: Java 25, encryption on, three separate nodes, no HTTP leader map, snapshot interval 20, prior append then fresh-table seal workload (15s/rate30/concurrency9/maxTxn4/transfer2s). Alternating 8db0155 versus 8db0155+78cab1ba for three pairs: all six Jepsen histories passed, but all six encrypted process restarts failed on the compacted-prefix startup guard. Seed was not recorded by this workload, so these are matched-parameter repetitions, not identical seeded schedules; this does not establish that #1286 causes or resolves the original acknowledged-append loss. No dependency changes or implementation copies into #1290. I am isolating the distinct compacted-restart guard fix on feature/encryption-compacted-restart-checkpoint, leaving this PR branch untouched. Its scope is verified snapshot checkpoint evidence with fail-closed negative controls, not disabling compaction/rollback guards. |
|
Causal reproduction of the original #1290 append37/key16 failure is now established from the retained WAL, beyond the stochastic matrix. ReadOnly decoding found no version containing37 on any of the three replicas. Replaying the SAME committed WAL from its legitimate index1/term1 anchor into fresh Pebble+real FSMs: entry371/term5 writes key16 list ending43 at commitTS117485124856709124; entry372/term5 attempts a different value ending37 at the SAME key/commitTS (startTS117485124856709122, PrevCommitTS0, 1PC phaseNONE). Baseline8db0155d returns nil for372 while latest value does not match; 8db0155+thisPR78cab1ba returns write conflict for372. This identifies the distinct-entry exact-timestamp replay false-success fixed here as the cause of the captured37 loss. No runtime implementation will be duplicated in #1290. I will add only a new non-overlapping regression file kv/fsm_dynamodb_timestamp_collision_test.go in an isolated worktree based on this HEAD, using the captured request shapes; existing files and your worktree remain untouched. Guard work remains separate in #1293. No force-push/merge/deploy. |
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 (d23ab8a):
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. |
|
Added only kv/fsm_dynamodb_timestamp_collision_test.go in signed bootjp commit d23ab8a via normal fast-forward push from an isolated worktree; no runtime implementation or existing file was changed. Minimized from captured WAL entries371/372 (key16, same commitTS117485124856709124, different protobuf item values ending43 vs37, PrevCommitTS0). Baseline8db0155d fails both plaintext/encrypted variants because the second entry returns nil; this HEAD passes both variants with -race -count=10, while preserving legitimate same-entry replay and fresh read/recompute retaining both appends. KV lint: 0 issues. Full retained-WAL replay comparison logs are /private/tmp/elastickv-seal-wal-baseline.log and /private/tmp/elastickv-seal-wal-with1286.log; no keys were output/published. @codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
|
Current-head d23ab8a code receipt says no major issues and CI is green; paginated review roots contain no original_commit_id matching this HEAD. The mutable summary nevertheless reports findings and security details still loading, without a current-head security receipt or actionable finding. Please refresh the current-head security result; old-head root comments remain stale under the review protocol. No runtime ownership or implementation changes are proposed here. @codex security review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Current HEAD is still d23ab8a with successful CI, a current-head no-major-issues receipt, and completed clean security review. The refresh code-review summary has remained running since16:28:55Z for over30min; paginated current-head root inline findings remain empty. Please complete or refresh the unchanged HEAD review; no duplicate PR or additional runtime changes. @codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Four-source current-head checkpoint at d23ab8a: latest individual code receipt says no major issues (17:05:24Z), refreshed security receipt reports no issues, every CI check succeeds, and paginated root inline original_commit_id==HEAD count is0. Review bodies and older issue-style review findings refer to older SHAs and remain stale under the requested protocol. The mutable summary still sets historical unresolved-finding flags despite the clean current-head receipt; no actionable current-head finding was returned by any source. No duplicate PR, runtime edits, merge or deploy performed. |
Summary
setup!call.Failures and fixes
The scheduled Jepsen run acknowledged an append of
44on list 23, then a later successful read returned the list with45but without44. The history shows a leader change during these operations. Regression tests reproduce distinct-entry, same-batch, and retry timestamp collisions.The stricter timestamp fence exposed a separate filesystem Jepsen setup race: multiple local clients initialized the same FUSE register files concurrently. Their writes overtook one another at Raft apply, and the fence returned write conflicts that FUSE surfaced as
Resource temporarily unavailable. A shared setup lock prevents these self-conflicts; each new setup still resets register values as the existing test requires.Verification
go test ./storego test ./kv -skip '^TestShardedCoordinator_RenewHLCLeases_SkipsInFlightGroupWithoutBlockingPeers$'go test ./kv -run '^TestShardedCoordinator_RenewHLCLeases_SkipsInFlightGroupWithoutBlockingPeers$' -count=1go test ./adapter -run '^(TestListPushDedup_|TestItemWriteDedup_|TestRedis_MultiExecAtomic|TestRedis_RPushLRange|TestXAddDedup_)' -count=1go build ./internal/filesystem/fuseadaptergolangci-lint run ./store/... ./kv/... ./adapter/...(0 issues)78cab1ba: all steps passed with encryption off and on, including filesystem overwrite and rename workloads.Remaining risk
Exact timestamp plus matching values cannot uniquely identify a retry if two independent attempts write identical bytes. Adapter-side self-conflict probes also use exact timestamp existence. The unfenced raw-write batch path still treats equal
(key, timestamp)versions as replays within an entry. A durable attempt identity and a separate raw-write replay rule are needed to close those cases.The repository-wide macOS run cannot compile the Linux FUSE test (
Statfs_t.Namelen); the same unrelated error blocks the local commit hook. The fullkvsuite twice hit a one-second lease-warmup timing condition; that test passed on its own, and the remainingkvtests passed when it was excluded.