diff --git a/.github/actions/install-linux-engine-build-dependencies/action.yml b/.github/actions/install-linux-engine-build-dependencies/action.yml new file mode 100644 index 000000000..bc918e738 --- /dev/null +++ b/.github/actions/install-linux-engine-build-dependencies/action.yml @@ -0,0 +1,28 @@ +name: Install Linux engine build dependencies +description: >- + apt-installs packages under a wall-clock bound, escaping to a second mirror + rather than waiting out a slow one. + +# Three workflows need the same apt set and each used to carry its own unbounded +# `apt-get update && apt-get install`. A slow mirror turned that step from ~15s +# into 811s in one wheel run and 1400s in one test run on the same day, so the +# bound lives here rather than in three copies that drift. +# +# Composite-action steps cannot declare `timeout-minutes`, so the ceiling is the +# script's own per-command bound; callers add `timeout-minutes` on the `uses:` +# step as the native backstop, and `check-bounded-apt-install` fails the build +# if one forgets. + +inputs: + packages: + description: Whitespace-separated apt packages to install. + required: true + +runs: + using: composite + steps: + - name: Install system dependencies with a bounded retry + shell: bash + run: >- + "${{ github.action_path }}/install-system-dependencies-with-bounded-retry.sh" + ${{ inputs.packages }} diff --git a/.github/actions/install-linux-engine-build-dependencies/install-system-dependencies-with-bounded-retry.sh b/.github/actions/install-linux-engine-build-dependencies/install-system-dependencies-with-bounded-retry.sh new file mode 100755 index 000000000..3225c4a39 --- /dev/null +++ b/.github/actions/install-linux-engine-build-dependencies/install-system-dependencies-with-bounded-retry.sh @@ -0,0 +1,168 @@ +#!/usr/bin/env bash +# +# Install apt packages under a wall-clock bound, escaping to a second mirror +# rather than waiting out a slow one. +# +# The mode this exists for is a *slow* mirror, not a stalled one. A measured run +# fetched 35.6 MB at 48 kB/s over 12m17s while every request made forward +# progress, so neither of apt's own guards engages: `Acquire::Retries` needs a +# failure to retry and `Acquire::*::Timeout` bounds inactivity, and there was +# neither. A wall-clock bound is the only thing that detects that mode, and a +# different mirror is the only thing that recovers from it — a second try +# against the same host just spends the budget again at the same 48 kB/s. +# +# That is also why there is no attempt-count dial. `Acquire::Retries` already +# covers transient per-file errors inside a single run, so retrying the primary +# would be redundant: two attempts, one per mirror. +# +# 120s bounds one apt command and 60s the dpkg repair, and each carries a 10s +# SIGKILL grace on top. Two mirrors × (update + install) plus one repair is +# therefore 4×130 + 70 = 590s worst case — inside the caller's +# `timeout-minutes`, which is what lets this script report the failure itself +# instead of being killed mid-sentence. 120s is also ~8× the median step and +# ~1.8× the slowest *successful* update on record, so a merely-mediocre mirror +# still finishes on the primary. +# +# Env (all optional; the last five exist so the gate tests can drive this +# without root, apt, or a network): +# STREAMLIB_APT_ATTEMPT_TIMEOUT_SECONDS bound on one apt command (default 120) +# STREAMLIB_APT_FALLBACK_MIRROR_URL mirror used once the primary blows the bound +# STREAMLIB_APT_PRIVILEGE_PREFIX how to become root (default `sudo`; may be empty) +# STREAMLIB_APT_GET_COMMAND the apt-get to invoke +# STREAMLIB_APT_MIRROR_SWITCH_COMMAND the command that repoints apt at the fallback +# STREAMLIB_DPKG_REPAIR_COMMAND the command that finishes an interrupted dpkg +# STREAMLIB_APT_KILL_AFTER_SECONDS SIGKILL grace after the signal (default 10) + +set -euo pipefail + +# `timeout` reports 124 when the command honoured the signal and exited, and 137 +# (128 + SIGKILL) when `--kill-after` had to escalate. Both mean the bound fired. +readonly TIMEOUT_EXIT_STATUS=124 +readonly SIGKILL_ESCALATION_EXIT_STATUS=137 +readonly DPKG_REPAIR_TIMEOUT_SECONDS=60 + +attempt_timeout_seconds="${STREAMLIB_APT_ATTEMPT_TIMEOUT_SECONDS:-120}" +fallback_mirror_url="${STREAMLIB_APT_FALLBACK_MIRROR_URL:-http://archive.ubuntu.com/ubuntu/}" +apt_privilege_prefix="${STREAMLIB_APT_PRIVILEGE_PREFIX-sudo}" +apt_get_command="${STREAMLIB_APT_GET_COMMAND:-apt-get}" +mirror_switch_command="${STREAMLIB_APT_MIRROR_SWITCH_COMMAND:-}" +dpkg_repair_command="${STREAMLIB_DPKG_REPAIR_COMMAND:-dpkg --configure -a}" +kill_after_seconds="${STREAMLIB_APT_KILL_AFTER_SECONDS:-10}" + +if [ "$#" -eq 0 ]; then + echo "usage: ${0##*/} ..." >&2 + exit 2 +fi + +requested_packages=("$@") + +# Retries covers the transient per-file failure; the Timeout pair covers a +# connection that goes silent. apt keys Timeout per scheme and the runner's +# sources are a mix — Ubuntu over http, several vendor repos over https — so +# setting only one of them leaves half the fetch unbounded. Neither reaches a +# mirror that is merely slow; that is what the wall-clock bound below is for. +apt_acquire_options=( + -o Acquire::Retries=3 + -o Acquire::http::Timeout=30 + -o Acquire::https::Timeout=30 +) + +# `sudo timeout ...`, never `timeout sudo ...`: with sudo on the outside the +# signal lands on sudo, which relays SIGINT but cannot be made to pass SIGKILL +# on, so a `--kill-after` would leave an orphaned root apt-get holding +# /var/lib/dpkg/lock-frontend and the fallback attempt would fail on the lock. +# Running timeout as root puts it in the parent slot of apt-get itself. +# +# SIGINT first, because apt unwinds on it and leaves +# /var/cache/apt/archives/partial intact for the next attempt to resume from; +# SIGKILL only if it refuses to go — which is why the escalation status counts +# as a timeout too, in `describe_attempt_failure` below. +run_one_bounded_apt_attempt() { + local attempt_label="$1" + local exit_status=0 + + echo "==> apt attempt: ${attempt_label} (each command bounded to ${attempt_timeout_seconds}s)" + + # Unquoted on purpose: each may be several words, or empty. + # shellcheck disable=SC2086 + $apt_privilege_prefix timeout --signal=INT --kill-after="${kill_after_seconds}s" "${attempt_timeout_seconds}s" \ + $apt_get_command update "${apt_acquire_options[@]}" || exit_status=$? + + if [ "$exit_status" -ne 0 ]; then + return "$exit_status" + fi + + # shellcheck disable=SC2086 + $apt_privilege_prefix timeout --signal=INT --kill-after="${kill_after_seconds}s" "${attempt_timeout_seconds}s" \ + $apt_get_command install -y "${apt_acquire_options[@]}" "${requested_packages[@]}" \ + || exit_status=$? + + return "$exit_status" +} + +# A missing package and a stalled mirror are different diagnoses, and reporting +# the second for the first sends the reader hunting a network problem that is +# not there — the likeliest cause of a deterministic failure here is a +# version-pinned package name that a runner-image roll retired. +describe_attempt_failure() { + if [ "$1" -eq "$TIMEOUT_EXIT_STATUS" ] || [ "$1" -eq "$SIGKILL_ESCALATION_EXIT_STATUS" ]; then + echo "did not finish inside the ${attempt_timeout_seconds}s bound" + else + echo "failed with apt exit status $1" + fi +} + +switch_apt_to_fallback_mirror() { + if [ -n "$mirror_switch_command" ]; then + # shellcheck disable=SC2086 + $mirror_switch_command "$fallback_mirror_url" + return + fi + + # GitHub's Ubuntu images point apt at a mirrorlist file rather than at a host + # (`URIs: mirror+file:/etc/apt/apt-mirrors.txt`), so the whole switch is one + # file — rewriting sources.list would not move anything. + if [ -f /etc/apt/apt-mirrors.txt ]; then + # shellcheck disable=SC2086 + printf '%s\n' "$fallback_mirror_url" \ + | $apt_privilege_prefix tee /etc/apt/apt-mirrors.txt >/dev/null + echo "==> repointed /etc/apt/apt-mirrors.txt at ${fallback_mirror_url}" + else + echo "==> no /etc/apt/apt-mirrors.txt to repoint; the retry re-runs against the same mirror" >&2 + fi +} + +# The bound can fire while apt is unpacking rather than downloading, and the +# SIGINT reaches dpkg too. apt then refuses every later install with "dpkg was +# interrupted, you must manually run dpkg --configure -a" — which would make the +# fallback attempt fail deterministically and turn the escape hatch into a no-op. +# +# Bounded like everything else, and for the same reason: this runs right after a +# root apt-get was signalled, so it is exactly when /var/lib/dpkg/lock-frontend +# is most likely to still be held. An unbounded repair here would blow the +# worst case the header states and hand the kill back to `timeout-minutes`. +# 60s because it is local work with no network in it. +finish_any_interrupted_dpkg() { + # shellcheck disable=SC2086 + $apt_privilege_prefix timeout --signal=INT --kill-after="${kill_after_seconds}s" "${DPKG_REPAIR_TIMEOUT_SECONDS}s" \ + $dpkg_repair_command || true +} + +primary_status=0 +run_one_bounded_apt_attempt "primary mirror" || primary_status=$? +if [ "$primary_status" -eq 0 ]; then + exit 0 +fi + +echo "==> primary mirror $(describe_attempt_failure "$primary_status"); escaping to ${fallback_mirror_url}" >&2 +finish_any_interrupted_dpkg +switch_apt_to_fallback_mirror + +fallback_status=0 +run_one_bounded_apt_attempt "fallback mirror" || fallback_status=$? +if [ "$fallback_status" -eq 0 ]; then + exit 0 +fi + +echo "==> fallback mirror $(describe_attempt_failure "$fallback_status"); giving up" >&2 +exit 1 diff --git a/.github/workflows/python-wheel.yml b/.github/workflows/python-wheel.yml index 1daa702d2..58b1f6564 100644 --- a/.github/workflows/python-wheel.yml +++ b/.github/workflows/python-wheel.yml @@ -47,18 +47,21 @@ jobs: # would be readable by it. No workspace member has a git dependency. persist-credentials: false + # Same minimal engine build set as test.yml: the engine's build.rs + # compiles GLSL with glslc, the Linux dep set links Vulkan, and iceoryx2's + # PAL crates run bindgen. `python3-dev` is the extra — the wheel crate's + # unit tests link libpython (see the step below). - name: Install system dependencies - # Same minimal engine build set as test.yml: the engine's build.rs - # compiles GLSL with glslc, and the Linux - # dep set links Vulkan. - run: | - sudo apt-get update - sudo apt-get install -y \ - pkg-config \ - protobuf-compiler \ - libclang-dev \ - libvulkan-dev \ - glslc \ + timeout-minutes: 12 + uses: ./.github/actions/install-linux-engine-build-dependencies + with: + packages: >- + pkg-config + protobuf-compiler + libclang1-18 + libclang-common-18-dev + libvulkan-dev + glslc python3-dev - name: Install Rust diff --git a/.github/workflows/schemas.yml b/.github/workflows/schemas.yml index 49547f293..b4453c95f 100644 --- a/.github/workflows/schemas.yml +++ b/.github/workflows/schemas.yml @@ -21,17 +21,22 @@ jobs: steps: - uses: actions/checkout@v4 + # generate_openapi links the api-server, which pulls in streamlib-engine; + # its build.rs compiles GLSL with glslc and links Vulkan, its dependency + # iceoryx2-pal-posix runs bindgen — which needs libclang and clang's + # builtin headers — and the audio path links opus. Mirrors test.yml's + # Linux engine dep set. - name: Install system dependencies - # generate_openapi links the api-server, which pulls in streamlib-engine; - # its build.rs compiles GLSL with glslc and links Vulkan, and the audio path - # links opus. Mirrors test.yml's Linux engine dep set. - run: | - sudo apt-get update - sudo apt-get install -y \ - pkg-config \ - protobuf-compiler \ - libvulkan-dev \ - glslc \ + timeout-minutes: 12 + uses: ./.github/actions/install-linux-engine-build-dependencies + with: + packages: >- + pkg-config + protobuf-compiler + libclang1-18 + libclang-common-18-dev + libvulkan-dev + glslc libopus-dev - name: Install Rust diff --git a/.github/workflows/source-gates.yml b/.github/workflows/source-gates.yml index 072003eca..bb36e730f 100644 --- a/.github/workflows/source-gates.yml +++ b/.github/workflows/source-gates.yml @@ -5,7 +5,7 @@ name: Source Gates # These ran as seven separate workflows. Each paid ~26s compiling the xtask # binary and ~13s compiling its test binary to do a few seconds of file walking, # and all seven shared one cache key — so they raced, and six of every seven -# cache saves were discarded. One job compiles xtask once and runs all nine +# cache saves were discarded. One job compiles xtask once and runs all ten # gates plus the whole fixture suite. # # `check-all-source-gates` runs every gate before reporting, so one job still diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8196e4270..efbbdf36a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -42,17 +42,25 @@ jobs: # be readable by it. No workspace member has a git dependency. persist-credentials: false + # `-p streamlib` pulls streamlib-engine, whose build.rs compiles GLSL + # compute shaders with glslc; the Linux engine dep set links Vulkan, and + # iceoryx2's PAL crates run bindgen. Minimal subset — no GPU/codec + # backends. + # + # `timeout-minutes` is the native backstop the composite action cannot + # declare for itself; `check-bounded-apt-install` fails the build if it + # goes missing. 12 minutes sits above the script's own 590s worst case so + # the script reports the failure rather than being killed mid-sentence. - name: Install system dependencies - # `-p streamlib` pulls streamlib-engine, whose build.rs compiles GLSL - # compute shaders with glslc; the Linux - # engine dep set links Vulkan. Minimal subset — no GPU/codec backends. - run: | - sudo apt-get update - sudo apt-get install -y \ - pkg-config \ - protobuf-compiler \ - libclang-dev \ - libvulkan-dev \ + timeout-minutes: 12 + uses: ./.github/actions/install-linux-engine-build-dependencies + with: + packages: >- + pkg-config + protobuf-compiler + libclang1-18 + libclang-common-18-dev + libvulkan-dev glslc - name: Install Rust diff --git a/xtask/src/check_bounded_apt_install.rs b/xtask/src/check_bounded_apt_install.rs new file mode 100644 index 000000000..a6968e47b --- /dev/null +++ b/xtask/src/check_bounded_apt_install.rs @@ -0,0 +1,1041 @@ +// Copyright (c) 2025 Jonathan Fontanez +// SPDX-License-Identifier: BUSL-1.1 + +//! Keeps every apt install in CI behind the bounded-retry action. +//! +//! A workflow that installs packages itself has no wall-clock bound, and the +//! mode that costs is not the one people write guards for — the script's own +//! header records the measurement and why apt's guards miss it. +//! +//! So this is a location rule: an *apt* install belongs in +//! `install-system-dependencies-with-bounded-retry.sh` and nowhere else under +//! `.github/`. Three workflows previously carried three copies of the same +//! unbounded pair of commands, which is exactly how one of them ends up with a +//! bound and the others do not — and composite actions are scanned too, because +//! this repo's first `.github/actions/` directory arrived with that script. +//! +//! It also requires `timeout-minutes` on every *workflow* step that calls the +//! action. Composite-action steps cannot declare one, so the caller's step is +//! the only place the native backstop can live, and a backstop nobody can +//! forget is the whole point of gating it. +//! +//! Stated blind spots. `timeout-minutes:` counts only at a step's own key +//! indentation, so a `with:` input of that name does not satisfy the gate. A +//! step spelled across a YAML anchor or an `!!merge` key is not recognised as a +//! step at all. The front-end list is the apt family only, so +//! `release-wheel.yml`'s `dnf install` inside its manylinux container passes — +//! that job is release-time, runs in a container this action cannot serve, and +//! is deliberately out of scope; it is a real unbounded fetch all the same. Two +//! more live on [`line_fetches_packages`], which owns the shell heuristic. +//! +//! The scan root is `.github/` alone, so a workflow step that shells out to a +//! script elsewhere in the repo is uncovered — `repo-gates.yml` already runs +//! `bash scripts/check-license-headers.sh`, and `scripts/docker/host-prereqs.sh` +//! already installs apt packages unbounded. Widening the root would put every +//! developer-facing script in the repo under a CI rule written for CI. +//! +//! `.github/ISSUE_TEMPLATE/` is exempt: it is prose by construction, cannot run +//! anything, and is the one place under `.github/` where "run apt install …" is +//! a sentence rather than a step. +//! +//! [`ensure_the_gate_is_still_wired`] is what stops the whole gate from passing +//! vacuously if the shapes it keys on move. + +use anyhow::Result; +use std::path::Path; + +/// The action every workflow must go through to install apt packages. +const BOUNDED_APT_INSTALL_ACTION_REFERENCE: &str = + "./.github/actions/install-linux-engine-build-dependencies"; + +/// Front-ends that fetch packages, in the spellings a CI step actually uses. +/// +/// `apt-get` alone would be one hyphen from useless: `apt install -y` is the +/// spelling most people reach for first and is just as unbounded. +const PACKAGE_FETCH_FRONT_ENDS: &[&str] = &["apt-get", "apt", "aptitude"]; + +/// Subcommands that make one of those front-ends reach the network. +const PACKAGE_FETCH_SUBCOMMANDS: &[&str] = &[ + "install", + "reinstall", + "update", + "upgrade", + "dist-upgrade", + "full-upgrade", + "build-dep", +]; + +/// `dpkg` is mostly read-only (`-l`, `-L`, `-S`), so only the installing flags +/// count — otherwise the gate would fire on a perfectly good package query. +const DPKG_INSTALL_FLAGS: &[&str] = &["-i", "--install", "--unpack"]; + +/// The one file under `.github/` allowed to fetch packages. +const BOUNDED_RETRY_SCRIPT_RELATIVE_PATH: &str = ".github/actions/install-linux-engine-build-dependencies/install-system-dependencies-with-bounded-retry.sh"; + +/// Everything CI runs lives here — workflows and composite actions alike. +const GITHUB_CI_SCAN_ROOT: &str = ".github"; + +/// Only a workflow step can carry `timeout-minutes`; a composite action's cannot. +const WORKFLOW_FILE_PREFIX: &str = ".github/workflows/"; + +/// Prose by construction — the one place under `.github/` where an apt command +/// is quoted rather than run. Everything else is scanned, extension or not: a +/// Docker container action's `Dockerfile` is a first-class Actions shape and a +/// natural home for an unbounded `RUN apt-get install`. +const PROSE_ONLY_EXEMPT_PREFIX: &str = ".github/ISSUE_TEMPLATE/"; + +/// A line that installs apt packages outside the bounded-retry script. +#[derive(Debug, PartialEq, Eq)] +pub struct UnboundedAptInvocation { + pub file_path: String, + pub line_number: usize, + pub line: String, +} + +/// A workflow step calling the action without the native step-level ceiling. +#[derive(Debug, PartialEq, Eq)] +pub struct ActionCallWithoutTimeout { + pub workflow_path: String, + pub line_number: usize, +} + +#[derive(Debug, Default)] +pub struct BoundedAptInstallScanReport { + pub unbounded_apt_invocations: Vec, + pub action_calls_without_timeout: Vec, + pub workflow_steps_calling_the_action: usize, + pub files_scanned: usize, +} + +pub fn run(workspace_root: &Path) -> Result<()> { + let report = scan(workspace_root)?; + + crate::ensure_source_walking_gate_read_source( + "check-bounded-apt-install", + GITHUB_CI_SCAN_ROOT, + report.files_scanned, + "an unbounded apt install to hang a job for 13 minutes", + )?; + ensure_the_gate_is_still_wired(workspace_root, &report)?; + + let mut failure_lines: Vec = Vec::new(); + + for invocation in &report.unbounded_apt_invocations { + failure_lines.push(format!( + "{}:{}: fetches apt packages under {} — install through `{}` instead, which \ + bounds each command and escapes to a second mirror. The only file allowed to \ + run apt is `{}`. Offending line: {}", + invocation.file_path, + invocation.line_number, + GITHUB_CI_SCAN_ROOT, + BOUNDED_APT_INSTALL_ACTION_REFERENCE, + BOUNDED_RETRY_SCRIPT_RELATIVE_PATH, + invocation.line.trim(), + )); + } + + for call in &report.action_calls_without_timeout { + failure_lines.push(format!( + "{}:{}: step calls the bounded-apt-install action without `timeout-minutes` — \ + a composite action cannot declare one, so this step is the only place the \ + native ceiling can live", + call.workflow_path, call.line_number, + )); + } + + anyhow::ensure!( + failure_lines.is_empty(), + "check-bounded-apt-install found {} violation(s) across {} file(s):\n{}", + failure_lines.len(), + report.files_scanned, + failure_lines.join("\n"), + ); + + tracing::info!( + "check-bounded-apt-install: {} files scanned, {} bounded action call(s), no unbounded fetch", + report.files_scanned, + report.workflow_steps_calling_the_action, + ); + Ok(()) +} + +/// Fail if the shapes this gate keys on have moved out from under it. +/// +/// Every check here is a substring match against one of two constants. Rename +/// the action directory and `calls_the_action` matches nothing, every violation +/// list comes back empty, and the gate reports green while each step quietly +/// loses its ceiling — a gate that cannot fail is worse than no gate, because +/// the workflow list reads as covered. +fn ensure_the_gate_is_still_wired( + workspace_root: &Path, + report: &BoundedAptInstallScanReport, +) -> Result<()> { + anyhow::ensure!( + workspace_root + .join(BOUNDED_RETRY_SCRIPT_RELATIVE_PATH) + .is_file(), + "check-bounded-apt-install expects the bounded-retry script at `{}` and it is not \ + there — the gate's package-fetch ban would then point callers at a file that does \ + not exist. Update BOUNDED_RETRY_SCRIPT_RELATIVE_PATH if the script moved.", + BOUNDED_RETRY_SCRIPT_RELATIVE_PATH, + ); + + anyhow::ensure!( + report.workflow_steps_calling_the_action > 0, + "check-bounded-apt-install matched `{}` in no workflow step — either every apt \ + install was removed from CI, or the action was renamed and this gate is now \ + checking nothing. Update BOUNDED_APT_INSTALL_ACTION_REFERENCE.", + BOUNDED_APT_INSTALL_ACTION_REFERENCE, + ); + + Ok(()) +} + +pub fn scan(workspace_root: &Path) -> Result { + let mut report = BoundedAptInstallScanReport::default(); + + for file_path in crate::list_repository_files_under(workspace_root, GITHUB_CI_SCAN_ROOT)? { + if file_path == BOUNDED_RETRY_SCRIPT_RELATIVE_PATH + || file_path.starts_with(PROSE_ONLY_EXEMPT_PREFIX) + { + continue; + } + + let absolute = workspace_root.join(&file_path); + let Ok(contents) = std::fs::read_to_string(&absolute) else { + // A non-UTF-8 file under .github/ holds no shell for apt to run. + continue; + }; + + report.files_scanned += 1; + collect_unbounded_apt_invocations(&file_path, &contents, &mut report); + + if file_path.starts_with(WORKFLOW_FILE_PREFIX) { + collect_action_calls(&file_path, &contents, &mut report); + } + } + + Ok(report) +} + +fn collect_unbounded_apt_invocations( + file_path: &str, + contents: &str, + report: &mut BoundedAptInstallScanReport, +) { + for (index, line) in contents.lines().enumerate() { + if line.trim_start().starts_with('#') || !line_fetches_packages(line) { + continue; + } + report + .unbounded_apt_invocations + .push(UnboundedAptInvocation { + file_path: file_path.to_owned(), + line_number: index + 1, + line: line.to_owned(), + }); + } +} + +/// Does this shell line reach the network for packages? +/// +/// Whitespace tokenisation, then exact token equality on the command — which is +/// what keeps a path like `/var/cache/apt/archives` from reading as an `apt` +/// invocation, and lets `sudo`, `DEBIAN_FRONTEND=noninteractive` and any other +/// prefix fall out for free. +/// +/// A trailing `\` continuation counts, because the subcommand is on the next +/// line. A front-end as the *last* token of a line does not: that is far more +/// often a step named "Install system dependencies with apt" than a command. +/// +/// Global flags between the front-end and its subcommand are skipped, so +/// `apt-get -y install` and `apt-get -qq update` both read as fetches. `--` +/// stops the skip: after end-of-options a word is prose, not a subcommand. +/// +/// Two blind spots, stated rather than papered over: a front-end whose +/// subcommand is built from a shell variable (`apt-get "$verb"`), and a flag +/// whose value is a separate token (`apt-get -o Foo=bar update`) — the value +/// does not start with `-`, so the skip stops on it. +fn line_fetches_packages(line: &str) -> bool { + let mut tokens = line.split_whitespace().peekable(); + + while let Some(token) = tokens.next() { + if PACKAGE_FETCH_FRONT_ENDS.contains(&token) { + let mut following = tokens.peek().copied(); + while following.is_some_and(|word| word != "--" && word.starts_with('-')) { + tokens.next(); + following = tokens.peek().copied(); + } + match following { + Some("\\") => return true, + Some(subcommand) if PACKAGE_FETCH_SUBCOMMANDS.contains(&subcommand) => return true, + _ => continue, + } + } + + if token == "dpkg" + && tokens + .peek() + .is_some_and(|flag| DPKG_INSTALL_FLAGS.contains(flag)) + { + return true; + } + } + + false +} + +fn collect_action_calls( + workflow_path: &str, + contents: &str, + report: &mut BoundedAptInstallScanReport, +) { + for step in split_into_step_blocks(contents) { + // A commented-out `uses:` is not a call. Counting one would also inflate + // `workflow_steps_calling_the_action`, which is what + // `ensure_the_gate_is_still_wired` reads to decide the gate is live — + // so a repo whose every real call had been commented out would still + // look wired. + if !step + .uncommented_lines() + .any(|line| line.contains(BOUNDED_APT_INSTALL_ACTION_REFERENCE)) + { + continue; + } + + report.workflow_steps_calling_the_action += 1; + + // Only at the step's own key indentation: a `with:` input that happens + // to be named `timeout-minutes` is an input GitHub ignores, not a ceiling. + let declares_a_timeout = step.uncommented_lines().any(|line| { + line.trim_start().starts_with("timeout-minutes:") + && indentation_width(line) == step.key_indentation_width + }); + + if !declares_a_timeout { + report + .action_calls_without_timeout + .push(ActionCallWithoutTimeout { + workflow_path: workflow_path.to_owned(), + line_number: step.first_line_number, + }); + } + } +} + +struct WorkflowStepBlock<'a> { + first_line_number: usize, + /// Indentation of the step's own keys — `name:`, `uses:`, `timeout-minutes:` + /// — as measured, not as assumed from the `-`. `- name:` and a bare `-` + /// put them somewhere other than marker + 2. + key_indentation_width: usize, + lines: Vec<&'a str>, +} + +impl<'a> WorkflowStepBlock<'a> { + fn uncommented_lines(&self) -> impl Iterator { + self.lines + .iter() + .copied() + .filter(|line| !line.trim_start().starts_with('#')) + } +} + +/// YAML forbids tab indentation, so a byte count is a column count. +fn indentation_width(line: &str) -> usize { + line.len() - line.trim_start().len() +} + +/// Split a workflow into top-level YAML list-item blocks. +/// +/// A block runs from its `-` line to the next non-blank line indented no +/// further, which keeps a nested `with:` mapping inside the step it belongs to. +/// Blocks never overlap: a `-` already swallowed by an open block is one of that +/// step's own list items, not a sibling step, and spawning a block for it would +/// produce a step that excludes its own `timeout-minutes`. +/// +/// Indentation-based rather than parsed. `serde_yaml` is already in the +/// workspace, so the cost is not the dependency — it is that its `Value` carries +/// no source spans, and every failure this gate emits names a `file:line` the +/// author can jump to. +fn split_into_step_blocks(contents: &str) -> Vec> { + let lines: Vec<&str> = contents.lines().collect(); + let mut blocks: Vec> = Vec::new(); + let mut next_unclaimed_line = 0usize; + + for (index, line) in lines.iter().enumerate() { + if index < next_unclaimed_line { + continue; + } + + let trimmed = line.trim_start(); + // `- key: value` and a bare `-` with the keys on following lines are + // both legal spellings of one sequence item. + if trimmed != "-" && !trimmed.starts_with("- ") { + continue; + } + + let list_marker_indentation_width = indentation_width(line); + let mut block_lines: Vec<&str> = vec![*line]; + let mut end = index + 1; + + for (following_index, following_line) in lines.iter().enumerate().skip(index + 1) { + if following_line.trim().is_empty() { + continue; + } + if indentation_width(following_line) <= list_marker_indentation_width { + break; + } + block_lines.push(following_line); + end = following_index + 1; + } + + // `- key:` puts the first key on the marker line, after however much + // space follows the dash; a bare `-` puts it on the next line. + let after_the_dash = &trimmed[1..]; + let key_indentation_width = if after_the_dash.trim().is_empty() { + block_lines + .get(1) + .map_or(list_marker_indentation_width + 2, |line| { + indentation_width(line) + }) + } else { + list_marker_indentation_width + 1 + indentation_width(after_the_dash) + }; + + next_unclaimed_line = end; + blocks.push(WorkflowStepBlock { + first_line_number: index + 1, + key_indentation_width, + lines: block_lines, + }); + } + + blocks +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use std::os::unix::fs::PermissionsExt; + use std::path::PathBuf; + use std::process::Command; + use tempfile::TempDir; + + /// The family idiom for the workspace root in a gate's tests: free, and it + /// needs neither cargo on PATH nor the package lock. + fn workspace_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .expect("xtask/ always has a workspace root above it") + .to_path_buf() + } + + /// Wraps step text in the smallest workflow that parses, so each test reads + /// as the steps it is actually about. + fn scan_workflow_steps(steps: &str) -> BoundedAptInstallScanReport { + scan_workflow_text(&format!("jobs:\n t:\n steps:\n{steps}")) + } + + fn scan_workflow_text(contents: &str) -> BoundedAptInstallScanReport { + let mut report = BoundedAptInstallScanReport { + files_scanned: 1, + ..Default::default() + }; + collect_unbounded_apt_invocations("test.yml", contents, &mut report); + collect_action_calls("test.yml", contents, &mut report); + report + } + + const A_BOUNDED_STEP: &str = " - name: Install system dependencies\n timeout-minutes: 10\n uses: ./.github/actions/install-linux-engine-build-dependencies\n with:\n packages: glslc libvulkan-dev\n"; + + #[test] + fn rejects_a_bare_apt_get_in_a_workflow() { + let report = scan_workflow_steps( + " - name: Install system dependencies\n \ + run: |\n sudo apt-get update\n sudo apt-get install -y glslc\n", + ); + assert_eq!( + report.unbounded_apt_invocations.len(), + 2, + "both apt-get lines must be flagged: {report:?}" + ); + } + + #[test] + fn accepts_a_workflow_that_uses_the_bounded_action() { + let report = scan_workflow_steps(A_BOUNDED_STEP); + assert!( + report.unbounded_apt_invocations.is_empty(), + "no raw apt-get: {report:?}" + ); + assert!( + report.action_calls_without_timeout.is_empty(), + "the step declares timeout-minutes: {report:?}" + ); + assert_eq!(report.workflow_steps_calling_the_action, 1); + } + + #[test] + fn rejects_an_action_call_missing_the_native_ceiling() { + let report = scan_workflow_steps( + " - name: Install system dependencies\n \ + uses: ./.github/actions/install-linux-engine-build-dependencies\n \ + with:\n packages: glslc\n", + ); + assert_eq!( + report.action_calls_without_timeout.len(), + 1, + "a step without timeout-minutes must be flagged: {report:?}" + ); + } + + #[test] + fn a_timeout_on_a_neighbouring_step_does_not_count() { + let report = scan_workflow_steps( + " - name: Something else\n \ + timeout-minutes: 8\n run: echo hi\n - name: Install system dependencies\n \ + uses: ./.github/actions/install-linux-engine-build-dependencies\n \ + with:\n packages: glslc\n", + ); + assert_eq!( + report.action_calls_without_timeout.len(), + 1, + "the sibling's bound must not satisfy the action's step: {report:?}" + ); + } + + #[test] + fn a_with_input_named_timeout_minutes_does_not_satisfy_the_gate() { + let report = scan_workflow_steps( + " - name: Install system dependencies\n \ + uses: ./.github/actions/install-linux-engine-build-dependencies\n \ + with:\n timeout-minutes: 10\n packages: glslc\n", + ); + assert_eq!( + report.action_calls_without_timeout.len(), + 1, + "an input is not a step ceiling: {report:?}" + ); + } + + #[test] + fn a_bare_dash_step_is_still_a_step() { + // `-` alone with the keys on following lines is a legal sequence item, + // and a splitter that misses it would let an unbounded step through. + let report = scan_workflow_steps( + " -\n name: Install system dependencies\n \ + uses: ./.github/actions/install-linux-engine-build-dependencies\n", + ); + assert_eq!( + report.workflow_steps_calling_the_action, 1, + "the bare-dash step must be seen: {report:?}" + ); + assert_eq!( + report.action_calls_without_timeout.len(), + 1, + "and it must be flagged as unbounded: {report:?}" + ); + } + + #[test] + fn a_nested_list_item_does_not_split_its_step_away_from_its_ceiling() { + let report = scan_workflow_steps( + " - name: Install system dependencies\n \ + timeout-minutes: 10\n uses: ./.github/actions/install-linux-engine-build-dependencies\n \ + with:\n packages: glslc\n extra:\n - ./.github/actions/install-linux-engine-build-dependencies\n", + ); + assert!( + report.action_calls_without_timeout.is_empty(), + "the nested item belongs to the bounded step, not to a step of its own: {report:?}" + ); + } + + #[test] + fn a_commented_out_action_call_is_not_a_call() { + // It must not demand a ceiling, and — the one that matters — it must not + // count toward the liveness check, or a repo whose every real call had + // been commented out would still read as wired. + let report = scan_workflow_steps( + " - name: Something else\n \ + # uses: ./.github/actions/install-linux-engine-build-dependencies\n \ + run: echo hi\n", + ); + assert_eq!( + report.workflow_steps_calling_the_action, 0, + "a commented `uses:` is not a call: {report:?}" + ); + assert!( + report.action_calls_without_timeout.is_empty(), + "and it demands no ceiling: {report:?}" + ); + } + + #[test] + fn the_step_key_indentation_width_is_measured_not_assumed() { + // `-` followed by three spaces puts the keys at marker + 4. Deriving the + // key indent as marker + 2 would look straight past this ceiling. + let report = scan_workflow_steps( + " - name: Install system dependencies\n \ + timeout-minutes: 10\n \ + uses: ./.github/actions/install-linux-engine-build-dependencies\n", + ); + assert_eq!(report.workflow_steps_calling_the_action, 1); + assert!( + report.action_calls_without_timeout.is_empty(), + "the widely-indented ceiling must count: {report:?}" + ); + } + + #[test] + fn a_step_named_after_apt_is_not_an_apt_invocation() { + // A dangling front-end at end of line is far more often a step title + // than a command. + let report = scan_workflow_steps( + " - name: Install system dependencies with apt\n \ + timeout-minutes: 10\n \ + uses: ./.github/actions/install-linux-engine-build-dependencies\n", + ); + assert!( + report.unbounded_apt_invocations.is_empty(), + "a step title is not an invocation: {report:?}" + ); + assert!( + report.action_calls_without_timeout.is_empty(), + "and it is correctly bounded: {report:?}" + ); + } + + #[test] + fn a_dangling_front_end_counts_only_when_it_is_a_continuation() { + assert!(line_fetches_packages(" sudo apt-get \\")); + assert!(!line_fetches_packages(" - name: Set up apt")); + } + + #[test] + fn a_global_flag_between_the_front_end_and_its_subcommand_is_skipped() { + assert!(line_fetches_packages( + " sudo apt-get -y install glslc" + )); + assert!(line_fetches_packages(" apt-get -qq update")); + // `--` is end-of-options: past it a word is prose, not a subcommand. + assert!(!line_fetches_packages( + " - name: Install with apt -- update the pins" + )); + } + + #[test] + fn a_read_only_dpkg_query_is_not_a_package_fetch() { + assert!(!line_fetches_packages(" run: dpkg -L libclang1-18")); + assert!(!line_fetches_packages( + " run: ls /var/cache/apt/archives" + )); + assert!(line_fetches_packages(" run: sudo dpkg -i local.deb")); + assert!(line_fetches_packages( + " sudo DEBIAN_FRONTEND=noninteractive apt install -y cowsay" + )); + } + + #[test] + fn skips_a_commented_apt_get() { + let report = scan_workflow_steps(" # was: sudo apt-get install -y glslc\n"); + assert!( + report.unbounded_apt_invocations.is_empty(), + "a comment naming apt-get is not an invocation: {report:?}" + ); + } + + #[test] + fn the_repo_itself_passes_the_gate() { + run(&workspace_root()).expect("the repo's own CI surface must be bounded"); + } + + #[test] + fn the_gate_refuses_to_pass_vacuously_when_nothing_calls_the_action() { + let report = BoundedAptInstallScanReport { + files_scanned: 8, + ..Default::default() + }; + let failure = ensure_the_gate_is_still_wired(&workspace_root(), &report) + .expect_err("zero action calls means the gate is checking nothing"); + assert!( + format!("{failure:#}").contains("this gate is now checking nothing"), + "the failure must name the vacuous-pass hazard: {failure:#}" + ); + } + + #[test] + fn the_gate_refuses_to_pass_when_the_bounded_retry_script_is_gone() { + let empty_tree = TempDir::new().unwrap(); + let report = BoundedAptInstallScanReport { + files_scanned: 8, + workflow_steps_calling_the_action: 3, + ..Default::default() + }; + let failure = ensure_the_gate_is_still_wired(empty_tree.path(), &report) + .expect_err("a missing script must fail the gate"); + assert!( + format!("{failure:#}").contains(BOUNDED_RETRY_SCRIPT_RELATIVE_PATH), + "the failure must name the missing script: {failure:#}" + ); + } + + // ---- the bounded-retry script's own behaviour ---------------------------- + // + // The ticket asks for a synthetic check that the timeout path actually + // fires, rather than a green run that happened to get a fast mirror. These + // drive the real script with `STREAMLIB_APT_GET_COMMAND` pointed at a + // fixture, so they need no root, no apt and no network. + + /// Drives the real script against a fake `apt-get` that logs every + /// subcommand it is handed, so a test can assert what ran and in what order. + struct BoundedRetryScriptHarness { + _temp: TempDir, + script: PathBuf, + apt_get_fixture: PathBuf, + mirror_switch_fixture: PathBuf, + dpkg_repair_fixture: PathBuf, + privilege_prefix_fixture: PathBuf, + invocation_log: PathBuf, + } + + impl BoundedRetryScriptHarness { + fn new(apt_get_fixture_body: &str) -> Self { + let temp = TempDir::new().unwrap(); + + let invocation_log = temp.path().join("invocations.log"); + fs::write(&invocation_log, "").unwrap(); + + Self { + script: workspace_root().join(BOUNDED_RETRY_SCRIPT_RELATIVE_PATH), + apt_get_fixture: write_executable_fixture( + temp.path().join("apt-get-fixture"), + apt_get_fixture_body, + ), + mirror_switch_fixture: write_executable_fixture( + temp.path().join("mirror-switch-fixture"), + "#!/usr/bin/env bash\nprintf 'switched %s\\n' \"$1\" \ + >> \"$STREAMLIB_APT_FIXTURE_INVOCATION_LOG\"\n", + ), + dpkg_repair_fixture: write_executable_fixture( + temp.path().join("dpkg-repair-fixture"), + "#!/usr/bin/env bash\nprintf 'dpkg-repaired\\n' \ + >> \"$STREAMLIB_APT_FIXTURE_INVOCATION_LOG\"\n", + ), + privilege_prefix_fixture: write_executable_fixture( + temp.path().join("privilege-prefix-fixture"), + "#!/usr/bin/env bash\nprintf 'privileged %s\\n' \"$*\" \ + >> \"$STREAMLIB_APT_FIXTURE_INVOCATION_LOG\"\nexec \"$@\"\n", + ), + _temp: temp, + invocation_log, + } + } + + /// Runs with the privilege prefix in play instead of erased, so the + /// `sudo timeout` word order is observable. Every other test empties the + /// prefix, and with it empty both orderings emit identical argv. + fn run_recording_the_privileged_argv(&self) -> BoundedRetryScriptRun { + self.run_with_environment(&[ + ( + "STREAMLIB_APT_PRIVILEGE_PREFIX", + self.privilege_prefix_fixture.as_os_str(), + ), + ("STREAMLIB_APT_ATTEMPT_TIMEOUT_SECONDS", "5".as_ref()), + ]) + } + + fn run_with_attempt_bound(&self, attempt_timeout_seconds: u64) -> BoundedRetryScriptRun { + self.run_with_environment(&[ + ("STREAMLIB_APT_PRIVILEGE_PREFIX", "".as_ref()), + ( + "STREAMLIB_APT_ATTEMPT_TIMEOUT_SECONDS", + attempt_timeout_seconds.to_string().as_ref(), + ), + ]) + } + + fn run_with_environment( + &self, + overrides: &[(&str, &std::ffi::OsStr)], + ) -> BoundedRetryScriptRun { + let mut command = Command::new(&self.script); + command + .arg("glslc") + .env("STREAMLIB_APT_GET_COMMAND", &self.apt_get_fixture) + .env( + "STREAMLIB_APT_MIRROR_SWITCH_COMMAND", + &self.mirror_switch_fixture, + ) + .env("STREAMLIB_DPKG_REPAIR_COMMAND", &self.dpkg_repair_fixture) + .env("STREAMLIB_APT_FIXTURE_INVOCATION_LOG", &self.invocation_log); + + for (name, value) in overrides { + command.env(name, value); + } + + let result = command + .output() + .expect("the bounded-retry script must be executable"); + + BoundedRetryScriptRun { + succeeded: result.status.success(), + invocation_log: fs::read_to_string(&self.invocation_log).unwrap(), + stderr: String::from_utf8_lossy(&result.stderr).into_owned(), + } + } + } + + struct BoundedRetryScriptRun { + succeeded: bool, + invocation_log: String, + stderr: String, + } + + fn write_executable_fixture(path: PathBuf, body: &str) -> PathBuf { + fs::write(&path, body).unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).unwrap(); + path + } + + /// Logs the whole argv, not just the subcommand: the `Acquire::*` options + /// are part of the contract, so a fixture that recorded only `$1` could not + /// hold them. + const LOG_THE_WHOLE_INVOCATION: &str = + "printf '%s\\n' \"$*\" >> \"$STREAMLIB_APT_FIXTURE_INVOCATION_LOG\""; + + /// Succeeds on `update`, stalls on `install` until the mirror has been + /// switched — the shape of the measured incident, where `update` finished + /// in 65s (inside the bound) and `install` was the command that ran 12m17s. + /// + /// A fixture that stalls on *every* subcommand never reaches the install, so + /// the install-side status handling would go unexercised. + /// + /// `exec` so the signal reaches `sleep` itself rather than the shell that + /// spawned it. `timeout` signals the whole process group, so the shell's + /// child dies either way and `Command::output()` sees its pipes close — but + /// that is a property of `timeout`'s default, not of this fixture, and a + /// stray survivor would hold those pipes for the whole sleep. + const STALL_ONLY_ON_THE_INSTALL: &str = "\ + if [ \"$1\" = update ]; then exit 0; fi\n\ + if grep -q switched \"$STREAMLIB_APT_FIXTURE_INVOCATION_LOG\"; then exit 0; fi\n\ + exec sleep 60\n"; + + /// Exits non-zero immediately on the install — a broken package name, not a + /// slow mirror. The distinction is what the failure message has to get right. + const FAIL_THE_INSTALL_OUTRIGHT: &str = "\ + if [ \"$1\" = update ]; then exit 0; fi\n\ + exit 100\n"; + + fn apt_fixture(body: &str) -> String { + format!("#!/usr/bin/env bash\n{LOG_THE_WHOLE_INVOCATION}\n{body}") + } + + fn count_of(log: &str, subcommand: &str) -> usize { + log.lines() + .filter(|line| line.split_whitespace().next() == Some(subcommand)) + .count() + } + + #[test] + fn a_healthy_mirror_installs_without_switching() { + let harness = BoundedRetryScriptHarness::new(&apt_fixture("exit 0\n")); + let run = harness.run_with_attempt_bound(5); + let log = &run.invocation_log; + + assert!(run.succeeded, "a healthy mirror must succeed; log:\n{log}"); + assert_eq!(count_of(log, "update"), 1, "log:\n{log}"); + assert_eq!(count_of(log, "install"), 1, "log:\n{log}"); + assert!( + !log.contains("switched"), + "a healthy run must not touch the mirror; log:\n{log}" + ); + } + + #[test] + fn every_apt_command_carries_the_retry_and_timeout_options() { + // These cover the transient per-file failure and the connection that + // goes silent. The wall-clock bound covers neither. + let harness = BoundedRetryScriptHarness::new(&apt_fixture("exit 0\n")); + let run = harness.run_with_attempt_bound(5); + + assert_eq!( + run.invocation_log.lines().count(), + 2, + "update and install must both have run, or this asserts over nothing; log:\n{}", + run.invocation_log + ); + for invocation in run.invocation_log.lines() { + for option in [ + "Acquire::Retries=3", + "Acquire::http::Timeout=30", + "Acquire::https::Timeout=30", + ] { + assert!( + invocation.contains(option), + "`{option}` missing from `{invocation}`" + ); + } + } + } + + #[test] + fn the_bound_runs_under_the_privilege_prefix_not_the_other_way_round() { + // `timeout sudo apt-get` would put sudo in timeout's child slot, where + // SIGKILL lands on sudo and leaves an orphaned root apt-get holding + // /var/lib/dpkg/lock-frontend — so the fallback attempt fails on the + // lock and the escape hatch means nothing. Both orderings emit identical + // argv when the prefix is empty, which is why this test supplies one. + let harness = BoundedRetryScriptHarness::new(&apt_fixture("exit 0\n")); + let run = harness.run_recording_the_privileged_argv(); + + let privileged: Vec<&str> = run + .invocation_log + .lines() + .filter_map(|line| line.strip_prefix("privileged ")) + .collect(); + + assert_eq!(privileged.len(), 2, "log:\n{}", run.invocation_log); + for argv in privileged { + assert!( + argv.starts_with("timeout "), + "the prefix must wrap timeout, not the other way round: `{argv}`" + ); + } + } + + #[test] + fn a_slow_install_trips_the_bound_and_escapes_to_the_fallback() { + let harness = BoundedRetryScriptHarness::new(&apt_fixture(STALL_ONLY_ON_THE_INSTALL)); + let run = harness.run_with_attempt_bound(2); + let log = &run.invocation_log; + + assert!( + log.contains("switched"), + "the install-side bound must fire and repoint the mirror; log:\n{log}" + ); + assert!( + run.succeeded, + "the fallback mirror must carry the install home; log:\n{log}" + ); + assert_eq!( + count_of(log, "install"), + 2, + "the install must be attempted once per mirror; log:\n{log}" + ); + assert!( + run.stderr.contains("did not finish inside the 2s bound"), + "a stalled mirror must be reported as a timeout; stderr:\n{}", + run.stderr + ); + } + + #[test] + fn an_interrupted_dpkg_is_repaired_before_the_fallback_attempt() { + // The bound can fire mid-unpack, and the SIGINT reaches dpkg too. apt + // then refuses every later install until dpkg is reconfigured, which + // would make the fallback attempt fail deterministically. + let harness = BoundedRetryScriptHarness::new(&apt_fixture(STALL_ONLY_ON_THE_INSTALL)); + let run = harness.run_with_attempt_bound(2); + let log = &run.invocation_log; + + let repaired = log.lines().position(|line| line == "dpkg-repaired"); + let switched = log.lines().position(|line| line.starts_with("switched")); + assert!( + repaired.is_some() && switched.is_some() && repaired < switched, + "dpkg must be repaired before the mirror switch; log:\n{log}" + ); + } + + #[test] + fn a_broken_package_is_not_reported_as_a_slow_mirror() { + // The likeliest deterministic failure here is a version-pinned package + // name a runner-image roll retired. Calling that a timeout sends the + // reader hunting a network problem that is not there. + let harness = BoundedRetryScriptHarness::new(&apt_fixture(FAIL_THE_INSTALL_OUTRIGHT)); + let run = harness.run_with_attempt_bound(5); + + assert!(!run.succeeded, "a broken package must fail the step"); + assert!( + run.stderr.contains("failed with apt exit status 100"), + "the real exit status must be reported; stderr:\n{}", + run.stderr + ); + assert!( + !run.stderr.contains("did not finish inside"), + "nothing timed out, so nothing may say so; stderr:\n{}", + run.stderr + ); + } + + #[test] + fn an_install_that_ignores_the_signal_is_still_reported_as_a_timeout() { + // `timeout` reports 124 when the command honoured the signal, and 137 + // when `--kill-after` had to escalate to SIGKILL. apt inside a dpkg + // transaction can be the second, and calling that an apt failure is the + // same misdiagnosis the 124 branch exists to prevent. + // Deliberately not `exec sleep`, unlike STALL_ONLY_ON_THE_INSTALL: exec + // would replace the shell and discard the trap, `sleep` would honour + // SIGINT, and the escalation this test exists for would never happen. + let harness = BoundedRetryScriptHarness::new(&apt_fixture( + "if [ \"$1\" = update ]; then exit 0; fi\ntrap '' INT\nsleep 60\n", + )); + let run = harness.run_with_environment(&[ + ("STREAMLIB_APT_PRIVILEGE_PREFIX", "".as_ref()), + ("STREAMLIB_APT_ATTEMPT_TIMEOUT_SECONDS", "1".as_ref()), + ("STREAMLIB_APT_KILL_AFTER_SECONDS", "1".as_ref()), + ]); + + assert!(!run.succeeded, "stderr:\n{}", run.stderr); + assert!( + run.stderr.contains("did not finish inside"), + "a SIGKILL escalation is a timeout, not an apt failure; stderr:\n{}", + run.stderr + ); + assert!( + !run.stderr.contains("apt exit status"), + "nothing here is an apt exit status; stderr:\n{}", + run.stderr + ); + } + + #[test] + fn both_mirrors_failing_on_the_install_exits_non_zero_rather_than_hanging() { + let harness = BoundedRetryScriptHarness::new(&apt_fixture(FAIL_THE_INSTALL_OUTRIGHT)); + let run = harness.run_with_attempt_bound(5); + let log = &run.invocation_log; + + assert!( + !run.succeeded, + "exhausting both mirrors must fail; log:\n{log}" + ); + assert_eq!( + count_of(log, "install"), + 2, + "the install must be tried once per mirror, no more; log:\n{log}" + ); + } + + #[test] + fn a_failing_update_does_not_go_on_to_install() { + let harness = BoundedRetryScriptHarness::new(&apt_fixture("exit 100\n")); + let run = harness.run_with_attempt_bound(5); + let log = &run.invocation_log; + + assert!( + !run.succeeded, + "exhausting both mirrors must fail; log:\n{log}" + ); + assert_eq!( + count_of(log, "update"), + 2, + "one update per mirror; log:\n{log}" + ); + assert_eq!( + count_of(log, "install"), + 0, + "a failed update must not be followed by an install; log:\n{log}" + ); + } +} diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 864a758bd..01985937e 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -8,6 +8,7 @@ use clap::{Parser, Subcommand}; use std::path::{Path, PathBuf}; pub mod check_boundaries; +pub mod check_bounded_apt_install; pub mod check_clock_usage; pub mod check_device_wait_idle; pub mod check_no_escalate_in_lifecycle; @@ -24,6 +25,45 @@ pub mod normal_build_dep_graph; /// invisible to it. pub const RUST_CRATE_SOURCE_ROOT_DIR_NAMES: &[&str] = &["src", "processors"]; +/// Tracked (and untracked-but-not-ignored) files under one repo-relative root. +/// +/// `git ls-files` rather than a filesystem walk, for the reason every gate here +/// shares: CI walks a clean checkout, so "the files in the repo" is the +/// semantics meant, and the scan roots hold virtualenvs and build trees that +/// are not ours to gate. `-z` because a path containing a newline would +/// otherwise split into two entries and drop both from the scan. +pub fn list_repository_files_under( + workspace_root: &Path, + repo_relative_root: &str, +) -> Result> { + let output = std::process::Command::new("git") + .args([ + "ls-files", + "-z", + "--cached", + "--others", + "--exclude-standard", + "--", + ]) + .arg(repo_relative_root) + .current_dir(workspace_root) + .output() + .with_context(|| format!("failed to run `git ls-files` for {repo_relative_root}"))?; + + anyhow::ensure!( + output.status.success(), + "`git ls-files {repo_relative_root}` failed: {}", + String::from_utf8_lossy(&output.stderr), + ); + + Ok(String::from_utf8(output.stdout) + .with_context(|| format!("`git ls-files {repo_relative_root}` emitted non-UTF-8 paths"))? + .split('\0') + .filter(|path| !path.is_empty()) + .map(str::to_owned) + .collect()) +} + /// Refuse a source-walking gate run that read no source at all. /// /// A gate whose scan roots moved out from under it is indistinguishable from a @@ -48,7 +88,7 @@ pub fn ensure_source_walking_gate_read_source( /// Every source-walking gate, paired with the subcommand name that runs it alone. /// /// Each gate reads the tree and reports; none builds the workspace. That is what -/// lets one process run all nine in well under a second, and why CI runs them as +/// lets one process run all ten in well under a second, and why CI runs them as /// a single job rather than one runner per gate. const ALL_SOURCE_WALKING_GATES: &[(&str, fn(&Path) -> Result<()>)] = &[ ("lint-logging", lint_logging::run), @@ -69,6 +109,7 @@ const ALL_SOURCE_WALKING_GATES: &[(&str, fn(&Path) -> Result<()>)] = &[ check_no_unbounded_cstr_from_ptr::run, ), ("check-clock-usage", check_clock_usage::run), + ("check-bounded-apt-install", check_bounded_apt_install::run), ]; /// Run every source-walking gate, reporting all failures rather than the first. @@ -278,6 +319,19 @@ enum Commands { /// `docs/decisions/one-monotonic-clock.md`. CheckClockUsage, + /// CI gate keeping every apt install in CI behind + /// `.github/actions/install-linux-engine-build-dependencies`. Fails on any + /// `apt-get` under `.github/workflows/`, and on any step calling that + /// action without `timeout-minutes`. An inline `apt-get update && apt-get + /// install` has no wall-clock bound, and the mode that costs is a mirror + /// that is slow rather than stalled — one measured run fetched 35.6 MB at + /// 48 kB/s over 12m17s while every request made progress, so neither + /// `Acquire::Retries` (nothing failed) nor `Acquire::http::Timeout` + /// (nothing went idle) engaged. Composite-action steps cannot declare + /// `timeout-minutes`, so the caller's step is the only place the native + /// backstop can live. + CheckBoundedAptInstall, + /// Drift trip-wire for the vendored vulkanalia fork trees /// (`vendor/tatolab-vulkanalia{,-sys,-vma}`): hashes each vendored crate /// dir and fails on any byte change vs. the recorded hash — the guard @@ -324,6 +378,7 @@ fn main() -> Result<()> { check_no_unbounded_cstr_from_ptr::run(&workspace_root()?)? } Commands::CheckClockUsage => check_clock_usage::run(&workspace_root()?)?, + Commands::CheckBoundedAptInstall => check_bounded_apt_install::run(&workspace_root()?)?, Commands::CheckVendoredVulkanalia => check_vendored_vulkanalia::run(&workspace_root()?)?, Commands::CheckAllSourceGates => run_all_source_walking_gates(&workspace_root()?)?, Commands::RunLocalCiGates => run_local_ci_gates(&workspace_root()?)?,