Fix local-mount syncBack full-tree walk after healthy autosync#138
Merged
Conversation
📝 WalkthroughWalkthroughAuto-sync now tracks mount-side dirty paths, exposes flushPending/getDirtyPaths/watchersHealthy, widens MountHandle.syncBack to accept optional path filters, and makes launchOnMount gate fast-path final sync-back on watcher readiness—falling back to a full sync when watchers are degraded. ChangesAuto-sync dirty-path tracking and sync-back optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f6dd5be2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+95
to
+97
| const finalSynced = await handle.syncBack({ | ||
| signal: opts.shutdownSignal, | ||
| ...(finalSyncBackPaths ? { paths: finalSyncBackPaths } : {}), |
There was a problem hiding this comment.
When watchersHealthy() is true, finalize() always passes paths to syncBack, including an empty array, which skips the full tree walk entirely. After this commit, autoSync.stop() also skips runReconcile() on the healthy path (it only flushes queued watcher paths), and launchOnMount() never waits for autoSync.ready() before spawning the child. In short-lived runs, mount edits that occur before any watcher path is queued can now be dropped at shutdown because neither a full reconcile nor a full syncBack() runs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/local-mount/src/mount.ts (1)
204-204: 💤 Low valueMinor inconsistency:
setTimeoutvssetImmediatefor yielding.This line uses
setTimeout(resolve, 0)whileyieldToEventLoop()at line 351 usessetImmediate. Both yield control, butsetImmediateexecutes after I/O callbacks and before timers, whilesetTimeout(..., 0)schedules a timer callback. The functional difference is negligible here, but usingsetImmediateconsistently (or extracting to the existing helper) would be cleaner.♻️ Suggested consistency fix
if (signal && syncedForFile > 0 && !signal.aborted) { - await new Promise<void>((resolve) => setTimeout(resolve, 0)); + await new Promise<void>((resolve) => setImmediate(resolve)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-mount/src/mount.ts` at line 204, Replace the one-off await new Promise<void>((resolve) => setTimeout(resolve, 0)) with the existing event-loop yielding utility to keep behavior consistent; locate the statement in mount.ts (the await new Promise... call) and either call the helper function yieldToEventLoop() used elsewhere or use setImmediate via the same pattern as yieldToEventLoop(), ensuring the same scheduling semantics as the usage at line 351.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/local-mount/src/mount.ts`:
- Line 204: Replace the one-off await new Promise<void>((resolve) =>
setTimeout(resolve, 0)) with the existing event-loop yielding utility to keep
behavior consistent; locate the statement in mount.ts (the await new Promise...
call) and either call the helper function yieldToEventLoop() used elsewhere or
use setImmediate via the same pattern as yieldToEventLoop(), ensuring the same
scheduling semantics as the usage at line 351.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2ef517f-7ba4-497d-bd25-b4d09c8b63bc
📒 Files selected for processing (11)
.trajectories/compacted/compact_5kbxmj590rd2_2026-05-11.json.trajectories/compacted/compact_5kbxmj590rd2_2026-05-11.md.trajectories/index.jsonpackages/local-mount/CHANGELOG.mdpackages/local-mount/README.mdpackages/local-mount/src/auto-sync.test.tspackages/local-mount/src/auto-sync.tspackages/local-mount/src/launch.test.tspackages/local-mount/src/launch.tspackages/local-mount/src/mount.test.tspackages/local-mount/src/mount.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
willwashburn
force-pushed
the
fix-134-dirty-syncback
branch
from
83f6dd5 to
4a50f74
Compare
Address @chatgpt-codex-connector P1 on #138: even when watchers stay healthy, the dirty-path set can be empty at shutdown — short-lived runs where the watcher hadn't enqueued anything before stop(), or edits the watcher missed via filesystem races. The previous code took the fast path unconditionally and passed `paths: []` to handle.syncBack(), which iterates zero files and skips the walk entirely. Combined with autoSync.stop() no longer running a full reconcile on the healthy path (it only flushes queued watcher paths), those mount edits would never propagate to projectDir. Only take the fast path when there's at least one dirty path. An empty dirty set falls through to the full walk as a safety net. The cost is trivial on no-change runs (the walker exits with synced=0 quickly) and restores the dropped-edits invariant. Add a regression test pinning the contract: watchersHealthy=true + getDirtyPaths empty → syncBack called without `paths`, i.e. full walk. Defer the mount.ts setTimeout→setImmediate nitpick from coderabbitai: the existing "syncBack returns a partial count when aborted mid-walk" test schedules controller.abort() via setTimeout, and matching the same queue is what makes the abort observable between file syncs. Switching to setImmediate races the abort and the test fails. Added an inline comment explaining the intentional choice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/local-mount/src/launch.test.ts (1)
201-239: ⚡ Quick winAdd explicit
stop()assertion on readiness-failure path.This test checks fallback behavior, but it doesn’t verify watcher teardown after
ready()throws. Adding that assertion would harden leak prevention on the error path.Proposed test hardening
it('falls back to full syncBack when auto-sync readiness fails', async () => { mkdirSync(mountDir, { recursive: true }); let syncBackPaths: string[] | undefined; + let stopCalled = false; const createSpy = vi.spyOn(mountModule, 'createMount').mockResolvedValue({ mountDir, startAutoSync: () => ({ - stop: async () => {}, + stop: async () => { + stopCalled = true; + }, flushPending: async () => 0, reconcile: async () => 0, getDirtyPaths: () => ['tracked.txt'][Symbol.iterator](), watchersHealthy: () => true, totalChanges: () => 0, @@ expect(result.exitCode).toBe(0); expect(syncBackPaths).toBeUndefined(); + expect(stopCalled).toBe(true); } finally { createSpy.mockRestore(); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-mount/src/launch.test.ts` around lines 201 - 239, The test should assert that the watcher teardown is invoked when startAutoSync().ready() throws: replace the inline stop: async () => {} with a spyable async function (e.g., create a stopSpy = vi.fn(async () => {}) and return stop: stopSpy from the mocked startAutoSync object), then after awaiting launchOnMount(...) add expect(stopSpy).toHaveBeenCalled(); keep restore of createSpy as-is. Target symbols: createMount, startAutoSync, stop, and launchOnMount.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/local-mount/src/launch.test.ts`:
- Around line 201-239: The test should assert that the watcher teardown is
invoked when startAutoSync().ready() throws: replace the inline stop: async ()
=> {} with a spyable async function (e.g., create a stopSpy = vi.fn(async () =>
{}) and return stop: stopSpy from the mocked startAutoSync object), then after
awaiting launchOnMount(...) add expect(stopSpy).toHaveBeenCalled(); keep restore
of createSpy as-is. Target symbols: createMount, startAutoSync, stop, and
launchOnMount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 491a1bcd-00a8-41e3-99a7-efa5d2f3d262
📒 Files selected for processing (3)
packages/local-mount/src/launch.test.tspackages/local-mount/src/launch.tspackages/local-mount/src/mount.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/local-mount/src/mount.ts
- packages/local-mount/src/launch.ts
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
2 participants
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.
Summary
Closes #134.
Verification