diff --git a/AGENTS.md b/AGENTS.md index cf73d68..f74c330 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -117,6 +117,28 @@ Current CLI output conventions: - `json`: structured JSON output (`task.toJSON()` / array of `toJSON()` objects) - `quiet`: suppresses non-error output +### Locking Model + +Two lock layers now exist: + +- **Project lock** (`/project.lock` via `PROJECT_LOCK_FILE`): + - Used in `Project.createTask` and `Project.deleteTask`. + - Purpose: serialize project-wide critical sections, especially next-ID allocation. + - API: `Project.lock()`, `Project.unlock(force=false)`, `Project.isLocked()`. + - `unlock()` is a no-op if missing; PID must match unless `force=true`. + +- **Per-task lock** (`.lock`): + - Used around all task-mutating operations: + - `write`, `create`, `addComment`, `updateTitle`, `updateDescription`, `updateLabels`, `deleteFile`. + - API: `Task.lock()`, `Task.unlock(force=false)`, `Task.isLocked()`. + - Same PID ownership semantics as project lock. + +Implementation notes: + +- Locks are acquired via atomic lockfile creation (`writeFile(..., { flag: "wx" })`) then PID write. +- Mutating flows release locks in `finally` using `unlock(true)` to avoid stale lockfiles on failure. +- Read paths (`view`, `list`, `search`, `Task.read`) remain lock-free. + ### Task Identifier Formats Commands accepting `` support: diff --git a/CHANGELOG.md b/CHANGELOG.md index 2092fef..0a23690 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,14 @@ All notable changes to this project will be documented in this file. ### Added -- _Nothing yet._ +- Project-wide lockfile support via `PROJECT_LOCK_FILE` (`project.lock`). +- `Project.lock()`, `Project.unlock(force?)`, and `Project.isLocked()` APIs. +- Project lock coverage for `Project.createTask()` and `Project.deleteTask()` to serialize critical sections (including ID allocation). +- Per-task lockfile support using `.lock`. +- `Task.lock()`, `Task.unlock(force?)`, and `Task.isLocked()` APIs. +- Per-task lock wrapping for all task-mutating operations: `write`, `create`, `addComment`, `updateTitle`, `updateDescription`, `updateLabels`, and `deleteFile`. +- Unit tests for project/task lock lifecycle, PID ownership checks, forced unlock behavior, and lock cleanup. +- Reference docs updates for constants/model APIs to document locking semantics. ## [v0.9.0] - 2026-05-16 diff --git a/docs/src/reference/constants.md b/docs/src/reference/constants.md index f4cc1d0..96aecb7 100644 --- a/docs/src/reference/constants.md +++ b/docs/src/reference/constants.md @@ -50,3 +50,9 @@ Shared constants used across `taskdb`, with optional environment-variable overri - Value: `32768` - Used for grouped task directory computation (`Task.groupDir`). + +### `PROJECT_LOCK_FILE: string` + +- Value: `project.lock` +- Project-wide lockfile name used by `Project.lock` / `Project.unlock` / `Project.isLocked`. +- Resolved relative to project root (`/project.lock`). diff --git a/docs/src/reference/model-project.md b/docs/src/reference/model-project.md index 83fbef5..a12d0ce 100644 --- a/docs/src/reference/model-project.md +++ b/docs/src/reference/model-project.md @@ -41,6 +41,38 @@ Lists project subdirs treated as status directories (directories not in `NON_STA Ensures `/` exists. Returns `true` when the status directory is new. +## Locking + +### `lockFilePath: string` (getter) + +Absolute path to the project lockfile: + +- `/` +- default filename is `project.lock` + +### `lock(): Promise` + +Acquires the project-wide lock: + +1. creates lockfile atomically (`wx`) +2. writes current `process.pid` into lockfile + +Throws when lock already exists or filesystem operations fail. + +### `unlock(force = false): Promise` + +Releases the project-wide lock. + +Behavior: + +- missing lockfile -> no-op +- `force = true` -> remove lockfile regardless of PID +- `force = false` -> remove only when lockfile PID matches current `process.pid`; otherwise throws + +### `isLocked(): Promise` + +Returns whether the project lockfile exists. + ## Task ID and symlink operations ### `maxTaskId(): Promise` @@ -107,6 +139,10 @@ Returns hydrated tasks sorted by id asc. Creates task with next id, writes canonical file, and creates status symlink. +This operation is wrapped in a project-wide lock (`lock()` / `unlock(true)` in `finally`) to prevent duplicate ID allocation under concurrency. + ### `deleteTask(task: Task): Promise` Removes all status symlinks and deletes canonical task file. + +This operation is wrapped in a project-wide lock (`lock()` / `unlock(true)` in `finally`). diff --git a/docs/src/reference/model-task.md b/docs/src/reference/model-task.md index 17297be..5a86e04 100644 --- a/docs/src/reference/model-task.md +++ b/docs/src/reference/model-task.md @@ -69,6 +69,35 @@ Absolute path: `///`. Absolute group directory path for this task. +### `lockFilePath: string` (getter) + +Absolute task lockfile path (`.lock`, colocated with canonical task file). + +## Locking methods + +### `lock(): Promise` + +Acquires per-task lock: + +1. creates lockfile atomically (`wx`) +2. writes current `process.pid` into lockfile + +Throws when lock already exists or filesystem operations fail. + +### `unlock(force = false): Promise` + +Releases per-task lock. + +Behavior: + +- missing lockfile -> no-op +- `force = true` -> remove lockfile regardless of PID +- `force = false` -> remove only when lockfile PID matches current `process.pid`; otherwise throws + +### `isLocked(): Promise` + +Returns whether the task lockfile exists. + ## Instance methods ### `buildBody(): string` @@ -83,6 +112,8 @@ Builds full file text via `gray-matter.stringify` (frontmatter + body). Ensures group dir exists and writes file content. +This method acquires the per-task lock before writing. + ### `create(): Promise` First-write lifecycle: @@ -91,26 +122,38 @@ First-write lifecycle: 2. set `created` and `updated` (`makeRfc3339`) 3. write file +This method runs under the per-task lock. + ### `addComment(comment: string): Promise` Appends comment row, bumps `updated`, writes. +This method runs under the per-task lock. + ### `updateTitle(title: string): Promise` Updates title, bumps `updated`, writes. +This method runs under the per-task lock. + ### `updateDescription(description: string): Promise` Updates description, bumps `updated`, writes. +This method runs under the per-task lock. + ### `updateLabels(labels: string[]): Promise` Replaces labels, bumps `updated`, writes. +This method runs under the per-task lock. + ### `deleteFile(): Promise` Deletes canonical task file only (does not remove status symlinks). +This method runs under the per-task lock. + ### `toJSON(): object` Returns plain object for CLI JSON output. diff --git a/src/constants.ts b/src/constants.ts index 7cddf6a..c09d2b0 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -38,3 +38,6 @@ export const COMMENTS_HEADER = process.env.TASKDB_COMMENTS_HEADER ?? "## Task Comments"; export const MAX_FILES_PER_DIR = 32768; + +/** Project-wide lockfile used for create/delete critical sections. */ +export const PROJECT_LOCK_FILE = "project.lock"; diff --git a/src/models/project.ts b/src/models/project.ts index bc8e9f7..1dbdb02 100644 --- a/src/models/project.ts +++ b/src/models/project.ts @@ -1,10 +1,19 @@ -import { lstat, mkdir, readdir, symlink, unlink } from "node:fs/promises"; +import { + lstat, + mkdir, + readdir, + readFile, + symlink, + unlink, + writeFile, +} from "node:fs/promises"; import { join } from "node:path"; import { ALL_TASKS_DIR, COMPLETE_TASKS_DIR, DEFAULT_STATUSES, NON_STATUS_DIRS, + PROJECT_LOCK_FILE, } from "../constants.ts"; import type { OutputFn } from "../types.ts"; import { toSlug } from "../utils/slug.ts"; @@ -109,6 +118,81 @@ export class Project { return isNew; } + // ── Locking ─────────────────────────────────────────────────────────────────── + + /** + * Absolute path to the project-scoped lockfile. + * + * The filename is controlled by {@link PROJECT_LOCK_FILE} and is resolved + * relative to this project's root directory. + * + * @returns Absolute filesystem path to `project.lock`. + */ + get lockFilePath(): string { + return join(this.path, PROJECT_LOCK_FILE); + } + + /** + * Acquire the project-wide lock. + * + * This creates the lockfile with `wx` semantics (fail if it already exists), + * then writes the current process PID into the file. + * + * @throws When the lockfile already exists or when filesystem writes fail. + */ + async lock(): Promise { + await writeFile(this.lockFilePath, "", { flag: "wx" }); + await writeFile(this.lockFilePath, String(process.pid)); + } + + /** + * Release the project-wide lock. + * + * Behavior: + * - If the lockfile does not exist, this is a no-op. + * - If `force` is `true`, delete the lockfile regardless of owner PID. + * - Otherwise, only delete when the lockfile PID matches `process.pid`. + * + * @param force When `true`, bypasses PID ownership checks. Defaults to `false`. + * @throws When `force` is `false` and lockfile PID does not match current PID. + * @throws When filesystem operations fail for reasons other than missing file. + */ + async unlock(force: boolean = false): Promise { + try { + if (force) { + await unlink(this.lockFilePath); + return; + } + + const raw = await readFile(this.lockFilePath, "utf8"); + const lockPid = parseInt(raw.trim(), 10); + if (lockPid !== process.pid) { + throw new Error( + `Cannot unlock project lock owned by PID ${lockPid}; current PID is ${process.pid}`, + ); + } + await unlink(this.lockFilePath); + } catch (error) { + const e = error as NodeJS.ErrnoException; + if (e?.code === "ENOENT") return; + throw error; + } + } + + /** + * Check whether the project lock is currently present. + * + * @returns `true` when the lockfile exists; otherwise `false`. + */ + async isLocked(): Promise { + try { + await lstat(this.lockFilePath); + return true; + } catch { + return false; + } + } + // ── Max task ID ─────────────────────────────────────────────────────────────── /** @@ -419,33 +503,38 @@ export class Project { labels: string[] = [], warn?: OutputFn, ): Promise { - const nextId = (await this.maxTaskId()) + 1; - const slug = toSlug(title); - - const task = new Task({ - id: nextId, - slug, - title, - labels, - created: "", - updated: "", - description, - comments: [], - projectPath: this.path, - status, - }); - - await task.create(); // stamps created/updated and writes the file - - const isNew = await this.ensureStatusDir(status, Task.groupDir(task.id)); - if (isNew && warn) { - warn( - `Warning: "${status}" is a new status directory (not previously known).`, - ); - } - await this.createStatusSymlink(status, task.id, task.filename); + await this.lock(); + try { + const nextId = (await this.maxTaskId()) + 1; + const slug = toSlug(title); + + const task = new Task({ + id: nextId, + slug, + title, + labels, + created: "", + updated: "", + description, + comments: [], + projectPath: this.path, + status, + }); + + await task.create(); // stamps created/updated and writes the file + + const isNew = await this.ensureStatusDir(status, Task.groupDir(task.id)); + if (isNew && warn) { + warn( + `Warning: "${status}" is a new status directory (not previously known).`, + ); + } + await this.createStatusSymlink(status, task.id, task.filename); - return task; + return task; + } finally { + await this.unlock(true); + } } /** @@ -458,10 +547,15 @@ export class Project { * @param task Task instance to delete. */ async deleteTask(task: Task): Promise { - const statusDirs = await this.getStatusDirs(); - for (const dir of statusDirs) { - await this.removeStatusSymlink(dir, task.id, task.filename); + await this.lock(); + try { + const statusDirs = await this.getStatusDirs(); + for (const dir of statusDirs) { + await this.removeStatusSymlink(dir, task.id, task.filename); + } + await task.deleteFile(); + } finally { + await this.unlock(true); } - await task.deleteFile(); } } diff --git a/src/models/task.ts b/src/models/task.ts index e2e34dc..5c1ff0e 100644 --- a/src/models/task.ts +++ b/src/models/task.ts @@ -1,5 +1,5 @@ import matter from "gray-matter"; -import { mkdir, unlink } from "node:fs/promises"; +import { mkdir, readFile, unlink, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { ALL_TASKS_DIR, @@ -139,6 +139,18 @@ export class Task { return join(this.projectPath, ALL_TASKS_DIR, Task.groupDir(this.id)); } + /** + * Absolute path to this task's lockfile. + * + * The lockfile is colocated with the canonical markdown file and uses the + * `.lock` naming convention. + * + * @returns Absolute filesystem path to this task lock. + */ + get lockFilePath(): string { + return `${this.filePath}.lock`; + } + // ── Serialization ─────────────────────────────────────────────────────────── /** @@ -292,10 +304,107 @@ export class Task { * Ensures the grouped directory exists, then writes the canonical markdown file. */ async write(): Promise { + await this.withLock(async () => { + await this.writeUnlocked(); + }); + } + + /** + * Persist task content without taking a lock. + * + * Intended for internal use only from code paths that have already acquired + * the per-task lock (via {@link withLock}). + * + * @throws When directory creation or file write fails. + */ + private async writeUnlocked(): Promise { await mkdir(this.groupDirPath, { recursive: true }); await Bun.write(this.filePath, this.buildFileContent()); } + // ── Locking ───────────────────────────────────────────────────────────────── + + /** + * Acquire the per-task lock. + * + * This creates the lockfile with `wx` semantics (fail if it already exists), + * then writes the current process PID into the file. + * + * @throws When the lockfile already exists or when filesystem writes fail. + */ + async lock(): Promise { + await writeFile(this.lockFilePath, "", { flag: "wx" }); + await writeFile(this.lockFilePath, String(process.pid)); + } + + /** + * Release the per-task lock. + * + * Behavior: + * - If the lockfile does not exist, this is a no-op. + * - If `force` is `true`, delete the lockfile regardless of owner PID. + * - Otherwise, only delete when the lockfile PID matches `process.pid`. + * + * @param force When `true`, bypasses PID ownership checks. Defaults to `false`. + * @throws When `force` is `false` and lockfile PID does not match current PID. + * @throws When filesystem operations fail for reasons other than missing file. + */ + async unlock(force: boolean = false): Promise { + try { + if (force) { + await unlink(this.lockFilePath); + return; + } + + const raw = await readFile(this.lockFilePath, "utf8"); + const lockPid = parseInt(raw.trim(), 10); + if (lockPid !== process.pid) { + throw new Error( + `Cannot unlock task lock owned by PID ${lockPid}; current PID is ${process.pid}`, + ); + } + await unlink(this.lockFilePath); + } catch (error) { + const e = error as NodeJS.ErrnoException; + if (e?.code === "ENOENT") return; + throw error; + } + } + + /** + * Check whether this task is currently locked. + * + * @returns `true` when the task lockfile exists; otherwise `false`. + */ + async isLocked(): Promise { + try { + await Bun.file(this.lockFilePath).text(); + return true; + } catch { + return false; + } + } + + /** + * Run a function under the per-task lock. + * + * Always attempts lock cleanup in `finally` using `unlock(true)` so stale + * locks are not left behind after failures. + * + * @typeParam T Return type of the wrapped async function. + * @param fn Async callback to execute while lock is held. + * @returns The callback result. + * @throws Re-throws any error raised by lock acquisition or callback execution. + */ + private async withLock(fn: () => Promise): Promise { + await this.lock(); + try { + return await fn(); + } finally { + await this.unlock(true); + } + } + // ── Lifecycle ──────────────────────────────────────────────────────────────── /** @@ -305,19 +414,23 @@ export class Task { * 3. Write the file. */ async create(): Promise { - this.slug = toSlug(this.title); - const now = makeRfc3339(); - this.created = now; - this.updated = now; - await this.write(); + await this.withLock(async () => { + this.slug = toSlug(this.title); + const now = makeRfc3339(); + this.created = now; + this.updated = now; + await this.writeUnlocked(); + }); } /** Append a timestamped comment, bump `updated`, and persist. */ async addComment(comment: string): Promise { - const now = makeRfc3339(); - this.comments.push({ commentedAt: now, comment }); - this.updated = now; - await this.write(); + await this.withLock(async () => { + const now = makeRfc3339(); + this.comments.push({ commentedAt: now, comment }); + this.updated = now; + await this.writeUnlocked(); + }); } /** @@ -326,9 +439,11 @@ export class Task { * @param title New task title. */ async updateTitle(title: string): Promise { - this.title = title; - this.updated = makeRfc3339(); - await this.write(); + await this.withLock(async () => { + this.title = title; + this.updated = makeRfc3339(); + await this.writeUnlocked(); + }); } /** @@ -337,9 +452,11 @@ export class Task { * @param description New description markdown. */ async updateDescription(description: string): Promise { - this.description = description; - this.updated = makeRfc3339(); - await this.write(); + await this.withLock(async () => { + this.description = description; + this.updated = makeRfc3339(); + await this.writeUnlocked(); + }); } /** @@ -348,14 +465,18 @@ export class Task { * @param labels Full replacement set of labels. */ async updateLabels(labels: string[]): Promise { - this.labels = labels; - this.updated = makeRfc3339(); - await this.write(); + await this.withLock(async () => { + this.labels = labels; + this.updated = makeRfc3339(); + await this.writeUnlocked(); + }); } /** Permanently delete the task file. Does NOT touch any symlinks. */ async deleteFile(): Promise { - await unlink(this.filePath); + await this.withLock(async () => { + await unlink(this.filePath); + }); } // ── Presentation ───────────────────────────────────────────────────────────── diff --git a/tests/models/project.test.ts b/tests/models/project.test.ts index 6ea9adb..40328cf 100644 --- a/tests/models/project.test.ts +++ b/tests/models/project.test.ts @@ -1,8 +1,15 @@ import { test, expect, describe, beforeEach, afterEach, spyOn } from "bun:test"; -import { mkdir, rm, lstat, readlink } from "node:fs/promises"; +import { + mkdir, + rm, + lstat, + readFile, + readlink, + writeFile, +} from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { DEFAULT_STATUSES } from "../../src/constants.ts"; +import { DEFAULT_STATUSES, PROJECT_LOCK_FILE } from "../../src/constants.ts"; import { Project } from "../../src/models/project.ts"; import { Task } from "../../src/models/task.ts"; @@ -69,6 +76,44 @@ describe("Project.getStatusDirs", () => { }); }); +// ── lock / unlock / isLocked ────────────────────────────────────────────────── + +describe("Project.lock / unlock / isLocked", () => { + test("lock creates lockfile with current pid", async () => { + await project.lock(); + const raw = await readFile(join(projectPath, PROJECT_LOCK_FILE), "utf8"); + expect(raw.trim()).toBe(String(process.pid)); + }); + + test("isLocked reflects lockfile presence", async () => { + expect(await project.isLocked()).toBe(false); + await project.lock(); + expect(await project.isLocked()).toBe(true); + await project.unlock(); + expect(await project.isLocked()).toBe(false); + }); + + test("unlock is no-op when lockfile missing", async () => { + await project.unlock(); + expect(await project.isLocked()).toBe(false); + }); + + test("unlock throws when pid does not match", async () => { + const lockPath = join(projectPath, PROJECT_LOCK_FILE); + await writeFile(lockPath, "999999"); + await expect(project.unlock()).rejects.toThrow( + "Cannot unlock project lock", + ); + }); + + test("unlock(force=true) removes lockfile regardless of pid", async () => { + const lockPath = join(projectPath, PROJECT_LOCK_FILE); + await writeFile(lockPath, "999999"); + await project.unlock(true); + expect(await project.isLocked()).toBe(false); + }); +}); + // ── maxTaskId ───────────────────────────────────────────────────────────────── describe("Project.maxTaskId", () => { @@ -132,6 +177,11 @@ describe("Project.createTask", () => { const task = await project.createTask("T", "", "done"); expect(task.status).toBe("done"); }); + + test("cleans up project lock after create", async () => { + await project.createTask("T"); + expect(await project.isLocked()).toBe(false); + }); }); // ── resolveTask ─────────────────────────────────────────────────────────────── @@ -261,6 +311,12 @@ describe("Project.deleteTask", () => { } expect(threwLink).toBe(true); }); + + test("cleans up project lock after delete", async () => { + const task = await project.createTask("T"); + await project.deleteTask(task); + expect(await project.isLocked()).toBe(false); + }); }); // ── searchTasks ────────────────────────────────────────────────────────────── diff --git a/tests/models/task.test.ts b/tests/models/task.test.ts index 4a15e57..88e13fc 100644 --- a/tests/models/task.test.ts +++ b/tests/models/task.test.ts @@ -1,5 +1,12 @@ import { test, expect, describe, beforeEach, afterEach } from "bun:test"; -import { mkdir, rm } from "node:fs/promises"; +import { + lstat, + mkdir, + readFile, + rm, + unlink, + writeFile, +} from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { Task } from "../../src/models/task.ts"; @@ -115,6 +122,64 @@ Just a description. }); }); +// ── lock / unlock / isLocked ────────────────────────────────────────────────── + +describe("Task.lock / unlock / isLocked", () => { + test("lock creates lockfile with current pid", async () => { + const task = new Task({ + id: 1, + slug: "t", + title: "T", + labels: [], + created: "", + updated: "", + description: "", + comments: [], + projectPath, + }); + + await task.lock(); + const raw = await readFile(task.lockFilePath, "utf8"); + expect(raw.trim()).toBe(String(process.pid)); + await task.unlock(true); + }); + + test("unlock throws when pid does not match", async () => { + const task = new Task({ + id: 1, + slug: "t", + title: "T", + labels: [], + created: "", + updated: "", + description: "", + comments: [], + projectPath, + }); + + await writeFile(task.lockFilePath, "999999"); + await expect(task.unlock()).rejects.toThrow("Cannot unlock task lock"); + await task.unlock(true); + }); + + test("unlock is no-op when lockfile missing", async () => { + const task = new Task({ + id: 1, + slug: "t", + title: "T", + labels: [], + created: "", + updated: "", + description: "", + comments: [], + projectPath, + }); + + await task.unlock(); + expect(await task.isLocked()).toBe(false); + }); +}); + // ── Create / read / write ───────────────────────────────────────────────────── describe("Task.create + Task.read", () => { @@ -255,6 +320,28 @@ describe("Task.updateTitle / updateDescription / updateLabels", () => { }); }); +describe("Task.deleteFile", () => { + test("deletes file and cleans lock", async () => { + const task = new Task({ + id: 1, + slug: "t", + title: "T", + labels: [], + created: "", + updated: "", + description: "", + comments: [], + projectPath, + }); + + await task.create(); + await task.deleteFile(); + + await expect(lstat(task.filePath)).rejects.toThrow(); + await expect(unlink(task.lockFilePath)).rejects.toThrow(); + }); +}); + describe("Task.toJSON", () => { test("includes all expected fields", () => { const task = new Task({