Skip to content

feat: Collect blocking errors per batch - #2442

Merged
taratatach merged 3 commits into
masterfrom
feat/handle-multiple-sync-errors
Jul 3, 2026
Merged

feat: Collect blocking errors per batch#2442
taratatach merged 3 commits into
masterfrom
feat/handle-multiple-sync-errors

Conversation

@taratatach

@taratatach taratatach commented Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

syncBatch stopped at the first apply throw: the outer catch
handled a single SyncError and every later change in the batch
waited for the failed change's retry delay. The GUI can display N
alerts but only ever received one.

The loop now catches per-apply. Each failed change is registered
via registerBlockingCause (accumulating causes and alerts);
independent changes after the failure are still applied. Dependents
of a failed change are skipped for this cycle via blockedIds, a
local Set cross-referenced with DependencyGraph.directPrerequisites.
localSeq is frozen as soon as blockedIds is non-empty so the
failed change re-enters the feed on the next cycle; already-applied
independents come back as no-ops. A single scheduleRetry at the
end of the batch starts one global retry timer with
minRetryDelay(causes).

handleSyncError returns a 'blocked' | 'skipped' | 'recovered' | 'fatal' tag and no longer touches localSeq — the loop owns it.
apply returns Promise<void> and no longer calls
setLocalSeq for the same reason.

Fixes #2439 and #2441

Please make sure the following boxes are checked:

  • PR is not too big
  • it improves UX & DX in some way
  • it includes unit tests matching the implementation changes
  • it includes scenarios matching a new behaviour or has been manually tested
  • it includes relevant documentation

@taratatach taratatach self-assigned this Jul 1, 2026
taratatach added 3 commits July 3, 2026 11:56
@taratatach
refactor: Extract blocking-cause helpers
eb6ab55 
  `blockSyncFor` mixed four concerns: `lifecycle.block()`,
  `_blockedCauses.set`, the retry `setInterval`, and the
  `user-alert`/`offline` side effects. This is fine for a single
  cause but blocks the multi-error feature: we need to register N
  causes (with their alerts) before starting a single retry timer.

  Split `blockSyncFor` into `registerBlockingCause` (cause
  registration + side effects, no lifecycle, no interval) and
  `scheduleRetry` (`lifecycle.block` + a single `setInterval`
  whose delay is `minRetryDelay(causes)`). `blockSyncFor` becomes
  a thin wrapper over the two so existing call sites (RemoteWatcher,
  `syncBatch` catch) keep their behavior unchanged.

  `minRetryDelay` picks the shortest delay across causes so retry
  happens soonest; `retryAll` already iterates every cause on each
  tick, so one global interval is enough.
@taratatach
refactor: Extract handleSyncError method
d6ad182 
  The `catch` block of `syncBatch` held a ~120-line switch on
  `err.code` that interleaved logging, blocking, skipping, conflict
  creation, and recovery for the failed change. Extracting it into
  `handleSyncError(err, change)` makes the disposition table
  readable on its own and prepares the ground for the multi-error
  loop, which needs to call it per failed change rather than once
  for the whole batch.

  Pure move: the switch, the log level selection, and the
  `blockSyncFor`/`skipChange`/`createConflict` calls are
  unchanged. The outer `catch` now just guards `willStop` and
  non-`SyncError` throws, then delegates. The loop still breaks
  at the first throw — multi-error collection comes next.
@taratatach
feat: Collect blocking errors per batch
bce0b0e 
  `syncBatch` stopped at the first `apply` throw: the outer `catch`
  handled a single `SyncError` and every later change in the batch
  waited for the failed change's retry delay. The GUI can display N
  alerts but only ever received one.

  The loop now catches per-`apply`. Each failed change is registered
  via `registerBlockingCause` (accumulating causes and alerts);
  independent changes after the failure are still applied. Dependents
  of a failed change are skipped for this cycle via `blockedIds`, a
  local `Set` cross-referenced with `DependencyGraph.directPrerequisites`.
  `localSeq` is frozen as soon as `blockedIds` is non-empty so the
  failed change re-enters the feed on the next cycle; already-applied
  independents come back as no-ops. A single `scheduleRetry` at the
  end of the batch starts one global retry timer with
  `minRetryDelay(causes)`.

  `handleSyncError` returns a `'blocked' | 'skipped' | 'recovered' |
  'fatal'` tag and no longer touches `localSeq` — the loop owns it.
  `apply` returns `Promise<void>` and no longer calls
  `setLocalSeq` for the same reason.
@taratatach
taratatach force-pushed the feat/handle-multiple-sync-errors branch from 1e56f2c to bce0b0e Compare July 3, 2026 10:32
@taratatach taratatach changed the title Feat/handle multiple sync errors feat: Collect blocking errors per batch Jul 3, 2026
@taratatach
taratatach marked this pull request as ready for review July 3, 2026 10:34
@taratatach
taratatach merged commit 5954011 into master Jul 3, 2026
29 of 39 checks passed
@taratatach
taratatach deleted the feat/handle-multiple-sync-errors branch July 3, 2026 13:24
taratatach added a commit that referenced this pull request Jul 3, 2026
@taratatach
refactor: Stub scheduleRetry in incompat tests
3f385ee 
  The platform incompatibilities integration tests stubbed
  `blockSyncFor` to assert which changes got blocked, but the
  per-batch error collection (#2442) means blocking causes now flow
  through `scheduleRetry`, called once at the end of the batch with
  the array of accumulated causes. Stubbing `blockSyncFor` no
  longer reflects what the sync actually does.

  We now stub `scheduleRetry` and have `shouldHaveBlockedFor`
  assert it was called with a causes array containing a cause
  matching the expected path and `IncompatibleDoc` code. sinon's
  `calledWithMatch` uses `deepEqual` (full equality) for array
  expectations rather than partial matching, so we use
  `sinon.match.some(sinon.match({...}))` to assert the array
  contains at least one element partially matching the expected
  cause. Failures are asserted via `should(bool).be.true(msg)` with
  a self-contained message listing the actual blocked paths, since
  sinon's default formatter dumps the matcher internals
  (`{ test, message }`) and is unreadable.
@taratatach taratatach mentioned this pull request Jul 3, 2026
Merged
5 tasks
taratatach added a commit that referenced this pull request Jul 4, 2026
@taratatach
fix: Chain moveFrom through incompatible moves (#2444)
1389fee 
  When a remote move renamed `i` to an incompatible path `i*`, then
  again to a compatible path `z`, `move` set `dst.moveFrom = src` (`i*`),
  pointing at a path never materialized on the filesystem. The local
  watcher then tried to move a non-existent `i*` to `z` and failed.

  `move` now chains through `src.moveFrom` when the local side is not
  up-to-date, so `dst.moveFrom` records the actual local path to move
  from (the original `i`) instead of the intermediate incompatible
  path. When the local side is already up-to-date or `src` has no
  `moveFrom`, it uses `src` directly as before.

  The platform incompatibilities integration tests are adapted to the
  per-batch error collection (#2442): `blockSyncFor` is no longer the
  right stub since blocking causes now flow through `scheduleRetry`,
  called once per batch with the accumulated causes array. The new
  `shouldHaveBlockedFor` uses `sinon.match.some` to assert the array
  contains a matching cause, and reports actual blocked paths on
  failure. New integration cases cover the
  compatible → incompatible → compatible rename sequence.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@taratatach taratatach

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Don't stop synchronization of a batch at the first error

1 participant

@taratatach