Implements an mmap backend for pack files and benches against direct IO. - #1269
Implements an mmap backend for pack files and benches against direct IO.#1269sstanfield wants to merge 45 commits into
Conversation
GitHub Actions
Waiting
GitHub Actions
Waiting
| /// Split one bucket (in modulus order) into itself plus a newly appended bucket, rehashing and | ||
| /// redistributing its elements. The split bucket is overwritten in place; the new bucket is | ||
| /// written at the current file end (which the random-write mmap extends). | ||
| fn split_one_bucket(&mut self) -> Result<(), AppendError> { |
There was a problem hiding this comment.
[MEDIUM, latent] split_one_bucket destroys the split bucket in place before repopulating it
The split zeroes the live bucket through the mapping (which also clears its 8-byte overflow pointer and unlinks the whole odx chain), then re-inserts the collected elements one at a time and stamps the CRC last. Two windows the buffered HdxIndex does not have (it builds complete CRC'd buffers and writes each as a unit):
- Error path: if any
save_to_bucket_bufferin the redistribution loop fails (for example ENOSPC or EIO on the odx overflow append), the function bails with the bucket already zeroed and its chain unlinked. Every digest in that bucket is permanently gone, and no heal path rebuilds an hdx. - Crash window: dirty pages can be written back at any moment between
fill(0)and the trailingadd_crc32. Power loss there persists a CRC-invalid bucket, and every later lookup that hashes there becomes a hardFetchError::CrcFailed.
Fix direction: build both replacement buckets in scratch buffers (CRC included), do the odx appends first, then memcpy the finished buckets into the mapping.
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
There was a problem hiding this comment.
Round-2 review at head e765364 (round 1 was at 219394e). First, credit where due: this revision addressed the previous round decisively.
Previous 7 findings: 4 fixed, 2 obsolete, 1 residual. The HIGH pdx-phantom/trunc_and_heal wipe is properly closed: recover_pack discards indexes wholesale and rebuilds from the pack's own records, never deriving a truncation target from an index. The split clear-before-collect hazard, the try_clone EOF erosion on the live serve path (now bounded by consensus_output_end + .take(cap)), and the per-serve overalloc are all fixed; PackFileIo for File and ConsensusPackDirect are gone; the panic in split redistribution now returns Err. The remaining residual is the comment-only single-writer/SIGBUS assumption (see inline comment on data_file.rs).
The merge of main is faithful. I checked the ~840 hand-resolved lines in consensus_pack.rs from merge 9740a8f line-by-line in both directions against both parents: nothing from main's #1226/#1230 work was dropped or weakened, and nothing of the mmap restructure was lost. The only novel line is a forced mut drop.
Crash design is much stronger, with one blocking gap. ordered_sync (data before marker), the WAL rebuild, and the zero-pad record framing are sound; I verified 13 crash windows that are handled correctly. The blocker is the HIGH inline comment: tail_is_torn treats the ordinary mmap power-loss shape (torn record followed by an intact one, from unordered page writeback between msyncs) as fatal CorruptPack, so a node can fail to start after a plain power cut. That shape is new with the mmap default; I would not flip the default until it heals.
Also worth resolving before merge: the PR body still argues against switching to mmap ("not a clear win" on the Linux SSD bench) while the branch now makes mmap the only backend; the description and the default-flip rationale should match the code.
7 inline comments follow: 1 high, 3 medium, 3 low.
MavenRain
left a comment
MavenRain
left a comment
There was a problem hiding this comment.
Prior comments appear to have been acknowledged and/or addressed
| /// grown region survives a crash (`msync` alone does not persist size growth). | ||
| fn grow_to(&mut self, new_cap: u64) -> io::Result<()> { | ||
| self.remap(new_cap)?; | ||
| self.file.sync_all()?; |
There was a problem hiding this comment.
grow_to makes the size extension durable before any of the new bytes exist, and I think that leaves a shape no open path can heal.
fn grow_to(&mut self, new_cap: u64) -> io::Result<()> {
self.remap(new_cap)?; // -> file.set_len(new_len) at :352
self.file.sync_all()?; // <-- FSYNC, size now durable
self.flushed_end.store(self.end, Ordering::Relaxed);
Ok(())
}On a fresh file open_with leaves a 0-length file unmapped (:217-218), next_capacity floors the first allocation at DEFAULT_INITIAL_SIZE = 1 << 20 (:80, :315-324), and Write::write calls ensure_capacity at :671 and only then copy_from_slice at :674. So the first write always executes set_len(1 MiB) -> fsync -> memcpy, and at the instant the fsync returns the file on disk is 1 MiB of zeros.
An all-zero header never passes its CRC: crc32 of an all-zero payload is never zero (22 zeros -> 0xa829b1e0, 24 -> 0xa3c1ca20, 64 -> 0x758d6336), while the stored trailer reads 0. All four header loads therefore fail — DataHeader (pack.rs:667-669), PdxHeader (position_index/index.rs:56-58), HdxHeader (digest_index/index.rs:116-118), OdxHeader (odx_header.rs:118-120).
The part that worries me is that there is no recovery path for it. Every "is this file brand new?" dispatch is keyed on length, and length is exactly what this fsync makes durable: pack.rs:407 (if file_end == 0), position_index/index.rs:149-150 (is_empty()), digest_index/index.rs:333-335, odx_header.rs:44-46 — all four take the else branch on a 1 MiB zero file. PackError::is_missing_static_files (consensus_pack.rs:2199-2210) matches only LoadHeaderError::IO(NotFound), so CrcFailed is never collapsed to "absent"; the error propagates through ConsensusChain::new (consensus.rs:350) to EpochManager::new (crates/node/src/manager/node.rs:600) and aborts node startup with no remediation message. recover_pack's delete-and-rebuild is unreachable here too — the indexes are opened at consensus_pack.rs:878, :945-946, :991-993, all before recover_pack runs at :958/:996.
On main this shape could not occur. git show c703dfb8:...data_file.rs shows the old DataFile was a File opened .append(true) with a 16 KiB Vec<u8> write buffer: no set_len, no pre-sizing, no fsync anywhere in the write path. The physical length always equalled the bytes actually written, so the same crash left a 0-length file that every open path re-initializes.
Two scoping notes on my own claim, so this is not overstated:
- It needs a machine/kernel crash or power loss, not a process crash. Dirty
MAP_SHAREDpages live in the page cache attached to the inode and survive SIGKILL. - The hdx file is incidentally safe, because writing the 2 MiB bloom (
digest_index/index.rs:344) forces a secondgrow_towhose fsync flushes the already-dirty header page.
The exposure windows differ a lot, though. The data file is exposed only until the first commit(); the pdx and odx are exposed for the whole epoch, because their create sites call fsync_directory (position_index/index.rs:158, odx_header.rs:54-56), which makes only the directory entry durable and never the file contents, and Inner::persist never syncs indexes.
Could we move the fsync so the size never becomes durable ahead of the content it was made for? Please don't just delete it — I chased the msync-vs-fsync question separately and the msync-default design is sound precisely because this fsync is what makes i_size durable.
// data_file.rs
/// Grow the file to `new_cap`. The size extension is deliberately NOT fsync'd here: the caller
/// runs [`Self::commit_growth`] *after* the new bytes are in the mapping, so the file's length
/// never becomes durable ahead of the content it was extended for. A crash before that leaves
/// the file at its old (possibly 0) length -- the state every open path already heals -- instead
/// of a longer, all-zero file whose header fails its CRC with no recovery path.
fn grow_to(&mut self, new_cap: u64) -> io::Result<()> {
self.remap(new_cap)
}
/// Persist a size extension together with the bytes written into it: `msync` the dirty region
/// first, then `fsync` (which alone persists the new size/metadata).
fn commit_growth(&mut self) -> io::Result<()> {
self.flush_dirty(true)?;
self.file.sync_all()?;
self.flushed_end.store(self.end, Ordering::Relaxed);
Ok(())
}
/// Returns `true` if the file was grown (the caller must then `commit_growth` after writing).
fn ensure_capacity(&mut self, needed: u64) -> io::Result<bool> {
if self.remap_needed.load(Ordering::Relaxed) {
self.capacity = self.end;
self.remap_needed.store(false, Ordering::Relaxed);
}
if needed <= self.capacity {
return Ok(false);
}
let new_cap = self.next_capacity(needed);
if self.opts.grow_mode == GrowMode::Segment && new_cap > self.opts.max_map_size {
return Err(io::Error::new(
io::ErrorKind::Unsupported,
"segment mode not yet implemented: mapping reached max_map_size",
));
}
self.grow_to(new_cap)?;
Ok(true)
}// data_file.rs -- Write::write (:671)
let grew = self.ensure_capacity(start + n)?;
let start_us = start as usize;
match &mut self.backing {
Backing::Rw(map) => map[start_us..start_us + buf.len()].copy_from_slice(buf),
_ => return Err(io::Error::other("no writable mapping")),
}
match self.opts.write_mode {
WriteMode::Append => self.end += n,
WriteMode::Random => { self.seek_pos += n; self.end = self.end.max(self.seek_pos); }
}
if grew {
// Size extension + its content land together; the file is never durably longer than
// the data it holds.
self.commit_growth()?;
}
Ok(buf.len())The same grew/commit_growth pattern would apply in ensure_len (:406), where the zero-extension is the intended content.
As defence in depth, it would also help to make each freshly written header durable before returning, so a reopen sees either a 0-length file (self-heals) or a valid header. sync_disk() currently has no production caller at all; calling it after the header write at pack.rs:409, position_index/index.rs:155, digest_index/index.rs:357, and in open_odx_file_mmap after odx_header.rs:93 would give it a legitimate one.
Test-wise, a regression test that builds the padded shape (set_len(1 MiB) on a fresh file) rather than the physically-exact shape used by test_open_append_heals_dataless_torn_first_record (consensus_pack.rs:3753) would cover this.
| // Opened ahead of the meta check: the torn-record heal below needs the position index as | ||
| // an independent witness of whether any consensus output was ever committed here. | ||
| let mut consensus_pos_idx = Self::open_pdx_file(&base_dir, data.header(), false)?; | ||
| let consensus_pos_idx = Self::open_pdx_file(&base_dir, data.header(), false)?; |
There was a problem hiding this comment.
I do not think this "independent witness" ever gets consulted after an unclean shutdown — first_record_is_dataless_tear looks dead in exactly the case it was written for.
// consensus_pack.rs:839-843
let ends_inside_first_record = matches!(
data.record_size(DATA_HEADER_BYTES as u64),
Err(FetchError::IO(ref e)) if e.kind() == io::ErrorKind::UnexpectedEof
);
ends_inside_first_record && consensus_pos_idx.is_empty()record_size -> record_size_bytes (pack.rs:426-460) can only yield UnexpectedEof when data_file.slice(..) returns None, i.e. the requested range exceeds end (data_file.rs:277-279). After a crash the data file is padded to capacity, so slice always succeeds and a torn first record surfaces as FetchError::CrcFailed (or RequestedSizeTooLarge) instead. The left operand is false, so the heal never fires — regardless of what the pdx says.
open_append then returns PackError::EpochLoad (:904-916), whose message tells the operator to stop the node and rm -rf epoch-{N}. That is precisely the crash the heal was written for, and the old buffered backend healed it automatically. The doc comment directly above the function states the assumption the padding breaks:
"The position index settles it: every consensus read resolves through it, so an empty one means no output was ever committed here."
The second witness (consensus_pos_idx.is_empty()) is defeated by the same padding — see my comment on position_index/index.rs:147. Both witnesses fail for one root cause.
Suggested direction: widen the first witness and replace the second with a padding-immune one. The second change is what makes widening the first safe — consensus_digests.len() == 0 plus an unmoved data_file_length is header-authoritative proof that no output was ever committed, which consensus_pos_idx.is_empty() alone is not.
// consensus_pack.rs
fn first_record_is_dataless_tear(
data: &mut Pack<PackRecord>,
consensus_pos_idx: &PositionIndex<IndexPositions>,
consensus_digests: &HdxIndex,
) -> bool {
// A tear inside record zero presents as UnexpectedEof only on a physically exact file. The
// mmap backend pads the physical file past the logical end after an unclean exit, so the
// same tear reads as a 0-size, CRC-failing record out of the padding.
let first_record_unreadable = match data.record_size(DATA_HEADER_BYTES as u64) {
Err(FetchError::IO(ref e)) if e.kind() == io::ErrorKind::UnexpectedEof => true,
Err(FetchError::CrcFailed) | Err(FetchError::RequestedSizeTooLarge(..)) => true,
_ => false,
};
// Dataless witnesses that padding cannot inflate: the digest index's header-authoritative
// key count and its `data_file_length` commit marker.
let nothing_committed = consensus_digests.is_empty()
&& consensus_digests.data_file_length() <= DATA_HEADER_BYTES as u64
&& consensus_pos_idx.is_empty();
first_record_unreadable && nothing_committed
}That needs Self::open_digest_indexes(&base_dir, data.header(), false)? (currently at :945) hoisted above the meta check at :888, alongside this open_pdx_file call.
| } | ||
| } | ||
| let mut pdx_file = DataFile::open(dir.join(file_name), read_only)?; | ||
| let mut pdx_file = MmapDataFile::open(dir.join(file_name), read_only)?; |
There was a problem hiding this comment.
Re-opening a narrowed slice of #1269 (comment), which was closed as OBE. The trunc_and_heal half of that thread genuinely is obsolete — recover_pack replaced it and does not walk the pdx backwards. But the len() mechanism itself is byte-identical to main, and it still has consumers outside recover_pack, so I think part of it survives and is worth keeping open.
// position_index/index.rs:197-205
pub fn len(&self) -> usize {
let len = self.pdx_file.len() as usize;
len.saturating_sub(PDX_HEADER_SIZE) / T::buffer_len()
}
pub fn is_empty(&self) -> bool { self.len() == 0 }PdxHeader (:26-32) holds only type_id, version, uid, appnum — no entry count — so the file length is the only source of truth. MmapDataFile::open_with sets end: orig_len from file.metadata()?.len() (data_file.rs:211, 233), with no header field, no sentinel scan, and no truncate-on-open. Writing the 26-byte pdx header alone extends the physical file to 1 MiB and fsyncs that size, which works out to (1048576 - 26) / 8 = 131068 phantom entries for PositionIndex<u64> and / 28 = 37448 for PositionIndex<IndexPositions>.
Worth contrasting with the sibling HdxIndex in this same PR, which does keep an authoritative count (digest_index/index.rs:79 values, :409-411 len() returns self.header.values). The digest index is immune to the padding; the position index is not.
On main this could not happen: git show c703dfb8:...data_file.rs has len() as self.data_file_end + self.write_buffer.len(), and the old backend never called set_len to extend. PositionIndex::len() is unchanged — the semantics changed underneath it.
One thing that makes it worse than transient: the geometry "repair" a few lines below at :179-192 sees the misaligned tail and, on a writable open, does set_len(26 + len()*stride). That converts transient zero padding into 37,448 (or 131,068) durable, zero-filled index records that a subsequent clean Drop then preserves.
A correction to my own framing, so this is not overstated: the chain to first_record_is_dataless_tear is not the operative one. is_empty() is the right operand of &&, and the left operand fails first for an independent reason (see my comment on consensus_pack.rs:878). The other two named consumers do self-correct, but not cheaply and not robustly:
Inner::files_consistent(consensus_pack.rs:603-624) loads 28 zero bytes;IndexPositions::decodehas its own CRC32 (:2122-2130) so it returnsErr,files_consistentis false, andrecover_packrunstruncate_all()and rebuilds. Conservative direction — cost rather than corruption, but that cost is the full-epoch replay.EpochRecordDb::heal_records(epoch_records.rs:1136-1171) walks down through ~131 k phantom slots and converges on the right answer at ~131 k wasted iterations. Its correctness rests on an accident, though:PosIndexValue for u64has no CRC (index.rs:355-362), so every zero slot decodes as position 0, and the loop is saved only becauserecords.record_size(0)reads the pack header'sb"teln"magic (1,852,403,060) which exceedsMAX_RECORD_SIZE(16 MiB). That feels fragile to rely on.
Suggested fix — give PdxHeader an authoritative count, mirroring HdxHeader::values:
// position_index/index.rs
/// 26 -> 34: adds the 8-byte `values` count.
pub const PDX_HEADER_SIZE: usize = 34;
struct PdxHeader {
type_id: [u8; 8],
version: u16,
uid: u64,
appnum: u32,
/// Authoritative number of records. Written LAST, as a commit marker (same contract as
/// `HdxHeader::data_file_length`): whenever this is durable, the records it counts are too.
values: u64,
}
impl<T: PosIndexValue> PositionIndex<T> {
pub fn len(&self) -> usize { self.values as usize }
}At open, take the count as durable-but-bounded so padding can never inflate it and a torn tail can never over-count — replacing the geometry block at :179-192:
let stride = T::buffer_len() as u64;
let physical_records = index.pdx_file.len().saturating_sub(PDX_HEADER_SIZE as u64) / stride;
index.values = header.values.min(physical_records);
// Drop padding AND any torn tail in one step; also re-establishes `end` for appends.
let exact = PDX_HEADER_SIZE as u64 + index.values * stride;
if !read_only {
if index.pdx_file.len() != exact { index.pdx_file.set_len(exact)?; }
} else if index.pdx_file.len() < exact {
return Err(LoadHeaderError::InvalidIndexGeometry);
}sync would publish the count last (records durable first, then header + sync_range over the header), and truncate_to_index/truncate_all would need to update self.values.
That is an on-disk format change (PDX_HEADER_SIZE 26 -> 34), so it needs a call from you rather than from me: either gate it on data_header.version() and bump PACK_VERSION, or accept that existing pdx files are rebuildable from the data-log WAL (recover_pack/heal_records already do exactly that). It also interacts with the v0 handling in recover_pack.
Test gap either way: nothing in crates/storage uses std::mem::forget/ManuallyDrop/abort, so no test ever sees a padded index. Append N records, mem::forget the index, reopen, assert len() == N.
| // fresh region is already an empty bucket (the fill below is a cheap defensive memset). | ||
| if let Some(buffer) = self.hdx_file.slice_mut(split_pos, Self::BUCKET_SIZE) { | ||
| // Note this will zero the CRC as well (we want that- marks it "dirty"). | ||
| buffer.fill(0); |
There was a problem hiding this comment.
Continuing #1269 (comment) on the same function. That thread covers the in-process element loss; this is about the ordering that causes it, plus a durable variant of it. Both buckets are zeroed through the mapping before redistribution, so every error return after this point loses data that was intact when the function was entered.
Error paths reachable after the fills:
| Line | Exit | Trigger |
|---|---|---|
:680 |
self.hdx_file.ensure_len(..)? |
after fill #1, before fill #2 — io::Error from set_len/map_mut/fsync |
:685 |
return Err(AppendError::ReadOnly) |
after fill #1 |
:694 |
return Err(AppendError::CrcError) |
after both fills — the rehash guard |
:697, :699 |
save_to_bucket_buffer(..)? |
after both fills |
save_to_bucket_buffer itself fails at :591 (ReadOnly), :599 (odx_file.seek), :601 (odx_file.write_all -> WriteDataError). The odx path is genuinely reachable during a split: collect_bucket_elements gathers the main bucket plus its whole overflow chain (:547-571), so redistribution routinely re-spills past BUCKET_ELEMENTS = 32 and hits write_all.
The zeros go straight through the mapping — slice_mut (data_file.rs:300-311) returns Some(&mut map[start..start+len]) borrowed directly from Backing::Rw(MmapMut), no staging buffer. And if the split triggers growth, ensure_len -> grow_to calls self.file.sync_all() (data_file.rs:367-374), so the zeroing gets fsync'd to disk before redistribution runs. Because the fill clears bytes 0..8 — the overflow-chain head — the loss is not bounded at 32 entries; the bucket's entire odx chain becomes unreachable.
On main this was commit-on-success: git show c703dfb8:...index.rs:546-590 built two private Vec<u8> buffers and committed them only via dirty_bucket_cache.insert(..) after redistribution succeeded. This branch's own first mmap draft kept that property too — git show a0dee966:...digest_index/index_mmap.rs:461-499 builds buffer/buffer2 locally then calls write_bucket. The in-place fill(0) came later, in 0b3f8f24 ("Make the new mmap index the default").
There is also an amplification: self.capacity is refreshed only after a successful split (:640), so the while condition in expand_buckets still holds on the next save and split_one_bucket runs again with buckets already incremented, zeroing the next bucket. Under a persistent ENOSPC that destroys one more bucket per save attempt.
The durable half does get caught: save_consensus_output/save_consensus_batches advance set_data_file_length only after a successful index save (consensus_pack.rs:1147-1152, :1199-1207), so files_consistent fails on the next open and recover_pack deletes both digest-index directories and rebuilds from the WAL, and the pack wedges via InvalidConsensusNumber (:1136-1140) rather than papering over the gap. The exposure is the in-process window: run_pack_loop (:138-196) keeps serving after the error, so contains_batch/batch()/consensus_header_by_digest return "not found" for records physically present in the pack — including to peers — until the node stops.
Could we restore commit-on-success and hoist the only fallible io ahead of the mutation? That matches both main and this branch's own earlier draft, and it also fixes the header-advance ordering I flagged at :652.
/// Bucket for `key` under an explicit (modulus, buckets) geometry, so the split can compute
/// post-split placement before the header is advanced.
fn hash_to_bucket_at(&self, key: &[u8], modulus: u32, buckets: u32) -> u64 {
let hash = self.hasher_builder.hash_one(key);
let modulus = modulus as u64;
let bucket = hash % modulus;
if bucket >= buckets as u64 { bucket - modulus / 2 } else { bucket }
}
fn split_one_bucket(&mut self) -> Result<(), AppendError> {
let old_modulus = self.modulus;
let old_buckets = self.buckets();
let split_bucket = (old_buckets - (old_modulus / 2)) as u64;
let new_bucket = old_buckets as u64;
let new_modulus = (old_buckets + 1).next_power_of_two();
let split_pos = self.bucket_pos(split_bucket);
let new_pos = self.bucket_pos(new_bucket);
// 1. Read phase: shared borrows only, nothing mutated yet.
let elements = self.collect_bucket_elements(split_bucket)?;
// 2. Grow for the new bucket BEFORE anything is destroyed, so the one genuinely fallible
// io in the split happens while both buckets are still intact.
self.hdx_file.ensure_len(new_pos + Self::BUCKET_SIZE as u64)?;
// 3. Redistribute into private buffers under the POST-split geometry. Every error return
// below leaves the on-disk buckets and the header exactly as they were.
let mut split_buf = vec![0_u8; Self::BUCKET_SIZE];
let mut new_buf = vec![0_u8; Self::BUCKET_SIZE];
for (hash, rec_pos) in elements {
let bucket = self.hash_to_bucket_at(hash.as_slice(), new_modulus, old_buckets + 1);
if bucket != split_bucket && bucket != new_bucket {
return Err(AppendError::CrcError);
}
let buf = if bucket == split_bucket { &mut split_buf } else { &mut new_buf };
Self::save_to_owned_buffer(buf, hash.as_slice(), rec_pos, &mut self.odx_file)?;
}
// 4. Commit: memcpys through the mapping (infallible after `ensure_len`), header last.
let Some(dst) = self.hdx_file.slice_mut(split_pos, Self::BUCKET_SIZE) else {
return Err(AppendError::ReadOnly);
};
dst.copy_from_slice(&split_buf); // CRC trailer stays zero => "dirty", as before
let Some(dst) = self.hdx_file.slice_mut(new_pos, Self::BUCKET_SIZE) else {
return Err(AppendError::ReadOnly);
};
dst.copy_from_slice(&new_buf);
self.inc_buckets();
self.modulus = new_modulus;
Ok(())
}That needs the body of save_to_bucket_buffer (:575) factored into a save_to_owned_buffer(buffer: &mut [u8], ..) operating on a caller-owned slice (no mapping borrow), with save_to_bucket_buffer becoming a thin wrapper that slices the mapping and delegates. Also refreshing self.capacity in expand_buckets unconditionally would stop a failed split leaving the loop condition primed to re-split. The only residue on an error is then an orphaned odx record — a space leak, unreferenced.
| let old_modulus = self.modulus; | ||
| // The bucket being split. | ||
| let split_bucket = (self.buckets() - (old_modulus / 2)) as u64; | ||
| self.inc_buckets(); |
There was a problem hiding this comment.
inc_buckets() here (and self.modulus four lines down) publish the post-split geometry before anything that can fail has run, and nothing rolls them back.
To be explicit up front: this is not a request to restore the panic! that #1269 (comment) asked you to remove. Returning AppendError there was the right call and I am not asking to undo it. The ask is only to move these two mutations to after the fallible work.
The ordering is that both fields mutate before collect_bucket_elements (:667), before ensure_len (:680), and before either bucket is written, while hash_to_bucket (:458-468) reads exactly those two fields — so routing changes the instant :656 runs.
There is no rollback on any path. expand_buckets (:639) and Index::save (:839) propagate with ?; consensus_pack.rs:1147-1149/:1199-1201 map to PackError::IndexAppend; run_pack_loop (:138-196) sends the error back and keeps looping — no poisoning, no abort, no reset. And Drop/sync then persist the inconsistent geometry unconditionally: save sets self.synced = false at :837 before expand_buckets, so Drop (:816-830) always runs ordered_sync -> write_header_only() (:769) -> HdxHeader::write_header (:175-209), which recomputes the header CRC at :206. The bad buckets lands on disk inside a CRC-valid header. MmapDataFile::drop then truncates the hdx to end (data_file.rs:716), so if ensure_len was the failure the file is genuinely shorter than buckets * BUCKET_SIZE.
Live-process effect: keys that now hash to new_bucket read as NotFound (see my comment at :556), and writes to them return a bogus AppendError::ReadOnly (:589-591) on a writable index.
Being fair about scope — main has the identical ordering. git show c703dfb8:...index.rs:546-556 runs inc_buckets() then self.modulus = (self.buckets() + 1).next_power_of_two() before remove_bucket, before the redistribution loop, and before the final if iter.crc_failure() { return Err(..) }. The header was advanced early in every version of this function. What changed is the consequence: main panicked at the rehash guard, so the process died rather than continuing with a corrupt in-memory header; now it returns and the node keeps running on it. The right resolution is to make the early advance unnecessary rather than to bring back the panic.
The on-disk half is caught indirectly — on a failed save both data_file_length and the position-index tail lag, so files_consistent (consensus_pack.rs:603-624) fails and recover_pack (:645-660) removes hash/ and bhash/ and rebuilds from the WAL.
The fix is already in the patch I sketched on :676: inc_buckets() / self.modulus = new_modulus move to the last two lines, after every fallible step, with redistribution using hash_to_bucket_at(key, new_modulus, old_buckets + 1) so it can compute post-split placement without publishing it. Adding the open_hdx_file geometry check I mention at :556 would cover the already-persisted case as defence in depth.
| /// torn/zero-padded tail (safe to truncate) or mid-log corruption (an error). A torn tail | ||
| /// yields only unreadable garbage until EOF; if any later record still decodes then valid | ||
| /// data survived past the damage, so the damaged record was not the final one. | ||
| fn tail_is_torn( |
There was a problem hiding this comment.
Deferring entirely to #1269 (comment) on how tail_is_torn classifies a torn tail — I have nothing to add there and agree with the direction. Two separate mechanical points about this function, though: what it costs to run on a padded file, and a counter desync in the iterator it drives.
Cost. recover_pack:664 -> data.raw_iter() -> pack.rs:553-558 -> try_clone() returns (file, self.end) (data_file.rs:455-465), and end was initialized from the padded physical length. try_clone explicitly does not truncate (that moved to reconcile_to_end in 4590c787), and nothing truncates before the iteration — data.truncate(consistent_end) happens at :738-740, after the scan. So this loop walks the padding one zero frame at a time:
// pack_iter.rs:125-138
// val_size = 0; buffer.resize(0,0); read_exact(&mut []) == Ok
*pos += 4 + 0 + 4; // ADVANCES *BEFORE* THE CRC CHECK
if calc_crc32 != read_crc32 { return Err(FetchError::CrcFailed); }crc32(b"\x00\x00\x00\x00") == 0x2144df1c, not zero, so every zero frame fails CRC and advances exactly 8 bytes, and Some(Err(_)) => continue here swallows it. With DEFAULT_INITIAL_SIZE = 1 MiB and DEFAULT_MAX_MAP_SIZE = 128 MiB (data_file.rs:80-82):
| Logical data | Capacity | Padding | iterations |
|---|---|---|---|
| 100 MiB | 128 MiB | 28 MiB | 3,670,016 |
| 300 MiB | 384 MiB | 84 MiB | 11,010,048 |
| worst case | — | 128 MiB | 16,777,216 |
pos desync. read_record_file's *pos >= end guard at :110 is the one mechanism meant to stop the walk before the padding, but *pos is only advanced at :136, after read_exact(buffer) succeeds. The two error returns above it — RequestedSizeTooLarge at :128 and the ? on file.read_exact(buffer) at :130 — return with the reader's stream position already advanced past the 4-byte size prefix while pos is unchanged. position() (:85) reports self.reader.stream_position(), not pos, so from the first such error the two counters diverge permanently: the guard freezes while the reader keeps walking to physical EOF, and iter.position()? — which recover_pack uses for output_end at :690 and :714 — is measuring a different thing than the guard is. It terminates, but not via the guard, and pos advancing before the CRC comparison means a frame that failed its CRC is still counted as consumed.
For the cost half, a bulk zero check would collapse it:
// archive/pack_iter.rs, in impl<V, R: Read + Seek> PackIter<V, R>
/// True when every remaining byte up to the logical `end` is zero -- i.e. the tail is mmap
/// capacity padding, not record data.
///
/// A pack that was not closed cleanly reopens with `end` set from the PADDED physical file
/// length, so the tail handed to a recovery scan can be up to `DEFAULT_MAX_MAP_SIZE` (128 MiB)
/// of zeros. Each 8 zero bytes decodes as a 0-size record whose CRC fails, so a record-by-record
/// walk costs ~16.8 M iterations. This reads the remainder in bulk instead. On `true` the
/// iterator is left at `end`; on `false` it is rewound so the caller's walk is unaffected.
pub fn rest_is_zero(&mut self) -> io::Result<bool> {
let start = self.reader.stream_position()?;
if start >= self.end { self.pos = self.end; return Ok(true); }
let mut remaining = self.end - start;
let mut buf = vec![0_u8; 64 * 1024];
let mut all_zero = true;
while remaining > 0 {
let want = remaining.min(buf.len() as u64) as usize;
self.reader.read_exact(&mut buf[..want])?;
if buf[..want].iter().any(|&b| b != 0) { all_zero = false; break; }
remaining -= want as u64;
}
if all_zero { self.pos = self.end; }
else { self.reader.seek(io::SeekFrom::Start(start))?; }
Ok(all_zero)
}// consensus_pack.rs:766 -- tail_is_torn, first statement
// Fast path: an all-zero remainder is mmap capacity padding left by an unclean shutdown,
// which is by definition a clean tail. Checking it in bulk avoids decoding up to
// DEFAULT_MAX_MAP_SIZE / 8 == ~16.8 M zero frames one CRC failure at a time.
if matches!(iter.rest_is_zero(), Ok(true)) { return true; }This one is small and self-contained, so it seems like the right thing to land first regardless of how the classification question in the linked thread resolves.
| Self::collect_from_buffer(buf, &mut out, &mut seen); | ||
| Self::read_overflow_pos(buf) | ||
| } | ||
| None => return Ok(out), |
There was a problem hiding this comment.
This is the sibling of item 4 in #1269 (comment) — that one covers find_in_bucket's None => return Ok(None) at :522; this is the same swallow in collect_bucket_elements. Raising it separately because the reachability turned out to be broader than "short hdx after an unclean shutdown", and because it interacts with hardening elsewhere in the pack.
The asymmetry is stark in both functions. find_in_bucket: hdx slice at :514-515, miss arm at :522 -> None => return Ok(None), while the odx slice at :526 has None => return Err(FetchError::CrcFailed) at :528. Here: hdx slice at :550-551, miss at :556 -> None => return Ok(out), odx at :559 -> Err(AppendError::CrcError) at :561. Same failure, opposite treatment, depending only on which file the missing bytes were in.
slice returns None iff (a) offset + len overflows, (b) range_end > self.end, or (c) backing == Backing::Empty && len != 0 (data_file.rs:287). Case (c) is the most reachable and I had missed it initially: a failed remap (see my comment at data_file.rs:351) leaves backing == Empty with end unchanged, so every in-range bucket slice starts returning None and load returns FetchError::NotFound (:846-857) for a fully populated index. Case (b) is reachable too without any external corruption — the phantom-bucket state from the early header advance at :652 persists buckets = N+1 with N buckets on disk.
That matters more than "a lookup misses", because FetchError::NotFound is explicitly classified as a benign absence by fetch_error_is_absent (consensus_pack.rs:1495-1504). consensus_header_by_digest (:1240-1253) uses that classifier precisely so a "stored-but-unreadable header can never be silently mistaken for an absent one" — and the Ok(None) arm defeats that hardening, reporting a structurally broken index through the one error the hardening treats as quiet. contains_batch / contains_consensus_header degrade the same way.
Two related detection gaps turned up alongside it:
open_hdx_file(:362-388) validates version, appnum, uid, geometry and salt/pepper but never checksHEADER_SIZE + BLOOM_SIZE_BYTES + buckets * BUCKET_SIZE <= hdx_file.len(), so a header claiming more buckets than the file holds is accepted silently.crc_dirty_buckets(:779-788) andbucket_crc_scan(:795-807) useif let Some(..)and therefore skip out-of-range buckets without counting them, so a truncated hdx reportsBucketCrcReport { dirty: 0, corrupt: 0 }— "clean".
One claim of mine that does not hold, for the record: I first thought this let split_one_bucket zero both buckets and silently drop every element. It does not. slice_mut (data_file.rs:300-311) has an identical bounds check and a strictly narrower backing match, so slice(x,n) == None implies slice_mut(x,n) == None; split_one_bucket re-slices the same split_pos/BUCKET_SIZE at :674 and takes the else branch at :678 -> return Err(AppendError::ReadOnly) before any buffer.fill(0). Neither bucket is zeroed. The residue there is only the misleading error kind — ReadOnly for what is actually "unmapped or out of range".
Suggested fixes:
// index.rs::find_in_bucket -- replace the `None` arm at :522
// The bucket is inside `0..header.buckets` by construction, so a slice miss means the hdx is
// truncated, the header's bucket count outran the file, or the mapping is gone. Reporting
// `NotFound` here would be indistinguishable from a genuine absence (see `fetch_error_is_absent`).
None => return Err(FetchError::IO(io::Error::new(
io::ErrorKind::UnexpectedEof,
format!("hdx bucket {bucket} unreadable: header claims {} buckets, hdx len {}",
self.buckets(), self.hdx_file.len()),
))),// index.rs::collect_bucket_elements -- replace the `None` arm at :556, matching the odx arm below
None => return Err(AppendError::CrcError),// index.rs::open_hdx_file -- after the geometry check at :379
// A header claiming more buckets than the file can hold means a split was interrupted
// mid-flight; every lookup into the missing tail would silently read as "not found".
let need = (HEADER_SIZE + BLOOM_SIZE_BYTES) as u64
+ header.buckets as u64 * Self::BUCKET_SIZE as u64;
if hdx_file.len() < need { return Err(LoadHeaderError::InvalidIndexGeometry); }Plus counting rather than skipping out-of-range buckets in bucket_crc_scan (:795-807), so a short hdx cannot report (0, 0).
| /// Drop the current mapping, resize the physical file to `new_len`, and re-map it (leaving it | ||
| /// unmapped when `new_len == 0`). Never holds a mapping past EOF. | ||
| fn remap(&mut self, new_len: u64) -> io::Result<()> { | ||
| self.backing = Backing::Empty; // release any existing map before resizing |
There was a problem hiding this comment.
remap drops the mapping on the first line but only restores capacity on the success path, so a failure leaves capacity describing a mapping that no longer exists.
:351 self.backing = Backing::Empty; // drops the mapping first
:352 self.file.set_len(new_len)?; // <-- early return #1
:354 self.capacity = 0; // only on the `new_len == 0` branch
:358 let map = unsafe { MmapMut::map_mut(&self.file)? }; // <-- early return #2
:359 self.backing = Backing::Rw(map); self.capacity = new_len; // success path only
The complete list of capacity assignments is :218, :354, :360, :383, :577 — no failure-path reset anywhere. After a failed grow_to, remap_needed is already false (cleared at :384 on the way in), so ensure_capacity's block at :378-385 is skipped and :386's if needed <= self.capacity { return Ok(()); } fires against the stale value. refresh_data_file_end (:561-581) has the identical shape: backing = Backing::Empty at :567, maps with ? at :571/:574, updates capacity/end only at :577-578.
With backing == Empty the consequences are worse than a stale number:
flush_dirtyreturnsOk(())(:500-502) andsync_rangereturnsOk(())(:536-538). Those are false durability successes —sync_all/flushreport a barrier they never ran.Write::write->Err(io::Error::other("no writable mapping"))(:675), masking the real errno.slice/slice_mut->None(:287/:309);Read::read->Ok(0)(:629). Theslicecase is what makes the bucket miss atdigest_index/index.rs:556reachable on a fully populated index.
Traced end to end, since HdxIndex has no poison flag: ensure_len fails at index.rs:680 -> hdx unmapped, capacity stale -> Drop -> ordered_sync (:757-772) -> write_bloom -> ensure_capacity(HEADER_SIZE + BLOOM_SIZE_BYTES) <= stale capacity -> Ok(()) -> Err("no writable mapping"). The real ENOSPC/ENOMEM is gone; what an operator sees is tracing::error!("HdxIndex: failed to sync on drop: no writable mapping") (index.rs:825).
One more hazard in the same function: if set_len succeeds and map_mut fails during a shrink, self.end keeps the old larger value and Drop (:716) calls self.file.set_len(self.end), re-extending the just-truncated file with a zero tail.
I did chase one claim of my own to a dead end, for the record: I thought this could feed a misdiagnosis in first_record_is_dataless_tear, and it cannot. Its single call site (consensus_pack.rs:904, inside open_append) runs immediately after Pack::open -> MmapDataFile::open (:195-245), which either establishes a mapping over the whole physical file or returns Err; no write, and therefore no remap, happens on data in between. And if a mapping were somehow absent, end == 0, so pack_len > DATA_HEADER_BYTES at :887 would be false. data.truncate(DATA_HEADER_BYTES) at :931 is gated correctly.
Suggested fix — zero capacity on the way in, so every early return is self-describing:
// data_file.rs
/// Drop the current mapping, resize the physical file to `new_len`, and re-map it (leaving it
/// unmapped when `new_len == 0`).
///
/// On failure the file is left unmapped, so `capacity` is zeroed on every early return: a stale
/// capacity would let `ensure_capacity` report success against a mapping that no longer exists,
/// turning the real errno (ENOSPC/ENOMEM/EMFILE) into a generic "no writable mapping" on the next
/// write and making `flush_dirty`/`sync_range` claim a durability barrier they never ran. With
/// `capacity == 0` every later `ensure_capacity` re-enters the grow path and re-surfaces the
/// true cause.
fn remap(&mut self, new_len: u64) -> io::Result<()> {
self.backing = Backing::Empty; // release any existing map before resizing
self.capacity = 0; // no mapping is live from here until we install one
self.file.set_len(new_len)?;
if new_len == 0 { return Ok(()); }
// SAFETY: single-writer model; the file was sized to `new_len` immediately above.
let map = unsafe { MmapMut::map_mut(&self.file)? };
self.backing = Backing::Rw(map);
self.capacity = new_len;
self.advise_backing();
Ok(())
}The same shape applies to refresh_data_file_end (self.capacity = 0; self.end = 0; right after :567). And could the durability helpers stop reporting success when there is nothing to sync?
// data_file.rs::flush_dirty, and the same guard in sync_range
let Backing::Rw(map) = &self.backing else {
// A writable file with data but no mapping means a remap failed; reporting Ok here
// would claim a durability barrier that never ran.
return Err(io::Error::other("no writable mapping: cannot flush"));
};Optionally, giving HdxIndex the failed: Option<io::Error> poison that PackInner already has (pack.rs:316-349) would make the first real errno what every later save/load/sync reports.
| // walk would fail its CRC on the zeros. Safe here for the same reason as the | ||
| // persist above: the epoch has concluded, so nothing re-grows the padding before | ||
| // the copy. Skip the export on error. | ||
| if let Err(e) = self.consensus_chain.reconcile_current().await { |
There was a problem hiding this comment.
Following up on #1269 (comment) at this same line — item 2 there survives 4590c787 unchanged, and I found the same shape on a second, network-triggerable path.
What 4590c787 changed: the old try_clone did set_len(self.end) + remap_needed = true inline, and the commit split that into a try_clone that no longer truncates plus a new reconcile_to_end that does. reconcile_current now routes to reconcile_to_end, so the truncation is explicit rather than a side effect. But reconcile_to_end still sets remap_needed, still does no fsync of file or parent directory, and the pack is still open and writable — so the assumption in the comment above this line is exactly as unenforced as before the commit.
Ordering as it stands: persist_current (:576) -> reconcile_current (:586) -> trigger_export (:638) -> rx.await (:644) -> spawn_blocking(std::fs::copy(..)) (:657-659), copying the whole epochs/epoch-{N}/data pack, unbounded. Nothing enforces quiescence in that gap: ConsensusPack::is_static (consensus_pack.rs:134, :340) is set once at open and never flipped — there is no seal(). The pack is not dropped or replaced (rotation happens in ConsensusChain::new_epoch, consensus.rs:457, on the next epoch's entry). run_pack_loop (:138) is alive and PackMessage::ConsensusOutput (:142-144) is still accepted. save_consensus_output (consensus.rs:750-790) rejects only non-advancing numbers (:758) and wrong-epoch outputs (:764), and a late epoch-N output passes both. No mutex, no recorded length, no assertion — only the comments at :572-575, :583-585, :650-652.
The second path is get_epoch_stream, and it is worse: get_static returns the writable current pack when the epoch matches (consensus.rs:1229-1233), get_epoch_stream calls reconcile_data_len() on it (:604), and the consumer streams to physical EOF (sync_codec.rs:126-131 resolves cap = u64::MAX, :167 take(cap)). So a peer sync request can physically truncate and force a remap of the writing node's live pack.
Being fair about how bad this is in practice, because I initially had it worse than it is:
- The window is bounded by
abort_all_tasks()atrun_epoch.rs:507— milliseconds to at most 3 s (the subscriber'sdrain_pending_on_shutdowndeadline,subscriber.rs:401-407) — not the multi-minute export duration.export_epoch_stateis awaited atrun_epoch.rs:472, before the abort. - Both failure modes fail loudly at import rather than silently: padding ->
stream_importCRC failure (the repo already documents and tests this exact regression atconsensus_pack.rs:3814-3820); extra records ->PackError::InvalidConsensusNumber(:1117-1122). - Both shapes mostly pre-date this PR:
git show c703dfb8:...close_epoch.rs(:576,:634,:648) and...consensus.rsshow them onmain. What is new is the padding arm — pre-PR a late append produced only extra valid records. Thereconcile_*calls this PR adds are a partial mitigation that leaves the underlying assumption untouched. - In practice the pack usually drops before the copy, and
MmapDataFile::dropsilently un-pads it.
Separately: :544 never re-exports a successfully-renamed bundle, so a bad consensus_data is published and never retried.
Three independent options, and any one of them removes the class:
Record the length at reconcile and bound the copy (smallest). Make Inner::reconcile_data_len return Ok(self.data.file_len()), thread it through PackMessage::ReconcileDataLen and ConsensusChain::reconcile_current, then replace std::fs::copy:
let src = std::fs::File::open(&src_consensus)?;
let mut dst = std::fs::File::create(©_dst)?;
// Bounded by the length captured at reconcile time: a late append re-grows and re-pads the
// live pack, and a whole-file copy would carry the padding (importer CRC failure) or records
// past the epoch record's final_consensus number (PackError::InvalidConsensusNumber).
let n = std::io::copy(&mut src.take(sealed_len), &mut dst)?;
if n != sealed_len {
return Err(std::io::Error::new(std::io::ErrorKind::UnexpectedEof,
format!("consensus pack shrank during export: copied {n} of {sealed_len} bytes")));
}
dst.flush()?; dst.sync_all()Turn the comment into an enforced seal (defence in depth). Add sealed: Arc<AtomicBool> to ConsensusPack, a seal() that stores true, a check at the top of save_consensus_output returning PackError::Sealed(epoch), and call seal() from here. A late drain output then fails loudly at subscriber.rs:674, which is already only a warn! on the shutdown path.
Bound get_epoch_stream instead of truncating a live pack. Replace the reconcile_data_len at consensus.rs:604 with pack.consensus_output_end(epoch_record.final_consensus.number).await? (the same primitive get_partial_epoch_stream already uses at :639), return it as the stream cap, and drop the u64::MAX arm in sync_codec.rs:126-131 so both sync paths are uniformly bounded.
| //! Because the file is sized ahead of the data, the physical file is padded to `capacity >= end` | ||
| //! while actively appending (`end` is the logical data length). The physical file is reconciled to | ||
| //! **exactly `end`** at every point an external consumer can observe it — [`Self::try_clone`] (for | ||
| //! `PackIter`/`raw_iter`, which read to EOF) and `Drop` (clean close) both truncate to `end` — |
There was a problem hiding this comment.
A second doc cluster, distinct from the five sites in #1269 (comment) — these five went stale in 4590c787 specifically, and three of them are in this file. No build impact; grouping them so they can be one pass.
| Site | Problem |
|---|---|
this module doc, :22-24 |
says try_clone truncates to end |
remap_needed field doc, :175-177 |
says try_clone sets it |
ensure_capacity comment, :379 |
blames try_clone |
crates/storage/Cargo.toml:32 |
references a nonexistent data_file_mmap.rs |
digest_index/index.rs:307-309 |
intra-doc link is a self-reference |
try_clone (:455-465) contains no set_len at all. The truncation lives in reconcile_to_end (:485) and Drop (:716), and the only site that ever sets remap_needed = true is reconcile_to_end (:486) — the other three writes (:238, :384, :432) all store false. try_clone's own doc (:447-454) already states the correct contract, so it directly contradicts this module doc three screens above it, and try_clone_returns_end_without_truncating (:948-991) asserts the correct behaviour. 4590c787 split the old try_clone into today's try_clone + reconcile_to_end, but its only data_file.rs hunk starts at old-line 440, so it never reached the module doc, the field doc, or the ensure_capacity comment. pack.rs was correctly updated in the same commit (pack.rs:554, :1026-1028).
Cargo.toml:32 is a plain # comment above memmap2, not a path = key, so nothing breaks — cargo metadata --no-deps runs clean and the archive directory contains only data_file.rs. It dates to 0ec80c93 (when the file existed) and was orphaned by 6be4b6fb ("Just use the mmap backend for pack files now"). index.rs:307-309's [HdxIndex::open_hdx_file](super::index::HdxIndex::open_hdx_file) resolves to the very function it documents; it dates to 0b3f8f24, when it sat on a distinct HdxIndexMmap struct.
Suggested wording:
data_file.rs:22-24-> "...— [Self::reconcile_to_end] (called before an external byte-for-byte consumer such asstd::fs::copyor a read-to-EOF stream) andDrop(clean close) both truncate toend; [Self::try_clone] itself does not truncate — it only flushes and hands back the logicalendso callers such asPackIter/raw_itercan bound their reads to it —"data_file.rs:175-177-> "Set by [Self::reconcile_to_end] after it truncates the file toend; ..."data_file.rs:379-> "//reconcile_to_endtruncated the physical file toend..."crates/storage/Cargo.toml:32-> "# mmap-backed data file backend (see archive/data_file.rs)."digest_index/index.rs:307-309-> drop the dangling cross-reference; keep "Always uses the memory-mapped file backend."
One more line worth adding while you are in this doc: the msync-vs-fsync argument at :28-34 holds on Linux and macOS, but on FUSE and some network filesystems msync handling is implementation-defined in ways fsync is not. If a datadir on such a mount is ever supported, sync_disk is the correct barrier — worth saying so here.
…shape Root cause confirmed from source: PackIter::read_record_file advances its cursor past the full frame before the CRC check (pack_iter.rs:136 precedes :138), so after a torn record k+1 fails CRC, the cursor sits byte-exact on k+2, which decodes — and tail_is_torn then rejected the whole pack as CorruptPack, bricking node startup on the expected [good][torn][good] power-loss shape. A key refinement surfaced during implementation (worth flagging since it changed the design): I traced the index-sync model and found the digest/position indexes sync on Drop (clean close), not on persist() — so the on-disk "attested watermark" lags the WAL after a real crash. That means neither a pure watermark guard nor pure tail_is_torn is correct alone: - A pure watermark guard would wrongly reject test_recover_pack_restamps_index_length_on_a_torn_tail (it truncates previously-attested data). - Pure tail_is_torn is the original bug. The shipped fix is a combined heuristic in recover_pack: at a torn record, error only when consistent_end < attested_end and a record still decodes after the tear. attested_end is the max of the digest index's data_file_length and the position index's last output_end — so test_recover_mid_log_corruption_errors (which keeps the position index attesting the committed outputs) stays correctly fatal, while the unacked out-of-order-writeback tail truncates cleanly. Fix 3 — remediation: CorruptPack now carries a contextual message (pack path, db validate hint, "don't delete chain-data dirs"); the two bare meta-unreadable EpochLoad doors (open_append_exists, open_static) got the same db validate hint. Updated the one internal match arm in primary/network/mod.rs and the test matches!. #2 deferred per your call — refined the first_record_is_dataless_tear doc to state the residual one-fault brick precisely and reference the follow-up. Tests: new test_recover_truncates_unacked_torn_tail_with_later_good_record pins the fix — I verified it fails under the old behavior and passes with the fix. All existing recovery/heal tests unchanged and green. One open follow-up you may want to track alongside deferred-#2: the residual stale-digest-entry note (an incomplete output's header/batch digests saved before the tear is detected) — pre-existing and inert, left untouched to keep this hotfix minimal.
…ck.rs) - PositionIter now normalizes its buffer to whole entries at construction, so its fixed-width slicing is sound for any input. - IndexPositions::decode (and the u64 / (u64,u64) decoders) return FetchError on a bad length instead of panic!; the encode panics stay (they're fed our own fixed buffers — a true internal invariant, now documented). - Honest finding: the audit showed these panics weren't actually reachable through the public APIs (iter/load cap to whole-entry counts and read_exact errors on a short file first). So this is genuine defense-in-depth, not a reachable-bug fix — I noted that in the plan rather than overselling it. The read/recovery paths were otherwise already Result-based, and recovery runs synchronously at open (before the background thread), so corruption already surfaces as errors, not panics. Workstream B — offline failure-mode classifier (pack_validate.rs, db.rs) - New classify_physical_corruption walks the data stream read-only and classifies the physical failure into TornMetaEmpty / CorruptMetaWithData / TornTrailingTail / MidLogCorruption, with an is_truncatable() helper and a Display that prints the recommended operator action (safe-auto-heal vs data-loss → replace-from-peer, mirroring the runtime remediation wording). - Handles the two iterator subtleties: a record torn within its size prefix reads as EOF (detected via file_len vs position), and a payload-past-EOF read doesn't advance the cursor (the probe is progress-guarded against spinning). The past-EOF-inflation ambiguity is the known deferred-#2 limitation, noted in the code. - Wired into db validate: it now classifies physical corruption instead of bailing with a bare read error.
…ue produces an Err, never a panic that wedges the pack worker — exactly the append-only-integrity convention. New error variants (error/fetch.rs, error/insert.rs): FetchError::CorruptIndex(String) and AppendError::CorruptIndex(String) — distinct from NotFound/CrcFailed. The two exhaustive matches were extended: fetch_error_is_absent maps CorruptIndex → false (item 4's core — a corrupt index is not "absent"), and Pack's append-failure match treats it as a non-IO error. The four fixes (digest_index/index.rs): 1. Bounded element count — bucket_elements now returns Option<usize> (rejects counts > BUCKET_ELEMENTS); scan_bucket/collect_from_buffer return Result and error instead of slicing past the 1296-byte buffer. This also makes every B256::from_slice get exactly 32 bytes. 2. First overflow-hop validated — find_in_bucket and collect_bucket_elements now validate each pointer before slicing, seeding the strictly-backwards bound with odx_file.len() so the first hop (from a possibly-corrupt main bucket) gets the same monotonic check as later hops. No panic, no infinite loop. 3. checked_sub in split_one_bucket — a broken invariant surfaces as CorruptIndex instead of wrapping to a huge index misreported as ReadOnly. 4. Out-of-range mapping → CorruptIndex, not NotFound — a short/corrupt hdx no longer masquerades as a missing key (applied to both the read path and the split path). Existing overflow-chain errors migrated CrcFailed/CrcError → CorruptIndex so all structural corruption uses one honest variant (reads don't check CRC).
…uence by attacking the root cause: the export copied the pack data file to physical EOF (capturing mmap padding), which forced the fragile reconcile-then-quiescent-copy dance. Copying a bounded logical length dissolves all three gaps. Storage plumbing (consensus_pack.rs, consensus.rs): - Added PackMessage::DataFileLen + ConsensusPack::data_file_len() + ConsensusChain::current_data_len() — a lightweight actor query for the logical end. - Removed ConsensusChain::reconcile_current (now unused → resolves gap 3, the unguarded precondition). Kept reconcile_data_len/reconcile_to_end — still used by get_epoch_stream to serve a sealed, static epoch (a quiescent context with no live-append race). Export path (close_epoch.rs): - Replaced the reconcile_current() call with capturing data_len while epoch N is current. - Replaced std::fs::copy (to EOF) with a bounded copy of exactly data_len bytes + dst.sync_all(), and added a parent-dir fsync after the atomic rename (resolves gap 1 — the export artifact is now durable; the live pack correctly stays msync-only by design). - Updated the stale "epoch has concluded / nothing re-grows the padding" comments — the correctness no longer rests on that assumption. Why it's correct (resolves gap 2, the quiescence race): [0, data_len) is immutable append-only data flushed by persist_current, and the physical file is always >= data_len, so the bounded read captures exactly the records and never the padding — regardless of any concurrent append or padding re-growth.
…runcation SIGBUS window, sized to the reachability reality I established during planning. Key finding (relayed in the plan): the in-process window is currently guarded by lifecycle — get_static short-circuits the live epoch to the writer handle, open_static runs only on sealed epochs and is gated by files_consistent, and the stream_import replace path unlinks+renames (a different inode, not a truncate). So this is a latent footgun (the single-writer invariant is load-bearing but comment-only in the mmap layer), not a live bug. Per your choice, I did the contained defense-in-depth clamp + tightened the SAFETY comments (no flock). Changes: - MmapDataFile::set_read_bound(logical_end) (data_file.rs) — read-only handles only; clamps end down to the caller's attested length (never grows, no re-map). Since slice/read/len are all bounded by end, reads never touch a page above the committed data — the exact region a writer truncation removes — so the known truncations (Drop, try_clone set_len, reconcile_to_end, recover_pack truncate) can't SIGBUS a bounded read-only handle. - Pack::set_read_bound (pack.rs) — delegates to the data file. - Inner::open_static (consensus_pack.rs) — after files_consistent, clamps the read-only data handle to the digest index's attested data_file_length, with a debug_assert_eq! that the sealed pack's physical length already equals it. A no-op today, but it makes the read bound provably the attested end and protects if that guard is ever loosened. - Tightened the two SAFETY: single-writer comments (open_with read-only branch + refresh_data_file_end) to state the enforcing invariant precisely (read-only handles must only map a sealed file; why that holds; and the set_read_bound defense-in-depth).
… dropping the collected elements — per your directive (restore the previous bucket; the triggering save already errors out).
The fix (digest_index/index.rs):
- Split split_one_bucket into a thin wrapper + a private redistribute_split (the mutating half, verbatim). The wrapper now:
a. Collects the elements and snapshots the split bucket's raw bytes before any mutation (collect moved above inc_buckets, so a collect failure leaves the index untouched).
b. On any error from redistribute_split, restores modulus, header.buckets, and the split bucket's bytes, then propagates the error.
- Why it's correct: restoring the split bucket's bytes restores its overflow pointer, and the odx is append-only (a failed re-insert only appended records — the original chain is intact), so every original
element is reachable again. The new bucket, ensure_len growth, and orphan odx records are inert once buckets is reverted; values is untouched (inc_values=false); capacity/expand_at_capacity are only updated by
expand_buckets after a successful split.
- The triggering-save error-out was already in place: save calls expand_buckets()? before inserting the triggering key, so a failed split makes save return Err (propagated to the record writer via IndexAppend →
the actor → the oneshot) before the key is inserted or bloom-accrued.
…red the zeroed-page criticality assessment (no format change). Deliverable #1 — the wiring (pack_validate.rs, consensus_pack.rs): - Exposed the sidecar digest-dir names as pub const CONSENSUS_DIGEST_NAME/BATCH_DIGEST_NAME. - Added IndexBucketScan { consensus, batch } + index_scan: Option<IndexBucketScan> on PackValidationReport. - validate_pack_file now runs a best-effort, read-only scan of the hash/bhash hdx indexes (via HdxIndex::open_hdx_file(read_only=true) + bucket_crc_scan) after the data-stream walk. It never writes, so the detector can't itself launder a zeroed bucket. Absent sidecar dirs → index_scan = None (data-only, as before); an unreadable index → an issue; any dirty/corrupt bucket flips the verdict to Invalid even when the data stream is clean. - The Display gained an "index buckets" section with per-index dirty/corrupt counts and the distinct remediation (data is intact; remove hash/bhash to rebuild from the WAL). db validate picks it up unchanged. - Test test_validate_scans_index_bucket_crcs covers clean (scanned, (0,0), Valid), a payload flip (corrupt ≥ 1, Invalid), a whole-bucket zero (dirty ≥ 1, Invalid), and a bare data file (index_scan = None). It corrupts the file tail bucket (layout-independent of the feature-gated bloom size). Deliverable #2 — criticality (the "how bad / when" you asked for): The launder is harmless in the normal dirty case (correct CRC stamped over a good payload). It's harmful only when a bucket's payload is silently zeroed at rest — then the next ordered_sync launders it into a valid empty bucket, losing ≤32 digest→position mappings + one overflow chain. Impact = false-negative digest lookups (serving/dedup degradation) — not consensus-safety-critical (records live in the WAL; by-number lookups use the pdx) and fully recoverable by rebuild. Corrupt pages (non-zero wrong CRC) are never laundered and stay detectable; only fully-zeroed pages alias Dirty. The single-crash window is already closed by ordered_sync, leaving only silent-write-loss / bit-rot-at-rest triggers. Net: LOW — the real gap was the unreachable detector, now fixed. The launder-prevention (non-zero dirty sentinel) is a format change and stays out of scope; I documented it as a known residual on IndexBucketScan, with db validate as the mitigation.
- consensus_pack.rs — PackMessage::ReconcileDataLen variant + its actor arm, ConsensusPack::reconcile_data_len (pub async), Inner::reconcile_data_len, and the test_reconcile_before_copy_lets_raw_copy_stream_import test (its bounded-copy replacement, test_bounded_copy_of_padded_pack_stream_imports, already covers the path). - archive/pack.rs — Pack::reconcile_len and PackInner::reconcile_len. - archive/data_file.rs — MmapDataFile::reconcile_to_end, plus the now-dead remap_needed machinery: the AtomicBool field + doc, its open_with initializer, the ensure_capacity branch that read it, the set_len store, and AtomicBool from the sync::atomic import. Fixed the try_clone doc (raw/read-to-EOF consumers bound to the returned end; padding only removed on clean-close Drop) and trimmed the try_clone_returns_end_without_truncating test to its bounded-read half.
…ively distinguish a cleanly-sealed file from a crashed/padded one. All changes are in crates/storage. Core change — crates/storage/src/archive/data_file.rs - clean_close_sentinel(end) builds the 8-byte marker: crc32(end) ‖ crc32(crc32(end)) (little-endian). - sentinel_matches / detect_sentinel validate it — comparing against the full deterministic construction verifies both the length tie-in (first4 == crc32(orig_len-8)) and the self-consistency (last4 == crc32(first4)), per your choice. - Drop: after set_len(end) and before the final sync_all, appends the sentinel (skipped for a 0-length file). - open_with and refresh_data_file_end: detect and strip the sentinel via the shared helper, setting end = orig_len - 8 on a match, or leaving end at physical EOF and setting the new opened_unclean flag (with a pub fn opened_unclean() accessor) otherwise. Read-only opens with a missing sentinel flag-only (no error), keeping the existing set_read_bound clamp as the SIGBUS defense.
…and sync it early.
3 participants
Can run the bench with
cargo test --release -p tn-storage pack_file_bench -- --ignored --nocapture --test-threads 1. Note that running on Linux with a real disk is best (can set TMPDIR to a directory not on a tmpfs filesystem).Benching on Linux with a real SSD leads to mmap NOT being a big win. See results below, it can help in some cases and hurt in others. We may want to keep both backends but not change to mmap right now. Worth discussing more though.
Output of Claudes analysis of 11 runs on a linux machine using a real SSD (not temps):
Consensus pack-file benchmark: background thread vs direct IO
Benchmark:
crates/storage/src/pack_bench.rs(pack_file_bench,#[ignore]d — on-demand).It compares the production
ConsensusPack(one background thread per pack; every publicasync fnsends a message over an mpsc channel and awaits aoneshotreply) againstConsensusPackDirect(crates/storage/src/consensus_pack_direct.rs) — a lock-free twin thatowns
Innerand runs every call inline on the caller (&mut self, no thread, no channel,no lock). Both share the exact same
InnerIO and decode helpers, so the per-rowthr − dirdeltais purely the background-thread/channel cost.
The matrix is
{backend} × {transport} × {width}:buf= bufferedDataFile(fsyncbarrier) vsmmap=MmapDataFile(msyncbarrier). Switches all of a pack's files (data + position index + digest index).
thr= background thread + channel,dir= inline&mutcalls (baseline).x4/x16/x64; shallow leader vs deep sub-DAG).Run:
Environment & method
Numbers below are the median of 11 release runs on a Linux box backed by an SSD (not tmpfs) —
the representative setup. Times are ms for 200 outputs/column (per-op = value / count).
Op counts differ per row — normalize before comparing:
save_seq,save_durable,header_by_number,full_output,output_bytes,header_by_digest= 200 ops;batch_by_digest= 512;persist bulk,prefix_stream,full_stream,stream_import,latest_header,read_last_committed,reopen_static= 1 op.Caveat — column-position confound. A run executes the 12 columns serially (buf-thr x4 → … →
mmap-dir x64), so SSD writeback state drifts across a run and absolute cross-column numbers (e.g.
bufvsmmap, which sit in different positions) carry a position bias. The clean, position-controlled signal is
thr − dir(adjacent columns, same position) and the low-variance rows.The durability rows are additionally very noisy (coefficient of variation up to ~57 %).
Results — median of 11 runs (ms/200 ops)
Analysis
1. Background-thread overhead ≈ 3–4 µs/op (robust)
Measured as
thr − diron the near-zero-work rows (adjacent columns → position bias cancels):output_bytes(÷200)batch_by_digest(÷512)header_by_digest(÷200)The background thread + channel round-trip (send → wake worker → work → reply → wake caller = two
context switches) costs a fixed ~3–4 µs/op — tiny-output x4 columns run a bit higher (6–8 µs).
This matches the earlier macOS estimate and is negligible against any real IO or decode. The
dirbaseline being ~sub-µs (e.g.output_bytesdir 0.02–0.19 ms / 200 ≈ 0.1–1 µs/op) confirms the~4 µs is genuinely the thread/channel, not other bookkeeping.
2. Durability: no uniform mmap win on real SSD — a reversal
save_durable(apersist()barrier per output), median buf vs mmap by width:The ordering flips with output size, the values are highly variable (CV up to ~57 %), and they
are non-monotonic in width — i.e. the durable-write cost is dominated by the SSD write barrier + OS
writeback (plus the column-position confound), not cleanly by the backend.
persist bulkshows thesame crossover (mmap ~27 ms vs buf ~6 ms at x4; mmap ~6 ms vs buf ~14 ms at x64). On real SSD
there is no blanket msync-vs-fsync durability win — it depends on output size and is noisy.
3. Appends, reads, streams
save_seq(append, no barrier): buf ≈ mmap and stable (CV 1–4 %); mmap ~66 % slower only attiny x4, equal at x16/x64.
output_bytes,*_by_digest) are cheap and similar across backends. Thedecode-bound
full_outputis mmap-slower for small outputs (x4 7.71 vs 2.22 ms — first-touchpage faults on the mapping vs a warm buffered read) and converges by x64.
reopen_staticand small-epochstream_import: mmap slower (mapping + grow-fsyncsetup), converging to parity at x64.
Why this differs from the earlier macOS run
An earlier single run on macOS showed mmap
save_durable5–16× faster than buffered. That was anartifact of the platform: on macOS
fsyncis not a full power-loss barrier (that needsF_FULLFSYNC), so it under-measured durable-write cost — andmsynceven more so. On Linux/SSD bothfsyncandmsyncactually force the drive, so the durable-write rows become comparable, size-dependent, and noisy. Treat the macOS durability numbers as non-representative.
Recommendation
durable writes, decodes, streaming — and it keeps blocking file IO off the async runtime's workers.
The lock-free
dirbaseline confirms that ~4 µs is the thread/channel, not other overhead.(x64 ~2.8×) and hurts small outputs / cold opens, and the measurement is noisy. Buffered stays
the safe default; mmap is a workload-specific option (large outputs), not a default to flip.
Production consensus outputs carry real batches (≫ this bench's 1 tx/batch), i.e. they sit in or
beyond the x64 regime where mmap may help — but confirm with realistic batch sizes and a
position-controlled comparison before flipping the default.
Method notes for re-running the aggregation
The medians above were computed from
pack_bench.txt(11 concatenated run outputs) by extracting the12 numeric cells of each labelled row per run and taking the per-cell median; per-op costs use the
op counts listed under Environment & method. For sharper numbers, raise
NUM_OUTPUTS/WIDTHS,randomize/repeat column order to break the position confound, and report medians over many runs.
Appendix — original macOS single-run (SUPERSEDED)
Single run, macOS, release, ms/200 ops:
Original findings (as written at the time):
output_bytes(thr~0.8–1.2 ms vsdir~0.02–0.47 ms /200 ops, size-independent). ✅ Still holds — the Linux/SSD medians reproduce ~3–4 µs/op.
save_durablemmap(msync) vs buf(fsync) ≈ 5.8×–16× faster (buf~26–31 ms/op vs mmap ~1.6–5.4 ms/op);
persist bulk~13–16×. ❌ Artifact of macOSfsync; notreproduced on Linux/SSD, where the ordering flips with output size and is noise-dominated.
full_output/zstd,save_seq) sped up in release; thefsync-bound
save_durable(buf) barely moved → confirmed those rows are IO-bound.tempfile; don't over-read the sub-ms x4rows or the couple of inverted
full_outputcells.Original recommendation: keep the background thread (~4 µs/op negligible) — still valid; chase
mmap/msync durability — retracted, see the main recommendation above.