From bdfe1a993db70166281d58754599929edb61000c Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:43:20 +0000 Subject: [PATCH 1/6] computerd: accept glob patterns in MOUNT_IGNORE MOUNT_IGNORE took plain paths from the mount root, so it could not say "every node_modules, wherever it is". In a monorepo that meant listing every package by hand. It now takes a small subset of gitignore: `*` within a path segment, `**` for any number of segments, and a leading `!` to exclude. MOUNT_IGNORE=**/node_modules,!/vendor/node_modules,/packages/*/dist Every pattern must start with "/" (from the mount root) or "**/" (at any depth). A bare `node_modules` would mean root-only here and any depth in gitignore, so it is rejected with both spellings suggested rather than left to be misread. Braces, character classes, `?`, and backslashes are rejected rather than matched literally, and so is any pattern that would make the whole mount local-only. 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 follows from how the two sides are stored: once a directory is on local disk, the synced filesystem has no directory for an excluded child to live in. An exclusion under a local-only directory therefore does nothing, and computerd warns about it at startup and lists it on /__computerd/info as `ineffectiveExclusions`. The ignore block reports `patterns` in the order written, replacing `paths` and `redundant`, since order now changes the meaning and a pattern cannot be joined onto the mount point. Deciding a path still never touches disk. The patterns are compiled once, and the check walks the path's directories against them. Globs make some situations routine that were rare with fixed paths, and the local-only layer now handles them. A synced directory listing includes local-only children at any depth, read from the matching directory under the local root, but not the scaffolding directories that only exist to hold them. Renaming a synced directory moves its local-only contents with it, so packages/foo/node_modules is not left behind on disk; the two moves cannot be atomic together, so a failure on the local side is logged rather than returned. Removing a synced directory returns ENOTEMPTY while it still holds local-only contents, which the synced side cannot see, and cleans up its scaffolding once it is gone. --- packages/computerd/src/cli/computerd.test.ts | 6 +- packages/computerd/src/cli/computerd.ts | 12 +- .../computerd/src/fuse/ignore-config.test.ts | 34 +- packages/computerd/src/fuse/ignore-config.ts | 10 +- packages/computerd/src/fuse/ignore.test.ts | 307 +++++++++++------- packages/computerd/src/fuse/ignore.ts | 236 +++++++++----- .../computerd/src/fuse/passthrough.test.ts | 172 ++++++++-- packages/computerd/src/fuse/passthrough.ts | 85 +++-- 8 files changed, 590 insertions(+), 272 deletions(-) diff --git a/packages/computerd/src/cli/computerd.test.ts b/packages/computerd/src/cli/computerd.test.ts index 8bf4a335..4afab3a8 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. 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..e5f2f991 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,241 @@ 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("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("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 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("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("* 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 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("** 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("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("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("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("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("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 ignore after an exclusion wins again", () => { + const ignore = set(["!/dist", "/dist"]); + expect(ignore.ignores("dist")).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("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 . and .. segments", () => { + rejects("/a/../b", /"\." or "\.\." segment/); + rejects("/./a", /"\." or "\.\." segment/); + rejects("**/..", /"\." or "\.\." 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 an empty segment", () => { + rejects("/a//b", /empty path segment/); + }); + + 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..bb83da93 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,156 @@ 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.`); + } + if (parts.every((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..31b5cfa2 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,131 @@ 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 { ops, warnings } = build(); + mkdirSync(join(root, "packages/foo/node_modules"), { recursive: true }); + // A non-empty directory already at the destination makes the local + // rename fail with ENOTEMPTY. + mkdirSync(join(root, "packages/bar/node_modules/other"), { recursive: true }); + + expect(status((cb) => ops.rename("/packages/foo", "/packages/bar", cb))).toBe(0); + expect(warnings.join("\n")).toMatch(/packages\/foo.*packages\/bar/); + }); + + 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..b6303989 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,30 @@ 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); + try { + if (fs.readdirSync(scaffolding).length > 0) { + cb(ERRNO.ENOTEMPTY); + return; + } + } catch { + // No local side; nothing to check. + } + 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 +549,10 @@ export function withLocalPassthrough( const destinationLocal = isLocal(destination); if (!sourceLocal && !destinationLocal) { - ops.rename(source, destination, cb); + ops.rename(source, destination, (code) => { + if (code === 0) moveLocalContents(source, destination); + cb(code); + }); return; } @@ -711,24 +737,45 @@ 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}`)); + } + + // 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 { From ebd019ceb09be644f1ddd5dd8a89ca5c57480b5c Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:46:48 +0000 Subject: [PATCH 2/6] computer: accept MOUNT_IGNORE patterns in ContainerBackend's `ignore` computerd now reads MOUNT_IGNORE as glob patterns and reports them as `patterns` on /__computerd/info. The client follows. `ignore` takes the same patterns, such as "**/node_modules" and "!/vendor/node_modules", and the constructor checks them against the rules computerd applies. A pattern computerd would refuse, like a bare "node_modules", now throws before any container starts instead of surfacing as a daemon that exits during startup. The rules are duplicated rather than imported, since this package does not depend on computerd, and the two test suites cover the same cases. A pattern containing a comma is rejected too, because it would split into two entries in MOUNT_IGNORE. connect() compares the declared and applied patterns in order, after normalizing the declaration the way computerd does. Order matters once exclusions are involved: the same two patterns can keep a path synced one way round and local-only the other. `BackendHandle.ignore.paths` becomes `patterns`, and readIgnoreReport no longer joins entries onto the mount point, which made sense for plain paths but not for "**/node_modules". A computerd that reports `paths` predates patterns, so it is treated as unsupported and a connect that declared `ignore` fails. --- packages/computer/src/backend.ts | 12 +- .../container-backend-ignore.test.ts | 99 +++---- .../backends/container/container-backend.ts | 29 +- .../container/ignore-assertion.test.ts | 280 +++++++++--------- .../backends/container/ignore-assertion.ts | 198 +++++++------ 5 files changed, 316 insertions(+), 302 deletions(-) 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..9a57f1fb 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,90 @@ 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("diffIgnore", () => { - test("agrees when the sets match", () => { - expect(diffIgnore(["node_modules", "dist"], ["node_modules", "dist"])).toBeNull(); +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(); }); - 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 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 slash decoration on either side", () => { - expect(diffIgnore(["/dist/", "node_modules"], ["dist", "node_modules"])).toBeNull(); + test("requires ** to be a whole segment", () => { + rejects("/a**/b", /whole path segment/); }); - 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 patterns that would make the whole mount local-only", () => { + 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("rejects . and .. segments, and empty segments", () => { + rejects("/a/../b", /"\." or "\.\." segment/); + rejects("/./a", /"\." or "\.\." segment/); + rejects("/a//b", /empty path segment/); }); - 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 unsupported syntax", () => { + for (const pattern of ["/*.{js,ts}", "/[ab]", "/a?", "/a\\*"]) { + rejects(pattern, /not supported/); + } }); - test("reports both directions at once", () => { - expect(diffIgnore(["a", "b"], ["b", "c"])).toEqual({ missing: ["a"], unexpected: ["c"] }); + test("rejects a comma, which would split into two patterns", () => { + rejects("/a,/b", /comma/); }); - 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("bounds pattern length", () => { + rejects(`/${"a".repeat(5000)}`, /longer than 4096/); }); }); @@ -131,126 +144,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..1d593b80 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,82 @@ 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.`); + } + if (parts.every((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 +154,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 +163,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 { From 0e9e60624e2d38f682724113be65839544a701bb Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:47:48 +0000 Subject: [PATCH 3/6] computerd: cover MOUNT_IGNORE patterns on a real FUSE mount Adds an end-to-end test for patterns: with `**/node_modules,!/vendor/node_modules,!**/node_modules/.bin`, a nested node_modules lands on local disk, the vendored one and ordinary source stay in the VFS, and the .bin exclusion is reported as having no effect. It then renames the synced package that holds the nested node_modules and checks the contents moved with it, and removes the package with rm -rf and checks both sides are gone. Like the other real-FUSE tests, it runs only on Linux with /dev/fuse and skips elsewhere. --- packages/computerd/src/cli/computerd.test.ts | 69 ++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/packages/computerd/src/cli/computerd.test.ts b/packages/computerd/src/cli/computerd.test.ts index 4afab3a8..285b0c2b 100644 --- a/packages/computerd/src/cli/computerd.test.ts +++ b/packages/computerd/src/cli/computerd.test.ts @@ -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(); From edd9f351519c34128a996c7843e6ba874cb649d6 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:48:45 +0000 Subject: [PATCH 4/6] docs: document MOUNT_IGNORE patterns The computerd README described MOUNT_IGNORE as plain paths with no glob syntax. It now covers the patterns: what each form matches, the anchoring rule, last-match-wins, and why an exclusion under a local-only directory does nothing. It lists the patterns rejected at startup and why, and that ContainerBackend checks the same rules in its constructor. The /__computerd/info example and the handle example show `patterns` and `ineffectiveExclusions` instead of `paths` and `redundant`. A new section explains how computerd keeps a synced directory and the local-only paths under it in step on listing, rename, and removal, and the one case it can't: a rename that changes which patterns match. The changeset and the performance doc's wording follow. --- .changeset/container-ignore-assertion.md | 5 + docs/19_performance.md | 2 +- packages/computerd/README.md | 127 +++++++++++++++++------ 3 files changed, 99 insertions(+), 35 deletions(-) create mode 100644 .changeset/container-ignore-assertion.md diff --git a/.changeset/container-ignore-assertion.md b/.changeset/container-ignore-assertion.md new file mode 100644 index 00000000..7ba89cc7 --- /dev/null +++ b/.changeset/container-ignore-assertion.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/computerd/README.md b/packages/computerd/README.md index 62677cea..1e7ec25e 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/../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,25 @@ 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 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 +296,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, From 000cfd7c4959a2372a117c42cda37ed349661f6a Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Fri, 2 Oct 2026 11:18:30 +0000 Subject: [PATCH 5/6] computerd, computer: reject MOUNT_IGNORE patterns of only wildcards `/**` was refused for making the whole mount local-only, but `/*` was accepted, and it does the same thing: it matches every top-level entry, and everything under a local-only directory is local-only. So a workspace configured with `/*`, easily written when meaning `/*.log`, would keep every file off the Durable Object and lose it all when the container was replaced. computerd and ContainerBackend now refuse any pattern made only of `*` and `**` segments, such as `/*`, `**/*`, and `/*/*`. Targeted wildcards like `/*.log` and `/packages/*/dist` are unaffected. --- .../src/backends/container/ignore-assertion.test.ts | 12 ++++++++++++ .../src/backends/container/ignore-assertion.ts | 5 ++++- packages/computerd/README.md | 2 +- packages/computerd/src/fuse/ignore.test.ts | 12 ++++++++++++ packages/computerd/src/fuse/ignore.ts | 5 ++++- 5 files changed, 33 insertions(+), 3 deletions(-) diff --git a/packages/computer/src/backends/container/ignore-assertion.test.ts b/packages/computer/src/backends/container/ignore-assertion.test.ts index 9a57f1fb..ae6388ff 100644 --- a/packages/computer/src/backends/container/ignore-assertion.test.ts +++ b/packages/computer/src/backends/container/ignore-assertion.test.ts @@ -119,6 +119,18 @@ describe("checkIgnorePatterns", () => { } }); + test("rejects wildcards that match every path at some depth", () => { + for (const pattern of ["/*", "/**/*", "**/*", "/*/*", "/*/**", "!/*"]) { + rejects(pattern, /whole mount/); + } + }); + + test("still accepts targeted wildcards", () => { + expect(() => + checkIgnorePatterns(["/*.log", "/packages/*/dist", "**/*.tsbuildinfo", "/build-*"]), + ).not.toThrow(); + }); + test("rejects . and .. segments, and empty segments", () => { rejects("/a/../b", /"\." or "\.\." segment/); rejects("/./a", /"\." or "\.\." segment/); diff --git a/packages/computer/src/backends/container/ignore-assertion.ts b/packages/computer/src/backends/container/ignore-assertion.ts index 1d593b80..57bd53ce 100644 --- a/packages/computer/src/backends/container/ignore-assertion.ts +++ b/packages/computer/src/backends/container/ignore-assertion.ts @@ -134,7 +134,10 @@ export function checkIgnorePatterns(patterns: readonly string[]): void { if (parts.some((part) => part !== "**" && part.includes("**"))) { fail(`uses "**" inside a segment. "**" must be a whole path segment.`); } - if (parts.every((part) => part === "**")) { + // 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."); } } diff --git a/packages/computerd/README.md b/packages/computerd/README.md index 1e7ec25e..45c65e4a 100644 --- a/packages/computerd/README.md +++ b/packages/computerd/README.md @@ -198,7 +198,7 @@ These fail the daemon at startup, because a dropped pattern means a full | --- | --- | | `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 | +| `**`, `/**`, `/`, `/*`, `**/*` | 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 | diff --git a/packages/computerd/src/fuse/ignore.test.ts b/packages/computerd/src/fuse/ignore.test.ts index e5f2f991..b003599b 100644 --- a/packages/computerd/src/fuse/ignore.test.ts +++ b/packages/computerd/src/fuse/ignore.test.ts @@ -233,6 +233,18 @@ describe("resolveMountIgnore: rejected patterns", () => { } }); + 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/); diff --git a/packages/computerd/src/fuse/ignore.ts b/packages/computerd/src/fuse/ignore.ts index bb83da93..17479865 100644 --- a/packages/computerd/src/fuse/ignore.ts +++ b/packages/computerd/src/fuse/ignore.ts @@ -151,7 +151,10 @@ function compilePattern(entry: string, index: number, root: string): CompiledPat if (parts.some((part) => part !== "**" && part.includes("**"))) { fail(`uses "**" inside a segment. "**" must be a whole path segment.`); } - if (parts.every((part) => part === "**")) { + // 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."); } From 246a845d443d0e6403a6de372a8dcabcef383fdf Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Fri, 2 Oct 2026 11:19:40 +0000 Subject: [PATCH 6/6] computerd: refuse a rename onto a directory holding local-only paths The synced side never sees local-only children, so it would let a rename replace a directory that still held, say, node_modules on local disk. The follow-up move of the source's own local-only contents onto that occupied directory then failed, and was only logged, because the synced rename had already succeeded. The source's node_modules was left at a path the mount no longer showed. In the merged view the destination is not empty, so the rename now returns ENOTEMPTY before touching either side, the same answer rmdir already gave for such a directory. A destination holding only empty scaffolding is still replaced, and the source's contents move into it. --- ...assertion.md => container-ignore-globs.md} | 0 packages/computerd/README.md | 5 +- .../computerd/src/fuse/passthrough.test.ts | 46 +++++++++++++++++-- packages/computerd/src/fuse/passthrough.ts | 27 ++++++++--- 4 files changed, 65 insertions(+), 13 deletions(-) rename .changeset/{container-ignore-assertion.md => container-ignore-globs.md} (100%) diff --git a/.changeset/container-ignore-assertion.md b/.changeset/container-ignore-globs.md similarity index 100% rename from .changeset/container-ignore-assertion.md rename to .changeset/container-ignore-globs.md diff --git a/packages/computerd/README.md b/packages/computerd/README.md index 45c65e4a..81766a35 100644 --- a/packages/computerd/README.md +++ b/packages/computerd/README.md @@ -273,8 +273,9 @@ stays synced. computerd keeps the two sides in step: 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 returns `ENOTEMPTY` while it still holds - local-only contents, which the synced side can't see. `rm -rf` removes +- 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 diff --git a/packages/computerd/src/fuse/passthrough.test.ts b/packages/computerd/src/fuse/passthrough.test.ts index 31b5cfa2..212f55d3 100644 --- a/packages/computerd/src/fuse/passthrough.test.ts +++ b/packages/computerd/src/fuse/passthrough.test.ts @@ -762,16 +762,54 @@ describe("withLocalPassthrough: synced directories that hold local-only paths", }); test("a synced rename succeeds even if the local move fails, and says so", () => { - const { ops, warnings } = build(); + 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 }); - // A non-empty directory already at the destination makes the local - // rename fail with ENOTEMPTY. - mkdirSync(join(root, "packages/bar/node_modules/other"), { 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 }); diff --git a/packages/computerd/src/fuse/passthrough.ts b/packages/computerd/src/fuse/passthrough.ts index b6303989..3177a4d2 100644 --- a/packages/computerd/src/fuse/passthrough.ts +++ b/packages/computerd/src/fuse/passthrough.ts @@ -514,13 +514,9 @@ export function withLocalPassthrough( // disk. Check the local side first, and clean up its scaffolding // once the synced directory is gone. const scaffolding = localPath(path); - try { - if (fs.readdirSync(scaffolding).length > 0) { - cb(ERRNO.ENOTEMPTY); - return; - } - } catch { - // No local side; nothing to check. + if (hasLocalContents(path)) { + cb(ERRNO.ENOTEMPTY); + return; } ops.rmdir(path, (code) => { if (code === 0) { @@ -549,6 +545,15 @@ export function withLocalPassthrough( const destinationLocal = isLocal(destination); if (!sourceLocal && !destinationLocal) { + // 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); @@ -753,6 +758,14 @@ export function withLocalPassthrough( 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