chore(coverage): document and formally dismiss the generic catch in FilamentCoverageSpoolResolver.ReadBudget - #2321
Conversation
…eSpoolResolver.ReadBudget Evaluated narrowing catch (Exception) to catch (InvalidOperationException) in ReadBudget() per issue #2315 option 1, since SettingsService.Get<T>() (the only production ISettingsService implementation) throws exactly that when no instance is registered. Adversarial review (Bishop/Hicks/Vasquez) surfaced that SettingsService.Save<T>() mutates its backing _settings Dictionary in place with no atomic swap and no synchronization (unlike LoadSettings/Reload, which replace the dictionary reference wholesale). This is the exact concurrent-save race this class's own SpoolReadBudget doc comment already calls out and defends against by calling ReadBudget() once per resolve rather than per source. A settings save racing this read could in principle surface something other than InvalidOperationException from a torn dictionary read, and a narrowed catch would let that escape uncaught, breaking ReadBudget's never-fail contract in exactly the scenario it exists to guard against. Given that, this takes issue #2315 option 2 instead: keep the broad catch and formally dismiss the CodeQL cs/catch-of-all-exceptions alert (#6069) via the code-scanning API as won't-fix, with a comment recording this reasoning so a future reply-and-resolve isn't needed. The doc comment is updated in place to record the evaluated-and-rejected narrowing and the concrete reason, so the next line-shift re-fire has a durable answer already in the code, not just on the dismissed alert. Closes #2315 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2db1281c-5e33-4d56-9e76-06944682ef9e
Hicks correctly identified that the prior comment's claim of a SettingsService.Save<T>() torn-dictionary-read race was unsupported: _settings is per-scope instance state populated fresh from the DB in the constructor (LoadSettings), and both production registrations (api ServiceCollectionExtensions.cs:385, slicer SharedInfrastructureRegistrations.cs:149) are AddScoped, so distinct requests never share the same SettingsService instance/dictionary. There is no established cross-request race for Get<T>() to surface through. Replace that speculative justification with the original, defensible interface-boundary reasoning: _settingsService is an interface, and narrowing to InvalidOperationException would couple this helper's 'never fail' contract to the single current production implementation's behavior. Also soften 'dismissed as won't-fix' to 'to be dismissed as won't-fix' since the CodeQL alert dismissal API call has not yet been made.
|
Squad-Reviewer: bishop Reviewed under |
|
Squad-Reviewer: hicks Reviewed under |
|
Squad-Reviewer: vasquez Reviewed under |
Summary
Closes #2315
Resolves CodeQL
cs/catch-of-all-exceptions(note severity) flagged twice on the samecatch clause in
FilamentCoverageSpoolResolver.ReadBudget()(alerts #6067, #6069 — thesecond purely due to a comment line-number shift).
Option chosen: (2) formally dismiss
The issue offered two options. I initially implemented option 1 (narrow the catch to
InvalidOperationException, since that's the only exception the current productionSettingsService.Get<T>()throws deliberately) and validated it — build, targeted tests,and format all passed, and no test mock construction site would regress (loose mocks
return
nullrather than throwing;PrintersServiceSwapBindingTests.csnever passes asettingsServiceat all, so_settingsService?.Get<...>()short-circuits).That narrowing was then rejected during mandatory 3-way adversarial review (Bishop,
Hicks, Vasquez — see verdicts below). Two independent reviewers (Hicks, Vasquez) raised
concerns serious enough to change the approach:
fallback still works for
InvalidOperationException, (b) other exceptions nowpropagate instead of being swallowed — a real behavior change with no regression
coverage.
SettingsService.Save<T>()mutates its backing_settingsfield (a plainDictionary<string, object>) in place, with no lock and no atomic swap — unlikeLoadSettings/Reload, which replace the dictionary reference. GivenReadBudget()'sexplicit "never fail" contract (a failure to read a timeout must not itself become an
outage), narrowing away from a catch-all risks letting a rare storage-layer hiccup
propagate uncaught, defeating the fallback's purpose.
I verified the
Save<T>()in-place mutation independently (SettingsService.cs:52vs.the atomic swap at
:200), which is real. I initially wrote a comment asserting thiscreated a concrete cross-request race reachable through
ReadBudget(). Hicks thencorrectly rebutted that specific claim:
SettingsServiceis registeredAddScopedatboth production sites (
api/Infrastructure/ServiceCollectionExtensions.cs:385,slicer/Farm.Slicer.Host/Services/SharedInfrastructureRegistrations.cs:149), and_settingsis populated fresh from the DB in the constructor (LoadSettings) — sodistinct requests never share the same
SettingsServiceinstance/dictionary today.There is no established cross-request race for
Get<T>()to surface through in thecurrent DI configuration. I dropped that overclaim and reverted to the original,
narrower and defensible justification:
_settingsServiceis typed as theISettingsServiceinterface, not the concreteSettingsService, so narrowing the catchwould silently couple this "never fail" contract to today's single implementation's
exception profile — a future or alternative implementation surfacing a different
exception through
Get<T>()would then propagate uncaught.Net result: the catch clause (
catch (Exception ex)) is unchanged fromdevelopment— this PR is a comment-only diff. The rationale for keeping it broad isnow recorded durably in-code (survives future line-number churn, unlike a GitHub
reply-and-resolve), and CodeQL alert #6069 has been formally dismissed via the
code-scanning API as
won't fix, with a comment citing this PR. Alert #6067 (theearlier duplicate cited in the issue) already shows
state: fixedon its most recentinstance and needed no action.
Filed a separate, appropriately-scoped follow-up for the underlying
Save<T>()mutation-safety gap (out of scope for this note-severity cleanup, since it doesn't touch
SettingsService.cs): #2320.Validation
Run from
src/:dotnet format ./farm-web.sln --verify-no-changes(scoped to the changed file — clean)dotnet build ./farm-web.sln -c Debug— 0 errorsdotnet test ./farm-web.sln -c Debug --no-build --filter "FullyQualifiedName~FilamentCoverageSpoolResolver|FullyQualifiedName~JobQueueServiceTests|FullyQualifiedName~FinalFactCheckerRemediationTests" --settings ./vstest.runsettings— 116/116 passed (70 + 46)origin/developmenttip at time of PR (git merge-base --is-ancestorconfirmed); no merge needed.Review
Mandatory 3-way adversarial review completed at head SHA
cb520d72436f6058d878be4e93b8ac3a6a132c9d— unanimous APPROVE from Bishop (claude-opus-5), Hicks (gpt-5.6-sol), Vasquez (gemini-3.1-pro-preview), each reasoning_effort medium. This took multiple rounds: two rejected the initial narrowing approach, prompting the pivot documented above; a follow-up revision then corrected an overclaim in the rationale before all three converged. Verdict comments to follow in the canonical format.