# Deep Review: 20260411-220257-pr-292
| | |
|---|---|
| **Date** | 2026-04-11 22:02 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 1 |
| **Author** | [@jandubois](https://github.com/jandubois) |
| **PR** | [#292](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/292) — Windows typeperf sampler |
| **Commits** | `9bed00e` ci: add Windows machine-info and perf-sampler instrumentation
`5290abd` ci: add typeperf-based perf sampler for comparison testing
`e2511a8` ci: use BLG format to capture processes started mid-run
`e44cc1b` ci: remove PowerShell perf sampler in favor of typeperf |
| **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro |
| **Verdict** | **Merge with fixes** — the diagnostic is otherwise clean, but the environment-variable filter silently drops every CI/RDD/runner variable it was written to capture, and a `relog` failure on stop deletes the only raw BLG artifact. Both are worth fixing before merge; everything else is polish. |
| **Wall-clock time** | 16 min 41 s |
---
## Executive Summary
PR #292 adds two Windows-only CI composite actions that upload diagnostics alongside the BATS log bundle: `windows-machine-info` (one-shot hardware/OS/Defender/WSL snapshot via PowerShell) and `windows-typeperf-sampler` (background `typeperf` BLG collection, `relog` → TSV conversion on stop, and a Go summarizer that prints top-N processes by CPU and working set per sample). The sampler was iterated inside the PR from direct-TSV to BLG specifically to capture processes that start after collection begins, and the PowerShell sampler that commit `5290abd` introduced alongside it was removed again in `e44cc1b` in favor of `typeperf` only.
The code is well-scoped and the design choices are sound: BLG + `relog` is the correct way to deal with typeperf's locked-at-first-sample column set, splitting one-shot and continuous sampling into two actions matches their lifecycles, and the Go summarizer indexes columns once from the header and streams rows. Two issues are worth fixing before merge. First, the env-var filter in `machine-info.ps1:170` anchors its regex with `^(...)$`, so prefix alternatives like `GITHUB_`, `RUNNER_`, `RDD_`, and `MSYS` only match exact variable names that do not exist — `GITHUB_WORKSPACE`, `RUNNER_OS`, `RDD_TRACE`, `MSYSTEM` are all silently dropped, defeating most of the diagnostic. Second, the stop path in `windows-typeperf-sampler/action.yaml` invokes `relog` without checking `$LASTEXITCODE` and unconditionally `Remove-Item $blgFile -Force`s the BLG, so any conversion failure permanently loses the raw capture — and because the workflow step sets `continue-on-error: true`, the job will not even fail. Both bugs are straightforward to fix.
Remaining findings are suggestions: add `Wait-Process` after `Stop-Process` so typeperf's file handle is guaranteed closed before `relog` runs; move or gate the 256 MB disk benchmark so it stops warming the file cache just before the BATS run it is meant to diagnose; wrap the machine-info FileStream benchmark in try/finally so a mid-write exception does not leak the handle; reconcile the stale `summarize-perf-counters`/`scripts/...` header comment in `summarize-typeperf.go`; print or remove the unused `handles`/`threads` counters; distinguish `io.EOF` from real CSV parse errors; and pass `-y` to `typeperf` for symmetry with the `relog` call.
---
## Critical Issues
None.
---
## Important Issues
I1. **Env-var filter's regex matches only exact names, dropping every prefix alternative** — `.github/actions/windows-machine-info/machine-info.ps1:170` [Claude Opus 4.6]
```powershell
Get-ChildItem env: | Where-Object { $_.Name -match '^(LOCALAPPDATA|TEMP|TMP|USERPROFILE|HOMEDRIVE|GITHUB_|RUNNER_|RDD_|MSYS|PATH)$' } |
Sort-Object Name | Format-Table -AutoSize Name, Value
```
`^(...)$` anchors force each alternative to match the whole variable name. `LOCALAPPDATA`, `TEMP`, `TMP`, `USERPROFILE`, `HOMEDRIVE`, and `PATH` are exact names and match fine. But `GITHUB_`, `RUNNER_`, `RDD_`, and `MSYS` were intended as prefixes — no environment variable is literally named `GITHUB_` or `RUNNER_`. Every `GITHUB_WORKSPACE`, `GITHUB_ACTIONS`, `GITHUB_SHA`, `RUNNER_OS`, `RUNNER_TEMP`, `RDD_TRACE`, `RDD_LOG_LEVEL`, `MSYSTEM`, `MSYS2_PATH_TYPE` fails the `$` anchor and is dropped. I verified the behavior with `python3 -c "import re; re.match(r'^(GITHUB_|...)$', 'GITHUB_WORKSPACE')"` — no match.
The whole point of this section is correlating runner state with flaky timeouts. Losing the Actions metadata and the RDD tuning knobs defeats most of the value; right now the block only exposes `LOCALAPPDATA`, `TEMP`, `TMP`, `USERPROFILE`, `HOMEDRIVE`, and `PATH`.
Fix: turn the prefix entries into proper patterns. (Intentionally keep `GITHUB_TOKEN` out by not adding a `GITHUB_.*TOKEN.*` catch-all, though `GITHUB_TOKEN` is not set on composite-action steps unless explicitly mapped.)
```diff
- Get-ChildItem env: | Where-Object { $_.Name -match '^(LOCALAPPDATA|TEMP|TMP|USERPROFILE|HOMEDRIVE|GITHUB_|RUNNER_|RDD_|MSYS|PATH)$' } |
+ Get-ChildItem env: | Where-Object { $_.Name -match '^(LOCALAPPDATA|TEMP|TMP|USERPROFILE|HOMEDRIVE|PATH|GITHUB_.*|RUNNER_.*|RDD_.*|MSYS.*)$' } |
Sort-Object Name | Format-Table -AutoSize Name, Value
```
I2. **Stop step deletes the raw BLG even when `relog` failed, losing the only capture** — `.github/actions/windows-typeperf-sampler/action.yaml:48-56` [Codex GPT 5.4]
```yaml
if (Test-Path $blgFile) {
$size = (Get-Item $blgFile).Length / 1MB
Write-Output ("typeperf.blg: {0:N1} MB" -f $size)
relog $blgFile -f TSV -o $tsvFile -y
$lines = (Get-Content $tsvFile | Measure-Object -Line).Lines
Write-Output ("typeperf.tsv: {0:N1} MB, {1} lines" -f ((Get-Item $tsvFile).Length / 1MB), $lines)
Remove-Item $blgFile -Force
```
PowerShell does not throw on a non-zero exit from a native command; a failed `relog` silently flows into the `Get-Content`/`Measure-Object` call on a missing TSV (non-terminating error, execution continues) and into the unconditional `Remove-Item $blgFile -Force` at line 54. Because the workflow step in `.github/workflows/bats.yaml:171-177` sets `continue-on-error: true`, the job will not fail — it will just upload an empty `rdd-logs/_diag/` with no raw BLG to retry conversion against. For a diagnostic action whose whole job is preserving evidence, that is the worst failure mode.
Fix: gate subsequent steps on `$LASTEXITCODE` and only delete the BLG after both conversion and summarization succeed; keep the BLG on failure so it can be re-processed locally.
```diff
if (Test-Path $blgFile) {
$size = (Get-Item $blgFile).Length / 1MB
Write-Output ("typeperf.blg: {0:N1} MB" -f $size)
relog $blgFile -f TSV -o $tsvFile -y
+ if ($LASTEXITCODE -ne 0 -or -not (Test-Path $tsvFile)) {
+ Write-Warning "relog failed; preserving $blgFile for inspection"
+ exit 1
+ }
$lines = (Get-Content $tsvFile | Measure-Object -Line).Lines
Write-Output ("typeperf.tsv: {0:N1} MB, {1} lines" -f ((Get-Item $tsvFile).Length / 1MB), $lines)
- Remove-Item $blgFile -Force
go run "${{ github.action_path }}/summarize-typeperf.go" `
$tsvFile 10 > "$outputDir/typeperf-summary.txt"
+ if ($LASTEXITCODE -eq 0) {
+ Remove-Item $blgFile -Force
+ }
Write-Output "summary written to $outputDir/typeperf-summary.txt"
}
```
---
## Suggestions
S1. **`Stop-Process` followed immediately by `relog` leaves a small file-lock race** — `.github/actions/windows-typeperf-sampler/action.yaml:42-51` [Claude Opus 4.6, Gemini 3.1 Pro]
```powershell
Stop-Process -Id $typeperfPid -Force -ErrorAction SilentlyContinue
Remove-Item $pidFile -Force -ErrorAction SilentlyContinue
Write-Output "stopped typeperf pid=$typeperfPid"
# ...
relog $blgFile -f TSV -o $tsvFile -y
```
`Stop-Process -Force` issues `TerminateProcess`, which is asynchronous: the kernel signals cancellation but the process object and its file handles are not fully released until every outstanding I/O completes. On a busy runner `typeperf` can still hold `typeperf.blg` for a few milliseconds when `relog` starts, producing a file-sharing violation. The race window is tiny and this is defensive rather than a demonstrated failure, but the cost of a lost summary partly defeats the instrumentation, so it is worth eliminating.
Gemini rated this Important; I downgraded it to a Suggestion because `typeperf` flushes each BLG sample immediately and Windows file-handle teardown after `TerminateProcess` is usually complete within milliseconds, so the practical failure rate on CI runners is near zero. `Wait-Process` is still the right call.
Fix:
```diff
Stop-Process -Id $typeperfPid -Force -ErrorAction SilentlyContinue
+Wait-Process -Id $typeperfPid -Timeout 5 -ErrorAction SilentlyContinue
Remove-Item $pidFile -Force -ErrorAction SilentlyContinue
```
S2. **Stale header comment points maintainers at a file that never existed** — `.github/actions/windows-typeperf-sampler/summarize-typeperf.go:5-10` [Claude Opus 4.6, Codex GPT 5.4]
```go
// summarize-perf-counters reads a TSV file produced by typeperf and prints
// a compact summary of system counters and top processes per sample.
//
// Usage:
//
// go run scripts/summarize-perf-counters.go [topN]
```
The file is `summarize-typeperf.go` under `.github/actions/windows-typeperf-sampler/`, and the workflow invokes that exact path from `action.yaml:55-56`. The header refers to a `summarize-perf-counters` name and `scripts/summarize-perf-counters.go` path that `git log --all -- scripts/summarize*` confirms has never existed in this repo — it looks like leftover boilerplate from an external adaptation. Anyone trying to reproduce the summary locally will be sent to a file that is not there.
The companion `fmt.Fprintf(os.Stderr, "usage: summarize-perf-counters ...")` at line 46 has the same stale name.
Fix:
```diff
-// summarize-perf-counters reads a TSV file produced by typeperf and prints
-// a compact summary of system counters and top processes per sample.
+// summarize-typeperf reads a TSV file produced by typeperf (via relog from
+// BLG) and prints a compact summary of system counters and top processes
+// per sample.
//
// Usage:
//
-// go run scripts/summarize-perf-counters.go [topN]
+// go run .github/actions/windows-typeperf-sampler/summarize-typeperf.go [topN]
package main
```
And update the usage line at line 46 to match.
S3. **Machine-info's 256 MB disk benchmark warms the file cache right before the workload it is supposed to diagnose** — `.github/workflows/bats.yaml:150-165`, `.github/actions/windows-machine-info/machine-info.ps1:79-128` [Codex GPT 5.4]
```yaml
- name: "Windows: capture machine info"
if: runner.os == 'Windows'
uses: ./.github/actions/windows-machine-info
with:
output-path: rdd-logs/_diag/machine-info.txt
continue-on-error: true
- name: "Windows: start typeperf sampler"
...
- run: make -C bats --keep-going --jobs=$(nproc) --output-sync=target
```
`machine-info.ps1:79-128` deliberately writes and reads back a 256 MB sequential file on the same volume as the BATS workload, and the code comment at lines 80-86 explicitly notes that this measures the cache-warm sequential rate (not raw disk) because "FileStream with no buffering still goes through the OS file cache". That is fine as a runner-characterization data point, but placing the step right before the BATS step means every Windows job now starts with a 256 MB warm-up burst that both drains ~5 seconds of wall clock and changes the disk/cache state before the test the diagnostic is meant to correlate with begins.
Fix: either move the machine-info step to run after the BATS step (runner characteristics do not change during the job), or gate the disk benchmark behind an action input and default it off.
S4. **`FileStream` benchmark does not dispose the handle on error, so a mid-write exception leaks the file** — `.github/actions/windows-machine-info/machine-info.ps1:87-114` [Gemini 3.1 Pro]
```powershell
$fs = [System.IO.File]::Create($benchFile)
for ($i = 0; $i -lt $sizeMB; $i++) {
$fs.Write($buf, 0, $buf.Length)
}
$fs.Flush($true) # flush to disk
$fs.Close()
```
If `$fs.Write` throws mid-loop (disk full, antivirus locking, I/O error) control jumps past `$fs.Close()` into the outer `catch` block. The handle is leaked to GC. The `finally` then tries `Remove-Item $benchFile -Force -ErrorAction SilentlyContinue` on a still-locked file; with `SilentlyContinue` the delete failure is swallowed and a 256 MB file lingers under `%TEMP%` until the runner is recycled. The identical pattern repeats in the read loop at lines 104-111.
Fix: wrap each stream in its own try/finally so the handle is disposed deterministically.
```diff
- $fs = [System.IO.File]::Create($benchFile)
- for ($i = 0; $i -lt $sizeMB; $i++) {
- $fs.Write($buf, 0, $buf.Length)
- }
- $fs.Flush($true) # flush to disk
- $fs.Close()
+ $fs = [System.IO.File]::Create($benchFile)
+ try {
+ for ($i = 0; $i -lt $sizeMB; $i++) {
+ $fs.Write($buf, 0, $buf.Length)
+ }
+ $fs.Flush($true) # flush to disk
+ } finally {
+ $fs.Dispose()
+ }
```
S5. **`handles` and `threads` counters are collected, parsed, and then never printed** — `.github/actions/windows-typeperf-sampler/summarize-typeperf.go:24-31,34-40,101-111,151-170,186` and `.github/actions/windows-typeperf-sampler/typeperf-counters.txt:14-15` [Claude Opus 4.6]
```go
type procSnap struct {
name string
pid int
cpu float64
wsMB float64
handles int
threads int
}
```
`printTopN` only prints `name`, `pid`, `cpu`, `wsMB`:
```go
fmt.Printf(" %-30s PID %6d CPU %7.1f%% WS %8.1f MB\n",
s.name, s.pid, s.cpu, s.wsMB)
```
The `handles`/`threads` fields on `procSnap`, the matching fields on `procIndex`, the header detection for `Handle Count`/`Thread Count`, and the per-sample extraction at lines 160-168 all exist only to populate two fields that nothing reads. The corresponding `\Process(*)\Handle Count` and `\Process(*)\Thread Count` lines in `typeperf-counters.txt` each add one column per tracked process per sample to the BLG (and the intermediate TSV) for no benefit.
Handle and thread counts are actually useful for leak diagnosis, so extending the output is probably preferable to deleting the counters:
```diff
- fmt.Printf(" %-30s PID %6d CPU %7.1f%% WS %8.1f MB\n",
- s.name, s.pid, s.cpu, s.wsMB)
+ fmt.Printf(" %-30s PID %6d CPU %7.1f%% WS %8.1f MB H %6d T %4d\n",
+ s.name, s.pid, s.cpu, s.wsMB, s.handles, s.threads)
```
S6. **CSV read loop breaks on any error, silently truncating the summary on malformed rows** — `.github/actions/windows-typeperf-sampler/summarize-typeperf.go:118-122` [Claude Opus 4.6]
```go
for {
record, err := reader.Read()
if err != nil {
break
}
rowNum++
```
`encoding/csv.Reader` returns `io.EOF` at clean end, but also returns `csv.ErrFieldCount` or quoting errors mid-file. The current loop collapses all of them into a silent `break`, leaving the misleading `Samples: N` footer at line 176 with a low count and no explanation. Worth distinguishing `io.EOF` from real parse errors so a partial TSV still summarizes what it can while flagging the failure on stderr.
```diff
for {
record, err := reader.Read()
+ if errors.Is(err, io.EOF) {
+ break
+ }
if err != nil {
- break
+ fmt.Fprintf(os.Stderr, "read row %d: %v\n", rowNum+1, err)
+ break
}
rowNum++
```
(Adds `errors` and `io` imports.)
S7. **`typeperf` start lacks `-y`, so an existing BLG would hang the hidden process** — `.github/actions/windows-typeperf-sampler/action.yaml:29-30` [Claude Opus 4.6]
```powershell
$proc = Start-Process -PassThru -WindowStyle Hidden -FilePath typeperf `
-ArgumentList "-cf", $counterFile, "-f", "BIN", "-o", $blgFile, "-si", "1"
```
If `$blgFile` already exists, `typeperf` prompts to overwrite and blocks on stdin. With `-WindowStyle Hidden` the prompt is invisible and the sampler hangs indefinitely. Fresh CI runners never hit this, but `relog` on line 51 already passes `-y` for exactly the same reason; the start step should be symmetric as cheap insurance against a reused runner.
```diff
- -ArgumentList "-cf", $counterFile, "-f", "BIN", "-o", $blgFile, "-si", "1"
+ -ArgumentList "-cf", $counterFile, "-f", "BIN", "-o", $blgFile, "-si", "1", "-y"
```
---
## Design Observations
### Strengths
- **BLG-then-relog is the correct architecture for this problem** `(in-scope)` [Claude Opus 4.6, Codex GPT 5.4]. Commit `e2511a8` correctly identifies that typeperf's direct-TSV mode locks the column set at the first sample and misses processes that start later; switching to BLG and converting via `relog` at stop decouples low-overhead collection from post-processing and eliminates the blind spot entirely. The commit message's explanation is worth keeping even though squashing it into `5290abd` would have been defensible.
- **Separation of one-shot machine-info and continuous typeperf sampler matches their lifecycles** `(in-scope)` [Claude Opus 4.6]. Runner hardware and OS state do not change during a job, so capturing them once at start is the right shape; perf counters do change and need continuous sampling. Splitting them into two composite actions keeps each one independently usable.
- **Go summarizer indexes columns once and streams rows** `(in-scope)` [Claude Opus 4.6, Gemini 3.1 Pro]. `summarize-typeperf.go:76-113` builds the process-column index from the header in a single pass, then the per-row loop does simple lookups — no repeated header scans. The column-index struct initializes to `-1` so missing counters are safely handled by `colFloat`/`colInt` boundary checks, preventing panics on abbreviated rows.
## Testing Assessment
No automated test coverage is expected for CI-only diagnostic scripts, and the agents did not flag its absence. The more pragmatic gap is that the regex bug in I1 would have been visible to anyone who looked at the first `machine-info.txt` artifact produced on CI — worth eyeballing the artifact on the first post-merge run to confirm the fix actually surfaces `GITHUB_*`, `RUNNER_*`, `RDD_*`, and `MSYS*` variables. The end-to-end start→BATS→stop cycle producing both `typeperf.tsv` and `typeperf-summary.txt` also has no validation other than watching the first Windows CI run.
## Documentation Assessment
The header comment in `summarize-typeperf.go` is stale (see S2). Beyond that, the PR description in [#292](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/292) still describes the sampler as running "alongside the existing PowerShell sampler so we can compare overhead and data quality" and lists both `perf-sampler.jsonl` (PowerShell) and `typeperf.tsv` (new) in its artifacts table — but commit `e44cc1b` removed the PowerShell sampler before the final state, so the description is out of sync with what merges. Worth updating before merge so the commit history matches the PR narrative.
## Commit Structure
Four commits form a coherent narrative: (1) `9bed00e` introduces machine-info and the original PowerShell sampler, (2) `5290abd` adds the typeperf variant alongside it, (3) `e2511a8` fixes the direct-TSV blind spot by switching to BLG, (4) `e44cc1b` deletes the PowerShell sampler once typeperf proved superior. The `e2511a8` fix could arguably have been squashed into `5290abd`, but leaving it separate preserves useful context about why BLG is required over direct-TSV — the reason is non-obvious enough that the audit trail is worth the extra commit.
## Acknowledged Limitations
- **Disk benchmark is cache-warm, not raw disk** — `machine-info.ps1:80-86` [Codex GPT 5.4, Gemini 3.1 Pro]. The code comment explicitly notes that `FileStream` without buffering still traverses the OS file cache, so the reported MB/s reflects the cache-warm sequential rate rather than raw disk throughput. The author calls this out correctly; with S3 it becomes more important because the benchmark is what warms the cache just before BATS runs.
---
## Agent Performance Retro
### [Claude] — Claude Opus 4.6
Claude produced the most findings (6) and the only diagnosis of the regex bug in `machine-info.ps1` (I1), which is the single most consequential issue in the review because it silently zeroes out the whole "Environment" section of the diagnostic artifact. Its approach was methodical: it read each file, built a mental model of what the diagnostic was supposed to expose, and then noticed that what it *would* actually expose did not match. Claude also caught three smaller, easy-to-miss quality issues in `summarize-typeperf.go`: the stale header comment (S2, also found by Codex), the dead `handles`/`threads` code path (S5), and the `io.EOF`-vs-parse-error loop (S6). The `typeperf -y` defensive hardening (S7) is unique to Claude as well.
Claude independently identified the `Stop-Process`/`relog` race Gemini flagged but calibrated it lower (Suggestion, not Important), which matches the realistic CI behavior better. Its only gap is that it missed Codex's relog/BLG loss (I2) — a small block of code that does not *look* risky until you think about `$LASTEXITCODE` semantics on native commands in PowerShell. It also marked `bats.yaml` as "Reviewed, no issues" while Codex correctly flagged a step-ordering concern in that file (S3 in this report).
Tool usage: 21 calls (Bash 10, Read 9, Grep 1). The Bash calls include `git log --all`, `git blame`, and verification of the PowerShell regex behavior — exactly the depth the prompt asks for.
### [Codex] — Codex GPT 5.4
Codex produced the fewest findings (3) but every single one is substantive: the `relog` failure mode (I2) that wipes the raw BLG is the most original finding in the review because it requires knowing that PowerShell does not throw on native command non-zero exit codes, and it directly interacts with the workflow's `continue-on-error: true`, which Codex also cited by file:line. The disk-bench-perturbs-CI-cache concern (S3) is a systems-level observation that neither other agent made, and the stale header comment (S2) was found in parallel with Claude.
Codex had zero false positives, zero duplicates, and a 100 % signal rate on its three findings. It missed several smaller polish items (dead handles/threads counters in `summarize-typeperf.go`, the FileStream disposal leak, the env var regex, the `io.EOF` loop, the `typeperf -y` symmetry), some of which are in files it reviewed. Its one "Reviewed, no issues" coverage miss is `typeperf-counters.txt:14-15` — the two lines that configure the counters feeding Claude's S5 dead-code finding.
Tool usage: 22 calls (21 shell, 1 write_stdin). Good depth; the shell breakdown likely includes multiple `git log`/`git blame` invocations and file reads via `sed`/`cat`.
### [Gemini] — Gemini 3.1 Pro
Gemini produced 2 findings. Its unique contribution is the `FileStream` disposal-on-error leak in `machine-info.ps1` (S4 in this report) — a genuine PowerShell resource-management observation that neither other agent caught, and one Gemini is well-calibrated for because it pattern-matches on `try`/`finally` idioms without needing deep semantic tracing. Its other finding, the `Stop-Process`/`relog` race, is duplicated by Claude (S3 → S1 in consolidation). Gemini rated that race `Important`; I downgraded it to `Suggestion` because the race window after `TerminateProcess` is typically sub-millisecond on Windows and typeperf flushes BLG writes immediately, so the practical failure rate on GitHub runners is near zero.
Gemini marked `summarize-typeperf.go`, `bats.yaml`, and `typeperf-counters.txt` as "Reviewed, no issues"/Trivial while the other agents found three distinct issues in those files. The Go summarizer gap is particularly notable — it is a 236-line Go file, exactly the kind of code Gemini is supposed to be strong at, but it produced zero findings there. The session file shows no recorded tool calls, consistent with Gemini's well-known habit of skipping `git blame` to conserve its daily Pro-model quota.
Tool usage: session did not record individual tool calls. From the prompt-to-response elapsed time (~3m 52s) and the depth of its two findings, Gemini clearly read at least `machine-info.ps1` and `windows-typeperf-sampler/action.yaml` but apparently did not explore the Go file or the workflow in enough detail to form opinions there.
### Summary
| | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro |
|---|---|---|---|
| Duration | 7m 27s | 3m 41s | 3m 52s |
| Findings | 1I 5S | 1I 2S | 0I 2S |
| Tool calls | 21 (Bash 10, Read 9, Grep 1) | 22 (shell 21, stdin 1) | not recorded |
| Design observations | 3 | 1 | 2 |
| False positives | 0 | 0 | 0 |
| Unique insights | 4 | 2 | 1 |
| Files reviewed | 6 | 6 | 6 |
| Coverage misses | 1 | 1 | 3 |
| **Totals** | **1I 5S** | **1I 2S** | **0I 2S** |
| Downgraded | 0 | 0 | 1 (I→S) |
| Dropped | 0 | 0 | 0 |
**Reconciliation.** One severity change during consolidation: Gemini P1 "Race condition when stopping typeperf" was rated `important` and consolidated as `suggestion` (S1 in this report). Rationale: after `TerminateProcess`, file-handle cleanup on Windows completes in sub-millisecond time; typeperf flushes BLG writes at sample time rather than at exit, so there is no pending I/O holding the file; and neither this PR's stop path nor the widely deployed `Stop-Process`-then-use pattern in other CI scripts shows the failure empirically. `Wait-Process` is still the right defensive change, so the finding is kept — just at the correct severity. No false positives from any agent. No dropped findings.
**Overall.** All three agents added distinct value: Claude is the clear leader on breadth and caught the highest-impact bug (I1); Codex is the clear leader on signal-to-noise ratio, delivering the other Important finding (I2) plus two substantive suggestions with zero noise; Gemini contributed one genuinely unique suggestion (S4) that the other two missed. The multi-agent ensemble was justified for this PR — removing any single agent would have dropped a real finding from the report.
---
## Review Process Notes
### Skill improvements
- **Add a PowerShell "native command exit code" heuristic to the review dimensions.** Codex's I2 was the single most original finding in this review, and it required knowing that in PowerShell, a non-zero exit from an external command like `relog` does *not* throw and does *not* stop execution — subsequent statements run against whatever state the failed command left behind. This is a general review principle that will come up again any time we review PowerShell in CI: "When reviewing PowerShell that invokes native commands, verify that `$LASTEXITCODE` (or `if (-not $?)`) is checked after each external invocation; PowerShell does not automatically throw on non-zero exits, so errors silently propagate into subsequent statements that assume success." This is a real prompt-gap signal because only one of three agents found the issue, and the inference is a domain rule that generalizes beyond this PR.
### Repo context updates
No repo context gaps surfaced in this review. The PR touches only CI infrastructure and has no interaction with the controllers, kube-apiserver, Lima lifecycle, or any of the architectural areas that `deep-review-context.md` covers. No entries in that file are contradicted by the reviewed code, so no pruning is needed.
---
## Appendix: Original Reviews
### [Claude] — Claude Opus 4.6
Now writing the review.
# Code Review: PR #292 — Windows typeperf sampler
## Executive Summary
Adds two Windows CI composite actions: `windows-machine-info` (one-shot system snapshot) and `windows-typeperf-sampler` (background perf counter collection via `typeperf` with a Go post-processor). Implementation is clean and well-scoped; the main issue is a broken regex in `machine-info.ps1` that silently drops most of the intended diagnostic environment variables. Note: the PR summary claims both PowerShell and typeperf samplers run alongside each other, but commit `e44cc1b` removes the PowerShell sampler — the summary is stale relative to the final state.
## Findings
### Critical Issues
None.
### Important Issues
**I1. Environment variable filter misses all prefix-based variables** — `.github/actions/windows-machine-info/machine-info.ps1:170` (important, regression)
```powershell
Get-ChildItem env: | Where-Object { $_.Name -match '^(LOCALAPPDATA|TEMP|TMP|USERPROFILE|HOMEDRIVE|GITHUB_|RUNNER_|RDD_|MSYS|PATH)$' } |
Sort-Object Name | Format-Table -AutoSize Name, Value
```
The regex anchors force an exact full-string match against each alternative. `LOCALAPPDATA`, `TEMP`, `TMP`, `USERPROFILE`, `HOMEDRIVE`, and `PATH` match fine (they are exact names). But `GITHUB_`, `RUNNER_`, `RDD_`, and `MSYS` are intended as *prefixes* — no environment variable is named literally `GITHUB_` or `RUNNER_`. Every variable like `GITHUB_WORKSPACE`, `RUNNER_OS`, `RUNNER_TEMP`, `RDD_INSTANCE`, `MSYSTEM`, `MSYS2_PATH_TYPE` fails the `$` anchor and is dropped.
Verified by running `echo 'GITHUB_WORKSPACE' | grep -E '^(...|GITHUB_|...)$'` — no match.
The whole point of this diagnostic is correlating runner state with flaky timeouts; losing the CI/Actions variables and RDD tuning knobs defeats most of the value.
Fix:
```diff
- Get-ChildItem env: | Where-Object { $_.Name -match '^(LOCALAPPDATA|TEMP|TMP|USERPROFILE|HOMEDRIVE|GITHUB_|RUNNER_|RDD_|MSYS|PATH)$' } |
+ Get-ChildItem env: | Where-Object { $_.Name -match '^(LOCALAPPDATA|TEMP|TMP|USERPROFILE|HOMEDRIVE|PATH|GITHUB_.*|RUNNER_.*|RDD_.*|MSYS.*)$' } |
Sort-Object Name | Format-Table -AutoSize Name, Value
```
### Suggestions
**S1. Stale doc comment on `summarize-typeperf.go`** — `.github/actions/windows-typeperf-sampler/summarize-typeperf.go:5-10` (suggestion, gap)
```go
// summarize-perf-counters reads a TSV file produced by typeperf and prints
// a compact summary of system counters and top processes per sample.
//
// Usage:
//
// go run scripts/summarize-perf-counters.go [topN]
package main
```
The subject name (`summarize-perf-counters`) and path (`scripts/summarize-perf-counters.go`) are both wrong — the file is `summarize-typeperf.go` under `.github/actions/windows-typeperf-sampler/`. Appears to be leftover from an earlier name/location.
Fix:
```diff
-// summarize-perf-counters reads a TSV file produced by typeperf and prints
-// a compact summary of system counters and top processes per sample.
+// summarize-typeperf reads a TSV file produced by typeperf (via relog from
+// BLG) and prints a compact summary of system counters and top processes
+// per sample.
//
// Usage:
//
-// go run scripts/summarize-perf-counters.go [topN]
+// go run .github/actions/windows-typeperf-sampler/summarize-typeperf.go [topN]
package main
```
**S2. `handles` and `threads` counters collected but never reported** — `.github/actions/windows-typeperf-sampler/summarize-typeperf.go:29-30,38-39,108-111,160-168` and `typeperf-counters.txt:14-15` (suggestion, enhancement)
```go
type procSnap struct {
name string
pid int
cpu float64
wsMB float64
handles int // never printed
threads int // never printed
}
```
`printTopN` at line 186 prints only `name`, `pid`, `cpu`, `wsMB`. The `handles`/`threads` fields on `procSnap` (lines 29-30) and on `procIndex` (lines 38-39), the header detection at lines 108-111, and the per-sample extraction at lines 160-168 are all dead code. The matching `\Process(*)\Handle Count` and `\Process(*)\Thread Count` counters in `typeperf-counters.txt:14-15` bloat the BLG/TSV for no benefit (each counter adds one column per tracked process per sample).
Fix: either extend the output format to display them (handle/thread counts are useful for leak diagnosis) or remove the dead code and counter lines. Extending is probably the better choice given why those counters were added in the first place:
```diff
- fmt.Printf(" %-30s PID %6d CPU %7.1f%% WS %8.1f MB\n",
- s.name, s.pid, s.cpu, s.wsMB)
+ fmt.Printf(" %-30s PID %6d CPU %7.1f%% WS %8.1f MB H %6d T %4d\n",
+ s.name, s.pid, s.cpu, s.wsMB, s.handles, s.threads)
```
**S3. `Stop-Process` followed immediately by `relog` risks a file-lock race** — `.github/actions/windows-typeperf-sampler/action.yaml:42,51` (suggestion, enhancement)
```powershell
Stop-Process -Id $typeperfPid -Force -ErrorAction SilentlyContinue
Remove-Item $pidFile -Force -ErrorAction SilentlyContinue
# ...
relog $blgFile -f TSV -o $tsvFile -y
```
`Stop-Process -Force` issues `TerminateProcess`, which is asynchronous — it requests cancellation of pending I/O and returns before the kernel fully tears down the process. On a busy runner the typeperf process may still hold the BLG file when `relog` starts, producing a "file in use" error. Low probability but easy to eliminate, and the cost of a failure (missing summary) partly defeats the point of the instrumentation.
Fix:
```diff
Stop-Process -Id $typeperfPid -Force -ErrorAction SilentlyContinue
+Wait-Process -Id $typeperfPid -Timeout 5 -ErrorAction SilentlyContinue
Remove-Item $pidFile -Force -ErrorAction SilentlyContinue
```
**S4. CSV read errors silently swallowed mid-file** — `.github/actions/windows-typeperf-sampler/summarize-typeperf.go:118-122` (suggestion, enhancement)
```go
for {
record, err := reader.Read()
if err != nil {
break
}
rowNum++
```
The loop breaks on any error, including mid-file parse errors from `encoding/csv` (wrong field count, bad quoting under `LazyQuotes`). A truncated or corrupt sample would cause the summary to silently stop short, and the `Samples: N` footer at line 176 would misleadingly show a low count without explanation. Worth distinguishing `io.EOF` from other errors:
```diff
for {
record, err := reader.Read()
+ if errors.Is(err, io.EOF) {
+ break
+ }
if err != nil {
- break
+ fmt.Fprintf(os.Stderr, "read row %d: %v\n", rowNum+1, err)
+ break
}
rowNum++
```
(Also add `errors` and `io` imports.)
**S5. `typeperf` lacks `-y` so an existing BLG would hang the sampler** — `.github/actions/windows-typeperf-sampler/action.yaml:29-30` (suggestion, enhancement)
```powershell
$proc = Start-Process -PassThru -WindowStyle Hidden -FilePath typeperf `
-ArgumentList "-cf", $counterFile, "-f", "BIN", "-o", $blgFile, "-si", "1"
```
If `$blgFile` already exists, `typeperf` prompts to overwrite and blocks on stdin. With `-WindowStyle Hidden`, the prompt is invisible and typeperf hangs indefinitely. CI jobs use fresh runners, so this is unlikely in practice — but `relog` on line 51 already passes `-y` for the same reason; the start step should be symmetric:
```diff
- -ArgumentList "-cf", $counterFile, "-f", "BIN", "-o", $blgFile, "-si", "1"
+ -ArgumentList "-cf", $counterFile, "-f", "BIN", "-o", $blgFile, "-si", "1", "-y"
```
## Design Observations
### Strengths
- Split between one-shot machine-info snapshot and continuous typeperf sampler is the right factoring: they have different lifecycles and runtimes, and each is independently useful for diagnosis.
- Switching from direct-to-TSV to BLG (commit `e2511a8`) was the right call — the commit message correctly identifies why direct TSV mode misses mid-run processes.
- Deleting the BLG after converting to TSV (action.yaml:54) keeps artifact bloat under control.
- The Go post-processor indexes once from the header, then streams rows — avoids re-scanning column metadata per row.
## Testing Assessment
CI instrumentation code; no test coverage is expected. The machine-info regex bug (I1) would have been caught by a single manual run and inspection of the output file — a reminder to eyeball the artifact on the first CI run that includes it.
## Documentation Assessment
- S1 covers the stale package doc in `summarize-typeperf.go`.
- The PR summary in the review prompt describes the sampler as running "alongside the existing PowerShell sampler so we can compare overhead and data quality", but the final commit `e44cc1b` removes the PowerShell sampler. The PR description should be updated to match the final state before merge.
## Commit Structure
The four commits form a coherent narrative (add → add second variant → fix → consolidate). `e2511a8` is arguably a fix that could have been squashed into `5290abd`, but leaving it separate preserves useful context about why BLG is required over direct-to-TSV. No concerns.
## Coverage Summary
| File | Status |
|------|--------|
| `.github/actions/windows-machine-info/action.yaml` | Reviewed, no issues |
| `.github/actions/windows-machine-info/machine-info.ps1` | **I1** |
| `.github/actions/windows-typeperf-sampler/action.yaml` | **S3, S5** |
| `.github/actions/windows-typeperf-sampler/summarize-typeperf.go` | **S1, S2, S4** |
| `.github/actions/windows-typeperf-sampler/typeperf-counters.txt` | **S2** |
| `.github/workflows/bats.yaml` | Reviewed, no issues |
### [Codex] — Codex GPT 5.4
### Executive Summary
This PR adds two Windows CI diagnostics: a machine-info artifact and a new `typeperf`-based sampler that records raw counters to BLG, converts them to TSV, and summarizes the result after BATS completes. The main issue is in the stop path: the action deletes the raw BLG even when `relog` fails, which turns a transient post-processing failure into permanent diagnostic data loss; I also found two lower-severity issues around the pre-test disk benchmark perturbing the runner state and a stale usage comment in the Go summarizer.
### Findings
**Critical Issues**
None.
**Important Issues**
I1. **Failed `relog` runs discard the only raw capture artifact** — `.github/actions/windows-typeperf-sampler/action.yaml:48-56` `(important, regression)`
```yaml
if (Test-Path $blgFile) {
$size = (Get-Item $blgFile).Length / 1MB
Write-Output ("typeperf.blg: {0:N1} MB" -f $size)
relog $blgFile -f TSV -o $tsvFile -y
$lines = (Get-Content $tsvFile | Measure-Object -Line).Lines
Write-Output ("typeperf.tsv: {0:N1} MB, {1} lines" -f ((Get-Item $tsvFile).Length / 1MB), $lines)
Remove-Item $blgFile -Force
```
The stop step invokes `relog` at line 51, but it never checks whether the native command succeeded before it reads `$tsvFile` at lines 52-53, deletes `$blgFile` at line 54, and runs the Go summarizer at lines 55-56. In PowerShell, a non-zero exit from `relog` does not automatically throw, so a conversion failure can flow straight through this block. Because the workflow deliberately masks sampler failures with `continue-on-error: true` in `.github/workflows/bats.yaml:157-177`, that path will not fail the job; it will just delete the only raw BLG artifact and leave you with no way to inspect or re-run conversion later. For a diagnostics-only action, losing the raw capture is the worst possible failure mode.
Fix: gate the rest of the block on `relog` success, and only remove the BLG after both conversion and summarization succeed. If conversion fails, keep `typeperf.blg` and emit an explicit warning.
```diff
if (Test-Path $blgFile) {
$size = (Get-Item $blgFile).Length / 1MB
Write-Output ("typeperf.blg: {0:N1} MB" -f $size)
relog $blgFile -f TSV -o $tsvFile -y
- $lines = (Get-Content $tsvFile | Measure-Object -Line).Lines
- Write-Output ("typeperf.tsv: {0:N1} MB, {1} lines" -f ((Get-Item $tsvFile).Length / 1MB), $lines)
- Remove-Item $blgFile -Force
- go run "${{ github.action_path }}/summarize-typeperf.go" `
- $tsvFile 10 > "$outputDir/typeperf-summary.txt"
+ if ($LASTEXITCODE -ne 0 -or -not (Test-Path $tsvFile)) {
+ Write-Warning "relog failed; preserving $blgFile for inspection"
+ exit 1
+ }
+ $lines = (Get-Content $tsvFile | Measure-Object -Line).Lines
+ Write-Output ("typeperf.tsv: {0:N1} MB, {1} lines" -f ((Get-Item $tsvFile).Length / 1MB), $lines)
+ go run "${{ github.action_path }}/summarize-typeperf.go" `
+ $tsvFile 10 > "$outputDir/typeperf-summary.txt"
+ if ($LASTEXITCODE -eq 0) {
+ Remove-Item $blgFile -Force
+ }
}
```
**Suggestions**
S1. **The new machine-info step perturbs the disk/cache state before the test run it is supposed to diagnose** — `.github/workflows/bats.yaml:150-165`, `.github/actions/windows-machine-info/machine-info.ps1:79-123` `(suggestion, regression)`
```yaml
- name: "Windows: capture machine info"
if: runner.os == 'Windows'
uses: ./.github/actions/windows-machine-info
with:
output-path: rdd-logs/_diag/machine-info.txt
- run: make -C bats --keep-going --jobs=$(nproc) --output-sync=target
```
`bats.yaml` runs the machine-info action immediately before the BATS step at lines 150-165, and `machine-info.ps1` deliberately writes and rereads a 256 MB file on the same volume at lines 79-123. The comment at lines 80-86 even explains that this benchmark targets the same path BATS uses and measures a cache-warm rate. That makes the diagnostic self-interfering: every Windows run now starts with a large warm-up I/O burst that changes the disk and file-cache state before the actual workload begins. This does not break correctness, but it does bias the timing data you are trying to collect.
Fix: make the throughput benchmark opt-in via an action input, or move it after the BATS step so the artifact still captures runner characteristics without perturbing the test itself.
S2. **The summarizer’s top-level comment still points maintainers at the deleted script name/path** — `.github/actions/windows-typeperf-sampler/summarize-typeperf.go:5-10` `(suggestion, regression)`
```go
// summarize-perf-counters reads a TSV file produced by typeperf and prints
// a compact summary of system counters and top processes per sample.
//
// Usage:
//
// go run scripts/summarize-perf-counters.go [topN]
```
The file now lives at `.github/actions/windows-typeperf-sampler/summarize-typeperf.go`, and the workflow invokes that exact path at `.github/actions/windows-typeperf-sampler/action.yaml:55-56`. The header comment at lines 5-10 still refers to the old `summarize-perf-counters` name and `scripts/summarize-perf-counters.go` path, so anyone trying to reproduce a summary locally will be sent to a file that no longer exists.
Fix: update the comment to match the current filename and invocation path.
```diff
-// summarize-perf-counters reads a TSV file produced by typeperf and prints
+// summarize-typeperf reads a TSV file produced by typeperf and prints
// a compact summary of system counters and top processes per sample.
//
// Usage:
//
-// go run scripts/summarize-perf-counters.go [topN]
+// go run .github/actions/windows-typeperf-sampler/summarize-typeperf.go [topN]
```
### Design Observations
**Strengths**
- Using BLG as the collection format and converting with `relog` later is a sound design choice for this problem. It decouples low-overhead sampling from post-processing and avoids the blind spot that the original TSV-only approach had for processes that started after collection began.
### Testing Assessment
1. No test covers the stop-path failure mode where `relog` or the Go summarizer fails after the BLG has been collected. A small fixture-based test or scripted failure injection should verify that the raw BLG is preserved.
2. The Go summarizer has no fixture tests for real `relog` output, so there is no automated coverage for header parsing, duplicate process instances (`foo`, `foo#1`), or malformed rows.
3. The workflow change has no end-to-end validation that `start` plus `stop` actually produces `typeperf.tsv` and `typeperf-summary.txt` on Windows runners.
### Documentation Assessment
The main documentation gap is the stale usage comment in `summarize-typeperf.go` called out in S2. I did not find any broader repo docs that obviously need updating for this CI-only change.
### Acknowledged Limitations
- **Code comment**: “FileStream with no buffering still goes through the OS file cache, so this is the cache-warm sequential rate, not raw disk speed” — `.github/actions/windows-machine-info/machine-info.ps1:83-86`. The comment correctly acknowledges that the benchmark is not a raw-disk measurement; with this PR, that limitation matters more because the benchmark now runs on every Windows BATS job before the tests start.
### Coverage Summary
- `.github/actions/windows-machine-info/action.yaml` — Reviewed, no issues
- `.github/actions/windows-machine-info/machine-info.ps1` — S1
- `.github/actions/windows-typeperf-sampler/action.yaml` — I1
- `.github/actions/windows-typeperf-sampler/summarize-typeperf.go` — S2
- `.github/actions/windows-typeperf-sampler/typeperf-counters.txt` — Reviewed, no issues
- `.github/workflows/bats.yaml` — I1, S1
### [Gemini] — Gemini 3.1 Pro
### Executive Summary
This PR adds two Windows CI diagnostic utilities for use during BATS integration testing: a hardware/OS state capture script (`machine-info.ps1`) and a low-overhead performance sampling tool (`typeperf`). These tools provide granular, mid-run resource utilization metrics without the overhead of PowerShell counter loops. The implementation is robust, efficiently parsing TSV outputs via a custom Go summarizer. The primary finding is a race condition when stopping the background `typeperf` process, which could lead to file lock contention during log conversion.
### Critical Issues
None.
### Important Issues
I1. **Race condition when stopping typeperf** — `.github/actions/windows-typeperf-sampler/action.yaml:42-45` (important, regression)
```powershell
$typeperfPid = Get-Content $pidFile
Stop-Process -Id $typeperfPid -Force -ErrorAction SilentlyContinue
Remove-Item $pidFile -Force -ErrorAction SilentlyContinue
Write-Output "stopped typeperf pid=$typeperfPid"
```
`Stop-Process` executes asynchronously by default—it signals the process to terminate but does not wait for it to fully exit. Because the script immediately proceeds to run `relog $blgFile` in the subsequent block, there is a race condition. If the operating system has not yet finished tearing down `typeperf` and releasing its file handles, `relog` will fail with a file sharing violation, resulting in a missing TSV file and skipped summary output.
Fix: Add `Wait-Process` to synchronously wait for the process to exit and release its resources before proceeding.
```diff
if (Test-Path $pidFile) {
$typeperfPid = Get-Content $pidFile
Stop-Process -Id $typeperfPid -Force -ErrorAction SilentlyContinue
+ Wait-Process -Id $typeperfPid -Timeout 5 -ErrorAction SilentlyContinue
Remove-Item $pidFile -Force -ErrorAction SilentlyContinue
Write-Output "stopped typeperf pid=$typeperfPid"
}
```
### Suggestions
S1. **Missing disposal for file streams on error** — `.github/actions/windows-machine-info/machine-info.ps1:87-99` (suggestion, regression)
```powershell
$fs = [System.IO.File]::Create($benchFile)
for ($i = 0; $i -lt $sizeMB; $i++) {
$fs.Write($buf, 0, $buf.Length)
}
$fs.Flush($true) # flush to disk
$fs.Close()
```
If an exception is thrown during `$fs.Write` (e.g., if the drive runs out of space), the script jumps directly to the `catch` block, skipping `$fs.Close()`. The file stream will remain open, locking `$benchFile`. When the `finally` block attempts `Remove-Item`, it will fail silently due to `-ErrorAction SilentlyContinue`, leaving a 256MB garbage file in the temporary directory until the PowerShell process exits or garbage collection reclaims it. The identical pattern occurs in the read loop below it.
Fix: Ensure the stream is disposed within a `finally` block so the file handle is reliably released regardless of errors.
```diff
- $fs = [System.IO.File]::Create($benchFile)
- for ($i = 0; $i -lt $sizeMB; $i++) {
- $fs.Write($buf, 0, $buf.Length)
- }
- $fs.Flush($true) # flush to disk
- $fs.Close()
+ try {
+ $fs = [System.IO.File]::Create($benchFile)
+ for ($i = 0; $i -lt $sizeMB; $i++) {
+ $fs.Write($buf, 0, $buf.Length)
+ }
+ $fs.Flush($true) # flush to disk
+ } finally {
+ if ($fs -is [System.IO.Stream]) { $fs.Dispose() }
+ }
```
### Design Observations
#### Concerns
None.
#### Strengths
- **Graceful degradation in CSV parsing:** `summarize-typeperf.go` handles missing counter columns elegantly by initializing process indices to `-1` and using safe `colFloat` boundary checks. This prevents panics and accurately skips missing data.
- **Error resilience in diagnostics:** Explicitly setting `$ErrorActionPreference = 'Continue'` at the start of `machine-info.ps1` prevents transient environmental errors (e.g., missing Defender components) from aborting the entire data-gathering script.
### Testing Assessment
The PR introduces CI instrumentation and diagnostic scripts. Formal unit/integration tests are not applicable, but these scripts should be validated through a dry-run CI execution to verify pathing, tool availability, and artifact generation.
### Documentation Assessment
None.
### Acknowledged Limitations
- **Code comment** in `.github/actions/windows-machine-info/machine-info.ps1:81-84`: The author correctly acknowledges that the disk benchmark (`FileStream` with no buffering) still goes through the OS file cache, measuring the cache-warm sequential rate rather than raw disk IO bounds.
### Coverage Summary
- `.github/actions/windows-machine-info/action.yaml`: Trivial
- `.github/actions/windows-machine-info/machine-info.ps1`: S1
- `.github/actions/windows-typeperf-sampler/action.yaml`: I1
- `.github/actions/windows-typeperf-sampler/summarize-typeperf.go`: Reviewed, no issues
- `.github/actions/windows-typeperf-sampler/typeperf-counters.txt`: Trivial
- `.github/workflows/bats.yaml`: Reviewed, no issues
---
## Resolution
| | |
|---|---|
| **Addressed** | 2026-04-11 |
| **Commit** | Fixups squashed into `7494dc1`, `8b38091`, `29c2bd4` via `git rebase --autosquash` |
| # | Finding | Action |
|---|---------|--------|
| 1 | Important #1: Env-var filter regex matches only exact names | Fixed |
| 2 | Important #2: Stop step deletes raw BLG on relog failure | Fixed |
| 3 | Suggestion #1: `Stop-Process` + `relog` file-lock race | Fixed |
| 4 | Suggestion #2: Stale `summarize-perf-counters` header comment | Fixed |
| 5 | Suggestion #3: Disk benchmark warms file cache before BATS | Skipped |
| 6 | Suggestion #4: `FileStream` benchmark does not dispose handle on error | Fixed |
| 7 | Suggestion #5: `handles`/`threads` counters collected but never printed | Fixed |
| 8 | Suggestion #6: CSV read loop swallows parse errors | Fixed |
| 9 | Suggestion #7: `typeperf` start lacks `-y` | Fixed |
| 10 | Documentation Gap #1: PR #292 description is stale | Documentation updated |