fix(hooks): Make cache-miss download race-safe - #1012
Merged
MaxymVlasov merged 8 commits intoAug 20, 2026
Conversation
`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
- Shrink the function header to the same Globals/Arguments/Outputs shape every other function here uses; drop the standalone rationale paragraph (already covered by the previous commit message). - Trim inline comments to one line each, keep only the non-obvious ones (race-check-before-mv, atomic-rename, lost-race handling). - Rename `staging_dir` -> `tmp_dir`, matching this file's own `tmp_file` mktemp precedent (terraform_wrapper_module_for_each.sh). Assisted-by: Sisyphus:claude-sonnet-5 opencode
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## downloadable_and_version_controlled_hooks #1012 +/- ##
=============================================================================
+ Coverage 98.81% 98.87% +0.05%
=============================================================================
Files 11 11
Lines 762 800 +38
Branches 12 13 +1
=============================================================================
+ Hits 753 791 +38
Misses 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
MaxymVlasov
changed the title
MaxymVlasov
marked this pull request as ready for review
August 19, 2026 16:57
yermulnik
approved these changes
Aug 19, 2026
Co-authored-by: George Yermulnik (Georgii Iermulnik) <yz@yz.kiev.ua>
`common::populate_tool_cache` only checked whether `cached_bin` already existed right before the `mv` that publishes it - after the full download had already run. Check for it again up front too, so a process that starts after a sibling has already finished publishing skips the download entirely instead of redundantly repeating it. The pre-`mv` check stays: it's what stops a slower sibling's `mv` from silently replacing a binary another process may already be executing (`mv` doesn't refuse an existing destination, it just replaces it). Assisted-by: Sisyphus:claude-sonnet-5 opencode
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens tool-version caching in the shell hook shared logic to be safe under concurrent cache-miss downloads (multiple pre-commit processes racing the same (tool, version)), and adds a regression test that reproduces the original race with real network downloads.
Changes:
- Add
common::populate_tool_cacheto stage installs in a per-process temp directory and then publish into the shared cache. - Update
common::resolve_tool_pathto use the new cache-population helper instead of downloading directly into the shared cache directory. - Add a new network-marked pytest regression test that runs two concurrent hooks against an empty cache and asserts the cache entry is valid and staging dirs are cleaned.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
hooks/_common.sh |
Introduces a staging-and-publish cache population function and wires it into tool path resolution to avoid concurrent unzip/curl conflicts. |
tests/pytest/tool_version_test.py |
Adds concurrent hook runner helper and a network regression test that exercises the cache-miss race scenario. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
yermulnik
approved these changes
Aug 20, 2026
`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
yermulnik
approved these changes
Aug 20, 2026
MaxymVlasov
merged commit Aug 20, 2026
43bf759
into
downloadable_and_version_controlled_hooks
50 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Put an
xinto the box if that apply:Description of your changes
What
This section was generated by AI.
common::resolve_tool_path's cache-miss path downloaded/unzipped every pinned tool version directly into the shared, version-keyed cache directory. When multiple pre-commit hook processes raced the same uncached(tool, version)at once, they stepped on each other'scurl/unzipoutput: one process' cleanup could delete the archive out from under another's still-runningunzip(cannot find or open ... .zip), orunzipcould meet a binary a sibling already extracted and block on an interactive overwrite prompt - hanging under pre-commit's non-interactive stdin.common::populate_tool_cache: downloads/installs into a private per-process temp directory (mktemp -d), then atomically publishes the resulting binary into the cache via a plain-filemv. A process that loses the race discards its own redundant copy instead of corrupting the winner's.test_concurrent_cache_miss_is_race_free(network): 2 concurrent real downloads against an empty cache, asserting a clean single binary and no leftover temp dirs.Why
Fix race condition during concurrent (pre-commit usually proceeds with 4 bunches simultaneously), which end in over9999 errors like this:
How can we test changes
This section was generated by AI.
All pre-commit hooks pass locally:
shellcheck,shfmt,ruff check,ruff format,wemake-python-styleguide,mypy(py3.10/3.12/3.14).Assisted-by
Specific models used per commit are specified in the commit messages.