diff --git a/README.md b/README.md index 14e69c6..1507d67 100644 --- a/README.md +++ b/README.md @@ -521,10 +521,13 @@ Each original is copied to a temporary file named 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. +Downloads and the content cache use the same scheme with +`.quak--.tmp` names, and opening a library deletes those in the +cache whose process is no longer running, so a download another process has in +progress in the same cache is left alone. 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 diff --git a/TODO.md b/TODO.md index 9335d2c..96a74ff 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,13 @@ Tag v1.0.0. # Completed Steps +- 2026-09-23: Opening a library no longer deletes another process's download in + progress (issue 105). The download writer's temp files are named + `.quak--.tmp`, and `removeLeftoverTempFiles`, moved from the + backup into the download module, deletes a `.quak-*.tmp` file only when the + process ID in its name is no longer running. The content cache calls it at + `open()` for `originals/` and `thumbnails/`, the backup as before. Temp files + written before this change carry no process ID and are no longer deleted. - 2026-09-23: Fixed the backup's per-collection folders (issue 103). Two files in one collection with the same title, and two collections with the same name, each get their ID added to the name (`IMG_0001 (12345).JPG`, `Trip (10)/`), so diff --git a/src/backup.ts b/src/backup.ts index 78df900..5e90f8b 100644 --- a/src/backup.ts +++ b/src/backup.ts @@ -45,7 +45,7 @@ import { import { copyFile, rename, rm } from "node:fs/promises"; import { basename, dirname, extname, join, relative } from "node:path"; -import { fsyncPath } from "./download/index.js"; +import { fsyncPath, removeLeftoverTempFiles } from "./download/index.js"; import { safeExtension, sanitizeFileName } from "./filename.js"; import type { Collection, EnteFile } from "./model/types.js"; @@ -184,36 +184,6 @@ const copyAtomic = async (src: string, dest: string): Promise => { } }; -// 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 }); - } - } -}; - // Ensure `linkPath` is a symlink to `target`, rebuilding a missing, wrong, or // non-symlink entry. Throws on failure (a directory in the way, no permission) // so the caller records it and moves on rather than aborting the run. diff --git a/src/download/index.ts b/src/download/index.ts index 5345450..06cd18a 100644 --- a/src/download/index.ts +++ b/src/download/index.ts @@ -1,4 +1,5 @@ -import { randomUUID } from "node:crypto"; +import { randomBytes } from "node:crypto"; +import { readdirSync, rmSync } from "node:fs"; import { open, rename, rm } from "node:fs/promises"; import type { FileHandle } from "node:fs/promises"; import { dirname, join } from "node:path"; @@ -180,6 +181,38 @@ export const fsyncPath = async (path: string): Promise => { } }; +// 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 a killed process left in `dir`: the writer's +// `.quak--.tmp` and the backup copy's +// `.quak-backup---.tmp`. Only files whose process is no +// longer running are removed, so another process writing into the same +// directory keeps its own. A reused process ID can only keep a leftover a while +// longer, never remove a live one. +export const removeLeftoverTempFiles = (dir: string): void => { + let names: string[]; + try { + names = readdirSync(dir); + } catch { + return; + } + for (const name of names) { + const match = /^\.quak-(?:.*-)?(\d+)-[0-9a-z]*\.tmp$/.exec(name); + if (match && !isRunning(Number(match[1]))) { + rmSync(join(dir, name), { force: true }); + } + } +}; + // 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 @@ -205,8 +238,12 @@ const stageAtomic = async ( ): Promise => { const dir = dirname(destination); // The random suffix keeps concurrent downloads of the same destination - // from stepping on each other's temporary file. - const tmpPath = join(dir, `.quak-${randomUUID()}.tmp`); + // from stepping on each other's temporary file; the process ID lets + // `removeLeftoverTempFiles` tell a leftover from a write in progress. + const tmpPath = join( + dir, + `.quak-${process.pid}-${randomBytes(16).toString("hex")}.tmp`, + ); try { const handle = await open(tmpPath, "w"); try { diff --git a/src/library/content.ts b/src/library/content.ts index eb52948..ee0691e 100644 --- a/src/library/content.ts +++ b/src/library/content.ts @@ -39,6 +39,7 @@ import { downloadFile, downloadThumbnail, type ProgressCallback, + removeLeftoverTempFiles, } from "../download/index.js"; import { safeExtension } from "../filename.js"; import type { EnteFile } from "../model/types.js"; @@ -46,8 +47,6 @@ import type { Priority, RequestPools } from "./pools.js"; const DIR_MODE = 0o700; const FILE_MODE = 0o600; -const TEMP_PREFIX = ".quak-"; -const TEMP_SUFFIX = ".tmp"; const GIB = 1024 * 1024 * 1024; // Owner ruling (#36): bound the originals cache at 100 GiB, but back off when // the volume has under 50 GiB free so the cache never crowds the disk. @@ -654,6 +653,9 @@ export class ContentCache implements PhotoContent, ThumbnailsAPI { } private async scan(dir: string, into: Map): Promise { + // Another process sharing this cache may still be writing its temp + // files, so only those whose process has exited are removed. + removeLeftoverTempFiles(dir); let entries: string[]; try { entries = await readdir(dir); @@ -661,12 +663,6 @@ export class ContentCache implements PhotoContent, ThumbnailsAPI { return; } for (const name of entries) { - if (name.startsWith(TEMP_PREFIX) && name.endsWith(TEMP_SUFFIX)) { - await rm(join(dir, name), { force: true }).catch( - () => undefined, - ); - continue; - } const id = fileIDFromName(name); const path = join(dir, name); if (id !== undefined && existsSync(path)) into.set(id, path); diff --git a/test/download/download.test.ts b/test/download/download.test.ts index 304dbd9..d0a975d 100644 --- a/test/download/download.test.ts +++ b/test/download/download.test.ts @@ -1563,7 +1563,13 @@ describe("writeAtomic", () => { // directory so that new entry is on disk too. Do the directory fsync // before the rename, or skip it, and a crash can lose the rename. expect(durabilityHook.events).toHaveLength(3); - expect(durabilityHook.events[0]).toMatch(/^sync:w:.*\.tmp$/); + // The temp name carries this process's ID, so a library opening the + // same cache can tell a write in progress from a leftover. + const tempSync = durabilityHook.events[0]!; + expect(tempSync.startsWith(`sync:w:${dir}/`)).toBe(true); + expect(tempSync.slice(`sync:w:${dir}/`.length)).toMatch( + new RegExp(`^\\.quak-${process.pid}-[0-9a-f]{32}\\.tmp$`), + ); expect(durabilityHook.events[1]).toBe(`rename:${dest}`); expect(durabilityHook.events[2]).toBe(`sync:r:${dir}`); }); diff --git a/test/library/content.test.ts b/test/library/content.test.ts index 188e8a7..5db599c 100644 --- a/test/library/content.test.ts +++ b/test/library/content.test.ts @@ -33,6 +33,7 @@ import { mkdirSync, statSync, } from "node:fs"; +import { spawnSync } from "node:child_process"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -161,15 +162,28 @@ describe("ContentCache.open", () => { expect(statSync(thumbnails).mode & 0o777).toBe(0o700); }); - it("reaps orphan temp files but keeps complete content", async () => { + it("removes temp files of an exited process, keeping those of a running one and complete content", async () => { const originals = join(cacheDir, "originals"); const thumbnails = join(cacheDir, "thumbnails"); mkdirSync(originals, { recursive: true }); mkdirSync(thumbnails, { recursive: true }); - const orphan = join(originals, ".quak-abc123.tmp"); + // A child that has already exited: its process ID is not running. + const exitedPID = spawnSync(process.execPath, ["-e", ""]).pid; + const orphan = join(originals, `.quak-${exitedPID}-abc123.tmp`); + const orphanThumb = join(thumbnails, `.quak-${exitedPID}-abc456.tmp`); + // This test's own process stands in for another process still + // downloading into the same cache. + const inProgress = join(originals, `.quak-${process.pid}-def123.tmp`); + const inProgressThumb = join( + thumbnails, + `.quak-${process.pid}-def456.tmp`, + ); const complete = join(originals, "1.jpg"); const thumb = join(thumbnails, "2.jpg"); writeFileSync(orphan, "half-written"); + writeFileSync(orphanThumb, "half-written"); + writeFileSync(inProgress, "half-written"); + writeFileSync(inProgressThumb, "half-written"); writeFileSync(complete, "whole"); writeFileSync(thumb, "whole-thumb"); @@ -177,8 +191,12 @@ describe("ContentCache.open", () => { await cache.open(); expect(existsSync(orphan)).toBe(false); + expect(existsSync(orphanThumb)).toBe(false); + expect(existsSync(inProgress)).toBe(true); + expect(existsSync(inProgressThumb)).toBe(true); expect(existsSync(complete)).toBe(true); expect(existsSync(thumb)).toBe(true); + expect(cache.pathsFor(1)).toEqual({ originalPath: complete }); }); it("records already-cached files so their paths appear in pathsFor", async () => {