Lite runs housekeeping on its own loops, so a long archive or retention step no longer skips collection slots (#5731)

[erikdarlingdata]

Claude posting for Erik Darling.

Fixes #5731. Builds on #5730; merge after it.

Why

Lite's collection loop awaited six housekeeping calls after the collectors (archival, Query Store backfill, retention, findings cleanup, dismissed-alert cleanup, analysis). A long archive, compaction or analysis pass kept the loop from its next slot, so collection slots were skipped (the skips #5730 now reports).

What changes (lane A1: the loops)

• CollectionBackgroundService.ExecuteAsync no longer awaits any housekeeping call. Three loops run through the existing RunPileupCadenceAsync (no new loop type), each ticking every minute (HousekeepingInterval), behind the same gate as before (!IsPaused && !_collectorService.LocalDatabaseIsDown()):
  • maintenance: archival, retention, findings cleanup, dismissed-alert cleanup, in that order (archival and retention both delete or merge parquet files; the purges follow archival because a reset empties and restores the dismissed-alert sidecar);
  • Query Store backfill;
  • analysis.
• No loop ticks before the collection loop's first round has finished (a TaskCompletionSource set after round one, and again after the while so a stop cannot strand a loop). Nothing used to run before the first round, and the backfill is due on cycle 1.
• A step that throws costs that step for that tick, not the steps after it or the loop.
• Shutdown awaits all four loops (await pileupLoop; kept, then Task.WhenAll(housekeepingLoops)).
• The collection loop keeps the connection check, RunDueCollectorsAsync, LogProcessMemory, the wait and the Lite logs and records collection cycles it skipped (#5643) #5730 report. Its Laps shrink to connection_check and collectors.
• Every RunXIfDueAsync keeps its name, signature, due check, stamp and text; the constructor is unchanged.
• The Query Store backfill comment "sequence is the concurrency bound" is rewritten: the per-server QueryStoreServerGate is the bound now.
• Seams: StartHousekeepingLoops(loops, interval, canRun, firstRound, delay, onFault, token), BuildHousekeepingLoops(), records HousekeepingLoop / HousekeepingStep.

Not fixed by this PR: the 512 MB size-triggered reset (ArchiveAllAndResetAsync) still pauses collection for its whole length (#5733). The bounded write-lock waits are lane B's; the housekeeping ledger and the CollectionCycleSkip.Build message change are lane A2.

FindingStore: does anything update old rows?

No. Lite/Analysis/FindingStore.cs only INSERTs into analysis_findings (batch insert) and DELETEs (CleanupOldFindingsAsync, rows older than the retention horizon); no UPDATE of analysis_findings exists anywhere under Lite/. Cleanup (read-lock DELETE of old rows) overlapping an analysis append (disjoint new rows) is the case the class note and DuckDbInitializer.s_dbLock doc say was measured safe, so letting the two overlap is fine.

Test plan

• New HousekeepingLoopTests (5): a blocked maintenance step does not delay 5 collection slots (CountSkippedSlots 0 each, real WaitForNextCycleAsync on a fake clock); step order; no tick before the first round; cancel ends all loops, including one still waiting for the first round; a throwing step costs only itself and a gated tick runs nothing; source pin on the real step order, loops started before while, Task.WhenAll at shutdown.
• Changed CollectionCycleSkipTests.TheLoop_TimesEveryAwaitedStep_AndReportsAfterTheWait: two Lap( calls, no await Run*IfDueAsync in the loop body, the loops start before while.
• Revert check: with await RunRetentionIfDueAsync(); put back after Lap("collectors"), the changed Lite logs and records collection cycles it skipped (#5643) #5730 test and the source pin in HousekeepingLoopTests both fail (2 of 30 failed); restored and green.
• Lite.Tests: 160 in the classes that read the file (HousekeepingLoopTests, CollectionCycleSkipTests, LitePileupCadenceTests, LiteSampleAgeDeliveryTests, HousekeepingClockStepTests, ArchiveViewRefreshAfterDeleteTests, AnalysisBudgetTests, FindingsRetentionHorizonPinTests, EntraCredentialSelectionOrderingTests, CollectionCycleResumeTests, ArchiveResetRestoresStateTests, WriteLockBudgetTests, SharedBaselineCacheTests, AnalysisEnabledSettingTests, AzureMasterLiteWiringTests), 0 failed. *Census* 67, -trait Stage=Guard 496, 0 failed.
• Darling.Tests: AlertMasterSwitchSurfaceTests 9, SameStatementPileupSourceCensusTests 5, *Census* 545 (1 skipped), *Ratchet* 15, 0 failed.
• Full suites not run (CI decides). No dogfood run on a live server.
• Dev was merged after Lite logs and records collection cycles it skipped (#5643) #5730's squash (d10adfc) with 64fa26a as the merge base, so the only difference from dev is this PR's own 20 files. Merge head a79b2b8: Lite and Lite.Tests build 0 errors, 0 warnings; targeted classes plus every Lite.Tests Census class 411 run, 0 failed; -trait Stage=Guard 502 of 502, 0 failed.

Housekeeping ledger (lane A2)

• New Lite/Services/HousekeepingLedger.cs. Time(name, fn) runs a step and records its name, start and length when it ran 1 s or more; the newest 32 are kept. One lock guards every access (three loops write, the collection loop reads). A step that throws is recorded as failed (a stop is not) and the exception goes on unchanged. A step still running is listed too, with its length so far, because the step holding the store when the report is written has not ended yet.
• RunHousekeepingLoopAsync runs each step through the ledger (the one foreach that knows loop and step names). StartHousekeepingLoops gained a trailing optional ledger parameter, so the A1 tests and the seam are unchanged; the service passes its own.
• CollectionCycleSkip.Build gained optional housekeeping and storeLockHolder parameters. ReportSkippedSlotsAsync passes ledger.Overlapping(previousCycleStart, nextCycleStart) and the first StoreLockHolders.Snapshot() write holder held 1 s or more. With no overlapping entries the message is the old text, byte for byte (pinned), so the earlier pins keep their text; the holder rides with the housekeeping sentence and is never added alone. With entries the message gains: Housekeeping in that window: archival 252s, retention 3s. (the longest four, a repeated name summed, marked "(failed)" or "(still running)"), then The store write lock was held by ArchiveService.ExportAsync for 252s when this was written. Durations use the existing Format (252s, not 4m12s). slowest_item is unchanged: still the cycle's own slowest step.
• StoreLockHolders.Snapshot() shows who holds the lock now, not who held it during the gap, so the sentence says "when this was written".
• Overlap: a step overlaps when it started before the gap ended and ended after the gap began; a step ending exactly at the start or starting exactly at the end does not.

Test plan (lane A2)

• New HousekeepingLedgerTests (11): overlap math at each edge, under 1 s ignored and exactly 1 s kept, failed step recorded and exception rethrown, running step listed, newest 32 kept, three writers plus a timed writer and a reader with no exception and exactly 32 kept, the loop records slow and failed steps, Build text with and without entries (and a holder alone adds nothing), longest four / summed name / failed / running / holder, source pin on the wiring. Added CollectionCycleSkipTests.ASkippedCycle_NamesTheHousekeepingStepsThatOverlappedTheGap_InTheLogAndTheRow (log line, row text, slowest_item unchanged).
• Revert check: with ledger.Time(...) taken out of the loop and the entries ignored in Build, 5 of 37 failed (the new report test, the loop test, the wiring pin, and the two Build sentence tests); restored and green.
• Lite.Tests: 187 in the classes that read the files (the A1 list plus HousekeepingLedgerTests), 0 failed. *Census* 67, -trait Stage=Guard 496, 0 failed.
• Darling.Tests: AlertMasterSwitchSurfaceTests, SameStatementPileupSourceCensusTests, *Census*, *Ratchet*: 569 run, 1 skipped, 0 failed.
• Full suites not run (CI decides). No dogfood run: a real skipped-slots report with a live ledger is unseen.

Double-check

• First housekeeping pass is now one tick (1 min) after round one, not in round one itself.
• Housekeeping now overlaps collectors; the 4 GB COPY memory limit against live collectors is plan risk 1 (check LogProcessMemory in a dogfood run).

SECTION: Fixed
ENTRY: - Lite housekeeping no longer holds up the collection loop ([#5740]) - archival, retention, the findings and dismissed-alert purges, the Query Store backfill and the scheduled analysis ran as steps of the collection loop, so a long one skipped collection slots. They now run on their own loops, once a minute, and start after the first collection round. A size-triggered database reset (#5733) still pauses collection while it runs. When slots are skipped anyway, the warning and the collection_log row now also name the housekeeping steps that overlapped the gap.

Retention budget and hold watchdog

Claude posting for Erik Darling.

• Retention budget. RetentionService.CleanupOldArchivesAndRefreshViewsAsync now waits for the write lock with LocalDataService.WriteLockBudget (5 s, no second number) through a RetentionService.WriteLockBudget property that a test can shorten. On StoreLockTimeoutException it logs at Information and returns null ("not done"; the return type is now Task<int?>). RunRetentionIfDueAsync returns without stamping on null, so _lastRetentionTime stays due and the next housekeeping tick retries. Only the stamp lines of RunRetentionIfDueAsync changed.
• Hold watchdog. LockReleaser.Dispose logs one AppLogger.Warn("DuckDb", ...) when a WRITE hold lasted over DuckDbInitializer.WriteHoldWarnAfter (5 s default, an instance property so a test can lower it). It names the holder from StoreLockHolders (new HolderName(id)) and the hold length. Reads pay one timestamp and nothing else.
• Tests (Lite.Tests/HousekeepingLockBudgetTests.cs, 4): a retention timeout leaves the stamp unset and the archive undeleted, and the retry stamps and deletes (plan test 6); the cleanup returns null on timeout and the count otherwise; a write hold past the threshold logs one warning with the holder; a short write hold and a long read hold log nothing. Budgets and thresholds are 30-500 ms; nothing sleeps 5 s.
• Revert checks. Stamp made unconditional: the retention test fails. Watchdog condition forced false: the watchdog test fails. Both restored.
• Run after merging the branch (0 warnings): HousekeepingLockBudgetTests 4, ArchiveViewRefreshAfterDeleteTests 4, HousekeepingClockStepTests 20, HousekeepingLoopTests 5, HousekeepingLedgerTests 11, WriteLockBudgetTests 25, TimingTestCensusTests 3, GuardStageCensusTests 9, all green. Before the merge: the 73 Lite.Tests classes that read the touched files or are Census/Ratchet classes, plus -trait Stage=Guard (496), 0 failures.
• Not run: the five Darling.Tests classes that read DuckDbInitializer.cs / CollectionBackgroundService.cs (they read other methods; not built on this laptop), and the full suite.

Bounded lock waits (lane B1)

Claude posting for Erik Darling.

The archive's write-lock waits now use a budget: ArchiveService.WriteLockBudget, which defaults to LocalDataService.WriteLockBudget (5 s shipped, the number the #5371 CHECKPOINT uses). A test sets a shorter one. On a timeout the unit is skipped and retried on a later pass, so a collector is held off by about the budget plus one chunk's hold, not by a read's whole length.

• Promote plus DELETE per table (ArchiveOldDataAsync) and the recovery of an interrupted export (DeleteArchivedRowsAsync): on StoreLockTimeoutException the temp parquet is deleted, the table is skipped and the line is Information. The clause sits ahead of the per-table Failed to archive table Error catch. The rows stay in the table and the next pass takes them. A recovery timeout leaves the journal, so the export skips that table (incomplete pass) until the next pass.
• Compaction swap per group: on a timeout the group's merged temps are removed and the group is left for the next pass. The lock is taken before the swap touches any file, so the inputs are as the merge found them.
• Swap-journal replay: on a timeout nothing is resolved, every journal's month (or day) stays out of this run's merge, and the next pass replays them.
• Return value: ArchiveOldDataAsync returns Task<bool>, true when every table was done. It is false for a skipped unit, a table whose earlier journal could not be finished, or a stop on the token. CompactParquetFiles returns bool too (false when a lock timed out); both existing call sites and the roughly 30 test call sites that call it as a statement compile unchanged.
• Retry stamp: RunArchivalIfDueAsync stamps _lastArchiveTime through ArchiveStampAfterRun(complete, now). True stamps the clock; false stamps 5 minutes (ArchiveRetryDelay) short of due, read by the same HousekeepingIsDue rule. Only the stamp lines of that method changed.
• Cancellation: optional CancellationToken on ArchiveOldDataAsync, checked between tables and before compaction. A stop returns false (not an exception, so shutdown does not log an Error), and the archive views are still rebuilt for tables already promoted. A COPY in flight is not interrupted, and the zero-wait probe of the archive lock and the read connection open take CancellationToken.None on purpose. RunArchivalIfDueAsync does not pass a token yet; its signature is lane A1's, so wiring the loop's stopping token is one argument there.
• Stop token reaches the pass: RunArchivalIfDueAsync(CancellationToken cancellationToken = default) hands the housekeeping loop's stop token to ArchiveOldDataAsync, so a service stop ends an archive pass between tables and leaves _lastArchiveTime on the retry stamp. Pinned by RunArchivalIfDueAsync_AStopDuringThePass_EndsItBetweenTables_AndStampsTheRetry, which fails when the token is not passed through.
• Left alone: the reset sites (ArchiveAllAndResetAsync's export and restore locks) stay exclusive, and so does DeleteCollectorStateKeyAsync.

Test plan (lane B1)

New Lite.Tests/ArchiveLockBudgetTests.cs (named so, because lane B2 already has HousekeepingLockBudgetTests; one timing-census allow-list entry added, the 20 s bound is a hang guard):

• a promote behind a held read gives up within the budget, returns false, leaves no parquet, temp or journal, keeps all rows, and a reader that keeps asking during the wait gets in within budget plus slack
• the skipped tables are archived on the next pass once the lock is free (returns true)
• a compaction swap behind a held read returns false and leaves every file as it was; the next pass compacts the group
• a swap-journal replay behind a held read returns false, keeps the journal and leaves the group unmerged
• cancellation: a token cancelled after the first table's promote stops before the second (false, second table untouched); a token cancelled up front archives nothing and skips compaction
• a full pass returns true
• ArchiveStampAfterRun: complete is a full interval away, incomplete is due in exactly 5 minutes
• RunArchivalIfDueAsync through reflection, behind a held read: stamp is about 5 minutes from due; then with the lock free, the stamp is the clock
• Revert check: with AcquireWriteLock(WriteLockBudget) changed back to AcquireWriteLock() everywhere in ArchiveService.cs, the promote test fails ("still waiting for the write lock after 30 s: the wait has no budget"), so does the next-pass test, and so does the compaction test. Restored afterwards.
• 856 passed, 0 failed across every Lite.Tests class that reads ArchiveService.cs or ArchiveOldDataAsync plus the housekeeping, store-lock and cycle classes (22 class filters) on the pre-merge commit
• after merging lane A1/B2: 88 passed, 0 failed over my class, HousekeepingLockBudgetTests, HousekeepingLoop, HousekeepingLedger, HousekeepingClockStep, CollectionCycle*, ArchiveViewRefreshAfterDelete, ArchiveResetRestoresState, WriteLockBudget, every *Census* and *Ratchet* class and -trait Stage=Guard; one failure there (TimingTestCensusTests, my new class was not on the allow list) was fixed and TimingTestCensusTests plus both budget classes pass: 16 of 16
• build: 0 warnings, 0 errors
• full Lite.Tests suite and CI: not run on this laptop

Double-check

• A time-due pass that skipped a unit retries after 5 minutes, but a pass that throws still leaves _lastArchiveTime unstamped (as before), so it retries every tick.
• ArchiveAllAndResetAsync runs the interrupted-work recovery unbounded (see Review round 1); only the post-reset compaction stays on the bounded budget, and a timeout there defers it to the next pass without failing the reset.

Review round 1

Fixes for review 5498955923 (commits c9b6167, 0a910ad). The ENTRY line above stays true as written.

• Reset recovery is unbounded (double-archive finding). RecoverInterruptedArchiveWorkAsync, RecoverPendingArchivesAsync, DeleteArchivedRowsAsync and ReplayCompactionSwapJournals take unbounded; ArchiveAllAndResetAsync passes true, so its recovery waits for the write lock like the reset's other three locks and cannot leave a journal behind because a reader held the lock. The scheduled pass keeps WriteLockBudget and its journal check. The post-reset compaction stays bounded (a deferred compaction is idempotent; the next hourly pass merges the group), and the comment says so.
• Reset guard. Before the reset's export of each table, a surviving .archive-pending journal (the recovery failed for a reason other than the lock) is logged and the attempt is abandoned like a failed export: no file is written, every row stays in the database, and the retry backoff applies. The brief said to skip the table; skipping it and then resetting would have dropped its rows, which is the hazard the reviewer named, so the whole reset waits instead.
• Comment at WriteLockBudget names the three unbounded reset locks (unfinished-reset-export cleanup, export of every table, promote/preserve/reset) and says the recovery inside the reset is unbounded too.
• Swap-journal regex: one private static readonly Regex s_swapJournalName on the class, used at both sites.
• GetSkippedCycleSummaryAsync: the four arg_max columns are one struct_pack, so every "last" field comes from the same newest row.
• Collection log page. The log grid and get_collection_log (one server and fleet) still show collection_cycle/SKIPPED rows; the two reads carry a comment saying so, and the sentence in CollectionCycleSkip.cs now admits it. run_count counts collector rows only and the payload adds skipped_cycle_rows. The returned-time span fields stay over the page's rows, cycle rows included (comment in the payload). The tool description is NOT changed here: it is shared byte-for-byte with Darling, so a sentence about the new field and the cycle rows needs a matching Darling change.

Tests:

• ArchiveLockBudgetTests +2: a journal left by a killed promote, a read held past the budget during the reset: the reset waits, and each row is in the archive files exactly once (50/50 and 20/20 rows/distinct); a journal the recovery cannot delete (read-only) abandons the reset with no new file and a backoff.
• SkippedCycleSurfaceTests +1: the newest row with NULL step, step time and note returns NULL, not the older row's values.
• CollectionLogToolTests +2 (one server, fleet): 2 collector rows and 1 cycle row give run_count 2, skipped_cycle_rows 1, 3 runs, and the oldest-returned time is the cycle row's.
• ArchiveWatermarkCacheTests reflection call updated for the new parameter.

Revert checks (each restored): reset recovery back to bounded: the held-read test fails; guard disabled: the unfinishable-journal test fails; four arg_max calls: the NULL test fails; run_count = rows.Count: both log-page cases fail.

Run (0 warnings, Lite.Tests): 903 tests over 71 classes (every Lite.Tests class that reads a changed file, ArchiveLockBudgetTests, HousekeepingLockBudgetTests, ArchiveResetRestoresStateTests, SkippedCycleSurfaceTests, the Census classes), two failures found and fixed (a single-quoted collector name in my comment tripped NoFileUnderLite_CopiesTheCollectorNameAsASingleQuotedSqlLiteral; the reflection call above), then re-run green; -trait Stage=Guard 496 of 496. Darling.Tests (reads Lite source): McpPayloadContractCensusTests plus every Census/Ratchet class, 568 run, 0 failed, 1 skipped; skipped_cycle_rows needed no roster entry (the census has none for run_count either). Not run: the full suite and CI.

[claude]

Smaller things, and one stale invariant outside the diff

Five notes that did not warrant their own inline comment, the last of which is in a file this PR does not touch.

1. CollectionResetGate's doc now names an invariant this PR removes. Lite/Database/CollectionResetGate.cs:27–:28 reads: "CollectionBackgroundService runs collect-then-archive in a strict loop, so the reset cannot land mid-cycle there. But MainWindow.ConnectToServer — opening a server tab — calls RunAllCollectorsForServerAsync on a bare Task.Run, unsequenced against anything." After this PR the first half is false: archival is on the maintenance loop and the size-triggered reset can land squarely inside a scheduled sweep.

The mechanism still holds — RunDueCollectorsAsync registers at RemoteCollectorService.cs:557, so TryBeginResetAsync drains or defers — and the PR body flags the behaviour as known (#5733). But the class's own explanation of why the gate exists now reads backwards, and the deferral path it describes as the rare tab-open case is the common scheduled one. Worth a sentence there, since that file is where someone will go to reason about the reset.

2. The skipped_cycles block is Lite-only on a tool whose payload argues against exactly that. Lite/Mcp/McpHealthTools.cs:656–:664, about retried_reads: "Rendered anyway because both SKUs' blocks carry one field set — a client must not learn a different shape depending on which answered — and because a missing field reads as an older build rather than as an inapplicable measurement."

Darling's twin (Darling/.../Mcp/DarlingMcpDataTools.cs:2049) gains nothing here, so get_collection_health now answers with a different key set depending on the SKU. I do not think the two-store parity rule proper applies: Darling has no monolithic collection cycle on a fixed grid, DarlingCollectorRunner schedules per collector, and there is no collection_cycle row for it to summarise — so this is not one fact answered two ways. The tool description is also unchanged, so the byte-identical text does not lie to Darling's callers. But by the reasoning the file states two blocks up, the honest shape is a structurally-empty block on Darling with a note saying why, rather than an absent key. Your call; flagging it because the precedent is written down in the same payload.

3. holder.Write ? "write" : "read" is dead. Lite/Services/CollectionCycleSkip.cs:142. The only caller filters write holds already: CollectionBackgroundService.cs:361 is Snapshot().FirstOrDefault(h => h.Write && h.Held >= HousekeepingLedger.MinimumLength), so the "read" arm can never render. Either drop the conditional or widen the caller — a long read hold is in fact the thing that starves the archive promote, so handing it to the sentence may be the more useful change.

4. SkippedCycleSummary.LastDurationMs reaches no surface. It is read from the store (LocalDataService.SkippedCycles.cs:136) and asserted by SkippedCycleSurfaceTests.TheSummary_CountsCyclesAndSlots_AndTheNewestRowSuppliesTheLastFields, but it is absent from SkippedCyclesBlock (McpHealthTools.cs:728) and from SummaryLine. Every sibling field is on one of the two surfaces. If last_duration_ms belongs on the MCP block, add it; if not, the column and the property can go, and the pin with them.

5. The log watchdog writes while still holding the write lock. DuckDbInitializer.cs:388–:411 logs before ExitWriteLock, deliberately, so StoreLockHolders.HolderName can still name the holder. Correct, and the cost only lands on holds already past five seconds — but it does mean a slow AppLogger.Warn extends the very hold it is complaining about, and every new reader waits through it. Reading the name into a local before the if and logging after the unlock would keep both properties. Minor; noting it because the whole point of the budget work is that nothing unbounded sits between a writer and the collectors.

---

What I checked and found clean, so it does not get re-litigated:

• Every v_collection_log / collection_log reader in Lite/ (53 sites). Beyond the two log-page reads in the inline comment, all are safe: collector-equality predicates (BaselineProvider.EventBaselineSql:529, RunningJobs:69/113/124, JobHistory:252, QueryStats:741/1498, Config.cs:242, RuntimePrecondition:38 and the first subquery of :92, RemoteCollectorService.cs:2236/2247, QueryWindowFloor:344/388/457), status = 'SUCCESS' filters (CollectorDurationTrend:860, Blocking:1337/1372), a status = 'PERMISSIONS' filter (CollectionHealth:57), or a C#-side name set (RunTime.cs:152). The lateral joins at CollectionHealth.cs:366–:419 all key on h.collector_name, so they inherit the outer filter.
• BaselineDiscontinuity.MarkerRowsSql matches any collector's row on error_message LIKE '%<label>=%'. The new message carries no = anywhere — not in the gap sentence, not in the housekeeping sentence, and not in StoreLockHolders.Name, which is File.Method — and TheReportText_CarriesNoEpochLabel_SoTheBaselineMarkerReadNeverPicksItUp pins it. The two "No equals sign" comments earn their keep.
• StoreLockHolders.Name is Path.GetFileNameWithoutExtension(file) + "." + member, so the compile-time [CallerFilePath] never reaches the collection_log row or the MCP payload. No build-path leak.
• arg_max(x, (a, b)) tuple ordering is already in use at CollectionHealth.cs:216, so the syntax is not new ground.
• SKIPPED is already in EnumeratedCollectorDriver.CollectionLogStatuses:494, so get_collection_log's ValidateChoice does not refuse a filter for rows the store now holds.
• sqlMs/duckDbMs going nullable in WriteCollectionLogRowAsync does not change any collector row: LogCollectionAsync passes (int)sqlMs, so the HasValue branch is always taken.
• The Query Store backfill writes under a read lock with the connection opened inside it (RemoteCollectorService.QueryStoreBackfill.cs:520–:523), so moving it onto its own loop does not expose it to the reset the way [BUG] Lite - Table ".main.*" could not be found #2594 exposed the tab-open path. The rewritten "QueryStoreServerGate is the bound now" comment is accurate.
• ArchiveOldDataAsync's timeout clause deletes the right temp: exportedTemp is assigned at :315, the COPY runs at :335, and WritePendingArchive is inside the write lock at :365 — so a lock timeout leaves no journal and no promoted file, and the rows stay put. CompactParquetFiles likewise takes its lock before the swap touches a file.
• CountSkippedSlots is right at each edge I could reason about: on-time is 0, a hair early or late is 0, #4732's backward step is negative-ahead and therefore 0, and prev + n*interval gives n-1.

[claude]

Verdict: CHANGES REQUESTED

Claims

• VERIFIED — "ExecuteAsync no longer awaits any housekeeping call" — CollectionBackgroundService.cs:288-291; CollectionCycleSkipTests.TheLoop_TimesEveryAwaitedStep_AndReportsAfterTheWait asserts no await Run*IfDueAsync( in the body
• VERIFIED — "Three loops through the existing RunPileupCadenceAsync, each ticking every minute behind the same gate" — :433-450, :493, :81, gate at :231
• VERIFIED — "No loop ticks before the first round, and again after the while" — TCS at :227, set at :303 and :322, awaited at :486
• VERIFIED — "A step that throws costs that step for that tick, not the steps after it" — per-step try/catch at :498-522, cancellation rethrown at :514
• VERIFIED — "Shutdown awaits all four loops" — :327 and :338
• VERIFIED — "The backfill's 'sequence is the concurrency bound' comment is rewritten; QueryStoreServerGate is the bound" — RemoteCollectorService.QueryStoreBackfill.cs:37-43, gate real at :103-113
• VERIFIED — "No UPDATE of analysis_findings anywhere under Lite/" — grep returns nothing; the overlap is INSERT plus DELETE
• VERIFIED — "Retention returns null on timeout and RunRetentionIfDueAsync returns without stamping" — RetentionService.cs:162-192, CollectionBackgroundService.cs:770-779
• VERIFIED — "The hold watchdog logs one Warn past WriteHoldWarnAfter; reads pay one timestamp" — DuckDbInitializer.cs:388-411, _acquiredAt a field initializer at :376, _write && guard
• VERIFIED — "ArchiveOldDataAsync/CompactParquetFiles return bool and the ~30 statement call sites compile unchanged" — every site is statement-position or asserts the bool (ArchiveLockBudgetTests.cs:228/317/343/357; discarded at ArchiveService.cs:2034)
• VERIFIED — "The reset's export and restore locks stay exclusive" — the only remaining AcquireWriteLock() calls are :1690, :1809, :1884
• VERIFIED — "ArchiveStampAfterRun: complete a full interval away, incomplete due in exactly 5 minutes" — :719-720 against HousekeepingIsDue at :622, pinned by ACompletePass_StampsTheClock_AnIncompleteOneStampsAFiveMinuteRetry
• VERIFIED — "The reset still runs the recovery and post-reset compaction through the same bounded code" — ArchiveService.cs:1793 and :2034; correct, and it is the doc comment at :115-121 that is wrong
• UNVERIFIED — every test count, revert check and "0 warnings" — Lite/Lite.Tests target net10.0-windows, so nothing builds here; I read the new assertions instead of running them
• UNVERIFIED — "a reader that keeps asking gets in within budget plus slack" — ArchiveLockBudgetTests.cs:198 is a live timing measurement
• UNVERIFIED — "the 4 GB COPY limit against live collectors is plan risk 1" — needs the dogfood run the body says was not done
• REFUTED — "Every RunXIfDueAsync keeps its name, signature, due check, stamp and text" — RunArchivalIfDueAsync gained a CancellationToken (:722) and a changed stamp (:752); RunRetentionIfDueAsync's stamp is now conditional (:774). Lanes B1/B2 state both, so this is a stale body sentence, not a code defect

Riskiest lines

1. Lite/Services/ArchiveService.cs:717 — the new timeout clause in RecoverPendingArchivesAsync is safe for the scheduled pass, which re-checks the journal at :299, and not for ArchiveAllAndResetAsync, which runs the same recovery at :1793 with no journal check before exporting every table: an unresolved journal's rows land in a second parquet and are double-counted by v_* — UNPINNED
2. Lite/Services/CollectionBackgroundService.cs:288-291 — the size-triggered reset, archival, retention and analysis now run concurrently with a collection sweep rather than strictly after it — pinned for structure by HousekeepingLoopTests.SlowMaintenanceStep_DoesNotDelayTheNextCollection and TheRealLoops_HoldTheSixSteps_InTheRulesOrder_AndAreStartedBeforeTheCollectionLoopAndAwaitedAtShutdown; the reset-versus-sweep overlap is UNPINNED (acknowledged as #5733)
3. Lite/Services/LocalDataService.SkippedCycles.cs:103-106 — four independent arg_max calls where LocalDataService.CollectionHealth.cs:355 documents that arg_max skips a NULL argument and packs the columns into one struct for it — SkippedCycleSurfaceTests.TheSummary_CountsCyclesAndSlots_AndTheNewestRowSuppliesTheLastFields seeds non-null values everywhere, so UNPINNED for this risk

Traps near this diff

• "Lite and Darling ship in parity: a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit (#3594)." — does not apply — the mechanism is Lite-only: CollectionBackgroundService.cs:45 is one shared 1-minute grid, DarlingCollectorRunner schedules per collector and writes no collection_cycle row; the description is unchanged and no census compares get_collection_health's key set across SKUs. The weaker field-shape precedent at McpHealthTools.cs:656-664 is in the top-level comment
• "A total_* assigned from a .Count is a count of the page unless the set is provably whole (#3699)." — applies — run_count = rows.Count at McpHealthTools.cs:1094 now counts collection_cycle/SKIPPED rows as runs, because ReadRecentCollectionLogAsync (LocalDataService.CollectionHealth.cs:781) is the one read left outside the nine-predicate set
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies — the swap-journal regex is written character-for-character twice in one method, ArchiveService.cs:851 and :891, unpinned
• "A roster-based census does not see the file it does not roster; an anti-pattern census sweeps the class (#3699)." — does not apply — NoFileUnderLite_CopiesTheCollectorNameAsASingleQuotedSqlLiteral sweeps every .cs/.xaml/.sql under Lite/; only the positive count is a roster
• "CASE WHEN interval > 0 THEN delta / interval ELSE 0 END spells 'unknowable' as 0 (#3707)." — does not apply — SkippedCycleSummary.None is Count = 0 with every other field null (LocalDataService.SkippedCycles.cs:79), stated at McpHealthTools.cs:725 and pinned by TheMcpBlock_CarriesTheContractFields_AndZeroIsARealAnswer
• "An alert evaluated every ~30 s against an observation that changes once per COLLECTION re-fires on every cooldown expiry (#3579 → #3628)." — does not apply — not an alert; written once per late wait from the single site at CollectionBackgroundService.cs:312, with ACycleOnTime_AndABackwardClockStep_LogNothingAndWriteNothing pinning the quiet case
• "A test that reads the clock twice measures the CI scheduler: one anchor owns both ends of an interval (#3676)." — does not apply — ArchiveLockBudgetTests is on the timing-census allow list with its reason, the 20 s bound at :197 is the hang guard that reason names, and :307/:313 bracket one interval from one anchor

Divergence (blind read vs PR body)

Three places, none a disagreement about what the code does.

The body does not account for roughly half the diff. It opens with "Builds on #5730; merge after it," then grades only #5731's four lanes. The branch carries #5730 too, so the diff against main includes CollectionCycleSkip.cs, LocalDataService.SkippedCycles.cs, the nine collector_name <> 'collection_cycle' predicates across seven LocalDataService.*.cs files, LogSkippedCycleAsync, the skipped_cycles MCP block, the McpInstructions sentence and the new SkippedCyclesSummary TextBlock — none of it mentioned. On a full pass that is my scope whatever its provenance, so I reviewed it, and two of my three riskiest lines sit in it.

Lane A1's RunXIfDueAsync sentence is corrected by lanes B1/B2 rather than updated in place. B1 says the token is not passed yet, and the next bullet says it is and names the test that fails without it. The tree agrees with the later text; nothing to fix in the code.

The body's double-check is more accurate than the code comment it describes. ArchiveService.cs:118 says the reset "keeps its unbounded wait on purpose," true of :1690/:1809/:1884 and false of :1793 and :2034. That gap is where the data-integrity finding lives: the body reasoned about whether a deferred recovery fails the reset (it does not), not about what the reset then exports — the journal's rows, a second time.

Reviewer checklist

• Build Lite and run Lite.Tests on Windows; nothing here compiles on Linux, including int → int? against ArchiveViewRefreshAfterDeleteTests.cs:97/:115 where Assert.Equal now infers int?
• Reproduce the reset-with-surviving-journal path: kill between WritePendingArchive (ArchiveService.cs:365) and the DELETE, hold a read lock past WriteLockBudget, force a size-triggered reset, then count rows in v_<table>
• Call get_collection_log against a store holding a collection_cycle/SKIPPED row and decide whether that row belongs on the page, reading run_count and oldest_returned_collection_time
• Dogfood plan risk 1: watch LogProcessMemory while an archive COPY under the 4 GB limit overlaps a live sweep, which could not happen before this PR
• Watch a live run for the hold-watchdog Warning: five seconds against a promote plus DELETE on a large store may be loud enough to want a different number
• Confirm the first housekeeping tick landing one minute after round one delays nothing that mattered at startup, the backfill's cycle-1 due check in particular

Claude posting for Erik Darling. Addressed in c9b6167 and 0a910ad: the reset runs its recovery with an unbounded wait and abandons the reset while an unfinished journal remains; the comment names the three unbounded reset locks; one shared journal-name regex; the skipped-cycle summary reads one row; get_collection_log counts collector rows only and reports skipped_cycle_rows.