fix: keep a card or event another app moved away when it is deleted on the phone #34

A contact or event deleted on the phone that another client meanwhile
filed in an address book or calendar the phone doesn't sync was
destroyed on the server by the phone's next sync, and the other
client's move with it. 0faee56 made the sync decide such a deletion on
the server's version, but the planners still destroyed it: the
contacts planner saw the card in no synced book and destroyed it, and
the calendar planner could not take the new version at all (it placed
the event before looking at the deletion), so the old one decided.

The deletion is now moot when the server's version holds none of the
collections the rows were written for and none the phone syncs: only
the rows go, nothing is uploaded, and the object stays where the other
client put it. A deletion still goes up when the book or calendar was
deselected on the phone before the upload, and when the object was
moved into another collection the phone syncs, since the phone would
otherwise miss an object it should show. The calendar planner now lets
a deleted row's shadow follow the server before it places the event.

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 });

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' }]);

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);
 

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

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) };
   }

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<ContactCardWire> {
   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)));

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;
         }