diff --git a/.claude/skills/duckdb/SKILL.md b/.claude/skills/duckdb/SKILL.md index 77a8a3b0ed..bd6a91db6d 100644 --- a/.claude/skills/duckdb/SKILL.md +++ b/.claude/skills/duckdb/SKILL.md @@ -7,23 +7,22 @@ disable-model-invocation: false # Query Lite's DuckDB Database -Safely query the PerformanceMonitor Lite DuckDB database using Python. This skill exists because DuckDB only allows one read-write connection at a time. When Lite is running (collecting data), it holds the write connection. External readers MUST connect in read-only mode. +Safely query the PerformanceMonitor Lite DuckDB database using Python. This skill exists because DuckDB only allows one read-write connection at a time. When Lite is running (collecting data), it holds the write connection, so an external reader connects in read-only mode. ## Database Location -Since the #1832 fix (data moved OUT of Velopack's install root, which Setup.exe deletes), the store lives at: +The store lives at: ``` %LOCALAPPDATA%\PerformanceMonitorLite-Data\monitor.duckdb ``` -Builds from BEFORE that fix (3.3.0 and earlier) keep it at the old path, and the file only moves when a fixed build first runs: +Lite 3.3.0 and earlier kept it at the path below, and the file moves to the path above only when a newer build first runs. Check the path above first, and fall back to this one when it does not exist: ``` %LOCALAPPDATA%\PerformanceMonitorLite\monitor.duckdb ``` -**Check the -Data path first; fall back to the old path if it does not exist.** -## How to Query (ALWAYS use this pattern) +## How to Query -Use Python with the `duckdb` module. **ALWAYS connect in read-only mode.** +Use Python with the `duckdb` module, always with this pattern: ```python python -c " @@ -40,13 +39,12 @@ con.close() " ``` -## CRITICAL RULES +## Rules -1. **ALWAYS use `read_only=True`** -- without this, the connection will be blocked by Lite's write lock and hang or fail -2. **NEVER use `duckdb.connect()` without `read_only=True`** -- the default is read-write which WILL conflict with the running app -3. **Use forward slashes** in the path (Python on Windows handles this fine) -4. **Close the connection** when done -- don't leave read locks dangling -5. **Use Python** -- duckdb 1.4.4 is installed +1. **Pass `read_only=True` to `duckdb.connect()`.** The default is read-write. Lite's write lock blocks a read-write connection, and it hangs or fails. +2. **Use forward slashes** in the path (Python on Windows handles this fine) +3. **Close the connection** when done, so no read lock is left dangling +4. **Use Python** with its `duckdb` module ## Common Queries diff --git a/.claude/skills/lane-orders/SKILL.md b/.claude/skills/lane-orders/SKILL.md index d70db08784..34c697fc1a 100644 --- a/.claude/skills/lane-orders/SKILL.md +++ b/.claude/skills/lane-orders/SKILL.md @@ -18,8 +18,6 @@ conflict. - ``. If your brief leaves one unfilled, don't invent a value. Skip the step that needs it and say so in the PR body. -In wave E (2026-09-24), a lane whose brief left `` unfilled wrote its changelog entry to a -made-up path inside the repo. Isolated worktrees live under `.claude/worktrees/` inside the repo, so a repo-scanning test must not exclude that path. @@ -28,7 +26,7 @@ path. Before dispatch: - Rank the queue and get the OK the guardrails require. -- Post every ruling on its issue AND put it in the first prompt. +- Post every ruling on its issue and put it in the first prompt. - Pre-approve any store migration by name. Every other lane stops and reports instead. - Fast-forward the main checkout's local `dev`. A `lane` agent loads this skill from that checkout, and a stale checkout can lack it. @@ -42,16 +40,23 @@ At dispatch: fixes what the review found. - Give each lane its own rig port. - Set a wall-clock check at each lane's deadline; don't wait on notifications. -- Size each lane to finish under 200k. In wave B (2026-09-23), a lane's context grew about 1.9k tokens per call - from a 40k base, so 200k is about 80 calls. A #3898 lane that converted three tools used 100-117 calls. - In wave C (2026-09-24), the base fell to 24k, but a lane with two twin pairs (two tools on both products) still - used 103-167 calls and peaked at 214-242k. Give a #3898 lane ONE twin pair, or two tools on one product. -- Check that the effort level took effect: grep a lane transcript for `"effort":`. In wave C, `effort: high` in the - agent file did not apply, and every lane turn logged the seat's `max`. -- Every head clause about a status must keep its condition's scope. In wave C, each head-fact fix the coordinator - made was a scope error: "nothing collected yet" for "none collected in the window", or one state left out. +- Measure a field number before a plan depends on it. A design built on a derived figure (a lag, a row count, a + timing) gets that figure measured first, read-only, by whoever has access to the field. #6254's plan went through + seven revisions on a lag figure nobody had measured; a one-hour measurement answered it. +- Stack or serialize PRs that touch the same files instead of running them in parallel. Each parallel PR on the same + files needed a conflict fix and another full CI run. +- Give browser and click-through checks their own small lane, apart from the code change. Screenshots and page + reads fill a lane's context fast. +- Size each lane to finish under 200k. A lane's context grows about 1.9k tokens per call from a 24-40k base, so + 200k is about 80-90 calls. A lane that converts three tools uses 100-117 calls. A lane with two twin pairs (two + tools on both products) uses 103-167 calls and peaks at 214-242k. Give a lane that converts tools one twin pair, + or two tools on one product. +- Check that the effort level took effect: grep a lane transcript for `"effort":`. An `effort:` set in the agent + file does not always apply, and a lane can then run at the seat's level. +- Every head clause about a status must keep its condition's scope: "none collected in the window" is not + "nothing collected yet". Leave no state out. - Paste into the brief the few lines a lane needs (an example, a ruling) instead of pointing it at a PR or file. - In wave B, every lane read an 8-10k-character PR section to see one example. + A pointer costs every lane a read of 8-10k characters to find one example. - Label each issue the lane takes `in-progress` (`gh issue edit --add-label in-progress`), so the board shows it as taken; remove the label when the issue closes or the work stops. - Before dispatching several lanes for one issue, create its issue branch @@ -64,17 +69,17 @@ At each report: 1. Stop the agent. 2. Check that the shared checkout is clean. 3. Read the PR's core product diff against the checklist below. Lane PRs arrive as drafts, which can't be merged - from the UI. On 2026-09-23, three PRs opened ready were merged before this step finished, and one of them - put a regression on dev. + from the UI, but a lane can open one ready instead, so read the diff before anything can merge. 4. A security PR, or an irreversible data rewrite, gets a review round before it leaves draft. 5. Mark it ready (`gh pr ready `) and arm auto-merge only after that. Auto-merge waits for CI. At each wave boundary, backtrack every issue number the wave filed or touched: - close what is done; - give every open issue a disposition comment and a work-order rank; -- fix the `in-progress` labels. -A wave that files follow-ups faster than it closes them hides its own backlog. On 2026-09-23 a backtrack of -#3898-#4043 found 22 stale `in-progress` labels and three worked issues missing one. +- fix the `in-progress` labels; +- run `pwsh -NoProfile -File .claude/skills/lane-orders/rig-sweep.ps1` and act on its summary: it stops the rigs + whose lane is gone, and lists unregistered `postgres.exe` postmasters for you to check by PID (#5787). +A wave that files follow-ups faster than it closes them hides its own backlog. After each wave, measure it with `lane-audit.py`, as the guardrails' "Measure every wave" section says. Record the WAVE line, and the defects you fixed in each PR, in the handoff. @@ -82,8 +87,7 @@ WAVE line, and the defects you fixed in each PR, in the handoff. When a PR's required check fails on a test it doesn't touch, don't just re-run it. Take a census of the last two days' failed first attempts: `gh run list --workflow Build`, each run's attempts through the REST `actions/runs//attempts//jobs` endpoint, and a `[FAIL]` grep of each failed job's log. Fix every flake it -finds, in one PR. A flake that stalls every lane outranks the rest of the queue (2026-09-23: four flakes in one -census, one of them introduced by that night's own wave). +finds, in one PR. A flake that stalls every lane outranks the rest of the queue. **Verification checklist** (the invariants agents most often miss): - **Storage precision:** PostgreSQL and DuckDB keep microseconds; a .NET tick is not stored. @@ -102,11 +106,11 @@ census, one of them introduced by that night's own wave). - Read files by offset/limit. Never read a whole large file. - grep prints whole lines. Pipe it through `cut -c1-200` in files with long lines: each MCP `Description(...)` is one line of 1-3k characters. -- Run targeted test classes while iterating. Run the FULL suite ONCE, at the end. +- Run targeted test classes while iterating. Run the full suite once, at the end. - Cap command output: pipe through `head`, `tail -n` or `grep`. Never dump full CI logs, full JSON, or whole issue/PR bodies. - Plan to finish before your watchdog's first warning, at 200k. Past about 150k, start no new item: finish the - one in hand, then the finish phase. On 2026-09-23, 40% of lane cost came after that warning. + one in hand, then the finish phase. Each call past that point costs more than any call before it. - Never end your turn to wait for a background notification. Run the full suite in the foreground (about 10 minutes), or poll your own output file. - Put your full report in the PR body (see "Final report"). Your last message is 400 words or fewer and links the @@ -115,9 +119,7 @@ census, one of them introduced by that night's own wave). - Before you call a failure pre-existing, re-run it alone on a freshly created database, after merging `origin/dev`. If it passes there, the cause was your database or your stale base, not the code; don't file it. If it still fails, check that dev's own latest CI run passes it. If dev passes, the failure is yours, even in a file you - never opened: pin and census tests read the source files you changed. (#4025's lane called - `CollectionLogDrainForensicsStoreTests` pre-existing, but it counts `DarlingWorker`'s fault arms, and the lane - had added one.) + never opened: pin and census tests read the source files you changed. - Finish, or stop at a committed and pushed checkpoint with a handoff, by ``. - **Never stop to ask.** If an item needs a decision nobody has made, park it with the question in your report and move on. @@ -139,23 +141,30 @@ census, one of them introduced by that night's own wave). 1. Use a current runtime zip at ``. If it is missing or stale, run `Darling/tools/fetch-pg-runtime.ps1`, which is what CI runs. A stale zip fails the runtime-version pin tests. -2. Extract it to ``, then run `initdb -D /data -U darling -A trust --encoding=UTF8`. -3. Append to `postgresql.conf`: - - `shared_preload_libraries = 'timescaledb,pg_stat_statements'`. This is the exact value the `darling-pg` job's +2. Start the rig with the start helper, never by hand. Use your own ``, one per lane, and a `` + under your scratchpad folder or a worktree (the helpers refuse any other place). `-Owner` is your worktree path: + + pwsh -NoProfile -File C:\GitHub\PerformanceMonitor\.claude\skills\lane-orders\rig-start.ps1 -Port -RigDir -Owner + + It refuses a busy port or a non-empty ``, extracts `-Zip` (default: the runtime zip above) into + ``, runs `initdb -D /data -U darling -A trust --encoding=UTF8`, and appends to + `postgresql.conf`, before the server starts: + - `shared_preload_libraries = 'timescaledb,pg_stat_statements'`, the exact value the `darling-pg` job's "Initialize and start throwaway PostgreSQL" step writes in `.github/workflows/build.yml`. Without - `pg_stat_statements` a rig SKIPS the live tests that count statements through it (for example + `pg_stat_statements` a rig skips the live tests that count statements through it (for example `StoreSizeCacheLiveTests`) instead of running them, so a green local run proves less than CI's. - - `port = ` - - `listen_addresses = '127.0.0.1'` - - `timescaledb.max_background_workers` and `max_worker_processes`, as CI's `darling-pg` job sets them. + - `port`, `listen_addresses = '127.0.0.1'`, and `timescaledb.max_background_workers` and + `max_worker_processes` as CI's `darling-pg` job sets them. - `timezone = 'UTC'` and `log_timezone = 'UTC'`. initdb takes the machine's time zone, but CI's runners run - in UTC, and a rig must match CI. On 2026-09-25, time-sensitive live tests failed on rigs in - America/New_York, and a clean full run on a UTC rig passed all of them. A rig in UTC does not excuse a - failure: CI still decides whose it is. - - Change `postgresql.conf` before you start the server. A restart during a running suite breaks that run's - results, and so does a re-run on the database that run left behind. -4. Check the port is free. Start the server in the background, because `pg_ctl -w start` hangs a tool call, and - confirm "ready to accept connections" in the log. + in UTC, and a rig must match CI: time-sensitive live tests have failed on a rig in another time zone. A rig + in UTC does not excuse a failure: CI still decides whose it is. + + It then starts the server without holding your tool call open, checks the log says "ready to accept + connections", and registers the cluster in `C:\GitHub\worktrees\.rig-registry.tsv` (outside every repo, never + committed): data directory, postmaster PID, port, start time (UTC) and owner. +3. Never edit `postgresql.conf` and restart during a suite. A restart during a running suite breaks that run's + results, and so does a re-run on the database that run left behind. +4. A rig started by hand is not registered: the sweep lists it but cannot tell whether its lane is gone. 5. Create `darlingtest` for the suite, and a separate `probe` database for hand-run SQL. A suite database reused across several full runs produces false failures; drop and recreate it before a final run. Also create a database named `darling`, exactly as CI's init step does: `createdb -h 127.0.0.1 -p -U darling darling`. @@ -163,7 +172,14 @@ census, one of them introduced by that night's own wave). without it. 6. Set `DARLING_TEST_PG` to `Host=127.0.0.1;Port=;Username=darling;Database=darlingtest`. Set `DARLING_TEST_PGRUNTIME=` only when you touch the managed runtime or role provisioning. -7. Stop the server when you finish. +7. Stop the rig when you finish, and before your wall clock runs out, with the stop helper: + + pwsh -NoProfile -File C:\GitHub\PerformanceMonitor\.claude\skills\lane-orders\rig-stop.ps1 -DataDir /data + + It runs `pg_ctl stop -m fast` from the rig's own runtime; when `pg_ctl` is missing it stops the PID in + `postmaster.pid`, only once that PID proves to be `postgres.exe` started on that data directory. It never stops + by image name. Then it deletes the rig folder and drops the registry line. A rig you leave running is stopped by + the coordinator's `rig-sweep.ps1` at the wave boundary once your worktree is gone or its postmaster has died. ## Build and test @@ -172,16 +188,43 @@ census, one of them introduced by that night's own wave). - The tests are MTP executables: run `Darling.Tests.exe` / `Lite.Tests.exe` from `bin/Debug//`, filtered with `-class "*Name*"` or `-method "*Name*"`. `dotnet test` does not work. - For a lone failure in a class you didn't touch, re-run that class alone before calling it yours. +- **The gate, before every push.** From the worktree root, run `bash .github/scripts/pre-push.sh`. It builds both + test projects, runs the whole `Stage=Guard` trait of both suites as CI's first job does, then the classes CI's + test map selects for your diff plus every class in a test file that names a symbol you changed, capped. It prints + result lines only and ends in `GATE GREEN`, `GATE RED` or `GATE STOP`. Push only on `GATE GREEN`, and put its two + Guard result lines and its `classes:` line in the PR body. A `symbols dropped` line means most of the suite names + those symbols: run your brief's own class list for them. A line ` ran 0 tests: the filter matched nothing` + makes the gate `GATE RED`, because a run that matches no test exits 0 without running anything. `GATE STOP: + selection is the whole suite (darling N, lite M)` (exit 3) means the selection is over 400 classes in one project + (or CI's map answered FULL, which is a whole-suite selection too), which the laptop never runs: push on its green Guard lines plus your brief's own classes, and CI decides the rest. + `--dry-run` lists the classes and each batch's size. CI decides the rest. + The map's part (#5872) is the same set a pull request's CI passes run: a test, docs or `.github/` diff gets no + drift, so it selects its own classes, their xunit collection mates, the classes that read its files as text and + the always-run ones (`Stage=Guard`, whole-tree readers, `Gate=Env`, new test classes): about 330 Darling classes, + but a Darling test file reaches only the 9 classes whose map pattern is a kept `Darling/**` (`KEEP_DARLING_TREE` in + `test-map.py`), plus the classes that read the file. `selection.json` names each file the map did not know under + `unknown`. +- **Run the classes that name what you changed.** Run `python .github/scripts/ci-select.py local-classes + --base origin/dev`, then run its `-class` lines, plus `-class "*Census*"`, `-class "*Ratchet*"` and + `-trait Stage=Guard`. It lists the test classes whose source names a symbol your change declares or edits, most + specific first. A class marked `needs rig` runs only with `DARLING_TEST_PG` set. +- **Or take CI's own filter.** Run `python .github/scripts/ci-select.py local-filter --base origin/dev`, with + `--map ` when a nightly test map is at hand. Each `darling: ` or `lite: ` line is the filter for + that suite's test executable (40 classes a line, so one line may become several); strip the prefix and paste the + rest. The ` classes:` line only lists them. The last line is the guard set. A note on stderr says whether + the classes are CI's whole answer or, when that answer is FULL or over 300 classes, the part a lane runs. - **Filter by every class in every test file you touch**, not by the file's name. One file can hold several classes (`PgPlanCaptureCollectorDefinitionTests.cs` also holds `PgPlanLogParserTests`), and filtering on one class skips - the others, including tests you just added. That let a forged-header regression reach CI (#4016). -- Census, meta and pin tests are real gates: - - `McpPayloadContractCensusTests` and the MCP inventory pins; - - `StartupCommandTimeoutTests` and `StorageCommandTimeoutTests`; - - `AlertReadFailureSurfaceTests` and `DocCommentHygiene`; - - `LivePostgresCollectionHygieneTests` and `LiveCleanupConversionRatchetTests`. - - Update a pin deliberately, with the reason. Never weaken one. + the others, including tests you just added. +- **Tests are load-bearing (#5833).** A test earns its place by failing on a real defect: a leaked error string, a + read against a monitored server without a deadline, a Lite fix that skipped Darling. + - A census, pin or ratchet that a correct change breaks is moved only when its rule still holds. When its rule is + spelling (a signature at a line, a SQL fragment's wording, a comment's text, a trait roster, a coverage + percentage), delete it and name it in the PR body. Never weaken one whose rule holds. + - Never add a census, pin, ratchet or parity test that reads source text. Cover a change with a behaviour test + that fails on the defect and passes on the fix. + - A review's UNCOVERED line is answered with such a behaviour test, or with one sentence in the PR body saying + why none is needed. - **Live classes:** - Clean up through `LiveStoreCleanup.RunAsync(connectionString, bodySucceeded, ...)`, with `bodySucceeded` set as the body's last statement. @@ -197,7 +240,7 @@ census, one of them introduced by that night's own wave). 3. Fix it in the surrounding style. Comments say why and cite the issue. Results stay identical unless the issue asks for a behavior change. 4. **Lite parity is mandatory** wherever the same read or pattern exists in Lite. -5. Pin it with a regression test that fails on the old shape; prove that once by reverting. Add live tests wherever +5. Cover it with a behaviour test that fails on the old code; prove that once by reverting. Add live tests wherever the relations or rows read change. 6. Fix small in-lane defects in the same PR. File an issue only for large or design-level work, and list it. @@ -207,13 +250,16 @@ Only a lane pre-approved by name may add one. Versions stay dense: take the next renumber if dev moved. A version bump also obliges: - the sentinel in `ViewerDataService.StoreSchemaProbeSql`, plus its `MapProbedSchemaVersion` arm; - the `GetBoolean` list in `GetStoreSchemaVersionAsync`, plus `ViewerSchemaVersionGateTests`; -- "newest migration" tests written as `Scripts[^1]`. +- "newest migration" tests written as `Scripts[^1]`; +- a rung that changes how an older build reads existing data: its line in Darling/README.md's "Rolling back to an + older build" list, added in the same PR as the rung, with the rung's real number, saying what the older build + reads differently and what to do before rolling back past it. Never edit an existing migration. ## Finish -- No attribution lines (Erik, 2026-10-01): no `Co-Authored-By` or `Claude-Session` trailer in a commit, and no +- No attribution lines: no `Co-Authored-By` or `Claude-Session` trailer in a commit, and no "Generated with" line in the PR body. A brief or a harness reminder that asks for them loses to this rule. - Open the PR against `dev` **as a draft** (`gh pr create --draft`). Title: the outcome in plain language, plus `(#)`. The body starts with `Closes #.`, then `## Why`, `## What changes` and `## Test plan` @@ -223,11 +269,9 @@ Never edit an existing migration. PR for one issue. The coordinator names one lane, usually the last, to write the PR's top `## Why` and `## What changes` for the whole issue and its one CHANGELOG entry. Lanes that fix review findings push to the PR's own branch. -- Write the PR body in one Write, from the diff and your numbers. Run - `python "C:/Users/edarl/.claude/skills/plain-english/check.py" ` ONCE, fix the real hits in one pass, - and publish (Erik, 2026-10-06; this replaces the 2026-09-25 rule against running it). Don't re-run it to chase a - clean score: a lane writes its body at its largest context, and one lane spent 53 minutes and 278k tokens - rewriting. The body must be true. +- Write the PR body in one Write, from the diff and your numbers, and publish. Never run the plain-English checker + on a PR body, an issue or a comment: it is for blogs, social posts and demo script comments. A lane writes its + body at its largest context, where a rewrite costs the most. The body must be true. - **Don't mark it ready, don't merge, and don't enable auto-merge.** The coordinator does all three after verifying. - A user-visible change gets a CHANGELOG entry in `/-reported.txt`, in this form: @@ -241,8 +285,8 @@ Never edit an existing migration. Plain punctuation, no em dashes. A test-only PR gets none. - Check every claim in the entry against your diff, the negative ones too. A removed issue reference is deleted text, so that entry can't say nothing was deleted. A change to a description must not read as a - change to behavior. Name every user-visible change the PR makes. Lane entries were wrong in 8 of 42 at - the #4080 splice and 6 of 41 at #4143. + change to behavior. Name every user-visible change the PR makes. About one lane entry in six is + wrong. ## Final report (the PR body) diff --git a/.claude/skills/lane-orders/rig-common.ps1 b/.claude/skills/lane-orders/rig-common.ps1 new file mode 100644 index 0000000000..e50e8edbd7 --- /dev/null +++ b/.claude/skills/lane-orders/rig-common.ps1 @@ -0,0 +1,246 @@ +# Shared by rig-start.ps1, rig-stop.ps1 and rig-sweep.ps1 (#5787). Dot-source only. +# +# A rig is one throwaway PostgreSQL cluster an agent starts for live test classes: +# \pgsql\ the runtime zip, extracted +# \data\ the cluster (initdb) +# \pg.log the server log +# Every rig is registered in one file outside every repo, so a sweep can find the ones whose agent is gone. + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +# One documented location, outside every repo and never committed. Tab-separated, no header, one line per cluster: +# data_dir pid port started_utc owner +$script:RigRegistryPath = 'C:\GitHub\worktrees\.rig-registry.tsv' + +# The only places a rig data directory may live. Everything else is refused, including a registry line that points +# elsewhere: the main checkout, Program Files, ProgramData (the Darling service's own managed cluster) and the rest. +$script:RigAllowedRoots = @( + 'C:\GitHub\worktrees\', + 'C:\GitHub\PerformanceMonitor\.claude\worktrees\', + 'C:\Users\edarl\AppData\Local\Temp\claude\' +) + +# Top-level entries a rig folder holds. A folder with anything else in it keeps those files: only data and pgsql go. +$script:RigFolderEntries = @('pgsql', 'data', 'pg.log') + +function ConvertTo-RigPath { + param([Parameter(Mandatory)][string]$Path) + $full = [System.IO.Path]::GetFullPath($Path.Replace('/', '\')) + return $full.TrimEnd('\') +} + +# Returns $null when the path is an acceptable rig data directory, or the reason it is refused. +function Get-RigPathRefusal { + param([Parameter(Mandatory)][string]$DataDir) + $full = ConvertTo-RigPath $DataDir + $rigFolder = Split-Path -Parent $full + $root = $script:RigAllowedRoots | Where-Object { ($rigFolder + '\').StartsWith($_, [System.StringComparison]::OrdinalIgnoreCase) -and ($rigFolder + '\').Length -gt $_.Length } | Select-Object -First 1 + if (-not $root) { + return "'$full' is not inside a rig folder under an allowed root ($($script:RigAllowedRoots -join ', '))." + } + if (Test-Path -LiteralPath (Join-Path $rigFolder '.git')) { + return "'$rigFolder' holds a .git entry: it is a repo or worktree root, not a rig folder." + } + foreach ($p in @($rigFolder, $full)) { + $item = Get-Item -LiteralPath $p -Force -ErrorAction SilentlyContinue + if ($item -and ($item.Attributes -band [System.IO.FileAttributes]::ReparsePoint)) { + return "'$p' is a junction or symbolic link." + } + } + return $null +} + +# Reads under the registry lock too: Remove-RigRegistryLine rewrites the whole file, and a reader outside the lock +# can meet a sharing violation or a half-written file (#5787 review). +function Read-RigRegistry { + $lines = @(Invoke-RigRegistryLocked { + if (Test-Path -LiteralPath $script:RigRegistryPath) { Get-Content -LiteralPath $script:RigRegistryPath } + }) + $rows = foreach ($line in $lines) { + if ([string]::IsNullOrWhiteSpace($line)) { continue } + $f = $line.Split("`t") + [pscustomobject]@{ + DataDir = $f[0] + Pid = if ($f.Count -gt 1) { $f[1] } else { '' } + Port = if ($f.Count -gt 2) { $f[2] } else { '' } + StartedUtc = if ($f.Count -gt 3) { $f[3] } else { '' } + Owner = if ($f.Count -gt 4) { $f[4] } else { '' } + Line = $line + } + } + return @($rows) +} + +# Every registry read and change runs under one machine-wide mutex: lanes start and stop rigs at the same time. +# A named mutex is owned by its thread, so a holder may nest calls (rig-sweep.ps1 holds it across one rig's +# decide-and-stop, and Stop-RigCluster takes it again to drop the line). The wait covers one rig's stop (up to +# about 90 seconds) by another caller. +function Invoke-RigRegistryLocked { + param([Parameter(Mandatory)][scriptblock]$Body) + $mutex = [System.Threading.Mutex]::new($false, 'Global\PerformanceMonitorRigRegistry') + $held = $false + try { + try { $held = $mutex.WaitOne([TimeSpan]::FromSeconds(180)) } catch [System.Threading.AbandonedMutexException] { $held = $true } + if (-not $held) { throw "Timed out waiting for the rig registry lock." } + & $Body + } + finally { + if ($held) { $mutex.ReleaseMutex() } + $mutex.Dispose() + } +} + +function Add-RigRegistryLine { + param([string]$DataDir, [int]$ProcessId, [int]$Port, [string]$Owner) + $started = [DateTime]::UtcNow.ToString('yyyy-MM-ddTHH:mm:ssZ') + $line = ($DataDir, $ProcessId, $Port, $started, ($Owner -replace "[`t`r`n]", ' ')) -join "`t" + Invoke-RigRegistryLocked { + $dir = Split-Path -Parent $script:RigRegistryPath + if (-not (Test-Path -LiteralPath $dir)) { New-Item -ItemType Directory -Path $dir | Out-Null } + Add-Content -LiteralPath $script:RigRegistryPath -Value $line -Encoding utf8NoBOM + } +} + +function Remove-RigRegistryLine { + param([Parameter(Mandatory)][string]$DataDir) + $target = ConvertTo-RigPath $DataDir + Invoke-RigRegistryLocked { + if (-not (Test-Path -LiteralPath $script:RigRegistryPath)) { return } + $keep = @(Get-Content -LiteralPath $script:RigRegistryPath | Where-Object { + -not [string]::IsNullOrWhiteSpace($_) -and + -not [string]::Equals((ConvertTo-RigPath ($_.Split("`t")[0])), $target, [System.StringComparison]::OrdinalIgnoreCase) + }) + Set-Content -LiteralPath $script:RigRegistryPath -Value $keep -Encoding utf8NoBOM + } +} + +# Every running postgres.exe postmaster (no --forkchild), with the data directory its command line names. +function Get-PostmasterProcesses { + $all = @(Get-CimInstance Win32_Process -Filter "Name = 'postgres.exe'") + foreach ($p in $all) { + $cmd = [string]$p.CommandLine + if ($cmd -match '--forkchild') { continue } + $data = $null + if ($cmd -match '(?:^|\s)-D\s*"([^"]+)"') { $data = $Matches[1] } + elseif ($cmd -match '(?:^|\s)-D\s*(\S+)') { $data = $Matches[1] } + [pscustomobject]@{ + ProcessId = [int]$p.ProcessId + DataDir = if ($data) { ConvertTo-RigPath $data } else { '' } + CommandLine = $cmd + Children = @($all | Where-Object { $_.ParentProcessId -eq $p.ProcessId }).Count + } + } +} + +# True only when the PID is a live postgres.exe postmaster whose command line names this data directory. +function Test-RigPostmaster { + param([int]$ProcessId, [string]$DataDir) + $target = ConvertTo-RigPath $DataDir + $proc = Get-Process -Id $ProcessId -ErrorAction SilentlyContinue + if (-not $proc -or $proc.ProcessName -ne 'postgres') { return $false } + $cim = Get-CimInstance Win32_Process -Filter "ProcessId = $ProcessId" + if (-not $cim) { return $false } + $cmd = ([string]$cim.CommandLine).Replace('/', '\') + return $cmd.IndexOf($target, [System.StringComparison]::OrdinalIgnoreCase) -ge 0 +} + +# The postmaster PID from \postmaster.pid, or 0. +function Get-RigPidFilePid { + param([string]$DataDir) + $pidFile = Join-Path $DataDir 'postmaster.pid' + if (-not (Test-Path -LiteralPath $pidFile)) { return 0 } + $first = Get-Content -LiteralPath $pidFile -TotalCount 1 + $n = 0 + if ([int]::TryParse(([string]$first).Trim(), [ref]$n)) { return $n } + return 0 +} + +# Stops the cluster in DataDir (pg_ctl first, then the verified postmaster PID), deletes the rig, drops its +# registry line. Returns an object saying what it did. Refuses any path Get-RigPathRefusal rejects, touching nothing. +function Stop-RigCluster { + param([Parameter(Mandatory)][string]$DataDir, [int]$RegisteredPid = 0) + $data = ConvertTo-RigPath $DataDir + $refusal = Get-RigPathRefusal $data + if ($refusal) { + return [pscustomobject]@{ DataDir = $data; Refused = $refusal; Stopped = $false; Deleted = $false; Note = '' } + } + $rigFolder = Split-Path -Parent $data + $result = [pscustomobject]@{ DataDir = $data; Refused = $null; Stopped = $false; Deleted = $false; Note = '' } + + if ((Test-Path -LiteralPath $data) -and -not (Test-Path -LiteralPath (Join-Path $data 'PG_VERSION'))) { + $result.Refused = "'$data' exists but holds no PG_VERSION: it is not a PostgreSQL data directory." + return $result + } + + # The postmaster to stop: postmaster.pid first, the registered PID second; either must prove itself. + $postmaster = 0 + foreach ($candidate in @((Get-RigPidFilePid $data), $RegisteredPid)) { + if ($candidate -gt 0 -and (Test-RigPostmaster -ProcessId $candidate -DataDir $data)) { $postmaster = $candidate; break } + } + + if ($postmaster -eq 0) { + # A live postgres at the postmaster.pid PID that does not name this data directory: deleting under a running + # server would tear files out from under it, so keep everything and say so. + $filePid = Get-RigPidFilePid $data + $proc = if ($filePid -gt 0) { Get-Process -Id $filePid -ErrorAction SilentlyContinue } else { $null } + if ($proc -and $proc.ProcessName -eq 'postgres') { + $result.Refused = "postmaster.pid names live postgres PID $filePid, whose command line does not name '$data'; nothing touched." + return $result + } + } + + if ($postmaster -gt 0) { + $children = @(Get-CimInstance Win32_Process -Filter "ParentProcessId = $postmaster AND Name = 'postgres.exe'" | ForEach-Object { [int]$_.ProcessId }) + $pgCtl = Join-Path $rigFolder 'pgsql\bin\pg_ctl.exe' + if (Test-Path -LiteralPath $pgCtl) { + $out = & $pgCtl stop -D $data -m fast -w -t 60 2>&1 + if ($LASTEXITCODE -ne 0) { $result.Note += "pg_ctl stop exited $LASTEXITCODE ($(($out | Out-String).Trim())); " } + } + else { + $result.Note += 'pg_ctl.exe missing; stopped the verified postmaster PID; ' + } + if (Get-Process -Id $postmaster -ErrorAction SilentlyContinue) { + # Re-verify right before the kill: never stop a PID that is not this cluster's postmaster. + if (Test-RigPostmaster -ProcessId $postmaster -DataDir $data) { + Stop-Process -Id $postmaster -Force + } + } + # Backends exit on their own once the postmaster is gone; give them time, then stop the stragglers by PID. + $deadline = [DateTime]::UtcNow.AddSeconds(20) + while ([DateTime]::UtcNow -lt $deadline) { + $alive = @($children | Where-Object { Get-Process -Id $_ -ErrorAction SilentlyContinue }) + if ($alive.Count -eq 0 -and -not (Get-Process -Id $postmaster -ErrorAction SilentlyContinue)) { break } + Start-Sleep -Milliseconds 500 + } + foreach ($c in $children) { + $proc = Get-Process -Id $c -ErrorAction SilentlyContinue + $cim = Get-CimInstance Win32_Process -Filter "ProcessId = $c" + if ($proc -and $proc.ProcessName -eq 'postgres' -and $cim -and $cim.ParentProcessId -eq $postmaster) { + Stop-Process -Id $c -Force + } + } + if (Get-Process -Id $postmaster -ErrorAction SilentlyContinue) { + $result.Note += "postmaster $postmaster still running; folder kept; " + return $result + } + $result.Stopped = $true + } + + # Delete: the whole rig folder when it holds only rig entries; otherwise data and pgsql only. + if (Test-Path -LiteralPath $rigFolder) { + $extra = @(Get-ChildItem -LiteralPath $rigFolder -Force | Where-Object { $script:RigFolderEntries -notcontains $_.Name }) + $targets = if ($extra.Count -eq 0) { @($rigFolder) } else { @($data, (Join-Path $rigFolder 'pgsql')) | Where-Object { Test-Path -LiteralPath $_ } } + if ($extra.Count -gt 0) { $result.Note += "rig folder holds other files ($(($extra.Name | Select-Object -First 3) -join ', ')); removed data and pgsql only; " } + foreach ($t in $targets) { + for ($attempt = 1; $attempt -le 5; $attempt++) { + try { Remove-Item -LiteralPath $t -Recurse -Force; break } + catch { if ($attempt -eq 5) { $result.Note += "delete of '$t' failed: $($_.Exception.Message); "; return $result } Start-Sleep -Seconds 2 } + } + } + $result.Deleted = $true + } + + Remove-RigRegistryLine $data + return $result +} diff --git a/.claude/skills/lane-orders/rig-start.ps1 b/.claude/skills/lane-orders/rig-start.ps1 new file mode 100644 index 0000000000..a6802b8baf --- /dev/null +++ b/.claude/skills/lane-orders/rig-start.ps1 @@ -0,0 +1,88 @@ +<# +.SYNOPSIS + Starts a throwaway PostgreSQL cluster (a rig) for live test classes and registers it (#5787). +.DESCRIPTION + Unpacks the runtime zip into RigDir, runs initdb on RigDir\data with the settings CI's darling-pg job uses plus + UTC time zones, starts the server on Port, and appends one line to C:\GitHub\worktrees\.rig-registry.tsv so + rig-sweep.ps1 can stop it once its owner is gone. Stop it with rig-stop.ps1 -DataDir \data. +.EXAMPLE + pwsh -NoProfile -File C:\GitHub\PerformanceMonitor\.claude\skills\lane-orders\rig-start.ps1 -Port 55611 ` + -RigDir C:\Users\edarl\AppData\Local\Temp\claude\\scratchpad\fix-123-thing\rig -Owner C:\GitHub\PerformanceMonitor\.claude\worktrees\agent-x +#> +param( + [Parameter(Mandatory)][int]$Port, + # The rig folder: must not exist yet, or be empty, and must sit under an allowed root (see rig-common.ps1). + [Parameter(Mandatory)][string]$RigDir, + # The worktree path that uses the rig, or an agent id when there is none. The sweep stops the rig once a + # worktree-path owner no longer exists. + [Parameter(Mandatory)][string]$Owner, + [string]$Zip = 'C:\GitHub\PerformanceMonitor\Darling\artifacts\pg-runtime.zip' +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' +. (Join-Path $PSScriptRoot 'rig-common.ps1') + +$rig = ConvertTo-RigPath $RigDir +$data = Join-Path $rig 'data' +$refusal = Get-RigPathRefusal $data +if ($refusal) { throw "Refused: $refusal" } +if ((Test-Path -LiteralPath $rig) -and @(Get-ChildItem -LiteralPath $rig -Force).Count -gt 0) { + throw "Refused: '$rig' already exists and is not empty. Pick a new rig folder, or stop the old rig with rig-stop.ps1." +} +if (-not (Test-Path -LiteralPath $Zip)) { throw "Runtime zip '$Zip' not found. Run Darling\tools\fetch-pg-runtime.ps1." } +if (Get-NetTCPConnection -LocalPort $Port -State Listen -ErrorAction SilentlyContinue) { throw "Port $Port is already in use." } +$taken = @(Read-RigRegistry | Where-Object { $_.Port -eq [string]$Port }) +if ($taken.Count -gt 0) { throw "Port $Port is registered to '$($taken[0].DataDir)' (owner $($taken[0].Owner)). Pick another port, or run rig-sweep.ps1." } + +$started = $false +try { + New-Item -ItemType Directory -Force -Path $rig | Out-Null + Expand-Archive -LiteralPath $Zip -DestinationPath $rig + $bin = Join-Path $rig 'pgsql\bin' + if (-not (Test-Path -LiteralPath (Join-Path $bin 'initdb.exe'))) { throw "The zip has no pgsql\bin\initdb.exe." } + + $initOut = & (Join-Path $bin 'initdb.exe') -D $data -U darling -A trust --encoding=UTF8 2>&1 + if ($LASTEXITCODE -ne 0) { throw "initdb failed ($LASTEXITCODE): $(($initOut | Select-Object -Last 5) -join ' ')" } + + # The darling-pg job's settings in .github/workflows/build.yml, plus UTC: CI's runners run in UTC. + Add-Content -LiteralPath (Join-Path $data 'postgresql.conf') -Encoding utf8NoBOM -Value @( + "shared_preload_libraries = 'timescaledb,pg_stat_statements'", + "port = $Port", + "listen_addresses = '127.0.0.1'", + "timescaledb.max_background_workers = 74", + "max_worker_processes = 85", + "log_line_prefix = '%m [%p] %c '", + "timezone = 'UTC'", + "log_timezone = 'UTC'" + ) + + # The postmaster inherits every inheritable handle of the process that starts it, including the caller's + # stdout pipe, so a plain `pg_ctl -w start` (or Start-Process with redirection, which inherits too) holds a + # tool call open until the server stops. Start-Process without -NoNewWindow or redirection goes through + # ShellExecute in a hidden window, which passes no handles; pg.log carries everything pg_ctl would print. + $log = Join-Path $rig 'pg.log' + $pgCtl = Start-Process -FilePath (Join-Path $bin 'pg_ctl.exe') -PassThru -WindowStyle Hidden ` + -ArgumentList @('start', '-D', "`"$data`"", '-o', "`"-p $Port`"", '-w', '-t', '90', '-l', "`"$log`"") + $null = $pgCtl.Handle + if (-not $pgCtl.WaitForExit(120000)) { throw "pg_ctl start did not return in 120 seconds." } + $started = $true + if ($pgCtl.ExitCode -ne 0) { throw "pg_ctl start failed ($($pgCtl.ExitCode)): $((Get-Content -LiteralPath $log -Tail 10 -ErrorAction SilentlyContinue) -join ' ')" } + if (-not (Select-String -LiteralPath $log -Pattern 'ready to accept connections' -Quiet)) { throw "The log at '$log' never said 'ready to accept connections'." } + + $postmaster = Get-RigPidFilePid $data + if (-not (Test-RigPostmaster -ProcessId $postmaster -DataDir $data)) { throw "postmaster.pid PID $postmaster is not this cluster's postmaster." } + + Add-RigRegistryLine -DataDir $data -ProcessId $postmaster -Port $Port -Owner $Owner + Write-Host "Rig started: data $data, PID $postmaster, port $Port, owner $Owner. Registered in $script:RigRegistryPath." + Write-Host "Stop it with: pwsh -NoProfile -File $(Join-Path $PSScriptRoot 'rig-stop.ps1') -DataDir $data" +} +catch { + # Leave nothing behind that no registry line points at. + $failure = $_ + if ($started -or (Test-Path -LiteralPath $rig)) { + $cleanup = Stop-RigCluster -DataDir $data + if ($cleanup.Refused -or $cleanup.Note) { Write-Warning "Cleanup after the failed start: $($cleanup.Refused) $($cleanup.Note)" } + } + throw $failure +} diff --git a/.claude/skills/lane-orders/rig-stop.ps1 b/.claude/skills/lane-orders/rig-stop.ps1 new file mode 100644 index 0000000000..11a7a26ee6 --- /dev/null +++ b/.claude/skills/lane-orders/rig-stop.ps1 @@ -0,0 +1,36 @@ +<# +.SYNOPSIS + Stops a rig by its data directory, deletes the rig folder and drops its registry line (#5787). +.DESCRIPTION + Stops with \pgsql\bin\pg_ctl.exe stop -m fast. When pg_ctl is missing or the server outlives it, stops the + postmaster PID from \postmaster.pid, but only after confirming that PID is postgres.exe and its command line + names this data directory. Never stops anything by image name. Refuses, touching nothing, a data directory that is + not inside a rig folder under an allowed root (see rig-common.ps1). Works on unregistered clusters too. +.EXAMPLE + pwsh -NoProfile -File C:\GitHub\PerformanceMonitor\.claude\skills\lane-orders\rig-stop.ps1 -DataDir \data +#> +param( + [Parameter(Mandatory)][string]$DataDir +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' +. (Join-Path $PSScriptRoot 'rig-common.ps1') + +$data = ConvertTo-RigPath $DataDir +# Read the line and stop under the registry lock, as rig-sweep.ps1 does, so a sweep and a stop never act on +# the same rig at once (#5787 review). +$registered = @() +$r = Invoke-RigRegistryLocked { + $script:registered = @(Read-RigRegistry | Where-Object { [string]::Equals((ConvertTo-RigPath $_.DataDir), $data, [System.StringComparison]::OrdinalIgnoreCase) }) + $registeredPid = 0 + if ($script:registered.Count -gt 0) { [void][int]::TryParse($script:registered[0].Pid, [ref]$registeredPid) } + Stop-RigCluster -DataDir $data -RegisteredPid $registeredPid +} +if ($r.Refused) { + Write-Host "Refused: $($r.Refused)" + exit 2 +} +Write-Host ("Rig {0}: stopped {1}, deleted {2}, registry line {3}. {4}" -f $data, $r.Stopped, $r.Deleted, + $(if ($registered.Count -gt 0) { 'dropped' } else { 'none (unregistered)' }), $r.Note).Trim() +if ($r.Note -match 'still running|failed') { exit 1 } diff --git a/.claude/skills/lane-orders/rig-sweep.ps1 b/.claude/skills/lane-orders/rig-sweep.ps1 new file mode 100644 index 0000000000..6f9f8e699f --- /dev/null +++ b/.claude/skills/lane-orders/rig-sweep.ps1 @@ -0,0 +1,73 @@ +<# +.SYNOPSIS + Stops and deletes every registered rig whose owner is gone or whose postmaster is gone, and lists running + postgres.exe postmasters nobody registered (#5787). +.DESCRIPTION + For each line of C:\GitHub\worktrees\.rig-registry.tsv: + - owner is a path that no longer exists -> stop, delete the rig, drop the line; + - the registered postmaster PID is gone -> stale: stop whatever is left, delete the rig, drop the line; + - otherwise -> keep. + A line whose data directory is outside the allowed roots is refused and kept. Then every running postgres.exe + postmaster whose data directory is not registered is listed with its PID and data directory, and left running. + Run it at every wave boundary. +.EXAMPLE + pwsh -NoProfile -File C:\GitHub\PerformanceMonitor\.claude\skills\lane-orders\rig-sweep.ps1 +#> +param() + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' +. (Join-Path $PSScriptRoot 'rig-common.ps1') + +$stopped = 0; $deleted = 0; $kept = 0; $refused = 0; $gone = 0 + +foreach ($snapshot in Read-RigRegistry) { + $data = ConvertTo-RigPath $snapshot.DataDir + + # Read, decide and act for one rig under the registry lock, so a rig another lane stopped and unregistered + # after the snapshot is not stopped twice (#5787 review). The block returns one outcome word. + $outcome = Invoke-RigRegistryLocked { + $row = @(Read-RigRegistry | Where-Object { [string]::Equals((ConvertTo-RigPath $_.DataDir), $data, [System.StringComparison]::OrdinalIgnoreCase) }) | Select-Object -First 1 + if (-not $row) { + Write-Host "gone $data (unregistered by someone else since the sweep began)" + return 'gone' + } + $pmPid = 0 + [void][int]::TryParse($row.Pid, [ref]$pmPid) + + # An owner is a worktree path or an agent id; only a path can be checked for existence. + $ownerIsPath = [System.IO.Path]::IsPathRooted($row.Owner) + $ownerGone = $ownerIsPath -and -not (Test-Path -LiteralPath $row.Owner) + $pidGone = -not ($pmPid -gt 0 -and (Test-RigPostmaster -ProcessId $pmPid -DataDir $data)) + + if (-not $ownerGone -and -not $pidGone) { + Write-Host "keep $data (PID $pmPid, port $($row.Port), owner $($row.Owner), started $($row.StartedUtc))" + return 'kept' + } + + $why = if ($ownerGone) { "owner '$($row.Owner)' no longer exists" } else { "postmaster PID $pmPid is gone" } + $r = Stop-RigCluster -DataDir $data -RegisteredPid $pmPid + if ($r.Refused) { + Write-Host "REFUSED $data ($why): $($r.Refused) Line kept." + return 'refused' + } + Write-Host ("sweep {0} ({1}): stopped {2}, deleted {3}. {4}" -f $data, $why, $r.Stopped, $r.Deleted, $r.Note).Trim() + return ('swept', $(if ($r.Stopped) { 'stopped' }), $(if ($r.Deleted) { 'deleted' })) -join ' ' + } + switch -Wildcard ($outcome) { + 'gone' { $gone++ } + 'kept' { $kept++ } + 'refused' { $refused++ } + 'swept*' { if ($outcome -match 'stopped') { $stopped++ }; if ($outcome -match 'deleted') { $deleted++ } } + } +} + +# Registered data directories after the sweep, to tell which running postmasters nobody registered. +$registered = @(Read-RigRegistry | ForEach-Object { ConvertTo-RigPath $_.DataDir }) +$unregistered = @(Get-PostmasterProcesses | Where-Object { -not $_.DataDir -or $registered -notcontains $_.DataDir }) +foreach ($u in $unregistered) { + $dataText = if ($u.DataDir) { $u.DataDir } else { "(no -D on the command line: $($u.CommandLine))" } + Write-Host "UNREGISTERED PID $($u.ProcessId) data $dataText ($($u.Children) child processes). Not stopped." +} + +Write-Host "Summary: stopped $stopped, deleted $deleted, kept $kept, unregistered $($unregistered.Count), refused $refused, already gone $gone." diff --git a/.claude/skills/maintenance/SKILL.md b/.claude/skills/maintenance/SKILL.md index 86d4125cab..202031bae3 100644 --- a/.claude/skills/maintenance/SKILL.md +++ b/.claude/skills/maintenance/SKILL.md @@ -36,7 +36,9 @@ Low-risk patch/minor bumps can go in one PR; majors and anything release-critica 5. **CI tool & action pins.** In `.github/workflows/*.yml`: confirm `uses:` actions are on current majors, and that any `dotnet tool install` is **version-pinned to match its library** (e.g. `vpk --version` must equal the Velopack PackageReference). Bump GitHub Actions that are behind / deprecated. -6. **Dependabot (optional — not a finding).** Two separate features: *security alerts* (passive CVE notifications that close the gap between manual `--vulnerable` passes — mild value) and *version-update PRs* (automated bump PRs — redundant and noisy once you do periodic manual sweeps). For PerformanceStudio, Erik relies on the manual passes; treat Dependabot as **optional and do not report it as a finding**. If alerts are ever wanted they're a one-toggle enable (repo Settings → Code security, no config file); the version-update PRs aren't wanted. +6. **Dependabot.** Two separate features: *security alerts* (passive CVE notifications between manual `--vulnerable` passes) and *version-update PRs* (automated bump PRs). Alerts are a one-toggle enable in repo Settings → Code security, with no config file. Both repos run version updates from `.github/dependabot.yml`, into `dev`, and the manual sweep above still runs. The repos differ in one step: + - **PerformanceMonitor** commits `packages.lock.json`, and Dependabot does not regenerate it. Each NuGet PR needs `dotnet restore PerformanceMonitor.sln --force-evaluate` and a commit of every lock file it rewrites before CI can pass. The header of `.github/dependabot.yml` explains why, and what to check by hand on a `Microsoft.Data.SqlClient` or `Azure.Identity` bump. + - **PerformanceStudio** commits no lock files, so its Dependabot PRs need no extra restore step. ## B. Code & Build Health diff --git a/.claude/skills/release-checklist/SKILL.md b/.claude/skills/release-checklist/SKILL.md index cd482d2674..f30a6d79da 100644 --- a/.claude/skills/release-checklist/SKILL.md +++ b/.claude/skills/release-checklist/SKILL.md @@ -24,11 +24,6 @@ from it at build time. Do not add any of those four elements to a csproj: `Produ fails the build on a second `` property anywhere in the tree, on a re-added derived property, and on a workflow or script that reads a version from any path but `Directory.Build.props`. -This replaced six independently hand-bumped csproj files (#3222). Two of them had been sitting three -releases behind, `deprecated/Dashboard/Dashboard.csproj` was declaring 3.6.0 while its binary stamped -3.3.0.0, and the release gate took its judgement from that file — so nothing in CI compared the version it -gated on against the version the artifacts were named with. Both now read the file above. - Verify no other files contain hardcoded version strings that need updating. ### 2. CHANGELOG @@ -40,7 +35,7 @@ Ensure `CHANGELOG.md` has an entry for the new version with: - **Fixed** section for bug fixes - Reference links at the bottom for all issue numbers -If a week of parallel PRs left duplicate section headers in `[Unreleased]` (two `### Fixed` blocks from keep-both merges), consolidate them at cut time with a content-preserving script that VERIFIES the entry-line multiset is unchanged — never re-sort by hand (a 242-line silent reorder shipped that way once). +If a week of parallel PRs left duplicate section headers in `[Unreleased]` (two `### Fixed` blocks from keep-both merges), consolidate them at cut time with a content-preserving script that verifies the entry-line multiset is unchanged. Never re-sort by hand: a hand re-sort can silently reorder hundreds of lines. If the previous version's changelog entry is missing, add that too. @@ -76,7 +71,7 @@ Do NOT add internal/implementation changes — only user-facing features and beh Applies ONLY when `install/` or `upgrades/` changed since the last release (the deprecated Dashboard edition remains on bug-fix support, so its schema discipline still holds when it IS touched): 1. All schema changes MUST go through the `upgrades/{from}-to-{to}/` folder — never ad hoc, never baked into base install scripts. View-only changes flow through `CREATE OR ALTER` re-runs and need no upgrade script. -2. **List all .sql files in the upgrade folder and compare against `upgrade.txt`** — every script must be listed or it won't run (PR #608 shipped a script not listed in `upgrade.txt`). +2. **List all .sql files in the upgrade folder and compare against `upgrade.txt`.** A script that is not listed does not run. 3. Verify each script uses `IF NOT EXISTS` checks for idempotency, and that any column additions in `01_install_database.sql` CREATE TABLE blocks have corresponding ALTER TABLE scripts in the upgrade folder. ### 4. Build Verification @@ -110,6 +105,7 @@ Spin up temporary instances with the Azure CLI (`az`) and AWS CLI (`aws`): - `database_size_stats` / size collectors cover ALL databases, not just master (the #1631 fallback class) - server row shows engine edition 5, and no collector that does not apply to Azure SQL DB errors. The SQL Agent collectors (`running_jobs`, `job_history`, `agent_status`) are marked not applicable when the server is added (`add_servers` answers "Agent surface: not applicable") and never run, so they leave no `collection_log` row and no health entry. That is the expected result, not a gap (verified v3.10.0) - `collection_log` clean for the cloud server across the cycles + - the always-on XE sessions exist in the Azure database: `PerformanceMonitor_Deadlock`, `PerformanceMonitor_BlockedProcess` and `PerformanceMonitor_Attention` (the client-timeout session, #5640; `SELECT name FROM sys.database_event_sessions`), or this install's own `PerformanceMonitor_Darling__...` / `PerformanceMonitor_Lite__...` fallback of the same capture. `PerformanceMonitor_Attention` has two actions that no Azure run has proved: `sqlserver.client_hostname` and `sqlserver.sql_text`. If the create is rejected with error 25744 (`The action ... is not available for Azure SQL Database`), put the named action on the closed list, as #3753 did for the long-query trace, in `AlwaysOnXeSessions.AttentionAzureRejectedActions` (a one-line edit: the Azure DDL is generated from it), and re-run - the serverless DB's 60s auto-pause + resume (error 40613 transients) does not wedge collection — rows resume after the pause without a service restart 4. Firewall churn: DELETE the allow rule, wait 3–5 minutes, re-add it. Collection must resume on its own without restarting the service. (CAVEAT, verified v3.2.0: a bare rule-deletion often does NOT sever an actively-collecting client — connection pooling keeps the open connection alive and Azure gates only NEW connections — so treat a no-outage result as inconclusive rather than a pass; the recovery logic's authoritative validation is the unit suites.) To force a real outage (verified v3.10.0): open admin sessions in each user database BEFORE deleting the rule, in case the machine you test from shares the Darling host's egress IP. Delete the rule, wait 6 minutes for it to take effect, then KILL Darling's sessions (`program_name` = `PerformanceMonitorDarling`) from those sessions. KILL is denied in master even for the server admin, so collectors pooled on master keep running. Collectors that open new connections then hit 40615: the `collection_log` message must name the firewall, and those collectors must store rows again after the rule is re-added, with the service's `started_at` unchanged. 5. Clean up: `az group delete --name rg-release-test --yes --no-wait` @@ -119,7 +115,7 @@ Spin up temporary instances with the Azure CLI (`az`) and AWS CLI (`aws`): ``` aws rds create-db-instance --db-instance-identifier pm-release-test --db-instance-class db.t3.xlarge --engine sqlserver-ee --master-username admin --master-user-password "$SQL_TEST_PASSWORD" --allocated-storage 20 --region us-east-1 ``` -2. **Network reachability is NOT automatic** (bit on v3.3.0: the fresh instance answered "server was not found" until fixed). Verify `PubliclyAccessible` is true, and open 1433 on the instance's security group — the account-default SG has no public inbound: +2. **Network reachability is not automatic:** a fresh instance answers "server was not found" until it is opened up. Verify `PubliclyAccessible` is true, and open 1433 on the instance's security group — the account-default SG has no public inbound: ``` aws rds describe-db-instances --db-instance-identifier pm-release-test --region us-east-1 --query 'DBInstances[0].[PubliclyAccessible,VpcSecurityGroups[0].VpcSecurityGroupId]' --output text aws ec2 authorize-security-group-ingress --group-id --protocol tcp --port 1433 --cidr 0.0.0.0/0 --region us-east-1 @@ -148,7 +144,15 @@ Quick click-through of any new features or significant changes listed in the cha ### 8. Desktop App Upgrade — Velopack Setup.exe + single-instance handoff -Covers the **desktop apps' own** upgrade via Velopack (`*-Setup.exe`) for **Lite** and the **Darling Viewer**. Test the upgrade over a **running** prior version: +Covers the **desktop apps' own** upgrade via Velopack (`*-Setup.exe`) for **Lite** and the **Darling Viewer**. + +This step is a gate. Run it after step 10 opens the release PR and before step 11 tags the release. Run it on every release, even when the upgrade code did not change. Publishing notifies everyone who watches the repo, and the apps offer the new version in Help → About. A check after publishing finds a broken upgrade only after users can install it. + +Before the cut, build each new Setup.exe from the release PR's head commit as `build.yml` does. Run `dotnet publish` for the self-contained build, then `vpk download` and `vpk pack` with the same arguments, except `-p`. In `build.yml`, `-p` points at the signed payload (`signed/Lite-Velopack`, `signed/DarlingViewer-Velopack`). Point it at the unsigned publish folder instead (`publish/Lite-velopack`, `publish/DarlingViewer-velopack`). These Setup.exe files are unsigned. Run each one on a clean Windows machine over the prior release's signed Setup.exe. + +After publishing, repeat item 2 with the signed Setup.exe to confirm that signing changed nothing. That repeat is not the gate. + +Test the upgrade over a **running** prior version: 1. Install the prior release's Setup.exe, launch the app, and **minimize it to the tray** (leave it running). 2. Run the new release's Setup.exe (and separately test Help → About → download → restart). Confirm the **new** version actually runs afterward — not the stale in-memory one. @@ -156,7 +160,7 @@ Covers the **desktop apps' own** upgrade via Velopack (`*-Setup.exe`) for **Lite 4. **Elevated case:** run the old version **as administrator**, launch the new one non-elevated → expect the *"Restart as administrator"* prompt; elevating completes the takeover. A *same-version* elevated instance should just surface (no UAC). 5. Confirm Lite never closes the Darling Viewer and vice-versa (scoped by exe name). -Local proxy without a release: bump ``, rebuild, run over the old build → expect the close-and-takeover prompt. The decision logic is unit-tested (`Lite.Tests/SingleInstanceDecisionTests`); this step validates the live Win32/Velopack seam (`SingleInstanceCoordinator` / `ProcessInspector` in `PerformanceMonitor.Ui`). When the seam saw no changes since the prior release, a quick post-publish regression pass is acceptable instead of a pre-cut gate. Design: `plans/single-instance-upgrade-handoff.md`. +Local proxy without a release: bump ``, rebuild, run over the old build → expect the close-and-takeover prompt. The decision logic is unit-tested (`Lite.Tests/SingleInstanceDecisionTests`); this step validates the live Win32/Velopack seam (`SingleInstanceCoordinator` / `ProcessInspector` in `PerformanceMonitor.Ui`). Design: `plans/single-instance-upgrade-handoff.md`. ### 8a. Darling service install and upgrade (every release) @@ -206,16 +210,16 @@ Then run `uninstall-darling.ps1`, with `-PurgeData` on a machine you will reuse, #### (d) The Linux tarball and the container image -The release also attaches `PerformanceMonitorDarling-linux-x64-.tar.gz` and `SHA256SUMS-linux.txt`. The `darling-linux` job in `build.yml` builds them: a framework-dependent publish of the service, tarred with no wrapper folder. The same job pushes `ghcr.io/erikdarlingdata/performancemonitor-darling`, tagged with the version and `latest`. +The release also attaches `PerformanceMonitorDarling-linux-x64-.tar.gz`, `PerformanceMonitorDarling-compose-.tar.gz` and `SHA256SUMS-linux.txt` (#6123). The `darling-linux` job in `build.yml` builds them. The tarball is a framework-dependent publish of the service, tarred with no wrapper folder. The compose archive is the `Darling/compose/` folder from git in one wrapper folder, with the `darling` image line changed from `:latest` to `:`. The `darling-linux-publish` job uploads them and pushes `ghcr.io/erikdarlingdata/performancemonitor-darling`, tagged with the version and `latest`. It starts only after the Windows `build` job has passed, signature checks included (#5804), so the Linux assets reach the release some minutes after the Windows ones; a release whose Windows build failed has no Linux assets and no new image. The tarball waits as a workflow artifact for seven days, so a "Re-run failed jobs" within a week publishes it; after that use "Re-run all jobs". -Linux has no install script, no upgrade script and no systemd unit file in the tarball. The supported shapes are in the README's "Run on Linux" section. One is the compose stack in `Darling/compose/`. The other is the tarball extracted to `/opt/darling` under a unit written from the README's example, with your own PostgreSQL. +Linux has no install script, no upgrade script and no systemd unit file in the tarball. The supported shapes are in the README's "Run on Linux" section. One is the compose stack, from the compose archive. The other is the tarball extracted to `/opt/darling` under a unit written from the README's example, with your own PostgreSQL. There is no scripted Linux upgrade path. An upgrade is the same manual steps as a fresh install, over an existing store. So the release must check this by hand: -- **Artifacts:** the tarball and `SHA256SUMS-linux.txt` are on the release, the checksum matches, and the versioned and `latest` image tags exist. -- **Fresh install, compose:** bring the stack up from this release's `Darling/compose/` files on a fresh volume. The compose file names `:latest`, which still points at the previous release until this one publishes. So run the check with this release's own image tag (`:`), set in the compose file or by an override file for the check. Pass: `docker compose ps` shows the darling container healthy (not `unhealthy`). The log shows the config load and `Postgres store ready`. `collection_log` fills, as in the Windows checks. +- **Artifacts:** the tarball, the compose archive and `SHA256SUMS-linux.txt` are on the release, both checksums match, and the versioned and `latest` image tags exist. +- **Fresh install, compose:** download `PerformanceMonitorDarling-compose-.tar.gz` from the release, extract it, and bring the stack up from the folder it makes, on a fresh volume. Its compose file already names this release's own image tag (`:`), so no edit is needed. Check that the `darling` service's `image:` line ends in `:`, not `:latest`. Pass: `docker compose ps` shows the darling container healthy (not `unhealthy`). The log shows the config load and `Postgres store ready`. `collection_log` fills, as in the Windows checks. - **Fresh install, systemd:** extract the tarball to `/opt/darling` and install `libgssapi-krb5-2`. Write the unit from the README. Point `DARLING_CONFIG` at a config for a throwaway PostgreSQL 15+ with TimescaleDB, and start it. Pass: the unit is active and the same store checks hold. -- **Upgrade:** repeat each shape over a store the previous release created. For compose, keep the volume and recreate the container from this release's compose file and image. For systemd, stop the unit, move `/opt/darling` aside, extract the new tarball, keep the config, and start the unit. Pass: the service starts, the store migrates to the new top rung, and collection continues with the earlier rows still present. +- **Upgrade:** repeat each shape over a store the previous release created. For compose, keep the volume and recreate the container from this release's compose archive and image. For systemd, stop the unit, move `/opt/darling` aside, extract the new tarball, keep the config, and start the unit. Pass: the service starts, the store migrates to the new top rung, and collection continues with the earlier rows still present. - **Oldest supported:** apply the same fixture rule as (b). If that release did not ship a Linux tarball, say so in the record and use the oldest release that did. - **Record:** the same fields as (b), plus the image digest or tarball checksum you tested. @@ -237,6 +241,8 @@ Before cutting the release, verify the nightly build has been clean: ### 11. Tag and Release +Do not tag or publish until step 8 passes on the unsigned Setup.exe files from the release PR's head. Publishing notifies watchers, so every check that can block the release runs before it. + After PR is merged to main: ``` git checkout main && git pull diff --git a/.editorconfig b/.editorconfig index 5ff966f9b5..0056c4fe13 100644 --- a/.editorconfig +++ b/.editorconfig @@ -11,3 +11,7 @@ root = true # the proper fix is migrating to LoggerMessage source generators, which is # a larger separate effort. Until then, this rule is pure noise. dotnet_diagnostic.CA1873.severity = none + +# RS0030: a symbol listed in a BannedSymbols.txt is an error (#5943). Only the two test projects carry the +# analyzer and a BannedSymbols.txt (Thread.Sleep); the rule is idle everywhere else. +dotnet_diagnostic.RS0030.severity = error diff --git a/.gitattributes b/.gitattributes index 59dd302286..6b85e6d555 100644 --- a/.gitattributes +++ b/.gitattributes @@ -21,6 +21,10 @@ # and the tab-indented continuation, so line-ending normalisation would change what it is evidence of. Darling/Darling.Tests/Fixtures/auto_explain_real_block.txt -text +# A script signed by Set-AuthenticodeSignature (#5796): the release guard's reader is checked against +# its exact bytes, and the signature block's CRLF line endings are part of what it reads. +.github/scripts/fixtures/signed-golden.ps1 -text + # The #4336 managed-conf rehearsal fixture holds real postgresql.conf bytes (initdb stock plus # product-managed blocks and hand edits) that a live PostgreSQL parses; the rewrite it exercises is also # LF-sensitive line-count/removal counting, so this stays LF regardless of the repo-wide eol=crlf default. @@ -32,3 +36,5 @@ Darling/Darling.Tests/Fixtures/ManagedConf/rehearsal-postgresql.conf text eol=lf tools/pg-verification-rig/Dockerfile text eol=lf tools/pg-verification-rig/*.yml text eol=lf tools/pg-verification-rig/*.sql text eol=lf +# Screenshot baselines for the web UI check (#5744): PNGs, never diffed or converted as text. +tools/ui-smoke/baselines/*.png binary diff --git a/.github/REVIEW-TRAPS.md b/.github/REVIEW-TRAPS.md index 8a7b3f053d..28f7b00be6 100644 --- a/.github/REVIEW-TRAPS.md +++ b/.github/REVIEW-TRAPS.md @@ -73,12 +73,13 @@ reviewer is reading. Keep each line one line — the review quotes them. ## Tests and censuses -- A census fires on any NEW string constant in `PgTargetFactKeys`: `PgTargetMcpSurfaceTests.EveryPgRecommendation_NamesARegisteredTool_…` requires a `PgTargetToolRecommendations.ByFactKey` row for every key, and `PgTargetKnobsTests`' "other lanes' keys fall to 0" pin grades every key (#3688). A non-key alias is excluded by predicate, with the reason inline. +- **Ruling (#5833): tests are load-bearing.** The censuses, pins and rosters named in this section are being deleted under #5833. Never add one, and never answer a trap with a test that reads source text or asserts how code is spelled (a signature at a line, a SQL fragment's wording, a comment's text, a trait roster, a coverage percentage). A trap is answered with a behaviour test that fails on the defect and passes on the fix, or with one sentence in the PR body saying why none is needed. +- A NEW string constant in `PgTargetFactKeys` without a `PgTargetToolRecommendations.ByFactKey` row is a fact whose recommendation names no registered tool (#3688); `PgTargetMcpSurfaceTests.EveryPgRecommendation_NamesARegisteredTool_…` and `PgTargetKnobsTests`' "other lanes' keys fall to 0" check fail on it. The fix is the row the fact needs; a non-key alias is not a fact and is excluded by predicate, with the reason inline. - `InternalsVisibleTo` on PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688). -- A pin that encodes the lie: `ACappedResult_WithholdsTheUnsatisfiedSummary_…` handed the builder two facets at `limit = 2` and asserted `truncated` — the `>= limit` inference as its own proof (#3699). Ask what a pin would fail ON. -- A roster-based census does not see the file it does not roster: `EveryPagedTool_ObservesTruncation_…` walked ten tools and missed three in another file (#3699). An anti-pattern census sweeps the class (every tool body); rosters are for the positive contract only. +- A test that encodes the lie: `ACappedResult_WithholdsTheUnsatisfiedSummary_…` handed the builder two facets at `limit = 2` and asserted `truncated` — the `>= limit` inference as its own proof (#3699). Ask what defect a test would fail ON. +- A roster-based census does not see the file it does not roster: `EveryPagedTool_ObservesTruncation_…` walked ten tools and missed three in another file (#3699). A wider roster or a class-sweeping census is not the answer: each tool gets a behaviour test that fails when its page is cut without `truncated`. - A test that reads the clock twice measures the CI scheduler: the delta-seeder tests failed at 123 and 243 on unrelated PRs (#3676). One anchor owns both ends of an interval. -- An expression-bodied member holding a `{ }` property pattern is read SHORT by the T-SQL convention guard's member walk (#3667; the #3607 precedent) — a member the walk cannot read whole is one no census can see into. +- An expression-bodied member holding a `{ }` property pattern is read SHORT by the T-SQL convention guard's member walk (#3667; the #3607 precedent) — a source-reading guard is blind to any shape it cannot parse, which is one reason it is not coverage. - A test that constructs a "four maintenance anomalies in one run" fixture is testing a shape no live run produces (#3709, the one-profile-per-run line above). Fixtures should carry the population the engine actually emits, or say they do not. ## Workflows and CI @@ -88,6 +89,7 @@ reviewer is reading. Keep each line one line — the review quotes them. - `gh api --paginate --jq` runs the filter once PER PAGE: a `max_by` or `last` is per-page, not global; the newest row is the last LINE of the emitted stream (claude-review-guard.yml's verdict scan). - A summary helper reading `"$1"` truncates every message whose `echo` wraps with a trailing backslash — the wrapped pieces are separate arguments; read `"$*"` (claude-review-guard.yml). - `claude-code-action` refuses to run when `.github/workflows/claude-review.yml` differs from the DEFAULT branch's copy, exits SUCCESS in seconds and sets no output (#2229 cause 1); a PR that edits that file cannot be Claude-reviewed until it merges, and every dev PR after it goes unreviewed until the next release sync (#3657's pending-release arm). +- Our wrong test is a bug we introduced, never a flake: a test that dev's own push broke (a pinned date that aged out, a census that two merged PRs broke together) failed ten unrelated PRs in a week (#5942). It gets an issue and a fix, and a line in `.github/ci-quarantine.txt` only while that fix lands: ` | | | #`, honored for 72 hours, only while the latest completed dev push run failed that exact test, and never on a PR that touches the test's file or one the test map says reaches it. A line without an issue is not a quarantine, it is a hidden failure; do not add a retry or a skip in the test instead. ## Repository mechanics diff --git a/.github/ci-history/replay-accepted.txt b/.github/ci-history/replay-accepted.txt index 412eee2bef..ab855998d7 100644 --- a/.github/ci-history/replay-accepted.txt +++ b/.github/ci-history/replay-accepted.txt @@ -9,8 +9,8 @@ cc29d98125 Darling.Tests.RepoFileAdoptionTests e6dbdbea94 Darling.Tests.IncidentAttachmentDeliveryTests # A Lite class failed on a Darling-only diff, before the Reads=Darling selection (build.yml, 2026-10-04) -# existed or while the pull request still carried the older workflow. DarlingReadsTraitGuardTests derives the -# Reads=Darling tag from the class source, so a class without it reads no Darling file: the diff did not cause +# existed or while the pull request still carried the older workflow. The Reads=Darling set is derived from the +# class source (the ci-select.py reads-classes scan since #5961; DarlingReadsTraitGuardTests before it), so a class outside it reads no Darling file: the diff did not cause # the failure. d0d482835c PerformanceMonitorLite.Tests.PerformanceCalendarDataTests cd34bbcb31 PerformanceMonitorLite.Tests.StatusBarSizeReadLockTests @@ -33,7 +33,7 @@ a75333e8e2 PerformanceMonitorLite.Tests.ScenarioTests eca647c2f4 PerformanceMonitorLite.Tests.QueryStoreRegressionsToolTests eca647c2f4 PerformanceMonitorLite.Tests.SignificantWaitsToolTests # A Darling class failed on a pull request whose recorded changes are Lite files only (cut 3a/3c of #5459). The -# class source names no Lite path (LiteReadsTraitGuardTests derives the Reads=Lite tag from it, and these carry +# class source names no Lite path (the Reads=Lite set is derived from it, by the ci-select.py reads-classes scan since #5961 and by LiteReadsTraitGuardTests before that, and these carry # none; the one 'Lite' in PlanSync4512CancellationTests is a doc comment), so no Lite edit can reach what it # tests: each is a timing or live-store test that failed on a first attempt, not a selection gap. c3a18e1785 Darling.Tests.PgTargetWaitTests @@ -127,3 +127,14 @@ f1a1894fd8 Darling.Tests.EventWindowedReadsAreBoundedLivePostgresTests # on the same error. Attempt 2 of the same head passed every shard. A first-attempt environment fault of the # shard's server; the 28 changed files (viewer data-start work) did not cause it. 3bb3ae8fe9 Darling.Tests.StoreMetricsLatestSkipScanLiveTests + +# Shared-project replay (#5951): four failures on three pull requests that changed PerformanceMonitor.Common files, found when +# the keep-full rule for the shared projects was narrowed to constants, enums and registration files. None of the four classes has a +# map edge to the shared file the pull request changed, and each failure is not caused by it. LiteDeltaSeederTests (7859d25653) and +# DarlingUpgradeInstallTreeWaitTests (950bb7dbbd) failed on a first attempt and are green on the same sha now. +# LatestValueLookbackLivePostgresTests (1e52a8e34f, 28c0765ab1) failed a plan-shape assertion on PgFactCollector.DatabaseSizeSql, +# a file that pull request never touched. +7859d25653 PerformanceMonitorLite.Tests.LiteDeltaSeederTests +950bb7dbbd Darling.Tests.DarlingUpgradeInstallTreeWaitTests +1e52a8e34f Darling.Tests.LatestValueLookbackLivePostgresTests +28c0765ab1 Darling.Tests.LatestValueLookbackLivePostgresTests diff --git a/.github/ci-quarantine.txt b/.github/ci-quarantine.txt new file mode 100644 index 0000000000..5f871f5cc3 --- /dev/null +++ b/.github/ci-quarantine.txt @@ -0,0 +1,23 @@ +# The CI quarantine (#5942). A test that dev's own latest push broke is left out of an UNRELATED pull request while +# its fix lands. It is not a place to park a flaky test. Our wrong test is a bug we introduced: it gets an issue and a +# fix, and the entry names that issue (.github/REVIEW-TRAPS.md, "Workflows and CI"). +# +# One test per line: +# +# | | | # +# +# - The test is spelled as the runner prints it in a failed job's log (`Darling.Tests.SomeTests.TheMethod [FAIL]`); the +# arguments of a theory case are left off, and the entry leaves out every case of the method. It must start with +# Darling.Tests. or Lite.Tests. +# - The run is the dev push run of build.yml that failed it. The time is when the line was added. +# - The issue is the bug filed for the test and its fix. +# +# A pull request honors an entry only while ALL of these hold; otherwise the test runs as usual: +# - the latest completed dev push run of build.yml failed this exact test (read through the Actions API); +# - the entry is under 72 hours old, whatever else is true (it never renews: add a new line to extend it); +# - the pull request's test map selection is usable and does not reach the test (it changes neither the test's own file +# nor a file the map says the test runs, reads or names). +# Only pull requests honor it: a push to dev runs everything, which is how dev learns the test is fixed. The Guard job +# and both shard runners read it. A line that is not in this format is ignored. +# +# Delete the line when the fix merges. This file ships empty. diff --git a/.github/darling-paths-filter.yml b/.github/darling-paths-filter.yml index d2870c19e5..99c9528e92 100644 --- a/.github/darling-paths-filter.yml +++ b/.github/darling-paths-filter.yml @@ -49,12 +49,23 @@ darling: # #5616: the Darling PostgreSQL shard script and the Lite shard script are run by the shard jobs and # parsed by Darling.Tests pins, so an edit to either has to reach the suite. - '.github/scripts/run-darling-pg-shard.ps1' + - '.github/scripts/darling-shard-pack.py' + - '.github/scripts/darling-shard-weights.json' - '.github/scripts/run-lite-shard.ps1' # #5627: the Darling upgrade-shape workflow and its script. The workflow runs the install and upgrade scripts under # Darling/tools (already covered above) over real releases, and Darling.Tests parses both files, so an edit to # either has to reach the suite. - '.github/workflows/darling-upgrade-shapes.yml' - '.github/scripts/darling-upgrade-shapes.ps1' + # #5701: the UI-smoke workflow stands up a third copy of the #1888 throwaway cluster, and Darling.Tests parses it + # (CiClusterWorkerSizingTests), so an edit to it has to reach the suite. tools/ui-smoke holds seed.sql, whose row + # counts UiSmokeSeedAppliesLiveTests pins against a live store: a seed-only edit would otherwise skip the PostgreSQL + # job and leave a stale count literal to surface as an unrelated red on the next Darling pull request. + - '.github/workflows/darling-ui-smoke.yml' + - 'tools/ui-smoke/**/!(*.md)' + # #5745: the WPF smoke workflow copies the same cluster step again (viewer walk on a seeded store), so it is parsed by + # CiClusterWorkerSizingTests too. + - '.github/workflows/wpf-smoke.yml' # Lite/** is deliberately ABSENT, though build.yml's own `darling` filter carries it. That entry # exists because Darling.Tests reads Lite source in its cross-app parity guards, and those guards run diff --git a/.github/lite-paths-filter.yml b/.github/lite-paths-filter.yml new file mode 100644 index 0000000000..c30bc1344c --- /dev/null +++ b/.github/lite-paths-filter.yml @@ -0,0 +1,78 @@ +# The ONE Lite path gate, the Lite counterpart of .github/darling-paths-filter.yml (#5669). Three readers +# evaluate THIS text and no other copy of it: +# - each leg of the `lite-tests` matrix in .github/workflows/build.yml (its "Detect changed paths" step reads +# it through dorny/paths-filter's documented `filters: ` form, then "Decide how much of the +# Lite suite runs" turns the answers into full / reads / none); +# - the `lite-scope` job, which runs the same dorny step over the pull request's files AND asks +# .github/scripts/ci-select.py `lite-mode` (the Python matcher, which reads this file too, and so do its +# failure-corpus replay and the test-map shadow check). The Lite shards are skipped only when both engines +# say nothing here matched; any disagreement runs them; +# - the pins in Lite.Tests CrossAppGuardCiGateTests, DarlingReadsTraitGuardTests and WatermarkPolicyTests. +# +# These were inline in the lite-tests job, byte copies of the build job's root / core / lite filters. The copies +# stay a copy: CrossAppGuardCiGateTests.TheShardJobsFilters_AreByteForByteCopiesOfTheBuildJobs pins this file's +# root_shard, core_shard and lite_shard + darling_reads_shard equal to the build job's own sets. Two engines +# (picomatch in dorny, the Python matcher in ci-select.py) read the same patterns from this one file, so a +# pattern edit cannot reach one engine and not the other. It lives outside .github/workflows/ deliberately: +# anything inside that directory is parsed as a workflow. +# +# The REASONING for every entry lives at the build job's own filter block and is not duplicated here: +# duplicated prose is prose that rots out of step. What matters is that the sets stay identical, so the shards +# run the Lite suite in exactly the cases the build job's step used to, or the split loses coverage silently. +# That includes the entries that look foreign to Lite (the three Darling trees): Lite.Tests reads those files +# in its cross-app and docs guards, so an edit there has to reach this suite. +# +# Every area pattern is `dir/**/!(*.md)`, any non-markdown file under the area, and NOT a `dir/**` include plus +# a bare `!**/*.md` exclude: dorny evaluates each pattern as an INDEPENDENT predicate, so a bare negation is its +# own rule meaning "any non-markdown file anywhere" (see .github/darling-paths-filter.yml for the measured case). +root_shard: + - 'PerformanceMonitor.sln' + - 'Directory.Build.props' + - 'global.json' + - 'nuget.config' + - 'NuGet.config' + - '.github/workflows/build.yml' +core_shard: + - 'PerformanceMonitor.Alerting/**/!(*.md)' + - 'PerformanceMonitor.Analysis/**/!(*.md)' + - 'PerformanceMonitor.Collectors/**/!(*.md)' + - 'PerformanceMonitor.Common/**/!(*.md)' + - 'PerformanceMonitor.Notifications/**/!(*.md)' + - 'PerformanceMonitor.PlanAnalysis/**/!(*.md)' + - 'PerformanceMonitor.Ui/**/!(*.md)' +lite_shard: + - 'Lite/**/!(*.md)' + - 'Lite.Tests/**/!(*.md)' + # #5616: the Lite shard script and its packer are test inputs (Lite.Tests CrossAppGuardCiGateTests parses the script), so an edit to either has to reach the Lite suite. + - '.github/scripts/run-lite-shard.ps1' + - '.github/scripts/lite-shard-pack.py' + # #5656: Lite.Tests pins read install/ scripts as source (CpuSchedulerStatsCollectorDefinitionTests reads install/17, + # DmvResourceDatabaseSentinelTests reads install/56). A PR that edits only an install script fires `installer` + # and `sql-validation`, neither of which runs Lite.Tests, so the nightly would be the first to catch drift. + - 'install/**/!(*.md)' + # #5669: this file decides whether the Lite shards start at all, so an edit to it has to reach the suite. + - '.github/lite-paths-filter.yml' +# The rest of the build job's `lite` set: the three Darling trees Lite.Tests reads as source. +# A diff that reaches ONLY these (no lite_shard, core_shard or root_shard entry) runs just the +# Lite.Tests classes the ci-select.py reads-classes scan finds reading a Darling path (#5961), in one shard; see the scope step below. +# lite_shard followed by darling_reads_shard is the build job's `lite` set, entry for entry. +darling_reads_shard: + - 'Darling/PerformanceMonitor.Darling.Viewer/**/!(*.md)' + - 'Darling/Darling.Tests/**/!(*.md)' + - 'Darling/PerformanceMonitor.Darling.Service/**/!(*.md)' +# Darling.Tests files that Lite.Tests.csproj COMPILES into its own assembly. An edit to one +# changes code that dozens of Lite.Tests classes run, not just text they read, so it takes the +# whole suite. Not part of the build job's `lite` set (those entries are covered there by the +# Darling.Tests tree); DarlingReadsTraitGuardTests pins this list to the project's linked compiles. +lite_linked_shard: + - 'Darling/Darling.Tests/CSharpSourceWalker.cs' + - 'Darling/Darling.Tests/ClippedTextProbe.cs' + - 'Darling/Darling.Tests/InputNamesProbe.cs' + - 'Darling/Darling.Tests/McpInProcessHost.cs' + - 'Darling/Darling.Tests/McpArgumentTypeRows.cs' + - 'Darling/Darling.Tests/InvestigationToolContract.cs' + - 'Darling/Darling.Tests/StatementScrubCanary.cs' + - 'Darling/Darling.Tests/StaTestThread.cs' + - 'Darling/Darling.Tests/TimingTestScanner.cs' + - 'Darling/Darling.Tests/TestTimeHelpers.cs' + - 'Darling/Darling.Tests/WpfExitSwitchTests.cs' diff --git a/.github/scripts/ci-failure-split.py b/.github/scripts/ci-failure-split.py new file mode 100644 index 0000000000..76521fc52a --- /dev/null +++ b/.github/scripts/ci-failure-split.py @@ -0,0 +1,98 @@ +#!/usr/bin/env python3 +"""Split recent failed Build runs into the deliberate "CI not run" marker and real failures (#5943). + +A pull request whose CI is declined (a draft, or the merge-train label) still runs one job that fails on purpose +with the annotation title "CI not run" (build.yml, "Fail when CI did not run"), so the pull request cannot merge +without an admin override. Counted as an ordinary failure it made up 147 of 200 recent failed runs, and the real +failure rate looked far worse than it was. This script counts that marker apart: + + skip_marker a failed job carries a "CI not run" annotation: the run never started CI, by design + gate_failed a failed job carries a "CI gate failed" annotation: the job that decides whether CI runs failed + real anything else: CI ran and something in it failed + +Usage (needs an authenticated gh): + + python .github/scripts/ci-failure-split.py [--repo owner/name] [--workflow Build] [--limit 200] + +Prints one line per class with its count and its share of the failed runs, then the real failure rate against the +runs where CI ran (successes and cancelled runs count in the denominator, the marker runs do not). +""" +from __future__ import annotations + +import argparse +import json +import subprocess +import sys + +MARKER_TITLE = "CI not run" +GATE_TITLE = "CI gate failed" +CLASSES = ("skip_marker", "gate_failed", "real") + + +def classify(annotation_titles) -> str: + """The class of ONE failed run, from the titles of the annotations on its failed jobs.""" + titles = set(annotation_titles) + if MARKER_TITLE in titles: + return "skip_marker" + if GATE_TITLE in titles: + return "gate_failed" + return "real" + + +def summarize(failed_classes, other_runs: int = 0) -> dict: + """Counts per class, each class's share of the failed runs, and the real failure rate. + + other_runs is every examined run that did not fail (success, cancelled, ...). The real failure rate is real / + (real + gate_failed + other_runs): the marker runs are left out of the denominator, because CI never ran for them. + """ + counts = {name: 0 for name in CLASSES} + for name in failed_classes: + counts[name] += 1 + failed = sum(counts.values()) + share = {name: (counts[name] / failed if failed else 0.0) for name in CLASSES} + denominator = counts["real"] + counts["gate_failed"] + other_runs + return { + "failed": failed, + "counts": counts, + "share": share, + "real_failure_rate": (counts["real"] / denominator if denominator else 0.0), + } + + +def _gh(*args: str): + out = subprocess.run(["gh", *args], check=True, capture_output=True, text=True).stdout + return json.loads(out) if out.strip() else None + + +def _titles_for_run(repo: str, run_id: int) -> list: + jobs = _gh("api", f"repos/{repo}/actions/runs/{run_id}/jobs?per_page=100", "--jq", '[.jobs[] | select(.conclusion == "failure") | .id]') or [] + titles = [] + for job_id in jobs: + annotations = _gh("api", f"repos/{repo}/check-runs/{job_id}/annotations?per_page=100", "--jq", "[.[].title]") or [] + titles.extend(t for t in annotations if t) + if MARKER_TITLE in titles: + break + return titles + + +def main(argv=None) -> int: + parser = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) + parser.add_argument("--repo", default="erikdarlingdata/PerformanceMonitor") + parser.add_argument("--workflow", default="Build") + parser.add_argument("--limit", type=int, default=200) + args = parser.parse_args(argv) + + runs = _gh("run", "list", "-R", args.repo, "--workflow", args.workflow, "--limit", str(args.limit), "--json", "databaseId,conclusion") or [] + failed_ids = [r["databaseId"] for r in runs if r["conclusion"] == "failure"] + classes = [classify(_titles_for_run(args.repo, run_id)) for run_id in failed_ids] + result = summarize(classes, other_runs=len(runs) - len(failed_ids)) + + print(f"{len(runs)} runs of {args.workflow}, {result['failed']} failed") + for name in CLASSES: + print(f" {name:12} {result['counts'][name]:4} {result['share'][name]:6.1%} of failed runs") + print(f"real failure rate (marker runs left out): {result['real_failure_rate']:.1%}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/ci-select.py b/.github/scripts/ci-select.py index eec905f8e9..3c8ba5b249 100644 --- a/.github/scripts/ci-select.py +++ b/.github/scripts/ci-select.py @@ -11,13 +11,28 @@ test-map-select job writes it as an artifact), and `map-keep` reads that artifact for one suite: the Darling PG, Lite and no-store Darling passes of a pull request keep only those classes, and run everything when the answer is FULL, missing or unreadable. See map_select() for the map's schema. `--replay --map FILE` also replays the - corpus against such a map and reports what it would miss (it does not fail on that yet). + corpus against such a map and reports what it would miss (it does not fail on that yet); with `--symbols BASE` + it also prints the classes each row selects before and after the changed-symbol reason (#5798). +* `local-classes` and `local-filter` are for a lane on its own machine. `local-classes` lists, most specific first, + the test classes whose source names a symbol the branch declares or edits (#5776). `local-filter` (#5798) prints + the filter CI's selection gives the branch: per suite, one line is the test executable's arguments + (`darling: -class "*.A" -class "*.B"`) and the next is the class list (`darling classes: A B`), and the last line + is the guard set (`guard: -trait Stage=Guard`). With `--map` (the nightly test map) it is map-select's own answer; + without one, or when that answer is FULL or over 300 classes, it is the part a lane can run, at most 300: the + classes of a changed test file, the map's text-pattern classes for the changed files (with `--map`) and the + classes that name a changed symbol, which CI selects too. A note on stderr says which. A suite with more than 40 + classes gets one filter line per 40. Run it as + `python .github/scripts/ci-select.py local-filter [--base origin/dev] [--map test-map.json.gz]`. +* `quarantine` (#5942) decides, once per pull request run in the test-map-select job, which entries of + .github/ci-quarantine.txt the run honors, and `quarantine-args` turns the decision into `-method- ` runner + arguments for the Guard job and the two shard runners. See quarantine_decide() for the rule. * `--replay` reads .github/ci-history/failures.jsonl (a trimmed record of every run with a real test failure) and checks that each failing class would have been selected for that run's changed files. A cut to the rules lands only while the replay finds 0 misses. A Guard-tagged test runs it on every pull request. The path patterns are NOT copied here. They are read from the filter blocks in .github/workflows/build.yml and -from .github/darling-paths-filter.yml, the same text dorny/paths-filter evaluates, so the replay and the +from .github/darling-paths-filter.yml and .github/lite-paths-filter.yml, the same text dorny/paths-filter +evaluates, so the replay and the workflow cannot disagree about which area a file belongs to. What lives here is the decision logic that used to be spread over `if:` expressions and a bash if-chain. @@ -33,11 +48,13 @@ import re import subprocess import sys +import tempfile import xml.etree.ElementTree as ET ROOT = os.path.normpath(os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..")) WORKFLOW = os.path.join(ROOT, ".github", "workflows", "build.yml") DARLING_FILTER = os.path.join(ROOT, ".github", "darling-paths-filter.yml") +LITE_FILTER = os.path.join(ROOT, ".github", "lite-paths-filter.yml") CORPUS = os.path.join(ROOT, ".github", "ci-history", "failures.jsonl") ACCEPTED = os.path.join(ROOT, ".github", "ci-history", "replay-accepted.txt") @@ -155,12 +172,16 @@ def load_file_filters(path: str = DARLING_FILTER) -> dict[str, list[str]]: class Rules: """The pattern sets, loaded once.""" - def __init__(self, workflow: str = WORKFLOW, darling_filter: str = DARLING_FILTER) -> None: + def __init__(self, workflow: str = WORKFLOW, darling_filter: str = DARLING_FILTER, lite_filter: str = LITE_FILTER) -> None: wf = load_workflow_filters(workflow) self.build = wf["build"] - self.lite_shards = wf["lite-tests"] + # The Lite shard sets live in their own file (#5669), the one the shard legs and the lite-scope job give + # dorny/paths-filter, so both engines read the same text. A missing or empty file raises: no silent none. + self.lite_shards = load_file_filters(lite_filter) + if not self.lite_shards: + raise SystemExit(f"{lite_filter} holds no filters") self.tree = wf["darling-tree-guards"] - self.guard = wf["guard-tests"] + self.guard = wf["guard-build"] self.gate = load_file_filters(darling_filter) @staticmethod @@ -222,7 +243,8 @@ def tree_scope(event: str, all_count: int, lite_count: int) -> str: def scope_filter(scope: str) -> str: - """The Darling.Tests runner arguments for a scope ('' is the whole suite).""" + """The Darling.Tests runner arguments for a scope ('' is the whole suite). `-trait Reads=Lite` is a token the consumers + expand (#5961): no assembly trait of that name exists, the classes come from `reads-classes --suite darling`.""" if scope == "guard": return "-trait Stage=Guard" if scope == "reads-lite": @@ -287,6 +309,37 @@ def decide(files: list[str], event: str, rules: Rules) -> dict: r"^[ \t]*(?:(?:public|internal|private|protected|sealed|static|abstract|partial)\s+)*class\s+(\w+)") _TRAIT = re.compile(r'Trait\(\s*"(\w+)"\s*,\s*"(\w+)"\s*\)') +# #5872: scan_classes() marks a class that neither holds nor inherits a [Fact]/[Theory] (a fake, a logger, a harness a +# test file also declares). The map never has such a class, because it runs nothing, so map_select() must not count it +# as a "new class": 377 Darling and 143 Lite helper types were selected on every pull request that way. +NO_TESTS_TRAIT = "Tests=None" +_HOLDS_MARK = "\x00holds" +_BASE_MARK = "\x00base=" +_TEST_ATTR_LINE = re.compile(r"^\[(?:[^\]]*[\s,(])?(?:Fact|Theory)\b") +COLLECTION_PREFIX = "Collection=" +COLLECTION_MATE_CAP = 60 +_COLLECTION_ATTR = re.compile(r'\bCollection\(\s*"([^"]+)"\s*\)') +_BASE_LIST = re.compile(r"^\s*(?:<[^>]*>\s*)?(?:\([^)]*\)\s*)?:\s*([\w.]+)") + + +def _holders_of(lines: list[str]) -> set[str]: + """The classes of one file that hold a test: a [Fact] or [Theory] attribute belongs to the nearest class declared + above it and indented less than it (the same walk as _test_holders()).""" + out: set[str] = set() + stack: list[tuple[int, str]] = [] + for line in lines: + indent = len(line) - len(line.lstrip(" \t")) + m = _CLASS_DECL.match(line) + is_test = not m and bool(_TEST_ATTR_LINE.match(line.strip())) + if m or is_test: + while stack and stack[-1][0] >= indent: + stack.pop() + if m: + stack.append((indent, m.group(1))) + elif is_test and stack: + out.add(stack[-1][1]) + return out + # The test map is built on shards that set only DARLING_TEST_PG and DARLING_TEST_PGRUNTIME (nightly.yml's "Run this # shard's classes" step; a test pins that env block to MAP_SHARD_ENV). A class that gates its tests on any OTHER # DARLING_TEST_PG_* or DARLING_TEST_PGRUNTIME_* variable (the log-format targets, the log-rotation targets, the @@ -362,16 +415,27 @@ def _walk_classes(root: str): continue relfile = os.path.relpath(path, root).replace(os.sep, "/") gated = any(GATE_ENV_LITERAL.search(line) for line in lines) + holders = _holders_of(lines) for n, line in enumerate(lines): m = _CLASS_DECL.match(line) if not m: continue traits: set[str] = {GATE_TRAIT} if gated else set() + # scan_classes() turns these two markers into NO_TESTS_TRAIT and drops them (#5872). + if m.group(1) in holders: + traits.add(_HOLDS_MARK) + tail = line[m.end():] + if ":" not in tail and "{" not in tail and n + 1 < len(lines): + tail += " " + lines[n + 1] + b = _BASE_LIST.match(tail) + if b: + traits.add(_BASE_MARK + b.group(1).split(".")[-1]) j = n - 1 while j >= 0: s = lines[j].strip() if s.startswith(("[", "///", "//", "*", "/*")) or s.endswith("*/") or s == "" and False: traits.update(f"{k}={v}" for k, v in _TRAIT.findall(s)) + traits.update(COLLECTION_PREFIX + c for c in _COLLECTION_ATTR.findall(s)) j -= 1 else: break @@ -383,6 +447,23 @@ def scan_classes(root: str = ROOT) -> dict[str, dict[str, set[str]]]: out: dict[str, dict[str, set[str]]] = {suite: {} for suite in SUITE_DIRS} for suite, _file, name, traits in _walk_classes(root): out[suite].setdefault(name, set()).update(traits) + # #5872: a class holds a test when it (any declaration of the name) carries a [Fact] or [Theory], or its base class + # does, followed through the suite's own classes; every other class gets NO_TESTS_TRAIT. + for classes in out.values(): + holds = {c for c, t in classes.items() if _HOLDS_MARK in t} + grew = True + while grew: + grew = False + for c, t in classes.items(): + if c not in holds and any(x[len(_BASE_MARK):] in holds for x in t if x.startswith(_BASE_MARK)): + holds.add(c) + grew = True + for c, t in classes.items(): + t.difference_update({x for x in t if x == _HOLDS_MARK or x.startswith(_BASE_MARK)}) + if c not in holds: + t.add(NO_TESTS_TRAIT) + # #5961: Reads=Lite / Reads=Darling are derived from the source, not read off a [Trait] attribute. + with_reads_traits(out, root) return out @@ -411,6 +492,358 @@ def class_selected(suite: str, traits: set[str], d: dict) -> bool: return True +# --------------------------------------------------------------------------------------------------------- +# The classes that read the OTHER product's files (#5961), derived from the source, not from a trait. +# --------------------------------------------------------------------------------------------------------- +# +# A pull request that changes only Lite files runs the Darling.Tests classes that read a Lite path (and the Guard stage); +# one that changes only Darling files named in lite-paths-filter.yml's reads filter runs the Lite.Tests classes that read a +# Darling path. Those two class sets used to be hand-written `[Trait("Reads", ...)]` attributes with a C# census beside +# them, and the runners filtered on the trait. They now run `-class` lists from reads_classes() below, so there is no +# roster to keep: a new class that reads a Lite path is in the set the moment it is written. +# +# This is a port of GuardStageScanner.ReadsALitePath and of the Darling-path detector Lite.Tests had (comments are ignored, +# a string literal is read for a path segment, a recursive enumeration from the repo root reads every file). The scan is per +# FILE, as the census was: a helper class in the same file may hold the path a test reads, so every test class (one that +# holds a [Fact] or [Theory] in its own body) in a file that reads the other tree is in the set. A path assembled from a +# constant another file declares is not visible to a source scan; the replay and the nightly are the backstop for that. + +_RC_ENUM_CALL = re.compile( + r"\b(?:EnumerateFiles|EnumerateFileSystemEntries|EnumerateDirectories|GetFiles|GetFileSystemEntries|GetDirectories)\s*\(") +# Darling.Tests' spelling of "the repo root" (GuardStageScanner.s_repoRootExpression); Lite.Tests' was narrower. +_RC_ROOT_DARLING = re.compile(r"^(?:\w+\.)*\w*(?:RepoRoot|FindRoot|ResolveRoot)\w*\(\s*\)$|^(?:RepoFile\.)?Root$") +_RC_ROOT_LITE = re.compile(r"^(?:\w+\.)*(?:RepoRoot|FindRepoRoot)\(\s*\)$") +_RC_ASSIGN = re.compile(r"\b(\w+)\s*(?:=>|=)\s*([^;=]+);") +_RC_RECURSE = re.compile(r"RecurseSubdirectories\s*=\s*true") +_RC_CLASS = re.compile( + r"^[ \t]*(?:(?:public|internal|private|protected|sealed|static|abstract|partial)\s+)*class\s+(\w+)", re.MULTILINE) +_RC_NAMESPACE = re.compile(r"^[ \t]*namespace\s+([\w.]+)", re.MULTILINE) +_RC_TEST_ATTR = re.compile(r"\[(?:Fact|Theory)\b") +_RC_ENUM_WORD = r"Enumerate|GetFiles|GetDirectories|GetFileSystemEntries" +# The detectors need the other tree's name (the copyright header says "Darling Data", so a bare Darling is no signal). +_RC_CHEAP = {"darling": re.compile(r"Lite|" + _RC_ENUM_WORD), "lite": re.compile(r'Darling[./\\"]|' + _RC_ENUM_WORD)} + + +def _rc_run(text: str, i: int, c: str) -> int: + j = i + while j < len(text) and text[j] == c: + j += 1 + return j - i + + +def _rc_char_literal_end(text: str, i: int) -> int: + j = i + 1 + while j < len(text): + if text[j] == "\\": + j += 2 + continue + if text[j] == "'": + return j + 1 + if text[j] == "\n": + return i + 1 + j += 1 + return j + + +def _rc_opener(text: str, i: int): + """(kind, dollars, quote_run, body_start) of a string literal opening at i, or None. kind: R regular, V verbatim, W raw.""" + j, dollars, verbatim = i, 0, False + while j < len(text) and text[j] in "$@": + if text[j] == "$": + dollars += 1 + elif verbatim: + return None + else: + verbatim = True + j += 1 + if j >= len(text) or text[j] != '"': + return None + if verbatim: + return "V", dollars, 1, j + 1 + quotes = _rc_run(text, j, '"') + if quotes >= 3: + return "W", dollars, quotes, j + quotes + if quotes == 2: + return "R", dollars, 0, j + 2 + return "R", dollars, 1, j + 1 + + +def _rc_scan_code(text: str, i: int, code: bytearray, hole, bodies) -> int: + """Port of CSharpSourceWalker.ScanCode: marks code characters, collects string-literal bodies as (start, end).""" + depth, n = 0, len(text) + while i < n: + c = text[i] + if hole is not None and depth == 0: + if c == "}" and _rc_run(text, i, "}") >= hole[1]: + return i + if c == ":": + if i + 1 < n and text[i + 1] == ":": + code[i] = code[i + 1] = 1 + i += 2 + continue + return i + if c == "/" and i + 1 < n and text[i + 1] == "/": + nl = text.find("\n", i) + i = n if nl < 0 else nl + continue + if c == "/" and i + 1 < n and text[i + 1] == "*": + close = text.find("*/", i + 2) + i = n if close < 0 else close + 2 + continue + if c == "'": + i = _rc_char_literal_end(text, i) + continue + op = _rc_opener(text, i) if c in "$@\"" else None + if op is not None: + _kind, _dollars, run, body_start = op + if run == 0: + bodies.append((body_start, body_start)) + i = body_start + continue + i, body_end = _rc_scan_literal(text, body_start, code, op, bodies) + bodies.append((body_start, body_end)) + continue + if c in "([{": + depth += 1 + elif c in ")]}": + depth -= 1 + code[i] = 1 + i += 1 + return i + + +def _rc_scan_literal(text: str, i: int, code: bytearray, frame, bodies) -> "tuple[int, int]": + """(index just past the literal, index where its body ends).""" + kind, dollars, quote_run, _ = frame + n = len(text) + while i < n: + c = text[i] + if kind == "W" and c == '"': + run = _rc_run(text, i, '"') + if run >= quote_run: + return i + run, i + i += run + continue + if kind == "V" and c == '"': + if i + 1 < n and text[i + 1] == '"': + i += 2 + continue + return i + 1, i + if kind == "R" and c == "\\": + i += 2 + continue + if kind == "R" and c == '"': + return i + 1, i + if kind == "R" and c == "\n": + return i, i + if dollars > 0 and c == "{": + run = _rc_run(text, i, "{") + if dollars == 1: + i += run // 2 * 2 + if run % 2 == 0: + continue + i = _rc_scan_hole(text, i + 1, code, frame, bodies) + continue + if run >= dollars: + i = _rc_scan_hole(text, i + dollars, code, frame, bodies) + continue + i += run + continue + if dollars > 0 and c == "}": + i += _rc_run(text, i, "}") + continue + i += 1 + return i, i + + +def _rc_scan_hole(text: str, i: int, code: bytearray, frame, bodies) -> int: + dollars = frame[1] + i = _rc_scan_code(text, i, code, frame, bodies) + if i < len(text) and text[i] == ":": + while i < len(text) and not (text[i] == "}" and _rc_run(text, i, "}") >= dollars): + i += 1 + if i < len(text) and text[i] == "}": + i += min(_rc_run(text, i, "}"), dollars) + return i + + +def cs_code_and_literals(text: str) -> "tuple[str, list[tuple[int, str]]]": + """(source with comments and literal text blanked, newlines kept; [(start, body)] of every string literal) - + CSharpSourceWalker.StripCommentsAndStrings and .StringLiteralBodies, from one walk.""" + code = bytearray(len(text)) + spans: list[tuple[int, int]] = [] + _rc_scan_code(text, 0, code, None, spans) + stripped = "".join(ch if code[k] else ("\n" if ch == "\n" else " ") for k, ch in enumerate(text)) + bodies = [(s, "".join(" " if code[k] else text[k] for k in range(s, e))) for s, e in spans] + return stripped, bodies + + +def _rc_brace_end(code: str, open_at: int) -> int: + """Index just past the brace block opening at open_at (CSharpSourceWalker.BraceBalanced); len(code) if it never closes.""" + depth = 0 + for i in range(open_at, len(code)): + if code[i] == "{": + depth += 1 + elif code[i] == "}": + depth -= 1 + if depth == 0: + return i + 1 + return len(code) + + +def _rc_call_args(code: str, start: int) -> "list[str]": + args, depth, frm = [], 0, start + for i in range(start, len(code)): + c = code[i] + if c in "([{": + depth += 1 + elif c in ")]}": + if depth == 0: + args.append(code[frm:i]) + return args + depth -= 1 + elif c == "," and depth == 0: + args.append(code[frm:i]) + frm = i + 1 + return [] + + +def enumerates_repo_root_recursively(code: str, root_re: "re.Pattern[str]") -> bool: + """Whether a call such as Directory.EnumerateFiles(root, "*.cs", SearchOption.AllDirectories) starts at the repo root: + its first argument is a repo-root call or a variable the file sets from one, and it is recursive.""" + root_vars = {m.group(1) for m in _RC_ASSIGN.finditer(code) if root_re.match(m.group(2).strip())} + recursive_options = bool(_RC_RECURSE.search(code)) + for call in _RC_ENUM_CALL.finditer(code): + args = _rc_call_args(code, call.end()) + if not args: + continue + first = args[0].strip() + from_root = bool(root_re.match(first)) or first in root_vars + recursive = any("AllDirectories" in a for a in args) \ + or (recursive_options and len(args) >= 3 and "TopDirectoryOnly" not in args[-1]) + if from_root and recursive: + return True + return False + + +def _rc_bare_literal_is_a_path(text: str, start: int) -> bool: + before = text[max(0, start - 150):start] + cut = before.rfind(";") + if cut >= 0: + before = before[cut + 1:] + return "Combine(" in before or "new[]" in before or "new string[]" in before + + +def reads_lite_path(text: str) -> bool: + """GuardStageScanner.ReadsALitePath: a string literal (not a comment) naming a path segment Lite (alone, or beside a + slash), Lite.Tests or a PerformanceMonitorLite* project, or a recursive enumeration from the repo root.""" + code, bodies = cs_code_and_literals(text) + for _start, body in bodies: + norm = body.replace("\\", "/") + segs = [s for s in norm.split("/") if s] + if any(s.startswith("PerformanceMonitorLite") or s == "Lite.Tests" for s in segs): + return True + if body == "Lite" or ("/" in norm and "Lite" in segs): + return True + return enumerates_repo_root_recursively(code, _RC_ROOT_DARLING) + + +def reads_darling_path(text: str) -> bool: + """The Darling-path twin Lite.Tests had: a literal naming PerformanceMonitor.Darling.*, Darling.Tests, Darling beside a + slash, or a bare "Darling" that is a path-join argument or a segment-array element; or a recursive repo-root walk.""" + code, bodies = cs_code_and_literals(text) + for start, body in bodies: + norm = body.replace("\\", "/") + segs = [s for s in norm.split("/") if s] + if any(s.startswith("PerformanceMonitor.Darling.") or s == "Darling.Tests" for s in segs): + return True + if "/" in norm and "Darling" in segs: + return True + if body == "Darling" and _rc_bare_literal_is_a_path(text, start): + return True + return enumerates_repo_root_recursively(code, _RC_ROOT_LITE) + + +def test_classes_of_text(text: str) -> "list[str]": + """Full names (Namespace.Outer+Inner) of the classes in one source text that hold a [Fact] or [Theory] in their OWN body + (a nested class does not take its enclosing class's tests). The namespace is the nearest `namespace` line above.""" + code, _ = cs_code_and_literals(text) + found: list[tuple[str, int, int, int]] = [] + for m in _RC_CLASS.finditer(code): + open_at = code.find("{", m.start()) + if open_at >= 0: + found.append((m.group(1), m.start(), open_at, _rc_brace_end(code, open_at))) + spaces = [(m.start(), m.group(1)) for m in _RC_NAMESPACE.finditer(code)] + out: list[str] = [] + for name, idx, open_at, end in found: + own = list(code[open_at:end]) + for _n, nidx, _o, nend in found: + if nidx > open_at and nend <= end: + for k in range(nidx - open_at, nend - open_at): + own[k] = " " + if not _RC_TEST_ATTR.search("".join(own)): + continue + outers = sorted((f for f in found if f[2] < idx and f[3] >= end and f[1] != idx), key=lambda f: f[2]) + ns = next((n for pos, n in reversed(spaces) if pos < idx), "") + out.append((ns + "." if ns else "") + "+".join([f[0] for f in outers] + [name])) + return out + + +def _rc_project_links(project: str) -> "list[str]": + """The files a csproj compiles in through an explicit Compile Include with a Link (wildcards skipped).""" + try: + with open(project, encoding="utf-8-sig") as fh: + xml = fh.read() + except OSError: + return [] + base = os.path.dirname(project) + out = [] + for m in re.finditer(r"]*Include=\"([^\"*]+)\"[^>]*Link=\"[^\"]+\"", xml): + out.append(os.path.normpath(os.path.join(base, m.group(1).replace("\\", os.sep)))) + return out + + +def _rc_suite_files(root: str, suite: str) -> "list[str]": + base = os.path.join(root, SUITE_DIRS[suite]) + files = [] + for dp, dn, fn in os.walk(base): + dn[:] = [d for d in dn if d not in ("bin", "obj")] + files += [os.path.join(dp, f) for f in fn if f.endswith(".cs")] + project = os.path.join(base, ("Darling.Tests" if suite == "darling" else "Lite.Tests") + ".csproj") + have = set(files) + files += [f for f in _rc_project_links(project) if f not in have] + return sorted(files) + + +def reads_classes(root: str = ROOT, suites: "tuple[str, ...]" = ("darling", "lite")) -> "dict[str, set[str]]": + """{"darling": full names of the Darling.Tests classes that read a Lite path, "lite": full names of the Lite.Tests + classes that read a Darling path}. Replaces the [Trait("Reads", "Lite")] and [Trait("Reads", "Darling")] attributes.""" + out: dict[str, set[str]] = {"darling": set(), "lite": set()} + for suite, detector in (("darling", reads_lite_path), ("lite", reads_darling_path)): + for path in (_rc_suite_files(root, suite) if suite in suites else []): + try: + with open(path, encoding="utf-8-sig") as fh: + text = fh.read().replace("\r\n", "\n") + except OSError: + continue + # A file that names neither tree and calls no file enumeration cannot read a path: skip the character walk. + if not _RC_CHEAP[suite].search(text) or not _RC_TEST_ATTR.search(text): + continue + if detector(text): + out[suite].update(test_classes_of_text(text)) + return out + + +def with_reads_traits(classes: "dict[str, dict[str, set[str]]]", root: str = ROOT) -> None: + """Put Reads=Lite / Reads=Darling on the scanned classes the source scan names, in place (the rules in class_selected()).""" + reads = reads_classes(root) + for suite, trait in (("darling", "Reads=Lite"), ("lite", "Reads=Darling")): + simple = {re.split(r"[.+]", n)[-1] for n in reads[suite]} + for name, traits in classes[suite].items(): + if name in simple: + traits.add(trait) + + # --------------------------------------------------------------------------------------------------------- # Slow classes: pull requests skip them unless the change reaches them (#5459, change 5). # --------------------------------------------------------------------------------------------------------- @@ -434,7 +867,8 @@ def class_selected(suite: str, traits: set[str], d: dict) -> bool: _BUILD_INPUT_NAMES = {"directory.build.props", "directory.build.targets", "directory.packages.props", "global.json", "nuget.config", "packages.lock.json"} _BUILD_INPUT_PATHS = {".github/workflows/build.yml", ".github/scripts/ci-select.py", ".github/darling-paths-filter.yml", - ".github/scripts/run-darling-pg-shard.ps1", ".github/scripts/run-lite-shard.ps1", + ".github/lite-paths-filter.yml", ".github/scripts/run-darling-pg-shard.ps1", ".github/scripts/run-lite-shard.ps1", + ".github/scripts/darling-shard-pack.py", ".github/scripts/darling-shard-weights.json", ".github/scripts/lite-shard-pack.py"} _BUILD_INPUT_SUFFIXES = (".csproj", ".props", ".targets", ".sln", ".slnx") _WEB_SUFFIXES = (".js", ".mjs", ".css", ".html") @@ -629,6 +1063,86 @@ def replay(corpus: str = CORPUS, root: str = ROOT, quiet: bool = False) -> int: "**/xunit.runner.json", ) +# The shared projects (#5951). A change under one of them used to run everything, because both are referenced by +# every product and a compiled `const` or `enum` value is copied into the caller at build time, so the map's call +# edges cannot see the caller change. The measured price was 182 merged dev pull requests in four weeks, all FULL, 71 +# of which a map selection would have narrowed to a median 1,180 classes. The rule now reads the diff: FULL only for +# a change to a constant or an enum, or to a registration file below; any other change selects by the map's edges. +SHARED_PROJECT_DIRS = ("PerformanceMonitor.Common/", "PerformanceMonitor.PlanAnalysis/") + +# The files of the shared projects that register or discover types, or load a resource by name, so a census class +# sees the change without any edge to the file: a change to one of them is FULL (#5951). Each has the one-line reason. +# Found with: grep -rnE "AddSingleton|AddScoped|AddTransient|GetTypes\(\)|Assembly\." under the two projects. +REGISTRATION_FILES = { + "PerformanceMonitor.Common/Mcp/McpSchemaCompat.cs": + "registers the MCP tool catalog and parameter types in the service collection (AddSingleton) and discovers " + "the tools by reflecting over a tool type's methods and attributes", + "PerformanceMonitor.Common/Mcp/McpToolParameterTypes.cs": + "holds the registry the reflection scan in McpSchemaCompat fills from each tool method's parameters", + "PerformanceMonitor.Common/Services/UpdateCheckService.cs": + "reads the running build's version from the entry assembly (Assembly.GetEntryAssembly)", + "PerformanceMonitor.PlanAnalysis/WaitStatsConfig.cs": + "loads the embedded Resources/WaitStats.json by manifest resource name (Assembly.GetManifestResourceNames)", +} + +# A `const` declaration on a diff line (modifiers and attributes allowed before it), and an `enum` declaration. +_CONST_DECL = re.compile(r"^\s*(?:\[[^\]]*\]\s*)*(?:(?:public|private|protected|internal|static|new|unsafe)\s+)*const\s+\S+\s+\w+\s*=") +_ENUM_DECL = re.compile(r"^\s*(?:\[[^\]]*\]\s*)*(?:(?:public|private|protected|internal|new)\s+)*enum\s+\w+") +_ENUM_HEADER = re.compile(r"\benum\b") + + +def shared_change_reason(shared: list[str], diff_text: "str | None", + registrations: "dict[str, str]" = REGISTRATION_FILES) -> "str | None": + """Why a change to these shared-project files runs everything (#5951), or None when the map may select by edges. + + A file in `registrations` is FULL. Otherwise `diff_text` (`git diff -U0` with the csharp driver's hunk headers, as + symbol_diff_text() builds it) is read: FULL when a `+` or `-` line declares a `const` or an `enum`, or a hunk + sits inside an enum body (its header is the enum's declaration, the nearest type or member line above it). + Without a diff, or for a `.cs` file the diff does not hold, the answer is FULL: a change that is not read is not + narrowed. A shared file that is not C# holds no constant or enum, so it is never FULL for this rule.""" + for f in shared: + if f in registrations: + return f"{f} is a shared-project registration file ({registrations[f]})" + cs_files = [f for f in shared if f.endswith(".cs")] + if not cs_files: + return None + if diff_text is None: + return f"{cs_files[0]} is in a shared project and its diff could not be read (keep-full)" + sections: dict[str, list[str]] = {} + current: "list[str] | None" = None + in_header = False + for line in diff_text.split("\n"): + if line.startswith("diff --git "): + m = re.match(r"diff --git a/(.+?) b/(.+)$", line) + current = sections.setdefault(m.group(2), []) if m else None + in_header = True + elif current is None: + continue + elif line.startswith("@@"): + in_header = False + current.append(line) + elif not in_header and line[:1] in ("+", "-"): + current.append(line) + for f in cs_files: + lines = sections.get(f) + if lines is None: + return f"{f} is in a shared project and the diff does not hold it (keep-full)" + for ln in lines: + if ln.startswith("@@"): + header = ln.split("@@", 2)[2] if ln.count("@@") >= 2 else "" + if _ENUM_HEADER.search(header): + return f"{f} changes an enum member (a compiled value the map's edges cannot see)" + elif _CONST_DECL.match(ln[1:]): + return f"{f} changes a const (a compiled value the map's edges cannot see): {ln[1:].strip()[:80]}" + elif _ENUM_DECL.match(ln[1:]): + return f"{f} adds or removes an enum (a compiled value the map's edges cannot see)" + return None + + +# map_select()'s `reason` for a change that holds documentation files only (#5894). +DOCS_ONLY_REASON = ("documentation-only change: only a class that reads a changed document as text is selected " + "(CI's docs fast path runs no test)") + # The documentation allowlist (the `docs` filter in build.yml); selection_args() passes the live one. DOC_PATTERNS = ( "**/*.md", @@ -698,11 +1212,45 @@ def _parse_time(text: object) -> "datetime.datetime | None": return t if t.tzinfo else t.replace(tzinfo=datetime.timezone.utc) +def _pattern_hit(g: object, live: list[str], docs_changed: list[str]) -> bool: + """Whether a class's text patterns `g` (a glob list) match a changed file: `live` are the changed files outside + the documentation allowlist, `docs_changed` the rest.""" + if not isinstance(g, list): + return False + # A class that reads a documentation file by name (the migration ladder reads CHANGELOG.md) is selected by a + # change to exactly that file; the documentation allowlist only keeps wildcard patterns from matching it. + return (any(matches(p, f) for p in g for f in live) + or any(p == f for p in g if "*" not in p for f in docs_changed)) + + +def text_pattern_classes(test_map: object, changed: list[str], doc_patterns: "tuple[str, ...] | list[str]") -> dict: + """{suite: {Class, ...}} of the map's classes whose text patterns (a glob list, not "tree") match a changed + file: the classes map_select() gives the reason "text pattern" (#5798). Read straight from the map, so it + answers when map_select() is FULL too; an unreadable map gives nothing.""" + if not isinstance(test_map, dict): + return {} + classes, patterns = test_map.get("classes"), test_map.get("text_patterns") + if not isinstance(classes, dict) or not isinstance(patterns, dict): + return {} + live = [f for f in changed if not any(matches(p, f) for p in doc_patterns)] + live_names = set(live) + docs_changed = [f for f in changed if f not in live_names] + out: dict[str, set[str]] = {} + for suite, entries in classes.items(): + for cls in entries if isinstance(entries, dict) else (): + g = patterns.get(cls) + if isinstance(g, list) and all(isinstance(p, str) for p in g) and _pattern_hit(g, live, docs_changed): + out.setdefault(suite, set()).add(cls) + return out + + def map_select(test_map: object, changed: list[str], drift: "list[str] | None", discovered: dict, *, event: str = "pull_request", keep_full: "tuple[str, ...] | list[str]" = KEEP_FULL, doc_patterns: "tuple[str, ...] | list[str]" = DOC_PATTERNS, now: "datetime.datetime | None" = None, max_age_days: "float | None" = MAP_MAX_AGE_DAYS, - max_drift: int = MAP_MAX_DRIFT, base_ref: str = "") -> dict: + max_drift: int = MAP_MAX_DRIFT, base_ref: str = "", + named: "dict[str, set[str]] | None" = None, guard_stage_separate: bool = False, + shared_diff: "str | None" = None, shared_dirs: "tuple[str, ...]" = SHARED_PROJECT_DIRS) -> dict: """Which test classes a pull request runs, from a class-to-file map. Pure: no file, git or clock access (`now` is the clock; `max_age_days=None` turns the age check off, for a replay of old runs). @@ -717,12 +1265,27 @@ def map_select(test_map: object, changed: list[str], drift: "list[str] | None", Arguments: `changed` is the pull request's files; `drift` the files changed between the map's commit and the merge base (None when they could not be computed); `discovered` is `{suite: {Class: {"Key=Value", ...}}}` as - scan_classes() returns it, which is how a class the map has never seen is found. + scan_classes() returns it, which is how a class the map has never seen is found. `named` is `{suite: {Class, ...}}`: + the test classes whose source names a symbol the pull request declares or changes (symbol_classes(), tight tokens + only, #5776); None or empty adds nothing. A class is selected when it carries Stage=Guard, its own file changed, it covers a changed file, a text pattern - of it matches a changed file, it reads the whole tree and any non-documentation file changed, or the map has - never seen it. A file changed since the map counts as changed too (it may have grown new call edges). - A keep-full file forces FULL only when the pull request's own change holds it: one that changed on dev since + of it matches a changed file, it reads the whole tree and any non-documentation file changed, the map has + never seen it and it holds a test, it shares an xunit collection (of at most COLLECTION_MATE_CAP classes) with a + class whose own file changed, or its source names a changed symbol (`named`). The last reason only adds classes, + so a selection never shrinks for it. With `guard_stage_separate` (#5894) the caller runs the whole Stage=Guard + trait as a stage of its own (the pre-push gate does), so a Stage=Guard class is left out of `selected` and + `seconds`: `why` keeps it, marked "run in the guard stage", and the answer says `guard_stage_separate`. A pull + request whose every changed file is documentation (`doc_patterns`) selects only the classes that read a changed + document as text (no Guard, new or environment-gated class, no changed symbol); the answer says `docs_only` and + `reason` says why: CI's docs fast path runs nothing for it either (#5894). A file changed since the map counts as changed too (it may have grown new + call edges), but only when the change holds a product source file the map knows (#5872). `via` names, per + "covers a changed file" class, the changed file the walk came from; `unknown` names each file the map did not know + and the rule that took it; `drift` is how many drifted files counted. + A change under a shared project (`shared_dirs`, #5951) is not keep-full as a whole: it is FULL only when + shared_change_reason() finds a registration file, or a const or enum in `shared_diff` (the pull request's + `-U0` diff text; None means the diff could not be read, which is FULL). Any other shared file selects by the + map's edges. A keep-full file forces FULL only when the pull request's own change holds it: one that changed on dev since the map goes through the class links like any other drifted file. Returns `{"full": bool, "reason": str, "selected": {suite: [Class, ...]}, "why": {suite: {Class: reason}}, @@ -765,10 +1328,11 @@ def map_select(test_map: object, changed: list[str], drift: "list[str] | None", for f in changed: if any(matches(p, f) for p in keep_full): return _full(f"{f} is a keep-full file") - effective = list(dict.fromkeys(list(changed) + list(drift))) - live = [f for f in effective if not any(matches(p, f) for p in doc_patterns)] - live_names = set(live) - + shared = [f for f in changed if f.startswith(shared_dirs) and not f.endswith(".md")] + if shared: + why_shared = shared_change_reason(shared, shared_diff) + if why_shared: + return _full(why_shared) known: set[str] = {u for u in universe if isinstance(u, str)} parsed: dict[str, dict[str, tuple[set[str], set[str]]]] = {} for suite, entries in classes.items(): @@ -788,20 +1352,36 @@ def map_select(test_map: object, changed: list[str], drift: "list[str] | None", else: return _full("test map unreadable") + # #5872: the drift (files changed on dev between the map's commit and the merge base) counts as changed only when + # this pull request's own change holds a product source file the map knows. Drift is there to catch a drifted file + # that grew a call edge INTO the change, which the map's links cannot show yet; nothing grows an edge into a doc, + # a script or workflow under .github/, or a test file. A drifted test that newly calls a changed test helper + # names one of its symbols, so `named` (the changed-symbol reason below) still selects it, and a test class added + # since the map runs as a "new class" whatever the drift. Counting the drift for every change made a one-file test, + # docs or script change select nearly the whole tree (52-84 drifted files: about 2,300 of 2,870 Darling classes). + # A miss here is caught by the push to dev, which runs everything (narrows() is False for a push). + drift_applies = any(f in known and not _TEST_PROJECT_FILE.search(f) and not f.startswith(".github/") + and not any(matches(p, f) for p in doc_patterns) for f in changed) + effective = list(dict.fromkeys(list(changed) + (list(drift) if drift_applies else []))) + live = [f for f in effective if not any(matches(p, f) for p in doc_patterns)] + live_names = set(live) docs_changed = [f for f in effective if f not in live_names] def text_hit(cls: str) -> bool: g = globs.get(cls) if g == "tree": return bool(live) - if not isinstance(g, list): - return False - # A class that reads a documentation file by name (the migration ladder reads CHANGELOG.md) is selected by a - # change to exactly that file; the documentation allowlist only keeps wildcard patterns from matching it. - return (any(matches(p, f) for p in g for f in live) - or any(p == f for p in g if "*" not in p for f in docs_changed)) + return _pattern_hit(g, live, docs_changed) + + # #5894: a change of documentation files only. CI's `docs` filter takes the docs fast path for it (decide(): + # docs_fastpath, no guard run, no shards) and runs nothing, so the selection holds only the classes that read a + # changed document as text (#5872: a class pinned to README.md still runs for it): no Guard class, no "new class", + # no class gated on an environment, no changed symbol. + docs_only = bool(changed) and not live new_classes = any(cls not in parsed.get(suite, {}) for suite, found in discovered.items() for cls in found) + # A file the map does not know is named with the rule that caught it (#5872), in the FULL reason or in `unknown`. + unknown: dict[str, str] = {} for f in live: if f in known: continue @@ -809,9 +1389,17 @@ def text_hit(cls: str) -> bool: continue if f.endswith(".cs"): if _TEST_PROJECT_FILE.search(f) and new_classes: + unknown[f] = "selecting the new test classes the tree scan finds" continue # a new test file: its classes are found through `discovered` - return _full(f"new file not in the map: {f}") - return _full(f"{f} is in no class, no text pattern and not in the map") + # Types found by reflection, DI or attribute scanning change what a census class sees with no covered + # file changing, so a new product .cs file still runs everything. + return _full(f"unknown file: {f}: selecting everything (new file not in the map)") + if f.startswith(".github/"): + # A script or workflow under .github/ that no class covers and no text pattern reads cannot change a test + # class's outcome (the keep-full build.yml returned FULL above), so it selects nothing of its own. + unknown[f] = "selecting the classes that read it as text (none)" + continue + return _full(f"unknown file: {f}: selecting everything (in no class, no text pattern and not in the map)") # A class the map has never seen runs because `discovered` lists it. If the tree scan found no class at all for a # suite the map has classes for, the scan itself is broken and every new class would be silently left out. @@ -820,6 +1408,7 @@ def text_hit(cls: str) -> bool: return _full(f"no test class was found in the tree for suite {suite}") live_set = set(live) + via: dict[str, dict[str, str]] = {} selected: dict[str, list[str]] = {} why: dict[str, dict[str, str]] = {} seconds: dict[str, float] = {} @@ -833,20 +1422,54 @@ def text_hit(cls: str) -> bool: reasons[cls] = "own file changed" elif live_set & covered: reasons[cls] = "covers a changed file" + # #5872: the changed file (a shared base class, fixture or product file) the dependency walk came from. + via.setdefault(suite, {})[cls] = sorted(live_set & covered)[0] elif text_hit(cls): reasons[cls] = "reads the tree" if globs.get(cls) == "tree" else "text pattern" - for cls, traits in found.items(): + for cls, traits in ([] if docs_only else found.items()): if cls not in mapped: - reasons[cls] = "new class" + # #5872: a helper type (a fake, a logger, a harness) declares no test, so the map never has it and the + # runner runs nothing for it; only a class that holds or inherits a test is new. + if NO_TESTS_TRAIT not in traits: + reasons[cls] = "new class" elif "Stage=Guard" in traits: reasons.setdefault(cls, "guard stage") - if GATE_TRAIT in traits: + if GATE_TRAIT in traits and NO_TESTS_TRAIT not in traits: reasons.setdefault(cls, "gated on an environment the map shards lack") - selected[suite] = sorted(reasons) - why[suite] = {c: reasons[c] for c in sorted(reasons)} - seconds[suite] = round(sum(_seconds(classes[suite][c]) for c in reasons if c in mapped), 3) + # #5872: a changed test class's xunit collection mates run too: they share its fixture and its serialized + # statics, and moving a class into a collection (a changed [Collection] attribute) changes what runs beside + # them. A collection over COLLECTION_MATE_CAP classes (live-postgres, about 650) is a serialization bucket for + # one shared store, not a fixture its members read state from, so its mates are not added. + members: dict[str, list[str]] = {} + for cls, traits in found.items(): + if NO_TESTS_TRAIT not in traits: + for t in traits: + if t.startswith(COLLECTION_PREFIX): + members.setdefault(t, []).append(cls) + for cls in [c for c, r in reasons.items() if r == "own file changed"]: + for t in sorted(x for x in found.get(cls, ()) if x.startswith(COLLECTION_PREFIX)): + if len(members.get(t, ())) <= COLLECTION_MATE_CAP: + for mate in members.get(t, ()): + reasons.setdefault(mate, "shares a collection with a changed class") + # #5776: a class whose source names a symbol the change declares or edits runs too. The map links a class to + # the files it RAN; a class that only reads a source file as text, or reflects over a catalog, has no such + # link, and a small pull request whose files the map knows would leave it out. Last, with setdefault, so a + # class keeps the reason it already had and the selection only grows. + for cls in sorted({} if docs_only else (named or {}).get(suite, ())): + if cls in mapped or cls in found: + reasons.setdefault(cls, "names a changed symbol") + # #5894: the caller runs the whole Stage=Guard trait on its own, so a Guard class in the selection would run twice. + # It keeps its `why` entry (the printed summary still counts it) but is not selected. + in_guard_stage = {c for c in reasons if "Stage=Guard" in found.get(c, ())} if guard_stage_separate else set() + kept = sorted(c for c in reasons if c not in in_guard_stage) + selected[suite] = kept + why[suite] = {c: ("run in the guard stage" if c in in_guard_stage else reasons[c]) for c in sorted(reasons)} + seconds[suite] = round(sum(_seconds(classes[suite][c]) for c in kept if c in mapped), 3) total[suite] = len(set(mapped) | set(found)) - return {"full": False, "reason": "", "selected": selected, "why": why, "seconds": seconds, "total": total} + return {"full": False, "reason": DOCS_ONLY_REASON if docs_only else "", "selected": selected, "why": why, + "seconds": seconds, "total": total, "via": via, "drift": len(drift) if drift_applies else 0, + "unknown": unknown, "docs_only": docs_only, + "guard_stage_separate": guard_stage_separate} def load_map(path: str) -> "dict | None": @@ -873,30 +1496,77 @@ def selection_args(rules: Rules) -> dict: """The keep-full and documentation patterns, read from the same filters the workflow evaluates.""" keep = list(KEEP_FULL) for name in ("root", "core", "installer_core"): - keep.extend(rules.build.get(name, [])) + # #5951: the shared projects are judged by map_select()'s shared_change_reason(), not as keep-full wholesale. + keep.extend(p for p in rules.build.get(name, []) if not p.startswith(SHARED_PROJECT_DIRS)) return {"keep_full": tuple(dict.fromkeys(keep)), "doc_patterns": tuple(rules.build["docs"])} +def _median(values: list) -> float: + v = sorted(values) + n = len(v) + return 0.0 if not n else float(v[n // 2]) if n % 2 else (v[n // 2 - 1] + v[n // 2]) / 2 + + +def _selected_count(sel: dict, size: int) -> int: + """How many classes a map_select() answer runs: `size` (every class of the tree) when it is FULL.""" + return size if sel["full"] else sum(len(v) for v in sel["selected"].values()) + + def map_replay(test_map: "dict | None", corpus: str = CORPUS, root: str = ROOT, quiet: bool = False, - misses_out: "str | None" = None) -> int: + misses_out: "str | None" = None, symbols_base: "str | None" = None) -> int: """Like replay(), but asks the map: would each failing class of each corpus row have been selected, taking the map as current (no drift, no age check)? Returns the miss count. Reports only; the caller does not fail on it yet. The printed list is capped at 40; `misses_out` names a file that gets EVERY miss as one JSON row per line - (date, event, head_sha, suite, class, changed_files), so none goes unclassified (#5459).""" + (date, event, head_sha, suite, class, changed_files), so none goes unclassified (#5459). + + With `symbols_base` (#5798), each row whose head commit this clone has is also asked with the classes that name + a symbol the commit changed against its merge base with `symbols_base` (map_select()'s `named`), and the + report prints the classes selected per row before and after that rule (median and maximum) and how many the + rule names on its own. A miss is still counted on the selection without it, so the rule cannot hide one.""" extra = selection_args(Rules()) accepted = load_accepted() tree = scan_classes(root) + if isinstance(test_map, dict) and isinstance(test_map.get("classes"), dict): + tree_sel = {s: v for s, v in tree.items() if s in test_map["classes"]} + else: + tree_sel = tree rows = checked = misses = full_rows = accepted_seen = 0 missed: list[str] = [] rows_out: list[dict] = [] + index = test_file_index(root) if symbols_base else None + size = sum(len(v) for v in tree_sel.values()) + before: list[int] = [] + after: list[int] = [] + named_counts: list[int] = [] + partial_before: list[int] = [] + partial_after: list[int] = [] + no_commit = 0 with open(corpus, encoding="utf-8") as fh: for line in fh: if not line.strip(): continue row = json.loads(line) rows += 1 - sel = map_select(test_map, row["changed_files"], [], tree, event=row["event"], max_age_days=None, **extra) + sdiff = commit_diff_text(root, symbols_base, row["head_sha"]) if symbols_base else None + sel = map_select(test_map, row["changed_files"], [], tree, event=row["event"], max_age_days=None, + shared_diff=sdiff, **extra) full_rows += bool(sel["full"]) + if symbols_base: + named = symbol_classes(root, symbols_base, head=row["head_sha"], index=index) + if named is None: + no_commit += 1 + else: + with_named = map_select(test_map, row["changed_files"], [], tree_sel, event=row["event"], + max_age_days=None, named=named, shared_diff=sdiff, **extra) + without = map_select(test_map, row["changed_files"], [], tree_sel, event=row["event"], + max_age_days=None, shared_diff=sdiff, **extra) + b, a = _selected_count(without, size), _selected_count(with_named, size) + before.append(b) + after.append(a) + named_counts.append(sum(len(v) for v in named.values())) + if not without["full"]: + partial_before.append(b) + partial_after.append(a) for cls in row["failed_classes"]: prefix, simple = split_failed_class(cls) suite = SUITE_BY_PREFIX.get(prefix) @@ -920,6 +1590,18 @@ def map_replay(test_map: "dict | None", corpus: str = CORPUS, root: str = ROOT, print("MAP-MISS", m) if len(missed) > 40: print(f"... {len(missed) - 40} more misses not printed; --misses-out FILE lists every one") + if symbols_base: + print(f"symbol rule (#5798): {len(before)} rows measured ({no_commit} without their head commit here), " + f"{size} classes in the tree, cap {symbol_cap(index)} test files") + if before: + print(f" selected per row: before median {_median(before):g} max {max(before)}, after median " + f"{_median(after):g} max {max(after)}") + print(f" the rule names on its own: median {_median(named_counts):g} max {max(named_counts)}") + if partial_before: + added = [a - b for a, b in zip(partial_after, partial_before)] + print(f" rows not FULL ({len(partial_before)}): before median {_median(partial_before):g} max " + f"{max(partial_before)}, after median {_median(partial_after):g} max {max(partial_after)}, " + f"added median {_median(added):g} max {max(added)}") if misses_out: with open(misses_out, "w", encoding="utf-8", newline="\n") as out: for r in rows_out: @@ -940,7 +1622,10 @@ def map_keep(selection: object, suite: str) -> "tuple[set[str] | None, str]": return None, f"the selection is FULL ({selection.get('reason') or 'no reason given'})" chosen = selection.get("selected") names = chosen.get(suite) if isinstance(chosen, dict) else None - if not isinstance(names, list) or not names or not all(isinstance(n, str) and n for n in names): + # #5894: an empty list is trusted only when the selection says why it is empty: Guard classes left out because the + # caller runs that stage itself, or a documentation-only change. CI's selection sets neither. + explained = selection.get("guard_stage_separate") is True or selection.get("docs_only") is True + if not isinstance(names, list) or (not names and not explained) or not all(isinstance(n, str) and n for n in names): return None, f"the selection holds no usable class list for the {suite} suite" return set(names), "" @@ -1074,6 +1759,673 @@ def shadow_row(selection: "dict | None", failed: "dict[str, set[str]]", seen: "d # CLI. # --------------------------------------------------------------------------------------------------------- +# --------------------------------------------------------------------------------------------------------- +# Changed symbols: the test classes whose source names what a change declares or edits (#5776). +# --------------------------------------------------------------------------------------------------------- +# +# Most test classes that went red on a lane's pull request after it passed the lane's own runs named a symbol the +# change declared or edited: a source scan that greps for `_fleetGateStats?.RecordSlot(`, a pin on a constant's +# value, a census over a method's callers. The class-to-file map does not link them (they read the file as text or +# reflect over it), and on a big change its coverage answer picks 80% or more of every class, which no lane runs. +# `local-classes` lists the classes that name a changed symbol, most specific first, for a lane to run before it +# pushes; map_select() takes the same answer (tight tokens only) as one more reason to select a class. +# +# Tokens. TIGHT: the names a changed .cs/.js/.xaml line declares (class, record, struct, enum, interface, const, +# method, property, field), the member a C# hunk sits in (its hunk header, with git's csharp driver), and SQL object +# names (v_* anywhere, CREATE/ALTER TABLE/VIEW/FUNCTION/PROCEDURE in .sql). WIDE adds every identifier of 6 or more +# characters on a changed line. A token taken only from test files is dropped (a test's own names select nothing +# else), and so is a stop word or a name shorter than 4 characters. A token named in more than `cap` test files says +# too little to choose by: it is dropped and printed as NOISY. A class declared in a changed test file is listed too. +# +# What it cannot see: a class that names no changed symbol at all (it reflects over the whole tool catalog or the +# collector set); those carry Stage=Guard and run in the guard stage, except the few slow ones +# on its SlowCatalogClasses list, which the shards run. Matching is per test FILE and case-sensitive on +# whole words, so every class of a file that names the token is listed. + +SYMBOL_EXTENSIONS = (".cs", ".sql", ".js", ".xaml") +SYMBOL_SUITES = ("darling", "lite") +SYMBOL_CAP = 25 +# #5798: the cap is a share of the test files, so it grows with the tree: 1% of the 3,023 test files of 2026-10-14 +# is 30. SYMBOL_CAP is its floor. Measured on the corpus (`--replay --map FILE --symbols origin/dev`, 414 rows), +# the rule names a median of 51 classes on its own (at most 700, 18% of the 3,826) and adds a median of 1 (at most +# 202) to a map answer that is not FULL, well under the third of the tree that would call for a tighter cap. +SYMBOL_CAP_SHARE = 0.01 +SYMBOL_MIN_LEN = 4 +WIDE_MIN_LEN = 6 +LOCAL_CLASS_LIMIT = 300 +RIG_LITERAL = '"DARLING_TEST_PG"' # the quoted variable name a live class reads to skip without a store + +_CS_KEYWORDS = frozenset(""" +abstract as base bool break byte case catch char checked class const continue decimal default delegate do double else +enum event explicit extern false finally fixed float for foreach goto if implicit in int interface internal is lock +long namespace new null object operator out override params private protected public readonly ref return sbyte sealed +short sizeof stackalloc static string struct switch this throw true try typeof uint ulong unchecked unsafe ushort using +virtual void volatile while add alias and ascending async await by descending dynamic equals from get global group +init into join let managed nameof nint not notnull nuint on or orderby partial record remove required scoped select set +unmanaged value var when where with yield function export import const let undefined +""".split()) +_TEST_API_WORDS = frozenset(""" +Assert Equal NotEqual True False Null NotNull Contains DoesNotContain Empty NotEmpty Single Throws ThrowsAsync ThrowsAny +ThrowsAnyAsync Same NotSame InRange IsType IsAssignableFrom StartsWith EndsWith Matches DoesNotMatch Fact Theory +InlineData MemberData ClassData Trait Skip SkipUnless Collection CollectionDefinition IClassFixture ICollectionFixture +IAsyncLifetime IDisposable IAsyncDisposable Dispose DisposeAsync InitializeAsync Xunit ITestOutputHelper Fixture +TestContext Task ValueTask CancellationToken ToString Equals GetHashCode Length Count Any All First Select Where +ToList ToArray String Exception Console Regex +""".split()) +_COMMON_WORDS = frozenset(""" +Server Servers Collector Collectors Query Queries Name Names Value Values Type Data Result Results Text Message Error +Errors Status Time Start Stop Read Write Item Items List Keys Source Target Path File Files Line Lines Store Service +Options Settings Config Connection Command Database Table Index Logger Test Tests Main Helper Model State Info Context +Default Event Events Window View Builder Reader Writer Parse Format Create Update Delete Execute Build Check Close Open +""".split()) +STOP_WORDS = _CS_KEYWORDS | _TEST_API_WORDS | _COMMON_WORDS + +_MODIFIERS = (r"(?:public|private|protected|internal|static|readonly|const|async|override|virtual|sealed|abstract|" + r"partial|new|extern|unsafe|volatile|required|file|event|implicit|explicit)") +_DECL_TYPE = re.compile(r"\b(?:class|record|struct|enum|interface)\s+(?:class\s+|struct\s+)?([A-Za-z_]\w*)") +_DECL_CONST = re.compile(r"\bconst\s+[\w<>\[\]?,.\s]+?\s([A-Za-z_]\w*)\s*=") +_DECL_MEMBER = re.compile(r"^\s*(?:\[[^\]]*\]\s*)*(?:" + _MODIFIERS + r"\s+)+[\w<>\[\]?,.()\s]*?([A-Za-z_]\w*)\s*" + r"(?:<[^>]*>\s*)?(?:\(|\{|=>|=|;)") +# A method without a modifier (`Task ReadAsync(`): the first word must be a type, not a statement keyword, and +# the line must not end a statement (`return Foo(x);` calls, it does not declare). +_DECL_BARE = re.compile(r"^\s*([\w.]+(?:<[^>]*>)?(?:\[\])?\??)\s+([A-Za-z_]\w*)\s*(?:<[^>]*>\s*)?\((?!.*;\s*$)") +_BARE_NOT_TYPE = frozenset("return await new throw else yield case goto using in is as not and or when var typeof " + "nameof if while for foreach switch lock catch".split()) +_JS_DECL = re.compile(r"\bfunction\s*\*?\s*([A-Za-z_$][\w$]*)|\b(?:const|let|var|class)\s+([A-Za-z_$][\w$]*)" + r"|^\s*(?:async\s+)?([A-Za-z_$][\w$]*)\s*\([^)]*\)\s*\{|([A-Za-z_$][\w$]*)\s*:\s*(?:async\s+)?function") +_XAML_DECL = re.compile(r'x:Name="(\w+)"|x:Class="[\w.]*?(\w+)"|\{Binding\s+(?:Path=)?(\w+)') +_SQL_OBJECT = re.compile(r"\b(?:CREATE|ALTER)\s+(?:OR\s+(?:REPLACE|ALTER)\s+)?(?:MATERIALIZED\s+)?" + r"(?:TABLE|VIEW|FUNCTION|PROCEDURE|PROC)\s+(?:IF\s+NOT\s+EXISTS\s+)?([\w\[\]\".]+)", re.I) +_SQL_VIEW = re.compile(r"\bv_\w+") +# #5798: a store migration is `private const string V183Sql = ...` plus `new Migration(183, ...)`; the tests name it +# V183 (in a rung pin, a sentinel or a comment), so both give the token V183. `V183Sql` matches a reference too +# (`sb.Append(V183Sql)`), wanted: a line that uses a migration's SQL touches that migration. The V-number comes on +# top of what the other rules take from the line (`new Migration(` still gives `Migration`, which so many test files +# name that the cap drops it as noisy): a rule here may only add tokens, never take one away, or the selection +# could run fewer classes than before. A known limit: an edit inside an existing migration's verbatim SQL body +# (the lines between `V183Sql = @"` and its closing quote) declares nothing, so it gives no token. +_MIGRATION = re.compile(r"\bV(\d+)Sql\b|\bnew\s+Migration\s*\(\s*(\d+)\s*,") +_WORD = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +_HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+\d+(?:,\d+)? @@ ?(.*)$") + + +def _git(root: str, *args: str) -> "str | None": + try: + return subprocess.run(["git", "-C", root, *args], capture_output=True, text=True, encoding="utf-8", + errors="replace", timeout=120, check=True).stdout + except (OSError, subprocess.SubprocessError): + return None + + +def symbol_diff_text(root: str, *revs: str) -> "str | None": + """`git diff -U0 --no-renames ` over the symbol file types, plus the .cs part a second time for its hunk + headers. The repository's .gitattributes sets no diff driver, so a C# hunk header is the last column-0 line + above it: the type of a file-scoped namespace. The first pass uses git's built-in csharp driver, whose header is + the member the hunk sits in; the second keeps the default, so both the member and its type are read. None when + git fails.""" + fd, attrs = tempfile.mkstemp(suffix=".gitattributes") + try: + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write("*.cs diff=csharp\n") + members = _git(root, "-c", f"core.attributesFile={attrs}", "diff", "-U0", "--no-renames", *revs, "--", + *[f"*{e}" for e in SYMBOL_EXTENSIONS]) + finally: + os.remove(attrs) + types = _git(root, "diff", "-U0", "--no-renames", *revs, "--", "*.cs") + if members is None or types is None: + return None + headers = [ln for ln in types.split("\n") if ln.startswith(("diff --git ", "--- ", "+++ ", "@@ "))] + return members + "\n" + "\n".join(headers) + + +def symbol_diff(root: str, base: str, untracked: bool = True) -> "tuple[str, str, list[str]] | None": + """(merge base, symbol_diff_text() of the WORKING TREE against it, every changed path) for `base`, so it answers + before a commit too. With `untracked`, a new file git does not track yet joins as all added lines. None when git + fails.""" + mb = _git(root, "merge-base", base, "HEAD") + if mb is None or not mb.strip(): + return None + mb = mb.strip() + text = symbol_diff_text(root, mb) + names = _git(root, "diff", "--name-only", "--no-renames", mb) + if text is None or names is None: + return None + paths = [ln.strip() for ln in names.split("\n") if ln.strip()] + if untracked: + extra = _git(root, "ls-files", "--others", "--exclude-standard") or "" + for rel in (ln.strip() for ln in extra.split("\n")): + if not rel or rel in paths: + continue + paths.append(rel) + if not rel.endswith(SYMBOL_EXTENSIONS): + continue + try: + with open(os.path.join(root, rel), encoding="utf-8-sig", errors="replace") as fh: + body = fh.read().split("\n") + except OSError: + continue + text += f"diff --git a/{rel} b/{rel}\n--- /dev/null\n+++ b/{rel}\n@@ -0,0 +1,{len(body)} @@\n" + text += "".join(f"+{ln}\n" for ln in body) + return mb, text, paths + + +def _declared(path: str, line: str) -> set[str]: + """The names a line of `path` declares (the TIGHT tokens of one line).""" + out = set(_SQL_VIEW.findall(line)) + if path.endswith(".sql"): + out.update(m.split(".")[-1].strip('[]"') for m in _SQL_OBJECT.findall(line)) + elif path.endswith(".js"): + out.update(g for m in _JS_DECL.findall(line) for g in m if g) + elif path.endswith(".xaml"): + out.update(g for m in _XAML_DECL.findall(line) for g in m if g) + elif path.endswith(".cs"): + out.update(_DECL_TYPE.findall(line)) + out.update(_DECL_CONST.findall(line)) + out.update(f"V{a or b}" for a, b in _MIGRATION.findall(line)) + m = _DECL_MEMBER.match(line) + if m: + out.add(m.group(1)) + m = _DECL_BARE.match(line) + if m and m.group(1) not in _BARE_NOT_TYPE: + out.add(m.group(2)) + return out + + +def changed_symbols(diff_text: str, wide: bool = False) -> "tuple[dict[str, set[str]], set[str]]": + """({token: {paths it came from}}, {changed test files}) from symbol_diff()'s text. Removed lines count as + changed (a renamed or deleted name is what a stale test still names). Tokens from test files only are dropped.""" + found: dict[str, set[str]] = {} + test_files: set[str] = set() + path = "" + for raw in diff_text.split("\n"): + line = raw.rstrip("\r") + if line.startswith("diff --git "): + path = "" + continue + if line.startswith("--- "): + if line[4:] != "/dev/null": + path = line[6:] if line.startswith("--- a/") else line[4:] + continue + if line.startswith("+++ "): + if line[4:] != "/dev/null": + path = line[6:] if line.startswith("+++ b/") else line[4:] + if path and _TEST_PROJECT_FILE.search(path): + test_files.add(path) + continue + if not path: + continue + hunk = _HUNK.match(line) + if hunk: + words = _declared(path, hunk.group(1)) + elif line[:1] in ("+", "-"): + body = line[1:] + words = _declared(path, body) + if wide: + words |= {w for w in _WORD.findall(body) if len(w) >= WIDE_MIN_LEN} + else: + continue + for w in words: + found.setdefault(w, set()).add(path) + tokens = {w: paths for w, paths in found.items() + if len(w) >= SYMBOL_MIN_LEN and w not in STOP_WORDS + and any(not _TEST_PROJECT_FILE.search(p) for p in paths)} + return tokens, test_files + + +def test_file_index(root: str = ROOT) -> "list[tuple[str, str, list[str], bool, set[str]]]": + """(suite, repo path, [class names], needs a PostgreSQL rig, {words}) for every test source file of SYMBOL_SUITES.""" + out = [] + for suite in SYMBOL_SUITES: + base = os.path.join(root, SUITE_DIRS[suite]) + for dp, dn, fn in os.walk(base): + dn[:] = [d for d in dn if d not in ("bin", "obj")] + for f in fn: + if not f.endswith(".cs"): + continue + path = os.path.join(dp, f) + try: + with open(path, encoding="utf-8-sig", errors="replace") as fh: + text = fh.read() + except OSError: + continue + classes = [m.group(1) for m in (_CLASS_DECL.match(ln) for ln in text.split("\n")) if m] + if classes: + out.append((suite, os.path.relpath(path, root).replace(os.sep, "/"), classes, + RIG_LITERAL in text, set(_WORD.findall(text)))) + return out + + +def symbol_cap(index: list) -> int: + """The most test files a token may be named in and still select (#5798): SYMBOL_CAP_SHARE of the test files of + `index`, never under SYMBOL_CAP.""" + return max(SYMBOL_CAP, round(SYMBOL_CAP_SHARE * len(index))) + + +def symbol_matches(tokens: "dict[str, set[str]]", test_files: set[str], index: list, cap: "int | None" = None) -> dict: + """Match tokens to test classes. Returns {"hits": {(suite, Class): {"tokens": set, "rig": bool, "own": bool}}, + "counts": {token: test files naming it}, "noisy": {token: count}, "cap": the cap used} (a token over `cap` + files is noisy and unused; None takes symbol_cap(index)).""" + if cap is None: + cap = symbol_cap(index) + counts = {t: 0 for t in tokens} + for _suite, _rel, _classes, _rig, words in index: + for t in tokens.keys() & words: + counts[t] += 1 + noisy = {t: n for t, n in counts.items() if n > cap} + usable = {t for t, n in counts.items() if 0 < n <= cap} + hits: dict[tuple[str, str], dict] = {} + for suite, rel, classes, rig, words in index: + named = usable & words + own = rel in test_files + if not named and not own: + continue + for cls in classes: + h = hits.setdefault((suite, cls), {"tokens": set(), "rig": False, "own": False}) + h["tokens"] |= named + h["rig"] = h["rig"] or rig + h["own"] = h["own"] or own + return {"hits": hits, "counts": counts, "noisy": noisy, "cap": cap} + + +def ordered_hits(match: dict) -> "list[tuple[str, str, dict]]": + """Most specific first: a changed test file's own classes, then by the fewest test files that name the class's + rarest token, then by more tokens, then by name.""" + counts = match["counts"] + + def key(item): + (suite, cls), h = item + rarest = min((counts[t] for t in h["tokens"]), default=10 ** 9) + return (0 if h["own"] else 1, rarest, -len(h["tokens"]), suite, cls) + + return [(s, c, h) for (s, c), h in sorted(match["hits"].items(), key=key)] + + +def named_classes(match: dict) -> "dict[str, set[str]]": + """{suite: {Class, ...}} of the hits that name a token (a changed test file's own classes alone are not named).""" + out: dict[str, set[str]] = {} + for (suite, cls), h in match["hits"].items(): + if h["tokens"]: + out.setdefault(suite, set()).add(cls) + return out + + +def commit_diff_text(root: str, base: str, head: str) -> "str | None": + """symbol_diff_text() of `head` against its merge base with `base`: the diff map_select()'s `shared_diff` reads + for a replayed commit (#5951). None when this clone lacks the commit, so a shared-project row stays FULL.""" + mb = _git(root, "merge-base", base, head) + return symbol_diff_text(root, mb.strip(), head) if mb and mb.strip() else None + + +def symbol_classes(root: str, base: str, cap: "int | None" = None, head: "str | None" = None, + index: "list | None" = None) -> "dict[str, set[str]] | None": + """map_select()'s `named`: {suite: {Class, ...}} whose source names a TIGHT token of the change against `base` + (tracked files only). With `head`, the change is `head` against its merge base with `base` (the replay's + measure, #5798); without it, the working tree. None when git cannot say, so the selection is what it was + without this reason.""" + if head is None: + diff = symbol_diff(root, base, untracked=False) + if diff is None: + return None + text = diff[1] + else: + mb = _git(root, "merge-base", base, head) + text = symbol_diff_text(root, mb.strip(), head) if mb and mb.strip() else None + if text is None: + return None + tokens, _tests = changed_symbols(text) + return named_classes(symbol_matches(tokens, set(), test_file_index(root) if index is None else index, cap)) + + +def _class_args(names: list[str]) -> list[str]: + """`-class` argument lines for an MTP test executable, 40 classes a line.""" + return [" ".join(f'-class "*.{c}"' for c in names[i:i + 40]) for i in range(0, len(names), 40)] + + +def local_classes(root: str, base: str, map_path: "str | None" = None, wide: bool = False, cap: "int | None" = None, + limit: int = LOCAL_CLASS_LIMIT) -> "tuple[int, list[str]]": + """The `local-classes` report as (exit code, lines). See the section comment above.""" + diff = symbol_diff(root, base) + if diff is None: + return 2, [f"local-classes: git could not diff the working tree against the merge base of {base} and HEAD"] + mb, text, paths = diff + tokens, test_files = changed_symbols(text, wide) + match = symbol_matches(tokens, test_files, test_file_index(root), cap) + cap = match["cap"] + rows = ordered_hits(match) + usable = sum(1 for t, n in match["counts"].items() if 0 < n <= cap) + lines = [f"local-classes: merge base {mb[:10]} of {base}, {len(paths)} changed files, " + f"{len(tokens)} {'wide' if wide else 'tight'} tokens ({usable} named in 1 to {cap} test files), " + f"{len(rows)} classes"] + for t in sorted(match["noisy"], key=lambda t: (-match["noisy"][t], t)): + lines.append(f"NOISY {t} {match['noisy'][t]}") + if len(rows) > limit: + lines.append(f"{len(rows)} classes: CI will run FULL; run the first {limit} below") + rows = rows[:limit] + if not rows: + lines.append("no test class names a changed symbol") + for suite, cls, h in rows: + why = ", ".join(sorted(h["tokens"], key=lambda t: (match["counts"][t], t))) + if h["own"]: + why = "own file changed" + (f"; {why}" if why else "") + lines.append(f"{suite} {cls} {why}" + (" needs rig" if h["rig"] else "")) + for suite in SYMBOL_SUITES: + for rig in (False, True): + names = [c for s, c, h in rows if s == suite and h["rig"] == rig] + label = f"{suite} (needs rig, DARLING_TEST_PG)" if rig else suite + for args in _class_args(names): + lines.append(f"RUN {label}: {args}") + if map_path: + test_map = load_map(map_path) + sha = test_map.get("sha") if isinstance(test_map, dict) else None + drift = git_drift(sha, mb, root) if isinstance(sha, str) else None + discovered = scan_classes(root) + if isinstance(test_map, dict) and isinstance(test_map.get("classes"), dict): + discovered = {s: v for s, v in discovered.items() if s in test_map["classes"]} + sel = map_select(test_map, paths, drift, discovered, named=named_classes(match), shared_diff=text, + **selection_args(Rules())) + lines.append(f"MAP coverage answer ({map_path}):") + if sel["full"]: + lines.append(f"MAP FULL: {sel['reason']}") + for suite in sorted(sel["selected"]): + reasons: dict[str, int] = {} + for r in sel["why"][suite].values(): + reasons[r] = reasons.get(r, 0) + 1 + lines.append(f"MAP {suite}: {len(sel['selected'][suite])} of {sel['total'][suite]} classes (" + + ", ".join(f"{k} {v}" for k, v in sorted(reasons.items())) + ")") + return 0, lines + + +GUARD_FILTER = "-trait Stage=Guard" +_TEST_ATTRIBUTE = re.compile(r"\[(?:Fact|Theory)\b") + + +def _test_holders(root: str, index: list, files: set[str]) -> set[str]: + """The classes of `files` that hold a test: a [Fact] or [Theory] between the class's declaration and the next + one. A helper class a test file also declares (a fake, a fixture row) has no test, so a filter skips it.""" + out: set[str] = set() + for _suite, rel, _classes, _rig, _words in index: + if rel not in files: + continue + try: + with open(os.path.join(root, rel), encoding="utf-8-sig", errors="replace") as fh: + lines = fh.read().split("\n") + except OSError: + continue + # (indent, class) of the classes open at this line, by indentation: a test attribute belongs to the nearest + # class indented less than it, so a nested helper declared above more tests does not take them. + stack: list[tuple[int, str]] = [] + for line in lines: + indent = len(line) - len(line.lstrip(" \t")) + m = _CLASS_DECL.match(line) + if m or _TEST_ATTRIBUTE.match(line.strip()): + while stack and stack[-1][0] >= indent: + stack.pop() + if m: + stack.append((indent, m.group(1))) + elif stack and _TEST_ATTRIBUTE.match(line.strip()): + out.add(stack[-1][1]) + return out + + +def local_filter(root: str, base: str, map_path: "str | None" = None, cap: "int | None" = None, + limit: int = LOCAL_CLASS_LIMIT) -> "tuple[int, list[str], list[str]]": + """The `local-filter` answer (#5798) as (exit code, stdout lines, stderr notes). Per suite, the test + executable's filter (`: -class "*.A" -class "*.B"`, a line per 40 classes), then a line that lists the + classes (` classes: A B`); the last line is the guard set, which CI's guard job runs on every pull request. + + The classes are CI's own selection for the branch: with `map_path`, map_select() over the changed files with the + changed-symbol reason, exactly as the test-map-select job asks it. Without a map, or when that answer is FULL or + over `limit` classes (a lane does not run the whole suite), they are the part of it a lane can run: the + classes of a changed test file, then (with `map_path`) the classes whose text pattern in the map matches a + changed file, then the classes that name a changed symbol, most specific first and cut at `limit`. CI selects + each of them too. A note on stderr says which. Stage=Guard classes are left to the guard line. A suite's filter + takes 40 classes a line, so one line stays well under Windows' command-line limit.""" + diff = symbol_diff(root, base) + if diff is None: + return 2, [], [f"local-filter: git could not diff the working tree against the merge base of {base} and HEAD"] + mb, text, paths = diff + tokens, test_files = changed_symbols(text) + index = test_file_index(root) + match = symbol_matches(tokens, test_files, index, cap) + holders = _test_holders(root, index, {rel for _s, rel, _c, _r, _w in index + if rel in test_files or match["hits"].keys() & {(_s, c) for c in _c}}) + discovered = scan_classes(root) + + def runnable(suite: str, cls: str) -> bool: + # Stage=Guard classes are left to the guard line. + return "Stage=Guard" not in discovered.get(suite, {}).get(cls, set()) + + # The part a lane can run, most specific first: a changed test file's classes, then the map's text-pattern + # classes, then the classes that name a changed symbol (in ordered_hits' order). + ranked = ordered_hits(match) + own = [(s, c) for s, c, h in ranked if h["own"] and c in holders and runnable(s, c)] + named = [(s, c) for s, c, h in ranked if not h["own"] and c in holders and runnable(s, c)] + pattern: list[tuple[str, str]] = [] + chosen: dict[str, set[str]] = {} + notes = [f"local-filter: merge base {mb[:10]} of {base}, {len(paths)} changed files"] + fallback = True + if map_path: + test_map = load_map(map_path) + sha = test_map.get("sha") if isinstance(test_map, dict) else None + drift = git_drift(sha, mb, root) if isinstance(sha, str) else None + mapped = discovered + if isinstance(test_map, dict) and isinstance(test_map.get("classes"), dict): + mapped = {s: v for s, v in discovered.items() if s in test_map["classes"]} + args = selection_args(Rules()) + sel = map_select(test_map, paths, drift, mapped, named=named_classes(match), shared_diff=text, **args) + count = sum(len(v) for v in sel["selected"].values()) + part = ("the classes that name a changed symbol, sit in a changed test file or have a text pattern in the " + "map that matches a changed file") + if sel["full"]: + notes.append(f"CI's map answer is FULL ({sel['reason']}): CI runs every class; below is the part a lane " + f"runs ({part})") + elif count > limit: + notes.append(f"CI's map answer runs {count} classes, over {limit}: below is the part a lane runs ({part})") + else: + fallback = False + for suite, cls in own + named: + chosen.setdefault(suite, set()).add(cls) + for suite, names in sel["selected"].items(): + chosen.setdefault(suite, set()).update(c for c in names if runnable(suite, c)) + notes.append(f"CI's map answer: {count} classes") + if fallback: + # #5798: a class that reads a source file as text (the web tests read wwwroot/js/**) names no symbol, so + # the symbol rule misses it; CI selects it through the map's text_patterns, and so does a lane. + hits = text_pattern_classes(test_map, list(dict.fromkeys(paths + (drift or []))), args["doc_patterns"]) + pattern = [(s, c) for s in sorted(hits) for c in sorted(hits[s]) + if c in discovered.get(s, {}) and runnable(s, c)] + else: + notes.append("no --map: below are the classes that name a changed symbol or sit in a changed test file, " + "which CI selects too; pass the nightly test map for CI's whole answer") + if fallback: + order = list(dict.fromkeys(own + pattern + named)) + if len(order) > limit: + notes.append(f"{len(order)} classes, over {limit}: below are the first {limit}, most specific first") + order = order[:limit] + for suite, cls in order: + chosen.setdefault(suite, set()).add(cls) + lines: list[str] = [] + for suite in SYMBOL_SUITES: + names = sorted(chosen.get(suite, ())) + # 40 classes a line, as local-classes prints them: 700 `-class "*.X"` arguments on one line come near + # Windows' 32,767-character command line (#5100). + for arg_line in _class_args(names): + lines.append(f"{suite}: {arg_line}") + if names: + lines.append(f"{suite} classes: " + " ".join(names)) + if not lines: + notes.append("no class outside the guard set is selected") + lines.append(f"guard: {GUARD_FILTER}") + return 0, lines, notes + + +# --------------------------------------------------------------------------------------------------------- +# The CI quarantine (#5942): a test that dev's own latest push broke is left out of an unrelated pull request. +# --------------------------------------------------------------------------------------------------------- + +QUARANTINE_FILE = os.path.join(ROOT, ".github", "ci-quarantine.txt") +QUARANTINE_HOURS = 72 +# map_select()'s `why` reasons that mean the pull request reaches the class through a file or a symbol of its own. +# "reads the tree", "guard stage", "new class" and "gated on an environment" are NOT reach: they select a class for +# nearly every change, and an unrelated pull request failing on one of them is the case this list exists for. +QUARANTINE_REACH = ("own file changed", "covers a changed file", "text pattern", "names a changed symbol", + "shares a collection with a changed class") +# A test runner's name prefix says which suite a test belongs to. +QUARANTINE_SUITES = (("Darling.Tests.", "darling"), ("Lite.Tests.", "lite")) +_FAIL_LINE = re.compile(r"^\S+Z\s+(?P\S.*?)\s+\[FAIL\]\s*$") +_ANSI = re.compile(r"\x1b\[[0-9;]*[A-Za-z]") + + +def parse_quarantine(text: str) -> "tuple[list[dict], list[str]]": + """(entries, malformed lines) of the quarantine file's text. A line is ` | | | #`: + the test is `Namespace.Class.Method` as the runner prints it (arguments of a theory case left off), the run is the + dev push run that failed it, the time is ISO 8601 UTC and the issue is the bug filed for the test. Blank lines and + lines starting with `#` are comments. A malformed line is reported and never honored.""" + entries: list[dict] = [] + bad: list[str] = [] + for raw in text.splitlines(): + line = raw.strip() + if not line or line.startswith("#"): + continue + parts = [p.strip() for p in line.split("|")] + at = _parse_time(parts[2]) if len(parts) == 4 else None + if len(parts) != 4 or not parts[0] or not parts[1].isdigit() or at is None or not re.fullmatch(r"#\d+", parts[3]): + bad.append(line) + continue + entries.append({"name": parts[0], "run": parts[1], "at": at, "issue": parts[3]}) + return entries, bad + + +def failed_tests_of_log(text: str) -> set[str]: + """The tests a job log reports as failed: the runner prints `