Skip to content

worker: fix two silent output-corruption bugs (absolute symlinks + zero-digest files) #2346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
palfrey merged 6 commits into from
May 20, 2026
Merged

worker: fix two silent output-corruption bugs (absolute symlinks + zero-digest files) #2346

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
16825a0
Resolve absolute symlinks in output upload instead of uploading raw t…
erneestoc May 17, 2026
b6c4264
Fix zero-digest files missing from worker execution directories
erneestoc May 17, 2026
adf8e17
Close test coverage gaps for ported output-upload fixes
erneestoc May 17, 2026
0724d3c
Address review feedback from palfrey on PR #2346
erneestoc May 19, 2026
2d36828
running_actions_manager_test: use /dev/null in upload_dir_and_symlink…
erneestoc May 19, 2026
91a9f6e
Merge branch 'main' into ec/pr2243-output-upload-correctness
MarcusSorealheis May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions nativelink-store/src/filesystem_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,16 +749,19 @@ impl<Fe: FileEntry> FilesystemStore<Fe> {
}

pub async fn get_file_entry_for_digest(&self, digest: &DigestInfo) -> Result<Arc<Fe>, 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())
Expand Down
22 changes: 19 additions & 3 deletions nativelink-store/tests/filesystem_store_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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(())
}

Expand Down
154 changes: 127 additions & 27 deletions nativelink-worker/src/running_actions_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1366,34 +1376,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 {
Expand Down
Loading
Loading