Detect a pending restart on Windows PostgreSQL targets from pg_file_settings (#4251)

[erikdarlingdata]

Part of #4251 (PostgreSQL targets; lane A1b does the managed store on #4336).

Why

pg_settings.pending_restart is backend-local in PostgreSQL. It only flips true on the connection that was open when pg_reload_conf() ran. PgServerConfigCollector reconnects every cycle, so a setting that is genuinely still pending a restart (someone edited postgresql.conf or ran ALTER SYSTEM, then reloaded) reads pending_restart = false forever on this column alone. This is not a Windows-only PostgreSQL bug, but it is most visible on Windows deployments because there every connection is a fresh EXEC_BACKEND process with no long-lived session to have caught the original reload.

pg_file_settings names the same fact from the file rather than backend state: a row for that setting whose error = 'setting could not be applied' at a context = 'postmaster' setting is pending a restart regardless of which connection asks. The same error text also marks an outright rejected value at a non-postmaster context (verified against a live PostgreSQL 18.6 rig below), so context = 'postmaster' is the discriminator, not the error text alone.

What changes

• PerformanceMonitor.Collectors/PgServerConfigCollector.cs — the only place pending_restart is computed. BuildQuery now sends one of two query texts, chosen by CollectorContext.PgFileSettingsReadable:
  • QueryText (today's, unchanged) when the view is unreadable.
  • QueryTextWithFileSettings when it is readable: the pg_settings arm's pending_restart column becomes s.pending_restart OR EXISTS (correlated pg_file_settings row, error = 'setting could not be applied', s.context = 'postmaster'). The override arm (pg_db_role_setting) is untouched — an override has no file line and cannot be pending a restart.
  • No reader changes: PgTargetFactCollector.Config.cs, .Posture.cs, .Sessions.cs and the logging-audit tool all just read the stored pending_restart column, which is unaffected in shape.
• PerformanceMonitor.Collectors/PgFileSettingsCapability.cs (new) — whether a target can read pg_file_settings, resolved once per hour per target and cached, the same shape PgReadBinaryFileCapability uses for pg_read_binary_file. Two grants are required, not one (measured on a live 18.6 rig, not assumed from the docs): GRANT SELECT ON pg_file_settings alone still throws permission denied for function pg_show_all_file_settings, because a plain view delegates its owner's privilege to the TABLES it reads, never to a FUNCTION it calls — that call is checked against the calling role's own EXECUTE grant. The probe checks both (has_table_privilege and has_function_privilege) in one round trip, so PgServerConfigCollector never sends a query that might fail on this view — it decides which of its two query texts to send before either runs.
• PerformanceMonitor.Collectors/CollectorContext.cs — new PgFileSettingsReadable bool (default false), the same settable-not-init-only shape as PgReadBinaryFileGranted.
• Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs — new ResolvePgFileSettingsReadableAsync, called on the same server-scoped path right beside ResolvePgReadBinaryFileGrantAsync, for pg_server_config only. Logs ONCE at Information per target when the view turns out to be unreadable (PgFileSettingsCapability.ShouldLogUnreadable), not every cycle — the collector keeps collecting pg_settings either way, so this is informational, never a fault.
• Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPgServerStateTools.cs (get_pg_server_config) and DarlingMcpPgLoggingAuditTools.cs (get_pg_logging_audit) — attach a pending_restart_caveat line when the collector's own cached verdict says this target's pg_file_settings is unreadable. Target OS is not tracked anywhere in CollectorTargetInfo or elsewhere for PostgreSQL targets (checked — there is no existing fact for it), so per the brief this uses no OS condition: the line shows whenever the view is confirmed unreadable, on any OS, worded to name Windows as the most visible case.

CHANGELOG entry

SECTION: Fixed
ENTRY:

• PostgreSQL pending_restart can now be trusted after a reload, on Windows targets ([Detect a pending restart on Windows PostgreSQL targets from pg_file_settings (#4251) #4345]) - pg_settings.pending_restart is backend-local, so on Windows, where the collector's reconnect-every-cycle model always sees a brand new backend, a setting that had actually been changed and reloaded but still needed a restart could read false forever. Unix targets never had this gap: there, the postmaster itself applies the reload and every backend it forks inherits the flag, so pg_settings alone was already correct. The collector now also checks pg_file_settings on Windows targets, which is read from the file rather than a connection's own state, when the monitoring role can read it (two grants: SELECT on the view and EXECUTE on pg_show_all_file_settings()). get_pg_server_config and get_pg_logging_audit note when a Windows target's role cannot read it, and say what the two grants expose, so an operator can weigh the trade before granting them; a non-Windows target is untouched and never asked to widen its role for this.
  REF:
  [Detect a pending restart on Windows PostgreSQL targets from pg_file_settings (#4251) #4345]: Detect a pending restart on Windows PostgreSQL targets from pg_file_settings (#4251) #4345

Test plan

• Diagnosis confirmed on a live rig (PostgreSQL 18.6, Windows) with raw SQL before writing any code:
  • ALTER SYSTEM SET shared_buffers = '256MB'; SELECT pg_reload_conf(); then a new connection: pg_settings.pending_restart = f for shared_buffers (context postmaster) — the bug, reproduced.
  • Meanwhile pg_file_settings correctly shows error = 'setting could not be applied' for that row.
  • A hand-edited work_mem = '1kB' (out of range; ALTER SYSTEM itself validates range and refuses to write this, so it was appended to postgresql.auto.conf directly, exactly how a hand-edited postgresql.conf would) produces the identical error text at context = 'user' — proving context = 'postmaster' is the necessary discriminator, not the error text alone.
  • GRANT SELECT ON pg_file_settings TO a pg_monitor role alone still threw permission denied for function pg_show_all_file_settings — only GRANT SELECT + GRANT EXECUTE ON FUNCTION pg_show_all_file_settings() together worked. This corrected my own first assumption (and possibly the issue's implicit one) that one grant would suffice; PgFileSettingsCapability probes both.
• New file Darling/Darling.Tests/PgServerConfigPendingRestartLiveTests.cs, 3 live tests against a real rig (DARLING_TEST_PG), all passing:
  • APostmasterSettingPendingARestart_ReadsFalseOnPgSettingsAlone_ButTrueOnceFileSettingsIsFolded_In — reproduces PostgreSQL on Windows: pending_restart reads false on connections opened after the reload, so we can miss a pending restart #4251 end to end (a genuinely new, unpooled connection reads pending_restart = false; the pre-fix query path reproduces that; the fixed query path reports true for the same live state). I proved the first assertion fails on the pre-fix collector query by construction: the "before" run uses the untouched QueryText, byte-identical to what shipped before this PR.
  • ARejectedValueAtANonPostmasterContext_IsNeverStoredAsPendingARestart — the discriminator.
  • ANonSuperuserRoleWithoutTheGrants_KeepsTodaysAnswer_AndTheCapabilityLogsItOnce — a pg_monitor-only role still collects the full pg_settings snapshot (not an error), and ShouldLogUnreadable fires exactly once.
  • Note: Npgsql pools physical connections by default, so every connection standing in for "a fresh collector cycle" uses Pooling=false — an early version of this test used pooled connections and consistently failed because a reused, already-open backend correctly sees pending_restart (only a truly new backend has the blind spot). Once I disabled pooling for those connections, all 3 tests passed reliably across repeated runs, including after a full pg_ctl restart.
• Fixed a source-pin test my change legitimately altered: PgServerConfigScopeRungTests.TheToolAttachesTheOverrideSection_AndLeavesTheServerWideCountsAlone asserted the literal text if (overrides.Count == 0); updated to the new if (overrides.Count == 0 && !fileSettingsUnreadable) and added assertions for the new attached pending_restart_caveat node.
• Targeted classes, all green (build: 0 Warning(s), 0 Error(s)):
  • PgServerConfigTests, PgServerConfigOverrideLivePostgresTests, PgServerConfigScopeRungTests, PgServerConfigToolBoundTests, DarlingMcpPgLoggingAuditToolsTests, McpPayloadContractCensusTests, StorageCommandTimeoutTests, PgServerConfigPendingRestartLiveTests — 150/150.
  • PgTargetFactCollectorTests, DocCommentHygieneTests, LivePostgresCollectionHygieneTests, DarlingCoreToolProfileTests — 104/104.
  • Could not find a class literally named an "MCP inventory pin" under that name in Darling.Tests in the time available; McpPayloadContractCensusTests and DarlingCoreToolProfileTests above are the closest matches I located and both pass. Worth the coordinator double-checking there isn't a differently-named pin I missed (I did not add or remove any MCP tool, only an attached field on two existing tools' output, so I don't expect one to be affected).
  • LiveCleanupConversionRatchetTests — passes; my three new live tests originally tripped it (finally blocks not routed through LiveStoreCleanup), fixed with LiveStoreCleanup.RunOwnedAsync (the cleanup has to run on the same admin connection / rig data directory, not a fresh store connection, so RunOwnedAsync rather than RunAsync).
• Merged origin/dev (clean, no conflicts), rebuilt (0 Warning(s), 0 Error(s)), ran the full Darling.Tests suite once against a freshly recreated darlingtest: 14227 total, 0 errors, 4 failed, 48 skipped (702s).
  • 1 of the 4 was LiveCleanupConversionRatchetTests flagging my own new file — fixed (see above) and re-verified green, but I did not re-run the complete 14k-test suite a second time afterward given context budget; I re-ran only the affected classes (PgServerConfigPendingRestartLiveTests + LiveCleanupConversionRatchetTests, 18/18 green).
  • The other 3 — CaptureDownChunkOrderTests.TheShippedRead_ExecutesOnlyTheNewestChunk_AndTheNewestRunDecides_AgainstDevPostgres, ServerListAndSummaryPlanShapeTests.TheShippedReads_TouchFarFewerChunks_AndReturnTheSameNewestCollection_AgainstDevPostgres, TrendPayloadBudgetLiveTests.EveryDefaultAnswer_StaysNearTheBudget_AndTheLargestAnswerStaysUnderTheCap — are in files I never touched, unrelated to PostgreSQL server config, and look like plan-shape/chunk-count/payload-budget assertions that may be sensitive to a freshly created rig's autovacuum/chunk state rather than to this change. I did not have the context budget left to run the "re-run alone on a fresh database, then check dev's own CI" verification the skill requires before calling these pre-existing. Flagging them for the coordinator rather than asserting they're unrelated.
• Linux: not verified — there is no Linux PostgreSQL on this machine. The fix is the same on both platforms: a pending row in pg_file_settings means pending regardless of platform; only the frequency with which a fresh, never-reloaded connection gets asked differs (Windows's EXEC_BACKEND model reconnects for everything; a long-lived Unix session that was open during the reload would already show it correctly even without this fix, which is presumably why the issue reads as Windows-specific in practice).
• Full suite not re-run a second time after the LiveCleanupConversionRatchetTests fix (context budget) — see above.

Round-1 fixes (P2c: M2, M3, L1, L4)

Fixes the other half of round 1's review of this PR. Lane P2b fixes H1, M1, L2, L3 on the same branch (grant advice, the probe, the cache) and owns the CHANGELOG entry above; this section covers only the pending-restart predicate and its tests.

• M3 (PerformanceMonitor.Collectors/PgServerConfigCollector.cs:213-235, comment at :190-212): replaced the pending_restart predicate in QueryTextWithFileSettings. The EXISTS now matches a postmaster-context pg_file_settings row two ways: by name, case-insensitively (lower(pfs.name) = lower(s.name)), since ParseConfigFp stores the name as written and a hand-edited Shared_Buffers = ... would otherwise have missed it; or, for the row PostgreSQL records with no name when a postmaster-context setting leaves every config file (ALTER SYSTEM RESET, or a deleted line), by matching that row's exact error text (parameter "<name>" cannot be changed without restarting the server), since pfs.name is empty on it and there is nothing to correlate by name. The comment notes both error strings are untranslated (pstrdup/psprintf, no _(), guc-file.l:14 and :18), so lc_messages never changes either one. I kept the review's SQL logic exactly and only re-aligned the trailing AS pending_restart, to this file's own column (index 44, same as every other column in the list) rather than the review's markdown spacing.
  • Live pin: PgServerConfigPendingRestartLiveTests.cs:215 (APostmasterSettingRemovedFromEveryFile_StillReadsPendingARestart) removes the max_worker_processes line the rig's own postgresql.conf sets, reloads, asserts the stored row reads pending_restart = true, then restores the file and reloads in a finally.
• M2 (comment at PgServerConfigCollector.cs:207-212; reading guide at DarlingMcpPgServerStateTools.cs:619): corrected the comment, which claimed s.context = 'postmaster' is what tells "pending" apart from "rejected" — it is not; pg_file_settings has no column for that, so a postmaster-context value PostgreSQL rejected outright (bad syntax, out of range) reads pending_restart = true too, same as a genuinely pending one. Added the review's sentence to get_pg_server_config's reading guide: "Where pg_file_settings is read, true can also mean the file holds a value PostgreSQL rejected, which would stop the next start; the server log names it as 'invalid value for parameter'."
  • Live pin: PgServerConfigPendingRestartLiveTests.cs:276 (ARejectedValueAtAPostmasterContext_CannotBeToldApartFromPending_AndReadsTrueToo) appends max_connections = 0 to the rig's postgresql.auto.conf directly (ALTER SYSTEM validates range itself and refuses to write it, same as the existing work_mem test), reloads, and asserts the stored row reads pending_restart = true — the documented caveat this pin locks in, not a bug.
• L1 (PgServerConfigScopeRungTests.cs:572): pasted the review's offline pin, TheFileSettingsQuery_IsTheDefaultQuery_WithOnlyThePendingRestartColumnChanged, unchanged from the review's text except Deltas = NoDeltas.Instance in place of Deltas = null! (the class already defines NoDeltas for exactly this). It locates the differing span with IndexOf rather than hardcoding the predicate's text, so M3's new, longer predicate needed no further adjustment to keep it passing.
• L4 (PgServerConfigPendingRestartLiveTests.cs:93, :169): added Assert.SkipWhen guards — a non-Windows version() before the first test's core assertion (a fresh connection reading pending_restart = false is Windows-only, per H1: EXEC_BACKEND rebuilds a new backend's settings from the postmaster's saved values, and the pending flag is not among them), and !Directory.Exists(dataDirectory) before the work_mem test's file edit. Applied the same directory-exists guard (:226, :287) to the two new live pins added for M2 and M3, since they also edit local config files and would otherwise throw rather than skip on a target whose data directory is not this machine.

Tests (P2c)

All from Darling/Darling.Tests/Darling.Tests.csproj, build 0 Warning(s) / 0 Error(s), against a dedicated rig (PostgreSQL 18.6, Windows, port 55980, its own data directory — separate from lane P2b's).

• *PgServerConfig* (every class in both files I touched, plus the rest of the family): 34/34 offline (11 correctly skipped without DARLING_TEST_PG set), then 253/253 with the rig's connection string set and 0 skipped — every live test, including the 2 new pins, ran for real rather than skipping.
• McpPayloadContractCensusTests, McpToolsListBudgetTests, DocCommentHygiene*: 150/150.
• LivePostgresCollectionHygieneTests: 11/11 (live).
• MCP inventory pins: found no class literally named that; ran the closest matches for a tool-count/description-length regression, DarlingPeerDisclosureTests and ReadmeDerivedCountPinTests: 36/36, unaffected — M2's fix only lengthens one existing tool's description text and adds no tool.
• Final check on the land branch (fast-forward merge of this work onto the shared PR head, no conflicts to resolve): rebuilt, reran everything above together against the live rig: 253/253, 0 failed, 0 skipped.
• Did not run the full suite — lane P2b owns that per the brief.

Nothing from M2/M3/L1/L4 was deferred; all four are done and pinned live on my own rig, pushed to this branch.

Round-1 fixes (P2b: H1, M1, L2, L3)

Fixes the other half of round 1's review: the grant advice, the capability probe, and the stale-verdict cache. Built on top of P2c's commit (merged clean, no conflicts) and origin/dev (also merged clean).

• H1(a) — the advice only applies on Windows targets (PerformanceMonitor.Collectors/PgFileSettingsCapability.cs:71-84): ProbeSql now returns a second fact in the same round trip as a readable:isWindows scalar (the same "combined:bool" shape PgReadBinaryFileCapability.IsGrantedAsync already parses for its own encoding-plus-verdict probe) — whether pg_catalog.version() ~* '(windows|visual c\+\+|msvc|mingw)'. By PostgreSQL's process model the stale pending_restart this whole feature exists to fix only happens on Windows (EXEC_BACKEND rebuilds a new backend's settings from the postmaster's saved values; on Unix every backend is forked from the postmaster and inherits the flag), so granting these two objects on Linux, RDS, Aurora, Azure or Cloud SQL fixes nothing. DarlingCollectorRunner.ResolvePgFileSettingsReadableAsync (DarlingCollectorRunner.cs:1319-1367) now sets context.PgFileSettingsReadable = readable && isWindowsTarget, so a non-Windows target always keeps the plain query even when the grants ARE present, and the one-time log line only fires when isWindowsTarget && !readable. PgFileSettingsCapability.TryGetCachedVerdict (:171-186) grew a third out bool isWindowsTarget parameter for the two MCP callers to gate their caveat the same way (DarlingMcpPgServerStateTools.cs:680-684, DarlingMcpPgLoggingAuditTools.cs:74-79).
  • Pinned in PgFileSettingsCapabilityTests.TheWindowsRegexClassifiesKnownVersionStrings, a [Theory] against the two Linux strings and the pre-17 Windows string from the review, plus this rig's own live version() text captured verbatim: PostgreSQL 18.6 on x86_64-windows, compiled by msvc-19.44.35228, 64-bit (confirmed against the running rig with SELECT version(); before writing the pin).
  • ResolvePgFileSettingsReadableAsync_GatesOnWindows is a 4-row theory covering all of (readable, Windows) × (readable, non-Windows) × (unreadable, Windows) × (unreadable, non-Windows), asserting context.PgFileSettingsReadable only comes out true for (readable, Windows).
• H1(b) — say what the grants expose: UnreadableCaveat (PgFileSettingsCapability.cs:87-96) and the DarlingCollectorRunner log line (DarlingCollectorRunner.cs:1348-1361) both now use the review's paste, naming the exposure (every line of every config file, including superseded or misspelled lines that can carry a password, and each file's path) rather than only naming the grant. Added a new runbook section, docs/postgres-first-target-runbook.md:136-159 ("Windows only: the pg_file_settings grants"), since neither Darling/README.md nor the runbook mentioned this grant pair at all before this PR — the review's finding.
• M1 — forget a stale "readable" verdict on 42501 (DarlingCollectorRunner.cs:1401-1420): added ForgetStaleFileSettingsVerdict, mirroring ForgetStaleReadBinaryFileVerdict. Without it, a revoked grant (or a login switched to a lower-privilege role) left the cached "readable" verdict standing for up to the hour-long CacheTtl, and every pg_server_config cycle in that window would send the enhanced query, get a 42501, and lose the WHOLE pg_settings snapshot — the one outcome the collector must never have.
  • In-lane fix beyond the review's literal line numbers: the review (and my brief, quoting it) said to call this from DarlingWorker.cs's general handler at ~9268/~10324 (LogTailGeneralFault, called from the general catch (Exception ex) block). I traced the actual fault path before wiring it up: PostgresTargetProvider.Classify maps SqlState 42501 to CollectorTargetFault.Permissions for every collector unconditionally (Targets/PostgresTargetProvider.cs:122), and PostgresFaultOutcome's generic Permissions arm returns Status "PERMISSIONS", which is not "ERROR" — so a pg_server_config 42501 is caught by the FIRST catch clause (DarlingWorker.cs:10161-10205, Status: not "ERROR") and return 0'd there. It never reaches the general catch/LogTailGeneralFault at all. Calling ForgetStaleFileSettingsVerdict only where the review said to would have shipped dead code — never invoked for the exact fault it exists to handle. I added the call in BOTH places ForgetStaleReadBinaryFileVerdict is already called (DarlingWorker.cs:9350, inside LogTailGeneralFault, a no-op here since collectorName != "pg_server_config" never true for this handler's own log-tail readers; and DarlingWorker.cs:10275, inside the PERMISSIONS-status block, which is where a real pg_server_config 42501 actually lands), with a comment at each explaining why. Verified functionally, not just by the guard clause: ForgetStaleFileSettingsVerdict_DropsOnlyAStaleReadableVerdictOnPgServerConfigs42501 in the new test file calls the method directly and checks the cache before/after for all three review-specified cases.
  • Three-case unit test: PgFileSettingsCapabilityTests.ForgetStaleFileSettingsVerdict_DropsOnlyAStaleReadableVerdictOnPgServerConfigs42501 (cached readable + 42501 on pg_server_config drops it; 42501 on another collector leaves it; a cached unreadable verdict is left alone), plus ..._IgnoresNonMatchingFaults for a non-PostgresException and a wrong SQLSTATE.
  • CollectionLogDrainForensicsStoreTests still passes unchanged (13/13 on the drain: null/fetchPhases: null census) — this fix adds a call inside two EXISTING fault arms, not a new early-return arm, so nothing there needed updating.
• L2 — harden ProbeSql (merged into the same constant as H1(a)'s second column, PgFileSettingsCapability.cs:71-84): both object names now go through to_regclass/to_regprocedure under a coalesce(..., false), so a target missing the view or function (never seen in practice — core since 9.5) reads as "not readable" instead of raising 42P01/42883 on every cycle.
• L3 — restored the dropped space in DarlingMcpPgLoggingAuditTools.cs:143 ("... - statements," → "... - statements, "), so the note field reads "statements, locks" again instead of "statements,locks".

Tests (P2b)

Build: Darling/Darling.Tests/Darling.Tests.csproj, 0 Warning(s), 0 Error(s), throughout. Rig: PostgreSQL 18.6, Windows, port 55979, its own data directory (C:\GitHub\worktrees\rig-p2b) — separate from lane P2c's.

• New file Darling/Darling.Tests/PgFileSettingsCapabilityTests.cs (21 tests): the Windows-regex theory, the combined-scalar cache round trip, TryGetCachedVerdict's new 3-arg shape, malformed/missing-scalar defensiveness (matched to PgReadBinaryFileCapability's own "no separator = fully malformed" convention — my first draft treated a bare "true" scalar as the readable half with an empty Windows half and a test caught the mismatch before I pushed), the ForgetStaleFileSettingsVerdict 3-case pin, and ResolvePgFileSettingsReadableAsync's Windows-gating theory. All 21 pass.
• Targeted classes together (270/270, 0 failed): DocCommentHygieneTests, PgReadBinaryFileAdvisoryTests, PgReadBinaryFileCapabilityLiveTests, PgReadBinaryFileCapabilityTests, PgFileSettingsCapabilityTests, PgServerConfigOverrideLivePostgresTests, PgServerConfigPendingRestartLiveTests, PgServerConfigScopeRungTests, PgServerConfigTests, PgServerConfigToolBoundTests, PgServerConfigToolBoundLiveTests, CollectionLogDrainForensicsStoreTests, LivePostgresCollectionHygieneTests, McpPayloadContractCensusTests, McpToolsListBudgetTests — run twice, once before and once after merging P2c's commit and origin/dev, both green.
• Merged P2c's commit (ce151ba1, auto-merge, no conflicts — touched PgServerConfigCollector.cs and a different region of DarlingMcpPgServerStateTools.cs than mine) and origin/dev (auto-merge, no conflicts, a large merge: a new raw-chunk-interval reconciler, Lite File I/O/memory trend bucketing, store-upgrade work, none of it touching the files this fix changed).
• Full Darling.Tests suite, run ONCE, against a freshly recreated darlingtest (dropped and recreated after the origin/dev merge, matching that merge's migrations): 14309 total, 0 errors, 1 failed, 55 skipped, 1 not run, 622s.
  • The 1 failure, TrendPayloadBudgetLiveTests.EveryDefaultAnswer_StaysNearTheBudget_AndTheLargestAnswerStaysUnderTheCap, is pre-existing and not mine: it's the same test P2c already flagged as unrelated (plan-shape/payload-budget, in a file neither of us touched); it still failed after merging origin/dev and on a freshly created database (the skill's bar for calling a failure pre-existing); and I checked dev's own latest CI Build run (bcf18734, 2026-09-25 20:35 UTC) — its conclusion is also "failure", so dev itself does not currently pass this test. Per the skill's rule ("If dev passes, the failure is yours" — implying the converse), I did not chase this further given context budget; flagging it for the coordinator.
  • I did not identify the "1 not run" by name in the time I had left; it may be a companion test in the same fixture/collection as the failure above. Flagging rather than guessing.
• Rig stopped (pg_ctl stop -m fast) after the run.

Coordinator: please double-check

• The M1 call-site correction above (DarlingWorker.cs:9350 and :10275, not the ~9268/~10324 the review/brief cited) — I'm confident in the trace (PostgresTargetProvider.Classify → PostgresFaultOutcome's generic Permissions arm → the first catch clause), and it's proven by a passing test that calls the method directly, but it's a real deviation from the review's literal wording worth a second look.
• The "1 not run" in the full-suite run.
• Whether TrendPayloadBudgetLiveTests's pre-existing failure (also failing on dev's latest CI) is worth its own issue — I did not file one given context budget, but the coordinator's own census-of-flakes process (lane-orders, "when a PR's required check fails on a test it doesn't touch") is the right next step for it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3

pm-pr lane report (N1-N5)

Head pushed: 9dcd498dbc2ae4869e968680d43754c4531c8cd3 (was 14d53b98f2a4ff4e6081d7c8a24b39ef20cbf873; merged origin/dev on top, no conflicts, kept both sides).

• N1 (Medium). PerformanceMonitor.Collectors/PgFileSettingsCapability.cs: rewrote the Windows-version probe's regex literal as '(windows|visual c[+][+]|msvc|mingw)' (bracket expression, no backslash), both in ProbeSql and the doc comment. Added the pin TheProbeCarriesNoBackslash_SoItParsesTheSameUnderEitherStringSetting (Assert.DoesNotContain('\\', PgFileSettingsCapability.ProbeSql)) to PgFileSettingsCapabilityTests. Proved it RED against 14d53b98's original ProbeSql (stashed the collector fix, kept the new test, built, ran: 1 failed - Item found in collection, the \ at position 326), then restored the fix and reran: GREEN, 22 tests in the class, 0 failed. Did not add the optional live pin (setting standard_conforming_strings = off mid-test) - the ruling accepted R2's pasted code as written, and that pin was explicitly offered as optional/not run.
• N2. Added APgServerConfig42501_LandsOnThePermissionsArm_AndThatArmForgetsTheVerdict to PgFileSettingsCapabilityTests, pasted verbatim from the review, plus the RepoRoot() helper and using System.IO; / using System.Runtime.CompilerServices;. Confirmed DarlingWorker.cs:10275's ForgetStaleFileSettingsVerdict call sits inside the PERMISSIONS arm the pin targets (is { Status: not "ERROR" } outcome) at 10247, ForgetStaleFileSettingsVerdict call at 10275, before the next catch ().
• N3. DarlingMcpPgServerStateTools.cs: ended the reading-guide sentence at "...which would stop the next start; the server log names the rejected setting."
• N4. PerformanceMonitor.Analysis/PgTargetAdvice.Buffer.cs:273: replaced the pending-restart sentence with R2's pasted text ("...unless PostgreSQL rejected it, in which case the server log names it and that restart would fail.").
• N5. PgServerConfigPendingRestartLiveTests.cs: changed the skip condition from versionText.Contains("Windows", ...) to Regex.IsMatch(versionText, "(windows|visual c[+][+]|msvc|mingw)", RegexOptions.IgnoreCase), matching the product's own pattern; added using System.Text.RegularExpressions;.

Local runs (macOS, non-live unless noted):

• Build: 0 Warning(s), 0 Error(s), both before and after the origin/dev merge.
• PgFileSettingsCapabilityTests + PostgresFaultOutcomeTests + DarlingMcpPgServerStateToolsTests: Total 113, Failed 0.
• McpToolGuideHead*, McpPayloadContractCensus*, McpPageContractTests, DocCommentHygiene*, DarlingMcpPgLoggingAuditToolsTests, PgTargetMcpSurfaceTests, DarlingCoreToolProfileTests, DarlingMcpToolsTests, CollectionLogDrainForensics*: Total 399, Failed 0, Skipped 1 (a live class, no DARLING_TEST_PG).
• After the dev merge, re-ran PgFileSettingsCapabilityTests, PostgresFaultOutcomeTests, PgServerConfigPendingRestartLiveTests, PgTargetBufferDrillDownTests: Total 129, Failed 0, Skipped 6 (all live, no DARLING_TEST_PG).
• Live PG: started timescale/timescaledb:2.30.1-pg18 on port 55452 (container pmpr-4345, -c timezone=UTC -c log_timezone=UTC), created role darling. TrendPayloadBudgetLiveTests: Total 1, Failed 0 - matches the ruling (passes on dev and in this PR's CI). PgServerConfigPendingRestartLiveTests on this rig: 4 of 5 non-N5-skip cases skipped for unrelated reasons (this Linux Docker host's data directory isn't local, and N5's own Windows-pattern skip correctly fires since the rig's version() says Linux); the fifth (ANonSuperuserRoleWithoutTheGrants...) failed on a SCRAM/password rig setup issue in the test's own non-superuser role creation, unrelated to N1-N5 - stopped and removed the container after.
• Left for CI: the three Darling PG shards (full suite), and the full non-live suite the ruling didn't ask this lane to run locally.

CHANGELOG entry: rechecked against the final diff - the entry describes the pending_restart/pg_file_settings behavior, which N1-N5 don't change (N1 is a regex-portability fix with no behavior change on a standard-conforming-strings server; N2-N5 are pins, wording and a test skip). No edit needed. scrubcheck clean on the full diff (0 hits, 17/17 controls fired).

[erikdarlingdata]

Security review, round 1 (lane P2R): #4345 / #4251

Verdict: ship after fixes. The mechanism is sound on Windows, and nothing read from pg_file_settings is stored or logged. But the grant advice widens the monitoring role and does not say so. It is also shown on targets where the grant buys nothing. A revoke costs up to an hour of lost pg_server_config snapshots, and the new predicate is wrong in two directions. H1, M1, M2 and M3 should land before this leaves draft.

Checked out at 5ca8f470. I read the diff against origin/dev file by file. I checked the PostgreSQL source for ProcessConfigFileInternal in REL_14_STABLE (guc-file.l) and REL_18_STABLE (guc.c). I ran nothing against any server.

---

High

H1. The grant advice widens the monitoring role without saying what it exposes, and it is shown where it buys nothing.

Where the advice appears:

• PerformanceMonitor.Collectors/PgFileSettingsCapability.cs:65: UnreadableCaveat. It goes out as pending_restart_caveat from DarlingMcpPgServerStateTools.cs:778 and DarlingMcpPgLoggingAuditTools.cs:139, on every call for an affected target.
• Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs:1329: the one-time Information log line.
• It is not in Darling/README.md or docs/postgres-first-target-runbook.md. An operator therefore learns about it only from the service log or from an MCP client relaying it.

What the two grants give the role:

• pg_show_all_file_settings() returns every uncommented line of postgresql.conf, postgresql.auto.conf and every included file, with its value, file path and line number.
• That includes lines that are not the running value: superseded duplicates, pending values, rejected values, and lines whose parameter name is misspelled. For example, an older primary_conninfo = '... password=...' still sitting above a newer one, or a typo such as primry_conninfo = '... password=...'.
• With pg_monitor, which the README asks for, the role already sees the running value of superuser-only settings. What these grants add is every line that is not running.
• On a self-hosted target that already has the log-reader grants (pg_read_server_files plus pg_read_file), the role can read those files anyway, so nothing new is exposed there. On every other target the exposure is new.

Where it is shown: every PostgreSQL target whose role lacks the grants, including Linux, RDS, Aurora, Azure and Cloud SQL. By PostgreSQL's process model, the stale pending_restart only happens on Windows:

• On Unix, the postmaster itself runs ProcessConfigFile(PGC_SIGHUP). set_config_option sets GUC_PENDING_RESTART in the postmaster's memory, and every later backend is forked from the postmaster and inherits the flag.
• On Windows (EXEC_BACKEND), a new backend rebuilds its settings from the postmaster's saved values, and the status flag is not among them.
• So on non-Windows targets the caveat's first sentence ("pending_restart above may under-report") is false, and it pushes a widening that fixes nothing. On RDS and Aurora the master user is not a superuser, so it most likely cannot grant on this superuser-owned view anyway (not tested).
• I did not measure the Linux side: this PR's rig and CI's PG runner are both Windows (CI unpacks pg-runtime.zip on a Windows runner). One run on a Linux PostgreSQL settles it. The first live test's Assert.False(rawPendingRestart, ...) at PgServerConfigPendingRestartLiveTests.cs:107 is the probe to run there.

What is stored or logged from the view: nothing. The EXISTS reads only name and error and yields one boolean per pg_settings row. The probe yields one boolean. No value, path or line from the view reaches the store, a log or an MCP answer. Good.

A narrower route through a product-shipped object is not available. The product ships no objects on PostgreSQL targets: its SECURITY DEFINER functions (DarlingManagedRoles, CustomAlertEvaluator, the store query-stats tools) are all on the store side. A target-side wrapper that returns only name and error would be a new install step and a design decision. I don't recommend inventing it in this PR.

Fix: do both of the following.

• (a) Scope the caveat and the log line to Windows targets. The product has no Windows fact for PostgreSQL targets today, but the capability probe can return one in the same round trip. For example, add a second column pg_catalog.version() ~* '(windows|visual c\+\+|msvc|mingw)'. Up to 16 the string reads "compiled by Visual C++ build ...", and 17+ reads "on x86_64-windows, compiled by msvc-..."; confirm both on the rig. On a non-Windows target, skip the probe, the caveat and the log line.
• (b) Where the advice is kept, say what it exposes, in all three places and in the runbook's grant section. Text to paste into UnreadableCaveat, and to adapt for the log line:

public const string UnreadableCaveat =
    "pending_restart above can under-report on this Windows PostgreSQL server: on Windows, a connection "
    + "opened after pg_reload_conf() reads pending_restart = false for a setting that is still waiting for a "
    + "restart, and this collector opens a new connection every cycle. Reading pg_file_settings closes that "
    + "gap, but it needs two grants: GRANT SELECT ON pg_file_settings and GRANT EXECUTE ON FUNCTION "
    + "pg_show_all_file_settings() to the monitoring role. Both are superuser-only by default for a reason: "
    + "they show every line of every configuration file, including lines that are not the running value "
    + "(a superseded or misspelled primary_conninfo with its password, for example) and each file's path. "
    + "Grant them only if that exposure is acceptable for this role; the collector reads nothing from the "
    + "view but a setting's name and its error text.";

---

Medium

M1. After a revoke, every pg_server_config cycle fails for up to an hour.

• PgFileSettingsCapability.Invalidate (PgFileSettingsCapability.cs:166) has no caller outside the tests.
• After a REVOKE, or after the login is switched to a role with fewer privileges, the cached "readable" verdict stands for up to CacheTtl (one hour). Each cycle sends QueryTextWithFileSettings, PostgreSQL raises 42501, and the cycle stores nothing. The whole pg_settings snapshot is lost: the outcome PgServerConfigCollector.cs:196 says "this collector must never" have.
• pg_read_binary_file already has this exact arm: DarlingCollectorRunner.ForgetStaleReadBinaryFileVerdict (DarlingCollectorRunner.cs:1365), reached from DarlingWorker.LogTailGeneralFault (DarlingWorker.cs:9268) through the general fault handler at DarlingWorker.cs:10324.

Fix: mirror it, and call it from the same general-handler spot.

/// <summary>
/// #4251: drops a stale "readable" pg_file_settings verdict when pg_server_config's file-settings query is
/// refused (42501: the grant was revoked, or the login changed, since the check), so the next cycle re-probes
/// and falls back to the pg_settings-only query instead of failing until the hour runs out.
/// </summary>
internal static void ForgetStaleFileSettingsVerdict(string collectorName, Exception ex, ServerRuntime server)
{
    if (ex is not PostgresException { SqlState: "42501" }
        || collectorName != "pg_server_config"
        || CollectorFaultCopyPhase.IsProvenStoreWrite(ex))
    {
        return;
    }

    var targetKey = ReadBinaryFileCacheKey(server);
    if (PgFileSettingsCapability.TryGetCachedVerdict(targetKey, out var readable) && readable)
    {
        PgFileSettingsCapability.Invalidate(targetKey);
    }
}

Add a unit test with three cases:

• a cached "readable" verdict plus 42501 on pg_server_config drops the verdict;
• 42501 on another collector leaves it;
• a cached "unreadable" verdict is left alone.

Fault-arm census tests count DarlingWorker's arms (CollectionLogDrainForensicsStoreTests, for example), so update them on purpose.

M2. A postmaster-context value that PostgreSQL rejected now reads as pending a restart, and a restart with it fails.

• The predicate is at PgServerConfigCollector.cs:221-223. ProcessConfigFileInternal sets error = 'setting could not be applied' whenever set_config_option returns 0.
• For a postmaster-context setting, that happens in two cases:
  • the file value is valid but different: the real pending restart, where PostgreSQL also sets GUC_PENDING_RESTART;
  • the file value fails to parse or is out of range, such as max_connections = 0 in a hand-edited postgresql.conf. parse_and_validate_value returns 0 before the pending flag is ever set.
• So s.context = 'postmaster' does not separate "pending" from "rejected", although the comment at PgServerConfigCollector.cs:198-202 says context is what tells them apart.
• PostgreSQL's own column says false for the rejected case. The new query says true, on every platform, for every superuser or granted role. The value also feeds the analysis facts (PgTargetFactCollector.Config.cs:323, PgTargetFactCollector.Posture.cs:157).
• An operator who restarts to "apply" it gets a server that refuses to start ("configuration file ... contains errors").

pg_file_settings has no column that separates the two cases, so the fix is to say so rather than claim otherwise:

• correct the comment at :198-202;
• add one sentence to get_pg_server_config's reading guide for pending_restart: "where pg_file_settings is read, true can also mean the file holds a value PostgreSQL rejected, which would stop the next start; the server log names it as 'invalid value for parameter'";
• pin it with a live case shaped like the work_mem test: append max_connections = 0, reload, and assert what is stored.

M3. On Windows, a postmaster-context setting removed from the file (ALTER SYSTEM RESET, or a deleted line) is still missed.

• When a postmaster-context setting leaves every config file, ProcessConfigFileInternal records a pg_file_settings row with no name. Its error is parameter "<name>" cannot be changed without restarting the server, built with psprintf and no _().
• It is recorded before the if (!applySettings) continue; check, so pg_show_all_file_settings() shows it. I confirmed both in REL_14_STABLE guc-file.l and REL_18_STABLE guc.c.
• The EXISTS matches on pfs.name = s.name, so that row never matches.
• A realistic case: ALTER SYSTEM RESET shared_preload_libraries (or wal_level; both are commented out in the default postgresql.conf) and a reload on Windows still read false.
• Related Low: ParseConfigFp stores the name as written (opt_name = pstrdup(yytext), no downcasing), so a hand-edited Shared_Buffers = ... misses pfs.name = s.name as well.

Fix, replacing the column in QueryTextWithFileSettings:

    (s.pending_restart
        OR (s.context = 'postmaster'
            AND EXISTS (
                SELECT 1
                FROM pg_catalog.pg_file_settings AS pfs
                WHERE (lower(pfs.name) = lower(s.name)
                       AND pfs.error = 'setting could not be applied')
                OR    pfs.error = 'parameter "' || s.name
                                  || '" cannot be changed without restarting the server'
            )))                                  AS pending_restart,

Neither string goes through _(): pstrdup("setting could not be applied") and the psprintf above, at both 14 and 18. So lc_messages does not change them. I confirmed the first requested check: the errmsg in the existing predicate is plain pstrdup, untranslated.

To pin it live, remove a postmaster-context line the rig's own postgresql.conf sets (the appended max_worker_processes, for example), reload, assert true, then restore the file and reload. If that is judged too risky for the shared rig, add an offline text pin and list the live case as not run.

---

Low

L1. Nothing offline keeps the two query texts in step. Diffing the two const strings confirms they differ only in the pending_restart column (one line becomes eight), as P2 said. But only live tests exercise the second text. A column added to QueryText alone would ship a file-settings route whose column list disagrees with PayloadColumns, and only a live run with a granted role would catch it. Paste into PgServerConfigScopeRungTests (needs System.Text.RegularExpressions):

[Fact]
public void TheFileSettingsQuery_IsTheDefaultQuery_WithOnlyThePendingRestartColumnChanged()
{
    static string Text(bool readable) => PgServerConfigCollector.Instance.BuildQuery(new CollectorContext
    {
        ServerId = -4251,
        ServerName = "pin-4251",
        CollectionTime = DateTime.UtcNow,
        Deltas = null!,
        PgFileSettingsReadable = readable,
    }).Text;

    var plain = Text(false);
    var withFile = Text(true);
    const string plainColumn = "    s.pending_restart                       AS pending_restart,";
    Assert.Single(Regex.Matches(plain, Regex.Escape(plainColumn)));

    var start = withFile.IndexOf("    (s.pending_restart", StringComparison.Ordinal);
    Assert.True(start >= 0);
    const string tail = "AS pending_restart,";
    var end = withFile.IndexOf(tail, start, StringComparison.Ordinal) + tail.Length;
    Assert.Equal(plain, withFile[..start] + plainColumn + withFile[end..]);
}

L2. The probe catches nothing, which is right for 42501 but not quite for a missing object.

• has_table_privilege and has_function_privilege never raise 42501, so there is nothing to catch narrowly.
• A failed probe is never cached as "unreadable", so an error stays distinct from "not readable". Good.
• The probe is cached per target (StorageName) for one hour. A new grant is picked up within the hour.
• The log latch fires once per target per process and never again, even across hourly re-checks. Good.
• The text-name forms raise 42P01 or 42883 when the object is missing. That would fail every pg_server_config cycle, uncached, on such a target, where today's collector works. That is unlikely on any supported PostgreSQL, since the view has been in core since 9.5.
• Cheap hardening, so that a missing object reads as "not readable":

public const string ProbeSql =
    "SELECT coalesce(pg_catalog.has_table_privilege(current_user, "
    + "pg_catalog.to_regclass('pg_catalog.pg_file_settings'), 'SELECT') "
    + "AND pg_catalog.has_function_privilege(current_user, "
    + "pg_catalog.to_regprocedure('pg_catalog.pg_show_all_file_settings()'), 'EXECUTE'), false)::text";

L3. The logging-audit note lost a space. DarlingMcpPgLoggingAuditTools.cs:140 now ends "... - statements," where dev had "... - statements, ", so the note reads "statements,locks". Restore the trailing space.

L4. The live tests assume a local Windows server.

• PgServerConfigPendingRestartLiveTests.cs:107 asserts that a fresh connection reads false. On a Linux server it should read true (see H1), so the test fails there instead of skipping.
• :162 appends to the server's postgresql.auto.conf through the local file system, which throws against any server whose data directory is not on this machine.
• Both pass in CI, whose runner is Windows.
• Fix: Assert.SkipWhen on a non-Windows version() for the first test, and on !Directory.Exists(dataDirectory) for the second.

---

Pre-existing, outside this diff (for the coordinator to file)

The collector already stores pg_settings.setting for every row. With pg_monitor that includes the running primary_conninfo on a standby. That value carries password=... whenever the standby was built with pg_basebackup -R and a password. It is stored in the monitoring store, and as a non-default setting it is part of get_pg_server_config's default answer (I found no filter that drops it). Nothing redacts it: there are no matches for primary_conninfo or redact in the collector, the reader or the tool. This PR does not introduce it, but it is the same kind of exposure H1 is about, and a larger one. Suggest a separate issue: redact password= in conninfo-valued settings at collection time.

---

CI on 5ca8f470 (the three failures P2 left unchecked)

All three passed in CI; none was skipped. CI shards classes by SHA256(class name)[0] % 3.

Test class	Shard	Result
CaptureDownChunkOrderTests	1	Passed. Shard: 4419 total, 0 failed. This PR's PgServerConfigPendingRestartLiveTests and PgServerConfigScopeRungTests also ran here and passed.
TrendPayloadBudgetLiveTests	2	Passed. Shard: 5127 total, 0 failed.
ServerListAndSummaryPlanShapeTests	0	Passed. Shard: 4681 total, 0 failed.

The other checks:

• Build, Linux build, whole-tree guards and all four Lite shards pass.
• The one red check is review (Claude Auto Review). claude-code-action exited without running Claude, so that is workflow infrastructure, not a test.

P2's three local failures were therefore its own rig or base, not this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3

[erikdarlingdata]

Security review, round 2 (lane R2): #4345 / #4251

Verdict: ship after fixes. Every round-1 item is fixed, and the H1 credential-exposure finding is closed: a non-Windows target is never asked for the grants, and every place that asks now says what they expose. But round 2's own Windows regex brings in one Medium (N1): on a target with standard_conforming_strings = off, the probe fails every cycle and pg_server_config stops collecting on that target on every platform. dev collects there today. The fix is one line. The Lows below are cheap and optional.

I checked out 14d53b98 and read the P2b and P2c commits (36291b8b, ce151ba1) file by file against round 1's text. I built Darling.Tests with 0 warnings and ran the targeted classes. For N1 and N2 I wrote the proposed fix and pins locally and proved them both ways, then restored the files. Nothing is committed. I ran nothing against any PostgreSQL server: this lane had no rig.

---

Round-1 items

H1 (a), fixed.

• The runner sets PgFileSettingsReadable = readable && isWindowsTarget (DarlingCollectorRunner.cs:1339), so a non-Windows target always gets the plain query.
• The log line is gated on isWindowsTarget && !readable (:1341).
• Both caveat readers now also require fileSettingsIsWindows (DarlingMcpPgServerStateTools.cs:681-684, and the same gate in DarlingMcpPgLoggingAuditTools.cs).
• The probe itself still runs hourly on every target. That is how the platform is learned in one round trip, and it is fine apart from N1.
• The regex is pinned offline (PgFileSettingsCapabilityTests.cs:141-153) against an x86_64 Linux string, an aarch64 Linux string (the shape RDS and Aurora on Graviton report), a pre-17 "Visual C++ build" Windows string, and the rig's 18.6 "x86_64-windows, compiled by msvc" string. That pin runs .NET's regex engine on the extracted literal, not PostgreSQL's, and that gap is how N1 got through.
• The live probe test (PgServerConfigPendingRestartLiveTests.cs:358) does run the new SQL on CI's Windows rig, so to_regclass/to_regprocedure and the ~* parse there. It asserts only the readable half.

H1 (b), fixed.

• UnreadableCaveat is round 1's text word for word.
• The log line carries the same exposure warning.
• The runbook has a new "Windows only" section with the two grants, the exposure paragraph, and the darling_monitor role name the rest of the runbook uses.

M1, fixed. The trace is confirmed, but only the method is proven.

• 42501 maps to CollectorTargetFault.Permissions (PostgresTargetProvider.cs:122).
• pg_server_config is not a log reader, so PostgresFaultOutcome returns the generic ("PERMISSIONS", ...).
• The runner's server-scoped path does not swallow it: its only catch around the read is the budget-expired filter at DarlingCollectorRunner.cs:2939.
• So the fault lands on catch (PostgresException ex) when (... is { Status: not "ERROR" } outcome) (DarlingWorker.cs:10240-10247) and reaches the new call at :10275. The call in LogTailGeneralFault (:9350) is a no-op for this fault, as P2b says.
• In practice: on a Windows target whose grant is revoked, or whose login changes, within the hour, one cycle fails and the next re-probes. That is the intended minimum.
• What proves the end-to-end path: nothing. The three new tests call ForgetStaleFileSettingsVerdict directly, so deleting line 10275 passes every test. See N2 for a pin.

M2, fixed.

• The comment at PgServerConfigCollector.cs:207-212 now says that context does not separate "pending" from "rejected".
• The reading guide has the sentence. See N3 for one inaccuracy in it.
• The live max_connections = 0 pin passes in CI.
• The analysis advice still says the opposite; see N4.

M3, fixed.

• The predicate is round 1's: lower() on both names, plus the nameless row matched by its exact error text.
• The verbatim string's "" yields ", and || binds tighter than =, so the comparison is right.
• Neither string goes through _().
• The nameless row's name is NULL, and (NULL AND x) OR true still matches it.
• The live pin passes on CI's Windows rig. There it can only pass through the new arm, because a fresh backend reads pending_restart = false on Windows.
• Two comment nits, at :202 and :204: the row's name is NULL, not "empty", and "guc-file.l:14 and :18" reads as line numbers. It means guc-file.l in 14 and guc.c in 18.

L1, fixed. The two-texts pin is round 1's, and it passes.

L2, fixed. coalesce(... to_regclass ... to_regprocedure ..., false). It runs live in CI through the probe test.

L3, fixed. The space is restored.

L4, fixed, with one gap (N5).

• The first test skips on a version() without "Windows".
• All three file-editing tests skip when the data directory is not local.

---

New findings

N1 (Medium). With standard_conforming_strings = off, the probe fails every cycle and pg_server_config never collects. This is on every platform, and dev collects there today.

• Where: PerformanceMonitor.Collectors/PgFileSettingsCapability.cs:76: ~* '(windows|visual c\\+\\+|msvc|mingw)'. It is the only backslash-bearing regex literal I found in the collectors or the service.
• Scenario: the setting is off server-wide, or through ALTER ROLE/ALTER DATABASE ... SET. It is deprecated but still supported, and the product guards against it elsewhere (StoreStatementStats, HypotheticalIndexExperiment).
  • With it off, PostgreSQL lexes a plain '...' literal in its escape state (scan.l xe), and \+ becomes +.
  • The pattern becomes visual c++. The second + lands in atom position, and regcomp.c's parseqatom raises REG_BADRPT ("invalid regular expression: quantifier operand invalid", SQLSTATE 2201B).
  • IsReadableAsync throws before anything is cached, and ResolvePgFileSettingsReadableAsync (DarlingCollectorRunner.cs:1333) does not catch it.
  • 2201B classifies Unclassified, so every hourly pg_server_config cycle logs ERROR and stores nothing.
  • The probe runs on Linux, RDS and Aurora targets too, so this is not Windows-scoped.
• Evidence: from PostgreSQL source (scan.l, regcomp.c REL_17_STABLE), not run, since this lane had no rig.
• Fix to paste. A bracket expression has no backslash, so it reads the same under either setting:

        + "|| ':' || (pg_catalog.version() ~* '(windows|visual c[+][+]|msvc|mingw)')::text";

Also change the doc comment at :64 to (windows|visual c[+][+]|msvc|mingw). Then add this pin to PgFileSettingsCapabilityTests. I verified that it fails on 14d53b98, and that it passes with the fix, as do the existing regex theory and all 23 tests in the class:

    /// <summary>#4251 round-2 review: no backslash anywhere in the probe. Under standard_conforming_strings =
    /// off (server-wide, or ALTER ROLE/DATABASE ... SET) PostgreSQL reads a backslash-plus in a plain literal as
    /// a bare plus, the regex becomes "visual c++", and it fails to compile (2201B, quantifier operand invalid)
    /// on every cycle, uncached, so pg_server_config never collects on that target. A bracket expression reads
    /// the same under either setting.</summary>
    [Fact]
    public void TheProbeCarriesNoBackslash_SoItParsesTheSameUnderEitherStringSetting()
        => Assert.DoesNotContain('\\', PgFileSettingsCapability.ProbeSql);

Optional live pin (not run): in PgServerConfigPendingRestartLiveTests, just before IsReadableAsync(roleConnection, ...) at :358, run SET standard_conforming_strings = off on roleConnection. By the source above it should raise 2201B on 14d53b98 and pass with the fix. It would also prove that the plain collector query still reads the full snapshot in such a session.

N2 (Low). Nothing pins M1's real landing spot.

• Where: DarlingWorker.cs:10275.
• Scenario: a refactor that drops or moves that line passes every test, and the hour-long loss that M1 fixed comes back.
• Pin to paste into PgFileSettingsCapabilityTests. It needs using System.IO; and using System.Runtime.CompilerServices;. I verified that it fails with the line commented out and passes on the head:

    /// <summary>#4251 round-2 review: a pg_server_config 42501 classifies PERMISSIONS, so it lands on the
    /// worker's PostgreSQL PERMISSIONS arm and never reaches the general handler. The tests above prove the
    /// method; this pins the call on the arm that actually fires, which nothing else would miss.</summary>
    [Fact]
    public void APgServerConfig42501_LandsOnThePermissionsArm_AndThatArmForgetsTheVerdict()
    {
        Assert.Equal("PERMISSIONS", DarlingWorker.PostgresFaultOutcome(Pg("42501"), "pg_server_config").Status);

        var worker = File.ReadAllText(Path.Combine(
            RepoRoot(), "Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"));
        var arm = worker.IndexOf("is { Status: not \"ERROR\" } outcome)", StringComparison.Ordinal);
        Assert.True(arm >= 0, "the PostgreSQL PERMISSIONS arm moved; re-point this pin");
        var nextCatch = worker.IndexOf("catch (", arm, StringComparison.Ordinal);
        /* Anchored at the start of a line, so a commented-out call does not satisfy it. */
        Assert.Matches(
            new Regex(@"^\s*DarlingCollectorRunner\.ForgetStaleFileSettingsVerdict\(collectorName, ex, runtime\);",
                RegexOptions.Multiline),
            worker[arm..nextCatch]);
    }

    private static string RepoRoot([CallerFilePath] string thisFile = "")
    {
        var dir = Path.GetDirectoryName(thisFile);
        while (dir is not null && !File.Exists(Path.Combine(dir, "PerformanceMonitor.sln")))
        {
            dir = Path.GetDirectoryName(dir);
        }

        Assert.NotNull(dir);
        return dir!;
    }

N3 (Low). The reading guide names a log line that the pinned case does not produce.

• Where: DarlingMcpPgServerStateTools.cs:619, the sentence "...the server log names it as 'invalid value for parameter'".
• Scenario: parse_and_validate_value logs "invalid value for parameter" only when the value does not parse. An out-of-range value, which is this PR's own max_connections = 0 case, logs "0 is outside the valid range for parameter "max_connections" (1 .. 262143)". Both messages are also translated through lc_messages. An agent that searches the log for the quoted phrase finds nothing.
• Fix: end the sentence at "...which would stop the next start; the server log names the rejected setting." It is shorter, so the description budget pins are safe.

N4 (Low). The analysis advice still tells the operator that a rejected value "takes effect at the next restart".

• Where: PerformanceMonitor.Analysis/PgTargetAdvice.Buffer.cs:273.
• Scenario: on a Windows target with both grants, a hand-edited out-of-range shared_buffers now stores pending_restart = true. The advice then says "a different value is in the file and takes effect at the next restart". That restart fails. This is the harm round 1's M2 named, but the M2 fix covered only the MCP reading guide.
• Fix:

            sb.Append("The snapshot reports pending_restart for this setting: the file holds a different value than the " +
                      "running server. It takes effect at the next restart, unless PostgreSQL rejected it, in which case " +
                      "the server log names it and that restart would fail. ");

The Posture text ("check which value is actually in force") is already neutral. The Sessions text is pinned at PgTargetSessionsTests.cs:444 and :890, and it states only the running ceiling, so I would leave it.

N5 (Low). L4's skip misses pre-17 Windows.

• Where: PgServerConfigPendingRestartLiveTests.cs:93: versionText.Contains("Windows", ...).
• Scenario: a PostgreSQL 16-or-older Windows version() says "compiled by Visual C++ build" and never "Windows". On such a rig, #4251's own core repro skips silently instead of running. CI's rig is 18.x, so CI is unaffected today.
• Fix: test with the same pattern the product uses:

        Assert.SkipWhen(!Regex.IsMatch(versionText, "(windows|visual c[+][+]|msvc|mingw)", RegexOptions.IgnoreCase),
            "This #4251 repro only reproduces on Windows (EXEC_BACKEND); the target's version() does not say Windows.");

---

What else the fixes could have broken

• MCP description budget, fault-arm census and pin tests. I built Darling.Tests with 0 warnings and ran PgFileSettingsCapabilityTests, PgServerConfig*, PostgresFaultOutcomeTests, McpToolGuideHead*, McpPayloadContractCensus*, McpPageContractTests, DocCommentHygiene*, DarlingMcpPgLoggingAuditToolsTests, PgTargetMcpSurfaceTests, DarlingCoreToolProfileTests, DarlingMcpToolsTests, CollectionLogDrainForensics*, *Census*, *Inventory*, *Budget* and *FaultArm*. Total 734, Failed 0, Skipped 41: the live classes, since no DARLING_TEST_PG was set.
• This PR's CI on 14d53b98 (run 36188459104): all three Darling PG shards pass, with totals 4423, 5179 and 4707 and 0 failed. None of the new live tests appears as skipped.
• Cache and log latch. Both facts live in one cache entry.
  • ResolvePgFileSettingsReadableAsync reads the Windows half with a second TryGetCachedVerdict. If an Invalidate or Reset lands between the two reads, that cycle reads non-Windows: the plain query and no log line, which fails safe.
  • A malformed scalar reads false for both facts.
  • The latch is now used only for Windows plus unreadable, so a non-Windows target never uses it. That is correct.
  • Keying is unchanged from round 0: the runner uses StorageName and the readers use resolved.ServerName.

TrendPayloadBudgetLiveTests on dev

It does not fail on dev.

• dev's latest Build run (36186703566, bcf18734) failed one test in shard 1: ComposeStoreRolesLiveTests.AStartThatDoesNotProvision_KeepsEachSurfaceOnItsRole_WithAnEarlierStartsCredential_Gated (Total 4417, Failed 1).
• The last completed dev run before it (36182207396) was green, and the three runs between were cancelled.
• TrendPayloadBudgetLiveTests also passed in this PR's own CI run on 14d53b98.

So P2b's local failure was not "pre-existing on dev". Its rig or a reused test database is the likelier cause. CI is green, so it does not block this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3

[erikdarlingdata]

Ruling on review round 2 (coordinator)

Review: pullrequestreview-5323311272 (lane R2, head 14d53b9). Every round-1 item is fixed, and the High is closed. N1-N5 are accepted as written, with R2's pasted code:

• N1 (Medium): write the probe's pattern as '(windows|visual c[+][+]|msvc|mingw)', so it reads the same with standard_conforming_strings on or off. Add R2's pin, which fails on 14d53b9.
• N2: R2's pin that the permission-error handler (DarlingWorker.cs ~10275) calls ForgetStaleFileSettingsVerdict.
• N3: end the reading-guide sentence at "...which would stop the next start; the server log names the rejected setting."
• N4: R2's replacement text for PgTargetAdvice.Buffer.cs:273.
• N5: the live test's skip uses the product's pattern.

One fix lane does all five. They are mechanical and pasted, so there is no third review round (the two-round cap holds). The lane runs every class in each touched test file, the MCP description budget pins and TrendPayloadBudgetLiveTests on a fresh database, then merges when CI is green. TrendPayloadBudgetLiveTests passes on dev and in this PR's CI; P2b's local failure came from its rig, not the code.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3