Prune manual-deploy rollback backups, and report them once (#2525)

[erikdarlingdata]

Closes #2525.

The shape of it

A dogfood box was carrying 46 _rollback_manual_* directories, 5.48 GB, the oldest three weeks old — and the #2185 install-location report named every one of them, every start.

Every one of those 46 warnings was true. That is what makes them worth fixing rather than filtering. A guard that fires 46 times for something our own procedure created has stopped guarding by being too loud — a real layout problem, a stray DLL or a half-extracted upgrade, arrives as warning 47 in a list of 46 identical ones and nobody will ever see it. It is the same failure as a pin that never bites, wearing the opposite clothes: in both cases the check is still there, still running, still green-lighting the thing it exists to catch.

So both halves changed, and neither is sufficient alone.

Where retention went, and why

The issue's framing was right: there was nothing to bolt pruning onto. Darling/tools/ holds install-darling.ps1 (registers a service, or updates an existing binPath) and uninstall-darling.ps1. Neither lays a new build over an old one. That step — stop, back up, copy, start, verify — has never had a script in this repo; it lived in people's heads and in ad-hoc SSM scripts, which is precisely why the part nobody remembers by hand (deleting the last twenty deploys' backups) is the part that accumulated 5.48 GB.

Bolting retention onto the service was the other option and it is the wrong one: the service must not delete what it did not create. That absolutism is already load-bearing in DarlingStoreUpgrade (#1775 reports hand-made store copies and removes none of them) and weakening it here would be worse than the disk cost.

So this adds the missing step: Darling/tools/upgrade-darling.ps1, shipped in the zip beside the exe by the same csproj mechanism as the install/uninstall scripts. It owns retention because it owns the backups.

What it does beyond pruning, each because the documented procedure already required it or a past deploy paid for it:

• Resolves the install root from the registered service's ImagePath, not from where the script happens to sit.
• Verifies the source zip's SHA256 against -Sha256 or a SHA256SUMS.txt beside it, and refuses an unverified zip unless you say -SkipHashCheck out loud. This overwrites the binaries of a running monitoring host.
• Refuses an -InstallRoot the registered service does not actually run from, and refuses when no service is registered at all — both before anything is stopped or copied. (See review round 3 below: without the first of these, a run against a stale copy of the tree reports complete success having upgraded nothing.)
• Names processes holding the install tree and stops. It never kills one — the bundled PostgreSQL runs from pg-runtime under that directory and a blanket sweep takes the store down as its first act. (The last time this guard fired in the field, what it caught was an operator's own psql.exe.) A test asserts Stop-Process/taskkill/.Kill( appear nowhere in the file. It checks twice — once before the stop, filtered to what a service stop will not clear, and once after with no exclusions; see review round 1.
• Refuses to run from the install directory itself. The script ships in the zip, so it also lives in the install root — an upgrade run from the installed copy would write the new build over the .ps1 PowerShell is reading. -PruneOnly is exempt: it copies nothing, and the installed copy is exactly the one the service's report names.
• Backs up the install root's files (not pg-runtime, not viewer), prunes past -KeepRollbacks after taking the new backup so the tree never has fewer rollback points than retention promises, and confirms darling.json is byte-identical afterwards.
• Safe to re-run at every step, which matters because steps 5–9 leave the service down if anything fails. A backup taken inside -BackupWindowMinutes (60) is reused, not replaced — otherwise a re-run after a failed extract backs up the half-extracted tree over the only good copy.

-KeepRollbacks defaults to 3: the release, the one before it, and the one before that. The fourth cannot roll you back to anything you want — on the field box, the 43rd could only have returned it to a build from three weeks earlier.

Recognition

DarlingInstallDirectoryReport gains a third class. Not "the product's layout" (it isn't) and not "unaccounted for" (we know exactly what it is): a directory in the deploy procedure's namespace gets one line for the whole set — count, total size, the oldest, and upgrade-darling.ps1 -PruneOnly.

Severity follows retention, not disk. At or under 3 it is Information; past it, a Warning naming the excess. Warning about the intended state of a box that has been upgraded three times is how this report would talk itself straight back into being ignored.

Per-directory lines stay for directories the product genuinely cannot explain, because for those the path is the message — and they are readable again now that 46 backups are not sitting on top of them.

The field case, run against the real report:

BEFORE:  47 lines  (1 summary + 46 per-directory)
AFTER:    1 line

[Warning] 46 deploy rollback backups in the install directory C:\PerformanceMonitorDarling,
holding 5.5 GB — 43 more than the deploy procedure keeps, and the oldest of them is
_rollback_manual_20260720-000000. Reported once with a running total rather than one warning
each: our own deploy script made every one of these, so there is nothing in the list to
diagnose, and a line each would bury the directories above that DO need looking at. Prune them
with upgrade-darling.ps1 -PruneOnly, which keeps the newest 3. The service never creates or
deletes one.

And warning 47, which is the whole point — 46 backups plus one stray directory now reads:

[Warning] 1 director(ies) in the install directory ... are not part of the product's layout ...
[Warning] Directory not part of the product's layout: ...\Npgsql-8.0.3 (0 MB). ...
[Warning] 46 deploy rollback backups in the install directory ... Prune them with ...

This half matters even though retention landed, and that is not hedging: the boxes carrying the backlog today got it before any script pruned anything, and an upgrade does not remove a directory the product did not create.

One convention, two implementations, and a pin between them

The script's spelling and the service's matcher must agree or the whole thing silently reverts — the service goes back to 46 warnings and nobody notices, because each of them is true. Two independent literals is how #2525 happened in the first place.

So DarlingRollbackBackups owns the string, and TheDeployScriptAndTheService_AgreeOnWhatARollbackBackupIsCalled lifts Test-DarlingRollbackBackupName out of the shipped .ps1 by brace matching, runs it under Windows PowerShell over a 12-row case table, and compares it to DarlingRollbackBackups.IsRollbackBackup — and checks both against the table, so a mutual mistake cannot pass itself off as agreement. Same idiom DarlingInstallLocationTests uses to keep the installer's and the service's install-location rules answering alike.

The retention selection is executed the same way, against planted trees, in a fixture where the name ordering and the timestamp ordering deliberately disagree — a prune sorting on names would select the two directories the test requires it to keep.

What reading the existing tests turned up

I read the existing DarlingInstallDirectoryReportTests before changing anything. Two of them used _rollback_manual_ as their foreign example, which is no longer foreign:

• Report_SevenFieldDirectories_ProduceOneSummaryAndSevenLines_AndAllSurvive was really pinning three things: the summary appears once and not once per directory; no directory is silently dropped; nothing is deleted. All three still hold, and the test still asserts all three — its subject is now seven directories nobody can account for. What it looked like it was pinning, but was not, is that a _rollback_manual_ directory gets a line of its own. That was the field's naming used as a realistic stand-in; Manual-deploy rollback directories are never pruned: 5.5 GB and 48 warnings on every start #2525 is the discovery that the name was never arbitrary. Renamed to ..._SevenUnaccountedForDirectories_....
• Report_NamesForeignDirectories_WithTheirSize and Report_WhenTheSizeProbeIsExhausted_StillReportsEveryDirectory: same substitution, same properties intact. The second one's real pin — an exhausted budget degrades a directory's size and never its presence — is untouched.
• Nothing else moved. Report_NeverCallsAProductDirectoryForeign, the satellite-resource pair, and the empty-directory case are all unchanged, and the last one is the reason a bare _rollback_manual_ (no stamp — a name no procedure produces) is deliberately outside the convention and still reported individually.

Proving the pins red

Both halves were run locally against the shipped artifacts, not copies.

The report: a throwaway console project referencing PerformanceMonitor.Darling.Service.csproj (which targets plain net10.0) named Darling.Tests so the existing InternalsVisibleTo applies — so the real DarlingInstallDirectoryReport runs. 24 checks. With the fix: 24 pass. With IsRollbackBackup forced to return false — the pre-#2525 behaviour — 18 fail, and the 6 that stay green are exactly the properties that were already true (survival, product-layout silence, the seven-unaccounted-for case, the bare-prefix edge).

One pin was passing for the wrong reason and got tightened: Assert.Contains("holding at least ") matched the unaccounted-for summary, which carries the same words, so it went green against a build with no rollback line at all. It now asserts on the rollback line specifically. A pin that passes for a reason other than the one it names is worse than no pin.

The script: Microsoft.PowerShell.SDK hosted from a console app (no pwsh on this machine), parse-checking the file and executing the extracted functions against planted trees. 56 checks, all passing — and two real defects came out of running them rather than reading them:

1. A backup dated in the FUTURE read as "recent." Clock skew, or a directory restored with its metadata, gives a negative elapsed time, which is less than any window — so the upgrade would have skipped taking a backup at all. The two errors are not symmetric: a spare backup costs 120 MB the next prune reclaims, a missing one costs the rollback the whole procedure exists to provide.
2. A failure while composing a LOG LINE was caught by the delete's handler and counted as a failed delete. The run reported two directories removed and two failures, for the same two directories, while the disk agreed with neither. Those counts drive the exit code and the "re-running is safe" advice. The try now wraps only the measure and the delete; reporting happens after it on a success flag, and a test pins that Note (/Warn (/$removed++ do not appear inside the guarded block.

Neither was visible in the source. Both are the kind of thing an Assert.Contains on script text can never see.

What review found (three rounds, five real defects)

Every one of these was in the new script, and every one is the same category: a guard that is wrong about when it runs, not about what it checks. Worth recording because it is the exact failure #2525 itself is about, found four more times in the fix for it.

1. The stop guard refused every real upgrade. It ran before Stop-Service with no exclusions, so it found the service's own exe — which lives in the install root and is running by definition on any install worth upgrading. The workaround is -SkipStopGuard, which is the worst outcome available: a guard that fails closed on the happy path teaches people to disable it, and then it guards nothing. Now it runs twice — before the stop, filtered to what the stop will not clear (catching an operator's psql.exe or a left-open Viewer at the price of a re-run and no outage), and after the stop with no exclusions (catching a postmaster that outlived it, which is precisely what must never be killed). The Viewer is deliberately not excused: we ship it, but the service does not own it and stopping the service does not close it.
2. Recovery advice after a failed copy was incomplete. The backup holds root files, so a half-extracted tree restored that way leaves old root binaries against partly-new viewer\/wwwroot\/runtimes\. Backing those up instead was rejected — it multiplies the retained disk this issue is about, to cover a case whose real fix is a zip the operator still has, and would still miss pg-runtime. The message now names both halves, and the decision is written down where the backup is taken.
3. An explicit -InstallRoot was never checked against the service's actual ImagePath — the sharpest one, because the failure is a silent success. Stop-Service/Start-Service act by name, so pointing at a stale copy of the tree stops the real service (a real outage), writes the build where nothing reads it, restarts the real service on its old binaries, finds darling.json unchanged because it was never touched, and reports success end to end. Now cross-checked before anything is stopped, and unconditionally — gating it on $PSBoundParameters would make it absent for exactly the caller it protects, so a pin asserts that identifier appears nowhere in the file.
4. A missing service crashed late instead of failing early, after the backup, prune and copy had already changed the tree.
5. Path equality had three hand-rolled spellings; it now has one (Test-DarlingSamePath), used by all three destructive-step gates. Shipping two spellings of one rule inside the fix for a bug caused by two spellings of one rule seemed worth avoiding.

The ordering pins are new and deliberate: review correctly noted that nothing pinned control flow, and control flow is what all of these were. "The check exists" and "the check is early enough to have prevented anything" are different claims, so the tests assert positions relative to the prune exit, the stop guard, Stop-Service and the copy.

Filed, not fixed: #2529. An in-place upgrade is an overlay — Expand-Archive -Force deletes nothing — so a file the old build shipped and the new one dropped stays forever, and the layout report cannot see it because it walks top-level directories. The obvious repair needs an allowlist of everything that legitimately lives in the tree and never came from a zip (darling.json, the DPAPI blobs, the backups, pg-runtime, operator files); getting that wrong in the loud direction warns about darling.json on every upgrade, which is this same issue with a new subject. The issue carries four options and the measurement that should decide — diff two consecutive release zips and find out whether we have ever actually removed a shipped file.

What the issue got slightly wrong

• 48 warnings. The report emits N per-directory lines plus 1 summary, so 46 directories produce 47 lines, not 48. The 48th is from somewhere else — most likely the Make the retained-copy sweep reach every sibling, and report the store copies that are not ours (#1770) #1775 store-copy report, which scans %ProgramData% and has its own summary, or one additional non-rollback directory on that box. Worth knowing because it means there may be a genuine unaccounted-for directory sitting in that log already.
• _rollback_manual_ also exists in %ProgramData%, spelled <datadir>_rollback_manual_<stamp> — the prefix as a suffix, beside the store's data directory, holding a whole PostgreSQL cluster. Those are deliberately not covered here: one can be hundreds of gigabytes, they are identified structurally by PG_VERSION rather than by name, and no script of ours creates or prunes them. Make the retained-copy sweep reach every sibling, and report the store copies that are not ours (#1770) #1775's report keeps naming them individually and should.

Testing

23 new tests (5742 → 5765 in Darling.Tests), and the delta was checked against the [Fact] count so none of them is a test that silently never ran.

Darling.Tests builds clean on macOS with EnableWindowsTargeting. It cannot run here — net10.0-windows, no Microsoft.WindowsDesktop.App — and nothing in this PR was tested on real Windows from this machine: no service was stopped, no zip extracted, no install tree overwritten. The PowerShell functions ran under PowerShell 7.4 via the SDK, and CI runs them under Windows PowerShell 5.1, which is the version the script targets. The xUnit pins are the arbiter and they run on the Windows build job.

The end-to-end flow — elevation, Stop-Service, Expand-Archive over a live tree, Start-Service — is unexercised by any test and only a real upgrade on a real box will exercise it. That belongs on a dogfood box, with -ListRollbacks first and -PruneOnly second before anything copies a build.

[claude]

The stop guard runs before the service is stopped, so it will fire on every normal upgrade of a running install.

Get-DarlingProcessesUnderPath $InstallRoot (called here) finds any process whose executable path lives under $InstallRoot. But at this point in the script the Darling service — the thing being upgraded — is still running, and its own exe (PerformanceMonitor.Darling.Service.exe) lives at $InstallRoot\PerformanceMonitor.Darling.Service.exe. The bundled PostgreSQL under pg-runtime is very likely running too. So $holders will contain at least the service process itself on essentially every real-world invocation, and the script will Fail here unless -SkipStopGuard is passed — which defeats the guard and contradicts the documented flow ("stop, back up, copy, start" — step 4 names/stops before step 5 stops the service, per the .DESCRIPTION block above).

The function's own doc comment says the opposite of where it's called: Get-DarlingProcessesUnderPath's comment (line ~237) reads "Stopping the service stops the store properly; anything still holding the tree after that is a person's session" — implying this check belongs after Stop-Service, to catch genuine strays (an operator's stray psql.exe, per the PR description's own field example) rather than the service being upgraded.

Suggest moving this guard to run after Stop-Service/WaitForStatus('Stopped', ...) succeeds (around line 511, before the backup step), so it only catches processes that are still holding the tree once the service itself is down — which is also the scenario it can't currently protect against (pg-runtime processes that outlive the service stop).

Note the C# test suite (DarlingDeployRollbackRetentionTests) only exercises individual extracted functions in isolation via RunWindowsPowerShell/ExtractFunction — nothing pins the overall script's control-flow ordering, so this wouldn't be caught by the new pins.

[erikdarlingdata]

Correct, and this was the serious one — thank you. The service exe lives at $InstallRoot\PerformanceMonitor.Darling.Service.exe and is running by definition on any install worth upgrading, so the guard would have failed on every real invocation. And the workaround is -SkipStopGuard, which is the worst possible outcome: a guard that fails closed on the happy path teaches people to disable it, and then it guards nothing. That is the same disease as the 46 warnings this PR is about, one layer earlier.

Fixed in 555a7be, but not by moving it — by running it twice, because the two positions answer different questions and neither subsumes the other:

• Phase one, before the stop, filters out what the stop will clear (the service exe, anything under pg-runtime\). What is left is what a service stop will not close: an operator's own psql.exe, a shell sitting in the folder, a Darling Viewer left open holding viewer\*.dll. Those are caught for the price of a re-run and no outage, which is the case from the field report.
• Phase two, after the stop, runs with no exclusions at all. A hit there is usually a postmaster that outlived the stop — precisely the process nothing may kill. Phase one structurally cannot see this.

The Viewer is deliberately not excused even though we ship it: the service does not own it and stopping the service does not close it, so it holds viewer\ exactly as hard as any third-party app would. That is now a table-driven pin, along with pg-runtime-prev not matching a pg-runtime prefix.

You were also right that nothing pinned control flow, and that is the class of bug this was — so the new pin is about ordering (phase one before Stop-Service, phase two after, and which of the two filters), not about text. The excusal predicate now takes its separator from the runtime rather than a hardcoded \, so a rule that decides what gets excused from a guard is verifiable off-Windows; it runs green against a planted tree.

[claude]

Minor/secondary, lower confidence: the rollback backup only copies the install root's top-level files (deliberately, per the comment above) — viewer\, wwwroot\, runtimes\ and pg-runtime\ are never captured. That's fine for the happy path (retry succeeds), but the failure message a few lines down after a double copy failure says:

"restore the files from the newest _rollback_manual_* directory"

If the copy fails partway through Expand-Archive/Copy-Item -Recurse (step 8), those subdirectories can be left in a mixed old/new state, and restoring only the backed-up root files won't fix that — the operator would end up with old binaries paired with a partially-new viewer/wwwroot/runtimes. Worth either backing up those directories too (accepting the size cost) or making the failure guidance explicit that a full recovery also needs re-extracting the previous zip's viewer/wwwroot/runtimes over the top, not just the _rollback_manual_* files.

[erikdarlingdata]

Also correct. The backup is root files only — that is exactly what keeps one to ~120 MB rather than ~1 GB, and it is why the field box reached 5.48 GB in 46 deploys rather than 46 GB — so after a partial Expand-Archive the advice was genuinely incomplete: viewer\, wwwroot\ and runtimes\ can be left mixed old-and-new, and restoring root files over that leaves old binaries paired with partly-new subdirectories.

I took the guidance option rather than the bigger-backup option, deliberately, and wrote the reasoning into the script at the point the backup is taken so the next reader does not re-open it:

• Backing those directories up multiplies precisely the retained disk this issue is about, to cover a case whose real fix is re-extracting a zip the operator still has.
• It would still be incomplete — pg-runtime is never backed up either, and never should be.
• runtimes\ is the one with real teeth (native assets the service loads), and the previous zip restores it correctly, which a partial backup would not necessarily.

So in 555a7be the double-failure message now names both halves in order: re-extract the previous version's zip over the install root, then copy the _rollback_manual_* files over the top — and says why, that the backup does not hold those directories. It also leads with the thing to try first, which is re-running (the backup is reused, not replaced, so that is safe). The README carries the same caveat, and since the accuracy of that message is now the entire mitigation, it is pinned rather than trusted.

[claude]

Reviewed the diff (CHANGELOG, new upgrade-darling.ps1, DarlingRollbackBackups, DarlingInstallDirectoryReport changes, and the new/updated Darling test files). No T-SQL or Lite-edition changes in this PR, so the T-SQL style rules and Lite/Darling parity check don't apply here — this is Darling-only deploy tooling with no Lite counterpart (Lite has no server-installed service/upgrade procedure).

One correctness bug worth fixing before merge (left as an inline comment): the new stop guard in upgrade-darling.ps1 runs before Stop-Service, so it will detect the running Darling service's own exe (and likely pg-runtime's postgres processes) as "holding" the install tree on essentially every normal upgrade of a live install — forcing -SkipStopGuard every time and defeating the guard. The guard function's own doc comment ("anything still holding the tree after that is a person's session") implies it was intended to run after the service stop. None of the new C# tests exercise the script's overall control-flow ordering (they only extract and unit-test individual functions), so this wouldn't have been caught by the new pins.

A second, lower-confidence note on the same file about the failure-recovery guidance after a double copy failure being incomplete (rollback backup only covers root files, not viewer/wwwroot/runtimes).

Everything else — the shared DarlingRollbackBackups naming constant/retention count between the C# service and the PowerShell script, the per-directory-handler try/catch fix (paying off the #1775 lesson), the report-vs-prune-budget ordering, the wildcard-avoidance in Get-DarlingRollbackBackups, the self-overwrite refusal keyed on $PSScriptRoot, and the SHA256 verification path — looks correct and well-tested against the shared case table.

[claude]

Expand-Archive -Force (and the Copy-Item -Recurse -Force fallback for a folder source) only overwrite files that exist in the new build — neither one deletes a file that existed in the old install but was removed from the new one. A dependency dropped between versions, a renamed assembly, an old satellite-resource folder for a culture the new build no longer ships — all of that lingers in the install tree forever with no cleanup and, unlike a whole foreign top-level directory, no reporting: DarlingInstallDirectoryReport only walks top-level directories, not stray files inside directories the product still owns (viewer\, wwwroot\, runtimes\, or the root itself).

Given how carefully this script accounts for every other partial-state scenario (half-extracted trees, future-dated backups, mixed old/new subdirectories after a failed copy), it's worth calling out explicitly whether this is an accepted gap or worth a follow-up — e.g. diffing the new build's file manifest against the install root's files after the copy and at least warning about anything left over that the new build didn't ship.

[erikdarlingdata]

Right on the mechanism, and it is a real gap: Expand-Archive -Force is an overlay, it deletes nothing, and you are also right that DarlingInstallDirectoryReport structurally cannot catch it — it walks top-level directories, so a stale DLL in the root or inside viewer\ is invisible to it by construction, not by oversight.

I filed it as #2529 rather than fixing it here, and named it in a comment at the copy step so the next reader finds it where it matters.

The reason it is not a quick add: the obvious repair (diff the new build's manifest against the install root, warn about the remainder) needs an allowlist of everything that legitimately lives in that tree and never came from a zip — darling.json, the DPAPI credential blobs, the .bak-* config backups, the _rollback_manual_* directories this very script creates, pg-runtime\ and pg-runtime-prev\, pg-runtime.zip, plus whatever an operator put there. Get that list wrong in the loud direction and it warns about darling.json on every upgrade — which is #2525 again with a new subject, and the same guard-stops-guarding outcome. That is the wrong thing to design in the last commit of a PR about noise.

The issue carries four options and, more usefully, the measurement that should decide between them: diff the file lists of two consecutive release zips and find out whether we have ever actually removed a shipped file. If no release in the last year did, this is a documented gap and nothing more. My current preference is the variant that diffs old zip against new zip rather than zip against install root — it cannot produce a false positive, because everything it can name provably came out of one of our own zips, so it needs no allowlist at all.

[erikdarlingdata]

This is the sharpest of the three, and you are right that my previous fix only closed half of it. Fixed in ae03235.

The thing that makes it worse than an ordinary bug is the shape of the failure: it is a silent success. Every check passes, darling.json is unchanged (because it was never touched), the service reaches Running, the script prints its whole happy path — and the real service was stopped for nothing, the build went into a directory nothing reads, and the service came back on its old binaries. The operator believes they upgraded. The next person is debugging a version that never shipped. That is strictly worse than the crash my last commit fixed, because a crash tells you something happened.

-InstallRoot is now cross-checked against the registered ImagePath before anything is stopped or copied, and unconditionally rather than only when the parameter was passed. When it was not passed the two are equal by construction, so it costs a registry read — and gating it on $PSBoundParameters would make the check absent for precisely the caller it exists to protect. There is a pin asserting PSBoundParameters appears nowhere in the file, for that reason. An ImagePath that cannot be parsed warns rather than refuses: the service is registered and the auto-resolve path would already have failed, so the honest statement is "this was not confirmed", not "this is wrong".

While doing it I collapsed path equality to one spelling, Test-DarlingSamePath, now used by all three destructive-step gates — self-overwrite, source-is-the-install-directory, and this one. Three hand-rolled comparisons deciding whether an upgrade may touch a tree is the same two-spellings-of-one-rule failure this PR is a case study in, and I would rather not ship the disease in the cure. It deliberately tolerates the ways people actually type paths (trailing separator, case, a . segment), because a registered ImagePath and a hand-typed -InstallRoot routinely differ by exactly those and a stricter comparison would refuse every correct upgrade — while a sibling directory sharing a prefix (...DarlingOld) is correctly not the same path. Table-driven, run against the shipped function.

[claude]

If -InstallRoot is passed explicitly for a directory that has the service exe but isn't backed by a currently-registered 'PerformanceMonitor Darling' service (e.g. pointed at a build staged outside the actual install, or the service was renamed/removed but binaries left behind), Get-Service here returns $null, this block is skipped, and the script proceeds through backup/prune/copy as if everything is fine. It then hits the unguarded Start-Service -Name $serviceName at line 655, which throws a raw terminating error instead of one of the script's usual friendly Fail messages — worth an explicit check (mirroring the "not installed" guard already done for the auto-resolve path around line 367) rather than relying on a late, unfriendly crash.

[erikdarlingdata]

Correct, and fixed in 55197c5. The auto-resolve path reads the install root out of the registered ImagePath so it cannot get here without a service, but -InstallRoot skips that check entirely — and the consequence is worse than an ugly error: the script would have taken a backup, pruned, and completed the copy before failing, so it changes the tree and then crashes uninterpretably.

There is now an explicit refusal before the stop guard, the stop and the copy. It sits after the -PruneOnly exit deliberately — reclaiming disk from a tree whose service is gone is a perfectly reasonable thing to want and copies nothing — and that placement is pinned by ordering against all four landmarks, since "the check exists" and "the check is early enough to have prevented anything" are different claims.

Related, while I was there: the service status is now re-read at the stop rather than carried down from the existence check. It is a snapshot, and between the two the service can legitimately have been stopped by someone else or died on its own.

[claude]

Review summary

Scope: Darling-only (a new Darling/tools/upgrade-darling.ps1, DarlingRollbackBackups.cs, and changes to DarlingInstallDirectoryReport.cs/its tests). No T-SQL, no Dashboard/Installer changes, and Lite has no service/install-directory concept to keep in parity with here — so no Lite/Darling drift to flag.

Overall this is very well-built: the deploy-script/service naming convention is pinned by a single shared constant plus a test that runs the actual .ps1 predicate (via brace-extraction) against the C# predicate over a shared case table, the retention selection is tested with deliberately conflicting name/timestamp ordering so a name-sort bug can't hide, and the two defects called out in the PR description (future-dated backup read as "recent", and log-line failures miscounted as delete failures) are both directly pinned by tests. The stop-guard's two-phase design (filtered before Stop-Service, unfiltered after) is correctly ordered and tested for ordering, not just text presence. IsRollbackBackup / Test-DarlingRollbackBackupName agree on the case-insensitive, prefix-plus-one-char-minimum rule, and the never-delete / per-directory-failure-isolation properties (learned from #1775) are carried over correctly into Remove-DarlingRollbackBackups.

Two things worth a look, posted inline:

1. Stale files after an upgrade aren't cleaned up or reported. Expand-Archive -Force / Copy-Item -Recurse -Force never delete a file that existed in the old build but was dropped from the new one, and DarlingInstallDirectoryReport only scans top-level directories — so a stray leftover file inside viewer\/wwwroot\/runtimes\/root would linger silently forever, unlike a whole foreign top-level directory.
2. An unguarded Start-Service on line 655 can throw a raw terminating error instead of a friendly Fail if -InstallRoot is pointed at a directory not backed by the currently-registered service (the $service null path around line 546 falls through silently).

Neither is a blocker given how narrow the second one is and how explicitly documented the first one's tradeoffs are elsewhere in the script, but both seemed worth flagging given how much rigor the rest of the script applies to edge cases.

[claude]

Correctness: explicit -InstallRoot is never checked against the registered service's actual ImagePath.

The comment above this block (lines 519-520) explicitly acknowledges the gap — "an explicit -InstallRoot skips that check entirely" — but the fix that follows only refuses when the service isn't registered at all. It never verifies that the registered service's ImagePath directory actually equals $InstallRoot.

Concrete failure: an operator passes -InstallRoot pointing at a stale/decommissioned copy of the tree (old runbook, copy-paste from another box, a leftover directory that still happens to contain PerformanceMonitor.Darling.Service.exe so the line-381 check passes too). The script will:

1. Stop the real PerformanceMonitor Darling service (a real outage window), because Stop-Service/Start-Service act on the service by name, not by path.
2. Take a backup, prune, and lay the new build down in the wrong, unrelated directory.
3. Restart the real service, which comes back up running the old, untouched binaries from its actual ImagePath.
4. Report success end-to-end ("Service is Running", darling.json unchanged, etc.) — because nothing it checked was ever wrong.

The operator sees a clean run and believes the upgrade happened; the service never changed, and there was an unnecessary stop/start of production monitoring. This is exactly the kind of silent-but-plausible failure the rest of this script goes out of its way to prevent (self-overwrite refusal, unregistered-service refusal, stop-guard, config hash check).

Suggest cross-checking Get-DarlingInstallRootFromService $serviceName against $InstallRoot (case-insensitive) whenever -InstallRoot was passed explicitly, and Failing before anything is stopped if they disagree — the same "fail before touching anything" pattern already used for the unregistered-service and self-overwrite cases just above/below this.

[claude]

Reviewed the full diff (PowerShell script, both new C# files, test suites, csproj, README, CHANGELOG).

Scope check: this PR is Darling-only (install-tree upgrade tooling + the layout report's rollback-backup recognition). Full Edition (install/, Installer/, Dashboard/) and Lite have no equivalent install-directory/service-upgrade concept — Full's T-SQL collectors are versioned/re-run via the installer, and Lite is a standalone desktop app with no service or install tree to upgrade. I don't see a Lite/Darling parity gap here; there's nothing on the other side of this feature to keep in sync with.

No T-SQL in this PR, so the T-SQL style section of CONTRIBUTING.md doesn't apply. The C# additions (DarlingRollbackBackups, the DarlingInstallDirectoryReport changes) are internal-static, well-scoped, and match existing patterns in the file (never-throws Report, budget-bounded size probes, "at least" wording when a probe is exhausted).

What I verified works correctly:

• DarlingRollbackBackups.IsRollbackBackup (C#) and Test-DarlingRollbackBackupName (PS1) implement the same prefix-plus-one-more-char rule, case-insensitively, and the two defects called out in the PR description (future-dated backup reading as "recent"; log-line failures inside Remove-DarlingRollbackBackups's try being misreported as delete failures) are both fixed in the shipped script — the try block now wraps only the measure+delete, and Test-DarlingRollbackBackupIsRecent requires $elapsed -ge 0.
• The two-phase stop guard (filtered pre-stop, unfiltered post-stop) correctly excuses only the service exe and pg-runtime\, not the Viewer.
• Prune ordering is by LastWriteTimeUtc, not by name, and pruning happens after the new backup is taken so the tree never dips below the retention promise.
• upgrade-darling.ps1 is wired into the csproj the same way as the existing install/uninstall scripts, so the report's -PruneOnly advice actually ships.

One correctness gap worth a look (left as an inline comment on upgrade-darling.ps1 around the "service must be registered" check): when -InstallRoot is passed explicitly, the script never verifies it actually matches the registered service's ImagePath. It only checks that some service by that name exists. A mismatched -InstallRoot (stale copy, copy-paste error) would stop/restart the real service for nothing while silently upgrading an unrelated directory — and the script would report success throughout, since every other check it performs would still pass. The comment right above that check already flags "an explicit -InstallRoot skips that check entirely," so this looks like a known gap that didn't get fully closed rather than an oversight.

Everything else — SHA256 verification, the self-overwrite refusal, the wildcard-free directory selection, the retry-twice-then-fail copy, the config-hash sanity check — held up under review.

[claude]

Review: #2525 rollback-backup retention + reporting

Went through the full diff — DarlingRollbackBackups.cs, the DarlingInstallDirectoryReport.cs changes, upgrade-darling.ps1 in full, and both test files — plus the csproj/README updates that ship the script.

No blocking correctness, security, or Lite/Darling parity issues found.

A few things I specifically checked and are handled correctly:

• The shared-constant/shared-predicate design (DarlingRollbackBackups.IsRollbackBackup in C#, Test-DarlingRollbackBackupName in the script) is exercised against the same case table from both sides via TheDeployScriptAndTheService_AgreeOnWhatARollbackBackupIsCalled, so the two spellings can't drift silently — the actual root cause of Manual-deploy rollback directories are never pruned: 5.5 GB and 48 warnings on every start #2525.
• Retention math (Select-DarlingRollbackBackupsToPrune) floors $keep at 1 even if called with 0, so a delete can never select every backup.
• The future-dated-backup clock-skew fix in Test-DarlingRollbackBackupIsRecent ($elapsed -ge 0) is correct — a negative elapsed time now falls through to taking a fresh backup rather than silently skipping one.
• Remove-DarlingRollbackBackups correctly scopes the try/catch to just the measure+delete, with reporting (Note/Warn/counters) outside it — avoids the Make the retained-copy sweep reach every sibling, and report the store copies that are not ours (#1770) #1775-style bug where a failure composing a log line gets miscounted as a delete failure.
• The two-phase process-holder guard (unfiltered check only after Stop-Service) is correctly ordered — an unfiltered pre-stop check would refuse every real upgrade since the service's own exe is always running.
• Self-overwrite refusal keys off $PSScriptRoot (where the running script is) rather than $Source, which correctly handles the case of a zip staged inside the install directory.
• The -InstallRoot-vs-registered-ImagePath cross-check runs unconditionally (not gated on whether -InstallRoot was passed), so it isn't silently absent for the one caller who needs it.
• No process is ever force-killed (pinned by TheDeployScript_NeverKillsAProcessUnderTheInstallTree), and the SHA256 verification defaults to refuse-if-unverifiable rather than silently proceeding.
• Budget-sharing between the unaccounted-for and rollback-backup measurement passes is correct — unaccounted-for directories get first call on the 5s probe budget, so 46 rollback backups can't starve the one directory an operator actually needs to see.

Parity: this is Darling-only server-install tooling (a Windows service + in-place upgrade procedure). Lite is a standalone desktop app with no server-side install/deploy step, so there's no Lite counterpart for this feature and no drift to flag.

No T-SQL was touched by this PR, so the collector/style conventions in CONTRIBUTING.md don't apply here.

Nicely instrumented — the two live defects mentioned in the PR description (future-dated backup, log-line-in-try) are both correctly fixed in the diff, and the regression tests for both are meaningful (they exercise the shipped script via brace-extraction + real PowerShell execution, not just text assertions).