Stop IntegrationContext from disposing a class fixture it doesn't own (GH-3423)#3427
Merged
Merged
Conversation
…GH-3423) DefaultApp is an IClassFixture, so xUnit owns its lifetime and disposes it when the test class finishes. IntegrationContext.Dispose() disposed it anyway — after every test method — and DefaultApp.Dispose() nulled the Host. The guard that papered over that, RecycleIfNecessary(), silently rebuilt the shared host as WolverineHost.Basic(), which carries none of DefaultApp's IncludeType/RegisterMessageType/service registrations. So from the second test method in a class onward, tests could be running against a differently-configured host than the one they were written for, and whether a given test passed came down to ordering. The ownership was also backwards in the other direction: with() builds a host *for the test*, and that one was never disposed at all. Now the test disposes what it built and leaves the fixture alone. This removes the NullReferenceExceptions out of DefaultApp.ChainFor<T>() (a null Host) and fixes the leaked with() hosts. It does NOT close #3423. 19 of the 20 filtered-subset failures survive this change and have a separate cause: running a StorageActionCompliance-derived class before another IntegrationContext class leaves the later host's HandlerGraph unable to handle conventionally-discovered messages (HandlerGraph.CanHandle returns false). The obvious explanation — DisableConventionalDiscovery leaking across hosts — is disproven: a host built after a DisableConventionalDiscovery host discovers handlers normally. Mechanism still unidentified; details on the issue. Full CoreTests assembly stays green (1931/1931). The Persistence subset goes 20 -> 19. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partial fix for #3423. This does not close the issue — see "What this doesn't fix" below. I'd rather land the correct-ownership change on its own merits than bundle it with a cause I haven't pinned yet.
The defect
DefaultAppis an xUnit class fixture — xUnit owns its lifetime and disposes it when the test class finishes.IntegrationContext.Dispose()disposed it anyway, after every test method:DefaultApp.Dispose()nulls the host, and the guard that papered over the resulting damage silently substituted a differently-configured host:WolverineHost.Basic()has none ofDefaultApp'sIncludeType<MessageConsumer>(),IncludeType<InvokedMessageHandler>(),RegisterMessageType(...)calls, or theIAdditionService/IIdentityService/ITrackedTaskRepositoryregistrations. So from the second test method in a class onward, tests could be running against a host they were never written for — and whether any given test passed came down to ordering.The ownership was backwards in the other direction too:
with(...)builds a host for the test, and that one was never disposed at all — a straight leak of a startedIHostper call.The fix
The test disposes what it built; the fixture is left to xUnit.
RecycleIfNecessary()and theHost = null!dance are deleted rather than kept as a band-aid — with the fixture no longer torn down mid-class, there's nothing to recycle. The one caller (find_handlers_with_the_default_handler_discovery) was already a no-op and is removed.What this fixes
NullReferenceExceptions out ofDefaultApp.ChainFor<T>()(a nullHost)with()hostsWhat this doesn't fix
19 of the 20 filtered-subset failures survive this change, with a separate cause. Running a
StorageActionCompliance-derived class before anotherIntegrationContextclass leaves the later host'sHandlerGraphunable to handle conventionally-discovered messages —HandlerGraph.CanHandle(...)returns false, sofindInvokerfalls through to routing and throwsIndeterminateRoutesException.Deterministic repro:
The obvious explanation —
DisableConventionalDiscovery()on the compliance host leaking into hosts built later — is disproven: I built a host withDisableConventionalDiscovery(), disposed it, then built a plain host, and conventional discovery worked fine. Mechanism still unidentified. Written up on #3423 so the next person doesn't re-run the same dead end.Verification
CoreTestsassembly: 1931 passed, 0 failed (unchanged — this is what CI runs)CoreTests.Persistencesubset: 20 → 19 failures🤖 Generated with Claude Code