A store upgrade that fails before its commit point puts the old cluster back as it found it, and only claims a revert that happened (#3927)

[erikdarlingdata]

Closes #3927.

CHANGELOG: this PR carries no CHANGELOG edit. Its entry lands with the batched splice.

Why

DarlingStoreUpgrade.UpgradeDataDirectoryAsync had three defects in its failure paths before the commit point. An adversarial review reproduced all three on copies of a real 17.10 + TimescaleDB 2.28.1 store.

1. Hard-link mode never undid pg_upgrade's rename. Once linking starts, pg_upgrade renames the old cluster's global/pg_control to pg_control.old, so the old cluster cannot be started while it shares files with the new one. The pre-commit handler never renamed it back, so any later failure left a store that could not start on either runtime. That includes pg_upgrade failing late and the directory swap failing. The handler then logged "NO data has been lost (the pre-upgrade data directory was never modified)". Review measurement E7a: renaming it back gives an intact old cluster when the new one was never started outside pg_upgrade, which is also what PostgreSQL's pg_upgrade docs say.
2. RevertRuntime moved the runtimes under a live postmaster. TryStopAsync only warned, and on Windows both Directory.Move calls succeed under a running server (E12/E13). The next connection was served by an 18.4 backend under an 18.6 postmaster. The next start adopted that postmaster, because pg_ctl status answers "running" for one on any port, and then could not connect.
3. A half-finished revert left no runtime. If pgsql moved to pgsql.failed and the second move failed, there was no pgsql directory at all.

The Failed outcome and the Failed self-alert claimed a clean revert ("reverted, collecting normally, no data was lost") in all three cases.

What changes

• Pre-commit catch. It calls the new RecoverFromPreCommitFailureAsync (its own method so a test can inject exactly this failure). That method stops the old cluster if this upgrade started it, drops the half-built cluster, puts the pre-upgrade data directory back, reverts the runtime, and only then says what happened.
• Cancel path (!swapped). It does the same put-back in place, inside the guard and before RevertRuntimeForCancel, which keeps the existing containment pin's shape.
• RestoreLinkedControlFile is a static helper that renames pg_control.old back. It runs in hard-link mode only, and only when pg_control.old exists and pg_control does not, so it can never overwrite a control file. It logs what it did. A failed rename is CRITICAL, names the exact manual rename, and nothing after it calls the store running.
• RevertRuntime returns whether it reverted (existing call sites still compile and discard the value):
  • It refuses, at CRITICAL with the stop command and the manual move, while postmaster.pid names a live postgres process. A reused PID naming anything else is not a server.
  • When the second move fails after the first succeeded, it moves pgsql.failed back and reports no revert.
  • It no longer reports a completed revert as failed when only the blocked marker will not write. That used to be CRITICAL "Could not revert", naming a hand restore from a rescued copy that was already gone. The stamp stays in that case, which still stops the next start from retrying the package.
• The outcome says what happened. StoreUpgradeOutcome gains PreUpgradeData (Untouched, ControlFileRestored, NotRestored) and RuntimeReverted, both trailing and defaulted to the clean revert. UsedLinkMode is now true on a link-mode failure. The worker's Failed arm maps these to three new trailing report fields.
• The Failed alert states only what happened, per shape:
  • The clean revert keeps its reassurance, with a reason that is true in both modes.
  • The restored control file explains what was undone.
  • A data directory that could not be put back says the store cannot start and what to move. This alert cannot really be delivered, because no store is up to send it, but it must not lie either.
  • A runtime that was not reverted says so, and says the next restart will fail until the rescued runtime is put back.
• TryStopAsync no longer ends "the next start adopts it", and the class and method doc comments no longer promise a store that is never left down.

Also fixed, same handler: the swap's own undo can fail

Step 7 moves the store to its retained name and moves it back when the second move fails. If that move back fails too, nothing is at the configured path. The old handler then called the store back. The next start ran initdb for an EMPTY store there, and the retention sweep deleted the real one (an unmarked pg-old-N) two starts later. The put-back now retries that move once. If the retry fails, it logs CRITICAL naming the move, the pg_control.old rename inside it in hard-link mode, and why restarting first would be the expensive mistake. Step 7 itself is untouched.

Filed, not fixed here: #3951

CompleteAfterStartAsync rewrites a Failed outcome to ExtensionUpdated whenever the post-start extension update runs, and BuildStoreUpgradeReport maps that to no alert. After #3908 bumps TimescaleDB, that would suppress this PR's "runtime NOT reverted" alert. CompleteAfterStartAsync is #3908's to rewrite. #3951 also tracks the now-stale StoreUpgradeStatus.Failed doc line, which is next to #3908's edit of that enum.

Coordination with #3908

The #3908 branch edits the same file. This PR stays inside:

• the three catch blocks;
• TryStopAsync, RevertRuntime and RevertRuntimeForCancel;
• new members directly below RevertRuntimeForCancel;
• trailing parameters on StoreUpgradeOutcome and StoreUpgradeReport;
• the evaluator's Failed branch and the worker's Failed arm;
• two doc comments.

None of these overlaps a #3908 hunk. BridgeTimescaleAsync, steps 1-7, StartClusterAsync/StopClusterAsync, CompleteAfterStartAsync, TryAdvanceRuntimeAsync, StoreUpgradeStatus and DarlingManagedPostgres.cs are unchanged. The new tests reference RuntimeStampFileName symbolically, so #3908's stamp rename flows through.

Test plan

The new class is DarlingStoreUpgradeRevertTests (30 tests). None of them needs PostgreSQL:

• Runtimes and data directories are marker files.

• A live postmaster is PING.EXE copied as postgres.exe, placed where the real one runs from. A running image does not block a directory move, and I measured that too.

• Every failure is a file held open, which blocks a rename.

• Restore helper: restores; never overwrites (both files, neither file); a rename it cannot make is CRITICAL and names the rename.

• Recovery seam, link mode: a planted pg_control.old and a failure injected before the commit point. The control file comes back before the runtime revert (log order), and the outcome is ControlFileRestored, reverted, UsedLinkMode.

• Recovery seam, other shapes: a restore that fails; copy mode; a live server the upgrade could not stop; the store stranded under its retained name, moved back or not movable.

• The real catch clauses: the real UpgradeDataDirectoryAsync failing at start-old-cluster, and the same run with a pre-cancelled token.

• RevertRuntime: refusal under a live postgres, stale PIDs (another process, no process, garbage, blank), a negated single-user PID, the half-move undo, the marker, the plain revert.

• The seam to the alert: the worker's Failed-arm mapping, and the alert text for each new shape.

• DarlingStoreUpgradeTests 79 (4 gated skips), DarlingSelfAlertTests 215 (3 gated skips), AlertStoredValueTests 16.

• Whole non-live Darling.Tests on the merged tree: 12,554, 0 failed (530 gated skips).

• Darling.Tests Release build: 0 warnings, 0 errors.

Red-watch

Each mutation reverted one fix, then the test project was rebuilt and the class re-run. Every mutation failed only the tests aimed at it.

Mutation	Went red
Put-back never restores pg_control	the 3 link-mode seam tests
Cancel path skips the put-back	the real cancel-catch test and the containment pin
Pre-commit catch restored to the original inline handler	the real pre-commit-catch test
No live-postmaster refusal	RevertRuntime refusal, and recovery under a live server
Any live PID refuses (name check removed)	the stale-PID case "another process"
Half-move not undone	the half-move test
Marker failure reported as a failed revert	the marker test
Failed alert text restored to the original	all 4 new-shape alert tests
Worker Failed arm restored	4 of the 5 mapping cases (the clean one is the default)
The swap's failed undo not retried	both stranded-store tests and both real-catch tests
Summary reassures unconditionally	restore-fails, live-server and stranded-store tests
Restore overwrites an existing pg_control	the never-overwrites test
Restore never restores	both helper tests and the 3 link-mode seam tests

Not verified here: no live pg_upgrade was run. The hard limits kept ports 55432/55433 and live upgrades off-limits, so the E7a/E12/E13 measurements are the review's, on real stores. These tests pin the behavior around them.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SFctxdyLSdZbycGtvwXk8j