Repository navigation
Say which databases a per-database cycle skipped - #2624
Conversation
Both runners tolerate a per-database failure by design - one offline database must not cost the other twenty-nine - and both escalate only when EVERY database failed. In between was a hole with no evidence in it: the cycle recorded SUCCESS, whatever the survivors produced, and a note composed solely from enumeration probe failures, which a thrown exception is not. When the failing database is the only one holding data, that is SUCCESS with zero rows, and zero rows is exactly the shape of a target that genuinely has nothing to report. That is how the payload-arity bug survived three schema versions. All three broken collectors run per database. All three failed identically in the one database that had data. Only pg_extension_availability surfaced as an ERROR, and only because it returns rows in every database including postgres, so all-failed tripped the escalation. The other two logged SUCCESS with zero rows and I read them as a quiet server. A partial loss now composes a note naming the skipped databases (capped at three plus a count), how many of how many failed, and the first error, merged with any probe-failure note rather than replacing it. The note says outright that the row count is about the survivors, because the row count is the first thing read off the cycle and on a partial loss it is a number about a subset wearing the shape of a number about the server. Composed in the shared driver, called from both runners, and pinned by a test that counts the arms: every catch that increments the failure count must also name the database, or the note reports a count with the wrong set of names beside it - worse than no names, because it reads as complete. Proven red by deleting one Add call from the Lite runner. Skipping the database is still correct. Skipping it quietly is not. Fixes #2623. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// all-failed case rethrows the first failure so the run is classified as an error, and a note on a | ||
| /// row about to be marked ERROR would only compete with the error message. | ||
| /// </summary> | ||
| public static string? BuildPartialFailureNote( |
There was a problem hiding this comment.
This closes the hole for the direct-connection per-database loop (RunsPerDatabase — Azure SQL DB / PostgreSQL), but the identical hole survives in the shared on-prem enumeration path, EnumeratedCollectorDriver.RunAsync (below in this file). That loop's onItemError callback — in both DarlingCollectorRunner.cs and RemoteCollectorService.DefinitionRunner.cs's BuildEnumerationQuery/RunAsync branch — logs a warning per failed item and moves on, but nothing tracks attempted-vs-failed counts, EnumeratedRunResult only carries (Rows, SqlMs, StorageMs), and neither host composes a note or rethrows even when every item fails.
So for an on-prem SQL Server collector that enumerates databases (e.g. query_store, index_object_stats — anything using the [db].sys.sp_executesql idiom), a cycle where 29 of 30 databases throw inside their per-item query still logs a bare SUCCESS with whatever the lone survivor produced — the exact shape of #2622/#2623, one level up. That's arguably the more consequential gap, since on-prem SQL Server (not Azure SQL DB/Postgres) is this loop's primary audience.
The PR description's framing — "both runners tolerate a per-database failure by design and escalate only when EVERY database failed" — doesn't actually hold for this path: it never escalates, partial or total. Is RunAsync intentionally out of scope here (a follow-up), or should BuildPartialFailureNote/MergeNotes (and an all-failed rethrow) be wired into it too, given it's the "composed once in the shared driver" location this PR is trying to centralize on?
| /// </para> | ||
| /// | ||
| /// <para> | ||
| /// So the assertions here are about the ABSENCE being explained, not about the failure being prevented. |
There was a problem hiding this comment.
Parity nit: every other test file in this repo pinning per-database-loop / shared-driver behavior has a hand-duplicated Darling.Tests twin, each carrying a cross-reference comment along the lines of "editing one app's copy alone fails a build" — e.g. PerDatabaseProbeFailureTests ↔ DarlingPerDatabaseProbeFailureTests, PayloadProbeFailureTests ↔ DarlingPayloadProbeFailureTests, EmptyEnumerationNoteTests ↔ DarlingEmptyEnumerationNoteTests, BlockedProcessReportUnresolvedReasonTests ↔ Darling... equivalent.
This new file has no Darling.Tests counterpart. BothRunnersComposeThePartialFailureNote does check both runner source files textually from here, but that's weaker than the established pattern — nothing in the Darling.Tests project itself goes red if Darling's side of this drifts, and a future contributor working only in Darling/ has no local signal. Worth adding a Darling/Darling.Tests/DarlingPartialDatabaseFailureNoteTests.cs mirror to match convention.
|
Reviewed. This is a solid, well-tested fix for the specific hole it targets (the direct-connection Azure SQL DB / PostgreSQL per-database loop in Two things worth a look before merge, left as inline comments:
No T-SQL in this diff, so the T-SQL style rules don't apply. No security, correctness, or performance issues found in the changed code itself — |
Fixes #2623.
The hole
Both runners tolerate a per-database failure by design — one offline database must not cost the other twenty-nine — and both escalate only when every database failed:
In between sat a gap with no evidence in it at all. The cycle recorded SUCCESS, whatever the survivors produced, and a note composed solely from enumeration probe failures — which a thrown exception is not.
When the failing database is the only one holding data, that is SUCCESS with zero rows. Zero rows is exactly the shape of a target that genuinely has nothing to report.
Why this one matters
It is how #2622 survived three schema versions. All three collectors broken by the arity bug run per database. All three failed identically in the one database that had data. Only
pg_extension_availabilitysurfaced as an ERROR, and only because it happens to return rows in every database includingpostgres, so all-failed tripped the escalation.The other two logged:
and I spent most of an hour reading them as a target with no large tables. Against a real fleet target with thirty databases, one broken database means 29/30 of the truth reported as an unqualified SUCCESS, indefinitely.
The change
A partial loss now composes a note naming the skipped databases, how many of how many failed, and the first error, merged with any probe-failure note rather than replacing it:
The "survivors ONLY" clause is the point. The row count is the first thing an operator reads off a cycle, and on a partial loss it is a number about a subset wearing the shape of a number about the server.
Names, unlike the probe-failure note which carries only a count: a probe failure is usually one login problem repeated across every database, where names add nothing, while this is usually a few specific databases and the name is the whole lead. Capped at three plus a count for when it isn't.
All-failed still composes nothing — that path rethrows so the run gets classified, and a note would only compete with the error message.
The guard
Composed once in the shared driver, called from both runners, and pinned by a test that counts the arms: every
catchthat increments the failure count must also name the database, or the note reports a count with the wrong set of names beside it — worse than no names, because it reads as complete.Proven red by deleting one
Addcall from the Lite runner:Shared rather than duplicated because these two have already drifted apart on this exact loop once — Darling carried its own copy of the single-database fallback predicate, which is how #857's fix missed it.
Skipping the database is still correct. Skipping it quietly is not.