Conversation
🦋 Changeset detectedLatest commit: 8ec640e 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 |
commit: |
![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.
A child entry can arrive before its parent directory when the parent has a newer revision. Applying that stream used to fail repeatedly with a missing-parent error. Create absent parent directories before applying files and symbolic links. Replace conflicting ancestor files and symbolic links when the incoming child requires a directory, and allow safe parent creation next to read-only mounts. Cover the streaming pull path and synchronous push path with regression tests.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Protect read-only mount paths when replacing a blocking ancestor, and repair wrong-type ancestors for directory entries as well as files and symlinks. Resolve the common existing-parent case with one query so deeply nested sync entries do not repeatedly walk each path prefix.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Let file applies use an existing parent that resolves through a symlink. Keep structural parent repair for missing targets so child-first streams still converge.
Resolve a reachable symlinked parent to its real directory before applying files, directories, or symlinks. Keep replacing dangling symlink ancestors so child-first streams can still create their required parent directories.
| for (let i = 0; i < parts.length - 1; i++) { | ||
| const ancestorPath = `/${parts.slice(0, i + 1).join("/")}`; | ||
| const ancestor = resolveInode(db, ancestorPath, { followSymlinks: false }); | ||
| if (ancestor === null) { | ||
| mkdirForSyncParents(db, parentPath, { recursive: true }, () => mtime); | ||
| return { path: canonical }; | ||
| } | ||
| if (ancestor.type === "dir") continue; | ||
| const blockingRoot = readOnlyRootFor(db, ancestorPath); | ||
| if (blockingRoot !== undefined) return { path: canonical, blockingRoot }; | ||
| removeInodeTreeAtPath(db, ancestorPath, ancestor.inode, ancestor.type); | ||
| mkdirForSyncParents(db, parentPath, { recursive: true }, () => mtime); | ||
| return { path: canonical }; | ||
| } |
There was a problem hiding this comment.
🟡 A shortcut link to a folder is deleted when the folder it points to lacks the needed subfolder
An existing shortcut that points at a real folder is deleted and replaced by a new empty folder (removeInodeTreeAtPath at packages/dofs/src/sync/apply.ts:248) whenever the required subfolder underneath it is missing, so everything that used to be visible through that shortcut disappears from that location.
Impact: Files a user could previously reach through a linked folder path silently vanish from that path after a sync, even though the same link is deliberately preserved when the subfolder happens to already exist.
Inconsistency between the reachable-parent fast path and the ancestor-repair loop in ensureParentDirectories
ensureParentDirectories (packages/dofs/src/sync/apply.ts:214-253) has two mutually inconsistent outcomes for the same symlink ancestor:
-
Reachable case —
/workspace -> /target(dir) and/target/newdirexists.resolveInodeWithoutSymlinksreturns null (symlink on path),resolveInode(parentPath)follows to/target/newdir(dir), so the reachable branch atpackages/dofs/src/sync/apply.ts:226-236rewrites the entry path to/target/newdir/file.txtand preserves the symlink. This is the behaviour blessed by the new testswrites a file through a reachable symlink parentandcreates a directory through a reachable symlink parent. -
Unreachable case — same
/workspace -> /target(dir, contents intact) but/target/newdirdoes not exist.resolveInode("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/workspace/newdir")now returns null, so control drops into the loop at line 238. Ati = 0,ancestorPath = "/workspace"resolves (withfollowSymlinks: false) to asymlink,readOnlyRootFor("/workspace")is undefined, and line 248 unlinks the symlink outright.mkdirForSyncParentsthen materialises a literal empty/workspaceand/workspace/newdir.
After case 2 the contents of /target (e.g. keep.txt) are no longer reachable at /workspace/... at all, even though the only thing actually missing was one subdirectory under the link target. The minimal repair would have been to create newdir under the resolved target — exactly what case 1 already assumes is the right model.
The existing tests only exercise the symlink-destruction path with a dangling link (symlink(db, "/missing", "/workspace", ...) at packages/dofs/src/sync/apply.test.ts:260), so the resolvable-link-plus-missing-subdir combination is uncovered.
Prompt for agents
In packages/dofs/src/sync/apply.ts, ensureParentDirectories handles a symlink ancestor in two contradictory ways.
When the parent path resolves through a symlink to an existing directory (the branch around line 226 that calls resolveInode(parentPath) and pathOf), the symlink is preserved and the write is redirected to the resolved real path. The new tests assert this is the desired behaviour.
When the same symlink points at a real directory but the required subdirectory under it does not yet exist, resolveInode(parentPath) returns null, so control falls into the per-ancestor loop. There the symlink is treated as a 'blocking' ancestor and is removed via removeInodeTreeAtPath, then a literal directory chain is created in its place. The link target's contents are still on disk but are no longer visible under the link's path.
The two branches should agree. A reasonable approach is: before treating a symlink ancestor as blocking, follow it with resolveInode (followSymlinks: true). If it resolves to a directory, translate the remaining path components onto the resolved real path (via pathOf) and create the missing subdirectories there, preserving the link — mirroring what the reachable-parent branch already does. Only treat a symlink ancestor as blocking when it is dangling or resolves to a non-directory.
Add a regression test covering: /target exists as a directory with content, /workspace is a symlink to /target, /target/newdir does NOT exist, and a file entry for /workspace/newdir/file.txt is applied. The symlink and /target's existing content should survive.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #107.
Sync entries are ordered by revision. A directory can have a newer revision than a file inside it, so the file may arrive first even though its parent exists on the sender.
For example, the receiver could see this stream:
The old apply path tried to create
file.txtimmediately. Since/workspace/newdirwas not present on the receiver yet, the operation failed with:Retrying did not help because the sender returned the same entries in the same order. A failed pull could stop container changes from reaching durable storage, while a failed push to a replacement container could prevent every later command from starting.
This change makes the receiver create missing parent directories before it applies a file or symbolic link. The same stream now works like this:
If an ancestor already exists as the wrong kind of entry, the incoming child still tells us that the sender sees a directory there. For example:
Replacing a symbolic link removes the link without removing its target. Missing ancestors that are shared with a registered read-only mount can also be created. Entries that overlap the read-only mount are still reported as skipped instead of aborting the batch.
The behavior applies to both sync directions. Streaming pulls use
applyChanges, while pushes use the synchronousapplyChangesSyncpath. Both now recover from child-before-parent ordering without changing the cursor, database schema, or RPC format.The regression tests cover files and symbolic links through both apply paths. They also cover local files and symbolic links blocking an incoming parent directory, preservation of a symbolic link target, and parent creation next to a read-only mount.
Run the package tests locally with:
npm run build npm test --workspace @cloudflare/dofsFormatting, linting, type checking, and the workspace build were also run successfully. A patch changeset records the behavior change.