Skip to content

SDSTOR-22907 craft: leader pre-resolution of unresolved slots before SyncRSCommitLSN - #183

Open
sbinmalek wants to merge 5 commits into
eBay:dev/v6.xfrom
sbinmalek:SDSTOR-22907
Open

sbinmalek wants to merge 5 commits into
eBay:dev/v6.xfrom
sbinmalek:SDSTOR-22907

Conversation

@sbinmalek

@sbinmalek sbinmalek commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Before a CRAFT leader proposes a SyncRSCommitLSN(upto) RAFT entry, it must resolve every slot
<= upto not yet confirmed present or Empty on the quorum -- otherwise the leader could propose
past a hole no one has resolved.

  • Adds CraftReplDev::pre_resolve_slots(upto): computes candidates as the union of known local
    gaps (missing_lsns_ <= upto) and slots beyond this leader's own append frontier that upto
    reaches past (e.g. a client Resolve naming a dLSN this leader never received, or a login
    rs_commit_lsn that is the quorum's max, not this replica's own).
  • Broadcasts via a new CraftPeerFetcher::fetch_from_quorum and aggregates every responding No changes this session
    member's reply order-independently: Empty beats data regardless of which member is processed
    first, a slot no member reports data or Empty for is quorum-lacks-evidence (minted fresh as
    Empty), and a slot some member has real data for is written into this leader's own journal.
    Zero responding members (a documented normal outcome of the interface, distinct from "some
    members responded with no evidence") fails closed rather than minting Empty verdicts with no
    evidence behind them.
  • A single misbehaving member's malformed response (unrequested/duplicate LSN) discards only that
    member's reply, not the whole broadcast.
  • Not yet wired into append()/request_resolution() (both remain stubs) -- that's SDSTOR-22908.

Adds QuorumSlotResponse and a fetch_from_quorum broadcast method to the
existing interface, plus the pre_resolve_slots declaration it backs.
Leader-only pre-resolution of every slot <= upto not yet confirmed present
or Empty, before proposing SyncRSCommitLSN. Broadcasts via
fetch_from_quorum and aggregates responses order-independently (Empty
beats data regardless of which member is processed first), writing
locally-missing data into this leader's own journal and minting fresh
Empty verdicts where the quorum has no evidence.
@sbinmalek sbinmalek changed the title Sdstor 22907 SDSTOR-22907 craft: leader pre-resolution of unresolved slots before SyncRSCommitLSN Sep 23, 2026
@sbinmalek
sbinmalek requested a balanced review from Copilot September 23, 2026 12:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unbounded candidate generation, unsafe failure accounting, malformed-response handling, and concurrent journal writes can violate resolution safety.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread src/lib/craft/craft_repl_dev.cpp
Comment thread src/lib/craft/craft_repl_dev.cpp
@codecov-commenter

codecov-commenter commented Sep 23, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (dev/v6.x@d2e0087). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/lib/craft/craft_repl_dev.cpp 0.00% 1 Missing and 2 partials ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@             Coverage Diff             @@
##             dev/v6.x     #183   +/-   ##
===========================================
  Coverage            ?   49.14%           
===========================================
  Files               ?       19           
  Lines               ?     1229           
  Branches            ?      538           
===========================================
  Hits                ?      604           
  Misses              ?      268           
  Partials            ?      357           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbinmalek
sbinmalek marked this pull request as ready for review September 23, 2026 14:46
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.

3 participants