From aa10436a235e07f108038dcdfeb14ac29171b0a4 Mon Sep 17 00:00:00 2001 From: Josh Liebow-Feeser Date: Mon, 24 Aug 2026 23:02:07 +0000 Subject: [PATCH] [ci] Make pre-push checks safe to run in parallel Fresh runners can start several cargo-zerocopy processes before rustup has installed the pinned nightly toolchain. Rustup shares download state between those processes, so concurrent installation can fail when one process moves another process's partial file. Install nightly before starting the checks in parallel. Preserve the hook protocol on stdin, scope automatic installation to the bootstrap command, and wait for every child. An early failure no longer discards diagnostics from checks that are still running. Also snapshot every first-party Cargo.lock before running the nominally read-only checks and reject any change. A hermetic fake-repository test covers bootstrap ordering, stdin preservation, lockfile accounting, subdirectory invocation, and child failures. *Authored by an agent, posting via joshlf's account* gherrit-pr-id: Gdqabzhpevkhxg3fzk6bvbtpws7nk2vwv --- ci/check_actions.sh | 5 + githooks/pre-push | 161 +++++++++++++++--- githooks/test_pre_push.py | 337 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 477 insertions(+), 26 deletions(-) create mode 100755 githooks/test_pre_push.py diff --git a/ci/check_actions.sh b/ci/check_actions.sh index 12e56d4f67..89296c2656 100755 --- a/ci/check_actions.sh +++ b/ci/check_actions.sh @@ -21,6 +21,11 @@ if [ ! -x "$HOME/.cargo/bin/action-validator" ]; then fi export PATH="$HOME/.cargo/bin:$PATH" +# The pre-push hook coordinates concurrent repository checks and is itself +# production code. Exercise it with fake check scripts so this validation does +# not recursively run the real hook or depend on installed Rust toolchains. +python3 githooks/test_pre_push.py + # Files to exclude from validation (e.g., because they are not Actions/Workflows) # Use relative paths matching `find .github` output EXCLUDE_FILES=( diff --git a/githooks/pre-push b/githooks/pre-push index 2b70c287ed..886dad249b 100755 --- a/githooks/pre-push +++ b/githooks/pre-push @@ -10,33 +10,122 @@ set -eo pipefail echo "Running pre-push git hook: $0" +REPO_ROOT="$(git rev-parse --show-toplevel)" +cd "$REPO_ROOT" + +# These checks assert properties of tracked source; none should rewrite a +# first-party workspace lockfile as a side effect. Discover the inventory so a +# newly added workspace is protected automatically. Vendored packages and test +# fixtures own independent lockfiles which these repository checks never use, +# so exclude those source snapshots from this workspace-level contract. +LOCKFILES=() +while IFS= read -r -d '' lockfile; do + case "$lockfile" in + */vendor/*|*/tests/fixtures/*) continue ;; + esac + LOCKFILES+=("$lockfile") +done < <(git ls-files -z -- '*Cargo.lock') + +if [[ ${#LOCKFILES[@]} -eq 0 ]]; then + echo "No first-party Cargo.lock files found; refusing to run unguarded" >&2 + exit 1 +fi + +lockfile_hash() { + local lockfile="$1" + if [[ -f "$lockfile" ]]; then + # Unlike sha256sum, `git hash-object` is available on both Linux and + # macOS wherever this Git hook can run. Bypass clean filters so this + # snapshot represents the bytes that a check could mutate on disk. + git hash-object --no-filters -- "$lockfile" + else + # Preserve a pre-existing deletion. If a check recreates the file, the + # post-check value will still differ from this marker. + echo '' + fi +} + +LOCKFILE_HASHES_BEFORE=() +for lockfile in "${LOCKFILES[@]}"; do + LOCKFILE_HASHES_BEFORE+=("$(lockfile_hash "$lockfile")") +done + +# check_fmt.sh uses cargo-zerocopy's pinned nightly toolchain. On a fresh +# runner, it previously reached rustup concurrently with other checks. Rustup +# shares download and rollback paths across toolchains, so one installation +# could remove another installation's partial file. Bootstrap nightly before +# retaining parallelism between the checks themselves. +# +# Keep this list coordinated with the backgrounded scripts below. If another +# check starts using a different cargo-zerocopy descriptor, initialize it here +# before that check may run in parallel. +CHECKS_FAILED=0 +TOOLCHAINS_READY=1 +bootstrap_toolchain() { + local descriptor="$1" + local status + # Git sends its ref-update protocol to pre-push hooks on stdin. Bootstrap + # is deliberately noninteractive, and neither cargo-zerocopy nor rustup may + # consume input needed by a chained git-lfs or GHerrit hook. + if CARGO_ZEROCOPY_AUTO_INSTALL_TOOLCHAIN=1 \ + ./zerocopy/cargo.sh "+$descriptor" --version \ + /dev/null; then + return + else + status=$? + fi + echo "zerocopy/cargo.sh +$descriptor --version failed with status $status" \ + >&2 + CHECKS_FAILED=1 + TOOLCHAINS_READY=0 +} + +bootstrap_toolchain nightly + # Forego redirecting stdout to /dev/null on check_fmt.sh because the output from # `cargo fmt` is useful (and the good stuff is not delivered by stderr). # -# Background all jobs and wait for them so they can run in parallel. -./ci/check_actions.sh & ACTIONS_PID=$! -./ci/check_fmt.sh & FMT_PID=$! -./ci/check_job_dependencies.sh >/dev/null & JOB_DEPS_PID=$! -./zerocopy/ci/check_all_toolchains_tested.sh >/dev/null & TOOLCHAINS_PID=$! -./zerocopy/ci/check_readme.sh >/dev/null & README_PID=$! -./zerocopy/ci/check_stale_stderr.sh >/dev/null & STALE_STDERR_PID=$! -./zerocopy/ci/check_versions.sh >/dev/null & VERSIONS_PID=$! -./zerocopy/ci/check_msrv_is_minimal.sh >/dev/null & MSRV_PID=$! - -# `wait ` exits with the same status code as the job it's waiting for. -# Since we `set -e` above, this will have the effect of causing the entire -# script to exit with a non-zero status code if any of these jobs does the same. -# Note that, while `wait` (with no PID argument) waits for all backgrounded -# jobs, it exits with code 0 even if one of the backgrounded jobs does not, so -# we can't use it here. -wait $ACTIONS_PID -wait $FMT_PID -wait $TOOLCHAINS_PID -wait $JOB_DEPS_PID -wait $README_PID -wait $STALE_STDERR_PID -wait $VERSIONS_PID -wait $MSRV_PID +# Background all jobs and wait for them so they can run in parallel. Do not +# launch them after a bootstrap failure: they could otherwise race to repair +# the same incomplete toolchain state. Static script-inventory and lockfile +# checks below still run, including when bootstrap fails. +wait_for_check() { + local name="$1" + local pid="$2" + local status + if wait "$pid"; then + return + else + status=$? + fi + echo "$name failed with status $status" >&2 + CHECKS_FAILED=1 +} + +if [[ "$TOOLCHAINS_READY" -eq 1 ]]; then + ./ci/check_actions.sh & ACTIONS_PID=$! + ./ci/check_fmt.sh & FMT_PID=$! + ./ci/check_job_dependencies.sh >/dev/null & JOB_DEPS_PID=$! + ./zerocopy/ci/check_all_toolchains_tested.sh >/dev/null & TOOLCHAINS_PID=$! + ./zerocopy/ci/check_readme.sh >/dev/null & README_PID=$! + ./zerocopy/ci/check_stale_stderr.sh >/dev/null & STALE_STDERR_PID=$! + ./zerocopy/ci/check_versions.sh >/dev/null & VERSIONS_PID=$! + ./zerocopy/ci/check_msrv_is_minimal.sh >/dev/null & MSRV_PID=$! + + # A bare `wait` loses individual failures. Sequential bare waits under + # `set -e` abandon the remaining children after the first failure. Record + # each status explicitly so every child is reaped and every useful + # diagnostic has a chance to finish. + wait_for_check "ci/check_actions.sh" "$ACTIONS_PID" + wait_for_check "ci/check_fmt.sh" "$FMT_PID" + wait_for_check "ci/check_job_dependencies.sh" "$JOB_DEPS_PID" + wait_for_check \ + "zerocopy/ci/check_all_toolchains_tested.sh" "$TOOLCHAINS_PID" + wait_for_check "zerocopy/ci/check_readme.sh" "$README_PID" + wait_for_check "zerocopy/ci/check_stale_stderr.sh" "$STALE_STDERR_PID" + wait_for_check "zerocopy/ci/check_versions.sh" "$VERSIONS_PID" + wait_for_check "zerocopy/ci/check_msrv_is_minimal.sh" "$MSRV_PID" +fi # Ensure that this script calls all scripts in `ci/*` and `zerocopy/ci/*`. This # isn't a foolproof check since it just checks for the string in this script @@ -50,13 +139,33 @@ wait $MSRV_PID shopt -s extglob GLOBIGNORE="./*/@(release_crate_version|check_todo|release_anneal_version).sh" # We don't want to run these for f in ./ci/*; do - grep "$f" githooks/pre-push >/dev/null || { echo "$f not called from githooks/pre-push" >&2 ; exit 1; } + if ! grep "$f" githooks/pre-push >/dev/null; then + echo "$f not called from githooks/pre-push" >&2 + CHECKS_FAILED=1 + fi done # We don't want to run release_crate_version here, and zerocopy/ci/check_fmt.sh # is called by ci/check_fmt.sh above rather than directly. GLOBIGNORE="./zerocopy/ci/@(release_crate_version|check_fmt).sh" for f in ./zerocopy/ci/*; do - grep "$f" githooks/pre-push >/dev/null || { echo "$f not called from githooks/pre-push" >&2 ; exit 1; } + if ! grep "$f" githooks/pre-push >/dev/null; then + echo "$f not called from githooks/pre-push" >&2 + CHECKS_FAILED=1 + fi done unset GLOBIGNORE shopt -u extglob + +# Keep this comparison at the end of the hook so any future synchronous check +# added after the parallel fan-out remains inside the same mutation guard. +for index in "${!LOCKFILES[@]}"; do + lockfile="${LOCKFILES[$index]}" + hash_after="$(lockfile_hash "$lockfile")" + if [[ "$hash_after" != "${LOCKFILE_HASHES_BEFORE[$index]}" ]]; then + echo "$lockfile was modified by a nominally read-only pre-push check" \ + >&2 + CHECKS_FAILED=1 + fi +done + +exit "$CHECKS_FAILED" diff --git a/githooks/test_pre_push.py b/githooks/test_pre_push.py new file mode 100755 index 0000000000..f26fc44512 --- /dev/null +++ b/githooks/test_pre_push.py @@ -0,0 +1,337 @@ +#!/usr/bin/env python3 +# +# Copyright 2026 The Fuchsia Authors +# +# Licensed under a BSD-style license , Apache License, Version 2.0 +# , or the MIT +# license , at your option. +# This file may not be copied, modified, or distributed except according to +# those terms. + +"""Regression tests for pre-push bootstrap, children, and lockfile checks.""" + +import os +from pathlib import Path +import shutil +import subprocess +import tempfile +import unittest + + +_ROOT = Path(__file__).resolve().parents[1] +_HOOK = _ROOT / "githooks" / "pre-push" +_LOCKFILES = ( + "anneal/Cargo.lock", + "anneal/v1/Cargo.lock", + "exocrate/Cargo.lock", + "tools/Cargo.lock", + "zerocopy/Cargo.lock", +) +_CHECKS = ( + "ci/check_actions.sh", + "ci/check_fmt.sh", + "ci/check_job_dependencies.sh", + "zerocopy/ci/check_all_toolchains_tested.sh", + "zerocopy/ci/check_readme.sh", + "zerocopy/ci/check_stale_stderr.sh", + "zerocopy/ci/check_versions.sh", + "zerocopy/ci/check_msrv_is_minimal.sh", +) +_EXEMPT_CHECKS = ( + "ci/check_todo.sh", + "ci/release_anneal_version.sh", + "zerocopy/ci/check_fmt.sh", + "zerocopy/ci/release_crate_version.sh", +) + +_CHECK_STUB = """\ +#!/usr/bin/env bash +set -eu +check=${0#./} +if [[ -n "${CARGO_ZEROCOPY_AUTO_INSTALL_TOOLCHAIN:-}" ]]; then + echo "bootstrap auto-install setting leaked into $check" >&2 + exit 24 +fi +if [[ "$check" == "${MUTATING_CHECK:-}" ]]; then + echo mutation >> "$MUTATE_LOCKFILE" +fi +if [[ "$check" == "${DELETING_CHECK:-}" ]]; then + rm -f "$DELETE_LOCKFILE" +fi +if [[ "$check" == "${SLOW_CHECK:-}" ]]; then + sleep 0.2 +fi +touch "$MARKER_DIR/${check//\\//_}" +if [[ "$check" == "${FAIL_CHECK:-}" ]]; then + exit 23 +fi +""" + +_CARGO_STUB = """\ +#!/usr/bin/env bash +set -eu +invocation="$*" +if [[ "${CARGO_ZEROCOPY_AUTO_INSTALL_TOOLCHAIN:-}" != 1 ]]; then + echo "bootstrap did not enable noninteractive auto-install" >&2 + exit 24 +fi +if [[ "${PROBE_BOOTSTRAP_STDIN:-}" == 1 ]] && IFS= read -r line; then + printf '%s\\n' "$line" > "$MARKER_DIR/bootstrap_stdin" + echo "bootstrap consumed pre-push protocol input" >&2 + exit 25 +fi +if ! mkdir "$MARKER_DIR/cargo_bootstrap_lock"; then + echo "concurrent cargo bootstrap" >&2 + exit 42 +fi +trap 'rmdir "$MARKER_DIR/cargo_bootstrap_lock"' EXIT +printf '%s\\n' "$invocation" >> "$MARKER_DIR/cargo_invocations" +if [[ "$invocation" == "${MUTATING_CARGO_INVOCATION:-}" ]]; then + echo mutation >> "$MUTATE_LOCKFILE" +fi +sleep 0.05 +if [[ "$invocation" == "${FAIL_CARGO_INVOCATION:-}" ]]; then + exit 23 +fi +""" + + +class FakeRepository: + """A minimal repository whose checks expose hook coordination bugs.""" + + def __init__(self): + self._temporary_directory = tempfile.TemporaryDirectory() + self.path = Path(self._temporary_directory.name) + self.markers = self.path / "markers" + + (self.path / "githooks").mkdir() + self.markers.mkdir() + shutil.copy2(_HOOK, self.path / "githooks" / "pre-push") + + for lockfile in _LOCKFILES: + path = self.path / lockfile + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(f"original {lockfile}\n", encoding="utf-8") + + # Checked-in dependencies and fixtures have independent lockfiles and + # are deliberately outside the first-party workspace contract. + for lockfile in ( + "zerocopy/vendor/example/Cargo.lock", + "anneal/v1/tests/fixtures/example/Cargo.lock", + ): + path = self.path / lockfile + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("source snapshot\n", encoding="utf-8") + + for check in _CHECKS: + path = self.path / check + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(_CHECK_STUB, encoding="utf-8") + path.chmod(0o755) + + # These scripts are deliberately inventoried but exempt from direct + # execution. Their presence tests the hook's exclusion contract. + for check in _EXEMPT_CHECKS: + path = self.path / check + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("#!/usr/bin/env bash\nexit 99\n", encoding="utf-8") + path.chmod(0o755) + + cargo = self.path / "zerocopy/cargo.sh" + cargo.write_text(_CARGO_STUB, encoding="utf-8") + cargo.chmod(0o755) + + subprocess.run( + ["git", "init", "--quiet"], cwd=self.path, check=True + ) + subprocess.run(["git", "add", "."], cwd=self.path, check=True) + + def close(self): + self._temporary_directory.cleanup() + + def run_hook(self, start_directory=".", **environment): + child_environment = os.environ.copy() + child_environment.pop( + "CARGO_ZEROCOPY_AUTO_INSTALL_TOOLCHAIN", None + ) + child_environment.update(environment) + child_environment["MARKER_DIR"] = str(self.markers) + return subprocess.run( + ["bash", str(self.path / "githooks" / "pre-push")], + cwd=self.path / start_directory, + env=child_environment, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=False, + ) + + def run_hook_then_capture_stdin(self, hook_input, **environment): + child_environment = os.environ.copy() + child_environment.pop( + "CARGO_ZEROCOPY_AUTO_INSTALL_TOOLCHAIN", None + ) + child_environment.update(environment) + child_environment["MARKER_DIR"] = str(self.markers) + downstream_input = self.markers / "downstream_stdin" + return subprocess.run( + [ + "bash", + "-c", + 'bash "$1"; status=$?; cat > "$2"; exit "$status"', + "pre-push-test-wrapper", + str(self.path / "githooks" / "pre-push"), + str(downstream_input), + ], + cwd=self.path, + env=child_environment, + input=hook_input, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=False, + ) + + +class PrePushTest(unittest.TestCase): + def setUp(self): + self.repository = FakeRepository() + + def tearDown(self): + self.repository.close() + + def cargo_invocations(self): + return ( + self.repository.markers / "cargo_invocations" + ).read_text(encoding="utf-8").splitlines() + + def marker_for(self, check): + return self.repository.markers / check.replace("/", "_") + + def test_bootstraps_serially_and_runs_every_check(self): + result = self.repository.run_hook() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual( + self.cargo_invocations(), + ["+nightly --version"], + ) + for check in _CHECKS: + self.assertTrue(self.marker_for(check).is_file(), check) + + def test_bootstrap_preserves_protocol_input_for_later_hooks(self): + protocol = ( + "refs/heads/main 1111111111111111111111111111111111111111 " + "refs/heads/main 2222222222222222222222222222222222222222\n" + "refs/heads/topic 3333333333333333333333333333333333333333 " + "refs/heads/topic 4444444444444444444444444444444444444444\n" + ) + result = self.repository.run_hook_then_capture_stdin( + protocol, PROBE_BOOTSTRAP_STDIN="1" + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertFalse( + (self.repository.markers / "bootstrap_stdin").exists() + ) + self.assertEqual( + (self.repository.markers / "downstream_stdin").read_text( + encoding="utf-8" + ), + protocol, + ) + + def test_can_run_from_a_repository_subdirectory(self): + result = self.repository.run_hook(start_directory="zerocopy") + self.assertEqual(result.returncode, 0, result.stderr) + + def test_bootstrap_failure_still_checks_lockfiles(self): + lockfile = _LOCKFILES[-1] + result = self.repository.run_hook( + FAIL_CARGO_INVOCATION="+nightly --version", + MUTATING_CARGO_INVOCATION="+nightly --version", + MUTATE_LOCKFILE=str(self.repository.path / lockfile), + ) + self.assertNotEqual(result.returncode, 0) + self.assertIn( + "zerocopy/cargo.sh +nightly --version failed with status 23", + result.stderr, + ) + self.assertIn(f"{lockfile} was modified", result.stderr) + self.assertEqual( + self.cargo_invocations(), + ["+nightly --version"], + ) + for check in _CHECKS: + self.assertFalse(self.marker_for(check).exists(), check) + + def test_each_first_party_lockfile_is_guarded(self): + for operation in ("mutate", "delete"): + for lockfile in _LOCKFILES: + with self.subTest(operation=operation, lockfile=lockfile): + self.repository.close() + self.repository = FakeRepository() + if operation == "mutate": + environment = { + "MUTATING_CHECK": "ci/check_actions.sh", + "MUTATE_LOCKFILE": str( + self.repository.path / lockfile + ), + } + else: + environment = { + "DELETING_CHECK": "ci/check_actions.sh", + "DELETE_LOCKFILE": str( + self.repository.path / lockfile + ), + } + result = self.repository.run_hook(**environment) + self.assertNotEqual(result.returncode, 0) + self.assertIn( + f"{lockfile} was modified by a nominally read-only", + result.stderr, + ) + + def test_allows_preexisting_dirty_or_missing_lockfile(self): + lockfile = self.repository.path / _LOCKFILES[0] + lockfile.write_text("preexisting local edit\n", encoding="utf-8") + result = self.repository.run_hook() + self.assertEqual(result.returncode, 0, result.stderr) + + self.repository.close() + self.repository = FakeRepository() + (self.repository.path / _LOCKFILES[0]).unlink() + result = self.repository.run_hook() + self.assertEqual(result.returncode, 0, result.stderr) + + def test_ignores_source_snapshot_lockfiles(self): + lockfile = self.repository.path / "zerocopy/vendor/example/Cargo.lock" + result = self.repository.run_hook( + MUTATING_CHECK="ci/check_actions.sh", + MUTATE_LOCKFILE=str(lockfile), + ) + self.assertEqual(result.returncode, 0, result.stderr) + + def test_waits_for_every_child_after_an_early_failure(self): + slow_check = "zerocopy/ci/check_msrv_is_minimal.sh" + lockfile = _LOCKFILES[-1] + result = self.repository.run_hook( + FAIL_CHECK="ci/check_actions.sh", + SLOW_CHECK=slow_check, + MUTATING_CHECK=slow_check, + MUTATE_LOCKFILE=str(self.repository.path / lockfile), + ) + self.assertNotEqual(result.returncode, 0) + self.assertIn( + "ci/check_actions.sh failed with status 23", result.stderr + ) + self.assertIn(f"{lockfile} was modified", result.stderr) + self.assertTrue(self.marker_for(slow_check).is_file()) + + def test_reports_a_non_first_child_failure(self): + failed_check = "zerocopy/ci/check_stale_stderr.sh" + result = self.repository.run_hook(FAIL_CHECK=failed_check) + self.assertNotEqual(result.returncode, 0) + self.assertIn(f"{failed_check} failed with status 23", result.stderr) + + +if __name__ == "__main__": + unittest.main()