Fix reported symlink issues - #93
Conversation
🦋 Changeset detectedLatest commit: cc964a1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Thanks for your interest in Cloudflare Computer. This repository does not accept unsolicited pull requests. Please use one of the accepted contribution paths instead:
If a maintainer asked you to open this pull request, they can add the |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
commit: |
Follow target components in filesystem order so relative links use the resolved parent and cannot skip non-directory components before dot or parent segments.
Resolve each parent prefix separately so a regular file in a deep path reports ENOTDIR instead of being folded into ENOENT.
Resolve final links one component at a time until the write reaches an existing file or the missing target to create. Count parent and final links against one shared limit.
Record the real parent path for new dirents so writes through symlinked directories clear negative cache entries for the path that now exists.
Check both lexical and resolved paths for every write target, including intermediate links and buffered commits, so links cannot enter or escape read-only mounts.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Match read resolution by clamping leading parent segments at the filesystem root before validating and following write targets.
Use descendant-only checks while following symlink targets so an ancestor directory may contain a read-only mount without making writable siblings read-only.
Treat every absolute path as a descendant when the read-only mount root is the filesystem root.
| // straight into the path-keyed buffer. | ||
| const pending = getPendingWriteBufferByPath(db, canonical); | ||
| if (pending !== undefined) { | ||
| assertNotReadOnly(db, pendingTargetPath(db, pending, canonical)); |
There was a problem hiding this comment.
🟡 Every buffered write to a newly created file now performs an extra database walk
Each buffered write to a not-yet-committed file walks the directory tree in the database to rebuild the file's real path (pendingTargetPath(db, pending, canonical) at packages/dofs/src/fs/writeFile.ts:978) before the write is stored in memory, and it does so even when no read-only mounts exist at all.
Impact: Writing a large new file becomes measurably slower, because the in-memory write buffer that was designed to avoid database work now issues one query per directory level on every single write.
pathOf is an O(depth) query loop evaluated unconditionally as an argument
pendingTargetPath calls childPath → pathOf(db, parentInode) (packages/dofs/src/sync/paths.ts:6-26), which issues one SELECT ... FROM vfs_dirents WHERE child_inode = ? per path level. Because it is evaluated as the argument to assertNotReadOnly, it runs even when getReadOnlyMountRoots is empty and the guard is a no-op. The same pattern is repeated in openWriteBufferSync (packages/dofs/src/fs/writeFile.ts:712) and truncateFileSync (packages/dofs/src/fs/writeFile.ts:1056).
The pending-create buffer exists specifically so that per-syscall FUSE writes touch no SQL until release (packages/dofs/src/fs/writeBuffer.ts:1-11); the FUSE driver issues one writeRangeSync per write op, so a multi-megabyte file now pays depth-many point lookups per write. Caching the resolved target path on the pending entry at open time (or short-circuiting when there are no read-only mount roots) would restore the original cost.
Was this helpful? React with 👍 or 👎 to provide feedback.
Resolve open and clean release paths without write mount guards. Keep writable resolution on actual buffered mutations and dirty commits.
Keep both lexical and resolved aliases for pending create buffers so paths exposed by real-directory listings remain accessible before release.
Reuse the resolved target captured when opening a pending create so each buffered write and truncate stays in memory without walking parent dirents.
Commit pending child files before rename or rmdir changes their ancestor dirents, preserving path visibility and collision checks.
Drop dirty in-memory bytes when the final release cannot pass mount checks or commit, matching pending-create failure cleanup.
Translate read-only provider failures to the POSIX EROFS code for write, truncate, and release operations instead of reporting EIO.
Include pending paths reached through symlinks when a lexical ancestor is renamed so the moved alias remains usable.
Conservatively commit pending creates before structural directory operations so alternate lexical and symlink aliases cannot become stale.
Record traversed directory inodes when opening pending creates and flush only entries affected by a structural directory operation.
Track symlink inodes traversed by pending creates and commit affected buffers before rename or unlink changes those links.
Index pending creates by parent inode and leaf name, falling back to that index when another path alias misses the fast path.
Remember the lexical and effective paths that authorized a buffered mutation so final release does not depend on which hardlink alias closes last.
| export function findPendingWriteBuffer(db: Database, path: string): WriteBufferEntry | undefined { | ||
| const { parts, path: canonical } = canonicalizePath(path); | ||
| const direct = getPendingWriteBufferByPath(db, canonical); | ||
| if (direct !== undefined || parts.length === 0) return direct; | ||
|
|
||
| const leafName = parts.at(-1); | ||
| if (leafName === undefined) return undefined; | ||
| const parentPath = parts.length === 1 ? "/" : `/${parts.slice(0, -1).join("/")}`; | ||
| const parent = resolveInode(db, parentPath); | ||
| if (parent?.type !== "dir") return undefined; | ||
| return getPendingWriteBufferByParent(db, parent.inode, leafName); | ||
| } |
There was a problem hiding this comment.
🟡 Every file read and stat now performs an extra directory lookup even when nothing is being buffered
Each read, stat, and existence check now resolves the file's parent directory a second time (resolveInode at packages/dofs/src/fs/pendingWriteBuffer.ts:18) before doing its own lookup, even when there are no half-written files at all, so routine file access costs roughly twice the lookup work.
Impact: Read-heavy workloads over the mounted filesystem get noticeably slower for no benefit.
Why the extra lookup is unconditional
findPendingWriteBuffer first tries the O(1) path-keyed map, and on a miss it always falls through to resolveInode(db, parentPath) plus a parent-keyed map probe. The miss is the common case: the path-keyed map is only populated while a deferred-create buffer is open, which is rare.
The function is now on every hot read path: readRangeSync (packages/dofs/src/fs/readFile.ts:139, invoked per FUSE read syscall through SQLiteWorkspaceProvider.readRangeSync/readSync), readFile (packages/dofs/src/fs/readFile.ts:38), statShared (packages/dofs/src/fs/stat.ts:43), and the provider's lstatSync/readFileSync/existsSync/chmodSync (packages/dofs/src/provider.ts:198, :360, :471, :513).
Even with the path→inode resolve cache warm, resolveInode still re-reads the node row (packages/dofs/src/fs/resolve.ts:90), so each of these calls issues at least one additional SQL statement per operation; on a cold cache it runs a full recursive-CTE walk.
A guard that returns early when the database has no pending-create buffers at all would remove the cost entirely for the normal case.
Prompt for agents
findPendingWriteBuffer in packages/dofs/src/fs/pendingWriteBuffer.ts falls back to resolving the parent directory (resolveInode) whenever the path-keyed pending map misses. That miss is the normal case, because pending-create buffers only exist between openWriteBufferForCreateSync and release. The function is now called on every read/stat/exists path (readFile.ts readRangeSync and readFile, stat.ts statShared, provider.ts lstatSync/readFileSync/existsSync/chmodSync), so each of those operations pays an additional path resolution — at minimum an extra node-row SELECT even with the resolve cache warm, and a full recursive-CTE walk on a cold cache.
Add a cheap short-circuit so the parent-resolution fallback only runs when the database actually has pending-create buffers. writeBuffer.ts already owns the per-Database cache maps (byPendingPath / byPendingParent); exposing something like hasPendingWriteBuffers(db) (or checking byPendingParent.size) and returning undefined early in findPendingWriteBuffer would restore the previous cost for the common no-pending case while keeping the alias lookup behaviour when pending buffers exist.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
@cloudflare/dofshad three symlink gaps that made writes behave differently from reads. Relative symlink targets such astarget.txtfailed during resolution because they were treated as invalid absolute paths. Writes through a symlinked parent directory failed withENOTDIR, even though reads through the same path worked. Writes to a final symlink wrote chunks onto the symlink node instead of the target file, which left the target unchanged and broke the invariant that symlink nodes do not own file chunks.This change makes symlink handling consistent across resolution and writes. Relative symlink targets now resolve from the directory that contains the symlink. Write parent resolution now uses the normal resolver, so symlinked directories work the same way for reads and writes. Final symlinks are followed for normal writes, while exclusive writes still fail on the existing symlink with
EEXIST. If a final symlink points at a missing file, the write creates that target, including relative targets resolved from the symlink parent. The write path also checks read-only mounts for both the original path and the resolved target path.A quick manual check is to create a symlink and write through it:
The test coverage includes relative symlink resolution, dangling relative targets, writes through symlinked directories, final symlink writes, dangling final symlink creation, exclusive writes, range writes, and checks that symlink nodes do not receive file chunks.
Fixes #54. Fixes #90. Fixes #65. Fixes #55.