Repository navigation
test: verify DynamoDB snapshot seals with real Raft - #1290
Conversation
|
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 32 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 |
|
@codex review |
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. |
🛡️ 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. |
|
Additional execution evidence at 69f2d93: the final three real-Raft regressions passed five race-enabled repetitions (27.061s). A second three-process Jepsen run with ELASTICKV_RAFT_SNAPSHOT_COUNT=20 passed with six verified leader changes and no failed transfers. All three processes were then stopped and reopened from the same data directories: snapshot indices 401/402/402 survived, the term advanced from 8 to 9, and all 17 DynamoDB items matched the pre-restart scan. Raw histories/results and before/after status+items are retained locally. These are local execution results, not a claim that the queued Linux CI has passed. The unchanged backup renewal test also passed five isolated repetitions after the broad-run deadline failure. |
![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: 69f2d932d0
ℹ️ 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".
|
Addressed P1 history contamination and P2 forwarded transaction locks in signed/verified commit 2205e67. Added isolated-table DB setup, strict reset error handling, wire lock recognition, and item/transaction retry regressions. Target race tests and scoped lint passed; full Jepsen unit suite: 209 tests / 586 assertions passed. Existing append -> seal acceptance on the same three processes passed with six verified transfers; subsequent same-directory process restart preserved all 30 items. Current-HEAD Linux CI remains the merge gate. @codex 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2205e678f7
ℹ️ 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".
|
Signed follow-up 8db0155 normalizes all eleven DynamoDB write dispatch sites: first/subsequent seals, item/transaction commits, schema/migration, and cleanup. This addresses the additional seal-conflict review and the old encrypted CI CreateTable failure. Read-voucher and timestamp selection paths are preserved. Audited all callers; shared SQS predicates unchanged. Full DynamoDB/dedup race tests passed (18.816s); concurrent follower CreateTable with nine clients passed five race repetitions (8.309s); targeted seal/schema wire-retry tests passed; scoped lint: zero issues. @codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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. |
|
Additional stress result, not a pass: at 8db0155, a local encrypted three-process run with snapshot interval 20 and six verified leader changes returned append valid? false (G-single-item-realtime/G1c-realtime), history 20261022T235149.907+0900. The preceding append history and reset completed successfully. Existing PR #1286 addresses a potentially overlapping Raft apply/timestamp issue; I am comparing its existing patch in an isolated diagnostic worktree rather than creating a duplicate implementation. No causal conclusion yet. This PR remains draft and the design remains Partial despite the short CI and current-head review successes. |
|
Final bounded review-loop checkpoint: HEAD 8db0155 is signed/Verified, code+security review have no findings, and all required CI succeeded, including Ubuntu full tests and encryption off/on Jepsen. Keep draft: the additional encrypted snapshot-20 stress failed; the #1286 diagnostic combination passed one workload but failed encrypted restart. Baseline 8db0155 also reproduces that restart refusal on the retained compacted-log data: startup guard scans (6,877] below MemoryStorage first index. PR body now distinguishes all successes, failures, and remaining acceptance. No duplicate core fix or unrelated PR branch change was made. |
|
Final documentation-only commit f432933 records the additional encrypted consistency and compacted-log restart failures in the design itself. Runtime code is unchanged from 8db0155, whose complete CI and code/security review passed. Design remains Partial; no safe-merge/design-complete claim and no duplicate #1286 core implementation. Please review this final evidence reconciliation. @codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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. |
|
Additional stress evidence (no acceptance implementation changes): three alternating pairs at the original 15s/rate30/concurrency9 parameters passed the append checker on both 8db0155 and 8db0155+78cab1ba; all six process restarts still failed on the compacted-history guard. A longer two-pair encrypted matrix (45s/rate60/concurrency12/maxTxn4/leader interval2s, snapshot interval20, no HTTP map) then reproduced consistency failure on one of two baseline runs (G-single-item-realtime/G0-realtime, 20 verified leader changes/no nemesis failures); both combined runs passed their append checkers. Seed was not recorded; these are parameter-matched stochastic repetitions, not identical-seed replay or causal proof. Original append37/key16 failure remains a full-goal blocker. The separate startup guard fix is being developed from main on feature/encryption-compacted-restart-checkpoint; it does not copy #1286 into this PR or authorize skipping legacy compacted history without snapshot evidence. |
|
The original append37/key16 failure now has causal evidence, not just one passing comparison: ReadOnly decoding of all three retained Pebble databases found no version containing37. Replaying the exact committed WAL from its valid index1/term1 anchor into fresh Pebble+real FSMs showed entry371/term5 writes list ending43 at commitTS117485124856709124; distinct entry372 attempts list ending37 at the same key/commitTS, StartTS117485124856709122, PrevCommitTS0. Baseline8db returns nil without storing37; the same replay with #1286 rejects372 with WriteConflict. A minimized regression is added ONLY to existing #1286 (signed d23ab8a), fails plaintext/encrypted baseline and passes -race -count10 with the fix, including legitimate replay and fresh retry. No #1286 implementation/dependency change was copied into #1290. Separate guard PR #1293 preserves fail-closed legacy handling and negative controls. The original fatal stress histories remain retained; this is not a claim that the full design/operational goal or all historical recovery work is complete. |
|
Signed test-only follow-up a0f5506 separates two real-Raft delayed-write checks: an old commit watermark must conflict (global fence may reject it), and a stale read plan with a fresh post-seal commit timestamp must specifically conflict on the seal key. Neither assertion is removed. This catches seals independently after #1286 integration; no runtime implementation or dependency was copied. Standalone leader-change regression passed five race repetitions; full SnapshotSeal selection passed three race repetitions; isolated #1286+#1290+#1293 targeted race suite passed and scoped adapter lint has zero issues. Separately, three encrypted process stress runs (45s/rate60/concurrency12/maxTxn4, snapshot20, no HTTP leader map) each passed append checking with 20 verified transfers/zero failures, then all-three-process in-place restart preserved 39/63/32 items respectively. Evidence: /private/tmp/elastickv-seal-stability-UySI2q/{results.jsonl,1.log,2.log,3.log}. These bounded integration runs do not erase original failing histories or prove production stability/legacy snapshot recovery. @codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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. |
|
Extended bounded integration campaign also passed: two encrypted 3-process runs at 120s/rate60/concurrency12/maxTxn4/snapshot20/no HTTP map, with 51 and49 verified transfers respectively, zero nemesis failures, append valid, then same-directory all-process restart preserving62 and79 items. Combined with the previous three45s runs this is five successful local stress/restart repetitions and160 verified transfers, not proof of production long-term stability. Runtime combination remains isolated (#1286 d23ab8a + acceptance + #1293 753e121); no base/dependency changes. Seed not recorded. Evidence: /private/tmp/elastickv-seal-stability-OEaJJg/results.jsonl, /private/tmp/elastickv-seal-jepsen-kLFC9P/jepsen.log, /private/tmp/elastickv-seal-jepsen-hfgaOD/jepsen.log. Original red histories remain retained. |
|
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. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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. |
|
Final signed HEAD 4c51221 incorporates main b086203, including the reviewed encryption write budget and FUSE create/open fixes. This is an actual main merge; no empty CI-refresh commit or check-status override was used. Snapshot seal/write-error and encryption budget race tests passed twice (adapter 10.612s, encryption 1.882s); diff lint reports 0 issues. GitHub verifies bootjp/contact@bootjp.me. Previous dcd7bf9 review is stale for this HEAD. @codex review |
|
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. 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 disposition
Current HEAD
4c512215c53c43382f4845776d221108ce26efa9incorporates reviewed main throughb086203e, including #1286, #1293, #1279 and #1233. The final merge commit is signed/GitHub Verified as bootjp contact@bootjp.me. Snapshot-seal/write-error and encryption-budget race tests passed twice (adapter 10.612s, encryption 1.882s); differential lint reports zero issues after integration. Final-head CI and the explicitly requested post-push code review are required before ready/merge; prior-head approvals are not treated as current approval. The acceptance-specific runtime changes remain those already described below. The managed CodeQL check on the previous head remained in progress despite its workflow reporting success, and GitHub rejected both job/workflow retry requests; no check-status override or empty commit was used.This is not a design-completion or production-stability claim. Additional encrypted snapshot-interval-20 stress found a consistency failure outside the short successful historical CI. Exact committed-WAL replay identified the same-key/same-commitTS false-replay bug fixed by #1286; #1293 separately fixes the compacted-log encryption startup guard using verified snapshot evidence. Both prerequisite fixes are now merged and included through main. The historical failures and bounded integration results below are retained; recovery of legacy snapshots without witnesses and operational rollout remain separate work.
Scope and dependency
The documentation prerequisite #1287 and seal protocol #1259 are merged. The base is now
main; this PR does not reimplement their protocol. The runtime prerequisites #1286 and #1293 are also merged and incorporated through main.Rechecked all 36 open PRs and the 35 most recent merged PRs before publishing. No other seal acceptance PR exists. #1229 touches a different item-write section; its admin implementation is unchanged. Parent/other work in #1090/#1222/#1224/#1232/#1245/#1280 and shared test support is not modified.
Changes
2205e678: resetjepsen_appendonce in DB setup before any clients recreate it, so preceding append histories cannot contaminate the seal check. Reset failures fail setup closed. Addressed P2 with strict forwarded-lock recognition and actual item/transaction retry regressions.8db0155daddresses seal-dispatch normalization and the encrypted CI CreateTable conflict: all eleven DynamoDB write dispatch sites now normalize known wire failures, including schema/migration/cleanup. Eight read-timestamp calls share one wrapper without changing voucher behavior; leader-issued seals and raw cleanup retain their original dispatch paths. Added wire seal conflict/lock retries, injected CreateTable conflict recovery, and nine-client real-Raft follower table setup (five race-enabled repetitions passed).Audited all callers using
rg: threecommitItemWritecallers, onerunTransactWriteAttemptcaller, and runtime configuration'sbuildLeaderDynamocaller.The follow-up audit covers every DynamoDB write dispatch: eight read-timestamp calls, one leader-issued seal, and two direct cleanup calls. No shared SQS predicate or Raft timestamp semantics were changed.
Verified locally
go test -race ./adapter -run '^TestDynamoDBSnapshotSealRealRaft': three regressions passed; five-repeat run passed. The leader-change test subsequently passed ten race-enabled repeats after fixing its wait for leader propagation.go test -race ./adapter -run 'Test(ItemWriteDedup|DynamoDB|OnePhaseDedup)' -count=1: passed.go test . -run '^TestBuildLeaderDynamoWithoutHTTPMap$' -count=1: passed.lein -o test: 209 tests, 586 assertions, zero failures/errors after initializing the existing Redis submodule, including table-reset regressions.valid? true. History:20261022T231204.299+0900retained in the acceptance worktree's Jepsen store.69f2d932: snapshots 401/402/402 retained across all three processes reopening from their original directories; term 8 -> 9, all 17 items unchanged.20261022T233318.774+0900and20261022T233334.075+0900. The new reset setup prevents history contamination.Not claimed / pending
Statfs_t.Namelencompile failure, an unrelated backup renewal deadline failure, and a transient vet failure from editing the test during that broad run. The latter is corrected and covered by subsequent target execution. Full Linux CI is required.f432933c: Ubuntu race tests, Jepsen off/on, lint, build. These short runs do not override the additional stress failure below.8db0155dalso is not a pass: an unchanged SQS FIFO visibility test failed under the full load. It passed 20 isolated Linux repetitions (32.077s). The full Ubuntu CI separately passed. SQS code was not changed to hide this result.Outstanding acceptance findings
8db0155d, snapshot interval 20, preceding append history followed by a fresh table reset, 15-second seal workload / rate 30 / concurrency 9 / six verified transfers: overallvalid? false,G-single-item-realtime/G1c-realtime. History20261022T235149.907+0900is retained with logs under the acceptance Jepsen store and/private/tmp/elastickv-seal-jepsen-FpXSbX/. An acknowledged append 37 to key 16 was absent from a later successful read that included 46. No success is claimed for this run.78cab1ba) with8db0155dpassed one identical encrypted workload with six verified transfers. This suggests overlap, not a causal proof or completed stress campaign. It was built in a separate uncommitted diagnostic worktree; no Fix lost Redis appends during Raft leader handoff #1286 implementation was copied into this PR or modified. Evidence shared on #1286.(6, 877]from MemoryStorage after compaction and refused startup. The final8db0155dbinary also reproduces that startup-guard failure on the retained diagnostic data, without the Fix lost Redis appends during Raft leader handoff #1286 patch. Logs remain under/private/tmp/elastickv-seal-jepsen-dscUKN/. Plaintext restart successes above must not be generalized to this encrypted case.Follow-up diagnosis and bounded integration
/private/tmp/elastickv-seal-stability-UySI2q/{results.jsonl,1.log,2.log,3.log}. Seeds were not recorded; this is parameter-matched stochastic evidence, not identical-seed replay or production stability proof./private/tmp/elastickv-seal-stability-OEaJJg/{results.jsonl,1.log,2.log}. Five successful local integration stress/restart repetitions total160 verified transfers; no production long-term stability claim follows.Historical acceptance and #1293 code/security reviews and CI were successful. Runtime fix integration through their normal PR prerequisites is now complete. Final-head review/CI must still pass before merging this PR; operational rollout and legacy recovery remain separately unproven. The overall design/operational acceptance is not complete.
Author: bootjp