From 3bf71ee3fd1f55fbe0f53956973de64b85b2bdf9 Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:29:06 +0200 Subject: [PATCH 1/3] test: run cargo unit tests with miri as well (#97535) ### What? Adds a CI job that runs selected Rust unit tests under [Miri](https://github.com/rust-lang/miri) to detect undefined behavior in unsafe code. The job currently covers the low-level crates that are compatible with Miri: - `turbo-persistence` - `turbo-rcstr` - `turbo-tasks-malloc` It also makes those crates Miri-compatible by: - using provenance-free tagged-pointer operations in `turbo-rcstr`; - disabling mimalloc and native compression under Miri; - making mmap support a default-enabled Cargo feature that is disabled for Miri and wasm; - running persistence through file I/O when mmap is disabled; - skipping only tests measured to exceed the Miri time budget; - removing leaked test arenas from the analyzer predicate tests. ### Why? Miri can detect invalid memory access and other undefined behavior that normal Rust tests may not expose. Running it in CI gives low-level unsafe code an additional correctness check. The job uses an explicit package allowlist because Turbo Tasks builds its generated registry from linker sections, and Miri does not support the linker-defined section symbols required by that registry. ### How? - Installs the `miri` Rust component in CI and the development container. - Adds a dedicated `test-cargo-unit-miri` task and reusable workflow configuration. - Makes `memmap2` optional behind the default `mmap` feature and disables that feature in Miri CI and the wasm dependency graph. - Uses structural `cfg(miri)` branches for allocator and compression paths Miri cannot execute. - Re-enables three persistence compaction tests after measuring them successfully under Miri. - Keeps normal native behavior unchanged and documents measured Miri exclusions at the affected tests. Verification included focused normal and Miri tests for persistence, rcstr, allocation accounting, compression, and the refactored leak-free predicate cases. --------- Co-authored-by: vercel-fleet-prod[bot] <318278635+vercel-fleet-prod[bot]@users.noreply.github.com> Co-authored-by: Tobias Koppers <1365881+sokra@users.noreply.github.com> --- .devcontainer/rust/devcontainer-feature.json | 2 +- .github/workflows/build_and_test.yml | 18 ++ .github/workflows/build_reusable.yml | 10 +- packages/next-swc/package.json | 3 +- packages/next-swc/turbo.jsonc | 9 + rust-toolchain.toml | 2 +- turbopack/crates/turbo-persistence/Cargo.toml | 7 +- .../crates/turbo-persistence/benches/mod.rs | 2 + .../crates/turbo-persistence/src/arc_bytes.rs | 21 +- .../turbo-persistence/src/bin/sst_inspect.rs | 6 +- .../src/compaction/interval_map.rs | 2 + .../src/compaction/selector.rs | 1 + .../turbo-persistence/src/compression.rs | 84 +++++--- turbopack/crates/turbo-persistence/src/db.rs | 15 +- turbopack/crates/turbo-persistence/src/lib.rs | 35 +++- .../crates/turbo-persistence/src/meta_file.rs | 20 +- .../crates/turbo-persistence/src/rc_bytes.rs | 11 +- .../turbo-persistence/src/shared_bytes.rs | 3 + .../src/static_sorted_file.rs | 45 +++- .../src/static_sorted_file_builder.rs | 7 +- .../crates/turbo-persistence/src/tests.rs | 65 +++++- .../turbo-persistence/src/write_batch.rs | 3 +- turbopack/crates/turbo-rcstr/src/dynamic.rs | 8 +- turbopack/crates/turbo-rcstr/src/lib.rs | 2 + .../crates/turbo-rcstr/src/tagged_value.rs | 73 ++++++- .../crates/turbo-tasks-backend/Cargo.toml | 7 +- .../src/kv_backing_storage.rs | 2 + .../src/utils/dash_map_multi.rs | 2 + .../tests/inline_read_execution.rs | 2 + .../crates/turbo-tasks-fs/src/watcher/mod.rs | 3 + .../tests/trybuild.rs | 3 + .../crates/turbo-tasks-malloc/Cargo.toml | 6 +- .../crates/turbo-tasks-malloc/src/counter.rs | 22 +- .../crates/turbo-tasks-malloc/src/lib.rs | 20 +- .../turbo-tasks-malloc/src/memory_pressure.rs | 4 +- .../crates/turbo-tasks/src/priority_runner.rs | 1 + .../crates/turbo-tasks/src/scope_unbounded.rs | 2 + turbopack/crates/turbopack-css/src/process.rs | 4 + .../src/analyzer/jsvalue/predicates.rs | 194 ++++++++++++------ 39 files changed, 554 insertions(+), 172 deletions(-) diff --git a/.devcontainer/rust/devcontainer-feature.json b/.devcontainer/rust/devcontainer-feature.json index 368c410bf5c3..f7098e7c26ba 100644 --- a/.devcontainer/rust/devcontainer-feature.json +++ b/.devcontainer/rust/devcontainer-feature.json @@ -7,7 +7,7 @@ // this should match the `rust-toolchain.toml` "version": "nightly-2026-08-20", "profile": "minimal", - "components": "rustfmt,clippy,rust-analyzer" + "components": "rustfmt,clippy,rust-analyzer,miri" } } } diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 14630bd49ba1..7fe39a4e2c95 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -300,6 +300,7 @@ jobs: with: needsRust: 'yes' needsNextest: 'yes' + needsRustJsInstall: 'yes' skipNativeBuild: 'yes' skipInstallBuild: 'yes' afterBuild: pnpm dlx turbo@${TURBO_VERSION} run test-cargo-unit ${TURBO_ARGS} @@ -313,6 +314,23 @@ jobs: runs_on_labels: '["ubuntu-latest-16-core-oss"]' secrets: inherit + # Turbo Tasks' link-section registry is unavailable under Miri, so this runs the explicit + # low-level package allowlist in packages/next-swc/package.json instead of the whole workspace. + test-cargo-unit-miri: + name: test cargo unit (miri) + needs: ['changes', 'build-next'] + if: ${{ needs.changes.outputs.docs-only == 'false' }} + + uses: ./.github/workflows/build_reusable.yml + with: + needsRust: 'yes' + needsNextest: 'yes' + skipNativeBuild: 'yes' + skipInstallBuild: 'yes' + afterBuild: pnpm dlx turbo@${TURBO_VERSION} run test-cargo-unit-miri ${TURBO_ARGS} + stepName: 'test-cargo-unit-miri' + secrets: inherit + test-bench: name: test cargo benches needs: ['optimize-ci', 'changes', 'build-next'] diff --git a/.github/workflows/build_reusable.yml b/.github/workflows/build_reusable.yml index 0d30cdd8c897..20aefba8cc92 100644 --- a/.github/workflows/build_reusable.yml +++ b/.github/workflows/build_reusable.yml @@ -36,6 +36,10 @@ on: required: false description: 'if nextest rust dep is needed' type: string + needsRustJsInstall: + required: false + description: 'if Rust JS dependencies should be installed' + type: string rustBuildProfile: required: false description: 'The profile to use for the build, default is `release-with-assertions`, also supports `` for debug and `release` for normal release' @@ -360,7 +364,7 @@ jobs: # risk caching an incomplete store # If keep conditions in sync breaks, we can split into restore and save # steps where saving runs based on the outcome of the install step - if: ${{ inputs.skipInstallBuild != 'yes' || inputs.needsNextest == 'yes' }} + if: ${{ inputs.skipInstallBuild != 'yes' || inputs.needsRustJsInstall == 'yes' }} uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 timeout-minutes: 5 id: cache-pnpm-store @@ -371,10 +375,10 @@ jobs: - run: pnpm install # condititions must be superset of cache step, otherwise we risk running install without a cache and then caching the incomplete store. - if: ${{ inputs.skipInstallBuild != 'yes' || inputs.needsNextest == 'yes' }} + if: ${{ inputs.skipInstallBuild != 'yes' || inputs.needsRustJsInstall == 'yes' }} - name: Install node-file-trace test dependencies - if: ${{ inputs.needsNextest == 'yes' }} + if: ${{ inputs.needsRustJsInstall == 'yes' }} working-directory: turbopack/crates/turbopack-tracing/tests/node-file-trace run: pnpm install --recursive diff --git a/packages/next-swc/package.json b/packages/next-swc/package.json index a42afe4345bd..4ae347fd2afa 100644 --- a/packages/next-swc/package.json +++ b/packages/next-swc/package.json @@ -19,7 +19,8 @@ "rust-check-doc": "RUSTDOCFLAGS='-Zunstable-options --output-format=json' cargo doc --no-deps --workspace", "rust-check-fmt": "cd ../..; cargo fmt -- --check", "rust-check-napi": "cargo check -p next-napi-bindings", - "test-cargo-unit": "cargo nextest run --workspace --exclude next-napi-bindings --exclude turbo-tasks-macros --cargo-profile release-with-assertions --no-fail-fast && cargo test --workspace --doc --profile=release-with-assertions --no-fail-fast" + "test-cargo-unit": "cargo nextest run --workspace --exclude next-napi-bindings --exclude turbo-tasks-macros --cargo-profile release-with-assertions --no-fail-fast && cargo test --workspace --doc --profile=release-with-assertions --no-fail-fast", + "test-cargo-unit-miri": "MIRIFLAGS='-Zmiri-disable-isolation -Zmiri-permissive-provenance' cargo miri nextest run -p turbo-persistence -p turbo-rcstr -p turbo-tasks-malloc --no-default-features --features turbo-tasks-malloc/custom_allocator --no-fail-fast" }, "napi": { "binaryName": "next-swc", diff --git a/packages/next-swc/turbo.jsonc b/packages/next-swc/turbo.jsonc index b20c048642ba..e17a285b3664 100644 --- a/packages/next-swc/turbo.jsonc +++ b/packages/next-swc/turbo.jsonc @@ -91,5 +91,14 @@ "!../../turbopack/crates/turbopack-tests/tests/execution/**/output/*", ], }, + "test-cargo-unit-miri": { + "dependsOn": ["rust-fingerprint"], + "inputs": [ + "../../target/.rust-fingerprint", + "../../crates/*/tests/**", + "../../turbopack/crates/*/tests/**", + "!../../turbopack/crates/turbopack-tests/tests/execution/**/output/*", + ], + }, }, } diff --git a/rust-toolchain.toml b/rust-toolchain.toml index decce1d880d2..72873e38c702 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -2,5 +2,5 @@ # if you move the file, also update any turbo.json inputs that references it. [toolchain] channel = "nightly-2026-08-20" -components = ["rustfmt", "clippy", "rust-analyzer"] +components = ["rustfmt", "clippy", "rust-analyzer", "miri"] profile = "minimal" diff --git a/turbopack/crates/turbo-persistence/Cargo.toml b/turbopack/crates/turbo-persistence/Cargo.toml index 872d4f5eaf33..5f020ce82234 100644 --- a/turbopack/crates/turbo-persistence/Cargo.toml +++ b/turbopack/crates/turbo-persistence/Cargo.toml @@ -5,7 +5,8 @@ edition = "2024" license = "MIT" [features] -default = [] +default = ["mmap"] +mmap = ["dep:memmap2"] verify_sst_content = [] strict_checks = [] stats = ["quick_cache/stats"] @@ -23,7 +24,7 @@ either = { workspace = true } fs-err = { workspace = true } jiff = { version = "0.2.10", features = ["serde"] } lz4_flex = { workspace = true } -memmap2 = "0.9.5" +memmap2 = { version = "0.9.5", optional = true } nohash-hasher = { workspace = true } parking_lot = { workspace = true } # See https://github.com/vercel/next.js/issues/91708 for why we need the legacy_x86_64_support feature @@ -51,6 +52,7 @@ turbo-tasks-malloc = { workspace = true, features = ["custom_allocator"] } [[bin]] name = "sst_inspect" path = "src/bin/sst_inspect.rs" +required-features = ["mmap"] [lints] workspace = true @@ -58,3 +60,4 @@ workspace = true [[bench]] name = "mod" harness = false +required-features = ["mmap"] diff --git a/turbopack/crates/turbo-persistence/benches/mod.rs b/turbopack/crates/turbo-persistence/benches/mod.rs index bc4a5e03efaf..28b84f167775 100644 --- a/turbopack/crates/turbo-persistence/benches/mod.rs +++ b/turbopack/crates/turbo-persistence/benches/mod.rs @@ -1,3 +1,5 @@ +#![cfg(feature = "mmap")] + use std::{cell::UnsafeCell, path::Path, sync::LazyLock, time::Duration}; use anyhow::Result; diff --git a/turbopack/crates/turbo-persistence/src/arc_bytes.rs b/turbopack/crates/turbo-persistence/src/arc_bytes.rs index 8b32bf939366..960bedd409c0 100644 --- a/turbopack/crates/turbo-persistence/src/arc_bytes.rs +++ b/turbopack/crates/turbo-persistence/src/arc_bytes.rs @@ -6,6 +6,7 @@ use std::{ sync::Arc, }; +#[cfg(feature = "mmap")] use memmap2::Mmap; use crate::{ @@ -24,6 +25,7 @@ enum Repr { data: *const [u8], _backing: Arc<[u8]>, }, + #[cfg(feature = "mmap")] Mmap { data: *const [u8], _backing: Arc, @@ -45,6 +47,7 @@ impl ArcBytes { fn backing_bytes(&self) -> Option<&[u8]> { match &self.repr { Repr::Arc { _backing, .. } => Some(_backing), + #[cfg(feature = "mmap")] Repr::Mmap { _backing, .. } => Some(_backing), Repr::Inline { .. } => None, } @@ -78,7 +81,9 @@ impl Deref for ArcBytes { match &self.repr { // SAFETY: `data` points into the backing held by the same variant, which keeps it // alive for as long as `self`. - Repr::Arc { data, .. } | Repr::Mmap { data, .. } => unsafe { &**data }, + Repr::Arc { data, .. } => unsafe { &**data }, + #[cfg(feature = "mmap")] + Repr::Mmap { data, .. } => unsafe { &**data }, // Borrowed from `self`, so this is recomputed after a move rather than stored. Repr::Inline { buf, len } => &buf[..*len as usize], } @@ -114,7 +119,11 @@ impl Eq for ArcBytes {} impl ArcBytes { /// Returns `true` if this `ArcBytes` is backed by a memory-mapped file. pub fn is_mmap_backed(&self) -> bool { - matches!(self.repr, Repr::Mmap { .. }) + #[cfg(feature = "mmap")] + return matches!(self.repr, Repr::Mmap { .. }); + + #[cfg(not(feature = "mmap"))] + false } /// Returns `true` if the backing `Arc` allocation is shared (i.e., there @@ -124,12 +133,15 @@ impl ArcBytes { pub fn is_shared_arc(&self) -> bool { match &self.repr { Repr::Arc { _backing, .. } => Arc::strong_count(_backing) > 1, - Repr::Mmap { .. } | Repr::Inline { .. } => false, + #[cfg(feature = "mmap")] + Repr::Mmap { .. } => false, + Repr::Inline { .. } => false, } } } impl SharedBytes for ArcBytes { + #[cfg(feature = "mmap")] type MmapHandle = Arc; fn slice(self, range: Range) -> Self { @@ -142,6 +154,7 @@ impl SharedBytes for ArcBytes { Self { repr: match self.repr { Repr::Arc { _backing, .. } => Repr::Arc { data, _backing }, + #[cfg(feature = "mmap")] Repr::Mmap { _backing, .. } => Repr::Mmap { data, _backing }, Repr::Inline { .. } => unreachable!("handled above"), }, @@ -168,6 +181,7 @@ impl SharedBytes for ArcBytes { data, _backing: _backing.clone(), }, + #[cfg(feature = "mmap")] Repr::Mmap { _backing, .. } => Repr::Mmap { data, _backing: _backing.clone(), @@ -179,6 +193,7 @@ impl SharedBytes for ArcBytes { } } + #[cfg(feature = "mmap")] unsafe fn from_mmap(mmap: &Arc, subslice: &[u8]) -> Self { debug_assert!( is_subslice_of(subslice, mmap), diff --git a/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs b/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs index 153edc2e9f0f..059f5911454d 100644 --- a/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs +++ b/turbopack/crates/turbo-persistence/src/bin/sst_inspect.rs @@ -264,10 +264,14 @@ fn collect_sst_info(db_path: &Path) -> Result>> { meta_seqs.sort_unstable(); + #[cfg(feature = "mmap")] + let access_mode = turbo_persistence::AccessMode::Mmap; + #[cfg(not(feature = "mmap"))] + let access_mode = turbo_persistence::AccessMode::File; let mut meta_files: Vec = meta_seqs .iter() .map(|&seq| { - MetaFile::open(db_path, seq, None, turbo_persistence::AccessMode::Mmap) + MetaFile::open(db_path, seq, None, access_mode) .with_context(|| format!("Failed to open {seq:08}.meta")) }) .collect::>()?; diff --git a/turbopack/crates/turbo-persistence/src/compaction/interval_map.rs b/turbopack/crates/turbo-persistence/src/compaction/interval_map.rs index 97101bedd90d..e404aa4e88cb 100644 --- a/turbopack/crates/turbo-persistence/src/compaction/interval_map.rs +++ b/turbopack/crates/turbo-persistence/src/compaction/interval_map.rs @@ -380,6 +380,7 @@ mod tests { } } + #[cfg(not(miri))] #[test] fn test_exhaustive_replace_versus_naive() { for_all_tiny_int_ranges([0, 1], |a| { @@ -400,6 +401,7 @@ mod tests { }); } + #[cfg(not(miri))] #[test] fn test_exhaustive_update_versus_naive() { for_all_tiny_int_ranges([1, 2], |a| { diff --git a/turbopack/crates/turbo-persistence/src/compaction/selector.rs b/turbopack/crates/turbo-persistence/src/compaction/selector.rs index 2506ab6c15ac..fa28fb058f92 100644 --- a/turbopack/crates/turbo-persistence/src/compaction/selector.rs +++ b/turbopack/crates/turbo-persistence/src/compaction/selector.rs @@ -588,6 +588,7 @@ mod tests { } } + #[cfg(not(miri))] #[test] fn simulate_compactions() { const KEY_RANGE: u64 = 10000; diff --git a/turbopack/crates/turbo-persistence/src/compression.rs b/turbopack/crates/turbo-persistence/src/compression.rs index e47ca5f9b739..a53f58bbfcf2 100644 --- a/turbopack/crates/turbo-persistence/src/compression.rs +++ b/turbopack/crates/turbo-persistence/src/compression.rs @@ -1,6 +1,11 @@ -use std::{cell::RefCell, mem::MaybeUninit, rc::Rc, sync::Arc}; +#[cfg(not(miri))] +use std::cell::RefCell; +use std::{mem::MaybeUninit, rc::Rc, sync::Arc}; -use anyhow::{Context, Result, ensure}; +#[cfg(not(miri))] +use anyhow::Context; +use anyhow::{Result, ensure}; +#[cfg(not(miri))] use lz4_flex::block::{ CompressTable, compress_into_with_table, decompress_into, get_maximum_output_size, }; @@ -16,6 +21,7 @@ pub enum Compression { Zstd3 = 1, } +#[cfg(not(miri))] thread_local! { /// Reuse lz4_flex's large hash table across independent blocks. Starting large improves /// compression speed and produces faster-to-decode streams for typical persistence blocks. @@ -37,30 +43,43 @@ fn decompress_block( ) -> Result<()> { debug_assert!( expected_len > 0, - "decompress_block called with uncompressed_length=0; uncompressed blocks should use \ - zero-copy mmap path" + "decompress_block called with uncompressed_length=0; uncompressed blocks are served \ + directly from their backing" ); - let bytes_written = match compression { - Compression::Lz4 => decompress_into(block, dest).map_err(anyhow::Error::from), - Compression::Zstd3 => ZSTD_DECOMPRESSOR.with_borrow_mut(|decompressor| { - decompressor - .decompress_to_buffer(block, dest) - .map_err(anyhow::Error::from) - }), + #[cfg(not(miri))] + { + let bytes_written = match compression { + Compression::Lz4 => decompress_into(block, dest).map_err(anyhow::Error::from), + Compression::Zstd3 => ZSTD_DECOMPRESSOR.with_borrow_mut(|decompressor| { + decompressor + .decompress_to_buffer(block, dest) + .map_err(anyhow::Error::from) + }), + } + .with_context(|| { + format!( + "Failed to decompress {compression:?} block ({} bytes compressed, {} bytes \ + uncompressed)", + block.len(), + expected_len + ) + })?; + ensure!( + bytes_written == expected_len as usize, + "Decompressed length does not match expected length: decompressed {bytes_written} \ + bytes, expected {expected_len}" + ); + } + #[cfg(miri)] + { + // Compression is skipped under Miri, so Miri-created blob payloads are verbatim. + let _ = compression; + ensure!( + block.len() == expected_len as usize, + "Miri builds skip compression, so a compressed block cannot be read under Miri" + ); + dest.copy_from_slice(block); } - .with_context(|| { - format!( - "Failed to decompress {compression:?} block ({} bytes compressed, {} bytes \ - uncompressed)", - block.len(), - expected_len - ) - })?; - ensure!( - bytes_written == expected_len as usize, - "Decompressed length does not match expected length: decompressed {bytes_written} bytes, \ - expected {expected_len}" - ); Ok(()) } @@ -108,18 +127,24 @@ pub fn checksum_block(data: &[u8]) -> u32 { /// Reusable compressor for a stream of blocks using the same family configuration. pub(crate) struct Compressor { compression: Compression, + #[cfg(not(miri))] zstd: Option>, } impl Compressor { pub(crate) fn new(compression: Compression) -> Result { + #[cfg(not(miri))] let zstd = match compression { Compression::Zstd3 => { Some(zstd::bulk::Compressor::new(3).context("Failed to create zstd compressor")?) } Compression::Lz4 => None, }; - Ok(Self { compression, zstd }) + Ok(Self { + compression, + #[cfg(not(miri))] + zstd, + }) } /// Compresses `block` into reusable storage, replacing its contents. @@ -130,6 +155,7 @@ impl Compressor { buffer: &mut Vec, ) -> Result<()> { buffer.clear(); + #[cfg(not(miri))] match self.compression { Compression::Lz4 => { let max_output_size = get_maximum_output_size(block.len()); @@ -156,6 +182,14 @@ impl Compressor { .context("zstd compression failed")?; } } + #[cfg(miri)] + { + // Compression is deliberately skipped under Miri. This avoids native Zstd, and using + // the same raw representation for both algorithms keeps blob reads on the matching + // copy path above. The caller's savings check stores SST blocks as uncompressed. + let _ = self.compression; + buffer.extend_from_slice(block); + } Ok(()) } } diff --git a/turbopack/crates/turbo-persistence/src/db.rs b/turbopack/crates/turbo-persistence/src/db.rs index 531224fa0769..fade89645792 100644 --- a/turbopack/crates/turbo-persistence/src/db.rs +++ b/turbopack/crates/turbo-persistence/src/db.rs @@ -17,9 +17,11 @@ use anyhow::{Context, Result, bail}; use auto_hash_map::AutoSet; use byteorder::{BE, ReadBytesExt, WriteBytesExt}; use dashmap::DashSet; +#[cfg(feature = "mmap")] use either::Either; use fs_err::{self as fs, File, OpenOptions, ReadDir}; use jiff::Timestamp; +#[cfg(feature = "mmap")] use memmap2::Mmap; use nohash_hasher::BuildNoHashHasher; use parking_lot::{Mutex, RwLock}; @@ -29,8 +31,10 @@ use smallvec::SmallVec; use tracing::span::EnteredSpan; pub use crate::compaction::selector::CompactConfig; +#[cfg(feature = "mmap")] +use crate::{AccessMode, mmap_helper::advise_mmap_for_persistence}; use crate::{ - AccessMode, DbConfig, FamilyKind, QueryKey, + DbConfig, FamilyKind, QueryKey, arc_bytes::ArcBytes, compaction::selector::{Compactable, get_merge_segments}, compression::{Compression, checksum_block, decompress_into_arc}, @@ -43,7 +47,6 @@ use crate::{ merge_iter::MergeIter, meta_file::{MetaEntryFlags, MetaFile, MetaLookupResult, StaticSortedFileRange}, meta_file_builder::MetaFileBuilder, - mmap_helper::advise_mmap_for_persistence, parallel_scheduler::ParallelScheduler, rc_bytes::RcBytes, sst_filter::SstFilter, @@ -703,7 +706,9 @@ impl TurboPersistence #[tracing::instrument(level = "info", name = "reading database blob", skip_all)] fn read_blob(&self, seq: u32, compression: Compression) -> Result { let path = self.path.join(format!("{seq:08}.blob")); + #[cfg(feature = "mmap")] let file = File::open(&path)?; + #[cfg(feature = "mmap")] let data: Either> = match self.config.access_mode { AccessMode::Mmap => { let mmap = unsafe { Mmap::map(file.file()) }.with_context(|| { @@ -722,10 +727,16 @@ impl TurboPersistence } AccessMode::File => Either::Right(fs::read(&path)?), }; + #[cfg(feature = "mmap")] let mut reader: &[u8] = match &data { Either::Left(mmap) => mmap, Either::Right(bytes) => bytes, }; + // Without mmap support, read the whole blob into memory. + #[cfg(not(feature = "mmap"))] + let data = fs::read(&path)?; + #[cfg(not(feature = "mmap"))] + let mut reader: &[u8] = &data; let uncompressed_length = reader .read_u32::() .context("Failed to read uncompressed length from blob file")?; diff --git a/turbopack/crates/turbo-persistence/src/lib.rs b/turbopack/crates/turbo-persistence/src/lib.rs index 67fe6ad4ad09..b9e34d6bd5ea 100644 --- a/turbopack/crates/turbo-persistence/src/lib.rs +++ b/turbopack/crates/turbo-persistence/src/lib.rs @@ -15,6 +15,7 @@ mod lookup_entry; mod merge_iter; pub mod meta_file; mod meta_file_builder; +#[cfg(feature = "mmap")] pub mod mmap_helper; mod parallel_scheduler; mod rc_bytes; @@ -40,6 +41,7 @@ pub use db::{ #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum AccessMode { /// Memory-map the file and access blocks via the mapped region. + #[cfg(feature = "mmap")] Mmap, /// Read blocks directly from the file via pread (no mmap). File, @@ -78,8 +80,32 @@ pub struct DbConfig { pub access_mode: AccessMode, } -/// Reads the `TURBO_PERSISTENCE_MMAP` env var (cached). Returns `AccessMode::File` when the var -/// is set to `"0"`, `AccessMode::Mmap` otherwise. +/// Returns the default access mode for this execution environment. +/// +/// Builds without mmap support always use file I/O. Builds with mmap support honor +/// `TURBO_PERSISTENCE_MMAP=0`; mmap remains the default otherwise. +fn default_access_mode() -> AccessMode { + #[cfg(not(feature = "mmap"))] + return AccessMode::File; + + #[cfg(feature = "mmap")] + access_mode_env_var() +} + +/// Returns mmap mode when the feature is enabled, and file mode otherwise. +/// +/// Call sites that specifically want mmap use this helper because `AccessMode::Mmap` does not +/// exist without the feature; they fall back to file I/O and still exercise surrounding logic. +#[cfg(any(test, feature = "verify_sst_content"))] +pub(crate) fn mmap_access_mode() -> AccessMode { + #[cfg(not(feature = "mmap"))] + return AccessMode::File; + + #[cfg(feature = "mmap")] + AccessMode::Mmap +} + +#[cfg(feature = "mmap")] fn access_mode_env_var() -> AccessMode { static ACCESS_MODE_ENV: std::sync::LazyLock = std::sync::LazyLock::new(|| { if std::env::var("TURBO_PERSISTENCE_MMAP") @@ -95,8 +121,7 @@ fn access_mode_env_var() -> AccessMode { } impl DbConfig { - /// Returns a config with all defaults, reading the `TURBO_PERSISTENCE_MMAP` env var - /// to determine the access mode. + /// Returns a config with all defaults, using the execution environment's default access mode. pub fn new() -> Self { Self { family_configs: [FamilyConfig { @@ -104,7 +129,7 @@ impl DbConfig { kind: FamilyKind::SingleValue, compression: Compression::Lz4, }; FAMILIES], - access_mode: access_mode_env_var(), + access_mode: default_access_mode(), } } } diff --git a/turbopack/crates/turbo-persistence/src/meta_file.rs b/turbopack/crates/turbo-persistence/src/meta_file.rs index e2948c70cf71..580c419a67b3 100644 --- a/turbopack/crates/turbo-persistence/src/meta_file.rs +++ b/turbopack/crates/turbo-persistence/src/meta_file.rs @@ -4,21 +4,24 @@ use std::{ mem::take, ops::Deref, path::{Path, PathBuf}, - sync::OnceLock, + sync::{Arc, OnceLock}, }; use anyhow::{Context, Result, bail, ensure}; use bitfield::bitfield; use byteorder::{BE, ReadBytesExt}; +#[cfg(feature = "mmap")] use fs_err::File; +#[cfg(feature = "mmap")] use memmap2::{Mmap, MmapOptions}; use smallvec::SmallVec; use zerocopy::{FromBytes, Immutable, IntoBytes, KnownLayout, Ref, big_endian as be}; +#[cfg(feature = "mmap")] +use crate::mmap_helper::advise_mmap_for_persistence; use crate::{ AccessMode, Compression, FamilyConfig, QueryKey, lookup_entry::LookupValue, - mmap_helper::advise_mmap_for_persistence, static_sorted_file::{BlockCache, SstLookupResult, StaticSortedFile, StaticSortedFileMetaData}, }; @@ -229,8 +232,15 @@ impl StaticSortedFileRange { } enum MetaFileBacking { + #[cfg(feature = "mmap")] Mmap(Mmap), - Bytes(Box<[u8]>), + /// Heap bytes for [`AccessMode::File`]. + /// + /// This is an `Arc<[u8]>` rather than a `Box<[u8]>` so that moving the backing into + /// [`MetaFile`] does not reborrow the bytes: a `Box` is a unique pointer, so the move + /// invalidates the `FilterRef`s that already borrow from it, which Miri reports as undefined + /// behavior under Stacked Borrows. An `Arc` moves its handle without retagging the allocation. + Bytes(Arc<[u8]>), } impl Deref for MetaFileBacking { @@ -238,6 +248,7 @@ impl Deref for MetaFileBacking { fn deref(&self) -> &Self::Target { match self { + #[cfg(feature = "mmap")] MetaFileBacking::Mmap(mmap) => mmap, MetaFileBacking::Bytes(bytes) => bytes, } @@ -307,6 +318,7 @@ impl MetaFile { access_mode: AccessMode, ) -> Result { let backing = match access_mode { + #[cfg(feature = "mmap")] AccessMode::Mmap => { let file = File::open(path)?; let mmap = unsafe { MmapOptions::new().map(file.file()) } @@ -317,7 +329,7 @@ impl MetaFile { advise_mmap_for_persistence(&mmap)?; MetaFileBacking::Mmap(mmap) } - AccessMode::File => MetaFileBacking::Bytes(fs_err::read(path)?.into_boxed_slice()), + AccessMode::File => MetaFileBacking::Bytes(fs_err::read(path)?.into()), }; // Parse the header from stable backing bytes via ReadBytesExt on &[u8]. let mut reader: &[u8] = &backing; diff --git a/turbopack/crates/turbo-persistence/src/rc_bytes.rs b/turbopack/crates/turbo-persistence/src/rc_bytes.rs index be380887b112..adb327728a08 100644 --- a/turbopack/crates/turbo-persistence/src/rc_bytes.rs +++ b/turbopack/crates/turbo-persistence/src/rc_bytes.rs @@ -6,6 +6,7 @@ use std::{ rc::Rc, }; +#[cfg(feature = "mmap")] use memmap2::Mmap; use crate::{ @@ -24,6 +25,7 @@ enum Repr { data: *const [u8], _backing: Rc<[u8]>, }, + #[cfg(feature = "mmap")] Mmap { data: *const [u8], _backing: Rc, @@ -48,6 +50,7 @@ impl RcBytes { fn backing_bytes(&self) -> Option<&[u8]> { match &self.repr { Repr::Rc { _backing, .. } => Some(_backing), + #[cfg(feature = "mmap")] Repr::Mmap { _backing, .. } => Some(_backing), Repr::Inline { .. } => None, } @@ -78,7 +81,9 @@ impl Deref for RcBytes { match &self.repr { // SAFETY: `data` points into the backing held by the same variant, which keeps it // alive for as long as `self`. - Repr::Rc { data, .. } | Repr::Mmap { data, .. } => unsafe { &**data }, + Repr::Rc { data, .. } => unsafe { &**data }, + #[cfg(feature = "mmap")] + Repr::Mmap { data, .. } => unsafe { &**data }, // Borrowed from `self`, so this is recomputed after a move rather than stored. Repr::Inline { buf, len } => &buf[..*len as usize], } @@ -112,6 +117,7 @@ impl Hash for RcBytes { } impl SharedBytes for RcBytes { + #[cfg(feature = "mmap")] type MmapHandle = Rc; fn slice(self, range: Range) -> Self { @@ -124,6 +130,7 @@ impl SharedBytes for RcBytes { Self { repr: match self.repr { Repr::Rc { _backing, .. } => Repr::Rc { data, _backing }, + #[cfg(feature = "mmap")] Repr::Mmap { _backing, .. } => Repr::Mmap { data, _backing }, Repr::Inline { .. } => unreachable!("handled above"), }, @@ -149,6 +156,7 @@ impl SharedBytes for RcBytes { data, _backing: _backing.clone(), }, + #[cfg(feature = "mmap")] Repr::Mmap { _backing, .. } => Repr::Mmap { data, _backing: _backing.clone(), @@ -160,6 +168,7 @@ impl SharedBytes for RcBytes { } } + #[cfg(feature = "mmap")] unsafe fn from_mmap(mmap: &Rc, subslice: &[u8]) -> Self { debug_assert!( is_subslice_of(subslice, mmap), diff --git a/turbopack/crates/turbo-persistence/src/shared_bytes.rs b/turbopack/crates/turbo-persistence/src/shared_bytes.rs index 92e887ff16cc..ac94095f8832 100644 --- a/turbopack/crates/turbo-persistence/src/shared_bytes.rs +++ b/turbopack/crates/turbo-persistence/src/shared_bytes.rs @@ -1,5 +1,6 @@ use std::ops::{Deref, Range}; +#[cfg(feature = "mmap")] use memmap2::Mmap; use crate::Compression; @@ -30,6 +31,7 @@ const _: () = assert!( /// once and used for both lookup (ArcBytes) and iteration (RcBytes) paths. pub trait SharedBytes: Clone + Deref + Sized { /// The ref-counted handle to the memory-mapped file. + #[cfg(feature = "mmap")] type MmapHandle: Deref; /// Returns a new instance that points to a sub-range of the current slice. @@ -50,6 +52,7 @@ pub trait SharedBytes: Clone + Deref + Sized { /// /// The caller must ensure that `subslice` points to memory within the /// given `mmap`. + #[cfg(feature = "mmap")] unsafe fn from_mmap(mmap: &Self::MmapHandle, subslice: &[u8]) -> Self; /// Creates an instance from a decompressed block. diff --git a/turbopack/crates/turbo-persistence/src/static_sorted_file.rs b/turbopack/crates/turbo-persistence/src/static_sorted_file.rs index 5107ab11922c..7c3f16168aed 100644 --- a/turbopack/crates/turbo-persistence/src/static_sorted_file.rs +++ b/turbopack/crates/turbo-persistence/src/static_sorted_file.rs @@ -1,9 +1,11 @@ +#[cfg(feature = "mmap")] +use std::ops::Range; use std::{ borrow::Cow, cmp::Ordering, hash::BuildHasherDefault, io, - ops::Range, + marker::PhantomData, path::Path, rc::Rc, sync::{ @@ -14,11 +16,14 @@ use std::{ use anyhow::{Context, Result, bail, ensure}; use fs_err::File; +#[cfg(feature = "mmap")] use memmap2::Mmap; use quick_cache::{Lifecycle, sync::GuardResult}; use rustc_hash::FxHasher; use smallvec::SmallVec; +#[cfg(feature = "mmap")] +use crate::mmap_helper::advise_mmap_for_persistence; use crate::{ AccessMode, Compression, QueryKey, arc_bytes::ArcBytes, @@ -26,7 +31,6 @@ use crate::{ compression::checksum_block, constants::MAX_INLINE_VALUE_SIZE, lookup_entry::{IterValue, LookupEntry, LookupValue}, - mmap_helper::advise_mmap_for_persistence, rc_bytes::RcBytes, shared_bytes::SharedBytes, static_sorted_file_builder::{ @@ -315,6 +319,7 @@ impl StaticSortedFileMetaData { } enum StaticSortedFileBacking { + #[cfg(feature = "mmap")] Mmap(Arc), File { file: Arc, @@ -360,6 +365,7 @@ enum IndexEntries { /// borrow from its own `backing` field, and an `ArcBytes` would bump and drop the `mmap` /// refcount on every lookup. All readers of a file share that one counter, so the contention /// scales with reader threads — measured ~3 ns single-threaded but ~70 ns at 8 threads. + #[cfg(feature = "mmap")] Mmap(Range), /// Read into memory at open time, for the non-mmap backing, which has nothing to borrow from. Owned(Box<[u8]>), @@ -419,6 +425,7 @@ impl IndexBlock { let entries = match backing { // Store a range, not the slice: `StaticSortedFile` owns the mmap these bytes live in. + #[cfg(feature = "mmap")] StaticSortedFileBacking::Mmap(mmap) => { let start = entry_bytes.as_ptr() as usize - mmap.as_ptr() as usize; IndexEntries::Mmap(start..start + entry_bytes.len()) @@ -447,6 +454,7 @@ impl StaticSortedFile { let path = db_path.join(&filename); let file = File::open(&path)?; let backing = match access_mode { + #[cfg(feature = "mmap")] AccessMode::Mmap => { let mmap = unsafe { Mmap::map(file.file()) }.with_context(|| { format!( @@ -504,12 +512,14 @@ impl StaticSortedFile { #[inline] fn index_entries(&self) -> &[[u8; INDEX_BLOCK_ENTRY_SIZE]] { let bytes = match (&self.index.entries, &self.backing) { + #[cfg(feature = "mmap")] (IndexEntries::Mmap(range), StaticSortedFileBacking::Mmap(mmap)) => { &mmap[range.clone()] } (IndexEntries::Owned(bytes), _) => &bytes[..], // `IndexBlock::parse` only produces `Mmap` entries for an mmap backing, and the // backing never changes after open. + #[cfg(feature = "mmap")] (IndexEntries::Mmap(_), StaticSortedFileBacking::File { .. }) => unreachable!( "mmap-ranged index entries with a file backing in {:08}.sst", self.meta.sequence_number @@ -749,17 +759,20 @@ impl StaticSortedFile { /// its refcount belongs to one cache entry rather than the whole file. enum BlockRef<'l> { /// Borrowed from the memory-mapped file. + #[cfg(feature = "mmap")] Mmap(&'l [u8]), - /// Owned, and shared with the block cache. - Cached(ArcBytes), + /// Owned, and shared with the block cache. The zero-sized marker keeps the backing borrow + /// lifetime represented in builds where the mmap variant is disabled. + Cached(ArcBytes, PhantomData<&'l ()>), } impl BlockRef<'_> { #[inline] fn as_slice(&self) -> &[u8] { match self { + #[cfg(feature = "mmap")] BlockRef::Mmap(data) => data, - BlockRef::Cached(block) => block, + BlockRef::Cached(block, _) => block, } } @@ -767,8 +780,10 @@ impl BlockRef<'_> { /// /// Only needed by callers that hand the bytes to something outliving the lookup. #[inline] + #[cfg_attr(not(feature = "mmap"), allow(unused_variables))] fn into_owned(self, backing: &StaticSortedFileBacking) -> ArcBytes { match self { + #[cfg(feature = "mmap")] BlockRef::Mmap(data) => { let StaticSortedFileBacking::Mmap(mmap) = backing else { // `get_or_read_block` only borrows from an mmap backing. @@ -777,7 +792,7 @@ impl BlockRef<'_> { // SAFETY: the borrow came from this mmap, via `get_or_read_block`. unsafe { ArcBytes::from_mmap(mmap, data) } } - BlockRef::Cached(block) => block, + BlockRef::Cached(block, _) => block, } } } @@ -799,6 +814,11 @@ fn get_or_read_block<'l>( verified_blocks: &[AtomicU64], compression: Compression, ) -> Result> { + // `verified_blocks` tracks which mmap-backed blocks already passed their checksum. There is + // no mmap backing without the feature, so the bitmap is unused there. + #[cfg(not(feature = "mmap"))] + let _ = verified_blocks; + #[cfg(feature = "mmap")] let mmap_block = if let StaticSortedFileBacking::Mmap(mmap) = backing { let (uncompressed_length, checksum, block_data) = get_raw_block_slice(mmap, meta, block_index).with_context(|| { @@ -818,6 +838,10 @@ fn get_or_read_block<'l>( } else { None }; + // Without an mmap backing there is never a zero-copy block to borrow, so every block goes + // through the decompress/cache path below. + #[cfg(not(feature = "mmap"))] + let mmap_block: Option<(u32, u32, &[u8])> = None; // Compressed: check cache; decompress and insert on miss. // File-backed blocks use the same cache, including uncompressed ones. @@ -834,6 +858,7 @@ fn get_or_read_block<'l>( // A cached block may have been evicted, so re-reading still // benefits from the bitmap to skip redundant CRC verification. match backing { + #[cfg(feature = "mmap")] StaticSortedFileBacking::Mmap(_) => verify_checksum_once( meta, &block_data, @@ -862,11 +887,13 @@ fn get_or_read_block<'l>( } GuardResult::Timeout => unreachable!(), }, + PhantomData, )) } /// Gets the raw block slice directly from a memory-mapped file. /// Returns `(uncompressed_length, checksum, block_data)`. +#[cfg(feature = "mmap")] fn get_raw_block_slice<'a>( mmap: &'a Mmap, meta: &StaticSortedFileMetaData, @@ -932,6 +959,7 @@ fn get_raw_block<'a>( block_index: u16, ) -> Result<(u32, u32, Cow<'a, [u8]>)> { match backing { + #[cfg(feature = "mmap")] StaticSortedFileBacking::Mmap(mmap) => { let (uncompressed_length, checksum, block) = get_raw_block_slice(mmap, meta, block_index)?; @@ -1026,6 +1054,7 @@ fn verify_checksum( /// since the check is deterministic and idempotent. Verification failures are /// *not* recorded in the bitmap, so a corrupted block will be re-checked (and /// fail again) on every access. +#[cfg_attr(not(feature = "mmap"), allow(dead_code))] fn verify_checksum_once( meta: &StaticSortedFileMetaData, data: &[u8], @@ -1054,6 +1083,7 @@ fn read_block_lookup( verify_checksum(meta, &block, checksum, block_index)?; if uncompressed_length == 0 { return match (backing, block) { + #[cfg(feature = "mmap")] (StaticSortedFileBacking::Mmap(mmap), Cow::Borrowed(block)) => { // SAFETY: block points into mmap. Ok(unsafe { ArcBytes::from_mmap(mmap, block) }) @@ -1077,6 +1107,7 @@ fn get_raw_block_iter( block_index: u16, ) -> Result<(u32, u32, RcBytes)> { match backing { + #[cfg(feature = "mmap")] StaticSortedFileIterBacking::Mmap(mmap) => { let (uncompressed_length, checksum, block) = get_raw_block_slice(mmap, meta, block_index)?; @@ -1184,6 +1215,7 @@ fn handle_key_match_generic( } enum StaticSortedFileIterBacking { + #[cfg(feature = "mmap")] Mmap(Rc), File { file: Rc, @@ -1290,6 +1322,7 @@ impl StaticSortedFileIter { let path = db_path.join(&filename); let file = File::open(&path)?; let backing = match access_mode { + #[cfg(feature = "mmap")] AccessMode::Mmap => { let mmap = unsafe { Mmap::map(file.file()) }.with_context(|| { format!( diff --git a/turbopack/crates/turbo-persistence/src/static_sorted_file_builder.rs b/turbopack/crates/turbo-persistence/src/static_sorted_file_builder.rs index 2cefa8f15ec3..fb08faea71b1 100644 --- a/turbopack/crates/turbo-persistence/src/static_sorted_file_builder.rs +++ b/turbopack/crates/turbo-persistence/src/static_sorted_file_builder.rs @@ -1434,7 +1434,6 @@ impl IndexBlockBuilder { mod tests { use super::*; use crate::{ - AccessMode, key::hash_key, lookup_entry::LookupValue, static_sorted_file::{ @@ -1566,7 +1565,7 @@ mod tests { block_count: meta.block_count, }, Compression::Lz4, - AccessMode::Mmap, + crate::mmap_access_mode(), ) } @@ -1913,7 +1912,7 @@ mod tests { block_count: meta1.block_count, }, Compression::Lz4, - AccessMode::Mmap, + crate::mmap_access_mode(), )?; let sst2 = StaticSortedFile::open( dir.path(), @@ -1922,7 +1921,7 @@ mod tests { block_count: meta2.block_count, }, Compression::Lz4, - AccessMode::Mmap, + crate::mmap_access_mode(), )?; let kc = make_cache(); let vc = make_cache(); diff --git a/turbopack/crates/turbo-persistence/src/tests.rs b/turbopack/crates/turbo-persistence/src/tests.rs index 524116129d6a..a28d10be11d2 100644 --- a/turbopack/crates/turbo-persistence/src/tests.rs +++ b/turbopack/crates/turbo-persistence/src/tests.rs @@ -1,23 +1,36 @@ -use std::{fs, path::Path, time::Instant}; +#[cfg(not(miri))] +use std::time::Instant; +use std::{fs, path::Path}; use anyhow::Result; +#[cfg(not(miri))] use rayon::iter::{IntoParallelIterator, ParallelIterator}; use rstest::rstest; +// Miri rejects a process that exits while rayon's global worker threads are still parked, so +// run the same tests on the serial scheduler there. The alias keeps the test bodies identical. +#[cfg(miri)] +use crate::parallel_scheduler::SerialScheduler as RayonParallelScheduler; use crate::{ AccessMode, Compression, DbConfig, FamilyConfig, FamilyKind, - constants::{MAX_INLINE_VALUE_SIZE, MAX_MEDIUM_VALUE_SIZE, MAX_SMALL_VALUE_SIZE}, + constants::MAX_INLINE_VALUE_SIZE, db::{CompactConfig, TurboPersistence, read_current_version}, lookup_entry::IterValue, + static_sorted_file::{StaticSortedFileIter, StaticSortedFileMetaData}, +}; +#[cfg(not(miri))] +use crate::{ + constants::{MAX_MEDIUM_VALUE_SIZE, MAX_SMALL_VALUE_SIZE}, meta_file::MetaFile, parallel_scheduler::ParallelScheduler, - static_sorted_file::{StaticSortedFileIter, StaticSortedFileMetaData}, write_batch::WriteBatch, }; +#[cfg(not(miri))] #[derive(Clone, Copy)] struct RayonParallelScheduler; +#[cfg(not(miri))] impl ParallelScheduler for RayonParallelScheduler { fn block_in_place(&self, f: impl FnOnce() -> R + Send) -> R where @@ -108,6 +121,7 @@ impl ParallelScheduler for RayonParallelScheduler { } } +#[cfg(not(miri))] fn tuple_key(prefix: u8, suffix: [u8; 4]) -> Box<[u8]> { let mut key = Vec::with_capacity(1 + suffix.len()); key.push(prefix); @@ -118,7 +132,7 @@ fn tuple_key(prefix: u8, suffix: [u8; 4]) -> Box<[u8]> { fn config_with_mmap(mmap: bool) -> DbConfig { DbConfig { access_mode: if mmap { - AccessMode::Mmap + crate::mmap_access_mode() } else { AccessMode::File }, @@ -152,7 +166,7 @@ fn multi_value_config_with_mmap(mmap: bool) -> DbConfig<1> { compression: Compression::Lz4, }], access_mode: if mmap { - AccessMode::Mmap + crate::mmap_access_mode() } else { AccessMode::File }, @@ -166,6 +180,9 @@ fn open_multi_value_db( open_db_with_config(path, multi_value_config_with_mmap(mmap)) } +// This stress test writes more than ten million entries and uses Rayon directly, so it is too slow +// to run under Miri and depends on concurrency that Miri cannot provide efficiently. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -552,6 +569,9 @@ fn full_cycle(#[case] mmap: bool) -> Result<()> { Ok(()) } +// This test exceeded 20 minutes under Miri because it writes and repeatedly reads tens of +// thousands of entries across multiple reopen/compaction cycles. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -664,6 +684,8 @@ fn persist_changes(#[case] mmap: bool) -> Result<()> { Ok(()) } +// This 50-iteration compaction/restore stress test exceeded 20 minutes under Miri. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -751,6 +773,8 @@ fn partial_compaction(#[case] mmap: bool) -> Result<()> { Ok(()) } +// This repeated large-file merge/removal stress test exceeded 20 minutes under Miri. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -1047,6 +1071,8 @@ fn batch_get_large_batch(#[case] mmap: bool) -> Result<()> { Ok(()) } +// Crossing the real blob-size boundary makes this test exceed 20 minutes under Miri. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -1372,6 +1398,8 @@ fn batch_get_after_restore(#[case] mmap: bool) -> Result<()> { /// Test that compaction works with many small values without overflowing block indices. /// Reproduces a CI benchmark failure with key_4/value_512/entries_1.98Mi/compacted. +// Miri was killed while processing this production-scale, two-million-entry workload. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -1418,6 +1446,8 @@ fn many_small_values_compaction(#[case] mmap: bool) -> Result<()> { /// Test compaction with MAX_SMALL_VALUE_SIZE (4096-byte) values. /// Worst case for small value blocks: fewest entries per block. +// This production-scale 512K-entry workload exceeded 20 minutes under Miri. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -1463,6 +1493,8 @@ fn many_max_small_values_compaction(#[case] mmap: bool) -> Result<()> { /// Test compaction with 4097-byte values (minimum medium size). /// Each medium value gets its own dedicated block, so this is the worst case for block count. +// This production-scale 128K-entry workload exceeded 20 minutes under Miri. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -1974,6 +2006,7 @@ fn multi_value_tombstone_shadows_older_sst_only(#[case] mmap: bool) -> Result<() } /// Returns the number of `.blob` files in the given directory. +#[cfg(not(miri))] fn count_blob_files(dir: &Path) -> usize { fs::read_dir(dir) .unwrap() @@ -1984,6 +2017,9 @@ fn count_blob_files(dir: &Path) -> usize { /// Test that compaction deletes blob files when their entries are superseded /// by newer values (SingleValue family). +// The first access-mode variant exceeded 20 minutes under Miri while processing the production-size +// blob boundary. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -2043,6 +2079,9 @@ fn compaction_deletes_superseded_blob(#[case] mmap: bool) -> Result<()> { /// Test that compaction deletes blob files when a key is deleted via tombstone /// (SingleValue family). +// The first access-mode variant exceeded 20 minutes under Miri while processing the production-size +// blob boundary. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -2094,6 +2133,9 @@ fn compaction_deletes_blob_on_tombstone(#[case] mmap: bool) -> Result<()> { /// Test that compaction deletes blob files for MultiValue families when a /// tombstone prunes older blob entries. +// The first access-mode variant exceeded 20 minutes under Miri while processing the production-size +// blob boundary. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -2143,6 +2185,9 @@ fn compaction_deletes_blob_multi_value_tombstone(#[case] mmap: bool) -> Result<( /// Test that compaction preserves blob files that are still referenced /// (not superseded). +// The first access-mode variant exceeded 20 minutes under Miri while processing the production-size +// blob boundary. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -2457,7 +2502,9 @@ fn count_tombstones( sequence_number: entry.sequence_number, block_count: entry.block_count, }; - for item in StaticSortedFileIter::open(path, sst, Compression::Lz4, AccessMode::Mmap)? { + for item in + StaticSortedFileIter::open(path, sst, Compression::Lz4, crate::mmap_access_mode())? + { if matches!( item?.value, IterValue::KeyDeleted | IterValue::KeyValueDeleted { .. } @@ -2634,6 +2681,8 @@ fn compaction_keeps_tombstone_when_older_sst_has_the_key() -> Result<()> { /// /// This is the case a sequence-number threshold gets wrong — it would treat the skipped SST as /// part of the job and drop a tombstone that is still load-bearing. +// This multi-SST partial-compaction scenario exceeded 20 minutes under Miri. +#[cfg(not(miri))] #[test] fn compaction_keeps_tombstone_when_skipped_sst_has_the_key() -> Result<()> { let tempdir = tempfile::tempdir()?; @@ -2814,6 +2863,8 @@ fn valued_tombstone_rejects_single_value_families() -> Result<()> { Ok(()) } +// This 8,000-key meta-sharding compaction test exceeded 20 minutes under Miri. +#[cfg(not(miri))] #[rstest] #[case(true)] #[case(false)] @@ -2821,7 +2872,7 @@ fn partial_compaction_retires_fully_consumed_meta_files(#[case] mmap: bool) -> R let tempdir = tempfile::tempdir()?; let path = tempdir.path(); let access_mode = if mmap { - AccessMode::Mmap + crate::mmap_access_mode() } else { AccessMode::File }; diff --git a/turbopack/crates/turbo-persistence/src/write_batch.rs b/turbopack/crates/turbo-persistence/src/write_batch.rs index 9c6a065f851e..c03ad1c407dc 100644 --- a/turbopack/crates/turbo-persistence/src/write_batch.rs +++ b/turbopack/crates/turbo-persistence/src/write_batch.rs @@ -533,7 +533,6 @@ impl<'db, K: StoreKey + Send + Sync, S: ParallelScheduler, const FAMILIES: usize use core::panic; use crate::{ - AccessMode, collector_entry::CollectorEntryValue, key::hash_key, lookup_entry::LookupValue, @@ -551,7 +550,7 @@ impl<'db, K: StoreKey + Send + Sync, S: ParallelScheduler, const FAMILIES: usize block_count: meta.block_count, }, self.family_configs[usize_from_u32(family)].compression, - AccessMode::Mmap, + crate::mmap_access_mode(), )?; let cache2 = BlockCache::with( 10, diff --git a/turbopack/crates/turbo-rcstr/src/dynamic.rs b/turbopack/crates/turbo-rcstr/src/dynamic.rs index 81de8a7838c9..80b08344613e 100644 --- a/turbopack/crates/turbo-rcstr/src/dynamic.rs +++ b/turbopack/crates/turbo-rcstr/src/dynamic.rs @@ -77,12 +77,12 @@ pub(crate) fn new_atom(text: Cow<'_, str>) -> RcStr { /// Construct a new dynamic RcStr from a DynamicPrehashedString pub(crate) fn new_atom_from_prehashed(prehashed: DynamicPrehashedString) -> RcStr { let entry: Arc = Arc::new(prehashed); - let mut entry = Arc::into_raw(entry); - debug_assert!(0 == entry as u8 & TAG_MASK); - entry = ((entry as usize) | DYNAMIC_TAG as usize) as *mut DynamicPrehashedString; + let entry = Arc::into_raw(entry); + debug_assert_eq!(entry.addr() & TAG_MASK as usize, 0); + let entry = entry.map_addr(|addr| addr | DYNAMIC_TAG as usize); let ptr: NonNull = unsafe { // Safety: Arc::into_raw returns a non-null pointer - NonNull::new_unchecked(entry as *mut _) + NonNull::new_unchecked(entry.cast_mut()) }; RcStr { diff --git a/turbopack/crates/turbo-rcstr/src/lib.rs b/turbopack/crates/turbo-rcstr/src/lib.rs index 7fd8ae6a878f..6a09869e41e8 100644 --- a/turbopack/crates/turbo-rcstr/src/lib.rs +++ b/turbopack/crates/turbo-rcstr/src/lib.rs @@ -951,6 +951,8 @@ mod tests { let decoded: RcStr = turbo_bincode_decode(&encoded).unwrap(); assert_eq!(decoded.as_str(), STATIC_STR); // Decoded via peek_read path should find the static constant + // Miri does not populate the linker section used by scattered-collect. + #[cfg(not(miri))] assert_eq!(decoded.tag(), STATIC_TAG); } diff --git a/turbopack/crates/turbo-rcstr/src/tagged_value.rs b/turbopack/crates/turbo-rcstr/src/tagged_value.rs index 7d200e7b871d..0eeeef1c3142 100644 --- a/turbopack/crates/turbo-rcstr/src/tagged_value.rs +++ b/turbopack/crates/turbo-rcstr/src/tagged_value.rs @@ -37,6 +37,13 @@ //! Reading the value back as an integer (`get_ptr`, `get_value`, `tag_byte`) only ever happens at //! run time, where pointer → integer is perfectly legal. +#[cfg(not(any( + target_pointer_width = "32", + target_pointer_width = "16", + feature = "atom_size_64", + feature = "atom_size_128" +)))] +use std::num::NonZeroUsize; use std::{num::NonZeroU8, os::raw::c_void, ptr::NonNull, slice}; use self::raw_types::*; @@ -209,11 +216,35 @@ impl TaggedValue { #[inline(always)] pub const fn new_tag(value: NonZeroU8) -> Self { - // An integer → pointer transmute, which const evaluation permits. - let value = value.get() as RawTaggedValue; - Self { - #[allow(clippy::transmute_int_to_non_zero)] - value: unsafe { std::mem::transmute::(value) }, + #[cfg(not(any( + target_pointer_width = "32", + target_pointer_width = "16", + feature = "atom_size_64", + feature = "atom_size_128" + )))] + { + // A provenance-free integer → pointer conversion for the pointer representation. + Self { + value: NonNull::without_provenance( + NonZeroUsize::new(value.get() as usize).unwrap(), + ), + } + } + #[cfg(any( + target_pointer_width = "32", + target_pointer_width = "16", + feature = "atom_size_64", + feature = "atom_size_128" + ))] + { + // An integer → pointer transmute, which const evaluation permits. + let value = value.get() as RawTaggedValue; + Self { + #[allow(clippy::transmute_int_to_non_zero)] + value: unsafe { + std::mem::transmute::(value) + }, + } } } @@ -240,12 +271,26 @@ impl TaggedValue { feature = "atom_size_128" )))] { - (self.value.as_ptr() as usize & !(TAG_MASK as usize)) as _ + self.value + .as_ptr() + .map_addr(|addr| addr & !(TAG_MASK as usize)) + .cast_const() + .cast() } } #[inline(always)] fn get_value(&self) -> RawTaggedValue { + #[cfg(not(any( + target_pointer_width = "32", + target_pointer_width = "16", + feature = "atom_size_64", + feature = "atom_size_128" + )))] + { + // Read the pointer representation's address without exposing provenance. + self.value.addr().get() + } #[cfg(all( any(target_pointer_width = "32", target_pointer_width = "16"), not(feature = "atom_size_128") @@ -255,10 +300,18 @@ impl TaggedValue { // value as an integer is well defined. Again, run time only. unsafe { std::mem::transmute::(self.value) } } - #[cfg(not(all( - any(target_pointer_width = "32", target_pointer_width = "16"), - not(feature = "atom_size_128") - )))] + #[cfg(all( + any( + target_pointer_width = "32", + target_pointer_width = "16", + feature = "atom_size_64", + feature = "atom_size_128" + ), + not(all( + any(target_pointer_width = "32", target_pointer_width = "16"), + not(feature = "atom_size_128") + )) + ))] { unsafe { std::mem::transmute::, RawTaggedValue>(Some( diff --git a/turbopack/crates/turbo-tasks-backend/Cargo.toml b/turbopack/crates/turbo-tasks-backend/Cargo.toml index b4ad77873dd1..6743fb49e54e 100644 --- a/turbopack/crates/turbo-tasks-backend/Cargo.toml +++ b/turbopack/crates/turbo-tasks-backend/Cargo.toml @@ -59,13 +59,18 @@ smallvec = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } turbo-bincode = { workspace = true } -turbo-persistence = { workspace = true } turbo-rcstr = { workspace = true } turbo-tasks = { workspace = true } turbo-tasks-hash = { workspace = true } turbo-tasks-malloc = { workspace = true, default-features = false } thread_local = { workspace = true } +[target.'cfg(not(target_family = "wasm"))'.dependencies] +turbo-persistence = { workspace = true } + +[target.'cfg(target_family = "wasm")'.dependencies] +turbo-persistence = { workspace = true, default-features = false } + [dev-dependencies] async-trait = { workspace = true } criterion = { workspace = true, features = ["async_tokio"] } diff --git a/turbopack/crates/turbo-tasks-backend/src/kv_backing_storage.rs b/turbopack/crates/turbo-tasks-backend/src/kv_backing_storage.rs index 134204a0f9bc..a979cfceea4e 100644 --- a/turbopack/crates/turbo-tasks-backend/src/kv_backing_storage.rs +++ b/turbopack/crates/turbo-tasks-backend/src/kv_backing_storage.rs @@ -620,6 +620,8 @@ mod tests { /// Tests that multiple distinct keys written in a single batch with flush can be read back. /// This mirrors the actual save_snapshot pattern: write many TaskCache entries, flush, commit. + // This test is too slow to run under Miri. + #[cfg(not(miri))] #[tokio::test(flavor = "multi_thread")] async fn test_batch_write_with_flush_and_reopen() -> Result<()> { let tempdir = tempfile::tempdir()?; diff --git a/turbopack/crates/turbo-tasks-backend/src/utils/dash_map_multi.rs b/turbopack/crates/turbo-tasks-backend/src/utils/dash_map_multi.rs index d1d5cf43216d..fe58815b5094 100644 --- a/turbopack/crates/turbo-tasks-backend/src/utils/dash_map_multi.rs +++ b/turbopack/crates/turbo-tasks-backend/src/utils/dash_map_multi.rs @@ -228,6 +228,8 @@ mod tests { use super::*; + // This test is too slow to run under Miri. + #[cfg(not(miri))] #[test] fn stress_deadlock() { const N: usize = 100000; diff --git a/turbopack/crates/turbo-tasks-backend/tests/inline_read_execution.rs b/turbopack/crates/turbo-tasks-backend/tests/inline_read_execution.rs index c35fccfb1db2..1445cdfc9e1f 100644 --- a/turbopack/crates/turbo-tasks-backend/tests/inline_read_execution.rs +++ b/turbopack/crates/turbo-tasks-backend/tests/inline_read_execution.rs @@ -358,6 +358,8 @@ async fn counted_leaf(nonce: u32) -> Result> { /// Inline execution nests: reading the deepest task of a chain of uncomputed tasks executes them /// one inside the other. The nesting cap keeps that from growing the stack without bounds. +// This test is too slow to run under Miri. +#[cfg(not(miri))] #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn test_deep_dependency_chain() { let mut nonce = 0; diff --git a/turbopack/crates/turbo-tasks-fs/src/watcher/mod.rs b/turbopack/crates/turbo-tasks-fs/src/watcher/mod.rs index f9f3239fdf2e..bdc042c89576 100644 --- a/turbopack/crates/turbo-tasks-fs/src/watcher/mod.rs +++ b/turbopack/crates/turbo-tasks-fs/src/watcher/mod.rs @@ -1134,6 +1134,9 @@ mod tests { /// `recursive_mode` is set explicitly rather than left to the platform default so that both /// watching strategies are covered on every host. `TURBO_TASKS_FORCE_WATCH_MODE` still /// overrides it, collapsing these into two cases. + // Miri cannot run the native cases because inotify is unsupported, while the polling cases + // require Turbo Tasks' link-section registry, which is unavailable under Miri. + #[cfg(not(miri))] #[rstest] #[case::native_recursive(None, DiskWatcherRecursiveMode::Recursive)] #[case::native_non_recursive(None, DiskWatcherRecursiveMode::NonRecursive)] diff --git a/turbopack/crates/turbo-tasks-macros-tests/tests/trybuild.rs b/turbopack/crates/turbo-tasks-macros-tests/tests/trybuild.rs index 314299db6f6a..730a02c21923 100644 --- a/turbopack/crates/turbo-tasks-macros-tests/tests/trybuild.rs +++ b/turbopack/crates/turbo-tasks-macros-tests/tests/trybuild.rs @@ -1,3 +1,6 @@ +// Miri cannot run trybuild because its subprocesses require the unsupported `posix_spawnattr_init`. +#![cfg(not(miri))] + // Unset RUSTC_WRAPPER before trybuild tests run. When sccache wraps rustc, it // emits "warning: ignoring -C extra-filename flag due to -o flag" which pollutes // trybuild's stderr snapshot comparisons. Unsetting it here means only the diff --git a/turbopack/crates/turbo-tasks-malloc/Cargo.toml b/turbopack/crates/turbo-tasks-malloc/Cargo.toml index 2fff14ae6dd2..5c5b208854c0 100644 --- a/turbopack/crates/turbo-tasks-malloc/Cargo.toml +++ b/turbopack/crates/turbo-tasks-malloc/Cargo.toml @@ -18,18 +18,18 @@ harness = false [dev-dependencies] criterion = { workspace = true } -[target.'cfg(not(target_family = "wasm"))'.dependencies] +[target.'cfg(all(not(target_family = "wasm"), not(miri)))'.dependencies] libmimalloc-sys = { version = "0.1.44", features = [ "extended", ], optional = true } -[target.'cfg(not(any(target_os = "linux", target_family = "wasm")))'.dependencies] +[target.'cfg(not(any(target_os = "linux", target_family = "wasm", miri)))'.dependencies] mimalloc = { version = "0.1.48", features = [ "v3", "extended", ], optional = true } -[target.'cfg(all(target_os = "linux", not(target_family = "wasm")))'.dependencies] +[target.'cfg(all(target_os = "linux", not(target_family = "wasm"), not(miri)))'.dependencies] mimalloc = { version = "0.1.48", features = [ "v3", "extended", diff --git a/turbopack/crates/turbo-tasks-malloc/src/counter.rs b/turbopack/crates/turbo-tasks-malloc/src/counter.rs index 4a94073ae86a..a864af80958b 100644 --- a/turbopack/crates/turbo-tasks-malloc/src/counter.rs +++ b/turbopack/crates/turbo-tasks-malloc/src/counter.rs @@ -11,7 +11,7 @@ use std::{cell::UnsafeCell, ptr::NonNull}; -#[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] +#[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] pub use self::global::get; use crate::AllocationCounters; @@ -20,7 +20,7 @@ use crate::AllocationCounters; /// Only compiled without the `custom_allocator` feature; see the module docs. Each thread holds /// its buffer in its own [`ThreadLocalCounter`] and passes it in, so the counter's state lives in /// exactly one place. -#[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] +#[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] mod global { use std::sync::atomic::{AtomicUsize, Ordering}; @@ -105,7 +105,7 @@ struct ThreadLocalCounter { /// global counter desprite not being allocated yet. It is unsigned so that /// means the global counter is always equal or greater than the real /// value. - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] buffer: usize, allocation_counters: AllocationCounters, } @@ -113,7 +113,11 @@ struct ThreadLocalCounter { impl ThreadLocalCounter { const fn new() -> Self { Self { - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all( + feature = "custom_allocator", + not(target_family = "wasm"), + not(miri) + )))] buffer: 0, allocation_counters: AllocationCounters::new(), } @@ -124,7 +128,7 @@ impl ThreadLocalCounter { self.allocation_counters.allocations += size; self.allocation_counters.allocation_count += 1; - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] self.buffered_add(size); } @@ -133,7 +137,7 @@ impl ThreadLocalCounter { self.allocation_counters.deallocations += size; self.allocation_counters.deallocation_count += 1; - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] self.buffered_remove(size); } @@ -144,7 +148,7 @@ impl ThreadLocalCounter { self.allocation_counters.allocations += new_size; self.allocation_counters.allocation_count += 1; - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] { match old_size.cmp(&new_size) { std::cmp::Ordering::Equal => {} @@ -155,7 +159,7 @@ impl ThreadLocalCounter { } fn unload(&mut self) { - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] global::flush_all(&mut self.buffer); self.allocation_counters = AllocationCounters::default(); } @@ -279,7 +283,7 @@ mod tests { /// read — it is process-wide, and this binary installs [`crate::TurboMalloc`] as its global /// allocator, so every other thread moves it concurrently. `buffer` is thread-local and /// exact. - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] #[test] fn counting() { use super::global::{MAX_BUFFER, TARGET_BUFFER}; diff --git a/turbopack/crates/turbo-tasks-malloc/src/lib.rs b/turbopack/crates/turbo-tasks-malloc/src/lib.rs index 70e16c7cd577..ad5bcb96db12 100644 --- a/turbopack/crates/turbo-tasks-malloc/src/lib.rs +++ b/turbopack/crates/turbo-tasks-malloc/src/lib.rs @@ -96,7 +96,7 @@ impl TurboMalloc { /// Without the `custom_allocator` feature this is a process-wide live-bytes counter instead, /// which is approximate because threads buffer their updates. pub fn memory_usage() -> usize { - #[cfg(all(feature = "custom_allocator", not(target_family = "wasm")))] + #[cfg(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri)))] { // `current_commit` is a relaxed atomic load, but `mi_process_info` also calls // `_mi_prim_process_info`, which is a `getrusage` (plus a `task_info` on macOS). All @@ -117,7 +117,7 @@ impl TurboMalloc { } current_commit } - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] { self::counter::get() } @@ -139,11 +139,11 @@ impl TurboMalloc { /// force=true: do all the work of `process=false` and then process global shared structures and /// return memory to the OS if possible, this is much slower and should only be done rarely. pub fn collect(force: bool) { - #[cfg(all(feature = "custom_allocator", not(target_family = "wasm")))] + #[cfg(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri)))] unsafe { libmimalloc_sys::mi_collect(force); } - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] { let _ = force; } @@ -178,17 +178,17 @@ impl TurboMalloc { /// Get the allocator for this platform that we should wrap with TurboMalloc. #[inline] fn base_alloc() -> &'static impl GlobalAlloc { - #[cfg(all(feature = "custom_allocator", not(target_family = "wasm")))] + #[cfg(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri)))] return &mimalloc::MiMalloc; - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] return &std::alloc::System; } #[allow(unused_variables)] unsafe fn base_alloc_size(ptr: *const u8, layout: Layout) -> usize { - #[cfg(all(feature = "custom_allocator", not(target_family = "wasm")))] + #[cfg(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri)))] return unsafe { mimalloc::MiMalloc.usable_size(ptr) }; - #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"))))] + #[cfg(not(all(feature = "custom_allocator", not(target_family = "wasm"), not(miri))))] return layout.size(); } @@ -271,7 +271,7 @@ mod tests { // On all supported platforms the value must be reported. #[cfg(any( - all(target_os = "linux", not(target_family = "wasm")), + all(target_os = "linux", not(target_family = "wasm"), not(miri)), target_os = "macos", windows, ))] @@ -279,7 +279,7 @@ mod tests { // On unsupported platforms we expect None and have nothing further to assert. #[cfg(not(any( - all(target_os = "linux", not(target_family = "wasm")), + all(target_os = "linux", not(target_family = "wasm"), not(miri)), target_os = "macos", windows, )))] diff --git a/turbopack/crates/turbo-tasks-malloc/src/memory_pressure.rs b/turbopack/crates/turbo-tasks-malloc/src/memory_pressure.rs index 8ace0be8bf11..3eeb5a8d1e0d 100644 --- a/turbopack/crates/turbo-tasks-malloc/src/memory_pressure.rs +++ b/turbopack/crates/turbo-tasks-malloc/src/memory_pressure.rs @@ -18,7 +18,7 @@ fn clamp_percent(value: f64) -> u8 { value.round().clamp(0.0, 100.0) as u8 } -#[cfg(all(target_os = "linux", not(target_family = "wasm")))] +#[cfg(all(target_os = "linux", not(target_family = "wasm"), not(miri)))] mod platform { use super::clamp_percent; @@ -184,7 +184,7 @@ mod platform { } #[cfg(not(any( - all(target_os = "linux", not(target_family = "wasm")), + all(target_os = "linux", not(target_family = "wasm"), not(miri)), target_os = "macos", windows, )))] diff --git a/turbopack/crates/turbo-tasks/src/priority_runner.rs b/turbopack/crates/turbo-tasks/src/priority_runner.rs index 16c6ba03f1c7..d8d0e0994fa1 100644 --- a/turbopack/crates/turbo-tasks/src/priority_runner.rs +++ b/turbopack/crates/turbo-tasks/src/priority_runner.rs @@ -697,6 +697,7 @@ mod tests { assert_eq!(runner.total_queued(), 3); } + #[cfg(not(miri))] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_cpu_bound_tasks() { struct ExecutorImpl; diff --git a/turbopack/crates/turbo-tasks/src/scope_unbounded.rs b/turbopack/crates/turbo-tasks/src/scope_unbounded.rs index 44f19864559c..8d2a44834c89 100644 --- a/turbopack/crates/turbo-tasks/src/scope_unbounded.rs +++ b/turbopack/crates/turbo-tasks/src/scope_unbounded.rs @@ -916,6 +916,8 @@ mod tests { } /// Sustained work keeps workers alive rather than churning them: + // This test is too slow to run under Miri. + #[cfg(not(miri))] #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn test_unbounded_busy_queue_does_not_churn_workers() { const ITEMS: usize = 20_000; diff --git a/turbopack/crates/turbopack-css/src/process.rs b/turbopack/crates/turbopack-css/src/process.rs index 96deade3c2ff..6c9053c793d7 100644 --- a/turbopack/crates/turbopack-css/src/process.rs +++ b/turbopack/crates/turbopack-css/src/process.rs @@ -880,6 +880,8 @@ mod tests { assert_ne!(lint_lightningcss(code), vec![], "lightningcss: {code}"); } + // Lightning CSS currently triggers a Miri Stacked Borrows violation in its string parser. + #[cfg(not(miri))] #[test] fn css_module_pure_lint() { assert_lint_success( @@ -1008,6 +1010,8 @@ mod tests { ); } + // Lightning CSS currently triggers a Miri Stacked Borrows violation in its string parser. + #[cfg(not(miri))] #[test] fn strip_bom_lets_lightningcss_parse() { let with_bom = "\u{feff}@layer a {}"; diff --git a/turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/predicates.rs b/turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/predicates.rs index cf5e82bf7c4e..9478fce6ebde 100644 --- a/turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/predicates.rs +++ b/turbopack/crates/turbopack-ecmascript/src/analyzer/jsvalue/predicates.rs @@ -481,108 +481,172 @@ mod tests { use crate::analyzer::{Bump, ConstantValue, JsValue, ThreadLocal, graph::EvalContext}; - // A leaked arena for building test `JsValue`s with a `'static` lifetime. Tests are - // short-lived processes, so the leak is inconsequential. - fn test_arena() -> &'static Bump { - Box::leak(Box::new(Bump::new())) + #[derive(Clone, Copy)] + enum TestAtom { + Num(f64), + Str(&'static str), + True, + False, + Null, + Undefined, + } + + impl TestAtom { + fn build<'a>(self, _arena: &'a Bump) -> JsValue<'a> { + match self { + TestAtom::Num(value) => JsValue::from(value), + TestAtom::Str(value) => JsValue::from(value), + TestAtom::True => ConstantValue::True.into(), + TestAtom::False => ConstantValue::False.into(), + TestAtom::Null => ConstantValue::Null.into(), + TestAtom::Undefined => ConstantValue::Undefined.into(), + } + } } - // `construct_test_ternary(cons, alt)` builds a ternary with an unknown test condition. - fn construct_test_ternary(cons: JsValue<'static>, alt: JsValue<'static>) -> JsValue<'static> { - JsValue::tenary( - test_arena(), - JsValue::unknown_empty(false, rcstr!("test")), - cons, - alt, - ) + #[derive(Clone, Copy)] + enum TestValue { + Atom(TestAtom), + PromiseNull, + Ternary(TestAtom, TestAtom), + } + + impl TestValue { + fn build(self, arena: &Bump) -> JsValue<'_> { + match self { + TestValue::Atom(value) => value.build(arena), + TestValue::PromiseNull => JsValue::promise(arena, ConstantValue::Null.into()), + TestValue::Ternary(consequent, alternate) => JsValue::tenary( + arena, + JsValue::unknown_empty(false, rcstr!("test")), + consequent.build(arena), + alternate.build(arena), + ), + } + } } + use TestAtom::*; + use TestValue::*; + #[rstest] - #[case(JsValue::from(1.0))] - #[case(JsValue::from("hi"))] - #[case(ConstantValue::True.into())] - #[case(JsValue::promise(test_arena(), ConstantValue::Null.into()))] - #[case(construct_test_ternary(JsValue::from(1.0), JsValue::from("hi")))] - fn is_truthy_positive(#[case] v: JsValue<'static>) { - assert_eq!(v.is_truthy(), Some(true), "expected '{v}' to be truthy"); + #[case(Atom(Num(1.0)))] + #[case(Atom(Str("hi")))] + #[case(Atom(True))] + #[case(PromiseNull)] + #[case(Ternary(Num(1.0), Str("hi")))] + fn is_truthy_positive(#[case] value: TestValue) { + let arena = Bump::new(); + let value = value.build(&arena); + assert_eq!( + value.is_truthy(), + Some(true), + "expected '{value}' to be truthy" + ); } #[rstest] - #[case(JsValue::from(0.0))] - #[case(JsValue::from(""))] - #[case(ConstantValue::False.into())] - #[case(ConstantValue::Null.into())] - #[case(ConstantValue::Undefined.into())] - #[case(construct_test_ternary(JsValue::from(0.0), JsValue::from("")))] - fn is_truthy_negative(#[case] v: JsValue<'static>) { - assert_eq!(v.is_truthy(), Some(false), "expected '{v}' to be falsy"); + #[case(Atom(Num(0.0)))] + #[case(Atom(Str("")))] + #[case(Atom(False))] + #[case(Atom(Null))] + #[case(Atom(Undefined))] + #[case(Ternary(Num(0.0), Str("")))] + fn is_truthy_negative(#[case] value: TestValue) { + let arena = Bump::new(); + let value = value.build(&arena); + assert_eq!( + value.is_truthy(), + Some(false), + "expected '{value}' to be falsy" + ); } #[rstest] - #[case(ConstantValue::Null.into())] - #[case(ConstantValue::Undefined.into())] - #[case(construct_test_ternary(ConstantValue::Null.into(), ConstantValue::Undefined.into()))] - fn is_nullish_positive(#[case] v: JsValue<'static>) { - assert_eq!(v.is_nullish(), Some(true), "expected '{v}' to be nullish"); + #[case(Atom(Null))] + #[case(Atom(Undefined))] + #[case(Ternary(Null, Undefined))] + fn is_nullish_positive(#[case] value: TestValue) { + let arena = Bump::new(); + let value = value.build(&arena); + assert_eq!( + value.is_nullish(), + Some(true), + "expected '{value}' to be nullish" + ); } #[rstest] - #[case(JsValue::from(0.0))] - #[case(JsValue::from(""))] - #[case(JsValue::from("hi"))] - #[case(ConstantValue::True.into())] - #[case(JsValue::promise(test_arena(), ConstantValue::Null.into()))] - #[case(construct_test_ternary(JsValue::from(0.0), JsValue::from("hi")))] - fn is_nullish_negative(#[case] v: JsValue<'static>) { + #[case(Atom(Num(0.0)))] + #[case(Atom(Str("")))] + #[case(Atom(Str("hi")))] + #[case(Atom(True))] + #[case(PromiseNull)] + #[case(Ternary(Num(0.0), Str("hi")))] + fn is_nullish_negative(#[case] value: TestValue) { + let arena = Bump::new(); + let value = value.build(&arena); assert_eq!( - v.is_nullish(), + value.is_nullish(), Some(false), - "expected '{v}' not to be nullish" + "expected '{value}' not to be nullish" ); } #[rstest] - #[case(JsValue::from("hi"))] - #[case(JsValue::from(""))] - #[case(construct_test_ternary(JsValue::from("a"), JsValue::from("b")))] - fn is_string_positive(#[case] v: JsValue<'static>) { - assert_eq!(v.is_string(), Some(true), "expected '{v}' to be a string"); + #[case(Atom(Str("hi")))] + #[case(Atom(Str("")))] + #[case(Ternary(Str("a"), Str("b")))] + fn is_string_positive(#[case] value: TestValue) { + let arena = Bump::new(); + let value = value.build(&arena); + assert_eq!( + value.is_string(), + Some(true), + "expected '{value}' to be a string" + ); } #[rstest] - #[case(JsValue::from(1.0))] - #[case(ConstantValue::True.into())] - #[case(ConstantValue::Null.into())] - #[case(construct_test_ternary(JsValue::from(1.0), JsValue::from(2.0)))] - fn is_string_negative(#[case] v: JsValue<'static>) { + #[case(Atom(Num(1.0)))] + #[case(Atom(True))] + #[case(Atom(Null))] + #[case(Ternary(Num(1.0), Num(2.0)))] + fn is_string_negative(#[case] value: TestValue) { + let arena = Bump::new(); + let value = value.build(&arena); assert_eq!( - v.is_string(), + value.is_string(), Some(false), - "expected '{v}' not to be a string" + "expected '{value}' not to be a string" ); } #[rstest] - #[case(JsValue::from(""))] - #[case(construct_test_ternary(JsValue::from(""), JsValue::from("")))] - fn is_empty_string_positive(#[case] v: JsValue<'static>) { + #[case(Atom(Str("")))] + #[case(Ternary(Str(""), Str("")))] + fn is_empty_string_positive(#[case] value: TestValue) { + let arena = Bump::new(); + let value = value.build(&arena); assert_eq!( - v.is_empty_string(), + value.is_empty_string(), Some(true), - "expected '{v}' to be an empty string" + "expected '{value}' to be an empty string" ); } #[rstest] - #[case(JsValue::from("hi"))] - #[case(JsValue::from(1.0))] - #[case(ConstantValue::True.into())] - #[case(construct_test_ternary(JsValue::from("a"), JsValue::from("b")))] - fn is_empty_string_negative(#[case] v: JsValue<'static>) { + #[case(Atom(Str("hi")))] + #[case(Atom(Num(1.0)))] + #[case(Atom(True))] + #[case(Ternary(Str("a"), Str("b")))] + fn is_empty_string_negative(#[case] value: TestValue) { + let arena = Bump::new(); + let value = value.build(&arena); assert_eq!( - v.is_empty_string(), + value.is_empty_string(), Some(false), - "expected '{v}' not to be an empty string" + "expected '{value}' not to be an empty string" ); } From 9014f0467bca07a1e94da0923e1c9b896a704a99 Mon Sep 17 00:00:00 2001 From: Jiwon Choi Date: Fri, 18 Sep 2026 10:36:57 +0200 Subject: [PATCH 2/3] Do not check Node version while agentic upgrade (#98852) If upgrade handles the Node version check, upgrade from Next version that allowed Node v18 -> Next version that require Node v20 can fail the upgrade. Next handles compatible Node version so proceed upgrade and let agent handle it afterwards. --- packages/next/src/lib/upgrade/prepare-upgrade.ts | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/packages/next/src/lib/upgrade/prepare-upgrade.ts b/packages/next/src/lib/upgrade/prepare-upgrade.ts index 69d5e6a9fcf6..22d0e9a342c3 100644 --- a/packages/next/src/lib/upgrade/prepare-upgrade.ts +++ b/packages/next/src/lib/upgrade/prepare-upgrade.ts @@ -37,7 +37,6 @@ export async function prepareUpgrade( await readFile(requireFromApp.resolve('next/package.json'), 'utf8') ) as { version: string - engines: { node: string | undefined } | undefined } const installedVersion = installedNext.version @@ -55,10 +54,7 @@ export async function prepareUpgrade( if (targetRequest === 'latest' || targetRequest === 'future') { const url = `${NPM_REGISTRY}next/latest` const { value } = await fetchJSON(url) - const release = value as { - version: string - engines: { node: string | undefined } | undefined - } | null + const release = value as { version: string } | null if ( !release || @@ -72,10 +68,6 @@ export async function prepareUpgrade( targetRequest === 'future' && semver.gt(installedVersion, release.version) ? installedVersion : release.version - const nodeRange = - targetVersion === installedVersion - ? (installedNext.engines?.node ?? null) - : (release.engines?.node ?? null) if ( targetRequest === 'latest' && @@ -97,12 +89,6 @@ export async function prepareUpgrade( } } - if (!nodeRange || !semver.satisfies(process.versions.node, nodeRange)) { - throw new Error( - `Next.js ${targetVersion} requires Node.js ${nodeRange ?? '(version unavailable)'}. Update Node.js before continuing.` - ) - } - let pendingFutureDefaults: FutureDefaultEntry[] = [] if (targetRequest === 'future') { From 579ff601b441a6dd09eb2502c7c06479e3a09595 Mon Sep 17 00:00:00 2001 From: "Eddy (Frontend Engineer)" <101346918+leejpsd@users.noreply.github.com> Date: Fri, 18 Sep 2026 19:07:48 +0900 Subject: [PATCH 3/3] example(cache-handler-redis): fix connection leak, hangs on a down Redis, and cross-page tag invalidation (#98716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three runtime problems in `examples/cache-handler-redis`. ### 1. One new Redis connection per request, never closed Next.js constructs the singular `cacheHandler` class once per request (`new CurCacheHandler(...)` in `IncrementalCache`, which `route-module.ts` creates per request). The example's constructor calls `createClient()` + `connect()`, so every request opens a connection that is never closed. ``` $ redis-cli client list | wc -l # before 102 $ for i in $(seq 1 10); do curl -s -o /dev/null localhost:3000/cet; done $ redis-cli client list | wc -l 132 # +3 per request ``` With Redis' default `maxclients 10000`, a single instance runs out after a few thousand requests. ### 2. Requests hang while Redis is unavailable The README says the handlers "degrade gracefully when Redis is unavailable, so the app still builds and runs, just without a shared cache". At runtime they don't: with Redis stopped, every request on every instance blocks until Redis comes back. ``` $ docker stop cache-handler-redis $ curl -s -o /dev/null -m 60 -w "%{http_code} %{time_total}s\n" localhost:3000/cet 000 60.007305s # (uncapped: 188s, returned the moment Redis was started again) ``` Same when the app is started while Redis is down. Cause: node-redis keeps retrying in the background and `client.connect()` does not settle until a connection succeeds. Isolated: ```js const c = createClient({ url: "redis://localhost:6379" }); c.on("error", () => {}); await Promise.race([c.connect(), new Promise(r => setTimeout(() => r("pending"), 15000))]); // -> "pending" after 15s, isOpen=true isReady=false (redis@6.2.1) ``` Because each request built a new handler, each request awaited a fresh, never-settling `connect()` in `getClient()`. `remote-cache-handler.js` has one module-level client, but its `getClient()` awaits the same promise, so it hangs the same way if the app starts while Redis is down. ### 3. `updateTag` never reaches other pages' remote entries The remote handler's `get` never checks the entry's own tags. Next.js only passes soft tags to `getExpiration`, and the `"use cache"` wrapper only knows about tags revalidated in the current request, so `updateTag("time-data")` from `/cet` left the `/gmt` entry stale on every instance, including the one that ran it, until it expired (`cacheLife` `expire: 3600`). The [`cacheHandlers` docs](https://nextjs.org/docs/app/api-reference/config/next-config-js/cacheHandlers#get) say `get` should report an entry whose tag was invalidated as missing or stale. ### Fix - Hoist the client in `cache-handler.js` to module scope (the pattern `remote-cache-handler.js` already uses). - `getClient()` awaits the connect promise raced against a 1s `unref()`'d timer (suggested in review), then returns the client when `isReady` and `null` otherwise. Requests during startup still wait for the connection, and a down Redis costs one bounded wait instead of blocking every request. The client keeps retrying and `isReady` flips back on its own. - `disableOfflineQueue: true`, so a command issued while the connection is down rejects immediately (`ClientOfflineError`) instead of being queued for the 5s command timeout. - Entry `get` / `set` catch the Redis call only and degrade to a miss. - Remote `get` compares the entry's tags against their revalidation timestamps (one `MGET`) and misses when any is newer, the same comparison as the built-in handler. `getExpiration` returns `Date.now()` when Redis can't answer, so the entry is discarded rather than served. - `revalidateTag` / `updateTags` throw when Redis isn't ready, so an invalidation that never reached Redis surfaces as an error instead of a success whose entries come back once Redis does. ### After Two `next start` instances on one Redis, same script on `next@16.3.5` and `16.4.0-canary.35` (identical results), canary's handlers vs this PR: | scenario | before | after | | --- | --- | --- | | `updateTag` from `/cet`, read `/gmt` on both instances | stale | refreshed | | `revalidateTag("time-data", "max")` | neither page refreshed | both refreshed | | tag lookup fails | 500 | 200, regenerated | | request while Redis is stopped | no response | 200, uncached | | `updateTag` while Redis is stopped | no response | 500, error logged | | app started without Redis | no response | first ISR read waits ≤1.1s once | | Redis connections, 200 requests | 173 → 773 | 7 → 7 | | `/cet` under load (autocannon, 10 connections × 10s, `16.3.5`) | 332 req/s, hits Redis `maxclients` | 1,106 req/s median, p99 20ms, 3 connections | | 10 readers during 20 `updateTag`s: reads served a value from before a completed invalidation | 3,089 of 3,089 | 0 of 33,899 | | Redis down ~4s under load | timeouts, caching doesn't come back | 0 errors, caching resumes | The tag check costs one Redis round trip per remote hit (−9 to −13% on a route that only reads one remote entry). Handler-level cases, full tables and the benchmark breakdown are in [this comment](https://github.com/vercel/next.js/pull/98716#issuecomment-5724918096). --- examples/cache-handler-redis/README.md | 4 +- examples/cache-handler-redis/cache-handler.js | 118 +++++++++++------ .../remote-cache-handler.js | 123 ++++++++++++++---- 3 files changed, 181 insertions(+), 64 deletions(-) diff --git a/examples/cache-handler-redis/README.md b/examples/cache-handler-redis/README.md index e29fd1f81c73..301f7cba5815 100644 --- a/examples/cache-handler-redis/README.md +++ b/examples/cache-handler-redis/README.md @@ -46,9 +46,9 @@ The `/[timezone]` page renders a mostly static shell and, inside it, a `'use cac - **ISR cache (`cache-handler.js`):** stores the prerendered page entries as JSON under a `nextjs:cache:` prefix, and tracks which keys belong to each tag in a Redis set (`nextjs:tag:`). `revalidateTag` deletes every key associated with a tag. -- **Remote cache (`remote-cache-handler.js`):** stores each `'use cache: remote'` entry under a `nextjs:use-cache:` prefix (the streamed value is base64-encoded). Tag revalidation is timestamp-based: `updateTags` records `nextjs:use-cache-tag:` = now, and `getExpiration` reports the latest time so Next treats older entries as stale. Clicking **Revalidate** calls [`updateTag('time-data')`](https://nextjs.org/docs/app/api-reference/functions/updateTag), which regenerates the remote entry. +- **Remote cache (`remote-cache-handler.js`):** stores each `'use cache: remote'` entry under a `nextjs:use-cache:` prefix (the streamed value is base64-encoded). Tag revalidation is timestamp-based: `updateTags` records `nextjs:use-cache-tag:` = now. On a hit, `get` compares the entry's own tags (from `cacheTag`) against those timestamps and reports a miss if any is newer, and `getExpiration` reports the latest time for the route's soft tags so Next discards older entries. Clicking **Revalidate** calls [`updateTag('time-data')`](https://nextjs.org/docs/app/api-reference/functions/updateTag), which regenerates the remote entry. -- **Building without Redis:** both handlers skip connecting during `next build` (they check `NEXT_PHASE`) and degrade gracefully when Redis is unavailable, so the app still builds and runs, just without a shared cache. +- **Building without Redis:** both handlers skip connecting during `next build` (they check `NEXT_PHASE`) and degrade gracefully when Redis is unavailable, so the app still builds and runs, just without a shared cache. Revalidation is the exception: `revalidateTag` and `updateTags` throw while Redis is unavailable, because an invalidation that never reached Redis would be lost, and the old entries would be served again once Redis is back. - **Redis server setup:** ensure your Redis server is running before starting the app. Configure the connection with `REDIS_URL` (defaults to `redis://localhost:6379`). diff --git a/examples/cache-handler-redis/cache-handler.js b/examples/cache-handler-redis/cache-handler.js index 848155b2a09d..7faba1aa1f03 100644 --- a/examples/cache-handler-redis/cache-handler.js +++ b/examples/cache-handler-redis/cache-handler.js @@ -43,49 +43,77 @@ function deserialize(text) { }); } +// Next.js constructs the `cacheHandler` class once per request, so the Redis +// client must live at module scope: creating it in the constructor would open +// a new connection on every request and never close it. +const client = createClient({ + url: process.env.REDIS_URL ?? "redis://localhost:6379", + // Fail commands immediately while the connection is down instead of queueing + // them until Redis is back. + disableOfflineQueue: true, +}); + +// Redis won't work without error handling. Do not throw here, otherwise the +// client won't reconnect after a connection drop. +client.on("error", (error) => { + if (process.env.NEXT_PRIVATE_DEBUG_CACHE) { + console.warn("Redis client error:", error); + } +}); + +// Connecting to Redis during `next build` can cause issues, so we only connect +// at runtime. +const connection = + process.env.NEXT_PHASE === PHASE_PRODUCTION_BUILD + ? Promise.resolve() + : client.connect().catch((error) => { + console.warn("Failed to connect to Redis:", error); + }); + +// `connect()` stays pending for as long as Redis is unreachable, so cap the +// wait: requests arriving during startup wait for the connection at most +// once, and while Redis is down every request is served uncached instead of +// blocking. The client keeps retrying in the background, so `isReady` flips +// back on its own once Redis is reachable again. +const CONNECT_TIMEOUT_MS = 1000; +const ready = Promise.race([ + connection, + // `unref()` so this timer never keeps the process alive. + new Promise((resolve) => setTimeout(resolve, CONNECT_TIMEOUT_MS).unref()), +]); + +// Resolve a connected client, or `null` when Redis is unavailable so the app +// keeps working (without a shared cache) instead of hanging or crashing. +async function getClient() { + await ready; + return client.isReady ? client : null; +} + module.exports = class CacheHandler { constructor(options) { this.options = options; - - this.client = createClient({ - url: process.env.REDIS_URL ?? "redis://localhost:6379", - }); - - // Redis won't work without error handling. Do not throw here, otherwise - // the client won't reconnect after a connection drop. - this.client.on("error", (error) => { - if (process.env.NEXT_PRIVATE_DEBUG_CACHE) { - console.warn("Redis client error:", error); - } - }); - - // Connecting to Redis during `next build` can cause issues, so we only - // connect at runtime. `this.connection` resolves once the client is ready. - this.connection = - process.env.NEXT_PHASE === PHASE_PRODUCTION_BUILD - ? Promise.resolve() - : this.client.connect().catch((error) => { - console.warn("Failed to connect to Redis:", error); - }); - } - - // Resolve a connected client, or `null` when Redis is unavailable so the - // app keeps working (without a shared cache) instead of crashing. - async getClient() { - await this.connection; - return this.client.isReady ? this.client : null; } async get(key) { - const client = await this.getClient(); + const client = await getClient(); if (!client) return null; - const entry = await client.get(CACHE_PREFIX + key); + let entry; + try { + entry = await client.get(CACHE_PREFIX + key); + } catch (error) { + // A connection dropping mid-request degrades to a cache miss. + if (process.env.NEXT_PRIVATE_DEBUG_CACHE) { + console.warn("Redis get failed:", error); + } + return null; + } + return entry ? deserialize(entry) : null; } async set(key, data, ctx) { - const client = await this.getClient(); + const client = await getClient(); if (!client || !data) return; // Collect tags from both sources: `ctx.tags` (fetch entries) and the @@ -105,19 +133,29 @@ module.exports = class CacheHandler { ? { expiration: { type: "EX", value: Math.max(1, Math.ceil(expire)) } } : {}; - await client.set( - CACHE_PREFIX + key, - serialize({ value: data, lastModified: Date.now(), tags }), - options, - ); + const value = serialize({ value: data, lastModified: Date.now(), tags }); + + try { + await client.set(CACHE_PREFIX + key, value, options); - // Index this key under each of its tags so `revalidateTag` can find it. - await Promise.all(tags.map((tag) => client.sAdd(TAG_PREFIX + tag, key))); + // Index this key under each of its tags so `revalidateTag` can find it. + await Promise.all(tags.map((tag) => client.sAdd(TAG_PREFIX + tag, key))); + } catch (error) { + if (process.env.NEXT_PRIVATE_DEBUG_CACHE) { + console.warn("Redis set failed:", error); + } + } } async revalidateTag(tags) { - const client = await this.getClient(); - if (!client) return; + const client = await getClient(); + // Don't report success for a revalidation that never reached Redis: once + // Redis is back, every instance would serve the old entries again. + if (!client) { + throw new Error( + "Redis is unavailable, so the tag revalidation was not recorded", + ); + } // `tags` is either a single tag or an array of tags. for (const tag of [tags].flat()) { diff --git a/examples/cache-handler-redis/remote-cache-handler.js b/examples/cache-handler-redis/remote-cache-handler.js index 8bfe82fadbad..cdb9864b4558 100644 --- a/examples/cache-handler-redis/remote-cache-handler.js +++ b/examples/cache-handler-redis/remote-cache-handler.js @@ -13,6 +13,9 @@ const TAG_PREFIX = "nextjs:use-cache-tag:"; const client = createClient({ url: process.env.REDIS_URL ?? "redis://localhost:6379", + // Fail commands immediately while the connection is down instead of queueing + // them until Redis is back. + disableOfflineQueue: true, }); client.on("error", (error) => { @@ -28,8 +31,20 @@ const connection = console.warn("Failed to connect to Redis (remote cache):", error); }); +// `connect()` stays pending for as long as Redis is unreachable, so cap the +// wait: requests arriving during startup wait for the connection at most +// once, and while Redis is down every request is served uncached instead of +// blocking. The client keeps retrying in the background, so `isReady` flips +// back on its own once Redis is reachable again. +const CONNECT_TIMEOUT_MS = 1000; +const ready = Promise.race([ + connection, + // `unref()` so this timer never keeps the process alive. + new Promise((resolve) => setTimeout(resolve, CONNECT_TIMEOUT_MS).unref()), +]); + async function getClient() { - await connection; + await ready; return client.isReady ? client : null; } @@ -38,7 +53,16 @@ module.exports = { const redis = await getClient(); if (!redis) return undefined; - const stored = await redis.get(ENTRY_PREFIX + cacheKey); + let stored; + try { + stored = await redis.get(ENTRY_PREFIX + cacheKey); + } catch (error) { + // A connection dropping mid-request degrades to a cache miss. + if (process.env.NEXT_PRIVATE_DEBUG_CACHE) { + console.warn("Redis get failed (remote cache):", error); + } + return undefined; + } if (!stored) return undefined; const data = JSON.parse(stored); @@ -50,6 +74,33 @@ module.exports = { return undefined; } + // Next.js only asks `getExpiration` about the route's soft tags, so the + // entry's own tags (from `cacheTag`) are checked here. If any of them was + // revalidated after this entry was created, on this instance or another, + // the entry is out of date: report a miss so Next.js regenerates it. + if (data.tags.length) { + let revalidatedAt; + try { + revalidatedAt = await redis.mGet( + data.tags.map((tag) => TAG_PREFIX + tag), + ); + } catch (error) { + // Without the tag timestamps we can't tell, so don't serve it. + if (process.env.NEXT_PRIVATE_DEBUG_CACHE) { + console.warn("Redis tag lookup failed (remote cache):", error); + } + return undefined; + } + + if ( + revalidatedAt.some( + (time) => time !== null && Number(time) > data.timestamp, + ) + ) { + return undefined; + } + } + return { // `value` must be a stream; rebuild it from the stored bytes. value: new ReadableStream({ @@ -99,38 +150,60 @@ module.exports = { } : {}; - await redis.set( - ENTRY_PREFIX + cacheKey, - JSON.stringify({ - value: bytes.toString("base64"), - tags: entry.tags, - stale: entry.stale, - timestamp: entry.timestamp, - expire: entry.expire, - revalidate: entry.revalidate, - }), - options, - ); + const value = JSON.stringify({ + value: bytes.toString("base64"), + tags: entry.tags, + stale: entry.stale, + timestamp: entry.timestamp, + expire: entry.expire, + revalidate: entry.revalidate, + }); + + try { + await redis.set(ENTRY_PREFIX + cacheKey, value, options); + } catch (error) { + if (process.env.NEXT_PRIVATE_DEBUG_CACHE) { + console.warn("Redis set failed (remote cache):", error); + } + } }, // Redis is the single source of truth and every read hits it, so there's no // local tag state to sync between requests. async refreshTags() {}, - // Return the most recent revalidation time across `tags`. Next treats an - // entry as stale when this is newer than the entry's `timestamp`. + // Return the most recent revalidation time across `tags`. Next.js calls + // this after a hit with the route's soft tags (the implicit `_N_T_` tags + // that `revalidatePath` uses) and discards the entry when the result is at + // or after the entry's `timestamp`. async getExpiration(tags) { + if (!tags.length) return 0; + + // If Redis can't answer, report the tags as revalidated just now: Next.js + // then discards the entry and regenerates it, the same miss `get` falls + // back to. Returning `0` would instead serve an entry that a + // `revalidatePath` may already have invalidated. const redis = await getClient(); - if (!redis || !tags.length) return 0; + if (!redis) return Date.now(); + + let values; + try { + values = await redis.mGet(tags.map((tag) => TAG_PREFIX + tag)); + } catch (error) { + if (process.env.NEXT_PRIVATE_DEBUG_CACHE) { + console.warn("Redis tag lookup failed (remote cache):", error); + } + return Date.now(); + } - const values = await redis.mGet(tags.map((tag) => TAG_PREFIX + tag)); const timestamps = values.filter(Boolean).map(Number); return timestamps.length ? Math.max(...timestamps) : 0; }, - // Record when each tag was last revalidated so `getExpiration` can report - // it. There's one key per distinct tag, overwritten in place, so a small - // fixed tag set (like this example's single `time-data` tag) never grows. + // Record when each tag was last revalidated, for `get` (the entry's own + // tags) and `getExpiration` (soft tags) to compare against. There's one key + // per distinct tag, overwritten in place, so a small fixed tag set (like + // this example's single `time-data` tag) never grows. // // An app that mints many distinct, short-lived tags (e.g. `user-`) would // instead keep a key per tag forever. To bound that, give each key a TTL @@ -140,7 +213,13 @@ module.exports = { // still cached and serve it as fresh when it should be stale. async updateTags(tags) { const redis = await getClient(); - if (!redis) return; + // Don't report success for a revalidation Redis never recorded: once Redis + // is back, every instance would serve the old entries again. + if (!redis) { + throw new Error( + "Redis is unavailable, so the tag revalidation was not recorded", + ); + } const now = String(Date.now()); await Promise.all(tags.map((tag) => redis.set(TAG_PREFIX + tag, now)));