fix(cli): claim the backup destination before dumping, not after (#273)

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.


Claude-Session: https://claude.ai/code/session_01GVdrZfcwVhvJpZWUKWxYs9

Co-authored-by: Claude <noreply@anthropic.com>

Author: jouwdan

apps/cli/src/backup.test.ts

@@ -1,4 +1,4 @@
-import { mkdir, mkdtemp, readdir, readFile, rm, stat } from 'node:fs/promises'
+import { mkdir, mkdtemp, readdir, readFile, rm, stat, writeFile } from 'node:fs/promises'
 import { tmpdir } from 'node:os'
 import path from 'node:path'
 
@@ -257,12 +257,37 @@ describe('claimBackupDestination', () => {
     }
   })
 
-  it('lets a non-permission failure through undisguised — an existing bundle is still EEXIST', async () => {
+  it('refuses an occupied path with the remedy, not a bare errno (MEI-149)', async () => {
     const dir = await mkdtemp(path.join(tmpdir(), 'meith-backup-test-'))
     try {
       const destination = path.join(dir, 'board.tar.gz')
       await claimBackupDestination(destination)
-      await expect(claimBackupDestination(destination)).rejects.toMatchObject({ code: 'EEXIST' })
+
+      let caught: unknown
+      try {
+        await claimBackupDestination(destination)
+      } catch (error) {
+        caught = error
+      }
+
+      const message = (caught as Error).message
+      expect(message).toContain(destination)
+      expect(message).toContain('something is already there')
+      expect(message).toContain('--out')
+      expect(message).not.toContain('EEXIST')
+    } finally {
+      await rm(dir, { recursive: true, force: true })
+    }
+  })
+
+  it("leaves the occupying file alone — refusing is not deleting somebody's bundle", async () => {
+    const dir = await mkdtemp(path.join(tmpdir(), 'meith-backup-test-'))
+    try {
+      const destination = path.join(dir, 'board.tar.gz')
+      await writeFile(destination, 'an earlier bundle')
+
+      await expect(claimBackupDestination(destination)).rejects.toThrow()
+      expect(await readFile(destination, 'utf8')).toBe('an earlier bundle')
     } finally {
       await rm(dir, { recursive: true, force: true })
     }

apps/cli/src/backup.ts

@@ -387,6 +387,14 @@ export async function claimBackupDestination(destination: string): Promise<void>
   try {
     await reserveBackupDestination(destination)
   } catch (error) {
+    if ((error as NodeJS.ErrnoException | undefined)?.code === 'EEXIST') {
+      throw new ValidationError(
+        `backup will not write over ${destination}: something is already there. Move it aside ` +
+          'or pass a different --out. A previous run killed part-way through can leave an ' +
+          'empty or truncated bundle at the path it had claimed; that file is not a backup ' +
+          'and is safe to delete.',
+      )
+    }
     translateWriteError(error, {
       command: 'backup',
       path: destination,
@@ -535,6 +543,9 @@ export async function backupCommand(args: readonly string[]): Promise<number> {
   const stage = await mkdtemp(path.join(tmpdir(), 'meith-backup-'))
   let destinationCreated = false
   try {
+    await claimBackupDestination(out)
+    destinationCreated = true
+
     console.log(
       env.DIRECT_DATABASE_URL === undefined
         ? 'Dumping the database…'
@@ -564,8 +575,6 @@ export async function backupCommand(args: readonly string[]): Promise<number> {
 
     const members = ['manifest.json', 'db.dump']
     if (uploads === 'included') members.push('uploads.tar.gz')
-    await claimBackupDestination(out)
-    destinationCreated = true
     await run('tar', ['czf', out, '-C', stage, ...members])
     await chmod(out, 0o600)
 

docs/guides/operations/operating.md

@@ -147,7 +147,9 @@ docker compose run --rm --no-deps --user "$(id -u):$(id -g)" -v "$PWD/backups":/
 
 `community backup --help` prints the flags the installed release actually has — include uploads when they use the local volume, and copy at least one version off the server.
 
-**Neither the `mkdir` nor the `--user` is optional**, and they are needed for the same reason `board:eject` needs them (see [the marketplace](../../customization/marketplace.md#moving-to-a-custom-board)). The image runs as a fixed, non-root account — `nextjs`, uid 1001 — which owns nothing on your host, so it can only write into a directory that account can already write to. Creating `backups` yourself first means it belongs to you rather than being auto-created root-owned by Docker; `--user "$(id -u):$(id -g)"` then makes the account doing the writing the account that owns the directory. Without them the run ends in `EACCES: permission denied` — and it ends there *after* the database dump, with no bundle to show for it. The command says so if it happens, and points back here.
+**Neither the `mkdir` nor the `--user` is optional**, and they are needed for the same reason `board:eject` needs them (see [the marketplace](../../customization/marketplace.md#moving-to-a-custom-board)). The image runs as a fixed, non-root account — `nextjs`, uid 1001 — which owns nothing on your host, so it can only write into a directory that account can already write to. Creating `backups` yourself first means it belongs to you rather than being auto-created root-owned by Docker; `--user "$(id -u):$(id -g)"` then makes the account doing the writing the account that owns the directory. Without them the run ends in `EACCES: permission denied`, saying which directory needs write access and pointing back here.
+
+**It ends there before dumping anything.** `backup` claims its destination as its first act — creating the file, empty and mode `0600`, before it connects to the database — so a path it cannot write is refused in under a second rather than after a dump that may take minutes. That also means two backups aimed at the same path cannot both run: the second is refused immediately rather than dumping and then losing. The command **never writes over an existing file**; if a previous run was killed part-way through it can leave an empty or truncated file at the path it had claimed, and the next run refuses that path by name. Such a file is not a backup and is safe to delete.
 
 Reading a bundle back needs neither addition: files inside the image are world-readable, so [Restore](#restore) and [disaster recovery](./disaster-recovery.md) mount `/backup` and read from it as they are.
 

docs/guides/operations/upgrading.md

@@ -219,8 +219,9 @@ docker compose up -d --build
 The `mkdir` and the `--user` are what let the container write the bundle
 onto your host at all — the image runs as uid 1001, which owns nothing
 there; [Backup](./operating.md#backup) explains it in full. Get them
-wrong and the run fails with `EACCES` *after* the dump, two lines before
-this runbook destroys the volume.
+wrong and the run refuses immediately, before it dumps anything, naming
+the directory that needs write access — so you find out here rather than
+two lines further down, where this runbook destroys the volume.
 
 The new image's `pg_dump` reads the old server fine — clients dump any
 older server, which is why the backup comes from the *new* build against