Repository navigation
adapter: validate S3 multipart snapshots and retain uncertain writes - #1357
Conversation
Signed-off-by: bootjp <contact@bootjp.me>
Signed-off-by: bootjp <contact@bootjp.me>
Signed-off-by: bootjp <contact@bootjp.me>
Signed-off-by: bootjp <contact@bootjp.me>
Signed-off-by: bootjp <contact@bootjp.me>
Signed-off-by: bootjp <contact@bootjp.me>
Signed-off-by: bootjp <contact@bootjp.me>
|
Caution Review failedThe pull request is closed. Walkthrough
Merge Risk:
|
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 (a46fba9):
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ 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 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". |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: a46fba9767
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
Signed-off-by: bootjp <contact@bootjp.me>
Signed-off-by: bootjp <contact@bootjp.me>
|
@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 (51409d9):
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: Didn't find any major issues. Bravo. 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. |
|
Latest HEAD 51409d9 passed both code and security re-review. The fixed earlier security thread is resolved. Both CI S3 artifacts were downloaded and parsed as EDN, with matching run SHA and register/multipart success receipts: off multipart has valid=true, 5 confirmed writes/165 reads, no missing/bad final reads; on has valid=true, 4 confirmed writes/171 reads, no missing/bad final reads. The Go CI failed in the unchanged TestMigrationQuorumPartitionAcceptance: a 63+ MiB moving-range negative probe lost leadership and returned outcome-unknown instead of its expected deterministic size rejection. The same test passes locally 3 times under race (68.321s). This does not prove the CI failure fixed or classify unknown as an abort; the original failure is retained. The complete failed Go job was rerun on the same SHA, without reducing tests or changing assertions. Merge remains gated on its success. |
|
The complete Go rerun failed again at the same 63.5 MiB moving-range probe, returning outcome-unknown after leadership loss instead of the expected typed size rejection. This is now recurrent CI evidence, not dismissed as a one-off. Both failure logs are retained. Merge remains blocked while diagnosing large-entry transport/CPU/I/O behavior; no assertion, payload size, or quorum requirement has been weakened, and no unknown write is treated as a confirmed abort. |
Signed-off-by: bootjp <contact@bootjp.me>
|
@codex review Please review exact new HEAD 1a6a6e2, including security review. Integrated current main 0b06781 and resolved the workflow conflict while keeping S3 multipart/history artifacts AND SQS Standard/FIFO acceptance. All workload counts/timeouts/rates/concurrency/drain settings are unchanged; two unused shell bindings were cleaned up. S3/SQS/FIFO race passes57.158s and the separately selected unchanged large-value TestMigrationQuorumPartitionAcceptance passes25.046s locally; full lint and changed-workflow actionlint/archive tests pass. Earlier 51409d native CI failures remain failed evidence, not a root-cause fix or an abort proof. Keep Draft and require all fresh CI, normal/security reviews on this exact HEAD. Check multipart read-key bounds and candidate retention against the newly integrated transaction-owner routing. |
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 (1a6a6e2):
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: Didn't find any major issues. What shall we delve into next? 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. |
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 (1a6a6e2):
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. |
|
Acceptance for 1a6a6e2: explicit post-push normal/security reviews passed; both ready-triggered reviews also completed at 02:27:34Z / 02:29:18Z. All current-head CI checks pass, including full Go race and unchanged full-length gRPC sequences, both Jepsen encryption modes, lint, build, CodeQL, DCO and TLA. Current-head root inline findings are empty. Downloaded S3 artifacts identify this exact SHA: both original register histories are valid; multipart histories are valid with off7/on3 confirmed writes, 166/181 successful reads, complete final keys and no bad final reads. Independent composition review found no new validated P1/P2. The old advisory originated at a46f and was corrected before this HEAD. The two earlier native quorum failures remain unproven-root-cause failures, not claimed repaired by the green run. This merges only the S3 A1 component; the full audit and async runtime remain incomplete. |
Design: docs/design/2026_09_26_partial_serializable_isolation_audit.md
Summary
Startup-lint prerequisite #1356 has merged; this PR targets main and now includes the reviewed main changes through #1367 (
0b06781b2cfd5cb59e6e66d9fb0acfabe7f93c0b). This PR implements the S3 portion of A1, not the entire isolation audit. The new pushed HEAD is1a6a6e265f2bc94fdd5c7ff66b2654f66866016d(signed, bootjp author/committer). All review/CI gates must match this SHA, not the earlier51409d937bimplementation/security checkpoint.Regression And Validation Evidence
go test -p 2 -race ./adapter ./kv ./store . -count=1 -timeout=30mpassed. It started before the last small candidate changes, so it is not represented as final-HEAD proof. Final committed-source focused tests and published-HEAD CI remain gates.Separate Review Passes
Main integration resolves the Jepsen workflow conflict by preserving both S3
multipart/history artifacts and the added SQS Standard workload. No workload,
timeout, iteration count, rate, concurrency or drain budget was removed/reduced.
Two unused shell bindings in that workflow were removed/renamed without changing
the 90-attempt startup loop. Actionlint passes for the changed per-push and
main-integrated scheduled workflows; archive round-trip/missing-store checks
also pass. A broader actionlint invocation still reports preexisting custom
runner-label/quoting diagnostics in the separate unchanged jepsen.yml, not a
claimed repository-wide workflow-lint success.
Post-integration S3/SQS/FIFO adapter race selection passes57.158s. The known
large-value TestMigrationQuorumPartitionAcceptance is NOT in that name selection;
it was separately run unchanged under race and passes25.046s. Full Go lint is
zero issues. The earlier two CI failures at 51409d9 remain failed artifacts:
leadership loss/outcome-unknown instead of a deterministic size rejection is not
classified as an abort or called fixed by a local pass. Fresh current-head CI
and explicit normal/security review remain mandatory before any ready/merge.
Independent Go Standards/Spec reviews had no remaining findings. Jepsen Standards review had no findings; Spec review found missing successful-history retention, now corrected and re-reviewed. Published-HEAD review and CI are required before merge.
Current-Head Acceptance
For
1a6a6e265f2bc94fdd5c7ff66b2654f66866016d, the explicit post-pushnormal review completed without major findings and the independent security
review reported no security issues. The earlier advisory's original commit
is
a46fba9767350874210931ad372a8a3a98d49469; its relocated inline commitfield is not a new finding against this HEAD.
gRPC consistency sequences and native quorum regression. This successful
run does not establish a root cause or repair for the two previous failures.
s3-acceptance-encryption-offands3-acceptance-encryption-onindependentlyidentify this exact source SHA and both S3 step outcomes as success.
The original register histories have
valid? trueand no failures. Themultipart histories have
valid? true, no linearizability failures, andrespectively 7/3 confirmed writes and 166/181 successful reads. All final
keys are present; both missing-final-key and bad-final-read lists are empty.
ordinary routing, inline and offloaded parts, concurrent replacement,
operational read failures, legacy version-zero metadata and lost successful
replies joined with conflicts. It passed (3.618s) without shared source edits.
adapter race passed (57.158s), and the unchanged native quorum test passed
separately (25.046s). These local passes do not replace published-head CI.
All current-head CI checks, including CodeQL, have now passed. Draft was
marked ready only after those gates passed. The ready-triggered normal and
security reviews completed at 02:27:34Z and 02:29:18Z on the same HEAD;
there are no new root inline findings. The earlier explicit post-push manual
normal/security receipts remain exact-head matches. No code changes or force
push are implied by recording these receipts.
Remaining Scope
This does not close A0/A0b/A2/A6 or the full design-doc goal. In particular, #1355 has a separately reproduced P1 involving a mixed local/replicated watermark and is not a dependency to merge blindly. No deployment or production cluster changes are included.
Author: bootjp