diff --git a/hooks/_common.sh b/hooks/_common.sh index fb9fd1036..b459b0b71 100644 --- a/hooks/_common.sh +++ b/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/.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'." diff --git a/tests/pytest/tool_version_test.py b/tests/pytest/tool_version_test.py index c091c9e9e..cb3de29e6 100644 --- a/tests/pytest/tool_version_test.py +++ b/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