Skip to content

Commit 606858b

Browse files
committed
fix(hooks): publish cached tool via ln, not mv
`mv` silently replaces an existing destination, so a slower sibling process could still clobber a binary another process already published and might be executing (surfaced as "fork/exec ... (deleted): no such file or directory"). `ln` (hard link, no `-f`) fails with EEXIST instead of overwriting, so publishing is now atomically "first process wins, nobody else touches it" - the temp dir's own `rm -rf` at the end still finishes the move to the binary's only remaining name. - Correct a comment that attributed the stdout-capture contract to `populate_tool_cache` itself; it belongs to its caller `resolve_tool_path`. - Add the same `hooks/` existence guard `_run_hook` already has to its sibling `_run_concurrent_hooks`, for the same packaging- regression message instead of a confusing raw exit code. Assisted-by: Sisyphus:claude-sonnet-5 opencode
1 parent a812e80 commit 606858b

2 files changed

Lines changed: 18 additions & 17 deletions

File tree

hooks/_common.sh

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -583,12 +583,13 @@ function common::populate_tool_cache {
583583
return 1
584584
}
585585

586-
# Redirect the installer's own stdout to stderr: this function's stdout is
587-
# a contract (the resolved path, captured via "$(...)" by every caller),
588-
# and installers like terraform.sh/tflint.sh call bare `unzip` (no `-q`),
589-
# which prints "Archive: ... inflating: ..." to stdout by default -
590-
# harmless noise in a Docker build log, but it would otherwise corrupt
591-
# the path this function returns.
586+
# Redirect the installer's own stdout to stderr: this is a plain
587+
# function call, not a "$(...)" capture, so anything printed here
588+
# would flow straight through to `common::resolve_tool_path`'s own
589+
# stdout - the resolved path, captured via "$(...)" by every caller
590+
# of *that* function - and installers like terraform.sh/tflint.sh
591+
# call bare `unzip` (no `-q`), which prints "Archive: ... inflating:
592+
# ..." to stdout by default.
592593
if ! (
593594
cd "$tmp_dir" || exit 1
594595
export "$env_var_name=$version"
@@ -599,17 +600,12 @@ function common::populate_tool_cache {
599600
return 1
600601
fi
601602

602-
# A sibling may have published while we were downloading - `mv`
603-
# doesn't refuse an existing destination, it just replaces it, even
604-
# while something else is still executing it.
605-
if [[ -x $cached_bin ]]; then
606-
rm -rf "$tmp_dir"
607-
return 0
608-
fi
609-
610-
# Atomic `rename(2)`: both sides are plain files, so a concurrent
611-
# reader never sees a partial write.
612-
if ! mv "$tmp_dir/$(basename "$cached_bin")" "$cached_bin" 2> /dev/null; then
603+
# `ln` (hard link, no `-f`) fails with EEXIST instead of silently
604+
# replacing an existing destination - unlike `mv`, which would
605+
# clobber a binary a sibling process may already be running.
606+
# `tmp_dir` is a sibling of `cache_dir` (both under the same parent),
607+
# so this is guaranteed to stay on one filesystem.
608+
if ! ln "$tmp_dir/$(basename "$cached_bin")" "$cached_bin" 2> /dev/null; then
613609
rm -rf "$tmp_dir"
614610
# Lost the race - the winner's copy is equally valid.
615611
[[ -x $cached_bin ]] && return 0

tests/pytest/tool_version_test.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,11 @@ def _run_concurrent_hooks( # pragma: win32 no cover
352352
Each process' merged stdout/stderr, in start order.
353353
"""
354354
hook_path = HOOKS_DIR / hook_name
355+
if not hook_path.is_file(): # pragma: no cover
356+
# `hooks/` is not part of the wheel, only of the sdist, so a
357+
# packaging regression must fail with a pointed message here
358+
# instead of as a confusing assertion mismatch further down.
359+
pytest.fail(f'Hook script not found: {hook_path}')
355360
processes = [
356361
subprocess.Popen( # noqa: S603
357362
(BASH, str(hook_path), *args, '--', 'a.tf'),

0 commit comments

Comments
 (0)