From dab492da1e249aeccc73dc02b1437b2fca5771f4 Mon Sep 17 00:00:00 2001 From: christian-westerkam Date: Wed, 21 Oct 2026 00:49:47 +0200 Subject: [PATCH] Lock during updates, repair duplicate ids, keep file identity on save - CLI holds a lock file from load to save, so concurrent commands can't lose an update; stale locks from crashed runs are taken over - load() renumbers duplicate ids from the old add() bug instead of refusing the file - save() keeps owner, group and setgid on the temp file, and falls back to an in-place write for hard-linked files, files whose owner/group can't be kept, and directories that aren't writable Co-Authored-By: Claude Opus 5.5 --- .gitignore | 1 + src/cli.js | 23 +++++++++++++++++++++ src/todo.js | 51 +++++++++++++++++++++++++++++++++++++---------- test/cli.test.js | 36 +++++++++++++++++++++++++++++++-- test/todo.test.js | 34 +++++++++++++++++++++++++++---- 5 files changed, 128 insertions(+), 17 deletions(-) diff --git a/.gitignore b/.gitignore index 605e1fb..0c537b5 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ node_modules/ todos.json *.tmp +*.lock .DS_Store # Personal Claude Code overrides (not shared) diff --git a/src/cli.js b/src/cli.js index 638c469..92902d0 100755 --- a/src/cli.js +++ b/src/cli.js @@ -1,4 +1,5 @@ #!/usr/bin/env node +import { openSync, closeSync, statSync, rmSync } from "node:fs"; import { load, save, add, has, complete, remove, format } from "./todo.js"; const FILE = process.env.TODO_FILE ?? "todos.json"; @@ -17,7 +18,29 @@ function parseId(todos, arg) { return id; } +// Hold a lock file from load to save, so two commands running at once can't lose an update. +// A lock older than 10s is left over from a crashed run and is taken over. +function lock(path) { + const lockPath = `${path}.lock`; + const deadline = Date.now() + 5000; + for (;;) { + try { + closeSync(openSync(lockPath, "wx")); + process.on("exit", () => rmSync(lockPath, { force: true })); + return; + } catch (err) { + if (err.code === "EACCES" || err.code === "EPERM") return; // read-only directory: can't lock + if (err.code !== "EEXIST") fail(`Could not lock ${path}: ${err.message}`); + } + const mtime = statSync(lockPath, { throwIfNoEntry: false })?.mtimeMs; + if (mtime !== undefined && Date.now() - mtime > 10_000) rmSync(lockPath, { force: true }); + else if (Date.now() > deadline) fail(`${path} is busy: another todo command holds ${lockPath} (delete it if none is running)`); + else Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 25); + } +} + if (!["ls", "add", "done", "rm"].includes(cmd)) fail(`Unknown command: ${cmd}\n${USAGE}`); +if (cmd !== "ls") lock(FILE); let todos; try { diff --git a/src/todo.js b/src/todo.js index 5d3f754..267188e 100644 --- a/src/todo.js +++ b/src/todo.js @@ -1,5 +1,6 @@ import { - readFileSync, writeFileSync, renameSync, existsSync, lstatSync, readlinkSync, statSync, chmodSync, accessSync, rmSync, constants, + readFileSync, writeFileSync, renameSync, existsSync, lstatSync, readlinkSync, statSync, chmodSync, chownSync, accessSync, rmSync, + constants, } from "node:fs"; import { dirname, resolve } from "node:path"; import { randomBytes } from "node:crypto"; @@ -21,20 +22,30 @@ export function load(path) { if (!Array.isArray(todos)) throw new Error(`Could not read ${path}: expected a list of todos`); const bad = todos.findIndex((t) => !isTodo(t)); if (bad !== -1) throw new Error(`Could not read ${path}: entry ${bad + 1} is not a valid todo`); + // Older versions could give two todos the same id. Keep the first and renumber the rest + // after the current highest id, so every todo can be targeted on its own again. const seen = new Set(); - const dup = todos.find((t) => seen.size === seen.add(t.id).size); - if (dup) throw new Error(`Could not read ${path}: id ${dup.id} is used more than once; renumber it by hand`); - return todos; + let next = todos.reduce((max, t) => Math.max(max, t.id), 0); + return todos.map((t) => { + if (!seen.has(t.id)) { + seen.add(t.id); + return t; + } + if (!isId(++next)) throw new Error(`Could not read ${path}: no ids left to renumber duplicate id ${t.id}`); + return { ...t, id: next }; + }); } // Atomic save: write a uniquely named temp file next to the real file, then rename it into // place, so an interrupted write never truncates the list. // - Symlinks are followed by hand, so a dangling link gets its target created, not replaced. -// - The temp file is created with the real file's permissions, so a private list is never -// readable by others, even mid-write. A file the user can't write (chmod 444) is refused. -// - Trade-offs of renaming: the directory must be writable, hard links to the old file are -// not updated, and the new file is owned by whoever ran the save. +// - The temp file gets the real file's permissions from the start (so a private list is never +// readable by others mid-write) and its owner and group. +// - A file the user can't write (chmod 444) is refused, as a plain write would be. +// - Where a rename can't keep the file's identity (other hard links, an owner or group we +// can't assign, a directory we can't write), it falls back to a plain in-place write. export function save(path, todos) { + const data = JSON.stringify(todos, null, 2) + "\n"; let tmp; try { let target = path; @@ -44,10 +55,28 @@ export function save(path, todos) { } const stat = statSync(target, { throwIfNoEntry: false }); if (stat) accessSync(target, constants.W_OK); - const mode = stat ? stat.mode & 0o777 : 0o666; + const writeInPlace = () => { + if (tmp) rmSync(tmp, { force: true }); + tmp = undefined; + writeFileSync(target, data); + }; + if (stat?.nlink > 1) return writeInPlace(); + const mode = stat ? stat.mode & 0o7777 : 0o666; tmp = `${target}.${process.pid}.${randomBytes(4).toString("hex")}.tmp`; - writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n", { mode, flag: "wx" }); - if (stat) chmodSync(tmp, mode); // undo the umask, so the mode matches exactly + try { + writeFileSync(tmp, data, { mode, flag: "wx" }); + } catch (err) { + if (stat && (err.code === "EACCES" || err.code === "EPERM")) return writeInPlace(); + throw err; + } + if (stat) { + try { + chownSync(tmp, stat.uid, stat.gid); + } catch { + return writeInPlace(); + } + chmodSync(tmp, mode); // after chown, which can clear setgid; also undoes the umask + } renameSync(tmp, target); } catch (err) { if (tmp) rmSync(tmp, { force: true }); diff --git a/test/cli.test.js b/test/cli.test.js index 82d60d2..3a8d8e1 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -1,7 +1,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { spawnSync } from "node:child_process"; -import { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync } from "node:fs"; +import { spawnSync, spawn } from "node:child_process"; +import { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync, utimesSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, dirname } from "node:path"; import { fileURLToPath } from "node:url"; @@ -49,6 +49,38 @@ test("add fails readably when no ids are left, without saving", () => { assert.equal(readFileSync(file, "utf8"), before); }); +test("concurrent adds don't lose updates or reuse ids", async () => { + const file = tempFile(); + const runs = Array.from({ length: 12 }, (_, i) => + new Promise((done) => { + const child = spawn(process.execPath, [CLI, "add", `t${i}`], { env: { ...process.env, TODO_FILE: file } }); + child.on("exit", done); + })); + assert.deepEqual(await Promise.all(runs), Array(12).fill(0)); + const todos = JSON.parse(readFileSync(file, "utf8")); + assert.equal(todos.length, 12); + assert.deepEqual(todos.map((t) => t.id).sort((a, b) => a - b), Array.from({ length: 12 }, (_, i) => i + 1)); + assert.equal(existsSync(`${file}.lock`), false); +}); + +test("a stale lock from a crashed run is taken over", () => { + const file = tempFile(); + writeFileSync(`${file}.lock`, ""); + const old = new Date(Date.now() - 60_000); + utimesSync(`${file}.lock`, old, old); + const res = run(file, "add", "a"); + assert.equal(res.status, 0, res.stderr); + assert.equal(existsSync(`${file}.lock`), false); +}); + +test("rm on a file with duplicate ids removes only the first of them", () => { + const file = tempFile(); + writeFileSync(file, JSON.stringify([{ id: 2, text: "b", done: false }, { id: 2, text: "c", done: false }])); + const res = run(file, "rm", "2"); + assert.equal(res.status, 0, res.stderr); + assert.equal(res.stdout.trim(), "[ ] 3. c"); +}); + test("add with no text fails without saving", () => { const file = tempFile(); const res = run(file, "add", " "); diff --git a/test/todo.test.js b/test/todo.test.js index ff67b4d..af2cfd7 100644 --- a/test/todo.test.js +++ b/test/todo.test.js @@ -1,6 +1,6 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { mkdtempSync, writeFileSync, readdirSync, symlinkSync, lstatSync, statSync, chmodSync } from "node:fs"; +import { mkdtempSync, writeFileSync, readdirSync, symlinkSync, linkSync, lstatSync, statSync, chmodSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, dirname } from "node:path"; import { load, save, add, has, complete, remove, format } from "../src/todo.js"; @@ -70,10 +70,36 @@ test("save writes through a symlink instead of replacing it", () => { assert.deepEqual(load(real).map((t) => t.text), ["a"]); }); -test("load rejects duplicate ids left by the old add() bug", () => { +test("load renumbers duplicate ids left by the old add() bug", () => { const file = tempFile(); - writeFileSync(file, JSON.stringify([{ id: 2, text: "b", done: false }, { id: 2, text: "c", done: false }])); - assert.throws(() => load(file), /id 2 is used more than once/); + writeFileSync(file, JSON.stringify([ + { id: 2, text: "b", done: false }, + { id: 2, text: "c", done: true }, + { id: 3, text: "d", done: false }, + ])); + assert.deepEqual(load(file).map((t) => [t.id, t.text, t.done]), [[2, "b", false], [4, "c", true], [3, "d", false]]); +}); + +test("save writes in place when the file has other hard links", () => { + const file = tempFile(); + const other = join(dirname(file), "other.json"); + save(file, []); + linkSync(file, other); + save(file, add([], "a")); + assert.deepEqual(load(other).map((t) => t.text), ["a"]); +}); + +test("save writes in place when only the file, not its directory, is writable", { skip: isRoot && "root ignores file permissions" }, () => { + const file = tempFile(); + save(file, []); + chmodSync(dirname(file), 0o500); + try { + save(file, add([], "a")); + assert.deepEqual(load(file).map((t) => t.text), ["a"]); + assert.deepEqual(readdirSync(dirname(file)), ["todos.json"]); + } finally { + chmodSync(dirname(file), 0o700); + } }); test("save through a dangling symlink creates the target and keeps the link", () => {