Repository navigation
Propagate ComposeStatementTimeoutSeconds through ApplyToConfig, with a family-derived pin (#2917) - #2919
Conversation
It was the one scalar in StoreConfigView with a same-named DarlingConfig property that ApplyToConfig never assigned, so the held config reported the darling.json value for the life of the process while config_service said otherwise. Six siblings on the same code path were assigned. This does not change the effective ceiling. DarlingManagedRoles reads config_service.compose_statement_timeout_seconds with its own SELECT and bakes it into ALTER ROLE viewer/mcp SET statement_timeout at startup provisioning, so an operator's change was always delivered on the next managed start -- #2357's design, and what StoreConfigView's own doc comment already said. What was broken was the held config's truthfulness, and the DarlingConfig doc comment invited a reader to trust it. That comment now states the restart-scoped delivery; the delivery question itself is #2918, not changed here. The pin is family-derived rather than another per-knob assertion: every existing test on this path names its own knob, which is why the seventh was dropped. StoreConfigViewPropagationTests reflects over the real types and round-trips every matched view scalar, pinning the deliberately unmatched ones as a set with written reasons.
|
Reviewed. Verified the core claims directly against the source rather than taking them on faith:
One-line production fix, well-scoped regression tests, doc comment correctly updated to stop overpromising "authoritative." No issues found. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -91,6 +91,14 @@ public sealed class DarlingConfig | |||
| /// composed query can never exceed (#2357). Seeds <c>config_service.compose_statement_timeout_seconds</c>; | |||
| /// the store is authoritative afterwards, like every other value here. | |||
There was a problem hiding this comment.
Minor doc inconsistency: "the store is authoritative afterwards, like every other value here" reads as claiming this knob behaves the same as its siblings, but the paragraph you just added three lines below says the opposite — its delivery is restart-scoped "unlike the other store-backed knobs." A reader skimming just this first sentence gets the wrong mental model before reaching the correction. Consider softening line 92 (e.g. "the store is authoritative for the held value afterwards") so it doesn't imply live-authority parity with the other knobs.
There was a problem hiding this comment.
Correct, and it was my error rather than a wording preference — I appended the correcting paragraph instead of editing the sentence it corrects, so the summary now says two opposite things three lines apart. The first sentence is the one a reader skims, so that is the one that had to change.
Fixed at the source in 1b79c55: "the store is authoritative for this property afterwards, but NOT on the same terms as its siblings." That hands the reader straight to the paragraph below instead of contradicting it.
| private static IEnumerable<PropertyInfo> ScalarViewProperties() => | ||
| typeof(StoreConfigView) | ||
| .GetProperties(BindingFlags.Public | BindingFlags.Instance) | ||
| .Where(p => p.PropertyType.IsPrimitive || p.PropertyType == typeof(string)) |
There was a problem hiding this comment.
ScalarViewProperties() only picks up IsPrimitive || string, so a future StoreConfigView scalar of a type that's neither (e.g. an enum, decimal, DateTime, Guid) is invisible to both tests here: it won't show up in EveryMatchedScalar_IsPropagatedByApplyToConfig (silently unchecked for propagation) and it won't fail TheUnmatchedScalars_AreExactlyTheOnesWithAWrittenReason either, since that test also starts from ScalarViewProperties(). That's exactly the "eighth" scenario this class's own doc comment (line 28: "covers knobs that do not exist yet") claims to guard against — ProbeValues's throw-on-unknown-type only fires for types that pass this filter in the first place, so it never triggers for a non-primitive/non-string scalar.
Not a problem for anything shipped today (all current scalars are bool/int/long/double/string), but worth either broadening the filter to catch any non-collection, non-reference-config scalar, or noting the limitation explicitly in the class doc so it isn't assumed to be exhaustive.
There was a problem hiding this comment.
This is a real hole in the pin and the sharper of the two findings — thank you. Your reasoning is exactly right, including the part that makes it nasty: ProbeValues's throw-on-unknown-type only ever sees types the filter already admitted, so it could never have fired for an enum/decimal/DateTime/Guid knob. The safety net was downstream of the gap it was supposed to catch.
I took the broaden option rather than the document-the-limitation one, since the class doc claiming "covers knobs that do not exist yet" while silently excluding the most likely next type is worse than having no pin. In 1b79c55:
ScalarViewPropertiesnow filters onIsValueType || string. Everything excluded is a reference type — the four sub-config objects (swapped wholesale by reference) and the two collections — so the rule is now stateable rather than incidental. Verified the widened filter admits exactly the same 13 properties today, so nothing about current coverage changed; only the future's.- Enums are handled generically (two distinct declared members) rather than left to throw, because a store-backed knob is far likelier to arrive as an enum than as a
Guid. A one-member enum throws, since it cannot distinguish an assignment from an unchanged default — that would be a vacuous pass, which is the failure mode this class exists to avoid. - A nullable value type is now admitted and throws until someone gives it probe values, which is the intended outcome.
- The enum arm and the throw are new machinery that no shipped
StoreConfigViewscalar exercises, so they would have rotted untested.ProbeValues_CoversEnums_AndRefusesWhatItCannotProbecovers both, and assertsIsPrimitiveis false for all five of the types you named — the hole itself is now pinned, not just patched.
Your first note is fixed too.
|
Reviewed. This is a scoped, well-verified fix — the one-line Scope check: New test design ( Left two inline notes, both minor / non-blocking:
No correctness, security, or performance concerns beyond those two nits. |
…ilter Both findings were correct. 1. The doc comment kept "the store is authoritative afterwards, like every other value here" and then contradicted it three lines later with "unlike the other store-backed knobs". I appended a correction instead of editing the sentence it corrected. Fixed at the source. 2. ScalarViewProperties filtered on IsPrimitive || string, which is false for enum, decimal, DateTime, Guid and TimeSpan. A future knob of any of those types would have been invisible to BOTH tests -- unprobed for propagation, and absent from the unmatched set that is supposed to force a written reason -- and ProbeValues' throw could never fire for it, since it only sees types the filter admitted. So the class doc's "covers knobs that do not exist yet" overclaimed. Widened to IsValueType || string, which is what makes the throw load-bearing. Enums are handled generically rather than left to throw (two distinct declared members; a one-member enum throws, since it cannot distinguish an assignment from an unchanged default). Verified the widened filter admits exactly the same 13 properties today, so current coverage is unchanged. The enum arm and the throw are new machinery that no shipped StoreConfigView scalar exercises, so they get their own test rather than shipping untested.
|
Reviewed. This is a clean, well-scoped one-line fix — the missing Verified against the code (not just the PR description):
No Lite counterpart exists for this machinery (managed Postgres roles / statement_timeout composition is Darling-only), so there's no parity gap to flag. Test coverage is solid: a direct regression test ( No correctness, security, or performance issues found. No T-SQL in this diff. |
1 participant
Fixes #2917.
StoreConfigProvider.ApplyToConfigassigns every scalar inStoreConfigViewthat has a same-namedDarlingConfigproperty — exceptComposeStatementTimeoutSeconds, which was seeded toconfig_service, selected by the view read, clamped to [5,600], mapped into the view, given a doc comment promising the store was authoritative, and then never assigned. Six siblings on the same code path (CapturePlans,QueryStoreBackfillEnabled,QueryStoreTextBudgetMb,PlanContentRetentionDays,MaxConcurrentSweeps,PlanXmlCompression) were all assigned.This does not change the effective ceiling, and that is the finding
I traced the consumer before writing the one-liner, and it changes what the fix claims. Nothing reads
DarlingConfig.ComposeStatementTimeoutSecondsexcept the seed.DarlingManagedRoles.ReadComposeStatementTimeoutAsyncreadsconfig.config_service.compose_statement_timeout_secondswith its ownSELECTandBuildProvisioningSqlbakes it intoALTER ROLE viewer/mcp SET statement_timeout— a persistent role attribute, written once per process fromDarlingWorker.cs:1025.ReloadFromStoreAsyncnever re-provisions.So an operator's change was always delivered on the next managed start. That is #2357's design, and
StoreConfigView.ComposeStatementTimeoutSeconds's own doc comment has always said so ("a change here reaches an existing install on its next restart"). The reload was never the delivery mechanism, so the missing assignment was not dropping an operator's value on the floor in any observable way.What it was doing is leaving the held config permanently untruthful:
config.ComposeStatementTimeoutSecondsreported thedarling.jsonvalue for the life of the process whileconfig_servicesaid otherwise, and theDarlingConfigdoc comment invited a reader to trust it — "the store is authoritative afterwards, like every other value here". A future consumer reading the held value would have silently gotten the file value. That comment now states the restart-scoped delivery, and says explicitly that the held property is the store's desired value rather than what the live roles are enforcing.It is also an inconsistency in the seam's contract rather than an instance of a design. #2862's plan-capture cadence deliberately relies on "a property ABSENT from
StoreConfigViewsurvives a reload untouched"; this one is present in the view and dropped anyway, which is neither intended behaviour.The delivery question is filed as #2918, not changed here — a reload observes the new value without re-asserting it onto the roles, unlike every other
config_serviceknob. That issue carries the mechanism, four options, and what should decide between them. It is a behaviour change on a prod path (which connection, how often apg_authidwrite is acceptable, the BYO split) and not this PR's call.The pin is family-derived
Every existing test on this path names its own knob. That is exactly why the seventh was dropped and why the eighth would be, so
StoreConfigViewPropagationTestsreflects over the real types instead:EveryMatchedScalar_IsPropagatedByApplyToConfig— round-trips everyStoreConfigViewscalar with a same-named, same-typed, settableDarlingConfigproperty throughApplyToConfig. Two probe values apiece, each pre-setting the held config to the other one, because several of these defaults are already equal across the two types and a single probe would pass against an assignment hardcoded to that value. Also catches cross-wiring (config.A = view.B).TheUnmatchedScalars_AreExactlyTheOnesWithAWrittenReason— pins the deliberately-unmatched set with reasons:ConfigVersionandPausedare worker-held (_lastConfigVersion,_paused), and the fourMcp*/Web*are nested underconfig.Mcp/config.Web(assigned, just not name-matched at the top level). A new view scalar therefore either gets covered automatically or fails here until someone writes down why it does not belong.ApplyToConfigtakes no I/O, so both are pure.ComposeStatementTimeoutStoreTestsgets the specific pin too, with a comment saying it pins propagation and not delivery.Verification
Darling.Testsisnet10.0-windowsand cannot execute on macOS, so I compiled the actualStoreConfigViewPropagationTests.csinto anet10.0console harness against the realPerformanceMonitor.Darling.Servicebuild (plus a verbatim hand-copy of the compose method, whose own file also references the WPFViewerDataService). Both files build clean inDarling.Testsitself — 0 errors.Proven red first. With the one assignment removed:
Both probe directions fail, naming the knob. With the assignment restored, all 5 cases pass. CI is the arbiter for the suite as a whole.