From 88c6d0d4c620365581cf6ad46950ab5a7222bd07 Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Mon, 20 Jul 2026 10:45:23 +0700 Subject: [PATCH] fix(ci): main runs were being cancelled, recording nothing The concurrency block intended never to cancel on main -- the comment said so -- and did the opposite: cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} The expression renders to the STRING "false", and a non-empty string is truthy in that position, so main was cancelled like any other ref. It failed silently for exactly as long as nobody merged twice in quick succession. Four main runs were cancelled during this batch of merges (#36, #37, #39, #32), each with ZERO jobs recorded -- so those commits have no evidence they ever built. The runs that were supposed to be the record of what shipped are the ones that got killed. Encoding the rule in the concurrency GROUP is unambiguous: on main the SHA gives every run its own group, so there is nothing to supersede; every other ref keeps a per-ref group, so a force-push still cancels the old run. tests/workflow_concurrency.rs guards both halves -- an expression-valued cancel-in-progress, and a group that lost its per-SHA component (which with cancel-in-progress: true would cancel main on every push, strictly worse than the bug it replaced). Mutation-verified: restoring the original two lines fails both. 118 tests. --- .github/workflows/ci.yml | 21 +++- src-tauri/tests/workflow_concurrency.rs | 130 ++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 src-tauri/tests/workflow_concurrency.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fd4fc69..fb67eea 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,10 +11,25 @@ on: workflow_call: # A force-push or a quick second commit should cancel the superseded run rather -# than queue behind it. Never cancel on main: those runs record what shipped. +# than queue behind it. Runs on main must NEVER be cancelled: they are the only +# record that what shipped was green. +# +# The obvious spelling of that does not work: +# +# cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} +# +# The expression renders to the STRING "false", and a non-empty string is +# truthy here, so main was cancelled anyway. It failed silently and in exactly +# the case that matters -- merging two PRs in quick succession killed the first +# one's run. Four main runs were cancelled before this was noticed, each with +# zero jobs recorded, leaving those commits with no evidence they ever built. +# +# Encoding the rule in the GROUP instead is unambiguous: on main the SHA makes +# every run its own group, so there is never a run to supersede. On any other +# ref the group is per-ref as before, so a new push still cancels the old run. concurrency: - group: ci-${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} + group: ci-${{ github.workflow }}-${{ github.ref }}-${{ github.ref == 'refs/heads/main' && github.sha || 'shared' }} + cancel-in-progress: true permissions: contents: read diff --git a/src-tauri/tests/workflow_concurrency.rs b/src-tauri/tests/workflow_concurrency.rs new file mode 100644 index 0000000..69483c1 --- /dev/null +++ b/src-tauri/tests/workflow_concurrency.rs @@ -0,0 +1,130 @@ +//! `cancel-in-progress` must be a literal, never a comparison expression. +//! +//! This looks obviously correct and does not work: +//! +//! ```yaml +//! cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} +//! ``` +//! +//! The expression renders to the *string* `"false"`, and a non-empty string is +//! truthy in that position, so the branch it was meant to protect gets cancelled +//! anyway. It failed silently for exactly as long as nobody merged two things in +//! quick succession, then quietly killed four runs on `main` — each with zero +//! jobs recorded, leaving those commits with no evidence they ever built. +//! +//! Conditional behaviour belongs in the concurrency GROUP, where a per-SHA +//! component simply leaves nothing to supersede. +//! +//! Lives outside `.github/` so it cannot match its own explanation. + +use std::path::PathBuf; + +fn workflows_dir() -> PathBuf { + PathBuf::from(concat!(env!("CARGO_MANIFEST_DIR"), "/../.github/workflows")) +} + +fn workflow_files() -> Vec<(String, String)> { + let dir = workflows_dir(); + let entries = + std::fs::read_dir(&dir).unwrap_or_else(|e| panic!("cannot read {}: {}", dir.display(), e)); + + let mut out = Vec::new(); + for entry in entries.flatten() { + let p = entry.path(); + if p.extension().and_then(|e| e.to_str()) != Some("yml") { + continue; + } + let name = p.file_name().unwrap().to_string_lossy().to_string(); + let content = std::fs::read_to_string(&p).expect("workflow is readable"); + out.push((name, content)); + } + assert!(!out.is_empty(), "found no workflows to scan"); + out +} + +/// Directive lines only. The `#` comments in ci.yml document the broken form on +/// purpose, and a naive scan flags the warning as loudly as the mistake. +fn directives(content: &str) -> impl Iterator { + content + .lines() + .enumerate() + .filter(|(_, l)| !l.trim_start().starts_with('#')) +} + +#[test] +fn cancel_in_progress_is_never_an_expression() { + let mut violations = Vec::new(); + + for (name, content) in workflow_files() { + for (i, line) in directives(&content) { + let Some((key, value)) = line.split_once(':') else { + continue; + }; + if key.trim() != "cancel-in-progress" { + continue; + } + let value = value.trim(); + if value != "true" && value != "false" { + violations.push(format!("{}:{}: {}", name, i + 1, line.trim())); + } + } + } + + assert!( + violations.is_empty(), + "cancel-in-progress must be a literal true/false. An expression renders \ + to a string, and any non-empty string is truthy -- so \ + `${{{{ github.ref != 'refs/heads/main' }}}}` cancels main rather than \ + protecting it. Put the condition in the concurrency group instead:\n {}", + violations.join("\n ") + ); +} + +/// The replacement only works if the group actually varies per commit on main. +/// A literal `cancel-in-progress: true` with a per-ref group would cancel main +/// on every push — strictly worse than what this replaced. +#[test] +fn main_runs_cannot_be_superseded() { + let ci = workflow_files() + .into_iter() + .find(|(n, _)| n == "ci.yml") + .expect("ci.yml exists") + .1; + + let group = directives(&ci) + .map(|(_, l)| l) + .find(|l| l.trim_start().starts_with("group:")) + .expect("ci.yml declares a concurrency group"); + + assert!( + group.contains("github.sha"), + "the concurrency group must include github.sha for main, or a second \ + push cancels the first run: {}", + group.trim() + ); + assert!( + group.contains("refs/heads/main"), + "the per-SHA component must be conditional on main, otherwise every \ + branch push gets its own group and force-pushes stop superseding: {}", + group.trim() + ); +} + +/// A scanner that reads nothing passes for the wrong reason. +#[test] +fn the_scan_sees_real_workflow_content() { + let files = workflow_files(); + assert!( + files.len() >= 2, + "expected several workflows, saw {}", + files.len() + ); + assert!( + files.iter().any(|(_, c)| directives(c).count() > 20), + "no workflow yielded a meaningful number of directive lines" + ); + assert!( + files.iter().any(|(n, _)| n == "ci.yml"), + "ci.yml should be among the scanned workflows" + ); +}