diff --git a/src/infrastructure/persistence/lockfile_repository.ts b/src/infrastructure/persistence/lockfile_repository.ts index fff31aff..4db2ace6 100644 --- a/src/infrastructure/persistence/lockfile_repository.ts +++ b/src/infrastructure/persistence/lockfile_repository.ts @@ -18,6 +18,7 @@ // along with Swamp. If not, see . import { dirname } from "@std/path"; +import { UserError } from "../../domain/errors.ts"; import { atomicWriteTextFile } from "./atomic_write.ts"; import { readUpstreamExtensions, @@ -99,11 +100,14 @@ export class LockfileRepository { } /** - * Returns the cached entry map. Callers receive a defensive shallow - * copy so external mutation cannot corrupt the cache. + * Returns the cached entry map. Callers receive a defensive deep copy + * so external mutation — including pushing into nested `files[]` / + * `include[]` arrays — cannot corrupt the cache. `structuredClone` + * (in scope on Deno via the global) covers every value shape the + * lockfile carries today (strings, arrays of strings, booleans). */ getAllEntries(): UpstreamExtensionsMap { - return { ...this.cache }; + return structuredClone(this.cache); } /** @@ -160,6 +164,13 @@ export class LockfileRepository { * the result, releases the lock, and updates this instance's cache. */ async removeEntry(name: string): Promise { + // Symmetric with writeEntry — defensive against callers who removed + // an extension in this process and the parent dir was cleaned up + // between then and now. In practice unreachable today (rm.ts only + // calls this after extensionRmPreview confirmed the entry exists, + // which read the lockfile successfully) but the asymmetry would + // surface as an unhelpful NotFound from acquireLock if it ever did. + await Deno.mkdir(dirname(this.lockfilePath), { recursive: true }); const lockFile = await this.acquireLock(); try { const current = await readUpstreamExtensions(this.lockfilePath); @@ -189,14 +200,18 @@ export class LockfileRepository { await new Promise((r) => setTimeout(r, LOCK_RETRY_DELAY_MS)); continue; } - throw new Error( + // UserError so the top-level handler renders the clean + // message instead of a stack trace — matches the pre-W2-prequel + // behavior in pull.ts (rm.ts threw plain Error; this + // consolidates on the better UX). + throw new UserError( "Could not acquire lock on upstream_extensions.json. Another operation may be in progress. Please retry.", ); } throw error; } } - throw new Error( + throw new UserError( "Could not acquire lock on upstream_extensions.json.", ); } diff --git a/src/infrastructure/persistence/lockfile_repository_test.ts b/src/infrastructure/persistence/lockfile_repository_test.ts index 56aba6b8..c2a9e62e 100644 --- a/src/infrastructure/persistence/lockfile_repository_test.ts +++ b/src/infrastructure/persistence/lockfile_repository_test.ts @@ -17,10 +17,11 @@ // You should have received a copy of the GNU Affero General Public License // along with Swamp. If not, see . -import { assertEquals, assertNotEquals } from "@std/assert"; +import { assertEquals, assertNotEquals, assertRejects } from "@std/assert"; import { join } from "@std/path"; import { LockfileRepository } from "./lockfile_repository.ts"; import { atomicWriteTextFile } from "./atomic_write.ts"; +import { UserError } from "../../domain/errors.ts"; async function withTempDir( fn: (dir: string) => Promise, @@ -172,6 +173,23 @@ Deno.test("LockfileRepository.removeEntry: deletes key and persists", async () = }); }); +Deno.test("LockfileRepository.removeEntry: creates parent dir (symmetric with writeEntry)", async () => { + await withTempDir(async (dir) => { + // Construct against a path whose parent dir does NOT exist yet. + // removeEntry must create it before acquireLock — otherwise + // Deno.open hits NotFound and propagates an unhelpful error. + const path = join(dir, "nested", "subdir", "upstream_extensions.json"); + const repo = new LockfileRepository(path, {}); + + // Should not throw. + await repo.removeEntry("@scope/never-existed"); + + // The empty lockfile is now persisted and the parent dir exists. + const onDisk = JSON.parse(await Deno.readTextFile(path)); + assertEquals(Object.keys(onDisk).length, 0); + }); +}); + Deno.test("LockfileRepository.removeEntry: missing key is a no-op", async () => { await withTempDir(async (dir) => { const path = join(dir, "upstream_extensions.json"); @@ -185,17 +203,28 @@ Deno.test("LockfileRepository.removeEntry: missing key is a no-op", async () => }); }); -Deno.test("LockfileRepository: getAllEntries returns a defensive copy", async () => { +Deno.test("LockfileRepository: getAllEntries returns a defensive deep copy", async () => { await withTempDir(async (dir) => { const path = join(dir, "upstream_extensions.json"); const repo = await LockfileRepository.create(path); - await repo.writeEntry("@scope/foo", "1.0.0", []); + await repo.writeEntry("@scope/foo", "1.0.0", ["models/foo.ts"], { + include: ["dep.ts"], + }); const map = repo.getAllEntries(); - delete map["@scope/foo"]; - // Repo's internal cache must not be affected by external mutation. + // Top-level key deletion must not affect the cache. + delete map["@scope/foo"]; assertNotEquals(repo.getEntry("@scope/foo"), null); + + // Nested array mutation must not affect the cache either — + // deep copy guards against `entries["@x/y"].files!.push(...)` + // patterns that a shallow copy would propagate. + const fresh = repo.getAllEntries(); + fresh["@scope/foo"].files!.push("INJECTED"); + fresh["@scope/foo"].include!.push("INJECTED"); + assertEquals(repo.getEntry("@scope/foo")?.files, ["models/foo.ts"]); + assertEquals(repo.getEntry("@scope/foo")?.include, ["dep.ts"]); }); }); @@ -224,6 +253,30 @@ Deno.test("LockfileRepository: concurrent writers all complete via acquireLock r }); }); +Deno.test("LockfileRepository: lock-acquisition exhaustion throws UserError (clean CLI message, not stack trace)", async () => { + await withTempDir(async (dir) => { + const path = join(dir, "upstream_extensions.json"); + const lockPath = `${path}.lock`; + + // Pre-create the lock file so every retry sees AlreadyExists. + // 10 retries × 100ms = ~1s before the writer gives up. + await Deno.writeTextFile(lockPath, ""); + + const repo = await LockfileRepository.create(path); + const error = await assertRejects( + () => repo.writeEntry("@scope/foo", "1.0.0", []), + UserError, + "Could not acquire lock on upstream_extensions.json", + ); + // Top-level error_output renderer keys off `instanceof UserError` to + // emit a clean message rather than a stack trace; this assertion + // pins the contract. + assertEquals(error instanceof UserError, true); + + await Deno.remove(lockPath); + }); +}); + Deno.test("LockfileRepository: cleans up .lock file on success path", async () => { await withTempDir(async (dir) => { const path = join(dir, "upstream_extensions.json");