diff --git a/README.md b/README.md index 1b11afc..e7aba8f 100644 --- a/README.md +++ b/README.md @@ -490,6 +490,16 @@ appears in. On subsequent runs, existing originals are skipped. If a download fails, the error is logged and the backup continues with the next file. The exit code is non-zero if any files failed. +Each original is copied to a temporary file named +`.quak-backup-.--.tmp` in the same directory, synced +to disk, and renamed into place, so an original is either complete or absent, +even after a power cut. A run that is killed can leave one of these temporary +files behind; the next backup deletes those whose process is no longer running. +Downloads and the content cache use the same scheme with `.quak-.tmp` +names. The rename replaces whatever was at the destination rather than writing +through it: a symlink there is replaced, not followed, and the new file has the +temporary file's permissions, not those of the file it replaced. + ## TODO - [x] Retry policy: no retry on 4xx, exponential backoff on 5xx and network diff --git a/TODO.md b/TODO.md index 49e8112..d78beaa 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,13 @@ Tag v1.0.0. # Completed Steps +- 2026-09-23: Hardened the backup tree's atomic copy (issue 22). `copyAtomic` + fsyncs its temp file before the rename and the directory after it, through the + download writer's `fsyncPath`; each backup run deletes `.quak-backup-*.tmp` + files whose process is no longer running. The README backup layout names the + temp files and states that the rename replaces a symlink and takes the temp + file's permissions. Added tests for a missing and an unwritable destination + directory for `downloadFile` and `downloadThumbnail`. - 2026-09-22: Hardened the client session lifecycle (issue 10). `Client.fromJSON` checks every snapshot field and each key's decoded length and names the bad field; `toJSON` reads the token through diff --git a/src/backup.ts b/src/backup.ts index 655a41c..71565f0 100644 --- a/src/backup.ts +++ b/src/backup.ts @@ -29,19 +29,20 @@ // rather than counted forever, which would poison a scheduled backup's exit code. import { - copyFileSync, lstatSync, mkdirSync, + readdirSync, readFileSync, readlinkSync, - renameSync, rmSync, statSync, symlinkSync, writeFileSync, } from "node:fs"; +import { copyFile, rename, rm } from "node:fs/promises"; import { basename, dirname, join, relative } from "node:path"; +import { fsyncPath } from "./download/index.js"; import { safeExtension, sanitizeFileName } from "./filename.js"; import type { Collection, EnteFile } from "./model/types.js"; @@ -154,8 +155,12 @@ const errorMessage = (err: unknown): string => err instanceof Error ? err.message : String(err); // Copy bytes into `dest` via a temp file in the same directory plus rename, so -// `dest` appears only once it is whole ("present means complete"). -const copyAtomic = (src: string, dest: string): void => { +// `dest` appears only once it is whole ("present means complete"). As in the +// download writer, the temp file is fsynced before the rename and the directory +// after it, so a power cut cannot leave a correctly named but short original. +// The temp name carries this process's ID so a later run can tell a leftover +// from a copy still in progress (see `removeLeftoverTempFiles`). +const copyAtomic = async (src: string, dest: string): Promise => { if (src === dest) return; const tmp = join( dirname(dest), @@ -164,10 +169,45 @@ const copyAtomic = (src: string, dest: string): void => { .slice(2)}.tmp`, ); try { - copyFileSync(src, tmp); - renameSync(tmp, dest); + await copyFile(src, tmp); + await fsyncPath(tmp); + // `rename` replaces the destination's directory entry: an existing + // symlink at `dest` is replaced, not followed, and the new file has + // the temp file's permissions (copied from `src`). + await rename(tmp, dest); + await fsyncPath(dirname(dest)); } finally { - rmSync(tmp, { force: true }); + await rm(tmp, { force: true }); + } +}; + +// A process-ID check: signal 0 delivers nothing and only reports whether the +// process exists. EPERM means it exists but belongs to another user. +const isRunning = (pid: number): boolean => { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return (err as NodeJS.ErrnoException).code === "EPERM"; + } +}; + +// Delete the temp files `copyAtomic` leaves behind when a backup is killed +// before its rename. Only files whose process is no longer running are +// removed, so a backup running at the same time keeps its own. A reused +// process ID can only keep a leftover a while longer, never remove a live one. +const removeLeftoverTempFiles = (dir: string): void => { + let names: string[]; + try { + names = readdirSync(dir); + } catch { + return; + } + for (const name of names) { + const match = /^\.quak-backup-.*-(\d+)-[0-9a-z]*\.tmp$/.exec(name); + if (match && !isRunning(Number(match[1]))) { + rmSync(join(dir, name), { force: true }); + } } }; @@ -254,6 +294,8 @@ export const runBackup = async ( mkdirSync(originalsDir, { recursive: true }); mkdirSync(collectionsDir, { recursive: true }); if (includeThumbnails) mkdirSync(thumbnailsDir, { recursive: true }); + removeLeftoverTempFiles(originalsDir); + removeLeftoverTempFiles(thumbnailsDir); const ledgerPath = join(downloadDirectory, "failures.json"); const ledger = loadLedger(ledgerPath); @@ -321,7 +363,7 @@ export const runBackup = async ( try { log(`Fetching original ${file.metadata.title} (${fileID})...`); const { path } = await lib.original(fileID); - copyAtomic(path, dest); + await copyAtomic(path, dest); downloaded++; } catch (err) { log( @@ -342,7 +384,7 @@ export const runBackup = async ( if (isPresent(dest)) continue; try { const { path } = await lib.thumbnail(fileID); - copyAtomic(path, dest); + await copyAtomic(path, dest); } catch (err) { recordFailure( file, diff --git a/src/download/index.ts b/src/download/index.ts index bb3f16e..d3a82f5 100644 --- a/src/download/index.ts +++ b/src/download/index.ts @@ -158,6 +158,18 @@ const streamDecrypt = async ( return totalPlain; }; +// Fsync a file or a directory, so its contents (for a directory, its entries) +// are on stable storage. Exported for the backup tree's copy, which needs the +// same durability as the writer below. +export const fsyncPath = async (path: string): Promise => { + const handle = await open(path, "r"); + try { + await handle.sync(); + } finally { + await handle.close(); + } +}; + // Stage a write to `destination` atomically and durably, then rename it into // place. `fill` writes the contents into the open temp file handle — either the // whole buffer at once (`writeAtomic`) or chunk by chunk as they decrypt @@ -193,16 +205,15 @@ const stageAtomic = async ( } finally { await handle.close(); } + // `rename` replaces the destination's directory entry rather than + // writing through it: an existing symlink at `destination` is + // replaced, not followed, and the new file has the temp file's + // permissions, not those of the file it replaced. await rename(tmpPath, destination); // Fsync the directory so the rename itself survives a crash: renaming // over a synced temp file still leaves the new directory entry in the // page cache until the directory is synced. - const dirHandle = await open(dir, "r"); - try { - await dirHandle.sync(); - } finally { - await dirHandle.close(); - } + await fsyncPath(dir); } catch (err) { // Best-effort cleanup. A failure to remove the temporary file must // never replace the error that actually explains what went wrong. diff --git a/test/cli/backup.test.ts b/test/cli/backup.test.ts index 7ae5e97..49815dd 100644 --- a/test/cli/backup.test.ts +++ b/test/cli/backup.test.ts @@ -42,15 +42,44 @@ import { rmSync, writeFileSync, } from "node:fs"; +import { spawnSync } from "node:child_process"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { Library } from "../../src/library/index.js"; import type { ContentSource } from "../../src/library/content.js"; import type { CollectionsPage, FilesPage } from "../../src/client.js"; import type { Collection, EnteFile } from "../../src/model/types.js"; +// `open` and `rename` are wrapped to record, in order, every fsync and rename, +// so a test can pin the sequence "fsync the temp file, rename, fsync the +// directory" that makes a copied original survive a power cut. `vi.hoisted` +// because `vi.mock` factories run before module-level constants exist. +const fsEvents = vi.hoisted(() => [] as string[]); + +vi.mock("node:fs/promises", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + open: async ( + ...args: Parameters + ): Promise>> => { + const handle = await actual.open(...args); + const realSync = handle.sync.bind(handle); + handle.sync = async (): Promise => { + fsEvents.push(`sync:${String(args[0])}`); + await realSync(); + }; + return handle; + }, + rename: async (from: string, to: string): Promise => { + fsEvents.push(`rename:${to}`); + await actual.rename(from, to); + }, + }; +}); + const USER_ID = 42; // Decrypted-byte length each stub original writes, keyed by fileID. @@ -530,4 +559,44 @@ describe("lib.backup", () => { expect(readLedger(outDir).files["101"]!.attempts).toBe(1); lib.close(); }); + + it("fsyncs a copied original before the rename and its directory after", async () => { + const lib = await openLibrary(stubSource()); + const outDir = join(root, "backup"); + const originals = join(outDir, "originals"); + const dest = join(originals, "100.jpg"); + fsEvents.length = 0; + + await lib.backup({ downloadDirectory: outDir }); + + const at = fsEvents.indexOf(`rename:${dest}`); + expect(at).toBeGreaterThan(0); + expect(fsEvents[at - 1]).toMatch( + /^sync:.*\/\.quak-backup-100\.jpg-\d+-[0-9a-z]*\.tmp$/, + ); + expect(fsEvents[at + 1]).toBe(`sync:${originals}`); + lib.close(); + }); + + it("removes temp files left by a killed backup but not those of one still running", async () => { + const outDir = join(root, "backup"); + const originals = join(outDir, "originals"); + mkdirSync(originals, { recursive: true }); + // A child that has already exited: its process ID is not running. + const exitedPID = spawnSync(process.execPath, ["-e", ""]).pid; + const leftover = `.quak-backup-100.jpg-${exitedPID}-abc123.tmp`; + // This test's own process stands in for a backup running at the same + // time. + const inProgress = `.quak-backup-101.jpg-${process.pid}-def456.tmp`; + writeFileSync(join(originals, leftover), "partial"); + writeFileSync(join(originals, inProgress), "partial"); + const lib = await openLibrary(stubSource()); + + await lib.backup({ downloadDirectory: outDir }); + + const names = readdirSync(originals); + expect(names).not.toContain(leftover); + expect(names).toContain(inProgress); + lib.close(); + }); }); diff --git a/test/download/download.test.ts b/test/download/download.test.ts index 52ca6f0..ae25b0e 100644 --- a/test/download/download.test.ts +++ b/test/download/download.test.ts @@ -48,6 +48,7 @@ */ import { + chmodSync, existsSync, mkdirSync, readdirSync, @@ -989,6 +990,51 @@ describe.each(entryPoints)( expect(readFileSync(outPath)).toEqual(Buffer.from(existing)); expect(readdirSync(dir)).toEqual(["rename-fails.bin"]); }); + + it("fails without creating anything when the destination directory does not exist", async () => { + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptFileBody( + patternBytes(64, 33), + key, + ); + const { api, file } = fixtureFor(key, header, ciphertext); + const dir = freshDir(); + const outPath = join(dir, "missing", "never.bin"); + + await expect(download(api, file, outPath)).rejects.toMatchObject({ + code: "ENOENT", + }); + + // The missing directory is not created on the caller's behalf. + expect(readdirSync(dir)).toEqual([]); + }); + + // Root ignores directory permissions, so this cannot fail as root + // (the Docker test image runs as root). + it.skipIf(process.getuid?.() === 0)( + "fails without creating anything when the destination directory is not writable", + async () => { + const key = + sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptFileBody( + patternBytes(64, 34), + key, + ); + const { api, file } = fixtureFor(key, header, ciphertext); + const dir = freshDir(); + const outPath = join(dir, "never.bin"); + chmodSync(dir, 0o500); + try { + await expect( + download(api, file, outPath), + ).rejects.toMatchObject({ code: "EACCES" }); + } finally { + chmodSync(dir, 0o700); + } + + expect(readdirSync(dir)).toEqual([]); + }, + ); }, );