Skip to content

fix: make startup warnings best-effort through ILoggerFactory - #96

Merged
AGiorgetti merged 2 commits into
developfrom
feat/codex-90-startup-warning-cleanup
Oct 6, 2026
Merged

AGiorgetti merged 2 commits into
developfrom
feat/codex-90-startup-warning-cleanup

Conversation

@AGiorgetti

@AGiorgetti AGiorgetti commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Addresses #90.

Summary

  • Deliver automatic open-generic startup warnings through the root ILoggerFactory, preserving category, event ID and registration metadata without creating a temporary logging scope.
  • Make only optional warning delivery best-effort: factory resolution, logger creation, IsEnabled and Log failures do not prevent returning the provider. Native ownership of any activated dependencies stays with that provider until the caller disposes it.
  • Keep provider construction, validation, strict open-generic rejection and diagnostic exclusions unchanged.
  • Document the approved compatibility change: direct ILogger<ServiceProviderFactory> registrations no longer supply these warnings; use standard ILoggerFactory logging, such as AddLogging. Warning-delivery errors are nonfatal and warnings can be lost.

Verification

  • Test-first: the 52-case regression set produced 26 failures and 26 passes before the fix on each of net8.0, net9.0 and net10.0. Includes native ownership controls.
  • Full local suite: 972/972 pass on each matching .NET 8.0.31, 9.0.20 and 10.0.12 runtime.
  • Native DI 10.0.0 hot-cache regression: 100 interleaved enumerations after observed native compilation pass on each modern runtime.
  • Release solution build passes, including netstandard2.0 library and net472 test compilation. Two existing net472 package-support warnings remain.
  • Regression coverage includes standard AddLogging, keyed/unkeyed warning metadata, unactivated custom scoped loggers, missing logger factories, each warning failure stage, sync/async/dual owned dependencies, caller-owned instances, exactly-once disposal and unchanged strict/validation controls.
  • Independent review found no correctness or scope blockers; targeted independent net10 run passed 79/79 affected tests.
  • Exact-head Windows CI passed for ff643d70f02ca4d0b564c333de5e3bdb6dad2ace: 972/972 tests on each of net472, net8.0, net9.0 and net10.0, plus all four native hot-cache probes. Successful run.
  • The first Windows run exposed one net472-only assertion mismatch in AggregateException.Message; a test-only follow-up now inspects the full exception. The production fix is unchanged.

Prepared with OpenAI Codex.

Use root ILoggerFactory logging without a temporary scope and make optional warning delivery best-effort. Keep provider ownership with the caller, including when logging activation fails, and document the direct ILogger compatibility change.

Co-authored-by: Codex <codex@openai.com>

Copy link
Copy Markdown
Contributor Author

Implementation and local verification are complete for head f8bfbc883d4d9a9e17b2091a37c7d2958998d5ab.

  • The production change removes the temporary logging scope and limits the catch to optional warning delivery through the root ILoggerFactory.
  • The approved behavior was tested first: 26 failing / 26 passing cases before the change on each modern runtime, then all 52 cases pass.
  • Exact published-head Release build and full tests pass: 972/972 on each of net8.0, net9.0 and net10.0. Native DI 10.0.0 hot-cache probes also pass on all three.
  • Independent review found no correctness or scope blockers. The direct-ILogger compatibility change and nonfatal warning policy are documented in README, XML comments and the unreleased changelog.

Windows CI (including net472 execution) is still being checked; the PR remains draft.

Prepared with OpenAI Codex.

AggregateException.Message does not include inner messages on .NET Framework. Assert against the full exception, as the existing validation regression tests do.

Co-authored-by: Codex <codex@openai.com>

Copy link
Copy Markdown
Contributor Author

The first Windows run passed all 972 tests on net8.0, net9.0 and net10.0. net472 passed 971/972; its sole failure was a new validation assertion relying on AggregateException.Message to include inner-exception details.

Head ff643d70f02ca4d0b564c333de5e3bdb6dad2ace changes only that assertion to inspect error.ToString(), matching existing validation tests. Independent review confirmed the test-only correction. The full modern suites remain green locally; exact-head Windows CI is being rerun. Production code is unchanged.

Prepared with OpenAI Codex.

Copy link
Copy Markdown
Contributor Author

Final verification is green for exact head ff643d70f02ca4d0b564c333de5e3bdb6dad2ace.

Windows CI run 37481613894 passed:

  • Release build
  • 972/972 tests on each of net472, net8.0, net9.0 and net10.0
  • Native DI 10.0.0 hot-cache regression on all four targets, including 100 interleaved enumerations after observed native compilation

The workflow tested this head merged into current develop e135918f06c2d0f645076506c005f471d3a9d892 (merge ref 8091fb1c9d8a234e006cdbc80b615535d02a6b4c). Local exact-head modern suites and hot-cache checks also pass. Independent review found no correctness or scope blockers, including the net472 assertion correction.

Ready for review; leaving this PR in draft for the agreed review/closure gate.

Prepared with OpenAI Codex.

@AGiorgetti
AGiorgetti marked this pull request as ready for review October 6, 2026 15:19
@AGiorgetti
AGiorgetti merged commit 7165c72 into develop Oct 6, 2026
2 checks passed
@AGiorgetti
AGiorgetti deleted the feat/codex-90-startup-warning-cleanup branch October 6, 2026 15:22
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.

1 participant