Skip to content

Sync → One failed file no longer discards the whole pass#112

Merged
revett merged 2 commits into
mainfrom
revett/fix/87
Jul 21, 2026
Merged

Sync → One failed file no longer discards the whole pass#112
revett merged 2 commits into
mainfrom
revett/fix/87

Conversation

@revett

@revett revett commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Resolves #87

Problem

  • A sync pass with any per file failure ended without uploading the manifest or writing state.json, even when every other action succeeded
  • Completed pushes sat in the bucket invisible to every other device, all finished work was replanned from scratch next pass, and a file that fails permanently (locked, oversized, rejected by the provider) wedged sync forever

Changes

  • executeSyncPlan now reports which actions completed and which failed, alongside the per file failures
  • The manifest is derived from completed actions only and is uploaded even when some actions failed, still conditional on the etag, so the bucket records what actually happened
  • A failed pass now returns a snapshot to persist: completed paths keep their new entries, while each failed path reverts to the ancestor's view and is replanned next pass
  • The plugin and integration test harness persist that snapshot on partial failure, keeping the error status and per file logging unchanged

Why

  • One bad file should never hold the rest of the vault hostage; each path now converges independently, the same per path archive advance the Unison specification uses
  • Reverting failed paths to the ancestor is what makes recording progress safe: advancing a failed pull's path would make the unchanged local copy read as a fresh edit and push it over the newer remote version

Greptile Summary

This PR fixes a bug (#87) where a sync pass with any per-file failure discarded all progress — the manifest was never uploaded and state.json was never advanced, so completed pushes sat in the bucket invisible to other devices and a permanently-failing file could wedge sync indefinitely.

  • executeSyncPlan now returns ExecuteResult with completed, failed, and failures arrays, splitting per-action tracking from per-operation failure detail so that a conflict's two-step failure (copy push + pull) is still counted as one logical file failure.
  • The manifest is derived from executed.completed only and is uploaded even on partial failure; revertFailedPaths then reverts each failed action's path to the ancestor's entry in the returned snapshot, ensuring those paths re-plan correctly next pass without risking stale local content being pushed over a newer remote version.

Confidence Score: 5/5

Safe to merge — all identified edge cases converge correctly verified against planSync's same-hash guard.

The revert-to-ancestor invariant is sound: planSync correctly treats local==remote!=ancestor as already converged, which is exactly the state a partially-succeeded conflict action leaves after revertFailedPaths runs. Manifest-upload-before-snapshot ordering is correct. Test coverage is thorough with three new end-to-end test cases.

No files require special attention.

Important Files Changed

Filename Overview
src/sync/execute.ts Refactored executeSyncPlan to return ExecuteResult with completed/failed/failures arrays; logic extracted into private executeAction helper; clean separation of per-action tracking
src/sync/sync.ts Added revertFailedPaths, changed manifest to use executed.completed, uploads manifest even on partial failure, returns snapshot on partial failure; SyncOutcome failure variant now carries snapshot
src/main.ts Plugin now persists snapshot on partial failure (when snapshot != null), keeping completed progress across passes
src/sync/sync.test.ts Three new integration-level tests covering the core #87 scenarios (failed push, failed pull, failure message counting); existing mid-sync test updated to use outcome.snapshot for retry and to verify manifest is uploaded on partial failure
src/sync/execute.test.ts Existing tests updated for new return shape; new test for conflict with failed copy push verifying completed/failed arrays and that action path is tracked correctly
src/sync/sync.itest.ts Integration test harness updated to mirror plugin logic: persists snapshot on partial failure so integration scenarios exercise the full recovery path

Reviews (2): Last reviewed commit: "Address comment" | Re-trigger Greptile

Comment thread src/sync/sync.ts Outdated
@revett
revett merged commit 58a2200 into main Jul 21, 2026
10 checks passed
@revett
revett deleted the revett/fix/87 branch July 21, 2026 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

One failed file discards the progress of the entire pass

1 participant