fix: keep contacts the server also filed in unsynced books when deleted on the device #34

A contact or group deleted on the device was uploaded against the
shadow of its last sync: a download into a deleted item wrote nothing.
When the server had meanwhile also filed the card in an address book
this device does not sync, the deletion still saw only the synced book
and destroyed the card, its copy in the other book included.

A download into a deleted contact or group now writes the new server
card into its shadow, and takes over an identity that was never
written, so deleting our own lost create goes by id rather than by a
uid lookup. The rows and the group title stay as they are: the device
delete still wins. The upload then removes only the synced memberships
of a card that is also in other books, as the calendar planner already
does for events.

Checked with planner tests for contacts and groups and end to end
through the engine with the real planner: the card stays in the
unsynced book, the device rows are purged, and an echo writes nothing.

src/device-sync/__tests__/contacts/groups.test.ts

@@ -183,6 +183,21 @@ describe('contacts groups: device edits of groups', () => {
     });
   });
 
+  it('only leaves the synced book when the server filed the group in an unsynced one after the device deleted it', async () => {
+    const { h, gid } = await withGroup();
+    const archive = h.server.addAddressBook(JMAP, { name: 'Archive' });
+    h.unselected.add(`${JMAP}/${archive}`);
+    h.device.user.deleteGroup((await localGroup(h, gid)).groupId);
+    h.server.serverUpdate('ContactCard', JMAP, gid, { [`addressBookIds/${archive}`]: true, 'name/full': 'Old friends' });
+    const plan = h.planner.planGroupDownload(h.card(gid), await localGroup(h, gid), h.ctx);
+    await h.applyOk(plan.ops);
+    // The delete wins over the rename; only the shadow follows the card.
+    expect(await localGroup(h, gid)).toMatchObject({ deleted: true, title: 'Friends' });
+    expect(h.planner.planGroupUpload(await localGroup(h, gid), h.ctx)).toEqual({
+      kind: 'upload', actions: [{ kind: 'update', id: gid, patch: { [`addressBookIds/${h.book}`]: null } }],
+    });
+  });
+
   it('reverts a rename of a read-only group', async () => {
     const { h, gid } = await withGroup();
     h.readOnlyBooks.add(h.book);

src/device-sync/__tests__/contacts/merge.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from 'vitest';
-import { Data, MimeType } from '../../android-columns';
+import { Data, MimeType, RawContacts } from '../../android-columns';
 import type { ContactCardWire } from '../../planner';
 import { appleCard, googleCard } from './fixtures';
 import { Harness, JMAP } from './harness';
@@ -139,13 +139,20 @@ describe('contacts merge: a dirty contact meets a new server version', () => {
     expect(typeof patch === 'object' && Object.keys(patch).some((k) => k.startsWith('nicknames'))).toBe(false);
   });
 
-  it('leaves a contact deleted on the device alone: the delete wins and uploads', async () => {
+  it('leaves the rows of a contact deleted on the device alone: the delete wins and uploads', async () => {
     const h = new Harness();
     const { id, rawId } = await h.seed(appleCard());
     h.device.user.deleteContact(rawId);
     h.server.serverUpdate('ContactCard', JMAP, id, { 'emails/k1/address': 'x@example.com' });
     const plan = h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx);
-    expect(plan).toMatchObject({ effect: 'none', stillDirty: true, ops: { ops: [] } });
+    // Only the shadow follows the card, for the deletion's upload.
+    expect(plan).toMatchObject({ effect: 'update', stillDirty: true, conflicts: 0 });
+    expect(plan.ops.ops.filter((o) => o.op !== 'assert')).toEqual([{
+      op: 'update', table: 'raw_contacts', id: rawId, expectCount: 1,
+      values: { [RawContacts.SYNC2]: expect.any(String) },
+    }]);
+    await h.applyOk(plan.ops);
+    expect(h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx)).toMatchObject({ effect: 'none', ops: { ops: [] } });
     expect(h.planner.planUpload(await h.contact(rawId), h.ctx)).toMatchObject({ kind: 'upload', actions: [{ kind: 'destroy', id }] });
   });
 

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

@@ -0,0 +1,54 @@
+// The contacts planner driven by the engine (fake providers, fake Stalwart):
+// cases that only show end to end, over several runs. The planner's rules
+// themselves are tested in the other files of this folder.
+
+import { describe, expect, it } from 'vitest';
+import { calendarPlanner } from '../../calendar/planner';
+import { contactsPlanner } from '../../contacts/planner';
+import { addServerCards, createHarness, type Harness } from '../engine/harness';
+
+function real(): Harness {
+  const h = createHarness();
+  h.deps.planners = { contacts: contactsPlanner, calendar: calendarPlanner };
+  return h;
+}
+
+/** An address book of the personal account that this device does not sync. */
+function unsyncedBook(h: Harness, name = 'Archive'): string {
+  const id = h.server.addAddressBook('a', { name });
+  h.prefs.contactsSelection[`a/${id}`] = false;
+  return id;
+}
+
+describe('contacts through the engine', () => {
+  it('keeps a card deleted on the device that the server meanwhile also filed in an unsynced book', async () => {
+    const h = real();
+    const archive = unsyncedBook(h);
+    const [ada] = addServerCards(h, ['Ada Lovelace']);
+    expect((await h.run()).outcome).toBe('ok');
+
+    h.device.user.deleteContact(h.contactNamed('Ada Lovelace')!.id);
+    h.server.serverUpdate('ContactCard', 'a', ada, { [`addressBookIds/${archive}`]: true });
+    const report = await h.run();
+
+    expect(report).toMatchObject({ outcome: 'ok', itemErrors: [], stats: { uploaded: { deleted: 1 } } });
+    expect(h.server.get('ContactCard', 'a', ada)).toMatchObject({ addressBookIds: { [archive]: true } });
+    expect(h.contacts()).toEqual([]);
+  });
+
+  it('keeps a group deleted on the device that the server meanwhile also filed in an unsynced book', async () => {
+    const h = real();
+    const archive = unsyncedBook(h);
+    const group = h.server.addCard('a', { uid: 'g1', kind: 'group', name: { full: 'Friends' }, addressBookIds: { [h.book]: true } });
+    expect((await h.run()).outcome).toBe('ok');
+
+    const row = h.device.rows('groups').find((g) => g.sourceid === `a/${group}`)!;
+    h.device.user.deleteGroup(Number(row._id));
+    h.server.serverUpdate('ContactCard', 'a', group, { [`addressBookIds/${archive}`]: true });
+    const report = await h.run();
+
+    expect(report).toMatchObject({ outcome: 'ok', itemErrors: [], stats: { uploaded: { deleted: 1 } } });
+    expect(h.server.get('ContactCard', 'a', group)).toMatchObject({ addressBookIds: { [archive]: true } });
+    expect(h.device.rows('groups')).toEqual([]);
+  });
+});

src/device-sync/__tests__/contacts/upload-editors.test.ts

@@ -309,6 +309,36 @@ describe('contacts upload: deleted contacts', () => {
     expect(await patchOf(h, rawId)).toEqual({ [`addressBookIds/${h.book}`]: null });
   });
 
+  it('only leaves the synced book when the server filed the card in an unsynced one after the device deleted it', 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, 'emails/k1/address': 'new@example.com' });
+    const rows = JSON.stringify(h.device.rows('data', 'raw_contact_id = ?', [rawId]));
+    await h.applyOk(h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx).ops);
+    // The delete wins over the server's edit: the rows stay as they were, only the shadow follows.
+    expect(JSON.stringify(h.device.rows('data', 'raw_contact_id = ?', [rawId]))).toBe(rows);
+    expect(await uploadPlan(h, rawId)).toEqual({ kind: 'upload', actions: [{ kind: 'update', id, patch: { [`addressBookIds/${h.book}`]: null } }] });
+    await h.upload(rawId);
+    expect(h.server.get('ContactCard', JMAP, id)).toMatchObject({ addressBookIds: { [archive]: true } });
+    expect(h.device.row('raw_contacts', rawId)).toBeUndefined();
+  });
+
+  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' }]);
+    const plan = await h.planUpload(rawId);
+    if (plan.kind !== 'upload') throw new Error(plan.kind);
+    // The create went through but its response was lost; then the user deleted the contact.
+    const id = h.send(plan.actions)!;
+    h.device.user.deleteContact(rawId);
+    await h.applyOk(h.planner.planDownload(h.card(id), await h.contact(rawId), h.ctx).ops);
+    expect(await h.contact(rawId)).toMatchObject({ sourceId: `${JMAP}/${id}`, pending: null, deleted: true });
+    expect(await uploadPlan(h, rawId)).toEqual({ kind: 'upload', actions: [{ kind: 'destroy', id, uid: h.card(id).uid }] });
+  });
+
   it('purges a new contact that never reached the server, and destroys a claimed one by uid', async () => {
     const h = new Harness();
     const fresh = h.device.user.insertContact(ACCOUNT, [{ [Data.MIMETYPE]: MimeType.NOTE, data1: 'x' }]);

src/device-sync/contacts/download.ts

@@ -288,8 +288,19 @@ export function planContactDownload(card: ContactCardWire, local: LocalContact |
   const identity = objectRef(ctx.jmapAccountId, server.id);
   if (!local) return insertPlan(server, ctx);
   const none = (stillDirty: boolean): DownloadPlan => ({ ops: { ref: identity, ops: [] }, conflicts: 0, stillDirty, effect: 'none', writes: 0 });
-  // Deleted on the device: the delete wins and uploads as a destroy.
-  if (local.deleted) return none(true);
+  if (local.deleted) {
+    // Deleted on the device: the delete wins over a server edit, so the rows stay as they are.
+    // Only the shadow (and an identity never written) follows the card: the deletion uploads
+    // against the card as it is now, which may also sit in books this device does not sync.
+    const shadow = withMemberOf(shadowOf(server), shadowMemberOf(local.shadow));
+    const raw = changedRawColumns(local, {
+      ...(local.sourceId ? {} : { [RawContacts.SOURCE_ID]: identity, [RawContacts.SYNC3]: null }),
+      [RawContacts.SYNC2]: JSON.stringify(shadow),
+    });
+    if (!Object.keys(raw).length) return none(true);
+    const ops = [assertContact(local, local.dirty), updateRawContact(local.rawContactId, raw)];
+    return { ops: { ref: identity, ops }, conflicts: 0, stillDirty: true, effect: 'update', writes: 1 };
+  }
 
   if (!local.sourceId) {
     // Our own create whose identity was never written: adopt it. The rows stay

src/device-sync/contacts/groups.ts

@@ -94,7 +94,13 @@ export function planGroupDownload(card: ContactCardWire, local: LocalGroup | nul
       values: { [Groups.SOURCE_ID]: identity, [Groups.TITLE]: remoteTitle ?? '', [Groups.GROUP_VISIBLE]: 1, [Groups.DIRTY]: 0, ...cardColumns(card, ctx) },
     }], 'insert');
   }
-  if (local.deleted) return plan([], 'none', 0, true);
+  if (local.deleted) {
+    // Deleted on the device: the delete wins over a server edit. Only the shadow (and an identity
+    // never written) follows the card, so the deletion uploads against the card as it is now.
+    const diff = changed(local, { ...(local.sourceId ? {} : { [Groups.SOURCE_ID]: identity, [Groups.SYNC3]: null }), ...cardColumns(card, ctx) });
+    if (!Object.keys(diff).length) return plan([], 'none', 0, true);
+    return plan([assertGroup(local, local.dirty), updateGroup(local, diff)], 'update', 0, true);
+  }
   if (!local.sourceId) {
     // Our own create whose identity was never written.
     return plan([