Skip to content

get_store_host: a slow size read fails alone and the polled read uses the stored size (#5718) - #5723

Merged
erikdarlingdata merged 4 commits into
devfrom
fix/5718-store-host-slow-size
Oct 13, 2026
Merged

erikdarlingdata merged 4 commits into
devfrom
fix/5718-store-host-slow-size

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Closes #5718.

Why

get_store_host failed every time on a test store, after 10.0 to 10.9 s (22 times in a day, an [ERROR] each time the page or an MCP client asked), with "Exception while reading from stream". DarlingStoreHostProfile.GatherStoreFactsAsync ran two store-scaling reads live on every call, each with a 10 s command timeout: the store size (pg_database_size) and the uncompressed-chunk total. On that store the hourly sweep's own versions of such reads measured: the store_metrics insert built on pg_database_size, mean 5.8 s / max 30.7 s over 60 calls; the per-hypertable size insert, mean 11.8 s / max 49.4 s. The code assumed a one-shot, user-invoked read, but the web card re-fetches it on every first paint, hashchange and span change (sweeps.js replays its cached payload on the 60 s poll ticks and does not re-fetch), and every MCP call is a fresh call. Each of those paid the slow live read and failed at 10 s while it was slow, and any one slow fact failed the whole profile.

Which of the two live reads times out first was not measured (the issue says so too). The fix covers both.

What changes

Darling/PerformanceMonitor.Darling.Service/DarlingStoreHostProfile.cs, Mcp/DarlingMcpStoreHostTools.cs, wwwroot/js/pages/sweeps.js, PerformanceMonitor.Darling.Storage/StoreSelfMetrics.cs.

  1. Each slow fact fails alone. A timeout in the size read, or in the uncompressed-chunk read, marks only that fact unavailable (timed out after 10 s); the version, buffer hit, temp bytes and settings still return. Timeouts are matched on type and SQLSTATE, never on message text: NpgsqlException over a TimeoutException (what Npgsql 10.0.3 throws for a client deadline), or PostgresException 57014 (a server statement_timeout). Cancellation from the caller's token is never caught (the filter also checks the token, and an OperationCanceledException never matches). A refused grant on the size read still degrades the same way, now with a reason (this role cannot read it (SQLSTATE ...)). The existing AlertReadRetrySeamTests measurement says the connection stays open and reusable after a client deadline, which the settings read that follows depends on; a new test checks it.
  2. The polled reads use the stored size. get_store_host over MCP, and the web page (which calls the same method, see below), take the store size from the hourly sweep's collect.store_metrics row, through a new StoreSelfMetrics.LatestStoreSizeWithTimeSql (the same predicate and ordering as LatestStoreSizeSql, with metric_time added; a test pins that equivalence). They no longer run pg_database_size. The store object gains size_as_of (the row's metric_time, naive UTC), size_age_seconds (as of the response, so a profile served from the 5 minute server cache still reports a true age; the web card does not use it, it derives the age from size_as_of at render time, see review round 1), and size_unavailable_reason (set when there is no stored row yet or the read failed; there is no fallback to the live read, which would bring the failure back). --check-settings keeps the live read (GatherAsync defaults useStoredStoreSize to false) and prints size unavailable: <reason> / uncompressed chunks: unavailable: <reason> when a fact is unavailable; its --json gains storeSizeUnavailableReason and uncompressedUnavailableReason.
  3. Uncompressed-chunk total: kept live, contained. The sweep stores no equivalent figure: store_metrics holds a per-object total, compressed before/after bytes and a chunk count, none of which is "bytes in chunks not yet compressed". So this fact stays a live read on every path, with the per-fact failure from step 1. When it times out, or the role is refused it for any reason other than TimescaleDB being absent, uncompressed_chunk_bytes, uncompressed_chunk_count and uncompressed_chunk_percent_of_ram are null and uncompressed_unavailable_reason says why. (HostStoreFacts.UncompressedChunkBytes/Count became nullable for this; a store without TimescaleDB, meaning SQLSTATE 42P01, 3F000 or 42883, still reports 0/0; any other refusal reports null with this role cannot read it (SQLSTATE ...).)
  4. Error text. A failure that still escapes the gather (a timeout in the version, buffer-hit or pg_settings read) now reaches the tool's catch as a store read timed out after 10 s (the store was slow to answer); retry in a moment (DarlingMcpStoreHostTools.FailureFor, passed through McpHelpers.FormatError, so the envelope shape and its census are unchanged) instead of "Exception while reading from stream". Anything else keeps its own message.
  5. MCP and web show the same thing. The web map's ["get_store_host"] entry calls DarlingMcpStoreHostTools.GetStoreHost (pinned by a test), so both outputs carry the same fields. The web card (sweeps.js) now shows Store size as <size> (hourly sweep, <age> old) and shows unavailable (<reason>) for either fact instead of a dash.
  6. Lite. Lite has no store host profile: grep -rn "StoreHost\|store_host" Lite --include=*.cs finds nothing. Darling only.

The served tool description is unchanged (the tools-list byte budget only goes down). The reading guide (the text after <<GUIDE>>, returned by get_tool_guide, outside that budget) now explains the stored size and its size_as_of / size_age_seconds, the null-with-reason cases, the live uncompressed-chunk figures that fail alone on a timeout, and that --check-settings reads the size live; pinned by McpToolGuideHeadsPlatformTests.StoreHost_GuideTail_ExplainsTheStoredSizeAndTheAloneFailingFacts_OffTheServedHead. No existing pin held this guide text. Touched classes, every Census and Ratchet class (738 tests, 0 failed) and -trait Stage=Guard (1584, 0 failed) pass.

Test plan

  • New StoreHostSlowSizeLiveTests / StoreHostSlowSizeTests (11 tests) on a throwaway PostgreSQL 17 + TimescaleDB cluster (own folder, own port, stopped afterwards): a size read forced to time out with pg_sleep behind the new StoreFactsReads seam (1 s deadline) leaves the rest of the profile intact with timed out after 1 s; the same for the uncompressed-chunk read, and the connection is still usable after it; a caller cancellation propagates as OperationCanceledException; a seeded store row recorded 7 minutes earlier (a value pg_database_size can never return) comes back from GetStoreHost as size_bytes, size_as_of and size_age_seconds of about 420; no stored row gives the "no hourly sweep" reason; the escaped-timeout text; the classifier (accepts a deadline and 57014, refuses cancellation, a permission error and a connect failure); source pins for the MCP call, the web map, the CLI keeping the live read, and the web card.
  • Revert check: with the timeout filters disabled, the stored-size flag removed from the tool and the error rewrite removed, 5 of the 11 tests fail (both slow-fact live tests, the stored-size live test, the error-text test, the call-site pin); restored, 11 of 11 pass.
  • Updated DarlingMcpStoreHostToolsLiveTests: it asserted size_bytes was non-null, which now needs a stored sweep row; it seeds one (and deletes it) and also asserts size_as_of / size_age_seconds under the mcp and viewer roles.
  • 803 tests across every class that mentions get_store_host or the store-size source (including McpToolsListBudgetTests, McpPayloadContractCensusTests, SerialLoopStoreSizeSourceTests, StoreHostProfileCacheTests, StartupHostProfileLogTests, DarlingStoreHostProfile*, DiagnosticsBundle*), 0 failed; 553 across all Census, Ratchet, pin and command-timeout classes (SlowClassCensusTests alone: 7 of 7); -trait Stage=Guard: 1,584 of 1,584. After merging origin/dev, the store-host, census and budget classes again: 180 run, 0 failed.
  • Not run: the full suite and Installer.Tests (CI decides). Not run: a real slow store; the 10 s boundary is exercised with a 1 s deadline.

For the reviewer: a first run of the "uncompressed read times out" test failed once because its default pg_database_size read exceeded the 1 s test deadline on a cold cluster; the test now stubs the other slow read, so each test exercises one fact.

Changelog

SECTION: Fixed
ENTRY:

  • get_store_host no longer fails when the store's size reads are slow ([get_store_host fails after 10 s when the store's live size reads are slow #5718]) - On a store whose size reads were slow, get_store_host (MCP and the web dashboard's store host card, which re-fetches it on each page load) failed every time after 10 s with "Exception while reading from stream", because the live store size and the live uncompressed-chunk total each had a 10 s limit and either one failing failed the whole profile. A slow size or uncompressed-chunk read now makes only that fact unavailable, with the reason (for example "timed out after 10 s"), and the rest of the profile returns. The MCP tool and the web card now take the store size from the hourly sweep's stored figure and report its age (size_as_of, size_age_seconds) instead of reading it live on every call; the uncompressed-chunk total is still read live because the sweep does not store it. --check-settings keeps the live size read and now prints the reason when a fact is unavailable. A timeout that still fails the whole read says it timed out and after how long.
    REF:
    [get_store_host fails after 10 s when the store's live size reads are slow #5718]: get_store_host fails after 10 s when the store's live size reads are slow #5718

Support bundle caller

DiagnosticsBundleRunner.StoreSectionAsync also calls GetStoreHost, so the bundle's store host section now carries the stored size too. It stays on the stored figure on purpose: a support bundle must not wait on a slow store, and the live pg_database_size walk is the read that measured 5.8 s mean / 30.7 s max. The section is the tool's whole payload, so it includes size_as_of, size_age_seconds, size_unavailable_reason and uncompressed_unavailable_reason, which tell the reader how old the number is or why it is missing. --check-settings is the only live caller. A test pins both facts.

Review round 1

All in commit 8d78abc81 (one commit, pushed to this branch).

  1. A refused uncompressed-chunk read answered 0 chunks. Only SQLSTATE 42P01, 3F000 and 42883 (the codes a database without TimescaleDB raises; IsTimescaleAbsent) keep the 0/0 answer. Every other PostgresException gives null bytes and count with uncompressed_unavailable_reason = this role cannot read it (SQLSTATE nnnnn), the timeout arm's shape. uncompressed_chunk_percent_of_ram was already computed only from a non-null byte count, so it is null with them. The tool guide sentence now says timeout or refusal. Tests: live theory for a missing view and a missing schema (both 42P01, answer 0/0), live theory for a raised 42501 and a division by zero (null + reason, connection still usable, size and buffer facts intact), and a pure theory on the classifier.
  2. The web card age froze. sweeps.js now has storeSizeAgeSeconds / storeSizeText, and the card cell calls storeSizeText(store, Date.now()): the age comes from size_as_of (via parseUtc, naive UTC) at render time, with size_age_seconds only as a fallback when size_as_of is absent. A replayed payload therefore ages. Test: a Node harness (store-host-size-age-harness.mjs, the repo's existing pattern) runs the shipped sweeps.js and shows one payload as 10.0 min, 11.0 min and 1.2 h old at three render times; Node-less machines skip it, and a source pin also checks the card derives the age from size_as_of. One trade-off: the age is now measured against the browser's clock, so a browser clock behind the store's reads 0 rather than negative.
  3. Support bundle. Kept on the stored figure, with the reason above; it already carried size_as_of and both reasons, now pinned by a test and explained in a comment.
  4. Stale derivations. The McpStoreHostProfileSeconds comment no longer says "TWO store-scaling statements" for the MCP path (it names the one store-scaling statement left and why the bound keeps two ceilings; the value is unchanged). The MCP read's wording in s_pgDatabaseSizeOwners now says only --check-settings pays the live size, and a new TheMcpStoreHostRead_IsNotAPgDatabaseSizePayer asserts the tool file has no pg_database_size, no StoreSizeSql, and passes useStoredStoreSize: true. (The file entry itself stays, because DarlingStoreHostProfile.cs still owns the literal.) I also reworded a tool comment that spelled pg_database_size so that assertion could be exact.
  5. Duplicate definition. StoreSelfMetrics.LatestStoreSizeSql and LatestStoreSizeWithTimeSql are now built from one private LatestStoreSizeTail const. The Replace-based pin is replaced by one that checks the two statements end in the same text and that the tail appears once in the source. The resulting SQL text is unchanged.
  6. PR body premise. Rewritten above (Why, step 2, changelog entry).

Tests run (rig: throwaway PostgreSQL with TimescaleDB, stopped): StoreHostSlowSizeTests + StoreHostSlowSizeLiveTests + SerialLoopStoreSizeSourceTests 46/46 (no skips); every *Census* and *Ratchet* class plus the touched and related classes 911 run, 0 failed, 1 skipped (a role-provisioning live test needing DARLING_TEST_PGRUNTIME); -trait Stage=Guard 1584 run, 1 failed on the first pass (TestsNoSilentPassGuardTests: my Node test returned from a catch block), fixed and re-run green; the LiveClockCensus class also failed once because my new test comment named the chunk catalog view, fixed by rewording. Not run: the full suite, and the live test that edits the rig's own conf (GatherAsync_ProducesEveryVerdict_EndToEnd_Gated, needs DARLING_TEST_PGRUNTIME).

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 12, 2026 23:13
why the figure is missing instead of failing the whole card. */
[cellText("Store size"), cellText(store.size_bytes == null
? "unavailable" + (store.size_unavailable_reason ? " (" + store.size_unavailable_reason + ")" : "")
: fmtMb(bytesToMb(store.size_bytes)) + (store.size_age_seconds == null ? "" : " (hourly sweep, " + fmtMs(store.size_age_seconds * 1000) + " old)"))],

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 age the card prints stops advancing after the first fetch.

size_age_seconds is a scalar the server computed once, at gather time, and it is baked into the payload. This page caches that payload and replays it on every poll tick without re-fetching — renderStoreHost at line 462:

const isPoll = !!(opts && opts.poll === true);
if (isPoll && lastStoreHostPayload) {
  renderStoreHostPayload(box, lastStoreHostPayload);   // no network call
  return;
}

So renderStoreHostPayload re-renders the same frozen integer on the 60 s poll, and only a first paint / hashchange / span change ever refreshes it.

Failure scenario: an operator opens Sweeps, the card reads Store size 225.0 GB (hourly sweep, 2.0 min old), and they leave the tab open. Sixty poll ticks later the stored figure is 62 minutes old, the service may have stopped sweeping entirely, and the card still reads 2.0 min old. The one number this PR adds to tell the reader how stale the size is is itself stale, without bound, and silently understates the age — which is the worse direction, because "2 minutes old" is the reading that makes an operator trust the byte count.

The payload already carries the fix: size_as_of is on the wire, and relTime is already in this file's import list and already parses the repo's naive-UTC stamps through parseUtc. Computing the age at render time ages correctly on the replay path:

Suggested change
: fmtMb(bytesToMb(store.size_bytes)) + (store.size_age_seconds == null ? "" : " (hourly sweep, " + fmtMs(store.size_age_seconds * 1000) + " old)"))],
: fmtMb(bytesToMb(store.size_bytes)) + (store.size_as_of == null ? "" : " (hourly sweep, " + relTime(store.size_as_of) + ")"))],

(relTime renders "12m ago" / "2h ago" rather than "12.0 min old"; if the "old" wording matters, derive the seconds from Date.now() - parseUtc(store.size_as_of) at render time instead. Either way the derivation belongs in renderStoreHostPayload, not in the cached payload.) size_age_seconds is still right for MCP callers, where the response is read once.

Comment on lines 746 to 753
catch (PostgresException)
{
/* timescaledb_information.chunks does not exist on this connection's database -- no TimescaleDB
extension here (plain PostgreSQL BYO, or preloaded-but-not-CREATE-EXTENSION'd, same shape the
JobExecutionLoggingSql remarks describe for a different GUC). Zero/zero is the honest answer. */
uncompressedBytes = 0;
uncompressedCount = 0;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Every non-timeout PostgresException here still becomes a measured "0 uncompressed chunks, 0.0% of RAM".

This PR gave the fact a null-with-reason vocabulary and then used it only for the timeout arm. Two blocks up, the size read's sibling catch now does the honest thing:

catch (PostgresException pg)
{
    storeSizeBytes = null;
    storeSizeAsOf = null;
    storeSizeReason = $"this role cannot read it (SQLSTATE {pg.SqlState})";
}

This one keeps the pre-#5718 reading — and the diff re-affirms it by writing the two zeros out explicitly. The comment justifies that for exactly one SQLSTATE ("timescaledb_information.chunks does not exist on this connection's database"), but the catch is unfiltered, so it swallows every other error code into the same answer.

Two reachable ones, neither of them "no TimescaleDB":

In both cases the wire says uncompressed_chunk_bytes: 0, uncompressed_chunk_count: 0, uncompressed_chunk_percent_of_ram: 0.0, uncompressed_unavailable_reason: null, and the web card prints 0 chunks, 0 MB (0.0% of RAM) — indistinguishable from a fully-compressed store. The #4211 figure is a RAM-pressure signal, so the fabricated zero reads as all clear in the one direction that matters.

This is the trap the repo already names: "CASE WHEN interval > 0 THEN delta / interval ELSE 0 END spells "unknowable" as 0 ... End at END (NULL) and carry the NULL to the wire" (#3707), and "Likewise IsDBNull ? 0: an unmeasured pass is null with a *_measured flag beside it, not 0 MB" (#3682).

Narrow the zero to the SQLSTATE the comment actually argues for, and let the rest take the reason path the size read now has:

Suggested change
catch (PostgresException)
{
/* timescaledb_information.chunks does not exist on this connection's database -- no TimescaleDB
extension here (plain PostgreSQL BYO, or preloaded-but-not-CREATE-EXTENSION'd, same shape the
JobExecutionLoggingSql remarks describe for a different GUC). Zero/zero is the honest answer. */
uncompressedBytes = 0;
uncompressedCount = 0;
}
catch (PostgresException pg) when (pg.SqlState == PostgresErrorCodes.UndefinedTable
&& reads.UncompressedChunkSizeSql == UncompressedChunkSizeSql)
{
/* timescaledb_information.chunks does not exist on this connection's database -- no TimescaleDB
extension here (plain PostgreSQL BYO, or preloaded-but-not-CREATE-EXTENSION'd, same shape the
JobExecutionLoggingSql remarks describe for a different GUC). Zero/zero is the honest answer. */
uncompressedBytes = 0;
uncompressedCount = 0;
}
catch (PostgresException pg)
{
/* #5718: anything else -- a chunk dropped by a retention/compression policy between the view read
and the per-chunk regclass cast (42P01 mid-walk), a column drift on a TimescaleDB upgrade
(42703, the #3918 shape), a refused grant -- is UNKNOWN, not zero. A fabricated 0 here reads as
"nothing uncompressed, 0.0% of RAM", which is the #4211 signal saying all-clear. */
uncompressedBytes = null;
uncompressedCount = null;
uncompressedReason = $"this role cannot read it (SQLSTATE {pg.SqlState})";
}

The when clause's second conjunct is only there because the 42P01 reading is a property of the shipped statement; if you would rather not couple to the seam, drop it and keep the SQLSTATE test alone. Either way the "a store without TimescaleDB still reports 0/0" claim in the PR body keeps holding — DarlingStoreHostProfileLiveTests line 170-171 still passes — and the reason wording stays the one DescribeReadTimeout's sibling already uses, so the two facts cannot word the same situation differently.

Comment on lines +112 to +116
/* #5718: the polled reads (this tool, and the web page that calls it every few minutes) take the
store size from the hourly sweep's stored figure and report its age, rather than paying
pg_database_size on every poll. --check-settings keeps the live read. */
return await DarlingStoreHostProfile.GatherAsync(
postgresConfig, connection, token, useStoredStoreSize: true);

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, but worth saying out loud: this flag reaches a third caller the PR body never names.

DarlingCloudIdentityProbeTests.GetStoreHostCallSite_OnlyInTheMcpToolTheWebDispatchAndTheDiagnosticsBundle pins three call sites, and the third is DiagnosticsBundleRunner.cs:493:

var host = DiagnosticsBundle.ParseReader(
    await DarlingMcpStoreHostTools.GetStoreHost(postgres, config.Postgres, new StoreHostProfileCache(TimeSpan.FromMinutes(1)), ct));

That is not a polled read — it is a one-shot verb an operator runs by hand, the same category as --check-settings, and the test's own comment says so ("read ... once, from a verb an operator runs by hand, as --check-settings does through GatherAsync"). The body's rule ("the polled reads use the stored size ... --check-settings keeps the live read") would put the bundle on the live side; the code silently puts it on the stored side, so a support bundle can now report a store size up to an hour old.

I don't think this needs reverting — the bundle embeds the whole JSON, so size_as_of / size_age_seconds / size_unavailable_reason travel with it, and a bundle that can't hang for 30 s on pg_database_size is arguably the better bundle. But it is a deliberate call that nothing in the PR records, and the one-minute cache it passes is now pointless for the size (the figure behind it only moves hourly). Please either state the choice in the body/changelog, or pass useStoredStoreSize: false through for the bundle.

Comment on lines +565 to 570
/// <see cref="GatherStoreFactsAsync"/> (see <c>SerialLoopStoreSizeSourceTests</c>, #3199). #5718: the
/// POLLED reads (<c>get_store_host</c> over MCP and the web page) no longer run this SQL at all — they take
/// the sweep's stored figure (<c>StoreSelfMetrics.LatestStoreSizeWithTimeSql</c>) and report its age,
/// because the web page polls every few minutes and this read measured 5.8 s mean / 30.7 s max on a slow
/// store; <c>--check-settings</c> is the only caller left, and it fails this one fact alone on a timeout.</summary>
public const string StoreSizeSql = "SELECT pg_database_size(current_database())";

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: this paragraph is right, and it leaves two derivations elsewhere describing the world it just changed.

1. ServiceCommandDeadlines.McpStoreHostProfileSeconds (ServiceCommandDeadlines.cs:483-491). Its derivation is spelled out and is now wrong for the MCP path:

GatherAsync runs, sequentially, the version/buffer/size/uncompressed-chunk reads (GatherStoreFactsAsync, each individually bounded at CliStoreReadSeconds) ... Only two of those five statements are store-scaling (StoreSizeSql, UncompressedChunkSizeSql ...), so the backstop clears TWO store-scaling statements at their own ceiling plus the same 5s margin

After this change the MCP path runs exactly one store-scaling statement, and (CliStoreReadSeconds * 2) + 5 is no longer the arithmetic of what it bounds. The constant's value is still a safe bound, so nothing breaks — but this repo treats those derivations as load-bearing rather than decorative, and the next reader sizing this will be reasoning from a premise the diff retired.

2. SerialLoopStoreSizeSourceTests.s_pgDatabaseSizeOwners's fourth-regime reason (SerialLoopStoreSizeSourceTests.cs:95-113) still says the entry covers "GatherStoreFactsAsync's only TWO callers ... --check-settings and the get_store_host MCP read below". That file's own doc says "The reasons are the assertion, not decoration — a third entry appearing means someone put a store-wide walk somewhere new, and the failure message hands them these two so they have to say which regime theirs is in." The census still passes (one literal, same file), but the reason now over-states who pays for the walk: only --check-settings does.

Comment on lines 1213 to 1221
/// so the same backward scan serves it. No parameters.
/// </summary>
public const string LatestStoreSizeWithTimeSql = $@"
SELECT total_bytes, metric_time
FROM collect.store_metrics
WHERE object_kind = '{StoreObjectKind}'
AND total_bytes IS NOT NULL
ORDER BY metric_time DESC
LIMIT 1";

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, and the pin in StoreHostSlowSizeTests.TheStoredSizeSql_IsTheRecordedSizeSqlWithTheTimeAdded does defuse the worst version of this — but the shape is one the traps file names:

Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy.

The predicate, the IS NOT NULL filter, the ordering and the LIMIT are now written twice, four lines apart, and kept honest by a test that string-replaces one into the other. That pin is asymmetric (a one-sided edit to either constant fails it), so it is better than the #3684 case — but it only exists because the copy exists. A shared tail makes the copy impossible and the pin unnecessary, and const string concatenation is a compile-time expression so both stay const:

    /// <summary>The body both latest-store-size reads share — predicate, ordering and bound — so the two
    /// projections below cannot drift apart (#5718; the #3684 "alias, do not copy" rule).</summary>
    private const string LatestStoreSizeBodySql = $@"
FROM collect.store_metrics
WHERE object_kind = '{StoreObjectKind}'
AND   total_bytes IS NOT NULL
ORDER BY metric_time DESC
LIMIT 1";

    public const string LatestStoreSizeSql = "\nSELECT total_bytes" + LatestStoreSizeBodySql;

    /// <summary>
    /// <see cref="LatestStoreSizeSql"/> with the row's <c>metric_time</c> (naive UTC) added (#5718): the
    /// polled <c>get_store_host</c> reads take the store size from the hourly sweep instead of paying
    /// <c>pg_database_size</c> on every poll, and report how old the figure is. Same predicate and ordering,
    /// so the same backward scan serves it. No parameters.
    /// </summary>
    public const string LatestStoreSizeWithTimeSql = "\nSELECT total_bytes, metric_time" + LatestStoreSizeBodySql;

SerialLoopStoreSizeSourceTests.TheDiskCheckExecutesTheRecordedSizeConstant reads LatestStoreSizeSql at runtime (Assert.Contains("collect.store_metrics", …), "ORDER BY metric_time DESC", "LIMIT 1") and TheRecordedSizeReaderAndWriterCannotDisagreeAboutTheKind checks 'store' is in it — all still satisfied by the concatenated value. Only the new TheStoredSizeSql_… pin becomes redundant.

claude[bot]
claude Bot previously requested changes Oct 12, 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

Two blockers, each with a patch in an inline comment; the mechanism itself (per-fact containment, the stored-size switch, the error rewording, the CLI keeping the live read) reads correct and is well pinned.

  • sweeps.js:498 — the age the card prints never advances after the first fetch.
  • DarlingStoreHostProfile.cs:746-753 — the uncompressed read still answers "0 chunks, 0.0% of RAM" for every non-timeout PostgresException, while its sibling size read, in this same diff, learned to say why.

Claims

  • VERIFIED — Timeouts matched on type and SQLSTATE, never message text; the caller's cancellation never swallowed — DarlingStoreHostProfile.cs:605-607, DarlingAlertReadAdapter.cs:415-431, plus !cancellationToken.IsCancellationRequested on both filters (:694, :740).
  • VERIFIED — The polled MCP/web reads take the stored size, run no pg_database_size, and a refused grant degrades with a reason — DarlingMcpStoreHostTools.cs:113-116; DarlingStoreHostProfile.cs:674-683, :701-708.
  • VERIFIED — size_age_seconds is computed as of the response — DarlingMcpStoreHostTools.cs:130-132 sits outside the cache.GetOrGatherAsync delegate. True for MCP; see REFUTED for the web.
  • VERIFIED — --check-settings keeps the live read and gains the reasons in text and --json; a store without TimescaleDB still reports 0/0 — DarlingCliCommands.cs:948, :1137-1140, the format hunks, :746-753.
  • VERIFIED — The web map calls GetStoreHost; Lite has no counterpart; the served head is byte-identical and the new prose rides the tail — DarlingWebEndpoints.cs:5790; empty Lite grep + DarlingMcpStoreHostTools.cs:31; every added sentence sits after <<GUIDE>>, and the canonical budget file records Served.Length only (McpToolsListBudgetTests.cs:729-730).
  • VERIFIED — FailureFor keeps the envelope and the nullable widening breaks no caller — McpHelpers.cs:783 renders ex.Message alone; the four positional new HostStoreFacts(...) sites still bind on the optional members.
  • UNVERIFIED — The production measurements (10.0-10.9 s failures, 22/day; 5.8 s mean / 30.7 s max; 11.8 s / 49.4 s) — field readings off a live slow store.
  • UNVERIFIED — Npgsql 10.0.3's client-deadline exception shape, and that the connection stays reusable after it. If it instead breaks the connection, BufferAndTempSql (:712) sits outside every try and reintroduces the whole-profile failure.
  • UNVERIFIED — The 11 new tests, the revert check, and the suite counts (738 / 1,584 / 803 / 553 / 180) — dotnet build is unavailable here, and five of the eleven Assert.SkipWhen out without DARLING_TEST_PG.
  • REFUTED — "the web page now polls it every 2 to 3 minutes", as the justification for the switch — sweeps.js:462-466 replays the cached payload on poll ticks and re-fetches only on first paint, hashchange or span change. The page did re-fetch every 60 s while broken (the fetch errored, so lastStoreHostPayload was never assigned and the guard never fired — that is the one-[ERROR]-per-poll symptom), but once the read succeeds the card stops calling the tool. The switch still stands on the MCP tool's own account; the same replay path is what breaks the new age field.

Riskiest lines

  1. wwwroot/js/pages/sweeps.js:498 — a server-computed scalar rendered on a path replayed from cache every 60 s, so the age freezes — UNPINNED (TheWebCard_ShowsTheSizeAge_… greps the file, it does not execute the render).
  2. DarlingStoreHostProfile.cs:746-753 — an unfiltered catch (PostgresException) answering 0/0 for every SQLSTATE — UNPINNED beyond the no-TimescaleDB case (DarlingStoreHostProfileLiveTests.cs:170-171 asserts the 0/0 that makes the fabricated zero look right).
  3. DarlingStoreHostProfile.cs:674-683 — the stored-size read and its DateTimeKind.Unspecified stamp; the naive-UTC convention checks out (PgMigrations.cs:4422, DarlingWorker.cs:10718, DarlingMcpStoreHostTools.cs:198-202) — pinned by APolledRead_TakesTheStoredSweepSize_…_Live and APolledReadWithNoStoredFigure_…_Live, both live-only.

Traps near this diff

  • "CASE WHEN ... ELSE 0 END spells "unknowable" as 0 ... End at END (NULL) and carry the NULL to the wire." (#3707) — applies — :746-753; uncompressed_chunk_percent_of_ram follows the fabricated zero to 0.0, and the null-and-reason vocabulary sits three lines up at :740-745.
  • "Likewise IsDBNull ? 0: an unmeasured pass is null with a *_measured flag beside it, not 0 MB (#3682)." — applies — same site; the diff adds the flag and leaves it null on the arm that needs it.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies — StoreSelfMetrics.cs:1201 and :1215, kept in step by a Replace-based pin. Milder than #3684 (the pin is asymmetric), but a shared const tail removes the copy and the pin.
  • "Lite and Darling ship in parity ... a fix to one SKU's tool or description without the counterpart's is a finding" — does not apply — the Lite grep is empty; DarlingMcpStoreHostTools.cs:31 records the tool on KnownLiteMissingMcpTools.
  • "A JSON number above 2^53 is rounded by every JavaScript client (#3487)." / "chunk_compression_stats returns ZERO rows ... count from timescaledb_information.chunks (#3610)." / "a clamp is a silent lie about the window (#3699)." — do not apply — the new 64-bit fields are a byte count (~2.4e11) and an age in seconds; the read already counts from timescaledb_information.chunks (:587-592); the only clamp is Math.Max(0L, …) on an output age, not an input window, with writer and reader on one clock.

Divergence (blind read vs PR body)

  1. The web-poll premise and the age field collide. The body justifies the switch with a poll that sweeps.js:462-466 does not perform — and that same replay path is exactly what makes size_age_seconds go stale on screen. "A cached profile still reports a true age" holds for MCP, not for the web card the sentence is about.
  2. A third caller the body never names. DiagnosticsBundleRunner.cs:493 also moves off the live read, though the body's own rule puts one-shot operator verbs on the live side with --check-settings.
  3. Two derivations left describing the old world. ServiceCommandDeadlines.cs:483-491 still derives the bound from "TWO store-scaling statements"; SerialLoopStoreSizeSourceTests.s_pgDatabaseSizeOwners still names the MCP read as a pg_database_size payer. Neither fails.

Everything else matched the blind read.

Reviewer checklist

  • Build and run the suite — nothing here was compiled. Watch that Assert.Equal(0, profile.Store.UncompressedChunkBytes) (DarlingStoreHostProfileLiveTests.cs:170-171) still infers T = long?, and that Assert.Equal(1, await after.ExecuteScalarAsync(ct)) (StoreHostSlowSizeTests.cs:166) binds against the boxed object?.
  • Run the five live tests with DARLING_TEST_PG against a cluster that may already hold object_kind = 'store' rows — APolledRead_… assumes its probe row is the newest one.
  • Confirm Npgsql leaves the connection reusable after a client CommandTimeout.
  • Load the Sweeps page against a real store and leave it open an hour — the card should age its "hourly sweep, N old" text, not freeze it.
  • On a store that has never swept, confirm the card and the payload read "no hourly sweep has recorded the store size yet".

…ard age follows the clock (#5718)

- The uncompressed-chunk read answers 0 chunks only when TimescaleDB is absent
  (42P01, 3F000, 42883); any other refusal is null with the SQLSTATE as its reason.
- The web card derives the stored size's age from size_as_of at render time, so a
  replayed poll payload keeps ageing; Node-run test.
- The support bundle stays on the stored size and says why in a comment; pinned.
- Update the McpStoreHostProfileSeconds derivation and the pg_database_size census
  note; assert the MCP read is not a pg_database_size payer.
- One shared tail for the two recorded-size statements, replacing the Replace pin.
@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 13, 2026 00:47

Claude posting for Erik Darling. Addressed in 8d78abc: the chunk read keeps 0 only for a missing TimescaleDB and reports anything else as unknown with its SQLSTATE; the web card computes the age from size_as_of at render time; the bundle and the deadline derivation say they read the stored size.

@erikdarlingdata
erikdarlingdata merged commit 576516d into dev Oct 13, 2026
26 of 28 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5718-store-host-slow-size branch October 13, 2026 01:28
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