Repair the Query Store split slices stored before #1907, and disclose what cannot be reached (#1912)

[erikdarlingdata]

Fixes #1912.

#1907 fixed Query Store's split slices at collection and made the read deterministic. It could not touch
what was already stored. This repairs that — fully in Lite, and as far as raw reaches in Darling — and
discloses, plainly, the part that is permanent.

The defect, in the stored rows

sys.query_store_runtime_stats returns the flushed and the still-in-memory slice of one
runtime_stats_interval_id as two additive rows. Builds before #1907 stored both, so every read that
keeps one row per interval reports a fraction of that interval's work, and every rollup materialized from
those rows carries that fraction. On the live evidence: 8 executions where 94 was true.

The pre-fix signature is exact — two or more rows sharing the entire dedup key and collection_time.
No row collected since #1907 can match it, because the collector now emits at most one row per interval per
cycle. That single fact is what makes the repair idempotent, safe to re-run, and incapable of touching
correctly-collected data.

Darling: --collapse-legacy-slices

An operator verb, mirroring --backfill-rollups (#1849): same Windows/managed-credential guard, same
--dry-run, same compiler-required disclosure sink (#1797). It surveys, collapses, then re-materializes the
rollups the collapsed rows fed.

Column handling is derived from QueryStoreCollector.PayloadColumns and each column's declared type
rather than restated — execution_count sums, every avg_ takes the count-weighted mean, min_/max_ take
the extreme. That derivation paid for itself on the first live run: PostgreSQL has no min(boolean), and
because the classifier reads the declared type, bool_or was a one-line fix rather than a hunt.

The refresh clamp is a safety property

Measured on PostgreSQL 18.4 + TimescaleDB 2.28.1, three fresh CAGGs in identical state, one variable each:

Refresh	Range	Pre-raw buckets surviving
force => TRUE	dropped-range only	0 of 144 — destroyed
force => TRUE	spanning dropped + live	144 — preserved
plain, no force	dropped-range only	0 — destroyed

A refresh whose range lies entirely within dropped raw chunks destroys the materialization there, with
force and without. My first probe said history survived; that was an artifact of the range spanning into
live data, which is why it took a single-variable design to see. Aiming at a nominal "pre-fix period" would
therefore have blanked the 21-day hourly and the indefinitely-kept daily below raw's floor — the one thing
#1759/#1793 forbid. The verb derives its window from the rows it actually collapsed, which are inside raw's
extent by definition, so safety falls out of the data flow instead of a bolted-on check. Pinned as a live
test, so a future TimescaleDB that makes it safe fails loudly rather than quietly widening the blast radius.

The shipped --backfill-rollups was checked for the same exposure and is safe by construction — it
converges down to min(collection_time) of raw and never refreshes below it. No bug to file.

Also measured: DML against compressed chunks works on 2.28.1 and leaves them compressed (DELETE 600 rows
6.0 ms, INSERT 6.4 ms, UPDATE 0.6 ms), so the collapse reaches the older half of the raw window without a
decompress cycle.

Lite: automatic, on the first launch after upgrading

Per the lead's call, and the parity argument is that consistency means the same semantics for the same
defect — same collapse, same weighted math, same disclosure — not the same invocation surface. Each app
follows its own precedent: Darling's is #1849's verb, Lite's is the automatic startup store migration that
v39 and #1832's data-root move established. A button would leave the users least equipped to find it holding
wrong numbers permanently.

Lite gets a FULL repair where Darling gets a bounded one, because Lite's long tier IS the parquet
archive — the same rows, rewritable — rather than a materialized rollup that cannot be rebuilt from raw that
no longer exists. Measured against a real archive: ~46 MB across three monthly files, 6.6–8.5% of rows in
split groups with exactly 2 rows per group every time, the largest file (28 MB / 533,109 rows) rewriting in
under a second. A real group verified: (4 execs @ 161093µs) + (10 @ 203358µs) → 14 @ 191282µs, the
count-weighted mean exactly.

Migration discipline (#1832 / v39 pattern)

Each file is rewritten beside the original and promoted only after two conservation invariants — row
count landing on exactly rows - rowsRemoved, and SUM(execution_count) unchanged, which is the
invariant the collapse's own arithmetic guarantees and the one that would catch a bad aggregate that still
produced the right row count. No backup copies: verify-before-promote is the safety.

Failures are isolated per file, in the rewrite and in the survey. Both of those came from tests catching
real gaps rather than foresight: the mid-rewrite failure test showed the first bad file aborted the whole run
— and since files process in name order, one bad old file could permanently block every newer one — and a
second run showed the same hole in the survey. The marker is written only on a fully successful pass, so a
partial repair retries next launch.

The marker is its own table, not a schema-version bump, for two load-bearing reasons: RunMigrationsAsync
drops and recreates tables per version step, so a data repair has no business there; and withholding the
schema version to force a retry would re-run those drops.

The DuckDB finding that would have shipped silently

An in-place parquet rewrite poisons DuckDB's external file cache. It keys on path at instance
scope, so replacing the bytes at a path already read makes every later read fail with No magic bytes found at end of file — on new connections too — until the process restarts. That would have left Lite unable to
read the archive it had just repaired.

The instance scope is the trap: my first isolation used separate in-memory instances, which made it look
per-connection and harmless. PRAGMA clear_cache does not exist and enable_object_cache is a different
cache; toggling enable_external_file_cache off and back on evicts the entries and leaves caching enabled.
Verified standalone.

Two smaller ones, also caught by running it: DuckDB widens SUM(BIGINT) to HUGEINT and returns a
BigInteger that Convert.ToInt64 refuses (the #1839 trap), cast in SQL; and ArchiveService's 4GB
memory_limit raise is now internal and reused rather than copied, because that floor is a
known-broken-below-2GB constraint and a second drifting literal is exactly how #942 broke compaction.

What is permanent, and said plainly

Darling's repair reaches only what raw still holds — a few days. Query Store numbers older than that stay
understated for the life of the store, because the daily tiers are kept indefinitely and cannot be rebuilt
from raw retention has dropped. No ordering of these operations reaches further back.

That is disclosed in three places: the #1845-style annotation in ComposeCompiler's doc beside the #1849
boundary it shares, the CHANGELOG, and a CHANGELOG operator action that leads with the timing — the verb
gets less useful every day you wait, so run it right after upgrading. DARLING01 named explicitly.

Testing

Watched red per mutation, each reverted:

Mutation	Went red
KeyColumnsFor assumes the newest schema	the unit assertion and the archive test, the latter with the field symptom exactly: Binder Error: Referenced column runtime_stats_interval_id not found in FROM clause
archive file that fails mid-rewrite	showed the run aborted instead of isolating — fixed the code, not the test
survey of an uncombinable file	same hole one layer up — fixed the code

The era-schema fixture is a parquet written without runtime_stats_interval_id or replica_role,
matching the real June 2026 archive file, so the era-appropriate key has to engage.

Live PostgreSQL 18.4 + TimescaleDB 2.28.1 — rollup shows a slice before the repair; the survey finds
exactly the split cycles; the collapse leaves a correctly-collected interval untouched; a re-run is a no-op;
after re-materializing the rollup reports the interval's true 125 with the weighted mean 1871 (not
the 2011 an unweighted average gives). Plus the clamp-safety pin and the verb's dry-run → repair →
nothing-left cycle.

Live DuckDB — hot-store collapse, old-era archive rewrite, failure isolation, and the startup
marker/retry contract.

Archive discipline: every archive in the tests is a scratch file the test wrote into a temp directory it
deletes. The measurements that justified this work were taken against Erik's real archive by copying it;
the originals still carry their original mtimes.

Suites: Darling 4033 passed / 0 failed against live PG (10 skipped = gated); Lite 1927 passed / 0
failed. Full-solution -t:Rebuild: 0 warnings, 0 errors. Installer.Tests deliberately not run.

Deferred

None. The permanent residue is a disclosure rather than a deferral — no further work reaches it.

🤖 Generated with Claude Code

[claude]

Review: #1912 Query Store split-slice repair (Lite + Darling)

Scope: ~2000 LOC across 10 files — a new Darling operator verb (--collapse-legacy-slices), a new automatic Lite startup repair, and their tests. Overall this is a careful, well-tested piece of work: idempotent-by-signature design, real conservation checks before promoting rewritten archive files, per-file failure isolation, and an honest disclosure of the permanent Darling residue (understated history older than raw retention). The live Darling tests pinning the TimescaleDB refresh-destroys-dropped-range behavior are a good defensive move.

One concurrency issue worth fixing before merge (inline comment below), plus a minor design-vs-implementation mismatch noted inline. No Lite/Darling parity problems beyond what is already intentionally and clearly documented (Lite = full repair via rewritable archive, Darling = bounded by raw retention) — that asymmetry is explained and justified in both the code comments and CHANGELOG.

No SQL-injection concerns: all dynamic SQL is built from fixed collector metadata (PayloadColumns, information_schema/DESCRIBE output) and constants, never from external input. No secrets or network/process handling introduced.

[claude]

Concurrency: this should be a write lock, not a read lock.

RepairAsync acquires AcquireReadLock() but then:

• runs DELETE/INSERT against the live hot table query_store_stats (lines ~419-431), and
• deletes and replaces archive parquet files on disk in-place (RewriteArchiveFileAsync → File.Delete + File.Move, lines ~532-533), files that the app's own union_by_name archive views actively read.

DuckDbInitializer's own doc comment on the lock says read locks let "unlimited concurrent UI queries" run, and that the lock exists to "coordinate UI readers with maintenance writers (CHECKPOINT, archive DELETEs, compaction)" — explicitly calling out archive file replacement as a write-lock-worthy operation. ArchiveService's retention path follows that: it does the (read-only) export under a read lock but explicitly switches to AcquireWriteLock() just for the DELETE, with the comment "the DELETE modifies table data and the next CHECKPOINT reorganizes the file — readers must not be mid-query when that happens or they get 'Reached the end of the file' errors."

This repair does the equivalent (writes to the hot table, and swaps files that back live views) under only a read lock, which — per ReaderWriterLockSlim semantics — allows any number of other UI readers to run concurrently, including a UI query hitting read_parquet(...) on the exact archive file mid File.Delete/File.Move.

Since MainWindow.xaml.cs fires RepairOnStartupAsync via Task.Run (not awaited) specifically so the window opens and stays interactive while a large archive rewrite runs, this race is concretely reachable, not theoretical — a dashboard tab querying Query Store data can run at the same moment this service is deleting/replacing the file backing it. The MainWindow.xaml.cs comment that the read lock "serializes with collection rather than racing it" is true for the collector (which uses a write lock and would block), but not for concurrent UI readers, which a read lock explicitly permits to run alongside.

Suggest switching the write phase of RepairAsync (hot-table collapse + archive file rewrite/swap) to AcquireWriteLock(), mirroring ArchiveService's pattern of keeping only the read-only survey/rewrite-to-temp under a read lock and taking the write lock just for the parts that mutate what's actually being read (the hot table DELETE/INSERT and the file promotion).

[claude]

Minor: CollapseSliceAsync is invoked here exactly once, over the entire survey span (survey.OldestUtc .. survey.NewestUtc + 1s) — but its own doc comment on QueryStoreSliceRepair.CollapseSliceAsync (Darling/PerformanceMonitor.Darling.Storage/QueryStoreSliceRepair.cs) frames it as "one half-open collection-time slice" and says "Slicing keeps that transaction — and the locks it takes on chunks a compression job may also want — short," implying the caller is meant to loop over multiple bounded slices for a large range. This call site never slices, so the whole collapse (temp-table build + DELETE + INSERT) runs as one transaction for whatever span the survey reports.

In practice this is bounded by raw retention (a few days, per the PR description), so it's unlikely to be a problem with typical settings — but if a store is configured with a longer raw retention window, this becomes one long-held transaction/lock rather than the short, chunked ones the design comment promises. Worth either slicing the call here (e.g., per day) to match the documented intent, or updating the doc comment to reflect that the current caller always passes the whole range in one shot.

[erikdarlingdata]

PR UP — #1931, all checks green, d7fb1849, MERGEABLE / CLEAN.

Both halves shipped on the shape you decided: Darling's --collapse-legacy-slices operator verb, Lite's automatic one-time startup repair with the #1832/v39 discipline (verify-before-promote on two conservation invariants, per-file isolation, marker withheld on partial so it retries, no backups).

Three engine findings, each of which would have shipped silently:

1. A refresh entirely within dropped raw chunks destroys the materialization — force or not. Pinned as a live test; the clamp derives from the collapsed rows so safety falls out of the data flow. --backfill-rollups checked and safe by construction.
2. An in-place parquet rewrite poisons DuckDB's external file cache (path-keyed, instance scope), which would have left Lite unable to read the archive it just repaired. Toggling enable_external_file_cache evicts it.
3. SUM(BIGINT) → HUGEINT → BigInteger (the [FEATURE] Blocking alert: threshold on total blocked wait time (level-triggered), not just event count #1839 trap), cast in SQL.

Two of the watched-reds fixed the CODE rather than the test — the mid-rewrite failure showed one bad file aborted the whole run, and the survey had the same hole a layer up.

Suites: Darling 4033/0 against live PG 18.4 + TSDB 2.28.1, Lite 1927/0, -t:Rebuild 0 warnings.

The permanent residue is disclosed in three places (ComposeCompiler doc beside the #1849 boundary, CHANGELOG, and an operator action that leads with the timing). DARLING01 named. No deferrals — nothing further reaches it.

[claude]

Review: #1931 — Repair pre-#1907 Query Store split slices

Scope reviewed: gh pr diff 1931 (10 files, +2043/-2) against dev.

Overview

Repairs the stored-row half of the #1907 Query Store split-slice defect: builds before #1907 stored the flushed and still-in-memory slice of one runtime_stats_interval_id as two additive rows, so any dedup-on-read kept only a fraction of the interval's true execution count. This PR adds:

• Darling: --collapse-legacy-slices verb (QueryStoreSliceRepair.cs + DarlingCliCommands.cs wiring), bounded to raw retention, re-materializing the rollups it fed.
• Lite: an automatic once-per-store startup repair (QueryStoreSliceRepairService.cs), reaching the full archive since Lite's long tier is rewritable parquet rather than a rollup.
• CHANGELOG entries disclosing what's permanently unreachable (rollups older than raw retention at the time the Darling verb is run).

The collapse math (count-weighted mean via avg * execution_count, sum for execution_count, extremes for min_/max_) is deliberately derived from the collector's own PayloadColumns/declared types rather than restated, which keeps Darling and Lite's aggregation rules from drifting apart. Both sides land on equivalent semantics — Darling's Postgres bool_or vs. Lite's DuckDB ANY_VALUE/native MIN/MAX on booleans is a real, documented platform difference, not drift.

Findings

Posted as inline comments:

1. Lite — QueryStoreSliceRepairService.RepairAsync/RepairOnStartupAsync mutate the DuckDB store under AcquireReadLock() instead of AcquireWriteLock(). Every other writer in this codebase (ArchiveService, DuckDbAlertHistoryStore, DuckDbMuteRuleStore, the collector's CHECKPOINT) takes the write lock for DELETE/INSERT/CREATE TABLE; the read lock is documented as allowing unlimited concurrent access, so this doesn't serialize against UI readers the way the code (and its own comment in MainWindow.xaml.cs) claims.
2. Lite — non-atomic archive promotion (File.Delete then File.Move in PromoteRewrittenFileAsync). A crash between the two lines permanently loses the archive file rather than merely leaving it "un-repaired," which is a step down from the interruption-safety this PR advertises. File.Move(tempPath, originalPath, overwrite: true) (.NET 6+, and the project targets net10.0) would close this in one atomic call.
3. Darling — the "PARTIAL ... re-run to retry the rollups" message is misleading. Once the collapse commits, the pre-fix signature SurveyAsync looks for is gone, so a re-run always short-circuits at "Nothing to repair" before it ever reaches the rollup re-materialization loop. A rollup that failed to refresh on the first pass has no working retry path via this verb.

Other notes (not blocking)

• Lite/Darling parity on the semantics of the repair (dedup key, weighted-average math, idempotence signature) checked out — Lite's key intentionally omits server_name (Darling's key keeps it for its fleet store) since server_id already scopes it deterministically; not a bug.
• No SQL-injection concern: all dynamically-interpolated identifiers (column names, table name, file paths) come from the collector's compile-time PayloadColumns, information_schema/DESCRIBE output, or the app's own archive directory — none of it is attacker-influenced input.
• Good test discipline: the PR's testing notes describe mutation-testing the era-aware key selection and the per-file failure isolation, and that shows in the test file — KeyColumns_AdaptToTheFilesOwnEra... and ArchiveRewrite_ThatFailsVerification_LeavesTheOriginalUntouched are exactly the right shape of test for this kind of repair code.

(Per instructions, not flagging missing-index DMV recommendations — not applicable here anyway.)

[claude]

RepairAsync mutates query_store_stats here (CREATE OR REPLACE TEMP TABLE / DELETE / INSERT) but only takes AcquireReadLock(). Every other DuckDB writer in this codebase takes AcquireWriteLock() for mutations — ArchiveService.cs:159/591/681, DuckDbAlertHistoryStore.cs (write lock on every insert/update, read lock only on selects), DuckDbMuteRuleStore.cs, and RemoteCollectorService.cs:414 for CHECKPOINT. The lock's own doc comment (DuckDbInitializer.cs:22-24) says read locks allow unlimited concurrent access and write locks exist specifically to serialize maintenance writers — "archive DELETEs" is the example given — against UI readers.

Because this is a ReaderWriterLockSlim, a read lock does not exclude other read-lock holders, so a UI query can run concurrently with this DELETE+INSERT transaction. Since the repair is kicked off via Task.Run from MainWindow_Loaded right as the UI starts querying (see MainWindow.xaml.cs), that's not just theoretical.

Same issue for the marker writes at lines 250 and 268 (CREATE TABLE IF NOT EXISTS / INSERT into query_store_slice_repair) — those should also be AcquireWriteLock().

Relatedly, the comment in MainWindow.xaml.cs ("it takes the same read lock every other store operation does, so it serializes with collection rather than racing it") isn't accurate for the same reason — a shared read lock doesn't serialize concurrent operations against each other.

[claude]

File.Delete(originalPath) followed by File.Move(tempPath, originalPath) isn't atomic — a crash/kill between these two lines leaves the archive file permanently missing rather than merely "un-repaired." The verify-before-promote check happens before this point, so it doesn't cover this window, and this is a step down from the interruption-safety this PR advertises for the archive repair specifically ("Nothing is at risk if it is interrupted").

Since the target framework is net10.0-windows, File.Move(tempPath, originalPath, overwrite: true) (available since .NET 6) would make the promotion a single atomic call and close this gap. (The delete-then-move pattern already exists elsewhere — ArchiveService.cs:148-150, DataRootMigration.cs — so this isn't a new pattern in the codebase, but it's worth tightening here given the stronger safety claim made for this feature and the fact the framework already supports the fix.)

[claude]

When some rollups fail to re-materialize, this reports "PARTIAL ... re-run to retry the rollups" — but a re-run won't actually retry them. SurveyAsync (called above at what's now line ~2176) only looks for the pre-fix split-slice signature (two-or-more rows sharing the whole dedup key + collection_time); once the collapse has committed, that signature is gone, so the next invocation hits the "Nothing to repair" branch and returns 0 before ever reaching this re-materialization loop.

So a rollup that failed here (e.g. a transient connection or lock-timeout error on RollupBackfill.RepairAsync) stays stale with no working recovery path through this verb — the operator is told to do something (re-run) that silently does nothing for the affected rollup.

[erikdarlingdata]

PUSHED — bb1f48c0, all checks green, MERGEABLE / CLEAN.

Confirming: option (a) landed. There is no Lite UI and none was ever started. The "remaining is the Lite operator UI" phrase was from a message sent before the (a) decision arrived; Lite's half has been the automatic startup migration since, wired into MainWindow and covered by the marker/retry tests.

Cache eviction moved inside the swap helper, as directed. PromoteRewrittenFileAsync now owns delete + move + evict + rebuild-views, because replacing the bytes at a path and making the store able to READ that path are one operation, not two. Any future in-place rewrite that goes through it is correct without its author knowing about DuckDB's instance-scoped path cache; one that hand-rolls File.Move breaks the archive it just fixed. The comment also records why the monthly archive cycle is deliberately not given the same treatment — it only writes new names and deletes old ones, never replaces bytes behind a read path, so it was never exposed and adding it there would imply a hazard it does not have.

Watched red, and it separates the two halves cleanly: removing the eviction while keeping the view rebuild fails both archive tests with the exact production symptom, No magic bytes found at end of file. The view rebuild alone does not save it — the eviction is the load-bearing part.

Also corrected a stale claim the restructure surfaced: the class doc still said the Lite repair is "operator-invoked, never automatic", which was true of the pre-(a) shape and is now the opposite of what ships.

Darling 4033/0 against live PG 18.4 + TSDB 2.28.1, Lite 1927/0, build 0 warnings.

[claude]

Correctness: a partial run's own "re-run to retry the rollups" advice can't be honored.

The rollup re-materialization window (survey.OldestUtc/NewestUtc, used down at the Floor(...) calls) comes from this SurveyAsync() call — which only finds rows that still carry the pre-fix split signature. That's fine on a clean first run, but consider either of these partial-failure paths:

1. The per-day collapse loop (line ~2258 while (sliceStart < collapseEnd)) throws partway through — earlier days are already committed (safe), but the method returns 1 before ever reaching the rollup-refresh block.
2. The collapse fully succeeds, but one target's RollupBackfill.RepairAsync throws in the foreach (var target in affected) loop below — you print PARTIAL — ... re-run to retry the rollups.

In both cases, re-running the verb calls SurveyAsync() again. Since the raw rows for the already-collapsed days no longer carry the split signature, survey.HasWork is false, so execution hits this early return and prints "Nothing to repair" — the rollup refresh step is never reached again. The tool's own printed guidance to "re-run to retry the rollups" doesn't actually work once the raw collapse has already succeeded; the operator's only way to fix a stale rollup after this is to fall back to --backfill-rollups (not obviously implied by the message here).

Worth persisting the collapsed range independently of the split-signature survey (or explicitly telling the operator to use --backfill-rollups if a rollup refresh fails), so a retry can still find the range that needs re-materializing even after the raw side is already fixed.

[claude]

Non-atomic swap contradicts the "nothing is at risk if interrupted" claim.

File.Delete(originalPath);
File.Move(tempPath, originalPath);

If the process dies between these two lines (crash, kill -9, power loss, antivirus lock), the archive file is gone entirely — not merely "un-repaired," but missing, with no original and no promoted replacement. That's a stronger failure than what the CHANGELOG and the class doc above promise: "Nothing is at risk if it is interrupted... A file that fails verification is left exactly as it was." The verification step (checked just before this) does guard against a bad rewrite, but not against a crash during the swap itself.

Since this targets net10.0-windows, File.Move(tempPath, originalPath, overwrite: true) (available since .NET Core 3.0) removes the gap — no delete step, so there's no window where neither file exists at originalPath.

(This mirrors the existing delete-then-move idiom already in ArchiveService.cs — e.g. around its own monthly-file promotion — so it's not a new pattern introduced here, but this feature is the one making an explicit interruption-safety guarantee, so it's the one place where tightening it seems most worth it.)

[claude]

Low-confidence / theoretical: refresh window is derived from a survey taken before the collapse and refresh run, not re-checked against retention at refresh time.

The whole safety argument for the "refresh is clamped to the rows actually collapsed" property (documented above and pinned by QueryStoreSliceRepairLiveTests) rests on "collapsed rows are inside raw's extent by definition." That's true at the instant SurveyAsync measures OldestUtc/NewestUtc. But this command then runs a per-day collapse loop and, afterward, a separate rollup-refresh pass — both against a live store where a retention drop_chunks job could run concurrently (this is a long-lived CLI process talking to a store the Darling service may still be actively managing).

If a retention sweep drops the chunk containing survey.OldestUtc between the initial survey and the later refresh calls (Floor(survey.OldestUtc!.Value, ...)), the refresh would target a range that has, by then, fallen below raw's floor — exactly the "refresh entirely within dropped chunks destroys the materialization" scenario the live test pins as catastrophic. This is a narrow window (command should typically run in seconds), and I don't have certainty about the retention job's actual cadence/locking here, so flagging as a possible edge case worth a second look rather than a confirmed bug.

[claude]

Review summary

This PR adds a repair for the pre-#1907 Query Store split-slice defect: a Darling operator verb (--collapse-legacy-slices, bounded by raw retention) and a Lite automatic startup repair (full, since Lite's long tier is a rewritable parquet archive). Both derive column-combination rules from the collector's own payload schema rather than restating them, which is a nice piece of design - a future Query Store column gets classified automatically instead of silently defaulting wrong.

Lite/Darling parity: The invocation-surface asymmetry (Lite = automatic on startup, Darling = explicit verb) is deliberate and explained in the PR description/CHANGELOG - same collapse semantics, same weighted math, same disclosure, different trigger per each app's own established precedent. No unintentional parity drift found; the two CombineExpression implementations (Darling's Postgres version using bool_or, Lite's DuckDB version relying on ANY_VALUE/native MIN/MAX over booleans) are consistent with each other's underlying engine differences, and both use the same weighted-average formula (avg * count, summed, divided by NULLIF(sum(count), 0), truncated to bigint).

Findings posted inline:

1. (Correctness, Darling) DarlingCliCommands.cs - the rollup re-materialization window comes from a SurveyAsync() call gated by the pre-fix split signature. Once the raw collapse has already succeeded (either from an earlier partial run that failed mid-loop, or one where a rollup-refresh target itself threw), a re-run finds no more split rows, prints "Nothing to repair," and never retries the rollup refresh - even though the PARTIAL message explicitly tells the operator to "re-run to retry the rollups." That promise can't be kept as written; --backfill-rollups is the actual escape hatch, but that isn't stated at the point of failure.

2. (Correctness/robustness, Lite) QueryStoreSliceRepairService.PromoteRewrittenFileAsync - File.Delete(originalPath) followed by File.Move(tempPath, originalPath) is not atomic. A crash between the two lines leaves the archive file missing outright, which is a stronger failure than the "nothing is at risk if interrupted" guarantee this feature explicitly makes. File.Move(tempPath, originalPath, overwrite: true) (available on the targeted net10.0) removes the gap. This mirrors an existing idiom already in ArchiveService.cs, so it's pre-existing risk rather than a novel regression - but this feature makes the strongest explicit safety claim about interruption, so it seems the best place to tighten it.

3. (Low confidence, Darling) Same file - the refresh window is computed from a survey taken before the collapse/refresh run, against a live store a retention job could still be operating on concurrently. If a drop_chunks sweep removed the surveyed range's chunk between the survey and the refresh calls, the refresh could target a range that has since fallen below raw's floor - the exact destructive scenario the clamp exists to avoid. Flagging as worth a second look rather than confirmed, since I don't have full visibility into the retention job's cadence.

Other notes (minor, not filed inline):

• DarlingCliCommands.cs picks up a UTF-8 BOM as part of this diff despite being an existing, previously BOM-less file - looks like an editor artifact rather than intentional.
• QueryStoreSliceRepair.BuildCollapseStatements in Darling splits one composed SQL string by searching for the literal substrings "DELETE FROM"/"INSERT INTO" rather than building the three statements independently. Safe today since all identifiers are fixed and compile-time known, but a fragile-looking pattern if anyone later parameterizes any part of that string.

Test coverage looks strong - live-Postgres tests pin the destructive-refresh-below-raw's-floor behavior, mutation testing is called out in the description, and the Lite tests assert on real archive rewrites rather than mocks. No SQL-injection or secrets-handling concerns - all identifiers are compile-time constants from the collector's own schema, not user input.

[erikdarlingdata]

BOTH FIXED — dd201c94, all checks green, MERGEABLE / CLEAN.

1. Lock correctness. The mutating phases now take AcquireWriteLock, mirroring ArchiveService's shipped idiom. The race was real rather than theoretical: the read lock excludes the collector, not the UI, and RepairOnStartupAsync is fired un-awaited precisely so the UI stays interactive — so a UI reader can be mid-read_parquet on the exact file being swapped.

Write-lock spans are tight and per file, not one lock around the run. Reading the original and writing the temp sibling touches nothing a reader can see, so only the swap instants need exclusivity — a large archive repair must not freeze the UI for its full duration when the swaps are the fast part.

Guarded by a source pin in the shape of the other #1907/#1912 pins. Watched red: reverting the hot collapse to the read lock drops the write-lock count 2 → 1 and fails it.

2. Lock duration (#1564/#1567 family). Fixed at the call site, not the doc: the verb now loops per-day slices over the survey span. A default 4-day raw tier is a handful; a widened retention gets the protection CollapseSliceAsync's doc actually promises. Each slice is its own transaction, so a mid-run failure keeps the committed ones and the idempotent re-run continues — the output says so. The nothing-left assertion after the loop is unchanged.

Darling 4033/0 against live PG 18.4 + TSDB 2.28.1, Lite 1928/0, -t:Rebuild 0 warnings (two xUnit2013 analyzer warnings my new pin introduced were fixed — the repo's zero-warning rule caught them).

PostgreSQL on 5619 is down, verified: connection refused, zero listeners.