fix(partitions): reserve offsets before confirming them - #3975
fix(partitions): reserve offsets before confirming them#3975krishvishal wants to merge 9 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3975 +/- ##
=============================================
- Coverage 85.17% 73.13% -12.05%
Complexity 1402 1402
=============================================
Files 1237 1237
Lines 181722 167455 -14267
Branches 148015 133749 -14266
=============================================
- Hits 154783 122463 -32320
- Misses 22882 40961 +18079
+ Partials 4057 4031 -26
🚀 New features to boost your workflow:
|
6a90640 to
d21c33a
Compare
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
the crash shapes reproduce on this branch with curl against a solo node: a crash below the flush threshold then a clean restart re-mints offset 0 or tombstones the partition, and a crash, one send, and a clean restart tombstones on the next boot. inline comments have the details.
outside the diff: init_partition in core/shard/src/lib.rs:3636 arms the floor on every simulator restart with no re-anchor, so the simulator diverges from the server here. the poll ceiling at core/partitions/src/iggy_partition.rs:1788-1806 serves an uncommitted offset 0 before the first commit - pre-existing, but it is how the spent-floor re-mint reaches consumers. the primary apply error at iggy_partition.rs:2676-2688 has no rollback_pipelined_prepare on the partitions plane - pre-existing, this fence is the new trigger. the doc on persist_offset_frontier_at at iggy_partition.rs:1122 still describes the install use it lost.
github-actions
Bot
added
S-waiting-on-author
2bfc229 to
7b66cfc
Compare
|
/ready |
github-actions
Bot
added
S-waiting-on-review
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
round 3 against ccf5adc (head b10db89): 21 of the 22 threads check out, the lease knob tests are the one reply that is not in the push, and the backoff bypass in reserve_offsets_through needs one more change - inline. out of the diff: should_increment_offset is still named in the build_partition_fresh doc (partition_helpers.rs:528) and the shard restore comment (shard/lib.rs:3677), and the install unlink at state_transfer.rs:2172 still drops only .log and .index - unreachable for an anchor today since anchors are solo-only, so hygiene.
github-actions
Bot
added
S-waiting-on-author
b10db89 to
37efb37
Compare
|
/ready |
github-actions
Bot
added
S-waiting-on-review
|
The three out-of-diff items from the review body are fixed too:
All six round-3 items are in the working tree, not in a push yet, so this head still shows none of them. Verification after the six: |
a3de9df to
71a0886
Compare
`restore_offset_frontier` is public, so its intra-doc link to the private `mint_floor` is a hard rustdoc error under `-D warnings`. It failed the pre-merge Rust lane before clippy, machete or any test job got a runner, hiding two more denials behind it: the lease default widened with an `as` cast, and `armed_mint_floor` reads only Copy state, so both trip the pedantic set.
`reanchor_to_offset_frontier` seals the recovered tail and plants the next segment at the restored frontier, betting that "every reader copes" with a hole on a segment boundary. `ensure_contiguous_chain` does not: it walks consecutive planned bounds and demands `end + 1 == next start`, so the boot AFTER the one that stopped the re-mint refused its own chain and tombstoned the partition, which sends then time out against. `offset_reserved` is what separates the two shapes. It already means "offsets this replica may have minted", so a gap ending inside it names offsets no file was ever meant to hold. A gap reaching past the claimed ceiling is still a stray file, and still refuses. Threading the ceiling in put `load_persisted_segments` one argument over the lint ceiling and `load_partition` one line over it, so the namespace triple collapses into the `IggyNamespace` every caller already holds and the offset-counter restore moves out to its own function.
71a0886 to
c701c7b
Compare
|
/ready |
3 participants
The defect
A solo node ACKs sends from its in-memory journal. The client receives a concrete base offset before the threshold-gated flush writes it to a segment. A SIGKILL in this window loses the only record that the offset was issued.
Two messages now share offset 4. Consumers positioned above the reissued range also miss the new messages.
offset_frontieralready existed in the superblock, but stable-view traffic never persisted it because its write gate only ran on view changes.The fix
Add
offset_reserved, a monotonic ceiling on offsets that may have been issued. Before an offset can escape, the append fence reserves its block in the superblock. On boot, minting starts at this ceiling.The reservation covers both primary mints and backup re-stamps. It requires one write per block, not per batch. With the default 64 Ki lease at 100,000 messages per second, this is about three fsyncs per second. A crash wastes at most one block of the
u64offset space.The lease is configurable through
[partition] offset_reservation_lease. A failed reservation rejects the append.Why a second field?
Combining the fields would make valid transfer offers inside the reserved block look like rewinds. The rewind guard must instead compare against stored data: sized segments plus the resident journal. The append counter may be one lease block ahead after recovery.
Segment re-anchoring
A gap inside a segment is not recoverable.
recover_segment_boundsexpects contiguous offsets and truncates everything after a gap, allowing another crash to reissue confirmed offsets.An empty tail that falsely claims a range is removed. A sized tail is sealed, and a new segment is created at the frontier. Graceful shutdown collapses the reservation to the frontier, so only crashes spend offsets.
This applies only to solo groups. A backup rejects prepares whose
base_offsetdoes not continue its counter.Upgrading and rolling back
The superblock record grows from 66 to 74 bytes. Upgrading is transparent: a 66-byte record decodes with the reservation seeded from the frontier it carries. Rolling back is not, since a build that predates the field rejects a 74-byte record, so a downgrade needs the data directory wiped on every node.
Anyone running a build from an EARLIER push of this branch must also wipe their data directory before running this one. Those builds planted re-anchor gaps without the
.anchorrecord the chain guard now requires, so this build reads such a chain as a lost segment and the solo arm tombstones the partition.masterplants no gaps and edge images come only frommaster, so nothing deployed is affected.