[erikdarlingdata]

Makes #3012's two operator treatments durable in code, and inverts the dependency that made the first one necessary. Fixes #3012 (closing keywords are a no-op on a dev merge, so this needs closing by hand).

What was wrong

Every hourly continuous-aggregate refresh carried start_offset => INTERVAL '3 days' against a hypertable with 4-day raw retention, so each run re-materialized roughly three quarters of the table. A refresh's cost is set by the window it re-scans, not by the rows that arrived in it, so the cost tracked retained volume rather than arrival rate — measured on the production store, refresh duration rose ~8x while rows arriving per hour fell ~3x, which is what falsifies "more rows to materialize" and leaves "too much window".

The heaviest hourly refresh reached 3,301–6,330 s against its own 1-hour cadence — 118–175% of it — at which point each run started into the tail of the last. That is when the lock queue turns it into a convoy: the running refresh holds AccessShareLock, the compression policy on the same hypertable queues an AccessExclusiveLock request behind it, and a queued exclusive request blocks every subsequent shared request, so collector store-writes pile up behind a lock nobody holds. 40 of 40 collector write errors in the reported window fell inside a running refresh or compression window.

What this changes

TimescaleSupport.HourlyRefreshStartOffset is 1 day, applied to all thirteen hourly refresh policies (the six named rollups plus the seven baseline aggregates). On the narrowed window the heavy refresh runs 864 s, 24% of cadence, so it can no longer outrun its own schedule.

TimescaleSupport.RefreshPhaseStepMinutes puts those thirteen on a 15-minute grid keyed on HourlyRefreshPhaseOrder — the view name, never a job id, because TimescaleDB job ids are per-deployment. Naming initial_start also switches each job to a fixed schedule, which is the less obvious half: without it TimescaleDB computes the next start from the previous finish, so one convoy phase-locks a family permanently — the incident's three jobs, on unrelated schedules with very different workloads, finished within 119 seconds of each other and re-started together every hour after that.

The two are complements, not alternatives. The heavy refresh is still ~33x its lightest sibling on the narrowed window, so narrowing is what makes it finish and phasing is what keeps its remaining 864 s out of everything else's way. They are pinned separately for that reason, and each pin is red on its own mutation with the other green — and reverting both at once reddens three disjoint tests rather than one, so neither pin is wearing two hats.

What the grid has to guarantee is that policies contending for the same relation start on different minutes, and that is now asserted for every such group rather than for the one family the incident evidence named. collect.query_stats feeds three hourly policies; their minutes are distinct today only because of where they sit in the order, and phases collide whenever two positions differ by a multiple of the slot count. The guard derives each view's contended relation from the shipped CREATE text via HourlyRefreshDefinitions, counts a view that is itself a source into its consumers' group (refreshing it writes what they read), and asserts its extraction found a source per view before concluding anything. Raised by review, not found here.

ConvergeContinuousAggregateRefreshAsync reaches stores that already have policies, and it runs before EnsureContinuousAggregatesAsync rather than after — which is a measured ordering requirement, not a preference.

add_continuous_aggregate_policy does not behave like add_compression_policy or add_retention_policy. Those return -1 and change nothing against a policy the store already has. This one does that only when the window matches; against a policy whose window differs it raises 22023 refresh interval overlaps with an existing continuous aggregate policy. So the create sweep running first would raise on all thirteen hourly views of every already-deployed store, swallow each one in its per-aggregate catch, and report the aggregates as unready while the windows stayed at three days. That premise was stated the other way in the first revision of this description and in several doc comments; the live test disproved it and both are corrected. Converging first leaves the create looking at policies that already match, which is the quiet -1 it was written for.

The converge selects only jobs that differ, compares start_offset as seconds so 1 day / 24:00:00 cannot read as a difference and re-alter the same job every start, reads the phase in UTC, and names no scheduled argument so it cannot arm a paused job. Behaviour by store state: a fresh store has no refresh jobs yet (its aggregates are created moments later), so the converge reads an empty set and returns 0; a store on the old values gets both halves on the first start after deploy and the create that follows then finds policies which match; a store already hand-patched to the right window is left alone on the window and re-phased only if its job is not on a fixed schedule or not on this grid.

That last case is no longer a judgement call. The live store was probed roughly two hours after the offsets were applied by hand: every job read fixed_schedule = false, and only one of six hourly refreshes was still on the minute it had been set to. A follow-up read put a number on the rate — the heavy refresh's next start was exactly its previous finish plus one hour, so it drifts by its own runtime every cycle and laps the hour in days. A hand-applied stagger is not a stagger with a slow leak; it is a stagger with a countdown, and pinning the schedule is what stops it.

Daily refreshes are untouched — DailyRefreshStartOffset is still 3 days, still finish-to-start, and RefreshPhaseMinutesFor throws for a daily view so one cannot be dragged onto the grid. That is deliberate and pinned: a daily job's 3-day window is 3 days of scan against an 86,400-second cadence rather than a 3,600-second one, it was never near its own schedule interval, and the 3 days are the buffer HourlyRetentionInterval's 90-day drop leans on. The obvious future regression here is a tidy-up that makes every refresh window "consistent".

The relationship, not just the constant

QueryStoreBackfill.Horizon was RetentionTierRouter.RawMaxAge alone, and that read as correct only because the hourly refresh window was also 3 days: the retention term was standing in for the refresh term. That is the coupling — the refresh window had to cover a backdating depth that retention chose, so every retention increase silently bought more refresh cost.

The horizon is now the smaller of the two conditions that both have to hold at the depth a backdated slice lands: raw retention must not immediately drop the rows, and the hourly aggregates must still re-materialize the buckets it touched. The rollups are materialized-only — the watermark is a hard partition, not a fallback — so a bucket outside the refresh window keeps whatever it was materialized with, and backfilled rows become invisible to every window that routes at hourly grain while still being visible at raw grain. That is a split between two tiers, not a delay.

The refresh term is now the binding one where rollups exist, and the cost is stated rather than hidden: RollupStoreHorizon is 23 hours (the window minus one refresh interval, because the window slides forward by exactly that much between runs), so post-outage catch-up and first-contact tail recovery reach less far back on a TimescaleDB store.

PlainStoreHorizon stays at the full 3 days. TimescaleDB is optional and auto-detected and QueryStoreBackfill is constructed unconditionally, so the first revision of this change narrowed plain-PostgreSQL stores too — where there are no hourly rollups for a backdated row to fall out of, every read goes to raw, and the refresh term buys nothing. Raised by review. HorizonFor(bool hasContinuousAggregates) is what the slice path calls and DarlingWorker threads the extension flag in as a provider so it cannot capture a pre-detection reading. A store that gains the extension later is not left with a split either: the aggregates are born WITH NO DATA and RetentionTierRouter already routes any window below a rollup's measured floor to raw.

Digging deeper on a rollup store would mean running refresh_continuous_aggregate off the backfill loop, which is a decision about putting the convoying operation on an unphased path — not a constant, and not in this change.

Deliberately not here

A per-job lock_timeout wrapper. It cannot be a role-level setting: every TimescaleDB background job runs as the same role as the collectors, so a role-level lock_timeout would cap collector sessions too. That analysis is on #3012 and the wrapper is a separate decision.

max_concurrent_sweeps needed no change — DarlingConfig, StoreConfigProvider and ViewerDataService.ServiceConfig all default to 4, neither sample config carries the key, and the store seed writes the code default. The settled value already had a correct durable home.

Verification

No schema change and no version bump — this is all idempotent runtime setup in the worker's TimescaleDB block.

The Windows suites cannot run on macOS, so the two pure-unit test files were compiled into a net10.0 console harness against the real built assemblies and run there: 45/45. That harness compiles two files rather than the suite, which is why a repo-wide hygiene guard and a Lite-side source scan both went red in CI on things it could not see; both of those guards have since been replicated locally over the whole tree with planted positive controls. The generated SQL and Horizon were also dumped back out of the built assembly rather than read from source. Both CI suites are green at the head SHA and both are named rather than inferred from a green build: Lite.Tests Total: 3323, Failed: 0, Skipped: 0, Not Run: 0 and Darling.Tests Total: 7596, Failed: 0. The live converge test is absent from the Darling PostgreSQL tests job's six-test SKIP list, so it executed against a real TimescaleDB store; that grep was run with a positive control on the identical invocation, because a failed fetch and a clean result look the same.

Proven red nine ways, one mutation at a time, each restored byte-identically against a source hash with the tree committed first:

mutation	red	green (independence)
HourlyRefreshStartOffset → 3 days	narrowing pin, 1 fail	stagger pin
HourlyRefreshStartSpan → FromDays(3)	narrowing pin + horizon pin, 2 fails	stagger pin
AddHourlyRefreshPolicySql passes phaseMinutes: null	stagger pin, 1 fail	narrowing pin
both of the above at once	narrowing + stagger + horizon, 3 disjoint fails	—
RefreshPhaseMinutesFor returns one slot for everything	stagger pin + contention pin, 2 fails	narrowing pin
HourlyAggregates reordered so two query_stats consumers land on :00	contention pin, 1 fail	the family-only assertion, which is the point
DailyRefreshStartOffset → 1 day	daily pin, 1 fail	both treatment pins
AddDailyRefreshPolicySql passes a phase	daily pin, 1 fail	both treatment pins
RollupStoreHorizon back to RawMaxAge alone	both horizon pins, 2 fails	both treatment pins
PlainStoreHorizon collapsed onto RollupStoreHorizon	plain-store pin, 1 fail	the min-relationship pin
HorizonFor ignores its argument	plain-store pin, 1 fail	the min-relationship pin
fixed_schedule => true dropped from the converge	converge pin, 1 fail	both treatment pins
comment-only control	—	all 45

The reorder row is a control as well as a red-first proof: it is exactly the case the family-only assertion would have missed, and it stays green there while the generalized guard fails naming collect.query_stats and both minutes.

What was unverified is now measured, and two of the four assumptions were wrong. There is no local TimescaleDB rig here, so the live test was the only way to settle them, and it settled them by failing twice. The catalog join was wrong (timescaledb_information.jobs reports the user view, not the materialization hypertable, so the read matched nothing and the converge would have reported a tidy zero everywhere). The if_not_exists premise was wrong (22023 on a differing window, not a quiet -1), which is what moved the converge ahead of the create. The other two hold: alter_job takes fixed_schedule and initial_start together on this runtime, and a policy created with initial_start does come back fixed_schedule = true — asserted, because the converge's no-op-ness rests on it.

Still not verified. The plain-PostgreSQL horizon branch is pinned by unit assertion but not exercised live — the fixture has TimescaleDB, so nothing runs the hasContinuousAggregates: false path against a store. A store on a TimescaleDB too old to expose initial_start is untested: the converge read fails, which is caught, logged at Debug and returns 0 — the same handling the compression converge has, but nothing exercises it. The 15-minute/4-slot grid over thirteen policies is the measured configuration rather than the best one; spreading over thirteen distinct minutes would contend less but is not what the 26 s / 2 s / 864 s / 140 s figures were taken on. The daily tier keeps finish-to-start scheduling, so the drift argument technically applies to it too — it just has 24x the cadence headroom. And compression policies are not pinned: they have no initial_start, so they still drift, and a compression tick wandering into a refresh window is how the convoy heads. A 14:52 collision was watched for and did not fire, but only because that compression run had nothing eligible (0 s) and that refresh run happened to take 194 s instead of its 864 s — two favourable coincidences, not a demonstration of safety. That is a required follow-up, not something this change addresses. None of the production figures quoted here were re-measured by me; they are relayed from #3012's own comments and from the monitoring session's probes.

Also not verified: whether re-phasing on the next service restart lands the live store's jobs on the same slots they were hand-set to. It will not necessarily — code wins by design — but every slot is 15 minutes from its neighbours either way, and at 24% of cadence the heavy refresh fits in any of them.

CHANGELOG entry

- **The hourly continuous-aggregate refreshes re-materialize a day instead of three, and start on distinct minutes of the hour** ([#3012]) - each hourly refresh carried `start_offset => INTERVAL '3 days'` against a hypertable with 4-day raw retention, so every run re-scanned roughly three quarters of the table. A refresh's cost is set by the WINDOW, not by the rows that arrived in it, which is why the measurement points the way it does: duration rose **~8x** while rows arriving per hour **fell ~3x** (999,101 down to 359,605) and run counts moved +23%. The heaviest hourly refresh reached **3,301-6,330 s against its own 1-hour cadence - 118-175% of it**, at which point each run started into the tail of the last, and the lock queue did the rest: the running refresh holds `AccessShareLock`, the compression policy on the same hypertable queues an `AccessExclusiveLock` request behind it, and a **queued** exclusive blocks every subsequent shared request, so collector store-writes convoyed behind a lock nobody held - **40 of 40** collector write errors in a 7-day window fell inside a running refresh or compression window, at worst three servers' collectors failing **11 ms apart**. On the narrowed window the same refresh runs **864 s, 24% of cadence**, so the overlap is structurally impossible rather than currently absent; ungranted locks 0 on every sample since, service-log errors 61/hr -> 3/hr, relaunch-skips 87/hr -> 7/hr. **Both treatments ship, because they address different halves**: the heavy refresh is still ~33x its lightest sibling on the 1-day window, so narrowing is what makes it finish and the 15-minute phase grid is what keeps its 864 s invisible to everything else - and naming `initial_start` also switches each job to a FIXED schedule, without which TimescaleDB computes the next start from the previous FINISH and one convoy phase-locks a family permanently (the incident's three jobs, on unrelated schedules with very different workloads, finished within 119 seconds of each other and re-started together every hour after). The grid is keyed on the VIEW NAME via `HourlyRefreshPhaseOrder`, never on a job id - job ids are per-deployment. **Its invariant is asserted over every relation two policies contend for**, not just the family the incident evidence named: `collect.query_stats` feeds three hourly policies whose minutes are distinct only incidentally, and phases collide whenever two positions differ by a multiple of the slot count, so the guard derives each view's contended relation from the shipped CREATE text and counts a view that is itself a source into its consumers' group - refreshing it WRITES what they READ. **The hand-applied stagger was measured decaying**: two hours after it was applied, every job read `fixed_schedule = f` and only one of six hourly refreshes was still on its slot, and the heavy refresh's next start was exactly its previous FINISH plus an hour - so it drifts by its own runtime every cycle and laps the hour in days. That is why `initial_start` is not decoration. **Existing stores are converged, and the converge runs BEFORE the create rather than after - a measured ordering requirement.** `add_continuous_aggregate_policy` does not behave like the `add_compression_policy` / `add_retention_policy` no-ops [#1778] and [#1937] were built on: it returns -1 only when the window MATCHES, and against a differing window it raises **22023 refresh interval overlaps with an existing continuous aggregate policy**. So the create sweep running first would raise on all thirteen hourly views of every already-deployed store, swallow each in its per-aggregate catch, and report the aggregates as unready while the windows stayed at three days. Converging first leaves the create looking at policies that already match. The converge selects only jobs that DIFFER, compares `start_offset` as SECONDS so `1 day` and `24:00:00` cannot read as a difference and re-alter the same job every start, and names no `scheduled` argument so it cannot arm a paused job. It keys on the VIEW, recovered by matching a job against EITHER its user view or its materialization hypertable - because `timescaledb_information.jobs` resolves a continuous-aggregate job back to its user view, so the materialization-only form that shipped first read back nothing at all and would have reported a tidy zero on every store. Both of those were caught by a live-store test, not by review or by reading. **The DAILY tier is deliberately untouched** at 3 days and finish-to-start, and `RefreshPhaseMinutesFor` throws for a daily view so one cannot be dragged onto the grid: a daily job's 3-day window is 3 days of scan against an 86,400-second cadence rather than a 3,600-second one, it never appeared in the convoy, and those 3 days are the buffer the hourly rollups' 90-day drop leans on - the regression this invites is a tidy-up that makes every window "consistent". **The `start_offset`-versus-retention coupling is fixed, not just the constant**: `QueryStoreBackfill.Horizon` was `RetentionTierRouter.RawMaxAge` alone, which read as correct only because the refresh window was ALSO 3 days - the retention term standing in for the refresh term, so every retention increase silently bought more refresh cost. It is now the smaller of the two conditions that both have to hold where a backdated slice lands: raw retention must not immediately drop the rows AND the hourly aggregates must still re-materialize the buckets touched. The rollups are materialized-only, so a bucket outside the refresh window keeps what it was materialized with and backfilled rows go invisible at hourly grain while staying visible at raw - a split between tiers, not a delay. The cost is stated rather than hidden: the horizon narrows from 3 days to **23 hours** (the window minus one refresh interval, because the window slides forward by that much between runs). A per-job `lock_timeout` wrapper was considered and left out: every TimescaleDB background job runs as the same role as the collectors, so a role-level `lock_timeout` would cap collector sessions too, and a per-job wrapper is a separate decision. `max_concurrent_sweeps` needed no change - all three code defaults already read 4. Proven red eleven ways, one mutation at a time, restored byte-identically against a source hash with the tree committed first: the narrowing and the stagger are each red on their own mutation while the other stays green, reverting BOTH reddens three disjoint tests rather than one (so neither pin wears two hats), and reordering the hourly list so two `query_stats` consumers share a minute reddens the contention guard while the family-only assertion stays green - which makes that mutation a control for the new check as well as a proof of it. Plus a comment-only control. Compression policies are deliberately NOT pinned here and still drift; that is a required follow-up. No schema change and no version bump.

Needs [#3012]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/3012 in the link-ref block.

Gate

Likely dual-gate — it changes a budget/timeout-adjacent configuration family. Not merging.

[claude]

Review

Went through the full diff (TimescaleSupport.cs, QueryStoreBackfill.cs, RetentionTierRouter.cs, RollupBackfill.cs, ComposeSourceRouter.cs, DarlingWorker.cs, and both test files) against dev.

Parity: This is Darling/TimescaleDB-only infrastructure (continuous-aggregate refresh policies). Lite has no TimescaleDB or continuous-aggregate concept — verified no TimescaleSupport/continuous_aggregate references exist under Lite/. No parity drift.

Correctness: Traced through the new constants (HourlyRefreshStartOffset = 1 day, RefreshPhaseStepMinutes = 15, HourlyRefreshPhaseOrder with 13 entries), the RefreshPhaseMinutesFor throw-on-unregistered-view guard, the min() derivation of QueryStoreBackfill.Horizon (23h = min(72h raw, 23h refresh-reachable-depth) — matches the pinned test value), and the UTC date_trunc/AT TIME ZONE arithmetic in both the create and converge SQL. All internally consistent, and the daily tier is correctly excluded from the phase grid by membership rather than name-matching. No injection risk — every interpolated value in the new SQL is a compile-time constant, and the mutating converge statement (SetContinuousAggregateRefreshSql) parameterizes the job id, offset, and phase properly, following the existing jsonb_set(...,'{drop_after}',...) precedent from ConvergeRetentionHorizonSql.

One finding posted inline: ConvergeContinuousAggregateRefreshAsync — the function that fixes already-deployed stores, which is the actual incident scenario — has no live-Postgres test, unlike its siblings ConvergeCompressionScheduleAsync (dedicated end-to-end live test) and ConvergeRetentionHorizonSql (exercised live via EnsureRetentionPoliciesAsync). Only string-shape assertions exist today. Details and a suggested test shape are on the inline comment.

Nothing else stood out — no missing-index-DMV suggestions, no other performance or security concerns.

[claude]

Review

This is a Darling/TimescaleDB-only change (TimescaleSupport, QueryStoreBackfill, RetentionTierRouter, RollupBackfill, DarlingWorker, ComposeSourceRouter, plus tests) — no Lite counterpart exists for continuous-aggregate refresh policies, so Lite/Darling parity is not applicable here.

Walked the core logic carefully:

• RefreshPhaseMinutesFor (index % RefreshPhaseSlots * RefreshPhaseStepMinutes) correctly gives the tested query_store_stats family (indices 2, 4, 5 → :30/:00/:15) distinct slots. With 13 policies over only 4 slots, other unrelated views do land on the same minute (e.g. query_stats_hourly, query_store_stats_interval_hourly, session_stats_baseline, and memory_baseline all get :00) — but per the docstring this is deliberate (measured only against the family that actually shared a source hypertable in the incident), not a bug.
• QueryStoreBackfill.Horizon = min(RawMaxAge, HourlyRefreshStartSpan - HourlyRefreshScheduleSpan) = min(3d, 23h) = 23h, matching the pinned test.
• ConvergeContinuousAggregateRefreshAsync: correctly scoped to hourly-only views (via HourlyRefreshPhaseOrder membership), compares start_offset as seconds (avoiding the 1 day/24:00:00 text-equality trap), never names scheduled => (can't arm a paused job), and is per-job failure-isolated. The jobs → continuous_aggregates join on materialization-hypertable schema/name matches TimescaleDB's documented catalog columns as far as I can tell, though the PR description itself already flags this as one of the things only the live-Postgres CI leg can confirm — worth watching that job.
• AddContinuousAggregatePolicySql's new signature (startOffset, endOffset, scheduleInterval, phaseMinutes) has no other call sites outside this file/tests, so the parameter-shape change is safe.
• No SQL injection surface change: all interpolated values (view, offsets, phase minutes) come from internal constants/derived enums, never external input — same trust model the file already used.

Left one minor inline nit on a comment in DarlingWorker.cs (identifier embedded in prose in a plain comment, plus a line that didn't get re-wrapped). Otherwise this matches the T-SQL/C# style conventions (schema-qualified SQL, naive-UTC handling, WHY-focused comments) and the extensive mutation-tested pins described in the PR body look sound from a static read. No correctness, security, or performance-regression issues found.

[claude]

Reviewed. This is a Darling-only change (TimescaleDB continuous-aggregate refresh timing); Lite has no CAGGs/tiered retention (Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs says so explicitly), so there's no Lite/Darling parity gap here. No T-SQL, no migration rung, no version bump — consistent with the PR description.

Traced the core logic end-to-end (TimescaleSupport.AddHourlyRefreshPolicySql/AddDailyRefreshPolicySql/RefreshPhaseMinutesFor, ConvergeContinuousAggregateRefreshAsync + its read/write SQL, QueryStoreBackfill.Horizon's min-of-two-conditions derivation, and the DarlingWorker startup ordering) and it all holds together: the daily tier is correctly excluded from the phase grid and from the converge (membership-based, not name-matching), the converge's stale-detection and idempotency logic checks out against if_not_exists's documented no-op behavior, the job_id/::integer cast avoids the #1586 trap, and the UTC-anchored initial_start math is consistent between the CREATE and ALTER paths.

Left one inline comment: the "policies sharing a hypertable land on different slots" guarantee the phase grid is supposed to provide is only asserted by test for the query_store_stats family, not for collect.query_stats itself — which also feeds three entries on HourlyRefreshPhaseOrder and is one of the two dominant tables named in the incident. Today's manual ordering happens to keep them on distinct slots, but nothing pins that, so a future reorder/insert could silently reintroduce the exact convoy this PR fixes. Everything else looks solid — no correctness, security, or performance issues found.

[claude]

Reviewed. This is Darling-only (TimescaleDB/PostgreSQL refresh-policy scheduling); Lite has no continuous-aggregate concept, so there's no Lite/Darling parity drift to flag here.

Traced the core logic and didn't find a correctness bug:

• QueryStoreBackfill.Horizon = min(RawMaxAge, HourlyRefreshStartSpan - HourlyRefreshScheduleSpan) works out to 23h (72h vs 23h), matching the pinned test, and its cross-class static-field dependency on TimescaleSupport's constants is safe under C#'s type-initialization ordering.
• RefreshPhaseMinutesFor throws (rather than defaulting) for any view not in HourlyRefreshPhaseOrder, including every daily view — confirmed the daily aggregates never get initial_start and stay on the 3-day/finish-to-start policy.
• HourlyRefreshPhaseOrder has 13 entries (6 named + 7 BaselineAggregates), matching the test's expected count, and the three query_store_stats family members land on distinct 15-minute slots (30/0/15) — the one collision case the design actually cares about (shared source hypertable → shared compression-lock contention).
• ConvergeContinuousAggregateRefreshAsync's jobs ⋈ continuous_aggregates join matches either the user-view identity or the materialization-hypertable identity; per the comment these are schema-disjoint (collect vs _timescaledb_internal) so a job can match at most one row — no double-count risk. start_offset is compared as extracted seconds (not string) so 1 day / 24:00:00 can't spuriously read as different. scheduled is deliberately left unnamed in the alter_job call so a paused job can't be armed.
• The new SQL follows the Postgres-section conventions in CONTRIBUTING.md: schema-qualified objects, AS on aliases, and the double AT TIME ZONE 'UTC' idiom for computing a UTC-anchored timestamptz (needed here because initial_start is a timestamptz parameter to alter_job/add_continuous_aggregate_policy, not a naive-UTC column).
• All interpolated SQL values (view names, offsets, phase minutes) come from internal constants/computed ints, never external input — no injection surface.
• ConvergeContinuousAggregateRefreshAsync is correctly gated behind _timescaleAvailable and sits in the same try/catch as its ConvergeCompressionScheduleAsync sibling in DarlingWorker.cs, matching the existing pattern.

Nothing else stood out. The mutation-testing table and the live-store test give good confidence for the parts that can't be checked from the diff alone (the four Postgres-runtime assumptions the PR description calls out as unverified locally).

[claude]

Reviewed the diff (Darling-only: TimescaleSupport.cs, QueryStoreBackfill.cs, RetentionTierRouter.cs, RollupBackfill.cs, DarlingWorker.cs, ComposeSourceRouter.cs, plus tests).

Parity: No Lite counterpart exists for continuous-aggregate refresh scheduling — Lite runs on embedded DuckDB with no TimescaleDB jobs/policies, so this is correctly Darling-only. No drift to flag.

Style: The new/changed SQL is PostgreSQL (TimescaleDB), not T-SQL, so the CONTRIBUTING.md T-SQL conventions (OPTION(RECOMPILE), COUNT_BIG, etc.) don't apply here. Table aliasing with AS is used consistently in the new ContinuousAggregateRefreshStateSql/SetContinuousAggregateRefreshSql.

Correctness spot-checks performed:

• RefreshPhaseMinutesFor's index % RefreshPhaseSlots * RefreshPhaseStepMinutes — operator precedence is left-to-right for %/*, giving the intended (index % 4) * 15. Verified the query_store_stats family (hourly/interval/corrected) lands on distinct slots (30/0/15) per the new test.
• QueryStoreBackfill.Horizon = min(RawMaxAge=72h, RefreshReachableDepth=23h) → 23h, matching the pinned TimeSpan.FromHours(23) test.
• The ContinuousAggregateRefreshStateSql OR-join (view-schema vs. materialization-hypertable-schema) can't double-count a job — the two schemas (collect vs _timescaledb_internal) are disjoint, so at most one branch matches per row.
• ConvergeContinuousAggregateRefreshAsync's blanket try/catch around the read loop (LogDebug + return 0 on any exception) looked at first glance like it could swallow a real failure and silently skip convergence for every hourly policy — but it mirrors the exact same pattern already used by ConvergeCompressionScheduleAsync, so it's consistent with established conventions rather than a new gap.
• No orphaned callers of the old AddContinuousAggregatePolicySql(view, endOffset, scheduleInterval) signature remain; all call sites were updated to the new AddHourlyRefreshPolicySql/AddDailyRefreshPolicySql split.
• All interpolated SQL values (view names, offsets, phase minutes) come from static internal constants/enumerations, never user input — no injection surface.

Wasn't able to run dotnet build/dotnet test in this sandbox (tool approval required for dotnet invocations), so this is a static review only — worth having CI confirm the build/live-Postgres test (EndToEnd_HourlyRefreshWindow_...) before merge, though the local Assert.Equal/Assert.Throws tests look sound by inspection.

No blocking issues found.

[claude]

Reviewed the Darling-only changes (TimescaleSupport, QueryStoreBackfill, DarlingWorker, RetentionTierRouter, RollupBackfill, tests). No Lite counterpart exists for continuous aggregates, so there's no parity gap to flag here.

Overall the mechanism (narrow the hourly refresh window + phase-stagger on a fixed schedule + converge existing stores before the idempotent create) is well-reasoned and thoroughly pinned by tests, including the ordering requirement around add_continuous_aggregate_policy's 22023 raise. Left two inline notes:

1. Doc-comment misplacement in TimescaleSupport.cs — the pre-existing BaselineAggregates summary wasn't moved when HourlyAggregates was inserted above it, so HourlyAggregates now carries two stacked <summary> blocks (one describing the wrong array) and BaselineAggregates is left undocumented.
2. Possible regression on plain-PostgreSQL stores — QueryStoreBackfill.Horizon now factors in the hourly refresh window (HourlyRefreshStartSpan - HourlyRefreshScheduleSpan = 23h) even though that only matters when TimescaleDB continuous aggregates exist. Since QueryStoreBackfill runs unconditionally regardless of whether Timescale is enabled, this looks like it narrows backfill/catch-up depth from ~3 days to 23 hours on the documented plain-Postgres deployment mode, with no corresponding correctness need there.

Both are called out with line references in the inline comments. Nothing else stood out on the correctness/security/perf axes — SQL text is parameterized where it takes runtime values ($1::integer/$2/$3), the alter_job un-named-params-mean-unchanged discipline is preserved, and the daily tier is correctly excluded from the phase grid.

[claude]

Reviewed the diff (Darling-only: TimescaleSupport.cs, QueryStoreBackfill.cs, RetentionTierRouter.cs, RollupBackfill.cs, ComposeSourceRouter.cs, DarlingWorker.cs, and the three test files). This isn't T-SQL, so the collector-query style rules (OPTION(RECOMPILE), etc.) don't apply; the embedded PostgreSQL follows the naive-UTC / bound-parameter conventions in CONTRIBUTING.md correctly (alter_job's job_id is bound as $1::integer, start_offset/phase as $2/$3; the raw-interpolated SQL only ever splices internal constants and enum-backed view names, never external input, so there's no injection surface there).

Logic check (traced by hand, not just trusting the test narrative):

• RefreshPhaseMinutesFor's round-robin (index % 4 * 15) does put several non-contending views on the same slot (e.g. query_stats_hourly and the 2nd/6th baseline aggregates all land on :00), but I traced the actual FROM collect.<x> source of every hourly CREATE and none of those same-slot views share a source relation — the collision guard's derivation is correct against the current view set.
• QueryStoreBackfill.Horizon = min(RawMaxAge, HourlyRefreshStartSpan - HourlyRefreshScheduleSpan) = min(3d, 23h) = 23h, matches the pinned test.
• Converge/ensure ordering in DarlingWorker.cs (ConvergeContinuousAggregateRefreshAsync before EnsureContinuousAggregatesAsync, both gated on _timescaleAvailable, after DropStaleContinuousAggregatesAsync) matches the documented 22023-avoidance requirement.
• Daily views are correctly excluded from the phase grid (RefreshPhaseMinutesFor throws, AddDailyRefreshPolicySql never calls it), so the "make every window consistent" regression this PR calls out as a foreseeable footgun can't happen by accident here.

Lite/Darling parity: No drift — confirmed Lite has no TimescaleDB/continuous-aggregate concept at all (DuckDB + Parquet archive), so there's nothing on the Lite side for the refresh-window/staggering treatment to mirror.

One stale comment this PR should probably also fix while it's touching this exact number: Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs lines 29-35 says "Lite can safely dig ~30 days where Darling stops at its raw tier's 3" — that "3" was accurate when QueryStoreBackfill.Horizon was RawMaxAge alone (3 days), but this PR drops Darling's horizon to 23 hours (min(RawMaxAge, refresh-reachable-depth)), so the comment now understates Darling's actual constraint by ~3x and no longer attributes it to "raw tier" alone. Not a functional bug (Lite's own horizon logic is untouched and independently correct), just a doc-drift nit worth a one-line fix given how load-bearing accurate comments are in this codebase.

No missing-index DMV suggestions (none relevant here anyway — this is scheduling/window logic, not a query).

[claude]

Reviewed the diff (TimescaleSupport.cs, QueryStoreBackfill.cs, DarlingWorker.cs, RetentionTierRouter.cs, RollupBackfill.cs, ComposeSourceRouter.cs + the test files) against the correctness/parity/security/perf criteria.

Correctness — checked and holds up:

• Verified the new regex-based contention guard (HourlyRefreshPhaseOrder / HourlyRefreshDefinitions / the FROM\s+collect\.(\w+) extraction in HourlyRefreshPhases_AreDistinctForEveryRelationTwoPoliciesContendFor) against the actual CREATE SQL text for all 13 hourly definitions by hand: collect.query_stats really does feed exactly 3 hourly policies (query_stats_hourly, query_stats_db_hourly, query_stats_baseline) and collect.query_store_stats feeds exactly 2 (query_store_stats_hourly, query_store_stats_interval_hourly), matching the test's own positive-control counts. The phase math (index % RefreshPhaseSlots * RefreshPhaseStepMinutes) gives each of those groups distinct slots given the current ordering.
• HourlyAggregates/BaselineAggregates/HourlyRefreshPhaseOrder field-initialization order in TimescaleSupport.cs is safe: the CreateXxxSql/view-name fields HourlyAggregates forward-references are const string, so C#'s static-field declaration-order rule (which would matter for static readonly) doesn't apply there, and HourlyRefreshPhaseOrder/HourlyRefreshDefinitions are declared textually after both HourlyAggregates and BaselineAggregates, so no null/default-value race at type-init time.
• QueryStoreBackfill's new _hasContinuousAggregates provider has exactly one production call site (DarlingWorker.cs), and it's wired to the same _timescaleAvailable flag set earlier in the same startup path — the default-true fallback (used only by tests/non-Darling hosts) is the conservative direction as documented, since it can only under-reach on a plain-Postgres store, never produce the silent raw/rollup split.
• Converge-before-ensure ordering, the 22023-vs--1 distinction for add_continuous_aggregate_policy, the daily-tier exclusion by HourlyRefreshPhaseOrder membership, and the min() horizon relationship in QueryStoreBackfill all check out against the code that implements them.
• No leftover stale "3-day"/hourly-window references found elsewhere in the touched files — the remaining "3 days" mentions are all legitimately about the (unchanged) daily tier.

Parity: No Lite changes, and none are needed — continuous aggregates / TimescaleDB refresh policies are a Darling-only (PostgreSQL/TimescaleDB) concept with no DuckDB/Lite counterpart, so this doesn't fall under the Two-Store Parity rules in CONTRIBUTING.md.

Security: No new user-controlled input reaches any interpolated SQL — the only interpolated values (view, phase, offsets) come from hardcoded internal constants/enums, never from configuration or network input. The parameterized paths (SetContinuousAggregateRefreshSql) correctly bind job_id/start_offset/phase positionally and cast job_id to ::integer per the noted #1586 trap.

Performance: The added converge query + per-stale-job alter_job calls run once at service startup against a small (~13-row) job set — negligible overhead, and the actual fix is a clear regression-reducer (864s vs 3,301-6,330s per the PR's own measurements).

One non-blocking note carried over from the PR description itself: compression policies are explicitly still unphased and can still collide with a refresh window (acknowledged as a required follow-up, not addressed here) — worth making sure there's a tracking issue for that so it doesn't get lost, since it's the same lock-queue mechanism this PR otherwise closes off for refreshes.

No blocking findings from this pass.