Avoid NullReferenceException when requested package version is not on the source#25
Open
droyad wants to merge 1 commit into
Open
Avoid NullReferenceException when requested package version is not on the source#25droyad wants to merge 1 commit into
droyad wants to merge 1 commit into
Conversation
droyad
commented
Jun 5, 2026
| // exist on the source. Propagate that null instead of dereferencing it below | ||
| // (SetThrottle/SetExceptionHandler), which otherwise throws a bare NullReferenceException for a | ||
| // version that isn't on the feed. | ||
| return null; |
Author
There was a problem hiding this comment.
Code below returns null as well, so it's a known return value.
droyad
commented
Jun 5, 2026
Comment on lines
+478
to
+481
| // FindPackageByIdResource returns a null downloader when the requested package version does not | ||
| // exist on the source. Propagate that null instead of dereferencing it below | ||
| // (SetThrottle/SetExceptionHandler), which otherwise throws a bare NullReferenceException for a | ||
| // version that isn't on the feed. |
Author
There was a problem hiding this comment.
Suggested change
| // FindPackageByIdResource returns a null downloader when the requested package version does not | |
| // exist on the source. Propagate that null instead of dereferencing it below | |
| // (SetThrottle/SetExceptionHandler), which otherwise throws a bare NullReferenceException for a | |
| // version that isn't on the feed. |
e05fadc to
38cb960
Compare
SourceRepositoryDependencyProvider.GetPackageDownloaderAsync retrieves a downloader from the inner FindPackageByIdResource and then unconditionally calls SetThrottle/SetExceptionHandler on it. FindPackageByIdResource returns a null downloader when the requested package version does not exist on the source, so for a missing version this threw a bare NullReferenceException. Restore never hits this because dependency resolution validates the version before downloading, but callers that request a downloader for a specific version directly (e.g. Calamari's V3 package download) do. Null- check the downloader and propagate the null, matching the not-found contract of FindPackageByIdResource.GetPackageDownloaderAsync. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
38cb960 to
f21310c
Compare
Author
|
Closing in favour of OctopusDeploy/Calamari#1994 |
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.
Bug
Fixes: n/a — surfaced in OctopusDeploy/Calamari (FD-440). The defect is upstream NuGet code, unchanged since 2017 and still present on upstream's default branch; no open NuGet/Home issue.
Description
SourceRepositoryDependencyProvider.GetPackageDownloaderAsyncgets a downloader from the innerFindPackageByIdResourceand then unconditionally callsSetThrottle/SetExceptionHandleron it:The inner method is declared
Task<IPackageDownloader?>and returnsnullby contract when the requested version doesn't exist on the source (e.g.HttpFileSystemBasedFindPackageByIdResourcereturnsnullwhen the version isn't in the flat-container list). So requesting a downloader for a missing version throws a bareNullReferenceExceptioninstead of surfacing "not found".Why it's gone unnoticed: restore never reaches this with a missing version, because dependency resolution (
FindLibraryAsync) validates the version before the download step. Callers that request a downloader for a specific version directly — e.g. Octopus Calamari's V3 package download — hit the unprotected path.Fix: null-check the downloader and propagate the
null, honouring the nullable contract of the resource rather than dereferencing it. No behaviour change for the resolve-then-download path; the previously-NRE case now returnsnullfor the caller to handle.Targeting / CI: targets
release/7.4.x. That branch's GitHub Actions build is currently broken (missing the SDK-resolution fix that only landed on 7.3.x), so #26 ports that fix to 7.4.x and should merge first for this PR to get a green run.No test added — this is a one-line defensive guard on a code path upstream exercises only indirectly; behavioural coverage lives downstream in Calamari (
GivesActionableErrorWhenV3FeedIsMissingTheRequestedVersion).PR Checklist