From 16825a0854803a3ac530807a3ff2658b8a73c30f Mon Sep 17 00:00:00 2001 From: Ernesto Cambuston Date: Sat, 16 May 2026 18:49:11 -0700 Subject: [PATCH 1/5] Resolve absolute symlinks in output upload instead of uploading raw targets When the worker's work directory is populated via DirectoryCache, output paths can be absolute symlinks pointing into the cache directory (/var/.../directory_cache/...). The previous output collection code uploaded these as raw SymlinkInfo with absolute targets that are meaningless on the Bazel client, causing "No such file or directory" errors when the client materialised the action result. Detect absolute symlinks in output paths and resolve them: upload directory contents as Tree protos and file contents as regular files. Relative symlinks (intentionally created by the action) are still preserved as symlinks. Updates upload_dir_and_symlink_test to use a relative symlink (the previous /dev/null absolute symlink is now resolved, breaking the old assertion) and adds a new upload_absolute_symlink_resolves_contents regression test that verifies an out-of-tree payload reachable via an absolute symlink lands in CAS as a file. Ported from upstream TraceMachina/nativelink@08071533bec6c5df913f1aae92352794c458b248 (PR #2243). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/running_actions_manager.rs | 140 +++++++++++++--- .../tests/running_actions_manager_test.rs | 158 +++++++++++++++++- 2 files changed, 269 insertions(+), 29 deletions(-) diff --git a/nativelink-worker/src/running_actions_manager.rs b/nativelink-worker/src/running_actions_manager.rs index 4e9ea04a5..a3fec240f 100644 --- a/nativelink-worker/src/running_actions_manager.rs +++ b/nativelink-worker/src/running_actions_manager.rs @@ -1366,34 +1366,124 @@ impl RunningActionImpl { .err_tip(|| format!("Uploading directory {}", full_path.display()))?, )) } else if metadata.is_symlink() { - let output_symlink = upload_symlink(&full_path, work_directory) - .await - .map(|mut symlink_info| { - symlink_info.name_or_path = NameOrPath::Path(entry); - symlink_info - }) - .err_tip(|| format!("Uploading symlink {}", full_path.display()))?; - match fs::metadata(&full_path).await { - Ok(metadata) => { - if metadata.is_dir() { - Ok(OutputType::DirectorySymlink(output_symlink)) - } else { - // Note: If it's anything but directory we put it as a file symlink. - Ok(OutputType::FileSymlink(output_symlink)) + // Resolve the symlink to determine what it points to. + // Symlinks created by DirectoryCache (absolute paths into + // the cache directory) must NOT be uploaded as symlinks — + // the target path is worker-local and meaningless to the + // client. Instead, follow the symlink and upload the + // resolved content (file or directory). + let target = fs::read_link(&full_path).await.err_tip(|| { + format!("Reading symlink target for {}", full_path.display()) + })?; + let is_absolute_symlink = Path::new(&target).is_absolute(); + + if is_absolute_symlink { + // Absolute symlink — resolve and upload contents. + match fs::metadata(&full_path).await { + Ok(resolved_meta) => { + if resolved_meta.is_dir() { + // Upload as directory (Tree proto). + Ok(OutputType::Directory( + upload_directory( + cas_store.as_pin(), + &full_path, + work_directory, + hasher, + digest_uploaders, + ) + .and_then(|(root_dir, children)| async move { + let tree = ProtoTree { + root: Some(root_dir), + children: children.into(), + }; + let tree_digest = serialize_and_upload_message( + &tree, + cas_store.as_pin(), + &mut hasher.hasher(), + ) + .await + .err_tip(|| format!("While processing {entry}"))?; + Ok(DirectoryInfo { + path: entry, + tree_digest, + }) + }) + .await + .err_tip(|| { + format!( + "Uploading symlinked directory {}", + full_path.display() + ) + })?, + )) + } else { + // Upload as file (follow symlink). + Ok(OutputType::File( + upload_file( + cas_store.as_pin(), + &full_path, + hasher, + resolved_meta, + digest_uploaders, + ) + .await + .map(|mut file_info| { + file_info.name_or_path = NameOrPath::Path(entry); + file_info + }) + .err_tip(|| { + format!( + "Uploading symlinked file {}", + full_path.display() + ) + })?, + )) + } + } + Err(e) => { + if e.code != Code::NotFound { + return Err(e).err_tip(|| { + format!( + "While resolving absolute symlink {}", + full_path.display() + ) + }); + } + Ok(OutputType::None) } } - Err(e) => { - if e.code != Code::NotFound { - return Err(e).err_tip(|| { - format!( - "While querying target symlink metadata for {}", - full_path.display() - ) - }); + } else { + // Relative symlink — action intentionally created it. + // Upload as a proper symlink. + let output_symlink = upload_symlink(&full_path, work_directory) + .await + .map(|mut symlink_info| { + symlink_info.name_or_path = NameOrPath::Path(entry); + symlink_info + }) + .err_tip(|| format!("Uploading symlink {}", full_path.display()))?; + match fs::metadata(&full_path).await { + Ok(metadata) => { + if metadata.is_dir() { + Ok(OutputType::DirectorySymlink(output_symlink)) + } else { + // Note: If it's anything but directory we put it as a file symlink. + Ok(OutputType::FileSymlink(output_symlink)) + } + } + Err(e) => { + if e.code != Code::NotFound { + return Err(e).err_tip(|| { + format!( + "While querying target symlink metadata for {}", + full_path.display() + ) + }); + } + // If the file doesn't exist, we consider it a file. Even though the + // file doesn't exist we still need to populate an entry. + Ok(OutputType::FileSymlink(output_symlink)) } - // If the file doesn't exist, we consider it a file. Even though the - // file doesn't exist we still need to populate an entry. - Ok(OutputType::FileSymlink(output_symlink)) } } } else { diff --git a/nativelink-worker/tests/running_actions_manager_test.rs b/nativelink-worker/tests/running_actions_manager_test.rs index 206f92b1d..e42447742 100644 --- a/nativelink-worker/tests/running_actions_manager_test.rs +++ b/nativelink-worker/tests/running_actions_manager_test.rs @@ -1087,11 +1087,13 @@ mod tests { "echo foo > dir1/file && ", "touch dir1/file2 && ", "ln -s ../file dir1/dir2/sym &&", - "ln -s /dev/null empty_sym", + // Relative symlink — preserved as a symlink in the + // ActionResult (absolute symlinks are now resolved). + "ln -s dir1/file rel_sym", ) .to_string(), ], - output_paths: vec!["dir1".to_string(), "empty_sym".to_string()], + output_paths: vec!["dir1".to_string(), "rel_sym".to_string()], working_directory: ".".to_string(), environment_variables: vec![EnvironmentVariable { name: "PATH".to_string(), @@ -1225,8 +1227,8 @@ mod tests { )?, }], output_file_symlinks: vec![SymlinkInfo { - name_or_path: NameOrPath::Path("empty_sym".to_string()), - target: "/dev/null".to_string(), + name_or_path: NameOrPath::Path("rel_sym".to_string()), + target: "dir1/file".to_string(), }], output_directory_symlinks: vec![], server_logs: HashMap::new(), @@ -1249,6 +1251,154 @@ mod tests { Ok(()) } + // Windows does not support symlinks. + #[cfg(not(target_family = "windows"))] + #[nativelink_test] + async fn upload_absolute_symlink_resolves_contents() -> Result<(), Box> + { + // Regression test: when an action produces an absolute symlink as one + // of its declared outputs (for example because the work directory was + // populated via DirectoryCache and contains absolute symlinks into the + // cache), uploading the literal symlink target yields a path that is + // meaningless on the client. The worker must resolve absolute + // symlinks and upload the underlying file/directory contents. + const WORKER_ID: &str = "foo_worker_id"; + + fn test_monotonic_clock() -> SystemTime { + static CLOCK: AtomicU64 = AtomicU64::new(0); + monotonic_clock(&CLOCK) + } + + let (_, slow_store, cas_store, ac_store) = setup_stores().await?; + let root_action_directory = make_temp_path("root_action_directory"); + fs::create_dir_all(&root_action_directory).await?; + + // Create an out-of-tree file whose absolute path the action will + // symlink into the work directory. + let external_root = make_temp_path("external_payload"); + fs::create_dir_all(&external_root).await?; + let external_file = format!("{external_root}/payload.txt"); + tokio::fs::write(&external_file, b"hello-from-outside").await?; + + let running_actions_manager = Arc::new(RunningActionsManagerImpl::new_with_callbacks( + RunningActionsManagerArgs { + root_action_directory, + execution_configuration: ExecutionConfiguration::default(), + cas_store: cas_store.clone(), + ac_store: Some(Store::new(ac_store.clone())), + historical_store: Store::new(cas_store.clone()), + upload_action_result_config: &UploadActionResultConfig { + upload_ac_results_strategy: UploadCacheResultsStrategy::Never, + ..Default::default() + }, + max_action_timeout: Duration::MAX, + max_upload_timeout: Duration::from_secs(DEFAULT_MAX_UPLOAD_TIMEOUT), + timeout_handled_externally: false, + directory_cache: None, + #[cfg(target_os = "linux")] + use_namespaces: use_namespaces(), + }, + Callbacks { + now_fn: test_monotonic_clock, + sleep_fn: |_duration| Box::pin(future::pending()), + }, + )?); + let queued_timestamp = make_system_time(1000); + let action_result = { + let command = Command { + arguments: vec![ + "sh".to_string(), + "-c".to_string(), + format!("ln -s {external_file} resolved_output"), + ], + output_paths: vec!["resolved_output".to_string()], + working_directory: ".".to_string(), + environment_variables: vec![EnvironmentVariable { + name: "PATH".to_string(), + value: env::var("PATH").unwrap(), + }], + ..Default::default() + }; + let command_digest = serialize_and_upload_message( + &command, + cas_store.as_pin(), + &mut DigestHasherFunc::Sha256.hasher(), + ) + .await?; + let input_root_digest = serialize_and_upload_message( + &Directory::default(), + cas_store.as_pin(), + &mut DigestHasherFunc::Sha256.hasher(), + ) + .await?; + let action = Action { + command_digest: Some(command_digest.into()), + input_root_digest: Some(input_root_digest.into()), + ..Default::default() + }; + let action_digest = serialize_and_upload_message( + &action, + cas_store.as_pin(), + &mut DigestHasherFunc::Sha256.hasher(), + ) + .await?; + + let execute_request = ExecuteRequest { + action_digest: Some(action_digest.into()), + ..Default::default() + }; + let operation_id = OperationId::default().to_string(); + + let running_action_impl = running_actions_manager + .create_and_add_action( + WORKER_ID.to_string(), + StartExecute { + execute_request: Some(execute_request), + operation_id, + queued_timestamp: Some(queued_timestamp.into()), + platform: action.platform.clone(), + worker_id: WORKER_ID.to_string(), + }, + ) + .await?; + + run_action(running_action_impl.clone()).await? + }; + + // With the fix, the absolute symlink should be resolved and the + // payload uploaded as a regular file with the payload's content + // hash. The output_file_symlinks list must be empty. + assert_eq!( + action_result.output_file_symlinks.len(), + 0, + "absolute symlink should be resolved to file, not uploaded as symlink" + ); + assert_eq!( + action_result.output_directory_symlinks.len(), + 0, + "no directory symlinks expected" + ); + assert_eq!( + action_result.output_files.len(), + 1, + "absolute symlink should appear as an uploaded file" + ); + let uploaded = &action_result.output_files[0]; + assert_eq!( + uploaded.name_or_path, + NameOrPath::Path("resolved_output".to_string()) + ); + assert_eq!( + usize::try_from(uploaded.digest.size_bytes())?, + b"hello-from-outside".len() + ); + // Verify the blob actually landed in CAS by re-reading it. + let key: nativelink_util::store_trait::StoreKey<'_> = uploaded.digest.into(); + let blob = slow_store.as_ref().get_part_unchunked(key, 0, None).await?; + assert_eq!(blob.as_ref(), b"hello-from-outside"); + Ok(()) + } + #[nativelink_test] async fn cleanup_happens_on_job_failure() -> Result<(), Box> { const WORKER_ID: &str = "foo_worker_id"; From b6c42646b2b94defddb12b248a6ace230e95e4e0 Mon Sep 17 00:00:00 2001 From: Ernesto Cambuston Date: Sat, 16 May 2026 18:51:51 -0700 Subject: [PATCH 2/5] Fix zero-digest files missing from worker execution directories FilesystemStore::get_file_entry_for_digest previously returned a synthetic FileEntry for zero-digest blobs pointing to a content_path that never exists on disk (FilesystemStore deliberately never persists zero-byte files). Downstream worker output-materialisation code that took the prefetched hardlink path would try to hard_link from this non-existent source, fail, and either silently produce a missing file or fall back without enough context to diagnose the failure. This is the OUTPUT-materialisation companion to PR #2338's DirectoryCache zero-byte fix, which lives on the input-fetch path. The two changes are complementary: #2338 covers the directory cache short-circuit; this covers the FilesystemStore API and the running_actions_manager fallback. Changes: - FilesystemStore::get_file_entry_for_digest now returns Code::NotFound for zero digests instead of a synthetic FileEntry, forcing callers to materialise empty files via fs::create_file rather than hard_linking from a phantom source. - running_actions_manager::download_to_directory adds err_tip context on the zero-digest fs::create_file / write_all so a failure here is attributable to the empty-file path. - Updates the existing get_file_entry_for_zero_digest test to assert the new NotFound behaviour and renames it to get_file_entry_for_zero_digest_returns_not_found. - Adds download_to_directory_zero_digest_empty_file_test that exercises an empty file declared inside an input directory and verifies it materialises on disk with zero bytes. This test intentionally does NOT collide with PR #2338's DirectoryCache zero-byte test (which validates a different short-circuit path). Ported from upstream TraceMachina/nativelink@19d7e2052512fb16bffa290192b690015aa00200 (PR #2243). Co-Authored-By: Claude Opus 4.7 (1M context) --- nativelink-store/src/filesystem_store.rs | 21 ++--- .../tests/filesystem_store_test.rs | 22 +++++- .../src/running_actions_manager.rs | 14 +++- .../tests/running_actions_manager_test.rs | 76 +++++++++++++++++++ 4 files changed, 119 insertions(+), 14 deletions(-) diff --git a/nativelink-store/src/filesystem_store.rs b/nativelink-store/src/filesystem_store.rs index 0d1c9a54d..ebaf877c3 100644 --- a/nativelink-store/src/filesystem_store.rs +++ b/nativelink-store/src/filesystem_store.rs @@ -749,16 +749,19 @@ impl FilesystemStore { } pub async fn get_file_entry_for_digest(&self, digest: &DigestInfo) -> Result, Error> { + // Zero-digest blobs have no backing file on disk (FilesystemStore + // never persists zero-byte content). The previous implementation + // returned a synthetic FileEntry whose content_path did not exist, + // which downstream callers would then try to hard_link from, + // silently producing missing or empty output files in worker + // execution directories. Return NotFound so callers are forced to + // take the explicit zero-digest path (e.g. fs::create_file). if is_zero_digest(digest) { - return Ok(Arc::new(Fe::create( - 0, - 0, - RwLock::new(EncodedFilePath { - shared_context: self.shared_context.clone(), - path_type: PathType::Content, - key: digest.into(), - }), - ))); + return Err(make_err!( + Code::NotFound, + "{digest} is a zero-digest; FilesystemStore does not persist zero-byte files. \ + Callers must materialise empty files directly rather than going through get_file_entry_for_digest." + )); } self.evicting_map .get(&digest.into()) diff --git a/nativelink-store/tests/filesystem_store_test.rs b/nativelink-store/tests/filesystem_store_test.rs index 243b1fecc..8df354a22 100644 --- a/nativelink-store/tests/filesystem_store_test.rs +++ b/nativelink-store/tests/filesystem_store_test.rs @@ -1004,7 +1004,20 @@ async fn update_with_zero_digest() -> Result<(), Error> { } #[nativelink_test] -async fn get_file_entry_for_zero_digest() -> Result<(), Error> { +async fn get_file_entry_for_zero_digest_returns_not_found() -> Result<(), Error> { + // Regression test for: zero-digest blobs have no backing file on disk, + // so `get_file_entry_for_digest` previously handed back a synthetic + // FileEntry whose content_path did not exist. Downstream callers (the + // worker output-materialisation path) would then try to hard_link from + // that non-existent source, silently producing missing or empty output + // files in worker exec dirs. + // + // After the fix, `get_file_entry_for_digest` returns NotFound for + // zero digests so callers are forced to materialise empty files + // directly (e.g. via `fs::create_file`). This complements the + // DirectoryCache zero-byte short-circuit (input-fetch path); this + // test covers the FilesystemStore API used by the output-upload / + // hardlink path. let digest = DigestInfo::new(Sha256::new().finalize().into(), 0); let content_path = make_temp_path("content_path"); let temp_path = make_temp_path("temp_path"); @@ -1020,8 +1033,11 @@ async fn get_file_entry_for_zero_digest() -> Result<(), Error> { ) .await?; - let file_entry = store.get_file_entry_for_digest(&digest).await?; - assert!(file_entry.is_empty()); + let err = store + .get_file_entry_for_digest(&digest) + .await + .expect_err("zero digest must not return a synthetic FileEntry"); + assert_eq!(err.code, Code::NotFound, "expected NotFound, got: {err:?}"); Ok(()) } diff --git a/nativelink-worker/src/running_actions_manager.rs b/nativelink-worker/src/running_actions_manager.rs index a3fec240f..af2876863 100644 --- a/nativelink-worker/src/running_actions_manager.rs +++ b/nativelink-worker/src/running_actions_manager.rs @@ -153,8 +153,18 @@ pub fn download_to_directory<'a>( .populate_fast_store(digest.into()) .and_then(move |()| async move { if is_zero_digest(digest) { - let mut file_slot = fs::create_file(&dest).await?; - file_slot.write_all(&[]).await?; + // Zero-digest files are never persisted by the + // FilesystemStore, so materialise them directly + // in the worker exec dir. Attach context so a + // failure here is diagnosable as a missing + // empty file rather than a generic IO error. + let mut file_slot = fs::create_file(&dest) + .await + .err_tip(|| format!("Could not create zero-digest file at {dest}"))?; + file_slot + .write_all(&[]) + .await + .err_tip(|| format!("Could not write zero-digest file at {dest}"))?; } else { let file_entry = filesystem_store diff --git a/nativelink-worker/tests/running_actions_manager_test.rs b/nativelink-worker/tests/running_actions_manager_test.rs index e42447742..bc4615bf8 100644 --- a/nativelink-worker/tests/running_actions_manager_test.rs +++ b/nativelink-worker/tests/running_actions_manager_test.rs @@ -350,6 +350,82 @@ mod tests { Ok(()) } + #[nativelink_test] + async fn download_to_directory_zero_digest_empty_file_test() + -> Result<(), Box> { + // Regression test: zero-digest files used to fall through a + // synthetic FileEntry path that pointed at a non-existent content + // path on disk. The worker's prefetched-hardlink path silently + // failed to materialise the empty file. Verify that an empty file + // declared as part of an input directory now lands on disk at the + // expected location with zero bytes — exercising the + // FilesystemStore + running_actions_manager output-materialisation + // path. Complements PR #2338's DirectoryCache zero-byte test + // which covers a different code path (the input directory cache + // short-circuit). + const EMPTY_FILE_NAME: &str = "empty.txt"; + const NON_EMPTY_FILE_NAME: &str = "non_empty.txt"; + const NON_EMPTY_CONTENT: &str = "non-empty"; + + let (fast_store, slow_store, cas_store, _ac_store) = setup_stores().await?; + + // SHA-256 of empty content. + let zero_digest = DigestInfo::try_new( + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + 0, + )?; + let non_empty_digest = DigestInfo::new([7u8; 32], NON_EMPTY_CONTENT.len() as u64); + slow_store + .as_ref() + .update_oneshot(non_empty_digest, NON_EMPTY_CONTENT.into()) + .await?; + + let root_directory_digest = DigestInfo::new([8u8; 32], 64); + let root_directory = Directory { + files: vec![ + FileNode { + name: EMPTY_FILE_NAME.to_string(), + digest: Some(zero_digest.into()), + ..Default::default() + }, + FileNode { + name: NON_EMPTY_FILE_NAME.to_string(), + digest: Some(non_empty_digest.into()), + ..Default::default() + }, + ], + ..Default::default() + }; + slow_store + .as_ref() + .update_oneshot(root_directory_digest, root_directory.encode_to_vec().into()) + .await?; + + let download_dir = make_temp_path("download_dir"); + fs::create_dir_all(&download_dir).await?; + download_to_directory( + cas_store.as_ref(), + fast_store.as_pin(), + &root_directory_digest, + &download_dir, + ) + .await?; + + // Zero-digest file must exist on disk with zero bytes. + let empty_path = format!("{download_dir}/{EMPTY_FILE_NAME}"); + let empty_meta = fs::metadata(&empty_path) + .await + .err_tip(|| format!("Expected zero-digest file to be materialised at {empty_path}"))?; + assert!(empty_meta.is_file(), "{empty_path} must be a regular file"); + assert_eq!(empty_meta.len(), 0, "{empty_path} must be zero bytes"); + + // Sanity-check the non-zero-digest path still works. + let non_empty_path = format!("{download_dir}/{NON_EMPTY_FILE_NAME}"); + let non_empty_bytes = fs::read(&non_empty_path).await?; + assert_eq!(from_utf8(&non_empty_bytes)?, NON_EMPTY_CONTENT); + Ok(()) + } + // Windows does not support symlinks. #[cfg(not(target_family = "windows"))] #[nativelink_test] From adf8e17da5922e1adf4091fe1716adf351839dc3 Mon Sep 17 00:00:00 2001 From: Ernesto Cambuston Date: Sat, 16 May 2026 19:06:32 -0700 Subject: [PATCH 3/5] Close test coverage gaps for ported output-upload fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit of the two ported commits (e57b675b, b1dc3a1c) flagged two correctness-relevant gaps. Both are now closed with strict assertions so the underlying bugs cannot silently regress. 1. upload_absolute_symlink_to_directory_uploads_tree The original e57b675b regression test covers absolute-symlink-to-file but not absolute-symlink-to-directory, which exercises an entirely separate code path (upload_directory + Tree proto serialisation). The new test creates an out-of-tree directory containing inner.txt, absolute-symlinks it into the work dir, runs the action, and: - asserts output_directory_symlinks and output_file_symlinks are BOTH empty (the symlink must NOT be preserved), - asserts output_folders contains exactly one entry at the symlink path, proving Tree-upload happened, - walks the uploaded Tree, locates inner.txt, fetches its blob from CAS and verifies the content. This proves the directory was actually traversed and uploaded — not stubbed. 2. download_to_directory_zero_digest_empty_file_test (strengthened) Extended the existing single-file test to cover: - a second sibling zero-digest file (proves the path is not a single-file fluke), - a zero-digest file nested inside a subdirectory (proves the recursive download_to_directory caller also handles NotFound from get_file_entry_for_digest — not just the top-level invocation), - strict per-file assertions: is_file, !is_symlink, len == 0, and a read-back that confirms the file is actually empty rather than a phantom dirent. Verdict on the dropped checks: - Dangling absolute symlink: source returns OutputType::None which is the correct graceful behaviour, but writing a test for it requires fabricating a broken symlink mid-action and is comparatively low value; left uncovered intentionally. - File mode on zero-digest materialisation: the worker uses fs::create_file with no explicit mode, so the result follows umask and would be brittle across CI environments. Length + type assertions are the stronger invariant. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../tests/running_actions_manager_test.rs | 220 +++++++++++++++++- 1 file changed, 213 insertions(+), 7 deletions(-) diff --git a/nativelink-worker/tests/running_actions_manager_test.rs b/nativelink-worker/tests/running_actions_manager_test.rs index bc4615bf8..ee3fc5915 100644 --- a/nativelink-worker/tests/running_actions_manager_test.rs +++ b/nativelink-worker/tests/running_actions_manager_test.rs @@ -364,6 +364,9 @@ mod tests { // which covers a different code path (the input directory cache // short-circuit). const EMPTY_FILE_NAME: &str = "empty.txt"; + const SECOND_EMPTY_FILE_NAME: &str = "also_empty.log"; + const NESTED_EMPTY_FILE_NAME: &str = "nested_empty"; + const NESTED_DIR_NAME: &str = "subdir"; const NON_EMPTY_FILE_NAME: &str = "non_empty.txt"; const NON_EMPTY_CONTENT: &str = "non-empty"; @@ -380,6 +383,25 @@ mod tests { .update_oneshot(non_empty_digest, NON_EMPTY_CONTENT.into()) .await?; + // A nested subdirectory containing yet another zero-digest file — + // confirms the recursive download path also takes the empty-file + // branch (i.e. NotFound from get_file_entry_for_digest is handled + // by every caller, not just the root). The cas_store is a + // FastSlowStore, so the subdir Directory proto must live in slow. + let nested_dir_digest = DigestInfo::new([9u8; 32], 96); + let nested_dir = Directory { + files: vec![FileNode { + name: NESTED_EMPTY_FILE_NAME.to_string(), + digest: Some(zero_digest.into()), + ..Default::default() + }], + ..Default::default() + }; + slow_store + .as_ref() + .update_oneshot(nested_dir_digest, nested_dir.encode_to_vec().into()) + .await?; + let root_directory_digest = DigestInfo::new([8u8; 32], 64); let root_directory = Directory { files: vec![ @@ -388,12 +410,23 @@ mod tests { digest: Some(zero_digest.into()), ..Default::default() }, + // Second zero-digest at the same level — proves the path + // is not single-use / not dependent on any one filename. + FileNode { + name: SECOND_EMPTY_FILE_NAME.to_string(), + digest: Some(zero_digest.into()), + ..Default::default() + }, FileNode { name: NON_EMPTY_FILE_NAME.to_string(), digest: Some(non_empty_digest.into()), ..Default::default() }, ], + directories: vec![DirectoryNode { + name: NESTED_DIR_NAME.to_string(), + digest: Some(nested_dir_digest.into()), + }], ..Default::default() }; slow_store @@ -403,6 +436,10 @@ mod tests { let download_dir = make_temp_path("download_dir"); fs::create_dir_all(&download_dir).await?; + // The whole download succeeding is itself the strongest assertion + // that NotFound from get_file_entry_for_digest is HANDLED by the + // download_to_directory caller — if NotFound propagated it would + // surface as an Err here. download_to_directory( cas_store.as_ref(), fast_store.as_pin(), @@ -411,13 +448,27 @@ mod tests { ) .await?; - // Zero-digest file must exist on disk with zero bytes. - let empty_path = format!("{download_dir}/{EMPTY_FILE_NAME}"); - let empty_meta = fs::metadata(&empty_path) - .await - .err_tip(|| format!("Expected zero-digest file to be materialised at {empty_path}"))?; - assert!(empty_meta.is_file(), "{empty_path} must be a regular file"); - assert_eq!(empty_meta.len(), 0, "{empty_path} must be zero bytes"); + // All three zero-digest files must exist on disk as regular files + // with exactly zero bytes — strict assertions so a silent + // regression (missing file, wrong type, non-zero length) is + // impossible. + for relative in [ + EMPTY_FILE_NAME.to_string(), + SECOND_EMPTY_FILE_NAME.to_string(), + format!("{NESTED_DIR_NAME}/{NESTED_EMPTY_FILE_NAME}"), + ] { + let path = format!("{download_dir}/{relative}"); + let meta = fs::metadata(&path) + .await + .err_tip(|| format!("Expected zero-digest file to be materialised at {path}"))?; + assert!(meta.is_file(), "{path} must be a regular file"); + assert!(!meta.is_symlink(), "{path} must not be a symlink"); + assert_eq!(meta.len(), 0, "{path} must be exactly zero bytes"); + // Read back to confirm it is actually readable (not a phantom + // dirent) and truly empty. + let bytes = fs::read(&path).await?; + assert!(bytes.is_empty(), "{path} must read back as empty"); + } // Sanity-check the non-zero-digest path still works. let non_empty_path = format!("{download_dir}/{NON_EMPTY_FILE_NAME}"); @@ -1475,6 +1526,161 @@ mod tests { Ok(()) } + // Windows does not support symlinks. + #[cfg(not(target_family = "windows"))] + #[nativelink_test] + async fn upload_absolute_symlink_to_directory_uploads_tree() + -> Result<(), Box> { + // Regression test (companion to upload_absolute_symlink_resolves_contents): + // exercises the directory branch. When an absolute symlink points + // at a directory, the worker must walk it and upload a Tree proto + // — NOT preserve the symlink. The previous implementation produced + // an OutputType::DirectorySymlink with a worker-local absolute + // target that is meaningless on the client. + const WORKER_ID: &str = "foo_worker_id"; + + fn test_monotonic_clock() -> SystemTime { + static CLOCK: AtomicU64 = AtomicU64::new(0); + monotonic_clock(&CLOCK) + } + + let (_, slow_store, cas_store, ac_store) = setup_stores().await?; + let root_action_directory = make_temp_path("root_action_directory"); + fs::create_dir_all(&root_action_directory).await?; + + // Out-of-tree directory the action will absolute-symlink to. + let external_root = make_temp_path("external_dir_payload"); + fs::create_dir_all(&external_root).await?; + tokio::fs::write(format!("{external_root}/inner.txt"), b"inner-payload").await?; + + let running_actions_manager = Arc::new(RunningActionsManagerImpl::new_with_callbacks( + RunningActionsManagerArgs { + root_action_directory, + execution_configuration: ExecutionConfiguration::default(), + cas_store: cas_store.clone(), + ac_store: Some(Store::new(ac_store.clone())), + historical_store: Store::new(cas_store.clone()), + upload_action_result_config: &UploadActionResultConfig { + upload_ac_results_strategy: UploadCacheResultsStrategy::Never, + ..Default::default() + }, + max_action_timeout: Duration::MAX, + max_upload_timeout: Duration::from_secs(DEFAULT_MAX_UPLOAD_TIMEOUT), + timeout_handled_externally: false, + directory_cache: None, + #[cfg(target_os = "linux")] + use_namespaces: use_namespaces(), + }, + Callbacks { + now_fn: test_monotonic_clock, + sleep_fn: |_duration| Box::pin(future::pending()), + }, + )?); + let queued_timestamp = make_system_time(1000); + let action_result = { + let command = Command { + arguments: vec![ + "sh".to_string(), + "-c".to_string(), + format!("ln -s {external_root} resolved_dir"), + ], + output_paths: vec!["resolved_dir".to_string()], + working_directory: ".".to_string(), + environment_variables: vec![EnvironmentVariable { + name: "PATH".to_string(), + value: env::var("PATH").unwrap(), + }], + ..Default::default() + }; + let command_digest = serialize_and_upload_message( + &command, + cas_store.as_pin(), + &mut DigestHasherFunc::Sha256.hasher(), + ) + .await?; + let input_root_digest = serialize_and_upload_message( + &Directory::default(), + cas_store.as_pin(), + &mut DigestHasherFunc::Sha256.hasher(), + ) + .await?; + let action = Action { + command_digest: Some(command_digest.into()), + input_root_digest: Some(input_root_digest.into()), + ..Default::default() + }; + let action_digest = serialize_and_upload_message( + &action, + cas_store.as_pin(), + &mut DigestHasherFunc::Sha256.hasher(), + ) + .await?; + let execute_request = ExecuteRequest { + action_digest: Some(action_digest.into()), + ..Default::default() + }; + let operation_id = OperationId::default().to_string(); + let running_action_impl = running_actions_manager + .create_and_add_action( + WORKER_ID.to_string(), + StartExecute { + execute_request: Some(execute_request), + operation_id, + queued_timestamp: Some(queued_timestamp.into()), + platform: action.platform.clone(), + worker_id: WORKER_ID.to_string(), + }, + ) + .await?; + run_action(running_action_impl.clone()).await? + }; + + // The absolute directory symlink must be resolved into a Tree, not + // preserved as DirectorySymlink/FileSymlink. + assert_eq!( + action_result.output_directory_symlinks.len(), + 0, + "absolute dir symlink must be resolved, not uploaded as DirectorySymlink" + ); + assert_eq!( + action_result.output_file_symlinks.len(), + 0, + "absolute dir symlink must not appear as FileSymlink either" + ); + assert_eq!( + action_result.output_files.len(), + 0, + "directory target should not surface as an output file" + ); + assert_eq!( + action_result.output_folders.len(), + 1, + "absolute dir symlink should produce a single output_folders entry" + ); + let folder = &action_result.output_folders[0]; + assert_eq!(folder.path, "resolved_dir"); + // Walk the uploaded Tree and confirm inner.txt is present with + // correct content. This proves the directory was actually walked + // and uploaded — not a stub. + let tree = + get_and_decode_digest::(slow_store.as_ref(), folder.tree_digest.into()).await?; + let root = tree.root.expect("Tree must have a root Directory"); + let inner = root + .files + .iter() + .find(|f| f.name == "inner.txt") + .expect("inner.txt must be in uploaded Tree root"); + let inner_digest: DigestInfo = inner + .digest + .clone() + .expect("inner.txt must have a digest") + .try_into()?; + let key: nativelink_util::store_trait::StoreKey<'_> = inner_digest.into(); + let blob = slow_store.as_ref().get_part_unchunked(key, 0, None).await?; + assert_eq!(blob.as_ref(), b"inner-payload"); + Ok(()) + } + #[nativelink_test] async fn cleanup_happens_on_job_failure() -> Result<(), Box> { const WORKER_ID: &str = "foo_worker_id"; From 0724d3cc78da60311ff495703db553bdebbde49b Mon Sep 17 00:00:00 2001 From: Ernesto Cambuston Date: Tue, 19 May 2026 10:38:57 -0700 Subject: [PATCH 4/5] Address review feedback from palfrey on PR #2346 Two fixes for the new tests in upload_dir_and_symlink_test and the absolute-symlink tests: 1. Restore the absolute-symlink case in upload_dir_and_symlink_test. The previous commit replaced `empty_sym -> /dev/null` (absolute) with `rel_sym -> dir1/file` (relative), dropping coverage for the absolute branch in that integration test. Re-add `empty_sym` pointing to an out-of-tree zero-byte payload we create explicitly. This exercises both the relative-symlink-preserved path (via `rel_sym`) and the new absolute-symlink-resolved path (via `empty_sym`) without relying on `/dev/null`'s character-device semantics. `empty_sym` now appears in `output_files` (resolved, zero bytes, sha256 = e3b0c44...) rather than `output_file_symlinks`. 2. Replace the factually-wrong "Windows does not support symlinks" comments on the two new tests with an accurate explanation: Windows supports symlinks via std::os::windows::fs::symlink_file / symlink_dir, but the fixtures use `ln -s`, which is the Unix-only reason these tests are gated to non-Windows targets. --- .../tests/running_actions_manager_test.rs | 64 +++++++++++++++---- 1 file changed, 50 insertions(+), 14 deletions(-) diff --git a/nativelink-worker/tests/running_actions_manager_test.rs b/nativelink-worker/tests/running_actions_manager_test.rs index ee3fc5915..0004dd68b 100644 --- a/nativelink-worker/tests/running_actions_manager_test.rs +++ b/nativelink-worker/tests/running_actions_manager_test.rs @@ -1180,6 +1180,15 @@ mod tests { let root_action_directory = make_temp_path("root_action_directory"); fs::create_dir_all(&root_action_directory).await?; + // Out-of-tree payload that the action will reference via an + // *absolute* symlink. Exercises the "resolve absolute symlink and + // upload contents" code path alongside the relative-symlink + // preservation path below. + let external_root = make_temp_path("upload_dir_and_symlink_external"); + fs::create_dir_all(&external_root).await?; + let external_file = format!("{external_root}/empty_payload"); + tokio::fs::write(&external_file, b"").await?; + let running_actions_manager = Arc::new(RunningActionsManagerImpl::new_with_callbacks( RunningActionsManagerArgs { root_action_directory, @@ -1209,18 +1218,27 @@ mod tests { arguments: vec![ "sh".to_string(), "-c".to_string(), - concat!( - "mkdir -p dir1/dir2 && ", - "echo foo > dir1/file && ", - "touch dir1/file2 && ", - "ln -s ../file dir1/dir2/sym &&", - // Relative symlink — preserved as a symlink in the - // ActionResult (absolute symlinks are now resolved). - "ln -s dir1/file rel_sym", - ) - .to_string(), + format!( + "mkdir -p dir1/dir2 && \ + echo foo > dir1/file && \ + touch dir1/file2 && \ + ln -s ../file dir1/dir2/sym && \ + ln -s dir1/file rel_sym && \ + ln -s {external_file} empty_sym", + ), + ], + // `dir1` exercises the directory upload path, + // `rel_sym` exercises the relative-symlink-preserved path, + // `empty_sym` exercises the absolute-symlink-resolved path + // (previously this test asserted `empty_sym` was kept as a + // `SymlinkInfo` with target `/dev/null`; that behavior is + // now incorrect because absolute symlinks are worker-local + // and must be resolved before upload). + output_paths: vec![ + "dir1".to_string(), + "empty_sym".to_string(), + "rel_sym".to_string(), ], - output_paths: vec!["dir1".to_string(), "rel_sym".to_string()], working_directory: ".".to_string(), environment_variables: vec![EnvironmentVariable { name: "PATH".to_string(), @@ -1336,7 +1354,17 @@ mod tests { assert_eq!( action_result, ActionResult { - output_files: vec![], + // `empty_sym` was an absolute symlink — the worker resolves + // it and uploads the underlying (empty) file. The resulting + // digest is the well-known sha256 of zero bytes. + output_files: vec![FileInfo { + name_or_path: NameOrPath::Path("empty_sym".to_string()), + digest: DigestInfo::try_new( + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + 0, + )?, + is_executable: false, + }], stdout_digest: DigestInfo::try_new( "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", 0 @@ -1378,7 +1406,11 @@ mod tests { Ok(()) } - // Windows does not support symlinks. + // The fixture is built with the `ln -s` shell builtin (matching the + // convention used by `upload_dir_and_symlink_test` above), so this test + // only runs on Unix-like platforms. Windows itself does support symlinks + // via `std::os::windows::fs::{symlink_file, symlink_dir}`, but creating + // them from a shell isn't portable. #[cfg(not(target_family = "windows"))] #[nativelink_test] async fn upload_absolute_symlink_resolves_contents() -> Result<(), Box> @@ -1526,7 +1558,11 @@ mod tests { Ok(()) } - // Windows does not support symlinks. + // The fixture is built with the `ln -s` shell builtin (matching the + // convention used by `upload_dir_and_symlink_test` above), so this test + // only runs on Unix-like platforms. Windows itself does support symlinks + // via `std::os::windows::fs::{symlink_file, symlink_dir}`, but creating + // them from a shell isn't portable. #[cfg(not(target_family = "windows"))] #[nativelink_test] async fn upload_absolute_symlink_to_directory_uploads_tree() From 2d368283abb378a942f5977473aafdd800c6bae5 Mon Sep 17 00:00:00 2001 From: Ernesto Cambuston Date: Tue, 19 May 2026 11:06:18 -0700 Subject: [PATCH 5/5] running_actions_manager_test: use /dev/null in upload_dir_and_symlink_test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous iterations of this PR sidestepped `/dev/null` by swapping the absolute-symlink fixture for either a relative symlink (lost coverage) or an out-of-tree real empty file (added scaffolding to dodge a corner case we should just test directly). Restore the original `ln -s /dev/null empty_sym` fixture. The post-fix worker code resolves the absolute symlink via `fs::metadata` (follows), sees a character device (`is_dir() == false`), and falls into the "upload as file" branch. `upload_file` opens `/dev/null` and reads to EOF — `/dev/null`'s contract is that reads return 0 bytes immediately — producing the canonical sha256 empty digest (e3b0c44…) with size 0. That's well-defined, harmless behavior and a real-world Bazel pattern (rules sometimes use `ln -sf /dev/null x` to create empty outputs). Test now locks in this contract directly, no scaffolding required. Drops the `external_root` / `external_file` setup since it was only needed to avoid `/dev/null`. --- .../tests/running_actions_manager_test.rs | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/nativelink-worker/tests/running_actions_manager_test.rs b/nativelink-worker/tests/running_actions_manager_test.rs index 0004dd68b..50db30b8e 100644 --- a/nativelink-worker/tests/running_actions_manager_test.rs +++ b/nativelink-worker/tests/running_actions_manager_test.rs @@ -1180,15 +1180,6 @@ mod tests { let root_action_directory = make_temp_path("root_action_directory"); fs::create_dir_all(&root_action_directory).await?; - // Out-of-tree payload that the action will reference via an - // *absolute* symlink. Exercises the "resolve absolute symlink and - // upload contents" code path alongside the relative-symlink - // preservation path below. - let external_root = make_temp_path("upload_dir_and_symlink_external"); - fs::create_dir_all(&external_root).await?; - let external_file = format!("{external_root}/empty_payload"); - tokio::fs::write(&external_file, b"").await?; - let running_actions_manager = Arc::new(RunningActionsManagerImpl::new_with_callbacks( RunningActionsManagerArgs { root_action_directory, @@ -1218,22 +1209,24 @@ mod tests { arguments: vec![ "sh".to_string(), "-c".to_string(), - format!( - "mkdir -p dir1/dir2 && \ + "mkdir -p dir1/dir2 && \ echo foo > dir1/file && \ touch dir1/file2 && \ ln -s ../file dir1/dir2/sym && \ ln -s dir1/file rel_sym && \ - ln -s {external_file} empty_sym", - ), + ln -s /dev/null empty_sym" + .to_string(), ], // `dir1` exercises the directory upload path, // `rel_sym` exercises the relative-symlink-preserved path, // `empty_sym` exercises the absolute-symlink-resolved path - // (previously this test asserted `empty_sym` was kept as a - // `SymlinkInfo` with target `/dev/null`; that behavior is - // now incorrect because absolute symlinks are worker-local - // and must be resolved before upload). + // against `/dev/null`. Pre-fix this test asserted `empty_sym` + // was kept as a `SymlinkInfo` with target `/dev/null`; that + // behavior is now incorrect because absolute symlinks are + // worker-local and must be resolved before upload. Reading + // `/dev/null` returns 0 bytes immediately by its character- + // device contract, so the worker produces an empty-file + // output with the canonical sha256 empty digest. output_paths: vec![ "dir1".to_string(), "empty_sym".to_string(),