Repository navigation
Pin the store-upgrade wiring, and correct a false claim in the operator alert - #1739
Merged
Merged
Conversation
Two findings, one from each reviewer, both about honesty of a different kind. The shepherd caught that my new degraded-alert text said the retained copy "will not age out on its own". It does. SweepRetainedDataDirectories reads a MISSING counter as 1, so a failed marker write costs exactly one extra service start -- the same reasoning we both used to establish that wrapping the write was safe. Telling an operator otherwise sends them to delete a multi-gigabyte directory for no reason. Same class of false operator-facing claim this round was about, just erring toward extra work instead of false comfort. DarlingSelfAlertEvaluator.cs:1191 now says it ages out one start later, and names the real signal: the countdown cannot advance while whatever blocked the write is still blocking it. The reviewer took my own mutation question and aimed it at the other new pins, which found that the HIGH fix itself was unpinned. Deleting `swapped = true`, neutralising the catch filter, dropping the cancellation guard, or deleting the preflight call all left the entire suite GREEN -- the logic tests pass because the logic is right, and the gated E2E is a happy path that by construction never throws after the swap and never meets an occupied port. The defect that reached an arming gate had no test that would notice it coming back. Closed with source-parsing pins on the HostHeaderGuardTests idiom, which exists in this repo for exactly this reason (#1648 was also a wiring omission a logic test could not see): DarlingStoreUpgradeTests.cs:319 pins that the post-commit catch stays AHEAD of the general one and never reverts, :343 that the cancellation path keeps its !swapped guard, :358 that the preflight is actually invoked before pg_upgrade. Catch-clause order is the mutation most likely to happen by accident -- consolidating error handling looks harmless, the compiler says nothing, and the store-bricking path returns silently. All four mutations verified caught, then reverted. Also documented why TryStopAsync is safe in the post-commit handler (oldStarted is already false) so nobody re-derives it. Full suite 3359 passed, 0 failed, 0 warnings; gated upgrade E2E green end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pg18-bundle-builder re-ran the four mutations independently and found a fifth the pins missed: moving RevertRuntimeForCancel out of the if (!swapped) block leaves it textually after the guard, so the order-only assertion passed green, while making the revert unconditional - the store-bricking path the guard exists to prevent. It compiles. Closed with a containment assertion; verified the mutation turns the pin red and the other two stay green, then reverted. Also corrected the post-commit test comment. Reordering the catch clauses does not compile at all (CS0160), so the compiler is what prevents that mutation - the comment claimed the compiler does not warn, and argued the pin was load- bearing for a case it is not. The test still earns its place on the two assertions that DO catch silent mutations: the filtered clause existing, and nothing reverting inside it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The gated upgrade fixture went red on its FIRST CI execution, which is what wiring it into nightly was for -- it had never run anywhere but my box. pg_upgrade reported: connection to server at "localhost" (::1), port 55432 failed: FATAL: password authentication failed for user "darling" It failed at the dry-run step, so the safety worked exactly as designed: nothing changed, the runtime reverted, the store came back on 17 with its data directory untouched. The fail-safe is fine. The upgrade was not. The credential was right; the HANDOFF was wrong. It rode a hardened temporary PGPASSFILE, which has to get four separate things right on every host: the ACL -- and pg_upgrade re-executes itself under a RESTRICTED token on Windows, so the reader is not quite the writer -- the encoding, a path that restricted child can reach, and cleanup. Four chances to differ between a developer box and a runner, and it took all four. Now PGPASSWORD in the child's environment. I chose the file originally to keep the password out of a process environment block, and that reasoning does not survive contact: an environment block is readable by the same user and by administrators, which is the identical audience that can already read the DPAPI credential file the password comes from -- except the environment never touches disk, where a killed process could strand a cleartext temp file the finally never ran for. So it is fewer moving parts AND less exposure, not a trade. Applies to vacuumdb too. Also closes a real hole the reviewer found in my own guard test: CancellationPath_DoesNotRevertOnceTheSwapCommitted asserted textual ORDER, not CONTAINMENT. Moving the revert OUT of its if (!swapped) block leaves it after the guard textually and the test stayed green, while a post-swap shutdown would brick the store again -- the same vacuous-pass shape that test exists to close, one level in. It now asserts no closing brace falls between guard and revert, verified by applying that exact mutation and watching it go red. Full suite 3359 passed, 0 failed, 0 warnings; gated E2E green end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
erikdarlingdata
enabled auto-merge
July 27, 2026 00:52
…ature/1706-pg-upgrade # Conflicts: # CHANGELOG.md # Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
erikdarlingdata
disabled auto-merge
July 27, 2026 00:53
The f58eb19 merge kept BOTH versions of the same entry: the original text and the corrected one that records the compiler-caught mutation and the fifth mutation the pins missed. Same title, divergent bodies - which is exactly the case where a keep-both-sides union is wrong, because the two sides are not two changes, they are one change edited. Kept the corrected version, dropped the stale one. The auth-fix entry stays as its own bullet: it is a genuinely separate user-visible change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review leftovers from the PGPASSFILE removal: the method carried TWO stacked <summary> elements, the first still describing the password-file approach and its "deleted in the callers finally" cleanup - false text sitting directly above the summary explaining why that approach was removed. Same class as the alert wording defect this PR already fixes, aimed at a maintainer instead of an operator. Also dropped the empty finally block the removal left behind. The remaining summary now cites the clause that licenses the deviation rather than asserting the conclusion: libpq warns against PGPASSWORD "as some operating systems allow non-root users to see process environment variables via ps", which is conditioned on an exposure Windows does not have. Records that the value is set on the CHILD ProcessStartInfo, so the service environment never carries it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
erikdarlingdata
enabled auto-merge
July 27, 2026 01:01
This was referenced Jul 27, 2026
pull Bot
pushed a commit
to ehtick/PerformanceMonitor
that referenced
this pull request
Jul 29, 2026
…ta#1745) An edit that inserts a new member and its <summary> ABOVE an existing summary strands that summary on the wrong member: the new member reads with a description of something else, and the member the block actually described is left undocumented. XML docs take the LAST summary, so tooling renders correctly, the build is clean, nothing warns, and no behavioral test can see it. Eight sites on dev. TWO were introduced by erikdarlingdata#1739/erikdarlingdata#1744 -- the PRs that fixed two other instances of this same class -- leaving RunCollectionLoopAsync and EvaluateCompressionJobsAsync undocumented. The other six were pre-existing across four projects. SEVEN of the eight are fixed by MOVING the orphaned block back to the member it describes. Only LocalDataService.QueryStore.cs held a genuine superseded duplicate safe to delete. A blind "remove the extra summary" sweep would have destroyed documentation at seven sites, so the guard's failure message says that outright. The pin walks every .cs outside bin/obj from the repo root and fails on </summary> immediately followed by <summary>. It FAILS rather than skips when it cannot find the tree: a guard that silently skips is a guard that silently stops guarding. Verified both ways -- green on the fixed tree, red naming Lite\Services\RemoteCollectorService.cs:29 when a stacked block is re-introduced, which is a Lite mutation caught by a test in Darling.Tests. Darling 3360, Lite 1575, Dashboard 768, zero failures. Documentation and one test only; no behavior changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pull Bot
pushed a commit
to ehtick/PerformanceMonitor
that referenced
this pull request
Jul 29, 2026
…rikdarlingdata#1737) TryAdvanceRuntimeAsync treated "the package's PostgreSQL major DIFFERS from the extracted runtime's" as a reason to update, without checking which direction. A PG 17 package landed beside an 18 store on DARLING01, the runtime was swapped, and the store was down ~7 minutes until the previous runtime was restored by hand. Nothing reached pg_upgrade, so no data was at risk; the store simply could not start. Three guards, each pinned and each mutation-checked. DIRECTION CHECK against the data directory's PG_VERSION. That file is the authority on what the store needs, and -- the reason it is used instead of the binaries -- readable WITHOUT executing anything. Every check that failed open on DARLING01 failed because it asked the binaries what they were, and those binaries could not launch (STATUS_DLL_NOT_FOUND). The guard sits OUTSIDE the no-stamp branch on purpose: the stamp records only that the zip CHANGED, never which way, so a stamped host receiving an older package downgrades identically. A refusal leaves the runtime alone and deliberately does NOT write the stamp -- recording the bad package as "seen" would silence the warning from the second start onward, while the wrong zip is still sitting beside the service. UNIDENTIFIABLE RUNTIME now stops instead of skipping. The check that should have caught this logged "data directory: 18, bundled runtime: unreadable - skipping the runtime version check. The store starts normally" one second before the bootstrap died. Backwards: the check could not run BECAUSE the binaries could not run. A known store major with an unidentifiable runtime now refuses, naming the rescued runtime to restore rather than surfacing a raw Win32 code. REVERTRUNTIME PG_VERSION GUARD, erikdarlingdata#1737's deferred item. It now takes the data directory and the major it assumes, and refuses once PG_VERSION has moved forward. Redundant against today's two callers; defence in depth against the third that forgets the flag -- which is exactly how erikdarlingdata#1738 happened, by a different route. Both comparisons were extracted as PURE predicates because inverting them in place left the ENTIRE suite green, and an inverted direction check refuses every legitimate upgrade while permitting every downgrade: the filed defect, doubled. Six mutations verified red then reverted, including the original bug restored against a new gated test that reproduces DARLING01 with real 17 and 18 packages. erikdarlingdata#1737 items 1 and 2 were verified already closed by erikdarlingdata#1739/erikdarlingdata#1744 rather than assumed. Full suite 3376 passed, 0 failed, 0 warnings; upgrade E2E still green. Closes erikdarlingdata#1738 Closes erikdarlingdata#1737 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opened by the shepherd so this is not lost:
24afb2b5was pushed tofeature/1706-pg-upgradeafter #1718 merged, so it is not an ancestor of dev and did not make3.2.0-nightly.20260727. The branch's old PR cannot carry it.Two things, both from #1718's review round (tracked in #1737):
1. The HIGH fix had nothing pinning it. Found by pg18-bundle-builder mutation-checking the new tests: deleting
swapped = true, reorderingcatch (Exception ex) when (swapped)after the unfiltered catch so the filtered clause becomes unreachable, dropping the!swappedcancellation guard, or deleting theAssertUpgradePortsFree()call all left the whole suite green. The logic tests pass because the logic is right, and the gated E2E is a happy path that structurally cannot reach either branch — so the wiring, which is where every defect in that round actually lived, was unprotected. Closed with source-parsing pins in theHostHeaderGuardTestsidiom this repo already uses for wiring invariants (DarlingStoreUpgradeTests.cs:319,:343,:358). All four mutations verified caught, then reverted.2. A false claim in the operator alert. The degraded-upgrade branch told the operator to remove the pre-upgrade data directory by hand "because it will not age out on its own". It does:
RollbackRetentionStarts = 2, andSweepRetainedDataDirectoriesparses a missing.startsfile as1, writes the counter, keeps the copy, and deletes it on the following start. A failed marker write costs exactly one extra service start — which is the same reasoning used to establish that wrapping the marker write was safe in the first place. Now says so, with the real conditional: the copy only stays put while whatever blocked the write is still blocking it.Same defect class as the round that produced it — operator-facing text asserting something the code does not do — just erring toward unnecessary work rather than false comfort.
Still needed before merge
pg_upgradeauthentication failure: the gated E2E passes locally but fails in the nightly withpassword authentication failed for user "darling"on port 55432, atstep 'pg_upgrade-check'. The fail-safe behaves correctly (nothing changed, runtime reverted, store back on 17), but the in-place 17-to-18 path currently works only on the author's machine.Author owns the arming decision; I have not armed it.
🤖 Generated with Claude Code