fix: move group members on the phone after an interrupted sync #34

When a group's members change on the server, the download fetches the
contacts whose membership flipped in a chunk after the group's. The
group's chunk already stored its new version, so when the sync stopped
before the members' chunk, the next sync found no flip any more and
the phone kept the old members. Those contacts are now listed with the
group's rows, as objects to fetch first, until their own chunk ran.

src/device-sync/__tests__/engine/group-listing.test.ts

@@ -5,6 +5,7 @@
 // row it inserted but never listed hides a later hard delete.
 
 import { describe, expect, it } from 'vitest';
+import { MimeType } from '../../android-columns';
 import { calendarPlanner } from '../../calendar/planner';
 import { contactsPlanner } from '../../contacts/planner';
 import { serializeSyncState } from '../../engine/sync-state';
@@ -175,6 +176,34 @@ describe('device sync engine: the groups a SyncState lists', () => {
     expect(h.server.get('ContactCard', 'a', group)).toBeUndefined();
   });
 
+  it("moves a group's membership on the device even when time ran out after the group's chunk", async () => {
+    const h = real();
+    const [ada, grace] = addServerCards(h, ['Ada Lovelace', 'Grace Hopper']);
+    const uid = (id: string) => h.server.get('ContactCard', 'a', id)!.uid as string;
+    const group = h.server.addCard('a', { uid: 'g1', kind: 'group', name: { full: 'Friends' }, members: { [uid(ada)]: true } });
+    await h.run();
+    const members = () =>
+      h.device.rows('data')
+        .filter((d) => d.mimetype === MimeType.GROUP_MEMBERSHIP)
+        .map((d) => h.contacts().find((c) => c.id === Number(d.raw_contact_id))?.name)
+        .sort();
+    expect(members()).toEqual(['Ada Lovelace']);
+    // Another client moves the membership from Ada to Grace: only the group card changes.
+    h.server.serverUpdate('ContactCard', 'a', group, { members: { [uid(grace)]: true } });
+    // 'changes', 'download' (the group's chunk), then out of time before the members' chunk.
+    h.checkpoints.count = 0;
+    h.checkpoints.onCheckpoint = (n) => {
+      if (n === 2) h.clock.now += RUN_BUDGET;
+    };
+    expect((await h.run()).outcome).toBe('cancelled');
+    h.checkpoints.onCheckpoint = undefined;
+
+    expect((await h.run()).outcome).toBe('ok');
+
+    expect(members()).toEqual(['Grace Hopper']);
+    expect(h.state()?.accounts.a.stale).toEqual([]);
+  });
+
   it('never destroys a listed group card that is in no synced book or no longer a group, and forgets it', async () => {
     const h = real();
     const archive = h.server.addAddressBook('a', { name: 'Archive' });

src/device-sync/engine/contacts-sync.ts

@@ -123,6 +123,8 @@ export class ContactsSync extends ItemSync {
   private uidRefs: Map<string, string[]> | null = null;
   /** Per account: raw contacts the upload phases looked at, for their memberships. */
   private readonly uploadRows = new Map<string, Set<number>>();
+  /** Group ref → ids of the contacts whose membership in it flips with the version being downloaded. */
+  private readonly refreshes = new Map<string, string[]>();
 
   constructor(env: RunEnv) {
     super(env);
@@ -409,27 +411,42 @@ export class ContactsSync extends ItemSync {
     };
   }
 
-  /** Contacts of `acct` whose membership in one of these group cards flipped since the group's shadow. */
+  /**
+   * Contacts of `acct` whose membership in one of these group cards flipped
+   * since the group's shadow; kept per group for `dependents`.
+   */
   private async membersToRefresh(acct: string, groups: Array<{ id: string; members?: Record<string, boolean> }>): Promise<string[]> {
     if (!groups.length) return [];
     const locals = await this.groupKind.loadByRefs(groups.map((g) => refOf(acct, g.id)));
-    const flipped = new Set<string>();
+    const flippedBy = new Map<string, Set<string>>();
     for (const group of groups) {
-      const before = new Set(onIds(locals.get(refOf(acct, group.id))?.shadow?.members as Record<string, boolean> | undefined));
+      const ref = refOf(acct, group.id);
+      const before = new Set(onIds(locals.get(ref)?.shadow?.members as Record<string, boolean> | undefined));
       const after = new Set(onIds(group.members));
-      for (const uid of before) if (!after.has(uid)) flipped.add(uid);
-      for (const uid of after) if (!before.has(uid)) flipped.add(uid);
+      const flipped = new Set([...before].filter((uid) => !after.has(uid)).concat([...after].filter((uid) => !before.has(uid))));
+      this.refreshes.delete(ref);
+      if (flipped.size) flippedBy.set(ref, flipped);
     }
-    if (!flipped.size) return [];
+    if (!flippedBy.size) return [];
     await this.ensureShadowIndex();
-    const out: string[] = [];
-    for (const uid of flipped) {
-      for (const ref of this.uidRefs?.get(uid) ?? []) {
-        const id = idInAccount(ref, acct);
-        if (id) out.push(id);
+    const out = new Set<string>();
+    for (const [ref, flipped] of flippedBy) {
+      const ids: string[] = [];
+      for (const uid of flipped) {
+        for (const member of this.uidRefs?.get(uid) ?? []) {
+          const id = idInAccount(member, acct);
+          if (id) ids.push(id);
+        }
       }
+      this.refreshes.set(ref, ids);
+      for (const id of ids) out.add(id);
     }
-    return out;
+    return [...out];
+  }
+
+  /** The members a group card gained or lost: refreshed in this page, or first thing next run. */
+  protected dependents(kind: Kind, acct: string, object: ServerObject): string[] {
+    return kind === this.groupKind ? this.refreshes.get(refOf(acct, object.id)) ?? [] : [];
   }
 
   protected async beforePlanning(acct: string, objects: ServerObject[]): Promise<void> {

src/device-sync/engine/item-sync.ts

@@ -155,6 +155,23 @@ export abstract class ItemSync {
   protected listing(_kind: Kind, _acct: string, _id: string, _present: boolean): StateChange | undefined {
     return undefined;
   }
+  /**
+   * Objects whose rows a download of `object` makes outdated (contacts: the
+   * members a group card gained or lost). They go on `stale` in the batch
+   * that writes it, so a stop before their own chunk leaves them to the next
+   * run's first fetch.
+   */
+  protected dependents(_kind: Kind, _acct: string, _object: ServerObject): string[] {
+    return [];
+  }
+
+  private staleWith(acct: string, ids: string[]): StateChange | undefined {
+    if (!ids.length) return undefined;
+    return (next) => {
+      const account = accountOf(next, acct);
+      account.stale = [...new Set([...account.stale, ...ids])];
+    };
+  }
   protected onDiscard(_next: SyncState, _accounts: Set<string>): void {}
   /** Whether a pending create of `target` may adopt this object (contacts: the target book holds it). */
   protected adoptionTargetMatches(acct: string, _object: ServerObject, target: string): boolean {
@@ -658,7 +675,7 @@ export abstract class ItemSync {
     const group = local ? plan.ops : prependOps(plan.ops, [insertGuard(kind, ref)]);
     return {
       group,
-      state: this.listing(kind, acct, object.id, true),
+      state: both(this.listing(kind, acct, object.id, true), this.staleWith(acct, this.dependents(kind, acct, object))),
       fresh: true,
       applied: () => {
         if (plan.effect === 'insert') this.env.report.stats.downloaded.created++;
@@ -694,7 +711,10 @@ export abstract class ItemSync {
     const plan = kind.planDownload(object, null, acct);
     return {
       group: concatGroups(ref, found.kind.planLocalDelete(found.local), plan.ops),
-      state: both(this.listing(found.kind, acct, object.id, false), this.listing(kind, acct, object.id, true)),
+      state: both(
+        both(this.listing(found.kind, acct, object.id, false), this.listing(kind, acct, object.id, true)),
+        this.staleWith(acct, this.dependents(kind, acct, object)),
+      ),
       applied: () => {
         this.env.report.stats.downloaded.updated++;
         this.markResolved(acct, object.id);