Fix data archives and restores failing on their own lock

startDataBackupRun already holds the backup run lock, but the archive
(createDataBackup) and restoreDataBackup took it again, so every run
failed a few seconds in with "Another backup or restore is already
running". The writer and the restore now take no lock of their own.

The run's notice (archiving, restoring, or why it failed) moves to its
own line under the actions row so the buttons don't shift, and archiving
reads in gray rather than the warning colour.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

src/functions/backend/backups/create-data-backup.ts

@@ -1,21 +0,0 @@
-import withBackupRunLock from "./with-backup-run-lock";
-import writeDataBackupArchive from "./write-data-backup-archive";
-
-/**
- * Archives the whole `DATA_DIR` (database, media, computer homes, bot and
- * workspace files, MCP installs) into `BACKUP_ARCHIVE_DIR` as
- * `cobble-data-{timestamp}.tar.gz`, then keeps only the newest
- * `AppData.DataBackupsKept`. Every entry sits under one root folder named
- * after the archive (`cobble-data-{timestamp}/`), so extracting it lands
- * one folder, never a heap of files in the current directory. The
- * database's own snapshots in `db/.backups` are inside `DATA_DIR`, so they
- * ride along and are left alone on disk.
- *
- * The archive is a filesystem copy of a live volume, so the SQLite file
- * inside it is only as consistent as the moment tar read it; a consistent
- * database snapshot is what the `db/.backups` list is for. Restoring a
- * whole-data archive is `restoreDataBackup` (`restore-data-backup.ts`).
- */
-export default async function createDataBackup() {
-    return await withBackupRunLock({ task: writeDataBackupArchive });
-}

src/functions/backend/backups/data-backup-run.test.ts

@@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test";
 import AppError from "@/src/utils/app-error";
 import { BootId, grabDataBackupRun, startDataBackupRun } from "./data-backup-run";
 import { isBackupRunning } from "./with-backup-run-lock";
+import restoreDataBackup from "./restore-data-backup";
 
 function deferred() {
     let resolve!: (name: string) => void;
@@ -80,4 +81,19 @@ describe("startDataBackupRun", () => {
         await settle();
         expect(grabDataBackupRun().status).toBe("done");
     });
+
+    test("a restore runs under the run's lock instead of taking it again", async () => {
+        startDataBackupRun({
+            kind: "restore",
+            task: async () =>
+                (await restoreDataBackup({ backup_name: "missing.tar.gz" }))
+                    .restored_name,
+        });
+        await settle();
+
+        const failed = grabDataBackupRun();
+        expect(failed.status).toBe("failed");
+        expect(failed.error).toBe("Archive not found");
+        expect(isBackupRunning()).toBe(false);
+    });
 });

src/functions/backend/backups/restore-data-backup.ts

@@ -5,7 +5,6 @@ import { AppData } from "@/src/data/app-data";
 import grabDirNames from "@/src/utils/grab-dir-names";
 import writeDataBackupArchive from "./write-data-backup-archive";
 import resolveDataBackupPath from "./resolve-data-backup-path";
-import withBackupRunLock from "./with-backup-run-lock";
 
 export type DataBackupRestoreResult = {
     /**
@@ -61,89 +60,86 @@ function findArchiveRoot({ staging_dir }: RootParams) {
  * is taken as a safety copy, then the data directory is emptied (the
  * database snapshots included) and filled from the archive.
  *
+ * Runs only inside `startDataBackupRun`, which holds the backup run lock
+ * for it (taking it again here would refuse every restore).
+ *
  * The caller restarts the process afterwards: the open SQLite handle still
  * points at the replaced file, so nothing may keep reading it.
  */
 export default async function restoreDataBackup({
     backup_name,
 }: Params): Promise<DataBackupRestoreResult> {
-    return await withBackupRunLock({
-        task: async () => {
-            const { DATA_DIR } = grabDirNames();
-            const archive_path = resolveDataBackupPath({ backup_name });
-            const archive_name = path.basename(archive_path);
-            const staging_dir = fs.mkdtempSync(
-                path.join(os.tmpdir(), "cobble-restore-"),
+    const { DATA_DIR } = grabDirNames();
+    const archive_path = resolveDataBackupPath({ backup_name });
+    const archive_name = path.basename(archive_path);
+    const staging_dir = fs.mkdtempSync(
+        path.join(os.tmpdir(), "cobble-restore-"),
+    );
+
+    try {
+        const extract = Bun.spawn(
+            ["tar", "-xzf", archive_path, "-C", staging_dir],
+            {
+                cwd: process.cwd(),
+                env: process.env,
+                stdin: "ignore",
+                stdout: "pipe",
+                stderr: "pipe",
+            },
+        );
+
+        const [stdout, stderr, exit_code] = await Promise.all([
+            new Response(extract.stdout).text(),
+            new Response(extract.stderr).text(),
+            extract.exited,
+        ]);
+
+        if (exit_code !== 0) {
+            throw new Error(
+                stderr.trim() ||
+                    stdout.trim() ||
+                    `tar failed (exit ${exit_code})`,
             );
+        }
 
-            try {
-                const extract = Bun.spawn(
-                    ["tar", "-xzf", archive_path, "-C", staging_dir],
-                    {
-                        cwd: process.cwd(),
-                        env: process.env,
-                        stdin: "ignore",
-                        stdout: "pipe",
-                        stderr: "pipe",
-                    },
-                );
+        // The archive's own name may differ from its file name: an
+        // upload is saved under a fresh timestamp. Its one root
+        // folder is what holds the data, so find it rather than
+        // assume it matches.
+        const source_dir = findArchiveRoot({ staging_dir });
 
-                const [stdout, stderr, exit_code] = await Promise.all([
-                    new Response(extract.stdout).text(),
-                    new Response(extract.stderr).text(),
-                    extract.exited,
-                ]);
+        if (!fs.existsSync(path.join(source_dir, "db"))) {
+            throw new Error(
+                `The archive has no database, so nothing was restored`,
+            );
+        }
 
-                if (exit_code !== 0) {
-                    throw new Error(
-                        stderr.trim() ||
-                            stdout.trim() ||
-                            `tar failed (exit ${exit_code})`,
-                    );
-                }
+        // The archive's snapshots would otherwise vanish with the
+        // current ones, so the safety copy is taken before them.
+        const safety = await writeDataBackupArchive();
 
-                // The archive's own name may differ from its file name: an
-                // upload is saved under a fresh timestamp. Its one root
-                // folder is what holds the data, so find it rather than
-                // assume it matches.
-                const source_dir = findArchiveRoot({ staging_dir });
+        // Everything goes, the snapshots in `db/.backups` included:
+        // the archive's own set replaces them.
+        for (const entry of fs.readdirSync(DATA_DIR)) {
+            fs.rmSync(path.join(DATA_DIR, entry), {
+                recursive: true,
+                force: true,
+            });
+        }
 
-                if (!fs.existsSync(path.join(source_dir, "db"))) {
-                    throw new Error(
-                        `The archive has no database, so nothing was restored`,
-                    );
-                }
+        for (const entry of fs.readdirSync(source_dir)) {
+            fs.cpSync(
+                path.join(source_dir, entry),
+                path.join(DATA_DIR, entry),
+                { recursive: true, force: true },
+            );
+        }
 
-                // The archive's snapshots would otherwise vanish with the
-                // current ones, so the safety copy is taken before them.
-                // The run lock is already held here, hence the unlocked
-                // writer rather than `createDataBackup`.
-                const safety = await writeDataBackupArchive();
-
-                // Everything goes, the snapshots in `db/.backups` included:
-                // the archive's own set replaces them.
-                for (const entry of fs.readdirSync(DATA_DIR)) {
-                    fs.rmSync(path.join(DATA_DIR, entry), {
-                        recursive: true,
-                        force: true,
-                    });
-                }
-
-                for (const entry of fs.readdirSync(source_dir)) {
-                    fs.cpSync(
-                        path.join(source_dir, entry),
-                        path.join(DATA_DIR, entry),
-                        { recursive: true, force: true },
-                    );
-                }
-
-                return {
-                    safety_name: safety.name,
-                    restored_name: archive_name,
-                };
-            } finally {
-                fs.rmSync(staging_dir, { recursive: true, force: true });
-            }
-        },
-    });
+        return {
+            safety_name: safety.name,
+            restored_name: archive_name,
+        };
+    } finally {
+        fs.rmSync(staging_dir, { recursive: true, force: true });
+    }
 }

src/functions/backend/backups/write-data-backup-archive.ts

@@ -11,9 +11,17 @@ export type CreatedDataBackup = {
 };
 
 /**
- * Writes one archive of the whole `DATA_DIR` and prunes the old ones.
- * Callers already hold the backup run lock (`withBackupRunLock`), so a
- * restore can take its safety copy without deadlocking on it.
+ * Writes one archive of the whole `DATA_DIR` (database, media, computer
+ * workspaces, bot and workspace files, MCP installs) into
+ * `BACKUP_ARCHIVE_DIR` and keeps only the newest `AppData.DataBackupsKept`.
+ * Every entry sits under one root folder named after the archive, so
+ * extracting it lands one folder. The SQLite file inside is only as
+ * consistent as the moment tar read it; `db/.backups` is for consistent
+ * snapshots.
+ *
+ * It takes no lock: callers run inside `startDataBackupRun`, which holds
+ * the backup run lock (`withBackupRunLock`) for the whole run, so the
+ * archive route and a restore's safety copy never take it twice.
  */
 export default async function writeDataBackupArchive(): Promise<CreatedDataBackup> {
     const { DATA_DIR } = grabDirNames();

src/pages/admin/settings/backups/(data)/data-backup-run-context.ts

@@ -14,6 +14,10 @@ export type DataBackupRunState = {
      * Follows a run a request just started until it ends.
      */
     track: (run: DataBackupRun) => void;
+    /**
+     * Shows why a run couldn't start (or clears it with `undefined`).
+     */
+    report: (error?: string) => void;
 };
 
 /**
@@ -23,4 +27,5 @@ export type DataBackupRunState = {
  */
 export const DataBackupRunContext = React.createContext<DataBackupRunState>({
     track: () => {},
+    report: () => {},
 });

src/pages/admin/settings/backups/(hooks)/use-data-backup-run.ts

@@ -94,5 +94,6 @@ export default function useDataBackupRun(): DataBackupRunState {
             setError(undefined);
             setRun(next);
         },
+        report: setError,
     };
 }

src/pages/admin/settings/backups/(partials)/data-backup-list-actions.tsx

@@ -1,6 +1,5 @@
 import { Archive, Upload } from "lucide-react";
 import Button from "@moduletrace/twui/layout/Button";
-import Tag from "@moduletrace/twui/elements/Tag";
 import Row from "@moduletrace/twui/layout/Row";
 import useStatus from "@moduletrace/twui/hooks/useStatus";
 import createDataBackup from "../(functions)/create-data-backup";
@@ -10,18 +9,17 @@ import { useContext, useState } from "react";
 
 /**
  * The data archive list's two ways in: make a fresh archive, or restore
- * one from a file. Also says how the archive or restore running now is
- * going, and why the last one failed.
+ * one from a file. How a run is going, and why one failed, is shown under
+ * the row by `DataBackupRunNotice`, so the buttons never move.
  */
 export default function DataBackupListActions() {
-    const { loading, setLoading, status, setStatus } = useStatus();
-    const { run, error, track } = useContext(DataBackupRunContext);
+    const { loading, setLoading } = useStatus();
+    const { run, track, report } = useContext(DataBackupRunContext);
     const [uploading, setUploading] = useState(false);
     const archiving = run?.kind == "archive";
-    const restoring = run?.kind == "restore";
 
     const create_backup = async () => {
-        setStatus(undefined);
+        report(undefined);
         setLoading(true);
 
         const res = await createDataBackup();
@@ -34,20 +32,12 @@ export default function DataBackupListActions() {
         }
 
         if (res.msg) {
-            setStatus({ error: true, msg: res.msg });
+            report(res.msg);
         }
     };
 
     return (
         <Row className="gap-2! flex-nowrap">
-            {status?.error && <Tag color="error">{status.msg}</Tag>}
-            {!status?.error && error && <Tag color="error">{error}</Tag>}
-            {archiving && (
-                <Tag color="warning">Archiving, this can take a while...</Tag>
-            )}
-            {restoring && (
-                <Tag color="warning">Restoring, the app restarts soon...</Tag>
-            )}
             <Button
                 title="Restore from an archive file"
                 variant="outlined"

src/pages/admin/settings/backups/(partials)/data-backup-run-notice.tsx

@@ -0,0 +1,26 @@
+import { useContext } from "react";
+import Tag from "@moduletrace/twui/elements/Tag";
+import { DataBackupRunContext } from "../(data)/data-backup-run-context";
+
+/**
+ * One line under the data archives' actions: the archive or restore
+ * running now, else why the last one failed. Nothing when neither, so the
+ * actions row keeps its layout either way.
+ */
+export default function DataBackupRunNotice() {
+    const { run, error } = useContext(DataBackupRunContext);
+
+    if (run?.kind == "archive") {
+        return <Tag color="gray">Archiving, this can take a while...</Tag>;
+    }
+
+    if (run?.kind == "restore") {
+        return <Tag color="warning">Restoring, the app restarts soon...</Tag>;
+    }
+
+    if (error) {
+        return <Tag color="error">{error}</Tag>;
+    }
+
+    return null;
+}

src/pages/admin/settings/backups/(sections)/data-backups-section.tsx

@@ -4,6 +4,7 @@ import Span from "@moduletrace/twui/layout/Span";
 import SettingsGroup from "../../(partials)/settings-group";
 import { AppContext } from "@/src/pages/__root";
 import DataBackupListActions from "../(partials)/data-backup-list-actions";
+import DataBackupRunNotice from "../(partials)/data-backup-run-notice";
 import DataBackupsTable from "../(partials)/data-backups-table";
 import { DataBackupRunContext } from "../(data)/data-backup-run-context";
 import useDataBackupRun from "../(hooks)/use-data-backup-run";
@@ -33,6 +34,7 @@ export default function DataBackupsSection() {
                     </Span>
                     <DataBackupListActions />
                 </Row>
+                <DataBackupRunNotice />
                 {backups[0] ? (
                     <DataBackupsTable backups={backups} />
                 ) : (

src/pages/api/admin/backups/create-data.ts

@@ -1,5 +1,5 @@
 import grabSuperAdminUser from "@/src/functions/backend/backups/grab-super-admin-user";
-import createDataBackup from "@/src/functions/backend/backups/create-data-backup";
+import writeDataBackupArchive from "@/src/functions/backend/backups/write-data-backup-archive";
 import { startDataBackupRun } from "@/src/functions/backend/backups/data-backup-run";
 import logEvent, { grabRequestIP, logError } from "@/src/functions/backend/logger";
 import safeErrorMessage from "@/src/utils/safe-error-message";
@@ -12,7 +12,8 @@ import type {
 /**
  * Starts archiving the whole `DATA_DIR` (Settings > Backups > Data
  * archives) and answers at once: the page polls `data-status` until the
- * archive is written.
+ * archive is written. `startDataBackupRun` holds the backup run lock, so
+ * the task calls the unlocked writer.
  */
 export const handler: BunextAPIRouteHandler<
     APIResponseObject<DataBackupRun>
@@ -33,7 +34,7 @@ export const handler: BunextAPIRouteHandler<
             kind: "archive",
             task: async () => {
                 try {
-                    const backup = await createDataBackup();
+                    const backup = await writeDataBackupArchive();
 
                     await logEvent({
                         title: "Data backup created",