Bound pg_index_bloat by bytes and classify the deadline it dies on

[erikdarlingdata]

pg_index_bloat has produced zero rows fleet-wide since it shipped. On the one target with pgstatindex installed, all eleven runs are status=ERROR, rows=0, Exception while reading from stream — including every run after #2618's command-timeout override landed. This bounds the work by bytes, gives the deadline a readable classification, and stops the ERROR arms from recording a literal zero where the duration goes.

Part of #2997.

The 300-second reading, and what the store can and cannot confirm

The issue's evidence points at a timeout at the 300-second override rather than a network fault, and source agrees: CommandTimeoutSecondsOverride => 300 is present, wired (DarlingCollectorRunner.cs:1012 honours it on the per-database path), and pinned. Npgsql's CommandTimeout is a socket read timeout that every backend message restarts, so it bounds backend silence — and pgstatindex is silent for the whole scan, so it gets no reprieve.

The failure durations cannot be read from the store rows, and that is the third defect rather than a gap in the investigation. RunOneAsync's general catch called LogCollectionAsync(..., "ERROR", 0, 0, 0, ...), and duration_ms is written as sqlMs + storageMs. So all eleven rows carry duration_ms = 0, sql_duration_ms = 0, rows_collected = 0: the measurement that would distinguish a 300-second death from an instantly-refused connection was destroyed at write time. The only surviving evidence for the duration is the indirect one the issue already reports — the 316-second sweep freeze, measured from other collectors' gaps. That is consistent with a single database hitting the deadline, and after this change the same run records its own elapsed time directly.

One amplification worth stating: because RunsPerDatabase is true and the per-database loop counts a failure and continues, a target where several databases have the extension pays the deadline per database, and only rethrows once all of them have failed. The 316-second observation implies one attempted database on that target; it is not the ceiling.

What changed

1. The budget bounds bytes, not just a count. 200 indexes is a count-based budget on a per-page cost. It bounds pages only where count correlates with bytes, and the one target with the extension is the counterexample: 200 sub-ceiling indexes admit 286 GB in a single LEFT JOIN LATERAL. The count budget stays for the legibility argument at :176 — an operator can predict "the 200 biggest" — and a cumulative sub-ceiling byte budget now sits beside it, gated on the LATERAL rather than on skipped_reason alone. Gating only the label is the shape that shipped: correct text on a statement that reads everything anyway.

Charging only sub-ceiling indexes to the running total is load-bearing, not tidiness. An over-ceiling index is never handed to pgstatindex, so it costs no pages; on that target the three over-ceiling indexes total 71 GB against a 20 GB budget, so an unfiltered running total would exhaust the allowance before the first measurable index and return zero measurements for a second, entirely new reason.

The 20 GB figure is deliberately conservative and the doc comment says why: the only hard datum is negative — 286 GB did not finish in 300 s, so effective throughput is below 0.95 GB/s and the true value is unknown, because a cut-off run bounds only its own duration. The :61 "a few seconds of I/O" estimate was never measured and the live failure bounds it well away from that. What would justify raising it is a SUCCESS row's own sql_duration_ms — which did not exist before item 3 below, and now does.

2. The deadline is classified. #2618 wanted "a classified timeout rather than a dropped connection" and could not get one: PostgresFaultOutcome takes a PostgresException, and a client-side deadline is an NpgsqlException wrapping a TimeoutException with no SQLSTATE. The gap was the parameter type, so no amount of widening the SQLSTATE map could ever have reached it. A dedicated arm now catches it and composes an authored sentence naming the collector, the database, the mechanism, and the measured elapsed time, and it distinguishes the two deadlines that both classify as CommandTimeout — SQLSTATE 57014 is the target's own statement_timeout and is fixed there; the client-side one is ours.

Two deliberate narrowings against the issue's suggested shape:

• The status stays ERROR. The store has five statuses and none means "ran out of time". PERMISSIONS is the non-fatal-degradation bucket and would exclude this from the error counts, the health band and the collection-failure self-alerts — which is exactly where a collector that has never once succeeded belongs. What changes is that the row is readable, not which bucket it lands in.
• ConnectionFatal is left with the general catch, which is where the reprobe lives. Classifying it in the new arm would have taken server.Runtime = null away from it and re-created the bug that arm exists to fix. The provider already classifies a deadline as CommandTimeout rather than ConnectionFatal so a slow statement cannot force a reconnect storm, and the new arm preserves that by not reprobing.

3. Both ERROR arms record real elapsed time. A stopwatch read only by the fault arms; the success path keeps reporting the runner's own measured sql/storage split, because a wall clock including dispatch overhead would quietly widen three published numbers to satisfy a log line.

4. The message names the database that actually faulted. Found in review, and it is the sharpest defect on the branch. ServerRuntime.ConnectedDatabase is init-only and stamped once during the initial connect-and-probe, from whatever database the probe landed on. A RunsPerDatabase collector never uses that connection — the per-database loop opens one per database — and when every database fails it rethrows the first failure bare. So both PostgreSQL fault arms named a database that had nothing to do with the fault, with full confidence. Seven collectors fan out that way, pg_index_bloat among them, which made it wrong for exactly the collector and target this PR is about.

Both arms are fixed, not just the new one. The ObjectMissing arm has had the same defect since #2638 — the arm whose entire purpose is stopping an operator from checking the wrong database, telling them to CREATE EXTENSION in the probe's database on every permission-degraded target. Repairing the new arm and filing that one would have been shipping half a fix for a single defect.

The name rides on Exception.Data rather than a wrapper exception because every arm of PostgresTargetProvider.Classify keys on the exception's type: wrapping would change the type all of those checks see and silently re-route the fault to a different handler in order to improve a sentence. StampingDoesNotChangeHowTheFaultClassifies pins that. Reading is total, so the collectors that do not fan out behave exactly as before.

5. Only an Npgsql fault earns the Npgsql narrative. Also from review. Classify's first branch answers CommandTimeout for any TimeoutException, and EnumeratedCollectorDriver.ItemBudgetException throws a bare one for the in-process per-item wall-clock budget — a cut the service makes on itself, which never reached the database and involves no Npgsql read. The arm would have told an operator that Npgsql cancelled a read mid-stream about a budget we abandoned ourselves. Latent, because no PostgreSQL collector declares PerItemWallClockBudget — but this branch already stamps that same budgetFailure with its database name, so the two mechanisms are one edit from meeting.

The filter now carries ex is NpgsqlException. Verified against the shipped assembly rather than assumed: Npgsql 10.0.3 gives PostgresException : NpgsqlException = True, so SQLSTATE 57014 still qualifies, while the bare TimeoutException does not and falls to the general catch — which classifies it CommandTimeout too, so it still forces no reprobe, and now reports the budget's own message with a real duration.

Out of scope, and why

The sweep freeze is not addressed directly, deliberately. 300 s of a 60,000 ms sweep budget blocking every 1-minute collector is real, and the mechanism that would fix it structurally is detaching the collector the way #2717 does for plan_correction. That is an independent architectural change with its own gate and its own risk, and it would be riding a fix whose entire purpose is that the run no longer takes 300 seconds. If the byte budget works there is nothing left to detach around; if it does not, the freeze is the symptom that says so, and detaching would have hidden it. Worth revisiting only if a bounded run still shows up in the sweep.

Largest-first is deterministic, so the measured set never rotates. That is pre-existing from #2617, but the byte budget makes it sharper: on a target with 286 GB of sub-ceiling index, a 20 GB cycle measures roughly the same top handful every day and the tail is never measured at all — honestly labelled, but never measured. Not fixed here because rotation is a third design axis the issue does not raise and it would need its own state.

Two shared source pins move, and one of them collides with another lane

The new catch arm is another early-return fault arm, so it passes drain: null and fetchPhases: null for exactly the reason the other seven do — nothing was drained, no item completed. CollectionLogDrainForensicsStoreTests.ThePeerMarkIsCapturedAtDispatchNotAtCompletion counts both, and the count is the point of that pin ("counted rather than merely present"), so it is bumped 7 to 8 rather than loosened. The test's other assertions all still pass unchanged, including DoesNotContain("sweepPeerMaxMs: null") — the new arm carries the body's mark like every other arm.

This collision has since landed and been resolved at nine. fix/2993 merged as #3001, and the merge conflicted on exactly this literal: both lanes added one early-return fault arm and each had independently moved the pin from 7 to 8. Both arms belong in the count, so the merged file holds nine and the pin now says nine (028337e). The conflict is the pin doing its job: neither lane could see the other, and without a counted assertion the number would simply have been wrong in whichever merged second. Nothing here touched their branch.

StartupFailureTriageTests.TheThreeSitesHaveDistinctBudgetVariables is a different case and was narrowed rather than bumped. It asserted Equal(3, CountOf(source, "System.Diagnostics.Stopwatch.StartNew();")) as a proxy for "exactly three triaged retry sites". Every retry budget in that file is a stopwatch, but not every stopwatch is a retry budget, so a per-run clock added to the collector fault arms — hundreds of lines and one concern away from startup triage — turned that pin red about something it has no opinion on. Bumping it to 4 would have pinned "there are exactly four stopwatches in DarlingWorker.cs", which is a meaningless coupling that the next unrelated timing breaks again. It now counts "RetryBudget = System.Diagnostics.Stopwatch.StartNew();", the suffix all three sites share, which is what its own sentence already claimed to measure and still forces a fourth retry site through it.

Review dispositions

Two findings on the first pass, both fixed in bbb2453 and both replied to in-thread and resolved:

• The timeout message named a knob most collectors do not have. A real defect, not a wording preference: the arm's filter is engine-plus-classification, so it fires for every Postgres collector that times out, and only pg_index_bloat and pg_buffer_usage set CommandTimeoutSecondsOverride — the rest fall back to DarlingCollectorRunner.CommandTimeoutSeconds. It now says "its command timeout" and a pin asserts the override's name appears in neither branch of the explanation. Reinstating the old wording turns TheTimeoutExplanationDistinguishesTheServersDeadlineFromOurs red; verified.
• A comment named a test that was never written (CycleByteBudgetIsThePinnedConstant, a draft name). Corrected to TheBudgetLiterals_AgreeWithTheirConstants. Not treated as a nit: a comment pointing at a pin is worth writing only if the pin is findable, and a wrong name reads as authoritative.

Third pass asked whether CycleMeasureBudgetBytes and MeasureCeilingBytes both being 20 GB was deliberate, given each doc comment argues its number independently. Asking it produced something better than a confirmation, in 1249648: the equality is a floor, for a reason neither comment gave. A cycle budget below the per-index ceiling opens a band in which an index can never be measured at all — under the ceiling so it earns no "too large" reason, yet alone exceeding the whole cycle so it is over budget on its own first row every run, carrying not measured this cycle (work budget) in perpetuity. That is a reason whose entire claim is that the index is deferred, attached to one that is permanently skipped: the unmeasured-versus-healthy confusion skipped_reason exists to prevent, one level up. Now pinned by TheCycleBudget_IsNeverBelowThePerIndexCeiling as an inequality, since raising the budget above the ceiling is legitimate tuning once a SUCCESS row supplies a duration; only the floor is load-bearing. Dropping the constant to 10 GB turns it red with the reason in the failure message.

The same review named a real coverage consequence, now recorded on the constant as accepted: at equality a single index just under the ceiling consumes almost the whole cycle, and on this target the largest sub-ceiling index is 19.68 GB, so that run measures essentially one index rather than the ~14 the average implies. Correct for a largest-first budget, and it sharpens the rotation limitation named above.

Later passes raised the ROWS versus default RANGE frame, which caught a false statement in one of my own test doc comments and then a second, opposite false statement in the first correction. The comment had claimed an unframed window sum returns the whole total on every row; that holds only without an ORDER BY, since with one the default frame is RANGE, a running total that groups peers. My first fix then asserted a tie would admit more bytes. Measuring settled it, in 4ed8278: on PostgreSQL 17.11 with a real tie — two schemas holding an index of the same name and the same 1,138,688 bytes, which is reachable because index_name is unique per schema rather than per database — ROWS gives 1,138,688 then 2,277,376 while the default RANGE gives 2,277,376 on both rows. So RANGE charges the tie's entire total to its first member and the pair goes all-or-nothing against the budget, skipped together where one would have fitted. It under-admits. The explicit ROWS frame was already correct and is pinned by TheByteBudget_AccumulatesLargestFirst; only the explanation was wrong, twice, and the measured numbers are in it now rather than a direction I reasoned to.

Second pass found nothing to block on and one non-blocking nit: the new filter hardcodes yieldsOnLockTimeout: false while the PostgresException arm above passes the collector's real value. Kept false, and said why in the code (4fcd14d) rather than only in a thread. The flag only decides the 55P03 branch inside Classify, so it cannot change a CommandTimeout answer; passing the real value would imply the yield question bears on this filter. Both plain-Exception filters in that method pass false for that reason — the pre-existing general catch included — and it is the SQLSTATE arm that is the exception, because it dispatches on the whole fault map.

Verification

Everything below ran on macOS; the Windows-only suites cannot execute here, so the real test files were compiled into throwaway net10.0 xunit v3 console hosts staged outside the repo with the required <AssemblyName> and <Compile Include> pointing at the real repo paths.

• Lite.Tests host, focused: 16 tests, 0 failed (11 pre-existing + 5 new).
• Darling.Tests host, expanded to every one of the 25 test files that source-scan DarlingWorker.cs plus their helpers: 432 tests, 0 failed, 9 skipped. The only failures were 7 in DarlingServiceInstallLocationTests, which asserts Windows path semantics (Expected "C:/Users" / Actual "C:\\/Users") and passes on the Windows runner — a macOS artifact, and CI confirms it.
• CollectionLogDrainForensicsStoreTests and DarlingEmptyEnumerationNoteTests reference the WPF Viewer, so they cannot compile into a net10.0 host that actually runs on macOS. Its eight assertions were instead evaluated directly against the real DarlingWorker.cs — the same file it reads through ReadSource — and all eight pass, including the two counts at 8. CI is the arbiter for the test itself.

Red proven per mutation, one at a time, each with the anchor count and the source hash, judged by which assertion failed:

Mutation	Result
Drop the byte term from the LATERAL ON	TheCycleBudget_BoundsBytes_NotJustIndexCount, TheBudgetLiterals_AgreeWithTheirConstants
Drop FILTER (WHERE index_bytes < ceiling)	TheByteBudget_ChargesOnlyTheIndexesItWillActuallyRead, TheBudgetLiterals_AgreeWithTheirConstants
Restore "ERROR", 0, 0, 0 on the general arm	TheErrorArmsRecordRealElapsedTimeRatherThanLiteralZeros
Zero the timeout arm's elapsed	TheErrorArmsRecordRealElapsedTimeRatherThanLiteralZeros
Collapse the authored explanation	TheTimeoutExplanationNamesTheCollectorTheDatabaseAndTheMeasuredElapsedTime, TheTimeoutExplanationDistinguishesTheServersDeadlineFromOurs
Comment-only control	0 failed — source hash and assembly hash both moved, which is why neither is behaviour evidence

The assertion that fails if the byte budget is reverted is TheCycleBudget_BoundsBytes_NotJustIndexCount, which requires the byte term inside the LATERAL ON and not merely the column's presence.

Live SQL, both majors the fleet runs. The shipped string was dumped from BuildQuery(ctx).Text — never a retyped copy — and run verbatim against real PostgreSQL 17.11 and 16.15 in containers with pgstattuple installed, against btree, GIN, BRIN and hash indexes. Identical results on both: three btree indexes measured, the non-btree ones never reaching the function, avg_leaf_density ~90 on freshly built indexes as the type header claims. This matters because CI never parses a collector query — the Darling PostgreSQL tests job exercises the store, not a target — so sum(...) FILTER (...) OVER (...) had no other route to being proven to parse.

The budget was then forced to bind, by substitution into that same shipped string, with a scenario built to discriminate the FILTER rather than merely exercise it: with the ceiling lowered so the largest index is over it, the two smaller indexes are both measured, which is only possible if the over-ceiling one was not charged. The negative control — the identical probe with the FILTER removed and nothing else changed — produces the predicted wrong answer, skipping both and measuring nothing.

CI, and one failure that is not this branch

First run on 4bbddf6: Lite.Tests 3,271 tests, 0 failed, 0 skipped and Dashboard.Tests 782, 0 failed both green. Darling.Tests failed on the two source pins above, now fixed; Darling PostgreSQL tests went green at 236s on bbb2453 once they were, which is the same suite plus a live store.

The new tests are confirmed to have executed, by count rather than by name. The last green dev run (33956294976) reports Lite.Tests 3,266 and Darling.Tests 7,490; this branch reports 3,271 and 7,496 — exactly +5 and +6, which is exactly what was added, with Skipped: 0 on Lite and no new entry in Darling's skip list. A name grep is not available here: under Microsoft.Testing.Platform the runner prints no per-test Passed lines, so the delta plus the skip counts is the evidence.

The live-store job additionally failed QueryStoreCorrectedRollupLiveTests.RetentionSweep_TreatsAFailedProbeAsUnknown_NeverAsACoverageRegression with InvalidOperationException: Connection is not open from IsArmedAsync. That is not this branch: nothing here touches the Query Store rollup or its retention sweep, the failure is a dropped store connection rather than an assertion, and the same test does not fail in the Windows build job, where it is skipped for want of a live store. Treated as a flake pending the re-run on this head; if it reproduces it belongs to whoever owns that sweep, not to this change.

Not verified

The pgmon store could not be read this session (no pgmon MCP in this session's tool set, and AWS CLI calls were blocked in this environment), so the eleven rows are taken from the issue rather than re-read, and the duration_ms = 0 claim is established from source rather than from the rows. Nothing here has run against the affected target; the observation that decides is its next scheduled attempt.

When the fix can first be observed

The affected target's next run tonight executes the currently-deployed build, not this branch. The deciding observation is the first daily run after the next nightly reaches the box. Tonight's expected failure is not evidence that this fix did not work.

[claude]

Review summary

Reviewed the byte-budget gate on pg_index_bloat and the Darling-side timeout classification/observability fix for #2997.

Correctness — SQL (PgIndexBloatCollector.cs): the new measured_bytes_through_here running-total window (filtered, coalesced, ordered largest-first) and the <= gate on the LEFT JOIN LATERAL are sound — the FILTER/coalesce handles the "over-ceiling rows sort first and contribute NULL" case correctly, the CASE ladder's precedence (ceiling → byte budget → count budget) matches what the LATERAL's ON clause actually excludes, and the literal/constant pairs are pinned by tests (TheBudgetLiterals_AgreeWithTheirConstants). No injection risk (all budget values are compile-time constants).

Correctness — Darling runner (DarlingWorker.cs): the new CommandTimeout catch is correctly positioned after the PostgresException arm (so SQLSTATE 57014 — server-side statement_timeout — still gets classified as ERROR there first and only reaches this arm via the serverCancelled re-check), correctly excludes SQL Server targets via the Engine == PostgreSql guard, and correctly avoids forcing a reconnect (matches the pre-existing ConnectionFatal-only reprobe condition). The stopwatch-based elapsed time now flows into sqlMs for both the new timeout arm and the general catch, fixing the three-literal-zero problem described in the PR body — verified against LogCollectionAsync's duration_ms = sqlMs + storageMs.

One correctness finding (inline): PostgresTimeoutExplanation's client-side message unconditionally blames "the collector's CommandTimeoutSecondsOverride," but the catch that invokes it fires for any Postgres collector classified as CommandTimeout — and only PgIndexBloatCollector/PgBufferUsageCollector actually set that override; every other Postgres collector uses the 60s default. A real timeout on one of those ~20 other collectors would get a diagnostic message that names a mechanism that isn't in play for it.

One minor nit (inline): a comment in PgIndexBloatCollector.cs references a test name that doesn't exist (CycleByteBudgetIsThePinnedConstant vs. the actual TheBudgetLiterals_AgreeWithTheirConstants).

Lite/Darling parity: PgIndexBloatCollector lives in the shared PerformanceMonitor.Collectors library, so the byte-budget SQL change applies uniformly to both apps automatically. Confirmed Lite has no runtime path that opens NpgsqlConnection/dispatches Postgres collectors against a live monitored target (RemoteCollectorService.cs is SQL-Server-DMV-only), so the DarlingWorker.cs timeout-classification/elapsed-time changes have no Lite counterpart to drift from — this is legitimately Darling-only functionality, not a parity gap.

No missing-index DMV suggestions, no security issues, no performance regressions identified (the running-total window is negligible next to the actual pgstatindex page reads it's bounding).

[claude]

Reviewed the diff (PgIndexBloatCollector.cs, DarlingWorker.cs, and the three test files). This is thorough and well-verified — walked through the logic in detail and didn't find correctness, parity, or security issues to block on.

Byte budget (PgIndexBloatCollector.cs)

• The over-ceiling vs. sub-ceiling boundary is consistent everywhere: the FILTER (WHERE k.index_bytes < ceiling), the LEFT JOIN LATERAL ... ON k.index_bytes < ceiling, and the >= ceiling "too large" label all agree.
• The running-sum/budget boundary is also consistent: rows join when measured_bytes_through_here <= budget, and are labeled skipped when > budget — complementary, no off-by-one.
• Verified algebraically that over-ceiling indexes always sort before all sub-ceiling ones under ORDER BY index_bytes DESC (since every over-ceiling value ≥ 20 GB > every sub-ceiling value), so the coalesce(..., 0) for the NULL-until-first-match window frame can't accidentally let an over-ceiling row slip under the byte budget.
• CycleMeasureBudgetBytes (public const) and CycleByteBudgetLiteral (the string spliced into SQL) agree (both 21474836480), and it's pinned by TheBudgetLiterals_AgreeWithTheirConstants so they can't drift silently.
• No OPTION(RECOMPILE) here, correctly — that's a T-SQL/SQL Server convention and no other Pg*Collector in this codebase uses it either.

Timeout classification (DarlingWorker.cs)

• Catch-order reasoning checks out: the new catch (Exception ex) when (... Classify(ex) == CommandTimeout) sits after the PostgresException arm (which declines SQLSTATE 57014 back to "ERROR", since PostgresFaultOutcome's default case doesn't special-case it), so both the client-side TimeoutException-wrapped case and the server-side 57014 case correctly land in the new arm and get told apart via serverCancelled.
• Confirmed against PostgresTargetProvider.Classify: 57014 and any wrapped TimeoutException both map to CommandTimeout, matching the new filter.
• The runClock stopwatch is declared before the try, so it's in scope for every catch arm; both LogCollectionAsync calls that used to hardcode 0, 0, 0 now pass a real elapsed value in the sqlMs slot (duration_ms = sqlMs + storageMs), verified no "ERROR", 0, 0, 0 call sites remain.
• No reprobe on the new arm — correctly leaves ConnectionFatal handling untouched in the general catch, so a slow statement still can't trigger a reconnect storm.

Parity — pg_index_bloat and the Postgres timeout-classification path are Darling-only (Lite has no PostgreSql target engine wiring in RemoteCollectorService); Lite.Tests/PgIndexBloatCollectorDefinitionTests.cs is just the shared PerformanceMonitor.Collectors library's existing test home, not evidence Lite runs this collector. No drift between the two apps here.

Minor, non-blocking nit: the new catch's filter hardcodes Classify(ex, yieldsOnLockTimeout: false), while the sibling PostgresException arm above it passes CollectorCatalog.YieldsOnLockTimeout(collectorName). It happens to be inert today — yieldsOnLockTimeout only affects the 55P03 branch, never the CommandTimeout branch this filter checks — but it reads like it should match its neighbor. Not asking for a change, just flagging in case it's an oversight rather than a deliberate simplification.

Nice test coverage on the tricky part (the FILTER/window-function interaction) via TheByteBudget_ChargesOnlyTheIndexesItWillActuallyRead and TheByteBudget_AccumulatesLargestFirst.

[claude]

Reviewed the diff (DarlingWorker.cs's new command-timeout catch arm + elapsed-time fix, and PgIndexBloatCollector.cs's byte-budget CTE) against CONTRIBUTING.md's conventions and for Lite/Darling parity. Summary:

Correctness — solid. Traced the SQLSTATE/exception-classification path end to end:

• PostgresFaultOutcome's default arm returns ("ERROR", ...) for a 57014 PostgresException, so the new catch-when (which requires Status: not "ERROR") correctly declines it and lets it fall through to the new timeout arm, which then classifies it via PostgresTargetProvider.Classify (→ CommandTimeout) and sets serverCancelled = true via the SqlState: "57014" pattern match. A client-side deadline (bare NpgsqlException wrapping TimeoutException, no SQLSTATE) takes the same arm with serverCancelled = false. Both message branches verified against their pinned test assertions.
• runClock is scoped per-RunOneAsync-call (declared once, above the try), so no cross-run bleed; the general catch's ConnectionFatal reprobe logic is untouched and the new arm deliberately doesn't reprobe (verified against TheTimeoutArmDoesNotStealTheReprobeFromConnectionFatal).
• LogCollectionAsync's parameter order confirms elapsedMs lands in the sqlMs slot, so duration_ms = sqlMs + storageMs now records real elapsed time instead of a literal zero on both ERROR arms.
• The new byte-budget CTE in PgIndexBloatCollector.cs: verified the FILTER (WHERE index_bytes < ceiling) / LEFT JOIN LATERAL ... ON ... / CASE WHEN boundary conditions are all consistent (same </>= cutpoints throughout), so there's no state where a row is measured by the LATERAL but still gets a non-null skipped_reason, or vice versa. coalesce(..., 0) correctly handles the NULL <= budget trap called out in the comment.

Lite/Darling parity — checked, no drift.

• PgIndexBloatCollector lives in the shared PerformanceMonitor.Collectors library, so the byte-budget SQL change applies identically to both apps by construction. Lite/Mcp/McpEngineCapability.cs confirms Lite has no PostgreSQL target seam at all (CollectorTargetEngine.PostgreSql is never set in that SKU), so this collector's Postgres path is Darling-exclusive in practice — nothing to mirror on the Lite side.
• Checked whether Lite's collector runner (Lite/Services/RemoteCollectorService.cs) has the same zero-duration-on-ERROR bug Darling had. It doesn't: Lite's LogCollectionAsync computes durationMs independently as (DateTime.UtcNow - startTime).TotalMilliseconds in one shared call reached by every catch arm, rather than deriving it from sqlMs + storageMs the way Darling's did. No parity fix needed there.

One nit posted inline on PgIndexBloatCollector.cs:83 — CycleMeasureBudgetBytes and MeasureCeilingBytes are numerically identical (both 20 GiB, same literal string), which their respective doc comments each argue for independently. Not a bug, but worth confirming it's deliberate given the edge case it creates (a single near-ceiling index could consume nearly the whole per-cycle budget on some target).

No security, injection, or SQL-Server-missing-index concerns. All SQL is compile-time literals/constants (no dynamic string building from user input). Test coverage for this change is unusually thorough and the PR description already documents two rounds of self-review with mutation-style verification — nothing else stood out.

[claude]

Reviewed the diff in depth (SQL in PgIndexBloatCollector.cs, the new catch arm + PostgresTimeoutExplanation in DarlingWorker.cs, and the accompanying tests). No correctness, security, or Lite/Darling parity issues found. Notes from what I specifically checked:

Catch-arm ordering (DarlingWorker.cs) — traced both deadline paths by hand against PostgresTargetProvider.Classify:

• SQLSTATE 57014 (server-side statement_timeout) hits the catch (PostgresException ex) when (PostgresFaultOutcome(...) is { Status: not "ERROR" }) arm first, but since CommandTimeout maps to ("ERROR", ...) in PostgresFaultOutcome, that when filter is false and it correctly falls through to the new arm, where ex is PostgresException { SqlState: "57014" } is true → serverCancelled = true.
• The client-side deadline (NpgsqlException wrapping TimeoutException) never matches the PostgresException type at all, so it skips straight to the new arm, and serverCancelled is correctly false.
• ConnectionFatal is untouched by the new arm and still falls to the general catch, so the reprobe-forcing logic there is preserved. yieldsOnLockTimeout: false in the new filter is confirmed harmless — that flag only gates the 55P03 branch inside Classify, never CommandTimeout.

Byte-budget SQL (PgIndexBloatCollector.cs) — verified the coalesce(... , 0) handles the case where the window's FILTER produces NULL (before any sub-ceiling row has been seen, since over-ceiling indexes sort first under DESC), and that over-ceiling rows still get correctly excluded from the LATERAL regardless of the coalesced 0, because the pre-existing k.index_bytes < ceiling term in the same ON already blocks them. The <=/> boundary is consistent between the LATERAL ON gate and the CASE reason text. Counted the literal occurrences the new tests pin against (drain: null/fetchPhases: null → 8, RetryBudget = ...StartNew() → 3, the two "ERROR", 0, <elapsed>, 0 call sites) and all match the source exactly.

Parity — DarlingWorker.cs has no Lite counterpart to update: Lite's RemoteCollectorService only talks to SQL Server and has no ITargetProvider/PostgresTargetProvider classification path at all, so there's nothing to keep in sync there. PgIndexBloatCollector.cs lives in the shared PerformanceMonitor.Collectors project, so both apps pick up the byte budget automatically from one source. Lite.Tests/PgIndexBloatCollectorDefinitionTests.cs being the home for these SQL-shape tests matches the existing convention for every other collector (e.g. IndexObjectStatsCollectorDefinitionTests.cs, PgBufferUsageCollectorDefinitionTests.cs), not a parity gap.

One non-blocking observation, already called out explicitly in the PR description so not a new finding: with CycleMeasureBudgetBytes == MeasureCeilingBytes (both 20 GB), a single sub-ceiling index near the ceiling will consume nearly the whole cycle budget on its own, so in practice most cycles on a large-index fleet will measure ~1 index rather than up to 200. The PR already documents this as an accepted consequence pending measured-set rotation, so just flagging for visibility rather than asking for a change.

[claude]

Reviewed the diff against CONTRIBUTING.md (T-SQL style rules don't directly apply here since this is PostgreSQL collector SQL, but checked the Postgres-specific conventions: naive-UTC timestamps N/A, no migration to schema-qualify, no DateTimeKind binding here).

SQL logic (PgIndexBloatCollector.cs) checks out:

• The FILTER (WHERE k.index_bytes < CeilingLiteral) / k.index_bytes >= CeilingLiteral boundary is consistent between the CASE labelling and the LATERAL join gate — no off-by-one at exactly 20 GB.
• The coalesce(..., 0) is load-bearing as documented: over-ceiling rows sort first under ORDER BY index_bytes DESC, so the FILTERed window sum is genuinely NULL for them until a sub-ceiling row appears in the frame; without the coalesce, NULL <= budget would leave the LATERAL gate neither open nor closed.
• ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW (not the default RANGE frame) is the right choice given index_name is in the ORDER BY only as a tiebreaker — RANGE would let tied peers share one cumulative value.
• Priority order in the CASE (ceiling → byte budget → count budget) matches the priority implied by the triple-AND LATERAL gate, so a row's reported skipped_reason can't disagree with whether it was actually measured.
• CycleMeasureBudgetBytes >= MeasureCeilingBytes is enforced only by a unit test, not the type system — worth being aware that nothing stops someone from re-widening the SQL literals independently of the C# constants (aside from TheBudgetLiterals_AgreeWithTheirConstants), but that's already covered by the tests added here.

DarlingWorker.cs catch-arm ordering: traced through PostgresTargetProvider.Classify — confirmed yieldsOnLockTimeout only gates the 55P03 branch, so hardcoding false in the two plain-Exception filters genuinely cannot change a CommandTimeout verdict, matching the code comment. Confirmed the only two paths that produce CollectorTargetFault.CommandTimeout are (a) an NpgsqlException/bare TimeoutException and (b) a PostgresException with SQLSTATE 57014, so serverCancelled = ex is PostgresException { SqlState: "57014" } in the new arm is exhaustive — no third case falls into the wrong branch. The new arm sitting after the PostgresException arm and before the general catch is correct: a 57014 PostgresException is declined by the first arm's Status: not "ERROR" filter (since PostgresFaultOutcome defaults 57014 to ("ERROR", ...)), and correctly falls through instead of being caught twice.

Parity: PgIndexBloatCollector lives in the shared PerformanceMonitor.Collectors library, so the SQL change applies identically to both apps. The DarlingWorker.cs fault-classification/timing changes have no Lite counterpart to update — grepped and confirmed Lite never dispatches pg_index_bloat (PostgreSQL collectors appear to be Darling-only), so this isn't a parity gap.

Test pins: verified independently — drain: null / fetchPhases: null are each at exactly 8 occurrences, RetryBudget = System.Diagnostics.Stopwatch.StartNew(); is at exactly 3, and the literal collectorName, "ERROR", 0, pattern (elapsedMs or runClock.ElapsedMilliseconds) is at exactly 2 — all match what the updated assertions expect.

No correctness, security, or parity issues found. Nice catch on the NpgsqlException-wrapping-TimeoutException gap — that really was unreachable through the SQLSTATE map no matter how it was widened.

[claude]

Reviewed the diff. One correctness finding posted inline on DarlingWorker.cs (call site around line 6332): the new PostgreSQL command-timeout explanation names runtime.ConnectedDatabase, which is stamped once at initial server probe and never updated — but pg_index_bloat and the other six RunsPerDatabase => true collectors open a fresh connection per database inside DarlingCollectorRunner's per-database loop and lose the actual databaseName by the time the rethrown exception reaches this catch. So for exactly the collector and fleet target this PR's investigation is built around, the authored sentence will report the wrong database. None of the new tests catch it because they call PostgresTimeoutExplanation directly with a hand-supplied database name rather than exercising the real per-database wiring.

Everything else checked out:

• The SQL (window-function byte budget, FILTER/coalesce handling of over-ceiling rows, ROWS vs default RANGE framing, CASE/LATERAL gating order) is internally consistent and the constants agree with their C# counterparts.
• runClock/elapsedMs wiring for the two ERROR-arm LogCollectionAsync calls is correct and no stray "ERROR", 0, 0, 0 call sites remain.
• Catch-arm ordering (new CommandTimeout arm after the PostgresException/status-not-ERROR arm, before the general catch) is sound, and it correctly avoids stealing the reprobe from ConnectionFatal.
• No Lite/Darling parity gap: Lite has no PostgreSQL-target code path at all (SQL Server + DuckDB only) and its LogCollectionAsync already records real elapsed time on every status, so there's nothing for this PR to mirror there.
• No missing-index DMV suggestions, no injection surface (static query text, no user input concatenated).

Worth noting the PR description itself already discloses a related, secondary wrinkle — that the recorded elapsed time can be a sum across multiple per-database timeout attempts rather than a single command's deadline — so I haven't re-flagged that separately.

[claude]

Reviewed the diff against CONTRIBUTING.md's SQL/parity/security checklist. Summary: this is a well-scoped, thoroughly self-tested change and I didn't find correctness bugs.

Correctness (SQL) — Traced the new byte-budget window carefully:

• coalesce(..., 0) on the FILTERed window sum is necessary and correct: over-ceiling indexes sort first under ORDER BY index_bytes DESC, so the filtered sum is NULL until the first sub-ceiling row, and NULL <= budget would otherwise evaluate NULL (neither open nor closed).
• CASE ordering (ceiling → byte budget → count budget) and the LATERAL ... ON gate (index_bytes < ceiling AND size_rank <= 200 AND measured_bytes_through_here <= budget) agree with each other, and with the reasons reported in skipped_reason.
• CycleMeasureBudgetBytes == MeasureCeilingBytes (both 20 GiB) is deliberate and pinned as a floor (TheCycleBudget_IsNeverBelowThePerIndexCeiling), with the "measures ~1 index per cycle" consequence documented and accepted. Verified the two SQL literals (CeilingLiteral, CycleByteBudgetLiteral) match their C# constants byte-for-byte.

Correctness (C#) — Walked the new catch (Exception ex) when (... == CommandTimeout) arm in DarlingWorker.RunOneAsync against PostgresTargetProvider.Classify:

• SQLSTATE 57014 (server-side statement_timeout) is correctly excluded from the previous PostgresException arm (that arm declines when Status == "ERROR", and 57014 falls to the default ("ERROR", ...) case), so it correctly falls through to the new arm and is identified via ex is PostgresException { SqlState: "57014" }.
• Classify is a pure type/property check with no I/O, so it's safe to call from an exception filter — it won't misclassify OperationCanceledException/TaskCanceledException as a timeout (no TimeoutException in that chain), so cancellation still falls through to the general catch as before.
• runClock is started after the two early-return gates but before the collector actually runs, so it measures what the doc comment claims.
• Regex-pinned literal counts (drain: null / fetchPhases: null = 9, the two "ERROR", 0, ... call sites, the 3 RetryBudget = Stopwatch.StartNew() sites vs. 4 total stopwatches) all match the current source — spot-checked with grep.
• CollectorFaultDatabase.Stamp mutates the same exception instance that firstFailure captures, and the per-database loop rethrows it via ExceptionDispatchInfo.Capture(firstFailure).Throw(), so the stamped Data entry survives the rethrow into RunOneAsync's catch blocks. Confirmed both fault arms now read the database via CollectorFaultDatabase.For(ex, runtime.ConnectedDatabase) rather than the stale ServerRuntime.ConnectedDatabase.

Lite/Darling parity — No drift. PgIndexBloatCollector.cs lives in the shared PerformanceMonitor.Collectors project, so the byte-budget fix applies identically to both apps. The DarlingWorker.cs/DarlingCollectorRunner.cs/CollectorFaultDatabase.cs changes are Darling-only, but that's correct rather than a gap: Lite has no PostgreSQL target seam at all (Lite/Mcp/McpEngineCapability.cs:57-60: "nothing in this SKU ever sets CollectorTargetEngine.PostgreSql"; DuckDbSchemaGenerator.cs filters to TargetEngine == SqlServer only), so pg_index_bloat never actually runs under Lite despite the shared definition class. The Lite.Tests/PgIndexBloatCollectorDefinitionTests.cs additions are appropriately scoped to the shared SQL-text-generation logic, not to a Lite execution path.

Style — SQL follows the file's existing conventions (uppercase keywords, AS-aliased, aligned AND/ON, block comments only, no stray --). T-SQL-specific rules (OPTION(RECOMPILE), etc.) don't apply since this is PostgreSQL.

Minor/non-blocking: the new CycleMeasureBudgetBytes doc comment references <c>:61</c> (a line number) to point at the per-index ceiling's "a few seconds of I/O" estimate. Every other cross-reference in this PR is enforced by a test pin; this one is a bare line number that will silently point at the wrong text the next time someone edits above it. Not worth blocking on, just flagging since it's the one loose thread in an otherwise very tightly pinned PR.

[claude]

Review summary

Reviewed the diff (SQL byte-budget change in PgIndexBloatCollector.cs, the Darling fault-classification/attribution changes in DarlingWorker.cs / DarlingCollectorRunner.cs / the new CollectorFaultDatabase.cs, and the accompanying tests) against CONTRIBUTING.md's style rules and for correctness, Lite/Darling parity, security, and performance.

Lite/Darling parity: No gap. The SQL/budget change lives in PerformanceMonitor.Collectors/PgIndexBloatCollector.cs, which is shared by both apps, so it applies to both automatically. The C# fault-classification/attribution changes are Darling-only, but that's correct rather than a parity miss: Lite has no live PostgreSQL collection path at all (ServerConnection only builds SqlConnections; RemoteCollectorService never dispatches a PostgresCollectorDefinitionBase collector against a target), and Lite's own error-logging path (RemoteCollectorService.LogCollectionAsync) already computes duration from a shared startTime on every status branch and never hardcodes a literal 0, and its per-database fan-out loop uses the loop's own databaseName directly rather than an ambient ServerRuntime-style field — so neither of the two defects this PR fixes (zeroed ERROR duration, wrong-database attribution) exists on the Lite side to begin with.

SQL correctness (PgIndexBloatCollector.cs): Traced the new byte-budget CTE/window/CASE/LATERAL-join logic by hand — the FILTER (WHERE k.index_bytes < ceiling) exclusion of over-ceiling indexes from the running total, the coalesce(..., 0) guard against NULL <= budget, the ROWS (not default RANGE) frame, and the boundary conditions between the CASE branches (> / >=) and the LATERAL ON clause (<= / <) are all complementary with no gaps or overlaps. CeilingLiteral / CycleByteBudgetLiteral string literals agree with the MeasureCeilingBytes / CycleMeasureBudgetBytes C# constants (verified by grep, matching what TheBudgetLiterals_AgreeWithTheirConstants pins). No injection risk — both are compile-time numeric constants, not external input.

C# fault handling (DarlingWorker.cs / DarlingCollectorRunner.cs / CollectorFaultDatabase.cs):

• Catch-arm ordering is correct: the pre-existing catch (OperationCanceledException) at the top of this chain still intercepts shutdown cancellation before it can reach the new filtered arms, so cancellation isn't misrouted into the new timeout-explanation path.
• The new arm sits correctly after the PostgresException arm so SQLSTATE 57014 (server-side statement_timeout) is tried there first and only falls through here when declined, letting serverCancelled distinguish the two deadlines as intended.
• runClock placement, the sqlMs/storageMs argument slots feeding duration_ms = sqlMs + storageMs, and the CollectorFaultDatabase stamp-before-firstFailure ordering in the per-database loop all check out against the pinned tests — I independently grepped the source and the counts (drain: null / fetchPhases: null = 9, "ERROR", 0, 0, 0 = 0 occurrences left, RetryBudget = ...StartNew(); = 3) match what the updated test assertions expect.
• CollectorFaultDatabase rides on Exception.Data rather than wrapping, correctly leaving PostgresTargetProvider.Classify's type-based dispatch untouched; ItemBudgetException returns a fresh TimeoutException per call, so stamping it doesn't risk cross-database/cross-thread contamination.

One finding posted inline: the new CommandTimeout catch arm's filter accepts any TimeoutException (per PostgresTargetProvider.Classify's existing first branch), not specifically an Npgsql-originated one, but PostgresTimeoutExplanation unconditionally authors an Npgsql-specific narrative ("Npgsql cancelled the read mid-stream"). EnumeratedCollectorDriver.ItemBudgetException throws a bare TimeoutException for an unrelated in-process wall-clock budget mechanism; if a future RunsPerDatabase PostgreSQL collector ever adopts PerItemWallClockBudget (today only SQL-Server-engine collectors do), this arm would fabricate an incorrect Npgsql-specific explanation for what is actually a budget-expiry timeout. Not live today, but the arm has no ex is NpgsqlException guard against it.

No missing-index DMV recommendations, no secrets/credentials, no obvious performance regressions — the byte budget is strictly a cost-reduction change.

[claude]

Review

Went through the full diff (PgIndexBloatCollector.cs, the two DarlingWorker.cs/DarlingCollectorRunner.cs fault-handling arms, the new CollectorFaultDatabase.cs, and both test files) against CONTRIBUTING's conventions and the parity/correctness/security checklist. No blocking issues found. Notes below are for the record, not asks.

Byte-budget SQL (PgIndexBloatCollector.cs) — traced the three-way CASE/LATERAL ON correspondence by hand:

• index_bytes >= Ceiling / < Ceiling (too-large reason vs. join eligibility)
• measured_bytes_through_here > CycleByteBudget / <= CycleByteBudget (work-budget reason vs. join eligibility)
• size_rank > 200 / <= 200 (count-budget reason vs. join eligibility)

Each reason condition is the exact complement of its join condition, and the CASE priority order (ceiling → byte budget → count) means a row that fails more than one gate still gets a sensible label. The FILTER (WHERE index_bytes < Ceiling) on the window sum correctly excludes over-ceiling rows from the running total (verified against the stated 71 GB / 20 GB numbers in the doc comment), and coalesce(..., 0) correctly handles the NULL that a FILTERed window sum produces before its frame contains a matching row. ROWS vs. the default RANGE framing is the right call given index_name is only unique per-schema (real tie source) — confirmed the doc comment's measured numbers are self-consistent.

Fault classification (DarlingWorker.cs) — walked the catch-arm ordering by hand against PostgresTargetProvider.Classify:

• SQLSTATE 57014 falls through PostgresFaultOutcome's default arm (_ => ("ERROR", ...)), so the when guard on the first PostgresException arm correctly declines it and lets it reach the new NpgsqlException-typed arm below, which is where serverCancelled gets set correctly.
• The new arm's ex is NpgsqlException guard correctly excludes the bare TimeoutException from EnumeratedCollectorDriver.ItemBudgetException, which still resolves to CommandTimeout via Classify but falls to the general catch — confirmed that path also doesn't force a reconnect (ConnectionFatal check requires Classify(...) == ConnectionFatal, not CommandTimeout).
• CollectorFaultDatabase.Stamp is called before firstFailure ??= ... at both call sites in DarlingCollectorRunner.cs, so the exception that actually gets rethrown is always the stamped one.

Lite/Darling parity — no drift. PgIndexBloatCollector is shared, and its SQL is identical for both apps (Lite.Tests is where collector-definition SQL tests already live for this repo, consistent with existing convention). The fault-classification/CollectorFaultDatabase changes are Darling-only, which is correct rather than a gap: Lite never sets CollectorTargetEngine.PostgreSql at all (per Lite/Mcp/McpEngineCapability.cs), so it has no equivalent code path to keep in parity.

Security — no user input reaches the new SQL (the byte/count literals are compile-time constants baked into both the C# constants and the query text, cross-pinned by TheBudgetLiterals_AgreeWithTheirConstants); no new file/network/process/secret handling.

Nice catches in the review history baked into this branch already (the RANGE-vs-ROWS tie-admission direction, the wrong-database-name defect on both fault arms, and narrowing the new arm to NpgsqlException so it can't claim an Npgsql narrative for a self-imposed budget cut) — traced all three against current source and they hold.