Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions .github/workflows/dotnet-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,28 @@ on:
# deliberate act — usually the hand-bisect above — and must never be cancelled by an
# unrelated push landing on main.
concurrency:
# PR runs group by `refs/pull/N/merge`, main pushes by `refs/heads/main` — i.e. both
# group by ref, and only a manual dispatch needs its own group. Kept on ONE line: in a
# folded (`>-`) scalar, continuation lines indented deeper than the first are preserved
# literally rather than folded, so a "tidier" multi-line version silently changes the
# string rather than just its layout.
group: build-test-${{ github.event_name == 'workflow_dispatch' && github.run_id || github.ref }}
# PR runs group by `refs/pull/N/merge`; a manual dispatch gets its own group; and a push to
# `main` groups by its own COMMIT. Kept on ONE line: in a folded (`>-`) scalar, continuation
# lines indented deeper than the first are preserved literally rather than folded, so a
# "tidier" multi-line version silently changes the string rather than just its layout.
#
# 🚨 MAIN GROUPS BY SHA, NOT BY REF — `cancel-in-progress: false` was NOT enough (#2412 was
# incomplete). GitHub keeps exactly ONE PENDING run per concurrency group: `false` protects the
# run that is already EXECUTING, but a third arrival still EVICTS the queued one, and an evicted
# run reports `cancelled` having never run a step. Grouping every main commit under one ref
# therefore reintroduced both failures the note below exists to prevent, silently.
#
# Measured 2026-08-30 (one merge burst, and `run_started_at` == `created_at` even while a run
# is only queued, which is what makes this read as "cancelled mid-run"):
# dca02bdd7 queued 11:42:10 → cancelled 11:53:02, 1s after dfdd2d01c was created
# 57de73c45 queued 11:54:57 → cancelled 12:08:54, 2s after 637ee3921 was created
# 637ee3921 queued 12:08:52 → cancelled 12:09:16, 1s after 0c584a2d5 was created
# Three landed commits untested, and CD published nothing for any of them: ACR's newest image
# stayed several commits behind while three CD runs reported green with every job `skipped`.
#
# A per-SHA group cannot contend with anything, so every commit that lands on main gets its own
# full run — which is precisely the cost the note below says is deliberately accepted.
group: build-test-${{ github.event_name == 'workflow_dispatch' && github.run_id || (github.ref == 'refs/heads/main' && github.sha || github.ref) }}
# 🚨 NEVER cancel a run on `main` (#2412). Superseding is safe on a PR branch — the later
# push tests a strict successor of what was cancelled, and that is where #2316's ~28% of
# runner demand is saved. On `main` it is neither safe nor recoverable, for two reasons:
Expand All @@ -176,6 +192,10 @@ concurrency:
# Measured the day this was written: main's five consecutive runs between 20:28 and 20:38
# were ALL `cancelled`, each by the next merge. The cost of this line is one full run per
# merge to main; that is the price of every landed commit being both tested and shippable.
#
# 🚨 This line is now the BELT, not the braces: with main grouped by SHA above, no two main runs
# ever share a group, so there is nothing for it to cancel. It stays because it is the statement
# of intent, and because it still governs any future ref that groups by name.
cancel-in-progress: ${{ github.event_name != 'workflow_dispatch' && github.ref != 'refs/heads/main' }}

# publish-unit-test-result-action runs per shard AND in the collector — both need
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
---
Name: Every commit that lands on main is tested again — merges no longer evict each other
Category: Fix
Description: Turning off cancel-in-progress was not enough: GitHub keeps only one pending run per concurrency group, so a burst of merges evicted the queued runs and three landed commits went untested and unpublished. Each main commit now gets its own group.
Icon: Bug
Order: -20260830
---

# Every commit that lands on main is tested again

Merge two or three PRs close together and the middle ones were **never built** — and nothing they
contained was ever published. The runs showed up as `cancelled`, which read like someone had
cancelled them.

## What was actually happening

`Build and Test` already declared `cancel-in-progress: false` for `main`, precisely so merges could
not kill each other's runs. That protects the run that is **already executing** — but GitHub keeps
exactly **one pending run per concurrency group**, and every main push shared one group. So the
third merge evicted the second's *queued* run, which then reported `cancelled` without having run a
single step.

Measured on 2026-08-30, in one burst:

| commit | queued | cancelled |
|---|---|---|
| `dca02bdd7` | 11:42:10 | 11:53:02 — 1 s after the next push |
| `57de73c45` | 11:54:57 | 12:08:54 — 2 s after the next push |
| `637ee3921` | 12:08:52 | 12:09:16 — 1 s after the next push |

Both consequences the setting exists to prevent happened anyway. Nothing compiled the tree that
actually landed (each PR is tested against the main it branched from, so the *merged* combination
is first built by main's own run). And nothing shipped: delivery gates on `Consolidate test results`
succeeding **for that commit**, which an evicted run never produces — three CD runs reported green
with every job skipped, while the newest published image stayed several commits behind.

## What changed

A push to `main` now groups by its **own commit**, so no two main runs ever share a group and none
can be evicted. Pull requests still group by ref and still supersede — that saving is worth keeping.

## The guard that missed it, and why

`MainRunsAreNeverCancelledGuard` was green throughout. It asserted the `cancel-in-progress`
expression evaluates false on main — which was true, and beside the point. The invariant is *"a main
run is never cancelled"*, and eviction violates it without touching that setting. The guard now also
evaluates the **group** expression for two different main commits and requires the results to
differ. It was verified by putting the old grouping back and watching it fail.
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,62 @@ public void BuildAndTest_CancelsOnPrBranches_ButNeverOnMain()
+ "is saved. Exclude main specifically, not every ref.");
}

/// <summary>
/// 🅿️ EVERY COMMIT THAT LANDS ON MAIN GETS ITS OWN CONCURRENCY GROUP.
///
/// <para>🚨 <c>cancel-in-progress: false</c> is NOT sufficient, which is why this test exists
/// beside the one above rather than inside it. GitHub keeps exactly ONE PENDING run per
/// concurrency group: <c>false</c> protects the run that is already executing, but a third
/// arrival still EVICTS the queued one, and an evicted run reports <c>cancelled</c> having
/// never run a single step. So while the guard above was green, three commits were cancelled
/// on main inside twenty minutes (2026-08-30: dca02bdd7, 57de73c45, 637ee3921 — each cancelled
/// 1-2 seconds after the next push was created), producing exactly the two failures the guard
/// above says it prevents: nothing built the tree that landed, and CD published nothing.</para>
///
/// <para>The invariant is therefore about the GROUP, not the flag: two different main commits
/// must never share a group, so they can never contend for that single slot. A PR must keep
/// sharing one, because superseding a PR branch is the saving worth having.</para>
/// </summary>
[Fact]
public void BuildAndTest_GivesEveryMainCommitItsOwnGroup_SoNoneCanBeEvicted()
{
var body = File.ReadAllText(Path.Combine(FindRepoRoot(), Workflow));

var group = Regex.Match(body, @"^\s*group:\s*(?<value>.+?)\s*$", RegexOptions.Multiline);
Assert.True(group.Success,
$"{Workflow} no longer declares a concurrency group — this guard can no longer see it.");

var expression = group.Groups["value"].Value;

string GroupFor(string eventName, string @ref, string sha) => EvaluateValue(expression,
new Dictionary<string, string>
{
["github.event_name"] = eventName,
["github.ref"] = @ref,
["github.sha"] = sha,
["github.run_id"] = "1001",
});

var first = GroupFor("push", "refs/heads/main", "aaaaaaaaaaaa");
var second = GroupFor("push", "refs/heads/main", "bbbbbbbbbbbb");

Assert.True(first != second,
$"{Workflow} puts every main commit in ONE concurrency group ('{expression}' yields "
+ $"'{first}' for two different commits). GitHub holds a single PENDING run per group, "
+ "so a burst of merges evicts the queued ones and they report 'cancelled' without "
+ "running a step — the tree that landed is never built, and CD never sees the "
+ "'Consolidate test results' success it gates delivery on. cancel-in-progress: false "
+ "does not prevent this; only a per-commit group does.");

var pr1 = GroupFor("pull_request", "refs/pull/2447/merge", "aaaaaaaaaaaa");
var pr2 = GroupFor("pull_request", "refs/pull/2447/merge", "bbbbbbbbbbbb");

Assert.True(pr1 == pr2,
$"{Workflow} now gives each PR COMMIT its own group ('{expression}'), which disables "
+ "superseding on PR branches — that is #2316's ~28% runner saving handed back. A PR's "
+ "group must depend on the ref only.");
}

/// <summary>
/// 🚨 The evaluator is code, so it gets the same treatment every gate here does: rows that
/// prove it can return BOTH answers, including the inverted expression that defeated the
Expand Down Expand Up @@ -154,6 +210,25 @@ private static bool Evaluate(string raw, IReadOnlyDictionary<string, string> con
return Truthy(value);
}

/// <summary>
/// The same parse as <see cref="Evaluate"/>, but returning the VALUE rather than its
/// truthiness — what a <c>group:</c> expression selects. Interpolated back into the literal
/// text around it, so the comparison is on the group string GitHub would actually key on.
/// </summary>
private static string EvaluateValue(string raw, IReadOnlyDictionary<string, string> context)
=> Regex.Replace(raw.Trim(), @"\$\{\{(?<inner>.*?)\}\}", match =>
{
var tokens = Tokenize(match.Groups["inner"].Value);
var position = 0;
var value = ParseOr(tokens, ref position, context);
if (position != tokens.Count)
throw new InvalidOperationException(
$"could not fully parse the group expression '{raw}' (stopped at token "
+ $"{position} of {tokens.Count}). Extend this evaluator deliberately rather "
+ "than letting the guard pass on an expression it does not understand.");
return value as string ?? value.ToString()!;
}, RegexOptions.Singleline);

private static List<string> Tokenize(string text)
{
var tokens = new List<string>();
Expand Down Expand Up @@ -205,7 +280,11 @@ private static object ParseOr(List<string> tokens, ref int position, IReadOnlyDi
{
position++;
var right = ParseAnd(tokens, ref position, context);
left = Truthy(left) || Truthy(right);
// 🚨 Actions' `||` returns an OPERAND, not a bool: `a || b` is `a` when `a` is truthy,
// else `b`. Collapsing it to a bool was harmless while this evaluator only ever read
// `cancel-in-progress` (which Truthy()s the result anyway) — but `group` SELECTS a
// string with exactly these operators, so the value has to survive.
left = Truthy(left) ? left : right;
}
return left;
}
Expand All @@ -217,7 +296,8 @@ private static object ParseAnd(List<string> tokens, ref int position, IReadOnlyD
{
position++;
var right = ParseComparison(tokens, ref position, context);
left = Truthy(left) && Truthy(right);
// Actions' `&&` returns an OPERAND too: `a && b` is `b` when `a` is truthy, else `a`.
left = Truthy(left) ? right : left;
}
return left;
}
Expand Down
Loading