From f6f3f3c89ebe5e607b617290a9e8c39765f4580e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:07:37 -0400 Subject: [PATCH 01/38] fix(trash): never sweep a note Obsidian trashed under a recorded name The retention sweep deleted whatever file sat at an expired trash_entries row's path. Emptying .trash/ by hand while the server runs leaves the row behind, so a note Obsidian later trashed under the same name was deleted when the row expired. Each recorded move now stores the landed file's identity (inode number, size and modification time, without the device number, which changes across container restarts on Docker Desktop). The sweep re-reads the identity under the trash lock and unlinks only on a match; a different file is kept and its row dropped. Rows recorded before this change carry no identity and are treated the same way, so files the server trashed before upgrading are no longer cleaned up automatically. Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 19 +- .../mcp-core/tools/vault-crud-tools.ts | 13 +- .../search/__tests__/search-index.test.ts | 99 +++++-- src/vault-mcp/search/search-index.ts | 78 ++++-- .../__tests__/trash-sweeper.test.ts | 255 ++++++++++++++---- .../__tests__/vault-filesystem.test.ts | 124 ++++++++- .../vault-operations/trash-sweeper.ts | 97 +++++-- .../vault-operations/vault-filesystem.ts | 62 ++++- 8 files changed, 602 insertions(+), 145 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 2928b7e39..d240ce189 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1203,7 +1203,9 @@ Docker hardening, and durability seatbelts above. 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 + table, together with the landed file's identity (its inode number, size + and modification time, read by `readTrashFileIdentity`). Recording is + fail-open — a failed identity read or 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 @@ -1216,10 +1218,17 @@ Docker hardening, and durability seatbelts above. 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. + entries older than `TRASH_RETENTION_DAYS`. Each unlink passes three + guards: + 1. The resolved path must sit inside `.trash/`, so a corrupted row + cannot reach live notes. + 2. The parent directory's realpath must sit inside `.trash/`, so a + directory symlink cannot redirect the path onto live notes. + 3. The file at the path must still have the row's recorded identity. + Emptying `.trash/` by hand leaves the row behind, and Obsidian can + later trash a different note under the same name; that file is + kept and the row dropped. A row recorded before identities were + kept is treated the same way. - 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. - **Verify-then-preflight-then-commit move** (`note-mover.ts`): under the 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..ec8f43ba1 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. + // The retention sweep deletes the file at an expired + // trash_entries row's path while it still has the identity the + // row recorded, and a row can outlive its time in .trash/ when a + // note is restored by hand. A move that does not record clears + // the row at its landed path, so the sweep cannot remove such a + // note trashed again, for example a "local" note meant to be + // kept. 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..ec5877d8e 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,63 @@ 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 })) + 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")?.fileIdentity).toBe(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 +8362,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 +8372,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 +8396,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 +8408,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") @@ -8362,7 +8422,10 @@ describe("trash entries (retention-sweep bookkeeping)", () => { it("trash entries survive a vault rebuild", async () => { 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 })) diff --git a/src/vault-mcp/search/search-index.ts b/src/vault-mcp/search/search-index.ts index f4b65145b..c8b1bbbe9 100644 --- a/src/vault-mcp/search/search-index.ts +++ b/src/vault-mcp/search/search-index.ts @@ -462,12 +462,15 @@ export const createSearchIndex = ( -- 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 + -- 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 +626,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 +2629,56 @@ 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, + ) } 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 => { @@ -2752,6 +2771,9 @@ export type SearchIndex = ReturnType export type TrashEntry = { trashPath: string trashedAt: number + /** The recorded file's readTrashFileIdentity; 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..407e19917 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,22 @@ import { describe, it, expect, vi, onTestFinished } from "vitest" -import { chmod, mkdtemp, mkdir, readFile, rm, stat, symlink, writeFile } from "node:fs/promises" +import { + chmod, + mkdtemp, + mkdir, + readFile, + rename, + rm, + stat, + symlink, + writeFile, +} from "node:fs/promises" import { join } from "node:path" import { tmpdir } from "node:os" import { DateTime } from "luxon" import { trashSweeper } from "../trash-sweeper.js" -import { vaultFs } from "../vault-filesystem.js" +import { readTrashFileIdentity, 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,16 +27,36 @@ const createTestVault = async (): Promise => { return vault } +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" + +/** The row the server records for the file now at `trashPath`, which must + * exist. */ +const recordedTrashFileAt = async ( + vault: string, + trashPath: string, +): Promise => { + return { trashPath, fileIdentity: await readTrashFileIdentity(join(vault, trashPath)) } +} + +/** 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 `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, - trashPath: string, + recordedFile: RecordedTrashFile, daysAgo: number, ): void => { vi.useFakeTimers() vi.setSystemTime(DateTime.now().minus({ days: daysAgo }).toMillis()) - index.recordTrashEntry(trashPath) + index.recordTrashEntry(recordedFile) vi.useRealTimers() } @@ -36,8 +66,8 @@ describe("sweepExpiredTrashEntries", () => { 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") + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/old.md"), 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/new.md")) const infoSpy = vi.spyOn(logger, "info") onTestFinished(() => infoSpy.mockRestore()) @@ -56,13 +86,14 @@ describe("sweepExpiredTrashEntries", () => { expired: 1, purged: 1, droppedMissing: 0, + droppedUnmatched: 0, }) }) it("drops the row without throwing when the expired file is already gone", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/emptied.md", 31) + recordEntryDaysAgo(index, recordedAbsentFile(".trash/emptied.md"), 31) await trashSweeper.sweepExpiredTrashEntries( { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, @@ -76,7 +107,7 @@ describe("sweepExpiredTrashEntries", () => { 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) + recordEntryDaysAgo(index, recordedAbsentFile(".trash/vanished-folder/x.md"), 31) await trashSweeper.sweepExpiredTrashEntries( { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, @@ -93,14 +124,17 @@ 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") + recordEntryDaysAgo(index, racedFile, 31) 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, @@ -124,7 +158,9 @@ 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. + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/../../escape.md"), 31) const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) @@ -148,7 +184,7 @@ 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) + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/../Live/x.md"), 31) const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) @@ -172,7 +208,7 @@ 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) + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/linkdir/live.md"), 31) const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) @@ -194,7 +230,7 @@ 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) + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/noaccess/stuck.md"), 31) // Remove all permissions from .trash/ so realpath on the parent fails. await chmod(join(vault, ".trash"), 0o000) onTestFinished(() => chmod(join(vault, ".trash"), 0o755)) @@ -223,7 +259,7 @@ 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) + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/locked/stuck.md"), 31) // Remove write permission from the parent so unlink fails with EACCES. await chmod(lockedDir, 0o555) onTestFinished(() => chmod(lockedDir, 0o755)) @@ -244,15 +280,22 @@ describe("sweepExpiredTrashEntries", () => { }) }) - 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("never unlinks an unrecorded delete that lands at a stale row's path with the recorded identity", async () => { + // A note restored from .trash/ by hand keeps its identity and leaves its + // row behind. A keep-forever "local" delete that lands it back at that + // path clears the row on the way in; otherwise the identity still matches + // and this sweep would unlink the note. const vault = await createTestVault() const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, ".trash/reused.md", 31) await writeFile(join(vault, "reused.md"), "keep forever", "utf8") + // A rename keeps a file's identity, so the note's identity here is the + // one it will have at .trash/reused.md. + const restoredNoteIdentity = await readTrashFileIdentity(join(vault, "reused.md")) + recordEntryDaysAgo( + index, + { trashPath: ".trash/reused.md", fileIdentity: restoredNoteIdentity }, + 31, + ) const deleteResult = await vaultFs.deleteNote( { vaultPath: vault, @@ -276,12 +319,126 @@ describe("sweepExpiredTrashEntries", () => { expect(index.getTrashEntry(".trash/reused.md")).toBeNull() }) + 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: (recordedFile) => recordEntryDaysAgo(index, recordedFile, 31), + }, + 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 trashSweeper.sweepExpiredTrashEntries( + { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, + logger, + ) + + 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).toHaveBeenCalledWith( + "trash entry no longer holds the file the server trashed — kept, row dropped", + { trashPath: ".trash/recycled.md" }, + ) + 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") + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/legacy.md"), 31) + 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 trashSweeper.sweepExpiredTrashEntries( + { vaultPath: vault, retentionDays: 30, trashEntryStore: legacyStore }, + logger, + ) + + 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).toHaveBeenCalledWith( + "trash entry no longer holds the file the server trashed — kept, row dropped", + { trashPath: ".trash/legacy.md" }, + ) + }) + + 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") + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/nosearch/stuck.md"), 31) + // 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 trashSweeper.sweepExpiredTrashEntries( + { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, + logger, + ) + + expect(index.getTrashEntry(".trash/nosearch/stuck.md")?.trashPath).toBe( + ".trash/nosearch/stuck.md", + ) + expect(warnSpy).toHaveBeenCalledWith("failed to read trash entry identity", { + trashPath: ".trash/nosearch/stuck.md", + error: expect.stringMatching(/EACCES/), + }) + await chmod(lockedDir, 0o755) + const stuckContent = await readFile(join(lockedDir, "stuck.md"), "utf8") + expect(stuckContent).toBe("stays") + }) + it("prunes the folder skeleton a purge 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") - recordEntryDaysAgo(index, ".trash/sub/deep/old.md", 31) + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/sub/deep/old.md"), 31) await trashSweeper.sweepExpiredTrashEntries( { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, @@ -300,7 +457,7 @@ describe("sweepExpiredTrashEntries", () => { 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) + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/shared/old.md"), 31) await trashSweeper.sweepExpiredTrashEntries( { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, @@ -319,7 +476,7 @@ 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) + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/recorded.md"), 31) await trashSweeper.sweepExpiredTrashEntries( { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, @@ -337,7 +494,7 @@ 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) + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/startup.md"), 31) trashSweeper.startTrashSweepSchedule( { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, @@ -393,8 +550,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 +592,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 +615,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") + recordEntryDaysAgo(index, recordedAbsentFile(".trash/raced.md"), 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, @@ -486,9 +643,9 @@ describe("purgeOrphanedTrashEntries", () => { 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 +677,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 +690,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()) @@ -556,7 +713,7 @@ describe("purgeOrphanedTrashEntries", () => { 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)) @@ -583,9 +740,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 +759,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..eca5c83d5 100644 --- a/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts @@ -27,6 +27,7 @@ import { atomicWriteFile, atomicWriteFileExclusive, pruneEmptyParents, + readTrashFileIdentity, resolveSafePath, } from "../vault-filesystem.js" import { @@ -1038,6 +1039,79 @@ describe("deleteNote", () => { }) }) +describe("readTrashFileIdentity", () => { + /** A whole-second timestamp, so every file set to it has the same mtimeNs. */ + const FIXED_EPOCH_SECONDS = 1_700_000_000 + + it("stays the same when the file is renamed into .trash/", async () => { + await mkdir(join(vault, ".trash"), { recursive: true }) + await writeFile(join(vault, "moved.md"), "content", "utf8") + const identityBeforeMove = await readTrashFileIdentity(join(vault, "moved.md")) + + await rename(join(vault, "moved.md"), join(vault, ".trash", "moved.md")) + + const identityAfterMove = await readTrashFileIdentity(join(vault, ".trash", "moved.md")) + expect(identityAfterMove).toBe(identityBeforeMove) + }) + + it("differs 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 }) + // Only the inode number tells the two files apart. + expect([secondStats.size, secondStats.mtimeNs]).toEqual([firstStats.size, firstStats.mtimeNs]) + + const firstIdentity = await readTrashFileIdentity(join(vault, "first.md")) + const secondIdentity = await readTrashFileIdentity(join(vault, "second.md")) + + expect(secondIdentity).not.toBe(firstIdentity) + }) + + it("differs 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 originalIdentity = await readTrashFileIdentity(notePath) + + await writeFile(notePath, "bbbb", "utf8") + await utimes(notePath, FIXED_EPOCH_SECONDS + 60, FIXED_EPOCH_SECONDS + 60) + + // Only the modification time changed: same inode, same size. + const rewrittenStats = await lstat(notePath, { bigint: true }) + expect([rewrittenStats.ino, rewrittenStats.size]).toEqual([ + originalStats.ino, + originalStats.size, + ]) + expect(await readTrashFileIdentity(notePath)).not.toBe(originalIdentity) + }) + + it("differs when only the size changes", 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 originalIdentity = await readTrashFileIdentity(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, + ]) + expect(await readTrashFileIdentity(notePath)).not.toBe(originalIdentity) + }) + + it("rejects when nothing is at the path", async () => { + await expect(readTrashFileIdentity(join(vault, "absent.md"))).rejects.toThrow(/ENOENT/) + }) +}) + 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 +1241,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,8 +1264,51 @@ 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 () => { diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index aef7736ca..a4935e244 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -1,16 +1,18 @@ /** 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, and the sweep unlinks a file only + * while it still has the identity its row recorded, so Obsidian's own trash + * entries and hand-placed files are out of reach, even under a name the + * server once used. */ 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 { pruneEmptyParents, readTrashFileIdentity, trashDomainLockKey } from "./vault-filesystem.js" import type { TrashEntryStore } from "../search/search-index.js" import type { Logger } from "../../logger.js" @@ -24,12 +26,36 @@ type SweepParams = { trashEntryStore: TrashEntryStore } -type SweepRowOutcome = "purged" | "missing" | "skipped" +type SweepRowOutcome = "purged" | "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. */ +type IdentityCheck = "matches" | "differs" | "missing" | "unreadable" + +/** Compares the file now at a trash entry's path with the identity recorded + * when the server trashed it. A row without a recorded identity never + * matches. A read failure other than a missing file is logged and reported + * as "unreadable". */ +const checkRecordedFileIdentity = async ( + params: { fullPath: string; trashPath: string; recordedIdentity: string | null }, + logger: Logger, +): Promise => { + try { + const currentIdentity = await readTrashFileIdentity(params.fullPath) + return currentIdentity === params.recordedIdentity ? "matches" : "differs" + } catch (error) { + if (isMissingPathError(error)) return "missing" + logger.warn("failed to read trash entry identity", { + trashPath: params.trashPath, + error: describeError(error), + }) + return "unreadable" + } +} + +/** 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 @@ -105,6 +131,30 @@ const sweepOneEntry = async ( return "skipped" } + // Identity gate — the file at the row's path must be the one the server + // trashed. 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 differs, or a row + // recorded before identities were kept, keeps the file and drops the row, + // since the recorded file can no longer be shown to be there. + const identityCheck = await checkRecordedFileIdentity( + { fullPath: resolvedPath, trashPath, recordedIdentity: currentEntry.fileIdentity }, + logger, + ) + + if (identityCheck === "unreadable") return "skipped" + if (identityCheck === "missing") { + trashEntryStore.deleteTrashEntry(trashPath) + return "missing" + } + if (identityCheck === "differs") { + trashEntryStore.deleteTrashEntry(trashPath) + logger.warn("trash entry no longer holds the file the server trashed — kept, row dropped", { + trashPath, + }) + return "unmatched" + } + // 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. @@ -168,6 +218,7 @@ const sweepExpiredTrashEntries = async (params: SweepParams, logger: Logger): Pr expired: expiredEntries.length, purged: countOutcome("purged"), droppedMissing: countOutcome("missing"), + droppedUnmatched: countOutcome("unmatched"), }) } @@ -176,20 +227,20 @@ 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 @@ -207,8 +258,8 @@ const purgeOrphanedTrashEntries = async ( 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 + // Re-read under the lock, because 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 diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index 7f82582ba..b5aca447c 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" @@ -549,6 +560,25 @@ export const trashDomainLockKey = (vaultPath: string): string => { return join(vaultPath, ".trash") } +/** The identity the retention sweep checks before it deletes a trashed file: + * inode number, size, and modification time, read without following a + * symlink. The trash move records it and the sweep (trash-sweeper.ts) + * compares against it, so both MUST read it here. Throws when nothing is at + * the path. + * - The device number is left out because a Docker Desktop bind mount gets a + * new one each time the container starts, and rows outlive restarts. + * - The inode alone is not enough, because ext4 reuses freed inode numbers. */ +export const readTrashFileIdentity = async (fullPath: string): Promise => { + // bigint keeps every digit: a file ID seen through a Windows bind mount can + // exceed 2^53, where a number would round two different IDs to one value. + const fileStats = await lstat(fullPath, { bigint: true }) + return `${fileStats.ino}:${fileStats.size}:${fileStats.mtimeNs}` +} + +/** Retention-sweep bookkeeping hook for a trash move: the vault-relative + * path the note landed at, and the landed file's readTrashFileIdentity. */ +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 @@ -573,17 +603,18 @@ const claimTrashTarget = async (targetPath: string): Promise => { * 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. */ + * safe. When `recordTrashEntry` is provided, the landed trash path and the + * landed file's identity are 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. */ 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, @@ -627,10 +658,12 @@ const moveNoteToTrash = async ( } // 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. + // occupant. Recording replaces that row. A move that does not record + // must clear it, because a note restored from .trash/ by hand and + // trashed again still has the identity its old row recorded, and the + // sweep would unlink it. Both are fail-open because the note has + // already moved, so a failed identity read or row write is logged + // rather than reported as a failed move. const tryClearStaleTrashEntry = (): void => { if (!params.clearStaleTrashEntry) return try { @@ -645,7 +678,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 fileIdentity = await readTrashFileIdentity(candidateFullPath) + params.recordTrashEntry({ trashPath: candidateRelativePath, fileIdentity }) } catch (recordError) { logger.warn("failed to record trash entry", { path: candidateRelativePath, @@ -679,7 +715,7 @@ const deleteNote = async ( /** 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 + recordTrashEntry?: RecordTrashEntry | 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. */ From 098823aa68fae97ba84915ea38f1019f26703d1c Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:16:10 -0400 Subject: [PATCH 02/38] fix(review): list the sweep's identity gate in SECURITY.md and word its warn for unrecorded rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SECURITY.md still said every sweep removal passes two gates; it now lists the identity check as the third, matching ARCHITECTURE.md. The kept-file warn claimed the entry 'no longer holds the file the server trashed', which is false for a row recorded before identities were kept; it now says the entry cannot be matched. The sweepExpiredTrashEntries docstring now says only matching files are removed. Ship-Check: pr-review · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- SECURITY.md | 14 +++++++++----- .../__tests__/trash-sweeper.test.ts | 4 ++-- src/vault-mcp/vault-operations/trash-sweeper.ts | 12 +++++++----- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index d677d316a..c9f3e4c63 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -68,11 +68,15 @@ mechanism-level detail. 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 + Every removal passes three gates: + - The recorded path must resolve inside `.trash/`, so a corrupted or + hand-edited index row is skipped with a warning instead of followed + to a live note + - The file's parent directory must realpath-resolve inside `.trash/`, + so a directory symlink planted there is skipped the same way + - The file must still have the inode number, size, and modification + time recorded when the server trashed it, so a different file under + a recorded name is kept ### Hidden paths 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 407e19917..074ee7f64 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -353,7 +353,7 @@ describe("sweepExpiredTrashEntries", () => { expect(obsidianCopy).toBe("obsidian trashed this") expect(index.getTrashEntry(".trash/recycled.md")).toBeNull() expect(warnSpy).toHaveBeenCalledWith( - "trash entry no longer holds the file the server trashed — kept, row dropped", + "trash entry cannot be matched to the file the server trashed — kept, row dropped", { trashPath: ".trash/recycled.md" }, ) expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { @@ -397,7 +397,7 @@ describe("sweepExpiredTrashEntries", () => { expect(legacyContent).toBe("recorded long ago") expect(index.getTrashEntry(".trash/legacy.md")).toBeNull() expect(warnSpy).toHaveBeenCalledWith( - "trash entry no longer holds the file the server trashed — kept, row dropped", + "trash entry cannot be matched to the file the server trashed — kept, row dropped", { trashPath: ".trash/legacy.md" }, ) }) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index a4935e244..5275022a3 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -149,9 +149,10 @@ const sweepOneEntry = async ( } if (identityCheck === "differs") { trashEntryStore.deleteTrashEntry(trashPath) - logger.warn("trash entry no longer holds the file the server trashed — kept, row dropped", { - trashPath, - }) + logger.warn( + "trash entry cannot be matched to the file the server trashed — kept, row dropped", + { trashPath }, + ) return "unmatched" } @@ -184,8 +185,9 @@ const sweepOneEntry = async ( return "purged" } -/** Removes every recorded trash entry older than `retentionDays` and drops - * rows whose files are already gone. Each row is processed under the shared +/** Removes every recorded trash entry older than `retentionDays` whose file + * still has its recorded identity, and drops the rows of entries whose files + * are gone or no longer match. 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 => { From b75f7e9b2f9bdbb2a16a9e1e599aaf1edeec00a2 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:19:02 -0400 Subject: [PATCH 03/38] docs(trash): state why the identity leaves out the device number The device number inside a Docker Desktop bind mount is assigned when the VM mounts the share, so it can change when Docker Desktop restarts; it did not change between two fresh containers. Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/vault-filesystem.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index b5aca447c..84dd80115 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -565,12 +565,14 @@ export const trashDomainLockKey = (vaultPath: string): string => { * symlink. The trash move records it and the sweep (trash-sweeper.ts) * compares against it, so both MUST read it here. Throws when nothing is at * the path. - * - The device number is left out because a Docker Desktop bind mount gets a - * new one each time the container starts, and rows outlive restarts. + * - 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. * - The inode alone is not enough, because ext4 reuses freed inode numbers. */ export const readTrashFileIdentity = async (fullPath: string): Promise => { - // bigint keeps every digit: a file ID seen through a Windows bind mount can - // exceed 2^53, where a number would round two different IDs to one value. + // 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 `${fileStats.ino}:${fileStats.size}:${fileStats.mtimeNs}` } From d0f012d500aed714d32045de1fb7db22c1f4da2e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:37:40 -0400 Subject: [PATCH 04/38] fix(trash): create the trash folder under the trash-domain lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A trash move created its .trash/ folder before taking the lock it shares with the retention sweep. A sweep row that unlinked the folder's last expired file could then remove the still-empty folder before the move's claim, so the delete failed with "cannot move to trash". The folder is now created inside the lock. Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/vault-filesystem.test.ts | 41 +++++++++++++++++++ .../vault-operations/vault-filesystem.ts | 7 +++- 2 files changed, 46 insertions(+), 2 deletions(-) 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 eca5c83d5..980956ab9 100644 --- a/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts @@ -29,7 +29,9 @@ import { pruneEmptyParents, readTrashFileIdentity, resolveSafePath, + trashDomainLockKey, } from "../vault-filesystem.js" +import { withFileLock } from "../../../utils/file-write-lock.js" import { OverwriteBlockedError, parseNote, @@ -1730,6 +1732,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/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index 84dd80115..0fda55627 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -631,8 +631,6 @@ const moveNoteToTrash = async ( }), ] - 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 @@ -640,6 +638,11 @@ const moveNoteToTrash = async ( // row's path between the sweep's re-read and its unlink — losing 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, ".trash", dir), { recursive: true }) + for (const candidateRelativePath of candidateRelativePaths) { const candidateFullPath = join(params.vaultPath, candidateRelativePath) From 60a6442bc307967a399d4ba9fb280140b1d14025 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:37:57 -0400 Subject: [PATCH 05/38] style: clarify trash sweep and trash move names, comments, and docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Name the trash move's 100-name bound and build its candidate paths one way; replace a floating .catch with try/catch. - Rename the sweep's "purged" row outcome to "unlinked", and the orphan purge's rowWasRefreshed and entryExists to rowChanged and entryStats; split the sweep's re-read guard into named steps. - Say what a file identity is, why the identity string's format is persisted, why the lock key cannot collide, and why the case-folded key needs a concrete example; give the restore-by-hand rationale one home with pointers. - ARCHITECTURE.md: split the trash claim loop into sub-bullets, mention that unrecorded moves drop stale rows, and use "gates" and "deletes". - SECURITY.md: move the identity gate from Path traversal to Data corruption prevention. Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 32 +++-- SECURITY.md | 9 +- .../mcp-core/tools/vault-crud-tools.ts | 12 +- src/vault-mcp/search/search-index.ts | 20 +-- .../vault-operations/trash-sweeper.ts | 102 ++++++++------ .../vault-operations/vault-filesystem.ts | 124 ++++++++++-------- 6 files changed, 171 insertions(+), 128 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d240ce189..77f42fce6 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1202,24 +1202,30 @@ Docker hardening, and durability seatbelts above. 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, together with the landed file's identity (its inode number, size - and modification time, read by `readTrashFileIdentity`). Recording is - fail-open — a failed identity read or 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. + - **Recording** — moves made under the `system` setting are recorded in + the index DB's `trash_entries` table, together with the landed file's + identity (its inode number, size and modification time, read by + `readTrashFileIdentity`). Recording is fail-open — a failed identity + read or row write logs a warning, the delete still succeeds, and an + unrecorded entry is never swept. + - **Stale rows** — a move that is not recorded (`local`, or a failed + record) drops any earlier row at its landed path, so the sweep never + deletes the newly trashed note. + - **Case-folded key** — the table's primary key is the case-folded path. + On a case-insensitive mount `.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. - **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 + exists on disk (uses lstat, so a dangling symlink's row is kept), so + manual emptying or `TRASH_RETENTION_DAYS=none` never leaves unbounded stale rows. - - **Retention sweep** — runs at startup and daily. Purges recorded - entries older than `TRASH_RETENTION_DAYS`. Each unlink passes three - guards: + - **Retention sweep** — runs at startup and daily. Deletes the files + of recorded entries older than `TRASH_RETENTION_DAYS`. Each unlink + passes three gates: 1. The resolved path must sit inside `.trash/`, so a corrupted row cannot reach live notes. 2. The parent directory's realpath must sit inside `.trash/`, so a diff --git a/SECURITY.md b/SECURITY.md index c9f3e4c63..2483d6667 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -68,15 +68,12 @@ mechanism-level detail. 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 three gates: + Every removal passes two containment gates: - The recorded path must resolve inside `.trash/`, so a corrupted or hand-edited index row is skipped with a warning instead of followed to a live note - The file's parent directory must realpath-resolve inside `.trash/`, so a directory symlink planted there is skipped the same way - - The file must still have the inode number, size, and modification - time recorded when the server trashed it, so a different file under - a recorded name is kept ### Hidden paths @@ -163,6 +160,10 @@ 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 inode number, size, and + modification time recorded when the server trashed it, so a different + note that Obsidian later trashed under a recorded name is kept - 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/src/vault-mcp/mcp-core/tools/vault-crud-tools.ts b/src/vault-mcp/mcp-core/tools/vault-crud-tools.ts index ec8f43ba1..4c774efad 100644 --- a/src/vault-mcp/mcp-core/tools/vault-crud-tools.ts +++ b/src/vault-mcp/mcp-core/tools/vault-crud-tools.ts @@ -1135,13 +1135,11 @@ 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 an expired - // trash_entries row's path while it still has the identity the - // row recorded, and a row can outlive its time in .trash/ when a - // note is restored by hand. A move that does not record clears - // the row at its landed path, so the sweep cannot remove such a - // note trashed again, for example a "local" note meant to be - // kept. A "none" delete lands nothing in .trash/ and never calls it. + // Passed for every option because moveNoteToTrash calls it only + // for a move it does not record ("local", or a "system" move + // whose record failed), to drop an earlier row at the landed + // path; moveNoteToTrash says why that row must go. A "none" + // delete lands nothing in .trash/ and never calls it. clearStaleTrashEntry: search.deleteTrashEntry, }, reqLogger, diff --git a/src/vault-mcp/search/search-index.ts b/src/vault-mcp/search/search-index.ts index c8b1bbbe9..7ebd96f72 100644 --- a/src/vault-mcp/search/search-index.ts +++ b/src/vault-mcp/search/search-index.ts @@ -458,14 +458,16 @@ 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; 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. + -- 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. + -- - 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, @@ -2664,6 +2666,8 @@ export const createSearchIndex = ( ) } + /** 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)) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 5275022a3..378dc773f 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -1,9 +1,9 @@ /** Trash bookkeeping — retention sweep (unlink expired files) and orphan * purge (drop rows whose files are gone). Both operate on trash_entries - * rows and never walk the .trash/ folder, and the sweep unlinks a file only - * while it still has the identity its row recorded, so Obsidian's own trash - * entries and hand-placed files are out of reach, even under a name the - * server once used. */ + * rows and never walk the .trash/ folder. The sweep unlinks a file only + * while it still has the identity its row recorded (inode number, size, and + * modification time), so Obsidian's own trash entries and hand-placed files + * are out of reach, even under a name the server once used. */ import { unlink } from "node:fs/promises" import { dirname, join, relative, resolve, sep } from "node:path" @@ -16,8 +16,8 @@ import { pruneEmptyParents, readTrashFileIdentity, trashDomainLockKey } from "./ import type { 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 = { @@ -26,7 +26,7 @@ type SweepParams = { trashEntryStore: TrashEntryStore } -type SweepRowOutcome = "purged" | "missing" | "unmatched" | "skipped" +type SweepRowOutcome = "unlinked" | "missing" | "unmatched" | "skipped" type IdentityCheck = "matches" | "differs" | "missing" | "unreadable" @@ -40,6 +40,7 @@ const checkRecordedFileIdentity = async ( ): Promise => { try { const currentIdentity = await readTrashFileIdentity(params.fullPath) + // A null recordedIdentity never equals a read identity, so it "differs". return currentIdentity === params.recordedIdentity ? "matches" : "differs" } catch (error) { if (isMissingPathError(error)) return "missing" @@ -70,15 +71,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) - 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 @@ -100,20 +107,21 @@ 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 missing parent (nothing there, or a file + // where a folder should be) means .trash/ was emptied outside the server, + // so the row is dropped. 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) { + if (!realTrashRoot || !realParent) { trashEntryStore.deleteTrashEntry(trashPath) return "missing" } + // 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", { @@ -122,8 +130,9 @@ 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), or a failed + // row delete in the missing-parent branch, 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), @@ -142,6 +151,8 @@ const sweepOneEntry = async ( logger, ) + // Every result but "matches" returns here, so only the file the server + // trashed reaches the unlink below. if (identityCheck === "unreadable") return "skipped" if (identityCheck === "missing") { trashEntryStore.deleteTrashEntry(trashPath) @@ -156,7 +167,7 @@ const sweepOneEntry = async ( return "unmatched" } - // ENOENT here means the file vanished after the guards ran — the host + // ENOENT here means the file vanished after the gates ran — the host // owns the bind mount and can empty the trash at any moment. Same // outcome as a missing parent: drop the row. try { @@ -166,8 +177,10 @@ const sweepOneEntry = async ( trashEntryStore.deleteTrashEntry(trashPath) return "missing" } - // 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), @@ -176,13 +189,13 @@ const sweepOneEntry = async ( } 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. + + // 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 instead of throwing. await pruneEmptyParents({ vaultPath: trashRoot, path: relative(trashRoot, resolvedPath) }, logger) - return "purged" + return "unlinked" } /** Removes every recorded trash entry older than `retentionDays` whose file @@ -196,8 +209,9 @@ const sweepExpiredTrashEntries = async (params: SweepParams, logger: Logger): Pr 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( { @@ -215,10 +229,13 @@ 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"), }) @@ -247,7 +264,8 @@ const startTrashSweepSchedule = (params: SweepParams, logger: Logger): void => { /** 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, @@ -256,25 +274,29 @@ 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, because 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. + // now lives at this path. Dropping that row would leave the new file + // unrecorded, so the sweep would never delete it. const currentEntry = params.trashEntryStore.getTrashEntry(entry.trashPath) - const rowWasRefreshed = !currentEntry || currentEntry.trashedAt !== entry.trashedAt + const rowChanged = !currentEntry || currentEntry.trashedAt !== entry.trashedAt - 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, diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index 0fda55627..80e1068f7 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -550,12 +550,14 @@ 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") } @@ -565,6 +567,9 @@ export const trashDomainLockKey = (vaultPath: string): string => { * symlink. The trash move records it and the sweep (trash-sweeper.ts) * compares against it, so both MUST read it here. Throws when nothing is at * the path. + * - The string is stored in trash_entries rows. Changing its format makes + * every recorded row a mismatch, so the sweep drops those rows and never + * deletes their files. * - 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 @@ -577,8 +582,9 @@ export const readTrashFileIdentity = async (fullPath: string): Promise = return `${fileStats.ino}:${fileStats.size}:${fileStats.mtimeNs}` } -/** Retention-sweep bookkeeping hook for a trash move: the vault-relative - * path the note landed at, and the landed file's readTrashFileIdentity. */ +/** Retention-sweep bookkeeping hook for a trash move. It receives the + * vault-relative path the note landed at and the landed file's + * readTrashFileIdentity. */ type RecordTrashEntry = (entry: { trashPath: string; fileIdentity: string }) => void /** Claims a trash destination with an exclusive create — the empty placeholder @@ -599,18 +605,17 @@ 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 and the - * landed file's identity are 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 @@ -621,54 +626,63 @@ const moveNoteToTrash = async ( }, 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)}` - }), - ] - - // 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, ".trash", dir), { recursive: true }) + 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, because a note restored from .trash/ by hand and - // trashed again still has the identity its old row recorded, and the - // sweep would unlink it. Both are fail-open because the note has - // already moved, so a failed identity read or row write is logged - // rather than reported as a failed move. + // existing row for it belongs to an earlier occupant that has since + // left .trash/: + // - A recorded move replaces that row. + // - An unrecorded move ("local", Obsidian's keep-forever trash) must + // clear it. A rename keeps a file's inode, size, and mtime, so a note + // restored from .trash/ by hand and trashed again still matches its + // old row, and the sweep would unlink it. + // - 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 { @@ -702,7 +716,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/`, ) }) } @@ -718,12 +732,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. */ + * 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 - /** 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. */ + /** 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, @@ -755,8 +768,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 @@ -765,6 +776,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, @@ -800,9 +813,8 @@ 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. + // trashOption is logged because the result text and the tool_result log + // say "trashed" for both "local" and "system". logger.info("deleted note", { path, trashOption: params.trashOption, From d21cd455e2105159bc53cf79340a0e413bfe87da Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:41:43 -0400 Subject: [PATCH 06/38] fix(trash): also match the inode change time before sweeping a file A note the server trashed, restored out of .trash/ by hand, and trashed again keeps its inode, size and modification time, so the sweep still deleted it when the original row expired. Every rename moves the inode change time, so the identity now includes it. Attribute writes move it too; the sweep then keeps that file past retention and logs a warning, which is the safe failure. Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 16 ++-- SECURITY.md | 8 +- .../__tests__/trash-sweeper.test.ts | 65 +++++++++++---- .../__tests__/vault-filesystem.test.ts | 82 +++++++++++++++++-- .../vault-operations/trash-sweeper.ts | 7 +- .../vault-operations/vault-filesystem.ts | 27 +++--- 6 files changed, 160 insertions(+), 45 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 77f42fce6..1add31d02 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1204,8 +1204,8 @@ Docker hardening, and durability seatbelts above. exclusive create, so an existing trash copy is never overwritten. - **Recording** — moves made under the `system` setting are recorded in the index DB's `trash_entries` table, together with the landed file's - identity (its inode number, size and modification time, read by - `readTrashFileIdentity`). Recording is fail-open — a failed identity + identity (its inode number, size, modification time and inode change + time, read by `readTrashFileIdentity`). Recording is fail-open — a failed identity read or row write logs a warning, the delete still succeeds, and an unrecorded entry is never swept. - **Stale rows** — a move that is not recorded (`local`, or a failed @@ -1231,10 +1231,14 @@ Docker hardening, and durability seatbelts above. 2. The parent directory's realpath must sit inside `.trash/`, so a directory symlink cannot redirect the path onto live notes. 3. The file at the path must still have the row's recorded identity. - Emptying `.trash/` by hand leaves the row behind, and Obsidian can - later trash a different note under the same name; that file is - kept and the row dropped. A row recorded before identities were - kept is treated the same way. + Otherwise the file is kept, the row dropped, and a warning logged. + This keeps: + - a different note that Obsidian trashed under the same name after + `.trash/` was emptied by hand, which leaves the row behind + - a trashed note restored by hand and trashed again, because each + rename moves its inode change time + - a file whose attributes changed while it sat in `.trash/` + - the file of a row recorded before identities were kept - 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. - **Verify-then-preflight-then-commit move** (`note-mover.ts`): under the diff --git a/SECURITY.md b/SECURITY.md index 2483d6667..aa539b5cb 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -161,9 +161,11 @@ mechanism-level detail. 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 inode number, size, and - modification time recorded when the server trashed it, so a different - note that Obsidian later trashed under a recorded name is kept + `.trash/` file only while it still has the inode number, size, + modification time, and inode change time recorded when the server + trashed it, so a different note that Obsidian later trashed under a + recorded name is kept, and so is a trashed note that was restored and + trashed again - 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/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts index 074ee7f64..6c1775133 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -31,7 +31,14 @@ 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" +const ABSENT_FILE_IDENTITY = "0: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. */ @@ -280,22 +287,14 @@ describe("sweepExpiredTrashEntries", () => { }) }) - it("never unlinks an unrecorded delete that lands at a stale row's path with the recorded identity", async () => { - // A note restored from .trash/ by hand keeps its identity and leaves its - // row behind. A keep-forever "local" delete that lands it back at that - // path clears the row on the way in; otherwise the identity still matches - // and this sweep would unlink the note. + 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, recordedAbsentFile(".trash/reused.md"), 31) await writeFile(join(vault, "reused.md"), "keep forever", "utf8") - // A rename keeps a file's identity, so the note's identity here is the - // one it will have at .trash/reused.md. - const restoredNoteIdentity = await readTrashFileIdentity(join(vault, "reused.md")) - recordEntryDaysAgo( - index, - { trashPath: ".trash/reused.md", fileIdentity: restoredNoteIdentity }, - 31, - ) const deleteResult = await vaultFs.deleteNote( { vaultPath: vault, @@ -308,6 +307,7 @@ 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 }, @@ -319,6 +319,43 @@ describe("sweepExpiredTrashEntries", () => { expect(index.getTrashEntry(".trash/reused.md")).toBeNull() }) + it("keeps a server-trashed note that was restored by hand and trashed again", async () => { + 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: (recordedFile) => recordEntryDaysAgo(index, recordedFile, 31), + }, + logger, + ) + await waitPastTimestampTick() + // Restored by hand, then trashed again by Obsidian. Both renames keep + // the inode, size and modification time the row recorded. + 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 trashSweeper.sweepExpiredTrashEntries( + { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, + logger, + ) + + const retrashedContent = await readFile(join(vault, ".trash", "restored.md"), "utf8") + expect(retrashedContent).toBe("deleted twice") + expect(index.getTrashEntry(".trash/restored.md")).toBeNull() + expect(warnSpy).toHaveBeenCalledWith( + "trash entry cannot be matched to the file the server trashed — kept, row dropped", + { trashPath: ".trash/restored.md" }, + ) + }) + 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:") 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 980956ab9..c8a7fc27a 100644 --- a/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts @@ -1045,15 +1045,79 @@ describe("readTrashFileIdentity", () => { /** A whole-second timestamp, so every file set to it has the same mtimeNs. */ const FIXED_EPOCH_SECONDS = 1_700_000_000 - it("stays the same when the file is renamed into .trash/", async () => { + /** 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("joins 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 string. + expect(linkStats.ino).not.toBe(targetStats.ino) + + const linkIdentity = await readTrashFileIdentity(join(vault, "link.md")) + + expect(linkIdentity).toBe( + `${linkStats.ino}:${linkStats.size}:${linkStats.mtimeNs}:${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 identityBeforeRead = await readTrashFileIdentity(notePath) + await waitPastTimestampTick() + + await readFile(notePath, "utf8") + + expect(await readTrashFileIdentity(notePath)).toBe(identityBeforeRead) + }) + + it("differs after 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, "moved.md"), "content", "utf8") - const identityBeforeMove = await readTrashFileIdentity(join(vault, "moved.md")) + await writeFile(join(vault, ".trash", "restored.md"), "content", "utf8") + const trashedStats = await lstat(join(vault, ".trash", "restored.md"), { bigint: true }) + const identityWhenTrashed = await readTrashFileIdentity(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 retrashedStats = await lstat(join(vault, ".trash", "restored.md"), { bigint: true }) + expect([retrashedStats.ino, retrashedStats.size, retrashedStats.mtimeNs]).toEqual([ + trashedStats.ino, + trashedStats.size, + trashedStats.mtimeNs, + ]) + expect(await readTrashFileIdentity(join(vault, ".trash", "restored.md"))).not.toBe( + identityWhenTrashed, + ) + }) + + it("differs after only the file's attributes change", async () => { + const notePath = join(vault, "chmodded.md") + await writeFile(notePath, "content", "utf8") + const originalStats = await lstat(notePath, { bigint: true }) + const originalIdentity = await readTrashFileIdentity(notePath) + await waitPastTimestampTick() - await rename(join(vault, "moved.md"), join(vault, ".trash", "moved.md")) + await chmod(notePath, 0o600) - const identityAfterMove = await readTrashFileIdentity(join(vault, ".trash", "moved.md")) - expect(identityAfterMove).toBe(identityBeforeMove) + const chmoddedStats = await lstat(notePath, { bigint: true }) + expect([chmoddedStats.ino, chmoddedStats.size, chmoddedStats.mtimeNs]).toEqual([ + originalStats.ino, + originalStats.size, + originalStats.mtimeNs, + ]) + expect(await readTrashFileIdentity(notePath)).not.toBe(originalIdentity) }) it("differs for another file with the same size and modification time", async () => { @@ -1082,7 +1146,7 @@ describe("readTrashFileIdentity", () => { await writeFile(notePath, "bbbb", "utf8") await utimes(notePath, FIXED_EPOCH_SECONDS + 60, FIXED_EPOCH_SECONDS + 60) - // Only the modification time changed: same inode, same size. + // 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, @@ -1091,7 +1155,7 @@ describe("readTrashFileIdentity", () => { expect(await readTrashFileIdentity(notePath)).not.toBe(originalIdentity) }) - it("differs when only the size changes", async () => { + it("differs 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) @@ -1270,7 +1334,7 @@ describe("deleteNote — trash behavior", () => { expect(recordTrashEntry).toHaveBeenCalledTimes(1) expect(recordTrashEntry).toHaveBeenCalledWith({ trashPath: ".trash/rec 1.md", - fileIdentity: `${landedStats.ino}:${landedStats.size}:${landedStats.mtimeNs}`, + fileIdentity: `${landedStats.ino}:${landedStats.size}:${landedStats.mtimeNs}:${landedStats.ctimeNs}`, }) }) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 378dc773f..5f6b9ead3 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -1,9 +1,10 @@ /** Trash bookkeeping — retention sweep (unlink expired files) and orphan * purge (drop rows whose files are gone). Both operate on trash_entries * rows and never walk the .trash/ folder. The sweep unlinks a file only - * while it still has the identity its row recorded (inode number, size, and - * modification time), so Obsidian's own trash entries and hand-placed files - * are out of reach, even under a name the server once used. */ + * while it still has the identity its row recorded (inode number, size, + * modification time, and inode change time), so Obsidian's own trash entries + * and hand-placed files are out of reach, even under a name the server once + * used. */ import { unlink } from "node:fs/promises" import { dirname, join, relative, resolve, sep } from "node:path" diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index 80e1068f7..f301ae28c 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -563,23 +563,28 @@ export const trashDomainLockKey = (vaultPath: string): string => { } /** The identity the retention sweep checks before it deletes a trashed file: - * inode number, size, and modification time, read without following a - * symlink. The trash move records it and the sweep (trash-sweeper.ts) - * compares against it, so both MUST read it here. Throws when nothing is at - * the path. + * inode number, size, modification time, and inode change time, read + * without following a symlink. The trash move records it and the sweep + * (trash-sweeper.ts) compares against it, so both MUST read it here. Each + * field can only stop a deletion, so a field that changes for an unrelated + * reason keeps a file past retention and never deletes one. Throws when + * nothing is at the path. * - The string is stored in trash_entries rows. Changing its 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 inode change time moves on every rename, so a note restored from + * .trash/ by hand and trashed again no longer matches. Attribute writes, + * such as a Finder tag, a sync tool's metadata, or chmod, move it too. * - 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. - * - The inode alone is not enough, because ext4 reuses freed inode numbers. */ + * restarts. */ export const readTrashFileIdentity = 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 `${fileStats.ino}:${fileStats.size}:${fileStats.mtimeNs}` + return `${fileStats.ino}:${fileStats.size}:${fileStats.mtimeNs}:${fileStats.ctimeNs}` } /** Retention-sweep bookkeeping hook for a trash move. It receives the @@ -677,9 +682,11 @@ const moveNoteToTrash = async ( // left .trash/: // - A recorded move replaces that row. // - An unrecorded move ("local", Obsidian's keep-forever trash) must - // clear it. A rename keeps a file's inode, size, and mtime, so a note - // restored from .trash/ by hand and trashed again still matches its - // old row, and the sweep would unlink it. + // clear it. The sweep already keeps a file that was renamed since its + // row was recorded, but only on a file system that updates the inode + // change time on rename, which POSIX leaves optional. There, a note + // restored from .trash/ by hand and trashed again would still match + // its old row, and the sweep would unlink it. // - 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. From 7bd21af7ef56d6807e1e1384d474a2176f57f521 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:56:12 -0400 Subject: [PATCH 07/38] test(trash): cover the sweep's remaining missing-file paths and pin each identity field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverage the identity gate left without a test: - A file that vanishes between the identity check and the unlink (the unlink ENOENT branch) drops its row and counts as missing. - A file standing where an entry's folder was (ENOTDIR on the identity read) drops the row without a warning. - A listed row cleared by a concurrent delete before its turn is skipped and its file kept. Test quality: - The inode, modification-time, and size identity tests compare the identity without its change time, so each fails when its own field is dropped; before, the change time alone made all three pass. - The already-gone and refreshed-row sweep tests assert the sweep's outcome counts, and the refreshed-row test asserts the in-lock re-read. - EACCES warnings and the missing-file rejection assert the exact message, path, and syscall. - The migration test asserts the whole new row under a fixed clock. - Sweep test titles say "unlink", matching the sweep's outcome name. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../search/__tests__/search-index.test.ts | 12 +- .../__tests__/trash-sweeper.test.ts | 161 +++++++++++++++++- .../__tests__/vault-filesystem.test.ts | 50 ++++-- 3 files changed, 201 insertions(+), 22 deletions(-) diff --git a/src/vault-mcp/search/__tests__/search-index.test.ts b/src/vault-mcp/search/__tests__/search-index.test.ts index ec5877d8e..bd7ab1136 100644 --- a/src/vault-mcp/search/__tests__/search-index.test.ts +++ b/src/vault-mcp/search/__tests__/search-index.test.ts @@ -8322,6 +8322,12 @@ describe("trash entries (retention-sweep bookkeeping)", () => { 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) @@ -8341,7 +8347,11 @@ describe("trash entries (retention-sweep bookkeeping)", () => { trashedAt: 1700000000, fileIdentity: null, }) - expect(warmIndex.getTrashEntry(".trash/new.md")?.fileIdentity).toBe(SAMPLE_FILE_IDENTITY) + 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", () => { 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 6c1775133..6e69787ba 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -8,11 +8,16 @@ import { 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 { readTrashFileIdentity, vaultFs } from "../vault-filesystem.js" import { createSearchIndex } from "../../search/search-index.js" @@ -68,7 +73,7 @@ const recordEntryDaysAgo = ( } 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") @@ -97,10 +102,14 @@ describe("sweepExpiredTrashEntries", () => { }) }) - 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, recordedAbsentFile(".trash/emptied.md"), 31) + 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 }, @@ -108,6 +117,14 @@ describe("sweepExpiredTrashEntries", () => { ) 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 () => { @@ -124,6 +141,79 @@ describe("sweepExpiredTrashEntries", () => { 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") + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/replaced/x.md"), 31) + // 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 trashSweeper.sweepExpiredTrashEntries( + { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, + logger, + ) + + 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") + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/vanishing.md"), 31) + // 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 trashSweeper.sweepExpiredTrashEntries( + { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, + logger, + ) + + 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 @@ -135,6 +225,7 @@ describe("sweepExpiredTrashEntries", () => { // keep this file. const racedFile = await recordedTrashFileAt(vault, ".trash/raced.md") recordEntryDaysAgo(index, racedFile, 31) + const getTrashEntry = vi.fn(index.getTrashEntry) const racingStore: TrashEntryStore = { listAllTrashEntries: index.listAllTrashEntries, listExpiredTrashEntries: (cutoffEpochSeconds: number) => { @@ -144,9 +235,11 @@ describe("sweepExpiredTrashEntries", () => { 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 }, @@ -156,6 +249,56 @@ describe("sweepExpiredTrashEntries", () => { 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") + recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/cleared.md"), 31) + 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 trashSweeper.sweepExpiredTrashEntries( + { vaultPath: vault, retentionDays: 30, trashEntryStore: racingStore }, + logger, + ) + + 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 () => { @@ -254,7 +397,7 @@ describe("sweepExpiredTrashEntries", () => { ) 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}'`, }) }) @@ -283,7 +426,7 @@ describe("sweepExpiredTrashEntries", () => { expect(content).toBe("perm error") 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")}'`, }) }) @@ -463,14 +606,14 @@ describe("sweepExpiredTrashEntries", () => { ) expect(warnSpy).toHaveBeenCalledWith("failed to read trash entry identity", { trashPath: ".trash/nosearch/stuck.md", - error: expect.stringMatching(/EACCES/), + 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 folder skeleton a purge empties, keeping .trash/ itself", async () => { + 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 }) @@ -488,7 +631,7 @@ 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 }) @@ -765,7 +908,7 @@ describe("purgeOrphanedTrashEntries", () => { expect(index.getTrashEntry(".trash/locked/stuck.md")?.trashPath).toBe(".trash/locked/stuck.md") 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")}'`, }) }) 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 c8a7fc27a..f7838e02b 100644 --- a/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts @@ -1052,6 +1052,14 @@ describe("readTrashFileIdentity", () => { await new Promise((resolve) => setTimeout(resolve, 25)) } + /** The identity minus its last field, the change time (the first test below + * pins the field order). A write or a utimes call also moves the change + * time, so comparing what is left shows the field a test names is enough + * on its own to tell two files apart. */ + const identityWithoutChangeTime = (identity: string): string => { + return identity.slice(0, identity.lastIndexOf(":")) + } + it("joins 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")) @@ -1120,23 +1128,26 @@ describe("readTrashFileIdentity", () => { expect(await readTrashFileIdentity(notePath)).not.toBe(originalIdentity) }) - it("differs for another file with the same size and modification time", async () => { + 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 }) - // Only the inode number tells the two files apart. + // 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 firstIdentity = await readTrashFileIdentity(join(vault, "first.md")) const secondIdentity = await readTrashFileIdentity(join(vault, "second.md")) - expect(secondIdentity).not.toBe(firstIdentity) + expect(identityWithoutChangeTime(secondIdentity)).not.toBe( + identityWithoutChangeTime(firstIdentity), + ) }) - it("differs after the file is rewritten in place at the same size", async () => { + 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) @@ -1152,10 +1163,13 @@ describe("readTrashFileIdentity", () => { originalStats.ino, originalStats.size, ]) - expect(await readTrashFileIdentity(notePath)).not.toBe(originalIdentity) + const rewrittenIdentity = await readTrashFileIdentity(notePath) + expect(identityWithoutChangeTime(rewrittenIdentity)).not.toBe( + identityWithoutChangeTime(originalIdentity), + ) }) - it("differs after the file grows, even with its modification time restored", async () => { + 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) @@ -1170,11 +1184,23 @@ describe("readTrashFileIdentity", () => { originalStats.ino, originalStats.mtimeNs, ]) - expect(await readTrashFileIdentity(notePath)).not.toBe(originalIdentity) + const resizedIdentity = await readTrashFileIdentity(notePath) + expect(identityWithoutChangeTime(resizedIdentity)).not.toBe( + identityWithoutChangeTime(originalIdentity), + ) }) - it("rejects when nothing is at the path", async () => { - await expect(readTrashFileIdentity(join(vault, "absent.md"))).rejects.toThrow(/ENOENT/) + 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(readTrashFileIdentity(absentPath)).rejects.toMatchObject({ + message: `ENOENT: no such file or directory, lstat '${absentPath}'`, + code: "ENOENT", + syscall: "lstat", + path: absentPath, + }) }) }) @@ -1378,9 +1404,9 @@ describe("deleteNote — trash behavior", () => { }) 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()) From 1ecdad8773775890fccc2736a87f452716c6f8cb Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:02:54 -0400 Subject: [PATCH 08/38] fix(trash): bound the change time by the trash moment instead of storing it Inside a Docker Desktop bind mount the container keeps reporting a file's old change time after its own rename while the host's value moves, so a stored change time stopped matching after any container restart and the sweep kept every file. The identity is back to inode number, size and modification time. The sweep now also keeps a file whose change time runs more than 60 seconds past the moment its row was recorded, which still catches a note restored by hand and trashed again later, and attribute writes after trashing. The warning names why each file was kept. Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 30 +- SECURITY.md | 10 +- src/vault-mcp/search/search-index.ts | 2 +- .../__tests__/trash-sweeper.test.ts | 323 +++++++++++------- .../__tests__/vault-filesystem.test.ts | 89 ++--- .../vault-operations/trash-sweeper.ts | 76 +++-- .../vault-operations/vault-filesystem.ts | 66 ++-- 7 files changed, 341 insertions(+), 255 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1add31d02..94ce72b97 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1204,9 +1204,9 @@ Docker hardening, and durability seatbelts above. exclusive create, so an existing trash copy is never overwritten. - **Recording** — moves made under the `system` setting are recorded in the index DB's `trash_entries` table, together with the landed file's - identity (its inode number, size, modification time and inode change - time, read by `readTrashFileIdentity`). Recording is fail-open — a failed identity - read or row write logs a warning, the delete still succeeds, and an + identity (its inode number, size and modification time, read by + `readTrashFileState`). Recording is fail-open — a failed identity read + or row write logs a warning, the delete still succeeds, and an unrecorded entry is never swept. - **Stale rows** — a move that is not recorded (`local`, or a failed record) drops any earlier row at its landed path, so the sweep never @@ -1225,20 +1225,26 @@ Docker hardening, and durability seatbelts above. unbounded stale rows. - **Retention sweep** — runs at startup and daily. Deletes the files of recorded entries older than `TRASH_RETENTION_DAYS`. Each unlink - passes three gates: + passes four gates: 1. The resolved path must sit inside `.trash/`, so a corrupted row cannot reach live notes. 2. The parent directory's realpath must sit inside `.trash/`, so a directory symlink cannot redirect the path onto live notes. 3. The file at the path must still have the row's recorded identity. - Otherwise the file is kept, the row dropped, and a warning logged. - This keeps: - - a different note that Obsidian trashed under the same name after - `.trash/` was emptied by hand, which leaves the row behind - - a trashed note restored by hand and trashed again, because each - rename moves its inode change time - - a file whose attributes changed while it sat in `.trash/` - - the file of a row recorded before identities were kept + 4. The file's inode change time must be no later than 60 seconds after + the row was recorded. The change time is not stored in the row, + because a Docker Desktop container keeps reporting a file's old + change time after its own rename while the host's value moves. + + A file that fails gate 3 or 4 is kept, its row dropped, and a warning + logged with the reason. This keeps: + - a different note that Obsidian trashed under the same name after + `.trash/` was emptied by hand, which leaves the row behind + - a trashed note restored by hand and trashed again more than a minute + later, because each rename moves its change time + - a file whose attributes changed while it sat in `.trash/` + - the file of a row recorded before identities were kept + - 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. - **Verify-then-preflight-then-commit move** (`note-mover.ts`): under the diff --git a/SECURITY.md b/SECURITY.md index aa539b5cb..deca15ad9 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -161,11 +161,11 @@ mechanism-level detail. 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 inode number, size, - modification time, and inode change time recorded when the server - trashed it, so a different note that Obsidian later trashed under a - recorded name is kept, and so is a trashed note that was restored and - trashed again + `.trash/` file only while it still has the inode number, size, and + modification time recorded when the server trashed it, and its inode + change time is no later than 60 seconds after that. A different note + that Obsidian later trashed under a recorded name is kept, and so is a + trashed note that was restored and trashed again after that minute - 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/src/vault-mcp/search/search-index.ts b/src/vault-mcp/search/search-index.ts index 7ebd96f72..351e2e2fb 100644 --- a/src/vault-mcp/search/search-index.ts +++ b/src/vault-mcp/search/search-index.ts @@ -2775,7 +2775,7 @@ export type SearchIndex = ReturnType export type TrashEntry = { trashPath: string trashedAt: number - /** The recorded file's readTrashFileIdentity; null on a row recorded before + /** 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 } 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 6e69787ba..b6a0ddfc8 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, onTestFinished } from "vitest" import { chmod, + lstat, mkdtemp, mkdir, readFile, @@ -19,7 +20,7 @@ import { DateTime } from "luxon" // 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 { readTrashFileIdentity, vaultFs } from "../vault-filesystem.js" +import { readTrashFileState, vaultFs } from "../vault-filesystem.js" import { createSearchIndex } from "../../search/search-index.js" import type { TrashEntry, TrashEntryStore } from "../../search/search-index.js" import { logger } from "../../../logger.js" @@ -36,7 +37,7 @@ 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:0" +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. @@ -51,7 +52,8 @@ const recordedTrashFileAt = async ( vault: string, trashPath: string, ): Promise => { - return { trashPath, fileIdentity: await readTrashFileIdentity(join(vault, trashPath)) } + const { identity } = await readTrashFileState(join(vault, trashPath)) + return { trashPath, fileIdentity: identity } } /** A row for a trash path with no file behind it. */ @@ -59,34 +61,73 @@ const recordedAbsentFile = (trashPath: string): RecordedTrashFile => { return { trashPath, fileIdentity: ABSENT_FILE_IDENTITY } } -/** 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 = ( +/** Records an entry stamped at `moment` instead of now. */ +const recordEntryAt = ( index: ReturnType, recordedFile: RecordedTrashFile, - daysAgo: number, + moment: DateTime, ): void => { - vi.useFakeTimers() - vi.setSystemTime(DateTime.now().minus({ days: daysAgo }).toMillis()) + vi.useFakeTimers({ toFake: ["Date"] }) + vi.setSystemTime(moment.toMillis()) index.recordTrashEntry(recordedFile) vi.useRealTimers() } +/** The trashedAt the index stored for a row the test has just recorded. */ +const trashedAtOf = (index: ReturnType, trashPath: string): number => { + const recordedEntry = index.getTrashEntry(trashPath) + + if (!recordedEntry) throw new Error(`no trash entry recorded for ${trashPath}`) + return recordedEntry.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" + describe("sweepExpiredTrashEntries", () => { 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, await recordedTrashFileAt(vault, ".trash/old.md"), 31) - index.recordTrashEntry(await recordedTrashFileAt(vault, ".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() @@ -105,16 +146,13 @@ describe("sweepExpiredTrashEntries", () => { 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, recordedAbsentFile(".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() @@ -131,12 +169,9 @@ describe("sweepExpiredTrashEntries", () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") // The recorded subfolder never exists — realpath on the parent ENOENTs. - recordEntryDaysAgo(index, recordedAbsentFile(".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() }) @@ -146,7 +181,7 @@ describe("sweepExpiredTrashEntries", () => { const index = createSearchIndex(":memory:") await mkdir(join(vault, ".trash", "replaced")) await writeFile(join(vault, ".trash", "replaced", "x.md"), "expired", "utf8") - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/replaced/x.md"), 31) + 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 }) @@ -156,10 +191,7 @@ describe("sweepExpiredTrashEntries", () => { 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/replaced/x.md")).toBeNull() const replacementContent = await readFile(join(vault, ".trash", "replaced"), "utf8") @@ -180,7 +212,7 @@ describe("sweepExpiredTrashEntries", () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "vanishing.md"), "expired", "utf8") - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/vanishing.md"), 31) + 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) => { @@ -195,10 +227,7 @@ describe("sweepExpiredTrashEntries", () => { 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", "vanishing.md"))).rejects.toThrow(/ENOENT/) expect(index.getTrashEntry(".trash/vanishing.md")).toBeNull() @@ -224,7 +253,7 @@ describe("sweepExpiredTrashEntries", () => { // 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") - recordEntryDaysAgo(index, racedFile, 31) + index.recordTrashEntry(racedFile) const getTrashEntry = vi.fn(index.getTrashEntry) const racingStore: TrashEntryStore = { listAllTrashEntries: index.listAllTrashEntries, @@ -241,10 +270,7 @@ describe("sweepExpiredTrashEntries", () => { 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") @@ -267,7 +293,7 @@ describe("sweepExpiredTrashEntries", () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "cleared.md"), "keep forever", "utf8") - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/cleared.md"), 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/cleared.md")) const getTrashEntry = vi.fn(index.getTrashEntry) const racingStore: TrashEntryStore = { listAllTrashEntries: index.listAllTrashEntries, @@ -282,10 +308,7 @@ describe("sweepExpiredTrashEntries", () => { const infoSpy = vi.spyOn(logger, "info") onTestFinished(() => infoSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: racingStore }, - logger, - ) + await sweepAfterRetention(vault, racingStore) const clearedContent = await readFile(join(vault, ".trash", "cleared.md"), "utf8") expect(clearedContent).toBe("keep forever") @@ -310,14 +333,11 @@ describe("sweepExpiredTrashEntries", () => { const index = createSearchIndex(":memory:") // The row holds the escape target's real identity, so only the // containment gate can keep it. - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/../../escape.md"), 31) + 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") @@ -334,14 +354,11 @@ 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, await recordedTrashFileAt(vault, ".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") @@ -358,14 +375,11 @@ 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, await recordedTrashFileAt(vault, ".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") @@ -380,17 +394,14 @@ describe("sweepExpiredTrashEntries", () => { await mkdir(lockedDir, { recursive: true }) await writeFile(join(lockedDir, "stuck.md"), "perm error", "utf8") const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".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", @@ -409,17 +420,14 @@ describe("sweepExpiredTrashEntries", () => { await mkdir(lockedDir, { recursive: true }) await writeFile(join(lockedDir, "stuck.md"), "perm error", "utf8") const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".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") @@ -436,7 +444,7 @@ describe("sweepExpiredTrashEntries", () => { // has to tell the new file apart from the one the row recorded. const vault = await createTestVault() const index = createSearchIndex(":memory:") - recordEntryDaysAgo(index, recordedAbsentFile(".trash/reused.md"), 31) + index.recordTrashEntry(recordedAbsentFile(".trash/reused.md")) await writeFile(join(vault, "reused.md"), "keep forever", "utf8") const deleteResult = await vaultFs.deleteNote( { @@ -452,17 +460,94 @@ describe("sweepExpiredTrashEntries", () => { 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("keeps a server-trashed note that was restored by hand and trashed again", async () => { + it("keeps a file whose change time runs more than 60 seconds past its row's moment", 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 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 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).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 moment", 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 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).toHaveBeenCalledWith(KEPT_FILE_WARNING, { + trashPath: ".trash/boundary.md", + reason: "changed", + }) + }) + + 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") @@ -473,30 +558,21 @@ describe("sweepExpiredTrashEntries", () => { protectedPaths: [], pruneEmptyFolders: false, trashOption: "system", - recordTrashEntry: (recordedFile) => recordEntryDaysAgo(index, recordedFile, 31), + recordTrashEntry: index.recordTrashEntry, }, logger, ) await waitPastTimestampTick() - // Restored by hand, then trashed again by Obsidian. Both renames keep - // the inode, size and modification time the row recorded. 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 trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + await sweepAfterRetention(vault, index) - const retrashedContent = await readFile(join(vault, ".trash", "restored.md"), "utf8") - expect(retrashedContent).toBe("deleted twice") + await expect(stat(join(vault, ".trash", "restored.md"))).rejects.toThrow(/ENOENT/) expect(index.getTrashEntry(".trash/restored.md")).toBeNull() - expect(warnSpy).toHaveBeenCalledWith( - "trash entry cannot be matched to the file the server trashed — kept, row dropped", - { trashPath: ".trash/restored.md" }, - ) + expect(warnSpy).not.toHaveBeenCalled() }) it("keeps a note Obsidian trashed under a recycled name after .trash/ was emptied by hand", async () => { @@ -510,7 +586,7 @@ describe("sweepExpiredTrashEntries", () => { protectedPaths: [], pruneEmptyFolders: false, trashOption: "system", - recordTrashEntry: (recordedFile) => recordEntryDaysAgo(index, recordedFile, 31), + recordTrashEntry: index.recordTrashEntry, }, logger, ) @@ -524,18 +600,15 @@ describe("sweepExpiredTrashEntries", () => { const infoSpy = vi.spyOn(logger, "info") onTestFinished(() => infoSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + 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).toHaveBeenCalledWith( - "trash entry cannot be matched to the file the server trashed — kept, row dropped", - { trashPath: ".trash/recycled.md" }, - ) + expect(warnSpy).toHaveBeenCalledWith(KEPT_FILE_WARNING, { + trashPath: ".trash/recycled.md", + reason: "replaced", + }) expect(infoSpy).toHaveBeenCalledWith("trash retention sweep complete", { retentionDays: 30, expired: 1, @@ -552,7 +625,7 @@ describe("sweepExpiredTrashEntries", () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "legacy.md"), "recorded long ago", "utf8") - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/legacy.md"), 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/legacy.md")) const withoutIdentity = (entry: TrashEntry): TrashEntry => ({ ...entry, fileIdentity: null }) const legacyStore: TrashEntryStore = { listAllTrashEntries: index.listAllTrashEntries, @@ -568,18 +641,15 @@ describe("sweepExpiredTrashEntries", () => { const warnSpy = vi.spyOn(logger, "warn") onTestFinished(() => warnSpy.mockRestore()) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: legacyStore }, - logger, - ) + 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).toHaveBeenCalledWith( - "trash entry cannot be matched to the file the server trashed — kept, row dropped", - { trashPath: ".trash/legacy.md" }, - ) + 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 () => { @@ -588,7 +658,7 @@ describe("sweepExpiredTrashEntries", () => { const lockedDir = join(vault, ".trash", "nosearch") await mkdir(lockedDir) await writeFile(join(lockedDir, "stuck.md"), "stays", "utf8") - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".trash/nosearch/stuck.md"), 31) + 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) @@ -596,10 +666,7 @@ describe("sweepExpiredTrashEntries", () => { 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/nosearch/stuck.md")?.trashPath).toBe( ".trash/nosearch/stuck.md", @@ -618,12 +685,9 @@ describe("sweepExpiredTrashEntries", () => { 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, await recordedTrashFileAt(vault, ".trash/sub/deep/old.md"), 31) + index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/sub/deep/old.md")) - await trashSweeper.sweepExpiredTrashEntries( - { vaultPath: vault, retentionDays: 30, trashEntryStore: index }, - logger, - ) + 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/) @@ -637,12 +701,9 @@ describe("sweepExpiredTrashEntries", () => { 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, await recordedTrashFileAt(vault, ".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") @@ -656,12 +717,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, await recordedTrashFileAt(vault, ".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") @@ -674,7 +732,14 @@ describe("startTrashSweepSchedule", () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") await writeFile(join(vault, ".trash", "startup.md"), "expired", "utf8") - recordEntryDaysAgo(index, await recordedTrashFileAt(vault, ".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 }, @@ -795,7 +860,7 @@ 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, recordedAbsentFile(".trash/raced.md"), 1) + 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 = { 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 f7838e02b..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,7 +27,7 @@ import { atomicWriteFile, atomicWriteFileExclusive, pruneEmptyParents, - readTrashFileIdentity, + readTrashFileState, resolveSafePath, trashDomainLockKey, } from "../vault-filesystem.js" @@ -1041,7 +1041,7 @@ describe("deleteNote", () => { }) }) -describe("readTrashFileIdentity", () => { +describe("readTrashFileState", () => { /** A whole-second timestamp, so every file set to it has the same mtimeNs. */ const FIXED_EPOCH_SECONDS = 1_700_000_000 @@ -1052,80 +1052,61 @@ describe("readTrashFileIdentity", () => { await new Promise((resolve) => setTimeout(resolve, 25)) } - /** The identity minus its last field, the change time (the first test below - * pins the field order). A write or a utimes call also moves the change - * time, so comparing what is left shows the field a test names is enough - * on its own to tell two files apart. */ - const identityWithoutChangeTime = (identity: string): string => { - return identity.slice(0, identity.lastIndexOf(":")) - } - - it("joins the link's own inode number, size, modification time and change time", async () => { + 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 string. + // the link would change the expected identity. expect(linkStats.ino).not.toBe(targetStats.ino) - const linkIdentity = await readTrashFileIdentity(join(vault, "link.md")) + const linkState = await readTrashFileState(join(vault, "link.md")) - expect(linkIdentity).toBe( - `${linkStats.ino}:${linkStats.size}:${linkStats.mtimeNs}:${linkStats.ctimeNs}`, - ) + 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 identityBeforeRead = await readTrashFileIdentity(notePath) + const stateBeforeRead = await readTrashFileState(notePath) await waitPastTimestampTick() await readFile(notePath, "utf8") - expect(await readTrashFileIdentity(notePath)).toBe(identityBeforeRead) + expect(await readTrashFileState(notePath)).toEqual(stateBeforeRead) }) - it("differs after the file is renamed out of .trash/ and back", async () => { + 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 trashedStats = await lstat(join(vault, ".trash", "restored.md"), { bigint: true }) - const identityWhenTrashed = await readTrashFileIdentity(join(vault, ".trash", "restored.md")) + 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 retrashedStats = await lstat(join(vault, ".trash", "restored.md"), { bigint: true }) - expect([retrashedStats.ino, retrashedStats.size, retrashedStats.mtimeNs]).toEqual([ - trashedStats.ino, - trashedStats.size, - trashedStats.mtimeNs, - ]) - expect(await readTrashFileIdentity(join(vault, ".trash", "restored.md"))).not.toBe( - identityWhenTrashed, - ) + const stateWhenRetrashed = await readTrashFileState(join(vault, ".trash", "restored.md")) + expect(stateWhenRetrashed.identity).toBe(stateWhenTrashed.identity) + expect(stateWhenRetrashed.changeTimeNs).toBeGreaterThan(stateWhenTrashed.changeTimeNs) }) - it("differs after only the file's attributes change", async () => { + 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 originalStats = await lstat(notePath, { bigint: true }) - const originalIdentity = await readTrashFileIdentity(notePath) + const originalState = await readTrashFileState(notePath) await waitPastTimestampTick() await chmod(notePath, 0o600) - const chmoddedStats = await lstat(notePath, { bigint: true }) - expect([chmoddedStats.ino, chmoddedStats.size, chmoddedStats.mtimeNs]).toEqual([ - originalStats.ino, - originalStats.size, - originalStats.mtimeNs, - ]) - expect(await readTrashFileIdentity(notePath)).not.toBe(originalIdentity) + 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 () => { @@ -1139,12 +1120,10 @@ describe("readTrashFileIdentity", () => { // left to tell the two files apart. expect([secondStats.size, secondStats.mtimeNs]).toEqual([firstStats.size, firstStats.mtimeNs]) - const firstIdentity = await readTrashFileIdentity(join(vault, "first.md")) - const secondIdentity = await readTrashFileIdentity(join(vault, "second.md")) + const firstState = await readTrashFileState(join(vault, "first.md")) + const secondState = await readTrashFileState(join(vault, "second.md")) - expect(identityWithoutChangeTime(secondIdentity)).not.toBe( - identityWithoutChangeTime(firstIdentity), - ) + expect(secondState.identity).not.toBe(firstState.identity) }) it("differs by modification time after the file is rewritten in place at the same size", async () => { @@ -1152,7 +1131,7 @@ describe("readTrashFileIdentity", () => { await writeFile(notePath, "aaaa", "utf8") await utimes(notePath, FIXED_EPOCH_SECONDS, FIXED_EPOCH_SECONDS) const originalStats = await lstat(notePath, { bigint: true }) - const originalIdentity = await readTrashFileIdentity(notePath) + const originalState = await readTrashFileState(notePath) await writeFile(notePath, "bbbb", "utf8") await utimes(notePath, FIXED_EPOCH_SECONDS + 60, FIXED_EPOCH_SECONDS + 60) @@ -1163,10 +1142,8 @@ describe("readTrashFileIdentity", () => { originalStats.ino, originalStats.size, ]) - const rewrittenIdentity = await readTrashFileIdentity(notePath) - expect(identityWithoutChangeTime(rewrittenIdentity)).not.toBe( - identityWithoutChangeTime(originalIdentity), - ) + 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 () => { @@ -1174,7 +1151,7 @@ describe("readTrashFileIdentity", () => { await writeFile(notePath, "short", "utf8") await utimes(notePath, FIXED_EPOCH_SECONDS, FIXED_EPOCH_SECONDS) const originalStats = await lstat(notePath, { bigint: true }) - const originalIdentity = await readTrashFileIdentity(notePath) + const originalState = await readTrashFileState(notePath) await writeFile(notePath, "much longer content", "utf8") await utimes(notePath, FIXED_EPOCH_SECONDS, FIXED_EPOCH_SECONDS) @@ -1184,10 +1161,8 @@ describe("readTrashFileIdentity", () => { originalStats.ino, originalStats.mtimeNs, ]) - const resizedIdentity = await readTrashFileIdentity(notePath) - expect(identityWithoutChangeTime(resizedIdentity)).not.toBe( - identityWithoutChangeTime(originalIdentity), - ) + 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 () => { @@ -1195,7 +1170,7 @@ describe("readTrashFileIdentity", () => { // read. A partial match, because the error also carries a stack trace. const absentPath = join(vault, "absent.md") - await expect(readTrashFileIdentity(absentPath)).rejects.toMatchObject({ + await expect(readTrashFileState(absentPath)).rejects.toMatchObject({ message: `ENOENT: no such file or directory, lstat '${absentPath}'`, code: "ENOENT", syscall: "lstat", @@ -1360,7 +1335,7 @@ describe("deleteNote — trash behavior", () => { expect(recordTrashEntry).toHaveBeenCalledTimes(1) expect(recordTrashEntry).toHaveBeenCalledWith({ trashPath: ".trash/rec 1.md", - fileIdentity: `${landedStats.ino}:${landedStats.size}:${landedStats.mtimeNs}:${landedStats.ctimeNs}`, + fileIdentity: `${landedStats.ino}:${landedStats.size}:${landedStats.mtimeNs}`, }) }) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 5f6b9ead3..c8a9a04b6 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -1,9 +1,10 @@ /** Trash bookkeeping — retention sweep (unlink expired files) and orphan * purge (drop rows whose files are gone). Both operate on trash_entries * rows and never walk the .trash/ folder. The sweep unlinks a file only - * while it still has the identity its row recorded (inode number, size, - * modification time, and inode change time), so Obsidian's own trash entries - * and hand-placed files are out of reach, even under a name the server once + * while it still has the identity its row recorded (inode number, size, and + * modification time) and its change time is no later than about a minute + * after the row was recorded, so Obsidian's own trash entries and + * hand-placed files are out of reach, even under a name the server once * used. */ import { unlink } from "node:fs/promises" @@ -13,8 +14,9 @@ import { describeError } from "../../utils/describe-error.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, readTrashFileIdentity, 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. An expired file can outlive TRASH_RETENTION_DAYS by up to @@ -27,26 +29,55 @@ type SweepParams = { trashEntryStore: TrashEntryStore } +/** How far a trashed file's change time may run past the moment its row was + * recorded before the sweep keeps the file. The server's own rename sets the + * change time just before the row is written; the margin covers the row's + * whole-second rounding and, on Docker Desktop, a 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 this window still + * matches. */ +const CHANGE_TIME_ALLOWANCE_SECONDS = 60 + +const NANOSECONDS_PER_SECOND = 1_000_000_000n + type SweepRowOutcome = "unlinked" | "missing" | "unmatched" | "skipped" -type IdentityCheck = "matches" | "differs" | "missing" | "unreadable" +/** Why a trashed file was kept: its row predates recorded identities, a + * different file now stands at the path, or the file changed status after + * the server trashed it (renamed, or its attributes written). */ +type MismatchReason = "unrecorded" | "replaced" | "changed" + +type IdentityCheck = "matches" | MismatchReason | "missing" | "unreadable" + +/** Decides whether a trashed file is still, unchanged, the one its row + * recorded. */ +const compareWithRecordedFile = ( + fileState: TrashFileState, + entry: TrashEntry, +): "matches" | MismatchReason => { + if (!entry.fileIdentity) return "unrecorded" + if (fileState.identity !== entry.fileIdentity) return "replaced" + + const changeTimeLimitNs = + BigInt(entry.trashedAt + CHANGE_TIME_ALLOWANCE_SECONDS) * NANOSECONDS_PER_SECOND + + if (fileState.changeTimeNs > changeTimeLimitNs) return "changed" + return "matches" +} -/** Compares the file now at a trash entry's path with the identity recorded - * when the server trashed it. A row without a recorded identity never - * matches. A read failure other than a missing file is logged and reported - * as "unreadable". */ +/** 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 checkRecordedFileIdentity = async ( - params: { fullPath: string; trashPath: string; recordedIdentity: string | null }, + params: { fullPath: string; entry: TrashEntry }, logger: Logger, ): Promise => { try { - const currentIdentity = await readTrashFileIdentity(params.fullPath) - // A null recordedIdentity never equals a read identity, so it "differs". - return currentIdentity === params.recordedIdentity ? "matches" : "differs" + return compareWithRecordedFile(await readTrashFileState(params.fullPath), params.entry) } catch (error) { if (isMissingPathError(error)) return "missing" logger.warn("failed to read trash entry identity", { - trashPath: params.trashPath, + trashPath: params.entry.trashPath, error: describeError(error), }) return "unreadable" @@ -142,13 +173,12 @@ const sweepOneEntry = async ( } // Identity gate — the file at the row's path must be the one the server - // trashed. 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 differs, or a row - // recorded before identities were kept, keeps the file and drops the row, - // since the recorded file can no longer be shown to be there. + // 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. Any file that + // cannot be shown to be the recorded one is kept and its row dropped. const identityCheck = await checkRecordedFileIdentity( - { fullPath: resolvedPath, trashPath, recordedIdentity: currentEntry.fileIdentity }, + { fullPath: resolvedPath, entry: currentEntry }, logger, ) @@ -159,11 +189,11 @@ const sweepOneEntry = async ( trashEntryStore.deleteTrashEntry(trashPath) return "missing" } - if (identityCheck === "differs") { + if (identityCheck !== "matches") { trashEntryStore.deleteTrashEntry(trashPath) logger.warn( "trash entry cannot be matched to the file the server trashed — kept, row dropped", - { trashPath }, + { trashPath, reason: identityCheck }, ) return "unmatched" } diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index f301ae28c..7819197c3 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -562,34 +562,43 @@ export const trashDomainLockKey = (vaultPath: string): string => { return join(vaultPath, ".trash") } -/** The identity the retention sweep checks before it deletes a trashed file: - * inode number, size, modification time, and inode change time, read - * without following a symlink. The trash move records it and the sweep - * (trash-sweeper.ts) compares against it, so both MUST read it here. Each - * field can only stop a deletion, so a field that changes for an unrelated - * reason keeps a file past retention and never deletes one. Throws when - * nothing is at the path. - * - The string is stored in trash_entries rows. Changing its 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 inode change time moves on every rename, so a note restored from - * .trash/ by hand and trashed again no longer matches. Attribute writes, - * such as a Finder tag, a sync tool's metadata, or chmod, move it too. - * - 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. */ -export const readTrashFileIdentity = async (fullPath: string): Promise => { +/** 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 every rename and attribute write moves. + * It is never stored, because inside a Docker Desktop bind mount the + * container keeps reporting the old change time after its own rename while + * the host's value moves, so a stored value stops matching after a + * restart. The sweep compares it with the moment the row was recorded. */ + 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 `${fileStats.ino}:${fileStats.size}:${fileStats.mtimeNs}:${fileStats.ctimeNs}` + 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 - * readTrashFileIdentity. */ + * TrashFileState identity. */ type RecordTrashEntry = (entry: { trashPath: string; fileIdentity: string }) => void /** Claims a trash destination with an exclusive create — the empty placeholder @@ -682,11 +691,12 @@ const moveNoteToTrash = async ( // left .trash/: // - A recorded move replaces that row. // - An unrecorded move ("local", Obsidian's keep-forever trash) must - // clear it. The sweep already keeps a file that was renamed since its - // row was recorded, but only on a file system that updates the inode - // change time on rename, which POSIX leaves optional. There, a note - // restored from .trash/ by hand and trashed again would still match - // its old row, and the sweep would unlink it. + // 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 moment, which misses a quick restore and a + // file system that leaves the change time alone on rename (POSIX + // allows both behaviours). // - 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. @@ -706,8 +716,8 @@ const moveNoteToTrash = async ( try { // The sweep compares against whatever is at the landed path, so the // identity is read there, after the rename. - const fileIdentity = await readTrashFileIdentity(candidateFullPath) - params.recordTrashEntry({ trashPath: candidateRelativePath, fileIdentity }) + const { identity } = await readTrashFileState(candidateFullPath) + params.recordTrashEntry({ trashPath: candidateRelativePath, fileIdentity: identity }) } catch (recordError) { logger.warn("failed to record trash entry", { path: candidateRelativePath, From 643dd3fd9b4da4e2e0f27ec00e7e166591c08386 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:20:51 -0400 Subject: [PATCH 09/38] fix(trash): compare the whole row in the orphan purge's re-read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The purge treated a row as unchanged when only its trashedAt matched, so a row replaced within the same second by another file's record, or by a case alias's record, could be dropped and leave that file unrecorded. It now also compares the path spelling and the recorded identity. Two comments are corrected: a rename moves the change time on most file systems, not all, and the "replaced" reason also covers a recorded file that was rewritten. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/trash-sweeper.test.ts | 44 +++++++++++++++++++ .../vault-operations/trash-sweeper.ts | 20 ++++++--- .../vault-operations/vault-filesystem.ts | 13 +++--- 3 files changed, 65 insertions(+), 12 deletions(-) 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 b6a0ddfc8..0724830e0 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -885,6 +885,50 @@ 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:") diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index c8a9a04b6..7dc80b3e7 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -42,9 +42,9 @@ const NANOSECONDS_PER_SECOND = 1_000_000_000n type SweepRowOutcome = "unlinked" | "missing" | "unmatched" | "skipped" -/** Why a trashed file was kept: its row predates recorded identities, a - * different file now stands at the path, or the file changed status after - * the server trashed it (renamed, or its attributes written). */ +/** Why a trashed file was kept: its row predates recorded identities, the + * file at the path is a different one or was rewritten, or the file changed + * status after the server trashed it (renamed, or its attributes written). */ type MismatchReason = "unrecorded" | "replaced" | "changed" type IdentityCheck = "matches" | MismatchReason | "missing" | "unreadable" @@ -313,11 +313,17 @@ const purgeOrphanedTrashEntries = async ( for (const entry of allEntries) { const wasPurged = await withFileLock(trashDomainLockKey(params.vaultPath), async () => { // Re-read under the lock, because a concurrent trash move can replace - // the row (INSERT OR REPLACE refreshes trashedAt), meaning a new file - // now lives at this path. Dropping that row would leave the new file - // unrecorded, so the sweep would never delete it. + // 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. const currentEntry = params.trashEntryStore.getTrashEntry(entry.trashPath) - const rowChanged = !currentEntry || currentEntry.trashedAt !== entry.trashedAt + const rowChanged = + !currentEntry || + currentEntry.trashPath !== entry.trashPath || + currentEntry.trashedAt !== entry.trashedAt || + currentEntry.fileIdentity !== entry.fileIdentity if (rowChanged) return false diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index 7819197c3..c915d1456 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -574,11 +574,14 @@ export type TrashFileState = { * share, which can change when Docker Desktop restarts, and rows outlive * restarts. */ identity: string - /** The inode change time, which every rename and attribute write moves. - * It is never stored, because inside a Docker Desktop bind mount the - * container keeps reporting the old change time after its own rename while - * the host's value moves, so a stored value stops matching after a - * restart. The sweep compares it with the moment the row was recorded. */ + /** 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, because inside a Docker Desktop bind mount the + * container keeps reporting the old change time after its own rename + * while the host's value moves, so a stored value stops matching after + * a restart. */ changeTimeNs: bigint } From 0e08d8693c2062082c57395cb1d78eb323305452 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:22:49 -0400 Subject: [PATCH 10/38] docs(trash): note the case-variant sibling a stale trash row can belong to Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/vault-filesystem.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index c915d1456..40835503c 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -691,7 +691,8 @@ const moveNoteToTrash = async ( // The exclusive claim proved nothing occupied the landed path, so any // existing row for it belongs to an earlier occupant that has since - // left .trash/: + // 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 From b5f9fd9aa4aa17829d59da6b5fe52b76f1500db0 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:24:04 -0400 Subject: [PATCH 11/38] docs(trash): state the change-time limit in the sweep's docstring Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/trash-sweeper.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 7dc80b3e7..ddec13791 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -230,8 +230,9 @@ const sweepOneEntry = async ( } /** Removes every recorded trash entry older than `retentionDays` whose file - * still has its recorded identity, and drops the rows of entries whose files - * are gone or no longer match. Each row is processed under the shared + * 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 => { From f9895e06b54c4b276ae67aa26d7913439837627c Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:31:39 -0400 Subject: [PATCH 12/38] style: clarify the trash sweep's change-time names and comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename the sweep's file check to say it covers the change time as well as the stored identity, move the change-time constants into the function that uses them, list each kept-file reason on its own line, correct the gate comment that claimed an unreadable file's row is dropped, and resolve the Docker Desktop and POSIX referents in comments and ARCHITECTURE.md. Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 7 +- src/vault-mcp/search/search-index.ts | 5 +- .../vault-operations/trash-sweeper.ts | 69 ++++++++++--------- .../vault-operations/vault-filesystem.ts | 16 ++--- 4 files changed, 51 insertions(+), 46 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 94ce72b97..ed98953ce 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1232,9 +1232,10 @@ Docker hardening, and durability seatbelts above. directory symlink cannot redirect the path onto live notes. 3. The file at the path must still have the row's recorded identity. 4. The file's inode change time must be no later than 60 seconds after - the row was recorded. The change time is not stored in the row, - because a Docker Desktop container keeps reporting a file's old - change time after its own rename while the host's value moves. + the row was recorded. The change time is not stored in the row: + after a Docker Desktop container renames a file, it keeps reporting + the file's old change time while the host's value moves, so a stored + value stops matching once the container restarts. A file that fails gate 3 or 4 is kept, its row dropped, and a warning logged with the reason. This keeps: diff --git a/src/vault-mcp/search/search-index.ts b/src/vault-mcp/search/search-index.ts index 351e2e2fb..79572d4c8 100644 --- a/src/vault-mcp/search/search-index.ts +++ b/src/vault-mcp/search/search-index.ts @@ -2774,9 +2774,10 @@ 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. */ + /** 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 } diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index ddec13791..30858e52a 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -29,28 +29,20 @@ type SweepParams = { trashEntryStore: TrashEntryStore } -/** How far a trashed file's change time may run past the moment its row was - * recorded before the sweep keeps the file. The server's own rename sets the - * change time just before the row is written; the margin covers the row's - * whole-second rounding and, on Docker Desktop, a 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 this window still - * matches. */ -const CHANGE_TIME_ALLOWANCE_SECONDS = 60 - -const NANOSECONDS_PER_SECOND = 1_000_000_000n - type SweepRowOutcome = "unlinked" | "missing" | "unmatched" | "skipped" -/** Why a trashed file was kept: its row predates recorded identities, the - * file at the path is a different one or was rewritten, or the file changed - * status after the server trashed it (renamed, or its attributes written). */ +/** 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 IdentityCheck = "matches" | MismatchReason | "missing" | "unreadable" +type RecordedFileCheck = "matches" | MismatchReason | "missing" | "unreadable" -/** Decides whether a trashed file is still, unchanged, the one its row - * recorded. */ +/** 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, @@ -58,6 +50,16 @@ const compareWithRecordedFile = ( 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 @@ -68,12 +70,13 @@ const compareWithRecordedFile = ( /** 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 checkRecordedFileIdentity = async ( +const checkRecordedFile = async ( params: { fullPath: string; entry: TrashEntry }, logger: Logger, -): Promise => { +): Promise => { try { - return compareWithRecordedFile(await readTrashFileState(params.fullPath), params.entry) + 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", { @@ -172,28 +175,28 @@ const sweepOneEntry = async ( return "skipped" } - // Identity 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. Any file that - // cannot be shown to be the recorded one is kept and its row dropped. - const identityCheck = await checkRecordedFileIdentity( + // 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 checkRecordedFile( { fullPath: resolvedPath, entry: currentEntry }, logger, ) // Every result but "matches" returns here, so only the file the server // trashed reaches the unlink below. - if (identityCheck === "unreadable") return "skipped" - if (identityCheck === "missing") { + if (recordedFileCheck === "unreadable") return "skipped" + if (recordedFileCheck === "missing") { trashEntryStore.deleteTrashEntry(trashPath) return "missing" } - if (identityCheck !== "matches") { + if (recordedFileCheck !== "matches") { trashEntryStore.deleteTrashEntry(trashPath) logger.warn( "trash entry cannot be matched to the file the server trashed — kept, row dropped", - { trashPath, reason: identityCheck }, + { trashPath, reason: recordedFileCheck }, ) return "unmatched" } @@ -232,9 +235,9 @@ const sweepOneEntry = async ( /** 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. */ + * 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) diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index 40835503c..e728bc8b7 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -578,10 +578,10 @@ export type TrashFileState = { * was recorded. * - Every attribute write moves it, and so does a rename on most file * systems (POSIX leaves that optional). - * - It is never stored, because inside a Docker Desktop bind mount the - * container keeps reporting the old change time after its own rename - * while the host's value moves, so a stored value stops matching after - * a restart. */ + * - 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 } @@ -601,7 +601,7 @@ export const readTrashFileState = async (fullPath: string): Promise void /** Claims a trash destination with an exclusive create — the empty placeholder @@ -698,9 +698,9 @@ const moveNoteToTrash = async ( // 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 moment, which misses a quick restore and a - // file system that leaves the change time alone on rename (POSIX - // allows both behaviours). + // 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. From f9d45f2c6ad35b38f3dc91b3f44fa153f2596fe0 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 12:38:27 -0400 Subject: [PATCH 13/38] test(trash): tighten the change-time limit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The exact-boundary test recorded its row at the real moment, so the file's real change time already met the limit and the test passed even when its reported change time never reached the identity read. The row is now recorded two minutes back, so only the reported change time can unlink the file. - New test: a change time well before the row's trashedAt still unlinks the file, which pins that the limit has no lower bound. - Test titles name trashedAt instead of "its row's moment". - recordEntryAt restores real timers in a finally block, so a throwing record cannot leave Date frozen for later tests. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/trash-sweeper.test.ts | 45 ++++++++++++++++--- 1 file changed, 39 insertions(+), 6 deletions(-) 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 0724830e0..edf933671 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -68,9 +68,13 @@ const recordEntryAt = ( moment: DateTime, ): void => { vi.useFakeTimers({ toFake: ["Date"] }) - vi.setSystemTime(moment.toMillis()) - index.recordTrashEntry(recordedFile) - vi.useRealTimers() + // 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 trashedAt the index stored for a row the test has just recorded. */ @@ -467,7 +471,7 @@ describe("sweepExpiredTrashEntries", () => { expect(index.getTrashEntry(".trash/reused.md")).toBeNull() }) - it("keeps a file whose change time runs more than 60 seconds past its row's moment", 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. @@ -493,11 +497,17 @@ describe("sweepExpiredTrashEntries", () => { }) }) - it("unlinks a file whose change time is exactly 60 seconds past its row's moment", async () => { + 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") - index.recordTrashEntry(await recordedTrashFileAt(vault, ".trash/boundary.md")) + // 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. @@ -544,6 +554,29 @@ describe("sweepExpiredTrashEntries", () => { }) }) + 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 From 16e431ec5ef6cf389eaf5ed2d8b599426ba3966a Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 13:25:24 -0400 Subject: [PATCH 14/38] fix(trash): keep a failed folder prune from ending the retention sweep A prune that throws on a hidden path segment, which only a corrupted or hand-edited row can hold, now logs a warning and the sweep moves on to the next row. The local deploy guide and .env.example now say that notes trashed by version 0.54.11 or earlier are kept and need emptying by hand. Co-Authored-By: Claude Opus 5.5 --- cli/src/env.ts | 3 +- deploy/local/.env.example | 3 +- deploy/local/README.md | 8 +++-- .../__tests__/trash-sweeper.test.ts | 33 +++++++++++++++++++ .../vault-operations/trash-sweeper.ts | 16 +++++++-- 5 files changed, 56 insertions(+), 7 deletions(-) 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/vault-operations/__tests__/trash-sweeper.test.ts b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts index edf933671..4edd82ed8 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -743,6 +743,39 @@ describe("sweepExpiredTrashEntries", () => { 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 it 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).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("never touches a trash file it has no row for, however old", async () => { const vault = await createTestVault() const index = createSearchIndex(":memory:") diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 30858e52a..9b2d15664 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -227,8 +227,20 @@ const sweepOneEntry = async ( // 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 instead of throwing. - await pruneEmptyParents({ vaultPath: trashRoot, path: relative(trashRoot, resolvedPath) }, logger) + // is never removed. It logs a folder it cannot remove, but it throws on a + // path with a hidden segment, which only a corrupted or hand-edited row can + // hold; the catch keeps such a row from ending the sweep for the rows after it. + 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" } From 9f7603e51001f43ff0954d31675be23aa9ae340a Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 13:31:15 -0400 Subject: [PATCH 15/38] docs(trash): state that pruneEmptyParents throws on an unsafe path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docstring covered only the logged folder-removal failures, so a caller could read it as never throwing. It now names the resolveSafePath error an unsafe path raises before any folder is touched, which the retention sweep's catch relies on. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/vault-filesystem.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index e728bc8b7..cff957b3e 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -133,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 ( From c75b5abbcfb5e3d149e4d5ab4457f420ed4ce8c7 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 13:59:10 -0400 Subject: [PATCH 16/38] docs(trash): state that a re-trash is caught only where renames move the change time Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 11 ++++++----- SECURITY.md | 3 ++- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index ed98953ce..9871fa806 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1232,17 +1232,18 @@ Docker hardening, and durability seatbelts above. directory symlink cannot redirect the path onto live notes. 3. The file at the path must still have the row's recorded identity. 4. The file's inode change time must be no later than 60 seconds after - the row was recorded. The change time is not stored in the row: - after a Docker Desktop container renames a file, it keeps reporting - the file's old change time while the host's value moves, so a stored - value stops matching once the container restarts. + the row was recorded. The change time is not stored in the row, + because after a Docker Desktop container renames a file it keeps + reporting the file's old change time while the host's value moves, + so a stored value would stop matching once the container restarts. A file that fails gate 3 or 4 is kept, its row dropped, and a warning logged with the reason. This keeps: - a different note that Obsidian trashed under the same name after `.trash/` was emptied by hand, which leaves the row behind - a trashed note restored by hand and trashed again more than a minute - later, because each rename moves its change time + later, on a file system whose renames move the change time (ext4 and + APFS do; POSIX leaves it optional) - a file whose attributes changed while it sat in `.trash/` - the file of a row recorded before identities were kept diff --git a/SECURITY.md b/SECURITY.md index deca15ad9..d5ac6ebf2 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -165,7 +165,8 @@ mechanism-level detail. modification time recorded when the server trashed it, and its inode change time is no later than 60 seconds after that. A different note that Obsidian later trashed under a recorded name is kept, and so is a - trashed note that was restored and trashed again after that minute + trashed note that was restored and trashed again after that minute on a + file system whose renames move the change time - 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 From 84e7c9d2724d746188c1007995c36d34ddc3abd9 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:00:08 -0400 Subject: [PATCH 17/38] docs(contributing): drop the unfinished template sync sentence cli/templates/ no longer exists, and the sentence about it stopped mid-clause and ran into the env-block sync rule that follows. Co-Authored-By: Claude Opus 5.5 --- CONTRIBUTING.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6c0bce579..383189065 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -102,7 +102,6 @@ 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 `npm run sync:cli-env-blocks` in the same PR — drift tests fail CI otherwise. From a4f3c07883a5f850f8226e3e16e33eaa742eb7d6 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:31:41 -0400 Subject: [PATCH 18/38] refactor(trash): name the sweep's file check for the verdict it returns Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/trash-sweeper.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 9b2d15664..60cc60928 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -70,7 +70,7 @@ const compareWithRecordedFile = ( /** 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 checkRecordedFile = async ( +const getRecordedFileCheck = async ( params: { fullPath: string; entry: TrashEntry }, logger: Logger, ): Promise => { @@ -180,7 +180,7 @@ const sweepOneEntry = async ( // 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 checkRecordedFile( + const recordedFileCheck = await getRecordedFileCheck( { fullPath: resolvedPath, entry: currentEntry }, logger, ) From 814600af4dc135054b8e782924257192ced7f6b7 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:32:55 -0400 Subject: [PATCH 19/38] docs(trash): state that a case-only sibling loses its row on a case-sensitive mount Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 4 +++- src/vault-mcp/search/search-index.ts | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 9871fa806..2387de8d9 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1215,7 +1215,9 @@ Docker hardening, and durability seatbelts above. On a case-insensitive mount `.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. + file on the old row's date. On a case-sensitive mount the two names are + two files sharing one row, so the file recorded first loses its row and + stays until `.trash/` is emptied by hand. - **Recorded trash bookkeeping** (`trash-sweeper.ts`): two row-driven operations (neither walks the folder): - **Orphan purge** — runs once at boot regardless of diff --git a/src/vault-mcp/search/search-index.ts b/src/vault-mcp/search/search-index.ts index 79572d4c8..51499a7d4 100644 --- a/src/vault-mcp/search/search-index.ts +++ b/src/vault-mcp/search/search-index.ts @@ -463,7 +463,9 @@ export const createSearchIndex = ( -- 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. + -- the old row's date. On a case-sensitive mount the two names are two + -- files sharing one row, 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. From 7d60ac07443712ee731100865af6fb8376707e07 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:57:49 -0400 Subject: [PATCH 20/38] fix(trash): keep a failed row drop from ending the retention sweep A store failure on a row delete (a full disk fails the delete but not the read) now keeps that row for the next sweep and moves on to the next row, the way a failed realpath or unlink already did. The module docstring now names the restore-and-retrash case the change-time limit cannot catch. Co-Authored-By: Claude Opus 5.5 --- .../__tests__/trash-sweeper.test.ts | 148 ++++++++++++++++++ .../vault-operations/trash-sweeper.ts | 43 +++-- 2 files changed, 179 insertions(+), 12 deletions(-) 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 4edd82ed8..373860800 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -115,6 +115,23 @@ const sweepAfterRetention = async ( 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 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("unlinks an expired entry's file and drops its row; a fresh entry and its file survive", async () => { const vault = await createTestVault() @@ -776,6 +793,137 @@ describe("sweepExpiredTrashEntries", () => { }) }) + 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()) + + 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")?.trashPath).toBe(".trash/stuck.md") + 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()) + + await sweepAfterRetention(vault, storeFailingToDrop(index, trashPath)) + + expect(index.getTrashEntry(trashPath)?.trashPath).toBe(trashPath) + 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()) + + await sweepAfterRetention(vault, storeFailingToDrop(index, ".trash/vanishing.md")) + + expect(index.getTrashEntry(".trash/vanishing.md")?.trashPath).toBe(".trash/vanishing.md") + 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()) + + 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")?.trashPath).toBe(".trash/recycled.md") + 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:") diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 60cc60928..6d4aeb8c2 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -5,7 +5,9 @@ * modification time) and its change time is no later than about a minute * after the row was recorded, so Obsidian's own trash entries and * hand-placed files are out of reach, even under a name the server once - * used. */ + * used. The exception is a note the server trashed, then restored by hand + * and trashed again: it still matches its row when that happens within the + * minute, or on a file system whose renames leave the change time alone. */ import { unlink } from "node:fs/promises" import { dirname, join, relative, resolve, sep } from "node:path" @@ -87,6 +89,25 @@ const getRecordedFileCheck = async ( } } +/** Drops a finished row and returns whether it was dropped. A failed drop + * (a full disk fails a delete but not a read) keeps the row for the next + * sweep instead of ending this one, whose later unlinks can free space. */ +const dropTrashEntry = ( + 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 @@ -150,8 +171,7 @@ const sweepOneEntry = async ( const realParent = await realpathOrNull(dirname(resolvedPath)) if (!realTrashRoot || !realParent) { - trashEntryStore.deleteTrashEntry(trashPath) - return "missing" + return dropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" } // Equal when the file sits directly in .trash/, not in a subfolder. @@ -165,9 +185,8 @@ const sweepOneEntry = async ( return "skipped" } } catch (error) { - // A realpath failure other than a missing path (EACCES, EIO), or a failed - // row delete in the missing-parent branch, keeps 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), @@ -189,11 +208,10 @@ const sweepOneEntry = async ( // trashed reaches the unlink below. if (recordedFileCheck === "unreadable") return "skipped" if (recordedFileCheck === "missing") { - trashEntryStore.deleteTrashEntry(trashPath) - return "missing" + return dropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" } if (recordedFileCheck !== "matches") { - trashEntryStore.deleteTrashEntry(trashPath) + if (!dropTrashEntry({ trashPath, trashEntryStore }, logger)) return "skipped" logger.warn( "trash entry cannot be matched to the file the server trashed — kept, row dropped", { trashPath, reason: recordedFileCheck }, @@ -208,8 +226,7 @@ const sweepOneEntry = async ( await unlink(resolvedPath) } catch (error) { if (isErrnoException(error, "ENOENT")) { - trashEntryStore.deleteTrashEntry(trashPath) - return "missing" + return dropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" } // 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 @@ -222,7 +239,9 @@ const sweepOneEntry = async ( return "skipped" } - trashEntryStore.deleteTrashEntry(trashPath) + // The file is gone whether or not the row drop succeeds; a row left behind + // is dropped as missing by the next sweep. + dropTrashEntry({ 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 From 20c40d74a7b19e66b1f9e28d2c1c7e9919c3042d Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:20:10 -0400 Subject: [PATCH 21/38] fix(trash): keep a failed row drop from ending the orphan purge The boot-time purge now drops rows through the same guard as the sweep, so one failed delete keeps that row and the purge moves on. The architecture note's attribute-change bullet now carries the one-minute allowance. Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 3 +- .../__tests__/trash-sweeper.test.ts | 30 +++++++++++++++++++ .../vault-operations/trash-sweeper.ts | 10 ++++--- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 2387de8d9..5895640af 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1246,7 +1246,8 @@ Docker hardening, and durability seatbelts above. - a trashed note restored by hand and trashed again more than a minute later, on a file system whose renames move the change time (ext4 and APFS do; POSIX leaves it optional) - - a file whose attributes changed while it sat in `.trash/` + - a file whose attributes changed in `.trash/` more than a minute after + it was trashed - the file of a row recorded before identities were kept - Both share a serializing lock with the trash move and re-read each 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 373860800..64fc83ff6 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -1210,6 +1210,36 @@ describe("purgeOrphanedTrashEntries", () => { }) }) + 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()) + + await trashSweeper.purgeOrphanedTrashEntries( + { vaultPath: vault, trashEntryStore: storeFailingToDrop(index, ".trash/stuck.md") }, + logger, + ) + + expect(index.getTrashEntry(".trash/stuck.md")?.trashPath).toBe(".trash/stuck.md") + 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") diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 6d4aeb8c2..bb478a734 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -90,8 +90,8 @@ const getRecordedFileCheck = async ( } /** Drops a finished row and returns whether it was dropped. A failed drop - * (a full disk fails a delete but not a read) keeps the row for the next - * sweep instead of ending this one, whose later unlinks can free space. */ + * (a full disk fails a delete but not a read) is logged and keeps the row + * for the next run instead of ending this one for the rows after it. */ const dropTrashEntry = ( params: { trashPath: string; trashEntryStore: TrashEntryStore }, logger: Logger, @@ -377,8 +377,10 @@ const purgeOrphanedTrashEntries = async ( return false } - params.trashEntryStore.deleteTrashEntry(entry.trashPath) - return true + return dropTrashEntry( + { trashPath: entry.trashPath, trashEntryStore: params.trashEntryStore }, + logger, + ) }) if (wasPurged) purgedCount++ From 69bca460a082db2e0833eabe6d9772220056116a Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:54:41 -0400 Subject: [PATCH 22/38] test(trash): prove the unlink ran when a vanished file's row drop fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the row drop failing, a file kept as unmatched leaves the same row, warning, and counts as a file that vanished at the unlink, so the test now asserts the file is gone. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts | 3 +++ 1 file changed, 3 insertions(+) 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 64fc83ff6..5153dcccd 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -878,6 +878,9 @@ describe("sweepExpiredTrashEntries", () => { 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")?.trashPath).toBe(".trash/vanishing.md") expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy).toHaveBeenCalledWith("failed to drop trash entry row", { From 958427a8fc7034255bb72ae176bcd7bc6bb25dce Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:54:43 -0400 Subject: [PATCH 23/38] docs(trash): say how the next sweep drops a row an unlink left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A row left behind after a successful unlink is dropped as missing, or as unmatched when another note has since been trashed under that name. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/trash-sweeper.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index bb478a734..3cd6512e5 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -239,8 +239,9 @@ const sweepOneEntry = async ( return "skipped" } - // The file is gone whether or not the row drop succeeds; a row left behind - // is dropped as missing by the next sweep. + // 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. dropTrashEntry({ trashPath, trashEntryStore }, logger) // The trash move created the file's folder chain, so an unlink can strand From c9e4b8e0874bc10592a922170024dc5281743454 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:02:14 -0400 Subject: [PATCH 24/38] style(trash): name the row-drop helper for its success result and give the sweep's comments a shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - tryDropTrashEntry: the try prefix (as in tryClearStaleTrashEntry) tells a reader at each call site that the boolean is whether the drop succeeded; its doc drops the undefined "finished row" - module docstring: the two unlink conditions as a list, and the re-trash exception no longer reads as the server restoring the note by hand - prune comment: one bullet per concern, using the file's "one bad row never aborts the sweep" wording - the case-only sibling note says the two files share one key, not one row (schema comment and ARCHITECTURE.md) - SECURITY.md: comma so "on a file system" no longer attaches to "that minute" - CONTRIBUTING.md: the env-block paragraph gets back a bold label like its Publishing and Beta testing siblings Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 2 +- CONTRIBUTING.md | 4 +- SECURITY.md | 2 +- src/vault-mcp/search/search-index.ts | 2 +- .../vault-operations/trash-sweeper.ts | 52 +++++++++++-------- 5 files changed, 34 insertions(+), 28 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 5895640af..c9962d044 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1216,7 +1216,7 @@ Docker hardening, and durability seatbelts above. 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 row, so the file recorded first loses its row and + two files sharing one key, so the file recorded first loses its row and stays until `.trash/` is emptied by hand. - **Recorded trash bookkeeping** (`trash-sweeper.ts`): two row-driven operations (neither walks the folder): diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 383189065..2258d4419 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -102,8 +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` -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 d5ac6ebf2..16d11a1f8 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -165,7 +165,7 @@ mechanism-level detail. modification time recorded when the server trashed it, and its inode change time is no later than 60 seconds after that. A different note that Obsidian later trashed under a recorded name is kept, and so is a - trashed note that was restored and trashed again after that minute on a + trashed note that was restored and trashed again after that minute, on a file system whose renames move the change time - 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 diff --git a/src/vault-mcp/search/search-index.ts b/src/vault-mcp/search/search-index.ts index 51499a7d4..5922869d3 100644 --- a/src/vault-mcp/search/search-index.ts +++ b/src/vault-mcp/search/search-index.ts @@ -464,7 +464,7 @@ export const createSearchIndex = ( -- 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 row, so the file recorded first loses its row and + -- 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 diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 3cd6512e5..146c62c3a 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -1,13 +1,18 @@ /** Trash bookkeeping — retention sweep (unlink expired files) and orphan * purge (drop rows whose files are gone). Both operate on trash_entries - * rows and never walk the .trash/ folder. The sweep unlinks a file only - * while it still has the identity its row recorded (inode number, size, and - * modification time) and its change time is no later than about a minute - * after the row was recorded, so Obsidian's own trash entries and - * hand-placed files are out of reach, even under a name the server once - * used. The exception is a note the server trashed, then restored by hand - * and trashed again: it still matches its row when that happens within the - * minute, or on a file system whose renames leave the change time alone. */ + * 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. The exception is a note + * the server trashed that was then restored by hand and trashed again. It + * still matches its row when that happens within the minute, or on a file + * system whose renames leave the change time alone. */ import { unlink } from "node:fs/promises" import { dirname, join, relative, resolve, sep } from "node:path" @@ -89,10 +94,10 @@ const getRecordedFileCheck = async ( } } -/** Drops a finished row and returns whether it was dropped. A failed drop - * (a full disk fails a delete but not a read) is logged and keeps the row - * for the next run instead of ending this one for the rows after it. */ -const dropTrashEntry = ( +/** Drops a trash entry's row and returns whether the drop succeeded. A failed + * drop (a full disk fails a delete but not a read) 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 => { @@ -171,7 +176,7 @@ const sweepOneEntry = async ( const realParent = await realpathOrNull(dirname(resolvedPath)) if (!realTrashRoot || !realParent) { - return dropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" + return tryDropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" } // Equal when the file sits directly in .trash/, not in a subfolder. @@ -208,10 +213,10 @@ const sweepOneEntry = async ( // trashed reaches the unlink below. if (recordedFileCheck === "unreadable") return "skipped" if (recordedFileCheck === "missing") { - return dropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" + return tryDropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" } if (recordedFileCheck !== "matches") { - if (!dropTrashEntry({ trashPath, trashEntryStore }, logger)) return "skipped" + 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 }, @@ -226,7 +231,7 @@ const sweepOneEntry = async ( await unlink(resolvedPath) } catch (error) { if (isErrnoException(error, "ENOENT")) { - return dropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" + return tryDropTrashEntry({ trashPath, trashEntryStore }, logger) ? "missing" : "skipped" } // 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 @@ -242,14 +247,15 @@ const sweepOneEntry = async ( // 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. - dropTrashEntry({ trashPath, trashEntryStore }, logger) + 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 it throws on a - // path with a hidden segment, which only a corrupted or hand-edited row can - // hold; the catch keeps such a row from ending the sweep for the rows after it. + // 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) }, @@ -378,7 +384,7 @@ const purgeOrphanedTrashEntries = async ( return false } - return dropTrashEntry( + return tryDropTrashEntry( { trashPath: entry.trashPath, trashEntryStore: params.trashEntryStore }, logger, ) From 71a73319000cccce936128bb7d9aed798b032f9b Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:02:21 -0400 Subject: [PATCH 25/38] test(trash): require exactly one warning when a folder prune fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The failed-prune test now checks the warning count like its failed-drop siblings, so a second warning on that path fails it. Its comment names the row that must not end the sweep instead of a bare "it". Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- .../vault-operations/__tests__/trash-sweeper.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 5153dcccd..1aabb0c2d 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -762,8 +762,8 @@ describe("sweepExpiredTrashEntries", () => { 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 it must not end the - // sweep for the rows listed after it. + // 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 }) @@ -780,6 +780,7 @@ describe("sweepExpiredTrashEntries", () => { 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', From 1dd4616379a99b94bdd5ee53d2f4f9f1cf9f556f Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:03:33 -0400 Subject: [PATCH 26/38] test(trash): require exactly one warning in each sweep and purge warning test Co-Authored-By: Claude Opus 5.5 --- .../vault-operations/__tests__/trash-sweeper.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 1aabb0c2d..bde40a3f3 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -363,6 +363,7 @@ describe("sweepExpiredTrashEntries", () => { 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", }) @@ -383,6 +384,7 @@ describe("sweepExpiredTrashEntries", () => { 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", }) @@ -404,6 +406,7 @@ describe("sweepExpiredTrashEntries", () => { 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", }) @@ -427,6 +430,7 @@ describe("sweepExpiredTrashEntries", () => { 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: `[Error]: EACCES: permission denied, realpath '${lockedDir}'`, @@ -453,6 +457,7 @@ describe("sweepExpiredTrashEntries", () => { 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: `[Error]: EACCES: permission denied, unlink '${join(lockedDir, "stuck.md")}'`, @@ -508,6 +513,7 @@ describe("sweepExpiredTrashEntries", () => { 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", @@ -565,6 +571,7 @@ describe("sweepExpiredTrashEntries", () => { 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", @@ -655,6 +662,7 @@ describe("sweepExpiredTrashEntries", () => { 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", @@ -696,6 +704,7 @@ describe("sweepExpiredTrashEntries", () => { 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", @@ -721,6 +730,7 @@ describe("sweepExpiredTrashEntries", () => { 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")}'`, @@ -1263,6 +1273,7 @@ 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: `[Error]: EACCES: permission denied, lstat '${join(lockedDir, "stuck.md")}'`, From 450b8486c3edf76d8d2d61043c6e3a4bbfcdfa40 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:30:59 -0400 Subject: [PATCH 27/38] fix(trash): keep a failed row re-read from ending the sweep or the purge Co-Authored-By: Claude Opus 5.5 --- .../__tests__/trash-sweeper.test.ts | 80 +++++++++++++++++++ .../vault-operations/trash-sweeper.ts | 29 ++++++- 2 files changed, 105 insertions(+), 4 deletions(-) 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 bde40a3f3..0585b50ed 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -115,6 +115,23 @@ const sweepAfterRetention = async ( 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 = ( @@ -804,6 +821,41 @@ describe("sweepExpiredTrashEntries", () => { }) }) + 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()) + + 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")?.trashPath).toBe(".trash/unread.md") + 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. @@ -1224,6 +1276,34 @@ 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()) + + await trashSweeper.purgeOrphanedTrashEntries( + { vaultPath: vault, trashEntryStore: storeFailingToRead(index, ".trash/unread.md") }, + logger, + ) + + expect(index.getTrashEntry(".trash/unread.md")?.trashPath).toBe(".trash/unread.md") + 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. diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 146c62c3a..5f5c0e441 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -94,9 +94,27 @@ const getRecordedFileCheck = async ( } } +/** 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 (a full disk fails a delete but not a read) is logged and leaves the - * row for the next run, so one bad row never aborts the sweep or the purge. */ + * 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, @@ -136,7 +154,7 @@ const sweepOneEntry = async ( // 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) return "skipped" @@ -360,7 +378,10 @@ const purgeOrphanedTrashEntries = async ( // would never delete it. // - The whole row is compared because two records in one second share // a trashedAt. - const currentEntry = params.trashEntryStore.getTrashEntry(entry.trashPath) + const currentEntry = tryGetTrashEntry( + { trashPath: entry.trashPath, trashEntryStore: params.trashEntryStore }, + logger, + ) const rowChanged = !currentEntry || currentEntry.trashPath !== entry.trashPath || From 9287f2d44001ff6cab85164ac929d5c19f59ef52 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:57:44 -0400 Subject: [PATCH 28/38] test(search): assert the whole trash entry survives a vault rebuild Co-Authored-By: Claude Opus 5.5 --- .../search/__tests__/search-index.test.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/vault-mcp/search/__tests__/search-index.test.ts b/src/vault-mcp/search/__tests__/search-index.test.ts index bd7ab1136..db35bd40f 100644 --- a/src/vault-mcp/search/__tests__/search-index.test.ts +++ b/src/vault-mcp/search/__tests__/search-index.test.ts @@ -8430,7 +8430,14 @@ 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({ trashPath: ".trash/survivor.md", @@ -8441,7 +8448,11 @@ describe("trash entries (retention-sweep bookkeeping)", () => { 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, + }) }) }) From 5f47b7b9064f2237acaaeca6f7a3eefafbad404f Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 18:50:35 -0400 Subject: [PATCH 29/38] test(trash): assert whole kept rows, and rebuild a day after recording Co-Authored-By: Claude Opus 5.5 --- .../search/__tests__/search-index.test.ts | 3 ++ .../__tests__/trash-sweeper.test.ts | 35 +++++++++++++------ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/src/vault-mcp/search/__tests__/search-index.test.ts b/src/vault-mcp/search/__tests__/search-index.test.ts index db35bd40f..8ef9d2db2 100644 --- a/src/vault-mcp/search/__tests__/search-index.test.ts +++ b/src/vault-mcp/search/__tests__/search-index.test.ts @@ -8445,6 +8445,9 @@ describe("trash entries (retention-sweep bookkeeping)", () => { }) 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) 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 0585b50ed..32cca1beb 100644 --- a/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts +++ b/src/vault-mcp/vault-operations/__tests__/trash-sweeper.test.ts @@ -77,12 +77,20 @@ const recordEntryAt = ( } } -/** The trashedAt the index stored for a row the test has just recorded. */ -const trashedAtOf = (index: ReturnType, trashPath: string): number => { +/** 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.trashedAt + 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 @@ -834,12 +842,13 @@ describe("sweepExpiredTrashEntries", () => { 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")?.trashPath).toBe(".trash/unread.md") + 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) @@ -869,11 +878,12 @@ describe("sweepExpiredTrashEntries", () => { 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")?.trashPath).toBe(".trash/stuck.md") + 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) @@ -903,10 +913,11 @@ describe("sweepExpiredTrashEntries", () => { 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)?.trashPath).toBe(trashPath) + expect(index.getTrashEntry(trashPath)).toEqual(keptRow) expect(warnSpy).toHaveBeenCalledTimes(1) expect(warnSpy).toHaveBeenCalledWith("failed to drop trash entry row", { trashPath, @@ -938,13 +949,14 @@ describe("sweepExpiredTrashEntries", () => { 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")?.trashPath).toBe(".trash/vanishing.md") + 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", @@ -970,12 +982,13 @@ describe("sweepExpiredTrashEntries", () => { 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")?.trashPath).toBe(".trash/recycled.md") + 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", @@ -1285,13 +1298,14 @@ describe("purgeOrphanedTrashEntries", () => { 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")?.trashPath).toBe(".trash/unread.md") + 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", { @@ -1315,13 +1329,14 @@ describe("purgeOrphanedTrashEntries", () => { 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")?.trashPath).toBe(".trash/stuck.md") + 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", { From 89bf9dd40721ff96f1b06d9a8c6e5a5c4861cf52 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 18:50:37 -0400 Subject: [PATCH 30/38] docs(trash): say why the orphan purge treats a row it cannot re-read as changed Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/trash-sweeper.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 5f5c0e441..e47bcf57e 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -378,6 +378,9 @@ const purgeOrphanedTrashEntries = async ( // 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, From 6b3e154c46a16d95b7410d979f1408c965de0704 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 19:17:31 -0400 Subject: [PATCH 31/38] docs(trash): state the window between the identity read and the unlink Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 4 ++++ SECURITY.md | 3 ++- .../vault-operations/trash-sweeper.ts | 23 +++++++++++-------- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index c9962d044..b5a881ab0 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1250,6 +1250,10 @@ Docker hardening, and durability seatbelts above. it was trashed - the file of a row recorded before identities were kept + Gates 3 and 4 read the file in the call just before the unlink, so a + file the host swaps in between the two is still unlinked. Node has no + atomic check-and-unlink to close that gap. + - 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. - **Verify-then-preflight-then-commit move** (`note-mover.ts`): under the diff --git a/SECURITY.md b/SECURITY.md index 16d11a1f8..12ddde077 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -166,7 +166,8 @@ mechanism-level detail. change time is no later than 60 seconds after that. A different note that Obsidian later trashed under a recorded name is kept, and so is a trashed note that was restored and trashed again after that minute, on a - file system whose renames move the change time + file system whose renames move the change time. The check is the call + just before the unlink, so a file swapped in between the two is not caught - 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/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index e47bcf57e..34dfba8b8 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -9,10 +9,12 @@ * 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. The exception is a note - * the server trashed that was then restored by hand and trashed again. It - * still matches its row when that happens within the minute, or on a file - * system whose renames leave the change time alone. */ + * 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" @@ -227,8 +229,8 @@ const sweepOneEntry = async ( logger, ) - // Every result but "matches" returns here, so only the file the server - // trashed reaches the unlink below. + // 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" @@ -242,9 +244,12 @@ const sweepOneEntry = async ( return "unmatched" } - // ENOENT here means the file vanished after the gates ran — the host - // owns the bind mount and can empty the trash at any moment. Same - // outcome as a missing parent: drop the row. + // 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) { From 9d1adc8e41bfa70df2886e4904e53cd28e462b53 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 19:43:37 -0400 Subject: [PATCH 32/38] docs(trash): say which gate drops a row whose parent folder is now a file Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/trash-sweeper.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/vault-mcp/vault-operations/trash-sweeper.ts b/src/vault-mcp/vault-operations/trash-sweeper.ts index 34dfba8b8..81f33470f 100644 --- a/src/vault-mcp/vault-operations/trash-sweeper.ts +++ b/src/vault-mcp/vault-operations/trash-sweeper.ts @@ -188,9 +188,11 @@ 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 (nothing there, or a file - // where a folder should be) means .trash/ was emptied outside the server, - // so the row is dropped. + // 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 realTrashRoot = await realpathOrNull(trashRoot) const realParent = await realpathOrNull(dirname(resolvedPath)) From 413b538cc5cc60492f5db65b31c4341372e79663 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 22:58:23 -0400 Subject: [PATCH 33/38] docs(trash): say in place why every delete passes the stale-row clear Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/mcp-core/tools/vault-crud-tools.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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 4c774efad..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,11 +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, - // Passed for every option because moveNoteToTrash calls it only - // for a move it does not record ("local", or a "system" move - // whose record failed), to drop an earlier row at the landed - // path; moveNoteToTrash says why that row must go. 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, From a6318b23652ea27c4670be8d37b05092e74ef171 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:02:32 -0400 Subject: [PATCH 34/38] docs(trash): trim the trash sections of ARCHITECTURE.md and SECURITY.md to the design Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 62 ++++++++++++++----------------------------------- SECURITY.md | 11 ++++----- 2 files changed, 22 insertions(+), 51 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b5a881ab0..9670ab535 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1202,22 +1202,14 @@ Docker hardening, and durability seatbelts above. 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. - - **Recording** — moves made under the `system` setting are recorded in - the index DB's `trash_entries` table, together with the landed file's - identity (its inode number, size and modification time, read by - `readTrashFileState`). Recording is fail-open — a failed identity read - or row write logs a warning, the delete still succeeds, and an - unrecorded entry is never swept. - - **Stale rows** — a move that is not recorded (`local`, or a failed - record) drops any earlier row at its landed path, so the sweep never - deletes the newly trashed note. - - **Case-folded key** — the table's primary key is the case-folded path. - On a case-insensitive mount `.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 - stays until `.trash/` is emptied by hand. + `system` moves are recorded in the index DB's `trash_entries` table with + the landed file's identity (inode number, size, modification time). + Recording is fail-open — a failed record logs a warning, the delete still + succeeds, and an unrecorded entry is never swept. A move that is not + recorded drops any earlier row at its landed path, so the sweep never + deletes the note just trashed. The table's primary + key is the case-folded path, so a case alias replaces its stale row 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 @@ -1226,34 +1218,16 @@ Docker hardening, and durability seatbelts above. manual emptying or `TRASH_RETENTION_DAYS=none` never leaves unbounded stale rows. - **Retention sweep** — runs at startup and daily. Deletes the files - of recorded entries older than `TRASH_RETENTION_DAYS`. Each unlink - passes four gates: - 1. The resolved path must sit inside `.trash/`, so a corrupted row - cannot reach live notes. - 2. The parent directory's realpath must sit inside `.trash/`, so a - directory symlink cannot redirect the path onto live notes. - 3. The file at the path must still have the row's recorded identity. - 4. The file's inode change time must be no later than 60 seconds after - the row was recorded. The change time is not stored in the row, - because after a Docker Desktop container renames a file it keeps - reporting the file's old change time while the host's value moves, - so a stored value would stop matching once the container restarts. - - A file that fails gate 3 or 4 is kept, its row dropped, and a warning - logged with the reason. This keeps: - - a different note that Obsidian trashed under the same name after - `.trash/` was emptied by hand, which leaves the row behind - - a trashed note restored by hand and trashed again more than a minute - later, on a file system whose renames move the change time (ext4 and - APFS do; POSIX leaves it optional) - - a file whose attributes changed in `.trash/` more than a minute after - it was trashed - - the file of a row recorded before identities were kept - - Gates 3 and 4 read the file in the call just before the unlink, so a - file the host swaps in between the two is still unlinked. Node has no - atomic check-and-unlink to close that gap. - + of recorded entries older than `TRASH_RETENTION_DAYS`. Each unlink is + guarded twice: + - **Containment** — 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. + - **Identity** — the file must still have its recorded identity, and + its change time must be no later than a minute after the server + trashed it. A file that fails is kept and its row dropped, so a note + Obsidian trashed under a recorded name after `.trash/` was emptied + by hand survives the sweep. - 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. - **Verify-then-preflight-then-commit move** (`note-mover.ts`): under the diff --git a/SECURITY.md b/SECURITY.md index 12ddde077..50d62a941 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -161,13 +161,10 @@ mechanism-level detail. 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 inode number, size, and - modification time recorded when the server trashed it, and its inode - change time is no later than 60 seconds after that. A different note - that Obsidian later trashed under a recorded name is kept, and so is a - trashed note that was restored and trashed again after that minute, on a - file system whose renames move the change time. The check is the call - just before the unlink, so a file swapped in between the two is not caught + `.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. `trash-sweeper.ts` + documents the two cases it cannot catch - 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 From 25c13c648d7a1d1f7e4eb9d7de64e42df716488e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:09:56 -0400 Subject: [PATCH 35/38] docs(security): describe the trash sweep's guards without code-level terms Co-Authored-By: Claude Opus 5.5 --- SECURITY.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 50d62a941..b43d59882 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -67,13 +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 containment gates: - - The recorded path must resolve inside `.trash/`, so a corrupted or - hand-edited index row is skipped with a warning instead of followed - to a live note - - The file's parent directory must realpath-resolve inside `.trash/`, - so a directory symlink planted there is skipped the same way +- 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 @@ -163,8 +163,9 @@ mechanism-level detail. - 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. `trash-sweeper.ts` - documents the two cases it cannot catch + 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 From 18b967910cc41645dfd684cf8c366818945e77a7 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:11:01 -0400 Subject: [PATCH 36/38] docs(trash): restore why the delete log carries trashOption Co-Authored-By: Claude Opus 5.5 --- src/vault-mcp/vault-operations/vault-filesystem.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vault-mcp/vault-operations/vault-filesystem.ts b/src/vault-mcp/vault-operations/vault-filesystem.ts index cff957b3e..5853a52e3 100644 --- a/src/vault-mcp/vault-operations/vault-filesystem.ts +++ b/src/vault-mcp/vault-operations/vault-filesystem.ts @@ -836,8 +836,9 @@ const deleteNote = async ( ? await pruneEmptyParents({ vaultPath: params.vaultPath, path }, logger) : 0 - // trashOption is logged because the result text and the tool_result log - // say "trashed" for both "local" and "system". + // 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, From 14a645982470ed659636b1ada35b82b84708c07d Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:30:53 -0400 Subject: [PATCH 37/38] docs(trash): shorten ARCHITECTURE.md's trash handling to the safeguards a reader needs Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 45 +++++++++++---------------------------------- 1 file changed, 11 insertions(+), 34 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 9670ab535..3b03283ca 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,38 +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` moves are recorded in the index DB's `trash_entries` table with - the landed file's identity (inode number, size, modification time). - Recording is fail-open — a failed record logs a warning, the delete still - succeeds, and an unrecorded entry is never swept. A move that is not - recorded drops any earlier row at its landed path, so the sweep never - deletes the note just trashed. The table's primary - key is the case-folded path, so a case alias replaces its stale row 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 a dangling symlink's row is kept), so - manual emptying or `TRASH_RETENTION_DAYS=none` never leaves - unbounded stale rows. - - **Retention sweep** — runs at startup and daily. Deletes the files - of recorded entries older than `TRASH_RETENTION_DAYS`. Each unlink is - guarded twice: - - **Containment** — 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. - - **Identity** — the file must still have its recorded identity, and - its change time must be no later than a minute after the server - trashed it. A file that fails is kept and its row dropped, so a note - Obsidian trashed under a recorded name after `.trash/` was emptied - by hand survives the sweep. - - 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 each `system` move 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 From c958ff574fe4a0db99c29a657137d2393610ceaa Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 10 Oct 2026 00:25:26 -0400 Subject: [PATCH 38/38] docs(trash): stop ARCHITECTURE.md promising every trash move is recorded Co-Authored-By: Claude Opus 5.5 --- ARCHITECTURE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 3b03283ca..99db74d0b 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1201,7 +1201,7 @@ Docker hardening, and durability seatbelts above. - **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 each `system` move with the + 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