Repository navigation
Hand the test logger a resolved config, not the container it outlives - #716
Merged
Merged
Conversation
Teardown logs AFTER the DI container is disposed. XUnitFileLogger held the
IServiceProvider and resolved IConfiguration on first log per category, so
anything logging from a Dispose path reached Autofac's already-disposed
LifetimeScope and threw.
Measured on Acme.Test (native xUnit host, the way CI runs it): 23 first-chance
ObjectDisposedExceptions per run, every one this chain —
MonolithMeshTestBase.DisposeAsync -> AsyncDisposeQueue.DrainAsync
-> CompilationCacheService.Dispose (logs during teardown)
-> XUnitFileLogger.GetMinLogLevel -> GetService<IConfiguration>
-> disposed LifetimeScope -> throw
They were caught (GetMinLogLevel swallows and defaults to Information), so they
never failed a test. They burned 23 throws plus 23 first-chance stack captures
on the teardown path and buried teardown-stragglers.log in fault-shaped noise
that reads exactly like a real defect — which is worth removing on its own, and
doubly so while that log is the diagnostic for the #613 teardown crashes.
The provider now resolves the configuration OBJECT once, in its constructor,
while the container is guaranteed alive, and hands that object to every logger.
IConfiguration is a plain object once resolved, so the reference stays valid
after the scope that produced it is gone — this removes the post-disposal
container access rather than guarding it.
Verified: Acme.Test 76/76 exit 0 via the native host, stragglers 23 -> 0.
NOT claimed: that this fixes the CI exit=2. Those exceptions were caught, and
exit=2 has never reproduced locally even with CI's exact invocation. AI.Test's
19 stragglers are a DIFFERENT chain (post-dispose hub construction, #613) and
are untouched by this.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes teardown-time logging noise in the test infrastructure by ensuring XUnitFileLogger no longer resolves configuration via a DI container that may already be disposed during test teardown.
Changes:
- Resolve and store an
IConfigurationinstance once inXUnitFileLoggerProvider(while the container is alive) instead of storingIServiceProvider. - Pass the resolved
IConfigurationto eachXUnitFileLogger, removing post-disposal container access inGetMinLogLevel.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
From the Copilot review, and correct per this repo's own no-swallow rule: the provider constructor's catch-all could mask a real DI failure (misconfigured container, faulting IConfiguration factory) as a silent log-level downgrade. The stated intent is only to tolerate a provider built from an already-disposed scope, so catch ObjectDisposedException specifically and let everything else surface loudly. Acme.Test 76/76; Fixture builds clean under -c Release -warnaserror. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Copilot's finding taken in |
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.
Salvages the third commit of #708 (
99577caba), which landed on that branch ~2 minutes after the PR was merged and therefore never reachedmain.The defect
Teardown logs AFTER the DI container is disposed.
XUnitFileLoggerheld theIServiceProviderand resolvedIConfigurationon first log per category, so anything logging from aDisposepath threw:Measured on
Acme.Testvia the native xUnit host (the way CI runs it): 23 first-chanceObjectDisposedExceptions per run, every one this chain. They were caught (GetMinLogLevelswallows and defaults toInformation), so they never failed a test — they burned 23 throws + 23 first-chance stack captures on the teardown path and buriedteardown-stragglers.login fault-shaped noise. That log is the working diagnostic for the #613 teardown crashes (it is how the #715 hub-construction chain was found), so keeping it signal-only has direct value.The fix
XUnitFileLoggerProviderresolves the configuration object once, in its constructor, while the container is guaranteed alive, and hands that object to every logger.IConfigurationis a plain object once resolved — the reference stays valid after the scope that produced it is gone — so this removes the post-disposal container access rather than guarding it.Verified:
Acme.Test76/76 exit 0 via the native host, stragglers 23 → 0;MeshWeaver.Fixturebuilds clean under-c Release -warnaserroron current main.Explicitly NOT claimed: that this fixes the transient CI
exit=2(those exceptions were caught; attempt 2 of the same commit ran clean with no code change).No What's New entry: test-infrastructure only.
🤖 Generated with Claude Code