Skip to content

Fleet Sweeps reports an error instead of "no sweeps" when its store read fails (#4315) - #4328

Merged
erikdarlingdata merged 5 commits into
devfrom
fix/4315-fleet-sweeps-fault-not-empty
Sep 25, 2026
Merged

erikdarlingdata merged 5 commits into
devfrom
fix/4315-fleet-sweeps-fault-not-empty

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

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_reports MCP 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, and GetOpenAndCarriedWatchItemsAsync no longer catch and swallow a store fault (OperationCanceledException still propagates as before). Each also drops its now-unused ILogger? logger parameter, 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, and BuildDetailAsync used by /api/sweeps/latest and /api/sweeps/{id}) drop the now-removed logger argument. /api/sweeps and /api/sweeps/watch-items have 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 sanitized DarlingWebFailureLog body/status (never ex.Message) and logs one line. /api/sweeps/latest and /api/sweeps/{id} have no local catch either: their catch answered ex.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 in BuildDetailAsync also reaches the backstop instead of producing a clean-looking empty document. Updated the Map and BuildDetailAsync doc 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 removed logger argument. Its outer catch (Exception ex) (already present, wrapping the whole tool body) now also catches these faults and answers through McpHelpers.FormatError - the tool's normal status: "error" envelope, never empty. Added one logger?.LogError call 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, unlike get_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. Updated BuildDetailAsync's doc comment to match: a child fault now fails the whole document instead of costing only its section.
  • No degraded field 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.js needed no change: apiGet already maps a non-2xx {"error": ...} body to {kind: "error", ...}, and every renderer in sweeps.js already checks res.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.
  • Checked what the backstop puts on the wire, per the ruling's instruction: DarlingWebFailureLog.Body/StatusCode already answer a generic sanitized message (GenericMessage/TimeoutMessage) or a 503/500, never ex.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_reports answers the exception text only to an MCP client, through McpHelpers.FormatError, as every MCP tool does; through /api/read/get_sweep_reports the web answer is Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293's fixed body.
  • No existing test asserted the empty-on-fault behavior - searched the whole Darling.Tests project 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 merging origin/dev).
  • Live rig (PostgreSQL 18 + TimescaleDB, port 55976, UTC, scratch darlingtest database): *FleetSweep*, *Sweep*, McpPayloadContractCensusTests, AlertReadFailureSurfaceTests, DocCommentHygiene* - 363 total, 0 failed, 0 skipped (one more than the pre-merge run's 362; origin/dev brought in an unrelated new test matching the wildcard).
  • New tests, all live against the rig, in 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 only fleet_sweep_server_verdicts (the run and watch-item tables stay up, so GetLatestSweepAsync and the span/watch-item reads still succeed and don't confound the assertion), calls DarlingMcpFleetSweepTools.GetSweepReports directly, and asserts status: "error" with no sweeps key.
    • GetSweepReports_AnswersError_OnAWatchItemFault_NeverAnEmptyWorklist - same shape, isolated to fleet_sweep_watch_items, asserts status: "error" with no watch_items key.
  • Proved the regression tests actually catch the old behavior: temporarily re-added the swallow-to-empty catch to GetServerVerdictsAsync alone, 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.
  • Full suite: not run here - CI runs it, per the lane's instructions.
  • Lite.Tests: not run - Fleet Sweeps is a central-store-only feature (get_sweep_reports has no Lite mirror; grepped the whole repo for the five changed method names and found no reference outside Darling/), so there is no Lite parity surface to check.
  • No TestServer-based live HTTP test was added for /api/sweeps and /api/sweeps/watch-items themselves. 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 broken NpgsqlDataSource this 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 by DarlingWebFailureHandlingTests' TestServer harness.

Rig

PostgreSQL 18 + TimescaleDB on port 55976, C:\GitHub\worktrees\rig-s2 (deleted after the run), timezone/log_timezone forced 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):

  • M1 (72ece03e): merge Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293 first. The merge of dev resolved DarlingFleetSweepEndpoints.cs to no local catch on /api/sweeps/latest and /api/sweeps/{id}, with BuildDetailAsync(postgres, run, context).
  • L1 and L2 (f1ab9c09): a comment in get_sweep_reports' catch says why a fault reached through /api/read/get_sweep_reports logs 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 in DarlingWebEndpoints.cs and DarlingMcpHostService.cs now say the reads throw. The merge (72ece03e) fixed the two in DarlingFleetSweepEndpoints.cs.
  • L3 (5a016d3a): the five fault tests assert a PostgresException with SQLSTATE 42P01 instead of any exception; the no-local-catch census covers /api/sweeps and /api/sweeps/watch-items; a live test pins exactly one Error line from the tool; a test pins ToHttpResult's fixed body. Each of the five reads was revert-proven against a live rig.

CHANGELOG entry

SECTION: Fixed
ENTRY:

🤖 Generated with Claude Code

https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ

erikdarlingdata and others added 2 commits September 25, 2026 13:16
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
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Round-1 security review at 6d35b4e

Read-only review of origin/dev...6d35b4e6: all four changed files, plus the callers and consumers each question named. That covers the #4276 backstop (DarlingWebHostService.cs:1212), DarlingWebFailureLog.cs, the /api/read/* dispatcher and ToHttpResult in DarlingWebEndpoints.cs, McpHelpers.FormatError, util.js (apiGet, classifyResponse), sweeps.js, a repo-wide git grep for the five reads, and #4293's head (5b6d0f9b), which I merged against this head with git merge-tree. I ran no tests locally.

One Medium, three Lows. No High.

Medium 1: /api/read/get_sweep_reports now puts store-fault exception text on the web wire

Where: Mcp/DarlingMcpFleetSweepTools.cs:169 (the catch returns McpHelpers.FormatError), DarlingWebEndpoints.cs:2776 (the web mirror's dispatch entry for this tool), and DarlingWebEndpoints.cs:3095 (the ServerError arm of ToHttpResult, unchanged from dev).

Who and what: anyone with a read seat can call it (it is a GET, so the viewer seat is enough), and so can any local process on the service box, because loopback needs no token. They get the store's PostgreSQL error text: the SQLSTATE and the server's message, which names relations and roles. On a connection fault they get the store's host and port.

Sequence:

  1. A caller sends GET /api/read/get_sweep_reports.
  2. One of the five reads fails. For example, the verdicts relation cannot be read, or its query hits the statement timeout.
  3. Before this PR the store swallowed that fault into an empty list. Now it throws into the tool's outer catch, which returns the FormatError envelope with the message Error during get_sweep_reports: <ex.Message>. The envelope is correct for the MCP wire, and this finding does not rank it.
  4. The dispatcher's try sees no exception, because the tool returned a string. So ToHttpResult runs, and its ServerError arm answers HTTP 500 with the body {"error": McpHelpers.ErrorMessageOf(result)}. That body carries the exception text.

Why Medium and not High: the leaking arm predates this PR. It is #4283's arm, and the dispatcher's own comment at DarlingWebEndpoints.cs:322 says so. This route already leaked a whole-store outage through GetLatestSweepAsync, which always threw. This PR widens the leak to partial faults in the five reads. If #4328 merges before #4293 and #4293 then stalls, this is the brief's High class on a live route.

The Fleet Sweeps page does not call this route (sweeps.js reads only /api/sweeps*), so the page's own error strip is unaffected.

The finding also makes one line of the PR body wrong: "Nothing here falls into #4293's class (raw exception text on the wire); no separate note needed beyond this one." Please correct it.

Fix: merge #4293 first. On its head, ToHttpResult sends the ServerError kind to ServerErrorResult. That answers DarlingWebFailureLog.Body(sentence), which is fixed text, under 500 or 503. It closes this route and every other /api/read/* tool. #4328 needs no code change for this finding, only the PR-body correction.

Low 1: once both PRs merge, /api/read/get_sweep_reports logs each fault twice

Where: Mcp/DarlingMcpFleetSweepTools.cs:168 (the new logger?.LogError), plus ServerErrorResult, which calls DarlingWebFailureLog.Report(logger, route, elapsedMs, sentence) (#4293 head, DarlingWebEndpoints.cs:3149).

Sequence: a fault reaches the tool's catch, which logs get_sweep_reports failed: ... with the exception and returns the error envelope. #4293's ToHttpResult classifies that envelope as ServerError, and ServerErrorResult logs Web dashboard read /api/read/get_sweep_reports failed ...: Error during get_sweep_reports: .... Both lines go to the same service logger, because the dispatch builder closes over it. The result is two Error lines for one fault. At 6d35b4e alone this path logs once, because dev's ToHttpResult does not log.

Fix, in whichever PR merges second. Pick one:

  • Say in the tool's catch comment that this path writes two lines on purpose. The tool's line carries the stack, and the dispatcher's line carries the route and elapsed time.
  • Or skip the tool's line when the dispatcher is the caller, for example with a flag the dispatch entry sets.

Do not pass a null logger for the dispatch entry. ReadWatchItemNamesAsync needs it for the names read's degrade line.

Low 2: comments that are stale now, or will be once #4293 merges

Fix: update the first two comments in this PR. Update the last two in whichever PR merges second.

Low 3: test gaps

  • The revert proof covered one of the five reads: the swallow was re-added to GetServerVerdictsAsync only. EachPresentationRead_ThrowsOnAStoreFault_NeverDegradesToEmpty asserts each read on its own line, so a revert of any single read would fail it by construction. Only the verdicts leg was actually shown red.
  • Assert.ThrowsAnyAsync<Exception> passes on any exception. Assert PostgresException with SqlState 42P01 instead, so the test proves the dropped relation is what threw.
  • Nothing pins that /api/sweeps and /api/sweeps/watch-items stay free of a local catch. A future catch there that answers an empty 200 would pass every test. After Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293, add both routes to TheSweepReads_HaveNoLocalCatch_NotExMessage; its AssertNoLocalCatch slicing works for them unchanged.
  • The tool's new log line is not pinned. Every tool test passes null or NullLogger. A capturing logger that asserts exactly one Error line would pin the "once" answer below.
  • No test in this PR checks the body /api/read/get_sweep_reports returns on a fault. Such a test would have caught Medium 1.

Questions that found nothing, and what I read

Q1, exception text on the dedicated routes: clean on all four.

  • GET /api/sweeps (DarlingFleetSweepEndpoints.cs:175) and GET /api/sweeps/watch-items (:241) have no local catch. A fault reaches the backstop, which answers application/json with the fixed GenericMessage under 500, or TimeoutMessage under 503, in an error field.
  • GET /api/sweeps/latest (:196) and GET /api/sweeps/{id} (:220) keep their local catches. Those answer through DarlingWebFailureLog.Body(ex) and StatusCode(ex) with the same fixed text, and a verdicts or ledger fault in BuildDetailAsync now lands there.
  • The backstop is registered before DarlingWebEndpoints.MapAll, so it wraps every sweep route. It writes only when the response has not started, and never writes ex.Message.
  • The 400 and 404 bodies are static sentences or validator sentences, not exception text.

Q2, failures stay visible: yes, on all three panels.

  • For a non-2xx response whose body has a string error field, classifyResponse (util.js:475) returns kind error. Even a stray 2xx envelope with status error maps to error.
  • renderDetail (sweeps.js:129) shows its empty strip only for a 404. It shows the error strip for 500 and 503, and the backstop never answers 404.
  • renderTimeline (:304) and renderWatchItems (:357) test for kind error before their "No sweeps in this span" and "Nothing is open right now" branches.
  • Each panel's loading strip is replaced on both branches. apiGet does not reject on an HTTP error, and each panel loads independently.

Q3, callers that relied on the empty list: none outside the two surfaces.

  • git grep at 6d35b4e finds the five reads only in DarlingFleetSweepEndpoints.cs, DarlingMcpFleetSweepTools.cs, FleetSweepStore.cs and FleetSweepStoreLivePostgresTests.cs.
  • The sweep engine and the daily rollup call the separate throwing twins, GetServerVerdictsForEngineAsync and GetSweepsBySpanForRollupAsync. Only their doc comments changed.
  • No worker, self-alert evaluator, fleet overview, timer or background service calls the five reads or get_sweep_reports. The self-alert evaluator names the tool only in prose. So no collection or alerting loop gains a new throw.
  • Nothing swallows the fault again. The tool answers status error, the routes answer 500 or 503, and the page shows its error strip.
  • The watch-item names read (ReadWatchItemNamesAsync calling GetSweepServerNamesAsync) still logs and degrades, by design (Fleet Sweep watch-item worklist shows raw server_id instead of the server name #3482). A names fault shows the bare server id, never an empty worklist. Not ranked.

Q4, logging: exactly once on every path at this head. No secrets.

  • /api/sweeps and /api/sweeps/watch-items log through the backstop's DarlingWebFailureLog.Report, which writes the sanitized route, the exception type, the SQLSTATE and the exception object.
  • /latest and /{id} log through their local catch's Report.
  • The MCP transport logs through the tool's new line. The MCP host registers its real logger (DarlingMcpHostService.cs:518), and neither call-tool filter logs error envelopes.
  • /api/read/get_sweep_reports logs through the tool's line only, until Web viewer: stop sending exception text to the browser on the remaining paths (#4283) #4293 lands (see Low 1).
  • The store's old per-read lines are gone, so nothing double-logs at this head.
  • No line carries a connection string, a password, a token or a header. Npgsql's messages carry none of these.
  • Two notes, not ranked. First, the tool's {Message} template argument repeats ex.Message, a pattern Web failure log: keep the log batch on a split character, answer bad requests with their own status #4286 review Low 5 moved away from. The file sink cleans the whole line, and no request text reaches these messages, because every input is typed or validated before the store is touched. Second, the tool logs a statement timeout at Error, where the web path logs Warning.

Q5, the sibling PR: the two conflict, and #4293 first is the safe order.

Q6, tests: they pin what they claim, with the gaps in Low 3.

  • The store-level test drops all four tables and asserts each of the five reads separately. The dropped table names match each read's SQL (FleetSweepStore.cs:313-439).
  • The old code swallowed every exception except a cancellation, so each assertion would fail if its read were reverted.
  • The two tool tests pin the verdicts leg and the default watch-item view end to end: status error, and no sweeps or watch_items key. The timeline and ledger legs are covered only at the store level. That is enough, because the tool's one catch wraps its whole body.
  • The tests ran in CI. The Darling PG shards set DARLING_TEST_PG and run the full suite. All three shards passed with 0 failed, and no test from FleetSweepStoreLivePostgresTests appears in any shard's skip list.

erikdarlingdata and others added 3 commits September 25, 2026 15:20
# 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
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Round 1 fixes for #4315, done in three commits on fix/4315-fleet-sweeps-fault-not-empty, HEAD now 5a016d3a.

1. Merged origin/dev (brings #4293) and resolved the conflict — commit 72ece03e

Darling/PerformanceMonitor.Darling.Service/DarlingFleetSweepEndpoints.cs was the only conflicted file, exactly the two hunks the brief named (/api/sweeps/latest and /api/sweeps/{id:long}). Resolved to: no local try/catch in either route (dev's #4283 shape — a fault reaches the #4276 backstop), and BuildDetailAsync(postgres, run, context) with no logger argument (this PR's already-simplified signature at line 243 — dev's own call passed a logger it doesn't take, since dev's copy of BuildDetailAsync still forwards a logger into the two child reads for its older log-and-degrade shape). The five FleetSweepStore reads keep throwing; get_sweep_reports' catch keeps its logger?.LogError. System.Diagnostics/...Hosting usings stayed out — nothing in the resolved file needs them (Stopwatch only appeared inside the deleted try blocks).

Fixed the two comments the merge left stale:

Tests: WebExceptionTextCensusTests 37 total, 0 failed, 1 skipped (the live timeout test, no DARLING_TEST_PG yet at that point) — TheSweepReads_HaveNoLocalCatch_NotExMessage run alone also passed. No FleetSweepStoreTests file exists (only FleetSweepStoreLivePostgresTests.cs), so that part of the instruction didn't apply.

2. The double log and two stale "log-and-degrade" comments — commit f1ab9c09

  • Mcp/DarlingMcpFleetSweepTools.cs:169 (inside get_sweep_reports' catch (Exception ex)): added a paragraph explaining that reached through /api/read/get_sweep_reports a fault logs twice on purpose — this line carries the exception and its stack, DarlingWebEndpoints' ToHttpResult -> ServerErrorResult carries the route and elapsed time. Did not touch the dispatch entry's logger argument (ReadWatchItemNamesAsync still needs it).
  • DarlingWebEndpoints.cs:2840 (the get_sweep_reports dispatch entry's comment): "the mirror's child reads log-and-degrade into the same service log" now reads "the mirror's child reads throw (Fleet Sweeps shows "no sweeps" when the store read fails #4315), the tool's own catch logs the exception once and answers the error envelope, and this entry's ToHttpResult classifies that envelope and answers the fixed body through ServerErrorResult".
  • Mcp/DarlingMcpHostService.cs:516: "get_sweep_reports' child reads log-and-degrade" now reads "get_sweep_reports' child reads throw on a store fault (Fleet Sweeps shows "no sweeps" when the store read fails #4315) and the tool's own catch logs the exception once".

Comment-only change, no test file touched for this item specifically; DocCommentHygieneTests (77 total, 0 failed) covers doc-comment shape and passed.

3. Test gaps — commit 5a016d3a

  • FleetSweepStoreLivePostgresTests.cs, EachPresentationRead_ThrowsOnAStoreFault_NeverDegradesToEmpty (line 393): all five assertions changed from Assert.ThrowsAnyAsync<Exception> to Assert.ThrowsAsync<PostgresException>, each followed by Assert.Equal("42P01", fault.SqlState). Checked for siblings project-wide (grep -rn ThrowsAnyAsync) — the only other fleet-sweep ThrowsAnyAsync is RecordSweepAsync's NUL-byte write test at line 246, a different concern (not a store fault), left alone.
  • FleetSweepWebFeedTests.cs, TheSweepReads_HaveNoLocalCatch_NotExMessage (line 648): added /api/sweeps and /api/sweeps/watch-items to the two existing routes. Confirmed each route literal ("/api/sweeps", "/api/sweeps/watch-items") appears exactly once in the source, so AssertNoLocalCatch's slice-to-next-MapGet (or end of file, for the last route) lands on the right body in each case.
  • New test FleetSweepStoreLivePostgresTests.GetSweepReports_LogsExactlyOnce_WithTheExceptionAttached_OnAStoreFault (line 505): drops all four fleet-sweep tables (reusing the existing live test's scratch-database pattern), calls the tool with a CapturingTestLogger, asserts exactly one Error entry, the real PostgresException attached (not just its text), and the answer is the error envelope, not empty.
  • New test WebExceptionTextCensusTests.ToHttpResult_ToolCaughtUndefinedTable_MapsTo500_NoRelationOrSqlStateText_OneError (line 561): no database — builds the envelope with McpHelpers.FormatError("get_sweep_reports", new PostgresException("relation \"x\" does not exist", "ERROR", "ERROR", "42P01")) exactly as the tool's catch does, feeds it through DarlingWebEndpoints.ToHttpResult, asserts a 500 whose body is DarlingWebFailureLog.GenericMessage and contains neither 42P01 nor relation.
  • CapturingTestLogger.cs:54: added ExceptionsAtLevel(LogLevel), additive (a new _exceptions list alongside the existing _lines, populated in Log) — every other consumer of this shared test helper is unaffected; spot-checked DarlingWebFailureHandlingTests (19 total, 0 failed) after the change.

Revert-proof, all five reads

Re-added a try { ... } catch (Exception) { } swallow to each of the five FleetSweepStore presentation reads in turn (one at a time, reverted before moving to the next), rebuilt, and ran EachPresentationRead_ThrowsOnAStoreFault_NeverDegradesToEmpty alone against the rig each time:

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 shared CapturingTestLogger change): 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 BuildDetailAsync signature (no logger)" over dev's call — confirmed by the fact that dev's own BuildDetailAsync definition still threads a logger into GetServerVerdictsAsync/GetWouldHavePagedAsync for 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 actual BuildDetailAsync definition, this PR's version, no logger) already proved this by compiling.
  • AssertNoLocalCatch's route-literal matching for /api/sweeps relies 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.

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 25, 2026 19:41
@erikdarlingdata
erikdarlingdata enabled auto-merge (squash) September 25, 2026 19:41
@erikdarlingdata
erikdarlingdata merged commit 5cdf508 into dev Sep 25, 2026
17 of 18 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/4315-fleet-sweeps-fault-not-empty branch September 25, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant