Skip to content

Return null from FaultGenerator when no fault is generated - #3220

Merged
martincostello merged 1 commit into
App-vNext:mainfrom
dualfroz:fix/faultgenerator-null-fault
Sep 6, 2026
Merged

martincostello merged 1 commit into
App-vNext:mainfrom
dualfroz:fix/faultgenerator-null-fault

Conversation

@dualfroz

@dualfroz dualfroz commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

The issue or feature being addressed

FaultGenerator throws InvalidOperationException ("Nullable object must have a
value.") instead of injecting no fault when its underlying generator yields no
outcome.

The implicit conversion operator on FaultGenerator
(src/Polly.Core/Simmy/Fault/FaultGenerator.cs) builds its delegate as:

return args => new ValueTask<Exception?>(generatorDelegate(args.Context)!.Value.Exception);

generatorDelegate is a Func<ResilienceContext, Outcome<VoidResult>?> produced by
GeneratorHelper<VoidResult>.CreateGenerator(). It legitimately returns null when:

  • no exception has been registered (an empty FaultGenerator), in which case
    CreateGenerator() returns _ => null; or
  • the registered weights sum to zero (for example AddException<T>(weight: 0)), in
    which case the selection loop matches no factory and returns null.

Because Outcome<TResult> is a readonly struct, Outcome<VoidResult>? is a
Nullable<Outcome<VoidResult>>. The null-forgiving ! operator has no runtime effect,
so the subsequent .Value dereferences an empty nullable and throws
InvalidOperationException.

ChaosFaultStrategy.ExecuteCore explicitly treats a null fault as "do not inject"
(if (fault is not null)), and its try block only catches
OperationCanceledException, so the exception escapes and fails the user's execution
that the strategy was meant to leave untouched.

The sibling OutcomeGenerator<TResult> handles the same situation correctly by
propagating the nullable value unchanged
(src/Polly.Core/Simmy/Outcomes/OutcomeGenerator.cs):

return args => new ValueTask<Outcome<TResult>?>(generatorDelegate(args.Context));

Details on the issue fix or feature implementation

Propagate null from the generator instead of dereferencing it, mirroring
OutcomeGenerator<TResult>:

return args => new ValueTask<Exception?>(generatorDelegate(args.Context)?.Exception);

When a fault is produced, Outcome<VoidResult>.Exception returns the same exception as
before; when the generator yields no outcome, the delegate now returns null and
ChaosFaultStrategy proceeds without injecting a fault.

Added two unit tests to test/Polly.Core.Tests/Simmy/Fault/FaultGeneratorTests.cs:

  • NoExceptionRegistered_ShouldReturnNull - an empty FaultGenerator returns null.
  • AllWeightsZero_ShouldReturnNull - a generator whose weights sum to zero returns null.

Both tests fail on the unmodified code with
System.InvalidOperationException : Nullable object must have a value. at
FaultGenerator.cs:81, and pass after the fix.

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

FaultGenerator's implicit conversion dereferenced the nullable Outcome
returned by the underlying generator via `!.Value`, throwing
InvalidOperationException ("Nullable object must have a value.") when the
generator produced no outcome - an empty generator, or registered weights
summing to zero. ChaosFaultStrategy treats a null fault as "do not inject"
and only catches OperationCanceledException, so the exception escaped and
failed the execution the strategy was meant to leave untouched.

Propagate the nullable outcome with `?.Exception`, mirroring
OutcomeGenerator<TResult>, so no fault is injected when none is generated.
@dualfroz

dualfroz commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

@dotnet-policy-service agree

@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.16%. Comparing base (df53959) to head (65d060c).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3220   +/-   ##
=======================================
  Coverage   96.16%   96.16%           
=======================================
  Files         311      311           
  Lines        7142     7142           
  Branches     1006     1007    +1     
=======================================
  Hits         6868     6868           
  Misses        221      221           
  Partials       53       53           
Flag Coverage Δ
linux 96.16% <100.00%> (ø)
macos 96.16% <100.00%> (ø)
windows 96.15% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@martincostello martincostello changed the title fix(simmy): return null from FaultGenerator when no fault is generated Return null from FaultGenerator when no fault is generated Sep 6, 2026
@martincostello martincostello added this to the v3.8.0 milestone Sep 6, 2026
@martincostello
martincostello merged commit 482bdf8 into App-vNext:main Sep 6, 2026
27 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your contribution @dualfroz - the changes from this pull request have been published as part of version 8.8.0 📦, which is now available from NuGet.org 🚀

This was referenced Sep 14, 2026
This was referenced Sep 15, 2026
IhateTrains pushed a commit to ParadoxGameConverters/ImperatorToCK3 that referenced this pull request Sep 16, 2026
Updated [Polly](https://github.com/App-vNext/Polly) from 8.7.0 to 8.8.0.

<details>
<summary>Release notes</summary>

_Sourced from [Polly's
releases](https://github.com/App-vNext/Polly/releases)._

## 8.8.0

## Highlights

* Add `EnableReloadsWithMonitor()` accepting custom `IOptionsMonitor` by
@​arashzjahangiri in App-vNext/Polly#3140
* Return null from `FaultGenerator` when no fault is generated by
@​dualfroz in App-vNext/Polly#3220

## What's Changed

* Update workflow timeout by @​martincostello in
App-vNext/Polly#3106
* Set `SOURCE_DATE_EPOCH` by @​martincostello in
App-vNext/Polly#3115
* Use `NUGET_API_KEY` by @​martincostello in
App-vNext/Polly#3116
* Test refactoring by @​martincostello in
App-vNext/Polly#3132
* Avoid test flakiness by @​martincostello in
App-vNext/Polly#3134
* Group github/codeql-action updates by @​martincostello in
App-vNext/Polly#3147
* Fix wrong word in hedging documentation by @​latent-9 in
App-vNext/Polly#3191
* OSMF sponsorship notice by @​joelhulen in
App-vNext/Polly#3194
* Fix XML doc comment on `PipelineExecutedArguments` by @​JiuYue0820 in
App-vNext/Polly#3201
* Update to xunit v3 by @​martincostello in
App-vNext/Polly#3131
* .NET 11 preparation by @​martincostello in
App-vNext/Polly#3225
* Fix flaky test by @​martincostello in
App-vNext/Polly#3231

## New Contributors

* @​arashzjahangiri made their first contribution in
App-vNext/Polly#3140
* @​latent-9 made their first contribution in
App-vNext/Polly#3191
* @​JiuYue0820 made their first contribution in
App-vNext/Polly#3201
* @​dualfroz made their first contribution in
App-vNext/Polly#3220

**Full Changelog**:
App-vNext/Polly@8.7.0...8.8.0


Commits viewable in [compare
view](App-vNext/Polly@8.7.0...8.8.0).
</details>

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=Polly&package-manager=nuget&previous-version=8.7.0&new-version=8.8.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants