From ba530fda6edf00c65ac64f6b8953343826930ef6 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Mon, 24 Aug 2026 09:08:23 +0200 Subject: [PATCH 1/2] Turbopack: add unit test coverage for FileSystemPath::hash_file (#97743) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What? Adds unit test coverage for `FileSystemPath::hash_file`, the helper introduced in #97507. It pins the contract per filesystem entry type: a regular file is hashed by its content, a symlink by its *own* link content (target and whether it is absolute or relative), and a directory or a path that doesn't exist can't be hashed at all. Tests only — no production code is touched. ### Why? `hash_file` exists because hashing a traced path through `read()` was wrong: `read()` follows a symlink, so a link pointing at a directory made the build fail outright with `reading file "…" Is a directory (os error 21)`, and a link pointing at a file silently produced the hash of the target's bytes. The latter matters because consumers of the trace re-create a symlink entry *as a symlink* (`copyTracedFiles` does `readlink` + `symlink`) rather than copying the resolved file, so the link — not the file behind it — is what actually gets written out and therefore what the hash has to describe. Nothing guarded that. The distinction is invisible at the call site (`hash_file` reads like an ordinary "hash this path" helper) and "just read the file and hash it" is a natural-looking simplification, which is exactly how the original crash was introduced. A missing symlink case is also the kind of thing that only shows up in the wild, on someone else's repository layout, at build time. ### How? The test drives a real `DiskFileSystem` over a temporary directory rather than exercising `LinkContent::hash` directly, because the bug being guarded lives in the entry-type dispatch — which branch gets chosen — not in the hashing itself. A test that hashed a `LinkContent` it constructed would still pass against the buggy implementation. It reuses the harness style of the existing `read_glob` tests (a `turbo_tasks::function(operation, root)` wrapper read strongly consistently), and creates symlinks through a helper that mirrors their platform handling, using a junction point for a directory link on Windows. The interesting design point is how "the link is hashed, not its target" is asserted. Doing it by mutating a file and re-hashing would pull in filesystem watching and invalidation, so it is expressed structurally instead: two directories each contain a file of the same name but with different content, and each has a symlink pointing at it through the same relative target. Those two links must hash *equally* (proving the target's content is irrelevant) while a third link with a different target must hash *differently* (proving the target itself is what counts). Both of those assertions fail against the pre-#97507 behaviour, as does the symlink-to-directory case. The remaining cases cover a dangling link and a link whose target leaves the filesystem root — both still hashable, since they are still links — and assert that every distinct link hashes distinctly, so nothing collapses into one shared "is a symlink" hash. The two assertions covering directories and missing paths document today's hard-error behaviour and carry a comment pointing at the open question already noted in `hash_file` about whether those should return `None` instead, so it is clear which assertions to revisit if that changes. ### Verification - `cargo test -p turbo-tasks-fs` (125 tests) - `cargo clippy -p turbo-tasks-fs --all-targets`, `cargo check -p turbo-tasks-fs --all-targets` - Confirmed the test actually guards the regression: reverting the symlink arm of `hash_file` to `read().hash()` makes it fail with the original `Is a directory (os error 21)` error, and it passes again once restored. 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> --- turbopack/crates/turbo-tasks-fs/src/path.rs | 166 ++++++++++++++++++++ 1 file changed, 166 insertions(+) diff --git a/turbopack/crates/turbo-tasks-fs/src/path.rs b/turbopack/crates/turbo-tasks-fs/src/path.rs index 1774296e780..86cf58ee3cc 100644 --- a/turbopack/crates/turbo-tasks-fs/src/path.rs +++ b/turbopack/crates/turbo-tasks-fs/src/path.rs @@ -841,4 +841,170 @@ mod tests { .await .unwrap() } + + mod hash_file { + use std::{ + fs::{create_dir_all, write}, + path::Path, + }; + + use turbo_tasks::OperationVc; + + use super::*; + use crate::DiskFileSystem; + + /// Creates a symbolic link, mirroring the platform handling of the `read_glob` tests. On + /// Windows a link to a directory is created as a junction point, which requires an + /// absolute target. + fn symlink(target: &Path, link: &Path) -> std::io::Result<()> { + #[cfg(unix)] + { + std::os::unix::fs::symlink(target, link) + } + #[cfg(windows)] + { + if std::fs::metadata(target).is_ok_and(|metadata| metadata.is_dir()) { + assert!( + target.is_absolute(), + "a junction point needs an absolute target" + ); + std::os::windows::fs::junction_point(target, link) + } else { + std::os::windows::fs::symlink_file(target, link) + } + } + } + + /// Two directories that hold a file of the *same name* but with *different content*, each + /// with a symlink pointing at it through the *same* relative target. Plus the entry types + /// that `hash_file` has to tell apart. + fn create_fixture(root: &Path, outside: &Path) { + write(outside.join("outside.txt"), b"outside").unwrap(); + + create_dir_all(root.join("data-a")).unwrap(); + write(root.join("data-a/value.txt"), b"aaa").unwrap(); + symlink(Path::new("value.txt"), &root.join("data-a/link")).unwrap(); + symlink( + Path::new("../data-b/value.txt"), + &root.join("data-a/link-other"), + ) + .unwrap(); + + create_dir_all(root.join("data-b")).unwrap(); + write(root.join("data-b/value.txt"), b"bbbbbb").unwrap(); + symlink(Path::new("value.txt"), &root.join("data-b/link")).unwrap(); + + create_dir_all(root.join("dir")).unwrap(); + write(root.join("dir/inside.txt"), b"inside").unwrap(); + // the regression from #97507: reading *through* this link hits the directory + symlink(&root.join("dir"), &root.join("link-dir")).unwrap(); + // a link whose target doesn't exist + symlink(Path::new("nope.txt"), &root.join("dangling")).unwrap(); + // a link whose target leaves the filesystem root + symlink(&outside.join("outside.txt"), &root.join("escaping")).unwrap(); + } + + #[turbo_tasks::function(operation, root)] + async fn hash_file_operation(disk_root: RcStr, entry: RcStr) -> Result> { + let fs = DiskFileSystem::new(rcstr!("temp"), Vc::cell(disk_root)); + let path = fs.root().await?.join(&entry)?; + Ok(path.hash_file(Vc::cell(rcstr!("salt")), HashAlgorithm::Xxh3Hash128Hex)) + } + + /// `Ok` with the hash, or `Err` with the (flattened) error message. + async fn hash_of(disk_root: &RcStr, entry: RcStr) -> Result { + let operation: OperationVc = hash_file_operation(disk_root.clone(), entry); + match operation.read_strongly_consistent().await { + Ok(hash) => Ok((*hash).clone()), + Err(err) => Err(format!("{err:#}")), + } + } + + /// `hash_file` hashes a symlink *itself* rather than what it points at, so that a link to a + /// directory can be hashed at all and so that the hash matches what consumers write out + /// (they recreate a symlink as a symlink). See #97507. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn hashes_by_entry_type() { + let scratch = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + create_fixture(scratch.path(), outside.path()); + + let tt = turbo_tasks::TurboTasks::new(TurboTasksBackend::new( + BackendOptions::default(), + noop_backing_storage(), + )); + let disk_root: RcStr = scratch.path().to_str().unwrap().into(); + tt.run_once(async move { + let file_a = hash_of(&disk_root, rcstr!("data-a/value.txt")).await; + let file_b = hash_of(&disk_root, rcstr!("data-b/value.txt")).await; + let link_a = hash_of(&disk_root, rcstr!("data-a/link")).await; + let link_b = hash_of(&disk_root, rcstr!("data-b/link")).await; + let link_other = hash_of(&disk_root, rcstr!("data-a/link-other")).await; + let link_dir = hash_of(&disk_root, rcstr!("link-dir")).await; + let dangling = hash_of(&disk_root, rcstr!("dangling")).await; + let escaping = hash_of(&disk_root, rcstr!("escaping")).await; + let dir = hash_of(&disk_root, rcstr!("dir")).await; + let missing = hash_of(&disk_root, rcstr!("gone.txt")).await; + + // A regular file hashes its content. + let file_a = file_a.expect("a file is hashable"); + let file_b = file_b.expect("a file is hashable"); + assert_ne!(file_a, file_b, "the two files have different content"); + + // A symlink is hashable, including one that points at a directory - reading + // through that link would fail with `Is a directory (os error 21)`. + let link_a = link_a.expect("a symlink to a file is hashable"); + let link_b = link_b.expect("a symlink to a file is hashable"); + let link_other = link_other.expect("a symlink to a file is hashable"); + let link_dir = link_dir.expect("a symlink to a directory is hashable"); + // A dangling link is still a link, and so is one that leaves the root (it is + // reported as `LinkContent::Invalid`). + let dangling = dangling.expect("a dangling symlink is hashable"); + let escaping = escaping.expect("a symlink leaving the root is hashable"); + + // The link is hashed, not the file it points at: `data-a/link` and `data-b/link` + // point at files with *different content* through the *same* target, so they hash + // the same... + assert_eq!( + link_a, link_b, + "the content of the target must not affect the hash of the link" + ); + // ...while a link with a different target hashes differently. + assert_ne!( + link_a, link_other, + "the target of the link must affect the hash of the link" + ); + // ...and a link never hashes like the file it points at. + assert_ne!(link_a, file_a); + + // All of the hashes above are distinct, i.e. nothing collapses into a shared + // "symlink" hash. + let hashes = [&link_a, &link_other, &link_dir, &dangling, &escaping]; + for (index, hash) in hashes.iter().enumerate() { + for other in &hashes[index + 1..] { + assert_ne!(hash, other, "every distinct link hashes distinctly"); + } + } + + // Entries that have no content to hash are errors today. `hash_file` carries an + // open question on whether these should return `None` instead - if that changes, + // these two assertions are the ones to revisit. + assert!( + dir.as_ref() + .is_err_and(|err| err.contains("Cannot hash content of non-file path")), + "a directory is not hashable, got {dir:?}" + ); + assert!( + missing + .as_ref() + .is_err_and(|err| err.contains("Cannot hash content of missing path")), + "a missing path is not hashable, got {missing:?}" + ); + + anyhow::Ok(()) + }) + .await + .unwrap() + } + } } From 713bd67572bd41eb32ad7fcc20f3b2850dbca2e3 Mon Sep 17 00:00:00 2001 From: Luke Sandberg Date: Mon, 24 Aug 2026 00:24:42 -0700 Subject: [PATCH 2/2] turbo-tasks: compile conditional cell updates once, not per cell type (#97763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What? Makes `CurrentCellRef::conditional_update_with_shared_reference` compile once instead of once per cell type. **−345 KiB** of code `__text` 66,832,376 → 66,479,352, measured on `libnext_napi_bindings.dylib` with the shipped release profile (thin LTO, `codegen-units = 1`, `--icf=all`). The 1,302 instantiations of this function collapse to **1**. ### Why? The function took its callback as `impl FnOnce`, so it was monomorphized for every `VcValueType` in the dependency graph — but its body is identical for all of them: read the cell, call the callback, hand the result to the *non-generic* `update_own_task_cell`. Nothing in it depends on the cell type. --- turbopack/crates/turbo-tasks/src/manager.rs | 52 +++++++++++++++------ 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/turbopack/crates/turbo-tasks/src/manager.rs b/turbopack/crates/turbo-tasks/src/manager.rs index 3033a894329..427dea2348a 100644 --- a/turbopack/crates/turbo-tasks/src/manager.rs +++ b/turbopack/crates/turbo-tasks/src/manager.rs @@ -1966,6 +1966,18 @@ pub struct CurrentCellRef { type VcReadTarget = <::Read as VcRead>::Target; +/// What a conditional cell update returns: the new content, the key hashes that changed, and an +/// optional hash of the value. +type CellUpdate = ( + SharedReference, + Option>, + Option, +); + +/// The callback [`CurrentCellRef::conditional_update_with_shared_reference`] takes. It is a `dyn` +/// trait object so that the function's body is compiled once rather than once per cell type. +type CellUpdateFn<'l> = dyn FnMut(Option<&SharedReference>) -> Option + 'l; + impl CurrentCellRef { /// Updates the cell if the given `functor` returns a value. fn conditional_update( @@ -1974,7 +1986,11 @@ impl CurrentCellRef { ) where T: VcValueType, { - self.conditional_update_with_shared_reference(|old_shared_reference| { + // `FnMut` cannot move out of its captures, and the callee calls this at most once, so + // the `FnOnce` is handed over through an `Option`. + let mut functor = Some(functor); + self.conditional_update_with_shared_reference(&mut |old_shared_reference| { + let functor = functor.take().expect("functor is called at most once"); let old_ref = old_shared_reference.and_then(|sr| sr.0.downcast_ref::()); let (new_value, updated_key_hashes, content_hash) = functor(old_ref)?; Some(( @@ -1986,16 +2002,12 @@ impl CurrentCellRef { } /// Updates the cell if the given `functor` returns a `SharedReference`. - fn conditional_update_with_shared_reference( - &self, - functor: impl FnOnce( - Option<&SharedReference>, - ) -> Option<( - SharedReference, - Option>, - Option, - )>, - ) { + /// + /// `functor` is a `dyn` trait object rather than a generic parameter on purpose. This body is + /// identical for every cell type, so making it generic monomorphized it once per + /// `VcValueType` in the dependency graph — over a thousand copies of the same code. The + /// indirect call it costs instead is negligible next to the cell read and update it wraps. + fn conditional_update_with_shared_reference(&self, functor: &mut CellUpdateFn<'_>) { let tt = turbo_tasks(); let cell_content = tt.read_own_task_cell(self.current_task, self.index).ok(); let update = functor(cell_content.as_ref().and_then(|cc| cc.1.0.as_ref())); @@ -2070,7 +2082,11 @@ impl CurrentCellRef { where T: VcValueType + PartialEq, { - self.conditional_update_with_shared_reference(|old_sr| { + let mut new_shared_reference = Some(new_shared_reference); + self.conditional_update_with_shared_reference(&mut |old_sr| { + let new_shared_reference = new_shared_reference + .take() + .expect("functor is called at most once"); if let Some(old_sr) = old_sr { let old_value = extract_sr_value::(old_sr); let new_value = extract_sr_value::(&new_shared_reference); @@ -2117,7 +2133,11 @@ impl CurrentCellRef { ) where T: VcValueType + PartialEq + DeterministicHash, { - self.conditional_update_with_shared_reference(move |old_sr| { + let mut new_shared_reference = Some(new_shared_reference); + self.conditional_update_with_shared_reference(&mut move |old_sr| { + let new_shared_reference = new_shared_reference + .take() + .expect("functor is called at most once"); if let Some(old_sr) = old_sr { let old_value = extract_sr_value::(old_sr); let new_value = extract_sr_value::(&new_shared_reference); @@ -2167,7 +2187,11 @@ impl CurrentCellRef { VcReadTarget: KeyedEq, as KeyedEq>::Key: std::hash::Hash, { - self.conditional_update_with_shared_reference(|old_sr| { + let mut new_shared_reference = Some(new_shared_reference); + self.conditional_update_with_shared_reference(&mut |old_sr| { + let new_shared_reference = new_shared_reference + .take() + .expect("functor is called at most once"); let Some(old_sr) = old_sr else { return Some((new_shared_reference, None, None)); };