diff --git a/src/device-sync/__tests__/engine/internals.test.ts b/src/device-sync/__tests__/engine/internals.test.ts index 9609e1b4..4fee104b 100644 --- a/src/device-sync/__tests__/engine/internals.test.ts +++ b/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 } }] } }]); diff --git a/src/device-sync/__tests__/engine/real-planners.test.ts b/src/device-sync/__tests__/engine/real-planners.test.ts index 03291e0b..bf494cdd 100644 --- a/src/device-sync/__tests__/engine/real-planners.test.ts +++ b/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(); diff --git a/src/device-sync/__tests__/fakes/fake-provider.ts b/src/device-sync/__tests__/fakes/fake-provider.ts index 8fbad6e2..100d5229 100644 --- a/src/device-sync/__tests__/fakes/fake-provider.ts +++ b/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), diff --git a/src/device-sync/engine/batch.ts b/src/device-sync/engine/batch.ts index 2375b86f..a7a454ea 100644 --- a/src/device-sync/engine/batch.ts +++ b/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; /** 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); } diff --git a/src/device-sync/engine/item-sync.ts b/src/device-sync/engine/item-sync.ts index ee169339..6b4c4c2c 100644 --- a/src/device-sync/engine/item-sync.ts +++ b/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++;