[erikdarlingdata]

Claude posting for Erik Darling.

Fixes #5938.

The failure

PR #5930 (four PNG baselines only), run 38888262525, job Darling PG tests (1) 116934424635, 2026-10-16 10:55Z:

Darling.Tests.QueryStoreIntervalBrinLiveTests.TheSummarize_WhileAManualVacuumRunsOnTheTable_SkipsTheIndex_AndNeverAsksForTheLock [FAIL]
  the summarize asked for the table's lock while a manual VACUUM was running
  Stack Trace:
    Darling\Darling.Tests\QueryStoreIntervalBrinLiveTests.cs(527,0): at ...TheSummarize_WhileAManualVacuumRunsOnTheTable_SkipsTheIndex_AndNeverAsksForTheLock()

Line 527 is Assert.False(waiterSeen, ...). The assertion on line 525 (skipped 1 being vacuumed) passed, so the summarize did skip the index. The only thing that failed was the test's watcher, which saw SOMEONE ungranted on the table.

The race

The code is right: SummarizeTargetsSql reads pg_stat_progress_vacuum for the table and SummarizeNewRangesAsync skips a being-vacuumed index before it opens a transaction or asks for any lock. The bug is in the test's two helpers.

1. SlowVacuum.StartAsync decided "the manual VACUUM is running" when ANY pg_stat_progress_vacuum row existed for the table. The table is brand new with many fresh rows, which is what the autovacuum launcher picks first. A worker on it satisfies that poll while the manual VACUUM has not started.
2. The manual VACUUM then queues for SHARE UPDATE EXCLUSIVE behind the worker (PostgreSQL cancels the autovacuum after deadlock_timeout, 1 s). That ungranted request sits in pg_locks on the table, and WaiterWatch (any ungranted lock on the relation) reads it as the summarize's.
3. The same worker can also hold the lock during the test's second summarize pass, which would read "being vacuumed" (NothingToDo) or time out on the lock (RetryLater) instead of Done.

TheTurnOff_WhileAManualVacuumRunsOnTheTable_... uses the same helper and has the same exposure (it watches the index, so the VACUUM's own queueing would not trip it, but the "VACUUM is running" poll and the second pass would).

What changed (test only)

Darling/Darling.Tests/QueryStoreIntervalBrinLiveTests.cs:

• SlowVacuum.StartAsync now waits for the pg_stat_progress_vacuum row whose pid is the manual VACUUM's own backend (connection.ProcessID). The test starts its step only once that backend is progressing, so the summarize is checked against the lock the test controls. A state observed, not a sleep.
• New NoAutovacuumAsync sets autovacuum_enabled = false on the scratch legacy table, before the rows are inserted, in both manual-VACUUM tests, so no worker can be on it.

No product code changed.

Reproduction and verification

Rig: throwaway PG from pg-runtime.zip, port 55938, in my scratch folder; stopped by pg_ctl stop at the end.

• Old test shape, default settings: 6 of 6 class runs passed (CI's rate is rare).
• Old test shape, rig set to autovacuum_naptime = 1s and autovacuum_vacuum_insert_threshold = 50 to make workers frequent: 1 failure in 8 class runs, in TheSummarize_WhileAManualVacuumRunsOnTheTable_SkipsTheIndex_AndNeverAsksForTheLock (I did not capture which assertion; the CI line is the one quoted above).
• New shape, same aggressive autovacuum rig: 15 of 15 runs of QueryStoreIntervalBrinLiveTests passed (31 tests each, 0 failed).
• *Census* classes: Total 499, Failed 0. *LivePostgresCollectionHygiene*: 10, Failed 0. *LiveCleanup*: 6, Failed 0. Build: 0 warnings, 0 errors.
• There is no class named *Ratchet* in Darling.Tests (LiveCleanupConversionRatchetTests does not exist on this base), so none ran.
• Not run: the full suite (CI decides).

The old-shape local repro is a single failure; I did not prove the autovacuum worker as the waiter with a pg_locks dump. The mechanism is the only reading consistent with the passed line-525 assertion and the failure on a code-free PR, and the new shape removes both ways it can interfere.

CHANGELOG entry

SECTION: none

Test-only change.

[claude]

Verdict: LGTM

Claims

• VERIFIED — SlowVacuum.StartAsync now waits for the pg_stat_progress_vacuum row whose pid is the manual VACUUM's own backend — Darling/Darling.Tests/QueryStoreIntervalBrinLiveTests.cs:387 (AND pid = {connection.ProcessID})
• VERIFIED — new NoAutovacuumAsync sets autovacuum_enabled = false on the scratch legacy table before the rows are inserted, in both manual-VACUUM tests — QueryStoreIntervalBrinLiveTests.cs:297-298 (definition), called at :422 and :525, both before their InsertManyAsync calls
• VERIFIED — "No product code changed" — gh pr diff 5939 --name-only lists only Darling/Darling.Tests/QueryStoreIntervalBrinLiveTests.cs
• VERIFIED — TheTurnOff_... watches the index so the VACUUM's own queueing would not trip it, while TheSummarize_... watches the table — WaiterWatch(watcher, LegacyBrin, ct) at :430 vs WaiterWatch(watcher, LegacyTable, ct) at :532
• VERIFIED — "no class named *Ratchet* in Darling.Tests" — grep -rl "class.*Ratchet" Darling/Darling.Tests/ returns nothing
• UNVERIFIED — the rig reproduction numbers (6/6 old-shape passes, 1/8 failure under aggressive autovacuum, 15/15 new-shape passes, Census/Hygiene/Cleanup run counts, 0 build warnings/errors) — these are live PostgreSQL rig runs that cannot be checked from a read-only tree

Riskiest lines

1. QueryStoreIntervalBrinLiveTests.cs:387 — the whole fix hinges on connection.ProcessID matching the manual VACUUM backend's pid in pg_stat_progress_vacuum, not an autovacuum worker's — pinned by TheTurnOff_WhileAManualVacuumRunsOnTheTable_LeavesTheIndexOn_AndNeverAsksForTheLock and TheSummarize_WhileAManualVacuumRunsOnTheTable_SkipsTheIndex_AndNeverAsksForTheLock
2. QueryStoreIntervalBrinLiveTests.cs:422,525 — NoAutovacuumAsync runs after CREATE INDEX but before InsertManyAsync, relying on the table still being empty (so no worker can already be on it) — pinned by the same two tests, each on a fresh ScratchPostgres database
3. QueryStoreIntervalBrinLiveTests.cs:430 vs :532 — WaiterWatch targets the index in one test and the table in the other; swapping these would silently reopen the flake in just one test — pinned by both tests' Assert.False(waiterSeen, ...)

Traps near this diff

• "A test that reads the clock twice measures the CI scheduler" — does not apply — each polling loop (:169-179, :311-325, unchanged by this diff) tracks a single Stopwatch instance per wait, not two independent clock reads
• "Lite and Darling ship in parity... a fix to one SKU's reader... without the counterpart's is a finding" — does not apply — QueryStoreIntervalBrin is a Postgres-only BRIN autosummarize feature with no Lite/DuckDB counterpart (grep for QueryStoreIntervalBrin outside Darling returns nothing)
• "Ruling (#5833): tests are load-bearing... never answer a trap with a test that reads source text" — does not apply — the changed tests already assert runtime behavior (StepOutcome, waiterSeen), and this diff adds no new text-reading test

Divergence (blind read vs PR body)

None — the body's account matches the diff on every claim graded above.

Reviewer checklist

• Run Darling.Tests.QueryStoreIntervalBrinLiveTests several times against a live PostgreSQL instance with autovacuum_naptime and autovacuum_vacuum_insert_threshold lowered (as the PR body describes) to confirm the flake is actually gone, since this cannot be exercised from a read-only tree