Give every Darling storage command an explicit deadline (Part of #2874)

[erikdarlingdata]

Part of #2874 — group 2 of the untimed-command sweep: PerformanceMonitor.Darling.Storage, 70 sites.

The census, re-derived

The issue's original count said 20; the corrected census said 67. Sweeping both construction shapes on current dev with statement-span semantics found 124 sites, 70 untimed — and the last of those 70 was found only by the C# scanner: two independent Python scanners each missed one real site by leaking a statement span across a closing brace (details below).

Five budget regimes, deliberate bounds — not one blanket value

regime	sites	bound	why
MCP read surface (DarlingPg*Reader, trend reads, QS trend routing)	48	new StorageCommandDeadlines.McpReadSeconds = 30	see below
Migration session scaffolding (PgMigrations)	6	existing MigrationCommandTimeoutSeconds (300)	the rung apply already used it; the scaffolding around it (search_path, version table, version read, stamp, database search_path) did not
Migration advisory-lock wait (PgMigrations)	1	new MigrationLockWaitTimeoutSeconds = 5 x MigrationCommandTimeoutSeconds (1500)	a service starting while a sibling migrated >30s died mid-lock-wait at the inherited default. Review was right that the first fix set this to ONE statement's budget while the lock is held for a whole multi-rung session — derived separately below
TimescaleDB setup / rearm	3	existing SetupTimeoutSeconds (300)	joins the 25 sibling sites already on it
TimescaleDB catalog + rollup-coverage reads	5	existing JobCatalogReadTimeoutSeconds (30)	measured: the coverage probe's unbounded min(collection_time) runs 685 ms cold on the largest store's 23 GB hypertable — chunk exclusion answers it, so this is NOT the #2795 shape, and 30s carries ~44x headroom. Measured rather than assumed because this read gates retention arming (#1680): a wrong bound here would hold retention silently.
Startup self-test	3	the file's own per-layer budget ((int)NetworkProbeTimeout.TotalSeconds = 8)	a self-test should answer fast; its own file already made that choice for the network layers
Plan-dim maintenance	3	hoisted MaintenanceStatementTimeoutSeconds = 300	the estimate already chose 300 as a literal; hoisted and shared. VACUUM FULL keeps its explicit CommandTimeout = 0 — unlimited is the deliberate choice there, not an omission.
Slice repair rail-lift	1	the file's own SliceStatementTimeoutSeconds (900)	the one site the Python scanners missed

The new value, bounded on both sides

McpReadSeconds = 30:

• Above the measured worst case. Every verified read in the family, timed against BOTH production stores: 97–685 ms (the 685 ms being the unbounded-min class on the largest store). The shipped trend reads are all per-server and time-windowed; a deliberately HARDER superset — the same aggregate unfiltered, fleet-wide, over seven days — measured 35.2 s, bounding any single shipped read far below it. ≥8x headroom over the most pessimistic estimate, ~44x over anything observed. (Measurement scope, stated honestly: the per-server windowed read itself returned in ~50 ms wall-clock on three runs but its output capture failed each time, so the floor rests on the verified reads plus the superset bound, not on that number.)
• Below the point where hanging beats failing. These reads have no enclosing CancelAfter and nothing restarts them — a stalled read holds a pooled store connection and hangs the MCP tool until the client gives up. 60 s is where a budget-rescued pass put a read (PgBaselineProvider inherits Npgsql's 30s default CommandTimeout, and is hitting it in production #2871); an unbudgeted interactive read sits strictly under that.

The lock wait, bounded on both sides (review finding)

MigrationLockWaitTimeoutSeconds = 5 x MigrationCommandTimeoutSeconds (1500 s). The first pass gave the pg_advisory_lock acquire the statement bound, which is the wrong quantity: the lock is taken ONCE and held while MigrateLockedAsync applies EVERY pending rung in the same session, so a several-rung upgrade could outlast the waiter while every individual statement stayed inside its own limit.

• Floored on the total hold time, counted from the ladder. Exactly four of the 108 rungs touch pre-existing data: V22 (index across every existing chunk of the populated index_object_stats hypertable), V23 (create_hypertable ... migrate_data => true), V39 (two partial indexes over query_stats and procedure_stats), V104 (index on pg_deadlocks). The other 104 create the table they then index, or are metadata-only ADD COLUMN / ALTER ... SET SCHEMA / DROP NOT NULL / view refreshes — the whole 108-rung ladder measured 0.301 s end to end on a fresh PostgreSQL 17.11 / TimescaleDB 2.29.2 store (local Docker rig), advisory lock and version stamps included. Each of the four is one command already capped at the statement bound, so four multiples is the floor and the fifth is margin for the next data-moving rung. Seeded locally for scale, warm: V22 1.29 s over 907 MB / 90 chunks, V23 9.37 s over a 608 MB heap, V39 1.44 s over 1.26 GB — against 2.72 GB, 0.69 GB and 24.5 GB in those same tables on the live 42-server store.
• Note this corrects MigrationCommandTimeoutSeconds' own framing. Its comment singles out V23 as the data-moving rung; V22, V39 and V104 build btree indexes over already-populated hypertables, which TimescaleDB propagates to every existing chunk. V39's two targets are the store's largest fact tables.
• Capped from above by nothing — that is the finding, not an omission. MigrateAsync has exactly one production caller, DarlingWorker.RunCollectionLoopAsync, on the plain stopping token: no CancelAfter on the path, no configured HostOptions.StartupTimeout (framework default is infinite), no health check or readiness probe in the repo, no container HEALTHCHECK, no orchestrator manifest. The installers' 60 s / 2 min WaitForStatus('Running') do not bound it either — the worker is a BackgroundService, so the service reports Running before the first migration statement runs. So the value comes from the asymmetry: a waiter that dies mid-wait hits LogCritical and returns out of the collection loop, collecting NOTHING until an operator restarts it with no retry; one that waits longer only delays its own first cycle, its MCP and web surfaces having started independently. Long but finite, so a wedged holder still yields a readable deadline rather than a silent hang.
• Proven wired, not just declared. With the statement bound temporarily at 3 s and the lock wait at 12 s, a contended MigrateAsync against a held advisory lock failed at 12.02 s; reverting only the acquire to the old constant moved that to 3.02 s. Both surfaced as Exception while reading from stream with an inner TimeoutException — the exact misdiagnosis 133 NpgsqlCommand sites still inherit Npgsql's undocumented 30s default timeout #2874 exists to remove. 1500 read back out of the rebuilt assembly by reflection (MVID changed across rebuilds, so each provably took).
• This NARROWS the startup failure rather than closing it, and the CHANGELOG no longer says "closing". A fifth data-moving rung, or one rung genuinely needing longer than its own bound, still outlasts the wait. Making the wait scale with pending rung count, or moving to pg_try_advisory_lock with a diagnostic retry loop, is deliberately NOT built here.

Census correction (review finding)

The pin's doc comment said 69 untimed and 49 of the .CreateCommand( shape while the CHANGELOG and this body said 70. Seventy is right. Re-derived by lifting the pin's own regexes and depth <= 0 span walker verbatim into a net10.0 harness and running it over the merge-base tree (8e046fa3) — the only tree the claim describes, since post-fix the count is zero by construction:

	total	new NpgsqlCommand(	.CreateCommand(
sites	124	74	50
untimed	70	20	50

So 50 of 70, not 49 of 69 — every one of the project's 50 .CreateCommand( sites was untimed, which is why that bucket cannot be 49 under any scanner variant. The 69 is traceable to a partly-corrected scan: the same walker with its depth counter clamped at zero reports 68, missing exactly PgMigrations.cs:3981 (the version read) and QueryStoreSliceRepair.cs:321 (the rail-lift). a4bd263c's comment named the first, 1593e4f8's named the second; the total was written when only one of the two had been found and never refreshed, and 49 follows by subtracting the correct 20 from the stale 69.

367 of 371 is verified and unchanged — it is Darling-wide, not project-scoped: .CreateCommand( across the three production Darling projects is .Service 131 + .Viewer 190 + .Storage 50 = 371, of which 127 + 190 + 50 = 367 were untimed. The harness also reproduces the issue's original new-NpgsqlCommand-only figures for .Storage (20) and .Viewer (2) exactly, which is the check that says it counts the way the census did.

The scanner finding worth keeping

Site 70 (QueryStoreSliceRepair's SET LOCAL rail-lift, inside a using (...) { } statement) was judged already-timed by two independent Python scanners — one clamping nesting depth at zero, one terminating only at exactly zero — because both leaked the statement span across the enclosing block's closing brace into a neighbour that DOES set a deadline. The CI-proven depth <= 0 walker from #2882 judged it correctly. The pin's span walker uses those semantics, and a fixture now reproduces the nested-using shape so the variant that misses it can't come back.

Pin

StorageCommandTimeoutTests — directory-scoped where the alert-pass pin is name-scoped, deliberately: the claim here is a property of the project ("every command's deadline was chosen"), not of a budget boundary no filename expresses, so a future file is covered the day it appears. Sweeps both construction shapes; floors on total site count and file count so an empty sweep fails loudly. The regime constants are NOT frozen here — only the new constant's band is pinned.

Proven red three ways, each failing a different assertion, via a net10.0 harness running the identical C# walker against the real tree and reflecting the constant from the rebuilt assembly (printed 30 → 2 → 120 → 30 across rebuilds, so each rebuild provably took):

1. revert one site → structural scan reports exactly DarlingPgWaitReader.cs:83
2. value 2 → lower band fails
3. value 120 → upper band fails

Not touched

InsertCollectionLogSql and its writers — no site in that family was untimed, no parameter handling changed anywhere in this PR, and the arity pin over its writers runs unchanged in CI. .Service (198) and .Viewer (192) remain; census map updated on #2874.

Verification scope: both projects build with EnableWindowsTargeting; the Windows suites cannot run on macOS, so the xUnit copy of the scanner is exercised by CI while the identical-algorithm harness above ran locally. Store measurements were read-only (default_transaction_read_only = on, statement timeouts set).

🤖 Generated with Claude Code

https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy

[claude]

This new doc comment (lines 121-126) is inserted directly after the existing doc comment for EstimateCompactedSql (the "fast estimate... sampled rows" block just above), with no declaration in between. In C#, adjacent /// blocks merge into a single doc comment for the next declaration — so both <summary> blocks now attach to MaintenanceStatementTimeoutSeconds here, and EstimateCompactedSql (a public const string, a few lines below) loses its documentation entirely. Since CONTRIBUTING.md calls for XML docs on public APIs and this codebase leans on comments to carry the "why", worth moving this summary below the EstimateCompactedSql declaration so each member keeps its own doc.

[claude]

This closes the specific case described in the PR body (a sibling migrating for >30s), but the bound only matches a single statement's budget, not the full session's. The advisory lock is acquired once, and MigrateLockedAsync then applies every pending rung sequentially in one session, each individually allowed up to MigrationCommandTimeoutSeconds (300s). If a store is several rungs behind and more than one of those rungs is non-trivial (today only V23's migrate_data conversion is called out as potentially slow, but nothing prevents a future rung from also being data-moving), the total hold time on this lock can exceed 300s even though every individual statement stayed within its own budget — and a sibling waiting here with the same 300s CommandTimeout would still die mid-wait, just less often than before. Worth considering whether the lock-wait timeout should scale with the number of pending rungs (or just be given deliberately more headroom than a single statement's bound) rather than mirroring MigrationCommandTimeoutSeconds directly.

[claude]

This doc comment (and the two others at lines 47 and 122) say "Sixty-nine"/"69" sites, but the CHANGELOG entry and the PR title/description both say "seventy"/"70" untimed sites. Since this codebase treats these comments as load-bearing measurements rather than decoration (per CONTRIBUTING.md's "comments carry measurements... issue numbers"), worth reconciling which count is actually correct before merge so the test file and the changelog don't quietly disagree about the census this PR is closing out.

[claude]

Reviewed this as a mechanical, well-scoped sweep giving every Darling storage command an explicit CommandTimeout — the diff matches its own description closely and I verified every file in PerformanceMonitor.Darling.Storage that constructs an NpgsqlCommand/.CreateCommand(...) now has a CommandTimeout set somewhere (no missed sites found by a project-wide sweep).

Three inline notes, none blocking but worth a look before merge:

1. PlanDimRecompression.cs — the new doc comment for MaintenanceStatementTimeoutSeconds merges with the preceding EstimateCompactedSql doc comment (no declaration between the two /// blocks), so EstimateCompactedSql silently loses its XML doc.
2. PgMigrations.cs — bumping the pg_advisory_lock wait to MigrationCommandTimeoutSeconds (300s) matches a single statement's budget but not the full migrate session's cumulative duration across multiple pending rungs, so the same failure mode can recur if a future rung is also slow.
3. StorageCommandTimeoutTests.cs — its doc comments say "69" sites in three places; the CHANGELOG and PR description say "70". Worth reconciling before merge.

Parity check: this only touches Darling's Npgsql layer. Confirmed Lite doesn't need a counterpart change here — DuckDBCommand.CommandTimeout defaults to 0 (unlimited), not Npgsql's undocumented 30s, and that's already called out in Lite/Services/RemoteCollectorService.cs. No drift.

No security or T-SQL style concerns — this PR doesn't touch any T-SQL, and no new SQL injection surface (all commands remain parameterized; the one string-interpolated ALTER DATABASE statement was pre-existing and only gained a timeout here).

[claude]

Minor count mismatch: this says "all forty-nine of those commands set no CommandTimeout," but the actual sweep here touches 48 sites, not 49 — grep -c "CommandTimeout = StorageCommandDeadlines.McpReadSeconds" across the diff returns 48, matching the PR description's own table ("MCP read surface … | 48"). Worth fixing since this codebase's own convention (and this very PR's body) treats exact counts in comments as load-bearing — a stale/off-by-one count here is the same failure mode the PR spends several paragraphs correcting for the census numbers.

[claude]

Reviewed. Well-scoped, well-documented sweep -- every new CommandTimeout assignment follows the codebase's existing per-regime pattern. Independent spot-check: a raw scan for command constructions across Darling/PerformanceMonitor.Darling.Storage/*.cs returns exactly 124 sites, matching the PR's claimed total, and 48 sites are wired to StorageCommandDeadlines.McpReadSeconds, matching the table in the description.

Notes:

• Left one inline nit: StorageCommandDeadlines.cs's doc comment says forty-nine MCP-read sites but the actual count (and the PR's own table) is 48.
• No Lite/Darling parity gap here -- this fixes an Npgsql-specific inherited-default problem (network round-trip commands with no explicit timeout); Lite's DuckDB access is in-process/embedded and has no equivalent undocumented-network-timeout failure mode, so there is nothing for Lite to mirror.
• MigrationLockWaitTimeoutSeconds's lack of an upper bound is explicitly disclosed as a known limitation rather than something silently introduced, and the changelog wording was correctly softened from closes to narrows to match.
• No correctness bugs, injection risk, or missing null/edge-case handling found in the changed sites -- the CommandTimeout assignments are consistently placed right after command construction, before execution.

[claude]

Reviewed. This is a clean, well-scoped change — verified the following, no issues found:

• Coverage is complete and accurate. Cross-checked every new NpgsqlCommand(/.CreateCommand( construction site in Darling/PerformanceMonitor.Darling.Storage/*.cs (touched and untouched files alike) against CommandTimeout assignments, including multi-line object initializers. Every site in the reader family, PgMigrations.cs, TimescaleSupport.cs, PlanDimRecompression.cs, QueryStoreSliceRepair.cs, and StoreConnectionSelfTest.cs now sets an explicit deadline. The one apparent gap (PayloadDimensionWriter.cs:89, no CommandTimeout in the initializer) is a deliberate pre-existing exception — commandTimeoutSeconds is optional and conditionally applied a few lines later for the general payload-flush path (per its own docstring, out of 133 NpgsqlCommand sites still inherit Npgsql's undocumented 30s default timeout #2874's scope) — not a miss.
• Lock-wait vs. statement-bound distinction is correct. Giving pg_advisory_lock its own MigrationLockWaitTimeoutSeconds (5× the statement bound) rather than reusing MigrationCommandTimeoutSeconds is the right call — Npgsql's CommandTimeout does cancel a blocked advisory-lock wait via cancellation request, and the old shared-bound version really would have killed a waiter mid multi-rung upgrade held by a sibling instance.
• Regime boundaries are sensible. MCP reads get the new bounded McpReadSeconds (30s, justified with measured data in both the constant's doc comment and StorageCommandTimeoutTests), while migration/setup/maintenance/self-test paths keep or hoist their own pre-existing budgets rather than being flattened to one blanket value. VACUUM FULL's CommandTimeout = 0 is correctly left alone.
• No Lite/Darling parity gap. This is inherently Darling-only — these are network round trips to a remote PostgreSQL store via Npgsql; Lite's DuckDB is embedded/local and doesn't share this failure mode, and 133 NpgsqlCommand sites still inherit Npgsql's undocumented 30s default timeout #2874 is explicitly scoped to Darling.Storage.
• Test file is solid. StorageCommandTimeoutTests.cs's directory-scoped sweep (vs. the sibling AlertPassCommandTimeoutTests' name-scoped one) is a good structural choice given the claim being made, and the depth <= 0 statement-span walker correctly handles the nested-using blind spot that tripped the two Python scanners during authoring.

No T-SQL changed (pure C#), so the T-SQL style conventions don't apply here. Didn't spot any correctness, security, or performance regressions in the diff.