diff --git a/src/device-sync/__tests__/calendar/lifecycle.test.ts b/src/device-sync/__tests__/calendar/lifecycle.test.ts index 038d2d00..9c9e8a4b 100644 --- a/src/device-sync/__tests__/calendar/lifecycle.test.ts +++ b/src/device-sync/__tests__/calendar/lifecycle.test.ts @@ -336,6 +336,34 @@ describe('calendar planner: accepted uploads, merges and deletions', () => { expect(await h.events()).toEqual([]); }); + it('leaves an event the server moved out of every synced calendar before the deletion reached it, and only purges the rows', async () => { + const { h, id } = await synced(single()); + const isSelected = h.ctx.isSelected; + h.ctx = { ...h.ctx, isSelected: (key) => key !== `${ACCT}/x` && isSelected(key) }; + h.fake.user.deleteEvent(id); + // The shadow follows the server although no synced calendar holds the event now. + expect(await h.download(single({ calendarIds: { x: true } }))).toMatchObject({ stillDirty: true, effect: 'update' }); + expect(h.row(id)).toMatchObject({ [Events.DELETED]: 1 }); + + const plan = calendarPlanner.planUpload((await h.local('e1'))!, h.ctx); + expect(plan.kind).toBe('purge'); + if (plan.kind === 'purge') await h.apply(plan.ops); + expect(await h.events()).toEqual([]); + }); + + it('still destroys an event the server moved to another synced calendar, or whose calendar was deselected before the upload', async () => { + const destroy = [{ kind: 'destroy', id: 'e1', uid: 'single-1@example.org' }]; + const moved = await synced(single()); + moved.h.fake.user.deleteEvent(moved.id); + await moved.h.download(single({ calendarIds: { w: true } })); + expect(actionsOf(calendarPlanner.planUpload((await moved.h.local('e1'))!, moved.h.ctx))).toEqual(destroy); + + const deselected = await synced(single()); + deselected.h.fake.user.deleteEvent(deselected.id); + deselected.h.ctx = { ...deselected.h.ctx, isSelected: () => false }; + expect(actionsOf(calendarPlanner.planUpload((await deselected.h.local('e1'))!, deselected.h.ctx))).toEqual(destroy); + }); + it('destroys a deleted event whose create had an unknown outcome by its uid', async () => { const h = await new Harness().setup(); const id = h.fake.user.insertEvent(h.rowIds.get('b')!, { [Events.TITLE]: 'x', [Events.DTSTART]: 0, [Events.DTEND]: 1000, [Events.EVENT_TIMEZONE]: 'UTC', [Events.STATUS]: 1 }); diff --git a/src/device-sync/__tests__/contacts/upload-editors.test.ts b/src/device-sync/__tests__/contacts/upload-editors.test.ts index d4783bef..eb2dd661 100644 --- a/src/device-sync/__tests__/contacts/upload-editors.test.ts +++ b/src/device-sync/__tests__/contacts/upload-editors.test.ts @@ -326,6 +326,36 @@ describe('contacts upload: deleted contacts', () => { expect(h.device.row('raw_contacts', rawId)).toBeUndefined(); }); + it('leaves a card the server moved out of every synced book before the deletion reached it, and only purges the rows', async () => { + const h = new Harness(); + const archive = h.server.addAddressBook(JMAP, { name: 'Archive' }); + h.unselected.add(`${JMAP}/${archive}`); + const { id, rawId } = await h.seed(appleCard()); + h.device.user.deleteContact(rawId); + h.server.serverUpdate('ContactCard', JMAP, id, { addressBookIds: { [archive]: true } }); + await h.applyOk(h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx).ops); + expect(await uploadPlan(h, rawId)).toMatchObject({ kind: 'purge' }); + await h.upload(rawId); + expect(h.device.row('raw_contacts', rawId)).toBeUndefined(); + expect(h.server.get('ContactCard', JMAP, id)).toMatchObject({ addressBookIds: { [archive]: true } }); + }); + + it('still destroys a card the server moved to another synced book, or whose book was deselected before the upload', async () => { + const moved = new Harness(); + const work = moved.server.addAddressBook(JMAP, { name: 'Work' }); + const a = await moved.seed(appleCard()); + moved.device.user.deleteContact(a.rawId); + moved.server.serverUpdate('ContactCard', JMAP, a.id, { addressBookIds: { [work]: true } }); + await moved.applyOk(moved.planner.planDownload(moved.card(a.id), await moved.contact(a.rawId), moved.ctx).ops); + expect(await uploadPlan(moved, a.rawId)).toEqual({ kind: 'upload', actions: [{ kind: 'destroy', id: a.id, uid: appleCard().uid }] }); + + const deselected = new Harness(); + const b = await deselected.seed(appleCard()); + deselected.device.user.deleteContact(b.rawId); + deselected.unselected.add(`${JMAP}/${deselected.book}`); + expect(await uploadPlan(deselected, b.rawId)).toEqual({ kind: 'upload', actions: [{ kind: 'destroy', id: b.id, uid: appleCard().uid }] }); + }); + it('deletes its own create by id once the download found it, not by a uid lookup', async () => { const h = new Harness(); const rawId = h.device.user.insertContact(ACCOUNT, [{ [Data.MIMETYPE]: MimeType.NOTE, data1: 'y' }]); diff --git a/src/device-sync/__tests__/engine/real-planners.test.ts b/src/device-sync/__tests__/engine/real-planners.test.ts index 0652f345..68596146 100644 --- a/src/device-sync/__tests__/engine/real-planners.test.ts +++ b/src/device-sync/__tests__/engine/real-planners.test.ts @@ -358,7 +358,25 @@ describe('device sync engine with the real planners', () => { expect(planned).toEqual([[archive]]); }); - it('still uploads the deletion of an event whose calendar the planner cannot place', async () => { + it('leaves a contact deleted on the device that another client moved out of every synced book', async () => { + const h = real(); + const archive = h.server.addAddressBook('a', { name: 'Archive' }); + h.prefs.contactsSelection[`a/${archive}`] = false; + const [ada] = addServerCards(h, ['Ada Lovelace', 'Grace Hopper']); + await h.run(); + h.device.user.deleteContact(h.contactNamed('Ada Lovelace')!.id); + // Another client files Ada in Archive only before the deletion reached the server. + h.server.serverUpdate('ContactCard', 'a', ada, { addressBookIds: { [archive]: true } }); + + const report = await h.run(); + + expect(report).toMatchObject({ outcome: 'ok', itemErrors: [], stats: { uploaded: { created: 0, updated: 0, deleted: 0 } } }); + expect(h.server.get('ContactCard', 'a', ada)).toMatchObject({ addressBookIds: { [archive]: true } }); + expect(h.contacts().map((c) => c.name)).toEqual(['Grace Hopper']); + }); + + /** Lunch synced, deleted on the device, then filed by another client in Archive, which this device does not sync. */ + async function lunchDeletedAndFiledAway() { const h = real(); const archive = h.server.addCalendar('a', { name: 'Archive' }); h.prefs.calendarSelection[`a/${archive}`] = false; @@ -366,6 +384,31 @@ describe('device sync engine with the real planners', () => { await h.run(CALENDAR_AUTHORITY); h.device.user.deleteEvent(Number(h.events()[0]._id)); h.server.serverUpdate('CalendarEvent', 'a', lunch, { calendarIds: { [archive]: true } }); + return { h, archive, lunch }; + } + + it('leaves an event deleted on the device that another client moved out of every synced calendar', async () => { + const { h, archive, lunch } = await lunchDeletedAndFiledAway(); + + const report = await h.run(CALENDAR_AUTHORITY); + + expect(report).toMatchObject({ outcome: 'ok', itemErrors: [], stats: { uploaded: { created: 0, updated: 0, deleted: 0 } } }); + expect(h.server.get('CalendarEvent', 'a', lunch)).toMatchObject({ calendarIds: { [archive]: true } }); + expect(h.events()).toEqual([]); + }); + + it('still uploads the deletion as it was planned when the planner cannot take the server version', async () => { + const { h, lunch } = await lunchDeletedAndFiledAway(); + h.deps.planners = { + ...h.deps.planners, + calendar: { + ...calendarPlanner, + planDownload: (event, local, ctx) => { + if (local?.deleted) throw new Error('cannot take it'); + return calendarPlanner.planDownload(event, local, ctx); + }, + }, + }; const report = await h.run(CALENDAR_AUTHORITY); diff --git a/src/device-sync/calendar/create.ts b/src/device-sync/calendar/create.ts index e1ed3fa3..a2dadf9e 100644 --- a/src/device-sync/calendar/create.ts +++ b/src/device-sync/calendar/create.ts @@ -261,6 +261,20 @@ export function createTarget(local: LocalEvent, ctx: CalendarContext): { key: st // ─── Deletions ────────────────────────────────────────── +/** + * Whether the server moved a deleted event out of the calendar its row is in + * (`own`), and into none this device syncs, before the device's deletion + * reached it: the deletion is moot then, and the event stays where it is now. + * A calendar deselected since is still the row's own, so that deletion goes + * up; so does one of an event moved into another synced calendar. + */ +export function movedAway(local: LocalEvent, own: { jmapAccountId: string; calendarId: string }, ctx: CalendarContext): boolean { + if (!local.shadow) return false; + const ownKey = collectionKey(own.jmapAccountId, own.calendarId); + const keys = Object.entries(local.shadow.calendarIds ?? {}).filter(([, on]) => on).map(([id]) => collectionKey(ctx.jmapAccountId, id)); + return !keys.some((key) => key === ownKey || ctx.isSelected(key)); +} + /** * A deleted event with identity: an object in several calendars loses only * the selected memberships; otherwise it is destroyed, with scheduling diff --git a/src/device-sync/calendar/planner.ts b/src/device-sync/calendar/planner.ts index 7a01bbfd..540f4190 100644 --- a/src/device-sync/calendar/planner.ts +++ b/src/device-sync/calendar/planner.ts @@ -27,7 +27,7 @@ import { isTaskLikeObject } from '../../lib/calendar-component-detection'; import { keepDirtyGroup, type UploadedUnits } from './accepted'; import { ATTENDEE_COLUMNS, CALENDAR_COLUMNS, EVENT_COLUMNS, EXCEPTION_CELLS, MASTER_CELLS, REMINDER_COLUMNS, isTruncatedBaseline, makeBaseline } from './columns'; import { planCalendars } from './calendars'; -import { claimOps, claimUid, claimedUid, computeCreate, createTarget, deleteActions } from './create'; +import { claimOps, claimUid, claimedUid, computeCreate, createTarget, deleteActions, movedAway } from './create'; import { decodeCalendar, decodeEvents, isExceptionRow, isNewMaster } from './decode'; import { isExcluded, overridesOf } from './exceptions'; import { eventImage } from './image'; @@ -65,6 +65,20 @@ function planDownload(event: CalendarEventWire, local: LocalEvent | null, ctx: C refuseTask(local?.shadow); const ref = refOf(local, event, ctx.jmapAccountId); const none = (stillDirty: boolean): DownloadPlan => ({ ops: { ref, ops: [] }, conflicts: 0, stillDirty, effect: 'none', writes: 0 }); + // A different object under a reused id: its rows are replaced, never patched. + const uidChanged = !!local?.shadow && typeof local.shadow.uid === 'string' && typeof event.uid === 'string' && local.shadow.uid !== event.uid; + + if (local?.deleted && local.split !== 'clone' && !uidChanged) { + // Deleted on the device: that wins over a server edit, so only the shadow follows, for the deletion. + // Also when no synced calendar holds the event any more: the deletion is then moot (see planUpload). + if (!local.shadow || deepEqual(local.shadow, event)) return none(true); + const group = new GroupBuilder(ref); + group.assert({ op: 'assert', table: 'events', id: local.eventId, values: { [Events.DELETED]: 1, [Events._SYNC_ID]: local.syncId }, expectCount: 1 }); + group.write({ op: 'update', table: 'events', id: local.eventId, values: { [Events.SYNC_DATA1]: JSON.stringify(event) }, expectCount: 1 }); + const ops = group.build(); + return { ops, conflicts: 0, stillDirty: true, effect: 'update', writes: writesOf(ops) }; + } + // An event another client moved out of every synced calendar still merges into its rows where they are // (they wait for an upload; the accepted write then removes them). const target = pickCalendar(event, local, ctx) ?? (local ? rowCalendar(local, ctx) : null); @@ -80,8 +94,6 @@ function planDownload(event: CalendarEventWire, local: LocalEvent | null, ctx: C // A split clone only looks like this object: it is a new event and uploads as one. The source row is the object's. if (local.split === 'clone') return none(true); - // A different object under a reused id: its rows are replaced, never patched. - const uidChanged = !!local.shadow && typeof local.shadow.uid === 'string' && typeof event.uid === 'string' && local.shadow.uid !== event.uid; if (uidChanged) { const group = new GroupBuilder(ref); for (const x of local.exceptions) deleteRow(group, x.eventId); @@ -98,16 +110,6 @@ function planDownload(event: CalendarEventWire, local: LocalEvent | null, ctx: C return { ops, conflicts: 0, stillDirty: false, effect: 'update', writes: writesOf(ops) }; } - if (local.deleted) { - // Deleted on the device: that wins over a server edit, so only the shadow follows, for the destroy. - if (!local.shadow || deepEqual(local.shadow, event)) return none(true); - const group = new GroupBuilder(ref); - group.assert({ op: 'assert', table: 'events', id: local.eventId, values: { [Events.DELETED]: 1, [Events._SYNC_ID]: local.syncId }, expectCount: 1 }); - group.write({ op: 'update', table: 'events', id: local.eventId, values: { [Events.SYNC_DATA1]: JSON.stringify(event) }, expectCount: 1 }); - const ops = group.build(); - return { ops, conflicts: 0, stillDirty: true, effect: 'update', writes: writesOf(ops) }; - } - const itemDirty = local.dirty || local.split === 'source' || local.exceptions.some((x) => x.dirty || x.deleted); const result = writeEvent(event, image, target, local, { mode: 'merge', @@ -266,6 +268,8 @@ function planUpload(local: LocalEvent, ctx: CalendarContext): Plan { return { kind: 'upload', actions: [{ kind: 'destroy', id: null, uid }] }; } if (!parseObjectRef(local.syncId)) return { kind: 'purge', ops: purgeGroup(local) }; + // Moved away on the server before the deletion reached it: nothing goes up, only the rows go. + if (movedAway(local, calendar, ctx)) return { kind: 'purge', ops: purgeGroup(local) }; if (ctx.isReadOnly(calendar.calendarId)) return { kind: 'revert', ops: purgeGroup(local), refetch: true, reason: SKIP.readOnly }; return { kind: 'upload', actions: deleteActions(local, ctx) }; } diff --git a/src/device-sync/contacts/upload.ts b/src/device-sync/contacts/upload.ts index 98845503..cceb0ed6 100644 --- a/src/device-sync/contacts/upload.ts +++ b/src/device-sync/contacts/upload.ts @@ -215,10 +215,24 @@ function readOnlyOnServer(local: LocalContact, ctx: Ctx): boolean { return !!local.shadow && ctx.isReadOnly(withoutMemberOf(local.shadow)); } +/** + * Whether the server moved the card out of every book its rows were written + * for (SYNC1), and into none this device syncs, before the device's deletion + * reached it: the deletion is moot then, and the card stays where it is now. + * A book deselected since still counts as the rows' own, so that deletion + * goes up; so does one of a card moved into another synced book. + */ +function movedAway(local: LocalContact, accountId: string, ctx: Ctx): boolean { + if (!local.shadow || !local.collections.length) return false; + const books = Object.entries(local.shadow.addressBookIds ?? {}).filter(([, on]) => on).map(([id]) => collectionKey(accountId, id)); + return !books.some((key) => local.collections.includes(key) || ctx.isSelected(key)); +} + function deletedPlan(local: LocalContact, ctx: Ctx): UploadPlan { const purge = (): OpGroup => group(local, [deleteWhereId('raw_contacts', local.rawContactId)]); const ref = parseObjectRef(local.sourceId); if (ref) { + if (movedAway(local, ref.accountId, ctx)) return { kind: 'purge', ops: purge() }; if (readOnlyOnServer(local, ctx)) return { kind: 'revert', ops: purge(), refetch: true, reason: 'readOnly' }; const books = Object.entries(local.shadow?.addressBookIds ?? {}).filter(([, on]) => on).map(([id]) => id); const selected = books.filter((id) => ctx.isSelected(collectionKey(ref.accountId, id))); diff --git a/src/device-sync/engine/item-sync.ts b/src/device-sync/engine/item-sync.ts index c747ddae..1f72ec28 100644 --- a/src/device-sync/engine/item-sync.ts +++ b/src/device-sync/engine/item-sync.ts @@ -659,7 +659,7 @@ export abstract class ItemSync { try { return this.downloadWork(kind, acct, object, found.local, written); } catch (error) { - // A planner that cannot place it (no synced calendar) leaves the deletion as it was planned. + // A planner that cannot take the server version leaves the deletion as it was planned. this.env.log(`deleted ${refOf(acct, object.id)} outside the selection keeps its shadow`, error); return null; }