diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 2928b7e39..99db74d0b 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1189,8 +1189,8 @@ Docker hardening, and durability seatbelts above. `withExclusiveMultiFileLock` (all-or-nothing fail-fast) acquires all locks in one synchronous tick — used by note-mover, which must lock the source, destination, and every backlink source for the whole - read-plan-write span. The trash move, the orphan purge, and the - retention sweep share one serializing key for the whole `.trash/` + read-plan-write span. The trash move and the cleanup in + `trash-sweeper.ts` share one serializing key for the whole `.trash/` domain. - **Properties-block guard** (`frontmatter.ts`): every rewrite refuses a note whose properties block it cannot keep, and every write's result is @@ -1198,30 +1198,15 @@ Docker hardening, and durability seatbelts above. properties. [Vault read/write](#vault-readwrite) gives the full rules and the repair path. -- **Trash claim loop** (`moveNoteToTrash` in `vault-filesystem.ts`): a - delete under Obsidian's `system` (default) or `local` trash setting - moves the note into `.trash/`. Each candidate name is claimed with an - exclusive create, so an existing trash copy is never overwritten. - `system`-mapped moves are recorded in the index DB's `trash_entries` - table. Recording is fail-open — a failed row write logs a warning, the - delete still succeeds, and an unrecorded entry is never swept. The - table's primary key is the case-folded path, so a case alias replaces - its stale row instead of leaving one that could purge the wrong sibling - on a case-insensitive mount. -- **Recorded trash bookkeeping** (`trash-sweeper.ts`): two row-driven - operations (neither walks the folder): - - **Orphan purge** — runs once at boot regardless of - `TRASH_RETENTION_DAYS`. Drops rows whose `.trash/` entry no longer - exists on disk (uses lstat, so dangling symlinks are kept), so - manual emptying or `retention=none` never leaves - unbounded stale rows. - - **Retention sweep** — runs at startup and daily. Purges recorded - entries older than `TRASH_RETENTION_DAYS`. Each unlink is - double-guarded: the resolved path and the parent directory's - realpath must both sit inside `.trash/`, so a corrupted row or a - directory symlink cannot reach live notes. - - Both share a serializing lock with the trash move and re-read each - row under it before acting, so neither operates on a stale snapshot. +- **Trash handling** (`moveNoteToTrash` in `vault-filesystem.ts`, + `trash-sweeper.ts`): a delete under Obsidian's `system` (default) or + `local` trash setting moves the note into `.trash/` and never overwrites + an existing trash copy. The server records `system` moves with the + trashed file's identity, and its daily retention sweep deletes only + those recorded files once they pass `TRASH_RETENTION_DAYS`. Before + deleting, it checks that the file still sits inside `.trash/` and is + still the unchanged file it trashed, so a corrupted record, a symlink, + or a note Obsidian later trashed under the same name is left alone. - **Verify-then-preflight-then-commit move** (`note-mover.ts`): under the lock, `moveNote` first scans the filesystem for backlinks the search index missed (closing a lag race); then reads every affected file and diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6c0bce579..2258d4419 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -102,9 +102,8 @@ development. A test fails if the versions drift. - Test: `npm test` includes `cli/src/**/*.test.ts` - Try it: `node cli/dist/bin.js init --help` -**Template sync rule:** `cli/templates/` holds verbatim copies of -The optional env blocks in `cli/src/env.ts` are derived from `deploy/*/.env.example`. -If you change any `.env.example`, run +**Env block sync:** The optional env blocks in `cli/src/env.ts` are derived +from `deploy/*/.env.example`. If you change any `.env.example`, run `npm run sync:cli-env-blocks` in the same PR — drift tests fail CI otherwise. **Publishing:** CLI releases are explicit and independent of server releases — diff --git a/SECURITY.md b/SECURITY.md index d677d316a..b43d59882 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -67,12 +67,13 @@ mechanism-level detail. - Memory file names reject `/` and `\` — prevents `../../outside`-style escapes from the memory directory — and leading dots, which would create hidden files -- The trash retention sweep deletes only files it previously recorded. - Every removal passes two gates — the recorded path must resolve inside - `.trash/`, and the file's parent directory must realpath-resolve inside - it too. A corrupted or hand-edited index row, or a directory symlink - planted in `.trash/`, is skipped with a warning instead of followed to - a live note +- The trash retention sweep deletes only files it previously recorded, + and only inside `.trash/`. It skips with a warning, instead of + following to a live note: + - a recorded path that points outside `.trash/`, from a corrupted or + hand-edited index row + - a file reached through a folder symlink planted in `.trash/` that + leads elsewhere ### Hidden paths @@ -159,6 +160,12 @@ mechanism-level detail. synced `.obsidian/` settings), the container refuses to start, preventing the sync engine from interpreting the empty vault as mass local deletions +- Trash sweep identity gate: the retention sweep deletes a recorded + `.trash/` file only while it still has the identity recorded when the + server trashed it and has not changed since, so a different note that + Obsidian later trashed under a recorded name is kept. It cannot catch a + note restored by hand and trashed again within a minute, or a file + swapped in during the instant between the check and the delete - Memory shrink guard: refuses writes that would remove >50% of a file's bytes — defense-in-depth against bugs that would silently erase most of a memory file diff --git a/cli/src/env.ts b/cli/src/env.ts index c40cdea0c..c07535ad3 100644 --- a/cli/src/env.ts +++ b/cli/src/env.ts @@ -53,7 +53,8 @@ MAX_PDF_RENDER_PAGES=5 # moved to .trash under that setting are cleaned up — notes you or Obsidian # put there are never touched. The server tracks its trashed notes even while # this is none, so switching none to a number cleans up the older backlog on -# the next restart. +# the next restart. Notes trashed by version 0.54.11 or earlier are kept; +# empty .trash by hand to remove them. TRASH_RETENTION_DAYS=30 # Your IANA timezone — affects daily note resolution and memory timestamps. diff --git a/deploy/local/.env.example b/deploy/local/.env.example index ebcb35390..7e4fde2dd 100644 --- a/deploy/local/.env.example +++ b/deploy/local/.env.example @@ -43,7 +43,8 @@ MAX_PDF_RENDER_PAGES=5 # moved to .trash under that setting are cleaned up — notes you or Obsidian # put there are never touched. The server tracks its trashed notes even while # this is none, so switching none to a number cleans up the older backlog on -# the next restart. +# the next restart. Notes trashed by version 0.54.11 or earlier are kept; +# empty .trash by hand to remove them. TRASH_RETENTION_DAYS=30 # Your IANA timezone — affects daily note resolution and memory timestamps. diff --git a/deploy/local/README.md b/deploy/local/README.md index 2cee67902..e26a8ad2b 100644 --- a/deploy/local/README.md +++ b/deploy/local/README.md @@ -307,9 +307,11 @@ links): set `TRASH_RETENTION_DAYS` in `.env` to change the window, or `TRASH_RETENTION_DAYS=none` to keep them forever. Only notes the server moved there are cleaned up; anything you or Obsidian put in `.trash` is - never touched. To reclaim space sooner, empty the `.trash` folder - yourself (it's hidden — press Cmd+Shift+. in Finder, or Ctrl+H in most - Linux file managers, to show it). + never touched. Notes the server moved there with version 0.54.11 or + earlier also stay. To reclaim space sooner, or to clear those older + notes, empty the `.trash` folder yourself (it's hidden — press + Cmd+Shift+. in Finder, or Ctrl+H in most Linux file managers, to show + it). - **"Move to Obsidian trash (.trash folder)"** — a deleted note moves to the same `.trash` folder and stays there until you empty it, exactly as in Obsidian. diff --git a/src/vault-mcp/mcp-core/tools/vault-crud-tools.ts b/src/vault-mcp/mcp-core/tools/vault-crud-tools.ts index c4d3c1260..0b098ba34 100644 --- a/src/vault-mcp/mcp-core/tools/vault-crud-tools.ts +++ b/src/vault-mcp/mcp-core/tools/vault-crud-tools.ts @@ -1135,12 +1135,13 @@ Returns: ${deleteNoteReturns} Notes how many empty folders were pruned when any // Only "system" moves are swept later, so only they are recorded; // "local" is Obsidian's keep-forever trash. recordTrashEntry: trashOption === "system" ? search.recordTrashEntry : undefined, - // The retention sweep deletes the file at each expired - // trash_entries row's path, and a row can outlive its file when - // .trash/ is emptied by hand. A move that does not record clears - // the row at its landed path, so the sweep cannot remove the new - // file, such as a "local" note meant to be kept. A "none" delete - // lands nothing in .trash/ and never calls it. + // Passed for every option, not only "system": + // - Emptying .trash/ by hand leaves rows behind, so a stale row + // can sit at the path this delete lands on. + // - A move the server does not record ("local", or a "system" + // move whose record failed) drops that row, so the retention + // sweep never deletes the note just trashed. + // - A "none" delete lands nothing in .trash/ and never calls it. clearStaleTrashEntry: search.deleteTrashEntry, }, reqLogger, diff --git a/src/vault-mcp/search/__tests__/search-index.test.ts b/src/vault-mcp/search/__tests__/search-index.test.ts index 78a2555d5..8ef9d2db2 100644 --- a/src/vault-mcp/search/__tests__/search-index.test.ts +++ b/src/vault-mcp/search/__tests__/search-index.test.ts @@ -8270,10 +8270,16 @@ describe("file content vector embeddings", () => { }) describe("trash entries (retention-sweep bookkeeping)", () => { + /** A stand-in identity — the store keeps whatever string it is given. */ + const SAMPLE_FILE_IDENTITY = "4096:12:1791557833301645709" + it("listAllTrashEntries returns every recorded entry", () => { const trashIndex = createSearchIndex(":memory:") - trashIndex.recordTrashEntry(".trash/a.md") - trashIndex.recordTrashEntry(".trash/sub/b.md") + trashIndex.recordTrashEntry({ trashPath: ".trash/a.md", fileIdentity: SAMPLE_FILE_IDENTITY }) + trashIndex.recordTrashEntry({ + trashPath: ".trash/sub/b.md", + fileIdentity: SAMPLE_FILE_IDENTITY, + }) const allEntries = trashIndex.listAllTrashEntries() @@ -8287,20 +8293,73 @@ describe("trash entries (retention-sweep bookkeeping)", () => { expect(trashIndex.listAllTrashEntries()).toEqual([]) }) - it("round-trips a recorded entry through get and list", () => { + it("round-trips a recorded entry, identity included, through get and both lists", () => { + vi.useFakeTimers() + onTestFinished(() => { + vi.useRealTimers() + }) + const trashTime = DateTime.fromISO("2026-01-01T00:00:00Z") + vi.setSystemTime(trashTime.toMillis()) const trashIndex = createSearchIndex(":memory:") - trashIndex.recordTrashEntry(".trash/Notes/gone.md") + trashIndex.recordTrashEntry({ + trashPath: ".trash/Notes/gone.md", + fileIdentity: SAMPLE_FILE_IDENTITY, + }) + const expectedEntry = { + trashPath: ".trash/Notes/gone.md", + trashedAt: trashTime.toUnixInteger(), + fileIdentity: SAMPLE_FILE_IDENTITY, + } - const entry = trashIndex.getTrashEntry(".trash/Notes/gone.md") - expect(entry?.trashPath).toBe(".trash/Notes/gone.md") + expect(trashIndex.getTrashEntry(".trash/Notes/gone.md")).toEqual(expectedEntry) + expect(trashIndex.listAllTrashEntries()).toEqual([expectedEntry]) // Every recorded entry is "expired" against a cutoff after its stamp. - const listed = trashIndex.listExpiredTrashEntries((entry?.trashedAt ?? 0) + 1) - expect(listed.map((listedEntry) => listedEntry.trashPath)).toEqual([".trash/Notes/gone.md"]) + expect(trashIndex.listExpiredTrashEntries(trashTime.toUnixInteger() + 1)).toEqual([ + expectedEntry, + ]) + }) + + it("adds the file_identity column to a pre-existing trash_entries table, leaving old rows without one", async () => { + const dir = await mkdtemp(join(tmpdir(), "warm-db-")) + onTestFinished(() => rm(dir, { recursive: true })) + vi.useFakeTimers() + onTestFinished(() => { + vi.useRealTimers() + }) + const trashTime = DateTime.fromISO("2026-01-01T00:00:00Z") + vi.setSystemTime(trashTime.toMillis()) + const dbPath = join(dir, "search.db") + // Simulate a database file created before the file_identity column. + const legacyDb = new Database(dbPath) + legacyDb.exec(` + CREATE TABLE trash_entries ( + trash_key TEXT PRIMARY KEY, trash_path TEXT NOT NULL, trashed_at INTEGER NOT NULL + ); + INSERT INTO trash_entries VALUES ('.trash/old.md', '.trash/Old.md', 1700000000); + `) + legacyDb.close() + + const warmIndex = createSearchIndex(dbPath) + warmIndex.recordTrashEntry({ trashPath: ".trash/new.md", fileIdentity: SAMPLE_FILE_IDENTITY }) + + expect(warmIndex.getTrashEntry(".trash/Old.md")).toEqual({ + trashPath: ".trash/Old.md", + trashedAt: 1700000000, + fileIdentity: null, + }) + expect(warmIndex.getTrashEntry(".trash/new.md")).toEqual({ + trashPath: ".trash/new.md", + trashedAt: trashTime.toUnixInteger(), + fileIdentity: SAMPLE_FILE_IDENTITY, + }) }) it("treats the cutoff as exclusive — a row stamped exactly at the cutoff is not expired", () => { const trashIndex = createSearchIndex(":memory:") - trashIndex.recordTrashEntry(".trash/boundary.md") + trashIndex.recordTrashEntry({ + trashPath: ".trash/boundary.md", + fileIdentity: SAMPLE_FILE_IDENTITY, + }) const entry = trashIndex.getTrashEntry(".trash/boundary.md") if (!entry) throw new Error("entry missing after record") @@ -8313,7 +8372,7 @@ describe("trash entries (retention-sweep bookkeeping)", () => { ).toEqual([".trash/boundary.md"]) }) - it("re-recording the same path restarts the retention clock", () => { + it("re-recording the same path restarts the retention clock and replaces the identity", () => { vi.useFakeTimers() onTestFinished(() => { vi.useRealTimers() @@ -8323,15 +8382,23 @@ describe("trash entries (retention-sweep bookkeeping)", () => { const secondTrashTime = DateTime.fromISO("2026-03-01T00:00:00Z") vi.setSystemTime(firstTrashTime.toMillis()) - trashIndex.recordTrashEntry(".trash/reused.md") + trashIndex.recordTrashEntry({ trashPath: ".trash/reused.md", fileIdentity: "100:5:1" }) const firstEntry = trashIndex.getTrashEntry(".trash/reused.md") vi.setSystemTime(secondTrashTime.toMillis()) - trashIndex.recordTrashEntry(".trash/reused.md") + trashIndex.recordTrashEntry({ trashPath: ".trash/reused.md", fileIdentity: "200:7:2" }) const refreshedEntry = trashIndex.getTrashEntry(".trash/reused.md") - expect(firstEntry?.trashedAt).toBe(firstTrashTime.toUnixInteger()) - expect(refreshedEntry?.trashedAt).toBe(secondTrashTime.toUnixInteger()) + expect(firstEntry).toEqual({ + trashPath: ".trash/reused.md", + trashedAt: firstTrashTime.toUnixInteger(), + fileIdentity: "100:5:1", + }) + expect(refreshedEntry).toEqual({ + trashPath: ".trash/reused.md", + trashedAt: secondTrashTime.toUnixInteger(), + fileIdentity: "200:7:2", + }) }) it("a case-alias record replaces the stale row instead of adding a second one", () => { @@ -8339,8 +8406,8 @@ describe("trash entries (retention-sweep bookkeeping)", () => { // are one file — two live rows for it would let a stale expired alias // purge the fresh copy. The folded primary key collapses them to one. const trashIndex = createSearchIndex(":memory:") - trashIndex.recordTrashEntry(".trash/Note.md") - trashIndex.recordTrashEntry(".trash/note.md") + trashIndex.recordTrashEntry({ trashPath: ".trash/Note.md", fileIdentity: SAMPLE_FILE_IDENTITY }) + trashIndex.recordTrashEntry({ trashPath: ".trash/note.md", fileIdentity: SAMPLE_FILE_IDENTITY }) const farFutureCutoff = DateTime.now().plus({ days: 1 }).toUnixInteger() const listed = trashIndex.listExpiredTrashEntries(farFutureCutoff) @@ -8351,7 +8418,10 @@ describe("trash entries (retention-sweep bookkeeping)", () => { it("deleteTrashEntry removes the row under any case alias", () => { const trashIndex = createSearchIndex(":memory:") - trashIndex.recordTrashEntry(".trash/ToDelete.md") + trashIndex.recordTrashEntry({ + trashPath: ".trash/ToDelete.md", + fileIdentity: SAMPLE_FILE_IDENTITY, + }) trashIndex.deleteTrashEntry(".trash/todelete.md") @@ -8360,15 +8430,32 @@ describe("trash entries (retention-sweep bookkeeping)", () => { expect(trashIndex.listExpiredTrashEntries(farFutureCutoff)).toEqual([]) }) - it("trash entries survive a vault rebuild", async () => { + it("trash entries survive a vault rebuild with their timestamp and identity", async () => { + // Only Date is faked, so the rebuild's own timers and file reads run as usual. + vi.useFakeTimers({ toFake: ["Date"] }) + onTestFinished(() => { + vi.useRealTimers() + }) + const trashTime = DateTime.fromISO("2026-01-01T00:00:00Z") + vi.setSystemTime(trashTime.toMillis()) const trashIndex = createSearchIndex(":memory:") - trashIndex.recordTrashEntry(".trash/survivor.md") + trashIndex.recordTrashEntry({ + trashPath: ".trash/survivor.md", + fileIdentity: SAMPLE_FILE_IDENTITY, + }) const emptyVault = await mkdtemp(join(tmpdir(), "trash-rebuild-")) onTestFinished(() => rm(emptyVault, { recursive: true, force: true })) + // A day later, so a rebuild that re-stamped the row from the clock would + // change trashedAt. + vi.setSystemTime(trashTime.plus({ days: 1 }).toMillis()) await trashIndex.rebuildFromVault({ vaultPath: emptyVault }, logger) - expect(trashIndex.getTrashEntry(".trash/survivor.md")?.trashPath).toBe(".trash/survivor.md") + expect(trashIndex.getTrashEntry(".trash/survivor.md")).toEqual({ + trashPath: ".trash/survivor.md", + trashedAt: trashTime.toUnixInteger(), + fileIdentity: SAMPLE_FILE_IDENTITY, + }) }) }) diff --git a/src/vault-mcp/search/search-index.ts b/src/vault-mcp/search/search-index.ts index f4b65145b..5922869d3 100644 --- a/src/vault-mcp/search/search-index.ts +++ b/src/vault-mcp/search/search-index.ts @@ -458,16 +458,23 @@ export const createSearchIndex = ( CREATE INDEX IF NOT EXISTS idx_tasks_status ON tasks(status); CREATE INDEX IF NOT EXISTS idx_tasks_due ON tasks(due); - -- Files this server moved to .trash/, for the retention sweep. trash_key - -- is the case-folded path (one row per physical file on case-insensitive - -- mounts, so a fresh delete's upsert replaces a stale case-alias row - -- instead of leaving it to purge the wrong sibling); trash_path keeps the - -- exact spelling for filesystem operations. No index: the table stays - -- small and the sweep is a daily full scan. + -- Files this server moved to .trash/, for the retention sweep: + -- - trash_key is the case-folded path, one row per physical file on + -- case-insensitive mounts. On those, .trash/a.md and .trash/A.md are one + -- file, so a delete landing at .trash/A.md replaces a stale .trash/a.md + -- row instead of keeping a second row that could expire the file on + -- the old row's date. On a case-sensitive mount the two names are two + -- files sharing one key, so the file recorded first loses its row and + -- is never swept. + -- - trash_path keeps the exact spelling for filesystem operations. + -- - file_identity is the landed file's identity, which the sweep matches + -- before deleting; NULL on a row recorded before the column existed. + -- No index: the table stays small and the sweep is a daily full scan. CREATE TABLE IF NOT EXISTS trash_entries ( - trash_key TEXT PRIMARY KEY, - trash_path TEXT NOT NULL, - trashed_at INTEGER NOT NULL + trash_key TEXT PRIMARY KEY, + trash_path TEXT NOT NULL, + trashed_at INTEGER NOT NULL, + file_identity TEXT ); `) // path UNINDEXED: stored for JOIN/DELETE but not searchable, saves index space @@ -623,6 +630,17 @@ export const createSearchIndex = ( db.exec(`ALTER TABLE non_md_files ADD COLUMN bytes INTEGER`) } + // Same idempotent migration for trash_entries.file_identity. Nullable, and + // never backfilled: a row recorded without it may already point at a file + // that is not the one the server trashed, so the sweep keeps such files. + const trashEntryColumns = db + .prepare(`PRAGMA table_info(trash_entries)`) + .all() + + if (!trashEntryColumns.some((column) => column.name === "file_identity")) { + db.exec(`ALTER TABLE trash_entries ADD COLUMN file_identity TEXT`) + } + // Prepared statements are compiled once here and reused across all calls. // Each returned statement retains its compiled SQL; preparing inside an // operation would compile it again on every invocation. @@ -2615,51 +2633,58 @@ export const createSearchIndex = ( // ── Trash entries (retention-sweep bookkeeping) ────────────── + type TrashEntryRow = { trash_path: string; trashed_at: number; file_identity: string | null } + const upsertTrashEntryStmt = db.prepare(` - INSERT OR REPLACE INTO trash_entries (trash_key, trash_path, trashed_at) - VALUES (?, ?, ?) + INSERT OR REPLACE INTO trash_entries (trash_key, trash_path, trashed_at, file_identity) + VALUES (?, ?, ?, ?) `) - const selectTrashEntryStmt = db.prepare<[string], { trash_path: string; trashed_at: number }>( - `SELECT trash_path, trashed_at FROM trash_entries WHERE trash_key = ?`, + const selectTrashEntryStmt = db.prepare<[string], TrashEntryRow>( + `SELECT trash_path, trashed_at, file_identity FROM trash_entries WHERE trash_key = ?`, ) - const selectAllTrashEntriesStmt = db.prepare<[], { trash_path: string; trashed_at: number }>( - `SELECT trash_path, trashed_at FROM trash_entries`, + const selectAllTrashEntriesStmt = db.prepare<[], TrashEntryRow>( + `SELECT trash_path, trashed_at, file_identity FROM trash_entries`, + ) + const selectExpiredTrashEntriesStmt = db.prepare<[number], TrashEntryRow>( + `SELECT trash_path, trashed_at, file_identity FROM trash_entries WHERE trashed_at < ?`, ) - const selectExpiredTrashEntriesStmt = db.prepare< - [number], - { trash_path: string; trashed_at: number } - >(`SELECT trash_path, trashed_at FROM trash_entries WHERE trashed_at < ?`) const deleteTrashEntryStmt = db.prepare(`DELETE FROM trash_entries WHERE trash_key = ?`) + const toTrashEntry = (row: TrashEntryRow): TrashEntry => { + return { trashPath: row.trash_path, trashedAt: row.trashed_at, fileIdentity: row.file_identity } + } + /** Records a file this server moved to .trash/ — the retention sweep only - * ever deletes recorded entries. Reusing a path (or a case alias of one, - * via the folded key) replaces the old row, restarting the retention clock - * for the file now at that path. */ - const recordTrashEntry = (trashPath: string): void => { - upsertTrashEntryStmt.run(caseFoldPath(trashPath), trashPath, DateTime.now().toUnixInteger()) + * ever deletes recorded entries, and only while the file at the path still + * has the recorded identity. Reusing a path (or a case alias of one, via + * the folded key) replaces the old row, restarting the retention clock for + * the file now at that path. */ + const recordTrashEntry = (entry: { trashPath: string; fileIdentity: string }): void => { + upsertTrashEntryStmt.run( + caseFoldPath(entry.trashPath), + entry.trashPath, + DateTime.now().toUnixInteger(), + entry.fileIdentity, + ) } + /** Looks the row up by the case-folded key, so the returned trashPath is + * the spelling recorded last, which can be a case alias of the argument. */ const getTrashEntry = (trashPath: string): TrashEntry | null => { const row = selectTrashEntryStmt.get(caseFoldPath(trashPath)) if (!row) return null - return { trashPath: row.trash_path, trashedAt: row.trashed_at } + return toTrashEntry(row) } /** Every recorded trash entry — the orphan purge's candidate list. */ const listAllTrashEntries = (): TrashEntry[] => { - return selectAllTrashEntriesStmt.all().map((row) => ({ - trashPath: row.trash_path, - trashedAt: row.trashed_at, - })) + return selectAllTrashEntriesStmt.all().map(toTrashEntry) } /** Rows recorded strictly before the cutoff — the sweep's candidate list. */ const listExpiredTrashEntries = (cutoffEpochSeconds: number): TrashEntry[] => { - return selectExpiredTrashEntriesStmt.all(cutoffEpochSeconds).map((row) => ({ - trashPath: row.trash_path, - trashedAt: row.trashed_at, - })) + return selectExpiredTrashEntriesStmt.all(cutoffEpochSeconds).map(toTrashEntry) } const deleteTrashEntry = (trashPath: string): void => { @@ -2751,7 +2776,11 @@ export type SearchIndex = ReturnType /** A file this server moved to .trash/, as recorded for the retention sweep. */ export type TrashEntry = { trashPath: string + /** Unix time when the row was recorded, rounded down to a whole second. */ trashedAt: number + /** The recorded file's `TrashFileState.identity`; null on a row recorded + * before identities were kept, which the sweep cannot match to any file. */ + fileIdentity: string | null } /** The slice of the index the trash sweeper needs — injected so diff --git a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts index 9aa87b96a..32cca1beb 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -1,12 +1,28 @@ import { describe, it, expect, vi, onTestFinished } from "vitest" -import { chmod, mkdtemp, mkdir, readFile, rm, stat, symlink, writeFile } from "node:fs/promises" +import { + chmod, + lstat, + mkdtemp, + mkdir, + readFile, + rename, + rm, + stat, + symlink, + unlink, + writeFile, +} from "node:fs/promises" import { join } from "node:path" import { tmpdir } from "node:os" import { DateTime } from "luxon" + +// Every node:fs/promises export becomes a pass-through spy, so a test can +// make the host remove a file at the exact moment the sweep unlinks it. +vi.mock("node:fs/promises", { spy: true }) import { trashSweeper } from "../trash-sweeper.js" -import { vaultFs } from "../vault-filesystem.js" +import { readTrashFileState, vaultFs } from "../vault-filesystem.js" import { createSearchIndex } from "../../search/search-index.js" -import type { TrashEntryStore } from "../../search/search-index.js" +import type { TrashEntry, TrashEntryStore } from "../../search/search-index.js" import { logger } from "../../../logger.js" /** A temp vault with a .trash/ folder, removed when the test finishes. */ @@ -17,34 +33,147 @@ const createTestVault = async (): Promise => { return vault } -/** Records an entry stamped `daysAgo` in the past — the sweeper reads real - * time, so back-dating the record is how a test makes an entry expired. */ -const recordEntryDaysAgo = ( - index: ReturnType, +type RecordedTrashFile = { trashPath: string; fileIdentity: string } + +/** An identity no real file has (no file is ever given inode 0), for a row + * whose file is absent or never compared. */ +const ABSENT_FILE_IDENTITY = "0:0:0" + +/** Linux stamps inode times from a clock that advances once per scheduler + * tick (up to 10 ms), so two changes inside one tick share a change time. + * Waiting past a tick makes the next change land a new one. */ +const waitPastTimestampTick = async (): Promise => { + await new Promise((resolve) => setTimeout(resolve, 25)) +} + +/** The row the server records for the file now at `trashPath`, which must + * exist. */ +const recordedTrashFileAt = async ( + vault: string, trashPath: string, - daysAgo: number, +): Promise => { + const { identity } = await readTrashFileState(join(vault, trashPath)) + return { trashPath, fileIdentity: identity } +} + +/** A row for a trash path with no file behind it. */ +const recordedAbsentFile = (trashPath: string): RecordedTrashFile => { + return { trashPath, fileIdentity: ABSENT_FILE_IDENTITY } +} + +/** Records an entry stamped at `moment` instead of now. */ +const recordEntryAt = ( + index: ReturnType, + recordedFile: RecordedTrashFile, + moment: DateTime, ): void => { - vi.useFakeTimers() - vi.setSystemTime(DateTime.now().minus({ days: daysAgo }).toMillis()) - index.recordTrashEntry(trashPath) + vi.useFakeTimers({ toFake: ["Date"] }) + // A throwing record would otherwise leave Date frozen for the tests after it. + try { + vi.setSystemTime(moment.toMillis()) + index.recordTrashEntry(recordedFile) + } finally { + vi.useRealTimers() + } +} + +/** The row the index holds for a path the test has just recorded. */ +const recordedRowOf = ( + index: ReturnType, + trashPath: string, +): TrashEntry => { + const recordedEntry = index.getTrashEntry(trashPath) + + if (!recordedEntry) throw new Error(`no trash entry recorded for ${trashPath}`) + return recordedEntry +} + +/** The trashedAt the index stored for a row the test has just recorded. */ +const trashedAtOf = (index: ReturnType, trashPath: string): number => { + return recordedRowOf(index, trashPath).trashedAt +} + +/** How many days after the real moment the sweeps below run: one more than + * their 30-day retention, so entries recorded now have expired. */ +const SWEEP_DAYS_LATER = 31 + +/** Runs a 30-day-retention sweep as if SWEEP_DAYS_LATER days had passed. + * Only Date is faked, so the kernel still stamps each file's change time at + * the real moment, as it does for the server's own trash move, and a row + * recorded now passes the sweep's change-time limit. */ +const sweepAfterRetention = async ( + vault: string, + trashEntryStore: TrashEntryStore, +): Promise => { + vi.useFakeTimers({ toFake: ["Date"] }) + onTestFinished(() => { + vi.useRealTimers() + }) + vi.setSystemTime(DateTime.now().plus({ days: SWEEP_DAYS_LATER }).toMillis()) + + await trashSweeper.sweepExpiredTrashEntries( + { vaultPath: vault, retentionDays: 30, trashEntryStore }, + logger, + ) + vi.useRealTimers() } +/** The warning the sweep logs for every file it keeps. */ +const KEPT_FILE_WARNING = + "trash entry cannot be matched to the file the server trashed — kept, row dropped" + +/** The index as a store whose row re-read fails for `failingPath`, as an I/O + * error on the index database would. */ +const storeFailingToRead = ( + index: ReturnType, + failingPath: string, +): TrashEntryStore => { + return { + listAllTrashEntries: index.listAllTrashEntries, + listExpiredTrashEntries: index.listExpiredTrashEntries, + getTrashEntry: (trashPath: string) => { + if (trashPath === failingPath) throw new Error("disk I/O error") + return index.getTrashEntry(trashPath) + }, + deleteTrashEntry: index.deleteTrashEntry, + } +} + +/** The index as a store whose row delete fails for `failingPath` the way a + * full disk fails it. */ +const storeFailingToDrop = ( + index: ReturnType, + failingPath: string, +): TrashEntryStore => { + return { + listAllTrashEntries: index.listAllTrashEntries, + listExpiredTrashEntries: index.listExpiredTrashEntries, + getTrashEntry: index.getTrashEntry, + deleteTrashEntry: (trashPath: string) => { + if (trashPath === failingPath) throw new Error("database or disk is full") + index.deleteTrashEntry(trashPath) + }, + } +} + describe("sweepExpiredTrashEntries", () => { - it("purges an expired entry's file and drops its row; a fresh entry and its file survive", async () => { + it("unlinks an expired entry's file and drops its row; a fresh entry and its file survive", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "old.md"), "expired", "utf8") await writeFile(join(vault, ".trash", "new.md"), "fresh", "utf8") - recordEntryDaysAgo(index, ".trash/old.md", 31) - index.recordTrashEntry(".trash/new.md") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/old.md")) + // Recorded a day before the sweep runs, so still inside the retention. + recordEntryAt( + index, + await recordedTrashFileAt(vault, ".trash/new.md"), + DateTime.now().plus({ days: SWEEP_DAYS_LATER - 1 }), + ) const infoSpy = vi.spyOn(logger, "info") onTestFinished(() => infoSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) await expect(stat(join(vault, ".trash", "old.md"))).rejects.toThrow(/ENOENT/) expect(index.getTrashEntry(".trash/old.md")).toBeNull() @@ -56,36 +185,110 @@ describe("sweepExpiredTrashEntries", () => { expired: 1, purged: 1, droppedMissing: 0, + droppedUnmatched: 0, }) }) - it("drops the row without throwing when the expired file is already gone", async () => { + it("drops the row and counts it as missing, without a warning, when the expired file is already gone", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/emptied.md", 31) + index.recordTrashEntry(recordedAbsentFile(".trash/emptied.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) expect(index.getTrashEntry(".trash/emptied.md")).toBeNull() + expect(warnSpy).not.toHaveBeenCalled() + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 1, + droppedUnmatched: 0, + }) }) it("drops the row when the entry's whole parent folder is gone", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") // The recorded subfolder never exists — realpath on the parent ENOENTs. - recordEntryDaysAgo(index, ".trash/vanished-folder/x.md", 31) + index.recordTrashEntry(recordedAbsentFile(".trash/vanished-folder/x.md")) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) expect(index.getTrashEntry(".trash/vanished-folder/x.md")).toBeNull() }) + it("drops the row without a warning when a file now stands where the entry's folder was", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await mkdir(join(vault, ".trash", "replaced")) + await writeFile(join(vault, ".trash", "replaced", "x.md"), "expired", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/replaced/x.md")) + // The file now at .trash/replaced still resolves inside .trash/, so the + // containment gates pass and the identity read fails with ENOTDIR. + await rm(join(vault, ".trash", "replaced"), { recursive: true }) + await writeFile(join(vault, ".trash", "replaced"), "a file where the folder was", "utf8") + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + + await sweepAfterRetention(vault, index) + + expect(index.getTrashEntry(".trash/replaced/x.md")).toBeNull() + const replacementContent = await readFile(join(vault, ".trash", "replaced"), "utf8") + expect(replacementContent).toBe("a file where the folder was") + expect(warnSpy).not.toHaveBeenCalled() + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 1, + droppedUnmatched: 0, + }) + }) + + it("drops the row when the file vanishes between the identity check and the unlink", async () => { + // The host owns the bind mount and can empty .trash/ at any moment, + // including after the identity check has matched the file. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "vanishing.md"), "expired", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/vanishing.md")) + // The sweep's only unlink call is this file's. The host removes the file + // first, so the sweep's own unlink then fails with ENOENT. + vi.mocked(unlink).mockImplementationOnce(async (target) => { + await rm(target) + return unlink(target) + }) + onTestFinished(() => { + vi.mocked(unlink).mockReset() + }) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + + await sweepAfterRetention(vault, index) + + await expect(stat(join(vault, ".trash", "vanishing.md"))).rejects.toThrow(/ENOENT/) + expect(index.getTrashEntry(".trash/vanishing.md")).toBeNull() + expect(warnSpy).not.toHaveBeenCalled() + // The file was present for the identity check, so only the unlink's + // ENOENT can count this row as missing rather than unlinked. + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 1, + droppedUnmatched: 0, + }) + }) + it("skips a listed row that was refreshed before its turn — the fresh file survives", async () => { // The expired list is a snapshot taken before any lock; a delete can // re-trash the same path (refreshing the row) before the sweep reaches @@ -93,28 +296,78 @@ describe("sweepExpiredTrashEntries", () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "raced.md"), "fresh copy", "utf8") - recordEntryDaysAgo(index, ".trash/raced.md", 31) + // The row holds the file's real identity, so only the in-lock re-read can + // keep this file. + const racedFile = await recordedTrashFileAt(vault, ".trash/raced.md") + index.recordTrashEntry(racedFile) + const getTrashEntry = vi.fn(index.getTrashEntry) const racingStore: TrashEntryStore = { listAllTrashEntries: index.listAllTrashEntries, listExpiredTrashEntries: (cutoffEpochSeconds: number) => { const listed = index.listExpiredTrashEntries(cutoffEpochSeconds) // The concurrent delete lands between the snapshot and the per-row - // lock: the row is refreshed to now. - index.recordTrashEntry(".trash/raced.md") + // lock, refreshing the row to now. + index.recordTrashEntry(racedFile) return listed }, - getTrashEntry: index.getTrashEntry, + getTrashEntry, deleteTrashEntry: index.deleteTrashEntry, } + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: racingStore }, - logger, - ) + await sweepAfterRetention(vault, racingStore) const racedContent = await readFile(join(vault, ".trash", "raced.md"), "utf8") expect(racedContent).toBe("fresh copy") expect(index.getTrashEntry(".trash/raced.md")?.trashPath).toBe(".trash/raced.md") + // The sweep re-read the listed row under the lock and unlinked nothing. + expect(getTrashEntry).toHaveBeenCalledTimes(1) + expect(getTrashEntry).toHaveBeenCalledWith(".trash/raced.md") + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 0, + droppedUnmatched: 0, + }) + }) + + it("skips a listed row that a concurrent delete cleared before its turn — the kept file survives", async () => { + // A "local" delete landing at the path clears the row while the sweep + // waits for the lock. The in-lock re-read then finds no row. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "cleared.md"), "keep forever", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/cleared.md")) + const getTrashEntry = vi.fn(index.getTrashEntry) + const racingStore: TrashEntryStore = { + listAllTrashEntries: index.listAllTrashEntries, + listExpiredTrashEntries: (cutoffEpochSeconds: number) => { + const listed = index.listExpiredTrashEntries(cutoffEpochSeconds) + index.deleteTrashEntry(".trash/cleared.md") + return listed + }, + getTrashEntry, + deleteTrashEntry: index.deleteTrashEntry, + } + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + + await sweepAfterRetention(vault, racingStore) + + const clearedContent = await readFile(join(vault, ".trash", "cleared.md"), "utf8") + expect(clearedContent).toBe("keep forever") + // The sweep re-read the listed row under the lock and unlinked nothing. + expect(getTrashEntry).toHaveBeenCalledTimes(1) + expect(getTrashEntry).toHaveBeenCalledWith(".trash/cleared.md") + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 0, + droppedUnmatched: 0, + }) }) it("refuses a row that resolves outside the vault — file untouched, row kept", async () => { @@ -124,18 +377,18 @@ describe("sweepExpiredTrashEntries", () => { await mkdir(join(vault, ".trash"), { recursive: true }) await writeFile(join(base, "escape.md"), "outside the vault", "utf8") const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/../../escape.md", 31) + // The row holds the escape target's real identity, so only the + // containment gate can keep it. + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/../../escape.md")) const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) const escapeContent = await readFile(join(base, "escape.md"), "utf8") expect(escapeContent).toBe("outside the vault") expect(index.getTrashEntry(".trash/../../escape.md")?.trashPath).toBe(".trash/../../escape.md") + expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy).toHaveBeenCalledWith("trash entry resolves outside .trash — skipped", { trashPath: ".trash/../../escape.md", }) @@ -148,17 +401,15 @@ describe("sweepExpiredTrashEntries", () => { await mkdir(join(vault, "Live"), { recursive: true }) await writeFile(join(vault, "Live", "x.md"), "live note", "utf8") const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/../Live/x.md", 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/../Live/x.md")) const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) const liveContent = await readFile(join(vault, "Live", "x.md"), "utf8") expect(liveContent).toBe("live note") + expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy).toHaveBeenCalledWith("trash entry resolves outside .trash — skipped", { trashPath: ".trash/../Live/x.md", }) @@ -172,17 +423,15 @@ describe("sweepExpiredTrashEntries", () => { await writeFile(join(vault, "RealNotes", "live.md"), "live note", "utf8") await symlink(join(vault, "RealNotes"), join(vault, ".trash", "linkdir")) const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/linkdir/live.md", 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/linkdir/live.md")) const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) const liveContent = await readFile(join(vault, "RealNotes", "live.md"), "utf8") expect(liveContent).toBe("live note") + expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy).toHaveBeenCalledWith("trash entry parent escapes .trash — skipped", { trashPath: ".trash/linkdir/live.md", }) @@ -194,24 +443,22 @@ describe("sweepExpiredTrashEntries", () => { await mkdir(lockedDir, { recursive: true }) await writeFile(join(lockedDir, "stuck.md"), "perm error", "utf8") const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/noaccess/stuck.md", 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/noaccess/stuck.md")) // Remove all permissions from .trash/ so realpath on the parent fails. await chmod(join(vault, ".trash"), 0o000) onTestFinished(() => chmod(join(vault, ".trash"), 0o755)) const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) expect(index.getTrashEntry(".trash/noaccess/stuck.md")?.trashPath).toBe( ".trash/noaccess/stuck.md", ) + expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy).toHaveBeenCalledWith("failed to resolve trash entry path", { trashPath: ".trash/noaccess/stuck.md", - error: expect.stringMatching(/EACCES/), + error: `[Error]: EACCES: permission denied, realpath '${lockedDir}'`, }) }) @@ -223,35 +470,32 @@ describe("sweepExpiredTrashEntries", () => { await mkdir(lockedDir, { recursive: true }) await writeFile(join(lockedDir, "stuck.md"), "perm error", "utf8") const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/locked/stuck.md", 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/locked/stuck.md")) // Remove write permission from the parent so unlink fails with EACCES. await chmod(lockedDir, 0o555) onTestFinished(() => chmod(lockedDir, 0o755)) const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) expect(index.getTrashEntry(".trash/locked/stuck.md")?.trashPath).toBe(".trash/locked/stuck.md") const content = await readFile(join(lockedDir, "stuck.md"), "utf8") expect(content).toBe("perm error") + expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy).toHaveBeenCalledWith("failed to remove expired trash entry", { trashPath: ".trash/locked/stuck.md", - error: expect.stringMatching(/EACCES.*stuck\.md/), + error: `[Error]: EACCES: permission denied, unlink '${join(lockedDir, "stuck.md")}'`, }) }) - it("never unlinks an unrecorded delete that landed at a stale row's path", async () => { - // A stale row can outlive its file (the user emptied .trash between - // sweeps). A keep-forever "local" delete that then lands at that path - // clears the row on the way in — otherwise this sweep would treat the - // fresh copy as the row's expired occupant and unlink it. + it("an unrecorded delete clears a stale row at its landed path before any sweep runs", async () => { + // A row can outlive its file in .trash/. A keep-forever "local" delete + // landing at that path drops the row on the way in, so the sweep never + // has to tell the new file apart from the one the row recorded. const vault = await createTestVault() const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/reused.md", 31) + index.recordTrashEntry(recordedAbsentFile(".trash/reused.md")) await writeFile(join(vault, "reused.md"), "keep forever", "utf8") const deleteResult = await vaultFs.deleteNote( { @@ -265,28 +509,270 @@ describe("sweepExpiredTrashEntries", () => { logger, ) expect(deleteResult.trashLocation).toBe(".trash/reused.md") + expect(index.getTrashEntry(".trash/reused.md")).toBeNull() - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) const keptContent = await readFile(join(vault, ".trash", "reused.md"), "utf8") expect(keptContent).toBe("keep forever") expect(index.getTrashEntry(".trash/reused.md")).toBeNull() }) - it("prunes the folder skeleton a purge empties, keeping .trash/ itself", async () => { + it("keeps a file whose change time runs more than 60 seconds past its row's trashedAt", async () => { + // A row recorded two minutes before the file last changed status stands + // for a note restored by hand and trashed again a while later: the + // renames kept its identity and moved only its change time. const vault = await createTestVault() const index = createSearchIndex(":memory:") - await mkdir(join(vault, ".trash", "sub", "deep"), { recursive: true }) - await writeFile(join(vault, ".trash", "sub", "deep", "old.md"), "expired", "utf8") - recordEntryDaysAgo(index, ".trash/sub/deep/old.md", 31) + await writeFile(join(vault, ".trash", "restored.md"), "deleted twice", "utf8") + recordEntryAt( + index, + await recordedTrashFileAt(vault, ".trash/restored.md"), + DateTime.now().minus({ minutes: 2 }), + ) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, + await sweepAfterRetention(vault, index) + + const restoredContent = await readFile(join(vault, ".trash", "restored.md"), "utf8") + expect(restoredContent).toBe("deleted twice") + expect(index.getTrashEntry(".trash/restored.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith(KEPT_FILE_WARNING, { + trashPath: ".trash/restored.md", + reason: "changed", + }) + }) + + it("unlinks a file whose change time is exactly 60 seconds past its row's trashedAt", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "boundary.md"), "expired", "utf8") + // Recorded two minutes back, so the file's real change time is past the + // limit and only the change time reported below can unlink it. + recordEntryAt( + index, + await recordedTrashFileAt(vault, ".trash/boundary.md"), + DateTime.now().minus({ minutes: 2 }), + ) + const trashedAt = trashedAtOf(index, ".trash/boundary.md") + // The sweep's only lstat call is the identity read, here reporting a + // change time at the very end of the allowance. + vi.mocked(lstat).mockImplementationOnce(async (target) => { + const fileStats = await lstat(target, { bigint: true }) + return Object.assign(fileStats, { ctimeNs: BigInt(trashedAt + 60) * 1_000_000_000n }) + }) + onTestFinished(() => { + vi.mocked(lstat).mockReset() + }) + + await sweepAfterRetention(vault, index) + + await expect(stat(join(vault, ".trash", "boundary.md"))).rejects.toThrow(/ENOENT/) + expect(index.getTrashEntry(".trash/boundary.md")).toBeNull() + }) + + it("keeps a file whose change time is one nanosecond past the 60-second allowance", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "boundary.md"), "expired", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/boundary.md")) + const trashedAt = trashedAtOf(index, ".trash/boundary.md") + // The sweep's only lstat call is the identity read, here reporting a + // change time just past the end of the allowance. + vi.mocked(lstat).mockImplementationOnce(async (target) => { + const fileStats = await lstat(target, { bigint: true }) + return Object.assign(fileStats, { ctimeNs: BigInt(trashedAt + 60) * 1_000_000_000n + 1n }) + }) + onTestFinished(() => { + vi.mocked(lstat).mockReset() + }) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + + await sweepAfterRetention(vault, index) + + const keptContent = await readFile(join(vault, ".trash", "boundary.md"), "utf8") + expect(keptContent).toBe("expired") + expect(index.getTrashEntry(".trash/boundary.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith(KEPT_FILE_WARNING, { + trashPath: ".trash/boundary.md", + reason: "changed", + }) + }) + + it("unlinks a file whose change time comes well before its row's trashedAt", async () => { + // Only a change after the row was recorded shows the file was touched + // since the server trashed it, so an earlier change time never keeps it. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "early.md"), "expired", "utf8") + // Recorded ten minutes after the file's change time, and still long + // enough before the sweep to have expired. + recordEntryAt( + index, + await recordedTrashFileAt(vault, ".trash/early.md"), + DateTime.now().plus({ minutes: 10 }), + ) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + + await sweepAfterRetention(vault, index) + + await expect(stat(join(vault, ".trash", "early.md"))).rejects.toThrow(/ENOENT/) + expect(index.getTrashEntry(".trash/early.md")).toBeNull() + expect(warnSpy).not.toHaveBeenCalled() + }) + + it("still unlinks a note restored by hand and trashed again within the 60-second allowance", async () => { + // The allowance covers the gap between the server's rename and its row, + // so a restore and re-trash inside it cannot be told apart from the + // server's own move. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, "restored.md"), "deleted twice", "utf8") + await vaultFs.deleteNote( + { + vaultPath: vault, + path: "restored.md", + protectedPaths: [], + pruneEmptyFolders: false, + trashOption: "system", + recordTrashEntry: index.recordTrashEntry, + }, + logger, + ) + await waitPastTimestampTick() + await rename(join(vault, ".trash", "restored.md"), join(vault, "restored.md")) + await rename(join(vault, "restored.md"), join(vault, ".trash", "restored.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + + await sweepAfterRetention(vault, index) + + await expect(stat(join(vault, ".trash", "restored.md"))).rejects.toThrow(/ENOENT/) + expect(index.getTrashEntry(".trash/restored.md")).toBeNull() + expect(warnSpy).not.toHaveBeenCalled() + }) + + it("keeps a note Obsidian trashed under a recycled name after .trash/ was emptied by hand", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, "recycled.md"), "server deleted this", "utf8") + await vaultFs.deleteNote( + { + vaultPath: vault, + path: "recycled.md", + protectedPaths: [], + pruneEmptyFolders: false, + trashOption: "system", + recordTrashEntry: index.recordTrashEntry, + }, logger, ) + // Emptying .trash/ by hand while the server runs leaves the row without + // its file; Obsidian then trashes a new note under the same name. + await rm(join(vault, ".trash", "recycled.md")) + await writeFile(join(vault, "recycled.md"), "obsidian trashed this", "utf8") + await rename(join(vault, "recycled.md"), join(vault, ".trash", "recycled.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + + await sweepAfterRetention(vault, index) + + const obsidianCopy = await readFile(join(vault, ".trash", "recycled.md"), "utf8") + expect(obsidianCopy).toBe("obsidian trashed this") + expect(index.getTrashEntry(".trash/recycled.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith(KEPT_FILE_WARNING, { + trashPath: ".trash/recycled.md", + reason: "replaced", + }) + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 0, + droppedUnmatched: 1, + }) + }) + + it("keeps the file and drops the row for an entry recorded without an identity", async () => { + // Rows recorded before identities were kept read back with a null + // identity. Such a row may already point at a file the server never + // trashed, so it can never authorize a delete. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "legacy.md"), "recorded long ago", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/legacy.md")) + const withoutIdentity = (entry: TrashEntry): TrashEntry => ({ ...entry, fileIdentity: null }) + const legacyStore: TrashEntryStore = { + listAllTrashEntries: index.listAllTrashEntries, + listExpiredTrashEntries: (cutoffEpochSeconds: number) => { + return index.listExpiredTrashEntries(cutoffEpochSeconds).map(withoutIdentity) + }, + getTrashEntry: (trashPath: string) => { + const entry = index.getTrashEntry(trashPath) + return entry ? withoutIdentity(entry) : null + }, + deleteTrashEntry: index.deleteTrashEntry, + } + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + + await sweepAfterRetention(vault, legacyStore) + + const legacyContent = await readFile(join(vault, ".trash", "legacy.md"), "utf8") + expect(legacyContent).toBe("recorded long ago") + expect(index.getTrashEntry(".trash/legacy.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith(KEPT_FILE_WARNING, { + trashPath: ".trash/legacy.md", + reason: "unrecorded", + }) + }) + + it("keeps the row and warns when the identity read fails with a non-ENOENT error", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + const lockedDir = join(vault, ".trash", "nosearch") + await mkdir(lockedDir) + await writeFile(join(lockedDir, "stuck.md"), "stays", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/nosearch/stuck.md")) + // Without search permission the folder itself still resolves, so the + // containment gate passes, but no entry inside it can be looked up. + await chmod(lockedDir, 0o600) + onTestFinished(() => chmod(lockedDir, 0o755)) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + + await sweepAfterRetention(vault, index) + + expect(index.getTrashEntry(".trash/nosearch/stuck.md")?.trashPath).toBe( + ".trash/nosearch/stuck.md", + ) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to read trash entry identity", { + trashPath: ".trash/nosearch/stuck.md", + error: `[Error]: EACCES: permission denied, lstat '${join(lockedDir, "stuck.md")}'`, + }) + await chmod(lockedDir, 0o755) + const stuckContent = await readFile(join(lockedDir, "stuck.md"), "utf8") + expect(stuckContent).toBe("stays") + }) + + it("prunes the folders an unlink empties, keeping .trash/ itself", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await mkdir(join(vault, ".trash", "sub", "deep"), { recursive: true }) + await writeFile(join(vault, ".trash", "sub", "deep", "old.md"), "expired", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/sub/deep/old.md")) + + await sweepAfterRetention(vault, index) await expect(stat(join(vault, ".trash", "sub", "deep", "old.md"))).rejects.toThrow(/ENOENT/) await expect(stat(join(vault, ".trash", "sub"))).rejects.toThrow(/ENOENT/) @@ -294,24 +780,229 @@ describe("sweepExpiredTrashEntries", () => { expect(trashRootStat.isDirectory()).toBe(true) }) - it("keeps a purged file's folder when it still holds other files", async () => { + it("keeps an unlinked file's folder when it still holds other files", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await mkdir(join(vault, ".trash", "shared"), { recursive: true }) await writeFile(join(vault, ".trash", "shared", "old.md"), "expired", "utf8") await writeFile(join(vault, ".trash", "shared", "kept.md"), "stays", "utf8") - recordEntryDaysAgo(index, ".trash/shared/old.md", 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/shared/old.md")) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) await expect(stat(join(vault, ".trash", "shared", "old.md"))).rejects.toThrow(/ENOENT/) const keptContent = await readFile(join(vault, ".trash", "shared", "kept.md"), "utf8") expect(keptContent).toBe("stays") }) + it("warns and moves on to the next row when pruning an unlinked file's folders fails", async () => { + // A hidden folder segment makes the prune's path check throw. Only a + // corrupted or hand-edited row can hold one, and that row must not end + // the sweep for the rows listed after it. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await mkdir(join(vault, ".trash", ".stuck"), { recursive: true }) + await writeFile(join(vault, ".trash", ".stuck", "x.md"), "expired", "utf8") + await writeFile(join(vault, ".trash", "after.md"), "expired too", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/.stuck/x.md")) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/after.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + + await sweepAfterRetention(vault, index) + + await expect(stat(join(vault, ".trash", ".stuck", "x.md"))).rejects.toThrow(/ENOENT/) + await expect(stat(join(vault, ".trash", "after.md"))).rejects.toThrow(/ENOENT/) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to prune emptied trash folders", { + trashPath: ".trash/.stuck/x.md", + error: '[Error]: hidden path blocked: ".stuck/x.md" targets a hidden file or folder', + }) + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 2, + purged: 2, + droppedMissing: 0, + droppedUnmatched: 0, + }) + }) + + it("leaves a row whose re-read fails and sweeps the rest", async () => { + // The summary log runs only after the last row, so it proves the failed + // read did not end the sweep, whichever row the store lists first. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "unread.md"), "expired", "utf8") + await writeFile(join(vault, ".trash", "other.md"), "expired too", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/unread.md")) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/other.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + const unreadRow = recordedRowOf(index, ".trash/unread.md") + + await sweepAfterRetention(vault, storeFailingToRead(index, ".trash/unread.md")) + + const unreadContent = await readFile(join(vault, ".trash", "unread.md"), "utf8") + expect(unreadContent).toBe("expired") + expect(index.getTrashEntry(".trash/unread.md")).toEqual(unreadRow) + await expect(stat(join(vault, ".trash", "other.md"))).rejects.toThrow(/ENOENT/) + expect(index.getTrashEntry(".trash/other.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to read trash entry row", { + trashPath: ".trash/unread.md", + error: "[Error]: disk I/O error", + }) + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 2, + purged: 1, + droppedMissing: 0, + droppedUnmatched: 0, + }) + }) + + it("keeps an unlinked file's row and finishes the sweep when the row drop fails", async () => { + // The summary log runs only after the last row, so it proves the failed + // drop did not end the sweep, whichever row the store lists first. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "stuck.md"), "expired", "utf8") + await writeFile(join(vault, ".trash", "other.md"), "expired too", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/stuck.md")) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/other.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + const stuckRow = recordedRowOf(index, ".trash/stuck.md") + + await sweepAfterRetention(vault, storeFailingToDrop(index, ".trash/stuck.md")) + + await expect(stat(join(vault, ".trash", "stuck.md"))).rejects.toThrow(/ENOENT/) + expect(index.getTrashEntry(".trash/stuck.md")).toEqual(stuckRow) + await expect(stat(join(vault, ".trash", "other.md"))).rejects.toThrow(/ENOENT/) + expect(index.getTrashEntry(".trash/other.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to drop trash entry row", { + trashPath: ".trash/stuck.md", + error: "[Error]: database or disk is full", + }) + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 2, + purged: 2, + droppedMissing: 0, + droppedUnmatched: 0, + }) + }) + + it.each([ + { label: "the file", trashPath: ".trash/emptied.md" }, + { label: "its whole folder", trashPath: ".trash/vanished-folder/x.md" }, + ])( + "keeps the row, counted as skipped, when $label is gone and the row drop fails", + async ({ trashPath }) => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + index.recordTrashEntry(recordedAbsentFile(trashPath)) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + const keptRow = recordedRowOf(index, trashPath) + + await sweepAfterRetention(vault, storeFailingToDrop(index, trashPath)) + + expect(index.getTrashEntry(trashPath)).toEqual(keptRow) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to drop trash entry row", { + trashPath, + error: "[Error]: database or disk is full", + }) + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 0, + droppedUnmatched: 0, + }) + }, + ) + + it("keeps the row, counted as skipped, when the file vanishes before the unlink and the row drop fails", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "vanishing.md"), "expired", "utf8") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/vanishing.md")) + vi.mocked(unlink).mockImplementationOnce(async (target) => { + await rm(target) + return unlink(target) + }) + onTestFinished(() => { + vi.mocked(unlink).mockReset() + }) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + const vanishingRow = recordedRowOf(index, ".trash/vanishing.md") + + await sweepAfterRetention(vault, storeFailingToDrop(index, ".trash/vanishing.md")) + + // With the drop failing, a file kept as unmatched would leave the same row, + // warning, and counts, so only the file's absence proves the unlink ran. + await expect(stat(join(vault, ".trash", "vanishing.md"))).rejects.toThrow(/ENOENT/) + expect(index.getTrashEntry(".trash/vanishing.md")).toEqual(vanishingRow) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to drop trash entry row", { + trashPath: ".trash/vanishing.md", + error: "[Error]: database or disk is full", + }) + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 0, + droppedUnmatched: 0, + }) + }) + + it("keeps an unmatched file and its row, without the row-dropped warning, when the row drop fails", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + await writeFile(join(vault, ".trash", "recycled.md"), "obsidian trashed this", "utf8") + // The recorded identity belongs to no file, so the file at the path is + // a different one and fails the identity check. + index.recordTrashEntry(recordedAbsentFile(".trash/recycled.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + const recycledRow = recordedRowOf(index, ".trash/recycled.md") + + await sweepAfterRetention(vault, storeFailingToDrop(index, ".trash/recycled.md")) + + const recycledContent = await readFile(join(vault, ".trash", "recycled.md"), "utf8") + expect(recycledContent).toBe("obsidian trashed this") + expect(index.getTrashEntry(".trash/recycled.md")).toEqual(recycledRow) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to drop trash entry row", { + trashPath: ".trash/recycled.md", + error: "[Error]: database or disk is full", + }) + expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { + retentionDays: 30, + expired: 1, + purged: 0, + droppedMissing: 0, + droppedUnmatched: 0, + }) + }) + it("never touches a trash file it has no row for, however old", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") @@ -319,12 +1010,9 @@ describe("sweepExpiredTrashEntries", () => { // recorded expired file so a passing test proves the sweep actually ran. await writeFile(join(vault, ".trash", "obsidian-own.md"), "obsidian trashed this", "utf8") await writeFile(join(vault, ".trash", "recorded.md"), "ours", "utf8") - recordEntryDaysAgo(index, ".trash/recorded.md", 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/recorded.md")) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) await expect(stat(join(vault, ".trash", "recorded.md"))).rejects.toThrow(/ENOENT/) const obsidianOwnContent = await readFile(join(vault, ".trash", "obsidian-own.md"), "utf8") @@ -337,7 +1025,14 @@ describe("startTrashSweepSchedule", () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "startup.md"), "expired", "utf8") - recordEntryDaysAgo(index, ".trash/startup.md", 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/startup.md")) + // Only Date moves forward, as in sweepAfterRetention, so the row has + // expired while the file's change time still meets the limit. + vi.useFakeTimers({ toFake: ["Date"] }) + onTestFinished(() => { + vi.useRealTimers() + }) + vi.setSystemTime(DateTime.now().plus({ days: SWEEP_DAYS_LATER }).toMillis()) trashSweeper.startTrashSweepSchedule( { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, @@ -393,8 +1088,8 @@ describe("purgeOrphanedTrashEntries", () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "present.md"), "still here", "utf8") - index.recordTrashEntry(".trash/present.md") - index.recordTrashEntry(".trash/gone.md") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/present.md")) + index.recordTrashEntry(recordedAbsentFile(".trash/gone.md")) const infoSpy = vi.spyOn(logger, "info") onTestFinished(() => infoSpy.mockRestore()) @@ -435,8 +1130,8 @@ describe("purgeOrphanedTrashEntries", () => { const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "a.md"), "content", "utf8") await writeFile(join(vault, ".trash", "b.md"), "content", "utf8") - index.recordTrashEntry(".trash/a.md") - index.recordTrashEntry(".trash/b.md") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/a.md")) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/b.md")) const infoSpy = vi.spyOn(logger, "info") onTestFinished(() => infoSpy.mockRestore()) @@ -458,15 +1153,15 @@ describe("purgeOrphanedTrashEntries", () => { const index = createSearchIndex(":memory:") // Back-date the original so the refresh (recorded at "now") gets a // different trashedAt — both within the same second would match. - recordEntryDaysAgo(index, ".trash/raced.md", 1) - // Control orphan: proves the purge ran (dropped if the function executes). - index.recordTrashEntry(".trash/control-orphan.md") + recordEntryAt(index, recordedAbsentFile(".trash/raced.md"), DateTime.now().minus({ days: 1 })) + // A control orphan, dropped whenever the purge runs, proves it ran. + index.recordTrashEntry(recordedAbsentFile(".trash/control-orphan.md")) const racingStore: TrashEntryStore = { listAllTrashEntries: () => { const listed = index.listAllTrashEntries() // A concurrent trash move refreshes the row between the snapshot // and the per-row lock. - index.recordTrashEntry(".trash/raced.md") + index.recordTrashEntry(recordedAbsentFile(".trash/raced.md")) return listed }, listExpiredTrashEntries: index.listExpiredTrashEntries, @@ -483,12 +1178,56 @@ describe("purgeOrphanedTrashEntries", () => { expect(index.getTrashEntry(".trash/control-orphan.md")).toBeNull() }) + it.each([ + { + label: "another file's record at the same path", + replacement: { trashPath: ".trash/raced.md", fileIdentity: "1:1:1" }, + }, + { + label: "a case alias's record", + replacement: { trashPath: ".trash/RACED.md", fileIdentity: ABSENT_FILE_IDENTITY }, + }, + ])("skips a row replaced within the same second by $label", async ({ replacement }) => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + // Both records share one moment, so their trashedAt values are equal. + const recordedMoment = DateTime.now().minus({ days: 1 }) + recordEntryAt(index, recordedAbsentFile(".trash/raced.md"), recordedMoment) + // A control orphan, dropped whenever the purge runs, proves it ran. + index.recordTrashEntry(recordedAbsentFile(".trash/control-orphan.md")) + const racingStore: TrashEntryStore = { + listAllTrashEntries: () => { + const listed = index.listAllTrashEntries() + // A concurrent trash move replaces the row between the snapshot and + // the per-row lock. + recordEntryAt(index, replacement, recordedMoment) + return listed + }, + listExpiredTrashEntries: index.listExpiredTrashEntries, + getTrashEntry: index.getTrashEntry, + deleteTrashEntry: index.deleteTrashEntry, + } + + await trashSweeper.purgeOrphanedTrashEntries( + { vaultPath: vault, trashEntryStore: racingStore }, + logger, + ) + + // No file is on disk at either spelling, so only the re-read's comparison + // can keep the replacement's row. + expect(index.getTrashEntry(".trash/raced.md")).toEqual({ + ...replacement, + trashedAt: recordedMoment.toUnixInteger(), + }) + expect(index.getTrashEntry(".trash/control-orphan.md")).toBeNull() + }) + it("skips a row deleted between listing and lock acquisition", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") - index.recordTrashEntry(".trash/deleted-mid-flight.md") - // Control orphan: proves the purge ran. - index.recordTrashEntry(".trash/control-orphan.md") + index.recordTrashEntry(recordedAbsentFile(".trash/deleted-mid-flight.md")) + // A control orphan, dropped whenever the purge runs, proves it ran. + index.recordTrashEntry(recordedAbsentFile(".trash/control-orphan.md")) const racingStore: TrashEntryStore = { listAllTrashEntries: () => { const listed = index.listAllTrashEntries() @@ -520,7 +1259,7 @@ describe("purgeOrphanedTrashEntries", () => { it("drops a row when the parent folder is gone", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") - index.recordTrashEntry(".trash/vanished-folder/x.md") + index.recordTrashEntry(recordedAbsentFile(".trash/vanished-folder/x.md")) await trashSweeper.purgeOrphanedTrashEntries( { vaultPath: vault, trashEntryStore: index }, @@ -533,9 +1272,9 @@ describe("purgeOrphanedTrashEntries", () => { it("logs checked and purged counts when orphans are found", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") - index.recordTrashEntry(".trash/a.md") - index.recordTrashEntry(".trash/b.md") - index.recordTrashEntry(".trash/c.md") + index.recordTrashEntry(recordedAbsentFile(".trash/a.md")) + index.recordTrashEntry(recordedAbsentFile(".trash/b.md")) + index.recordTrashEntry(recordedAbsentFile(".trash/c.md")) const infoSpy = vi.spyOn(logger, "info") onTestFinished(() => infoSpy.mockRestore()) @@ -550,13 +1289,73 @@ describe("purgeOrphanedTrashEntries", () => { }) }) + it("keeps an orphan's row and purges the rest when one row re-read fails", async () => { + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + index.recordTrashEntry(recordedAbsentFile(".trash/unread.md")) + index.recordTrashEntry(recordedAbsentFile(".trash/other.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + const unreadRow = recordedRowOf(index, ".trash/unread.md") + + await trashSweeper.purgeOrphanedTrashEntries( + { vaultPath: vault, trashEntryStore: storeFailingToRead(index, ".trash/unread.md") }, + logger, + ) + + expect(index.getTrashEntry(".trash/unread.md")).toEqual(unreadRow) + expect(index.getTrashEntry(".trash/other.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to read trash entry row", { + trashPath: ".trash/unread.md", + error: "[Error]: disk I/O error", + }) + expect(infoSpy).toHaveBeenCalledWith("orphaned trash entries purged", { + checked: 2, + purged: 1, + }) + }) + + it("keeps an orphan's row and purges the rest when one row drop fails", async () => { + // The summary log runs only after the last row, so it proves the failed + // drop did not end the purge, whichever row the store lists first. + const vault = await createTestVault() + const index = createSearchIndex(":memory:") + index.recordTrashEntry(recordedAbsentFile(".trash/stuck.md")) + index.recordTrashEntry(recordedAbsentFile(".trash/other.md")) + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const infoSpy = vi.spyOn(logger, "info") + onTestFinished(() => infoSpy.mockRestore()) + const stuckRow = recordedRowOf(index, ".trash/stuck.md") + + await trashSweeper.purgeOrphanedTrashEntries( + { vaultPath: vault, trashEntryStore: storeFailingToDrop(index, ".trash/stuck.md") }, + logger, + ) + + expect(index.getTrashEntry(".trash/stuck.md")).toEqual(stuckRow) + expect(index.getTrashEntry(".trash/other.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy).toHaveBeenCalledWith("failed to drop trash entry row", { + trashPath: ".trash/stuck.md", + error: "[Error]: database or disk is full", + }) + expect(infoSpy).toHaveBeenCalledWith("orphaned trash entries purged", { + checked: 2, + purged: 1, + }) + }) + it("keeps the row and warns when stat fails with a non-ENOENT error", async () => { const vault = await createTestVault() const lockedDir = join(vault, ".trash", "locked") await mkdir(lockedDir, { recursive: true }) await writeFile(join(lockedDir, "stuck.md"), "perm error", "utf8") const index = createSearchIndex(":memory:") - index.recordTrashEntry(".trash/locked/stuck.md") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/locked/stuck.md")) // Remove traverse permission from the parent so stat fails with EACCES. await chmod(lockedDir, 0o000) onTestFinished(() => chmod(lockedDir, 0o755)) @@ -569,9 +1368,10 @@ describe("purgeOrphanedTrashEntries", () => { ) expect(index.getTrashEntry(".trash/locked/stuck.md")?.trashPath).toBe(".trash/locked/stuck.md") + expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy).toHaveBeenCalledWith("failed to stat trash entry", { trashPath: ".trash/locked/stuck.md", - error: expect.stringMatching(/EACCES/), + error: `[Error]: EACCES: permission denied, lstat '${join(lockedDir, "stuck.md")}'`, }) }) @@ -583,9 +1383,9 @@ describe("purgeOrphanedTrashEntries", () => { await mkdir(join(vault, "Live"), { recursive: true }) await writeFile(join(vault, "Live", "note.md"), "live note", "utf8") const index = createSearchIndex(":memory:") - index.recordTrashEntry(".trash/../Live/note.md") - // Control orphan: proves the purge ran. - index.recordTrashEntry(".trash/control-orphan.md") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/../Live/note.md")) + // A control orphan, dropped whenever the purge runs, proves it ran. + index.recordTrashEntry(recordedAbsentFile(".trash/control-orphan.md")) await trashSweeper.purgeOrphanedTrashEntries( { vaultPath: vault, trashEntryStore: index }, @@ -602,9 +1402,9 @@ describe("purgeOrphanedTrashEntries", () => { const vault = await createTestVault() await symlink(join(vault, "nonexistent-target"), join(vault, ".trash", "dangling.md")) const index = createSearchIndex(":memory:") - index.recordTrashEntry(".trash/dangling.md") - // Control orphan: proves the purge ran. - index.recordTrashEntry(".trash/control-orphan.md") + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/dangling.md")) + // A control orphan, dropped whenever the purge runs, proves it ran. + index.recordTrashEntry(recordedAbsentFile(".trash/control-orphan.md")) await trashSweeper.purgeOrphanedTrashEntries( { vaultPath: vault, trashEntryStore: index }, diff --git a/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts b/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts index 46e26fb90..1077520dd 100644 --- a/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts @@ -27,8 +27,11 @@ import { atomicWriteFile, atomicWriteFileExclusive, pruneEmptyParents, + readTrashFileState, resolveSafePath, + trashDomainLockKey, } from "../vault-filesystem.js" +import { withFileLock } from "../../../utils/file-write-lock.js" import { OverwriteBlockedError, parseNote, @@ -1038,6 +1041,144 @@ describe("deleteNote", () => { }) }) +describe("readTrashFileState", () => { + /** A whole-second timestamp, so every file set to it has the same mtimeNs. */ + const FIXED_EPOCH_SECONDS = 1_700_000_000 + + /** Linux stamps inode times from a clock that advances once per scheduler + * tick (up to 10 ms), so two changes inside one tick share a change time. + * Waiting past a tick makes the next change land a new one. */ + const waitPastTimestampTick = async (): Promise => { + await new Promise((resolve) => setTimeout(resolve, 25)) + } + + it("reads the link's own inode number, size, modification time and change time", async () => { + await writeFile(join(vault, "target.md"), "a longer target body", "utf8") + await symlink(join(vault, "target.md"), join(vault, "link.md")) + const linkStats = await lstat(join(vault, "link.md"), { bigint: true }) + const targetStats = await lstat(join(vault, "target.md"), { bigint: true }) + // The link and its target differ in inode and size, so reading through + // the link would change the expected identity. + expect(linkStats.ino).not.toBe(targetStats.ino) + + const linkState = await readTrashFileState(join(vault, "link.md")) + + expect(linkState).toEqual({ + identity: `${linkStats.ino}:${linkStats.size}:${linkStats.mtimeNs}`, + changeTimeNs: linkStats.ctimeNs, + }) + }) + + it("stays the same when the file is only read", async () => { + const notePath = join(vault, "read-only.md") + await writeFile(notePath, "content", "utf8") + const stateBeforeRead = await readTrashFileState(notePath) + await waitPastTimestampTick() + + await readFile(notePath, "utf8") + + expect(await readTrashFileState(notePath)).toEqual(stateBeforeRead) + }) + + it("keeps the identity but moves the change time when the file is renamed out of .trash/ and back", async () => { + // A trashed note restored by hand and trashed again keeps its inode, size + // and modification time; only the change time records the two renames. + await mkdir(join(vault, ".trash"), { recursive: true }) + await writeFile(join(vault, ".trash", "restored.md"), "content", "utf8") + const stateWhenTrashed = await readTrashFileState(join(vault, ".trash", "restored.md")) + await waitPastTimestampTick() + + await rename(join(vault, ".trash", "restored.md"), join(vault, "restored.md")) + await rename(join(vault, "restored.md"), join(vault, ".trash", "restored.md")) + + const stateWhenRetrashed = await readTrashFileState(join(vault, ".trash", "restored.md")) + expect(stateWhenRetrashed.identity).toBe(stateWhenTrashed.identity) + expect(stateWhenRetrashed.changeTimeNs).toBeGreaterThan(stateWhenTrashed.changeTimeNs) + }) + + it("keeps the identity but moves the change time when only the file's attributes change", async () => { + const notePath = join(vault, "chmodded.md") + await writeFile(notePath, "content", "utf8") + const originalState = await readTrashFileState(notePath) + await waitPastTimestampTick() + + await chmod(notePath, 0o600) + + const chmoddedState = await readTrashFileState(notePath) + expect(chmoddedState.identity).toBe(originalState.identity) + expect(chmoddedState.changeTimeNs).toBeGreaterThan(originalState.changeTimeNs) + }) + + it("differs by inode number for another file with the same size and modification time", async () => { + await writeFile(join(vault, "first.md"), "same", "utf8") + await writeFile(join(vault, "second.md"), "same", "utf8") + await utimes(join(vault, "first.md"), FIXED_EPOCH_SECONDS, FIXED_EPOCH_SECONDS) + await utimes(join(vault, "second.md"), FIXED_EPOCH_SECONDS, FIXED_EPOCH_SECONDS) + const firstStats = await lstat(join(vault, "first.md"), { bigint: true }) + const secondStats = await lstat(join(vault, "second.md"), { bigint: true }) + // Same size and modification time, so the inode number is the only field + // left to tell the two files apart. + expect([secondStats.size, secondStats.mtimeNs]).toEqual([firstStats.size, firstStats.mtimeNs]) + + const firstState = await readTrashFileState(join(vault, "first.md")) + const secondState = await readTrashFileState(join(vault, "second.md")) + + expect(secondState.identity).not.toBe(firstState.identity) + }) + + it("differs by modification time after the file is rewritten in place at the same size", async () => { + const notePath = join(vault, "rewritten.md") + await writeFile(notePath, "aaaa", "utf8") + await utimes(notePath, FIXED_EPOCH_SECONDS, FIXED_EPOCH_SECONDS) + const originalStats = await lstat(notePath, { bigint: true }) + const originalState = await readTrashFileState(notePath) + + await writeFile(notePath, "bbbb", "utf8") + await utimes(notePath, FIXED_EPOCH_SECONDS + 60, FIXED_EPOCH_SECONDS + 60) + + // Same inode and size; the rewrite moved the modification time. + const rewrittenStats = await lstat(notePath, { bigint: true }) + expect([rewrittenStats.ino, rewrittenStats.size]).toEqual([ + originalStats.ino, + originalStats.size, + ]) + const rewrittenState = await readTrashFileState(notePath) + expect(rewrittenState.identity).not.toBe(originalState.identity) + }) + + it("differs by size after the file grows, even with its modification time restored", async () => { + const notePath = join(vault, "resized.md") + await writeFile(notePath, "short", "utf8") + await utimes(notePath, FIXED_EPOCH_SECONDS, FIXED_EPOCH_SECONDS) + const originalStats = await lstat(notePath, { bigint: true }) + const originalState = await readTrashFileState(notePath) + + await writeFile(notePath, "much longer content", "utf8") + await utimes(notePath, FIXED_EPOCH_SECONDS, FIXED_EPOCH_SECONDS) + + const resizedStats = await lstat(notePath, { bigint: true }) + expect([resizedStats.ino, resizedStats.mtimeNs]).toEqual([ + originalStats.ino, + originalStats.mtimeNs, + ]) + const resizedState = await readTrashFileState(notePath) + expect(resizedState.identity).not.toBe(originalState.identity) + }) + + it("rejects with lstat's ENOENT when nothing is at the path", async () => { + // The sweep reads the error code to tell a missing file from a failed + // read. A partial match, because the error also carries a stack trace. + const absentPath = join(vault, "absent.md") + + await expect(readTrashFileState(absentPath)).rejects.toMatchObject({ + message: `ENOENT: no such file or directory, lstat '${absentPath}'`, + code: "ENOENT", + syscall: "lstat", + path: absentPath, + }) + }) +}) + describe("deleteNote — trash behavior", () => { it('moves the note to .trash/ when trashOption is "local"', async () => { await writeFile(join(vault, "trash-me.md"), "content", "utf8") @@ -1167,10 +1308,11 @@ describe("deleteNote — trash behavior", () => { await expect(stat(join(vault, "sys.md"))).rejects.toThrow(/ENOENT/) }) - it("reports the landed trash path to recordTrashEntry exactly once, suffixes included", async () => { + it("reports the landed trash path and the landed file's identity to recordTrashEntry exactly once, suffixes included", async () => { // The recorded path must be where the file actually landed — a collision // shifts it to a suffixed name, and recording the original would make the - // retention sweep act on someone else's trash copy. + // retention sweep act on someone else's trash copy. The identity must + // describe that landed file, or the sweep would never match it. await mkdir(join(vault, ".trash"), { recursive: true }) await writeFile(join(vault, ".trash", "rec.md"), "older copy", "utf8") await writeFile(join(vault, "rec.md"), "fresh", "utf8") @@ -1189,14 +1331,57 @@ describe("deleteNote — trash behavior", () => { ) expect(result.trashLocation).toBe(".trash/rec 1.md") + const landedStats = await lstat(join(vault, ".trash", "rec 1.md"), { bigint: true }) expect(recordTrashEntry).toHaveBeenCalledTimes(1) - expect(recordTrashEntry).toHaveBeenCalledWith(".trash/rec 1.md") + expect(recordTrashEntry).toHaveBeenCalledWith({ + trashPath: ".trash/rec 1.md", + fileIdentity: `${landedStats.ino}:${landedStats.size}:${landedStats.mtimeNs}`, + }) + }) + + it("contains a failed identity read — warns, clears the stale row, and the delete still succeeds", async () => { + // The identity is read after the rename, so its failure must take the same + // fail-open path as a failed row write, never the claim cleanup that + // would delete the moved note. + await writeFile(join(vault, "noident.md"), "survives", "utf8") + const warnSpy = vi.spyOn(logger, "warn") + onTestFinished(() => warnSpy.mockRestore()) + const recordTrashEntry = vi.fn() + const clearStaleTrashEntry = vi.fn() + // The delete path's only lstat call is the identity read on the landed file. + vi.mocked(lstat).mockImplementationOnce(async () => { + throw new Error("EIO: injected lstat failure") + }) + + const result = await deleteNote( + { + vaultPath: vault, + path: "noident.md", + protectedPaths: [], + pruneEmptyFolders: false, + trashOption: "system", + recordTrashEntry, + clearStaleTrashEntry, + }, + logger, + ) + + expect(result.trashLocation).toBe(".trash/noident.md") + const trashedContent = await readFile(join(vault, ".trash", "noident.md"), "utf8") + expect(trashedContent).toBe("survives") + expect(recordTrashEntry).not.toHaveBeenCalled() + expect(clearStaleTrashEntry).toHaveBeenCalledTimes(1) + expect(clearStaleTrashEntry).toHaveBeenCalledWith(".trash/noident.md") + expect(warnSpy).toHaveBeenCalledWith("failed to record trash entry", { + path: ".trash/noident.md", + error: "[Error]: EIO: injected lstat failure", + }) }) it("contains a throwing recordTrashEntry — warns, and the delete still succeeds with the note intact in .trash/", async () => { - // Recording is fail-open: the rename has already happened, so a throw - // must not reach the claim-cleanup path, which would delete the moved - // note itself. An unrecorded entry is simply never swept. + // The rename has already happened when recording runs, so a throw must + // not reach the claim cleanup, which would delete the moved note itself. + // An unrecorded entry is never swept. await writeFile(join(vault, "unrec.md"), "survives", "utf8") const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) @@ -1612,6 +1797,45 @@ describe("deleteNote — trash behavior", () => { expect(await readFile(join(vault, resultB.trashLocation), "utf8")).toBe("payload-b") }) + it("creates the trash folder under the trash-domain lock, so a sweep's folder prune cannot fail the move", async () => { + await mkdir(join(vault, "Projects"), { recursive: true }) + await writeFile(join(vault, "Projects", "late.md"), "content", "utf8") + // As soon as the move's trash folder exists, queue what the retention + // sweep does after unlinking that folder's last expired file: prune it + // under the shared trash-domain lock. A folder created before the move + // takes the lock is still empty when the queued prune runs, so the prune + // removes it and the move's claim fails. + const queuedSweepPrunes: Promise[] = [] + vi.mocked(mkdir).mockImplementationOnce(async () => { + await mkdir(join(vault, ".trash", "Projects"), { recursive: true }) + queuedSweepPrunes.push( + withFileLock(trashDomainLockKey(vault), () => { + return pruneEmptyParents( + { vaultPath: join(vault, ".trash"), path: "Projects/swept.md" }, + logger, + ) + }), + ) + return undefined + }) + + const result = await deleteNote( + { + vaultPath: vault, + path: "Projects/late.md", + protectedPaths: [], + pruneEmptyFolders: false, + trashOption: "local", + }, + logger, + ) + + // The prune ran after the move, found the trashed note, and removed nothing. + expect(await Promise.all(queuedSweepPrunes)).toEqual([0]) + expect(result.trashLocation).toBe(".trash/Projects/late.md") + expect(await readFile(join(vault, ".trash", "Projects", "late.md"), "utf8")).toBe("content") + }) + it("removes its claim placeholder and preserves the source when the rename fails", async () => { await writeFile(join(vault, "renamefail.md"), "keep me", "utf8") const warnSpy = vi.spyOn(logger, "warn") diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index aef7736ca..81f33470f 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -1,21 +1,35 @@ /** Trash bookkeeping — retention sweep (unlink expired files) and orphan * purge (drop rows whose files are gone). Both operate on trash_entries - * rows, never walk the .trash/ folder, so Obsidian's own trash entries - * and hand-placed files are out of reach. */ + * rows and never walk the .trash/ folder. + * + * The sweep unlinks a file only while both of these hold: + * - the file still has the identity its row recorded (inode number, size, + * and modification time) + * - its change time is no later than about a minute after the row was + * recorded + * + * The two checks keep Obsidian's own trash entries and hand-placed files out + * of reach, even under a name the server once used. Two cases get through: + * - a note the server trashed that was then restored by hand and trashed + * again within the minute, or on a file system whose renames leave the + * change time alone + * - a file swapped in between the identity read and the unlink, a window of + * one call that Node offers no atomic way to close */ import { unlink } from "node:fs/promises" import { dirname, join, relative, resolve, sep } from "node:path" import { DateTime } from "luxon" import { describeError } from "../../utils/describe-error.js" -import { lstatOrNull, realpathOrNull } from "../../utils/fs.js" +import { isMissingPathError, lstatOrNull, realpathOrNull } from "../../utils/fs.js" import { isErrnoException } from "../../utils/is-errno-exception.js" import { withFileLock } from "../../utils/file-write-lock.js" -import { pruneEmptyParents, trashDomainLockKey } from "./vault-filesystem.js" -import type { TrashEntryStore } from "../search/search-index.js" +import { pruneEmptyParents, readTrashFileState, trashDomainLockKey } from "./vault-filesystem.js" +import type { TrashFileState } from "./vault-filesystem.js" +import type { TrashEntry, TrashEntryStore } from "../search/search-index.js" import type { Logger } from "../../logger.js" -/** Sweep cadence. Bounds how stale the retention window can get between - * runs — the window itself is TRASH_RETENTION_DAYS. */ +/** Sweep cadence. An expired file can outlive TRASH_RETENTION_DAYS by up to + * one interval before the next sweep deletes it. */ const SWEEP_INTERVAL_MS = 24 * 60 * 60 * 1000 type SweepParams = { @@ -24,12 +38,106 @@ type SweepParams = { trashEntryStore: TrashEntryStore } -type SweepRowOutcome = "purged" | "missing" | "skipped" +type SweepRowOutcome = "unlinked" | "missing" | "unmatched" | "skipped" -/** Processes one expired row: re-validate, contain, unlink, drop the row, - * prune the folders the unlink emptied. Runs under the shared trash-domain - * lock (the caller holds it), so no trash move can land a fresh file at - * this row's path mid-decision. */ +/** Why the sweep kept a trashed file and dropped its row: + * - `unrecorded` — the row was recorded before identities were kept. + * - `replaced` — the file at the path is a different file, or was rewritten. + * - `changed` — the file's change time runs past the change-time allowance, + * so it was renamed or had its attributes written after the server trashed + * it. */ +type MismatchReason = "unrecorded" | "replaced" | "changed" + +type RecordedFileCheck = "matches" | MismatchReason | "missing" | "unreadable" + +/** Decides whether a trashed file is still the file its row recorded and has + * not changed since, or names the reason it is not. */ +const compareWithRecordedFile = ( + fileState: TrashFileState, + entry: TrashEntry, +): "matches" | MismatchReason => { + if (!entry.fileIdentity) return "unrecorded" + if (fileState.identity !== entry.fileIdentity) return "replaced" + + // The server's own rename sets the change time just before the row is + // written. The allowance covers: + // - trashedAt rounding down to a whole second, which can put it up to a + // second before the change time + // - on Docker Desktop, the host clock (which stamps the change time) + // running a little ahead of the VM clock (which stamps the row) + // A note restored by hand and trashed again within the allowance still + // matches. + const CHANGE_TIME_ALLOWANCE_SECONDS = 60 + const NANOSECONDS_PER_SECOND = 1_000_000_000n + const changeTimeLimitNs = + BigInt(entry.trashedAt + CHANGE_TIME_ALLOWANCE_SECONDS) * NANOSECONDS_PER_SECOND + + if (fileState.changeTimeNs > changeTimeLimitNs) return "changed" + return "matches" +} + +/** Reads the file now at a trash entry's path and compares it with the row. + * A read failure other than a missing file is logged and reported as + * "unreadable". */ +const getRecordedFileCheck = async ( + params: { fullPath: string; entry: TrashEntry }, + logger: Logger, +): Promise => { + try { + const fileState = await readTrashFileState(params.fullPath) + return compareWithRecordedFile(fileState, params.entry) + } catch (error) { + if (isMissingPathError(error)) return "missing" + logger.warn("failed to read trash entry identity", { + trashPath: params.entry.trashPath, + error: describeError(error), + }) + return "unreadable" + } +} + +/** Re-reads a trash entry's row under the trash lock. A failed read is logged + * and returns null, which both callers treat as "leave this row for the next + * run", so one bad row never aborts the sweep or the purge. */ +const tryGetTrashEntry = ( + params: { trashPath: string; trashEntryStore: TrashEntryStore }, + logger: Logger, +): TrashEntry | null => { + try { + return params.trashEntryStore.getTrashEntry(params.trashPath) + } catch (error) { + logger.warn("failed to read trash entry row", { + trashPath: params.trashPath, + error: describeError(error), + }) + return null + } +} + +/** Drops a trash entry's row and returns whether the drop succeeded. A failed + * drop is logged and leaves the row for the next run, so one bad row never + * aborts the sweep or the purge. */ +const tryDropTrashEntry = ( + params: { trashPath: string; trashEntryStore: TrashEntryStore }, + logger: Logger, +): boolean => { + try { + params.trashEntryStore.deleteTrashEntry(params.trashPath) + return true + } catch (error) { + logger.warn("failed to drop trash entry row", { + trashPath: params.trashPath, + error: describeError(error), + }) + return false + } +} + +/** Re-validates one expired row, checks that it stays inside .trash/ and + * still holds the recorded file, then unlinks the file, drops the row, and + * prunes the folders the unlink emptied. Runs under the shared trash-domain + * lock (the caller holds it), so no trash move can land a fresh file at this + * row's path mid-decision. */ const sweepOneEntry = async ( params: { vaultPath: string @@ -44,15 +152,21 @@ const sweepOneEntry = async ( ): Promise => { const { vaultPath, trashPath, trashEntryStore } = params - // Re-read the row under the lock: the expired list is a snapshot from - // before any lock was taken, and a delete can refresh a listed row (the + // Re-read the row under the lock, because the expired list is a snapshot + // from before any lock was taken, and a delete can refresh a listed row (the // same path re-trashed) before this row's turn — its file is then fresh, // and unlinking it would destroy the copy retention exists to keep. - const currentEntry = trashEntryStore.getTrashEntry(trashPath) + const currentEntry = tryGetTrashEntry({ trashPath, trashEntryStore }, logger) - if (!currentEntry || currentEntry.trashedAt >= params.cutoffEpochSeconds) { - return "skipped" - } + if (!currentEntry) return "skipped" + + // The lookup folds case, so currentEntry can be a case alias that replaced + // the listed row during this sweep. Such a row was recorded after the sweep + // began, so it is not expired and returns here; past this check, + // currentEntry is the listed row and trashPath is its spelling. + const rowStillExpired = currentEntry.trashedAt < params.cutoffEpochSeconds + + if (!rowStillExpired) return "skipped" // Containment gate 1, lexical — the row must resolve inside .trash/. A // prefix check on the raw row text is not enough, because @@ -74,20 +188,22 @@ const sweepOneEntry = async ( // require it inside the real .trash root, because a directory symlink // planted inside .trash/ would redirect a lexically-clean path onto live // notes. The final component itself is never followed (unlink removes a - // symlink, not its target). A missing parent means the file is gone (the - // user emptied the trash) — drop the row. + // symlink, not its target). A parent that cannot be resolved means .trash/ + // was emptied outside the server, so the row is dropped here. That covers + // a parent that is gone or sits under a folder that is now a file. A + // parent that is itself now a file still resolves; the identity read below + // then fails with ENOTDIR, which counts as missing and drops the row there. try { - const realTrashRootOrNull = await realpathOrNull(trashRoot) - const realParentOrNull = await realpathOrNull(dirname(resolvedPath)) + const realTrashRoot = await realpathOrNull(trashRoot) + const realParent = await realpathOrNull(dirname(resolvedPath)) - if (realTrashRootOrNull === null || realParentOrNull === null) { - trashEntryStore.deleteTrashEntry(trashPath) - return "missing" + if (!realTrashRoot || !realParent) { + return tryDropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" } + // Equal when the file sits directly in .trash/, not in a subfolder. const parentInsideTrashRoot = - realParentOrNull === realTrashRootOrNull || - realParentOrNull.startsWith(realTrashRootOrNull + sep) + realParent === realTrashRoot || realParent.startsWith(realTrashRoot + sep) if (!parentInsideTrashRoot) { logger.warn("trash entry parent escapes .trash — skipped", { @@ -96,8 +212,8 @@ const sweepOneEntry = async ( return "skipped" } } catch (error) { - // Non-ENOENT realpath failure (EACCES, EIO) — keep the row so the - // next sweep retries; one bad row never aborts the sweep. + // A realpath failure other than a missing path (EACCES, EIO) keeps the + // row so the next sweep retries; one bad row never aborts the sweep. logger.warn("failed to resolve trash entry path", { trashPath, error: describeError(error), @@ -105,18 +221,47 @@ const sweepOneEntry = async ( return "skipped" } - // ENOENT here means the file vanished after the guards ran — the host - // owns the bind mount and can empty the trash at any moment. Same - // outcome as a missing parent: drop the row. + // Identity and change-time gate — the file at the row's path must be the one + // the server trashed, unchanged since. Emptying .trash/ by hand leaves the + // row behind, and Obsidian can later trash another note under the same name; + // deleting that file would destroy a note the server never trashed. A file + // that fails either check is kept and its row dropped. + const recordedFileCheck = await getRecordedFileCheck( + { fullPath: resolvedPath, entry: currentEntry }, + logger, + ) + + // Every result but "matches" returns here, so the unlink below runs only + // after the file at the path matched its row. + if (recordedFileCheck === "unreadable") return "skipped" + if (recordedFileCheck === "missing") { + return tryDropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" + } + if (recordedFileCheck !== "matches") { + if (!tryDropTrashEntry({ trashPath, trashEntryStore }, logger)) return "skipped" + logger.warn( + "trash entry cannot be matched to the file the server trashed — kept, row dropped", + { trashPath, reason: recordedFileCheck }, + ) + return "unmatched" + } + + // The host owns the bind mount and can change .trash/ at any moment: + // - ENOENT means the file vanished after the gates ran. Same outcome as a + // missing parent: drop the row. + // - A different file swapped in after the identity read would be unlinked. + // The read is the last call before this one, and Node has no atomic + // check-and-unlink to close the gap. try { await unlink(resolvedPath) } catch (error) { if (isErrnoException(error, "ENOENT")) { - trashEntryStore.deleteTrashEntry(trashPath) - return "missing" + return tryDropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" } - // Any other failure (permissions, I/O) keeps the row so the next run - // retries; one bad row never aborts the sweep. + // Any other failure keeps the row so the next run retries; one bad row + // never aborts the sweep. That includes permissions, I/O, and ENOTDIR + // from a parent folder replaced by a file since the identity read, which + // the next sweep's identity read reports as missing. logger.warn("failed to remove expired trash entry", { trashPath, error: describeError(error), @@ -124,28 +269,47 @@ const sweepOneEntry = async ( return "skipped" } - trashEntryStore.deleteTrashEntry(trashPath) - // The trash move mkdir'd the file's folder chain, so a purge can strand - // empty folder skeletons; pruning them (rooted at .trash/, which is never - // removed) keeps the folder's growth bounded along with its files. - // pruneEmptyParents walks up from `path` and stops at `vaultPath` — here - // the pruning root is .trash/, not the vault itself. - await pruneEmptyParents({ vaultPath: trashRoot, path: relative(trashRoot, resolvedPath) }, logger) - return "purged" + // The file is gone whether or not the row drop succeeds; the next sweep + // drops a row left behind, finding the file gone or a different file in + // its place. + tryDropTrashEntry({ trashPath, trashEntryStore }, logger) + + // The trash move created the file's folder chain, so an unlink can strand + // empty folders. + // - pruneEmptyParents walks up from `path` and stops at `vaultPath`, here + // .trash/ rather than the vault root, so .trash/ itself is never removed. + // - It logs a folder it cannot remove, but throws on a path with a hidden + // segment, which only a corrupted or hand-edited row can hold. The catch + // logs that throw, so one bad row never aborts the sweep. + try { + await pruneEmptyParents( + { vaultPath: trashRoot, path: relative(trashRoot, resolvedPath) }, + logger, + ) + } catch (error) { + logger.warn("failed to prune emptied trash folders", { + trashPath, + error: describeError(error), + }) + } + return "unlinked" } -/** Removes every recorded trash entry older than `retentionDays` and drops - * rows whose files are already gone. Each row is processed under the shared - * trash-domain lock (per row, so a long sweep never starves deletes), with - * an in-lock re-read deciding whether the row is still expired. */ +/** Removes every recorded trash entry older than `retentionDays` whose file + * still has its recorded identity and meets the change-time limit, and drops + * the rows of entries whose files are gone or fail either check. Each row is + * processed under the shared trash-domain lock (per row, so a long sweep + * never starves deletes), with an in-lock re-read deciding whether the row + * is still expired. */ const sweepExpiredTrashEntries = async (params: SweepParams, logger: Logger): Promise => { const cutoffEpochSeconds = DateTime.now().minus({ days: params.retentionDays }).toUnixInteger() const expiredEntries = params.trashEntryStore.listExpiredTrashEntries(cutoffEpochSeconds) const rowOutcomes: SweepRowOutcome[] = [] for (const expiredEntry of expiredEntries) { - // withFileLock is the serializing mode — each row queues behind any - // in-flight trash move on the shared key, and vice versa. + // withFileLock queues behind the current holder instead of failing, so + // each row waits for any in-flight trash move on the shared key, and a + // move waits for the row. const rowOutcome = await withFileLock(trashDomainLockKey(params.vaultPath), () => { return sweepOneEntry( { @@ -163,11 +327,15 @@ const sweepExpiredTrashEntries = async (params: SweepParams, logger: Logger): Pr const countOutcome = (outcome: SweepRowOutcome): number => { return rowOutcomes.filter((rowOutcome) => rowOutcome === outcome).length } + + // Skipped rows are `expired` minus the three counts below. Every skip has + // already logged a warning, except a row that changed after the listing. logger.info("trash retention sweep complete", { retentionDays: params.retentionDays, expired: expiredEntries.length, - purged: countOutcome("purged"), + purged: countOutcome("unlinked"), droppedMissing: countOutcome("missing"), + droppedUnmatched: countOutcome("unmatched"), }) } @@ -176,25 +344,26 @@ const sweepExpiredTrashEntries = async (params: SweepParams, logger: Logger): Pr * next one. The timer is unref'd and never holds the process open; a * failed run is logged and the chain continues. */ const startTrashSweepSchedule = (params: SweepParams, logger: Logger): void => { - const runAndReschedule = (): void => { - // .finally() re-arms the chain whether the sweep resolved or failed, - // without making the caller await it. - void sweepExpiredTrashEntries(params, logger) - .catch((error: unknown) => { - logger.error("trash retention sweep failed", { - error: describeError(error), - }) - }) - .finally(() => { - setTimeout(runAndReschedule, SWEEP_INTERVAL_MS).unref() + const runAndReschedule = async (): Promise => { + try { + await sweepExpiredTrashEntries(params, logger) + } catch (error) { + logger.error("trash retention sweep failed", { + error: describeError(error), }) + } finally { + // Re-arms the chain whether the sweep resolved or failed. + setTimeout(() => void runAndReschedule(), SWEEP_INTERVAL_MS).unref() + } } - runAndReschedule() + // Not awaited, so startup never waits on a sweep. + void runAndReschedule() } /** Drops trash_entries rows whose .trash/ file no longer exists — runs once * at boot regardless of TRASH_RETENTION_DAYS, so rows left behind by manual - * .trash/ emptying or retention=none don't accumulate. Never unlinks files. */ + * .trash/ emptying or TRASH_RETENTION_DAYS=none don't accumulate. Never + * unlinks files. */ const purgeOrphanedTrashEntries = async ( params: { vaultPath: string; trashEntryStore: TrashEntryStore }, logger: Logger, @@ -203,25 +372,41 @@ const purgeOrphanedTrashEntries = async ( if (allEntries.length === 0) return - // Sequential async loop — each iteration awaits the lock. + // Rows take the lock one at a time. Queuing them all at once (Promise.all) + // would put every row ahead of a delete that arrives mid-purge; this loop + // lets the delete in after the current row. The count is a let because a + // counter across awaited iterations reads plainer than an async fold. let purgedCount = 0 for (const entry of allEntries) { const wasPurged = await withFileLock(trashDomainLockKey(params.vaultPath), async () => { - // Re-read under the lock: a concurrent trash move can replace the - // row (INSERT OR REPLACE refreshes trashedAt), meaning a new file - // now lives at this path — dropping the row would orphan it. - const currentEntry = params.trashEntryStore.getTrashEntry(entry.trashPath) - const rowWasRefreshed = !currentEntry || currentEntry.trashedAt !== entry.trashedAt + // Re-read under the lock, because a concurrent trash move can replace + // the row with one for a new file at this path or at a case alias of it. + // - Dropping that row would leave the new file unrecorded, so the sweep + // would never delete it. + // - The whole row is compared because two records in one second share + // a trashedAt. + // - A re-read that returns no row counts as changed too. Either the + // row was deleted since the listing, or the read failed and the row + // waits for the next boot. + const currentEntry = tryGetTrashEntry( + { trashPath: entry.trashPath, trashEntryStore: params.trashEntryStore }, + logger, + ) + const rowChanged = + !currentEntry || + currentEntry.trashPath !== entry.trashPath || + currentEntry.trashedAt !== entry.trashedAt || + currentEntry.fileIdentity !== entry.fileIdentity - if (rowWasRefreshed) return false + if (rowChanged) return false // lstat (not stat) so a dangling symlink in .trash/ is still // recognized as present — stat would follow it, get ENOENT, and // drop the row, stranding an unlinkable symlink with no record. try { - const entryExists = await lstatOrNull(resolve(params.vaultPath, entry.trashPath)) + const entryStats = await lstatOrNull(resolve(params.vaultPath, entry.trashPath)) - if (entryExists) return false + if (entryStats) return false } catch (error) { logger.warn("failed to stat trash entry", { trashPath: entry.trashPath, @@ -230,8 +415,10 @@ const purgeOrphanedTrashEntries = async ( return false } - params.trashEntryStore.deleteTrashEntry(entry.trashPath) - return true + return tryDropTrashEntry( + { trashPath: entry.trashPath, trashEntryStore: params.trashEntryStore }, + logger, + ) }) if (wasPurged) purgedCount++ diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index 7f82582ba..5853a52e3 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -1,4 +1,15 @@ -import { writeFile, readdir, mkdir, open, unlink, rename, link, rm, rmdir } from "node:fs/promises" +import { + writeFile, + readdir, + mkdir, + open, + unlink, + rename, + link, + lstat, + rm, + rmdir, +} from "node:fs/promises" import { randomUUID } from "node:crypto" import { join, dirname, relative, resolve, parse, posix, isAbsolute, sep } from "node:path" import picomatch from "picomatch" @@ -122,7 +133,9 @@ export const isProtectedPath = (params: { * * The delete/move that triggered the prune has already succeeded, so a failure * to remove a folder (permissions, a race, a vanished dir) is logged and ends - * the walk rather than thrown — it never fails the tool call. + * the walk rather than thrown — it never fails the tool call. An unsafe `path` + * throws resolveSafePath's absolute/traversal/hidden error before any folder + * is touched. * Returns the number of folders removed. */ export const pruneEmptyParents = async ( @@ -539,16 +552,60 @@ type DeleteNoteResult = { * guard's prefix match. */ const TRASH_COLLISION_ERROR_PREFIX = "cannot move to trash" -/** The one serializing-lock key shared by every trash move and the retention - * sweep's per-row processing (trash-sweeper.ts) — an in-memory key for - * file-write-lock's map, not a lock file on disk. Both sides MUST lock this - * same key: the sweep decides what to unlink from a row it re-reads under - * the lock, and a trash move interleaving with that decision could hand it - * a fresh file at a stale row's path. */ +/** The serializing-lock key that every trash move and each retention-sweep + * row (trash-sweeper.ts) MUST share. + * - It is an in-memory key for file-write-lock's map, not a lock file on disk. + * - The sweep decides what to unlink from a row it re-reads under this lock, + * so a trash move interleaving with that decision could hand it a fresh + * file at a stale row's path. + * - Every other lock is on a note's own `.md` path, so none can share this + * folder key. */ export const trashDomainLockKey = (vaultPath: string): string => { return join(vaultPath, ".trash") } +/** What the retention sweep checks in a trashed file before deleting it. */ +export type TrashFileState = { + /** Inode number, size, and modification time, stored in the file's + * trash_entries row and matched exactly. + * - Changing the format makes every recorded row a mismatch, so the sweep + * drops those rows and never deletes their files. + * - The inode alone is not enough, because ext4 reuses freed inode numbers. + * - The device number is left out because, inside a Docker Desktop bind + * mount, it is a number Docker Desktop's VM assigned when it mounted the + * share, which can change when Docker Desktop restarts, and rows outlive + * restarts. */ + identity: string + /** The inode change time, which the sweep compares with the moment the row + * was recorded. + * - Every attribute write moves it, and so does a rename on most file + * systems (POSIX leaves that optional). + * - It is never stored. After a container renames a file inside a Docker + * Desktop bind mount, the container keeps reporting the file's old change + * time while the host's value moves, so a stored value stops matching + * once the container restarts. */ + changeTimeNs: bigint +} + +/** Reads a trashed file's TrashFileState without following a symlink. The + * trash move and the sweep (trash-sweeper.ts) MUST both read it here, so the + * recorded identity and the one the sweep reads share one format. Throws + * when nothing is at the path. */ +export const readTrashFileState = async (fullPath: string): Promise => { + // A bigint keeps every digit, because a file ID seen through a Windows bind + // mount can exceed 2^53, and a number would round two IDs to one value. + const fileStats = await lstat(fullPath, { bigint: true }) + return { + identity: `${fileStats.ino}:${fileStats.size}:${fileStats.mtimeNs}`, + changeTimeNs: fileStats.ctimeNs, + } +} + +/** Retention-sweep bookkeeping hook for a trash move. It receives the + * vault-relative path the note landed at and the landed file's + * `TrashFileState.identity`. */ +type RecordTrashEntry = (entry: { trashPath: string; fileIdentity: string }) => void + /** Claims a trash destination with an exclusive create — the empty placeholder * appears atomically, and only when nothing occupies the name. Returns false * when the name is occupied by anything: a regular file, a directory, or a @@ -567,70 +624,88 @@ const claimTrashTarget = async (targetPath: string): Promise => { } } -/** Moves a note to `.trash/`, creating parent directories as needed. - * Each candidate name — the original, then `note 1.md` … `note 100.md` — - * is claimed with an exclusive create before the move, so an existing - * trash copy can never be overwritten; a concurrent delete loses the - * claim and takes the next suffix instead. The claim itself is the - * existence check — no stat-based precheck decides whether a name is - * safe. When `recordTrashEntry` is provided, the landed trash path is - * reported to it for retention-sweep bookkeeping; a move that does not - * record reports the path to `clearStaleTrashEntry` instead, so a stale - * row left by an earlier occupant of the name cannot mark the new file - * for sweeping. Returns the vault-relative trash path. */ +/** Moves a note into `.trash/` under the first name it can claim, and returns + * the vault-relative trash path. + * - Each candidate name, the original and then `note 1.md` … `note 100.md`, + * is claimed with an exclusive create before the move. The claim is the + * only existence check, so an existing trash copy is never overwritten, + * and a concurrent delete that loses a claim takes the next name. + * - With `recordTrashEntry`, the landed path and the landed file's identity + * are recorded for the retention sweep. Without it, + * `clearStaleTrashEntry` drops any earlier row at the landed path, so the + * sweep never deletes the new file. A caller that passes neither leaves + * such a row in place. */ const moveNoteToTrash = async ( params: { vaultPath: string relativePath: string fullPath: string - recordTrashEntry?: ((trashRelativePath: string) => void) | undefined + recordTrashEntry?: RecordTrashEntry | undefined clearStaleTrashEntry?: ((trashRelativePath: string) => void) | undefined }, logger: Logger, ): Promise => { - const { dir, name, ext } = parse(params.relativePath) + // The claim loop stops after 100 numbered names, so a .trash/ crowded with + // copies of one name fails the delete with an error the vault owner can act + // on (clear old copies) instead of trying names without end. + const TRASH_NAME_SUFFIX_LIMIT = 100 + const { dir, base, name, ext } = parse(params.relativePath) + const trashFolder = join(".trash", dir) // Obsidian appends " 1", " 2", etc. for name collisions in .trash/. - const candidateRelativePaths = [ - `.trash/${params.relativePath}`, - ...Array.from({ length: 100 }, (_, index) => { - const suffixedFileName = `${name} ${index + 1}${ext}` - return `.trash/${join(dir, suffixedFileName)}` - }), - ] - - await mkdir(join(params.vaultPath, ".trash", dir), { recursive: true }) - - // Serialized with the retention sweep (trashDomainLockKey; withFileLock is - // the serializing mode, so concurrent moves and sweep rows queue rather - // than fail): the sweep re-reads each row and unlinks its file under this - // lock, so a move that ran unlocked could land a fresh file at a stale - // row's path between the sweep's re-read and its unlink — losing the - // just-trashed copy. + const suffixedFileNames = Array.from({ length: TRASH_NAME_SUFFIX_LIMIT }, (_, index) => { + return `${name} ${index + 1}${ext}` + }) + const candidateRelativePaths = [base, ...suffixedFileNames].map((fileName) => { + return join(trashFolder, fileName) + }) + + // withFileLock queues behind the current holder instead of failing, so + // concurrent moves and sweep rows take turns. Unlocked, a move could land a + // fresh file at a stale row's path between the sweep's re-read of that row + // and its unlink, and the sweep would delete the just-trashed copy. return withFileLock(trashDomainLockKey(params.vaultPath), async () => { + // The folder is created under the lock because a sweep row removes the + // .trash/ folders its unlink emptied. A folder created before the lock is + // taken can be removed again before the claim, which would then fail. + await mkdir(join(params.vaultPath, trashFolder), { recursive: true }) + for (const candidateRelativePath of candidateRelativePaths) { const candidateFullPath = join(params.vaultPath, candidateRelativePath) if (!(await claimTrashTarget(candidateFullPath))) continue + try { await rename(params.fullPath, candidateFullPath) } catch (renameError) { // The claim took but the move failed — drop our placeholder so it // doesn't strand a 0-byte file occupying a suffix. A failed cleanup is // logged, not thrown, so the rename failure propagates as the cause. - await rm(candidateFullPath, { force: true }).catch((cleanupError: unknown) => { + try { + await rm(candidateFullPath, { force: true }) + } catch (cleanupError) { logger.warn("failed to remove claim placeholder", { path: candidateRelativePath, error: describeError(cleanupError), }) - }) + } throw renameError } + // The exclusive claim proved nothing occupied the landed path, so any - // existing row for it belongs to an earlier, separately-removed - // occupant. Recording replaces that row; a move that does not record - // must clear it, or the next sweep would read the stale row and unlink - // this fresh file. Both writes are fail-open: the note has already moved, - // so a bookkeeping failure is logged rather than reported as a failed move. + // existing row for it belongs to an earlier occupant that has since + // left .trash/, or, on a case-sensitive mount, to a sibling whose name + // differs only in case (rows are keyed by the case-folded path): + // - A recorded move replaces that row. + // - An unrecorded move ("local", Obsidian's keep-forever trash) must + // clear it. A rename keeps a file's identity, so a note restored from + // .trash/ by hand and trashed again matches its old row. The sweep + // keeps it only when the rename moved its change time more than a + // minute past the old row's trashedAt. That misses a quick restore, + // and every restore on a file system that leaves the change time + // alone on rename, which POSIX permits. + // - Both writes are fail-open because the note has already moved, so a + // failed identity read or row write is logged, not reported as a + // failed move. const tryClearStaleTrashEntry = (): void => { if (!params.clearStaleTrashEntry) return try { @@ -645,7 +720,10 @@ const moveNoteToTrash = async ( if (params.recordTrashEntry) { try { - params.recordTrashEntry(candidateRelativePath) + // The sweep compares against whatever is at the landed path, so the + // identity is read there, after the rename. + const { identity } = await readTrashFileState(candidateFullPath) + params.recordTrashEntry({ trashPath: candidateRelativePath, fileIdentity: identity }) } catch (recordError) { logger.warn("failed to record trash entry", { path: candidateRelativePath, @@ -661,7 +739,7 @@ const moveNoteToTrash = async ( } throw new Error( - `${TRASH_COLLISION_ERROR_PREFIX} "${params.relativePath}" — 100 collisions in .trash/`, + `${TRASH_COLLISION_ERROR_PREFIX} "${params.relativePath}" — ${TRASH_NAME_SUFFIX_LIMIT} collisions in .trash/`, ) }) } @@ -677,12 +755,11 @@ const deleteNote = async ( pruneEmptyFolders: boolean trashOption: TrashOption /** Retention-sweep bookkeeping hook, forwarded to moveNoteToTrash. The - * caller decides which trash options are recorded (and therefore - * swept); omitted moves are kept in .trash/ forever. */ - recordTrashEntry?: ((trashRelativePath: string) => void) | undefined - /** Clears the sweep's row for a landed trash path when the move is not - * recorded, so an unrecorded move can never inherit an earlier occupant's - * retention clock. Forwarded to moveNoteToTrash. */ + * caller decides which trash options are recorded; a move made without + * this hook is not recorded, and the sweep leaves its file in .trash/. */ + recordTrashEntry?: RecordTrashEntry | undefined + /** Drops an earlier occupant's row at the landed trash path when the move + * is not recorded. Forwarded to moveNoteToTrash, which says why. */ clearStaleTrashEntry?: ((trashRelativePath: string) => void) | undefined }, logger: Logger, @@ -714,8 +791,6 @@ const deleteNote = async ( throw new Error(`note not found: "${path}"`) } - // The trash path bypasses resolveSafePath because .trash/ is a hidden - // path the guard rejects. Safe: `path` was already validated above. // Assigned inside the try, read after it — pruning and the completion // log below need the value, so const can't span the catch boundary let trashLocation: string | undefined @@ -724,6 +799,8 @@ const deleteNote = async ( // trash, and Obsidian's own fallback for an unavailable system trash // is .trash/. Only "none" means permanent delete. if (params.trashOption !== "none") { + // The trash path bypasses resolveSafePath because .trash/ is a hidden + // path the guard rejects. Safe: `path` was already validated above. trashLocation = await moveNoteToTrash( { vaultPath: params.vaultPath, @@ -759,9 +836,9 @@ const deleteNote = async ( ? await pruneEmptyParents({ vaultPath: params.vaultPath, path }, logger) : 0 - // trashOption is the only record of whether a trashed note is swept - // later ("system") or kept forever ("local") — the result text and the - // tool_result log say "trashed" for both. + // In the logs, trashOption is the only record of whether a trashed note + // is swept later ("system") or kept forever ("local") — the result text + // and the tool_result log say "trashed" for both. logger.info("deleted note", { path, trashOption: params.trashOption,