Collapse verb narrows its slice adaptively instead of dying at the timeout

[erikdarlingdata]

#2105 round three (ghauan, same store). With the decompression rail lifted by #2127, their run survived past the old four-minute wall and died ~15 minutes in with the same bare stream exception — which is exactly SliceStatementTimeoutSeconds (900s): a day-wide stage aggregation on a store carrying 60,654 split intervals doesn't fit the statement timeout, and the verb's slice width was a fixed AddDays(1).

The fix

The slice loop now runs the same shared adaptive schedule the Query Store backfill worker shipped this week (QueryStoreBackfillState.AdaptiveSpan, 24h base):

• a failed slice halves the window and retries the same start, announcing itself with a [RETRY] line that names the error — narrowing reads as progress, not a hang;
• a completed slice resets to full width, so healthy stores still repair in a handful of day-wide slices — the narrowing costs nothing until a slice actually fails;
• only a slice that fails at the ~22-minute floor gives up, to the existing idempotent re-run message (now naming the failing slice's start and width).

Raising the timeout instead would be the wrong lever: the slice transaction holds chunk locks the live service's compression jobs also want, and 15 minutes is already generous — the right response to "too big to fit" is a smaller bite, the exact design conclusion #2125 reached for the live path.

Tests

The halving schedule itself is the already-pinned AdaptiveSpan (floor and cap pins shipped with #2125). The verb's happy path stays covered by CollapseVerb_DryRunsWithoutChanging_ThenRepairs_ThenFindsNothingLeft; the retry arm is driven by real statement timeouts that can't be provoked deterministically in a live test without multi-minute CI stalls, so its correctness rides the shared policy's pins plus the loop's structure.

🤖 Generated with Claude Code

[claude]

Retry reuses a connection that may already be dead.

On failure the loop continues and calls CollapseSliceAsync again on the same connection (line 3037-3038). But the failure mode this PR exists to survive is SliceStatementTimeoutSeconds (900s) expiring — and per the doc comment on that constant a few lines up in QueryStoreSliceRepair.cs ("The failure read as 'Exception while reading from stream' ... which is how an Npgsql command timeout surfaces"), that's a bare stream exception, not a clean 57014 canceling statement due to user request. That's the signature of Npgsql's cancel-on-timeout failing to land and force-closing/breaking the connection rather than cleanly cancelling server-side.

If the connection is actually broken at that point, the next iteration's BeginTransactionAsync fails immediately — not because the narrower window still doesn't fit, but because there's no usable connection. The loop would then burn through all six halving steps almost instantly on local connection errors and give up, without the narrower windows ever really being tried against Postgres — quietly defeating the fix in exactly the scenario (a real statement timeout) it's meant to handle.

The PR description notes this exact retry path "can't be provoked deterministically in a live test," so it looks untested end-to-end. Worth checking connection.FullState/State and reopening (or just opening a fresh connection per slice, like QueryStoreBackfill.cs does per-attempt) before retrying, and verifying against a real 900s timeout before shipping — this repo already treats "a dead connection poisons every collector" as a known failure class elsewhere (DarlingWorker.cs).

[claude]

Minor: span is the nominal AdaptiveSpan width, but sliceEnd gets clamped to collapseEnd on the range's final iteration(s) (lines 3030-3033). If a clamped (shorter) final slice fails, this [RETRY] line and the give-up message on line 3056 both print span.TotalMinutes — the un-clamped nominal width, not the actual queried span (e.g. logging "+1440m" when the real slice that failed only covered a few hours). Undercuts the fix's own goal of giving the operator an accurate picture of what happened — consider logging sliceEnd - sliceStart instead of span.

[claude]

Review summary

Scope: this PR only touches DarlingCliCommands.cs (the --collapse-legacy-slices verb's slicing loop) and CHANGELOG.md — no T-SQL, no Lite files. Lite has no equivalent day-sliced code path to keep in parity with: QueryStoreSliceRepairService.cs's own doc comment explains the asymmetry is deliberate (Lite's repair isn't day-sliced against a statement timeout the way Darling's is), so no parity drift here.

Logic of the adaptive halving loop checks out. Walked through AdaptiveSpan's halving schedule against the loop's narrower < span termination test — for a 24h base it converges to a ~22.5-minute floor (matches the CHANGELOG's "~22-minute floor" wording) after 6 halvings, and the loop correctly stops digging in once narrower == span, so there's no infinite-retry risk. consecutiveFailures resets to 0 on any success, matching the "completed slice resets to full width" claim.

Two findings left as inline comments:

1. (Likely real, worth verifying before merge) The retry continues and calls CollapseSliceAsync again on the same NpgsqlConnection after a caught failure. The failure this PR targets — a SliceStatementTimeoutSeconds (900s) timeout — is documented elsewhere in this same file's neighbor (QueryStoreSliceRepair.cs) as surfacing as a bare "Exception while reading from stream," which reads like Npgsql's cancel-on-timeout failing to land cleanly and breaking the connection, rather than a clean Postgres-side cancel. If so, the retry's next BeginTransactionAsync would fail immediately on a dead connection, and the whole halving schedule would burn through in an instant without ever really re-querying Postgres at a narrower width — silently defeating the fix in exactly the scenario it's meant to survive. The PR description itself flags this exact path as untested against a real timeout.
2. (Minor) The [RETRY]/give-up messages log the nominal AdaptiveSpan width rather than the actual (possibly collapseEnd-clamped) queried slice width, so a failure on the last slice of a range can report a misleading duration.

No security concerns (all slice queries are parameterized; the only interpolated identifier is the private Table constant), and no missing-index-DMV suggestions per the standing guidance.

[erikdarlingdata]

Both catches were real — thank you, the first one would have made this fix a no-op in exactly the field case it targets. Fixed in ce45eed:

1. Dead-connection retry: the retry path now cycles the connection before re-attempting (close is safe on a broken one, reopen draws a fresh physical connection; nothing session-scoped is lost — the slice's SET LOCAL and command timeouts are transaction/command scoped).

2. Clamped-width honesty: the retry decision and both operator messages now use the width the slice ACTUALLY covered (sliceEnd - sliceStart) — and the halving search skips nominal steps that wouldn't narrow a clamped final slice, so it never reruns an identical window just to walk the schedule down.

[claude]

CloseAsync/OpenAsync here aren't wrapped in a try/catch, unlike the CollapseSliceAsync call above. If the reopen itself fails (very plausible right after detecting the connection is likely dead — e.g. the network blip is ongoing, or Postgres restarted), the NpgsqlException/SocketException propagates out of this catch block, out of CollapseLegacySlicesAsync, and up through Program.cs's top-level statements — which have no surrounding try/catch around this verb's dispatch (line ~273). The process crashes with an unhandled-exception stack trace instead of the clean "already-committed rows are safe, re-run is idempotent" message this whole method is otherwise careful to give.

Contrast with the backfill worker's analogous adaptive-shrink path (QueryStoreBackfill.RunSliceAsync): it opens a new connection per attempt inside a scope the caller's pre-existing top-level catch-and-log-and-continue already covers, so a failed reopen there just fails that one background tick. This CLI verb has no such safety net — it's a one-shot invocation.

Worth wrapping the reopen in its own try/catch that falls through to the same error.WriteLine(...); return 1; idempotent-rerun message, so a dead-connection reopen failure degrades gracefully like every other failure mode this verb handles.

[claude]

Minor resilience gap: QueryStoreBackfillState.AdaptiveSpan can never return less than the ~22.5-minute floor (24h base, 6 halvings), so whenever actualWidth is already ≤ that floor, this search always ends with narrower >= actualWidth and falls straight to the give-up branch — with zero retry attempts, not "retry down to the floor then give up."

That's not a rare edge case: the last slice of essentially every run is a leftover partial-day clamp (sliceEnd = collapseEnd), and there's no reason that leftover is ≥22.5 minutes — it can just as easily be 5 or 10 minutes. If that small tail slice hits any transient failure (a network blip, not an actual statement timeout — it's far too narrow to hit the 900s wall), it skips the connection-cycling retry entirely on its very first failure, even though the reconnect fix is motivated by exactly that kind of transient/connection-level failure and doesn't depend on narrowing actually happening.

It still degrades gracefully (the existing idempotent-rerun message), so this isn't data-unsafe, but it does mean the new retry mechanism silently doesn't apply to what's usually the last slice of the run. Might be worth special-casing "already at/below the floor" to retry once at the same width on a fresh connection before giving up, rather than giving up unconditionally.

Separately: this narrowing-selection logic is pure and would be easy to unit test in isolation (e.g. NextNarrowerSpan(fullWidth, consecutiveFailures, actualWidth)) without needing a live timeout — that's likely how this gap would get caught, since the PR's own live tests can't provoke it deterministically.

[claude]

Reviewed the adaptive-slicing change in DarlingCliCommands.CollapseLegacySlicesAsync. Overall the halving logic is sound (traced the while (narrower >= actualWidth) search — it always terminates in ≤6 steps via AdaptiveSpan's halving cap, and never retries an identical width). Two issues posted inline:

1. Correctness / error handling at a boundary — the reconnect (connection.CloseAsync() / OpenAsync()) added in the retry path isn't guarded by a try/catch, unlike the CollapseSliceAsync call itself. Since this is a one-shot CLI verb with no top-level exception handler in Program.cs, a failed reopen (plausible right after the code has just concluded the connection is likely dead) crashes the process with a raw stack trace instead of the clean "already-committed rows are safe, re-run is idempotent" message the rest of the verb is careful to give.

2. Resilience gap (lower severity) — when the actual slice width is already at or below the ~22.5-minute adaptive floor (routinely true for the leftover final slice of a run, which is rarely an exact day boundary), a failure gets zero retry attempts rather than one at the same width — the giveup path is taken immediately, bypassing the new connection-cycling recovery this PR adds for wider slices. Not data-unsafe, just an inconsistency in how far the new mechanism reaches.

Lite/Darling parity: no drift here — Lite's Query Store slice-repair (QueryStoreSliceRepairService) is architecturally different by explicit design (automatic startup repair over the full DuckDB/parquet archive, no day-slicing, no statement-timeout exposure), and that divergence is already documented in its own doc comments, so this Darling-only change doesn't need a Lite counterpart.

No security or SQL-injection concerns (all Npgsql params, no string-built SQL from user input in this diff). No missing-index findings offered, per instructions.

[erikdarlingdata]

Round two both taken, in 0eb0731:

1. Unguarded reopen — a failed reopen now degrades to the same clean idempotent-rerun message as every other failure mode (naming both the slice failure and the reopen failure), never an unhandled crash. You're right that this one-shot verb has no caller safety net, unlike the backfill worker's per-tick catch.

2. Below-floor tail slice — a slice at/below the ~22.5m floor now gets ONE same-width retry on a fresh connection before giving up, tracked per-slice (retriedAtWidth, reset on success and on any narrowing step). The run's usual last slice is no longer exempt from the retry mechanism.

3. Took the extraction suggestion too: the narrowing decision is now the pure NextNarrowingFailureCount, pinned for the first-halving step, the clamped-tail step-skipping, and the at/below-floor null that hands over to the same-width retry — exactly the case that would otherwise only surface in the field.

[claude]

Reviewed the diff (DarlingCliCommands.cs, its tests, CHANGELOG.md). Traced the new NextNarrowingFailureCount state machine by hand against QueryStoreBackfillState.AdaptiveSpan's halving/floor behavior and against the three new unit tests — the halving-skip logic (jumping past steps that wouldn't actually shrink a clamped tail slice) and the floor/null/same-width-retry handling all check out, including the "give up only after the one fresh-connection retry at the floor" fix the comments call out as a review catch. No off-by-one or infinite-loop risk found: span is always positive (floored at MinAdaptiveSpan), so sliceEnd > sliceStart holds on every iteration, and consecutiveFailures/retriedAtWidth reset correctly on success.

A few notes, none blocking:

• Lite/Darling parity: not applicable here. --collapse-legacy-slices is Darling/TimescaleDB-only (the retry exists specifically for the Postgres statement-timeout + chunk-decompression-lock failure mode); Lite's QueryStoreSliceRepairService.RepairAsync collapses the DuckDB hot store in one shot with separate per-archive-file isolation, so there's no sliced-retry counterpart to drift out of sync.
• Broad catch triggers the narrow/reopen dance for any exception, not just the timeout/broken-stream case the loop targets (e.g. a genuine permission or syntax error would still burn through up to 6 narrowing steps + a same-width retry, each cycling the connection, before surfacing the real error). This matches the existing convention in DarlingCollectorRunner's use of AdaptiveSpan (also doesn't discriminate by exception type), so it's consistent with the codebase rather than a new gap — flagging only in case that broader question wasn't already settled for this call site.
• Couldn't run dotnet build/dotnet test in this sandbox (Bash access to the toolchain was blocked), so the trace above is manual rather than compiler/test-verified — worth confirming CI is green.

No correctness, security, or SQL-injection issues found; the slice bounds stay parameterized ($1/$2) and unchanged by this PR.