Keep another process's downloads when opening a library (closes #105)
check / check (push) Successful in 1m18s
check / check (push) Successful in 1m18s
The download writer's temp files are now named .quak-<pid>-<random>.tmp. removeLeftoverTempFiles moves from the backup into the download module and deletes a .quak-*.tmp file only when the process ID in its name is no longer running; the content cache calls it at open() instead of deleting every temp file, so a download in progress in another process sharing the cache survives. The README backup layout and TODO.md are updated. Model: opus-5-5
This commit was merged in pull request #120.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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
@@ -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
@@ -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 {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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}`);
|
||||
});
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user