fix: stop contacts waiting forever on group memberships the server can't take #34

A contact added to or removed from a group this device can't change
stayed dirty on every run: the membership upload skips such groups,
while the contact's clean write kept the change pending until the
server showed it. Turning sync off and signing out then always warned
about changes waiting for the server.

The clean write now puts such a membership back to what the server
has, as it does for any edit of a read-only contact, and clears DIRTY;
when that was the contact's only change the upload counts it as
skipped. Memberships of writable groups in the contact's own account
still wait until the group card shows them.

This covers groups of another JMAP account. For groups in read-only
address books the planner is ready, but the engine does not tell it a
group's rights yet; that follows separately.

Checked with planner tests for additions, removals, both kinds of
group and a change next to an edit that uploads, and end to end
through the engine for a group of a shared account, where turning
sync off afterwards finds nothing waiting.

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

@@ -20,6 +20,15 @@ async function localGroup(h: Harness, groupId: string) {
   return g;
 }
 
+/** A group card in an address book this user may only read, on the device. */
+async function readOnlyGroup(h: Harness, members: Record<string, boolean> = {}) {
+  const shared = h.server.addAddressBook(JMAP, { name: 'Shared' });
+  h.readOnlyBooks.add(shared);
+  const gid = h.addCard({ uid: 'urn:uuid:group-shared', kind: 'group', name: { full: 'Shared' }, members, addressBookIds: { [shared]: true } });
+  await h.download(gid);
+  return gid;
+}
+
 /** A group card holding Anna (apple card), both on the device. */
 async function withGroup(members: Record<string, boolean> = { [appleCard().uid as string]: true }) {
   const h = new Harness();
@@ -127,6 +136,71 @@ describe('contacts groups: membership edits', () => {
     expect(h.planner.planMembershipUploads([await h.contact(rawId)], await h.groups(), h.ctx)).toEqual([]);
   });
 
+  it('puts back an addition to a group in read-only books instead of keeping the contact dirty', async () => {
+    const h = new Harness();
+    const gid = await readOnlyGroup(h, { 'urn:uuid:someone': true });
+    const { rawId } = await h.seed(appleCard());
+    h.device.user.addToGroup(rawId, (await localGroup(h, gid)).groupId);
+    expect((await h.upload(rawId)).kind).toBe('revert');
+    expect(h.membershipGroups(rawId)).toEqual([]);
+    expect(h.dirty(rawId)).toBe(false);
+    expect(h.planner.planMembershipUploads([await h.contact(rawId)], await h.groups(), h.ctx)).toEqual([]);
+  });
+
+  it('puts back a removal from a group in read-only books', async () => {
+    const h = new Harness();
+    const gid = await readOnlyGroup(h, { [appleCard().uid as string]: true });
+    const { rawId } = await h.seed(appleCard());
+    const membership = (await h.contact(rawId)).rows.find((r) => r.mimetype === MimeType.GROUP_MEMBERSHIP)!;
+    h.device.user.deleteData(membership.id);
+    expect((await h.upload(rawId)).kind).toBe('revert');
+    expect(h.membershipGroups(rawId)).toEqual([`${JMAP}/${gid}`]);
+    expect(h.dirty(rawId)).toBe(false);
+  });
+
+  it('puts back an addition to a group of another JMAP account', async () => {
+    const h = new Harness();
+    h.server.addAccount('t', { name: 'Team' });
+    const book = h.server.addAddressBook('t', { name: 'Team contacts' });
+    const team = h.server.addCard('t', { uid: 'urn:uuid:team', kind: 'group', name: { full: 'Team' }, addressBookIds: { [book]: true } });
+    const card = h.server.get('ContactCard', 't', team) as unknown as ContactCardWire;
+    await h.applyOk(h.planner.planGroupDownload(card, null, { ...h.ctx, jmapAccountId: 't' }).ops);
+    const { rawId } = await h.seed(appleCard());
+    h.device.user.addToGroup(rawId, (await h.groups()).find((g) => g.sourceId === `t/${team}`)!.groupId);
+    expect(h.membershipGroups(rawId)).toEqual([`t/${team}`]);
+    expect((await h.upload(rawId)).kind).toBe('revert');
+    expect(h.membershipGroups(rawId)).toEqual([]);
+    expect(h.dirty(rawId)).toBe(false);
+  });
+
+  it('still uploads a read-only contact joining a writable group (the group card is what changes)', async () => {
+    const h = new Harness();
+    const other = h.server.addAddressBook(JMAP, { name: 'Shared' });
+    h.readOnlyBooks.add(other);
+    const gid = h.addCard({ uid: 'urn:uuid:g5', kind: 'group', name: { full: 'Mine' }, members: { 'urn:uuid:someone': true } });
+    await h.download(gid);
+    const { rawId } = await h.seed({ ...appleCard(), addressBookIds: { [other]: true } });
+    h.device.user.addToGroup(rawId, (await localGroup(h, gid)).groupId);
+    expect((await h.upload(rawId)).kind).toBe('revert');
+    expect(h.membershipGroups(rawId)).toEqual([`${JMAP}/${gid}`]);
+    expect(h.dirty(rawId)).toBe(true);
+    expect(h.planner.planMembershipUploads([await h.contact(rawId)], await h.groups(), h.ctx)).toEqual([
+      { kind: 'update', id: gid, patch: { [`members/${appleCard().uid as string}`]: true } },
+    ]);
+  });
+
+  it('uploads the edits it can next to a membership it puts back, and settles', async () => {
+    const h = new Harness();
+    const gid = await readOnlyGroup(h);
+    const { id, rawId } = await h.seed(appleCard());
+    h.device.user.addToGroup(rawId, (await localGroup(h, gid)).groupId);
+    h.device.user.updateData(h.rowsByKey(rawId)['emails:k1']._id as number, { data1: 'anna@new.example' });
+    const plan = await h.upload(rawId);
+    expect(actionsOf(plan)).toEqual([{ kind: 'update', id, patch: { 'emails/k1/address': 'anna@new.example' } }]);
+    expect(h.membershipGroups(rawId)).toEqual([]);
+    expect(h.dirty(rawId)).toBe(false);
+  });
+
   it('does not take a server-side addition for a local removal while the contact is dirty', async () => {
     const { h, gid } = await withGroup({});
     const { rawId } = await h.seed(googleCard());

src/device-sync/__tests__/contacts/harness.ts

@@ -6,6 +6,7 @@
  */
 import { GroupMembership, Groups, RawContacts } from '../../android-columns';
 import { makeKeyMinter, parseObjectRef, uuidFrom } from '../../common/ids';
+import type { GroupRights } from '../../contacts/members';
 import { contactsPlanner } from '../../contacts/planner';
 import type {
   ContactCardWire,
@@ -48,7 +49,7 @@ export class Harness {
   readonly unselected = new Set<string>();
   /** When false the context has no `groupsOf` (memberships not synced). */
   groupIndex = true;
-  readonly ctx: ContactsContext;
+  readonly ctx: ContactsContext & GroupRights;
 
   constructor() {
     this.server.addAccount(JMAP, { name: ACCOUNT });
@@ -79,14 +80,20 @@ export class Harness {
           .filter((c) => c.kind === 'group' && (c.members as Record<string, boolean> | undefined)?.[uid] === true)
           .map((c) => `${JMAP}/${c.id as string}`);
       },
+      groupReadOnly(sourceId: string) {
+        const ref = parseObjectRef(sourceId);
+        const card = ref ? self.server.get('ContactCard', ref.accountId, ref.id) : undefined;
+        const books = Object.keys((card?.addressBookIds as Record<string, boolean> | undefined) ?? {});
+        return books.length > 0 && books.every((id) => self.readOnlyBooks.has(id));
+      },
     };
   }
 
   /** The context as the engine may build it without a group index. */
-  context(): ContactsContext {
+  context(): ContactsContext & GroupRights {
     if (this.groupIndex) return this.ctx;
     const { groupsOf: _drop, ...rest } = this.ctx;
-    return rest as ContactsContext;
+    return rest as ContactsContext & GroupRights;
   }
 
   async apply(group: OpGroup): Promise<BatchResult> {

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

@@ -3,16 +3,21 @@
 // themselves are tested in the other files of this folder.
 
 import { describe, expect, it } from 'vitest';
+import { MimeType } from '../../android-columns';
 import { calendarPlanner } from '../../calendar/planner';
 import { contactsPlanner } from '../../contacts/planner';
-import { addServerCards, createHarness, type Harness } from '../engine/harness';
+import { addServerCards, CONTACTS_AUTHORITY, createHarness, type Harness } from '../engine/harness';
 
-function real(): Harness {
-  const h = createHarness();
+function real(options: Parameters<typeof createHarness>[0] = {}): Harness {
+  const h = createHarness(options);
   h.deps.planners = { contacts: contactsPlanner, calendar: calendarPlanner };
   return h;
 }
 
+function memberships(h: Harness, rawContactId: number) {
+  return h.device.rows('data').filter((d) => Number(d.raw_contact_id) === rawContactId && d.mimetype === MimeType.GROUP_MEMBERSHIP);
+}
+
 /** 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 });
@@ -51,4 +56,24 @@ describe('contacts through the engine', () => {
     expect(h.server.get('ContactCard', 'a', group)).toMatchObject({ addressBookIds: { [archive]: true } });
     expect(h.device.rows('groups')).toEqual([]);
   });
+
+  it('puts back a membership in a group of another account, so no change waits for ever', async () => {
+    const h = real({ accounts: 'withShared' });
+    const book = h.server.all('AddressBook', 'team')[0].id as string;
+    h.prefs.contactsSelection[`team/${book}`] = true;
+    const team = h.server.addCard('team', { uid: 'g-team', kind: 'group', name: { full: 'Team' }, addressBookIds: { [book]: true } });
+    addServerCards(h, ['Ada Lovelace']);
+    expect((await h.run()).outcome).toBe('ok');
+
+    const ada = h.contactNamed('Ada Lovelace')!.id;
+    h.device.user.addToGroup(ada, Number(h.device.rows('groups').find((g) => g.sourceid === `team/${team}`)!._id));
+    const report = await h.run();
+
+    expect(report).toMatchObject({ outcome: 'ok', stats: { skipped: 1 } });
+    expect(h.contactNamed('Ada Lovelace')).toMatchObject({ dirty: false });
+    expect(memberships(h, ada)).toEqual([]);
+    expect(h.server.get('ContactCard', 'team', team)?.members).toBeUndefined();
+    // Turning sync off finds nothing waiting.
+    expect(await h.teardown(CONTACTS_AUTHORITY)).toEqual({ pending: 0 });
+  });
 });

src/device-sync/contacts/download.ts

@@ -14,13 +14,13 @@
  */
 import { Data, MimeType, RawContacts } from '../android-columns';
 import { canonicalJson } from '../common/json';
-import { objectRef } from '../common/ids';
+import { objectRef, parseCollectionKey, parseObjectRef } from '../common/ids';
 import type { ContactCardWire, ContactsContext, DownloadPlan, LocalContact, LocalDataRow } from '../planner';
 import type { ProviderOp, WriteRow } from '../types';
 import { changedFromBaseline, encodeBaseline, sameCell } from './cells';
 import { localChanges, sameUnit } from './diff';
 import { deletionsInferred, inPlaceEvidence, matchKind, reconcileKeys, type KindMatch } from './matching';
-import { planMemberships, remoteMemberships, shadowMemberOf, withMemberOf, withoutMemberOf } from './members';
+import { editableGroups, planMemberships, remoteMemberships, shadowMemberOf, withMemberOf, withoutMemberOf } from './members';
 import { assertContact, deleteData, insertUnit, updateData, updateRawContact, updateUnit } from './ops';
 import { shadowOf } from './photo';
 import { KIND_ORDER, projectCard, rowMatchesUnit, SPECS, type Unit } from './project';
@@ -224,10 +224,16 @@ export interface CleanWriteOptions {
   accepted?: boolean;
 }
 
+/** The JMAP account a local contact belongs to. */
+export function accountOf(local: LocalContact, ctx: Ctx): string {
+  return parseObjectRef(local.sourceId)?.accountId ?? parseCollectionKey(local.pending?.target)?.accountId ?? ctx.jmapAccountId;
+}
+
 /**
  * The ops that make a local contact's rows, shadow and raw columns the
  * server card's, behind the VERSION assert. Membership changes that the
- * server does not have yet stay (and keep the contact dirty).
+ * server does not have yet stay (and keep the contact dirty), unless it can
+ * never take them: those are put back (`revertedMembers`).
  */
 export function cleanWrite(local: LocalContact, card: ContactCardWire, ctx: Ctx, options: CleanWriteOptions) {
   const server = withoutMemberOf(card);
@@ -243,6 +249,7 @@ export function cleanWrite(local: LocalContact, card: ContactCardWire, ctx: Ctx,
     restore: true,
     parent: local.rawContactId,
     groupRowIdBySourceId: (g) => ctx.groupRowIdBySourceId(g),
+    editable: editableGroups(ctx, accountOf(local, ctx)),
   });
   const shadow = withMemberOf(shadowOf(server), members.memberOf);
   const raw = changedRawColumns(local, {
@@ -258,6 +265,7 @@ export function cleanWrite(local: LocalContact, card: ContactCardWire, ctx: Ctx,
     ops: ops.length ? [assertContact(local, options.assertDirty), ...ops] : [],
     writes: ops.length,
     pendingMembers: members.pending,
+    revertedMembers: members.reverted,
   };
 }
 

src/device-sync/contacts/members.ts

@@ -13,9 +13,12 @@
  * Per group, a local change wins where the server did not change, the server
  * wins elsewhere; both sides can only flip a membership the same way, so
  * memberships never conflict. A local change stays in the base until the
- * server has it, which keeps it visible to `planMembershipUploads`.
+ * server has it, which keeps it visible to `planMembershipUploads`; one the
+ * server can't take (a group in read-only books, or in another JMAP account)
+ * is put back by the next clean write instead.
  */
 import { Data, GroupMembership, MimeType } from '../android-columns';
+import { parseObjectRef } from '../common/ids';
 import type { ContactCardWire, ContactsContext, LocalDataRow } from '../planner';
 import type { ProviderOp } from '../types';
 import { text } from './cells';
@@ -23,6 +26,24 @@ import { entryKey } from './keys';
 import { deleteData, insertUnit, updateData } from './ops';
 import { MEMBER_OF, type Unit } from './project';
 
+/**
+ * Group rights the engine can add to the context (not part of
+ * ContactsContext yet): whether a group card, by SOURCE_ID, is in read-only
+ * address books only. Without it every synced group counts as writable.
+ */
+export interface GroupRights {
+  groupReadOnly?(sourceId: string): boolean;
+}
+
+/**
+ * The groups whose members an upload for a contact of `account` can change:
+ * that account's groups outside read-only books. A membership change of any
+ * other group can never reach the server (`planMembershipUploads` skips it).
+ */
+export function editableGroups(ctx: ContactsContext & GroupRights, account: string): (sourceId: string) => boolean {
+  return (sourceId) => parseObjectRef(sourceId)?.accountId === account && !(ctx.groupReadOnly?.(sourceId) ?? false);
+}
+
 /** Remote memberships of a card, or null when the engine can't tell (memberships are then left alone). */
 export function remoteMemberships(card: ContactCardWire, ctx: ContactsContext): string[] | null {
   if (typeof ctx.groupsOf !== 'function' || typeof card.uid !== 'string' || !card.uid) return null;
@@ -71,6 +92,8 @@ export interface MemberPlan {
   /** Groups the device added the contact to, or removed it from, that the server does not show yet. */
   added: string[];
   removed: string[];
+  /** Local changes of groups that are not `editable` were put back to what the server has. */
+  reverted: boolean;
 }
 
 export interface MemberInput {
@@ -83,13 +106,19 @@ export interface MemberInput {
   restore: boolean;
   parent: number | { ref: number };
   groupRowIdBySourceId(sourceId: string): number | null;
+  /**
+   * Groups whose members the device may change (see `editableGroups`); a local
+   * change of another group is put back to what the server has rather than
+   * waiting for an upload that never comes. Without it every group counts.
+   */
+  editable?(sourceId: string): boolean;
 }
 
 export function planMemberships(input: MemberInput): MemberPlan {
   const live = input.rows.filter((r) => r.mimetype === MimeType.GROUP_MEMBERSHIP);
   if (input.remote === null) {
     // Without the engine's group index memberships are not synced: nothing is written or kept pending.
-    return { ops: [], writes: 0, memberOf: input.base, pending: false, added: [], removed: [] };
+    return { ops: [], writes: 0, memberOf: input.base, pending: false, added: [], removed: [], reverted: false };
   }
   const ops: ProviderOp[] = [];
   const rowOf = new Map<string, LocalDataRow>();
@@ -119,11 +148,17 @@ export function planMemberships(input: MemberInput): MemberPlan {
     after.add(g);
   };
 
+  let reverted = false;
   for (const g of [...new Set([...base, ...remote, ...rowOf.keys()])].sort()) {
     const inBase = base.has(g);
     const inRemote = remote.has(g);
     const row = rowOf.get(g);
-    const change = inBase ? (!row && deletions ? 'removed' : null) : row ? 'added' : null;
+    let change = inBase ? (!row && deletions ? 'removed' : null) : row ? 'added' : null;
+    if (change && input.editable && !input.editable(g)) {
+      // A change the server can't take (a group in read-only books, or another account's): its state comes back.
+      if (change === 'added' ? !inRemote : inRemote) reverted = true;
+      change = null;
+    }
     if (change === 'added') {
       keep(g, row!);
       if (!inRemote) pendingAdded.add(g);
@@ -145,5 +180,6 @@ export function planMemberships(input: MemberInput): MemberPlan {
     pending: unsynced || pendingAdded.size > 0 || pendingRemoved.size > 0,
     added: [...pendingAdded],
     removed: [...pendingRemoved],
+    reverted,
   };
 }

src/device-sync/contacts/upload.ts

@@ -23,7 +23,7 @@ import type {
 import type { ProviderOp } from '../types';
 import { encodeBaseline } from './cells';
 import { localChanges } from './diff';
-import { cardUnits, cleanWrite } from './download';
+import { accountOf, cardUnits, cleanWrite } from './download';
 import { newEntries, unitEdits, unitEntries, type Edit, type NewEntry } from './entries';
 import { parseEntryKey } from './keys';
 import { deletionsInferred, inPlaceEvidence, matchKind, type KindMatch } from './matching';
@@ -263,18 +263,15 @@ export function planContactUpload(local: LocalContact, ctx: Ctx): UploadPlan<Con
     // Nothing mapped changed (a star, a ringtone, an edit the server can't take): the rows
     // become the shadow's again and DIRTY is cleared behind the VERSION assert.
     const write = cleanWrite(local, local.shadow, ctx, { assertDirty: true, clearDirty: true });
-    return { kind: 'clean', ops: group(local, write.ops.length ? write.ops : [assertContact(local, true)]) };
+    const ops = group(local, write.ops.length ? write.ops : [assertContact(local, true)]);
+    // A membership the server can't take was put back, as a read-only edit is.
+    return write.revertedMembers ? { kind: 'revert', ops } : { kind: 'clean', ops };
   }
   const actions: UploadAction<ContactCardWire>[] = [{ kind: 'update', id: parseObjectRef(local.sourceId)!.id, patch }];
   if (backedOff(local, ctx, actions)) return { kind: 'skip', reason: `poisoned:${local.poison!.type}` };
   return { kind: 'upload', actions };
 }
 
-/** The JMAP account a local contact belongs to. */
-function accountOf(local: LocalContact, ctx: Ctx): string {
-  return parseObjectRef(local.sourceId)?.accountId ?? parseCollectionKey(local.pending?.target)?.accountId ?? ctx.jmapAccountId;
-}
-
 export function planContactAccepted(local: LocalContact, server: ContactCardWire, ctx: Ctx): AcceptedPlan {
   if (local.deleted) {
     const purge = group(local, [deleteWhereId('raw_contacts', local.rawContactId)]);