Repository navigation
Fleet Sweeps reports an error instead of "no sweeps" when its store read fails (#4315) - #4328
Conversation
The five sweep presentation reads (span, verdicts, would-have-paged ledger, watch-items-by-state, open+carried watch items) logged and swallowed a store fault into an empty list, so a store the collector could not reach rendered as a genuinely quiet fleet on both the web page and get_sweep_reports. They now throw, reaching each surface's existing failure backstop instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
Round-1 security review at 6d35b4eRead-only review of One Medium, three Lows. No High. Medium 1:
|
# Conflicts: # Darling/PerformanceMonitor.Darling.Service/DarlingFleetSweepEndpoints.cs
get_sweep_reports' catch now explains that a fault reached through /api/read/get_sweep_reports logs twice on purpose: this line carries the exception and stack, ToHttpResult -> ServerErrorResult carries the route and elapsed time. DarlingWebEndpoints.cs and DarlingMcpHostService.cs both said the mirror's child reads "log-and-degrade" -- stale since #4315 made them throw. Both now say the child reads throw, the tool's own catch logs once and answers the error envelope, and (web path only) ToHttpResult/ServerErrorResult answers the fixed body. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
…pin (#4328 round 1, Low 3) FleetSweepStoreLivePostgresTests.EachPresentationRead_ThrowsOnAStoreFault_NeverDegradesToEmpty asserted Assert.ThrowsAnyAsync<Exception> for all five reads; now each asserts PostgresException with SqlState 42P01, the actual code a dropped relation raises, not just "some exception". FleetSweepWebFeedTests.TheSweepReads_HaveNoLocalCatch_NotExMessage covered only /api/sweeps/latest and /api/sweeps/{id}; now also covers /api/sweeps and /api/sweeps/watch-items, the census's own slicing works unchanged for both. Two new tests: - FleetSweepStoreLivePostgresTests.GetSweepReports_LogsExactlyOnce_WithTheExceptionAttached_OnAStoreFault pins get_sweep_reports' one log line on a store fault: exactly one Error entry, the real Exception attached (not just its message baked into text), and the answer is the error envelope, never empty. - WebExceptionTextCensusTests.ToHttpResult_ToolCaughtUndefinedTable_MapsTo500_NoRelationOrSqlStateText_OneError feeds the envelope get_sweep_reports' catch would build for a dropped-relation fault through ToHttpResult (no database) and asserts the wire body has neither 42P01 nor "relation". CapturingTestLogger gained ExceptionsAtLevel(LogLevel), additive: it now also records each Log call's Exception argument so a test can assert one reached the sink, not just that its text did. Revert-proved all five: re-adding a try/catch swallow to each of the five FleetSweepStore presentation reads in turn (GetSweepsBySpanAsync, GetServerVerdictsAsync, GetWouldHavePagedAsync, GetWatchItemsByStateAsync, GetOpenAndCarriedWatchItemsAsync) failed EachPresentationRead_ThrowsOnAStoreFault_NeverDegradesToEmpty at that read's own assertion line each time, then reverted before this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
|
Round 1 fixes for #4315, done in three commits on 1. Merged origin/dev (brings #4293) and resolved the conflict — commit
|
| Read swallowed | Test result | Failing line |
|---|---|---|
GetSweepsBySpanAsync |
FAIL — "No exception was thrown" | FleetSweepStoreLivePostgresTests.cs:425 (spanFault) |
GetServerVerdictsAsync |
FAIL — "No exception was thrown" | FleetSweepStoreLivePostgresTests.cs:429 (verdictsFault) |
GetWouldHavePagedAsync |
FAIL — "No exception was thrown" | FleetSweepStoreLivePostgresTests.cs:432 (ledgerFault) |
GetWatchItemsByStateAsync |
FAIL — "No exception was thrown" | FleetSweepStoreLivePostgresTests.cs:435 (watchStateFault) |
GetOpenAndCarriedWatchItemsAsync |
FAIL — "No exception was thrown" | FleetSweepStoreLivePostgresTests.cs:439 (openCarriedFault) |
Each swallow was reverted immediately after its failure was observed. git diff --stat on FleetSweepStore.cs after the fifth revert showed nothing — the file matched the committed state exactly, and there are two sibling methods (GetSweepsBySpanForRollupAsync, GetServerVerdictsForEngineAsync) that share the same SQL constants but are NOT part of the five presentation reads and were not touched.
Rig
Extracted pg-runtime.zip (dated Sep 23) to C:\GitHub\worktrees\rig-4328\pgsql, initdb -D C:\GitHub\worktrees\rig-4328\data -U darling -A trust --encoding=UTF8, appended shared_preload_libraries = 'timescaledb', port = 55996, listen_addresses = '127.0.0.1', timescaledb.max_background_workers = 74, max_worker_processes = 85 (CI's darling-pg job numbers), timezone = 'UTC', log_timezone = 'UTC' (initdb had picked America/New_York) before first start. Started, created darlingtest and probe, ran everything below against darlingtest, then pg_ctl stop -m fast at the end — server confirmed stopped.
Test totals (final state, all reverts undone, DARLING_TEST_PG set)
WebExceptionTextCensusTests: 38 total, 0 failed, 0 skipped (36 unchanged + 1 new; the live timeout test ran instead of skipping now that the rig was up).FleetSweepStoreLivePostgresTests: 8 total, 0 failed, 0 skipped (7 unchanged + 1 new; all live, none skipped).FleetSweepWebFeedTests: 39 total, 0 failed, 0 skipped.DarlingMcpFleetSweepToolsTests: 21 total, 0 failed, 0 skipped.DocCommentHygieneTests: 77 total, 0 failed, 0 skipped.DarlingWebFailureHandlingTests(spot check for the sharedCapturingTestLoggerchange): 19 total, 0 failed.
Build: Darling/Darling.Tests/Darling.Tests.csproj -c Debug, 0 Warning(s), 0 Error(s), each time (checked the Build succeeded line, not a piped exit code).
Did not run: the full suite (CI runs it) and Installer.Tests (never run, per instructions).
Nothing filed
Everything in the brief fit in-lane; no follow-up issues opened.
For the coordinator to double-check
- The merge resolution's choice of "this PR's
BuildDetailAsyncsignature (no logger)" over dev's call — confirmed by the fact that dev's ownBuildDetailAsyncdefinition still threads a logger intoGetServerVerdictsAsync/GetWouldHavePagedAsyncfor its pre-Fleet Sweeps shows "no sweeps" when the store read fails #4315 log-and-degrade posture, which this PR's simplified reads no longer accept; the unconflicted part of the merge (the actualBuildDetailAsyncdefinition, this PR's version, no logger) already proved this by compiling. AssertNoLocalCatch's route-literal matching for/api/sweepsrelies on the literal including a closing quote ("/api/sweeps") so it doesn't false-match inside"/api/sweeps/latest"etc. — verified by direct grep, each of the four route literals appears exactly once in the source.
Closes #4315.
Why
Fleet Sweeps' five presentation reads (the sweep timeline span, one sweep's per-server verdicts, its would-have-paged ledger, watch items by state, and the default open+carried watch-item view) each caught their own store fault, logged it, and returned an empty list. Both surfaces that call these reads render an empty list as a genuinely quiet fleet: the web page shows "No sweeps in this span. Widen the span, or check that fleet_sweep.enabled is on...", and the
get_sweep_reportsMCP tool answers an empty or silently-degraded document. A store the collector cannot reach - the exact case this page exists to surface - read as clean instead of broken, on both surfaces.The five throwing "engine" twins already sitting beside these reads (
GetServerVerdictsForEngineAsync,GetSweepsBySpanForRollupAsync) show the intended pattern: a read with no degraded rendering to fall back to should throw and let its caller answer loudly, rather than manufacture a healthy-looking answer.What changes
Darling/PerformanceMonitor.Darling.Storage/FleetSweepStore.cs:GetSweepsBySpanAsync,GetServerVerdictsAsync,GetWouldHavePagedAsync,GetWatchItemsByStateAsync, andGetOpenAndCarriedWatchItemsAsyncno longer catch and swallow a store fault (OperationCanceledExceptionstill propagates as before). Each also drops its now-unusedILogger? loggerparameter, matching the throwing "engine" reads' shape. Doc comments on these five, and on their engine twins (whose "opposite fault posture" contrast is no longer true - both sides throw now, they just stay separate methods), are updated to say so.Darling/PerformanceMonitor.Darling.Service/DarlingFleetSweepEndpoints.cs: the three routes that call the five reads (/api/sweeps,/api/sweeps/watch-items, andBuildDetailAsyncused by/api/sweeps/latestand/api/sweeps/{id}) drop the now-removedloggerargument./api/sweepsand/api/sweeps/watch-itemshave no local try/catch, so a fault now reaches the web pipeline's existing Web viewer: a read that times out returns an empty HTTP 500 and writes nothing to the service log #4276 top-of-pipeline backstop, which answers the sanitizedDarlingWebFailureLogbody/status (neverex.Message) and logs one line./api/sweeps/latestand/api/sweeps/{id}have no local catch either: their catch answeredex.Message, and Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293 removed it and merged first. This PR's merge of dev keeps it removed, so a verdicts or ledger fault inBuildDetailAsyncalso reaches the backstop instead of producing a clean-looking empty document. Updated theMapandBuildDetailAsyncdoc comments, which had claimed no fault could read as a healthy answer - true of the routes' own code, false once the store layer's swallow is counted, until this fix.Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFleetSweepTools.cs:get_sweep_reports's calls to the five reads drop the removedloggerargument. Its outercatch (Exception ex)(already present, wrapping the whole tool body) now also catches these faults and answers throughMcpHelpers.FormatError- the tool's normalstatus: "error"envelope, neverempty. Added onelogger?.LogErrorcall in that catch: the internal store-level log-and-degrade catches removed above were the only place a fault on this path used to reach the service log (the Deliver the sweep's memory: the daily rollup ceiling and get_sweep_reports (#3466, lane 4 of 4) #3473 review's reason this tool, unlikeget_fleet_overview/get_ag_health, carries its own logger seat), so without this the MCP path would have gone from "logged once, wrong answer" to "unlogged, right answer" - a net loss of trace. UpdatedBuildDetailAsync's doc comment to match: a child fault now fails the whole document instead of costing only its section.degradedfield was added anywhere - a sweep document whose verdicts or would-have-paged ledger can't be read fails the whole request, per the ruling.sweeps.jsneeded no change:apiGetalready maps a non-2xx{"error": ...}body to{kind: "error", ...}, and every renderer insweeps.jsalready checksres.kind === "error"before falling through to its empty-state rendering. The bug was entirely that the backend answered 200 with a healthy-looking empty body on a fault; once the backend stopped doing that, the existing frontend error-strip path applies with no edits.DarlingWebFailureLog.Body/StatusCodealready answer a generic sanitized message (GenericMessage/TimeoutMessage) or a 503/500, neverex.Message- that was fixed under Web viewer: a read that times out returns an empty HTTP 500 and writes nothing to the service log #4276, and the/api/read/*dispatcher's own catch answers the same shape (Log and answer web-viewer read failures instead of an empty 500 (#4276) #4281 review). The two detail routes' old local catch was Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293's class (raw exception text on the wire); Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293 removed it.get_sweep_reportsanswers the exception text only to an MCP client, throughMcpHelpers.FormatError, as every MCP tool does; through/api/read/get_sweep_reportsthe web answer is Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293's fixed body.Darling.Testsproject for every call site of the five reads before this change; all of them exercised only the happy path, so there was nothing to update, only new coverage to add.Test plan
dotnet build Darling/Darling.Tests/Darling.Tests.csproj- 0 Warning(s), 0 Error(s) (checked three times: after the production fix, after adding the new tests, and again after mergingorigin/dev).darlingtestdatabase):*FleetSweep*,*Sweep*,McpPayloadContractCensusTests,AlertReadFailureSurfaceTests,DocCommentHygiene*- 363 total, 0 failed, 0 skipped (one more than the pre-merge run's 362;origin/devbrought in an unrelated new test matching the wildcard).FleetSweepStoreLivePostgresTests.cs:EachPresentationRead_ThrowsOnAStoreFault_NeverDegradesToEmpty- drops all four sweep tables in a scratch database and asserts each of the five reads throws.GetSweepReports_AnswersError_OnAVerdictsFault_NeverADegradedDocument- records a real sweep, drops onlyfleet_sweep_server_verdicts(the run and watch-item tables stay up, soGetLatestSweepAsyncand the span/watch-item reads still succeed and don't confound the assertion), callsDarlingMcpFleetSweepTools.GetSweepReportsdirectly, and assertsstatus: "error"with nosweepskey.GetSweepReports_AnswersError_OnAWatchItemFault_NeverAnEmptyWorklist- same shape, isolated tofleet_sweep_watch_items, assertsstatus: "error"with nowatch_itemskey.GetServerVerdictsAsyncalone, rebuilt, and re-ran - the two verdicts-dependent tests above went red ([FAIL]) as expected; the watch-item-only test stayed green. Reverted, rebuilt, and confirmed all green again.git merge origin/dev(clean, no conflicts), rebuilt, re-ran the same targeted classes above.Lite.Tests: not run - Fleet Sweeps is a central-store-only feature (get_sweep_reportshas no Lite mirror; grepped the whole repo for the five changed method names and found no reference outsideDarling/), so there is no Lite parity surface to check./api/sweepsand/api/sweeps/watch-itemsthemselves. Both routes call the exact five store methods the live tests above prove now throw, and this test project's own precedent for this class of assertion (FleetSweepWebFeedTests.TheSweepReadCatches_AnswerTheRuledBodyAndStatus_NotExMessage) is a static source pin rather than a live request, for the same reason noted there: producing a genuine store fault needs a brokenNpgsqlDataSourcethis test project otherwise never builds for this surface. The general top-of-pipeline backstop's catch-and-answer behavior for an arbitrary thrown exception is already covered end-to-end byDarlingWebFailureHandlingTests'TestServerharness.Rig
PostgreSQL 18 + TimescaleDB on port 55976,
C:\GitHub\worktrees\rig-s2(deleted after the run),timezone/log_timezoneforced to UTC (the machine defaults to America/New_York).Round-1 review fixes
Round 1 is the security review in comment 5836820492 (no High; one Medium and three Lows). All four landed here (fix report: PR comment 5838472048):
72ece03e): merge Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293 first. The merge of dev resolvedDarlingFleetSweepEndpoints.csto no local catch on/api/sweeps/latestand/api/sweeps/{id}, withBuildDetailAsync(postgres, run, context).f1ab9c09): a comment inget_sweep_reports' catch says why a fault reached through/api/read/get_sweep_reportslogs twice (the tool's line has the stack, the dispatcher's has the route and the elapsed time), and the stale "log-and-degrade" comments inDarlingWebEndpoints.csandDarlingMcpHostService.csnow say the reads throw. The merge (72ece03e) fixed the two inDarlingFleetSweepEndpoints.cs.5a016d3a): the five fault tests assert aPostgresExceptionwith SQLSTATE 42P01 instead of any exception; the no-local-catch census covers/api/sweepsand/api/sweeps/watch-items; a live test pins exactly one Error line from the tool; a test pinsToHttpResult's fixed body. Each of the five reads was revert-proven against a live rig.CHANGELOG entry
SECTION: Fixed
ENTRY:
get_sweep_reportsMCP tool answered an empty or silently incomplete document instead of reporting that the read failed. Both surfaces now answer the failure: the web page shows its error strip, and the tool answers its usual error status.REF:
[Fleet Sweeps reports an error instead of "no sweeps" when its store read fails (#4315) #4328]: Fleet Sweeps reports an error instead of "no sweeps" when its store read fails (#4315) #4328
🤖 Generated with Claude Code
https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ