Part of #2874

[erikdarlingdata]

A retention command was inheriting the 30 s default, and the pins that exist to say so could not see it

DarlingRetention.PurgeOneAsync lifts a TimescaleDB decompression rail before a batched purge:

using (var lift = new NpgsqlCommand(
    "SET timescaledb.max_tuples_decompressed_per_dml_transaction = 0", connection))
{
    await lift.ExecuteNonQueryAsync(cancellationToken);
}

using var command = new NpgsqlCommand(deleteSql, connection) { CommandTimeout = DeleteTimeoutSeconds };

The lift command set no deadline, so it ran on Npgsql's undocumented 30 s default — the defect class #2874 exists to eliminate. Four of that file's five constructions already carried DeleteTimeoutSeconds; this one was missed, and the scan reported it clean because the two-statement window reached past the block's closing brace and found the initializer on the delete behind it. It now carries DeleteTimeoutSeconds.

It is the only such site in the repository's command constructions, and it sits outside every #2874 pin's scan scope — so no pin's published figure was false, but the class was live in shipped code rather than theoretical.

Three independent blind spots let it hide

All three are the same mistake: the scan answering about something other than the construction it is pointing at.

1. The span ran out of the scope it started in (false negative)

CSharpSourceWalker.StatementSpanFrom let its bracket depth go negative and keep counting semicolons. A construction in a using (...) header whose block body holds ONE statement spends the two-statement window on that statement and on the statement AFTER the block, so the FOLLOWING command's initializer satisfies the scan. The same leak runs out through the closing brace of any block a construction is the last statement of.

Reproduced against #2935's branch, one mutation at a time, each restored byte-identically:

• deadline removed from a using (...) block followed by a timed construction — Total: 6, Failed: 0, silent
• deadline removed from the trailing using var, nothing timed after it — 1 storage command(s) inherit Npgsql's 30s default ... PgMigrations.cs:4370, fires

The pin was directional: it saw an untimed site only when no timed construction followed within two statements. TheScanner_JudgesTheSiteItself_NotItsNeighbours, written to prevent exactly this, had no case with a one-statement body.

The span now ends with the SCOPE it started in. A closing BRACE that was open beforehand ends it; a parenthesis or bracket does not, because it delimits an expression and an expression that opened earlier does not change which block the construction lives in.

It still follows a statement header into the block — or the braceless statement — that header governs, and that half is load-bearing. Fourteen sites across the scanned projects set the deadline as that block's FIRST statement. Stopping at the header, the smaller change, reports every one of them; confirmed by mutation, which fails four pins on real code.

Judging only the construction and its initializer, the other candidate, is worse: all 193 viewer sites and 51 of .Storage's 124 set the deadline in the following statement, because a CreateCommand result cannot take an initializer. That is what the two-statement allowance exists for.

2. The neighbour no scope bound can exclude (false negative)

Two layouts survive the scope fix, because the sibling is in the same scope: an untimed construction directly ahead of a timed one, and an untimed using (...) header whose block OPENS with a timed sibling.

Both are closed by asking the question in two halves. A new ConstructionSpanFrom returns the site and its own object initializer, and the bare CommandTimeout = pattern is read from THAT and nothing wider. Only a MEMBER assignment — command.CommandTimeout = ..., the one placement a CreateCommand result allows — is read from the surrounding statement span. A value in an initializer belongs to the construction it is attached to; a value assigned through a member access belongs to whatever it names.

Cutting the statement span at the next construction instead is the tempting one-liner, and it is wrong. Two constructions can share one deadline — the conditional in ViewerDataService.FinOps.Locking.cs does — and cutting there reports the first arm as an offender. That shape was already a pinned case in the viewer pin, so the one-liner would have turned an existing green case red. It is now a case in all five pins.

3. Both sides of the question read RAW source (false positive and false negative)

The scans matched raw text when enumerating constructions, so a construction named in a comment or a string was reportable as an untimed site at a line where no edit could ever fix it. Demonstrated on real code: with a construction named in a comment in StorageVersion.cs, the landed pin reports StorageVersion.cs:19 — a comment — and the fixed pin reports nothing.

The mirror of it is worse, and the two-part rule above does not close it on its own: the deadline check also read raw text, so a deadline merely SPELLED in a comment satisfied a site that had none. Reproduced against this branch before fixing it — a block comment /* the deadline used to be command.CommandTimeout = 10 here */, a line comment, and a string literal all read as timed; all three now read untimed, and both legitimate controls (a real assignment with a comment in the gap, and an initializer) still read timed.

Both halves now route through one stripped code per file. The doc remarks that used to justify cutting spans from the ORIGINAL text — in the walker and in every pin — now say what the code does instead; those remarks were the thing that would otherwise let this be re-introduced, because a walk made literal-aware and then handed raw text throws the awareness away at the last step.

This codebase quotes code in its prose constantly, so both directions were live. Not one phantom sits inside any pin's scan scope, which is the figure that means something — a repo-wide count would go stale on the next commit, and this change itself adds more of them to the test suite.

The defects also interact: a phantom planted next to a timed statement is itself hidden by the over-wide window.

All five scans, not just the two that were reported

.Analysis and PgFactCollector judged a site by joining the next three RAW source lines, which can neither tell a construction from the same words in a comment nor tell this site's deadline from the next site's. The alert pass, consolidated to a single scan helper by #2934, judged a single raw span. All now use the same two-part rule over stripped source.

The clearest evidence is one run, one file, two pins: with an untimed construction planted immediately ahead of a timed one in PgFactCollector.Activity.cs, the landed three-line pin says clean while the migrated scan reports PgFactCollector.Activity.cs:61. Both pins read that exact file.

All five also take the alert pass's fully-qualified construction pattern — new\s+(?:[A-Za-z_][A-Za-z0-9_]*\s*\.\s*)*NpgsqlCommand\s*\( — which is strictly more general than the Npgsql.-only form and sees a construction qualified by any namespace. There is one qualified occurrence in the solution, in DarlingWorker.TryRefreshPgStatementTextAsync, a member the merged collection-sweep census enlists — so that census counts 13 where 14 constructions exist. It is already timed, and whether a monitored-target read belongs in that budget is that group's call, so it is flagged rather than changed here.

Every figure, re-measured after the fix

Counts are as measured at this head; dev adds command sites steadily, so the load-bearing claim is the last two columns rather than the first. Every figure below is the SHIPPED judgement, not a stand-in for it — the measuring tool compiles CommandDeadlineScanner itself.

scope	sites	timed	newly reported	phantoms in scope
.Storage	127	127	0	0
.Viewer	193	193	0	0
.Analysis	73	73	0	0
PgFactCollector.*.cs	32	32	0	0
alert pass (6 files)	45	45	0	0
MCP read surface	126	126	0	0
whole repository	1,862	—	1 (fixed here)	none in any scope

The MCP read surface is the densest population of the affected shape — 112 of its 119 Mcp/ sites place the deadline in a following statement, structurally, because .CreateCommand( returns a command. It measures clean under four independent rules: the landed span, the scope-bounded span, the shipped two-part rule, and a deliberately stricter control that additionally requires the assignment to name the identifier the construction was bound to. That control resolves 119 of 119 names, so the shipped rule's one residual — an assignment on a different variable inside the window — is empirically empty there.

Two pins in the family are NOT fixed by this

Stated plainly, because the headline would otherwise overreach. Five pins route through the shared judgement: .Storage, .Viewer, .Analysis, PgFactCollector and the alert pass. Two do not, and both still reproduce blind spots this closes elsewhere:

• CollectionSweepCommandTimeoutTests still cuts its span from RAW member source and matches an unqualified value regex over it, so a deadline spelled in a comment satisfies a site, and a sibling's assignment satisfies one too. Untouched here on purpose — it is another group's just-landed pin and its census constant is a scope judgement I should not make unilaterally.
• StoreSelfMetricsTimeoutTests is a whole-file count-equality pin over one file, not a per-site scan at all. Relocating a deadline from one command to another leaves its counts invariant, so it cannot see a move. All five of that file's sites currently carry the initializer, so it reports truthfully today.

Adopting the shared judgement in either is a one-line change now that it exists.

What this does NOT establish

The enumeration is regex-based, and that is a design limit, not an oversight. These pins find a construction by matching its written form, so a construction whose TYPE NAME is not at the site is invisible to them however the pattern is widened. Target-typed new is exactly that shape: in return new(...) or => new(...) the type lives in the member's return type, and in = new(...) it lives on the declaration. No pattern over the construction site can see it.

So the claim this PR supports is "every construction these patterns can see is judged correctly", not "every construction is judged". The gap is bounded rather than open, though — measured, not assumed:

• 2,755 target-typed new(...) sites in the repository, over stripped source so prose cannot contribute
• 327 of them sit in a file that names a command type at all
• 22 have a command type within 3,000 characters, a deliberately generous stand-in for "in the same member"
• all 22 read by hand: none constructs a command. Eleven are in test files no pin scans; the other eleven are records, singletons and collections — CommandPlan, AlertHistoryRecord, PlanForceActionRecord, MonitoredServerRow, PlanDimRecompression.Result, the two target-provider singletons, a ConcurrentDictionary and a List<string>

So the shape is a footnote today, not a hole — but it is a footnote a widened regex cannot keep true, and the next command written as return new(...) would be invisible to every pin. Closing it properly needs the target type resolved, which is a different tool from this one.

Two other things worth stating plainly. The .CreateCommand bare method group is matched by the pins that assert it is ABSENT and not by the five that judge deadlines, because a method group has no argument list for either pattern to anchor on. And the two-statement allowance is a syntactic claim — the construction's own statement plus the one after it — not a tuned window; it is now bounded by scope on one side and by the construction span on the other, so its size is no longer what decides a verdict.

4. A match with no argument list ran away (false negative)

Caught in review on this branch, and it was mine: two pins match a bare .CreateCommand METHOD GROUP on purpose, and ConstructionSpanFrom assumed its start was followed by the construction's own (. A method group has none, so the scan walked out of the statement, found the NEXT construction's parentheses, and returned that span with its initializer — a hand-off with no deadline of its own reading as timed.

Reproduced before fixing: builder.Register(factory.CreateCommand, options); followed by a timed construction read as timed, as did the Func<NpgsqlCommand> f = connection.CreateCommand; form. The walk to the opening parenthesis may now only cross what a construction's head is made of — the new keyword, a qualified name, generic arguments — so a match with nothing attached spans the reference alone and carries no deadline. Both hand-off forms now read untimed; both legitimate called forms still read timed.

5. A header and an argument list are not the same closing parenthesis (false positive)

Also caught in review, also mine, and introduced by the fix for the braceless case above. Once the span may end at a header's embedded statement, it has to know a header's ) from an argument list's. It did not, so Wrap(connection.CreateCommand()) — a construction nested in an ordinary call — had its span cut off before the assignment on the next line, reporting a correctly-timed command.

The two are told apart by what follows the parenthesis: a statement header is followed by the statement it governs, an argument list by the rest of its own expression. Reproduced before fixing (both the plain and the .Inner member-access form read as untimed), and both directions are now pinned — conflating them fails the new argument-list witness, and never recognising a header fails the braceless and scope witnesses plus the landed witnesses in McpReadCommandTimeoutTests and CollectionSweepCommandTimeoutTests, so the header path is load-bearing for two other groups' pins as well.

Two of the five defects here were mine, both found by review rather than by my own mutations. The pattern in both: a bound added for one shape changed the verdict for a shape I had already reasoned about and stopped re-checking. The fifteen-case matrix in the walker's tests now covers all of them together rather than one at a time.

6. The assignment half never checked WHICH command it named (false negative)

The last one, and the sharpest: \.CommandTimeout\s*= over the statement span accepts a SIBLING's assignment. An untimed using (...) header whose block opens with using var sibling = conn.CreateCommand(); and sibling.CommandTimeout = 10; spends both counted statements on the sibling, and the outer command reads as timed.

Every sibling fixture in this family — mine included — exercised the sibling through an object INITIALIZER, which has no leading dot and so never reached that regex. The assignment spelling it missed is the dominant one: 112 of the MCP surface's 119 sites. So the hole sat precisely where the fixtures did not look.

The assignment must now name the identifier the construction was bound to, taken as the identifier before the FIRST = of the construction's statement — the binder in a declaration, a header declaration and a field assignment alike, and absent exactly where there is no binder (return new ..., Wrap(conn.CreateCommand())), where the initializer is then the only place a deadline could be. First rather than last so a conditional whose arms are each a construction still resolves to the one variable they share.

Dropping the qualification fails the new witness in all five pins. And the shipped judgement now measures zero offenders in every scope, the viewer's 193 included — the conditional in ViewerDataService.FinOps.Locking.cs resolves correctly, which an earlier and cruder name walk of my own did not.

One judgement, shared

Review also pointed out that the two regexes and the judgement they feed were copy-pasted byte-for-byte into all five pins — the same duplication CSharpSourceWalker was extracted to end in #2913 (five copies of the source walk, already drifted, only one of them hardened) and BraceBalanced in #2923/#2927. Fair, and paid down here rather than left to drift: CommandDeadlineScanner owns both regexes and the two-part judgement, and all five pins call it. Neutering one regex in that one file now fails 17 tests across all five pins, which is the evidence that they genuinely share it.

CSharpSourceWalker stays generic — it walks C# source and knows nothing about deadlines. The domain judgement lives in its own type, which is the pattern IlCallSiteScanner already set for the IL walk.

Verification

Every guard proved red first, one mutation at a time, each asserted applied and compiling, each moving the behaviour under test, each restored by copy with a dirty-file count logged. Each mutation killed a different set:

• removing the plain-block stop, the entered-block stop, the header follow-through, and the braceless-header stop each fail a different walker witness; the header-only variant fails four pins on real code
• ConstructionSpanFrom returning the whole statement span fails every neighbour witness across all five pins; dropping the initializer fails the verbatim-SQL and comment-gap controls
• neutering either half of the two-part judgement fails a different set of cases, so both halves are independently pinned in every pin
• reading raw text instead of stripped fails the prose cases on the construction side and the comment-deadline cases on the value side; narrowing the pattern fails the qualified-shape case

End to end on real code, same planted mutation both sides: dev reports Failed: 0, the fix reports PgMigrations.cs:4144.

DarlingRetentionTests slices DarlingRetention.cs by string anchor, once by a fixed character window and once brace-matched from inside the method this change edits. Both anchors still resolve, both still hold, and a planted-phrase control confirms each can still fail.

Darling.Tests is net10.0-windows and cannot run on macOS, so CI is the arbiter. Locally, the real test files compiled by absolute path into a net10.0 xunit host staged inside the repo tree run 254 tests against 195 for the same harness on dev, with the same single pre-existing failure on both sides (LivePostgresCollectionHygieneTests, which fails identically without this change).

[claude]

Count mismatch: this entry says the corrected scan re-measures 1,848 command constructions repository-wide, but the PR description's summary table says 1,849 (| whole repository | 1,849 | — | 1 (fixed here) | 81, none in scope |). The same "1,848" figure is also baked into a test comment in CSharpSourceWalkerTests.cs ("the only site in 1,848 across the repository..."). Given this PR's whole premise is that previously-published figures need to be exact and verified, it's worth reconciling which count is actually correct before merging — a one-off discrepancy here undercuts the "re-measured and verified" claim.

[claude]

Nit: missing blank line between FirstConstruction's closing brace and the next member's XML doc comment (AStatementSpanCannotLeakPastACharLiteralQuote's preceding doc). Every other member in this file is separated by a blank line — this one pair reads as glued together.

[claude]

Review summary

Scope: this PR only touches Darling/Darling.Tests/* (the shared CSharpSourceWalker scanning helper and its consumers) and one production fix in DarlingRetention.cs. No Lite files, shared libraries, or migrations are touched, so the "Two-Store Parity" rules in CONTRIBUTING.md don't apply here — this is Darling-only tooling plus a Darling-only Npgsql timeout fix, and there's no Lite counterpart that needs a matching change.

Correctness — traced the new StatementSpanFrom/ConstructionSpanFrom logic by hand against the fixtures:

• The scope-bound fix (ending the span at a brace that was open before start) correctly closes both false-negative shapes described: a using (...) { } block with a single body statement, and a construction that's the last statement of a plain block.
• The header-follow-through (header/body/floor) correctly still reaches into the block a using (...) header governs, so the 14 sites that set the deadline as the block's first statement stay green.
• ConstructionSpanFrom correctly isolates a construction's own initializer from a neighbour's (traced through the "untimed directly followed by timed" and "untimed header whose block opens with a timed sibling" cases, plus the shared-conditional-deadline case in the ViewerDataService.FinOps.Locking.cs-style fixture) — splitting initializer-vs-member-assignment is the right fix and matches the stated reasoning.
• The DarlingRetention.cs fix itself (CommandTimeout = DeleteTimeoutSeconds on the SET timescaledb.max_tuples_decompressed_per_dml_transaction = 0 command) is consistent with the sibling commands in the same method (PurgeOneAsync already sets DeleteTimeoutSeconds on the delete command and DropChunksAsync does the same) — correct and minimal.

One residual gap worth knowing about (not a blocker): StatementSpanFrom's scope bound only stops at a {/} block. A using (...) header with a braceless single-statement body (e.g. using (var command = new NpgsqlCommand(...)) await command.ExecuteNonQueryAsync(...); with no { }) would still leak into the statement after the using scope ends, reproducing the same false-negative class this PR fixes for the braced form — because body never gets set when there's no {. I checked .Storage, .Viewer, and .Service for this shape and found none (every using (...) in this codebase is braced or chains into another using), so it isn't live today, just a gap in the generality of the fix relative to how it's described.

Left two inline nits: a count discrepancy between this PR's CHANGELOG entry (1,848) and the PR description's summary table (1,849), and a minor missing-blank-line style nit in the new test file.

No security, injection, or Lite/Darling parity concerns found.

[claude]

Doc-comment mix-up: this <summary> (407-412, "fourteen sites ... set its deadline as the block's first statement") describes AStatementSpanStillReachesADeadlineSetInsideTheBlockItsHeaderGoverns below (line 443), not AStatementSpanEndsWithABracelessHeadersOwnStatement (line 420), which already has its own correct summary right after (413-418). The result is two consecutive <summary> blocks stacked on one method — the first one orphaned (not attached to any member, likely a CS1587 warning) — while AStatementSpanStillReachesADeadlineSetInsideTheBlockItsHeaderGoverns ends up with no doc comment at all.

Given how much this PR (and this file in particular) leans on precise per-test doc comments to explain exactly what each fixture proves, this block should move down to sit directly above AStatementSpanStillReachesADeadlineSetInsideTheBlockItsHeaderGoverns.

[claude]

Reviewed. This PR is scoped entirely to the Darling test-scanner tooling (CSharpSourceWalker/the four command-timeout pins) plus the one real fix it exists to catch: DarlingRetention.PurgeOneAsync's lift command now carries CommandTimeout = DeleteTimeoutSeconds, consistent with the file's other four constructions. No T-SQL changed, and no Lite counterpart exists for this Npgsql-specific scan/fix (Lite is DuckDB-backed), so there's no parity drift here.

I traced the new StatementSpanFrom scope-bounding logic (brace-floor tracking, header follow-through, the negative-depth exit case) and ConstructionSpanFrom against the new fixtures, including some cases not in the test suite (chained/stacked using headers), and didn't find a case where it misjudges a site. It's solid.

One real finding, left as an inline comment: a doc-comment mix-up in CSharpSourceWalkerTests.cs — a <summary> describing the "fourteen sites that set the deadline as the header block's first statement" case ended up orphaned above the wrong test (two stacked <summary> blocks on AStatementSpanEndsWithABracelessHeadersOwnStatement), leaving AStatementSpanStillReachesADeadlineSetInsideTheBlockItsHeaderGoverns undocumented. No functional impact, just worth fixing given how much this file leans on precise per-test docs.

[claude]

Reviewed. This PR is Darling-only: a one-line production fix plus the test-infrastructure change that let it hide.

Production fix (DarlingRetention.cs:960-962) — the lift command (the TimescaleDB decompression-rail SET) now carries CommandTimeout = DeleteTimeoutSeconds, matching the other 4 command constructions in that file. Correct and minimal; a SET completing instantly makes the generous 300s budget harmless.

CSharpSourceWalker changes — traced StatementSpanFrom's new scope-bounded logic and the new ConstructionSpanFrom by hand against the fixtures in CSharpSourceWalkerTests.cs (header+braced block, header+braceless statement, plain block, neighbour-construction cases, initializer-with-interposed-comment). All traced correctly:

• The body/floor mechanism correctly stops the span at the closing brace of the scope the construction started in (bounding both the "one-statement using body" leak and the "last statement in a block" leak), while still following a statement header into the block/braceless-statement it governs.
• ConstructionSpanFrom correctly isolates a construction's own argument list + initializer, independent of the surrounding statement, which is what lets the neighbour-construction false positives get closed without breaking the legitimate shared-deadline case in ViewerDataService.FinOps.Locking.cs.
• The widened qualified-construction regex (new\s+(?:...\.)*NpgsqlCommand\s*\() can't false-positive on a longer identifier like NpgsqlCommandBuilder — verified there's no \( adjacency to match.

Can't actually run Darling.Tests here (net10.0-windows, Linux sandbox), so this is a static trace rather than an executed run — but the logic checks out and the PR description documents mutation testing per guard.

Parity — no Lite counterpart needed here. Lite's retention (Lite/Services/RetentionService.cs) runs against DuckDB, not Npgsql/TimescaleDB, so the decompression-rail lift and the untimed-command defect class don't apply to it. Confirmed no CommandTimeout/command-construction pattern there to check.

Security/perf — no user input involved (these are build/test-time static scanners over the repo's own source), no injection surface, no perf regression (test-only code + a no-op-cost timeout on production SET).

No findings to post inline.

[claude]

Reviewed. This is Darling-only (C#/Npgsql test tooling + a one-line production fix); no T-SQL and no Lite counterpart to check for parity — Lite never uses Npgsql, so there's no drift here.

Production fix (DarlingRetention.cs): adding CommandTimeout = DeleteTimeoutSeconds to the lift command is correct and matches the sibling delete command in the same method. Confirmed via grep that every other new NpgsqlCommand( construction in production Darling code is already timed or was already covered.

Scanner changes (CSharpSourceWalker.StatementSpanFrom/ConstructionSpanFrom): traced the depth/floor/body/header state machine by hand against all the new theory cases (nested using headers, braceless header statements, block-as-last-statement, shared-deadline conditional arms, stripped-text matching) — it matches the documented intent and the new tests all check out logically.

One theoretical (not live) blind spot worth noting for the backlog rather than blocking this PR: StatementSpanFrom sets header = true whenever any )/] opened before start closes during the walk — not just a genuine statement header like using (...). If a command were ever constructed inline as an argument to an enclosing call (e.g. Foo(new NpgsqlCommand(...)) or inside a lambda passed to a wrapper), the walk could misattribute an unrelated following { } block as "the block this header governs" and pick up an unrelated CommandTimeout assignment inside it, reporting the site clean when it isn't. I confirmed by grep that no current construction in Darling/ is written that way — every one is var x = .../using (var x = ...)/using var x = .../await using var x = ... — so this doesn't affect today's census, but it's the same failure shape (the scan answering about something other than the construction it's pointing at) this PR is otherwise closing, just for a shape that hasn't been written yet.

Also confirmed: the [#2874] changelog link reference exists, the entry is correctly placed under [Unreleased] / Fixed, and no version bump was needed (matches the PR's own note).

[claude]

SetsAnExplicitDeadline is called uniformly for every match of s_commandCtor (line 135), including the third alternative \.CreateCommand\s*[,);] — the bare method-group hand-off, which by design has no argument list directly attached (e.g. builder.Register(factory.CreateCommand, options);).

ConstructionSpanFrom (CSharpSourceWalker.cs:253) assumes its start index is immediately followed (modulo whitespace/dots/identifiers) by the construction's own (. For a bare method-group match there is no such paren, so its unbounded forward scan —

while (open < text.Length && !(code[open] && text[open] == '(')) open++;

— walks straight past the current statement into whatever code comes next, picks up the next unrelated construction's (...)/{...} (including its initializer), and hands that span to s_setsTimeout.IsMatch(...). If that neighboring construction happens to set CommandTimeout (very likely in these files, since ~all sites now do), the bare hand-off is reported as "timed" even though it is untimed — a false negative in exactly the class of blind spot this PR is otherwise closing.

Concretely, for

builder.Register(factory.CreateCommand, options);
var other = new NpgsqlCommand(Sql, connection) { CommandTimeout = 10 };

ConstructionSpanFrom starting at the .CreateCommand, match walks forward, finds the ( of new NpgsqlCommand(Sql, connection) on the next line, and returns a span running through { CommandTimeout = 10 } — so SetsAnExplicitDeadline returns true for the handoff site, which has no timeout of its own.

Contrast with ViewerCommandTimeoutTests/AnalysisPassCommandTimeoutTests, which handle this same shape via a dedicated s_commandFactoryHandoff regex + a NoXCommand_IsCreatedByABareMethodGroupHandoff test that forbids the shape outright, rather than trying to validate a deadline on it. This file instead folds the shape straight into s_commandCtor and lets it flow through ConstructionSpanFrom/SetsAnExplicitDeadline, which isn't sound for it.

Not live today (no such hand-off currently exists in the alert-pass scope, per the comment at line 130), so it doesn't break CI now — but it's a silent hole: someone can add an untimed .CreateCommand method-group hand-off in DarlingWorker.cs's reachable set later and have this guard report it clean by accident, which is the exact failure mode (#2786-shaped) this PR's narrative is about eliminating. Worth either dropping the third alternative from this file's s_commandCtor in favor of a forbid-the-shape test like the other two files, or bounding ConstructionSpanFrom's forward scan to the enclosing statement/scope so it can't run past its start's own statement when no ( immediately follows.

[claude]

Review summary

Scope: this PR touches only Darling/Darling.Tests/* (the CommandTimeout scanning guards) and one production fix in DarlingRetention.cs. No T-SQL, no Lite-side changes, so Lite/Darling parity doesn't apply here — the whole change is Darling-only test infrastructure plus the one-line production fix it was written to catch.

Production fix (DarlingRetention.cs:958-963): adding CommandTimeout = DeleteTimeoutSeconds to the SET timescaledb.max_tuples_decompressed_per_dml_transaction = 0 command is correct and consistent with the other four DeleteTimeoutSeconds sites in the same file.

CSharpSourceWalker.StatementSpanFrom / new ConstructionSpanFrom (CSharpSourceWalker.cs): traced through the scope-bounding logic (brace-depth tracking, header/body floor, the paren-before-start → header=true reset) against the new test fixtures in CSharpSourceWalkerTests.cs — the using (...) { one statement }, braceless-header, and shared-deadline-conditional cases all check out by hand.

One correctness finding (posted inline on AlertPassCommandTimeoutTests.cs): the bare .CreateCommand method-group alternative in that file's s_commandCtor regex gets routed through ConstructionSpanFrom/SetsAnExplicitDeadline along with real constructions, but ConstructionSpanFrom's forward scan for the next ( is unbounded when no paren is directly attached to the match — it can walk into an unrelated neighboring construction's initializer and false-positive a genuinely untimed method-group hand-off as "timed". ViewerCommandTimeoutTests/AnalysisPassCommandTimeoutTests sidestep this by forbidding the bare-handoff shape outright (NoXCommand_IsCreatedByABareMethodGroupHandoff) rather than trying to validate a deadline on it; this file doesn't. Not live today (no such site currently exists in the alert-pass reachable set), so it's a latent guard hole rather than a current failure.

No security, input-handling, or performance concerns — this is all static-analysis test tooling plus one timeout constant.

[claude]

Correctness: the "header" fallback also fires for a construction nested inside an unrelated call's argument list, silently shrinking the two-statement window to one statement.

header = true is set here whenever any (/[ that was open before start closes — the comment even says "a statement HEADER or an argument list". But the code below treats every header == true the same way as a genuine using (...)/if (...) header: at the very next ; at the floor depth, if body < 0 it returns immediately (lines 213-221), ignoring the statements parameter entirely.

That's correct for a real statement header (the scope really does end there), but wrong when the "open bracket that closes" is just an ordinary function call wrapping the construction, e.g.:

var command = Configure(connection.CreateCommand());
command.CommandTimeout = 5;

Trace: start is at the .CreateCommand( match, which sits inside Configure('s still-open (. Walking forward, CreateCommand()'s own parens balance to depth 0, then Configure(...)'s outer ) closes — this goes negative, so header = true is set (the "argument list" branch of the comment above, not a real header). The next ; is hit at depth == floor == 0 with body < 0, so the walk returns text[start..(i+1)] = ".CreateCommand());" only — the following command.CommandTimeout = 5; statement is excluded from the span.

Every one of these pins' SetsAnExplicitDeadline checks s_assignsTimeout against exactly this span (StatementSpanFrom(code, index, statements: 2)), so a command constructed as a bare argument to another call — a totally ordinary shape (Wrap(x.CreateCommand()), Foo(new NpgsqlCommand(...))) — with its deadline assigned on the following line would be reported as untimed, even though it's correctly timed. That's a false positive on correct code, exactly the failure mode this whole PR is built to eliminate (see the "worse than a miss" reasoning in the doc comments above).

I don't see this shape in the currently-scanned files (grepped for CreateCommand() nested in another call's parens — only the one genuine using (...) case in ViewerDataService.Blocking.cs), which is why the census stays green today. But it's untested and will misfire the moment someone writes a factory/wrapper that takes a freshly-created command as an argument.

Worth either: distinguishing a real statement-header close (only using/if/while/for/foreach/lock/switch/catch parens, which are always immediately followed by {, a braceless embedded statement, or nothing else on the line) from an ordinary argument-list close, or dropping the early-return-at-first-; and letting the normal seen >= statements counting run even when header is true but no body block ever appears.

[claude]

Review

Scope: a one-line production fix (DarlingRetention.PurgeOneAsync's TimescaleDB decompression-rail lift command now carries CommandTimeout = DeleteTimeoutSeconds, matching its four siblings in the same method) plus a substantial rework of the shared CSharpSourceWalker/ConstructionSpanFrom/StatementSpanFrom test-scanning infrastructure used by the five command-timeout CI pins (.Storage, .Viewer, .Analysis, PgFactCollector, alert pass). No T-SQL or Lite changes in this PR, and none are needed — the fix is Npgsql/TimescaleDB-specific and has no Lite counterpart (DuckDB has no equivalent decompression rail or per-command Npgsql timeout), so there's no parity drift here.

The production fix itself is correct and minimal.

I traced through the new StatementSpanFrom scope-bounding logic in detail against the PR's own test fixtures and found one real gap not covered by the extensive new test suite — left as an inline comment on CSharpSourceWalker.cs: the "header" fallback (used to make the two-statement window stop at a using (...)/if (...) block boundary) also triggers for a construction nested inside an unrelated function call's argument list (e.g. Wrap(connection.CreateCommand())), which truncates the span to one statement and can misreport a correctly-timed command as untimed on the very next CommandTimeout = assignment. Doesn't affect any currently-scanned file (verified by grep), but it's an unguarded blind spot in code whose entire purpose is closing exactly this kind of blind spot.

No other correctness, security, or performance issues found. The regex widening (qualified new Some.Namespace.NpgsqlCommand( shape) is applied consistently across all five test files.

[claude]

Correctness: the "header" fallback also fires for a construction nested inside an unrelated call's argument list, silently shrinking the two-statement window to one statement.

header = true is set here whenever any (/[ that was open before start closes — the comment even says "a statement HEADER or an argument list". But the code below treats every header == true the same way as a genuine using (...)/if (...) header: at the very next ; at the floor depth, if body < 0 it returns immediately (line 213-221), ignoring the statements parameter entirely.

That's correct for a real statement header (the scope really does end there), but wrong when the "open bracket that closes" is just an ordinary function call wrapping the construction, e.g.:

var command = Configure(connection.CreateCommand());
command.CommandTimeout = 5;

Trace: start is at the .CreateCommand( match, which sits inside Configure('s still-open (. Walking forward, CreateCommand()'s own parens balance to depth 0, then Configure(...)'s outer ) closes — this goes negative, so header = true is set (the "argument list" branch of the comment above, not a real header). The next ; is hit at depth == floor == 0 with body < 0, so the walk returns text[start..(i+1)] = ".CreateCommand());" only — the following command.CommandTimeout = 5; statement is excluded from the span.

Every one of these pins' SetsAnExplicitDeadline checks s_assignsTimeout against exactly this span (StatementSpanFrom(code, index, statements: 2)), so a command constructed as a bare argument to another call — a totally ordinary shape (Wrap(x.CreateCommand()), Foo(new NpgsqlCommand(...))) — with its deadline assigned on the following line would be reported as untimed, even though it's correctly timed. That's a false positive on correct code, exactly the failure mode this whole PR is built to eliminate (see the "worse than a miss" reasoning in the doc comments above).

I don't see this shape in the currently-scanned files (grepped for CreateCommand() nested in another call's parens — only the one genuine using (...) case in ViewerDataService.Blocking.cs), which is why the census stays green today. But it's untested and will misfire the moment someone writes a factory/wrapper that takes a freshly-created command as an argument.

Worth either: distinguishing a real statement-header close (only using/if/while/for/foreach/lock/switch/catch parens, which are always immediately followed by {, a braceless embedded statement, or nothing else on the line) from an ordinary argument-list close, or dropping the early-return-at-first-; and letting the normal seen >= statements counting run even when header is true but no body block ever appears.

[claude]

Simplification: SetsAnExplicitDeadline (and the s_setsTimeout / s_assignsTimeout regex pair it uses) is copy-pasted byte-for-byte across all five pins touched by this PR (AlertPassCommandTimeoutTests, AnalysisPassCommandTimeoutTests, FactCollectorCommandTimeoutTests, StorageCommandTimeoutTests, ViewerCommandTimeoutTests), along with a near-identical TheScanner_JudgesTheSiteItself_NotItsNeighbours theory in each.

That's exactly the shape of problem CSharpSourceWalker itself was extracted to solve (per its own doc comment: "Five source-walking pins were each carrying their own copy of this (#2913); this is the one implementation they now share"). This PR adds a brand-new two-regex/one-method unit and re-introduces the same five-way duplication for it rather than hoisting it onto the shared walker (e.g. as CSharpSourceWalker.SetsAnExplicitDeadline(code, index, timeoutFieldName) or similar). Worth folding in now while the logic is fresh, since the walker's own history shows how these copies drift once they've shipped.

Not a correctness issue — all five copies are currently identical — just flagging the maintenance risk this codebase has already paid down once.

[claude]

Review

The production fix (DarlingRetention.cs) is correct and minimal: the SET timescaledb.max_tuples_decompressed_per_dml_transaction = 0 lift now carries DeleteTimeoutSeconds instead of inheriting Npgsql's undocumented 30s default, matching the other four command constructions in that method. Straightforward, no concerns.

The scanner logic (CSharpSourceWalker.StatementSpanFrom / new ConstructionSpanFrom) — traced through the scope-bound rewrite (brace-ends-span, header-follows-into-governed-block/braceless-statement, argument-list-vs-header disambiguation via StartsAStatement) against the new test fixtures in CSharpSourceWalkerTests.cs. The floor/body/header state machine checks out for every shape the tests exercise (single-statement using bodies, plain trailing blocks, braceless headers, nested argument lists, shared-deadline conditional arms). Didn't find a case the tests don't already cover, and the PR body's own mutation-testing writeup is unusually thorough for this class of change.

Parity: no Lite counterpart needed here — this is Darling/Npgsql-specific static-analysis test tooling with no equivalent scanning infrastructure on the Lite/SqlCommand side, so there's no drift introduced.

Security/perf: none of this touches runtime request paths; it's build-time test tooling plus one bounded timeout on an existing internal command. No injection surface (regexes run over the repo's own source files, not external input).

One simplification note left as an inline comment: SetsAnExplicitDeadline and its two backing regexes are now duplicated verbatim across all five *CommandTimeoutTests.cs files, which is the same duplication shape CSharpSourceWalker was extracted to eliminate for the walk itself (#2913). Not a bug — all five copies are currently identical — just a candidate for hoisting onto the shared walker before it drifts.

[claude]

Reviewed. This PR is mostly test-infrastructure (the CSharpSourceWalker scope-bounded statement scanner and its five CommandTimeout pin suites) plus one real production fix.

Production fix (DarlingRetention.PurgeOneAsync, Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs:960-965) — correct and complete. The SET timescaledb.max_tuples_decompressed_per_dml_transaction = 0 command now carries CommandTimeout = DeleteTimeoutSeconds, matching the file's other 4 NpgsqlCommand constructions (verified all 5 now set it). No Lite counterpart is needed — Lite's retention path is embedded DuckDB with no TimescaleDB/Npgsql concept, consistent with the "no Lite twin" pattern already established for #2952.

CSharpSourceWalker.StatementSpanFrom / ConstructionSpanFrom (test-only, Darling/Darling.Tests/CSharpSourceWalker.cs) — traced the new scope-bound + header-follow logic by hand against the fixtures in CSharpSourceWalkerTests.cs (braced using block with a one-statement body, braceless using header, nested argument-list-vs-header disambiguation, and the header-block-first-statement case). All traces match the asserted expectations; the depth/floor/body bookkeeping correctly distinguishes a statement header's embedded block from an ordinary nested scope, and correctly stops at the enclosing scope's close rather than leaking into the following statement. I did not find a case among these paths that diverges from what the tests pin.

No correctness, parity, security, or performance issues found. Nothing here should trip the missing-index-DMV concern (not applicable — no T-SQL changed). Style (XML doc comments on internal helpers, block comments) is consistent with CONTRIBUTING.md's C# section.

[claude]

Blocking: unresolved merge-conflict markers committed to the branch. This range still has literal <<<<<<< HEAD / ======= / >>>>>>> origin/dev markers (lines 308, 323, 330, 335/336, 339, 344, 346, 350). This is not a diff-rendering artifact — grep -n "<<<<<<<" Darling/Darling.Tests/AlertPassCommandTimeoutTests.cs on the checked-out branch confirms they're in the file as committed.

This won't compile: Darling.Tests will fail with parser errors (<<<<<<< isn't valid C#), so CI must be red on this branch, or at minimum any build run against it will fail.

Worth noting the two conflict sides aren't even reconcilable as-is: the origin/dev side (lines 324-330, 347-349) still calls s_setsTimeout directly against a StatementSpanFrom window, but s_setsTimeout was deleted from this file earlier in the same diff (in favor of CommandDeadlineScanner.SetsAnExplicitDeadline). The HEAD side (309-322, 344-345) is the version that matches the rest of the PR's refactor (adds the three new InlineData cases for the scope/neighbour/comment blind spots and calls CommandDeadlineScanner.SetsAnExplicitDeadline). This needs a real conflict resolution — keep the HEAD side's test cases and drop the origin/dev side entirely — before this can merge.

[claude]

Review summary

Blocking issue: Darling/Darling.Tests/AlertPassCommandTimeoutTests.cs has unresolved merge-conflict markers committed to the branch (<<<<<<< HEAD / ======= / >>>>>>> origin/dev at lines 308–350). This is literal text in the file, not a diff artifact — confirmed with grep -n "<<<<<<<" against the checked-out branch. The file cannot compile as-is, so Darling.Tests is red on this branch. Left an inline comment with the specifics and which side to keep. This needs a real conflict resolution pass before merge (looks like a rebase/merge against origin/dev went wrong and got committed with conflict markers intact).

The rest of the change looks sound, modulo not being able to verify it builds given the above:

• The actual production fix in DarlingRetention.cs (PurgeOneAsync's TimescaleDB decompression-rail lift command now carrying CommandTimeout = DeleteTimeoutSeconds) is correct, matches the surrounding style, and is a real fix for a real gap — that using (...) block was inheriting Npgsql's 30s default.
• The CSharpSourceWalker.StatementSpanFrom scope-bound rework and the new ConstructionSpanFrom in CSharpSourceWalker.cs, plus the shared CommandDeadlineScanner, look like a reasonable consolidation of the five previously-duplicated regex scans, with test coverage for the blind spots described in the PR body (scope leaking past a block's closing brace, neighbour-construction false positives, and comment/literal-spelled deadlines).
• No T-SQL files are touched, so the T-SQL style conventions don't apply here.
• No Lite-side changes are needed for parity — Lite has no Npgsql/PostgreSQL command timeout concept (it's DuckDB-backed), so this is a Darling-only concern.

Once the conflict markers are resolved and the build is confirmed green, this looks mergeable.

[claude]

The "assignment belongs to the statement span" half never checks which command the assignment names, and the header-follows-into-block rule in StatementSpanFrom can hand it a sibling's assignment instead.

Repro (matches the "untimed header whose block opens with a timed sibling" shape this PR explicitly sets out to close, e.g. StorageCommandTimeoutTests.TheScanner_JudgesTheSiteItself_NotItsNeighbours):

using (var untimed = connection.CreateCommand())
{
    using var sibling = connection.CreateCommand();
    sibling.CommandTimeout = 10;
    await untimed.ExecuteNonQueryAsync(cancellationToken);
}

Trace through StatementSpanFrom(code, indexOfUntimedCreateCommand, statements: 2):

• the ) closing the using (...) header goes negative → header = true
• the following { sets body/floor to the block's depth, and seen is still 0 at that point (the header itself contained no top-level ;)
• inside the block, the two statements it now counts against the outer untimed construction are using var sibling = connection.CreateCommand(); (seen=1) and sibling.CommandTimeout = 10; (seen=2 → span returned)

The returned span contains sibling.CommandTimeout = 10, which matches s_assignsTimeout (\.\s*CommandTimeout\s*=) — so SetsAnExplicitDeadline reports untimed as timed, even though untimed never gets a CommandTimeout and sibling's assignment belongs to a completely different variable.

Every existing "block opens with a timed sibling" fixture (in this file's tests, ViewerCommandTimeoutTests, AnalysisPassCommandTimeoutTests, FactCollectorCommandTimeoutTests) only exercises the sibling via an object initializer ({ CommandTimeout = 10 }), which has no leading dot and so doesn't hit s_assignsTimeout. None of them exercise the sibling via the CreateCommand() + member-assignment spelling — which the PR's own numbers say is the dominant shape in this codebase (112 of 119 MCP sites). That's exactly the combination that slips through here.

Since the whole point of this pass is closing "the scan answering about something other than the construction it's pointing at," it seems worth pinning: the statement-span check would need to confirm the assignment's target identifier is the same variable the construction under test was bound to, not just that some .CommandTimeout = appears in the window.

[claude]

Reviewed this branch (the CommandDeadlineScanner/CSharpSourceWalker rework plus the DarlingRetention.PurgeOneAsync fix).

The production fix itself is correct and minimal — the lift command in PurgeOneAsync now carries CommandTimeout = DeleteTimeoutSeconds, matching the other four constructions in that file. No Lite counterpart is needed: Lite has no TimescaleDB/Npgsql surface (embedded DuckDB), so there's no parity drift here.

Left one inline finding on CommandDeadlineScanner.cs: the new two-part judgement (initializer from the construction span, member-assignment from the statement span) still doesn't verify which variable a .CommandTimeout = assignment targets. Combined with StatementSpanFrom's "follow a header into the block it governs" rule, an untimed command declared in a using (...) header whose block's first two statements are a nested CreateCommand() sibling followed by that sibling's own sibling.CommandTimeout = ...; assignment gets misattributed as timed — the outer command reads clean when it never got a deadline. Every existing "block opens with a timed sibling" fixture in this PR's test matrix (Storage/Viewer/Analysis/FactCollector) only exercises the sibling via an object initializer, which has no leading dot and doesn't hit s_assignsTimeout, so this specific combination isn't pinned. Given how much rigor went into closing every other neighbour-shape in this change, it seemed worth flagging precisely rather than as a vague nitpick — details and a minimal repro are in the inline comment.

Otherwise this is a solid, well-tested change: the mutation-tested walker logic, the stripped-source handling for both construction enumeration and deadline matching, and the consolidation of the duplicated regex/judgement into CommandDeadlineScanner all look sound from manual tracing against the code. No security or SQL-injection concerns (pure C# test tooling plus a timeout addition), and no other Lite/Darling parity issues.

[claude]

Review

The DarlingRetention.cs fix itself is correct and minimal: lift now carries DeleteTimeoutSeconds, matching the other four NpgsqlCommand constructions in that file, and the T-SQL/Lite-parity concerns from the review brief don't apply here (pure C# test tooling + one Npgsql call site; Lite has no TimescaleDB retention path or equivalent scanner, so there's nothing to drift out of parity).

However, the PR's central claim — "bound every timeout scan to the site it is pointing at," with the two-part CommandDeadlineScanner judgement applied "in every scope" — isn't actually true for two pre-existing pins that weren't touched by this diff, and both still reproduce blind spots this PR fixes everywhere else:

1. CollectionSweepCommandTimeoutTests.cs — value regex still reads raw (unstripped) text

EverySweepBodyCommand_SetsTheCollectionSweepDeadline (around line 150) matches the construction census against stripped text, but then does:

var span = CSharpSourceWalker.StatementSpanFrom(body, ctor.Index, statements: 2);   // `body` is RAW
if (!s_setsTimeout.IsMatch(span))                                                    // unqualified regex over RAW span

body here is the unstripped member source (see MemberBody), so this is exactly blind spot #3 from the PR description ("a deadline merely SPELLED in a comment... stands in for the deadline itself"), still live in the one guard that covers DarlingCollectorRunner.WriteBatchAsync — described in this same file's docs as "the store write of every collector, on every server, on every cycle." A comment like /* command.CommandTimeout = 10 was here */ sitting in the two-statement window after a genuinely untimed construction would read as timed. (The existing comment fixture in this file at the TheCopyScanner_... theory happens to avoid a literal . before Timeout, so it doesn't actually exercise this — it isn't the same witness the migrated files carry.) The identical unstripped-span pattern also appears at line 196 (s_setsImporterTimeout against raw text) and in the TheCopyScanner_SeesTheImporterDeadlineWithoutBorrowingANeighbours fixture-driven test at line 314.

2. Both CollectionSweepCommandTimeoutTests.cs and McpReadCommandTimeoutTests.cs — still vulnerable to blind spot #2/#6 (neighbour's initializer/assignment)

Neither file was switched to CommandDeadlineScanner.SetsAnExplicitDeadline/ConstructionSpanFrom; both still do s_setsTimeout.IsMatch(StatementSpanFrom(...)) with an unqualified CommandTimeout\s*= regex over the wide statement span. McpReadCommandTimeoutTests.cs (line 156-158) does strip the text first, so it's safe from #3, but concretely, for this file's own dominant shape:

await using var untimed = postgres.CreateCommand(Sql);
await using var next = new NpgsqlCommand(OtherSql, connection) { CommandTimeout = McpCommandDeadlines.ReadSeconds };

untimed's two-statement window reaches into next's own object initializer, and the unqualified regex matches it — the untimed site reads as timed. This is precisely the neighbour-initializer defect (#2) documented at length in this PR, and — per the PR's own numbers — 112/119 of the MCP surface's sites use the .CreateCommand()-then-assignment shape that makes this adjacency realistic. There's no "directly-adjacent neighbour" or "sibling-assignment" fixture in TheScanner_JudgesTheSiteItself_NotItsNeighbours for this file the way there now is in .Storage/.Viewer/.Analysis/PgFactCollector/alert-pass, which is consistent with this gap not having been noticed here.

Since the PR frames itself as closing this exact defect class repo-wide (and even lists "MCP read surface" as a re-measured, clean scope in its table), it'd be worth either wiring these two pins through CommandDeadlineScanner as well, or explicitly scoping the PR's claims to the five pins it actually touched.