Check the cache again after acquiring the download lock

[ilyagr]

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 (#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.

#!/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 <<EOF
#!/usr/bin/env dotslash
{"name": "hello", "platforms": {"$platform": {
  "size": $(wc -c < hello.txt), "hash": "sha256",
  "digest": "$("$dotslash" -- sha256 hello.txt)",
  "path": "hello.txt",
  "providers": [{"url": "http://127.0.0.1:8765/hello.txt"}]
}}}
EOF

# A file server that takes 2 seconds per request, so that concurrent fetches
# queue up on the DotSlash download lock.
python3 -c '
import http.server, time
class H(http.server.SimpleHTTPRequestHandler):
    def do_GET(self):
        time.sleep(2)
        super().do_GET()
http.server.ThreadingHTTPServer(("127.0.0.1", 8765), H).serve_forever()
' 2> 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):

$ sh repro.sh ./dotslash-before
downloads: 4, seconds: 8

After:

$ sh repro.sh target/release/dotslash
downloads: 1, seconds: 2

[meta-codesync]

This pull request has been imported. If you are a Meta employee, you can view this in D123508578. (Because this pull request was imported automatically, there will not be any future comments.)