Skip to content

Client Timeouts: the attention-rate measurement for the field week (#5736) - #5976

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/5736-attention-rate-measure
Oct 16, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/5736-attention-rate-measure

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Part of #5736. This is the measurement tool only. It does not change the bars and does not turn the alert on: that follows the numbers.

Why

The Client Timeouts alert (#5640) ships OFF with guessed bars of 10 (Warning) and 60 (Critical) attentions per minute. The plan in #5736 is a week of field capture (full about 2026-10-20), then each server's p99.9 of the per-minute rate plus the fleet spread, then the defaults. pm-worker needs one command per store that returns those numbers in the shape the bar-setting change needs.

What changes

Two read-only scripts, one per store, plus a behaviour test for each. Nothing in product code changes.

  • Darling/tools/attention-rate-measure.sql (PostgreSQL, run with psql -v from=... -v to=... -f). Reads collect.attention_events (the count rows, the exact per-run totals; the sample rows are never the total) and collect.collection_log (successful attention runs).
  • Lite/tools/attention-rate-measure.sql (DuckDB, quantile_cont). The same query over v_attention_events and v_collection_log, the archive views the alert reads, so a week that crosses an archive-and-reset still reads in full. Lite had no tools folder; Lite/tools/ is new.

Both scripts bucket per server per calendar minute, and a bucket exists only for a minute that had a successful attention run in the collection log. The bucket holds that minute's summed count rows, zero when the run counted nothing. A minute with no run has no bucket: the collector did not look, so it is not a quiet minute. A run that did not read the counter (for example SESSION_MISSING) is not a run. A count row in a minute with no successful run is not read.

Result columns (identical names and order in both stores): scope (server, then one fleet row), server_id, server_name, minutes_in_span, minutes_with_a_run, longest_gap_minutes, attentions, p50, p99, p999, max_per_minute, p999_3min_avg, top_minutes (the three busiest nonzero minutes as UTC minute=count; ...), and on the fleet row fleet_p999_min, fleet_p999_median, fleet_p999_max (the spread of the per-server p999). The fleet row carries the summed minutes and attentions, the largest gap, the largest single minute, and the server count in server_name.

  • minutes_in_span: first to last run, inclusive.
  • minutes_with_a_run: the buckets; every percentile is computed over these.
  • longest_gap_minutes: the longest stretch of consecutive minutes inside the span with no run (0 when none).

minutes_observed is dropped, not kept: it would have been the same number as minutes_with_a_run, and two names for one number invites the old reading. Both scripts agree on this.

One column beyond the ask, p999_3min_avg: the p99.9 of the 3-minute rolling average of the per-minute rate. The alert compares a three-minute average to its bars (AttentionEvaluator.WindowMinutes), so the per-minute p99.9 alone would set the bars higher than the quantity the alert actually tests. A window now needs three consecutive run minutes, so a hole breaks it; the column is NULL when no such window exists.

Known edge, same as the alert's: the first run after a restart stages the counter and writes no count row, so that minute reads as quiet. The buckets assume the default one-minute collector cadence (a minute holds one run); the header comment says so. Each header now names its twin: the two files are hand-written copies of one definition and nothing fails if a rule change lands in only one.

How pm-worker runs it

Darling (any role that can SELECT from collect; times are naive UTC, from inclusive, to exclusive):

psql -h <host> -p <port> -U <user> -d <store db> -v from='2026-10-13 00:00:00' -v to='2026-10-20 00:00:00' --csv -f Darling/tools/attention-rate-measure.sql > darling-attention-rate.csv

Lite, on the machine that ran Lite, with a duckdb CLI of the 1.5.x line (the version Lite ships):

  1. Close Lite first, so the store is checkpointed. A monitor.duckdb.wal file left beside the original means it is not, and a copy of the .duckdb alone is missing what the .wal holds (the newest minutes). Start Lite and close it again until no .wal remains.

  2. Copy only the monitor.duckdb file to a scratch folder. Do not copy the .wal and do not copy the archive folder.

  3. Run it on that same machine. The v_collection_log and v_attention_events views hold the original store's absolute archive path, baked in as a parquet glob when the store was opened (DuckDbInitializer.cs builds it from _archivePath at line 3537, and _archivePath comes from the original database path at line 508; App.xaml.cs:629 sets the default archive folder under the app data root). A copy therefore reads the archive from the original store's folder, wherever the copy sits, and -readonly cannot repoint it. A copy moved to another machine loses the archived weeks.

     duckdb -readonly <copy>/monitor.duckdb -csv -c "SET VARIABLE range_from = '2026-10-13 00:00:00'; SET VARIABLE range_to = '2026-10-20 00:00:00';" -f Lite/tools/attention-rate-measure.sql > lite-attention-rate.csv
    

Bar-setting rule, as in the issue: take each server's p999, then read the fleet row's fleet_p999_min, fleet_p999_median and fleet_p999_max; set Warning and Critical from that spread; top_minutes and the sample rows show which minutes (and, through get_client_timeouts, which apps) to look at when a server is far above the fleet. Read longest_gap_minutes and minutes_with_a_run against minutes_in_span first: a server with a long gap was not measured for that long, and its p99.9 stands on fewer buckets.

Review round 1

  • Blocking 1, the dense minute spine turned "the collector did not run" into "ran and counted zero": fixed. The generate_series spine is gone from both scripts. The buckets are the runs CTE (minutes with a SUCCESS attention run), left-joined to the counts, zero when the run counted nothing. The three coverage columns above are new in both scripts, same names and order. minutes_observed is dropped in favour of minutes_with_a_run (see above). The 3-minute rolling window also had to change: with holes, "the two preceding rows" are no longer two preceding minutes, so a window now counts only when its first and last minute are two minutes apart.
    • The old zero-fill test planted three rows and asserted minutes_observed = 100; it encoded the defect and is replaced by: 100 one-minute SUCCESS runs with one attention gives p50 = 0 and minutes_with_a_run = 100. The other tests now plant a run per minute too, instead of two end markers.
    • New hole test in both test classes: runs in minutes 1 to 30 and 71 to 100, one attention per run. Both dialects give minutes_in_span 100, minutes_with_a_run 60, longest_gap_minutes 40, 60 attentions, and p50 = p99 = p999 = max = 1 over the 60 buckets (so no zero buckets are invented), p999_3min_avg 1.00. The fleet row carries 100 / 60 / 40. All values are the same in PostgreSQL and DuckDB. The same planted data also ran through real psql.exe and through Python's duckdb and gave identical rows.
    • A limit of the hole test, said plainly: with one attention per run the percentiles are all 1, so they do not by themselves tell 60 buckets from 100; the coverage columns and the quiet-server test (zero buckets from real runs) carry that.
  • Blocking 2, the Lite archive instruction named a mechanism the views do not have: fixed. Verified by reading the two lines: DuckDbInitializer.cs:508 builds _archivePath from the original database path and line 3537 puts the absolute path into the parquet glob of the views; App.xaml.cs:629 sets ArchiveDirectory under the app data root. The script header, the section above and the Client Timeouts alert: set the default bars from a week of field capture, then turn it on #5736 comment now say: close Lite first, copy only the .duckdb file, run on the same machine, and the archive is read from the original store's folder. One correction to the instruction as briefed: I tested the .wal case with duckdb 1.4.4 in Python, and a read-only open of a store with a .wal beside it does open and replays the .wal. So the stated reason is that a copy of the .duckdb alone (the .wal not copied) is missing the newest minutes, not that the file cannot be opened read-only.
  • Nit, name the twin: done. Both headers name the other file and say a rule change must land in both.
  • Nit, named sort columns: done. ORDER BY scope DESC, server_id in both.
  • Lite command line. There is still no duckdb CLI on this machine. The shipped .sql was run through Python duckdb 1.4.4 (read-only connection to a planted store file, the same SET VARIABLE range_from and range_to statements, then the whole file as one statement), and through DuckDB.NET 1.5.5 in the test class. The CLI form itself (duckdb -readonly ... -c ... -f) is still unexercised.

Test plan

  • AttentionRateMeasureScriptLiveTests (Darling, throwaway PostgreSQL via DARLING_TEST_PG): 4 passed, 0 skipped. It runs the shipped file with psql's :'from' and :'to' filled in by text. On a planted day in 2001: (1) 100 one-minute runs with one attention: p50 0, p99 0.10, minutes_with_a_run 100; (2) the hole test above; (3) a 1000-run server with spikes of 40, 80 and 100 (the 100 as two count rows in one minute, plus a sample row with no count): p999 80.02, max 100, the three top minutes in order, p999_3min_avg 33.33; (4) the fleet row: min 0 (a silent server), median 9.01, max 80.02, 3 servers, 1110 minutes in span and with a run, gap 0, 230 attentions, and a SESSION_MISSING run that is not a bucket.
  • AttentionRateMeasureScriptTests (Lite, real DuckDB file through DuckDB.NET 1.5.5): the same four behaviours and numbers, 4 passed.
  • The Darling script run by hand with the real psql.exe and -v from=... -v to=... against the throwaway cluster, on the hole data: clean, 100 / 60 / 40.
  • The Lite script run through Python duckdb 1.4.4, read-only, same hole data: identical numbers.
  • Darling and Lite test projects build Debug with 0 warnings.
  • Every *Census* and *Ratchet* class in both test projects plus the new classes: Darling 501 passed, Lite 36 passed, 0 failed.
  • Not run: the full suites (per the brief). The DuckDB CLI form is unexercised, as above. The first pm-worker run on 10-20 is its real check.

No source-text, census or roster test was added (#5833).

CHANGELOG entry

SECTION: Added
ENTRY:

… week (#5736)

A psql script for the Darling store and a DuckDB query for Lite report the per-minute
attention rate per server (zero-filled minutes inside the observed span) with p50, p99,
p99.9, max, the three busiest minutes and the fleet spread of the per-server p99.9, plus a
planted-store behaviour test for each.
g.minute,
coalesce(c.attentions, 0) AS attentions
FROM span AS s
CROSS JOIN LATERAL generate_series(s.first_minute, s.last_minute, interval '1 minute') AS g(minute)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The span fill counts minutes the collector never ran as observed-and-quiet, and nothing in the output says so.

runs (line 83) already knows exactly which minutes had a successful attention run. span collapses it to min(minute)/max(minute) and this generate_series then re-expands the whole interval densely, so a minute with no run and no count row is indistinguishable in the result from a minute that ran and counted zero. minutes_observed is the count of those fabricated buckets (line 113), and there is no column reporting how many of them had a run.

The header is explicit that minutes outside the span are not observed (line 21), and silent about holes inside it. The restart edge it does name (line 24) is a different thing — one missing count row, not a stretch of unobserved minutes.

This is the #3695/#3592 trap from .github/REVIEW-TRAPS.md: "A rate divided by a NOMINAL span when the collector observed less of it is wrong in the direction of 'quieter' … analysis facts divide by observed time, and a window with a hole says so." Here it bites the quantity the whole change exists to produce.

Concrete failure, using this PR's own fixture: the spiky server is 1,000 minutes with spikes of 40/80/100 and p999 = 80.02. Add a 24-hour collection hole inside the span — service stopped, store unreachable, collector disabled — and the fill contributes 1,440 zero buckets. N goes to 2,440, the continuous index to 0.999 × 2439 = 2436.56, which now lands between a zero and the 40, giving p999 ≈ 22.44. Same server, same attentions, a bar set from it is ~3.5× too low, and the alert over-pages on exactly the servers whose stores had an outage during the field week.

Lite's twin is worse: Lite is a desktop app, and the script's own run instructions assume the operator closes it. A week's "capture" from a laptop that is shut overnight is mostly minutes that never happened — roughly 6,300 of 10,080 for a 15-hour nightly gap — all of them zero-filled and all of them counted in minutes_observed.

Both fixes are cheap and local:

  • build the spine from runs (plus the count minutes) instead of generate_series — at the documented one-minute cadence that is the span, minus the holes; or
  • keep the dense fill and project minutes_with_a_run beside minutes_observed (and ideally longest_gap_minutes), so a diluted p99.9 is visible rather than silent.

Whichever way it lands, the header's "every minute inside that span is a bucket" sentence needs the hole case stated.

Comment thread Lite/tools/attention-rate-measure.sql Outdated
SELECT
s.server_id,
s.server_name,
unnest(generate_series(s.first_minute, s.last_minute, INTERVAL 1 MINUTE)) AS minute

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same defect as the Darling twin (see the comment on Darling/tools/attention-rate-measure.sql:93): runs (line 53) holds the minutes that actually had a successful run, span throws that away, and this generate_series re-expands the interval densely, so an unobserved minute and a ran-and-counted-zero minute are the same bucket in the result.

It lands harder on this SKU. Lite is a desktop app, and this script's own run instructions (lines 11-16) assume the operator closes it or copies the file — i.e. the store is routinely not collecting. A seven-day window off a laptop that is shut overnight is around 6,300 minutes that never happened out of 10,080, every one of them zero-filled and every one of them counted in minutes_observed (line 124). The p99.9 the bars are to be set from is then mostly a measurement of how long the app was closed.

minutes_with_a_run beside minutes_observed, or a spine built from runs, closes it.

Comment on lines +49 to +51
/* The log rows sit only at the first and the last minute of the span, so every other minute exists only because
the query fills the span with zeros. Without the fill there is one bucket and it holds 10. */
Assert.Equal(100L, server.Long("minutes_observed"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is the defect written down as an expectation, which is .github/REVIEW-TRAPS.md's "A test that encodes the lie … Ask what defect a test would fail ON."

The planted evidence for this server is three rows: a log row at minute 0, a log row at minute 99, and one count row at minute 50 (lines 109-111). Minutes 1-49 and 51-98 carry no proof the collector ran at all — and the test asserts the script calls all 100 of them minutes_observed, with a comment that says plainly that 98 buckets exist only because the fill invented them.

That is exactly the shape a real collection hole has, so as written this test passes on the behaviour described in the comment on the two .sql files, and there is no test in either class that would fail if a 15-hour outage inside a span were counted as 900 quiet minutes.

If the fill stays, this is the place to also pin the distinction — plant a server with a genuine gap (log rows at minutes 0-9 and 90-99, nothing between) and assert whatever column tells the operator that 80 of its 100 minutes had no run. The Darling twin at AttentionRateMeasureScriptLiveTests.cs:45 needs the same.

Comment thread Lite/tools/attention-rate-measure.sql Outdated
Comment on lines +11 to +19
--
-- duckdb -readonly <copy of monitor.duckdb> \
-- -c "SET VARIABLE range_from = '2026-10-13 00:00:00'; SET VARIABLE range_to = '2026-10-20 00:00:00';" \
-- -f attention-rate-measure.sql
--
-- range_from and range_to are naive UTC timestamps (the clock of collection_time), range_from inclusive, range_to exclusive.
-- Both are required. Add -csv for a file the next change can read. The query reads the v_collection_log and v_attention_events
-- archive views, the same relations the alert reads, so a week that straddles an archive-and-reset still reads in full (the views
-- need the store's archive folder in its usual place beside the copy; with no parquet files they are the plain tables).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the views need the store's archive folder in its usual place beside the copy" is not what the views do.

v_attention_events and v_collection_log are built by DuckDbInitializer.CreateArchiveViewsBodyAsync, and the parquet source is baked into the view text as an absolute path, not a relative one:

  • DuckDbInitializer.cs:508 — _archivePath = Path.Combine(Path.GetDirectoryName(databasePath) ?? ".", "archive")
  • App.xaml.cs:629 — DatabasePath = Path.Combine(appDataRoot, "monitor.duckdb"), so that directory name is absolute
  • DuckDbInitializer.cs:3537 — globs.Add(Path.Combine(_archivePath, pattern).Replace("\\", "/"))
  • DuckDbInitializer.cs:3279 — that glob string is embedded in CREATE OR REPLACE VIEW v_<table> AS SELECT * FROM <table> UNION ALL BY NAME SELECT * FROM read_parquet('<absolute glob>', union_by_name=true)

So the copy's views read <original appDataRoot>/archive/*_attention_events.parquet, wherever the copy sits. Three consequences for the documented procedure:

  1. Putting an archive folder beside the copy does nothing — it is never consulted.
  2. A copy carried to another machine (the natural move for a field store, and the one these instructions invite) resolves a path that does not exist there, and read_parquet on a glob that matches nothing fails the whole query. Loud rather than silent, but the procedure as written does not work.
  3. -readonly forbids CREATE OR REPLACE VIEW, so the operator cannot repoint the views from the CLI either.

On the same machine with the original store still in place it does work — but then it is reading the live store's archive directory directly, including while ArchiveService compaction is swapping files in it, which is the one thing copying the database file was meant to avoid.

Worth saying what is actually required: run on the same machine, leave the real archive folder untouched at its original path, and expect reads to touch it live. The PR body's Lite command omits the archive folder entirely, which will read as "the copy is self-contained".

round(CAST(max(p.p999) AS numeric), 2)
FROM per_server AS p
HAVING count(*) > 0
ORDER BY 1 DESC, 2;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking. These are bare integers, so they really are output-column ordinals and the sort is correct ('server' > 'fleet', so 1 DESC puts the fleet row last; server_id NULL sorts last in both engines anyway). The ORDER BY 3 + 4 DESC trap in .github/REVIEW-TRAPS.md does not bite here — but its prescription, "Sort by named columns", still applies, and a UNION ALL is the one place people assume it is impossible. It is not: ORDER BY scope DESC, server_id is legal over a set operation in both PostgreSQL and DuckDB, because the sort resolves against the result's output names. Same line in the Lite twin (line 171).

claude[bot]
claude Bot previously requested changes Oct 16, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Careful SQL: the count/sample split, the SUCCESS-only span, the 3-minute roll that really models AttentionEvaluator, a CASE … END that ends at NULL rather than a fabricated 0, and eleven asserted numbers I recomputed as correct and identical in both dialects.

Blocking: the dense minute spine turns "the collector did not run" into "ran and counted zero", minutes_observed counts the invented buckets, and no column says how many minutes had a run — so a p99.9 diluted by a collection hole cannot be told from a real one, and the shipped bars are to be set from it. Worked example inline on Darling/tools/attention-rate-measure.sql:93. Second: the Lite archive-folder instruction describes a lookup the views do not perform.

Claims

  • VERIFIED — "four files, nothing in product code changes" — gh pr diff --name-only.
  • VERIFIED — Darling reads collect.attention_events / collect.collection_log, SUCCESS + attention only — Darling/…:288, :302, :290-291; both in collect (PgSchemaGenerator.cs:142, PgMigrations.cs:965).
  • VERIFIED — Lite reads "the archive views the alert reads" — Lite/…:658, :672 vs LocalDataService.Attention.cs:33, :35.
  • VERIFIED — "Lite/tools/ is new" — holds this file alone.
  • VERIFIED — "SESSION_MISSING does not widen the span" — Lite/…:661, pinned by the silent server (AttentionRateMeasureScriptTests.cs:133, :87).
  • VERIFIED — "result columns identical in both stores" — Darling/…:127-141 vs Lite/…:139-153, same fourteen, same order.
  • VERIFIED — "the alert compares a three-minute average to its bars" — AttentionEvaluator.cs:134, :199, :302, :337.
  • VERIFIED — "p999_3min_avg NULL under three minutes" — count(*) OVER w = 3 at Darling/…:109 / Lite/…:115. Read, not pinned.
  • VERIFIED — "default one-minute cadence" — CollectorScheduleDefaults.cs:76.
  • VERIFIED — every number the test plan quotes (100/0/0.10; 1000/80.02/100/33.33; 3/1110/230/0/9.01/80.02) — recomputed with the continuous index q × (N−1).
  • VERIFIED — "no source-text, census or roster test (#5833)" — both classes run the shipped .sql and assert values.
  • VERIFIED — no CHANGELOG.md edit, entry in the body — .claude/skills/lane-orders/SKILL.md:119, :270.
  • UNVERIFIED — "Darling live tests 3 passed, 0 skipped" — needs DARLING_TEST_PG; Assert.SkipWhen no-ops without it.
  • UNVERIFIED — "Lite 3 passed" — needs a DuckDB run; I checked the values by hand instead.
  • UNVERIFIED — "run by hand with real psql.exe" — the only exercise of psql's :'from'; the test substitutes it in C#.
  • UNVERIFIED — "both test projects build Debug, 0 warnings".
  • UNVERIFIED — "Darling 500 / Lite 35 / 49 passed" — needs the runner.
  • REFUTED — "the views need the archive folder in its usual place beside the copy" (Lite/…:19) — the globs are baked absolute (DuckDbInitializer.cs:508, App.xaml.cs:629, DuckDbInitializer.cs:3537, :3279): a copy reads the original store's archive directory, a folder beside the copy is never consulted, and -readonly cannot repoint the views.

Riskiest lines

  1. Darling/tools/attention-rate-measure.sql:93 (twin Lite/…:95) — the dense spine sets every percentile and absorbs collection holes — pinned by AServerWithOneAttentionInAHundredMinutes_HasAMedianOfZero_BecauseQuietMinutesAreZeroFilled, but that test asserts the defect; the hole case is UNPINNED.
  2. Lite/tools/attention-rate-measure.sql:19 — decides whether a week crossing an archive-and-reset reads in full, and names the wrong mechanism — UNPINNED (the test store has no parquet, so the union arm never runs).
  3. Darling/…:119 vs Lite/…:130 — percentile_cont(0.999) WITHIN GROUP vs quantile_cont(…, 0.999) must agree on interpolation and NULL handling — pinned by the 33.33 assertions (…ScriptTests.cs:78, …LiveTests.cs:77), the Darling half only under DARLING_TEST_PG.

Traps near this diff

  • "A rate divided by a NOMINAL span when the collector observed less of it is wrong in the direction of 'quieter' … a window with a hole says so (#3695, #3592)." — applies — Darling/…:93 / Lite/…:95 fill densely, minutes_observed (:113 / :124) counts never-ran minutes as quiet, and runs (:83 / :53) knew better.
  • "A test that encodes the lie … Ask what defect a test would fail ON (#3699)." — applies — …ScriptTests.cs:51 and …LiveTests.cs:45 assert minutes_observed = 100 from three planted rows.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies, non-blocking — two hand-written scripts plus two hand-written expectation tables, where the alert's own read was unified into AttentionWindowSql for this reason. The dialect gap makes aliasing impractical and each half has its own behaviour test, but nothing fails if a rule change lands in one script only; name the twin in both headers.
  • "ORDER BY 3 + 4 DESC is not 'ordinal 3 plus ordinal 4' … sort by named columns." — does not apply — Darling/…:160, Lite/…:171 are bare integers, so real ordinals; nit posted inline.
  • "CASE … ELSE 0 END spells 'unknowable' as 0 (#3707)." — does not apply — Darling/…:109-112 / Lite/…:115-118 end at END, so a short window is NULL and is dropped.
  • "Integer division in a rate column." — does not apply — the only division is inside avg(CAST(… AS double precision)) / AS DOUBLE (Darling/…:111, Lite/…:117).
  • "A retention-tiered store answers a 7-day question from a 4-day raw tier (#3590)." — does not apply — collection_log is kept 60 days (DarlingRetention.cs:48, :54) against attention_events' 30 (CollectorScheduleDefaults.cs:76); Lite routes through the archive views.
  • "Lite and Darling ship in parity (#3594)." — does not apply — both halves ship; the base-tables/v_-views split mirrors DarlingAlertReadAdapter.cs:916-920 vs LocalDataService.Attention.cs:33-37.
  • "Truncation inferred from a full page / a count off the page / a percent whose denominator is the PAGE." — do not apply — no LIMIT, page or share; top_minutes is cut inside the aggregate (Darling/…:122).
  • "Timestamps are naive UTC; strip the DateTimeKind before binding." — does not apply — timestamp in both tables (PgMigrations.cs:967, :4066), bound via SpecifyKind(…, Unspecified) (…LiveTests.cs:200).

Divergence (blind read vs PR body)

Three places, all one direction: the body gives the fill's upside and not its cost.

  1. "Without that fill a p99.9 over non-empty minutes overstates the rate" is true and is why the fill exists. The converse — filling minutes the collector never ran understates it, badly — is in neither the body, the headers nor the tests. My blind read found both cases collapsed into one bucket shape with no column separating them.
  2. The known-edges list names the restart edge and the cadence assumption, both of which the headers state, and stops before the collection hole — which is larger, commoner, and worst on Lite, whose own instructions assume the app is closed.
  3. Three accounts of one mechanism, none matching the code: the body says copy the file to a scratch folder, full stop; the script adds that the archive folder must sit "beside the copy"; the views hold the original store's absolute path.

Everything else reachable from a read-only tree I confirmed, the eleven numbers included.

Reviewer checklist

  • Settle the fill before the 2026-10-20 run, which is not repeatable: spine from runs, or project minutes_with_a_run / longest_gap_minutes so a holed week can be discarded.
  • On a Lite store that has archived, read SELECT sql FROM duckdb_views() WHERE view_name = 'v_attention_events', then run the documented procedure and see which archive directory is touched.
  • Run the Lite form through the DuckDB CLI, skipped in the test plan: -readonly against a copy that still has a .wal is untested, and DuckDB cannot replay a WAL read-only.
  • Confirm psql -v from=… -v to=… quoting on a real cluster.
  • Check the dense spine's cost on the largest field store before pointing it at a month.

…lection hole shows in coverage columns (#5736)

The dense minute spine turned a collection hole into quiet minutes. The spine is now the minutes with a SUCCESS attention run; new minutes_in_span, minutes_with_a_run and longest_gap_minutes columns replace minutes_observed in both scripts; the 3-minute window needs three consecutive run minutes. The Lite header now says the archive is read from the original store's folder.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

  • VERIFIED — "the dense minute spine is gone from both scripts" (Blocking 1 fixed) — Darling/tools/attention-rate-measure.sql:91-101 and Lite/tools/attention-rate-measure.sql:91-101 build per_minute from runs (SUCCESS-run minutes only) left-joined to counts; the old seen/span/generate_series/spine CTEs are deleted.
  • VERIFIED — "three coverage columns new in both scripts, same names and order" — minutes_in_span, minutes_with_a_run, longest_gap_minutes appear identically at Darling/…:111-113/:130-132/:149-151 and Lite/…:119-121/:138-140/:157-159.
  • VERIFIED — "the 3-minute rolling window now requires the window's first and last minute be two minutes apart" — Darling/…:108 (pm.minute - min(pm.minute) OVER w = interval '2 minutes') and Lite/…:108 (date_diff('minute', min(pm.minute) OVER w, pm.minute) = 2), exercised by the new hole test's p999_3min_avg = 1m assertion in both test files.
  • VERIFIED — "new hole test in both test classes: runs in minutes 1-30 and 71-100, giving minutes_in_span 100 / minutes_with_a_run 60 / longest_gap_minutes 40" — hand-traced the gap arithmetic (since_prior_run/prior_run_minute via lag, minus 1) against the planted minutes: gap between minute 30 and minute 71 is 41, minus 1 = 40, matching AttentionRateMeasureScriptLiveTests.cs:46 and AttentionRateMeasureScriptTests.cs:71.
  • VERIFIED — "the old zero-fill test that asserted minutes_observed = 100 from three planted rows is replaced" — the renamed test now plants 100 real one-minute runs via the new RunsAsync helper and asserts minutes_in_span/minutes_with_a_run/longest_gap_minutes separately, no longer a source-reading or defect-encoding assertion.
  • VERIFIED — "Lite archive instruction corrected: a copy still reads the original store's absolute archive path, -readonly cannot repoint it" — Lite/tools/attention-rate-measure.sql:16-18, matching the prior review's REFUTED finding about "beside the copy".
  • VERIFIED — "both headers name the twin" — Darling/…:3-4, Lite/…:2-3.
  • VERIFIED — "named sort columns" — ORDER BY scope DESC, server_id in both scripts (Darling/…:172, Lite/…:172).
  • UNVERIFIED — live-test and CLI run counts in the PR body's "Review round 1" and "Test plan" sections (requires DARLING_TEST_PG / a duckdb runtime, not available from a read-only tree).

Riskiest lines

  1. Darling/tools/attention-rate-measure.sql:113 / Lite/tools/attention-rate-measure.sql:121 — longest_gap_minutes via lag/prior_run_minute minus 1 — pinned by ACollectionHole_IsNotQuietMinutes_… (both test files, asserts 40).
  2. Darling/…:108 / Lite/…:108 — the 3-minute window's new contiguity guard, the fix for holes bleeding into p999_3min_avg — pinned by the same hole test's p999_3min_avg = 1m assertion.
  3. Darling/…:91-98 / Lite/…:91-98 — per_minute now built from runs alone, so a counts row with no matching SUCCESS run minute is silently dropped — pinned by the renamed hundred-run-minutes test (zero buckets stay zero) and the hole test (coverage columns reflect only run minutes), not separately pinned for a stray count-without-a-run (an edge the PR body says the collector cannot produce).

Traps near this diff

  • "A rate divided by a NOMINAL span when the collector observed less of it is wrong in the direction of 'quieter' … analysis facts divide by observed time, and a window with a hole says so (#3695, #3592)." — applies, and this follow-up is exactly the fix: percentiles are now computed only over minutes_with_a_run buckets (Darling/…:122-126, Lite/…:123-127), and minutes_in_span is a separate, clearly-named coverage column rather than the divisor.
  • "A test that encodes the lie … Ask what defect a test would fail ON (#3699)." — no longer applies to the renamed test — it now plants 100 real runs via RunsAsync and fails if the script zero-fills non-run minutes, rather than asserting the old spine's artifact.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies, non-blocking, already acknowledged in both headers ("change one and change the other in the same commit") and in the PR body; each half still carries its own independent behaviour tests with hand-traced expected values, not a cross-pin.
  • "Lite and Darling ship in parity (#3594)." — does not apply as a finding — both SQL scripts and both test files changed together with matching new tests, new constant, and new helpers.
  • "ORDER BY 3 + 4 DESC is not 'ordinal 3 plus ordinal 4' … sort by named columns." — does not apply — this commit replaces the previously-correct-but-unnamed ORDER BY 1 DESC, 2 with named columns (Darling/…:172, Lite/…:172), a readability fix, not a bug fix.

Divergence (blind read vs PR body)

None on the commits in this range — the PR body's "Review round 1" section describes exactly the two fixes (dense spine removed; archive-folder instruction corrected) that the diff makes, and the new hole-test numbers I hand-traced match the body's claimed 100/60/40/1.00 exactly.

Reviewer checklist

  • Run AttentionRateMeasureScriptLiveTests against a real PostgreSQL instance (DARLING_TEST_PG) and AttentionRateMeasureScriptTests against real DuckDB to confirm the hole-test numbers on the actual engines, not just by hand-trace.
  • Run the Lite script through the real duckdb CLI (still unexercised per the PR body) to confirm -readonly behavior against a copy with and without a .wal.

@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 16, 2026 22:16

Stale: this review is on an earlier head; the bot approved the current head after the fixes.

@erikdarlingdata
erikdarlingdata merged commit 2098669 into dev Oct 16, 2026
33 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5736-attention-rate-measure branch October 16, 2026 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant