Decode the loader status instead of printing a raw Win32 number (#2186)

[erikdarlingdata]

Closes #2186.

The report

A managed-Postgres bootstrap failed with this, and nothing else:

Managed Postgres bootstrap failed: initdb failed (exit code -1073741515) for C:\ProgramData\PerformanceMonitorDarling\pg. Output:

-1073741515 is 0xC0000135 = STATUS_DLL_NOT_FOUND. Windows killed initdb.exe in the loader, before a line of its own code ran, which is also why Output: is empty and always will be for this class of failure: the one field an operator reads is guaranteed blank exactly when the failure is a load failure. It reads as "no information available" rather than "this is a loader failure", and attention went to the follow-on missing-credential message and darling.json, neither of which was the fault.

Verifying the issue's claims first

Both held. InitializeClusterAsync interpolated the raw signed code (DarlingManagedPostgres.cs:891 on dev), and the #1738 sibling at line 1103 names STATUS_DLL_NOT_FOUND in a comment only - there was no decoder anywhere in the file to reuse, so this adds one rather than moving one.

One claim was worth testing rather than assuming: that the empty Output: really is guaranteed. A test drives a genuine loader failure through the real RunToolAsync and pins it - exit code -1073741515, captured output length 0.

Before / after, verbatim

Produced by a real Windows loader failure driven through the real EnsureRunningAsync, not a hand-written string: a scratch pg-runtime whose initdb.exe is a binary whose app-local DLL cannot be resolved (a doctored PATH), which Windows kills with 0xC0000135 and zero bytes on both streams. Nothing touched DARLING01 or the repo's pg-runtime.zip.

Before (dev):

initdb failed (exit code -1073741515) for C:\...\repro\pg. Output:

After (this branch):

initdb failed (exit code -1073741515 = 0xC0000135 STATUS_DLL_NOT_FOUND) for C:\...\repro\pg.
Windows killed initdb.exe in the LOADER: it never ran a line of its own code. This is a Windows failure, not a PostgreSQL one, and it is why the captured output is empty - for a loader failure an empty Output is EXPECTED, not missing information.
Two causes account for nearly all of these:
  (1) The Microsoft Visual C++ runtime is missing from C:\...\repro\pg-runtime\pgsql\bin. Packaging bundles vcruntime140.dll, vcruntime140_1.dll and msvcp140.dll there so the box needs no prerequisite - if any of the three is absent the install tree is a partial or damaged extract, so redeploy the package, or install the Microsoft Visual C++ 2015-2022 x64 redistributable.
  (2) The service account cannot read the install tree. The service runs as the virtual account NT SERVICE\PerformanceMonitor Darling, which is neither you nor Administrators, so an install under a user profile (Desktop, Downloads, anywhere below C:\Users) is unreadable to it - reinstall to a machine-scoped path such as C:\PerformanceMonitorDarling.
Two checks that tell them apart:
  (a) Run "C:\...\repro\pg-runtime\pgsql\bin\initdb.exe" --version from an elevated prompt. Failing there too means (1). Succeeding points at (2) - and note that these tools re-execute themselves under a restricted token that drops the Administrators group, so a tree readable only VIA Administrators still fails the real run even when it runs by hand.
  (b) Event Viewer > Windows Logs > Application, at the time of the failure: an Application Error or SideBySide entry usually names the exact module that could not be loaded.
Output:
(none - Windows ended the process before it could write anything, which is expected for this failure rather than missing information)

The leading clause is deliberately unchanged: existing field reports and the issue tracker are searchable by initdb failed (exit code, so the fix must not rename what operators paste into search.

The restricted-token caveat in check (a) is in there because the field thread's most confusing evidence was initdb --version succeeding by hand while the service run failed. PostgreSQL's frontend utilities re-execute themselves via get_restricted_token() / CreateRestrictedProcess with the Administrators and Power Users groups removed, so a tree readable only via Administrators fails the real run while passing the by-hand check (paquier.xyz, PG source). Verified before shipping the claim, and it is attached only to tools that actually do this (initdb, pg_ctl, pg_upgrade) - never to vacuumdb.

What changed

DarlingToolExitCode decodes any exit code at or above 0xC0000000 - the NTSTATUS error range, which no real program exits with deliberately - so an unlisted status is not left as opaque as -1073741515 was. It separates loader statuses (0xC0000135, 0xC0000139, 0xC000007B, 0xC0000142, 0xC0000022) from crashes (0xC0000005, 0xC0000374, 0xC0000409); a crash must not send anyone hunting for missing DLLs, which would be the same wrong-direction error pointed somewhere new.

Wired into every bundled-binary failure, per the issue's "or any bundled Postgres binary": initdb (bootstrap and upgrade), pg_ctl start/status/stop/reload, pg_upgrade and its --check, and the post-upgrade analyze.

Two messages also stopped blaming the wrong thing, which the decode made visible:

• pg_ctl status said "the data directory is not usable" for every unexpected code. That is pg_ctl's own exit 4 talking; on a Windows status it pointed an operator at deleting a healthy store to fix a missing DLL. The verdict is now conditional.
• pg_upgrade --check reported "the clusters are not compatible" on any non-zero exit. Only pg_upgrade can reach that verdict, so on a Windows status it was invented. NOTHING has been changed still holds either way.

And the #1738 refusal asserted "its binaries did not run" while discarding the one piece of evidence it held - the probe's own exit code died in a local. ReadRuntimeMajorAsync now carries it out, and the refusal quotes and decodes it.

Tests

Watched red first against an inert seam (the new API returning today's behavior): 16 failed, 9 passed, then green. The 9 that passed under the inert seam are the ones asserting today's correct behavior is preserved - ordinary exit codes stay bare, real output passes through untouched - so the red was the decode, not the harness.

• DarlingToolExitCodeTests - the decode, both diagnosis families, the empty-output note, that ordinary codes (initdb's 1, pg_ctl status's 3) are left completely alone, and the live RunToolAsync pin above.
• DarlingManagedPostgresTests - the shipped messages: the field report rebuilt from its own numbers, the conditional data-directory verdict, and that the start failure diagnoses before it points at a server log that a loader failure guarantees is absent. Plus a wiring pin at the source, because three correct builders that no throw site calls is precisely what Runtime advance has no direction check: a package with an OLDER PostgreSQL major replaces a working newer runtime and the store cannot start (hit on DARLING01) #1738 already was, and behavioral coverage cannot reach it - reproducing it needs a bundled Postgres that dies in the Windows loader, which is not something a CI runner can be asked to arrange.

Full Darling.Tests: 4256 passed, 0 failed, 236 skipped (all gated). Service project builds 0 Warning(s). The two warnings in a full rebuild (AbandonableStep.cs CA1068, NpgsqlRootCertificateValidationTests.cs CA2022) are in files this branch does not touch.

Not in scope

The installer half is #2187.

🤖 Generated with Claude Code

[claude]

Reviewed. This is a self-contained Darling-only C# change (DarlingToolExitCode.cs + call sites in DarlingManagedPostgres.cs/DarlingStoreUpgrade.cs, plus tests) — no T-SQL touched, so the style guide doesn't apply here, and Lite has no managed-Postgres bootstrap code at all, so there's no Lite/Darling parity gap to flag.

Went through the decode/diagnose/format logic in detail:

• NtStatusErrorFloor (0xC0000000) correctly scopes to the NTSTATUS "severity=Error, customer bit unset" range — ordinary positive tool exit codes (max 0x7FFFFFFF) can never cross it, and the threshold is applied consistently across Describe, Diagnose, and FormatOutput.
• Traced every rewritten throw site (BuildInitDbFailureMessage, BuildStatusFailureMessage, BuildStartFailureMessage, the pg_upgrade --check/pg_upgrade messages, the Runtime advance has no direction check: a package with an OLDER PostgreSQL major replaces a working newer runtime and the store cannot start (hit on DARLING01) #1738 refusal) against their tests — message assembly, ordering (diagnosis before "Server log tail:"), and the conditional "data directory is not usable" / "clusters are not compatible" verdicts all match what's asserted.
• The tuple change to ReadRuntimeMajorAsync ((int? Major, int ExitCode)) is threaded through both call sites correctly, including the revert-path caller that only needs .Major.
• No downstream code parses these exception/log strings by exact format (checked DarlingCliCommands.cs, Viewer), so appending the decoded status doesn't break anything.
• Structured logging call sites (pg_ctl reload, stop, post-upgrade analyze) have matching placeholder/argument counts.

No correctness, security, or performance issues found. One very minor, non-blocking nit: STATUS_CONTROL_C_EXIT (0xC000013A) falls through to the generic "Terminated" diagnosis text, which points the operator at Event Viewer — but a Ctrl-C-style termination typically doesn't leave an Application-log entry the way a loader failure or crash does. Low value fixing unless that status is actually reachable in practice.

Also noted: the CHANGELOG.md hunk in this PR shows as a near-total file rewrite (2676 insertions / 2674 deletions) in gh pr diff. Spot-checked several "removed" vs "added" lines byte-for-byte and they're identical — this looks like a diff-rendering artifact from the algorithm losing anchor points on a large file of long near-duplicate bullet lines, not an actual content change. The real delta is the new [#2186] bullet and its reference-link line, which read fine and follow the existing entry style.

[erikdarlingdata]

Heads up from the other work stream: this PR's Darling Linux build failure is not yours. It's a repo-wide break I've fixed in #2199.

The Dockerfile built from the floating mcr.microsoft.com/dotnet/sdk:10.0 tag, that tag advanced to an image shipping SDK 10.0.400, and global.json requests 10.0.302 with rollForward: latestPatch — which only rolls inside the 3xx band. So the in-container publish dies with A compatible .NET SDK was not found, surfacing as a bare exit code: 155 with no compiler diagnostics, which reads like a code failure and isn't one.

Your PR is the control that proved it: an unrelated change failing the identical step the same way. dev's own Linux build passed at 18:22 and would fail on its next run.

#2199 pins the build stage to sdk:10.0.302. Once that merges, rerun this job and it should go green with no change on your side.

[claude]

Minor: this changes the {ExitCode} structured-logging field from an int to a decorated string (e.g. "3" vs "-1073741515 = 0xC0000135 STATUS_DLL_NOT_FOUND"). Same pattern at the pg_ctl reload failed log (line ~2075) and the Post-upgrade analyze reported log in DarlingStoreUpgrade.cs (line ~2252).

For the exception-message call sites this is clearly the point of the PR, but these three are ILogger message templates, which typically flow into structured log sinks (Seq/App Insights/etc.) where ExitCode may be queried/alerted on as a number (e.g. ExitCode != 0, aggregations, dashboards). Collapsing it to a mixed-format string could silently break those queries downstream.

If that's a real consumer, consider keeping the raw int bound to {ExitCode} and adding the decoded text as a separate {ExitCodeDescription}/{Diagnosis}-style parameter instead of replacing the numeric field.

[claude]

Review

Scoped entirely to the Darling service (DarlingManagedPostgres.cs, DarlingStoreUpgrade.cs, new DarlingToolExitCode.cs, plus tests/CHANGELOG). No T-SQL touched.

Lite/Darling parity: No drift to flag. Lite has no managed-Postgres bootstrap at all (embedded DuckDB, no bundled Postgres runtime, no initdb/pg_ctl/pg_upgrade process spawning anywhere under Lite/), so there's no counterpart for this change to be out of sync with.

Correctness: Traced the decoder (DarlingToolExitCode.Describe/Diagnose/FormatOutput) and every call site against the new tests — the message-builder tests (BuildInitDbFailureMessage, BuildStatusFailureMessage, BuildStartFailureMessage) match the actual concatenation logic exactly, including the conditional " — the data directory is not usable." wording in BuildStatusFailureMessage and the diagnosis-before-log-tail ordering in the start-failure and pg_upgrade paths. The NTSTATUS-floor check (0xC0000000) is a clean discriminator given these tools only ever exit with small values on their own. ReadRuntimeMajorAsync's new (Major, ExitCode) tuple is only consumed for the refusal message when MustRefuseUnidentifiableRuntime is true, which per DarlingStoreUpgrade.cs:715-716 only fires when runtimeMajor is null — i.e. exactly when probeExitCode is guaranteed non-zero and meaningful. Didn't find any remaining bootstrap throw site still interpolating a bare exitCode (confirmed via the same grep the new TheBootstrapThrowSitesActuallyUseTheseMessages test pins).

Security: No injection surface — exePath/dataDirectory values are locally-derived filesystem paths used only in log/exception text, not passed to a shell. No secrets touched.

Performance: Negligible — string formatting only on already-slow failure/log paths, nothing in a hot path.

One inline note on a structured-logging field-type change (int → decorated string) in three ILogger calls that's worth a second look, but otherwise this is a tightly-scoped, well-tested fix.

[erikdarlingdata]

#2199 is merged, so the SDK pin is on dev now. Your Darling Linux build was already re-running when I checked; if it's still red, rebase onto dev to pick up the pinned sdk:10.0.302 and it'll clear — a plain rerun only helps if the job re-resolves the base, which it doesn't for the container step.

Confirmed the fix works before merging it rather than after: Darling Linux build went green on #2199 while it was still red on this PR, which isolates the cause to the floating tag.

[erikdarlingdata]

Status: green on everything this PR can affect, blocked by a repo-wide CI break it did not cause

Final sha c1e75ebf (merge of origin/dev, resolving a whole-file CHANGELOG.md conflict - see below).

statusCheckRollup:

check	result
build (Windows, full test suites)	success
Darling PostgreSQL tests	success
Check pull request target branch	success
Claude Auto Review	success
Darling Linux build	failure - #2200, not this branch

mergeable: MERGEABLE (mergeStateStatus: BLOCKED, i.e. awaiting the failing required check).

The Linux failure is #2200, filed separately

It fails before compiling anything:

Requested SDK version: 10.0.302
Install the [10.0.302] .NET SDK or update [/src/global.json] to match an installed SDK.
10.0.400 [/usr/share/dotnet/sdk]

Darling/Dockerfile:16 uses the floating tag mcr.microsoft.com/dotnet/sdk:10.0, which rolled to SDK 10.0.400; global.json pins 10.0.302 with rollForward: latestPatch, so the 4xx band is correctly refused. Four things say it is not this branch:

• sha 9a60f423 on this same PR passed this exact job; c1e75ebf adds only a CHANGELOG.md line plus a dev merge, and neither global.json nor Darling/Dockerfile differs between them.
• dev's own Build passed at 18:22 UTC and the next run failed at 18:38, with no repo change in between.
• Re-running the failed job reproduces it identically, so it is not a flake.
• In the same run, the Windows build job and Darling PostgreSQL tests both pass - only the container publish fails.

#2200 has the diagnosis and the suggested one-line fix (pin the Dockerfile tag to the band global.json names, which is what #1758 intended and the one place still floating). This PR is unmergeable until that lands, through no change of its own.

On the CHANGELOG conflict

It came back as a single whole-file conflict rather than the usual both-added-an-entry kind: dev's copy is CRLF, and my original edit had silently normalized the file to LF, so both sides looked entirely rewritten. Resolved by taking dev's file wholesale and re-inserting the entry and link-ref with CRLF preserved. git diff origin/dev HEAD --stat -- CHANGELOG.md is now 2 insertions, and dev's new #2190 entry sits intact right below the new one.

Also filed

#2197 - the follow-on missing-credential CLI message still reads as a first run even after a bootstrap has already failed. That is the half of #2185's misdirection that lives in DarlingCliCommands.cs, deliberately left out of this PR: different file, different surface, and it was under concurrent edit.

Verified locally on the merged tree

Full Darling.Tests: 4265 passed, 0 failed, 236 skipped (all gated). Service project builds 0 Warning(s); the two warnings in a full rebuild are in files this branch does not touch and are tracked in #2193.

[claude]

Nit: every other Windows-only gate in this test suite uses Assert.SkipUnless(OperatingSystem.IsWindows(), "...") (see DarlingConfigTests, DarlingFileSecurityTests, DarlingPgRuntimeVersionPinTests, QueryStoreSliceRepairLiveTests). This test instead early-returns, so on non-Windows CI it reports as passed with zero assertions run instead of skipped. That's a quiet deviation from the "236 skipped (all gated)" accounting the PR description relies on, and it means a future regression here wouldn't show up as a gap in the test report on Linux CI — it would just look like a passing test. Worth switching to the established Assert.SkipUnless pattern for consistency:

Assert.SkipUnless(OperatingSystem.IsWindows(), "requires the real Windows loader to reproduce a loader-kill exit status.");

[claude]

Review summary

Reviewed the diff (CHANGELOG.md, DarlingToolExitCode.cs (new), DarlingManagedPostgresTests.cs, DarlingToolExitCodeTests.cs (new), DarlingManagedPostgres.cs, DarlingStoreUpgrade.cs) against the PR description's claims.

Scope check: this is Darling-only (managed-Postgres bootstrap/upgrade). Lite has no equivalent managed-Postgres code path (grep for initdb/pg_ctl/pg_upgrade under Lite/ returns nothing), so there's no Lite/Darling parity drift to flag here.

Correctness spot-checks, all held up:

• DarlingToolExitCode.Describe/Diagnose/FormatOutput correctly discriminate on the NTSTATUS error floor (0xC0000000), and every call site that previously interpolated a raw {exitCode} now routes through Describe (verified via grep — no leftover raw interpolations in DarlingManagedPostgres.cs or DarlingStoreUpgrade.cs).
• The vacuumdb post-upgrade analyze path correctly uses only Describe/FormatOutput, never Diagnose — matches the PR's claim that the loader diagnosis is "never attached to vacuumdb."
• ReadRuntimeMajorAsync's new (int? Major, int ExitCode) return is threaded correctly through both call sites, including the .Major ?? 0 fallback after a runtime revert.
• The MustRefuseUnidentifiableRuntime message is accurate for both failure shapes it can be reached from: a non-zero probe exit code, and a zero exit code with unparseable pg_ctl --version output (in the latter case Diagnose(0, ...) correctly returns "", so no spurious loader boilerplate is appended).
• RunToolAsync's output-capture path confirms the "empty Output: is guaranteed for a loader kill" premise the whole PR rests on.
• Structured logging call sites (e.g. the pg_ctl reload LogCritical) have matching placeholder/argument counts after the edit.

Left one inline nit on a test-convention deviation (early-return instead of Assert.SkipUnless for a Windows-only test) — nothing that blocks merge.

No missing-index DMV suggestions, no security concerns (no new user input reaches these code paths — exePath/_dataDirectory are internally controlled), no performance regressions (this is exclusively string-building on already-failing paths).

[erikdarlingdata]

Fully green

Merged the current dev (which carries #2199's container SDK pin, the fix for the repo-wide break described in my previous comment). Final sha 8cda1626, mergeable: MERGEABLE, mergeStateStatus: CLEAN.

statusCheckRollup:

check	result
build	SUCCESS
Darling PostgreSQL tests	SUCCESS
Darling Linux build	SUCCESS
Claude Auto Review	SUCCESS
check-branches	SUCCESS

#2200 is closed as a duplicate of #2199.

CHANGELOG byte check

Flagged by a sibling lane: CHANGELOG.md is CRLF and carries 6 bare CR bytes, so an editor pass silently normalizes them and turns a 2-line entry into a ~5,350-line diff. Verified this branch against origin/dev on both measures, before and after the merge:

• git diff origin/dev HEAD --stat -- CHANGELOG.md → 2 insertions, unchanged.
• Byte census: this branch has 6 bare CR, 0 bare LF; origin/dev has 6 bare CR, 0 bare LF. Identical. The file is 2,118 bytes larger, which is exactly the entry plus the link-ref.

The earlier whole-file conflict was this hazard, caught and resolved by taking dev's copy and re-inserting with a script that splits and joins on CRLF (which preserves the bare CRs, since they sit inside the split line strings rather than at the boundaries) rather than saving the file from an editor. file CHANGELOG.md alone would not have caught it - it reports "CRLF" while saying nothing about whether the 6 bare CRs survived.

Not merging, per instructions.

[claude]

Reviewed the diff (CHANGELOG.md, DarlingToolExitCode.cs, DarlingManagedPostgres.cs, DarlingStoreUpgrade.cs, and the two test files).

Correctness — Traced the NTSTATUS-range logic in DarlingToolExitCode (0xC0000000 floor, unchecked((uint)exitCode) reinterpretation, the known-status dictionary and its Loader/Crash/Terminated kinds) against every call site and the new tests. It's consistent: ordinary tool exit codes (initdb's 1, pg_ctl status's 3/4) are left bare everywhere, the loader diagnosis is only appended for the five listed loader statuses, crashes get the separate "don't go hunting for DLLs" paragraph, and FormatOutput only claims the empty-output-is-expected story for actual Windows statuses. Grepped for any remaining raw {exitCode} interpolations outside structured-logging templates (where the placeholder is fed Describe(...)) and found none — every RunToolAsync/RunDetachingToolAsync call site in both files now routes through the decoder. The EnsureDataDirectoryMajorAsync tuple refactor (ReadRuntimeMajorAsync now returning (Major, ExitCode)) is wired correctly at both call sites, including the revert path that only wants .Major.

Lite/Darling parity — No parity gap. Lite has no managed-Postgres component (it embeds DuckDB, not a bundled PostgreSQL binary), and its only Process.Start usages are shell-execute for opening URLs/files, so there's no analogous exit-code-decoding surface to keep in sync. No Lite files are touched by this PR, which is correct.

Security — All the paths fed into Diagnose/exception messages (exePath, dataDirectory) come from internally constructed Path.Combine values, not external input, and are only ever used for string formatting/logging, never as shell input. Nothing new here.

Performance — No concerns; this is exception/log-message construction on failure paths only.

One very minor (non-blocking) nit: in EnsureDataDirectoryMajorAsync's refusal message (DarlingManagedPostgres.cs), the untouched sentence "...so the service is stopping here rather than failing deeper with a Win32 error code" now sits just before Diagnose(...) appends an actual decoded Win32/NTSTATUS code to the same message — slightly redundant now that the code is shown, though not incorrect.

Nothing else stood out. The test coverage here is unusually thorough (the red-then-green methodology, the live loader-failure repro, and the source-text wiring pin that guards against a repeat of #1738's "correct code nothing calls" failure mode).

[erikdarlingdata]

Green again on top of #2198

Merged origin/dev at 44fd7288 (which carries #2198). Final sha 9558744f, mergeable: MERGEABLE, mergeStateStatus: CLEAN.

statusCheckRollup:

check	result
build	SUCCESS
Darling PostgreSQL tests	SUCCESS
Darling Linux build	SUCCESS
Claude Auto Review	SUCCESS
check-branches	SUCCESS

Full Darling.Tests locally on the merged tree: 4271 passed, 0 failed, 236 skipped (all gated) - up 6 from the previous round, which is #2198's new DarlingInstallLocationTests.

The CHANGELOG conflict was one line this time, and that is the point

Last round it came back as a ~5,350-line whole-file conflict. This round it was a single 5-line hunk at the link-ref block - [#2186] on my side against [#2187] on dev's - because the byte-precise resolution held and there was no longer a competing normalization for git to see. Resolved keep-both-sides, ours then theirs, so both link-refs survive alongside every one already there.

Verified on both measures, as before:

• git diff origin/dev HEAD --stat -- CHANGELOG.md → 2 insertions.
• Byte census: this branch 6 bare CR / 0 bare LF; origin/dev 6 bare CR / 0 bare LF. Delta is 2,118 bytes, identical to the previous round, which is exactly my entry plus link-ref and nothing else.
• Zero conflict markers remain, and both prose entries are present - mine and Refuse an install location the service account can never read (#2187) #2198's "The installer refuses an install directory the service could never read".

Resolution was scripted, not editor-saved: split and join on CRLF, which leaves the 6 bare CRs untouched inside the line strings.

Not merging, per instructions.

[claude]

Reviewed this against correctness, Lite/Darling parity, security, and performance.

Scope check: all changes are confined to Darling/PerformanceMonitor.Darling.Service/{DarlingManagedPostgres,DarlingStoreUpgrade}.cs and the new DarlingToolExitCode.cs, plus their tests. This is managed-PostgreSQL bootstrap/lifecycle code with no Lite/ counterpart (no file under Lite/ references initdb, pg_ctl, pg_upgrade, or ManagedPostgres), so there is no parity drift to flag here.

Correctness:

• Describe/Diagnose/FormatOutput are pure, side-effect-free, and every call site that changed was checked for placeholder/arg-count matches (the reload-failure log went from 2 to 4 args, stop from 2 to 3, analyze from 2 to 3 - all line up).
• SafeFileName/SafeDirectoryName guard Path.GetFileName/GetDirectoryName against ArgumentException so a malformed path cannot turn a diagnostic into a second, more confusing exception.
• The Diagnose empty-string sentinel (used to decide whether to keep the old "data directory is not usable" / "clusters are not compatible" wording) is exercised by both the unit tests and a source-grep "wiring" test (TheBootstrapThrowSitesActuallyUseTheseMessages) that asserts the throw sites actually call the new builders and that no bootstrap failure regressed back to a bare exit code {exitCode} interpolation - a good guard against the exact "correct code nothing calls" failure mode the PR description calls out from Runtime advance has no direction check: a package with an OLDER PostgreSQL major replaces a working newer runtime and the store cannot start (hit on DARLING01) #1738.
• vacuumdb (post-upgrade analyze) correctly gets only Describe/FormatOutput, never Diagnose - matches the PR claim that the loader diagnosis is intentionally not attached to it.
• The NTSTATUS-range heuristic (>= 0xC0000000) is sound for the tools it is actually wired to (initdb/pg_ctl/pg_upgrade/vacuumdb all use small non-negative exit codes per their own docs), so there is no practical risk of misclassifying a legitimate tool exit as a Windows status today. Worth keeping in mind only if this decoder is ever pointed at a tool that uses negative exit codes as its own convention (e.g. -1 for generic failure), since that would also land in the NTSTATUS range.

Security: no new process/file/network surface - exePath/dataDirectory interpolated into these messages are internally-derived paths (runtime root plus fixed relative segments), not user input, so there is no injection concern in the diagnostic strings themselves.

No blocking issues found. The test coverage (decoder pins, shipped-message pins, and a source-wiring pin) is notably thorough for this kind of change.

[claude]

Review

Reviewed the diff for correctness, Lite/Darling parity, security, and performance. Summary: no blocking issues found.

Scope check (parity): This PR only touches Darling/PerformanceMonitor.Darling.Service and its tests — the managed-PostgreSQL bootstrap (initdb/pg_ctl/pg_upgrade). Lite stores to embedded DuckDB and has no equivalent code path (confirmed via a repo-wide search for ManagedPostgres/initdb/pg_ctl under Lite/ — no hits), so there is nothing for Lite to mirror here. No parity drift.

Correctness:

• DarlingToolExitCode.Describe/Diagnose/FormatOutput all gate on the same unchecked((uint)exitCode) >= 0xC0000000 NTSTATUS-error-severity check, so a code is classified consistently across all three helpers — no risk of e.g. Describe calling something a Windows status while FormatOutput treats it as ordinary.
• Verified every call site passes (output, exitCode) / (exitCode, exePath) in the order the helper signatures expect — no argument-order swaps, which would have been an easy mistake given how many call sites were touched.
• The ReadRuntimeMajorAsync signature change from Task<int?> to Task<(int? Major, int ExitCode)> is threaded through both call sites correctly, including the post-revert re-read at DarlingManagedPostgres.cs:1197 (.Major ?? 0).
• Diagnose returns string.Empty for a tool's own exit codes, so ordinary failures like initdb exit 1 are not buried under loader boilerplate — matches the stated intent and is pinned by Diagnose_IsSilentForAnOrdinaryExitCode.
• No downstream code (Viewer, alerting, etc.) pattern-matches on the old raw message strings, so rewording these exception/log messages is safe.
• The main risk area for this kind of change — a correct decoder nothing calls, which is exactly what Runtime advance has no direction check: a package with an OLDER PostgreSQL major replaces a working newer runtime and the store cannot start (hit on DARLING01) #1738 already was — is directly guarded by TheBootstrapThrowSitesActuallyUseTheseMessages, which scans the actual source for the throw-site wiring rather than just testing the builders in isolation.

Minor/non-blocking style nit: In DarlingStoreUpgrade.cs, StartClusterAsync and StopClusterAsync's rebuilt messages append Diagnose(...) directly after (pg_ctl exit ...) with no terminating period on the first clause, whereas BuildStartFailureMessage/BuildStatusFailureMessage in DarlingManagedPostgres.cs do add one. Cosmetic only — not worth blocking on.

Security: No new external input handling — exit codes and exe paths originate from the service's own spawned processes, not untrusted input. No secrets or SQL touched.

Performance: All changes are on error/failure paths (dictionary lookups + string building), no hot-path impact.

[erikdarlingdata]

Green on top of #2202

Merged origin/dev at fbd9a930 (carrying #2202). Final sha 20669f56, mergeable: MERGEABLE, mergeStateStatus: CLEAN.

statusCheckRollup:

check	result
build	SUCCESS
Darling PostgreSQL tests	SUCCESS
Darling Linux build	SUCCESS
Claude Auto Review	SUCCESS
check-branches	SUCCESS

Full Darling.Tests locally on the merged tree: 4273 passed, 0 failed, 238 skipped. Skips are up 2 from the previous round, and that is expected rather than coverage lost: a1916a9d converted RunTool_SurfacesAWindowsStatusAsTheSignedFieldValueWithNoOutput from an early return to Assert.SkipUnless, so on non-Windows it now reports as skipped instead of passing vacuously - and this run counts it once per target.

Carrying a1916a9d

This merge picks up the review commit pushed onto the branch, and both halves are improvements over what I wrote:

• {ExitCode} stays numeric. I had replaced the structured-logging value with the decoded string, which costs structured sinks a numeric field to filter and aggregate on. The decoded meaning now rides its own {ExitCodeMeaning} field. I checked this landed on all three sites I had degraded - the pg_ctl stop warning, the pg_ctl reload critical, and the post-upgrade analyze warning - so there is no straggler left behind.
• Assert.SkipUnless over a bare return. A guard that returns early is indistinguishable from a passing test in the report; skipped says what actually happened.

CHANGELOG

One hunk again, this round on the entry block ([#2186] against [#2189]) rather than the link-refs. Resolved keep-both-sides, ours then theirs.

• git diff origin/dev HEAD --stat -- CHANGELOG.md → 2 insertions.
• Byte census: this branch 6 bare CR / 0 bare LF; origin/dev 6 bare CR / 0 bare LF. Delta 2,118 bytes - the same figure for the third round running, which is exactly my entry plus link-ref.
• Zero markers remain; all five link-refs (#2186, #2187, #2189, #2190, #2171) present exactly once each.

One note in case it looks like an artifact: there is a blank line between #2189's entry and the rest of the list. That is dev's own formatting, not something the resolve introduced - origin/dev has it at the same place. I left it alone rather than tidying it, since reformatting dev's line would have broken the 2-insertion property that makes this verifiable.

Not merging, per instructions.