Keep another process's downloads when opening a library (closes #105) #120

Merged
clawbot merged 1 commits from issue-105-temp-reaping into next2 2026-09-23 07:25:29 +02:00
7 changed files with 86 additions and 49 deletions
Showing only changes of commit c5dd63ef3a - Show all commits
+7 -4
View File
@@ -516,10 +516,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-<random>.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-<pid>-<random>.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
+7
View File
@@ -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-<pid>-<random>.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.
- 2026-09-23: Re-vendored the lint and test setup from the template (issue 96).
Linting and testing are the `lint` and `test` phases of the `Dockerfile`;
`script/lint` and `script/test` each build one with `--no-cache`, and the last
+1 -31
View File
@@ -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<void> => {
}
};
// 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.
+40 -3
View File
@@ -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<void> => {
}
};
// 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-<pid>-<random>.tmp` and the backup copy's
// `.quak-backup-<name>-<pid>-<random>.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<void> => {
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 {
+4 -8
View File
@@ -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<number, string>): Promise<void> {
// 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);
+7 -1
View File
@@ -1560,7 +1560,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}`);
});
+20 -2
View File
@@ -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 () => {