Skip to content

The test-config guard travels with the config it validates (#2865) — and #2998 is not a hang - #3012

Merged
rbuergi merged 3 commits into
mainfrom
fix/2998-teardown-hang
Sep 1, 2026
Merged

rbuergi merged 3 commits into
mainfrom
fix/2998-teardown-hang

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🚨 Read this first: this is not a fix for a hang, because there wasn't one

I was sent to root-cause the ambient test-host hang in Systemorph/MeshWeaver.Plugins#1153 and #2865. Re-running both issues' own
measurements changed the task, so the headline is the re-measurement:

#2998 is settled and is not a hang. Its author retracted the body's framing in the comments and
retitled it. The suite was measured standalone: 787 tests, 10 m 32 s, terminated normally. It is
#39 of 48 in a serial CI step that needs ~80–90 minutes and has 60; 37 suites completed, it never
finished, and 9 after it never started. A capacity defect. The fix is
MeshWeaver.Plugins#1132 (4-way matrix), already open there. Nothing for core to do.

Two claims in that body are explicitly withdrawn and should not be re-derived:

  • "emits no test output at all" — the console logger prints two header lines then nothing until the
    summary. A passing 383-test suite in the same run showed 1 m 52 s of identical silence. And on
    one of the three runs the suite was not silent: it printed a per-test [FAIL] at 11m08s whose
    mesh disposed cleanly in 4 ms.
  • the ObjectDisposedException stragglers at TeardownOrderedScopeDisposal.CloseWhenDrained — these
    are first-chance, and that call site catches exactly that type. They arrive in bursts at ~6
    distinct instants (successive classes disposing = progress), and on the main run the last one
    landed 7m53s before the kill.

One more trap for whoever picks up #2865: the log line that issue's discriminator greps for —
Dropping StreamEndedEvent … no synchronization hub found on this hub or any parent — no longer
exists
. #2928 (merged 2026-09-01) split that diagnostic by message type: the StreamEndedEvent
case moved to Debug and was reworded to "the target stream is already gone, and a terminal
notice to a departed subscriber is a no-op"
. Re-running the documented grep on a fresh capture now
returns a false negative for two independent reasons — the level and the text. Match on
Dropping StreamEndedEvent alone, with Debug enabled for MeshWeaver.Data.DataExtensions.

Also stale: #2865 records its leading root (#2778) as "currently REVERTED". It was re-fixed by a
different route in #2868, merged 2026-08-31 04:54Z — after every occurrence #2865 lists. That
issue needs its measurement re-run before more investigation, not more investigation.

I did not attempt a local repro: this host sat at load 99–204 on 18 cores, where CPU
starvation produces this family's exact signature. Every check in this PR is load-independent.

What this PR actually fixes: a guard that could not reach the fleet

test/Directory.Build.props selects one of two Content branches for xunit.runner.json and
appsettings.json. VerifyXunitRunnerConfigCopied — the target that errors when it selects
neither — sat in test/Directory.Build.targets.

MSBuild imports Directory.Build.props and Directory.Build.targets independently, so a
consumer can take one without the other. Every satellite that consumes the plumbing does exactly
that:

checkout imports test/Directory.Build.props imports test/Directory.Build.targets *.Test projects
MeshWeaver.Plugins ✅ explicit, in src/Directory.Build.props ❌ 78
MeshWeaver.SocialMedia ✅ explicit, in src/Directory.Build.props ❌ 1
MeshWeaver.Education — neither ❌ —
MeshWeaver.Reinsurance — neither ❌ —
MeshWeaver.Manufacturing — neither ❌ —

Measured 2026-09-01: zero of five checkouts reference test/Directory.Build.targets at all.

They inherited the plumbing without the assertion that it had worked: 79 satellite test
projects without the guard, against core's ~15 with it.

(I first wrote this as "all four satellites import the props". Checking whether the checkouts
actually existed and what they imported cut it to two — recorded because the broader claim was the
one that felt obviously true.)

Why that matters, and why it belongs to #2865

When the selection picks neither branch the build still succeeds and the tests still run — under
xUnit's own defaults: parallelizeTestCollections=true, threads = core count, and no
methodTimeout
.

That last one is the diagnosability dependency. From #2865's own comment 4:

how the test dies transcript of the hung test usable
xUnit methodTimeout (30 s) written in full yes
host wall-clock cap (exit=124 / HOST_CRASHED) destroyed no

With methodTimeout in force a wedged test is killed individually and leaves its window behind.
Without it nothing bounds the test, the shard runs to its cap, the host is killed, and the in-flight
test's transcript is destroyed — which is exactly the structural obstacle #2865 records as the reason
the family cannot be diagnosed from CI artifacts at all. Whether a hang is investigable depends on
this file's includes having worked, and outside core nothing checked that they had.

The change

The guard now lives in test/Directory.Build.props, in the same file as the includes it
validates
, so importing the mechanism cannot leave the check behind. No satellite change required.
test/Directory.Build.targets is kept but emptied (there is no root-level one, so keeping it
preserves the auto-import surface exactly).

This is AGENTS.md's CI rule one layer down: a gate must never be separable from the input it
validates
, because a gate that never runs is indistinguishable from a gate that passed.

Verification that can fail

  • Mutation-proved reachable. dotnet msbuild … -t:VerifyXunitRunnerConfigCopied -p:TargetDir=<empty dir>
    errors from test/Directory.Build.props(221,5) — the target is defined, reachable and fires.
  • The new test fails on origin/main. Checked mechanically against git show HEAD: of both files:
    TheGuardIsDeclaredInTheFileThatCarriesThePlumbing → FAIL before / PASS after;
    TheGuardIsNotDeclaredInTheFileNobodyImports → FAIL before / PASS after.
  • MeshWeaver.Documentation.Test 169/169 green (full project, no --filter), all three new cases
    present in the trx. Includes DocumentationLinkIntegrityTest for the new doc page.
  • Release -warnaserror clean, one project per invocation: MeshWeaver.Documentation,
    MeshWeaver.Documentation.Test, MeshWeaver.Fixture, MeshWeaver.Hosting.Test.

🚨 Blast radius to watch after merge

The guard will now run for satellite test projects on their next build. I checked statically that it
should pass: no satellite csproj declares a conflicting xunit.runner.json include, and the three
Plugins projects that ship a local one are handled by the Exists(...) branch. If it does go red
somewhere, that is a real pre-existing defect being surfaced, not a regression from this PR
— those
assemblies were running with unbounded parallelism and no per-test timeout. Do not suppress it.

Still open (deliberately not guessed at)

What ends a live node's synchronization streams ~5 s into an operation that is still running
(#2776 question 1). The caller-side half is fixed (#2868) and the 5 s queue-advance bound is
deliberately independent of the caller's terminal. Both witness suites live in MeshWeaver.Plugins and
the class is bulk-only, so it needs a local bulk run inspected while wedged — on a quiet machine.
The RCA page records the discriminator and the two readings already falsified.

Note: #3009 (fix/2986-ride-out-the-shuttingdown-nack) is open in the same problem area. No source
overlap; both add a row to Data/Architecture.md, so expect a one-line index conflict whichever
lands second.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa

`test/Directory.Build.props` picks ONE of two Content branches for
xunit.runner.json and appsettings.json. `VerifyXunitRunnerConfigCopied` — the
target that errors when it picks NEITHER — sat in `test/Directory.Build.targets`.

MSBuild imports Directory.Build.props and Directory.Build.targets independently,
so a consumer can take one without the other, and every satellite that consumes
the plumbing does exactly that: MeshWeaver.Plugins (78 *.Test projects) and
MeshWeaver.SocialMedia (1) import `$(MeshWeaverRoot)/test/Directory.Build.props`
explicitly from their own src/Directory.Build.props, and NEITHER imports the
targets file. Measured 2026-09-01: ZERO of the five satellite checkouts
(.Plugins, .Education, .Reinsurance, .SocialMedia, .Manufacturing) reference
test/Directory.Build.targets at all. They inherited the plumbing without the
assertion that it had worked — 79 satellite test projects without the guard,
against core's ~15 with it.

When the selection picks neither branch the build still succeeds and the tests
still run, under xUnit's own defaults: parallelizeTestCollections=true, threads =
core count, and NO methodTimeout. The last one is why this belongs to the
ambient-hang family (#2865): with methodTimeout in force a wedged test is killed
individually and its transcript is WRITTEN — the only usable witness that issue
has found. Without it nothing bounds the test, the shard runs to its wall-clock
cap, the host is killed (exit=124 / HOST_CRASHED) and the in-flight test's
transcript is DESTROYED. Whether a hang is investigable at all depends on this
file's includes having worked, and outside core nothing checked that they had.

The guard now lives in test/Directory.Build.props, in the same file as the
includes it validates, so importing the mechanism cannot leave the check behind —
no satellite change required. This is AGENTS.md's CI rule one layer down: a gate
must never be separable from the input it validates, because a gate that never
runs is indistinguishable from a gate that passed.

Verification (host at load ~99/18 cores; every check below is load-independent):
  * mutation-proved reachable — `dotnet msbuild -t:VerifyXunitRunnerConfigCopied
    -p:TargetDir=<empty dir>` errors from Directory.Build.props(221,5);
  * DirectoryBuildGuardTravelsWithItsSubjectTest fails on origin/main and passes
    here, checked mechanically against `git show HEAD:` of both files;
  * MeshWeaver.Documentation.Test 169/169 green, the three new cases present in
    the trx; Release -warnaserror clean on every touched project.

Also conserves the RCA of the family as Doc/Architecture/AmbientTestHostHangs —
including that #2998 was re-measured as a CI capacity defect and is not a hang,
and that #2865's named root landed on 2026-08-31 after every occurrence it lists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa
Copilot AI lite review requested due to automatic review settings September 1, 2026 19:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new guard-travel test has a confirmed anti-vacuity hole (comment text can satisfy the “plumbing exists” regex), and there are verified stale/inconsistent measurement notes in newly added comments that should be corrected before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the shared test-configuration plumbing by relocating the VerifyXunitRunnerConfigCopied MSBuild guard into test/Directory.Build.props (the file satellites actually import), ensuring the guard can’t be “left behind” when the plumbing is consumed outside this repo. It also documents the ambient test-host hang/crash family and adds a regression test to pin the guard/plumbing co-location.

Changes:

  • Move VerifyXunitRunnerConfigCopied from test/Directory.Build.targets into test/Directory.Build.props and leave the targets file intentionally empty to preserve import surface behavior.
  • Add a documentation test that asserts the guard lives with its subject (and doesn’t drift back into the unused targets file).
  • Add new documentation: a “What’s New” entry and an architecture page recording the ambient hang family, plus an index entry.
File summaries
File Description
test/MeshWeaver.Documentation.Test/DirectoryBuildGuardTravelsWithItsSubjectTest.cs Adds assertions pinning guard/plumbing co-location and anti-vacuity checks.
test/Directory.Build.targets Empties the file and documents why the guard must not live here.
test/Directory.Build.props Hosts the guard target alongside the Content plumbing it validates.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-a-wedged-test-can-still-say-which-test-it-was.md Announces the behavioral guarantee (guard travels with config) as a user-facing fix.
src/MeshWeaver.Documentation/Data/Architecture/AmbientTestHostHangs.md Adds a standing RCA page clarifying signatures, misreadings, and what’s still open.
src/MeshWeaver.Documentation/Data/Architecture.md Links the new architecture page from the catalog.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/Directory.Build.props
rbuergi and others added 2 commits September 1, 2026 22:00
… before matching

Three Copilot review findings, all valid.

1. THE ANTI-VACUITY CHECK COULD PASS ON PROSE ALONE. ThePlumbingTheGuardChecksStillLivesInThatFile
   matched the RAW props text, and that file quotes `<Content Include="appsettings.json">` verbatim
   inside its own explanatory comment — so the guard would have stayed green after the real
   ItemGroup was deleted, satisfied by the documentation of the thing it guards. StripComments
   already existed for the other two assertions; it is now applied here too.

   Mutation-proved rather than asserted — both real appsettings ItemGroups deleted, comments intact:
     HEALTHY  old(raw) PASS   new(strip) PASS
     MUTANT   old(raw) PASS   new(strip) FAIL
   i.e. the old form could not fail, the new one does.

2. A stale note in the appsettings comment still said the target lived in Directory.Build.targets.
   It now points at the bottom of this file, and records the move.

3. The test class summary still carried the pre-measurement claim that .Education and .Reinsurance
   import the props. Corrected to the measured fact everywhere: only .Plugins (78 *.Test) and
   .SocialMedia (1) import it; the other three import neither; ZERO of five reference the targets.

Verified on the settled tree: Release -warnaserror clean (MeshWeaver.Documentation,
MeshWeaver.Documentation.Test) and MeshWeaver.Documentation.Test 169/169 green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa
# Conflicts:
#	src/MeshWeaver.Documentation/Data/Architecture.md
@rbuergi

rbuergi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Merged origin/main in to clear DIRTY. Recording the resolution because a merge is the one place a fix can silently disappear, and this one had a conflict where both sides had added something.

Why it had to be merged at all: a DIRTY PR runs no CI whatsoever — not a red, an absence. Left alone it would have sat looking merely "not green yet" indefinitely.

The conflict was one file, Data/Architecture.md, and both sides had extended the same table:

row HEAD (this branch) origin/main
Reliability & wedges base + Riding Out a ShuttingDown Address (landed with #2986 / PR #3009)
Testing & debugging + Ambient Test-Host Hangs (this branch) base

So neither side was a superset, and taking either wholesale would have dropped the other's new page from the index — the page would still exist and simply stop being reachable from the architecture map, which is exactly the kind of loss a green build does not notice.

Resolved by taking origin/main's Reliability row and HEAD's Testing row, keeping both additions. Verified after resolving:

$ grep -c "^<<<<<<<\|^=======\|^>>>>>>>" Data/Architecture.md
0
$ grep -o "RidingOutAShuttingDownAddress\|AmbientTestHostHangs" Data/Architecture.md | sort -u
AmbientTestHostHangs
RidingOutAShuttingDownAddress
$ ls Data/Architecture/ | grep -E "AmbientTestHostHangs|RidingOutAShuttingDownAddress"
AmbientTestHostHangs.md
RidingOutAShuttingDownAddress.md

Both rows point at pages that exist.

Checked that the merge did not revert main. git diff origin/main --stat afterwards contains only this branch's own additions, and #2986's fix is intact on the merged tree (MaxSameActivationReAsks / RecycleReAskPace present in SyncStreamOptions.cs and JsonSynchronizationStream.cs).

Re-verified after merging, because this branch touches test/Directory.Build.props and .targets, which every test project consumes:

  • dotnet build -c Release -warnaserror on MeshWeaver.Documentation.Test — 0 Warning(s), 0 Error(s)
  • full project, not a filter — 171 passed, 0 failed, including DocumentationLinkIntegrityTest (up from 168 pre-merge, i.e. the new guards are running rather than silently absent)

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

216 tests   216 ✅  2m 29s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit d56dfbe.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

586 tests   586 ✅  49s ⏱️
  2 suites    0 💤
  2 files      0 ❌

Results for commit d56dfbe.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

534 tests   532 ✅  1m 31s ⏱️
  4 suites    2 💤
  4 files      0 ❌

Results for commit d56dfbe.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 693 tests   1 693 ✅  2m 4s ⏱️
    3 suites      0 💤
    3 files        0 ❌

Results for commit d56dfbe.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    3 files      3 suites   1m 16s ⏱️
1 218 tests 1 026 ✅ 192 💤 0 ❌
1 219 runs  1 027 ✅ 192 💤 0 ❌

Results for commit d56dfbe.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

420 tests   420 ✅  1m 40s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit d56dfbe.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results

   14 files     14 suites   9m 51s ⏱️
4 667 tests 4 473 ✅ 194 💤 0 ❌
4 668 runs  4 474 ✅ 194 💤 0 ❌

Results for commit d56dfbe.

@rbuergi
rbuergi merged commit 88c97ae into main Sep 1, 2026
25 of 26 checks passed
@rbuergi
rbuergi deleted the fix/2998-teardown-hang branch September 13, 2026 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants