Batch sync path and index tombstone scans - #146
Conversation
🦋 Changeset detectedLatest commit: dba05e8 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 |
896fcaf to
40fdd51
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| UNION ALL | ||
| SELECT w.target, w.link, w.depth + 1, d.name, d.parent_inode | ||
| FROM walk w | ||
| JOIN vfs_dirents d ON d.child_inode = w.parent_inode | ||
| WHERE w.parent_inode <> ? |
There was a problem hiding this comment.
🟡 Cyclic dirents never terminate resolution
pathsOfMany recurses forever when corrupted dirents form a parent cycle. Unlike pathOf, this walk has neither a visited set nor depth bound. A change scan containing that inode can exhaust SQLite resources instead of omitting the unreachable path.
Learn more
The filesystem schema does not enforce an acyclic parent graph, so the existing resolver treats cycles as possible corruption. The recursive term follows each parent without recording visited inodes or limiting depth. A cycle therefore keeps producing rows and prevents db.all from returning. The prior pathOf implementation bounded the same upward walk and returned no path when the root was never reached. Because coalesceChanges now resolves all touched inodes through this CTE, one cyclic chain blocks the entire change scan.
Example: Suppose inode 20 is named under directory 10, directory 10 is named under 11, and directory 11 is named under 10. Resolving inode 20 produces the sequence 20 → 10 → 11 → 10 → 11 indefinitely. The expected result is to omit inode 20 as unreachable, matching pathsOf.
Recommended fix: Carry a visited-inode set or equivalent cycle marker in walk and stop recursion before revisiting a parent. Also retain an explicit depth ceiling so malformed acyclic chains preserve the bounded behavior of pathOf.
Was this helpful? React with 👍 or 👎 to provide feedback.
commit: |
A push tick resolved every touched inode to its path(s) with one dirent lookup per ancestor, issuing O(N x depth) statements. On a node_modules-sized change set that is tens of thousands of round-trips, and the cost is the statement count rather than the index: every one of those lookups was already a covering-index hit. pathsOfMany resolves a batch of inodes in a fixed number of recursive-CTE statements, seeding one walk per (target, dirent) so hardlink names all reach the wire. Measured on a node_modules-shaped tree: 6,300 statements to 2 at 900 files, 31,500 to 9 at 4,500 files, with output identical to the per-inode implementation for every inode. The tombstone scan (rev > ? AND op = 'delete' GROUP BY path) had no index it could drive both halves from, so the planner scanned vfs_changes in path order, ignored the rev predicate, and read the whole table to return the few rows in the watermark window. Schema v8 adds vfs_changes_by_op_rev; the upgrade is index-only and rewrites no rows. pathsOf and pathOf are retained: fs/rename, fs/rm, fs/writeFile and sync/apply still resolve a single inode.
40fdd51 to
dba05e8
Compare
1 participant
The walk to determine which files changed during a sync now happens once for a whole batch instead of once per file. The database walks the tree for every changed file once:
The names that come back are the same, including the awkward case of one file with several names. The old single-file version stays for callers that only ever need one.
The second problem was the scan that finds recently deleted files. Finding deletions newer than a marker would read the entire change log to return the handful of rows that were new. An index on the two columns the query filters by lets it jump straight to the recent entries:
On a log of 40,000 rows, returning the same 50 rows went from 11 ms to 0.06 ms.