Skip to content

Commit 74aa906

Browse files
erneestocclaudeMarcusSorealheis
authored
worker: fix two silent output-corruption bugs (absolute symlinks + zero-digest files) (#2346)
* 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 0807153 (PR #2243). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * 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 19d7e20 (PR #2243). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Close test coverage gaps for ported output-upload fixes 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) <noreply@anthropic.com> * running_actions_manager_test: use /dev/null in upload_dir_and_symlink_test 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`. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Marcus Eagan <marcuseagan@gmail.com>
1 parent a00bf8c commit 74aa906

4 files changed

Lines changed: 631 additions & 51 deletions

File tree

nativelink-store/src/filesystem_store.rs

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -749,16 +749,19 @@ impl<Fe: FileEntry> FilesystemStore<Fe> {
749749
}
750750

751751
pub async fn get_file_entry_for_digest(&self, digest: &DigestInfo) -> Result<Arc<Fe>, Error> {
752+
// Zero-digest blobs have no backing file on disk (FilesystemStore
753+
// never persists zero-byte content). The previous implementation
754+
// returned a synthetic FileEntry whose content_path did not exist,
755+
// which downstream callers would then try to hard_link from,
756+
// silently producing missing or empty output files in worker
757+
// execution directories. Return NotFound so callers are forced to
758+
// take the explicit zero-digest path (e.g. fs::create_file).
752759
if is_zero_digest(digest) {
753-
return Ok(Arc::new(Fe::create(
754-
0,
755-
0,
756-
RwLock::new(EncodedFilePath {
757-
shared_context: self.shared_context.clone(),
758-
path_type: PathType::Content,
759-
key: digest.into(),
760-
}),
761-
)));
760+
return Err(make_err!(
761+
Code::NotFound,
762+
"{digest} is a zero-digest; FilesystemStore does not persist zero-byte files. \
763+
Callers must materialise empty files directly rather than going through get_file_entry_for_digest."
764+
));
762765
}
763766
self.evicting_map
764767
.get(&digest.into())

nativelink-store/tests/filesystem_store_test.rs

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,7 +1004,20 @@ async fn update_with_zero_digest() -> Result<(), Error> {
10041004
}
10051005

10061006
#[nativelink_test]
1007-
async fn get_file_entry_for_zero_digest() -> Result<(), Error> {
1007+
async fn get_file_entry_for_zero_digest_returns_not_found() -> Result<(), Error> {
1008+
// Regression test for: zero-digest blobs have no backing file on disk,
1009+
// so `get_file_entry_for_digest` previously handed back a synthetic
1010+
// FileEntry whose content_path did not exist. Downstream callers (the
1011+
// worker output-materialisation path) would then try to hard_link from
1012+
// that non-existent source, silently producing missing or empty output
1013+
// files in worker exec dirs.
1014+
//
1015+
// After the fix, `get_file_entry_for_digest` returns NotFound for
1016+
// zero digests so callers are forced to materialise empty files
1017+
// directly (e.g. via `fs::create_file`). This complements the
1018+
// DirectoryCache zero-byte short-circuit (input-fetch path); this
1019+
// test covers the FilesystemStore API used by the output-upload /
1020+
// hardlink path.
10081021
let digest = DigestInfo::new(Sha256::new().finalize().into(), 0);
10091022
let content_path = make_temp_path("content_path");
10101023
let temp_path = make_temp_path("temp_path");
@@ -1020,8 +1033,11 @@ async fn get_file_entry_for_zero_digest() -> Result<(), Error> {
10201033
)
10211034
.await?;
10221035

1023-
let file_entry = store.get_file_entry_for_digest(&digest).await?;
1024-
assert!(file_entry.is_empty());
1036+
let err = store
1037+
.get_file_entry_for_digest(&digest)
1038+
.await
1039+
.expect_err("zero digest must not return a synthetic FileEntry");
1040+
assert_eq!(err.code, Code::NotFound, "expected NotFound, got: {err:?}");
10251041
Ok(())
10261042
}
10271043

nativelink-worker/src/running_actions_manager.rs

Lines changed: 127 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,18 @@ pub fn download_to_directory<'a>(
163163
.populate_fast_store(digest.into())
164164
.and_then(move |()| async move {
165165
if is_zero_digest(digest) {
166-
let mut file_slot = fs::create_file(&dest).await?;
167-
file_slot.write_all(&[]).await?;
166+
// Zero-digest files are never persisted by the
167+
// FilesystemStore, so materialise them directly
168+
// in the worker exec dir. Attach context so a
169+
// failure here is diagnosable as a missing
170+
// empty file rather than a generic IO error.
171+
let mut file_slot = fs::create_file(&dest)
172+
.await
173+
.err_tip(|| format!("Could not create zero-digest file at {dest}"))?;
174+
file_slot
175+
.write_all(&[])
176+
.await
177+
.err_tip(|| format!("Could not write zero-digest file at {dest}"))?;
168178
}
169179
else {
170180
let file_entry = filesystem_store
@@ -1384,34 +1394,124 @@ impl RunningActionImpl {
13841394
.err_tip(|| format!("Uploading directory {}", full_path.display()))?,
13851395
))
13861396
} else if metadata.is_symlink() {
1387-
let output_symlink = upload_symlink(&full_path, work_directory)
1388-
.await
1389-
.map(|mut symlink_info| {
1390-
symlink_info.name_or_path = NameOrPath::Path(entry);
1391-
symlink_info
1392-
})
1393-
.err_tip(|| format!("Uploading symlink {}", full_path.display()))?;
1394-
match fs::metadata(&full_path).await {
1395-
Ok(metadata) => {
1396-
if metadata.is_dir() {
1397-
Ok(OutputType::DirectorySymlink(output_symlink))
1398-
} else {
1399-
// Note: If it's anything but directory we put it as a file symlink.
1400-
Ok(OutputType::FileSymlink(output_symlink))
1397+
// Resolve the symlink to determine what it points to.
1398+
// Symlinks created by DirectoryCache (absolute paths into
1399+
// the cache directory) must NOT be uploaded as symlinks —
1400+
// the target path is worker-local and meaningless to the
1401+
// client. Instead, follow the symlink and upload the
1402+
// resolved content (file or directory).
1403+
let target = fs::read_link(&full_path).await.err_tip(|| {
1404+
format!("Reading symlink target for {}", full_path.display())
1405+
})?;
1406+
let is_absolute_symlink = Path::new(&target).is_absolute();
1407+
1408+
if is_absolute_symlink {
1409+
// Absolute symlink — resolve and upload contents.
1410+
match fs::metadata(&full_path).await {
1411+
Ok(resolved_meta) => {
1412+
if resolved_meta.is_dir() {
1413+
// Upload as directory (Tree proto).
1414+
Ok(OutputType::Directory(
1415+
upload_directory(
1416+
cas_store.as_pin(),
1417+
&full_path,
1418+
work_directory,
1419+
hasher,
1420+
digest_uploaders,
1421+
)
1422+
.and_then(|(root_dir, children)| async move {
1423+
let tree = ProtoTree {
1424+
root: Some(root_dir),
1425+
children: children.into(),
1426+
};
1427+
let tree_digest = serialize_and_upload_message(
1428+
&tree,
1429+
cas_store.as_pin(),
1430+
&mut hasher.hasher(),
1431+
)
1432+
.await
1433+
.err_tip(|| format!("While processing {entry}"))?;
1434+
Ok(DirectoryInfo {
1435+
path: entry,
1436+
tree_digest,
1437+
})
1438+
})
1439+
.await
1440+
.err_tip(|| {
1441+
format!(
1442+
"Uploading symlinked directory {}",
1443+
full_path.display()
1444+
)
1445+
})?,
1446+
))
1447+
} else {
1448+
// Upload as file (follow symlink).
1449+
Ok(OutputType::File(
1450+
upload_file(
1451+
cas_store.as_pin(),
1452+
&full_path,
1453+
hasher,
1454+
resolved_meta,
1455+
digest_uploaders,
1456+
)
1457+
.await
1458+
.map(|mut file_info| {
1459+
file_info.name_or_path = NameOrPath::Path(entry);
1460+
file_info
1461+
})
1462+
.err_tip(|| {
1463+
format!(
1464+
"Uploading symlinked file {}",
1465+
full_path.display()
1466+
)
1467+
})?,
1468+
))
1469+
}
1470+
}
1471+
Err(e) => {
1472+
if e.code != Code::NotFound {
1473+
return Err(e).err_tip(|| {
1474+
format!(
1475+
"While resolving absolute symlink {}",
1476+
full_path.display()
1477+
)
1478+
});
1479+
}
1480+
Ok(OutputType::None)
14011481
}
14021482
}
1403-
Err(e) => {
1404-
if e.code != Code::NotFound {
1405-
return Err(e).err_tip(|| {
1406-
format!(
1407-
"While querying target symlink metadata for {}",
1408-
full_path.display()
1409-
)
1410-
});
1483+
} else {
1484+
// Relative symlink — action intentionally created it.
1485+
// Upload as a proper symlink.
1486+
let output_symlink = upload_symlink(&full_path, work_directory)
1487+
.await
1488+
.map(|mut symlink_info| {
1489+
symlink_info.name_or_path = NameOrPath::Path(entry);
1490+
symlink_info
1491+
})
1492+
.err_tip(|| format!("Uploading symlink {}", full_path.display()))?;
1493+
match fs::metadata(&full_path).await {
1494+
Ok(metadata) => {
1495+
if metadata.is_dir() {
1496+
Ok(OutputType::DirectorySymlink(output_symlink))
1497+
} else {
1498+
// Note: If it's anything but directory we put it as a file symlink.
1499+
Ok(OutputType::FileSymlink(output_symlink))
1500+
}
1501+
}
1502+
Err(e) => {
1503+
if e.code != Code::NotFound {
1504+
return Err(e).err_tip(|| {
1505+
format!(
1506+
"While querying target symlink metadata for {}",
1507+
full_path.display()
1508+
)
1509+
});
1510+
}
1511+
// If the file doesn't exist, we consider it a file. Even though the
1512+
// file doesn't exist we still need to populate an entry.
1513+
Ok(OutputType::FileSymlink(output_symlink))
14111514
}
1412-
// If the file doesn't exist, we consider it a file. Even though the
1413-
// file doesn't exist we still need to populate an entry.
1414-
Ok(OutputType::FileSymlink(output_symlink))
14151515
}
14161516
}
14171517
} else {

0 commit comments

Comments
 (0)