diff --git a/.changeset/container-ignore-globs.md b/.changeset/container-ignore-globs.md new file mode 100644 index 00000000..7ba89cc7 --- /dev/null +++ b/.changeset/container-ignore-globs.md @@ -0,0 +1,5 @@ +--- +"@cloudflare/computer": patch +--- + +Add `ignore` to `ContainerBackend`: glob patterns, such as `**/node_modules` and `!/vendor/node_modules`, for paths the container keeps on its own disk instead of syncing. diff --git a/docs/19_performance.md b/docs/19_performance.md index 06ca987e..207e0768 100644 --- a/docs/19_performance.md +++ b/docs/19_performance.md @@ -66,7 +66,7 @@ the more realistic baseline for general usage. ## Local-only paths (`MOUNT_IGNORE`) -A path listed in `MOUNT_IGNORE` is served from the container's disk and +A path matched by `MOUNT_IGNORE` is served from the container's disk and never enters the VFS, the store, the change-pack encoding, or the pull. What this does **not** change is the FUSE round trip: the bytes still diff --git a/packages/computer/src/backend.ts b/packages/computer/src/backend.ts index bc549aa5..1099b665 100644 --- a/packages/computer/src/backend.ts +++ b/packages/computer/src/backend.ts @@ -90,16 +90,16 @@ export interface BackendHandle { // Durable Object). push/pull are no-ops, and the // reconcile-watermarks pass on connect is skipped. sync?: "remote" | "none"; - // Local-only paths this backend's container keeps on its own disk, - // as the container reports them (#179). Absent on backends with no - // such concept. + // The MOUNT_IGNORE patterns this backend's container applies, as the + // container reports them (#179). Paths they match stay on the + // container's own disk. Absent on backends with no such concept. // - // `supported: false` means the container predates the feature, so - // every path is synced regardless of what the host asked for. Worth + // `supported: false` means the container predates patterns, so every + // path is synced regardless of what the host asked for. Worth // logging: it is the difference between a configuration that works // and one that silently does nothing. ignore?: { - readonly paths: readonly string[]; + readonly patterns: readonly string[]; readonly root: string | undefined; readonly supported: boolean; }; diff --git a/packages/computer/src/backends/container/container-backend-ignore.test.ts b/packages/computer/src/backends/container/container-backend-ignore.test.ts index d098e330..ffcba6c6 100644 --- a/packages/computer/src/backends/container/container-backend-ignore.test.ts +++ b/packages/computer/src/backends/container/container-backend-ignore.test.ts @@ -1,15 +1,16 @@ -// connect()'s happy path constructs a WebSocketPair, a workerd global -// the node runner does not provide, so the full dial cannot complete -// here. These exercise the wire format the backend depends on, against -// a fake host. The comparison logic and the error text have their own -// suite in ignore-assertion.test.ts, and the end-to-end behavior is -// covered in computerd's cli tests against a real FUSE mount. +// How ContainerBackend configures and checks MOUNT_IGNORE. The first +// suite stops before the upgrade; the second stubs WebSocketPair, a +// workerd global the node runner does not provide, to run connect() +// through to the check. The comparison logic and the error text have +// their own suite in ignore-assertion.test.ts, and the end-to-end +// behavior is covered in computerd's cli tests against a real FUSE +// mount. import { afterEach, describe, expect, test, vi } from "vitest"; import { ContainerBackend } from "./container-backend.js"; import type { ContainerRuntimeInfo, IWorkspaceContainerAPI } from "./container-host.js"; import type { ContainerLaunchSpec } from "./container-launch-record.js"; -import { ContainerIgnoreMismatchError, readIgnoreReport } from "./ignore-assertion.js"; +import { ContainerIgnoreMismatchError } from "./ignore-assertion.js"; interface FakeHostOptions { // The `ignore` block /__computerd/info reports. Omitted models a @@ -36,7 +37,7 @@ function fakeHost(opts: FakeHostOptions = {}) { async interceptOutboundHttp() {}, async interceptAllOutboundHttp() {}, async fetchPort(port, url) { - const path = new URL(url).pathname; + const path = new URL(url instanceof Request ? url.url : url).pathname; fetches.push({ port, path }); if (path === "/__computerd/info") { return new Response( @@ -66,50 +67,13 @@ function fakeHost(opts: FakeHostOptions = {}) { } describe("ContainerBackend local-only paths", () => { - const readInfo = async (host: IWorkspaceContainerAPI) => { - const res = await host.fetchPort(8080, "http://container/__computerd/info"); - return readIgnoreReport(await res.json()); - }; - - test("reads the ignore block a current container reports", async () => { - const { host } = fakeHost({ - info: { - supported: true, - enabled: true, - root: "/tmp/workspace", - paths: ["node_modules", "dist"], - redundant: [], - }, - }); - expect(await readInfo(host)).toEqual({ - paths: ["/workspace/node_modules", "/workspace/dist"], - root: "/tmp/workspace", - mountPoint: "/workspace", - supported: true, - }); - }); - - test("treats a container with no ignore block as unsupported", async () => { - // The version-skew case the README warns about: the computerd image - // can lag the pinned client. Without this the old image looks like - // it is working while quietly syncing everything. - const { host } = fakeHost({}); - expect(await readInfo(host)).toEqual({ - paths: [], - root: undefined, - mountPoint: undefined, - supported: false, - }); - }); - - test("the backend requests /__computerd/info on the container port", async () => { - // Pins the path and port, so a rename upstream fails here rather - // than silently degrading every deployment to "unsupported". - const { host, fetches } = fakeHost({ - info: { supported: true, paths: [], root: "/tmp/workspace" }, - }); - await host.fetchPort(8080, "http://container/__computerd/info"); - expect(fetches).toContainEqual({ port: 8080, path: "/__computerd/info" }); + test("throws on a pattern computerd would refuse, before any container starts", () => { + // Otherwise the typo surfaces as a daemon that exits during startup. + const { host, starts } = fakeHost(); + expect(() => backendWith(host, ["**/node_modules", "node_modules"])).toThrow( + /"\/node_modules" or "\*\*\/node_modules"/, + ); + expect(starts).toHaveLength(0); }); const backendWith = (host: IWorkspaceContainerAPI, ignore?: readonly string[]) => @@ -130,12 +94,12 @@ describe("ContainerBackend local-only paths", () => { // in the start environment or the image would have to be rebuilt to // change it. const { host, starts } = fakeHost(); - await backendWith(host, ["/node_modules", "/.venv", "/dist"]) + await backendWith(host, ["**/node_modules", "!/vendor/node_modules", "/dist"]) .connect() .catch(() => undefined); expect(starts).toHaveLength(1); - expect(starts[0]?.env?.MOUNT_IGNORE).toBe("/node_modules,/.venv,/dist"); + expect(starts[0]?.env?.MOUNT_IGNORE).toBe("**/node_modules,!/vendor/node_modules,/dist"); }); test("sends no MOUNT_IGNORE when `ignore` is omitted", async () => { @@ -185,7 +149,7 @@ describe("ContainerBackend ignore check on a full connect", () => { async interceptOutboundHttp() {}, async interceptAllOutboundHttp() {}, async fetchPort(_port, url, init) { - const path = new URL(url).pathname; + const path = new URL(url instanceof Request ? url.url : url).pathname; const auth = new Headers(init?.headers).get("authorization"); if (path === "/health") return new Response("ok"); if (path === "/__computerd/info") infoAuth.push(auth); @@ -249,12 +213,16 @@ describe("ContainerBackend ignore check on a full connect", () => { } test("reads /__computerd/info with the client secret", async () => { - const fake = connectingHost({ supported: true, root: "/tmp/workspace", paths: ["dist"] }); - const handle = await backendFor(fake, ["/dist"]).connect(); + const fake = connectingHost({ + supported: true, + root: "/tmp/workspace", + patterns: ["**/node_modules", "/dist"], + }); + const handle = await backendFor(fake, ["**/node_modules", "/dist"]).connect(); expect(fake.infoAuth).toEqual([`Bearer ${SECRET}`]); expect(handle.ignore).toEqual({ - paths: ["/workspace/dist"], + patterns: ["**/node_modules", "/dist"], root: "/tmp/workspace", mountPoint: "/workspace", supported: true, @@ -264,16 +232,25 @@ describe("ContainerBackend ignore check on a full connect", () => { test("accepts a declaration spelled with the mount point", async () => { // computerd strips the mount prefix from "/workspace/dist" and - // applies "dist". The declaration means the same thing. - const fake = connectingHost({ supported: true, root: "/tmp/workspace", paths: ["dist"] }); + // applies "/dist". The declaration means the same thing. + const fake = connectingHost({ supported: true, root: "/tmp/workspace", patterns: ["/dist"] }); const handle = await backendFor(fake, ["/workspace/dist"]).connect(); await handle.close(); }); test("still rejects a real mismatch", async () => { - const fake = connectingHost({ supported: true, root: "/tmp/workspace", paths: ["dist"] }); + const fake = connectingHost({ supported: true, root: "/tmp/workspace", patterns: ["/dist"] }); await expect(backendFor(fake, ["/node_modules"]).connect()).rejects.toBeInstanceOf( ContainerIgnoreMismatchError, ); }); + + test("rejects a computerd that reports paths instead of patterns", async () => { + // A computerd from before patterns would have refused or misread + // "**/node_modules", so it must not pass for one that applied it. + const fake = connectingHost({ supported: true, root: "/tmp/workspace", paths: ["dist"] }); + await expect(backendFor(fake, ["/dist"]).connect()).rejects.toThrow( + /does not support MOUNT_IGNORE patterns/, + ); + }); }); diff --git a/packages/computer/src/backends/container/container-backend.ts b/packages/computer/src/backends/container/container-backend.ts index fb837cda..003a3897 100644 --- a/packages/computer/src/backends/container/container-backend.ts +++ b/packages/computer/src/backends/container/container-backend.ts @@ -58,7 +58,12 @@ import { WorkspaceTransportError } from "../../transport-failure.js"; import type { IWorkspaceContainerAPI, WorkspaceRef } from "./container-host.js"; import type { ContainerInstanceSize, ContainerLaunchSpec } from "./container-launch-record.js"; import { probeComputerdHealth } from "./health-probe.js"; -import { assertIgnoreMatches, type ResolvedIgnore, readIgnoreReport } from "./ignore-assertion.js"; +import { + assertIgnoreMatches, + checkIgnorePatterns, + type ResolvedIgnore, + readIgnoreReport, +} from "./ignore-assertion.js"; // What the backend's `container` factory returns: anything with // a getWorkspaceContainer() method — the shape withWorkspaceContainer @@ -112,13 +117,17 @@ export interface ContainerBackendOptions { heartbeatIntervalMs?: number; // Paths the container keeps on its local disk instead of the - // workspace (#179). Written as mount-relative absolute paths - // ("/node_modules"), and passed to the container at start time as - // MOUNT_IGNORE. + // workspace (#179), as glob patterns passed to the container at start + // time as MOUNT_IGNORE. Each starts with "/" (from the mount root) or + // "**/" (at any depth); "*" matches within a path segment, and a + // leading "!" excludes. The last matching pattern wins. + // + // ignore: ["**/node_modules", "!/vendor/node_modules", "/dist"] // - // connect() reads the resolved set back off /__computerd/info and - // refuses the connection if it disagrees, which catches an image - // whose computerd is too old to honor the variable. + // The constructor throws on a pattern computerd would refuse. connect() + // reads the applied patterns back off /__computerd/info and refuses the + // connection if they disagree, which catches an image whose computerd + // is too old to read them. ignore?: readonly string[]; // Number of forced restart attempts after startup readiness @@ -261,6 +270,7 @@ export class ContainerBackend implements WorkspaceBackend { this.id = options.id ?? "container-shell"; this.#egress = options.egress ?? { mode: "none" }; this.#egressToken = this.#egress.mode === "http-gateway" ? crypto.randomUUID() : undefined; + if (options.ignore !== undefined) checkIgnorePatterns(options.ignore); this.#options = { container: options.container, workspace: options.workspace, @@ -654,10 +664,11 @@ export class ContainerBackend implements WorkspaceBackend { signal: AbortSignal.timeout(this.#options.healthProbeTimeoutMs), }, ); - if (!res.ok) return { paths: [], root: undefined, mountPoint: undefined, supported: false }; + if (!res.ok) + return { patterns: [], root: undefined, mountPoint: undefined, supported: false }; return readIgnoreReport(await res.json()); } catch { - return { paths: [], root: undefined, mountPoint: undefined, supported: false }; + return { patterns: [], root: undefined, mountPoint: undefined, supported: false }; } } diff --git a/packages/computer/src/backends/container/ignore-assertion.test.ts b/packages/computer/src/backends/container/ignore-assertion.test.ts index 8b5fb2ce..ae6388ff 100644 --- a/packages/computer/src/backends/container/ignore-assertion.test.ts +++ b/packages/computer/src/backends/container/ignore-assertion.test.ts @@ -3,27 +3,34 @@ import { describe, expect, test } from "vitest"; import { assertIgnoreMatches, ContainerIgnoreMismatchError, - diffIgnore, + checkIgnorePatterns, type ResolvedIgnore, readIgnoreReport, } from "./ignore-assertion.js"; -// The failure guarded here is slow rather than loud: a stale or absent -// MOUNT_IGNORE looks exactly like a correct one until a dependency tree -// is written and pulled into the DO. So most of these tests are about -// the check firing, not about it passing. +// The failure guarded here is slow rather than loud: a container that +// isn't applying the patterns looks exactly like one that is until a +// dependency tree is written and pulled into the Durable Object. So most +// of these tests are about the check firing, not about it passing. -const supported = (paths: string[]): ResolvedIgnore => ({ - paths, +const supported = (patterns: string[]): ResolvedIgnore => ({ + patterns, root: "/tmp/workspace", mountPoint: "/workspace", supported: true, }); +const unsupported: ResolvedIgnore = { + patterns: [], + root: undefined, + mountPoint: undefined, + supported: false, +}; + describe("readIgnoreReport", () => { - test("reports paths as absolute container paths under the mount", () => { - // computerd reports mount-relative; the host wants something it can - // use against a container path without re-deriving the mount point. + test("reports patterns as computerd wrote them", () => { + // Patterns, not paths: "**/node_modules" can't be joined onto the + // mount point, so they're passed through untouched. const resolved = readIgnoreReport({ backend: { kind: "fuse" }, mountPoint: "/workspace", @@ -31,12 +38,12 @@ describe("readIgnoreReport", () => { supported: true, enabled: true, root: "/tmp/workspace", - paths: ["node_modules", "dist"], - redundant: [], + patterns: ["**/node_modules", "!/vendor/node_modules", "/dist"], + ineffectiveExclusions: [], }, }); expect(resolved).toEqual({ - paths: ["/workspace/node_modules", "/workspace/dist"], + patterns: ["**/node_modules", "!/vendor/node_modules", "/dist"], root: "/tmp/workspace", mountPoint: "/workspace", supported: true, @@ -46,84 +53,102 @@ describe("readIgnoreReport", () => { test("treats a computerd with no ignore block as unsupported", () => { // The old-image case, and the one most likely to occur in practice. // Not a parse error: absence is a meaningful answer. - const resolved = readIgnoreReport({ backend: { kind: "fuse" }, mountPoint: "/workspace" }); - expect(resolved).toEqual({ - paths: [], - root: undefined, - mountPoint: undefined, - supported: false, - }); + expect(readIgnoreReport({ backend: { kind: "fuse" }, mountPoint: "/workspace" })).toEqual( + unsupported, + ); + }); + + test("treats a computerd that reports paths instead of patterns as unsupported", () => { + // A computerd from before patterns reports plain `paths` and would + // reject or misread "**/node_modules". + expect( + readIgnoreReport({ + mountPoint: "/workspace", + ignore: { supported: true, root: "/tmp/workspace", paths: ["node_modules"] }, + }), + ).toEqual(unsupported); }); test("treats a malformed block as unsupported rather than throwing", () => { expect(readIgnoreReport({ ignore: null }).supported).toBe(false); expect(readIgnoreReport({ ignore: "yes" }).supported).toBe(false); expect(readIgnoreReport({ ignore: { supported: false } }).supported).toBe(false); + expect(readIgnoreReport({ ignore: { supported: true, patterns: [1] } }).supported).toBe(false); expect(readIgnoreReport(null).supported).toBe(false); expect(readIgnoreReport(undefined).supported).toBe(false); }); +}); - test("defaults paths to empty when the block omits them", () => { - const resolved = readIgnoreReport({ - mountPoint: "/workspace", - ignore: { supported: true, root: "/tmp/x" }, - }); - expect(resolved).toEqual({ - paths: [], - root: "/tmp/x", - mountPoint: "/workspace", - supported: true, - }); +describe("checkIgnorePatterns", () => { + // The same cases computerd rejects, so a typo fails before a container + // starts rather than as a daemon that exits during startup. + const rejects = (pattern: string, message: RegExp) => { + expect(() => checkIgnorePatterns([pattern])).toThrow(message); + }; + + test("accepts anchored patterns and exclusions", () => { + expect(() => + checkIgnorePatterns([ + "/dist", + "/workspace/dist", + "**/node_modules", + "/packages/*/dist", + "/app/**/node_modules", + "**/*.tsbuildinfo", + "!/vendor/node_modules", + "/dist/", + ]), + ).not.toThrow(); }); -}); -describe("diffIgnore", () => { - test("agrees when the sets match", () => { - expect(diffIgnore(["node_modules", "dist"], ["node_modules", "dist"])).toBeNull(); + test("requires every pattern to start with / or **/", () => { + rejects("node_modules", /"\/node_modules" or "\*\*\/node_modules"/); + rejects("*/dist", /must start with/); + rejects("!vendor/node_modules", /must start with/); + rejects("!", /must start with/); + rejects("**node_modules", /must start with/); }); - test("ignores declaration order", () => { - // computerd reports in declaration order after dropping redundant - // entries; a host listing the same paths differently means the same. - expect(diffIgnore(["dist", "node_modules"], ["node_modules", "dist"])).toBeNull(); + test("requires ** to be a whole segment", () => { + rejects("/a**/b", /whole path segment/); }); - test("ignores slash decoration on either side", () => { - expect(diffIgnore(["/dist/", "node_modules"], ["dist", "node_modules"])).toBeNull(); + test("rejects patterns that would make the whole mount local-only", () => { + for (const pattern of ["/", "**", "/**", "**/**", "!/**"]) { + rejects(pattern, /whole mount/); + } }); - test("collapses duplicates in the declaration", () => { - // computerd would have collapsed them, so the client must too or - // every duplicated entry becomes a spurious mismatch. - expect(diffIgnore(["dist", "dist"], ["dist"])).toBeNull(); + test("rejects wildcards that match every path at some depth", () => { + for (const pattern of ["/*", "/**/*", "**/*", "/*/*", "/*/**", "!/*"]) { + rejects(pattern, /whole mount/); + } }); - test("reports a path the container does not apply", () => { - expect(diffIgnore(["node_modules", "dist"], ["node_modules"])).toEqual({ - missing: ["dist"], - unexpected: [], - }); + test("still accepts targeted wildcards", () => { + expect(() => + checkIgnorePatterns(["/*.log", "/packages/*/dist", "**/*.tsbuildinfo", "/build-*"]), + ).not.toThrow(); }); - test("reports a path the container applies but the caller did not declare", () => { - expect(diffIgnore(["node_modules"], ["node_modules", "target"])).toEqual({ - missing: [], - unexpected: ["target"], - }); + test("rejects . and .. segments, and empty segments", () => { + rejects("/a/../b", /"\." or "\.\." segment/); + rejects("/./a", /"\." or "\.\." segment/); + rejects("/a//b", /empty path segment/); }); - test("reports both directions at once", () => { - expect(diffIgnore(["a", "b"], ["b", "c"])).toEqual({ missing: ["a"], unexpected: ["c"] }); + test("rejects unsupported syntax", () => { + for (const pattern of ["/*.{js,ts}", "/[ab]", "/a?", "/a\\*"]) { + rejects(pattern, /not supported/); + } }); - test("an empty declaration against a configured container is a mismatch", () => { - // Distinct from omitting `ignore` entirely, which skips the check. - // Declaring "nothing is local-only" against a container that makes - // node_modules local-only is a real disagreement. - expect(diffIgnore([], ["node_modules"])).toEqual({ - missing: [], - unexpected: ["node_modules"], - }); + test("rejects a comma, which would split into two patterns", () => { + rejects("/a,/b", /comma/); + }); + + test("bounds pattern length", () => { + rejects(`/${"a".repeat(5000)}`, /longer than 4096/); }); }); @@ -131,126 +156,109 @@ describe("assertIgnoreMatches", () => { test("omitting the declaration skips the check", () => { // The default. Adopting this option is opt-in, so an existing // deployment cannot start failing because a new field appeared. - expect(() => assertIgnoreMatches(undefined, supported(["node_modules"]))).not.toThrow(); - expect(() => - assertIgnoreMatches(undefined, { - paths: [], - root: undefined, - mountPoint: undefined, - supported: false, - }), - ).not.toThrow(); + expect(() => assertIgnoreMatches(undefined, supported(["/node_modules"]))).not.toThrow(); + expect(() => assertIgnoreMatches(undefined, unsupported)).not.toThrow(); }); test("passes when the declaration matches", () => { expect(() => - assertIgnoreMatches(["node_modules", "dist"], supported(["node_modules", "dist"])), + assertIgnoreMatches( + ["**/node_modules", "!/vendor/node_modules"], + supported(["**/node_modules", "!/vendor/node_modules"]), + ), ).not.toThrow(); }); - test("rejects a computerd that does not support the feature", () => { - // README warns the computerd image can lag the pinned client. An - // old image would otherwise look like it is working while quietly - // syncing a full node_modules. + test("compares declarations in computerd's normalized spelling", () => { + // computerd strips the mount point and trailing slashes and writes a + // leading /** as **, so these all configure what it reports. expect(() => - assertIgnoreMatches(["node_modules"], { - paths: [], - root: undefined, - mountPoint: undefined, - supported: false, - }), - ).toThrow(ContainerIgnoreMismatchError); + assertIgnoreMatches( + ["/workspace/dist/", "!/workspace/vendor/node_modules", "/**/node_modules"], + supported(["/dist", "!/vendor/node_modules", "**/node_modules"]), + ), + ).not.toThrow(); + }); + + test("does not strip a prefix that only looks like the mount point", () => { + expect(() => assertIgnoreMatches(["/workspacefoo"], supported(["/foo"]))).toThrow( + ContainerIgnoreMismatchError, + ); + }); + + test("a reordered list is a mismatch", () => { + // With exclusions, order changes the meaning: this pair keeps + // vendor/node_modules synced one way round and local-only the other. expect(() => - assertIgnoreMatches(["node_modules"], { - paths: [], - root: undefined, - mountPoint: undefined, - supported: false, - }), - ).toThrow(/does not support local-only paths/); + assertIgnoreMatches( + ["**/node_modules", "!/vendor/node_modules"], + supported(["!/vendor/node_modules", "**/node_modules"]), + ), + ).toThrow(/same patterns in a different order/); }); - test("the unsupported message says what the consequence is", () => { - // Not just "mismatch". The operator needs to know the paths will be - // pulled into the DO, which is the expensive part. + test("rejects a computerd that does not support patterns", () => { + // An old image would otherwise look like it is working while quietly + // syncing a full node_modules. try { - assertIgnoreMatches(["node_modules"], { - paths: [], - root: undefined, - mountPoint: undefined, - supported: false, - }); + assertIgnoreMatches(["**/node_modules"], unsupported); expect.unreachable("should have thrown"); } catch (error) { - expect((error as Error).message).toMatch(/pulled into the Durable Object/); - expect((error as Error).message).toMatch(/Upgrade the computerd image/); + expect(error).toBeInstanceOf(ContainerIgnoreMismatchError); + const message = (error as Error).message; + expect(message).toMatch(/does not support MOUNT_IGNORE patterns/); + expect(message).toMatch(/pulled into the Durable Object/); + expect(message).toMatch(/Upgrade the computerd image/); } }); - test("names which paths will be synced when the container is missing one", () => { + test("names patterns the container is not applying", () => { try { - assertIgnoreMatches(["node_modules", "dist"], supported(["node_modules"])); + assertIgnoreMatches(["**/node_modules", "/dist"], supported(["**/node_modules"])); expect.unreachable("should have thrown"); } catch (error) { const message = (error as Error).message; - expect(message).toMatch(/"dist"/); - expect(message).toMatch(/WILL be synced/); + expect(message).toMatch(/"\/dist"/); + expect(message).toMatch(/declared but not applied/); } }); - test("names which paths will not be synced when the container adds one", () => { - // The opposite direction is just as dangerous: the caller believes - // `target` is durable and it is not. + test("names patterns the container applies that were not declared", () => { try { - assertIgnoreMatches(["node_modules"], supported(["node_modules", "target"])); + assertIgnoreMatches(["**/node_modules"], supported(["**/node_modules", "/target"])); expect.unreachable("should have thrown"); } catch (error) { const message = (error as Error).message; - expect(message).toMatch(/"target"/); - expect(message).toMatch(/will NOT be synced/); + expect(message).toMatch(/"\/target"/); + expect(message).toMatch(/applied by the container but not declared/); } }); + test("an empty declaration against a configured container is a mismatch", () => { + // Distinct from omitting `ignore`, which skips the check. + expect(() => assertIgnoreMatches([], supported(["**/node_modules"]))).toThrow( + ContainerIgnoreMismatchError, + ); + }); + test("points at the setting that overrides `ignore`", () => { - // `ignore` is passed to the container as MOUNT_IGNORE, so a - // disagreement means something else set the variable after it. try { - assertIgnoreMatches(["a"], supported(["b"])); + assertIgnoreMatches(["/a"], supported(["/b"])); expect.unreachable("should have thrown"); } catch (error) { expect((error as Error).message).toMatch(/MOUNT_IGNORE in `containerEnv`/); } }); - test("accepts declarations spelled with the mount point", () => { - // computerd strips the mount prefix, so "/workspace/dist" and "/dist" - // configure the same path. Comparing them raw rejects a container - // that is doing exactly what was asked. - expect(() => - assertIgnoreMatches( - ["/workspace/dist", "/workspace/node_modules/"], - supported(["/workspace/dist", "/workspace/node_modules"]), - ), - ).not.toThrow(); - }); - - test("does not strip a prefix that only looks like the mount point", () => { - // "/workspacefoo" is not under "/workspace", so it names - // "/workspace/workspacefoo", not "/workspace/foo". - expect(() => assertIgnoreMatches(["/workspacefoo"], supported(["/workspace/foo"]))).toThrow( - ContainerIgnoreMismatchError, - ); - }); - - test("carries the declared and actual sets on the error", () => { + test("carries the declared and actual lists on the error", () => { // So a host can log or reconcile them without parsing the message. try { - assertIgnoreMatches(["a"], supported(["b"])); + assertIgnoreMatches(["/a"], supported(["/b"])); expect.unreachable("should have thrown"); } catch (error) { const mismatch = error as ContainerIgnoreMismatchError; - expect(mismatch.declared).toEqual(["a"]); - expect(mismatch.actual).toEqual(["b"]); + expect(mismatch.declared).toEqual(["/a"]); + expect(mismatch.actual).toEqual(["/b"]); expect(mismatch.supported).toBe(true); } }); diff --git a/packages/computer/src/backends/container/ignore-assertion.ts b/packages/computer/src/backends/container/ignore-assertion.ts index 8bf9696a..57bd53ce 100644 --- a/packages/computer/src/backends/container/ignore-assertion.ts +++ b/packages/computer/src/backends/container/ignore-assertion.ts @@ -16,37 +16,36 @@ export interface ComputerdIgnoreReport { readonly supported?: boolean; readonly enabled?: boolean; readonly root?: string; - readonly paths?: readonly string[]; - readonly redundant?: readonly string[]; + readonly patterns?: readonly string[]; + readonly ineffectiveExclusions?: readonly string[]; readonly fastPaths?: Readonly>; } /** What the backend exposes back to the host after a successful connect. */ export interface ResolvedIgnore { /** - * Absolute paths as they exist inside the container, under MOUNT_POINT. - * `node_modules` with a mount of /workspace reports /workspace/node_modules, - * so the value can be used directly against a container path without the - * caller re-deriving the mount. Empty when the feature is off. + * The MOUNT_IGNORE patterns computerd applied, normalized and in the + * order written, for example `["/dist", "!/vendor/node_modules"]`. + * Empty when the feature is off or unsupported. */ - readonly paths: readonly string[]; + readonly patterns: readonly string[]; /** * Where local-only content is stored on the container's disk * (MOUNT_IGNORE_PATH). Undefined when unsupported. */ readonly root: string | undefined; - /** The mount point the paths are rooted at. Undefined when unsupported. */ + /** The mount point the patterns are anchored at. Undefined when unsupported. */ readonly mountPoint: string | undefined; - /** False on a computerd predating the feature, so a host can degrade. */ + /** False on a computerd predating patterns, so a host can degrade. */ readonly supported: boolean; } -/** Joins a mount-relative entry onto the mount point. */ -function toContainerPath(entry: string, mountPoint: string): string { - const base = mountPoint.replace(/\/+$/, ""); - const rel = entry.replace(/^\/+/, ""); - return `${base}/${rel}`; -} +const UNSUPPORTED_REPORT: ResolvedIgnore = { + patterns: [], + root: undefined, + mountPoint: undefined, + supported: false, +}; export class ContainerIgnoreMismatchError extends Error { readonly declared: readonly string[]; @@ -68,57 +67,85 @@ export class ContainerIgnoreMismatchError extends Error { /** * Reads the `ignore` block out of a /__computerd/info body. * - * Tolerant by design: an older computerd has no such block, and that is - * a supported answer (`supported: false`) rather than a parse error. - * The caller decides whether it is acceptable. + * Tolerant by design: an older computerd has no such block, or reports + * plain `paths` from before patterns, and both are a supported answer + * (`supported: false`) rather than a parse error. The caller decides + * whether that is acceptable. */ export function readIgnoreReport(info: unknown): ResolvedIgnore { if (typeof info !== "object" || info === null || !("ignore" in info)) { - return { paths: [], root: undefined, mountPoint: undefined, supported: false }; + return UNSUPPORTED_REPORT; } const report = (info as { ignore?: unknown }).ignore; - if (typeof report !== "object" || report === null) { - return { paths: [], root: undefined, mountPoint: undefined, supported: false }; - } + if (typeof report !== "object" || report === null) return UNSUPPORTED_REPORT; const typed = report as ComputerdIgnoreReport; - if (typed.supported !== true) { - return { paths: [], root: undefined, mountPoint: undefined, supported: false }; + if (typed.supported !== true) return UNSUPPORTED_REPORT; + const patterns: unknown = typed.patterns; + if (!Array.isArray(patterns) || !patterns.every((entry) => typeof entry === "string")) { + return UNSUPPORTED_REPORT; } - // computerd reports entries mount-relative; the host wants paths it can - // use against the container directly, so they are joined onto the mount - // point from the same payload. const mountPoint = (info as { mountPoint?: unknown }).mountPoint; - const base = typeof mountPoint === "string" && mountPoint !== "" ? mountPoint : "/workspace"; return { - paths: Array.isArray(typed.paths) ? typed.paths.map((e) => toContainerPath(e, base)) : [], + patterns, root: typeof typed.root === "string" ? typed.root : undefined, - mountPoint: base, + mountPoint: typeof mountPoint === "string" && mountPoint !== "" ? mountPoint : "/workspace", supported: true, }; } -/** - * Compares a declared set against what the container applies; null when - * they agree. Order-insensitive and duplicate-collapsing, because - * computerd normalizes the same way and the two spellings mean the same - * thing. - */ -export function diffIgnore( - declared: readonly string[], - actual: readonly string[], -): { missing: string[]; unexpected: string[] } | null { - const declaredSet = new Set(declared.map(normalize)); - const actualSet = new Set(actual.map(normalize)); - - const missing = [...declaredSet].filter((entry) => !actualSet.has(entry)).sort(); - const unexpected = [...actualSet].filter((entry) => !declaredSet.has(entry)).sort(); - - if (missing.length === 0 && unexpected.length === 0) return null; - return { missing, unexpected }; +// The same rules computerd applies at startup (computerd's +// src/fuse/ignore.ts). Duplicated rather than shared because this +// package does not depend on computerd; the two test suites pin the +// same cases. Checked in the backend constructor, so a typo fails before +// a container starts rather than as a daemon that exits during startup. +const MAX_PATTERN_LENGTH = 4096; +const UNSUPPORTED_SYNTAX = /[{}[\]?\\]/; + +/** Throws on the first pattern computerd would refuse. */ +export function checkIgnorePatterns(patterns: readonly string[]): void { + for (const raw of patterns) { + const pattern = raw.trim(); + const fail = (reason: string): never => { + throw new Error(`\`ignore\` pattern ${JSON.stringify(raw)} ${reason}`); + }; + if (pattern.length > MAX_PATTERN_LENGTH) { + fail(`is longer than ${MAX_PATTERN_LENGTH} characters.`); + } + const body = pattern.startsWith("!") ? pattern.slice(1) : pattern; + if (!body.startsWith("/") && !body.startsWith("**/") && body !== "**") { + const bare = stripSlashes(body.replace(/^\*\*(?=[^/])/, "")) || "path"; + fail( + `must start with "/" (from the mount root) or "**/" (at any depth), ` + + `for example "/${bare}" or "**/${bare}".`, + ); + } + if (UNSUPPORTED_SYNTAX.test(body)) { + fail(`uses syntax that is not supported. Only "*", "**", and a leading "!" are.`); + } + if (body.includes(",")) { + fail("contains a comma, which separates patterns in MOUNT_IGNORE."); + } + const trimmed = stripSlashes(body); + const parts = trimmed === "" ? [] : trimmed.split("/"); + if (parts.some((part) => part === "")) fail("contains an empty path segment."); + if (parts.some((part) => part === "." || part === "..")) { + fail(`contains a "." or ".." segment.`); + } + if (parts.some((part) => part !== "**" && part.includes("**"))) { + fail(`uses "**" inside a segment. "**" must be a whole path segment.`); + } + // Only "*" and "**" segments match every path at some depth, and + // everything under a local-only directory is local-only: "/*" alone + // would keep the whole workspace off the Durable Object. + if (parts.every((part) => part === "**" || part === "*")) { + fail("would make the whole mount local-only, so nothing would be synced."); + } + } } /** - * Throws when the container disagrees. `declared === undefined` skips the + * Throws when the container is not applying exactly the declared + * patterns, in the declared order. `declared === undefined` skips the * check, so an existing deployment cannot start failing because a new * field appeared. */ @@ -130,7 +157,7 @@ export function assertIgnoreMatches( if (!resolved.supported) { throw new ContainerIgnoreMismatchError( - `This container's computerd does not support local-only paths, but ` + + `This container's computerd does not support MOUNT_IGNORE patterns, but ` + `\`ignore\` declared ${formatList(declared)}. Those paths would be ` + `recorded in the workspace and pulled into the Durable Object. ` + `Upgrade the computerd image, or remove \`ignore\` to accept the ` + @@ -139,58 +166,64 @@ export function assertIgnoreMatches( ); } - // resolved.paths are absolute container paths. A declaration may be - // written mount-relative ("/node_modules") or with the mount point - // ("/workspace/node_modules"), and computerd accepts both. Compare - // both sides on the mount-relative form. - const declaredRelative = declared.map((path) => stripMount(path, resolved.mountPoint)); - const actualRelative = resolved.paths.map((path) => stripMount(path, resolved.mountPoint)); - const difference = diffIgnore(declaredRelative, actualRelative); - if (difference === null) return; + // computerd reports patterns in a normalized spelling. Normalize the + // declaration the same way, then compare in order: with exclusions, + // the same patterns in a different order mean something different. + const expected = declared.map((pattern) => normalizePattern(pattern, resolved.mountPoint)); + const actual = resolved.patterns.map((pattern) => normalizePattern(pattern, undefined)); + if (expected.length === actual.length && expected.every((value, i) => value === actual[i])) { + return; + } + const actualSet = new Set(actual); + const expectedSet = new Set(expected); + const missing = expected.filter((value) => !actualSet.has(value)); + const unexpected = actual.filter((value) => !expectedSet.has(value)); const parts: string[] = []; - if (difference.missing.length > 0) { - parts.push( - `declared but not applied by the container: ${formatList(difference.missing)} ` + - `(these paths WILL be synced)`, - ); + if (missing.length > 0) { + parts.push(`declared but not applied by the container: ${formatList(missing)}`); + } + if (unexpected.length > 0) { + parts.push(`applied by the container but not declared: ${formatList(unexpected)}`); } - if (difference.unexpected.length > 0) { + if (parts.length === 0) { parts.push( - `applied by the container but not declared: ${formatList(difference.unexpected)} ` + - `(these paths will NOT be synced)`, + `the container applies the same patterns in a different order ` + + `(${formatList(actual)}), and the last matching pattern wins`, ); } throw new ContainerIgnoreMismatchError( - `Container ignore set does not match \`ignore\`: ${parts.join("; ")}. ` + + `Container MOUNT_IGNORE does not match \`ignore\`: ${parts.join("; ")}. ` + `\`ignore\` is passed to the container as MOUNT_IGNORE, so a ` + `MOUNT_IGNORE in \`containerEnv\` overrides it. Remove one of them, ` + - `or check that the computerd image reads MOUNT_IGNORE as a ` + - `comma-separated list.`, - { declared: [...declared], actual: [...resolved.paths], supported: true }, + `or check that the computerd image supports MOUNT_IGNORE patterns.`, + { declared: [...declared], actual: [...resolved.patterns], supported: true }, ); } /** - * Reduces an absolute container path to its mount-relative form, so a - * declaration and a report can be compared on the same footing. + * computerd's spelling of a pattern: mount point and trailing slashes + * stripped, a leading "/**" written as "**". */ -function stripMount(path: string, mountPoint: string | undefined): string { - if (mountPoint === undefined) return path; - const base = mountPoint.replace(/\/+$/, ""); - const trimmed = path.trim(); - if (base !== "" && (trimmed === base || trimmed.startsWith(`${base}/`))) { - return trimmed.slice(base.length + 1); +function normalizePattern(raw: string, mountPoint: string | undefined): string { + const pattern = raw.trim(); + const exclude = pattern.startsWith("!"); + let body = exclude ? pattern.slice(1) : pattern; + const base = mountPoint?.replace(/\/+$/, "") ?? ""; + if (base !== "" && (body === base || body.startsWith(`${base}/`))) { + body = body.slice(base.length) || "/"; } - return trimmed; + const parts = stripSlashes(body).split("/"); + const canonical = parts[0] === "**" ? parts.join("/") : `/${parts.join("/")}`; + return `${exclude ? "!" : ""}${canonical}`; } -function normalize(entry: string): string { - let value = entry.trim(); - while (value.startsWith("/")) value = value.slice(1); - while (value.endsWith("/")) value = value.slice(0, -1); - return value; +function stripSlashes(value: string): string { + let out = value; + while (out.startsWith("/")) out = out.slice(1); + while (out.endsWith("/")) out = out.slice(0, -1); + return out; } function formatList(entries: readonly string[]): string { diff --git a/packages/computerd/README.md b/packages/computerd/README.md index 62677cea..81766a35 100644 --- a/packages/computerd/README.md +++ b/packages/computerd/README.md @@ -144,7 +144,7 @@ not anything a user typed. ### Configuration `ContainerBackend` takes an `ignore` option and passes it to the -container's start environment, so changing the set is a deployment +container's start environment, so changing the patterns is a deployment change rather than an image rebuild. `LegacyContainerBackend` has no such option; set `MOUNT_IGNORE` through its `containerEnv` instead. @@ -152,54 +152,90 @@ such option; set `MOUNT_IGNORE` through its `containerEnv` instead. new ContainerBackend({ container: env.CONTAINER, workspace: { binding: "SESSIONS", id: sessionId }, - ignore: ["/node_modules", "/.venv", "/dist"], + ignore: ["**/node_modules", "!/vendor/node_modules", "**/.venv", "/dist"], }); ``` -That becomes `MOUNT_IGNORE=/node_modules,/.venv,/dist`. Setting the -variable directly, in `containerEnv` or a Dockerfile, works too and -takes precedence. - -`MOUNT_IGNORE` is a comma-separated list of paths anchored at the mount -root: `/node_modules` means `$MOUNT_POINT/node_modules`. There is no glob -syntax and no negation. A path is local-only if it equals an entry or -sits beneath it, so `/app/node_modules` matches only that path, and a -monorepo lists each `/node_modules` separately. A path containing a -comma cannot be expressed. `MOUNT_IGNORE_PATH` sets where local-only +That becomes +`MOUNT_IGNORE=**/node_modules,!/vendor/node_modules,**/.venv,/dist`. +Setting the variable directly, in `containerEnv` or a Dockerfile, works +too and takes precedence. `MOUNT_IGNORE_PATH` sets where local-only content is stored and defaults to `/tmp` + `$MOUNT_POINT`. -The set is compiled once at startup, so it cannot change under a running -container, and two sessions sharing one container see the same -durability boundary. `connect()` reads the resolved set back off -`/__computerd/info` and refuses the connection if it disagrees with what -was declared, which catches a computerd too old to honor the variable. -The handle exposes it as absolute container paths: +### Patterns + +`MOUNT_IGNORE` is a comma-separated list of glob patterns, a small +subset of gitignore. Every pattern starts with `/` (from the mount root) +or `**/` (at any depth): + +| Pattern | Means | +| --- | --- | +| `/dist` | `$MOUNT_POINT/dist` and everything under it | +| `/workspace/dist` | the same; the mount point is optional | +| `**/node_modules` | `node_modules` at any depth, including the root | +| `/packages/*/dist` | `*` matches within one path segment and never crosses `/` | +| `/app/**/node_modules` | any depth under `app` | +| `**/*.tsbuildinfo` | single files work too | +| `!/vendor/node_modules` | an exclusion: keeps a path synced | + +A pattern names a path and everything under it, so `/cache` and +`/cache/**` mean the same thing. A trailing `/` is ignored. Matching is +case-sensitive. + +The last matching pattern wins, and a path is local-only if it or any +directory above it is ignored. That is git's rule, and it decides what +an exclusion can do. `**/node_modules,!/vendor/node_modules` keeps +`vendor/node_modules` synced, because the exclusion applies at the same +level as the match. `**/node_modules,!**/node_modules/.bin` does nothing: +once `node_modules` is on local disk, the synced side has no directory +for `.bin` to live in. computerd warns about an exclusion like that at +startup. + +These fail the daemon at startup, because a dropped pattern means a full +`node_modules` goes into the Durable Object: + +| Pattern | Why | +| --- | --- | +| `node_modules` | not anchored. In gitignore it would match at any depth; write `/node_modules` or `**/node_modules` | +| `**node_modules`, `/a**/b` | `**` must be a whole path segment | +| `**`, `/**`, `/`, `/*`, `**/*` | would make the whole mount local-only. A pattern of only `*` and `**` segments matches every top-level entry, and everything under a local-only directory is local-only | +| `/a/../b`, `/./a`, `/a//b` | `.`, `..`, and empty segments | +| `/*.{js,ts}`, `/[ab]`, `/a?`, `/a\*` | braces, character classes, `?`, and escapes aren't supported | + +A pattern can't contain a comma, since commas separate patterns. +`ContainerBackend` checks `ignore` against the same rules in its +constructor, so a typo throws before a container starts. + +`MOUNT_IGNORE_PATH` must be absolute, must not be `/`, and must not be +equal to or inside `MOUNT_POINT`, since a root inside the mount would +resolve into itself. + +### Checking what the container applied + +The patterns are compiled once at startup, so they can't change under a +running container, and two sessions sharing one container see the same +durability boundary. `connect()` reads the applied patterns back off +`/__computerd/info` and refuses the connection unless they match what +was declared, in the same order, since order changes the meaning. That +catches a computerd too old to read patterns, and a `MOUNT_IGNORE` in +`containerEnv` overriding the option. The handle exposes them: ```ts const handle = await backend.connect(); handle.ignore; // { -// paths: ["/workspace/node_modules", "/workspace/.venv", "/workspace/dist"], +// patterns: ["**/node_modules", "!/vendor/node_modules", "**/.venv", "/dist"], // root: "/tmp/workspace", // mountPoint: "/workspace", // supported: true, // } ``` -`supported: false` means the container predates the feature and every -path is synced. +`supported: false` means the container predates patterns and every path +is synced. -### Validation - -`MOUNT_IGNORE_PATH` must be absolute, must not be `/`, and must not be -equal to or inside `MOUNT_POINT`, since a root inside the mount would -resolve into itself. Entries may not contain `.` or `..` segments. Each -of these fails the daemon at startup rather than quietly disabling the -feature, because a dropped entry means a full `node_modules` goes into -the Durable Object. Duplicates and entries nested inside another entry -are dropped as redundant and reported. - -`/__computerd/info` reports the normalized configuration: +`/__computerd/info` reports the patterns in computerd's normalized +spelling, with the mount point and trailing slashes removed: ```jsonc { @@ -207,8 +243,8 @@ are dropped as redundant and reported. "supported": true, "enabled": true, "root": "/tmp/workspace", - "paths": ["node_modules", "dist"], - "redundant": ["node_modules/.cache"], + "patterns": ["**/node_modules", "!/vendor/node_modules", "!**/node_modules/.bin"], + "ineffectiveExclusions": ["!**/node_modules/.bin"], "fastPaths": { "passthrough": false, "passthroughReason": "fuse-native binds libfuse 2.9; FOPEN_PASSTHROUGH requires the libfuse 3.17 API", @@ -222,6 +258,26 @@ are dropped as redundant and reported. writes skip the VFS and the transfer but still cross FUSE; see [19. Performance](../../docs/19_performance.md#local-only-paths-mount_ignore). +### Synced directories that hold local-only paths + +A local-only path is stored at the same relative path under +`MOUNT_IGNORE_PATH`, so `packages/app/node_modules` lives at +`/tmp/workspace/packages/app/node_modules`. The parent `packages/app` +stays synced. computerd keeps the two sides in step: + +- Listing a synced directory includes its local-only children. +- Renaming a synced directory moves its local-only contents too. The + two moves can't be atomic together, so if the local one fails, the + rename still succeeds and computerd logs where the contents were left. + A rename that changes which patterns match, such as moving a + directory out from under `/app/**/node_modules`, leaves contents on + local disk that are no longer local-only and so aren't reachable; + prefer `**/` patterns for trees that get moved. +- Removing a synced directory, or renaming another directory onto it, + returns `ENOTEMPTY` while it still holds local-only contents, which + the synced side can't see. `rm -rf` removes + the contents first, so it works as usual. + ### Renames across the boundary A rename whose source and destination sit on opposite sides of the @@ -241,6 +297,10 @@ renames into place. The fix is to ignore the staging path too: ignore: ["/dist", "/.tmp-build"]; ``` +With `**/` patterns, keep staging directories and their destinations on +the same side: `**/node_modules` already covers anything a package +manager stages inside `node_modules`. + Candidates worth checking are `.next`, `.turbo`, `node_modules/.cache`, and any staging directory a bundler creates next to its output. computerd logs this guidance on the first crossing rename per mount, diff --git a/packages/computerd/src/cli/computerd.test.ts b/packages/computerd/src/cli/computerd.test.ts index 8bf4a335..285b0c2b 100644 --- a/packages/computerd/src/cli/computerd.test.ts +++ b/packages/computerd/src/cli/computerd.test.ts @@ -107,8 +107,8 @@ test("computerd exposes file IO through real FUSE when FUSE_MOUNT=fuse", async ( supported: true, enabled: false, root: `/tmp${mountPoint}`, - paths: [], - redundant: [], + patterns: [], + ineffectiveExclusions: [], fastPaths: { passthrough: false, passthroughReason: expect.stringContaining("libfuse 2.9"), @@ -146,7 +146,7 @@ test("MOUNT_IGNORE keeps matching paths on local disk and out of the VFS", async expect(JSON.parse(info.body).ignore).toMatchObject({ enabled: true, root: ignoreRoot, - paths: ["node_modules", "dist"], + patterns: ["/node_modules", "/dist"], }); // Write through the mount into an ignored path. @@ -195,6 +195,75 @@ test("MOUNT_IGNORE keeps matching paths on local disk and out of the VFS", async expect(JSON.parse(stats.body).localPaths).toMatchObject({ crossLayerRenames: 1 }); }); +test("MOUNT_IGNORE patterns route node_modules at any depth, with exclusions", async (ctx) => { + const backend = await resolveFuseBackend("auto"); + if (backend.kind !== "fuse") { + ctx.skip(`requires real FUSE; auto resolved to ${backend.kind}`); + return; + } + + const port = await getAvailablePort(); + const mountPoint = await fs.mkdtemp(path.join(os.tmpdir(), "computerd-mount-")); + const ignoreRoot = await fs.mkdtemp(path.join(os.tmpdir(), "computerd-local-")); + await startComputerd({ + port, + mountPoint, + env: { + FUSE_MOUNT: "fuse", + MOUNT_IGNORE: "**/node_modules,!/vendor/node_modules,!**/node_modules/.bin", + MOUNT_IGNORE_PATH: ignoreRoot, + }, + }); + + const info = await request(`http://127.0.0.1:${port}/__computerd/info`); + expect(JSON.parse(info.body).ignore).toMatchObject({ + patterns: ["**/node_modules", "!/vendor/node_modules", "!**/node_modules/.bin"], + ineffectiveExclusions: ["!**/node_modules/.bin"], + }); + + const write = async (relative: string, contents: string) => { + await fs.mkdir(path.dirname(path.join(mountPoint, relative)), { recursive: true }); + await fs.writeFile(path.join(mountPoint, relative), contents); + }; + const onLocalDisk = (relative: string) => + fs.readFile(path.join(ignoreRoot, relative), "utf8").then( + () => true, + () => false, + ); + + await write("packages/app/node_modules/pkg/index.js", "nested"); + await write("vendor/node_modules/pinned/index.js", "vendored"); + await write("packages/app/node_modules/.bin/tool", "bin"); + await write("packages/app/src/main.ts", "source"); + + // A nested node_modules is local-only, as is .bin under it: the + // exclusion can't put it back once its parent is on local disk. + expect(await onLocalDisk("packages/app/node_modules/pkg/index.js")).toBe(true); + expect(await onLocalDisk("packages/app/node_modules/.bin/tool")).toBe(true); + // The excluded node_modules and ordinary source stay in the VFS. + expect(await onLocalDisk("vendor/node_modules/pinned/index.js")).toBe(false); + expect(await onLocalDisk("packages/app/src/main.ts")).toBe(false); + + // A synced directory lists its local-only child. + expect((await fs.readdir(path.join(mountPoint, "packages/app"))).sort()).toEqual([ + "node_modules", + "src", + ]); + + // Renaming the synced package moves its node_modules with it. + await fs.rename(path.join(mountPoint, "packages/app"), path.join(mountPoint, "packages/web")); + expect( + await fs.readFile(path.join(mountPoint, "packages/web/node_modules/pkg/index.js"), "utf8"), + ).toBe("nested"); + expect(await onLocalDisk("packages/web/node_modules/pkg/index.js")).toBe(true); + await expect(fs.stat(path.join(ignoreRoot, "packages/app"))).rejects.toThrow(); + + // rm -rf through the mount removes both sides. + await fs.rm(path.join(mountPoint, "packages/web"), { recursive: true }); + await expect(fs.stat(path.join(mountPoint, "packages/web"))).rejects.toThrow(); + await expect(fs.stat(path.join(ignoreRoot, "packages/web"))).rejects.toThrow(); +}); + test("/api serves a capnweb WorkspaceRPC session", async (_ctx) => { const { createWorkspaceClient } = await import("@cloudflare/computer-rpc/client"); const port = await getAvailablePort(); diff --git a/packages/computerd/src/cli/computerd.ts b/packages/computerd/src/cli/computerd.ts index ae83b180..03fed77e 100644 --- a/packages/computerd/src/cli/computerd.ts +++ b/packages/computerd/src/cli/computerd.ts @@ -654,12 +654,16 @@ async function main(): Promise { } if (ignoreConfig.enabled) { console.log( - `[info] MOUNT_IGNORE active: ${ignoreConfig.ignore.paths.length} path(s) ` + - `local-only under ${ignoreConfig.root} (${ignoreConfig.ignore.paths.join(", ")})`, + `[info] MOUNT_IGNORE active: local-only under ${ignoreConfig.root} ` + + `(${ignoreConfig.ignore.patterns.join(", ")})`, ); - if (ignoreConfig.ignore.redundant.length > 0) { + for (const exclusion of ignoreConfig.ignore.ineffectiveExclusions) { + // Same rule as git: a path can't be put back in sync once a + // directory above it is local-only, because the synced side has no + // directory for it to live in. console.log( - `[warn] MOUNT_IGNORE entries dropped as redundant: ${ignoreConfig.ignore.redundant.join(", ")}`, + `[warn] MOUNT_IGNORE exclusion ${exclusion} has no effect: a directory ` + + `above it is already local-only.`, ); } } diff --git a/packages/computerd/src/fuse/ignore-config.test.ts b/packages/computerd/src/fuse/ignore-config.test.ts index 3ddd4d5d..9476764a 100644 --- a/packages/computerd/src/fuse/ignore-config.test.ts +++ b/packages/computerd/src/fuse/ignore-config.test.ts @@ -11,13 +11,13 @@ describe("resolveMountIgnoreConfig: the root", () => { // Under /tmp rather than a tmpfs so a container snapshot captures // it. Snapshots are the only durability local-only content has. expect(defaultIgnoreRoot("/workspace")).toBe("/tmp/workspace"); - const config = resolveMountIgnoreConfig({ MOUNT_IGNORE: "node_modules" }, "/workspace"); + const config = resolveMountIgnoreConfig({ MOUNT_IGNORE: "/node_modules" }, "/workspace"); expect(config.root).toBe("/tmp/workspace"); }); test("honors an explicit MOUNT_IGNORE_PATH", () => { const config = resolveMountIgnoreConfig( - { MOUNT_IGNORE: "node_modules", MOUNT_IGNORE_PATH: "/var/local-only" }, + { MOUNT_IGNORE: "/node_modules", MOUNT_IGNORE_PATH: "/var/local-only" }, "/workspace", ); expect(config.root).toBe("/var/local-only"); @@ -25,7 +25,7 @@ describe("resolveMountIgnoreConfig: the root", () => { test("strips a trailing slash", () => { const config = resolveMountIgnoreConfig( - { MOUNT_IGNORE: "dist", MOUNT_IGNORE_PATH: "/var/local/" }, + { MOUNT_IGNORE: "/dist", MOUNT_IGNORE_PATH: "/var/local/" }, "/workspace", ); expect(config.root).toBe("/var/local"); @@ -34,7 +34,7 @@ describe("resolveMountIgnoreConfig: the root", () => { test("rejects a relative MOUNT_IGNORE_PATH", () => { expect(() => resolveMountIgnoreConfig( - { MOUNT_IGNORE: "dist", MOUNT_IGNORE_PATH: "relative/path" }, + { MOUNT_IGNORE: "/dist", MOUNT_IGNORE_PATH: "relative/path" }, "/workspace", ), ).toThrow(/absolute path/); @@ -45,7 +45,7 @@ describe("resolveMountIgnoreConfig: the root", () => { // an ignored path lands at a location that is also an ignored path. expect(() => resolveMountIgnoreConfig( - { MOUNT_IGNORE: "dist", MOUNT_IGNORE_PATH: "/workspace/.local" }, + { MOUNT_IGNORE: "/dist", MOUNT_IGNORE_PATH: "/workspace/.local" }, "/workspace", ), ).toThrow(/must not be inside MOUNT_POINT/); @@ -54,7 +54,7 @@ describe("resolveMountIgnoreConfig: the root", () => { test("rejects a root equal to the mount point", () => { expect(() => resolveMountIgnoreConfig( - { MOUNT_IGNORE: "dist", MOUNT_IGNORE_PATH: "/workspace" }, + { MOUNT_IGNORE: "/dist", MOUNT_IGNORE_PATH: "/workspace" }, "/workspace", ), ).toThrow(/must not be inside MOUNT_POINT/); @@ -62,14 +62,14 @@ describe("resolveMountIgnoreConfig: the root", () => { test("rejects the filesystem root", () => { expect(() => - resolveMountIgnoreConfig({ MOUNT_IGNORE: "dist", MOUNT_IGNORE_PATH: "/" }, "/workspace"), + resolveMountIgnoreConfig({ MOUNT_IGNORE: "/dist", MOUNT_IGNORE_PATH: "/" }, "/workspace"), ).toThrow(/filesystem root/); }); test("allows a sibling path that merely shares a prefix string", () => { // /workspace-cache is not inside /workspace, despite startsWith. const config = resolveMountIgnoreConfig( - { MOUNT_IGNORE: "dist", MOUNT_IGNORE_PATH: "/workspace-cache" }, + { MOUNT_IGNORE: "/dist", MOUNT_IGNORE_PATH: "/workspace-cache" }, "/workspace", ); expect(config.root).toBe("/workspace-cache"); @@ -94,7 +94,7 @@ describe("resolveMountIgnoreConfig: the set", () => { "/workspace", ); expect(config.enabled).toBe(true); - expect(config.ignore.paths).toEqual(["node_modules", "dist"]); + expect(config.ignore.patterns).toEqual(["/node_modules", "/dist"]); }); test("propagates a bad entry as a startup failure", () => { @@ -105,14 +105,22 @@ describe("resolveMountIgnoreConfig: the set", () => { }); describe("describeMountIgnore", () => { - test("reports the normalized set and the redundant entries", () => { + test("reports the normalized patterns in order, and exclusions that do nothing", () => { const config = resolveMountIgnoreConfig( - { MOUNT_IGNORE: "/node_modules,/node_modules/.cache,/dist" }, + { + MOUNT_IGNORE: "**/node_modules,!**/node_modules/.bin,!/vendor/node_modules,/workspace/dist", + }, "/workspace", ); const info = describeMountIgnore(config); - expect(info.paths).toEqual(["node_modules", "dist"]); - expect(info.redundant).toEqual(["/node_modules/.cache"]); + expect(info.patterns).toEqual([ + "**/node_modules", + "!**/node_modules/.bin", + "!/vendor/node_modules", + "/dist", + ]); + expect(info.ineffectiveExclusions).toEqual(["!**/node_modules/.bin"]); + expect(info).not.toHaveProperty("paths"); expect(info.enabled).toBe(true); expect(info.root).toBe("/tmp/workspace"); }); diff --git a/packages/computerd/src/fuse/ignore-config.ts b/packages/computerd/src/fuse/ignore-config.ts index cee198a4..b84bd0d8 100644 --- a/packages/computerd/src/fuse/ignore-config.ts +++ b/packages/computerd/src/fuse/ignore-config.ts @@ -74,8 +74,10 @@ export interface MountIgnoreInfo { readonly supported: true; readonly enabled: boolean; readonly root: string; - readonly paths: readonly string[]; - readonly redundant: readonly string[]; + // Patterns, not paths: "**/node_modules" can't be joined onto the + // mount point. + readonly patterns: readonly string[]; + readonly ineffectiveExclusions: readonly string[]; readonly fastPaths: { /** * Always false: fuse-native binds libfuse 2.9, passthrough needs the @@ -97,8 +99,8 @@ export function describeMountIgnore(config: MountIgnoreConfig): MountIgnoreInfo supported: true, enabled: config.enabled, root: config.root, - paths: config.ignore.paths, - redundant: config.ignore.redundant, + patterns: config.ignore.patterns, + ineffectiveExclusions: config.ignore.ineffectiveExclusions, fastPaths: { passthrough: false, passthroughReason: PASSTHROUGH_UNAVAILABLE_REASON, diff --git a/packages/computerd/src/fuse/ignore.test.ts b/packages/computerd/src/fuse/ignore.test.ts index 45453b33..b003599b 100644 --- a/packages/computerd/src/fuse/ignore.test.ts +++ b/packages/computerd/src/fuse/ignore.test.ts @@ -2,10 +2,6 @@ import { describe, expect, test } from "vitest"; import { MountIgnorePathError, parseMountIgnore, resolveMountIgnore } from "./ignore.js"; -// A naive `startsWith` passes every other test in this file and fails -// "does not treat node_modules_extra as node_modules", so that test is -// what actually pins the matcher. - describe("parseMountIgnore", () => { test("splits MOUNT_IGNORE on commas", () => { expect(parseMountIgnore("/node_modules,/.venv,/dist")).toEqual([ @@ -34,172 +30,253 @@ describe("parseMountIgnore", () => { }); }); +const MOUNT = "/workspace"; +const set = (patterns: string[]) => resolveMountIgnore(patterns, MOUNT); + describe("resolveMountIgnore: matching", () => { - test("matches the entry itself and everything under it", () => { - const set = resolveMountIgnore(["node_modules"]); - expect(set.ignores("node_modules")).toBe(true); - expect(set.ignores("node_modules/react")).toBe(true); - expect(set.ignores("node_modules/react/index.js")).toBe(true); - expect(set.ignores("node_modules/@scope/pkg/dist/x.js")).toBe(true); + test("an anchored pattern matches its path and everything under it", () => { + const ignore = set(["/node_modules"]); + expect(ignore.ignores("node_modules")).toBe(true); + expect(ignore.ignores("node_modules/react/index.js")).toBe(true); + expect(ignore.ignores("app/node_modules")).toBe(false); + }); + + test("** matches at any depth, including the root", () => { + const ignore = set(["**/node_modules"]); + expect(ignore.ignores("node_modules")).toBe(true); + expect(ignore.ignores("app/node_modules")).toBe(true); + expect(ignore.ignores("a/b/c/d/e/node_modules/x/index.js")).toBe(true); + expect(ignore.ignores("src/main.ts")).toBe(false); }); - test("does not match at arbitrary depth", () => { - // The deliberate limitation. `node_modules` names one location; - // a nested one must be listed explicitly. - const set = resolveMountIgnore(["node_modules"]); - expect(set.ignores("app/node_modules")).toBe(false); - expect(set.ignores("a/b/node_modules")).toBe(false); + test("matches whole segments, so node_modules_extra is not node_modules", () => { + // A naive suffix or prefix test passes most of this file and fails here. + const ignore = set(["**/node_modules", "/dist"]); + expect(ignore.ignores("node_modules_extra")).toBe(false); + expect(ignore.ignores("a/node_modules_extra/x")).toBe(false); + expect(ignore.ignores("a/my_node_modules")).toBe(false); + expect(ignore.ignores("dist2")).toBe(false); }); - test("matches a nested entry when it is listed", () => { - const set = resolveMountIgnore(["app/node_modules", "web/node_modules"]); - expect(set.ignores("app/node_modules")).toBe(true); - expect(set.ignores("app/node_modules/react/index.js")).toBe(true); - expect(set.ignores("web/node_modules")).toBe(true); - expect(set.ignores("api/node_modules")).toBe(false); - expect(set.ignores("node_modules")).toBe(false); + test("* matches within one segment and never crosses /", () => { + const ignore = set(["/packages/*/dist"]); + expect(ignore.ignores("packages/a/dist")).toBe(true); + expect(ignore.ignores("packages/a/dist/index.js")).toBe(true); + expect(ignore.ignores("packages/a/b/dist")).toBe(false); + expect(ignore.ignores("packages/dist")).toBe(false); }); - test("does not treat node_modules_extra as node_modules", () => { - // A plain startsWith check passes everything above and fails here. - const set = resolveMountIgnore(["node_modules"]); - expect(set.ignores("node_modules_extra")).toBe(false); - expect(set.ignores("node_modules_extra/x.js")).toBe(false); - expect(set.ignores("node_modulesX")).toBe(false); + test("* can sit inside a segment", () => { + const ignore = set(["**/*.tsbuildinfo"]); + expect(ignore.ignores("tsconfig.tsbuildinfo")).toBe(true); + expect(ignore.ignores("a/b/x.tsbuildinfo")).toBe(true); + expect(ignore.ignores("a/b/x.tsbuildinfo.bak")).toBe(false); }); - test("does not match a prefix of an entry", () => { - const set = resolveMountIgnore(["build/output"]); - expect(set.ignores("build")).toBe(false); - expect(set.ignores("build/output")).toBe(true); - expect(set.ignores("build/output/app.js")).toBe(true); - expect(set.ignores("build/outputs")).toBe(false); + test("** in the middle matches zero or more directories", () => { + const ignore = set(["/app/**/node_modules"]); + expect(ignore.ignores("app/node_modules")).toBe(true); + expect(ignore.ignores("app/a/b/node_modules")).toBe(true); + expect(ignore.ignores("node_modules")).toBe(false); + expect(ignore.ignores("web/node_modules")).toBe(false); + }); + + test("a trailing /** means the directory itself", () => { + const ignore = set(["/cache/**"]); + expect(ignore.ignores("cache")).toBe(true); + expect(ignore.ignores("cache/a/b")).toBe(true); + expect(ignore.ignores("cached")).toBe(false); + }); + + test("treats regular expression characters in a pattern literally", () => { + const ignore = set(["**/a.b+c(d)"]); + expect(ignore.ignores("x/a.b+c(d)")).toBe(true); + expect(ignore.ignores("x/aXb+c(d)")).toBe(false); }); test("matches case-sensitively, as Linux does", () => { - const set = resolveMountIgnore(["node_modules"]); - expect(set.ignores("node_modules")).toBe(true); - expect(set.ignores("Node_Modules")).toBe(false); + expect(set(["**/node_modules"]).ignores("Node_Modules")).toBe(false); }); test("tolerates leading and trailing slashes on the queried path", () => { - const set = resolveMountIgnore(["dist"]); - expect(set.ignores("/dist")).toBe(true); - expect(set.ignores("dist/")).toBe(true); - expect(set.ignores("/dist/app.js")).toBe(true); + const ignore = set(["/dist"]); + expect(ignore.ignores("/dist/")).toBe(true); + expect(ignore.ignores("")).toBe(false); + expect(ignore.ignores("/")).toBe(false); + }); + + test("ignores nothing when no patterns are configured", () => { + const ignore = set([]); + expect(ignore.isEmpty).toBe(true); + expect(ignore.ignores("node_modules")).toBe(false); + }); +}); + +describe("resolveMountIgnore: exclusions", () => { + test("an exclusion at the level of the match keeps that path synced", () => { + const ignore = set(["**/node_modules", "!/vendor/node_modules"]); + expect(ignore.ignores("vendor/node_modules")).toBe(false); + expect(ignore.ignores("vendor/node_modules/pkg/index.js")).toBe(false); + expect(ignore.ignores("app/node_modules")).toBe(true); + expect(ignore.ignores("node_modules")).toBe(true); }); - test("ignores nothing when no entries are configured", () => { - const set = resolveMountIgnore([]); - expect(set.ignores("node_modules")).toBe(false); - expect(set.isEmpty).toBe(true); - expect(set.paths).toEqual([]); + test("an exclusion under a local-only directory has no effect", () => { + // The parent lives on local disk, so the synced filesystem has no + // directory for the excluded child to live in. Same rule as git. + const ignore = set(["**/node_modules", "!**/node_modules/.bin"]); + expect(ignore.ignores("a/node_modules/.bin")).toBe(true); + expect(ignore.ignores("a/node_modules/.bin/tool")).toBe(true); }); - test("reports the covering entry, for diagnostics and error messages", () => { - const set = resolveMountIgnore(["node_modules", "target"]); - expect(set.entryFor("node_modules/react/index.js")).toBe("node_modules"); - expect(set.entryFor("target/debug/app")).toBe("target"); - expect(set.entryFor("src/main.ts")).toBeUndefined(); + test("the last matching pattern wins", () => { + const ignore = set(["**/build", "!/tools/build", "/tools/build/out"]); + expect(ignore.ignores("app/build")).toBe(true); + expect(ignore.ignores("tools/build")).toBe(false); + expect(ignore.ignores("tools/build/src.ts")).toBe(false); + expect(ignore.ignores("tools/build/out/a.js")).toBe(true); + }); + + test("an ignore after an exclusion wins again", () => { + const ignore = set(["!/dist", "/dist"]); + expect(ignore.ignores("dist")).toBe(true); + }); + + test("exclusions alone ignore nothing", () => { + const ignore = set(["!/vendor/node_modules"]); + expect(ignore.isEmpty).toBe(true); + expect(ignore.ignores("vendor/node_modules")).toBe(false); + }); + + test("checks a wildcard exclusion against what its wildcard could match", () => { + const ignore = set(["/build-*", "!/build-*/keep"]); + expect(ignore.ineffectiveExclusions).toEqual(["!/build-*/keep"]); + }); + + test("reports exclusions that cannot take effect", () => { + const ignore = set([ + "**/node_modules", + "!**/node_modules/.bin", + "!/app/node_modules/keep", + "!/vendor/node_modules", + ]); + expect(ignore.ineffectiveExclusions).toEqual([ + "!**/node_modules/.bin", + "!/app/node_modules/keep", + ]); }); }); describe("resolveMountIgnore: normalization", () => { - test("strips leading and trailing slashes from entries", () => { - const set = resolveMountIgnore(["/dist/", "node_modules/"]); - expect(set.paths).toEqual(["dist", "node_modules"]); - expect(set.ignores("dist/app.js")).toBe(true); + test("keeps patterns in the order written", () => { + // Order changes the meaning once exclusions are involved. + expect(set(["**/node_modules", "!/vendor/node_modules", "/dist"]).patterns).toEqual([ + "**/node_modules", + "!/vendor/node_modules", + "/dist", + ]); }); - test("accepts an absolute path inside the mount point", () => { - const set = resolveMountIgnore(["/workspace/dist"], "/workspace"); - expect(set.paths).toEqual(["dist"]); - expect(set.ignores("dist/app.js")).toBe(true); + test("strips the mount point from a fully-qualified pattern", () => { + expect(set(["/workspace/dist", "!/workspace/vendor/node_modules"]).patterns).toEqual([ + "/dist", + "!/vendor/node_modules", + ]); }); - test("anchors a leading slash at the mount root, not the filesystem root", () => { - // "/node_modules" means $MOUNT_POINT/node_modules. A path that looks - // like it names somewhere else on disk is still mount-relative, so - // the entry set can never reach outside the mount. - const set = resolveMountIgnore(["/etc/passwd"], "/workspace"); - expect(set.paths).toEqual(["etc/passwd"]); - expect(set.ignores("etc/passwd")).toBe(true); + test("does not strip a prefix that only looks like the mount point", () => { + expect(set(["/workspacefoo"]).patterns).toEqual(["/workspacefoo"]); }); - test("accepts the fully-qualified form of the same path", () => { - const set = resolveMountIgnore(["/workspace/dist", "/dist"], "/workspace"); - expect(set.paths).toEqual(["dist"]); + test("strips trailing slashes", () => { + expect(set(["/dist/", "**/node_modules/"]).patterns).toEqual(["/dist", "**/node_modules"]); }); - test("rejects a .. segment rather than resolving it", () => { - // Silently clamping would hide the mistake behind a path that looks - // intentional. - expect(() => resolveMountIgnore(["../escape"])).toThrow(MountIgnorePathError); - expect(() => resolveMountIgnore(["dist/../../etc"])).toThrow(/"\." or "\.\."/); + test("writes a leading /** as **", () => { + expect(set(["/**/node_modules"]).patterns).toEqual(["**/node_modules"]); }); - test("rejects a . segment", () => { - expect(() => resolveMountIgnore(["./dist"])).toThrow(MountIgnorePathError); + test("leaves the mount point alone when it is /", () => { + const ignore = resolveMountIgnore(["/workspace/dist"], "/"); + expect(ignore.patterns).toEqual(["/workspace/dist"]); + expect(ignore.ignores("workspace/dist")).toBe(true); }); +}); - test("rejects an entry naming the mount root", () => { - // Ignoring everything would make the workspace entirely non-durable, - // which is never what someone means. - expect(() => resolveMountIgnore(["/"])).toThrow(MountIgnorePathError); - expect(() => resolveMountIgnore([""])).toThrow(MountIgnorePathError); +describe("resolveMountIgnore: rejected patterns", () => { + const rejects = (pattern: string, message: RegExp) => { + expect(() => set([pattern])).toThrow(MountIgnorePathError); + expect(() => set([pattern])).toThrow(message); + }; + + test("requires every pattern to start with / or **/", () => { + // node_modules alone means root-only here and any depth in gitignore. + // Rejecting it means nobody has to remember which. + rejects("node_modules", /must start with "\/" .* or "\*\*\/"/); + rejects("*/dist", /must start with/); + rejects("!vendor/node_modules", /must start with/); }); - test("rejects an empty path segment", () => { - expect(() => resolveMountIgnore(["a//b"])).toThrow(MountIgnorePathError); + test("suggests both anchored spellings", () => { + rejects("node_modules", /"\/node_modules" or "\*\*\/node_modules"/); }); - test("reports the entry index so a long MOUNT_IGNORE is diagnosable", () => { - try { - resolveMountIgnore(["ok", "also-ok", "../bad"]); - expect.unreachable("resolve should have thrown"); - } catch (error) { - expect(error).toBeInstanceOf(MountIgnorePathError); - expect((error as MountIgnorePathError).index).toBe(2); - expect((error as MountIgnorePathError).entry).toBe("../bad"); + test("requires ** to be a whole segment", () => { + rejects("**node_modules", /must start with/); + rejects("/a**/b", /"\*\*" must be a whole path segment/); + rejects("/a/b**", /"\*\*" must be a whole path segment/); + }); + + test("rejects patterns that would make the whole mount local-only", () => { + for (const pattern of ["/", "**", "/**", "**/**", "/workspace", "/workspace/**", "!/**"]) { + rejects(pattern, /whole mount/); } }); -}); -describe("resolveMountIgnore: redundancy", () => { - test("drops a duplicate entry", () => { - const set = resolveMountIgnore(["dist", "dist"]); - expect(set.paths).toEqual(["dist"]); - expect(set.redundant).toEqual(["dist"]); + test("rejects wildcards that match every path at some depth", () => { + // "/*" matches every top-level entry, and everything under a + // local-only directory is local-only, so nothing would sync. + for (const pattern of ["/*", "/**/*", "**/*", "/*/*", "/*/**", "!/*"]) { + rejects(pattern, /whole mount/); + } + }); + + test("still accepts targeted wildcards", () => { + expect(() => set(["/*.log", "/packages/*/dist", "**/*.tsbuildinfo", "/build-*"])).not.toThrow(); + }); + + test("rejects . and .. segments", () => { + rejects("/a/../b", /"\." or "\.\." segment/); + rejects("/./a", /"\." or "\.\." segment/); + rejects("**/..", /"\." or "\.\." segment/); + }); + + test("rejects an empty segment", () => { + rejects("/a//b", /empty path segment/); }); - test("drops an entry nested inside an earlier one", () => { - // Keeping node_modules/.cache alongside node_modules would imply it - // does something, and it cannot. - const set = resolveMountIgnore(["node_modules", "node_modules/.cache"]); - expect(set.paths).toEqual(["node_modules"]); - expect(set.redundant).toEqual(["node_modules/.cache"]); - expect(set.ignores("node_modules/.cache/x")).toBe(true); + test("rejects syntax that is not supported, rather than matching it literally", () => { + for (const pattern of ["/*.{js,ts}", "/[ab]", "/a?", "/a\\*"]) { + rejects(pattern, /not supported/); + } }); - test("subsumes earlier entries when a broader one arrives later", () => { - const set = resolveMountIgnore(["app/node_modules", "app"]); - expect(set.paths).toEqual(["app"]); - expect(set.redundant).toEqual(["app/node_modules"]); - expect(set.ignores("app/node_modules/react")).toBe(true); - expect(set.ignores("app/src/main.ts")).toBe(true); + test("rejects an empty exclusion", () => { + rejects("!", /must start with/); }); - test("keeps siblings that merely share a prefix string", () => { - // `dist` and `dist-types` are unrelated locations despite the - // common prefix; neither is redundant. - const set = resolveMountIgnore(["dist", "dist-types"]); - expect(set.paths).toEqual(["dist", "dist-types"]); - expect(set.redundant).toEqual([]); + test("bounds pattern length", () => { + rejects(`/${"a".repeat(5000)}`, /longer than 4096/); }); - test("normalizes before deduplicating", () => { - const set = resolveMountIgnore(["/dist/", "dist"]); - expect(set.paths).toEqual(["dist"]); - expect(set.redundant).toEqual(["dist"]); + test("reports the index so a long MOUNT_IGNORE is diagnosable", () => { + try { + set(["/dist", "/ok", "bad"]); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(MountIgnorePathError); + expect((error as MountIgnorePathError).index).toBe(2); + expect((error as MountIgnorePathError).entry).toBe("bad"); + } }); }); diff --git a/packages/computerd/src/fuse/ignore.ts b/packages/computerd/src/fuse/ignore.ts index 36c7a7ce..17479865 100644 --- a/packages/computerd/src/fuse/ignore.ts +++ b/packages/computerd/src/fuse/ignore.ts @@ -1,13 +1,18 @@ -// Local-only subpaths of the mount. See packages/computerd/README.md. +// Local-only paths of the mount, as glob patterns. See +// packages/computerd/README.md. // -// Entries are plain paths relative to the mount root: no glob syntax -// and no negation. Deliberate, because an entry then resolves to a -// known location and the mapping onto MOUNT_IGNORE_PATH is a prefix -// substitution decided at startup, which an unanchored pattern cannot -// answer until a path arrives to match against it. +// A small subset of gitignore: `*` within a path segment, `**` for any +// number of segments, and `!` to exclude. Every pattern is anchored: it +// starts with "/" (from the mount root) or "**/" (at any depth). The +// last matching pattern wins, and a path is local-only if it or any +// directory above it is ignored, which is git's rule. So an exclusion +// can only put back a path whose parent is still synced. // -// The set is resolved once at startup and never re-read: entries that -// changed under a running command would mean migrating +// A local-only path is stored at the same relative path under +// MOUNT_IGNORE_PATH, whichever pattern matched it. +// +// The patterns are resolved once at startup and never re-read: patterns +// that changed under a running command would mean migrating // already-materialized paths between layers mid-write. /** An entry that cannot be used, carrying enough context to fix it. */ @@ -24,14 +29,13 @@ export class MountIgnorePathError extends Error { } export interface MountIgnoreSet { - /** Segment-aware: `node_modules` does not match `node_modules_extra`. */ + /** Whether a mount-relative path is local-only. Never touches disk. */ readonly ignores: (relativePath: string) => boolean; - /** The entry covering a path, or undefined when not local-only. */ - readonly entryFor: (relativePath: string) => string | undefined; - /** Normalized entries, in declaration order, as the mount applies them. */ - readonly paths: readonly string[]; - /** Entries dropped as duplicates or as nested inside another entry. */ - readonly redundant: readonly string[]; + /** Normalized patterns, in the order written. Order matters. */ + readonly patterns: readonly string[]; + /** Exclusions under a local-only directory, which therefore do nothing. */ + readonly ineffectiveExclusions: readonly string[]; + /** True when no pattern ignores anything. */ readonly isEmpty: boolean; } @@ -50,96 +54,159 @@ export function parseMountIgnore(raw: string | undefined): string[] { return entries; } +const MAX_PATTERN_LENGTH = 4096; +// Reserved rather than treated as literals, so nobody writes {js,ts} +// believing braces work. A comma never reaches here: it separates +// patterns in MOUNT_IGNORE. +const UNSUPPORTED = /[{}[\]?\\]/; + +type Segment = + | { kind: "literal"; value: string } + | { kind: "wildcard"; regex: RegExp; sample: string } + | { kind: "globstar" }; + +interface CompiledPattern { + readonly source: string; + readonly exclude: boolean; + readonly segments: readonly Segment[]; +} + /** - * Normalizes entries and builds the matcher. An absolute path outside - * the mount is rejected rather than reinterpreted. + * Validates and compiles the patterns. A pattern written with the mount + * point ("/workspace/dist") means the same as one without ("/dist"). */ export function resolveMountIgnore(entries: readonly string[], mountPoint = "/"): MountIgnoreSet { const root = normalizeMount(mountPoint); - const paths: string[] = []; - const redundant: string[] = []; + const compiled = entries.map((entry, index) => compilePattern(entry, index, root)); - for (const [index, original] of entries.entries()) { - let value = original.trim(); + const isEmpty = !compiled.some((pattern) => !pattern.exclude); - // A leading slash anchors the entry at the mount root, not at the - // filesystem root: "/node_modules" means "$MOUNT_POINT/node_modules". - if (value.startsWith("/") && root !== "/") { - if (value === root || value.startsWith(`${root}/`)) { - value = value.slice(root.length); + // Walk the path's directories from the root. The first one whose last + // matching pattern ignores it makes the whole path local-only. + const ignores = (relativePath: string): boolean => { + if (isEmpty) return false; + const path = stripSlashes(relativePath); + if (path === "") return false; + const segments = path.split("/"); + for (let depth = 1; depth <= segments.length; depth += 1) { + const prefix = segments.slice(0, depth); + let ignored = false; + for (const pattern of compiled) { + if (matchSegments(pattern.segments, 0, prefix, 0)) ignored = !pattern.exclude; } + if (ignored) return true; } + return false; + }; - const trimmed = stripSlashes(value); - if (trimmed === "") { - throw new MountIgnorePathError( - `Entry ${JSON.stringify(original)} resolves to the mount root. ` + - `Ignoring the whole mount would make the workspace non-durable.`, - original, - index, - ); - } + return { + ignores, + isEmpty, + patterns: compiled.map((pattern) => pattern.source), + ineffectiveExclusions: compiled + .filter((pattern) => pattern.exclude && isShadowed(pattern, ignores)) + .map((pattern) => pattern.source), + }; +} - const segments = trimmed.split("/"); - // Rejected rather than resolved: silently clamping an entry that walks - // out of the mount would hide the mistake behind a plausible path. - if (segments.some((segment) => segment === "." || segment === "..")) { - throw new MountIgnorePathError( - `Entry ${JSON.stringify(original)} contains a "." or ".." segment. ` + - `Entries must be plain paths relative to the mount root.`, - original, - index, - ); - } - if (segments.some((segment) => segment === "")) { - throw new MountIgnorePathError( - `Entry ${JSON.stringify(original)} contains an empty path segment.`, - original, - index, - ); - } +function compilePattern(entry: string, index: number, root: string): CompiledPattern { + const original = entry.trim(); + const fail = (reason: string): never => { + throw new MountIgnorePathError( + `MOUNT_IGNORE pattern ${JSON.stringify(original)} ${reason}`, + entry, + index, + ); + }; - // Keeping `node_modules/.cache` alongside `node_modules` would imply - // it does something, and it cannot. - const covered = paths.some((existing) => isAtOrUnder(trimmed, existing)); - if (covered) { - redundant.push(original); - continue; - } + if (original.length > MAX_PATTERN_LENGTH) { + fail(`is longer than ${MAX_PATTERN_LENGTH} characters.`); + } + const exclude = original.startsWith("!"); + let body = exclude ? original.slice(1) : original; + + if (!body.startsWith("/") && !body.startsWith("**/") && body !== "**") { + const bare = stripSlashes(body.replace(/^\*\*(?=[^/])/, "")) || "path"; + fail( + `must start with "/" (from the mount root) or "**/" (at any depth), ` + + `for example "/${bare}" or "**/${bare}".`, + ); + } + if (UNSUPPORTED.test(body)) { + fail(`uses syntax that is not supported. Only "*", "**", and a leading "!" are.`); + } - // The converse: a new entry may subsume ones already accepted. - for (let position = paths.length - 1; position >= 0; position -= 1) { - const existing = paths[position] as string; - if (isAtOrUnder(existing, trimmed)) { - redundant.push(existing); - paths.splice(position, 1); - } - } + // A leading slash anchors at the mount root, so the mount point itself + // is optional: "/workspace/dist" is "/dist". + if (root !== "/" && (body === root || body.startsWith(`${root}/`))) { + body = body.slice(root.length) || "/"; + } - paths.push(trimmed); + const trimmed = stripSlashes(body); + const parts = trimmed === "" ? [] : trimmed.split("/"); + if (parts.some((part) => part === "")) fail("contains an empty path segment."); + if (parts.some((part) => part === "." || part === "..")) { + fail(`contains a "." or ".." segment. Patterns name paths under the mount root.`); + } + if (parts.some((part) => part !== "**" && part.includes("**"))) { + fail(`uses "**" inside a segment. "**" must be a whole path segment.`); + } + // Only "*" and "**" segments match every path at some depth, and + // everything under a local-only directory is local-only: "/*" alone + // would keep the whole workspace off the Durable Object. + if (parts.every((part) => part === "**" || part === "*")) { + fail("would make the whole mount local-only, so nothing would be synced."); } - const isEmpty = paths.length === 0; + const segments = parts.map(compileSegment); + const canonical = parts[0] === "**" ? parts.join("/") : `/${parts.join("/")}`; + return { source: `${exclude ? "!" : ""}${canonical}`, exclude, segments }; +} - const entryFor = (relativePath: string): string | undefined => { - if (isEmpty) return undefined; - const path = stripSlashes(relativePath); - if (path === "") return undefined; - return paths.find((entry) => isAtOrUnder(path, entry)); - }; +function compileSegment(part: string): Segment { + if (part === "**") return { kind: "globstar" }; + if (!part.includes("*")) return { kind: "literal", value: part }; + const source = part + .split("*") + .map((piece) => piece.replace(/[.+^$|()\\/]/g, "\\$&")) + .join("[^/]*"); + return { kind: "wildcard", regex: new RegExp(`^${source}$`), sample: part.replaceAll("*", "x") }; +} - return { - paths, - redundant, - isEmpty, - entryFor, - ignores: (relativePath) => entryFor(relativePath) !== undefined, - }; +/** Whether `pattern[p..]` matches exactly `path[i..]`. */ +function matchSegments( + pattern: readonly Segment[], + p: number, + path: readonly string[], + i: number, +): boolean { + if (p === pattern.length) return i === path.length; + const segment = pattern[p] as Segment; + if (segment.kind === "globstar") { + // Zero or more segments: try every split. + for (let skip = i; skip <= path.length; skip += 1) { + if (matchSegments(pattern, p + 1, path, skip)) return true; + } + return false; + } + if (i === path.length) return false; + const name = path[i] as string; + const ok = segment.kind === "literal" ? name === segment.value : segment.regex.test(name); + return ok && matchSegments(pattern, p + 1, path, i + 1); } -/** The separator check is what stops `node_modules_extra` matching. */ -function isAtOrUnder(path: string, entry: string): boolean { - return path === entry || path.startsWith(`${entry}/`); +// An exclusion does nothing if a directory above what it names is +// local-only. Checked on one sample path, with "**" dropped and each "*" +// filled in, which is enough to catch "!**/node_modules/.bin" without +// any glob algebra. +function isShadowed(pattern: CompiledPattern, ignores: (path: string) => boolean): boolean { + const sample = pattern.segments + .filter((segment) => segment.kind !== "globstar") + .map((segment) => (segment.kind === "literal" ? segment.value : segment.sample)); + for (let depth = 1; depth < sample.length; depth += 1) { + if (ignores(sample.slice(0, depth).join("/"))) return true; + } + return false; } function stripSlashes(value: string): string { diff --git a/packages/computerd/src/fuse/passthrough.test.ts b/packages/computerd/src/fuse/passthrough.test.ts index c930c418..212f55d3 100644 --- a/packages/computerd/src/fuse/passthrough.test.ts +++ b/packages/computerd/src/fuse/passthrough.test.ts @@ -94,7 +94,7 @@ describe("withLocalPassthrough: routing", () => { }; test("creates and reads a file on local disk, never touching the VFS", () => { - const { ops, calls } = build(["node_modules"]); + const { ops, calls } = build(["/node_modules"]); let fh = 0; ops.create("/node_modules/pkg/index.js", 0o644, (code, handle) => { @@ -127,13 +127,13 @@ describe("withLocalPassthrough: routing", () => { }); test("creates missing parent directories on first write", () => { - const { ops } = build(["node_modules"]); + const { ops } = build(["/node_modules"]); ops.create("/node_modules/a/b/c/deep.js", 0o644, (code) => expect(code).toBe(0)); expect(readFileSync(join(root, "node_modules/a/b/c/deep.js"), "utf8")).toBe(""); }); test("passes non-ignored paths straight through to the VFS", () => { - const { ops, calls } = build(["node_modules"]); + const { ops, calls } = build(["/node_modules"]); ops.getattr("/src/main.ts", () => {}); ops.create("/src/new.ts", 0o644, () => {}); ops.unlink("/src/old.ts", () => {}); @@ -141,13 +141,13 @@ describe("withLocalPassthrough: routing", () => { }); test("does not route a path that merely shares a prefix", () => { - const { ops, calls } = build(["node_modules"]); + const { ops, calls } = build(["/node_modules"]); ops.getattr("/node_modules_extra/x.js", () => {}); expect(calls).toEqual(["getattr"]); }); test("routes by handle, so a VFS handle is never served locally", () => { - const { ops, calls } = build(["node_modules"]); + const { ops, calls } = build(["/node_modules"]); const buffer = Buffer.alloc(8); // 7 is what the recording VFS hands out; it must stay with the VFS. ops.read("/src/main.ts", 7, buffer, 8, 0, () => {}); @@ -155,7 +155,7 @@ describe("withLocalPassthrough: routing", () => { }); test("reports EBADF for an unknown local handle rather than guessing", () => { - const { ops } = build(["node_modules"]); + const { ops } = build(["/node_modules"]); const buffer = Buffer.alloc(8); let code = 0; ops.read("/node_modules/x.js", 0x4000_0000 + 999, buffer, 8, 0, (result) => { @@ -179,7 +179,7 @@ describe("withLocalPassthrough: deciding paths", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, }); ops.create("/node_modules/a/b/c/d/e/f.js", 0o644, (code) => expect(code).toBe(0)); @@ -193,7 +193,7 @@ describe("withLocalPassthrough: deciding paths", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, }); ops.mkdir("/node_modules", 0o755, () => {}); @@ -218,7 +218,7 @@ describe("withLocalPassthrough: deciding paths", () => { }); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, fs: counting, }); @@ -252,7 +252,7 @@ describe("withLocalPassthrough: rename", () => { }; test("renames within the local layer", () => { - const { ops } = build(["node_modules"]); + const { ops } = build(["/node_modules"]); ops.create("/node_modules/.staging", 0o644, () => {}); let code = -1; ops.rename("/node_modules/.staging", "/node_modules/final", (result) => { @@ -263,7 +263,7 @@ describe("withLocalPassthrough: rename", () => { }); test("delegates a rename entirely within the VFS", () => { - const { ops, calls } = build(["node_modules"]); + const { ops, calls } = build(["/node_modules"]); ops.rename("/src/a.ts", "/src/b.ts", () => {}); expect(calls).toEqual(["rename"]); }); @@ -278,7 +278,7 @@ describe("withLocalPassthrough: rename", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["dist"], MOUNT), + ignore: resolveMountIgnore(["/dist"], MOUNT), mountPoint: MOUNT, warn: (message) => warnings.push(message), }); @@ -307,7 +307,7 @@ describe("withLocalPassthrough: rename", () => { const source = recordingOps(); const { ops, stats } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["dist"], MOUNT), + ignore: resolveMountIgnore(["/dist"], MOUNT), mountPoint: MOUNT, warn: (message) => warnings.push(message), }); @@ -323,7 +323,7 @@ describe("withLocalPassthrough: rename", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["dist"], MOUNT), + ignore: resolveMountIgnore(["/dist"], MOUNT), mountPoint: MOUNT, warn: (message) => warnings.push(message), }); @@ -340,7 +340,7 @@ describe("withLocalPassthrough: rename", () => { // mid-copy into a half-written file where the caller was promised // all-or-nothing. EXDEV is what rename(2) returns between any two // filesystems. - const { ops, calls } = build(["dist"]); + const { ops, calls } = build(["/dist"]); let intoLocal = 0; ops.rename("/.tmp-build", "/dist", (code) => { @@ -374,7 +374,7 @@ describe("withLocalPassthrough: directory listing", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, }); @@ -396,7 +396,7 @@ describe("withLocalPassthrough: directory listing", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, }); @@ -413,7 +413,7 @@ describe("withLocalPassthrough: directory listing", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, }); @@ -446,7 +446,7 @@ describe("withLocalPassthrough: symlinks", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, }); @@ -469,7 +469,7 @@ describe("withLocalPassthrough: symlinks", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, }); ops.symlink("node_modules/pkg", "/src/link", () => {}); @@ -491,7 +491,7 @@ describe("withLocalPassthrough: errors", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, }); return ops; @@ -547,7 +547,7 @@ describe("withLocalPassthrough: descriptor and metadata operations", () => { const source = recordingOps(); const { ops } = withLocalPassthrough(source.ops, { root, - ignore: resolveMountIgnore(["node_modules"], MOUNT), + ignore: resolveMountIgnore(["/node_modules"], MOUNT), mountPoint: MOUNT, ...(fs === undefined ? {} : { fs: { ...realFs(), ...fs } }), }); @@ -681,3 +681,169 @@ describe("withLocalPassthrough: descriptor and metadata operations", () => { expect(changed).toEqual(["lchown"]); }); }); + +describe("withLocalPassthrough: synced directories that hold local-only paths", () => { + let root: string; + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), "computerd-passthrough-")); + }); + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); + + const build = (patterns = ["**/node_modules"]) => { + const source = recordingOps(); + const warnings: string[] = []; + const { ops } = withLocalPassthrough(source.ops, { + root, + ignore: resolveMountIgnore(patterns, MOUNT), + mountPoint: MOUNT, + warn: (message) => warnings.push(message), + }); + return { ops, calls: source.calls, warnings }; + }; + + const list = (ops: FuseOps, path: string): string[] => { + let names: string[] = []; + ops.readdir(path, (code, result) => { + expect(code).toBe(0); + names = result as string[]; + }); + return names; + }; + + const status = (run: (cb: (code: number) => void) => void): number => { + let result = 1; + run((code) => { + result = code; + }); + return result; + }; + + test("lists a local-only child of a synced directory, at any depth", () => { + const { ops } = build(); + mkdirSync(join(root, "packages/app/node_modules"), { recursive: true }); + expect(list(ops, "/packages/app")).toEqual(["vfs-entry", "node_modules"]); + }); + + test("does not list the scaffolding that holds a local-only path", () => { + // packages/ and packages/app/ exist under the local root only so + // node_modules has somewhere to live. They belong to the synced side. + const { ops } = build(); + mkdirSync(join(root, "packages/app/node_modules"), { recursive: true }); + expect(list(ops, "/")).toEqual(["vfs-entry"]); + expect(list(ops, "/packages")).toEqual(["vfs-entry"]); + }); + + test("lists nothing extra when the local side has no such directory", () => { + const { ops } = build(); + expect(list(ops, "/src")).toEqual(["vfs-entry"]); + }); + + test("respects exclusions when listing", () => { + const { ops } = build(["**/node_modules", "!/vendor/node_modules"]); + mkdirSync(join(root, "vendor/node_modules"), { recursive: true }); + expect(list(ops, "/vendor")).toEqual(["vfs-entry"]); + }); + + test("moves local-only contents when a synced directory is renamed", () => { + // Otherwise packages/foo/node_modules stays at the old path on disk, + // unreachable, and the new path has none. + const { ops, calls } = build(); + mkdirSync(join(root, "packages/foo/node_modules/pkg"), { recursive: true }); + writeFileSync(join(root, "packages/foo/node_modules/pkg/index.js"), "x"); + + expect(status((cb) => ops.rename("/packages/foo", "/apps/bar", cb))).toBe(0); + + expect(calls).toEqual(["rename"]); + expect(readFileSync(join(root, "apps/bar/node_modules/pkg/index.js"), "utf8")).toBe("x"); + expect(nodeFs.existsSync(join(root, "packages/foo"))).toBe(false); + }); + + test("a synced rename succeeds even if the local move fails, and says so", () => { + const source = recordingOps(); + const warnings: string[] = []; + const { ops } = withLocalPassthrough(source.ops, { + root, + ignore: resolveMountIgnore(["**/node_modules"], MOUNT), + mountPoint: MOUNT, + warn: (message) => warnings.push(message), + fs: { + ...realFs(), + renameSync: () => { + throw Object.assign(new Error("permission denied"), { code: "EACCES" }); + }, + }, + }); + mkdirSync(join(root, "packages/foo/node_modules"), { recursive: true }); + + expect(status((cb) => ops.rename("/packages/foo", "/packages/bar", cb))).toBe(0); + expect(warnings.join("\n")).toMatch(/packages\/foo.*packages\/bar/); + }); + + test("renaming onto a synced directory that holds local-only contents is ENOTEMPTY", () => { + // The synced side can't see b/node_modules, so it would let the + // rename replace b, and the local move onto the occupied b would + // then fail, stranding a/node_modules at a path the mount no + // longer shows. In the merged view b isn't empty, so refuse. + const { ops, calls } = build(); + mkdirSync(join(root, "a/node_modules/pkg"), { recursive: true }); + mkdirSync(join(root, "b/node_modules/other"), { recursive: true }); + + expect(status((cb) => ops.rename("/a", "/b", cb))).toBe(-39); + + expect(calls).toEqual([]); + expect(nodeFs.existsSync(join(root, "a/node_modules/pkg"))).toBe(true); + expect(nodeFs.existsSync(join(root, "b/node_modules/other"))).toBe(true); + }); + + test("renaming onto a synced directory with only empty scaffolding moves the contents", () => { + const { ops, calls } = build(); + mkdirSync(join(root, "a/node_modules/pkg"), { recursive: true }); + mkdirSync(join(root, "b"), { recursive: true }); + + expect(status((cb) => ops.rename("/a", "/b", cb))).toBe(0); + + expect(calls).toEqual(["rename"]); + expect(nodeFs.existsSync(join(root, "b/node_modules/pkg"))).toBe(true); + expect(nodeFs.existsSync(join(root, "a"))).toBe(false); + }); + + test("rmdir of a synced directory also removes its empty scaffolding", () => { + const { ops, calls } = build(); + mkdirSync(join(root, "app"), { recursive: true }); + + expect(status((cb) => ops.rmdir("/app", cb))).toBe(0); + + expect(calls).toEqual(["rmdir"]); + expect(nodeFs.existsSync(join(root, "app"))).toBe(false); + }); + + test("rmdir of a synced directory with local-only contents is ENOTEMPTY", () => { + // The synced side thinks app is empty because it never sees + // node_modules. Removing it would orphan node_modules on disk. + const { ops, calls } = build(); + mkdirSync(join(root, "app/node_modules"), { recursive: true }); + + expect(status((cb) => ops.rmdir("/app", cb))).toBe(-39); + + expect(calls).toEqual([]); + expect(nodeFs.existsSync(join(root, "app/node_modules"))).toBe(true); + }); + + test("a rename within node_modules stays local when ** decides the boundary", () => { + const { ops, calls } = build(); + mkdirSync(join(root, "app/node_modules/.staging"), { recursive: true }); + expect( + status((cb) => ops.rename("/app/node_modules/.staging", "/app/node_modules/pkg", cb)), + ).toBe(0); + expect(calls).toEqual([]); + }); + + test("renaming node_modules itself to a synced name is EXDEV", () => { + const { ops } = build(); + mkdirSync(join(root, "app/node_modules"), { recursive: true }); + expect(status((cb) => ops.rename("/app/node_modules", "/app/node_modules2", cb))).toBe(-18); + }); +}); diff --git a/packages/computerd/src/fuse/passthrough.ts b/packages/computerd/src/fuse/passthrough.ts index faa060e4..3177a4d2 100644 --- a/packages/computerd/src/fuse/passthrough.ts +++ b/packages/computerd/src/fuse/passthrough.ts @@ -46,7 +46,7 @@ import { unlinkSync, writeSync, } from "node:fs"; -import { dirname, join, posix } from "node:path"; +import { dirname, join } from "node:path"; import type { FuseOps, FuseStat } from "./driver.js"; import type { MountIgnoreSet } from "./ignore.js"; @@ -509,7 +509,26 @@ export function withLocalPassthrough( rmdir(path, cb) { if (!isLocal(path)) { - ops.rmdir(path, cb); + // The synced side never sees local-only children, so it would + // happily remove a directory that still holds node_modules on + // disk. Check the local side first, and clean up its scaffolding + // once the synced directory is gone. + const scaffolding = localPath(path); + if (hasLocalContents(path)) { + cb(ERRNO.ENOTEMPTY); + return; + } + ops.rmdir(path, (code) => { + if (code === 0) { + try { + fs.rmdirSync(scaffolding); + } catch { + // Absent, or something was created in between; either way + // the synced removal stands. + } + } + cb(code); + }); return; } localOps += 1; @@ -526,7 +545,19 @@ export function withLocalPassthrough( const destinationLocal = isLocal(destination); if (!sourceLocal && !destinationLocal) { - ops.rename(source, destination, cb); + // The synced side can't see the destination's local-only + // children, so it would let this replace a directory that still + // holds node_modules on disk. The local move onto it would then + // fail and strand the source's contents. In the merged view the + // destination isn't empty, which is ENOTEMPTY for rename(2). + if (hasLocalContents(destination)) { + cb(ERRNO.ENOTEMPTY); + return; + } + ops.rename(source, destination, (code) => { + if (code === 0) moveLocalContents(source, destination); + cb(code); + }); return; } @@ -711,24 +742,53 @@ export function withLocalPassthrough( ); } + // Local-only children of a synced directory. The matching directory + // under the local root also holds scaffolding (the parents a local-only + // path needs on disk), so only entries that are themselves local-only + // are listed. Almost always ENOENT: most synced directories have no + // local-only children. function localChildren(path: string): string[] { - const relative = toRelative(path, mountRoot); - const names: string[] = []; - for (const entry of options.ignore.paths) { - const parent = posix.dirname(entry); - const normalizedParent = parent === "." ? "" : parent; - if (normalizedParent !== relative) continue; - // Only list it if it has actually been created on disk. An - // unconfigured-but-unused entry should not appear as a phantom - // directory in a listing. - try { - fs.lstatSync(join(root, entry)); - names.push(posix.basename(entry)); - } catch { - // Not materialized yet; nothing to show. - } + let names: string[]; + try { + names = fs.readdirSync(localPath(path)) as string[]; + } catch { + return []; + } + const parent = toRelative(path, mountRoot); + return names.filter((name) => isLocal(parent === "" ? name : `${parent}/${name}`)); + } + + function hasLocalContents(path: string): boolean { + try { + return fs.readdirSync(localPath(path)).length > 0; + } catch { + return false; + } + } + + // After a synced directory is renamed, move whatever it held on local + // disk, or packages/foo/node_modules is left behind at the old path, + // unreachable, while the new path has none. The two renames can't be + // atomic together. The synced one has already succeeded and is what + // the caller asked for, so a failure here is logged, not returned. + function moveLocalContents(source: string, destination: string): void { + const from = localPath(source); + try { + fs.lstatSync(from); + } catch { + return; + } + try { + const to = localPath(destination); + ensureParent(to); + fs.renameSync(from, to); + } catch (error) { + warn( + `computerd: renamed ${source} to ${destination}, but could not move its ` + + `local-only contents on disk (${errnoOf(error) ?? String(error)}). They ` + + `remain under ${from} and are not reachable through the mount.`, + ); } - return names; } return {