Skip to content

Fix snapshot and filesystem integration regressions - #1295

Merged
bootjp merged 4 commits into
mainfrom
feature/ordered-gc-snapshot-fixture
Oct 22, 2026
Merged

bootjp merged 4 commits into
mainfrom
feature/ordered-gc-snapshot-fixture

Conversation

@bootjp

@bootjp bootjp commented Oct 22, 2026 •

Copy link
Copy Markdown
Owner

Changes

  • Write the chunk GC snapshot fixture in encoded-key order; retain the decoder ordering guard.
  • Check cancellation before every lease-reaper iteration, including when a ticker notification and cancellation are both ready. A pre-canceled reaper performs no work; return semantics remain unchanged.
  • Integrate the signed, reviewed commit from jepsen: avoid replaying ambiguous filesystem writes #1296 so ambiguous filesystem Jepsen writes are never replayed over a newer write. The strict checker and lost-register detection remain unchanged.

Integration

These independent main-integration regressions blocked one another in full CI. The fixes are combined here for full CI and manual review on one final pushed HEAD. #1296 remains the source of the single-attempt harness fix; its code is not reimplemented. Existing #1273/#1279/#1291 retain their shutdown and FUSE scope.

Evidence

  • GC fixture: deterministic red before fix; full backup race suite passed 3 times after fix; unsorted-input rejection remains covered.
  • Reaper: new tests reproduced 1 call on a pre-canceled context and 4 calls where only 2 were expected with ready ticks. New and existing reaper race tests passed 100 repeats after fix. All callers audited.
  • Jepsen source commit jepsen: avoid replaying ambiguous filesystem writes #1296: deterministic ordinary-filesystem regression red before fix, then full managed suite passed 237 tests / 676 assertions; actual encryption off/on filesystem CI passed in that PR.
  • Scoped Go lint: 0 issues before integration; combined targeted tests are rerun locally.
  • Full Linux CI and a passing manual review of the final pushed HEAD are mandatory before merge. Local complete Go lint hook has a known toolchain/macOS incompatibility; it is not treated as a successful check.

Author: bootjp

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You'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 21 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 55299af1-c3c2-4d70-9b72-f7939305584d
📥 Commits

Reviewing files that changed from the base of the PR and between 79e135b and 6d9ca91.

📒 Files selected for processing: 5
  • internal/backup/decode_test.go
  • jepsen/src/elastickv/filesystem_workload.clj
  • jepsen/test/elastickv/filesystem_workload_test.clj
  • main.go
  • main_filesystem_reaper_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@bootjp
bootjp marked this pull request as ready for review October 22, 2026 17:44
@bootjp

bootjp commented Oct 22, 2026

Copy link
Copy Markdown
Owner Author

Final pushed HEAD is f48c046; no production behavior or validation guard changes. The original fixture deterministically fails with ErrSnapshotUnsorted; reordering only the two fixture entries makes the full backup race suite pass three times and scoped lint reports 0 issues. Please review this exact final HEAD. CI success and this manual review passing are required before merge. @codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-22T18:04:26.709282Z 6d9ca91 Manual request
🔒 Security Review ✅ Completed 2026-10-22T18:04:56.726046Z 6d9ca91 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: f48c046996

ℹ️ 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".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: f48c046996

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ 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.

@bootjp bootjp changed the title test: keep chunk GC snapshot fixture in key order Fix snapshot and filesystem integration regressions Oct 22, 2026
@bootjp

bootjp commented Oct 22, 2026

Copy link
Copy Markdown
Owner Author

New final pushed HEAD: 6d9ca91. Previous f48c046 review results are stale and will not be used. This HEAD combines ordered GC snapshot fixture, cancellation-before-tick lease reaping (all callers audited, deterministic red then race100 green), and the exact signed/reviewed #1296 harness commit. Combined targeted Go race tests pass; strict filesystem checker, loss detection and snapshot ordering guard are unchanged. Please review this complete integrated final HEAD; all CI must also succeed before merge. @codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 6d9ca91934

ℹ️ 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".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 6d9ca91934

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant