Skip to content

Commit da3b71d

Browse files
committed
ci: leave the Windows recovery suites to the required test lane
This lane's pull-request filter named every source directory in the workspace closure of the crash and owner-death tests it runs, so it took a Windows runner on 118 of the last 200 merges. The filter was accurate — those suites really do import most of `runtime`, `runtime-host`, `storage` and `core`, and a generated import closure measured at 106 of the same 200, because they boot a real Runtime Host. The list was not the problem. What the run history shows is that the pull-request trigger was. Across 300 runs, 241 of them pull requests, this lane produced 12 reds and not one of them was unique: every pull-request failure sat on a commit whose `test` run had already failed, usually at the very same step — six at `Install dependencies`, one at the workspace tests that run the same owner-death suite on Linux. Its single unique catch in that window was a Windows-only NTFS alternate stream regression found by the unfiltered main push, which is the trigger that exists for exactly that. So the filter now names only what a Windows runner can prove and `test` cannot: how `npm ci` resolves and what the dependency patches and the Electron installer produce there, what the clean step removes there, and the Local IPC trust boundary, a PowerShell script with no other caller. That is 14 of 200 merges instead of 118. The install and build steps stay unconditional, because proving those suites still build and run on Windows is what the lane is for once it does run. The recovery authorities are now covered after merge rather than before it, by the unfiltered main push minutes later and by the nightly. `windows_recovery` is deliberately not a required context in `.asf.yaml`, so it was never what stood between a regression and `main` in any case. Generated-by: Claude Code
1 parent d931119 commit da3b71d

2 files changed

Lines changed: 42 additions & 43 deletions

File tree

.github/workflows/windows-recovery.yml

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,24 +17,31 @@
1717

1818
name: Windows recovery
1919

20-
# The paths below are a pre-filter, not this lane's real input. The real input
21-
# is the import closure of the crash and owner-death recovery authorities in
22-
# storage, runtime and Runtime Host, which reaches well past any list worth
23-
# hand-maintaining. So they name those workspaces and every workspace they hold
24-
# a TypeScript project reference to, plus the manifests, patches and scripts
25-
# the unconditional install and clean steps consume. That keeps a change there
26-
# reported before merge, and the nightly run covers the transitive edits the
27-
# list cannot match.
20+
# The paths below name only what a Windows runner can prove and the required
21+
# `test` lane cannot: how `npm ci` behaves here, what the dependency patches and
22+
# the Electron installer produce here, and the Local IPC trust boundary, which
23+
# is a PowerShell script with no other caller.
2824
#
29-
# GitHub evaluates a path filter against the first 300 files of the diff only,
30-
# so a pull request wider than that can skip the filter outright. A repository-wide sweep is exactly the change that touches every
31-
# recovery authority at once, which is one more reason the main push below
32-
# carries no filter at all.
25+
# They deliberately do not name the recovery authorities themselves. Those used
26+
# to be here, as every source directory in the workspace closure of the crash
27+
# and owner-death tests below, which put this lane on 118 of the last 200
28+
# merges. Over 300 runs it never once produced a pull-request red that the
29+
# `test` lane had not already produced on the same commit, usually at the very
30+
# same step: the recovery suites are ordinary TypeScript, so a defect in them
31+
# fails on Linux first and blocks the merge there. Its one unique catch in that
32+
# window — a Windows-only NTFS alternate stream regression — came from the main
33+
# push below, which is what that trigger exists for.
34+
#
35+
# So the authorities are covered after merge rather than before it, minutes
36+
# later, by a push run that carries no filter at all. That trigger is also
37+
# unfiltered because `required_status_checks` is `strict: false`, so a pull
38+
# request can go green against a stale base, and because GitHub evaluates a path
39+
# filter against the first 300 files of a diff only — and a repository-wide
40+
# sweep is exactly the change that touches every recovery authority at once.
3341
on:
3442
pull_request:
3543
branches: [main]
3644
paths:
37-
- 'package.json'
3845
- 'package-lock.json'
3946
- 'patches/**'
4047
- 'scripts/apply-dependency-patches.mjs'
@@ -43,21 +50,6 @@ on:
4350
- 'scripts/clean-build.mjs'
4451
- 'scripts/clean-paths.mjs'
4552
- 'scripts/windows-runtime-host-local-ipc-trust.ps1'
46-
- 'tsconfig.base.json'
47-
- 'tsconfig.lib.json'
48-
- 'packages/core/package.json'
49-
- 'packages/core/tsconfig.json'
50-
- 'packages/core/src/**'
51-
- 'packages/storage/package.json'
52-
- 'packages/storage/tsconfig.json'
53-
- 'packages/storage/src/**'
54-
- 'packages/runtime/package.json'
55-
- 'packages/runtime/tsconfig.json'
56-
- 'packages/runtime/src/**'
57-
- 'packages/runtime/scripts/**'
58-
- 'packages/runtime-host/package.json'
59-
- 'packages/runtime-host/tsconfig.json'
60-
- 'packages/runtime-host/src/**'
6153
- '.github/workflows/windows-recovery.yml'
6254
# Unfiltered on purpose: required_status_checks is `strict: false`, so a pull
6355
# request goes green against a stale base and only the merged result proves

scripts/ci-test-plan.test.mjs

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -687,29 +687,38 @@ test('the recovery lane keeps every run kind out of one shared concurrency group
687687
assert.match(workflow, /\n {2}cancel-in-progress: true/u);
688688
});
689689

690-
test('the recovery lane filters pull requests by the workspaces its steps execute', () => {
690+
test('the recovery lane leaves the suites it executes to the required test lane', () => {
691691
const workflow = readWorkflow('windows-recovery.yml');
692692
const filtered = new Set(pullRequestPathFilter('windows-recovery.yml'));
693693

694694
// Derived from the dist paths the steps run, then widened along the workspace
695695
// dependency graph the planner selects with. The separator class matches the
696696
// backslash form too, because these steps run under pwsh where both are
697-
// legal. A new workspace on this lane, or a new dependency under one of them,
698-
// fails here until the filter admits its sources and project file.
697+
// legal.
699698
const executed = [
700699
...new Set(
701700
[...workflow.matchAll(/packages[/\\]([^/\\]+)[/\\]dist[/\\]/gu)].map((match) => match[1]),
702701
),
703702
].sort();
704703
assert.deepEqual(executed, ['runtime', 'runtime-host', 'storage']);
705704

705+
// None of that closure belongs in the filter. These suites are ordinary
706+
// TypeScript, so `test` runs them on Linux on every pull request and fails
707+
// first; naming their sources here only bought a second, slower red. Listing
708+
// one again would put this lane back on most merges, so it fails here.
706709
const closure = dependencyClosure(executed.map((workspace) => `packages/${workspace}`));
707710
assert.ok(closure.includes('packages/core'), 'dependency closure must reach core');
708711
for (const dir of closure) {
709-
assert.ok(filtered.has(`${dir}/src/**`), `${dir}: sources`);
710-
assert.ok(filtered.has(`${dir}/tsconfig.json`), `${dir}: project file`);
711-
assert.ok(filtered.has(`${dir}/package.json`), `${dir}: manifest`);
712+
for (const entry of [`${dir}/src/**`, `${dir}/tsconfig.json`, `${dir}/package.json`]) {
713+
assert.ok(!filtered.has(entry), `${entry} belongs to the required test lane`);
714+
}
712715
}
716+
717+
// What the Windows runner proves instead is that the suites still build and
718+
// run here at all, which is why the unconditional install and build steps stay
719+
// unconditional even though nothing in the filter names a workspace.
720+
assert.match(workflow, /\n {6}- name: Install dependencies\n {8}run: npm\.cmd ci\n/u);
721+
assert.match(workflow, /\n {8}run: npm\.cmd run build:test\n/u);
713722
});
714723

715724
test('the recovery lane filter follows the postinstall launcher chain', () => {
@@ -732,25 +741,23 @@ test('the recovery lane filter follows the postinstall launcher chain', () => {
732741
}
733742
});
734743

735-
test('the recovery lane filters pull requests by what its install and clean steps consume', () => {
744+
test('the recovery lane filters pull requests by what only Windows can prove', () => {
736745
const filtered = new Set(pullRequestPathFilter('windows-recovery.yml'));
737746

738-
// `npm.cmd ci` and `npm.cmd run build:test` run unconditionally, so these are
739-
// first-class inputs of the lane rather than transitive edits the nightly can
740-
// be left to cover. A grouped dependabot bump touches only the manifests, and
741-
// the crash gates sit on a native file lock the Linux `test` lane never sees.
747+
// What is left after the workspace sources came out: how `npm.cmd ci` resolves
748+
// and what it produces on Windows, what `npm.cmd run build:test` cleans up
749+
// there, and a PowerShell script this lane is the only caller of. Each of
750+
// these can be green on Linux and red here, which is the whole test for
751+
// membership in this list.
742752
for (const path of [
743-
'package.json',
744753
'package-lock.json',
745754
'patches/**',
746755
'scripts/apply-dependency-patches.mjs',
747756
'scripts/install-electron-with-retry.mjs',
757+
'scripts/run-electron-installer.cjs',
748758
'scripts/clean-build.mjs',
749759
'scripts/clean-paths.mjs',
750760
'scripts/windows-runtime-host-local-ipc-trust.ps1',
751-
'tsconfig.base.json',
752-
'tsconfig.lib.json',
753-
'packages/runtime/scripts/**',
754761
'.github/workflows/windows-recovery.yml',
755762
]) {
756763
assert.ok(filtered.has(path), path);

0 commit comments

Comments
 (0)