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

hooks/_common.sh

@@ -544,6 +544,78 @@ function common::detect_os_arch {
   export TARGETOS TARGETARCH
 }
 
+#######################################################################
+# Download and install one tool version into a private, per-process
+# temp directory, then atomically publish the resulting binary into
+# the shared cache - concurrency-safe against other processes
+# populating the same (tool, version) entry at the same time.
+# 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 installer to invoke
+#   env_var_name (string) env var the installer reads its version from
+#   cache_dir (string) final, shared cache dir for this (tool, version).
+#     Must already exist.
+#   cached_bin (string) expected absolute path to the resolved binary
+# Outputs:
+#   Returns 0 once `cached_bin` exists, ours or a race winner's.
+#   Returns 1 with an error message if the 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"
+
+  if [[ -x $cached_bin ]]; then
+    return 0
+  fi
+
+  local tmp_dir
+  tmp_dir=$(mktemp -d "${cache_dir}.XXXXXXXXXX") || {
+    common::colorify "red" "ERROR: Failed to create a temp directory for '$tool_name' version '$version'."
+    return 1
+  }
+
+  # Redirect the installer's own stdout to stderr: this is a plain
+  # function call, not a "$(...)" capture, so anything printed here
+  # would flow straight through to `common::resolve_tool_path`'s own
+  # stdout - the resolved path, captured via "$(...)" by every caller
+  # of *that* function - and installers like terraform.sh/tflint.sh
+  # call bare `unzip` (no `-q`), which prints "Archive: ... inflating:
+  # ..." to stdout by default.
+  if ! (
+    cd "$tmp_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 "$tmp_dir"
+    return 1
+  fi
+
+  # `ln` (hard link, no `-f`) fails with EEXIST instead of silently
+  # replacing an existing destination - unlike `mv`, which would
+  # clobber a binary a sibling process may already be running.
+  # `tmp_dir` is a sibling of `cache_dir` (both under the same parent),
+  # so this is guaranteed to stay on one filesystem.
+  if ! ln "$tmp_dir/$(basename "$cached_bin")" "$cached_bin" 2> /dev/null; then
+    rm -rf "$tmp_dir"
+    # Lost the race - the winner's copy is equally valid.
+    [[ -x $cached_bin ]] && return 0
+    common::colorify "red" "ERROR: Failed to update '$cached_bin' with '$tool_name' version '$version'."
+    return 1
+  fi
+
+  rm -rf "$tmp_dir"
+}
+
 #######################################################################
 # Resolve a specific version of a wrapped tool's binary, downloading
 # and caching it on demand if it isn't already cached.
@@ -639,7 +711,7 @@ function common::resolve_tool_path {
     fi
 
     common::colorify "green" \
-      "NOTE: The requested '$tool_name' version '$version' will be downloaded/used instead of whatever is on \$PATH."
+      "NOTE: The requested '$tool_name' version '$version' will be used instead of whatever is on \$PATH."
   fi
 
   #
@@ -682,20 +754,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

@@ -48,7 +48,7 @@
 # `common::colorify` writes to stderr, hence the stderr/stdout merge in
 # `_run_hook` below.
 DOWNLOAD_MSG = "Downloading '"  # hooks/_common.sh:662
-STRICT_OVERRIDE_MSG = 'downloaded/used instead of whatever is on $PATH'  # :629
+STRICT_OVERRIDE_MSG = 'used instead of whatever is on $PATH'  # :629
 PREFER_LOCAL_MSG = "'--tool-version-mode=prefer-local'"  # :622
 NO_INSTALLER_MSG = 'no installer found'  # :658
 MODE_INVALID_MSG = "'--tool-version-mode=prefer_local' is not a valid value"
@@ -334,6 +334,49 @@ 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
+    if not hook_path.is_file():  # pragma: no cover
+        # `hooks/` is not part of the wheel, only of the sdist, so a
+        # packaging regression must fail with a pointed message here
+        # instead of as a confusing assertion mismatch further down.
+        pytest.fail(f'Hook script not found: {hook_path}')
+    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 +1058,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