fix: never duplicate an event's reminders after a busy calendar database #34

A provider that is busy may commit a batch up to a yield point and then
fail it. The engine retried every group of such a batch and relied on
the groups' asserts to refuse one that was already committed, but a
download into an unchanged event asserts only the event's own columns:
when it had only added a reminder or an attendee, the retry added it a
second time.

A download is now planned again from a new read after a failed batch
instead of being sent again, which turns a committed group into an echo.
The fake provider can commit a batch up to a yield point to test it.

src/device-sync/__tests__/engine/internals.test.ts

@@ -157,6 +157,18 @@ describe('provider batches', () => {
     expect(store.committed.accounts.a.groups).toEqual(['a/g1']);
   });
 
+  it('plans a download again after a failed batch, which may have been committed up to a yield point', async () => {
+    let failures = 1;
+    const p = port((ops) => (failures-- > 0 ? { ok: false, reason: 'provider', message: 'failed after a yield point' } : ok(ops)));
+    const planned: Work = { group: { ref: 'a/1', ops: [{ op: 'update', table: 'events', id: 1, values: { title: 'Planned from the new read' } }] } };
+    const download: Work = { group: { ref: 'a/1', ops: [insert('data', { stale: 'group' })] }, fresh: true, replan: async () => planned };
+    const other: Work = { group: { ref: 'a/2', ops: [insert('raw_contacts')] } };
+
+    await writer(p).write([download, other]);
+
+    expect(p.sent.slice(1)).toEqual([planned.group.ops.map((op) => ({ ...op, yieldAllowed: true })), buildBatch([other.group])]);
+  });
+
   it('skips groups that only assert', async () => {
     const p = port(ok);
     await writer(p).write([{ group: { ref: 'g', ops: [{ op: 'assert', table: 'raw_contacts', id: 1, values: { dirty: 0 } }] } }]);

src/device-sync/__tests__/engine/real-planners.test.ts

@@ -236,6 +236,29 @@ describe('device sync engine with the real planners', () => {
     });
   });
 
+  describe('a batch the provider committed up to a yield point before it failed', () => {
+    const alert = { al1: { '@type': 'Alert', trigger: { '@type': 'OffsetTrigger', offset: '-PT15M' }, action: 'display' } };
+
+    it("never inserts a clean event's new reminder twice", async () => {
+      const h = real();
+      const event = (uid: string, title: string) =>
+        h.server.addEvent('a', { uid, title, start: '2026-10-06T12:00:00', duration: 'PT1H', timeZone: 'Europe/Berlin', calendarIds: { [h.calendar]: true } });
+      const lunch = event('lunch-uid', 'Lunch');
+      const retro = event('retro-uid', 'Retro');
+      await h.run(CALENDAR_AUTHORITY);
+      // Only an alert for Lunch (its row's columns stay), a new title for Retro: one download batch, two groups.
+      h.server.serverUpdate('CalendarEvent', 'a', lunch, { alerts: alert });
+      h.server.serverUpdate('CalendarEvent', 'a', retro, { title: 'Retrospective' });
+      h.device.partialCommitNextBatch(1, 'assert', (ops) => ops.some((op) => op.op === 'insert' && op.table === 'reminders'));
+
+      expect((await h.run(CALENDAR_AUTHORITY)).outcome).toBe('ok');
+
+      const row = h.events().find((e) => e.title === 'Lunch')!;
+      expect(h.device.rows('reminders').filter((r) => Number(r.event_id) === Number(row._id)).map((r) => r.minutes)).toEqual([15]);
+      expect(h.events().map((e) => e.title).sort()).toEqual(['Lunch', 'Retrospective']);
+    });
+  });
+
   describe('a device edit the server refuses (forbidden)', () => {
     it('puts a contact back in place, keeping what only the device knows', async () => {
       const h = real();

src/device-sync/__tests__/fakes/fake-provider.ts

@@ -258,6 +258,7 @@ export class FakeDeviceProviders {
   private nextId = 1;
   private nextPhotoFile = 1000;
   private pendingFailure: { reason: BatchFailure; message: string } | null = null;
+  private partialFailure: { groups: number; reason: BatchFailure; when: (ops: ProviderOp[]) => boolean } | null = null;
   private beforeBatch: Array<() => void> = [];
   /** Every batch applied or refused, for assertions on write counts. */
   readonly batches: Array<{ account: string; authority: Authority; ops: ProviderOp[]; ok: boolean }> = [];
@@ -301,6 +302,16 @@ export class FakeDeviceProviders {
     this.beforeBatch.push(fn);
   }
 
+  /**
+   * The next applyBatch that `when` matches behaves like a provider that
+   * yielded under contention: the ops before its `groups + 1`-th yield point
+   * (its first `groups` groups) are committed, then the batch fails with
+   * `reason` and the rest is rolled back.
+   */
+  partialCommitNextBatch(groups: number, reason: BatchFailure = 'assert', when: (ops: ProviderOp[]) => boolean = () => true): void {
+    this.partialFailure = { groups, reason, when };
+  }
+
   /** Removes an account the way AccountManager does: the providers drop all its rows and SyncState. */
   removeAccount(accountName: string): void {
     for (const table of ['raw_contacts', 'groups', 'settings', 'calendars', 'colors'] as const) {
@@ -517,6 +528,19 @@ export class FakeDeviceProviders {
     }
     for (const hook of this.beforeBatch.splice(0)) hook();
 
+    if (this.partialFailure?.when(ops)) {
+      const { groups, reason } = this.partialFailure;
+      this.partialFailure = null;
+      const starts = ops.flatMap((op, i) => (i === 0 || (op as { yieldAllowed?: boolean }).yieldAllowed ? [i] : []));
+      const cut = starts[groups] ?? 0;
+      if (cut > 0) {
+        const committed = this.applyBatch(accountName, authority, ops.slice(0, cut));
+        if (!committed.ok) return committed;
+      }
+      record(false);
+      return { ok: false, reason, message: `injected ${reason} after a yield point` };
+    }
+
     const snapshot = {
       tables: cloneTables(this.tables),
       syncState: new Map(this.syncState),

src/device-sync/engine/batch.ts

@@ -11,10 +11,12 @@
  *   and never a yield point: the stored state can't get ahead of its rows.
  *
  * A failed batch may have been committed up to a yield point the provider
- * took, so its groups are retried one by one (their asserts make a retry of
- * a committed group fail harmlessly); a group that fails an assert is
- * re-read and re-planned up to `maxReplans` times, then given up. `tooLarge`
- * splits the batch; `permission` ends the run.
+ * took, so its groups are retried one by one: a group planned from the rows
+ * as they are (`fresh`, a download) is planned again from a new read, any
+ * other is sent again (its asserts make a retry of a committed group fail);
+ * a group that fails an assert is re-read and re-planned up to `maxReplans`
+ * times, then given up. `tooLarge` splits the batch; `permission` ends the
+ * run.
  *
  * A group whose rows the SyncState describes (a group row: `groups`) carries
  * that `state` change: every batch that applies such a group, a retry
@@ -36,6 +38,11 @@ export interface Work {
   failed?(reason: BatchFailure, message: string): void | Promise<void>;
   /** What the SyncState lists changes with these rows: stored by the batch that applies them. */
   state?: StateChange;
+  /**
+   * Planned from the rows as they are (a download), so `replan` gives the same group or its echo: after a failed
+   * batch it is planned again instead of sent again (its asserts need not tell a committed group).
+   */
+  fresh?: boolean;
 }
 
 export interface BatchWriterOptions {
@@ -210,7 +217,14 @@ export class BatchWriter {
       await this.retryAlone(works[0], result.reason, result.message);
       return;
     }
-    for (const work of works) await this.applyAlone(work, 0);
+    for (const work of works) {
+      if (work.fresh && work.replan) {
+        const next = await work.replan();
+        if (next) await this.applyAlone(next, 1);
+      } else {
+        await this.applyAlone(work, 0);
+      }
+    }
     if (tail) await this.applyTail(tail);
   }
 

src/device-sync/engine/item-sync.ts

@@ -642,6 +642,7 @@ export abstract class ItemSync {
     return {
       group,
       state: this.listing(kind, acct, object.id, true),
+      fresh: true,
       applied: () => {
         if (plan.effect === 'insert') this.env.report.stats.downloaded.created++;
         else this.env.report.stats.downloaded.updated++;