From 4150140a75177b12dde23442430c7a1cf54e9b05 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 2 Oct 2026 22:10:21 -0700 Subject: [PATCH] Check the cache again after acquiring the download lock ## The problem When several DotSlash processes need the same artifact at the same time, all but one of them block on the artifact's download lock. When the first one finishes and releases the lock, the next one acquires it and downloads the artifact again, even though it is already in the cache by then. So does every other waiting process: N concurrent invocations download the artifact N times, one after another, and `mv_no_clobber()` silently discards every copy but the first. I ran into this when `buck2` launched several `rustc` processes at once on a new machine and `rustc` was managed by DotSlash. With a ~180 MB toolchain tarball and five build actions waiting on it, a `buck2` build spent over ten minutes downloading the same tarball again and again. For archive formats, each redundant download also leaves its archive behind in the cache (https://github.com/facebook/dotslash/issues/168). The problem also happens when different DotSlash files that share an artifact (e.g. `node`, `npm` and `npx`) run concurrently. ## This commit The root of the problem is that callers only call `download_artifact()` after finding the artifact missing from the cache, but `download_artifact()` does not check again once it holds the lock. This adds that check. The check is also correct when no lock could be acquired: the artifact directory is moved into place with a rename, so if the executable exists, the artifact is complete. ## Reproduction/test There's a unit test in the diff, but here's how to reproduce the full problem, with real concurrency, in the shell with `python3` and `curl`. It serves a small file over HTTP with a 2-second delay per request and runs `dotslash -- fetch` on it four times concurrently. ```bash #!/bin/sh # Usage: repro.sh path/to/dotslash set -eu dotslash=$(realpath "$1") cd "$(mktemp -d)" export DOTSLASH_CACHE="$PWD/cache" echo hello > hello.txt platform=$(uname -s | sed 's/Darwin/macos/;s/Linux/linux/')-$(uname -m | sed 's/arm64/aarch64/') cat > hello < server.log & trap "kill $!" EXIT until curl -sf --head http://127.0.0.1:8765/hello.txt > /dev/null; do sleep 0.1 done start=$(date +%s) pids= for i in 1 2 3 4; do "$dotslash" -- fetch ./hello > /dev/null & pids="$pids $!" done wait $pids # Count GET requests, skip HEAD requests by `curl --head` above downloads=$(grep -c 'GET /hello.txt' server.log || true) echo "downloads: $downloads, seconds: $(($(date +%s) - start))" ``` Before this change (macOS, aarch64): ```console $ sh repro.sh ./dotslash-before downloads: 4, seconds: 8 ``` After: ```console $ sh repro.sh target/release/dotslash downloads: 1, seconds: 2 ``` --- src/download.rs | 70 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/src/download.rs b/src/download.rs index e5aef9a..2718963 100644 --- a/src/download.rs +++ b/src/download.rs @@ -62,6 +62,12 @@ pub fn download_artifact( let file_lock = acquire_download_lock_for_artifact(artifact_location) .context("failed to get artifact lock")?; + // Another process may have fetched the artifact while we were waiting for + // the lock. + if artifact_location.executable.exists() { + return Ok(()); + } + // Record warnings: only reported if no provider succeeds. let mut warnings = vec![]; @@ -330,9 +336,73 @@ pub fn acquire_download_lock_for_artifact( #[cfg(test)] mod tests { + use std::fs; + use rand::SeedableRng; use super::*; + use crate::config::Arg0; + use crate::provider::Provider; + + struct PanickingProviderFactory; + + impl ProviderFactory for PanickingProviderFactory { + fn get_provider(&self, provider_type: &str) -> anyhow::Result> { + panic!("unexpected fetch with provider type `{provider_type}`"); + } + } + + /// Simulates `download_artifact()` being called when the executable is + /// already in the cache. Callers only call it when they didn't find the + /// executable, but it can be there anyway by the time + /// `download_artifact()` holds the lock: + /// + /// - Another process fetched the artifact after the caller checked, + /// usually while this process waited for the lock. That process may + /// have run the same DotSlash file, or a different one whose artifact + /// is the same archive (e.g. `node` and `npx` from one Node.js tarball). + /// - Running the executable failed with a not-found error even though it + /// exists, e.g. because its shebang interpreter is missing. + /// + /// In either case, `download_artifact()` should not download the artifact + /// again. + #[test] + fn download_artifact_skips_fetch_if_already_present() { + let temp_dir = tempfile::tempdir().unwrap(); + let artifact_directory = temp_dir.path().join("artifact"); + let executable = artifact_directory.join("hello"); + fs::create_dir(&artifact_directory).unwrap(); + fs::write(&executable, "").unwrap(); + let artifact_location = ArtifactLocation { + artifact_directory, + executable, + lock_path: temp_dir.path().join("lock"), + arg0: Arg0::UnderlyingExecutable, + }; + let artifact_entry = ArtifactEntry { + size: 0, + hash: HashAlgorithm::Sha256, + digest: Digest::try_from( + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855".to_owned(), + ) + .unwrap(), + format: ArtifactFormat::Plain, + path: "hello".parse().unwrap(), + providers: vec![ + serde_json::from_str(r#"{"url": "https://example.com/hello"}"#).unwrap(), + ], + arg0: Arg0::UnderlyingExecutable, + readonly: true, + providers_order: ProvidersOrder::Sequential, + }; + + download_artifact( + &artifact_entry, + &artifact_location, + &PanickingProviderFactory, + ) + .expect("download_artifact should succeed without fetching"); + } #[test] fn providers_in_order_sequential() {