Skip to content

fix: make no-op deleteBefore advance cursor in commitQCState and simplify deleteBefore API#3122

Merged
wen-coding merged 1 commit into
mainfrom
wen/advance_next_commit_qcs_persist
Mar 26, 2026
Merged

fix: make no-op deleteBefore advance cursor in commitQCState and simplify deleteBefore API#3122
wen-coding merged 1 commit into
mainfrom
wen/advance_next_commit_qcs_persist

Conversation

@wen-coding

@wen-coding wen-coding commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • In no-op mode (persistence disabled), commitQCState.deleteBefore returned early without advancing s.next. When MaybePruneAndPersist was called with both an anchor and commitQCs, the anchor's persistCommitQC would fail with "out of sequence" because the cursor was stale.
  • Fix by ensuring the cursor advances regardless of whether a WAL is present.
  • Remove the redundant idx parameter from deleteBefore — in production, idx was always derived from the anchor's CommitQC.Index(). The new signature is deleteBefore(anchor *types.CommitQC).
  • Refactor tests to go through the production API (anchors) instead of internal truncation-only calls. Crash recovery tests now simulate crashes via WAL file manipulation rather than calling internal methods directly.
  • Improve no-op tests for blocks and commitqcs persisters to exercise prune-then-persist paths that would have caught this.

Test plan

  • TestNoOpCommitQCPersister persists QCs, prunes with a future anchor, then appends more — verifies cursor advances correctly in no-op mode
  • TestNoOpBlockPersister exercises multiple MaybePruneAndPersistLane calls verifying afterEach callbacks fire in no-op mode
  • TestCommitQCDeleteBeforePastAllCrashRecovery and TestCommitQCDeleteBeforeWithAnchorRecovers now simulate crashes via clearCommitQCWAL instead of internal API
  • All existing persist tests pass

@github-actions

github-actions Bot commented Mar 26, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedMar 26, 2026, 3:43 PM

@codecov

codecov Bot commented Mar 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 58.58%. Comparing base (78c53dd) to head (e3e6b07).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...t/internal/autobahn/consensus/persist/commitqcs.go 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3122      +/-   ##
==========================================
- Coverage   58.58%   58.58%   -0.01%     
==========================================
  Files        2099     2099              
  Lines      173525   173522       -3     
==========================================
- Hits       101657   101654       -3     
  Misses      62805    62805              
  Partials     9063     9063              
Flag Coverage Δ
sei-chain-pr 75.11% <83.33%> (?)
sei-db 70.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...t/internal/autobahn/consensus/persist/commitqcs.go 77.38% <83.33%> (-0.78%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wen-coding wen-coding force-pushed the wen/advance_next_commit_qcs_persist branch 3 times, most recently from 1335a24 to 82edd6d Compare March 26, 2026 15:11
In no-op mode (persistence disabled), deleteBefore returned early
without advancing s.next. When MaybePruneAndPersist was called with
both an anchor and commitQCs, the anchor's persistCommitQC would fail
with "out of sequence" because the cursor was stale.

Fix by separating the idx==0 early return from the iw.Get() call so
the cursor advances regardless of whether a WAL is present.

Also improve no-op tests for all three persisters (blocks, commitqcs,
globalblocks) to exercise prune-then-persist paths, and add an early
return to indexedWAL.TruncateAll when already empty.

Made-with: Cursor
@wen-coding wen-coding force-pushed the wen/advance_next_commit_qcs_persist branch from 82edd6d to e3e6b07 Compare March 26, 2026 15:42
@wen-coding wen-coding changed the title fix: no-op deleteBefore not advancing cursor in commitQCState fix: make no-op deleteBefore advance cursor in commitQCState and simplify deleteBefore API Mar 26, 2026
@wen-coding wen-coding added this pull request to the merge queue Mar 26, 2026
Merged via the queue into main with commit 381e3c3 Mar 26, 2026
41 checks passed
@wen-coding wen-coding deleted the wen/advance_next_commit_qcs_persist branch March 26, 2026 20:39
jewei1997 pushed a commit that referenced this pull request Mar 30, 2026
…lify deleteBefore API (#3122)

## Summary

- In no-op mode (persistence disabled), `commitQCState.deleteBefore`
returned early without advancing `s.next`. When `MaybePruneAndPersist`
was called with both an anchor and commitQCs, the anchor's
`persistCommitQC` would fail with "out of sequence" because the cursor
was stale.
- Fix by ensuring the cursor advances regardless of whether a WAL is
present.
- Remove the redundant `idx` parameter from `deleteBefore` — in
production, `idx` was always derived from the anchor's
`CommitQC.Index()`. The new signature is `deleteBefore(anchor
*types.CommitQC)`.
- Refactor tests to go through the production API (anchors) instead of
internal truncation-only calls. Crash recovery tests now simulate
crashes via WAL file manipulation rather than calling internal methods
directly.
- Improve no-op tests for blocks and commitqcs persisters to exercise
prune-then-persist paths that would have caught this.

## Test plan

- `TestNoOpCommitQCPersister` persists QCs, prunes with a future anchor,
then appends more — verifies cursor advances correctly in no-op mode
- `TestNoOpBlockPersister` exercises multiple `MaybePruneAndPersistLane`
calls verifying `afterEach` callbacks fire in no-op mode
- `TestCommitQCDeleteBeforePastAllCrashRecovery` and
`TestCommitQCDeleteBeforeWithAnchorRecovers` now simulate crashes via
`clearCommitQCWAL` instead of internal API
- All existing persist tests pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants