Skip to content

[claude] Throw a SyncException that says what failed and what landed - #156

Draft
myieye wants to merge 1 commit into
mainfrom
sync-exception
Draft

myieye wants to merge 1 commit into
mainfrom
sync-exception

Conversation

@myieye

@myieye myieye commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

[Claude, autonomous]

SyncWith/SyncMany throw a SyncException with PullError, PushError and Applied when applying commits fails, instead of the raw error or an AggregateException.

  • Breaking: a connection lost while pushing now arrives as SyncException.PushError, so FW Lite's offline check (HttpRequestException or OperationCanceledException) must handle it in the Lexbox PR that bumps Harmony.
  • Nothing uses Applied yet. The payoff is FW Lite acting on the direction that landed after a partial sync (notifications for pulled commits, stamping pushed ones).

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • 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 added this pull request to stack #150 October 26, 2026 09:46
@myieye
myieye removed this pull request from stack #150 October 26, 2026 10:12
@myieye
myieye changed the base branch from crdt-sync-service to sync-logging October 26, 2026 10:12
@myieye
myieye added this pull request to stack #158 October 26, 2026 10:12
var pullError = await pullTask;
var pushError = await pushTask;
if (pullError is not null || pushError is not null)
throw new SyncException(missingFromLocal, pullError, missingFromRemote, pushError);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is throwing here the right thing? or should we just change the change of SyncResults?

When applying commits fails, SyncWith and SyncMany now throw a sealed
SyncException with PullError, PushError and Applied (the commits that
did land), instead of rethrowing the raw error or an AggregateException.
Its message names the failed direction, both counts and the underlying
errors. Callers can now stamp pushed commits as synced, or process
pulled ones, after a partial sync. The contract is documented on
ISyncable.

Breaking: errors from applying commits no longer reach callers raw.
FW Lite's offline check (HttpRequestException or
OperationCanceledException around SyncWith) must also look at
SyncException.PushError, in the same Lexbox PR that bumps harmony.
Errors reading sync state or changes still propagate unwrapped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@myieye
myieye removed this pull request from stack #158 October 28, 2026 05:48
@myieye
myieye changed the base branch from sync-logging to main October 28, 2026 05:48
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