Skip to content

Commit

Permalink
fix(cache): restoration symlink (#7633)
Browse files Browse the repository at this point in the history
### Description

Fix for secondary problem discovered in #7410

Again, reading the docs for
[`Entry::link_name`](https://docs.rs/tar/latest/tar/struct.Entry.html#method.link_name)
it is recommended against using a header as it might have an
incomplete/differing link name from the actual entry. `restore_symlink`
already uses this method over accessing the link name via the header so
it doesn't need to be updated.

### Testing Instructions

Added failing unit test in first commit. The test adds a symlink to a
directory with a long path that gets restored before the target has been
restored. This results in us hitting the
`topologically_restore_symlinks` codepath which contained the bug.

Also tested against updates to
https://github.com/trappar/turbo-cache-missing-output-files which also
trigger this behavior.


Closes TURBO-2539
  • Loading branch information
chris-olszewski committed Mar 6, 2024
1 parent 210b149 commit cdcdc53
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 6 deletions.
9 changes: 8 additions & 1 deletion crates/turborepo-cache/src/cache_archive/create.rs
Expand Up @@ -397,9 +397,11 @@ mod tests {
let mut archive = CacheWriter::create(&tar_path)?;
let base = "this-is-a-really-really-really-long-path-like-so-very-long-that-i-can-list-all-of-my-favorite-directors-like-edward-yang-claire-denis-lucrecia-martel-wong-kar-wai-even-kurosawa";
let file_name = format!("{base}.txt");
let dir_symlink_name = format!("{base}-dir");
let really_long_file = AnchoredSystemPath::new(&file_name).unwrap();
let really_long_dir = AnchoredSystemPath::new(base).unwrap();
let really_long_symlink = AnchoredSystemPath::new("this-is-a-really-really-really-long-symlink-like-so-very-long-that-i-can-list-all-of-my-other-favorite-directors-like-jim-jarmusch-michelangelo-antonioni-and-terrence-malick-symlink").unwrap();
let really_long_dir_symlink = AnchoredSystemPath::new(&dir_symlink_name).unwrap();

let really_long_path = archive_dir_path.resolve(really_long_file);
really_long_path.create_with_contents("The End!")?;
Expand All @@ -410,7 +412,11 @@ mod tests {
let really_long_dir_path = archive_dir_path.resolve(really_long_dir);
really_long_dir_path.create_dir_all()?;

let really_long_dir_symlink_path = archive_dir_path.resolve(really_long_dir_symlink);
really_long_dir_symlink_path.symlink_to_dir(really_long_dir.as_str())?;

archive.add_file(archive_dir_path, really_long_file)?;
archive.add_file(archive_dir_path, really_long_dir_symlink)?;
archive.add_file(archive_dir_path, really_long_dir)?;
archive.add_file(archive_dir_path, really_long_symlink)?;

Expand All @@ -421,10 +427,11 @@ mod tests {

let mut restore = CacheReader::open(&tar_path)?;
let files = restore.restore(restore_dir_path)?;
assert_eq!(files.len(), 3);
assert_eq!(files.len(), 4);
assert_eq!(files[0].as_str(), really_long_file.as_str());
assert_eq!(files[1].as_str(), really_long_dir.as_str());
assert_eq!(files[2].as_str(), really_long_symlink.as_str());
assert_eq!(files[3].as_str(), really_long_dir_symlink.as_str());
Ok(())
}

Expand Down
7 changes: 2 additions & 5 deletions crates/turborepo-cache/src/cache_archive/restore.rs
Expand Up @@ -124,14 +124,11 @@ impl<'a> CacheReader<'a> {
let mut nodes = HashMap::new();

for entry in symlinks {
let processed_name = AnchoredSystemPathBuf::from_system_path(&entry.header().path()?)?;
let processed_name = AnchoredSystemPathBuf::from_system_path(&entry.path()?)?;
let processed_sourcename =
canonicalize_linkname(anchor, &processed_name, processed_name.as_path())?;
// symlink must have a linkname
let linkname = entry
.header()
.link_name()?
.expect("symlink without linkname");
let linkname = entry.link_name()?.expect("symlink without linkname");

let processed_linkname = canonicalize_linkname(anchor, &processed_name, &linkname)?;

Expand Down

0 comments on commit cdcdc53

Please sign in to comment.