Skip to content

A guard miss fails the pull request in about 6 minutes and skips every shard - #5471

Merged
erikdarlingdata merged 11 commits into
devfrom
fix/5459-guard-stage
Oct 7, 2026
Merged

erikdarlingdata merged 11 commits into
devfrom
fix/5459-guard-stage

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Fixes nothing alone. This is change 1 of #5459 (guard classes first, fail fast).

What a PR author sees now

A pull request that breaks a guard (a stacked doc comment, an untagged census class, a bad pin) now fails in about 10 minutes instead of after the whole run. A new job, "Guard tests", runs first and runs only the classes tagged [Trait("Stage", "Guard")] in both test suites. While it runs, the six Darling PG shards, the four Lite shards, the Linux build and the whole-tree guards wait. If it fails, they are skipped, build fails on its second step, and the two result checks fail with "Guard tests failed". Before this, one stacked summary left every shard running to the end.

The results of push, merge queue and release runs are unchanged, but those runs now wait for the guard job before the shards start (see the L3 note below). They run everything whatever the guards say (dev pushes stay the safety net and the Lite timing source).

Required checks and how each still reports

Exact names from the branch protection list: build, Darling PostgreSQL tests, Lite tests (result), Claude review guard.

  • build: needs [gate, guard-tests], still if: !cancelled(). New second step "Fail when the guard tests failed" (pull request only, before checkout) fails with an ::error naming the Guard tests job. When it fires, runs-on picks ubuntu-latest, so no Windows runner starts. A release runs as before (the guard job is skipped there).
  • Darling PostgreSQL tests and Lite tests (result): needs [gate, guard-tests, <matrix>]. Arms in order: gate failed, declined (draft or merge-train, unchanged text), then new "Guard tests failed" (not success, except skipped on a release), then the matrix result.
  • The skipped matrix arm used to print a notice and exit 0 (a skipped matrix read as a pass). It is now an error in both result jobs.
  • Claude review guard is another workflow and is untouched.

By-design skips stay green. I listed every if: on the shard jobs. The shards are skipped on purpose only when: the gate declined the run (draft or merge-train label; the declined arm reports that as a failure on a pull request as it always did), the guards failed on a pull request (new, an error), and for the Lite shards a release (the Lite result job has its own if: that excludes a release, so it never reads that skip). darling-pg and darling-linux have no other skip reason; their path filters skip steps, not the job. So no by-design skip reaches the new error arm. GuardStageWorkflowTests.TheShardJobs_AreSkippedOnlyForThePlannedReasons pins the exact if: of each shard, so a new skip cause is a deliberate edit.

Docs-only pull requests: all four required checks still report success. The proof is the YAML pins: the guard job's filter is a pinned copy of build's docs and all entries and every step after its decision step is gated on run == 'true', so it succeeds having built nothing (ADocsOnlyChange_LeavesTheGuardJobGreenHavingBuiltNothing); the shards' own filters and build's docs fast path are unchanged; and the guard arm accepts success. No separate docs-only proof run was done.

Merge-ref note

pull_request runs use build.yml from the merge ref (the head merged into dev). Every open PR's next NEW run (a push or a ready event, not a re-run of an old run) picks wave 1 up, and the census then flags any untagged guard-style class that PR adds.

What changes

  • .github/workflows/build.yml: the guard-tests job; needs/if on build, darling-pg, darling-linux, lite-tests, darling-tree-guards (same fail-fast, so it does not start a runner) and both result jobs; the new build step; GuardStageScanner.cs added to the lite_linked_shard filter.
  • Darling/Darling.Tests/GuardStageScanner.cs (new, linked into Lite.Tests): reads each test class's source: name, trait, live-postgres collection, recursive repo-root enumeration, DARLING_TEST_ variable reads and store helpers.
  • GuardStageCensusTests in each suite (rules R1 name, R2 repo-root walker, R3 no store; a pinned exemption list) and GuardStageWorkflowTests (R4 workflow pins).
  • [Trait("Stage", "Guard")] on 99 Darling.Tests classes and 39 Lite.Tests classes (script-tagged, one line each, reviewed by git diff --stat and spot checks). Guard-trait class counts from -list classes/json -trait Stage=Guard: Darling.Tests 102, Lite.Tests 40 (each includes the new census).
  • LockedModeRestoreCoverageTests now picks the build job's restore step by name ("Restore dependencies") instead of by "the only step with set -euo pipefail", because the guard job has a second one. RepoFileAdoptionTests lists the new workflow pin as an LF reader.
  • W1 removes no class from any run: the shards' class lists are unchanged, so guard classes run twice (about 4 test-minutes extra) and no corpus replay is needed.

Exemption list (Darling.Tests, 6; Lite.Tests, 0; size pinned, may only shrink):

  • AlertDeliveryChannelTests (walks the repo but also reaches the shared store through a helper)
  • DarlingEmptyEnumerationInventoryTests (live-postgres collection, reads DARLING_TEST_PG)
  • DarlingOwnedSecretsCensusTests (sets and reads its own DARLING_TEST_OIDC_SECRET_OWNEDSET)
  • DarlingPgRuntimeVersionPinTests (gated on DARLING_TEST_PGRUNTIME)
  • StoreApplicationNamePinTests (reads DARLING_TEST_PG for its scratch database)
  • WebExceptionTextCensusTests (live-postgres collection, reads DARLING_TEST_PG)

Seat tools (plan ruling 8): the seat's (Darling|Lite)\.Tests +Total: grep will also match the Guard tests job's two subset lines; read the job name with each. Shard totals do not change in W1.

Why the shards wait for the guard job

The shards list Guard tests in needs:. We weighed running the guards next to the shards and cancelling the run when a guard fails, and kept needs: for three reasons.

  • The next change makes the wait nearly free. Change 2 of CI costs about 110 runner-minutes for every code PR, whatever it changes: run the guard tests first, build the test projects once, and choose test classes by what changed #5459 builds the test projects once, in the guard job. The shards reuse that build, so they wait for it anyway. The guard tests then add only their own 1-2 minutes of test time.
  • Waiting is cheaper when a guard fails. With needs:, a guard failure costs about 6 runner-minutes in all (run b: 6.18). With a cancel, each shard has already run for about 6 minutes when the guards fail. That run costs about 60 runner-minutes.
  • A cancel can leave a required check pending forever. Cancelling the run needs the actions: write permission. It can also stop a required result check from ever being created, unless every result job uses always(). In that case the pull request waits on a check that never reports.

Guard job runtime

Locally (Debug, one machine): Darling.Tests guard set 1282 tests in about 36 s, Lite.Tests guard set 364 tests in about 22 s. On the proof run the job took 5 min 53 s end to end (14:42:06 to 14:47:59): Darling.Tests tests about 35 s, Lite.Tests about 14 s, the rest setup, locked restore and two Release builds.

Cost

Measured on the proof run of this change: 154.5 runner-minutes and 30 minutes wall. A comparable full run without the guard job on the same day took 148.6 runner-minutes and 21 minutes. That is +6 runner-minutes (+4%) and +9 minutes wall. Change 2 of #5459 makes the guard job the one build every shard reuses, which removes most of the wall-clock cost.

Test plan

  • Build both test projects: 0 errors, 0 warnings in files I touched.
  • -trait Stage=Guard Darling.Tests: Total 1282, Failed 0. Lite.Tests: Total 364, Failed 0.
  • GuardStageCensusTests Darling: Total 5, Failed 0. Lite: Total 5, Failed 0.
  • GuardStageWorkflowTests Total 7, Failed 0; reverting one shard if: made TheShardJobs_AreSkippedOnlyForThePlannedReasons fail (mutation proof). The census failed with 99 MISSING lines before tagging.
  • YAML pins Darling.Tests: CiShardCommandLineBudgetTests, DarlingPathFilterGateTests, CiClusterWorkerSizingTests, NightlyVersionInjectionTests, ReleaseStatementGateWorkflowTests, SignPathActionVersionTests, PasswordKeySelfCheckTests, DrillDownDopProvenanceParityTests, ParameterSensitivityFiringSignatureParityTests, QueryHighDopStaleMaxDopParityTests, ViewerSidebarDotRendersTheCardStatusTests, LockedModeRestoreCoverageTests: Total 90 (without LockedMode, which is in the guard set), Failed 0. Lite.Tests: CrossAppGuardCiGateTests Total 19 Failed 0; DarlingReadsTraitGuardTests, LaneAxisAlignerWiringTests, LiteRuntimePrerequisiteDocsTests, ThemeParityLiteDarlingTests, WatermarkPolicyTests Total 57 with CrossApp's Failed 0 after the fix below.
  • *Census* Darling.Tests: Total 445, Failed 0, Skipped 1. *Ratchet*: Total 15, Failed 0.
  • Fixed in this PR: CrossAppGuardCiGateTests.EveryCrossAppSourceRead_IsReachableByTheFilterThatGatesItsSuite failed when a Darling.Tests pin named the Lite project path; the pin matches the project file name without its folder instead.
  • YAML parses (PyYAML), job graph and needs/if printed and checked.
  • Full suites not run (CI decides).
  • Proof runs: see below.

Proof runs

Run a (the draft run, declined by the gate): build, Darling PostgreSQL tests and Lite tests (result) red with "CI not run" within 6 s; Guard tests, Darling PG tests, Lite tests, Darling Linux build and the whole-tree guards skipped.

Run b (marked ready, with one planted stacked <summary> in a Darling test file): started 14:41:54.

  • Guard tests: failed at 14:47:59 (6 min 5 s after run start). The log shows DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocks failed (Darling.Tests Total 1282, Failed 1) and Lite.Tests Total 364, Failed 0. Listed classes: 102 Darling.Tests, 40 Lite.Tests.
  • build: failed 14:48:09 on its new second step "Fail when the guard tests failed" (6 min 15 s after run start). Darling PostgreSQL tests failed 14:48:07 and Lite tests (result) failed 14:48:05 (about 6 min 10 s after run start), both with "Guard tests failed".
  • Shard jobs: all skipped (Darling PG tests, all six shards, one matrix entry on the jobs list; Lite tests, all four shards, one matrix entry; Darling Linux build; the whole-tree guards job).
  • Runner-minutes of the run (plan line 67 query): 6.18, against the plan's 10-15 estimate and the 109 median of a full run.
  • Claude review guard is another workflow and was not part of this measurement.

Run c (the revert commit, pushed after run b finished, no force-push) is for the seat to watch: expect green, about +7 runner-minutes and +4-5 min wall against the 109 median.

Review round 1

  • M1, the census and xunit disagreed about which classes are tagged. Both GuardStageCensusTests now also read the tagged set off the loaded test assembly with reflection. It sees class, base class and method traits, and trait names spelled with constants, on public concrete classes with a test. A class xunit selects that the source scan does not see as tagged fails. So does a class the scan reads as tagged that xunit does not select. Rule R3 (no store) runs over the reflected set too, including the live-postgres collection. The scan now also reads the files a csproj compiles in through a Link, which is what hid CrossSkuSurfaceSourceTests from the Darling census. Nothing was loosened and no exemption was added. Before the scanner change the new check failed on exactly CrossSkuSurfaceSourceTests (Darling.Tests, 1 of 9 failed). After it, 9 of 9 pass in each suite. Detector tests pin each shape (method-level trait, constants, inherited trait, live collection, phantom, unscanned) with private probe classes xunit does not run.
  • M1, a store test that skips passed the guard job. The guard job now runs both suites with -failSkips (spelling checked against the runner's help). A tagged test that skips for lack of a store now turns the job red. Both suites skip 0 in the guard set today (Darling.Tests 1288 run, Lite.Tests 368 run, all passing with the switch). GuardStageWorkflowTests pins the switch on the single test-running line.
  • L1. The guard loop sets $PSNativeCommandUseErrorActionPreference = $false explicitly, like the three other loops, with a comment. Pinned (set after $ErrorActionPreference, before the loop).
  • L2. The darling-tree-guards header no longer says it always reports a result so it can be a required check. It now says it skips on a pull request whose guards failed, and must not be made a required check without reading that skip. Pinned.
  • L3, no YAML change. On push and merge-queue runs the shards also wait for the guard job (about 6.5 minutes). Those events ignore its result. Change 2 makes the guard job the one build every shard reuses, so the wait then replaces the shards' own builds.

Review round 1 test totals (Total / Failed):

  • GuardStageCensusTests 9/0 in each suite. GuardStageWorkflowTests 9/0.
  • DarlingPathFilterGateTests 8/0. CiShardCommandLineBudgetTests 7/0. LockedModeRestoreCoverageTests 25/0. RepoFileAdoptionTests 2/0. CrossAppGuardCiGateTests (Lite.Tests) 19/0.
  • -trait Stage=Guard -failSkips: Darling.Tests 1288/0, Lite.Tests 368/0.
  • Every *Census* class in Darling.Tests 449/0 (1 skipped, a live class with no store here). Every *Ratchet* class 15/0.
  • Both test projects build with 0 warnings.
  • Not run here: the full suites, and the workflow itself. CI's own run on this push is the proof.

SECTION: None
ENTRY: - CI only ([#5471]) - no user-visible change.

Part of #5459.

Review round 2

  • Low 1, the reflection check saw fewer methods than xunit. GuardStageScanner.ReflectGuardClasses now takes types from GetExportedTypes(). It accepts a class that is not abstract or is sealed, so a static class counts. It binds test methods as Instance, Static, Public, NonPublic with FlattenHierarchy. That is xunit v3's own discovery. Each suite's probe test got three new probes. One is a static method with a method-level Stage=Guard trait. One is a private method with the same trait. One is a static class with a class-level trait. Before the fix the probe test failed on the static method ("a static test method with a method-level trait is not seen"). With only the method binding fixed it failed on the static class ("a static class is not seen"). After both fixes it passes.
  • Low 2, substring pins. In GuardStageWorkflowTests, the $PSNativeCommandUseErrorActionPreference = $false pin is now a multiline regex that needs an uncommented statement on a line of its own. The switch pin is a regex that needs -failSkips followed by whitespace or the end of the line. I planted two bad lines in build.yml and then restored it. A commented-out preference line failed the pin with the new message. -failSkips- failed it too. The unchanged file passes.
  • PR body. The "unchanged" sentence now says the results are unchanged and the runs wait for the guard job. The Cost section above is new.
  • I merged origin/dev first. It had no guard file or workflow conflicts.

Review round 2 test totals (Total / Failed):

  • GuardStageCensusTests: Darling.Tests 9/0, Lite.Tests 9/0. GuardStageWorkflowTests 9/0.
  • -trait Stage=Guard -failSkips: Darling.Tests 1288/0, Lite.Tests 368/0.
  • Darling.Tests: every *Census* class 449/0 (1 skipped: a live class that needs a store). -failSkips counts that skip as a failure, so these classes ran without the switch, *Ratchet* 15/0, *Hygiene* 90/0.
  • Lite.Tests: *Census* 55/0. There is no *Ratchet* or *Hygiene* class in Lite.Tests (Total 0).
  • Both test projects build with 0 warnings.
  • Not run here: the full suites and the workflow itself. CI's run on this push is the proof.

Dev sync

Merged dev; tagged the guard classes dev added since the base. The census named four: ProcedureTrendHourlyQueryViewPinTests, ProcedureTrendQueryViewPinTests, ScratchPostgresQuiesceCensusTests (Darling.Tests) and ProcedureTrendQueryPinTests (Lite.Tests). None needs a store, so each got [Trait("Stage", "Guard")] and none was exempted. Before the tags, GuardStageCensusTests failed 1 of 9 in each suite; after, 9/0 in each. A second merge of dev brought in no new class.

Totals after the final merge (Total / Failed): -trait Stage=Guard -failSkips Darling.Tests 1303/0, Lite.Tests 372/0. GuardStageWorkflowTests 9/0. ScratchPostgresQuiesceCensusTests 4/0. *Census* Darling.Tests 453/0 (1 skipped live class), Lite.Tests 55/0. *Ratchet* 15/0, *Hygiene* 90/0 in Darling.Tests. Both test projects build with 0 warnings. The .github diff against dev is only this PR's build.yml change, and dev's worker-crash warning step is still in both workflow files.

… about ten minutes (#5459)

A new Guard tests job runs the Stage=Guard classes of both test suites before any shard.
On a pull request the Darling PG shards, Lite shards, Linux build and whole-tree guards start
only after it succeeds, the build job fails at once, and the two result jobs report the
failure; a skipped matrix is now an error instead of a pass. About 140 existing guard
classes carry the trait, and a census per suite keeps new ones honest.

Part of #5459.
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 7, 2026 14:41
@erikdarlingdata erikdarlingdata changed the title A guard miss fails the pull request in about 10 minutes and skips every shard (#5459) A guard miss fails the pull request in about 6 minutes and skips every shard Oct 7, 2026
…fails skips, and its loop and header are corrected (#5459)

Review round 1 of #5471. M1: both censuses compare the source scan with a
reflection read of the test assembly (class, method, constant, inherited and
linked-file traits), apply the no-store rule to that set, and the scan now
reads files a csproj links in. The guard job runs both suites with -failSkips.
L1: the guard loop sets the native-command preference explicitly. L2: the
darling-tree-guards header no longer promises a result on every event.
@erikdarlingdata
erikdarlingdata marked this pull request as draft October 7, 2026 15:42
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 7, 2026 15:42
… and the workflow pins are anchored (#5459)

The census reflection now reads exported types, static classes, and static and non-public test methods, as xunit v3 does, with probes for a static method, a private method and a static class. The workflow pins match an uncommented PSNativeCommandUseErrorActionPreference line and the whole -failSkips switch.
@erikdarlingdata
erikdarlingdata merged commit 4a2519b into dev Oct 7, 2026
23 of 24 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5459-guard-stage branch October 7, 2026 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant