Retry crashed test hosts, widen render tolerance, end unit++ cleanly

[johnml1135]

Three test-infrastructure fixes that make test.ps1 green and honest on a developer machine. A crashed managed test host is now retried and reported as flaky instead of silently dropping the assemblies after it. Render baselines tolerate cross-machine font-smoothing drift without admitting a moved glyph. The native unit++ harness exits cleanly instead of hanging and being killed with exit code -1.

What a reviewer will ask: does the retry hide real failures? No. Only the vstest crash line triggers a retry; a run that reported a test failure is an answer and is never rerun. A run that passed only after a retry prints [FLAKY], writes TestResults/crash-retries.json, appends to the GitHub step summary, and exits 2, so an exit-code gate still fails. The question worth your time is whether exit 2 is the right policy, and note it reaches more than CI.yml: base-installer-cd.yml and patch-installer-cd.yml also run test.ps1, so a recovered crash would fail an installer release build too. The alternative is a recovered crash staying green with the report as the only signal.

Where to look

• test.ps1 Invoke-VsTestWithCrashRetry: one loop for both the single-assembly and per-assembly paths; Test-HostCrashed matches only Test host process crashed, because vstest also prints the aborted line on Ctrl+C and CI cancellation.
• RenderSnapshotVerifier.IsWithinTolerance: size must match, fewer than 100 pixels may differ, and their summed magnitude must stay under 10 full-pixel equivalents. Measured drift peaks at 50 and 1.57.
• Lib/src/unit++/main.cc: TerminateProcess after GlobalTeardown; static destructors no longer run.
• .github/workflows/CI.yml: crash-retries.json rides with the TRX artifacts.

Deliberately not here

• The host crash itself is not diagnosed. -Blame records the culprit test, but a stack needs procdump on PATH for /Blame:CollectDump on .NET Framework.
• No per-pixel colour floor and no area-relative pixel ratio in the render gate. Both are the industry norm and would let the counts tighten again; follow-up.
• No Flaky test category or quarantine list. Nothing has shown a per-test flake in CI yet.

Verification

Built with build.ps1 -CommentHygiene -TokenHygiene. RootSiteTests 99 passed, 1 skipped, with all 15 VerifyScenario cases green on a box where 12 failed before. Native TestGeneric [31-0-0] and TestViews [309-0-0], both exiting 0. Full suite once with all three fixes: 6067 tests, 6005 passed, 62 skipped, 0 failed, exit 0. The retry loop's crash branch was checked against a recorded crash transcript, not a live crash: none occurred during verification.

---

Reading this a year from now -- start here

Main CI was green throughout. Every one of these three problems appeared only on a developer machine: render baselines captured on one box on 2026-09-03 drifted on the same box a week later, the unit++ hang needs the desktop text-input host that a CI runner does not start, and the managed host crash showed up in one full run out of three locally and never in CI. The branch makes local runs trustworthy; it does not claim to fix a CI failure.

Decisions, and why

• Retry the crash, never the failure. Retrying a reported failure turns a nondeterministic assertion into a silent pass. A crash leaves no verdict, so rerunning it recovers missing results without laundering an answer.
• Exit 2 on a recovered crash. A magenta banner is invisible to a gate that reads the exit code, and no CI step reads crash-retries.json yet. The distinct code lets a future step treat "unstable" differently from "failed" without changing the script again.
• Match only the crash line. The active test run was aborted also appears on cancellation. With cancel-in-progress: true in CI, matching it would burn up to five retries on a run already being discarded.
• Both gates, not either. The touched-pixel count alone would admit a faint smear across the image; the magnitude alone would admit a small saturated change. Requiring both under their limits, plus identical size, keeps a one-pixel glyph shift failing on magnitude.
• TerminateProcess in the vendored harness. main.cc already carries repo-specific Windows patches, so patching it again follows precedent. The alternative, pumping messages or unloading the text-input stack, would be speculative work in a harness that has already printed its verdict.

Paths not taken

• Per-test [Retry]. NUnit's attribute retries assertion failures, not host crashes, which is the opposite of the policy wanted here.
• Marshal.ReleaseComObject in the render harness. Proposed as a crash cause; rejected because SimpleRootSite documents removing that call as unnecessary and the production GraphicsManager does not make it either.
• Soaking the full suite locally to catch the crash. Tried; a twelve-run unattended loop hung the workstation. Reproduction belongs in CI with -Blame and procdump.
• Widening only the pixel count. Would have admitted a 99-pixel saturated change. The magnitude gate exists to stop exactly that.

Evidence

Measured render drift per scenario, with both limits forced to zero so every scenario emitted its diff:

scenario	touched pixels	magnitude
many-paragraphs	50	1.57
footnote-heavy	16	0.50
complex	10	0.31
medium, multi-book, custom-heavy	5	0.16
long-prose, rtl-script	4	0.13
simple, deep-nested	3	0.09
lex-*, multi-ws, mixed-styles	0	0.00

Magnitude divided by touched pixels is about 8/255 everywhere: each differing pixel is off by roughly eight levels on all three channels, which is greyscale anti-aliasing drift. The verifier scores a pixel as channelDelta / (3 * 255), so 24/765 is the same 8/255 ratio.

• Old retry gap: the per-assembly fallback fired only on exit code -1; a host crash returns 1. Two aborted full runs stopped at exactly 4429 of 6067 tests.
• unit++ hang: TestGeneric runs the same OleInitialize/OleUninitialize sequence and exits cleanly; TestViews alone loads TextShaping.dll, msctf.dll and textinputframework.dll and leaves six threads parked after main returns. The runner's Stop-Process -Force is where -1 came from.
• Crash-line discrimination: vstest prints Test host process crashed only for a crashed host; the aborted line also appears on explicit cancellation (Suppress The active test run was aborted message when a run is explicitly cancelled microsoft/vstest#2270).

Preflight review details

Code Review Summary

Branch: test-infra/crash-retry-render-tolerance

Base: origin/main

Date: 2026-09-09

Review model: Claude Fable 5.1

Files changed: 5

Overview

Three local-only test-infrastructure defects surfaced while fixing the VwPattern
CI failure: render baselines drifted past a 4-pixel tolerance on a developer
box, native TestViews hung after main and was killed with exit code -1, and an
intermittent managed test-host crash aborted full runs at 4429 of 6067 tests
with the per-assembly fallback never firing. Main CI was green throughout.

The branch retries crashed hosts with mandatory flaky reporting and exit code 2,
gates render baselines on size plus pixel count plus summed magnitude, and ends
the unit++ process explicitly after GlobalTeardown.

Contract/API Changes

• test.ps1 gains -MaxCrashAttempts (1-20, default 5) and -Blame, and a
  new exit code 2 meaning all tests passed but a host crashed and was retried.
• RenderSnapshotVerifier tolerance changes from "at most 4 differing pixels"
  to "size matches, fewer than 100 differing pixels, summed magnitude under 10".
  RenderSnapshotComparisonReport and RenderPixelDiffSummary gain magnitude
  fields.
• unit++ executables no longer run static destructors after main.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

• A recovered crash exited 0, invisible to any gate that reads only the
  exit code (fixed during review: exit code 2, GitHub step summary entry, and
  crash-retries.json uploaded with the TRX artifacts)
• Retry only fired for multi-assembly runs; a -TestProject run got no
  retry (fixed during review: one Invoke-VsTestWithCrashRetry loop serves
  both paths)
• "The active test run was aborted" also matches CI cancellation, so a
  superseded run would burn five retries (fixed during review:
  Test-HostCrashed matches only the crash line)

Minor - Consider

• Crash-detection regex duplicated at two sites (fixed during review:
  extracted to Test-HostCrashed)
• Tolerance constants had no recorded rationale (fixed during review:
  measured drift and glyph-shift magnitude noted at the constants)
• TerminateProcess trade-off not stated (fixed during review:
  comment names the skipped static destructors)
• Absolute pixel count does not scale with image area, and no per-pixel
  colour floor excludes anti-aliasing drift before counting. Industry tools
  use a ratio plus a colour threshold. Deferred; noted in the PR body.
• Host crash root cause unproven. -Blame names the culprit test but a
  stack needs procdump for /Blame:CollectDump on .NET Framework. Deferred.

Required Validation / Evidence

• build.ps1 -CommentHygiene -TokenHygiene: clean.
• RootSiteTests single-assembly run through the new retry loop: 99 passed,
  1 skipped; all 15 VerifyScenario cases green where 12 failed before.
• Native TestGeneric [31-0-0] and TestViews [309-0-0], both exit 0.
• Full suite once: 6067 tests, 6005 passed, 62 skipped, 0 failed, exit 0.
• Not exercised live: the retry loop's crash branch. Verified by replaying the
  recorded crash output from an earlier aborted run through the predicate.
• Not run: test.ps1 -Coverage through the new loop.

Positive Observations

• Crash-only retry discrimination matches the Google and Fowler guidance that
  retrying a reported failure is how a suite starts lying.
• The size-mismatch hard fail matches every surveyed visual-regression tool.
• main.cc already carries repo-specific Windows patches, so this follows the
  file's precedent.

Interview Notes

• Author chose AND for the tolerance gates ("Both < 10 cumulative pixel AND
  < 100 touched AND size mismatch failure").
• Author asked for retry up to 5 attempts and named the failure mode to avoid:
  "The worst is to crash and say 'it's fine'". Exit code 2 follows from that.
• Author asked whether to soak the failing test 100 times; a twelve-run
  unattended soak hung the workstation, so reproduction is deferred to CI.
• Async-to-void change in RenderVerifyTests.VerifyScenario was suspect
  elimination for the host crash, kept because the await bought nothing.

In-Review Quality Check

• test.ps1 parses clean; powershell-compat clean on 5.1 and 7.0.
• gitlint clean on all three commits.

Suggested Review Focus

• Exit code 2 on a recovered crash: fail the CI gate, or track only?
• Are 100 pixels and magnitude 10 the right limits, given measured worst
  case 50 and 1.57 and that ~10 fully changed pixels would pass?
• TerminateProcess skipping static destructors in the unit++ harness.

🤖 Generated with Claude Code

---

This change is 

[github-actions]

NUnit Tests

    1 files  ± 0      1 suites  ±0   12m 20s ⏱️ -34s
6 105 tests +21  6 020 ✅ +21  85 💤 ±0  0 ❌ ±0 
6 114 runs  +21  6 029 ✅ +21  85 💤 ±0  0 ❌ ±0 

Results for commit 133eae5. ± Comparison against base commit 2b2c24a.

This pull request removes 1 and adds 22 tests. Note that renamed tests count towards both.

FwAvaloniaTests.DetailEditorParityTests ‑ ReferenceVector_ReadOnly_HasNothingToDetach

FwAvaloniaTests.DetailEditorParityTests ‑ ReferenceVector_ReadOnly_WiresOnlyTheItemSelectHandlers
FwAvaloniaTests.DetailFocusMemoryTests ‑ CaptureAndRestore_CarryTheVectorRowsCurrentItem_AcrossAViewRebuild
FwAvaloniaTests.DetailFocusMemoryTests ‑ RestoreAfterLayout_FocusesTheCurrentItem_OnlyWhenAskedTo_OrAfterAnItemRemoval
FwAvaloniaTests.DetailFocusMemoryTests ‑ RestoreAfterLayout_KeepsTheScrollOffset_WithNoFocusedEditor_WhenGivenBeforeLayout
FwAvaloniaTests.DetailMenuRequestTests ‑ ClickingAVectorItem_SelectsIt_AndTheLabelMenuRequestCarriesIt
FwAvaloniaTests.DetailMenuRequestTests ‑ ClickingAnotherVectorItem_MovesTheSelection_AndTheHighlight
FwAvaloniaTests.DetailMenuRequestTests ‑ CtrlClickingAVectorItem_RaisesTheDefaultActivation_ForThatItem
FwAvaloniaTests.DetailMenuRequestTests ‑ DeleteOrBackspace_OnAFocusedVectorItem_RemovesIt_ThroughTheEditContext
FwAvaloniaTests.DetailMenuRequestTests ‑ EditableVectorItem_WithAMenuBridge_HasNoLocalRemoveFlyout_AndWithoutOneKeepsIt
FwAvaloniaTests.DetailMenuRequestTests ‑ FocusingAVectorItem_SelectsIt_AndTheContextMenuKey_RaisesTheItemMenuUnderIt
…

♻️ This comment has been updated with latest results.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 64.51613% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.81%. Comparing base (e8a4dba) to head (133eae5).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...ommon/RenderVerification/RenderSnapshotVerifier.cs	64.51%	7 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1130      +/-   ##
==========================================
+ Coverage   38.62%   38.81%   +0.18%     
==========================================
  Files        1514     1517       +3     
  Lines      351047   351851     +804     
  Branches    40360    40510     +150     
==========================================
+ Hits       135580   136559     +979     
+ Misses     186267   186049     -218     
- Partials    29200    29243      +43     

Files with missing lines	Coverage Δ
...ommon/RenderVerification/RenderSnapshotVerifier.cs	74.04% <64.51%> (-0.64%)	⬇️

... and 41 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[papeh]

Slick changes! One suggested comment wording change.

[papeh]

I think "it fails regardless of the pixel tolerance" (as you have below) is a better wording