fix(cli): claim the backup destination before dumping, not after - #273
Merged
Conversation
backupCommand ran pg_dump and staged the uploads before it first tested whether --out could be written at all. MEI-104 made that failure legible; it was still late. An operator following the upgrade runbook — where the next instruction destroys the pgdata volume — waited out a full production dump to learn their path was wrong, which invites re-running under sudo (producing the root-owned bundle MEI-104 fixed) or skipping the backup. Claiming the destination first also makes two backups aimed at one path safe: the second is refused immediately rather than dumping and then losing the race. The cost is a wider window in which a killed run leaves an empty file at the claimed path, so EEXIST stops being a raw errno from open(…, 'wx') and says what is there and what to do about it. reserveBackupDestination keeps its untranslated contract; the translation sits in claimBackupDestination beside MEI-104's, and a refusal never touches the occupying file. Both documents that described the failure as arriving after the dump now describe it as arriving before one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GVdrZfcwVhvJpZWUKWxYs9
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
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.
Fixes MEI-149 — the follow-up I flagged on #271 rather than folding into it, since it is a behaviour change and that PR was a doc correction.
The problem
backupCommanddid the expensive work first and only then found out whether it could write the answer anywhere:MEI-104 made that failure legible. It did not make it early. An operator following the upgrade runbook has just been told to take a backup because migrations are forward-only, and
docker volume rm docker_pgdatais two lines below — so a late failure there invites re-running undersudo(producing exactly the root-owned bundle MEI-104 fixed) or skipping the backup.Measured, before and after
Same command both times: an occupied
--outand a deliberately unreachable database, which distinguishes which check runs first.Before — never looks at
--out; goes straight for the dump:After — refuses before connecting to anything, and never prints "Dumping the database…":
The occupying file was still 0 bytes afterwards — a refusal does not touch it.
The change
Three lines move; the rest is the consequences.
claimBackupDestination(out)and itsdestinationCreated = truemove to the top of thetry. The existingfinallyalready removes a reserved-but-unwritten destination, so an ordinary mid-run failure still leaves nothing behind. A failed claim leavesdestinationCreatedfalse, so the cleanup never deletes a file that was already there — asserted by a test.backup --out <same path>runs (a cron job and an operator) previously both dumped before one lost. The second is now refused immediately.EEXISTis translated. Claiming earlier widens the window in which a SIGKILL leaves an empty file at the claimed path, so that path stops being a bare errno and says what is there and what to do.reserveBackupDestinationkeeps its untranslated contract — the existing test still pins{ code: 'EEXIST' }— with the translation inclaimBackupDestinationbeside MEI-104's permission one.Docs
#271's own prose is what this falsifies, so both places are corrected in the same commit:
operating.md§Backupupgrading.mdEACCESafter the dump, two lines before this runbook destroys the volume"I grepped for any other doc still describing the late failure; there were none.
Tests
refuses an occupied path with the remedy, not a bare errno (MEI-149)— asserts the message names the path, says what is there, mentions--out, and does not leakEEXIST.leaves the occupying file alone — refusing is not deleting somebody's bundle— writes real content to the path, asserts the refusal, then asserts the content is byte-identical. This is the one that would catch a carelessfinally.reserveBackupDestination's raw-EEXISTcontract both still pass unchanged.Validation
pnpm verifypasses (exit 0) — 468 test files, 8197 tests, lint, guards, all three typechecks.mainviagit stash.pnpm comments:check,pnpm docs:links:check,pnpm site:docs:checkpass. Formatted only the files touched.Backup and restore round-tripCI job exercises the realcommunity backuphappy path; unchanged by this, since the claim succeeds and the ordering is invisible on success.One thing left as-is, deliberately
ENOENT— an--outinside a directory that does not exist — stays a raw errno. It now surfaces immediately like the others, and the message already names the full path, so a third translation looked like noise rather than help. Easy to add if you disagree.Generated by Claude Code