computerd: Add support for an ignore field to the ContainerBackend - #187
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 3d548cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+612
to
+615
| const destination = localPath(path); | ||
| ensureParent(destination); | ||
| fs.symlinkSync(target, destination); | ||
| cb(0); |
There was a problem hiding this comment.
🔴 Local symlinks cannot reach synced files
A relative link from ignored node_modules into synced src resolves against MOUNT_IGNORE_PATH, not the mount. symlink preserves the text, so tools following that link cannot find the synced file.
Learn more
A local-only directory has two path spaces: its mount path and its backing disk path. symlinkSync creates the link on the disk, so relative targets are interpreted relative to the backing disk rather than the visible mount directory. Cross-boundary symlinks used by package managers therefore resolve to the wrong target. The localPath mapping does not rewrite those targets.
Example: /workspace/node_modules/.bin/tool links to ../../src/tool.js. The corresponding backing link at /tmp/workspace/node_modules/.bin/tool resolves to /tmp/workspace/src/tool.js, not /workspace/src/tool.js.
Recommended fix: Define and test cross-boundary symlink semantics; preserve mount-namespace resolution for links instead of blindly following links in the backing filesystem.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+113
to
+121
| // 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. | ||
| // | ||
| // 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 honour the variable. | ||
| ignore?: readonly string[]; |
There was a problem hiding this comment.
🔍 Container example omits the new option
The public ignore option appears in documentation but not examples/container. Repository guidelines ask for examples to track public API changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+988
to
+991
| const routedOps = | ||
| options.localPaths === undefined | ||
| ? baseOps | ||
| : withLocalPassthrough(baseOps, options.localPaths).ops; |
There was a problem hiding this comment.
🔍 Description overstates disk routing
The PR description says all directories write directly to disk. The implementation routes only configured ignored paths to disk under real FUSE.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+228
to
+235
| const ensureParent = (target: string): void => { | ||
| const parent = dirname(target); | ||
| try { | ||
| fs.mkdirSync(parent, { recursive: true, mode: DEFAULT_DIR_MODE }); | ||
| options.onMaterialise?.(parent); | ||
| } catch (error) { | ||
| if (errnoOf(error) !== "EEXIST") throw error; | ||
| } |
There was a problem hiding this comment.
🟥 Ignored-directory symlinks escape local storage root
When an ignored directory contains a symlink, localPath-based operations follow it outside MOUNT_IGNORE_PATH. A container command can then access files elsewhere on the disk through the mount.
Was this helpful? React with 👍 or 👎 to provide feedback.
Everything a container command writes under MOUNT_POINT is recorded in the VFS and pulled into the Durable Object after the command. That is right for source and wrong for node_modules, .venv, target/ and dist/: tens of thousands of rebuildable files that never need to be durable, and no way to exclude them. This is the path set for #179, and only that. Nothing calls it yet. The decision cache and the passthrough I/O that will consume it belong in driver.ts and land separately; keeping the pure part apart is what lets its semantics be pinned with no mount, no syscalls and no container. Entries are plain paths relative to the mount root. No glob syntax and no negation: an entry names one location, and a path is local-only if it equals that entry or sits underneath it. The simplicity is the design. An entry resolves to a known location, so the mapping onto MOUNT_IGNORE_PATH is a prefix substitution fixed at startup rather than a question that can only be answered once a path arrives. Matching reduces to a segment-aware prefix test, which the driver's per-inode cache can collapse to one lookup per directory. And the set of paths that silently lose durability stays reviewable by reading it, which matters more than expressiveness when the cost of a wrong entry is data that exists only inside one container. The limitation is that an entry does not match at every depth: a monorepo lists app/node_modules and web/node_modules rather than writing node_modules once. That is more lines in a Dockerfile. If depth matching is ever needed, a single leading `**/` form is the smallest addition that stays resolvable. Entries are normalised once: slashes stripped, absolute paths under the mount accepted and made relative, "." and ".." rejected rather than resolved so an escaping entry cannot hide behind a path that looks intentional, and duplicates or entries nested inside another dropped as redundant. The normalised list is what the mount applies and what diagnostics should report. Verified by mutation. Four mutants, all killed: the segment boundary replaced by a plain startsWith, descendant matching reduced to equality, ".." rejection skipped, and nested-entry collapsing disabled. The first is the one that matters -- a naive startsWith passes every other test in the file and reports node_modules_extra as living under node_modules.
Wires the path set added in the previous commit into the FUSE op layer, so configuring MOUNT_IGNORE now does something. Matching paths are served from MOUNT_IGNORE_PATH (default /tmp/$MOUNT_POINT) instead of the VFS: never recorded, never pushed, never pulled. Implemented as a decorator over FuseOps rather than branches inside makeFUSEOps. The VFS driver stays unaware of the feature, so a bug here is bounded by the ignore set; disabling it is provably free, because an empty set returns the source object unchanged; and it composes the way the tracer already does. Ignored-ness is decided per directory and inherited. A node_modules tree is tens of thousands of entries under a handful of directories, so without inheritance every lookup would re-test the entry list. The cache is invalidated when a directory is removed, or a recreated path would keep a stale decision and silently land in the wrong layer. Local file handles are allocated from a high range so they cannot collide with the VFS driver's, which counts from 1. A handle that crossed layers would read one file and write another. A rename across the boundary returns EXDEV. The two sides are different filesystems, so the operation cannot be atomic, and copying underneath would turn a crash mid-copy into a half-written file where the caller was promised all-or-nothing. Tools already handle EXDEV by falling back to copy-then-unlink. Within one layer it is a real rename. Configuration fails closed at startup: a root that is relative, equal to or inside MOUNT_POINT, or the filesystem root is refused, as is a malformed entry. A silently dropped entry would send a full node_modules into the Durable Object, which is the failure this prevents. The root defaults under /tmp so a container snapshot captures it, since that is the only durability local-only content has. /__computerd/info reports the resolved set, the entries dropped as redundant, and fastPaths. Passthrough is reported false with its reason rather than omitted: the host kernel supports FOPEN_PASSTHROUGH but fuse-native binds libfuse 2.9, which cannot negotiate it, so this flips on a binding change rather than an infrastructure one. Writeback caching is unavailable for the same reason. Verified by mutation. Six mutants, all killed: EXDEV turned into a silent copy-through, the local handle range overlapped with the VFS's, parent creation removed, the decision cache left un-invalidated on rmdir, inheritance disabled, and the readdir merge dropped. The package suite is now fully green (255 passing). The 32 previously failing cli tests were spawning an unbuilt binary; building it as part of this work let them run, and the one real break -- an exact-equality assertion on /__computerd/info -- is updated to cover the new block.
Adds `ignore` to ContainerBackend and exposes the resolved set as `handle.ignore`. The backend reads /__computerd/info during connect() and refuses the connection when the container disagrees with what the caller declared. The option is a declaration, not a setting. The set belongs to the image, which reads MOUNT_IGNORE at startup; a client cannot change it. Making it configurable here would promise a per-session knob the architecture cannot honour, because the mount is per-container and compiled once -- two sessions sharing an image cannot hold different views of which paths are durable. Failing the connection rather than warning is deliberate. The failure being guarded is silent and slow: an image built without MOUNT_IGNORE, or carrying a stale set, is indistinguishable from a correct one until a command writes a large dependency tree and the whole thing is pulled into the Durable Object. That is the symptom #179 reports -- a command timeout, then a storage-timeout cascade the workspace does not recover from. A deployment error is cheaper discovered loudly. The check runs before the handle is published, and tears the transport down on rejection. Running it afterwards would let the first exec write into a path the caller believes is local-only, which is the state that is expensive to find later. Omitting `ignore` skips the check and accepts whatever the container provides, so this cannot break an existing deployment. A failure to reach /__computerd/info is likewise reported as unsupported rather than propagated: the endpoint is diagnostic, and a caller that declared nothing should not lose a working connection to a failed diagnostic request. A caller that did declare still fails. Comparison is order-insensitive and tolerates slash decoration on either side, because computerd normalises its own set and a host listing the same paths differently means the same thing. Both directions are reported: a path the image does not apply will be synced unexpectedly, and one it applies but the caller did not declare will not be synced when the caller thinks it is. Verified by mutation. Five mutants, all killed: accepting an unsupported container, never throwing on mismatch, checking even when the declaration is omitted, comparing only one direction, and dropping slash normalisation.
A rename between a MOUNT_IGNORE path and a synced one returns EXDEV. The kernel surfaces that to the caller as "cross-device link", which on a path that is plainly not a device is the kind of message an operator loses an afternoon to. The errno is all the FUSE callback can carry, so the guidance goes to the log instead: both sides named, why the rename cannot be atomic, and the entry to add to MOUNT_IGNORE to make it atomic again. Build tools that stage into a sibling and rename into place are the common cause, and ignoring the staging path alongside its destination is almost always the fix. Logged once per mount, not once per rename. A build that does this does it in a loop, and a line per occurrence would bury everything else in the log. The full count stays available on the passthrough stats for anyone who wants it. Verified by mutation: removing the once-guard, removing the call, and swapping which side is reported as local-only each fail a test.
Adds docs/20_local_only_paths.md covering MOUNT_IGNORE: what it does, the per-image rule, validation, diagnostics, and the EXDEV rename contract. The durability trade-off leads rather than trails. These paths are invisible to workspace.fs and the worker shell, and survive container replacement only through a snapshot -- which is the right trade for a dependency tree a package manager can rebuild and the wrong one for anything a user typed. A reader deciding whether to use this needs that before the syntax, not after it. EXDEV gets its own section because it is the one runtime failure a correctly configured deployment can still hit. It explains why the copy is refused rather than performed -- faking atomicity turns a crash mid-copy into a silent half-written file -- and gives the fix, with the staging-directory case called out since that is how build tools produce it. Also records what these paths do not make faster. The saving is the transfer, not the I/O: the bytes still cross the FUSE boundary, because passthrough needs libfuse 3.17 and fuse-native binds 2.9. Without that, the first benchmark against raw disk reads as a regression. 19_performance.md gains a matching note. Its npm install table stops when the install returns and does not count the pull into the Durable Object, which for a dependency tree is the larger cost and is the part #179 reports timing out. The ignored-vs-not table is added with its cells deliberately empty: bytes-pulled is the load-bearing number for this feature and has not been measured yet, and an invented figure in a performance document is worse than a visible gap.
Drop the U2/U3/U5 milestone markers. They refer to phases in a planning doc that is not published with this repo, so a reader has no way to resolve them. Issue references are kept. Cut comments that restate the code or duplicate docs/20_local_only_paths.md, and repoint a dangling "See DESIGN" at that doc. What stays is the reasoning a reader cannot recover from the source: why MOUNT_IGNORE is newline-delimited, why EXDEV is refused rather than copied, why the assertion fails the connection instead of warning, and why the store defaults under /tmp. Comments only; no behaviour change.
…aths
Three changes to the local-only path interface.
MOUNT_IGNORE is now a comma-separated list rather than newline-delimited,
so it reads naturally as a single environment variable:
MOUNT_IGNORE=/node_modules,/.venv,/dist
A leading slash now anchors at the mount root rather than the filesystem
root, so "/node_modules" means "$MOUNT_POINT/node_modules". The
fully-qualified form ("/workspace/dist") is still accepted. A path
containing a comma can no longer be expressed, and an entry that looks
like it names somewhere else on disk is reinterpreted as mount-relative
rather than rejected.
The backend's `ignore` option now configures rather than asserts: it is
passed to the container in its start environment, so the set belongs to
the deployment instead of the image and changing it needs no rebuild. An
explicit MOUNT_IGNORE in containerEnv still wins. connect() continues to
read the resolved set back and reject a disagreement.
handle.ignore.paths are now absolute container paths
("/workspace/node_modules") rather than mount-relative, and the handle
carries mountPoint so the comparison can still be made on equal footing.
The MOUNT_IGNORE write-up described shipped computerd behaviour, which belongs with the package rather than in the forward-looking design specification. It now lives as a section of the computerd README, with the performance discussion left to the existing section in the performance doc. Links and source comments that pointed at the old file are updated. The two computerd changesets are removed. The packages version together, so the existing @cloudflare/computer changeset already carries the release, and a separate entry per implementation detail only duplicates it in the changelog.
aron-cf
force-pushed
the
computerd-ignore
branch
from
October 1, 2026 13:18
1aacd9b to
66a95e3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ContainerBackend read /__computerd/info without the client secret. computerd protects every route except /health, so an enforcing container answered 401, the backend read that as "computerd does not support local-only paths", and every connect that declared `ignore` failed. The request now carries the bearer token. The comparison also disagreed with computerd about spelling. computerd accepts "/workspace/dist" and applies it as "dist", but the client compared the declaration raw and reported a mismatch. Declarations now have the mount point stripped before comparing, the same as the report. The mismatch error still said the set belonged to the image, which stopped being true once `ignore` began setting MOUNT_IGNORE at start. It now points at the likely cause: a MOUNT_IGNORE in `containerEnv` overriding the option. The new tests drive connect() through the upgrade against a fake host that enforces the secret the way computerd does. The earlier fake answered /__computerd/info without checking, which is how this got through.
The shim copies everything under the mount into the VFS, so it cannot keep a path local. computerd still resolved MOUNT_IGNORE and reported the paths as local-only on /__computerd/info, so the host's check passed while every ignored write was synced. FUSE_MOUNT=auto falls back to the shim when /dev/fuse is missing, which made this reachable from a misconfigured container with no explicit opt-in. computerd now fails at startup when MOUNT_IGNORE is set and the backend resolves to the shim, the same as it does for every other ignore misconfiguration. The real-FUSE test still set MOUNT_IGNORE in the old newline-separated form, which the comma-separated parser reads as one entry. It only runs on Linux with /dev/fuse, so it had not run since the format changed. It now uses the current form.
Several operations in the local-only layer either ignored the open file or acted on the wrong one. ftruncate truncated by path instead of by descriptor. After a rename or replacement the path names a different file, so truncating an open handle could destroy another file's contents. It now uses the descriptor. fsync returned success without flushing anything, so a program using fsync as a commit point could lose data it had been told was on disk. It now calls fsync or fdatasync on the descriptor. link was not routed at all. A hardlink between two local-only paths went to the VFS, which has never seen either file, and failed with ENOENT. Both-local links are now made on disk, and a link across the boundary returns EXDEV, which is what link(2) returns for two filesystems anywhere else. chown and utimens followed symlinks. The kernel resolves links before calling the daemon unless the caller asked for the link itself, as `chown -h` and `touch -h` do, so a path that reaches these calls as a symlink means the link. Following it acted on the target, which can be outside the local root. They now use lchown and lutimes. opendir accepted any path and left the error for readdir, and access reported success for any existing path regardless of the mode asked for. opendir now checks the path is a directory, and access checks the requested mode.
The cache meant to answer "is this path local-only?" from its parent directory grew with the dependency tree instead. A directory reached by inheritance was never cached itself, so only one level below an entry was ever inherited. Below that, every file was matched again and its path stored, and nothing removed entries when files were deleted. It also cost a syscall on every miss. To decide whether to cache a path, it called lstat on the matching location under the local root, which included every path in the synced tree. Every getattr in the VFS paid for a local disk lookup to answer a question that never involved local disk. The ignore set is a handful of entries and the test is a prefix comparison against each, which costs about what the cache lookup did. So the cache goes, along with the invalidation it needed on rename and rmdir, and the decision and cache-hit counters that only existed to show it working.
The local-only layer counts the operations it serves, its open handles, and the renames it refuses with EXDEV, but mountFuse dropped the accessor, so none of it was visible. The README said the rename count was available, and it was not. The mount now exposes the counters, and /__computerd/stats reports them under `localPaths` when MOUNT_IGNORE is active on a real FUSE mount.
The docs and the rename warning said callers fall back to copy-then- unlink on EXDEV. Some do: mv and Python's shutil.move copy instead. A program calling rename directly, such as Node's fs.rename or Go's os.Rename, gets the error and has to handle it itself, and the warning is now explicit about that. The README also notes that hardlinks across the boundary return EXDEV for the same reason. The code, comments, and docs this feature added used British spelling (normalise, materialise, behaviour, honour, favour). They now use US spelling, which the repository's prose guidelines ask for. Existing identifiers outside this feature, such as normaliseMountPoint in the FUSE driver, are left alone.
aron-cf
force-pushed
the
computerd-ignore
branch
from
October 1, 2026 21:30
66a95e3 to
4547229
Compare
Comment on lines
+114
to
+122
| // 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. | ||
| // | ||
| // 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. | ||
| ignore?: readonly string[]; |
There was a problem hiding this comment.
🔍 Backend naming differs from PR description
The description names CloudflareContainerBackend, but ignore belongs to ContainerBackend. Align the example with the exported backend users can configure.
Was this helpful? React with 👍 or 👎 to provide feedback.
commit: |
The local-only path check still described the set as a property of the image, with the client only able to state an expectation. That stopped being true when `ignore` began setting MOUNT_IGNORE at container start. The file header, doc comments, mismatch messages, and test names now say the container applies the set the backend passed it, and name the two real causes of a mismatch: a computerd too old to read MOUNT_IGNORE, or a MOUNT_IGNORE in `containerEnv` overriding the option.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a new
ignoreflag to the CloudflareContainerBackend class, all directories will write directly through to disk rather than be included in the fuse mount.It does this by setting
MOUNT_IGNOREin the container environment which is picked up bycomputerd.The
MOUNT_IGNORE_PATHcan also be set which will be the path to the directory that holds the actual files. It defaults to/tmp/$MOUNT_POINT. This allows you to backup/restore this directory if needed.