super-productivity/docs/sync-and-op-log/contributor-sync-model.md
Johannes Millan 0238b5a729
fix(sync): fence in-flight sync cycles across destructive config changes (#9088)
* fix(sync): fence in-flight sync cycles across config changes (#9074)

Destructive config changes (encryption enable/disable/password change,
provider/account switch) blocked NEW sync cycles but neither drained nor
cancelled a cycle already mid-await, which could then apply remote ops,
upload, acknowledge, or advance the cursor against the new epoch/target
(cross-epoch/cross-provider contamination, mixed-key server history).

Fix, per the issue's KISS sketch:

- Monotonic sync epoch on SyncProviderManager, bumped AFTER each
  provider switch / target-moving config write / bypass ingress, and at
  runWithSyncBlocked entry. Content-only saves do not bump.
- Every cycle entry point (main sync, immediate upload, WS download,
  force upload) captures the epoch with its cycle-guard claim and
  threads it as fenceEpoch.
- Provider I/O is fenced in one choke point: getOperationSyncCapable()
  returns a per-cycle delegate that re-asserts the epoch before every
  provider call (uploads, downloads, all setLastServerSeq cursor
  writes). Local writes (apply-lock closures incl. the full-state path,
  deferred acks, hydration, migration appends, rejected-ops handling,
  raw-rebuild resume) re-assert at the call site.
- Stale completions throw SyncEpochChangedError, handled everywhere as
  a benign abort (no error snack; UNKNOWN_OR_CHANGED) — each abort
  point lands in a crash-equivalent state by design.
- runWithSyncBlocked is serialized, sets the block flag FIRST, bumps
  the epoch, then drains the main sync AND the cycle-guard side
  channels (bounded, throws on timeout) — the fence cannot recall
  request bytes already on the wire, so destructive remote writes wait
  for the stale cycle to settle. ImmediateUploadService now routes
  through WrappedProviderService instead of a raw cast.

Closes the audit findings C1-1 (ORCH-1) and C1-2 (SWITCH-2).

* fix(sync): don't bump the sync epoch on first-time setup (#9074)

Every conflict-dialog E2E timed out on `SyncEpochChangedError (1 → 2)`:
the first-ever provider activation and the first-ever config save (no
previous privateCfg, so isSyncTargetChanged reports a target change)
both bumped the epoch, racing the fresh config's first sync into a
spurious abort — the dialog-producing cycle itself was fenced.

First-time setup has no OLD target an in-flight cycle could be running
against (a pre-activation cycle sees getActiveProvider() === null and
exits; a pre-first-save cycle is not ready), so these bumps fence
nothing and only cause false positives. Gate them: bump on a config
save only when a previous config existed, and on activation only when
a previous provider was active. Real switches (X→Y, X→null) and real
target moves still bump; the cache-invalidation emission keeps its
true-on-first-save semantics untouched.

* fix(sync): read the (provider, epoch) fence pair in one sync block (#9074)

The SuperSync provider-switch E2E still aborted its first post-switch
sync: the cycle captured the fence epoch at its guard claim but fetched
the provider object several awaits later, so a switch completing in
between handed the cycle the NEW provider with a STALE epoch — the
fence then aborted a cycle that was actually running against the new
target.

The consistency rule is pair atomicity, not claim atomicity: a switch
swaps the provider object and bumps the epoch in one synchronous block,
so reading getActiveProvider() and syncEpoch in one synchronous block
always yields a consistent pair (old+old aborts on the later bump;
new+new proceeds). Move the capture to the provider read in the main
sync and WS-download cycles (immediate upload aligned for uniformity —
it was same-block already only by accident of having no await between).
2026-07-16 22:34:00 +02:00

7.7 KiB

The Contributor Sync Model

The one thing to understand before writing any effect, reducer, or bulk dispatch that touches synced state.

Super Productivity syncs by replaying an operation log. Almost every sync correctness rule you will hit is a facet of a single invariant:

One user intent = exactly one operation. Replayed and remote operations must never re-trigger effects.

Reducers must run for remote/replayed operations (that is how state is rebuilt). Effects must not — the UI side effect (snack, sound, navigation) already happened on the originating client, and any cascading change is already its own entry in the operation log. Re-running effects on replay duplicates side effects and emits phantom operations that conflict with sync.

Everything below is that invariant applied at three points.


Boundary 1 — The action boundary

Effects inject LOCAL_ACTIONS, never inject(Actions).

LOCAL_ACTIONS is the standard actions stream with meta.isRemote filtered out (src/app/util/local-actions.token.ts). Remote/replayed operations are applied as one bulkApplyOperations action; LOCAL_ACTIONS ensures your effect only sees genuine local user intent.

  • Default for all effects: private _actions$ = inject(LOCAL_ACTIONS);
  • The only legitimate exception uses ALL_ACTIONS and handles isRemote itself: operation-log.effects.ts (captures/persists every action). You are almost certainly not adding a second.
  • Remote archive side effects are not an ALL_ACTIONS case: archive-operation-handler.effects.ts itself uses LOCAL_ACTIONS; the remote-client archive writes/deletes are driven separately by OperationApplierServiceArchiveOperationHandler.

Enforced by local-rules/no-actions-in-effects — you cannot get this wrong; the linter rejects inject(Actions) / Actions imports in *.effects.ts.

Boundary 2 — The selector boundary

Selector-driven effects must guard with skipDuringSyncWindow().

An effect that reacts to a selector (store state) instead of a specific action bypasses Boundary 1 entirely — it fires on every store change, including hydration and sync replay. Two timing gaps (initial startup before first sync; the post-sync re-evaluation window) make such effects emit operations with stale vector clocks that immediately conflict.

  • Use skipDuringSyncWindow() for selector-based effects that modify frequently-synced entities or perform "repair"/"consistency" work.
  • The narrower skipWhileApplyingRemoteOps() / HydrationStateService.isApplyingRemoteOps() exist for finer control.
  • Prefer action-based effects. A selector-based effect is the intuitive-but-usually-wrong choice; reach for it only when there is no action to key off.

Enforced by local-rules/require-hydration-guard (existing rule).

The atomicity rule — one intent, one op

Multi-entity changes are meta-reducers, not effects. Bulk dispatch loops yield.

  • A change that touches more than one entity for a single user intent (e.g. deleting a tag also removing it from every task) must be one reducer pass so it becomes one operation. Put it in src/app/root-store/meta/task-shared-meta-reducers/, not in an effect that dispatches a fan-out of follow-up actions. An effect-based fan-out emits N operations for one intent and re-runs on replay (a restatement of Boundary 1).
  • store.dispatch() is non-blocking. After a loop of 50+ dispatches, add await new Promise((r) => setTimeout(r, 0)) so captured operations don't lose intermediate state.

⚠️ local-rules/no-multi-entity-effect (warn) flags this heuristically — it catches the array-literal fan-out shape (map(() => [a(), b()])), not every multi-entity dispatch (e.g. a of(a(), b()) varargs fan-out slips past). The blessed pattern is a task-shared-meta-reducers/ reducer.


Decision table — "I'm writing an effect"

Question Answer Linter
Does it inject the actions stream? Use LOCAL_ACTIONS (not Actions) no-actions-in-effects (error)
Does it react to a selector instead of an action? Add skipDuringSyncWindow() require-hydration-guard (error)
Does one user intent change >1 entity? Make it a meta-reducer, not an effect ⚠️ no-multi-entity-effect (warn)
Does it dispatch in a loop of 50+? await new Promise(r => setTimeout(r, 0)) after the loop — (convention)

Two of the three are mechanically enforced — you do not need to memorize them, only understand why (the invariant at the top).


The sync-epoch fence (#9074)

A sync cycle spans many awaits; a destructive config change (provider/account switch, folder move, encryption enable/disable/password change) can land in any of those gaps. A stale cycle must not apply, upload, acknowledge, or advance the cursor against the new target/epoch afterwards.

  • SyncProviderManager.syncEpoch is a monotonic counter, bumped after each such change completes (and at runWithSyncBlocked entry, which additionally blocks new cycles first and then drains running ones, bounded). First-time setup (no previous config / first provider activation) does NOT bump — there is no old target to fence, and the bump would race the fresh config's first sync into a spurious abort.
  • Every cycle reads the (provider, epoch) pair in one synchronous block (a switch swaps the object and bumps the epoch in one synchronous block on its side, so a same-block read is always consistent) and threads the epoch as fenceEpoch. Capturing earlier — e.g. at the cycle claim — lets a switch complete in the awaits between and hands the cycle the new provider with a stale epoch: a spurious abort of the first post-switch sync.
  • Provider I/O is fenced in one place: getOperationSyncCapable(provider, { fenceEpoch }) returns a per-cycle delegate that re-asserts the epoch before every provider call. Local writes (apply inside the lock closures, ack persists, hydration, migration appends, rejected-ops handling, rebuild resume) re-assert via assertSyncEpochUnchanged at the call site.
  • A failed assert throws SyncEpochChangedError, handled at every entry point as a benign abort (no error snack, UNKNOWN_OR_CHANGED) — each abort point is crash-equivalent by design (deferred acks re-upload, a behind cursor re-downloads with dedup).

An unthreaded flow is an UNFENCED flow: fenceEpoch: undefined disables the assert. When adding a new sync entry point, capture and thread the epoch; when adding a new local write inside a cycle, add an assert before it. Deliberately unthreaded today: forceUploadLocalState / the USE_LOCAL/USE_REMOTE conflict-resolution flows (covered by the encryption flag + cycle guard), and key-recovery config writes (content-only, must NOT bump).


Why (deeper)