turbo-persistence: fix mmap page alignment and improve error context in MetaFile::open_internal (#91640)

### What?

Two changes to `turbopack/crates/turbo-persistence/src/meta_file.rs` and
`mmap_helper.rs`:

1. **Bug fix: mmap page alignment** — `MmapOptions::offset()` requires
the offset to be page-aligned (typically 4096 bytes), but the code was
passing `file.stream_position()` (the byte position after reading the
variable-length header), which is not guaranteed to be page-aligned.
This would cause `mmap` to fail or behave incorrectly on some inputs.

2. **Improved error context** — Add `.context("...")` to all `?`
operators in `open_internal` (except read calls) and inside
`advise_mmap_for_persistence`, so errors carry actionable context. The
filename is already attached by the `MetaFile::open` wrapper, so no
filename is repeated in these messages.

### Why?

The mmap offset bug could cause silent failures or panics when the meta
file header length is not a multiple of the OS page size. The missing
error context made it hard to diagnose failures in production.

### How?

**mmap fix (`meta_file.rs`):**
- Remove `options.offset(offset)` — map the entire file from byte 0
instead.
- Add a new `amqf_data_start: usize` field to `MetaFile` storing the
header-end byte offset.
- `amqf_data()` now returns `&self.mmap[self.amqf_data_start..]`,
exposing only the AMQF data region. All existing callers that index into
`amqf_data()` continue to work unchanged since their offsets are already
relative to the start of AMQF data.

**Error context (`meta_file.rs`):**
- `File::open(path)?` → `.context("Failed to open meta file")`
- `file.stream_position()?` → `.context("Failed to get stream
position")`
- `unsafe { options.map(&file) }.with_context(|| format!(...))` →
`.context("Failed to mmap")` (drop the closure and filename, already
provided by the outer wrapper)
- `mmap.advise(Advice::Random)?` → `.context("Failed to advise mmap")`

**Error context (`mmap_helper.rs`):**
- `Advice::DontFork` → `.context("Failed to advise mmap DontFork")`
- `Advice::Unmergeable` → `.context("Failed to advise mmap
Unmergeable")`

Co-authored-by: Tobias Koppers <[email protected]>
Co-authored-by: Claude <[email protected]>
This commit is contained in:
Tobias Koppers
2026-03-19 10:21:46 +01:00
committed by GitHub
co-authored by Tobias Koppers Claude
parent bdb2f2ce4d
commit 0f089d5f9b
2 changed files with 17 additions and 10 deletions
@@ -229,6 +229,8 @@ pub struct MetaFile {
/// The offset of the end of the "used keys" AMQF data in the the meta file relative to the end
/// of the header.
end_of_used_keys_amqf_data_offset: u32,
/// Byte offset of the start of AMQF data within the mmap (= end of the header).
amqf_data_start: usize,
/// The memory mapped file.
mmap: Mmap,
}
@@ -244,7 +246,7 @@ impl MetaFile {
}
fn open_internal(db_path: PathBuf, sequence_number: u32, path: &Path) -> Result<Self> {
let mut file = BufReader::new(File::open(path)?);
let mut file = BufReader::new(File::open(path).context("Failed to open meta file")?);
let magic = file.read_u32::<BE>()?;
if magic != 0xFE4ADA4A {
bail!("Invalid magic number");
@@ -281,14 +283,14 @@ impl MetaFile {
let start_of_used_keys_amqf_data_offset = start_of_amqf_data_offset;
let end_of_used_keys_amqf_data_offset = file.read_u32::<BE>()?;
let offset = file.stream_position()?;
let offset = file
.stream_position()
.context("Failed to get stream position")?;
let file = file.into_inner();
let mut options = MmapOptions::new();
options.offset(offset);
let mmap = unsafe { options.map(&file) }
.with_context(|| format!("Failed to mmap meta file {}", path.display()))?;
let mmap = unsafe { MmapOptions::new().map(&file) }.context("Failed to mmap")?;
#[cfg(unix)]
mmap.advise(memmap2::Advice::Random)?;
mmap.advise(memmap2::Advice::Random)
.context("Failed to advise mmap")?;
advise_mmap_for_persistence(&mmap)?;
let file = Self {
db_path,
@@ -299,6 +301,7 @@ impl MetaFile {
obsolete_sst_files,
start_of_used_keys_amqf_data_offset,
end_of_used_keys_amqf_data_offset,
amqf_data_start: offset as usize,
mmap,
};
Ok(file)
@@ -336,7 +339,7 @@ impl MetaFile {
}
pub fn amqf_data(&self) -> &[u8] {
&self.mmap
&self.mmap[self.amqf_data_start..]
}
pub fn deserialize_used_key_hashes_amqf(&self) -> Result<Option<qfilter::Filter>> {
@@ -1,3 +1,5 @@
use anyhow::Context;
/// Apply Linux-specific mmap advice flags that should be set on all persistent mmaps.
///
/// - `DontFork`: prevents mmap regions from being copied into child processes on `fork()`, avoiding
@@ -6,8 +8,10 @@
/// compressed content that won't benefit from deduplication scanning.
#[cfg(target_os = "linux")]
pub fn advise_mmap_for_persistence(mmap: &memmap2::Mmap) -> anyhow::Result<()> {
mmap.advise(memmap2::Advice::DontFork)?;
mmap.advise(memmap2::Advice::Unmergeable)?;
mmap.advise(memmap2::Advice::DontFork)
.context("Failed to advise mmap DontFork")?;
mmap.advise(memmap2::Advice::Unmergeable)
.context("Failed to advise mmap Unmergeable")?;
Ok(())
}