Skip to content

Keep applying the other sync direction when one fails - #148

Open
myieye wants to merge 3 commits into
mainfrom
sync-apply-directions-independently
Open

myieye wants to merge 3 commits into
mainfrom
sync-apply-directions-independently

Conversation

@myieye

@myieye myieye commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

[Claude, autonomous]

Apply pulled and pushed commits independently, so a pull that can't be applied (an edit whose create the remote withheld) no longer blocks the push and leaves the sync stuck forever: the push changes the commit counts, so the next sync sends the missing create.

  • Three scenarios that stay stuck even with this fix are skipped tests (known blockers): both sides withhold a create the other needs; the withholding side is ahead by timestamp; no reachable client has the create.
  • A single error is rethrown as is; two (pull and push) come back as an AggregateException. A follow-up replaces that with a SyncException carrying PullError/PushError.
  • SyncWith can now throw after the push landed. FW Lite only stamps SyncDate when it returns, so pushed commits keep showing as pending until a sync fully succeeds.

🤖 Generated with Claude Code

myieye and others added 2 commits October 20, 2026 15:27
When a sync pulls an edit whose create change the remote withheld,
applying the pull throws and the push never runs, so the commit counts
never change and every later sync fails the same way. These tests fail
until both directions are applied independently. Three scenarios that
stay stuck even then are skipped as known blockers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SyncWith and SyncMany now apply pulled and pushed commits independently
and rethrow afterwards: one error as is, several as an
AggregateException. A pull that can't be applied, e.g. an edit whose
create change was withheld, no longer blocks the push, so the next sync
plans differently and can recover. SyncMany now also skips empty
batches, as SyncWith already did.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 54 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 531d0519-23ef-4ae8-996a-06ed39163aaa
📥 Commits

Reviewing files that changed from the base of the PR and between ca7662d and 631f920.

📒 Files selected for processing: 1
  • src/SIL.Harmony/SyncHelper.cs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4fcbccc8-b7f2-4c27-b988-3e5c090c6f52
📥 Commits

Reviewing files that changed from the base of the PR and between a002af1 and ca7662d.

📒 Files selected for processing: 2
  • src/SIL.Harmony.Tests/Syncable/SyncRecoveryTests.cs
  • src/SIL.Harmony/SyncHelper.cs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

SyncWith and SyncMany now continue applying commit batches after an application failure and report collected failures after attempting the synchronization directions. A single failure retains its original exception details; multiple failures produce an AggregateException. New tests cover recovery across repeated sync attempts, SyncMany, and bidirectional failures. Three additional scenarios are added as skipped tests.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to ca766

The change appears ready to merge after normal checks. A failed application no longer prevents the other sync direction from being attempted.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: synchronization continues in the other direction when one direction fails.
Description check Passed The description directly explains the independent application of pulled and pushed commits, error handling, affected behavior, and known skipped scenarios.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@myieye myieye changed the title [claude] Keep applying the other sync direction when one fails Keep applying the other sync direction when one fails Oct 20, 2026
@myieye
myieye added this pull request to stack #150 October 20, 2026 15:34
@myieye
myieye requested a review from hahn-kev October 20, 2026 15:35
@myieye
myieye marked this pull request as ready for review October 20, 2026 15:35
Comment thread src/SIL.Harmony/SyncHelper.cs Outdated
Both batches are computed before either is applied, so the two
directions don't depend on each other. TryAddRange still returns each
direction's error, since awaiting Task.WhenAll would rethrow only the
first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

2 participants