Repository navigation
Part of #2874 - #2931
Part of #2874#2931
Conversation
All 125 command sites under Mcp/, in CustomViewStore and in the composed-query runner set no CommandTimeout and inherited Npgsql's undocumented 30 s default. Two regimes. The 124 shipped-shape reads take McpCommandDeadlines.ReadSeconds (20), floored above the mcp role's 15 s statement_timeout so the server-side timeout stays the one that fires on a managed store -- 57014 names its cause where Npgsql's deadline renders as a stream fault -- and capped below the 30 s it replaces, relationally against StorageCommandDeadlines.McpReadSeconds so the two halves of one tool call cannot disagree. The composed-query runner takes ComposedQuerySeconds, bound by reference to the operator's own clamped maximum, because compose_statement_timeout_seconds is the declared bound for that query class and a fixed constant under it would override a deliberate setting. The pin is scoped to Mcp/ recursively plus the two files by name, and carries a receiver allowlist: .Service builds commands against monitored targets as well as the store, so a store-derived deadline landing on a target command has to fail rather than look deliberate.
A red-first variant planted a monitored-target command in Mcp/ and came back GREEN. The reason is a shape no regex in this sweep matches: a QUALIFIED type name, `new Npgsql.NpgsqlCommand(`. It occurs once in the repo, in DarlingWorker's pg_stat_statements text fetch, which is a monitored-target command that already carries its own 60 s -- so the blind spot has no defect behind it and the untimed census does not move, but the site census for .Service is 262 rather than 261. Both the construction scan and the receiver allowlist now accept the qualified spelling, and the qualified STORE form is pinned as accepted so the widening cannot turn a legitimate construction into a false positive.
Review summaryReviewed this against CONTRIBUTING.md's C# conventions and the Lite/Darling parity requirement. This PR is purely mechanical — it adds
Lite/Darling parity: no drift. This is an Npgsql/ Correctness/security/perf: no issues found. Every touched command is a store-bound (not monitored-target) command, so a store-derived constant is appropriate everywhere it's applied; no SQL text changes, no injection surface change. The 20s value only tightens an inherited undocumented 30s Npgsql default, and the composed-query path still defers to the operator's own clamp-derived maximum (600s) rather than a hardcoded value that could cut off a deliberately-widened panel. One very minor nit, not blocking: in |
… clamp ceiling The composed-query runner took McpCommandDeadlines.ComposedQuerySeconds, bound to StoreConfigProvider.MaxComposeStatementTimeoutSeconds (600) -- the operator's clamped MAXIMUM rather than the value they configured. That never overrode a deliberate setting on a managed store, where the role GUC fires first, but on a bring-your-own store there is no role GUC, so 600 s was the real bound. ResolveComposedQuerySecondsAsync now reads config.config_service's compose_statement_timeout_seconds and clamps it through the store's own clamp. READ rather than plumbed, because the store is authoritative and #2918 made the knob hot-swappable: a value captured at host start is wrong after the first change, and the value a host could plumb is the file-loaded seed, not the live one. Reading per run has no cache to go stale and keeps the two callers identical -- MapAll has the host config in scope, the MCP tool has only the data source, so plumbing would have given the two surfaces different answers. The near miss is three lines away: ComposeStoreAvailability.GetRollupsAsync is deliberately cached per data source, and caching this the same way compiles and passes every band assertion. Resolved once per RUN, so a panel with annotation overlays pays one read rather than one per source, and every query in a run shares one ceiling. Fallback is 15, the shipped default in three other places; failing open to something larger would grant more time exactly when the store is least able to answer for itself. The old fact asserted equality with the clamp maximum and would now state the opposite of what it checks, so it is replaced: a fallback band, a structural per-run guard, and a live-Postgres class that CHANGES the column and requires the next resolve to see it -- the only thing that can tell a per-run read from a cached one.
|
Thanks — the independent census reproduction is the part I most wanted checked, and the 1:1 coverage count matches mine. On the naming nit: acted, and the count was low. There are six write sites among the 124, not three — the three you found in
So 119 reads + 6 writes = 124. Kept as one constant, and documented rather than renamed, for two reasons. They share the regime exactly — the same least-privilege pool, the same absent Two things landed after your pass, both worth a re-read:
Not merging, and #2874 stays open — groups B–E remain. |
The structural guard located RunComposedPanelAsync's declaration by matching the open paren followed by a newline, with a CRLF arm and an LF fallback. The declaration wraps its parameter list, so that anchor was line-ending-sensitive in exactly the way a source-scanning pin must not be -- blobs here are LF and working copies are CRLF. Anchored on the return type instead, which is on the same line as the name and carries no newline.
|
Reviewed the diff in full (all 34 modified/added Correctness — Every Fallback/clamp logic — Lite/Darling parity — No Lite changes here, and that's correctly justified rather than an oversight: Security — No new SQL string interpolation introduced; the composed-query runner already parameterizes via Minor/non-blocking nit: Overall this is an unusually well-verified mechanical change — didn't find a correctness, parity, or security issue to flag. |
LivePostgresCollectionHygieneTests reads only the 25 lines above a class
declaration when looking for [Collection("live-postgres")] or an "#1776
own-store" exemption. The marker was 26 lines up -- above the doc comment rather
than below it -- so it was invisible to the very rule it answers, and CI reported
the class as an undocumented shared-store consumer. The three existing own-store
classes all place theirs immediately above the declaration; this now does too,
and says why in the marker itself.
Reproduced locally by staging the throwaway xunit host inside the repo tree: the
rule resolves the source directory from AppContext.BaseDirectory, so a host built
in a scratch directory fails it for an unrelated reason -- which is why this
slipped through the first pass. Proven red by moving the marker back to 28 lines
up, which names the class as an offender again.
|
Reviewed. This is a well-scoped, Darling-only change (no T-SQL, no Lite touch — and Lite is explicitly and correctly excluded, since Verified in the working tree:
One soft note, not a blocker: No missing-index DMV concerns, no secrets/credential handling changes, no SQL Server T-SQL touched. |
…eal reach Two review notes, both acted on. The store-receiver allowlist had no boundary before postgres/_dataSource, so `targetPostgres.CreateCommand(` matched `postgres\.CreateCommand` as a substring. That is a false ACCEPT in the one guard whose job is to stop a monitored-target command from taking a store deadline -- the mirror case, an unrecognised receiver like `oldConnection`, already failed safe by asking for a decision. Boundaried with a lookbehind, with notpostgres / targetPostgres / targetConnection pinned as rejected and `this._dataSource` as accepted so the fix cannot over-reject. ResolveComposedQuerySecondsAsync catches any non-cancellation failure, not only the missing row and absent table its comment listed. That is intended: the composed query runs on the same pool moments later and surfaces the same underlying failure with its own error, so a deadline lookup should not be the thing that fails the panel. Now said, rather than left to be inferred from two examples.
…2951, #2952) Two shapes of one defect, both surfaced while deriving SerialLoopSeconds. The reload beacon assigned _lastConfigVersion on detecting a new version and reloaded afterwards, so a reload whose read failed had already recorded the version as applied: the operator's change stayed in the store, absent from the process, and was never retried because the next tick compared equal. The watermark now advances only inside if (await ReloadFromStoreAsync(...)), with LoadViewAsync as the success line since it is the atomic gate. The retry that creates is rate-limited in StoreConfigProvider — first failure warns, the streak goes to Debug, recovery reports once — so a persistently unreachable store does not emit 5,760 warnings a day. Not a retry policy; #2936 owns that for the migrate path, which has no loop of its own. ReadComposeStatementTimeoutAsync caught everything with a bare unlogged catch and returned 15, which the caller wrote onto the login roles. Since #2931 the client half of that backstop reads the same column live, so the silent default desynced them. An unreadable value now omits the two ALTER ROLE statements and reports a negative sentinel, so the roles keep their last known-good horizon and #2918's gate re-asserts. A second unfiltered catch in the same file, found by the pin's own file-wide sweep, takes the filter too. Neither has a Lite twin; that is now pinned rather than remembered.
1 participant
Part of #2874 — group A of five, the MCP and web read surface. This does not close the issue; four groups remain.
What was wrong
All 125 command sites on this surface set no
CommandTimeoutand inherited Npgsql's undocumented 30 s default: 118 underMcp/(114 of them the.CreateCommand(shape #2874's census could not see, andMcp/DarlingDataReader.csis 26 on its own), the 6CustomViewStorestatements both surfaces persist through, and the 1 composed-query runner inDarlingWebEndpoints.Census, re-derived — with positive controls first
Not taken from the issue. The landed pins' machinery lifted verbatim —
s_commandCtor,s_setsTimeout, the method-group regex, andCSharpSourceWalker.StatementSpanFrom(…, statements: 2)— into anet10.0harness. The controls are what establish it counts the way the landed pins count:.StorageTopDirectoryOnly.ViewerAllDirectories.AnalysisTopDirectoryOnlyAll three reproduce exactly.
.Servicethen comes out at 261 sites / 201 untimed (130 / 131 by shape) — which agrees with the census comment on the issue to the digit — of which these 125 are group A.Mcp/alone: 118 sites, all 118 untimed, 4 / 114 by shape. After this changeMcp/is 118 / 0 and the project is 201 → 76 untimed, the remainder belonging to groups B–E.The derivation
The enclosing constraint is a kind this sweep had not met. Not a clock (#2871's
CancelAfter) and not a connection permit (the viewer group'sMaxPoolSize = 10), but a server-sidestatement_timeouton the login role —ALTER ROLE mcp/viewer SET statement_timeout, written by startup provisioning fromconfig_service.compose_statement_timeout_seconds, shipped default 15 s, clamped 5–600. It is a role setting, so it bounds every statement those identities run and not merely the composed queries it is named for;DarlingTrendReaderandQueryStoreTrendRoutingboth already reason about it by name.Why a client-side deadline does not simply defer to the GUC
Three reasons, and they land differently per deployment shape rather than being one argument restated.
Darling/tools/provision-roles.sqlcreatesadminandviewerand deliberately nomcprole, and setsstatement_timeoutonvieweronly — hardcoded at 15 s, not read from the store, so a BYO operator's change to the column never reaches a role until they re-run the script by hand. The MCP host in that mode connects with the operator's ownpostgres.connectionString, typically the owner oradminidentity, which carries nostatement_timeout. So on a BYO store this surface has no server-side bound, and Npgsql's undocumented default is currently the only thing bounding it. A BYO operator also has no knob here at all, which is why these values are deliberately not aggressive.compose_statement_timeout_secondsso one heavy custom view can finish raises it for all 124 shipped reads. The client-side deadline is what keeps the ordinary read bounded while the backstop is deliberately loosened for the extraordinary one.SETtakes effect on that role's next session.DarlingConfigsays so outright — "this is not a kill switch" — andDarlingManagedRoleslogs that an already-connected client keeps the old ceiling until it reconnects. The MCP host builds its pool once at host start. ACommandTimeoutis set per command at construction, so it binds the very next command on an already-open pooled connection, which is exactly what the GUC cannot do.The asymmetry that makes it necessary rather than tidy
None of the 136
[McpServerTool]methods takes aCancellationToken. Every reader declaresCancellationToken cancellationToken = default; every tool calls it without one; the single tools file that names a token passesCancellationToken.Noneexplicitly (DarlingMcpCustomViewToolsintoDarlingWebEndpoints.RunComposedPanelAsync). So an agent that abandons a call leaves the query running and the pooled backend held — nothing on this path propagates abandonment. The web half is the mirror image and threadsHttpContext.RequestAbortedthroughout, so a disconnecting browser does cancel; the deadline is a backstop there and the only bound on the MCP side.The permit, at its real strength rather than borrowed
The MCP host builds its own data source, separate from the web host's and from the worker's collection pool — three independent
NpgsqlDataSource.Createcalls on connection strings that differ byUsername(mcp,viewer, and the bootstrap owner). Verified in source, not assumed, and it cuts the argument down rather than up: a stalled MCP read cannot starve fleet-wide collection through the pool. The widest fan-out here is aTask.WhenAllover two reads againstMaxPoolSize = 24— nothing like the viewer's ten-against-ten, and there is noCancelAfter,SemaphoreSlim,WaitAsyncorCreateLinkedTokenSourceanywhere on the path. What the bound still buys is the reason it exists at all: every pooled connection is a livepostgres.exeprocess, each spawn re-reserves the shared memory region, and this store logged 8could not reserve shared memory regionretries on 2026-09-03 — so a longer hold means more spawns, not just a busier pool.Two regimes, not one
McpCommandDeadlines.ReadSeconds = 20for the 124 shipped-shape reads.Floor — the managed store's own ceiling, 15 s, and it fires on this surface in production today. On 2026-09-04
get_query_store_duration_trendover a 7-day window returned57014: canceling statement due to statement timeoutagainst the live 42-server store, with #2736's corrected-rollup routing already engaged, while the same read at its shipped 24 h default returned its full series. Sitting above 15 s is what keeps the server-side timeout the one that fires on a managed store, because57014names its cause where Npgsql's own deadline renders asException while reading from streamand gets read as a network fault — the misdiagnosis #2826 exists to prevent. That floor dominates the measured one by more than an order of magnitude; this family's verified reads are sub-second.Ceiling — the 30 s it replaces, which the viewer group's standard requires it to sit meaningfully under rather than at, plus a relational bound against
StorageCommandDeadlines.McpReadSeconds: theDarlingPg*Readerfamily serves these same 136 tools from.Storageon the same pool, so one tool call must never get a looser ceiling depending on which project happens to hold the reader.The composed-query runner could not take that constant, and that is the split. It is the one site running operator-authored SQL, and
compose_statement_timeout_secondsis the operator's declaration of how long that class may run — a fixed 20 s would cut off the panel the knob was raised to 120 s for, a regression dressed as a fix.So it takes no constant at all.
McpCommandDeadlines.ResolveComposedQuerySecondsAsyncreadsconfig.config_service.compose_statement_timeout_secondsand clamps it through the store's ownClampComposeStatementTimeoutSeconds, so the client-side deadline is the operator's actual setting rather than the ceiling they could theoretically have chosen.Read rather than plumbed, and that is the load-bearing choice. The store is authoritative for this knob and #2918 made it hot-swappable — a control-plane reload re-asserts it onto the roles with no restart. So a value captured at host start is simply wrong after the first change, and the value a host could plumb is the file-loaded
DarlingConfig.ComposeStatementTimeoutSeconds, which is only the seed: the store wins afterwards. Reading per panel run has no cache to go stale, and it is also what keeps the two callers identical —MapAllhas the host's config in scope whileMcp/DarlingMcpCustomViewTools.cs:252has only the data source, so plumbing would have handed one surface the file value and the other a store read, which is exactly the divergence the single shared runner exists to prevent.The near miss is three lines away in the same method.
ComposeStoreAvailability.GetRollupsAsyncis deliberately cached per data source, because which rollups a store has changes only when someone migrates it. Caching the deadline the same way compiles, passes every band assertion, and pins it to whatever the knob said the first time a panel ran. Resolved once per run — so a panel with annotation overlays pays one read rather than one per source, and every query in one run shares one ceiling, matching the rolestatement_timeoutthey all run under.ComposedQueryFallbackSecondsis 15, the same shipped defaultDarlingConfig,ComposeSpecand the clamp's own non-positive arm all use, so a store that cannot answer behaves like a store nobody has tuned. Failing open to something larger would grant more time precisely when the store is least able to answer for itself — the wrong direction for the only bound a BYO store gets.Store vs monitored target — the claim no landed pin needed
.Storageand.Viewerare Npgsql-store-only projects, so "every command sets a deadline" was the whole invariant there..Servicealso builds commands against monitored targets, where a store-derived constant would be wrong in a way nothing else would catch: the value would look deliberate while bounding the wrong hop.Every one of the 125 sites in this scope was classified, and all 125 are store commands — 115
postgres.CreateCommand(, 6_dataSource.CreateCommand(, and 4new NpgsqlCommand(sql, connection)whose connection comes frompostgres.OpenConnectionAsync. There are no monitored-target commands here:Mcp/contains noSqlConnection, noSqlCommand, noTargetProviders.Forand nonew NpgsqlConnection(. A receiver allowlist writes that classification down, with the two real target shapes pinned as negatives —ITargetProvider.CreateCommand(plan, connection, commandTimeoutSeconds), where the deadline is threaded and is stronger than a constant, and the transaction-scoped HypoPG form — so a future target command here fails asking for a decision instead of inheriting a store bound.The pin
McpReadCommandTimeoutTests, scoped toMcp/recursively plusCustomViewStore.csandDarlingWebEndpoints.csby name — deliberately not the project..Servicecarries roughly fifteen budget regimes over ~200 untimed sites across five groups; a project-wide glob would claim regimes this pin has not derived and would collide with the sibling groups' pins.Mcp/is globbed because the whole directory is this surface; the two top-level files are named because the project root is shared.It covers all four known shapes plus one this sweep had not recorded, and asserts zero for the two that should be absent — the bare method group, and
BeginBinaryImportAsync, whoseNpgsqlBinaryImporter.Timeoutis initialised from the connection'sCommandTimeoutand which no landed pin's regex can see. The project's four live importers are the collection sweep's; asserting their absence here makes "this scope has none" a checked fact rather than an assumption. Both negatives carry positive controls through the identical form, so neither can pass by matching nothing.Every scan reads the stripped text, including the construction scan — which the landed pins run over raw source, where a prose mention of
.CreateCommand(becomes a phantom untimed site at a line number no edit could fix. Not hypothetical in kind:Mcp/DarlingMcpStoreMetricsTools.cscontains the sentence "the two readers above go through postgres.CreateCommand and leave nothing open", and it misses both regexes only because what follows the name is a space and a word rather than a delimiter. The divergence changes no count (125 either way) and closes a false-positive direction.A fifth construction shape, found because a red-first variant came back green
The planted monitored-target offender used
new Npgsql.NpgsqlCommand(and the pin did not see it: no regex in this sweep matches a qualified type name. The form occurs exactly once in the repo —DarlingWorker.cs'spg_stat_statementstext fetch — and that site is a monitored-target command already carrying its own 60 s, so the blind spot has no defect behind it and the untimed census is unchanged. The site census for.Serviceis 262, not 261. Both the construction scan and the receiver allowlist now accept the qualified spelling, with the qualified store form pinned as accepted so the widening cannot create a false positive.Proven red fifteen ways, one mutation at a time
Each verified applied by reading the file back rather than by a diffstat, and each rebuilt — a mutation that does not compile proves nothing — and each restored from an in-memory byte copy rather than
git checkout.…SetsAnExplicitDeadline—1 MCP/web read command(s) inherit Npgsql's 30s default…: DarlingDataReader.cs:225usingbody…SetsAnExplicitDeadlineCustomViewStoredeadline…SetsAnExplicitDeadlineMcp/subdirectory…SetsAnExplicitDeadline(recursion control)Mcp/, unqualified…IsBuiltAgainstTheStore_NotAMonitoredTarget—1 command(s) … have an unrecognised receiver…: DarlingTargetProbeReader.cs:7…IsBuiltAgainstTheStore_NotAMonitoredTarget.CreateCommandmethod-group hand-off plantedNoCommandInScope_IsCreatedByABareMethodGroupHandoffBeginBinaryImportAsyncplantedNoCommandInScope_UsesAnUntimedBinaryCopyImporterReadSeconds→ 15ReadSeconds→ 5ReadSeconds→ 30StorageCommandDeadlines.McpReadSeconds→ 15ComposedQuerySecondsdecoupled from the clamp maximumTheComposedQueryDeadline_TracksTheOperatorsDeclaredMaximum— that constant and that fact are both gone as of the follow-on below; the mutation is retired and its successors are S3–S6TheReceiverAllowlist_…(positive control on the allowlist itself)TheReceiverAllowlist_…onnotpostgres.CreateCommand(— without the boundary,targetPostgres.CreateCommand(matchespostgres\.CreateCommandas a substring, a false accept in the one guard meant to stop a target command taking a store deadlineCustomViewStore.csThe follow-on: eight more, and two of them are why the live test exists
staticTheResolver_ReadsTheConfiguredValue_AndSeesItChangeGetRollupsAsyncshapeTheResolver_ClampsAnOutOfRangeRow—Expected: 600 / Actual: 6000TheComposedQueryFallback_IsAValueAnOperatorCouldHaveConfiguredCommandTimeout = 0means no limit)Assert.InRange() … Range: (5 - 600) / Actual: 0TheComposedQueryRunner_ResolvesItsDeadlinePerRunEveryMcpAndWebReadCommand_SetsAnExplicitDeadline—McpCommandDeadlines.cs:186TheComposedQueryRunner_ResolvesItsDeadlinePerRun— a MOVE, soResolveComposedQuerySecondsAsync(postgresstays at 1 → 1 and only the structural ordering control can see itS1 and S2 first came back GREEN, and that was the mutation's fault rather than the pin's. The planted cache was read but never populated, so both compiled and changed no behaviour. A mutation that compiles but does not move the behaviour proves exactly as little as one that does not compile — "assert the mutation applied" has to mean the behaviour moved, not that the bytes did. Once the cache was actually written, both go red on the change-seeing assertion.
TheResolver_FallsBackOnAMigratedButUnseededStoreexists because running the live test found it:MigrateAsynccreatesconfig_service, but the row comes fromStoreConfigProvider.SeedIfEmptyAsync, which is a separate act. So "brand-new store, first panel run" reads no rows at all, and the resolver has to land on its fallback rather than hand a zero toCommandTimeout. There is a companion for a store with noconfig_servicetable at all (a pre-V78 store), which raises42P01.The live class mints its own scratch database (
ScratchPostgres, with the#1776 own-storemarker) rather than joining[Collection("live-postgres")]: every test in it mutates the knob and one asserts the behaviour of an unseeded store, so it cannot share the collection's long-lived database with sixty other classes.The three landed #2874 pins still pass and still assert what they claim.
StorageCommandTimeoutTests(6),AnalysisPassCommandTimeoutTests(13) andAlertPassCommandTimeoutTests(5) all execute green against this tree, scopes unchanged — none of them globs.Service, and the alert-pass pin names six top-level files, none of which this change touches. The viewer pin cannot be compiled on macOS (its project isnet10.0-windowsWPF), but its scope directory is untouched by this diff and the identical scan reproduces.Viewerat 193 / 0, which is what it asserts.What this does NOT do
compose_statement_timeout_secondswas not read back — the 15 s used throughout is the shipped default and the clamp, verified in source rather than on the box. The production evidence is the57014above plus successful live executions ofget_fleet_overview(the 8-wayDarlingFleetReaderfan-out over 42 servers), the server-registry read, and the 24 h trend.PgMigrations.MigrateAsync. It was used to prove the resolver's behaviour — that it sees a changed value, clamps an out-of-range row, and falls back on both a migrated-but-unseeded store and a store with noconfig_servicetable — not to measure any read's duration. It holds no collected data, so nothing here is a performance measurement.Darling.Testsisnet10.0-windowsand cannot run on macOS. The 68 tests reported here ran in a throwawaynet10.0xunit host compiling the real source files by absolute path withAssemblyName=Darling.Tests— 68/68 green withDARLING_TEST_PGpointed at that store, 61/68 with 7 skipped without it. CI is the arbiter. Expected delta: +47 —McpReadCommandTimeoutTests40 plusMcpComposedQueryDeadlineLivePostgresTests7, counted per class rather than derived.LivePostgresCollectionHygieneTests, which resolves its scan directory fromAppContext.BaseDirectory: a host built in a scratch directory fails that rule for an unrelated reason and tells you nothing about the class under test. That rule reads only the 25 lines above a class declaration when looking for[Collection("live-postgres")]or an#1776 own-storeexemption, so the marker sits immediately above the declaration rather than above the doc comment — the placement the three existing own-store classes all use, and the marker says so.