fix(hooks): make cache-miss download race-safe

`common::resolve_tool_path` downloaded/unzipped every pinned tool
version directly into the shared, version-keyed cache directory. When
N pre-commit hook processes raced the same uncached (tool, version) at
once, they stepped on each other's `curl`/`unzip` output: one
process' cleanup could delete the archive out from under another's
still-running `unzip` ("cannot find or open ... .zip"), or `unzip`
could meet a binary a sibling already extracted and block on an
interactive overwrite prompt - hanging under pre-commit's
non-interactive stdin.

- Add `common::populate_tool_cache`: downloads/installs into a
  private per-process staging dir (`mktemp -d`), then atomically
  publishes the resulting binary into the cache via a plain-file
  `mv`. A process that loses the race discards its own redundant
  copy instead of corrupting the winner's.
- Add `test_concurrent_cache_miss_is_race_free` (network): 2
  concurrent real downloads against an empty cache, asserting a
  clean single binary and no leftover staging dirs.

Assisted-by: Sisyphus:claude-sonnet-5 opencode

Author: MaxymVlasov

hooks/_common.sh

@@ -544,6 +544,104 @@ function common::detect_os_arch {
   export TARGETOS TARGETARCH
 }
 
+#######################################################################
+# Download and install one tool version into a private, per-process
+# staging directory, then atomically publish the resulting binary
+# into the shared cache.
+#
+# `tools/install/<tool>.sh` installer scripts use a fixed filename
+# relative to their CWD (e.g. `terraform.zip`), which is only safe
+# with one process per CWD at a time - true for their original
+# purpose (one `RUN` per tool, per Docker build), not for N pre-commit
+# hook processes racing the same cache miss. Without this, two such
+# processes could interleave on the same shared directory: one's
+# `rm "$PKG"` deleting the archive out from under the other's still-
+# running `unzip` ("cannot find or open ... .zip"), or `unzip` meeting
+# a binary a sibling already extracted and blocking on an interactive
+# overwrite prompt that hangs under pre-commit's non-interactive
+# stdin. Giving every caller its own staging directory removes the
+# shared CWD these scripts were never designed to share.
+# Globals:
+#   GITHUB_TOKEN - forwarded automatically; read directly by the
+#     invoked installer script
+# Arguments:
+#   tool_name (string) tool name, matching a `tools/install/<tool>.sh`
+#     file and its expected `${TOOL^^}_VERSION` environment variable
+#   version (string) exact version to install
+#   installer_script (string) absolute path to the
+#     `tools/install/<tool>.sh` script to invoke
+#   env_var_name (string) name of the environment variable the
+#     installer script reads its requested version from (e.g.
+#     "TERRAFORM_VERSION")
+#   cache_dir (string) final, shared cache directory for this
+#     (tool, version) pair. Must already exist.
+#   cached_bin (string) expected absolute path to the resolved binary
+#     inside `cache_dir` once installed
+# Outputs:
+#   Returns 0 once `cached_bin` exists - either from this process' own
+#   install, or from another process that won the race first. Returns
+#   1 with an error message if the download/install itself failed.
+#######################################################################
+function common::populate_tool_cache {
+  local -r tool_name="$1"
+  local -r version="$2"
+  local -r installer_script="$3"
+  local -r env_var_name="$4"
+  local -r cache_dir="$5"
+  local -r cached_bin="$6"
+
+  # `mktemp -d` guarantees a directory no other process is also using,
+  # so each process' installer run is isolated - regardless of how
+  # many processes hit the same cache miss at once.
+  local staging_dir
+  staging_dir=$(mktemp -d "${cache_dir}.XXXXXXXXXX") || {
+    common::colorify "red" "ERROR: Failed to create a staging directory for '$tool_name' version '$version'."
+    return 1
+  }
+
+  # Redirect the installer's own stdout to stderr: this function's stdout is
+  # a contract (the resolved path, captured via "$(...)" by every caller),
+  # and installers like terraform.sh/tflint.sh call bare `unzip` (no `-q`),
+  # which prints "Archive: ... inflating: ..." to stdout by default -
+  # harmless noise in a Docker build log, but it would otherwise corrupt
+  # the path this function returns.
+  if ! (
+    cd "$staging_dir" || exit 1
+    export "$env_var_name=$version"
+    "$installer_script" 1>&2
+  ); then
+    common::colorify "red" "ERROR: Failed to download '$tool_name' version '$version' via '$installer_script'."
+    rm -rf "$staging_dir"
+    return 1
+  fi
+
+  # Another process may have already published `cached_bin` while this
+  # one was downloading its own copy - if so, prefer that result over
+  # ours (both are the same requested version) instead of racing again
+  # on the `mv` below.
+  if [[ -x $cached_bin ]]; then
+    rm -rf "$staging_dir"
+    return 0
+  fi
+
+  # `mv` between two paths on the same filesystem is atomic (POSIX
+  # `rename(2)`), and both sides here are plain files, not
+  # directories, so a concurrent reader of `cached_bin` always sees
+  # either the previous state or the fully-written new one - never a
+  # partially-written file - on both GNU and BSD `mv`.
+  if ! mv "$staging_dir/$(basename "$cached_bin")" "$cached_bin" 2> /dev/null; then
+    rm -rf "$staging_dir"
+    # Lost the race between the check above and this `mv`: some other
+    # process' `mv` landed on `cached_bin` first. That's fine, its
+    # result is equally valid, ours is simply redundant.
+    [[ -x $cached_bin ]] && return 0
+    common::colorify "red" "ERROR: Failed to move '$tool_name' version '$version' into place at '$cached_bin'."
+    return 1
+  fi
+
+  rm -rf "$staging_dir"
+}
+
 #######################################################################
 # Resolve a specific version of a wrapped tool's binary, downloading
 # and caching it on demand if it isn't already cached.
@@ -682,20 +780,7 @@ function common::resolve_tool_path {
 
   mkdir -p "$cache_dir"
 
-  # Redirect the installer's own stdout to stderr: this function's stdout is
-  # a contract (the resolved path, captured via "$(...)" by every caller),
-  # and installers like terraform.sh/tflint.sh call bare `unzip` (no `-q`),
-  # which prints "Archive: ... inflating: ..." to stdout by default -
-  # harmless noise in a Docker build log, but it would otherwise corrupt
-  # the path this function returns.
-  if ! (
-    cd "$cache_dir" || exit 1
-    export "$env_var_name=$version"
-    "$installer_script" 1>&2
-  ); then
-    common::colorify "red" "ERROR: Failed to download '$tool_name' version '$version' via '$installer_script'."
-    exit 1
-  fi
+  common::populate_tool_cache "$tool_name" "$version" "$installer_script" "$env_var_name" "$cache_dir" "$cached_bin" || exit $?
 
   if [[ ! -x $cached_bin ]]; then
     common::colorify "red" "ERROR: '$tool_name' installer completed but expected binary was not found at '$cached_bin'."

tests/pytest/tool_version_test.py

@@ -334,6 +334,44 @@ def _run_hook(  # pragma: win32 no cover
     )
 
 
+def _run_concurrent_hooks(  # pragma: win32 no cover
+    count: int,
+    hook_name: str,
+    args: list[str],
+    *,
+    cwd: Path,
+    env: dict[str, str],
+) -> list[str]:
+    """Run `count` copies of a hook concurrently, on the same `a.tf`.
+
+    Every copy is started before any of them is waited on, so all
+    `count` copies genuinely overlap instead of running one after
+    another.
+
+    Returns:
+        Each process' merged stdout/stderr, in start order.
+    """
+    hook_path = HOOKS_DIR / hook_name
+    processes = [
+        subprocess.Popen(  # noqa: S603
+            (BASH, str(hook_path), *args, '--', 'a.tf'),
+            cwd=cwd,
+            env=env,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+            text=True,
+        )
+        for _ in range(count)
+    ]
+    outputs = [
+        proc.communicate(timeout=HOOK_TIMEOUT_SECONDS)[0] for proc in processes
+    ]
+    for output, proc in zip(outputs, processes, strict=True):
+        # 2 is tflint's own lint findings on the minimal fixture, not ours.
+        assert proc.returncode in {0, 2}, output
+    return outputs
+
+
 def test_cache_hit_uses_cached_binary(  # pragma: win32 no cover
     tmp_repo: Path,
     cache_dir: Path,
@@ -1015,3 +1053,52 @@ def test_real_download_on_cache_miss(  # pragma: win32 no cover
 
     # 2 is tflint's own lint findings on the minimal fixture, not ours.
     assert hook_run.returncode in {0, 2}, combined
+
+
+@pytest.mark.network
+def test_concurrent_cache_miss_is_race_free(  # pragma: win32 no cover
+    tmp_repo: Path,
+    cache_dir: Path,
+) -> None:
+    """Check N processes racing the same cache miss don't corrupt it.
+
+    Regression test for a race in `common::populate_tool_cache`:
+    before it staged each download in a private, per-process directory
+    and atomically published only the resulting binary, N processes
+    hitting the same uncached (tool, version) at once shared one
+    `curl`/`unzip` working directory - so one process' cleanup
+    (`rm "$PKG"`) could delete the archive out from under another's
+    still-running `unzip` ("cannot find or open ... .zip"), or `unzip`
+    could meet a binary a sibling had already extracted and block on
+    an interactive overwrite prompt, hanging (then failing) under
+    pre-commit's non-interactive stdin.
+    """
+    # 2 is the minimal N that can reproduce a race at all; every
+    # process beyond that adds real-network exposure (see this
+    # function's own `@pytest.mark.network`) without proving anything
+    # a race between 2 doesn't already prove.
+    outputs = _run_concurrent_hooks(
+        2,
+        'terraform_tflint.sh',
+        [f'--hook-config=--tool-version={PINNED_TFLINT_VERSION}'],
+        cwd=tmp_repo,
+        env=_hook_env(_pct_cache_env(cache_dir), os.environ['PATH']),
+    )
+
+    cached_bin = cache_dir / 'tflint' / PINNED_TFLINT_VERSION / 'tflint'
+    assert os.access(cached_bin, os.X_OK), outputs
+
+    version_check = subprocess.run(  # noqa: S603
+        (str(cached_bin), '--version'),
+        capture_output=True,
+        text=True,
+        check=False,
+        timeout=VERSION_CHECK_TIMEOUT_SECONDS,
+    )
+    assert version_check.returncode == 0, version_check.stderr
+    assert PINNED_TFLINT_VERSION in version_check.stdout
+
+    # Every staging dir is cleaned up whether its process won the race
+    # or lost it - none left behind regardless of outcome.
+    leftovers = list((cache_dir / 'tflint').glob(f'{PINNED_TFLINT_VERSION}.*'))
+    assert not leftovers, outputs