fix(server): truncate torn segment tails instead of resurrecting them - #3946
fix(server): truncate torn segment tails instead of resurrecting them#3946hubcio wants to merge 8 commits into
Conversation
After a crash left a half-written tail on a segment, recovery found the correct end of valid data, but reopening the segment read the raw file length back in and overwrote that result. The garbage tail came back to life, the next restart refused to boot, and replicas copied the bad bytes around. Recovery now bounds every segment without touching disk, checks the chain, and only then cuts torn tails off the files, rebuilding the index when damaged. Damage in the middle of a segment is never cut out silently: the partition is refused, its files set aside, and it is rebuilt from replicas (PartitionRecoveryRefused, replacing PartitionChainRefused and RecoveredSegmentSizeDivergence). Nodes already stuck on this bug can boot again. Writers verify the expected size against the file at open and refuse on mismatch.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3946 +/- ##
============================================
- Coverage 83.85% 82.62% -1.24%
Complexity 1358 1358
============================================
Files 1212 1213 +1
Lines 166843 162695 -4148
Branches 134304 130273 -4031
============================================
- Hits 139905 134419 -5486
- Misses 23298 24258 +960
- Partials 3640 4018 +378
🚀 New features to boost your workflow:
|
github-actions
Bot
added
S-waiting-on-author
Review of the torn-tail recovery found paths where boot could stall or destroy data. The residue probe now refuses anything wider than one maximum message and carries a byte budget, so ordinary zero- padded data cannot stall boot; exhausting either limit refuses recovery instead of truncating. The indexed walk refuses offset discontinuities instead of underflowing the stats counter. A rebuilt index is staged and renamed instead of written in place, so a crash cannot fabricate zero-run entries. A segment with no recoverable bytes is moved aside into the .fenced.N scheme instead of deleted, and at replica count one a refused partition is tombstoned instead of silently served empty. Operator-facing messages and the config notes now match the implemented behavior.
An fsync failure left the cursor already advanced, so the persist retry appended a byte-identical index entry that boot recovery now refuses as non-monotone. Match the messages writer: fsync first, advance after, so the retry overwrites the same slot. Also drop a writer test that passed with and without the size-guard fix.
The code is a post-condition assertion that cannot cross the wire today, so it should not occupy a slot inside a retired-code gap. Allocate above the highest 4xxx code, restore the previous allocation-rule wording, and regenerate the Go and Node tables.
The residue width cap refused the widest ordinary torn tails (a torn flush chunk can span hundreds of megabytes, not one record) and read a live config knob, so lowering max_message_size later would refuse healthy partitions on every replica. The probe now has no width gate: its budget charges one unit per candidate examined, scoped to the whole partition load, keeping work linear in residue regardless of file size. A frozen 256 MiB ceiling bounds max_message_size (new config validator) and every decoded batch header, so a bit-flipped length field can no longer allocate a segment-sized buffer on the boot path. The indexed walk refuses only backward offsets: a forward gap is minted by boot itself when the durable frontier passes the recovered end, so it is absorbed with a warning instead of refusing byte-clean data. The reactor yield now actually suspends: a fixed tiny sleep lost a machine- dependent race against the timer wheel's clock re-read, so a shared helper retries with growing durations until a timer registers, making the first poll suspend by construction.
The tombstone planted for a refused partition at replica count one did not hold. The reconciler consulted the tombstone only for already-routed namespaces, so the next pass rebuilt the partition fresh and clients hung on a routed-but-tombstoned namespace. And because quarantine moved the segment files but not the superblock, the following boot re-seeded an empty partition with no refusal logged at all. The reconciler now skips tombstoned namespaces outright and InsertOwned refuses to route one; the tombstone lifts only through ConfirmRemove, proof the deletion completed. A tombstone verdict no longer quarantines: the refused chain stays in place, so every boot re-derives and re-logs the refusal until an operator intervenes.
A .log with no .index beside it aborted the whole boot. Recovery opened the index unconditionally, and IggyIndexReader::new is a bare read-only open that folds every failure, ENOENT included, into CannotReadFile. That surfaces as a plain ServerError::Iggy, not a PartitionRecoveryRefused, so the shard builder cannot fence the one partition and take the node up without it. The pair is ordinary, not exotic: SegmentStorage::new creates the log before the index, so any crash between the two leaves exactly that shape, as does an operator restore that drops an index. The boot sweep collects every .log stem whether or not an index sits beside it, so such a segment always reaches this open. Nothing new is needed to repair it. A 0-byte index already routes to the index-less walk, which rebuilds the index from the batch headers it verifies. Stat the index through the NotFound-lenient file_len first and skip the reader when the length is zero, so an absent index takes that same path. Every other stat failure still fails stop, which moves the unopenable-index error from CannotReadFile to CannotReadFileMetadata without changing that the boot refuses and leaves the log byte-identical.
The indexed recovery walk absorbed a forward offset gap on the header alone: a crash can legitimately stamp the restored offset frontier into the tail segment, so a gap is not proof of damage. But base_offset is covered by the batch checksum, and an upward bit flip in an unverified header wears exactly the frontier-stamp shape: the walk adopted the garbage offset, poisoning the partition's offset counter at boot, while the same flip downward refused loudly as OffsetDiscontinuity. Checksum-verify the gap-opening batch before adopting its offset; the legit frontier stamp is server-minted and checksums clean. A failing gap batch breaks the walk instead, and the damage probe already classifies what follows: a torn tail truncates, a verifying batch past it refuses as InteriorDamage. Batches that continue the chain exactly stay header-trusted as before.
| }), | ||
| ); | ||
| } | ||
| if header.base_offset > expected_offset { |
There was a problem hiding this comment.
Corrected after reviewing
7a9c4b3f0andc727a1cc9. My original recommendation here was to revert this branch toheader.base_offset != expected_offset. That was wrong — a plain revert is an availability regression, and the checksum gate you added is what makes a revert safe. Corrected recommendation below; the rest of the original finding stands, re-scoped.
The checksum gate narrows this finding; it does not close it.
Confirmed at c727a1cc9 that a bit flip in base_offset now fails decode_batch_slice and truncates cleanly — end_offset = 1, log cut to the 400-byte valid prefix. write_batch_header_fields (batch.rs:452-458) does hash base_offset, so that route is genuinely shut.
But the flip was the cheapest witness of the finding, not the finding. Ran against c727a1cc9: appending a clean encoded_batch(2^63+2, 1) after encoded_batch(0, 2) under index_entry(0, 0) still recovers end_offset = 9223372036854775810 — the identical value, with no checksum corrupted anywhere. calculate_batch_checksum binds base_offset to the batch's own bytes, never to its position in this log, so it can prove self-consistency and cannot prove placement. Any whole intact record that lands in the tail from somewhere else — block recycle, misdirected write, a replay from elsewhere in the partition, an operator copy — verifies and is adopted. And the permanence chain is untouched: that value seeds current_offset (bootstrap.rs:2672-2679 → :2708-2710), and write_superblock's advance-only max (iggy_partition.rs:680-682) makes it survive an operator deleting the offending segment.
Concretely, and independent of the absorption: the walk never compares header.partition_id to the partition it is recovering. A partition-7 batch appended to partition 1's tail recovers end_offset = 9000002. It is adopted with no gap too — end_offset = 3 for a foreign batch that continues the chain exactly, in the indexed arm (header-trusted, never verified) and in the index-less arm (verifies its own checksum, continues the chain). So this does not live in the gap branch. The produce path checks exactly this thirty lines from its decode (server_common/src/send_messages.rs:471, batch.header.partition_id != namespace.partition_id() as u64 → InvalidCommand); recovery — the path that truncates and re-seeds the offset space — is the only reader that skips it. One comparison per arm, on every batch, immediately after peek_header and before the offset split, with a distinct refusal rather than break: a foreign record mid-log is not a torn tail, and preserving evidence is this module's policy for that class. Note it authenticates the partition component alone, not the packed namespace, so it catches partition-7-into-partition-1 but not stream-2/topic-1/partition-1 into stream-1/topic-1/partition-1. Necessary and cheap, not a complete placement authenticator.
The regression arm still refuses without verifying, so one bit gets opposite verdicts by direction. base_offset 101 → 97 gives OffsetDiscontinuity { expected_offset: 101, found_offset: 97, position: 7528 }, a refusal that never lifts at replica_count = 1; the same bit upward truncates cleanly and the node boots. Reproduced independently by two reviewers with matching numbers. The verify you just added is exactly the tool that classifies this — it is applied to one branch only.
The new break also has a third outcome its comment does not name. When the failing gap batch is the first thing past the last index entry, walked_any stays false and the IndexLogDivergence return at :1069 fires before refuse_if_survivor_past_damage ever runs — so neither claimed outcome happens: not the torn-tail truncate, and not the InteriorDamage refusal even with a verifying batch sitting past the damage. Both sub-cases were run; both give IndexLogDivergence { end_offset: 0, indexed_size_bytes: 0 }, whose operator text at server_error.rs:388-391 reads "the {N}-byte log holds no whole batch" about a log that holds a whole batch whose header decoded and whose checksum failed. That wants its own refusal variant. The verdict itself is correct and non-destructive — bytes preserved, nothing truncated — but it is a behaviour change: under cfd2265e1 those bytes were absorbed and served.
Suggested shape — one change rather than four
Hoist the verify above both branches, keep break on failure so the probe classifies the damage, and refuse only a verified discontinuity in either direction:
if header.base_offset != expected_offset {
let verifies = /* slice_at + decode_batch_slice, once, above both branches */;
if !verifies {
break; // damage -> probe classifies
}
return Err(identity.refusal(PartitionRecoveryRefusal::OffsetDiscontinuity { .. }));
}That removes the adoption, removes the direction asymmetry, and leaves the indexed arm's policy identical to the index-less arm's (:1122 break-on-!verifies, :1131 refuse-on-!=) — which closes the two-arms-disagree finding as a side effect. The only newly-truncatable bytes are bytes that failed their own checksum and have nothing verifying after them, which is precisely the torn tail this module exists to truncate. No verified byte becomes truncatable. And a verified regression means genuinely duplicated committed offsets, so that refusal stays and becomes meaningful instead of a coin flip on flip direction.
Why not a plain revert, which is what I originally suggested: refusing any != sends that upward single-bit flip from "truncates cleanly, node boots" to a permanent replica_count = 1 tombstone. On the shipped enforce_fsync = false that is a routine torn tail. A plain revert is strictly worse than the current head on this class — the verify is what makes the revert safe, and hoisting it means the revert keeps the useful half of this commit instead of discarding it.
Two smaller notes
Neither new fixture exercises the property the commit body names. Both flip corrupt[COMMAND_HEADER_SIZE + 4], i.e. blob byte 4, which sits inside frame 0's stored per-message checksum field (send_messages.rs:142-147 writes id at 8..24, deltas at 24..32, lengths at 32..40, leaving 0..8 for the checksum). So verify_and_recompute_batch_checksum returns InvalidMessageChecksum before header.batch_checksum is ever compared — measured InvalidMessageChecksum(17987071420115200071, 17987072171734476871, 5), where a real base_offset flip gives InvalidBatchChecksum(...). Deleting base_offset from write_batch_header_fields would leave both new tests green. base_offset is header bytes 8..16, so flipping inside corrupt[8..16] after encoded_batch stamps runs the per-message pass clean and then trips the batch comparison on base_offset specifically. Two fixtures are needed, since corrupt[8] ^= 0x04 is the downward case and the absorb path needs an upward one.
:325's end_offset - start_offset + 1 and :461's previous.end_offset + 1 are plain adds behind this branch. An absorbed encoded_batch(u64::MAX, 1) panics attempt to add with overflow at :325:44 in debug and wraps to a zero message count in release. That one is no longer flip-reachable — the gate did close it — but it wants checked_add regardless.
What stands verbatim from the original finding
The span count at :325 still over-counts across a verified absorbed gap (end_offset = 1003, messages_count = 1004, for four real messages), with a second site at state_transfer.rs:2563-2568. And walk_segment_payload (state_transfer.rs:762-767) still returns NonContiguous for any gap, forward included, so a segment carrying an absorbed one can never be installed by a peer — spill_transfer_segment:1723 and adopt_staged_segment:1789 fail on it on every attempt, forever. That is the part tightening the gate cannot fix: a more selective gate fires less often, but its output is still a segment shape the rest of the system refuses to handle and has no repair route for.
Also for the record: at replica_count = 1 this branch has no legitimate input at all. The frontier-stamp shape it exists to serve cannot be minted there — all three ProbeAsBackup sites gate on replica_count > 1 (bootstrap.rs:2410, bootstrap.rs:2607, partition_helpers.rs:594 via restarted at :547), and the only advancing frontier writers are state_transfer.rs:2044/:2047. Mid-chain, an absorbed gap is at least caught loudly by the chain guard (Hole { previous_end: 1003, next_start: 2 }); it is the tail segment that reaches service, and the tail is where end_offset seeds current_offset under an advance-only persist.
One route I checked and can rule out, so it does not get chased: superblock rot in offset_frontier is not an entry point. The record carries a trailing XxHash3_64 verified on read (journal/src/superblock.rs:408-415) with ping-pong fallback to the partner slot.
| self.fill_window_at(candidate)?; | ||
| let window_end = self.window_start + self.window.len() as u64; | ||
| while candidate.saturating_add(header_len) <= window_end { | ||
| if !self.budget.charge_candidate() { |
There was a problem hiding this comment.
Blocker: this budget provably cannot fire, so the probe has no cost bound at all — and the term that replaced the old one is uncharged.
Candidates advance strictly monotonically from damage_position + 1 with candidate += 1 as the only mutation, so a probe examines exactly residue − 256 offsets against a limit of 2 × residue. Modeled across every shape and every segment count from 1 to 1024 × 1 GiB: spent/limit == 0.5000, always. UnverifiedResidue is unreachable by any on-disk input, which is why its own test has to pre-spend u64::MAX / 2 to reach the path.
Meanwhile verify_slice (:1517) is charged nothing, and its spill read runs up to MAX_RECOVERABLE_BATCH_BYTES = 256 MiB per qualifying candidate. 0929f697b deleted spent_bytes += read_bytes + total_size, which was the term that bounded exactly this. The doc at :1478-1481 says verify slices are "already bounded by the strictly-forward window advance and the plausibility cap on claimed sizes" — that sentence is true of refills and false of verifies: verify_slice preads at an arbitrary position into spill and never touches window_start, and the cap bounds each read's size, not their number.
Measured on this branch:
| shape | read |
|---|---|
| 8 MiB residue, header every 248 B claiming 6 MiB | 49.5 GiB, 1.842 s (vs 0.012 s zero-filled) |
| 1 GiB residue, headers every 4 KiB claiming 8 MiB | 2,033 GiB |
| 1 GiB residue, 256 B pitch, 256 MiB claims | 786,433× — 768 TiB |
| at the 1.25 GiB segment ceiling | ~810 TiB, ~8.6 h |
The 248-byte pitch is exact and minimal: at period 248 the previous header's zero-reserved region [k+52, k+256) ends precisely at the next header's base_offset field. Producer payload bytes reach the residue verbatim with encryption off by default, so this is admissible input rather than a crafted file.
This compounds with two other properties into something worse than a slow boot. At replica_count = 1 a refusal is now a permanent tombstone that re-derives its verdict on every boot, so an adversarial residue means the node never boots again — re-inflicted on every restart attempt, from one write to the data dir.
Suggested fix — two counters, refills charged by neither:
- Enumeration: keep
charge_candidate, 1 unit per candidate, limit2 × residue. Structurallyresidue − 256against2 × residue= 50% margin at every width, once refills come off it. (Charging refills is what collapsed the honest margin: measuredspent = 2R − 4.19 MiBat every width, where the 4.19 MiB is one scan window of walk-leftover luck — 3.12% margin at 64 MiB, ~0.195% at the ceiling.) - Verification: new counter, charge
total_sizeper handed slice — in-window ones included, since an in-window verify still hashes every message up to the first bad checksum. Grown by the samegrow_for_residuecall, residue-derived so knob-immunity is untouched. Check before the read, so exhaustion never pays for the slice that broke the budget. - Limit: either
4 × residueplusProbeOutcome::BudgetExhausted if chain_end_offset.is_none() => Ok(()), or64 × residuewith no degradation. The degradation is sound becausechain_end_offsetisNoneexactly when the walk proved no batch, and on that segment exhaustion andNoSurvivorreach an identical outcome —bounds == None→recovered_empty→ the pair moves to.fenced.Nand empties are seeded. Bytes preserved either way; refusing there only adds a permanent tombstone. The looser bound avoids the degradation but its constant is calibrated to one synthetic fixture. Both close the channel; worst honest cost at64Ron a 1 GiB residue is 64 GiB of hashing ≈ 1.3 s against 4 TiB / ~88 s uncharged today.
Do not gate fits on SCAN_WINDOW_CAPACITY as a cheaper alternative. That makes a legal >4 MiB survivor invisible, so NoSurvivor is returned and truncate_to deletes it — silent destruction of committed data, reachable on the shipped 64 MiB max_message_size.
One note on the test at :2254: given_zero_padded_records_when_probing_should_scan_whole_residue_and_recover_empty was named ..._should_refuse_on_scan_budget before 0929f697b renamed it and inverted its body. It was the tripwire for this bound, so the rename recorded the bound's removal as expected behaviour. Whatever limit lands, please add a guard that asserts the handed-byte total against a multiple of residue_bytes on a shape with a walked prefix — that is the test whose absence let this through, and without it the next refactor removes the bound again.
| } | ||
| candidate += 1; | ||
| } | ||
| if self.take_refilled() { |
There was a problem hiding this comment.
Blocker: the yield is outside the candidate loop, so every verify spill read in a window lands in one un-preemptible stretch.
take_refilled() is polled here, after the inner candidate while closes. verify_slice sets refilled = true (:1564) but nothing reads it until this point, so a window that performs N spill reads yields once, after all N. Modeled maximum un-yielded synchronous pread64: 256 GiB (bait every 4 KiB, 256 MiB claims), 4 TiB at the 256 B pitch.
Recovery runs in front of BootstrapBarrier with the blocking pool sized at zero (server_common/src/executor.rs:88), so that stretch pins the shard core with signals unserviced — Ctrl-C included. Which means the "preemptible" half of 0929f697b's own title does not hold, independently of the volume problem in my comment on charge_candidate above.
Worth noting both walk arms get this right — :1000 and :1126 check per batch. Only the probe defers to the window boundary, which reads as an oversight rather than a policy.
Suggested fix: also yield inside the candidate loop after a verify that hit the spill path. take_refilled is a mem::take, so the existing outer-loop call and a new inner one cannot double-yield on the same read, and the outer one still covers windows that scanned without verifying.
Two things this fix cannot do, so they are worth separating out:
- Accounting will never close the residual. A single large synchronous
preadis inherently un-preemptible, and this module uses synchronousstd::fsdeliberately. So a bounded budget shrinks how many such reads happen but not how long one takes. The end state that fixes it properly is a chunked verify — stream the checksum through the 4 MiB window instead of spilling — which also drops the per-shard spill high-water from 256 MiB to 4 MiB with no change to which shapes are classifiable. That needs averify_and_recompute_batch_checksumthat accepts non-contiguous input, so it is abinary_protocolfollow-up rather than part of this fix. - Separately,
OFFER_HASH_CHUNK_LENis now mis-tuned. The two yields instate_transfer.rs(:748,:2979) were dead before this PR (sleep(Duration::ZERO), 32 ns) and are now real (11.5–13.0 µs measured). At a 1 MiB chunk that is 1024 yields/GiB against a 20.8 µs hash per chunk — +55% to +63% onverify_state_artifact_yielding, and +39% to +59% on the sender's segment walk. That is a runtime path, not boot-once. Raising it to 4 MiB brings the overhead to +14–16%, keeps the un-yielded stretch a bounded 82 µs CPU pass, and matchesSCAN_WINDOW_CAPACITY. One constant, in a file this PR already touches.
Addendum after
c727a1cc9. The new!verifiesbreak in the indexed arm's gap branch gives this finding one more entry route: shapes that previously absorbed a header-decodable forward gap, and so never enteredrefuse_if_survivor_past_damageat all, now hand it a residue starting at the gap batch. The finding itself is unchanged — same cost, same fix — but the probe is now reachable from the indexed arm too, not only from a walk that ran out of decodable batches.Also measured on the new head, so it is not confused with this finding: the gap verify that
c727a1cc9adds at:1019-1022is charged against no budget either, but it is bounded by construction. Walk positions strictly increase, each verify covers that batch's own extent, andextent > messages_sizebreaks before the slice — so the extents are disjoint and total verified bytes cannot exceed the walked span. That is the opposite of the situation here: the probe advancescandidateby one byte at a time, so its verify ranges overlap, and the overlap is the whole defect. The two are not the same fix, and the walk-side one needs no charge.
| unusable segment chain: {reason}" | ||
| "partition {stream_id}/{topic_id}/{partition_id} at {dir} refused segment \ | ||
| recovery: {reason}. Boot moves the partition's segment files into a \ | ||
| sibling `<partition dir>.fenced.N` directory and keeps them; with peer \ |
There was a problem hiding this comment.
Blocker, and the second iteration this string has been flagged: it is now actively false, and it renders.
cfd2265e1 made the single-replica damage path leave the refused files exactly where they are — the branch at bootstrap.rs:1947-1988 returns before quarantine_segment_files and its own log line says "leaving the refused segment files in place". This sentence still tells the operator boot "moves the partition's segment files into a sibling <partition dir>.fenced.N directory and keeps them".
It is not dead text. recover_partition_segments wraps the call in .map_err(|source| { error!(..., error = %source, ...); source }) at bootstrap.rs:2537-2545, and %source is Display on a thiserror type, so the full #[error] paragraph is formatted for every refusal raised inside load_persisted_segments — which is nine of the ten construction sites, including pass C's storage-open guard. Only hydrate_reopen_error's site escapes it (bare ?, no wrapper). So a single boot logs .fenced.N and "leaving the refused segment files in place" two lines apart, and the one operator-facing string sends them to a directory that does not exist.
Suggested fix: drop the disposition claim from the #[error] entirely and let the bootstrap.rs arms own it — they already log per-outcome and they are the only place that knows which branch ran. If the sentence stays, it has to branch on replica_count and on the quarantine's own success, which is more conditional logic than an error string should carry.
While in here: the tombstone error! at bootstrap.rs:1975-1983 carries no %reason, and it is the line an operator greps to enumerate dark partitions. The rest of the earlier logging ask is satisfied — the headline text is accurate on both branches now and nothing is double-logged.
| // nothing lifts this state short of a metadata commit, and a commit | ||
| // bumps `Streams::revision`, which forces the next pass past the | ||
| // fast-skip. | ||
| if partitions.is_tombstoned(&ns) { |
There was a problem hiding this comment.
Blocker: this gate has no exit, and the rc=1 allowlist routes around it. Both need fixing, and the order matters.
The fence is unliftable through the API. untombstone has exactly one caller — ConfirmRemove (shard/src/lib.rs:2231) — only tear_down_owned_partition enqueues it, and reconcile_removals (:798) iterates partitions.namespaces(), which never contains a namespace that was tombstoned before it was ever materialised. So nothing lifts a boot fence, and nothing deletes the refused files either.
Reproduced: DeleteTopic + CreateTopic on the same ids leaves contains=false tombstoned=true routed=None permanently. Slab keys recycle, so the recreated stream/topic/partition gets an identical ns — meaning a freshly created, empty topic inherits a fence for bytes it never had, and a restart does not clear it because boot re-derives the refusal off the same files. Compare the in-map branch at :560, which at least has has_pending_delete_failure as an escape. The operator's only exit today is moving files by hand.
And the rc=1 rebuild allowlist is an escape hatch from this same fix. bootstrap.rs:1935-1941 exempts Hole and EmptyNonTailSegment from tombstoning at replica_count = 1, on the rationale that "their segment bytes sit intact in quarantine and no damage verdict needs surfacing". Neither verdict establishes that. Hole fires on next.start_offset != previous.end_offset + 1 with both segments holding data; EmptyNonTailSegment fires on previous.size == 0 with data-bearing segments after it. Both then quarantine everything and rebuild empty, so polls succeed with zero messages over bytes that are all present — verbatim the failure bootstrap.rs:1925-1929's own comment gives as the reason the tombstone exists.
git log -S"rebuild_for_rejoin" and -S"EmptyNonTailSegment { .. }" on master..HEAD both return only 2a01e2df3, and master's bootstrap.rs has zero occurrences — so the allowlist is this PR's own code, not pre-existing behaviour.
There is also a route from the guard added at shard/src/lib.rs:2179 into that allowlist: the discard drops a build that already truncated segment 0 (its own comment says so), the next boot walks a 0-byte non-tail segment, that raises EmptyNonTailSegment, and the allowlist rebuilds it empty.
Suggested fix, and please land them in this order:
- First, give the fence an exit: have
reconcile_removalsalso collect namespaces that are tombstoned and absent from bothpartitionsand the committed target, and route them throughtear_down_owned_partition, which already does fence-writes →delete_partitions_from_disk→ConfirmRemove→untombstoneand already refuses a cross-shard delete. The delete only fires when metadata says the partition is gone, so it destroys nothing an operator did not delete. A bareuntombstoneis not enough on its own — it leaves the cause in place, so the next boot re-derives the refusal, and in the window before that the reconciler builds fresh over the refused files, which is the exact truncation this gate was added to stop. - Then narrow the allowlist: at
replica_count = 1, tombstone both verdicts unless every planned segment is size 0. The evidence-based form — gate on the planned chain's recoverable bytes rather than on the refusal's variant — needs the byte total carried on the refusal, which is the same fieldpartition_helpers.rs:657-663already says is needed, so it is a step the code plans anyway. Ship the simple form if that field is out of scope.
Not the other order. Narrowing the allowlist first sends more namespaces into a fence that has no exit, so the tombstone population grows before the door exists.
Separately, and as a follow-up rather than part of this: a tombstoned namespace answers IggyError::TransientNotAccepted (shard/src/lib.rs:2571, via ParkOutcome::Tombstoned). Clients do not hang — the deny path exists precisely to avoid the plane's silent drop — but a terminal, operator-only state is reported as retriable, so every SDK burns its full retry budget per call, forever. zero_out_all also makes the partition read as a healthy empty one through GetTopic and /stats. That wants a terminal discriminant and a counter, which means six SDK mirrors, so it does not belong in this PR.
| /// validation rejects a knob above this at boot; raising it is a | ||
| /// compatibility decision, not a tuning change, since segments written under | ||
| /// a larger value would exceed what recovery on an older build accepts. | ||
| pub const MAX_MESSAGE_SIZE_UPPER_BYTES: u64 = 256 * 1024 * 1024; |
There was a problem hiding this comment.
Blocker: this constant is documented as an invariant, and it is not enforced where batches are actually admitted.
The doc says the ceiling caps "the widest batch record any admission path can persist", and MAX_RECOVERABLE_BATCH_BYTES in segment_recovery.rs is derived from it and used to reject headers. But the new validator only checks message_bus.max_message_size, and the HTTP produce path never goes through the frame decoder:
partition_write_replicatedbuilds the request in-process viabuild_request_message(http/wire.rs:199, called fromhttp/submit.rs:383), soframing::read_message'stotal_size <= max_message_sizecheck (message_bus/src/framing.rs:107-122) never runs.framing::write_messagehas no size check either — only the read side caps — so atreplica_count > 1the primary journals the oversize batch locally and the peer's read rejects the frame, with the bytes already on disk.- There is no batch-total cap anywhere on produce:
SendMessages::validate→IggyMessagesBatch::validate(messages_batch.rs:219-226) checksMAX_PAYLOAD_SIZEper message, never the sum, andSendMessagesOwned::from_messages(send_messages.rs:157) computesbatch_lengthwith no ceiling. http.max_request_sizehas no validator in either validators file and is#[config_env(leaf)], so it is env-settable.
The code already says this outright at shard/src/lib.rs:963-968: "Derived from the BUS frame cap, not MAX_PAYLOAD_SIZE: the server never enforces the latter (its only enforcement sites are the legacy server and the SDK batch types), so the largest appendable batch is whatever the message bus will frame." The HTTP path is not framed by the bus, so on that path nothing frames it.
Reachability is precise and operator-gated, not accidental: JSON bodies carry base64, so raw payload is ~3/4 of the body, and you need http.max_request_size above roughly 342 MiB with five or more messages of ≤64 MB each. Shipped default is 2 MB, so there are three orders of magnitude of headroom. But once past it, peek_header returns None on a legally admitted, checksum-valid batch: the walk breaks and the tail is silently truncated, or InteriorDamage refuses and at replica_count = 1 tombstones permanently.
Suggested fix: validate http.max_request_size <= MAX_MESSAGE_SIZE_UPPER_BYTES at boot. Sound because a produce request carries at most one batch and base64 leaves ~25% slack, so bounding the body bounds the record; http/forward.rs:290-297 re-reads the same key so forwards inherit it. Optionally add a batch-total check beside the per-message one in IggyMessagesBatch::validate — if so, please reuse IggyError::TooBigMessagePayload rather than minting a discriminant, since codes are mirrored in the Go and Node tables and a new one for a config-only failure is not worth the regeneration. Note that validator lives in published iggy_common and the Rust SDK calls it client-side, so it is a behaviour change in a published crate, and that MAX_PAYLOAD_SIZE = 64 * 1000 * 1000 is decimal while everything here is binary MiB.
Either enforce it or drop the "widest batch record any admission path can persist" claim from this doc and from segment_recovery.rs:78-86. Shipping the claim without the enforcement is the part that will mislead the next reader.
Two more things this ceiling needs documented. A deployment currently running above 256 MiB has no non-destructive upgrade path: it cannot boot until the knob is lowered, and once booted recovery treats the wide batches already on disk as implausible and truncates or refuses the segment holding them. Nothing tells the operator to drain and re-produce below the ceiling first. And the validator ordering hides the ceiling error — validators.rs:164-177 runs before message_bus.validate() at :205, so max_message_size = "512 MiB" hits the artifact-floor error first and sends the operator to raise transfer_artifact_bytes_max; only after doing that do they meet the real ceiling, i.e. two boot cycles to learn the edit is impossible.
| the runtime clock is broken or frozen", | ||
| this.attempt | ||
| ); | ||
| this.attempt = (this.attempt * 2).min(YIELD_ATTEMPT_CAP); |
There was a problem hiding this comment.
Blocker: at the cap this is a fixed point, so release builds spin forever with no signal — and the debug_assert! covers the one build where the failure cannot happen.
(YIELD_ATTEMPT_CAP * 2).min(YIELD_ATTEMPT_CAP) == YIELD_ATTEMPT_CAP, so once attempt reaches 1 s the loop keeps allocating a Box::pin per iteration, polling, and retrying — no log, no timeout, on the boot path. Debug panics on the assert instead. Neither is a usable terminal shape: an unyieldable reactor should cost throughput, not the boot.
The assert also names the one condition that cannot trigger it. A frozen clock makes now + attempt > now, so TimerRuntime::insert succeeds and the yield hangs — no assert. Reaching the cap requires the clock to advance by ≥ attempt between sleep()'s Instant::now() and insert()'s re-read, 32 consecutive times with doubling stalls.
Suggested fix — bound the retry and drop the hand-written Future, in one edit. futures and tracing are already server_common dependencies:
const YIELD_MAX_ATTEMPTS: u32 = 32;
pub async fn yield_to_reactor() {
let mut attempt = YIELD_FIRST_ATTEMPT;
for _ in 0..YIELD_MAX_ATTEMPTS {
let mut timer = std::pin::pin!(compio::time::sleep(attempt));
if futures::poll!(timer.as_mut()).is_pending() {
timer.await;
return;
}
attempt = (attempt * 2).min(YIELD_ATTEMPT_CAP);
}
tracing::error!(
"no timer registration won in {YIELD_MAX_ATTEMPTS} attempts; \
continuing without yielding to the reactor"
);
}Identical semantics, one allocation fewer per attempt, ~60 lines shorter, and the terminal case degrades to the pre-PR no-yield behaviour and says so. With the bound there is nothing left to assert about the clock.
The design is right and the mechanism holds — but the module doc's justification is wrong by ~500× and in the wrong direction, and so is the commit message. :26-27 says "a debug-build cold path loses a 1 us head start essentially always". Measured on this branch: a bare sleep(from_micros(1)) registers 19999/20000 warm release, 19998/20000 warm debug, 3000/3000 cold release, and 2994/3000 cold debug — so it wins that race 99.8% of the time. 0929f697b's body cites the same figure. The retry is still worth keeping: at ~512 refills per GiB of walk, a 2e-3 failure rate silently skips a yield per GiB on a debug build, and the skip is invisible because the pass still completes. But please correct the number rather than shipping a rationale that inverts its own measurement. YIELD_FIRST_ATTEMPT's own doc ("on a warm path one microsecond outlives the registration window and the first attempt wins") is correct on the new figures and should stay.
Also worth correcting at :49-51: "the attempted duration does not throttle the caller". Measured µs/yield against the first attempt: 200 ns → 1.7, 1 µs → 11.5-12.6, 10 µs → 11-33, 50 µs → 49-65, 100 µs → 98-104, 500 µs → 485-498. It throttles roughly linearly above 10 µs. Harmless at 1 µs, but someone will raise the constant on the strength of that sentence.
For the record on the code itself, since it is easy to doubt: the first poll returns Pending across 400,000+ calls in debug and release, warm and cold, with zero exceptions, where sleep(Duration::ZERO) was Ready 20000/20000. Failed attempts leak nothing — insert returns None before any wheel entry exists — and poll_timer stores the caller's real waker, so the stored timer's wake reaches the task. The fix works; only its stated reason does not.
| # intact index the walk trusts batch headers (decodable, contiguous offsets), | ||
| # and bytes before the last index entry are not re-examined at boot at all -- | ||
| # at-rest damage there surfaces on the read path via validate_checksum. | ||
| # Damage in the middle of a segment, or trailing bytes too large or costly to |
There was a problem hiding this comment.
Updated after
7a9c4b3f0andc727a1cc9. Two things changed here.7a9c4b3f0adds a second true rebuild trigger — an index that is absent altogether — which makes the:468clause below more wrong, not less, and introduces an operator-visible boot-cost change this block did not mention.c727a1cc9keeps the forward-gap absorption behind a checksum, so Swap A is now the live wording rather than the alternative — folded into the replacement block, revised for the gate. Clause list and replacement block both updated below.
Blocker: four clauses in this paragraph describe behaviour the code does not have. It is the only operator-facing description of a destructive boot path and the repo has no docs/.
:473-474"trailing bytes too large or costly to prove torn, is never silently truncated: the partition is refused" — both halves are dead.0929f697bdeleted the width gate, and the commit's own new testgiven_wide_zeros_residue_when_recovering_should_truncate_at_breakasserts a 512 KiB residue truncates. "Costly to prove torn" is dead too, becauseBudgetExhaustedis unreachable by any on-disk shape (see my comment oncharge_candidate). Note this clause becomes live again if the budget fix lands as aSCAN_WINDOW_CAPACITYrefusal rather than a verify charge, so the wording depends on which fix ships.:470"with an intact index the walk trusts batch headers (decodable, contiguous offsets)" — the indexed arm now absorbs forward gaps while the index-less arm refuses any!=. The asymmetry is documented nowhere operator-facing. Dies if the absorption is reverted.:477-480"the refused files stay at their original paths … re-derived and re-logged on every boot" — not true forStorageSizeMismatch, which is raised in pass C aftertruncate_toalready ran, andbootstrap.rs:1958-1972's own comment concedes the next boot accepts the chain. That tombstone lasts one process, not until an operator intervenes.:480"single-replica refusals with no recoverable bytes at stake (a hole from a stray file, an orphaned empty segment)" — neither verdict establishes that; see my comment on the reconciler gate. Both can fence a fully populated chain and serve it empty.
Also missing entirely: .fenced.N has a second producer on a partition that recovers fine. fence_unrecoverable_segment_files (segment_recovery.rs:742) moves a tail segment whose bytes decode to nothing into the same directory name and recovers that segment empty while the partition keeps serving — one fresh directory per such segment, so .fenced.0, .fenced.1, … accumulate under a healthy partition. That is the one outcome in this feature that loses data and keeps serving, and the block currently teaches the opposite reading of a fence directory. This was asked for last iteration and is still not named.
Plus, new at c727a1cc9 / 7a9c4b3f0:
:468"the index is rebuilt when it was damaged" is now more wrong. It was already wrong about damaged — a damaged-but-nonempty index is floored to whole entries or refuses the partition; a rebuild happens only in the arm reached when the index holds no whole entry.7a9c4b3f0adds a second genuine trigger, absent altogether, which the sentence also does not name. So the set of rebuild triggers grew and the one case the sentence names is still not one of them.- The block says nothing about which arm absorbs a forward gap, and after
c727a1cc9that is the operator-visible part: whether you boot or tombstone depends on whether the index survived the same crash. The index-less walk refuses any offset gap; the indexed walk now refuses only a regression and absorbs a checksum-verified forward gap. - The missing-index boot cost is undocumented. A restore that dropped every
.indexnow boots by re-reading and re-checksumming every byte of every segment and rebuilding each index durably — two fsyncs per segment (stage_rebuilt_index'ssync_allat:684,install_rebuilt_index'sfsync_dirat:712), measured ~6.0 ms per barrier, so roughly 1-2 minutes of fsync alone at 10k segments on top of a full checksummed walk at 46-114 ms/GiB. Before, that shape refused the boot outright. Worth a line, because a boot that looks hung after such a restore is doing exactly this work. (Follow-up for the code rather than the doc: thosefsync_dircalls all target the samepartition_path, so one barrier after all installs would halve them.)
Replacement for :466-484
Written against the head as shipped — i.e. absorption kept behind the checksum gate. If the crew's recommendation on the gap branch lands (verify hoisted above both branches, adoption dropped), swap the third paragraph for Swap C below.
# At boot, segment recovery walks each partition's segments: bytes after the
# last decodable batch of a genuinely torn tail are physically truncated from
# the .log/.index files. A LOST index is rebuilt from the batches the walk
# proves -- lost meaning absent altogether, or holding no whole 24-byte
# entry. An index that still holds whole entries is NEVER rebuilt, only
# floored to whole entries, or the partition is refused when its entries
# contradict the log. Only the walk of a segment whose index was lost
# re-checksums every batch; with an intact index the walk trusts batch
# headers, except that a batch opening a forward offset gap must pass its
# batch checksum before the walk adopts its offset. Bytes before the last
# index entry are not re-examined at boot at all -- at-rest damage there
# surfaces on the read path via validate_checksum.
#
# A restore that dropped every .index therefore boots by re-reading and
# re-checksumming every byte of every segment and rebuilding each index
# durably, two fsyncs per segment: budget minutes, not seconds, on a
# partition holding thousands of segments. A boot that looks hung after such
# a restore is doing that work. Before, that same shape refused the boot
# outright.
#
# The two walks disagree on a forward offset gap, and which one runs is
# decided by whether the index survived the same crash. The index-less walk
# refuses ANY offset gap. The indexed walk refuses only a gap that
# REGRESSES; a forward gap whose opening batch passes its batch checksum is
# absorbed, and the segment's end offset then covers offsets no batch in it
# holds. That checksum proves the batch was minted by a server and not
# altered since -- NOT that it belongs at this position, in this segment, or
# in this partition: the walk does not compare a batch's own partition_id
# against the partition it was found in, so an intact batch that lands here
# from anywhere is adopted along with its offsets. Reported message counts
# for that topic (/stats, GetTopic messages_count) are inflated by the width
# of the gap, and a peer can never install the segment, because state
# transfer's own walk still refuses any gap -- so a partition that boots
# clean with one "forward offset gap" warning is permanently unrepairable
# from a peer. A gap whose opening batch FAILS its checksum is damage: the
# walk stops there and the bytes from that point on go through the damage
# probe like any other residue.
#
# Damage in the middle of a segment is never silently truncated: the
# partition is refused. With peer replicas the refused files are moved to a
# .fenced.N directory beside the partition, which is rebuilt empty and
# refilled from a peer. With replica_count = 1 there is no peer, so nothing
# is moved and nothing is rebuilt: the refused files stay at their original
# paths, the partition is tombstoned, and the same refusal is re-derived and
# re-logged on every boot until an operator intervenes. A tombstoned
# partition is unrouted -- clients get the retriable TransientNotAccepted
# status, never an empty poll that would read as a healthy empty partition.
# One refusal is not durable: a length divergence found when a writer
# reopens a file recovery just truncated clears on the next boot, so that
# tombstone lasts only for the life of the process.
#
# .fenced.N has a second producer, on a partition that recovers FINE: a tail
# segment holding bytes that decode to nothing anywhere is moved there and
# the segment recovered empty, so the partition serves without those bytes.
# One fresh directory per such segment, so .fenced.0, .fenced.1, ... can
# accumulate under a healthy partition. .fenced.N alone therefore does not
# mean a partition was refused -- grep the boot log for "refusing the
# recovered segment chain", which names the directory and the reason.
#
# The metadata WAL truncates genuinely torn tails too; interior WAL damage
# or oversized trailing bytes refuse boot instead.
Swap C — if the gap branch is fixed as recommended (verify hoisted above both branches, adoption dropped, partition_id checked), replace the whole third paragraph with:
# Either walk refuses a batch whose base offset does not continue the chain,
# or whose partition_id is not this partition's. A mismatch whose batch
# fails its own checksum is damage rather than data: the walk stops there
# and the residue goes to the damage probe, so a bit flip in a tail batch's
# offset truncates like any other torn tail instead of refusing the
# partition.
Swap B from the previous round (the rc=1 rebuild allowlist) is unchanged and still applies. The max_message_size items — the :962 bound, the HTTP cap via http.max_request_size, Go's hard 64 MiB const at foreign/go/internal/vsr/header.go:30, and the stale advice at :942 — are untouched by both new commits and still open.
2 participants
After a crash left a half-written tail on a segment, recovery found
the correct end of valid data, but reopening the segment read the
raw file length back in and overwrote that result. The garbage tail
came back to life, the next restart refused to boot, and replicas
copied the bad bytes around.
Recovery now bounds every segment without touching disk, checks the
chain, and only then cuts torn tails off the files, rebuilding the
index when damaged. Damage in the middle of a segment is never cut
out silently: the partition is refused, its files set aside, and it
is rebuilt from replicas (PartitionRecoveryRefused, replacing
PartitionChainRefused and RecoveredSegmentSizeDivergence). Nodes
already stuck on this bug can boot again. Writers verify the
expected size against the file at open and refuse on mismatch.