fix: keep unsynced changes waiting when sync is turned off before the first sync ended #34

Turning sync off uploads what the device changed without downloading
first; every upload carries the state the device last synced, so the
server refuses it when something changed meanwhile and the engine
downloads first. Before the first sync of an account had ended there
was no such state, and edits and deletions went up unguarded, able to
overwrite what another client had changed. They now wait, and the app
asks before they are dropped; new items still go up.

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

@@ -15,6 +15,7 @@ import {
   createHarness,
   renameDeviceContact,
   rowWrites,
+  RUN_BUDGET,
   type Harness,
 } from './harness';
 
@@ -295,6 +296,62 @@ describe('device sync engine with the real planners', () => {
     });
   });
 
+  describe('a deletion the teardown uploads', () => {
+    /** The contacts planner as fixed for data finding 4: a deleted contact's shadow follows the server. */
+    function refreshingDeletedShadows(h: Harness): void {
+      h.deps.planners = {
+        ...h.deps.planners,
+        contacts: {
+          ...contactsPlanner,
+          planDownload: (card, local, ctx) => {
+            if (!local?.deleted || !local.sourceId) return contactsPlanner.planDownload(card, local, ctx);
+            const shadow = JSON.stringify({ ...card, '~memberOf': local.shadow?.['~memberOf'] ?? [] });
+            const ops = [{ op: 'update' as const, table: 'raw_contacts' as const, id: local.rawContactId, values: { sync2: shadow } }];
+            return { ops: { ref: local.sourceId, ops }, conflicts: 0, stillDirty: true, effect: 'update' as const, writes: 1 };
+          },
+        },
+      };
+    }
+
+    it('is decided on the server version when the server changed since the last sync', async () => {
+      const h = real();
+      const archive = h.server.addAddressBook('a', { name: 'Archive' });
+      h.prefs.contactsSelection[`a/${archive}`] = false;
+      const [ada] = addServerCards(h, ['Ada Lovelace']);
+      await h.run();
+      refreshingDeletedShadows(h);
+      h.device.user.deleteContact(h.contactNamed('Ada Lovelace')!.id);
+      // Meanwhile another client files Ada in Archive too.
+      h.server.serverUpdate('ContactCard', 'a', ada, { [`addressBookIds/${archive}`]: true });
+
+      expect(await h.teardown(CONTACTS_AUTHORITY)).toEqual({ pending: 0 });
+
+      expect(h.server.get('ContactCard', 'a', ada)).toMatchObject({ addressBookIds: { [archive]: true } });
+      expect((h.server.get('ContactCard', 'a', ada)!.addressBookIds as object)).not.toHaveProperty(h.book);
+    });
+
+    it('waits, with the edits, while no state guards the upload (the first sync never ended)', async () => {
+      const h = real();
+      const [ada] = addServerCards(h, ['Ada Lovelace', 'Grace Hopper', ...Array.from({ length: 60 }, (_, i) => `P${i}`)]);
+      // The first sync (a full reconcile) runs out of time after its first chunk (Ada and Grace): no state yet.
+      h.checkpoints.onCheckpoint = (n) => {
+        if (n === 3) h.clock.now += RUN_BUDGET;
+      };
+      expect((await h.run()).outcome).toBe('cancelled');
+      h.checkpoints.onCheckpoint = undefined;
+      expect(h.state()?.accounts.a.itemsState).toBeNull();
+      renameDeviceContact(h, h.contactNamed('Ada Lovelace')!.id, 'Ada King');
+      h.device.user.deleteContact(h.contactNamed('Grace Hopper')!.id);
+      addDeviceContact(h, 'Hedy Lamarr');
+      // Meanwhile another client changes Ada.
+      h.server.serverUpdate('ContactCard', 'a', ada, { 'name/full': 'Ada, Countess of Lovelace' });
+
+      expect(await h.teardown(CONTACTS_AUTHORITY)).toEqual({ pending: 2 });
+
+      expect(h.serverNames().filter((n) => !/^P\d+$/.test(n))).toEqual(['Ada, Countess of Lovelace', 'Grace Hopper', 'Hedy Lamarr']);
+    });
+  });
+
   describe('a device edit the server refuses (forbidden)', () => {
     it('puts a contact back in place, keeping what only the device knows', async () => {
       const h = real();

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

@@ -800,14 +800,19 @@ export abstract class ItemSync {
    */
   protected async uploadAll(): Promise<void> {
     const accounts = this.accountIds();
-    for (const acct of accounts) await this.withRetries(acct, () => this.pairPhase(acct));
+    // A teardown downloads nothing first: `ifInState` is what keeps its uploads from overwriting a server change.
+    // An account without a stored state (its first sync never ended) uploads only new items; the rest waits.
+    const guarded = accounts.filter((acct) => this.env.mode !== 'teardown' || !!this.env.store.account(acct).itemsState);
+    for (const acct of guarded) await this.withRetries(acct, () => this.pairPhase(acct));
     await this.claimNewItems();
     const deletionsAllowed = await this.deletionPolicy(await this.loadUploadItems());
-    for (const acct of accounts) await this.withRetries(acct, () => this.prePhase(acct));
+    for (const acct of guarded) await this.withRetries(acct, () => this.prePhase(acct));
     for (const phase of ['create', 'update', 'delete'] as const) {
-      for (const acct of accounts) await this.withRetries(acct, () => this.phase(acct, phase, deletionsAllowed));
+      for (const acct of phase === 'create' ? accounts : guarded) {
+        await this.withRetries(acct, () => this.phase(acct, phase, deletionsAllowed));
+      }
     }
-    for (const acct of accounts) await this.withRetries(acct, () => this.extraPhase(acct, deletionsAllowed));
+    for (const acct of guarded) await this.withRetries(acct, () => this.extraPhase(acct, deletionsAllowed));
     for (const acct of accounts) await this.adoptExtraCreated(acct);
   }