commit f79be94fbc0ff58d6203f456c318a9f2031cda2d
parent 7563c14c9c8456b6ddd100ced0bbad6595f9c1e7
Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st>
Date: Thu, 1 Oct 2026 21:11:52 -0400
umtool: storage review fixes — own-mirror-only delete, leftover guard, no ghost mirror
dropMediaCopy deletes only the project's own mirror under a tiered media root
(untiered, the "media root" was the reports root, and a hand-made out link
into another project's out/ would have been deleted); anything else, and the
mirror after a resumed move-back, is reported as mediaCopyLeft, never deleted.
While out.moved-* or out.incoming exists, ensureOutDir and both movers refuse,
naming the leftover and the move that finishes it. ensureOutDir makes no
mirror for a project that does not exist. Tests 20 -> 24.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Diffstat:
2 files changed, 174 insertions(+), 19 deletions(-)
diff --git a/umtool/lib/report/storage.mjs b/umtool/lib/report/storage.mjs
@@ -140,6 +140,11 @@ export async function ensureOutDir(projectDir, opts = {}) {
}
if (s.kind === "other") throw new Error(`${out} exists and is not a directory`);
+ // A cut move leaves `out.moved-<ts>` or `out.incoming` beside a missing
+ // `out`. Making a fresh, empty out/ there would let the next move-out mirror
+ // it over the complete media copy (review L5): refuse, and say how to finish.
+ await assertNoLeftovers(projectDir, "out", "nothing was created");
+
const roots = rootsOf(opts);
const mirror = roots.tiered ? mediaMirror(projectDir, roots) : null;
if (!mirror) {
@@ -148,6 +153,9 @@ export async function ensureOutDir(projectDir, opts = {}) {
}
const problem = await mediaRootProblem(roots);
if (problem) throw new Error(`cannot make ${out}: ${problem}`);
+ // The project must exist before its mirror is made: otherwise the symlink
+ // fails and leaves an empty mirror on the media root (review N5).
+ if ((await pathState(projectDir)).kind !== "dir") throw new Error(`cannot make ${out}: ${projectDir} is not a directory`);
const target = path.join(/* turbopackIgnore: true */ mirror, "out");
// Recursive is safe here: the root itself was just seen to exist.
await mkdir(/* turbopackIgnore: true */ target, { recursive: true });
@@ -321,6 +329,32 @@ async function parkedOf(projectDir, name) {
.map((n) => path.join(/* turbopackIgnore: true */ projectDir, n));
}
+/** `<name>.incoming`, when a move-back left it (a cut between its copy and its rename). */
+async function incomingOf(projectDir, name) {
+ const p = path.join(/* turbopackIgnore: true */ projectDir, `${name}.incoming`);
+ return (await pathState(p)).kind === "dir" ? p : null;
+}
+
+/** Every leftover of a cut move of `<name>`: parked copies and an incoming copy. */
+export async function leftoversOf(projectDir, name) {
+ const incoming = await incomingOf(projectDir, name);
+ return [...(await parkedOf(projectDir, name)), ...(incoming ? [incoming] : [])];
+}
+
+/**
+ * Refuse while a cut move of `<name>` has left something behind. A writer that
+ * made a fresh `<name>/` there, and a move that then mirrored it over the
+ * complete copy, would lose everything but the leftover nobody names.
+ */
+async function assertNoLeftovers(projectDir, name, what) {
+ const left = await leftoversOf(projectDir, name);
+ if (!left.length) return;
+ throw new Error(
+ `a move of ${name}/ in ${projectDir} was cut (left: ${left.map((p) => path.basename(/* turbopackIgnore: true */ p)).join(", ")}) — ` +
+ `run \`umtool storage ${left.some((p) => p.endsWith(".incoming")) ? "move-back" : "move-out"} <project>\` to finish it; ${what}`,
+ );
+}
+
const defaults = (opts) => ({
rsyncBin: opts.rsyncBin ?? process.env.RSYNC_BIN ?? "rsync",
log: opts.log ?? (() => {}),
@@ -341,6 +375,10 @@ const defaults = (opts) => ({
* absent, nothing parked -> "absent", nothing to move
* a directory -> copy, mirror, verify, rename it to
* `<name>.moved-<stamp>`, link, delete the parked copy
+ * a directory + a leftover -> refused: a parked copy or `<name>.incoming`
+ * beside a real directory means a writer made a
+ * fresh one after a cut move (review L5)
+ * absent + `<name>.incoming` -> refused: a cut move-back is finished by move-back
*
* `dryRun` measures and changes nothing. Nothing in the project may be writing
* into `<name>` while it runs (the app's jobs are in its own memory, so a CLI
@@ -378,7 +416,11 @@ export async function moveDirToMedia(projectDir, name, opts = {}) {
if (s.kind === "missing") {
const parked = await parkedOf(projectDir, name);
- if (!parked.length) return { state: "absent", src, dest };
+ if (!parked.length) {
+ // A cut move-BACK is finished by move-back, never overtaken by a move-out.
+ if (await incomingOf(projectDir, name)) await assertNoLeftovers(projectDir, name, "nothing moved");
+ return { state: "absent", src, dest };
+ }
if (parked.length > 1) {
throw new Error(`${src} is missing and there are ${parked.length} parked copies (${parked.join(", ")}) — settle them by hand`);
}
@@ -394,7 +436,9 @@ export async function moveDirToMedia(projectDir, name, opts = {}) {
return { state: "moved", src, dest, resumed: true };
}
- // A real directory: the move itself.
+ // A real directory: the move itself -- unless a cut move left something
+ // behind, in which case this directory is a writer's fresh one.
+ await assertNoLeftovers(projectDir, name, "nothing moved");
const problem = await mediaRootProblem(roots);
if (problem) throw new Error(`cannot move ${src}: ${problem}`);
const measured = await measureTree(src);
@@ -416,14 +460,19 @@ export async function moveDirToMedia(projectDir, name, opts = {}) {
*
* a directory -> "already"
* absent, `<name>.incoming` -> the cut was between removing the link and
- * renaming the verified copy: rename it, then
- * delete the media copy when it can be named
+ * renaming the verified copy: rename it, and
+ * report (never delete) the project's mirror
* absent, nothing incoming -> "absent"
* a link whose target is gone -> refused: the drive is not mounted
* a link -> copy the target into `<name>.incoming`,
* mirror, verify, remove the link, rename,
- * delete the media copy and any directories
- * above it the move left empty (never the root)
+ * then delete the copy only when it is this
+ * project's own mirror under a tiered media
+ * root (and the directories above it the move
+ * left empty, never the root); any other
+ * target is left and reported (mediaCopyLeft)
+ * a parked `<name>.moved-*` -> refused: a cut move-out is finished first
+ * a directory + a leftover -> refused (a writer's fresh directory)
*
* @param {string} projectDir
* @param {string} name
@@ -438,8 +487,18 @@ export async function moveDirToLocal(projectDir, name, opts = {}) {
const incoming = path.join(/* turbopackIgnore: true */ projectDir, `${name}.incoming`);
const s = await pathState(src);
- if (s.kind === "dir") return { state: "already", src };
+ const mirror = roots.tiered ? mediaMirror(projectDir, roots) : null;
+ const ownCopy = mirror ? path.join(/* turbopackIgnore: true */ mirror, name) : null;
+ if (s.kind === "dir") {
+ await assertNoLeftovers(projectDir, name, "nothing moved");
+ // A move-back cut after its rename leaves the media copy behind (review
+ // L4): report it, never delete it blind.
+ const left = ownCopy && (await pathState(ownCopy)).kind === "dir" ? ownCopy : undefined;
+ return { state: "already", src, ...(left ? { mediaCopyLeft: left } : {}) };
+ }
if (s.kind === "other") throw new Error(`${src} is not a directory. Nothing moved.`);
+ // A cut move-OUT is finished by move-out first.
+ if ((await parkedOf(projectDir, name)).length) await assertNoLeftovers(projectDir, name, "nothing moved");
if (s.kind === "missing") {
if ((await pathState(incoming)).kind !== "dir") return { state: "absent", src };
@@ -447,10 +506,10 @@ export async function moveDirToLocal(projectDir, name, opts = {}) {
// `.incoming` outlives the link only once it verified.
log(`finishing a cut move: ${incoming} -> ${src}`);
await rename(/* turbopackIgnore: true */ incoming, src);
- const mirror = roots.tiered ? mediaMirror(projectDir, roots) : null;
- const from = mirror ? path.join(/* turbopackIgnore: true */ mirror, name) : undefined;
- if (from) await dropMediaCopy(from, roots.mediaRoot);
- return { state: "moved", src, from, resumed: true };
+ // The link is gone, so what was copied cannot be told from the disk: the
+ // project's own mirror is reported, never deleted (review L3).
+ const left = ownCopy && (await pathState(ownCopy)).kind === "dir" ? ownCopy : undefined;
+ return { state: "moved", src, resumed: true, ...(left ? { mediaCopyLeft: left } : {}) };
}
// A link.
@@ -465,18 +524,27 @@ export async function moveDirToLocal(projectDir, name, opts = {}) {
const verified = await copyMirrorVerify(from, incoming, { rsyncBin, log });
await unlink(/* turbopackIgnore: true */ src); // the link, not what it points at
await rename(/* turbopackIgnore: true */ incoming, src);
- await dropMediaCopy(from, roots.mediaRoot);
+ const left = await dropMediaCopy(from, ownCopy, roots.mediaRoot);
+ if (left) log(`left ${left} in place: it is not this project's own copy on the media root`);
log(`moved ${from} -> ${src} (${verified.files} file(s), ${gb(verified.bytes)})`);
- return { state: "moved", src, from, ...verified };
+ return { state: "moved", src, from, ...verified, ...(left ? { mediaCopyLeft: left } : {}) };
}
/**
* Delete a media copy that has been brought home, then every directory above
- * it the move left empty, stopping at -- never removing -- the media root. A
- * copy outside the media root (a link someone made by hand) is left alone.
+ * it the move left empty, stopping at -- never removing -- the media root.
+ *
+ * ONLY this project's own mirror (`ownCopy`, null when nothing is tiered), and
+ * only when the copy that came home IS that mirror (review L3). Without
+ * UMTOOL_MEDIA_DIR the "media root" would be the reports root itself, and a
+ * hand-made `out` link into another project's real out/ would be deleted after
+ * the copy. Anything else is left where it is, and its path returned so the
+ * caller can say so.
+ * @returns {Promise<string | undefined>} the path left in place, if any
*/
-async function dropMediaCopy(from, mediaRoot) {
- if (!inside(mediaRoot, from) || from === mediaRoot) return;
+async function dropMediaCopy(from, ownCopy, mediaRoot) {
+ if (!ownCopy || from !== ownCopy || !inside(mediaRoot, from) || from === mediaRoot) return from;
+ if ((await pathState(from)).kind !== "dir") return undefined;
await rm(/* turbopackIgnore: true */ from, { recursive: true, force: true });
for (let d = path.dirname(/* turbopackIgnore: true */ from); d !== mediaRoot && inside(mediaRoot, d); d = path.dirname(/* turbopackIgnore: true */ d)) {
try {
@@ -485,4 +553,5 @@ async function dropMediaCopy(from, mediaRoot) {
break; // not empty: something else lives there
}
}
+ return undefined;
}
diff --git a/umtool/lib/report/storage.test.mjs b/umtool/lib/report/storage.test.mjs
@@ -335,14 +335,99 @@ test("moveDirToLocal refuses a dangling link and finishes a cut rename", async (
assert.equal(await kind(path.join(w.projectDir, "out")), "link");
await rename(`${w.mediaRoot}.unplugged`, w.mediaRoot);
- // A cut between removing the link and renaming the verified copy.
+ // A cut between removing the link and renaming the verified copy. The link
+ // is gone, so what was copied cannot be told: the mirror is reported, kept.
execFileSync("cp", ["-a", path.join(w.mirror, "out"), path.join(w.projectDir, "out.incoming")]);
await rm(path.join(w.projectDir, "out"));
const r = await moveDirToLocal(w.projectDir, "out", w.roots);
assert.equal(r.state, "moved");
assert.equal(r.resumed, true);
assert.equal(await kind(path.join(w.projectDir, "out")), "dir");
- assert.equal(existsSync(path.join(w.mirror, "out")), false);
+ assert.equal(r.mediaCopyLeft, path.join(w.mirror, "out"));
+ assert.equal(existsSync(path.join(w.mirror, "out")), true);
+ // And from then on "already" keeps saying so (review L4).
+ const again = await moveDirToLocal(w.projectDir, "out", w.roots);
+ assert.equal(again.state, "already");
+ assert.equal(again.mediaCopyLeft, path.join(w.mirror, "out"));
+ } finally {
+ await w.done();
+ }
+});
+
+// ---------------------------------------------------------------------------
+// The review's fixes (L3, L5, N5)
+// ---------------------------------------------------------------------------
+
+test("move-back without a media root never deletes what the link pointed at (L3)", async () => {
+ const w = await world({ withOut: false });
+ try {
+ // Another project's REAL out/, and a hand-made link to it.
+ const other = path.join(w.reportsRoot, "other", "out");
+ await mkdir(other, { recursive: true });
+ await writeFile(path.join(other, "keep.mp4"), Buffer.alloc(1024, 3));
+ await symlink(other, path.join(w.projectDir, "out"));
+ const untiered = { reportsRoot: w.reportsRoot, mediaRoot: w.reportsRoot };
+ const r = await moveDirToLocal(w.projectDir, "out", untiered);
+ assert.equal(r.state, "moved");
+ assert.equal(r.mediaCopyLeft, other);
+ assert.equal(await kind(path.join(w.projectDir, "out")), "dir");
+ assert.deepEqual(await readFile(path.join(other, "keep.mp4")), Buffer.alloc(1024, 3));
+ } finally {
+ await w.done();
+ }
+});
+
+test("a tiered move-back of a link that is not the project's own mirror leaves the target (L3)", async () => {
+ const w = await world({ withOut: false });
+ try {
+ const elsewhere = path.join(w.mediaRoot, "someone-else", "out");
+ await mkdir(elsewhere, { recursive: true });
+ await writeFile(path.join(elsewhere, "x.mp4"), Buffer.alloc(512, 4));
+ await symlink(elsewhere, path.join(w.projectDir, "out"));
+ const r = await moveDirToLocal(w.projectDir, "out", w.roots);
+ assert.equal(r.mediaCopyLeft, elsewhere);
+ assert.equal(existsSync(path.join(elsewhere, "x.mp4")), true);
+ } finally {
+ await w.done();
+ }
+});
+
+test("a cut move's leftovers are a guard: no fresh out/, no move over them (L5)", async () => {
+ const w = await world({ withOut: false });
+ try {
+ // A move-out cut after the park: out/ is missing, the parked copy is the data.
+ const parked = path.join(w.projectDir, "out.moved-20261001T000000Z");
+ await mkdir(parked);
+ await writeFile(path.join(parked, "proj.mp4"), Buffer.alloc(4096, 1));
+ for (const roots of [w.roots, { reportsRoot: w.reportsRoot, mediaRoot: w.reportsRoot }]) {
+ await assert.rejects(ensureOutDir(w.projectDir, roots), /was cut \(left: out\.moved-20261001T000000Z\).*move-out <project>.*nothing was created/);
+ }
+ assert.equal(await kind(path.join(w.projectDir, "out")), "missing");
+ // A writer that made a fresh out/ anyway (an older binary): move-out refuses to mirror it over.
+ await mkdir(path.join(w.projectDir, "out"));
+ await assert.rejects(moveDirToMedia(w.projectDir, "out", w.roots), /was cut .* nothing moved/);
+ await assert.rejects(moveDirToLocal(w.projectDir, "out", w.roots), /was cut .* nothing moved/);
+ assert.deepEqual(await readdir(w.mediaRoot), []);
+ await rm(path.join(w.projectDir, "out"), { recursive: true });
+ await rm(parked, { recursive: true });
+
+ // A move-back cut after the unlink: move-out refuses; move-back finishes it.
+ await mkdir(path.join(w.projectDir, "out.incoming"));
+ await assert.rejects(moveDirToMedia(w.projectDir, "out", w.roots), /move-back <project>.*nothing moved/);
+ await assert.rejects(ensureOutDir(w.projectDir, w.roots), /left: out\.incoming/);
+ assert.equal((await moveDirToLocal(w.projectDir, "out", w.roots)).state, "moved");
+ assert.equal(await kind(path.join(w.projectDir, "out")), "dir");
+ } finally {
+ await w.done();
+ }
+});
+
+test("ensureOutDir for a project that does not exist leaves no empty mirror (N5)", async () => {
+ const w = await world({ withOut: false });
+ try {
+ const ghost = path.join(w.reportsRoot, "ghost");
+ await assert.rejects(ensureOutDir(ghost, w.roots), /is not a directory/);
+ assert.deepEqual(await readdir(w.mediaRoot), []);
} finally {
await w.done();
}
@@ -355,6 +440,7 @@ test("moveDirToLocal refuses a dangling link and finishes a cut rename", async (
test("the project walk skips out, clips and share-* (a link into an unplugged drive is never stat'd)", async () => {
assert.ok(SKIP_DIRS.has("out") && SKIP_DIRS.has("clips"));
assert.ok(skipsDir("share-emancipation") && skipsDir("clips") && !skipsDir("shares") && !skipsDir("project"));
+ assert.ok(skipsDir("out.moved-20261001T000000Z") && skipsDir("out.incoming") && !skipsDir("incoming"));
const w = await world();
try {
for (const hidden of ["clips", "share-x"]) {
