commit 3d17e85a9d5de84eed58928485187c6065263de9
parent 390024e2b67eb08797e96cea31d0b0546f27d067
Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st>
Date: Thu, 17 Sep 2026 15:22:48 -0400
storage: a channel killed mid-re-point is half done, not broken
Between the symlink and the config write there is one instant where the link
names the new target and config.json still names the old one. A process killed
there leaves an `inconsistent` channel that is STILL on the old root — so
every later re-point was refused wholesale, naming a state whose only remedy
was the job being refused.
The preflight now recognises that shape: when the link already points exactly
where this run would point it, the channel is resumable, and the job writes
its config and does not touch the link. The ledger records nothing for the
link, because this run did not move it — a rollback must undo what it did,
and unlinking a link it found in the right place would turn a half-done
channel into one with no data/ at all.
The identity written for the new root is now the one the preflight MEASURED
there, not a mountpoint derived from the old record. The derivation stays as
the fallback for when there was nothing to measure (no findmnt, a container,
no recorded uuid to probe against).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Diffstat:
2 files changed, 139 insertions(+), 14 deletions(-)
diff --git a/common/controller/storageLocations.test.ts b/common/controller/storageLocations.test.ts
@@ -373,6 +373,61 @@ test("a rerun after a crash finishes the channels that were left", async () => {
});
});
+test("a channel killed between its symlink and its config write is resumed, not refused", async () => {
+ await withTmp(async (h) => {
+ await seedRelocated(h, "alpha", h.rootA);
+ await seedRelocated(h, "beta", h.rootA);
+ await mkdir(path.join(h.rootB, "alpha", "data"), { recursive: true });
+ await mkdir(path.join(h.rootB, "beta", "data"), { recursive: true });
+ await setLocations(h, [loc("cold", h.rootA)]);
+
+ // THE ONE-INSTRUCTION WINDOW. alpha's link was moved and the process died
+ // before its config was written: link new, config old. inspect() calls that
+ // `inconsistent`, and alpha is still "on" the old root — so a preflight
+ // that only accepted ok/unreachable would refuse the whole location for
+ // ever, naming a state whose only remedy is the job it is refusing.
+ const alphaLink = path.join(h.paths.channelsDir, "alpha", "data");
+ await rm(alphaLink);
+ await symlink(path.join(h.rootB, "alpha", "data"), alphaLink);
+
+ const pre = await preflightRepoint({
+ paths: h.paths,
+ locationId: "cold",
+ newRoot: h.rootB,
+ io: h.io,
+ });
+ assert.deepEqual(pre.problems, []);
+ assert.deepEqual(pre.channels, ["alpha", "beta"]);
+ assert.deepEqual(pre.resumable, ["alpha"]);
+
+ const lines: string[] = [];
+ await repointStorageLocation({
+ paths: h.paths,
+ locationId: "cold",
+ newRoot: h.rootB,
+ io: h.io,
+ onLog: (l) => lines.push(l),
+ });
+
+ // alpha's config caught up with its link (which was never touched again),
+ // beta went the ordinary way, and the location moved.
+ assert.equal(
+ (await readChannelConfig(h.paths, "alpha"))?.dataDir,
+ path.join(h.rootB, "alpha", "data"),
+ );
+ assert.equal(
+ await readlink(path.join(h.paths.channelsDir, "alpha", "data")),
+ path.join(h.rootB, "alpha", "data"),
+ );
+ assert.equal(
+ (await readChannelConfig(h.paths, "beta"))?.dataDir,
+ path.join(h.rootB, "beta", "data"),
+ );
+ assert.equal(h.settings().storage.locations[0].root, h.rootB);
+ assert.ok(lines.some((l) => l.includes("link was already moved")));
+ });
+});
+
test("preflight refuses an in-transition channel and a root that is not a directory", async () => {
await withTmp(async (h) => {
await seedRelocated(h, "alpha", h.rootA);
diff --git a/common/controller/storageLocations.ts b/common/controller/storageLocations.ts
@@ -1,5 +1,5 @@
import path from "node:path";
-import { stat, symlink, unlink } from "node:fs/promises";
+import { readlink, stat, symlink, unlink } from "node:fs/promises";
import { getPaths, type Paths } from "../lib/paths";
import {
getSettings,
@@ -262,11 +262,40 @@ export type RepointPreflight = {
problems: string[];
// The channels that would be re-pointed, in the order the job would do it.
channels: string[];
+ // A SUBSET of `channels`: the ones whose symlink ALREADY points at the new
+ // target while config.json still names the old one. That is the state a crash
+ // in the one-instruction window between the symlink and the config write
+ // leaves behind, and it reads as `inconsistent` — so without this the channel
+ // stays "on" the old root for ever and every later re-point is refused
+ // wholesale, naming a state with no remedy. For these the job writes the
+ // config and does NOT touch the link: the link is already right.
+ resumable: string[];
+ // The identity the new root actually has, when the preflight was able to
+ // probe it (bins passed AND the location has a recorded uuid to compare
+ // against). The job writes THIS rather than deriving a mountpoint from the
+ // old record — it is a measurement, not an inference.
+ newVolume?: StorageVolume;
locationId: string;
oldRoot: string;
newRoot: string;
};
+// Does `channels/<slug>/data` already point exactly where a re-point would put
+// it? lstat/readlink, never stat: the target may not exist yet either, and a
+// stat would call a perfectly good link missing.
+async function linkAlreadyAt(
+ paths: Paths,
+ slug: string,
+ newTarget: string,
+): Promise<boolean> {
+ try {
+ const linkTarget = await readlink(path.join(paths.channelsDir, slug, "data"));
+ return path.resolve(linkTarget) === path.resolve(newTarget);
+ } catch {
+ return false;
+ }
+}
+
async function isDirectory(p: string): Promise<boolean> {
try {
return (await stat(p)).isDirectory();
@@ -296,6 +325,7 @@ export async function preflightRepoint(opts: {
ok: false,
problems: [],
channels: [],
+ resumable: [],
locationId: opts.locationId,
oldRoot: loc?.root ?? "",
newRoot,
@@ -336,6 +366,14 @@ export async function preflightRepoint(opts: {
opts.bins,
opts.probeOptions ?? {},
);
+ if (probe.identity.known && probe.identity.uuid === loc.volume.uuid) {
+ // The same disk, measured at the new root: uuid, fstype, label, the
+ // mountpoint it is actually on and the root's path relative to it. This
+ // is what the job stores, so `root === join(mountpoint, relPath)` holds
+ // because it was observed and not reconstructed.
+ const { known: _known, ...volume } = probe.identity;
+ base.newVolume = volume;
+ }
if (probe.identity.known && probe.identity.uuid !== loc.volume.uuid) {
base.problems.push(
`${newRoot} is on a different disk: it is on UUID ` +
@@ -363,7 +401,20 @@ export async function preflightRepoint(opts: {
);
continue;
}
- if (media.status !== "ok" && media.status !== "unreachable") {
+ // THE CRASH WINDOW, RECOGNISED RATHER THAN REFUSED. Between the symlink and
+ // the config write there is one instant where the link names the new target
+ // and config.json still names the old one. A process killed there leaves an
+ // `inconsistent` channel that is still "on" the old root — so the next
+ // re-point would refuse the whole location over a state whose only remedy
+ // is the job being refused. When the link already points exactly where this
+ // run would point it, the channel is not broken: it is half done, and the
+ // remaining half is the config write this job performs anyway.
+ const resumable = await linkAlreadyAt(
+ opts.paths,
+ slug,
+ relocatedDataDir(newRoot, slug),
+ );
+ if (!resumable && media.status !== "ok" && media.status !== "unreachable") {
base.problems.push(
`${slug}: its media reads as ${media.status} — ` +
`${media.detail ?? "disk and config do not agree"}. A re-point ` +
@@ -391,6 +442,7 @@ export async function preflightRepoint(opts: {
continue;
}
base.channels.push(slug);
+ if (resumable) base.resumable.push(slug);
}
// THE MISSING TARGETS ARE ONE REFUSAL, NOT n. A root that holds none of the
@@ -524,28 +576,42 @@ export async function repointStorageLocation(opts: {
};
ledger.push(entry);
const link = path.join(opts.paths.channelsDir, slug, "data");
+ // RESUMING SKIPS THE LINK, and must: it already points at newTarget, so
+ // unlinking and recreating it would be two syscalls to reach the state it
+ // is in — and a crash between them would turn a half-done channel into a
+ // channel with no `data/` at all, which is strictly worse than what we
+ // found. The ledger records nothing for the link for the same reason: a
+ // rollback must undo what THIS run did, and this run did not move it.
+ const resuming = pre.resumable.includes(slug);
try {
- await unlink(link);
- entry.unlinked = true;
- await symlink(newTarget, link);
- entry.relinked = true;
+ if (!resuming) {
+ await unlink(link);
+ entry.unlinked = true;
+ await symlink(newTarget, link);
+ entry.relinked = true;
+ }
await writeChannelConfig(opts.paths, slug, {
...(fresh as ChannelConfig),
dataDir: newTarget,
});
entry.configWritten = true;
} catch (err) {
- const step = !entry.unlinked
- ? "removing the old symlink"
- : !entry.relinked
- ? "creating the new symlink"
- : "writing config.json";
+ const step = resuming
+ ? "writing config.json (resuming a half-finished channel)"
+ : !entry.unlinked
+ ? "removing the old symlink"
+ : !entry.relinked
+ ? "creating the new symlink"
+ : "writing config.json";
throw new Error(
`${slug}: failed while ${step} — ${(err as Error).message}`,
{ cause: err },
);
}
- log(`${slug}: ${oldTarget} -> ${newTarget}`);
+ log(
+ `${slug}: ${oldTarget} -> ${newTarget}` +
+ (resuming ? " (link was already moved — finished its config)" : ""),
+ );
}
// The location last, from a FRESH read: the per-channel loop above wrote no
@@ -564,7 +630,10 @@ export async function repointStorageLocation(opts: {
// `root === join(mountpoint, relPath)` holds again — a refresh that
// wrote it before the re-point would record a mountpoint the root is
// not under.
- const volume = volumeForNewRoot(l, pre.newRoot);
+ // The measurement first, the inference only when there was nothing
+ // to measure (no findmnt, a container, a location with no recorded
+ // uuid to probe against).
+ const volume = pre.newVolume ?? volumeForNewRoot(l, pre.newRoot);
return {
...l,
root: pre.newRoot,
@@ -598,7 +667,8 @@ export async function repointStorageLocation(opts: {
};
}
-// The stored identity, re-anchored to the new root. The uuid/fstype/label are
+// THE FALLBACK, when the preflight had no probe to take. The stored identity,
+// re-anchored to the new root. The uuid/fstype/label are
// the disk's and do not change; the mountpoint and relPath are the halves that
// do, and they are recomputed from the new root so the invariant
// `root === join(mountpoint, relPath)` survives the re-point. A location with
