refactor: comprehensive NRT null-safety audit — eliminate null! suppressions, fix latent bugs, add nullable attributes by niemyjski · Pull Request #484 · FoundatioFx/Foundatio

niemyjski · 2026-03-28T20:31:13Z

Summary

Systematic audit and remediation of all nullable reference type (NRT) patterns across the Foundatio core library. This eliminates null! suppressions, corrects nullable annotations on public APIs to reflect actual runtime behavior, adds [NotNullWhen]/[NotNullIfNotNull]/[MemberNotNull]/[MaybeNull] attributes, marks DTO properties as required where appropriate, and fixes 5 latent bugs uncovered during the audit.

Scope: 203 files changed, ~1600 insertions, ~980 deletions across src/Foundatio/, src/Foundatio.Extensions.Hosting/, src/Foundatio.TestHarness/, src/Foundatio.Xunit*/, serializer projects, and test projects.

Validation: 0 build errors, 0 new warnings, all 1796 tests pass (0 failures, 13 skipped).

Bugs Uncovered and Fixed

1. `FoundatioServicesExtensions.UseInMemory()`/`UseFolder()` — NullReferenceException on null options

Before: options = null! as default parameter, then options.UseServices(sp) called directly — a NullReferenceException at service resolution time when callers passed no options (the common case).

After: Parameter changed to options? = null, body uses (options ?? new()).UseServices(sp).

Impact: Every UseInMemory() and UseFolder() call without explicit options would crash at DI resolution time. This was masked because the null! default made the compiler trust the value was non-null while the runtime happily passed null through.

Files: src/Foundatio/FoundatioServicesExtensions.cs

2. `SharedOptions.UseServices()` — Silent overwrite of user-configured defaults with null

Before: options.ResiliencePolicyProvider = serviceProvider.GetService<IResiliencePolicyProvider>() — if no IResiliencePolicyProvider was registered in DI, this would overwrite a user-provided value with null.

After: Each service is resolved into a local variable; only assigned to the options property if non-null.

Impact: Users who configured a ResiliencePolicyProvider, TimeProvider, Serializer, or LoggerFactory directly on options could have their configuration silently discarded when UseServices() was called and DI had no registration for that service type.

Files: src/Foundatio/Utility/SharedOptions.cs

3. `CacheLockProvider` — Null `_resiliencePolicyProvider` before immediate `.GetPolicy()` call

Before: _resiliencePolicyProvider = resiliencePolicyProvider ?? cacheClient.GetResiliencePolicyProvider() — if both were null, _resiliencePolicyProvider was null, and the very next line called _resiliencePolicyProvider.GetPolicy(...), causing a NullReferenceException.

After: Falls back to DefaultResiliencePolicyProvider.Instance as final default.

Impact: Any CacheLockProvider created without an explicit IResiliencePolicyProvider and with a cache client that also had no provider would crash in the constructor.

Files: src/Foundatio/Lock/CacheLockProvider.cs

4. `HybridCacheClient` — Wrong logger factory passed to local cache options

Before: LoggerFactory = loggerFactory (the raw constructor parameter, which could be null).

After: LoggerFactory = _loggerFactory (the resolved, non-null field that already fell back to NullLoggerFactory.Instance).

Impact: When loggerFactory parameter was null, the InMemoryCacheClientOptions would get null even though a proper default was available, potentially causing downstream NREs or losing log output from the local cache layer.

Files: src/Foundatio/Caching/HybridCacheClient.cs

5. `CacheLockProvider` DI registration — `GetService` for required `IMessageBus`

Before: sp.GetService<IMessageBus>() with comment "optional for more efficient lock release notifications" — but CacheLockProvider's constructor requires non-null IMessageBus and immediately subscribes to it.

After: sp.GetRequiredService<IMessageBus>() — correctly fails fast if no message bus is registered.

Impact: Without a registered IMessageBus, the lock provider would silently receive null and throw a NullReferenceException during construction, producing a confusing error instead of a clear DI resolution failure.

Files: src/Foundatio/FoundatioServicesExtensions.cs

Feedback-Driven Improvements

The following improvements were made based on PR review feedback:

Argument validation ordering — All ArgumentNullException.ThrowIfNull/ArgumentException.ThrowIfNullOrEmpty calls now come first in method bodies, with a blank line after the validation block
ArgumentNullException.ThrowIfNull adoption — Replaced manual null checks (throw new NullReferenceException) with ThrowIfNull in AsyncEvent, ActionableStream, ScopedLockProvider, and ConcurrentDictionaryExtensions
MessageBusException usage — Message<T>.Body getter now throws MessageBusException (domain-specific) instead of InvalidOperationException with a safe cast
LogError for Activator.CreateInstance failure — Upgraded from LogWarning in MessageBusBase since failing to create a typed message wrapper is a real error
InMemoryQueue deep clone restored — Enqueue-time data.DeepClone() was accidentally removed; restored to preserve caller-mutation isolation for retry data
IFileStorage GetObjectDataAsync simplified — Removed redundant result is T typed ? typed : default! check since Deserialize<T> already returns the correct type
DataDictionary TryGetData<T> annotated — Added [MaybeNull] on out T value parameter for correct NRT handling of unconstrained generic
InMemoryCacheClient CalculateEntrySize — Moved null check before try block as reviewer suggested
ScheduledTimer constructor — Reordered to put argument validation before field assignments
String.Equals usage — Replaced == with String.Equals(a, b) for consistency

Breaking Changes — Public API Surface

All changes below are intentional for the new major version. Each one corrects a nullable annotation to match actual runtime behavior.

Interface Return Types Made Nullable

Interface	Method	Before	After	Reason
`IQueue<T>`	`EnqueueAsync`	`Task<string>`	`Task<string?>`	Can return null on failure
`IQueue<T>`	`DequeueAsync`	`Task<IQueueEntry<T>>`	`Task<IQueueEntry<T>?>`	Returns null when queue is empty or timeout expires
`ILockProvider`	`AcquireAsync`	`Task<ILock>`	`Task<ILock?>`	Returns null when lock cannot be acquired
`IFileStorage`	`GetFileStreamAsync`	`Task<Stream>`	`Task<Stream?>`	Returns null when file does not exist
`IFileStorage`	`GetFileInfoAsync`	`Task<FileSpec>`	`Task<FileSpec?>`	Returns null when file does not exist
`IMessage`	`GetBody()`	`object`	`object?`	Returns null when data is empty or deserialization fails

Interface Properties Made Nullable

Interface	Property	Before	After	Reason
`IMessage`	`UniqueId`	`string`	`string?`	Optional metadata, not always set
`IMessage`	`CorrelationId`	`string`	`string?`	Optional distributed tracing ID
`IMessage`	`ClrType`	`Type`	`Type?`	Null when type cannot be resolved
`IQueueEntry<T>`	`CorrelationId`	`string`	`string?`	Optional, not always provided
`IQueueEntry<T>`	`EntryType`	`Type`	`Type?`	Null when Value is null
`IJobWithOptions`	`Options`	`JobOptions`	`JobOptions?`	Not set until job runner configures it

Extension Method Return Types Made Nullable

Class	Method	Before	After	Reason
`IFileStorageExtensions`	`GetFileContentsAsync`	`Task<string>`	`Task<string?>`	Null when file not found
`IFileStorageExtensions`	`GetFileContentsRawAsync`	`Task<byte[]>`	`Task<byte[]?>`	Null when file not found

Abstract/Virtual Method Signatures Changed

Class	Method	Before	After	Reason
`JobWithLockBase`	`GetLockAsync`	`Task<ILock>`	`Task<ILock?>`	Subclasses return null when lock unavailable
`QueueJobBase<T>`	`GetQueueEntryLockAsync`	`Task<ILock>`	`Task<ILock?>`	Default returns empty lock, but subclasses may return null
`QueueJobBase<T>`	`ProcessAsync`	`IQueueEntry<T>` param	`IQueueEntry<T>` param (non-nullable)	Null checks moved to `RunAsync` where nullable originates
`QueueBase<T>`	`EnqueueImplAsync`	Returns `Task<string>`	Returns `Task<string?>`	Implementations may return null
`QueueBase<T>`	`DequeueImplAsync`	Returns `Task<IQueueEntry<T>>`	Returns `Task<IQueueEntry<T>?>`	Returns null on empty queue

DTO Properties Made `required`

These properties are always set during construction and must be non-null. The required keyword enforces this at compile time (and via System.Text.Json at deserialization time).

Class	Properties	Reason
`WorkItemData`	`WorkItemId`, `Type`, `Data`	Core identity fields, always set by `EnqueueAsync`
`Message`	`Type`	Message routing field, always set during publish
`Subscriber` (internal)	`Type`, `Action`	Core subscription data, always set during subscribe
`CacheLockReleased`	`Resource`	Lock identity, always set on release
`InvalidateCache`	`CacheId`	Cache instance identity, always set
`NextPageResult`	`Files`	Pagination result, always non-null
`FileSpec`	`Path`	File identity, always set
`ResetEventWithRefCount` (private)	`Target`	Always set inline

DTO Properties Made Nullable

Class	Properties	Reason
`WorkItemData`	`UniqueIdentifier`, `SubMetricName`	Optional metadata, frequently null
`CacheLockReleased`	`LockId`	Can be null for broadcast releases
`InvalidateCache`	`Keys`	Null means flush-all
`FolderFileStorageOptions`	`Folder`	Cannot use `required` (SharedOptions uses `new()` constraint); validated in `FolderFileStorage` constructor

Utility Method Signatures Changed

Class	Method	Change	Reason
`PathHelper`	`ExpandPath`	Param `string` → `string?`, added `[NotNullIfNotNull]`	Callers pass nullable paths; attribute eliminates downstream `!`
`PathHelper`	`GetDataDirectory`	Returns `string?` (was `string`)	`AppDomain.CurrentDomain.GetData` can return null
`TimeUnit`	`TryParse`	Added `[NotNullWhen(true)]` on out param	Standard TryParse pattern for null-safety

Constructor Parameter Annotations

All constructor parameters for optional infrastructure services (ILoggerFactory, TimeProvider, IResiliencePolicyProvider) across the following classes changed from non-nullable with = null! to properly nullable with = null:

JobWithLockBase
QueueJobBase<T>
CacheLockProvider
HybridCacheClient
MaintenanceBase

These always had runtime null checks with fallback defaults; the annotations now match.

Nullable Attributes Added

File	Attribute	Purpose
`PathHelper.ExpandPath`	`[NotNullIfNotNull(nameof(path))]`	Compiler knows: non-null input → non-null output. Eliminates 3 `!` suppressions at call sites.
`TimeUnit.TryParse`	`[NotNullWhen(true)]` on `out TimeSpan? time`	Standard TryParse pattern — compiler knows `time` is non-null when method returns `true`.
`QueueBehaviorBase<T>.Attach`	`[MemberNotNull(nameof(_queue))]`	Guarantees `_queue` is set after `Attach()` completes. Allows keeping `= null!` for late-init field.
`StartupActionsContext.MarkStartupComplete`	`[MemberNotNull(nameof(Result))]`	Guarantees `Result` is set after method completes. Added `ArgumentNullException.ThrowIfNull(result)` guard.
`DataDictionary.TryGetData<T>`	`[MaybeNull]` on `out T value`	Unconstrained generic out parameter can be null when key not found or conversion fails.

ISerializer Deserialization Pattern

The Deserialize<T> extension methods now use a consistent three-way pattern:

var result = serializer.Deserialize(stream, typeof(T));
if (result is null)
    return default;       // null is valid deserialization result
if (result is T typed)
    return typed;         // happy path
throw new InvalidCastException(...);  // serializer returned wrong type -- fail loud

This replaces the previous (T)result hard cast which would throw InvalidCastException for null-to-value-type, masking a valid null deserialization result.

Developer Experience (DX) Notes

What this PR improves for consumers

No more surprise NREs from APIs that lie about nullability — DequeueAsync, AcquireAsync, GetFileInfoAsync, and GetBody() all returned non-null types but could return null at runtime. Consumers had to know to check for null despite the type system saying otherwise. Now the types are honest.
required properties catch missing fields at compile time — WorkItemData.WorkItemId, Message.Type, FileSpec.Path, etc. were always expected to be set but the compiler couldn't enforce it. Now forgetting to set them is a compile error.
[NotNullIfNotNull] on PathHelper.ExpandPath — Previously every caller had to write PathHelper.ExpandPath(path)! because the method accepted and returned non-nullable string but callers had nullable inputs. Now passing string? works cleanly and the compiler tracks nullability through.
Options parameters are honestly nullable — UseInMemory(InMemoryCacheClientOptions options = null) previously used null! to trick the compiler. Now properly typed as options? = null with ?? new() fallback, so IDE tooling correctly shows these as optional.

Test improvements

All test ! suppressions on GetData<T>()!, Deserialize<T>()!, GetFileContentsAsync()! replaced with explicit Assert.NotNull() — tests now fail with clear assertion messages instead of NullReferenceExceptions.
Test model properties (SimpleMessage.Data, SimpleWorkItem.*, SimpleModel.*, CloneModel.*, etc.) made nullable or required to match actual usage.

Remaining justified `null!` usages

A small number of = null! patterns remain where they are genuinely correct:

BenchmarkDotNet [GlobalSetup] lifecycle — fields assigned in setup, not constructor
Test lifecycle IDisposable — fields nulled in dispose, recreated in constructor
IHaveSerializer interface match — interface requires non-null ISerializer, impl uses = null! since it's always set externally
QueueBehaviorBase._queue — late-init via Attach(), guarded by [MemberNotNull]

Impact on Provider Repositories

The following downstream repositories will need minor updates for the interface changes (primarily adding ? to return types in their implementations):

Foundatio.Redis — Queue, Storage, Lock implementations
Foundatio.AzureStorage — Storage, Queue implementations
Foundatio.AzureServiceBus — Queue implementations
Foundatio.AWS — Queue, Storage implementations
Foundatio.Aliyun — Storage implementations
Foundatio.Minio — Storage implementations
Foundatio.Storage.SshNet — Storage implementations
Foundatio.Kafka — Messaging implementations
Foundatio.RabbitMQ — Messaging implementations

These are mechanical signature changes (adding ? to match the updated interface return types) and should be straightforward.

Systematically audit and fix all null! suppressions, nullable annotations, and null-safety patterns across the entire Foundatio core library. BREAKING CHANGES: - IQueue<T>.EnqueueAsync returns Task<string?> (was Task<string>) - IQueue<T>.DequeueAsync returns Task<IQueueEntry<T>?> (was Task<IQueueEntry<T>>) - ILockProvider.AcquireAsync returns Task<ILock?> (was Task<ILock>) - JobWithLockBase.GetLockAsync returns Task<ILock?> (was Task<ILock>) - QueueJobBase.GetQueueEntryLockAsync returns Task<ILock?> (was Task<ILock>) - IJobWithOptions.Options is now JobOptions? (was JobOptions) - IMessage.GetBody() returns object? (was object) - IMessage.UniqueId, CorrelationId, ClrType are now nullable - IQueueEntry<T>.CorrelationId, EntryType are now nullable - WorkItemData: WorkItemId, Type, Data are now required - Message.Type is now required - Subscriber.Type, Subscriber.Action are now required - CacheLockReleased.Resource is now required - InvalidateCache.CacheId is now required - NextPageResult.Files is now required - FileSpec.Path is now required - ResetEventWithRefCount.Target is now required - IFileStorage.GetFileStreamAsync returns Task<Stream?> (was Task<Stream>) - IFileStorage.GetFileInfoAsync returns Task<FileSpec?> (was Task<FileSpec>) - IFileStorage extension GetFileContentsAsync returns Task<string?> (was Task<string>) - IFileStorage extension GetFileContentsRawAsync returns Task<byte[]?> (was Task<byte[]>) - PathHelper.ExpandPath parameter changed to string? with [NotNullIfNotNull] - PathHelper.GetDataDirectory returns string? (was string) Bug fixes uncovered: - FoundatioServicesExtensions UseInMemory/UseFolder methods would NRE when called with null options (options.UseServices(sp) on null ref) - SharedOptions.UseServices would overwrite explicit user-configured defaults with null when DI container had no registration for a service - CacheLockProvider constructor could leave _resiliencePolicyProvider null then immediately call .GetPolicy() on it - HybridCacheClient passed raw loggerFactory param instead of resolved _loggerFactory to InMemoryCacheClientOptions - CacheLockProvider was registered with GetService<IMessageBus> (optional) but constructor requires non-null IMessageBus Nullable attributes added: - PathHelper.ExpandPath: [NotNullIfNotNull(nameof(path))] - TimeUnit.TryParse: [NotNullWhen(true)] on out parameter - QueueBehaviorBase.Attach: [MemberNotNull(nameof(_queue))] - StartupActionsContext.MarkStartupComplete: [MemberNotNull(nameof(Result))]

tests/Foundatio.Tests/Utility/ResiliencePolicyTests.cs

src/Foundatio/Jobs/JobRunner.cs

src/Foundatio/Storage/InMemoryFileStorage.cs

src/Foundatio.TestHarness/Storage/FileStorageTestsBase.cs

src/Foundatio.TestHarness/Utility/Configuration.cs

src/Foundatio/Resilience/ResiliencePolicy.cs

Copilot

Pull request overview

This PR performs a repo-wide nullable reference types (NRT) audit across Foundatio, updating public API nullability to match runtime behavior, removing many null-suppression patterns, and adding nullable flow attributes/required members to improve correctness and consumer experience for the next major version.

Changes:

Corrects nullability across core abstractions (queues, locks, messaging, storage, jobs, serializers) and updates implementations accordingly.
Adds nullable-flow attributes (e.g., [NotNullWhen], [NotNullIfNotNull], [MemberNotNull]) and introduces required on DTOs where values are mandatory.
Fixes several latent runtime bugs uncovered during the audit (options handling, DI GetRequiredService, resilience provider fallbacks, logger factory wiring).

Reviewed changes

Copilot reviewed 187 out of 187 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/Foundatio/Utility/DataDictionary.cs	Updates nullability annotations and adds nullable attributes for data access helpers.
src/Foundatio/Jobs/QueueJobBase.cs	Makes dequeue/process paths accept nullable queue entries and updates activity instrumentation.
src/Foundatio/Jobs/WorkItemJob/WorkItemJob.cs	Updates nullable handling for queue entry processing and activity creation.
tests/Foundatio.Tests/Utility/ResiliencePolicyTests.cs	Adjusts tests for nullable `CircuitBreaker` and policy access.
tests/Foundatio.Tests/Utility/SizeCalculatorTests.cs	Updates tests for nullable scenarios around sizing calculations.
build/common.props	Enables nullable across the build and tightens warning configuration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/Foundatio.Tests/Utility/ResiliencePolicyTests.cs

tests/Foundatio.Tests/Utility/SizeCalculatorTests.cs

src/Foundatio/Jobs/WorkItemJob/WorkItemJob.cs

src/Foundatio/Utility/DataDictionary.cs

src/Foundatio/Jobs/QueueJobBase.cs

…and fix resource disposal - QueueJobBase: use null-conditional on EntryType?.FullName and EntryType?.Name - ResiliencePolicyTests: add Assert.NotNull for CircuitBreaker, remove ! suppressions - SizeCalculator: accept object? parameter to allow null without suppression - DataDictionary: remove null! on serializer/ToType, return false when conversion yields null - FileStorageTestsBase: wrap StreamReader in using statement - Configuration: add null guard for Path.GetDirectoryName, use Path.Combine for parent dirs - WorkItemJob: assign traceState directly (already string?) instead of .ToString() - JobRunner: store FileSystemWatcher in static field to prevent undisposed local

tests/Foundatio.Tests/Utility/ResiliencePolicyTests.cs

src/Foundatio.TestHarness/Utility/Configuration.cs

src/Foundatio/Resilience/ResiliencePolicy.cs

…stemWatcher disposal - CacheClientTestsBase + 5 overrides: make test params nullable (string?) to fix xUnit1012 - ScopedFolderFileStorageTests: make scope param nullable to fix xUnit1012 - WithLockingJob, ThrottledJob, SampleQueueJob: remove unnecessary async/await (AsyncFixer01) - JobRunner: register Token callback to dispose FileSystemWatcher on shutdown Remaining 32 warnings are all in vendored FastCloner code (NRT) and external assembly strong name references (CS8002) — neither fixable without risk.

…ions Replace manual try/finally CTS disposal pattern with using var declarations in all 8 Execute/ExecuteAsync methods. The using declarations ensure deterministic disposal at method exit without the boilerplate try/finally blocks.

Copilot

Pull request overview

Copilot reviewed 191 out of 191 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio/Jobs/JobWithLockBase.cs

src/Foundatio.DataProtection/FoundatioStorageXmlRepository.cs

…fety - Fix operator precedence in JobWithLockBase activity name: wrap null-coalescing in parentheses so _jobName is used as fallback - Fix FoundatioStorageXmlRepository: make resiliencePolicyProvider nullable, fallback to default ResiliencePolicy instead of passing null!

…o RunAsync - Revert IQueueJob<T>.ProcessAsync to non-nullable parameter - Move null handling from ProcessAsync to RunAsync in QueueJobBase and WorkItemJob - Replace _resiliencePolicyProvider! with ?? DefaultResiliencePolicyProvider.Instance - Remove unnecessary null-conditional operators in InMemoryQueue.EnqueueImplAsync - Remove null-forgiving on QueueEntry value.DeepClone() - Replace options.Name! with options.Name ?? string.Empty in IJob and JobRunner

src/Foundatio/Queues/InMemoryQueue.cs

…NRE risks - InMemoryCacheClient: use EqualityComparer<T>.Default.Equals instead of currentValue!.Equals(expected) to prevent NRE when cached value is null - IWorkItemHandler.GetWorkItemLockAsync: return Task<ILock?> to match actual usage (callers null-check, custom impls may return null) - FolderFileStorage.GetFileStreamAsync: return null for missing files in read mode instead of throwing FileNotFoundException (matches interface) - DataDictionary.GetDataOrDefault: remove unnecessary serializer! since ToType<T> already accepts nullable ISerializer - DelegateWorkItemHandler: remove dead _handler null check (field is non-nullable) - MessageBusBase: fix options?.LoggerFactory -> options.LoggerFactory since options is guaranteed non-null by preceding null-throw guard

Copilot

Pull request overview

Copilot reviewed 190 out of 190 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio/Utility/TypeHelper.cs

src/Foundatio.Xunit/Logging/TestLoggerOptions.cs

src/Foundatio.Xunit.v3/Logging/TestLoggerOptions.cs

src/Foundatio/Messaging/InMemoryMessageBus.cs

src/Foundatio/Jobs/WorkItemJob/WorkItemJob.cs

src/Foundatio/Messaging/MessageBusBase.cs

src/Foundatio/Jobs/WorkItemJob/WorkItemJob.cs

src/Foundatio/Jobs/QueueJobBase.cs

docs/guide/nullable-reference-types.md

Copilot

Pull request overview

Copilot reviewed 190 out of 190 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio/Caching/InMemoryCacheClientOptions.cs

docs/guide/nullable-reference-types.md

src/Foundatio.DataProtection/FoundatioStorageXmlRepository.cs

src/Foundatio/Storage/IFileStorage.cs

Copilot

Pull request overview

Copilot reviewed 203 out of 203 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/Foundatio/Queues/QueueEntry.cs:24

QueueEntry now guards value, but queue is still not validated even though it is dereferenced immediately (_queue.GetTimeProvider()), which would yield a NullReferenceException if a caller passes null at runtime. Consider adding ArgumentNullException.ThrowIfNull(queue) (and optionally validating id) to keep argument validation consistent and fail fast with a clear exception type.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio/Storage/ScopedFileStorage.cs

src/Foundatio/Jobs/WorkItemJob/WorkItemJob.cs

Copilot

Pull request overview

Copilot reviewed 203 out of 203 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio/Serializer/ISerializer.cs

src/Foundatio/Caching/CacheValue.cs

- Use ArgumentException.ThrowIfNullOrEmpty for path validation - Add [return: MaybeNull] and XML documentation to GetObjectAsync dotnet/roslyn#30953

Copilot

Pull request overview

Copilot reviewed 203 out of 203 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio.Xunit/Logging/TestLogger.cs

src/Foundatio.Xunit.v3/Logging/TestLogger.cs

src/Foundatio/FoundatioServicesExtensions.cs

Copilot

Pull request overview

Copilot reviewed 204 out of 204 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio/Utility/MaintenanceBase.cs

niemyjski · 2026-04-09T18:23:56Z

PR #484 NRT Review — Final Resolution Summary

Build & Test Verification

Foundatio.slnx build: ✅ 0 warnings, 0 errors
Foundatio.All.slnx build (full workspace including Repositories, Redis, Parsers, AWS, Azure, Kafka, etc.): ✅ 0 warnings, 0 errors
Tests: ✅ 1,837 passed, 0 failed, 13 skipped (all skips are pre-existing/expected)

Code Fixes Applied This Review

TestLogger constructors (xunit + xunit.v3): Added ArgumentNullException.ThrowIfNull(options) — the old ?? new TestLoggerOptions() silently masked null arguments, which is inconsistent with NRT guarantees.

Comment Resolution Summary

Category A — Already Fixed by PR Author (~13 comments)

All confirmed as fixed in prior commits. Responses posted acknowledging the fixes.

Finding	Status
`UseInMemory()`/`UseFolder()` NRE (Copilot)	✅ Fixed — `SharedOptions.UseServices()` handles null
`SharedOptions.UseServices()` silent overwrite	✅ Fixed — `Has*` flags prevent overwriting explicit values
`CacheLockProvider` null policy provider	✅ Fixed — constructor validates non-null cache client
`HybridCacheClient` wrong logger factory	✅ Fixed — passes correct `ILoggerFactory`
`ConcurrentDictionary` null-value caching bug	✅ Fixed — defensive null checks in `WorkItemJob`/`MessageBusBase`
`CacheLockProvider` DI `GetService` → `GetRequiredService` for `ICacheClient`	✅ Fixed

Category B — Design Decisions (Responses Posted)

Topic	Resolution
`ISerializer.Deserialize<T>` return type	Standard .NET pattern: `[return: MaybeNull]` + `result is T typed ? typed : default`. Consistent across all 3 overloads.
`IFileStorage.GetObjectAsync<T>` nullability	Known Roslyn limitation (dotnet/roslyn#30953). `[return: MaybeNull]` not honored in async methods. Documented in XML comments.
`CacheValue<T>.Value` semantics	Intentional `Nullable<T>.Value` semantics — accessing when `HasValue` is false is a contract violation.
`SharedOptions` nullable handling	Correctly uses `Has*` flags; `TOption?` parameter is intentional.
`CacheLockProvider` `GetService` for `IMessageBus`	Correct — `IMessageBus` is genuinely optional (constructor accepts `IMessageBus?`).
`ScopedFileStorage` empty scope	Intentional — empty/whitespace scope means "no scoping".
`MaintenanceBase.ScheduleNextMaintenance` null timer	Defensive guard with warning log; throwing would crash the maintenance loop.
`FastCloner` vendored code	Only `#nullable enable annotations` / `#nullable disable warnings` added; original patterns preserved.

Category C — CodeQL Findings (All Addressed)

Finding	Files	Resolution
Generic catch clause	`WorkItemJob.cs`	Intentional — user-supplied handlers can throw anything; catch logs + marks work item as failed
Missed `using` opportunity	`ResiliencePolicy.cs`	False positive — `CancellationTokenSource` is conditionally created; `finally` block disposal is functionally equivalent
`Path.Combine` may drop arguments	`Configuration.cs`	False positive — paths are relative, not rooted
Dereferenced variable may be null (`policy`)	`ResiliencePolicyTests.cs`	False positive — `Assert.NotNull(policy)` precedes dereference
Dereferenced variable may be null (`options`)	`InMemoryQueue.cs`	False positive — base class `QueueBase.EnqueueAsync` ensures non-null before calling impl
Missing Dispose (`FileSystemWatcher`)	`JobRunner.cs`	Pre-existing; watcher lifetime is tied to the job runner
Missing Dispose (`MemoryStream`)	`InMemoryFileStorage.cs`	Pre-existing; stream ownership transfers to caller
Missing Dispose (`StreamReader`)	`FileStorageTestsBase.cs`	Pre-existing; test code

Verdict

All feedback from niemyjski, Copilot, and CodeQL has been reviewed and responded to. The PR is in good shape — NRT annotations are correct, latent bugs have been fixed, and no regressions were introduced. The workspace builds cleanly with zero warnings across all Foundatio repositories.

niemyjski

Self reviewed

Copilot

Pull request overview

Copilot reviewed 204 out of 204 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Foundatio/Lock/ThrottlingLockProvider.cs

src/Foundatio/Lock/CacheLockProvider.cs

github-code-quality bot found potential problems Mar 28, 2026

View reviewed changes

Merge origin/main into nrt/comprehensive-null-safety-audit

36ade7b

niemyjski self-assigned this Mar 28, 2026

niemyjski added the enhancement label Mar 28, 2026

niemyjski requested a review from Copilot March 28, 2026 23:17

Copilot started reviewing on behalf of niemyjski March 28, 2026 23:18 View session

Copilot AI reviewed Mar 28, 2026

View reviewed changes

github-code-quality bot found potential problems Mar 30, 2026

View reviewed changes

niemyjski added 2 commits March 29, 2026 19:58

niemyjski requested a review from Copilot March 30, 2026 01:22

Copilot started reviewing on behalf of niemyjski March 30, 2026 01:22 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

src/Foundatio/Jobs/JobWithLockBase.cs Show resolved Hide resolved

src/Foundatio.DataProtection/FoundatioStorageXmlRepository.cs Outdated Show resolved Hide resolved

src/Foundatio.DataProtection/FoundatioStorageXmlRepository.cs Outdated Show resolved Hide resolved

niemyjski added 2 commits March 29, 2026 20:30

github-code-quality bot found potential problems Mar 30, 2026

View reviewed changes

src/Foundatio/Queues/InMemoryQueue.cs Fixed Show fixed Hide fixed

niemyjski requested a review from Copilot April 8, 2026 15:58

Copilot started reviewing on behalf of niemyjski April 8, 2026 15:59 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

niemyjski requested a review from Copilot April 8, 2026 17:38

Copilot started reviewing on behalf of niemyjski April 8, 2026 17:39 View session

niemyjski commented Apr 8, 2026

View reviewed changes

docs/guide/nullable-reference-types.md Outdated Show resolved Hide resolved

niemyjski commented Apr 8, 2026

View reviewed changes

docs/guide/nullable-reference-types.md Outdated Show resolved Hide resolved

Copilot AI reviewed Apr 8, 2026

View reviewed changes

src/Foundatio/Caching/InMemoryCacheClientOptions.cs Show resolved Hide resolved

niemyjski commented Apr 8, 2026

View reviewed changes

docs/guide/nullable-reference-types.md Outdated Show resolved Hide resolved

niemyjski commented Apr 8, 2026

View reviewed changes

docs/guide/nullable-reference-types.md Show resolved Hide resolved

niemyjski commented Apr 8, 2026

View reviewed changes

src/Foundatio.DataProtection/FoundatioStorageXmlRepository.cs Outdated Show resolved Hide resolved

PR Feedback

806f94e

niemyjski requested a review from Copilot April 9, 2026 13:39

Copilot started reviewing on behalf of niemyjski April 9, 2026 13:39 View session

niemyjski commented Apr 9, 2026

View reviewed changes

src/Foundatio/Storage/IFileStorage.cs Outdated Show resolved Hide resolved

niemyjski commented Apr 9, 2026

View reviewed changes

src/Foundatio/Storage/IFileStorage.cs Show resolved Hide resolved

Copilot AI reviewed Apr 9, 2026

View reviewed changes

src/Foundatio/Storage/ScopedFileStorage.cs Show resolved Hide resolved

pr feedback

7afb60d

niemyjski requested a review from Copilot April 9, 2026 15:12

Copilot started reviewing on behalf of niemyjski April 9, 2026 15:13 View session

github-code-quality bot found potential problems Apr 9, 2026

View reviewed changes

src/Foundatio/Jobs/WorkItemJob/WorkItemJob.cs Dismissed Show dismissed Hide dismissed

src/Foundatio/Jobs/WorkItemJob/WorkItemJob.cs Dismissed Show dismissed Hide dismissed

Copilot AI reviewed Apr 9, 2026

View reviewed changes

src/Foundatio/Serializer/ISerializer.cs Show resolved Hide resolved

src/Foundatio/Caching/CacheValue.cs Show resolved Hide resolved

niemyjski added 2 commits April 9, 2026 11:58

pr feedback

237c4d2

Fix NRT annotations compiler warnings

b863c7b

- Use ArgumentException.ThrowIfNullOrEmpty for path validation - Add [return: MaybeNull] and XML documentation to GetObjectAsync dotnet/roslyn#30953

niemyjski requested a review from Copilot April 9, 2026 17:01

Copilot started reviewing on behalf of niemyjski April 9, 2026 17:01 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

src/Foundatio.Xunit/Logging/TestLogger.cs Show resolved Hide resolved

src/Foundatio.Xunit.v3/Logging/TestLogger.cs Show resolved Hide resolved

src/Foundatio/FoundatioServicesExtensions.cs Show resolved Hide resolved

pr feedback

1e61813

niemyjski requested a review from Copilot April 9, 2026 17:50

Copilot started reviewing on behalf of niemyjski April 9, 2026 17:50 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

src/Foundatio/Utility/MaintenanceBase.cs Show resolved Hide resolved

pr feedback

5c6c9d1

niemyjski requested a review from Copilot April 9, 2026 18:35

Copilot started reviewing on behalf of niemyjski April 9, 2026 18:36 View session

niemyjski commented Apr 9, 2026

View reviewed changes

Copilot AI reviewed Apr 9, 2026

View reviewed changes

src/Foundatio/Lock/ThrottlingLockProvider.cs Show resolved Hide resolved

src/Foundatio/Lock/CacheLockProvider.cs Show resolved Hide resolved

pr feedback

0d71aa6

niemyjski merged commit 9cc3f37 into main Apr 9, 2026
4 of 5 checks passed

niemyjski deleted the nrt/comprehensive-null-safety-audit branch April 9, 2026 19:40

dependabot bot mentioned this pull request Apr 13, 2026

Bump Foundatio.Xunit.v3 from 13.0.0-beta1 to 13.0.0-beta4 exceptionless/Exceptionless.DateTimeExtensions#137

Open

Uh oh!

Conversation

niemyjski commented Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Bugs Uncovered and Fixed

1. FoundatioServicesExtensions.UseInMemory()/UseFolder() — NullReferenceException on null options

2. SharedOptions.UseServices() — Silent overwrite of user-configured defaults with null

3. CacheLockProvider — Null _resiliencePolicyProvider before immediate .GetPolicy() call

4. HybridCacheClient — Wrong logger factory passed to local cache options

5. CacheLockProvider DI registration — GetService for required IMessageBus

Feedback-Driven Improvements

Breaking Changes — Public API Surface

Interface Return Types Made Nullable

Interface Properties Made Nullable

Extension Method Return Types Made Nullable

Abstract/Virtual Method Signatures Changed

DTO Properties Made required

DTO Properties Made Nullable

Utility Method Signatures Changed

Constructor Parameter Annotations

Nullable Attributes Added

ISerializer Deserialization Pattern

Developer Experience (DX) Notes

What this PR improves for consumers

Test improvements

Remaining justified null! usages

Impact on Provider Repositories

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

niemyjski commented Mar 28, 2026 •

edited

Loading

1. `FoundatioServicesExtensions.UseInMemory()`/`UseFolder()` — NullReferenceException on null options

2. `SharedOptions.UseServices()` — Silent overwrite of user-configured defaults with null

3. `CacheLockProvider` — Null `_resiliencePolicyProvider` before immediate `.GetPolicy()` call

4. `HybridCacheClient` — Wrong logger factory passed to local cache options

5. `CacheLockProvider` DI registration — `GetService` for required `IMessageBus`

DTO Properties Made `required`

Remaining justified `null!` usages