From 0faee568cc6d46a606370650f39d20db688fb5c8 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 27 Sep 2026 07:52:29 +0200 Subject: [PATCH] fix: let a contact deleted on the phone and moved away meanwhile survive #34 When another client moved a card out of every address book the device syncs while the device deleted the contact, the engine noted the card for dropping but never showed its new version to the planner, so the deletion was decided on the old copy and destroyed the card. A deleted item is now merged like any changed one first; the contacts planner then leaves a card that is in no synced book. When the planner cannot place the object, the deletion stays as it was. --- .../__tests__/engine/real-planners.test.ts | 50 +++++++++++++++++-- src/device-sync/engine/item-sync.ts | 31 ++++++++---- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/src/device-sync/__tests__/engine/real-planners.test.ts b/src/device-sync/__tests__/engine/real-planners.test.ts index d72a6541..cae21f0c 100644 --- a/src/device-sync/__tests__/engine/real-planners.test.ts +++ b/src/device-sync/__tests__/engine/real-planners.test.ts @@ -296,7 +296,7 @@ describe('device sync engine with the real planners', () => { }); }); - describe('a deletion the teardown uploads', () => { + describe('a deletion decided on the server version', () => { /** 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 = { @@ -313,7 +313,7 @@ describe('device sync engine with the real planners', () => { }; } - it('is decided on the server version when the server changed since the last sync', async () => { + it('in a teardown, 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; @@ -330,7 +330,51 @@ describe('device sync engine with the real planners', () => { 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 () => { + it('lets the planner see the server version of a contact deleted on the device and filed away meanwhile', 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); + const planned: string[][] = []; + const contacts = h.deps.planners.contacts; + h.deps.planners = { + ...h.deps.planners, + contacts: { + ...contacts, + planUpload: (local, ctx) => { + if (local.deleted) planned.push(Object.keys(local.shadow?.addressBookIds ?? {})); + return contacts.planUpload(local, ctx); + }, + }, + }; + h.device.user.deleteContact(h.contactNamed('Ada Lovelace')!.id); + // Another client files Ada in Archive only: she left every synced book. + h.server.serverUpdate('ContactCard', 'a', ada, { addressBookIds: { [archive]: true } }); + + expect((await h.run()).outcome).toBe('ok'); + + expect(planned).toEqual([[archive]]); + }); + + it('still uploads the deletion of an event whose calendar the planner cannot place', async () => { + const h = real(); + const archive = h.server.addCalendar('a', { name: 'Archive' }); + h.prefs.calendarSelection[`a/${archive}`] = false; + const lunch = h.server.addEvent('a', { uid: 'lunch-uid', title: 'Lunch', start: '2026-10-06T12:00:00', duration: 'PT1H', timeZone: 'Europe/Berlin', calendarIds: { [h.calendar]: true } }); + await h.run(CALENDAR_AUTHORITY); + h.device.user.deleteEvent(Number(h.events()[0]._id)); + h.server.serverUpdate('CalendarEvent', 'a', lunch, { calendarIds: { [archive]: true } }); + + const report = await h.run(CALENDAR_AUTHORITY); + + expect(report).toMatchObject({ outcome: 'ok', itemErrors: [], stats: { uploaded: { deleted: 1 } } }); + expect(h.server.get('CalendarEvent', 'a', lunch)).toBeUndefined(); + expect(h.events()).toEqual([]); + }); + + it('waits in a teardown, 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. diff --git a/src/device-sync/engine/item-sync.ts b/src/device-sync/engine/item-sync.ts index 31c75eaa..8473f1e5 100644 --- a/src/device-sync/engine/item-sync.ts +++ b/src/device-sync/engine/item-sync.ts @@ -164,14 +164,6 @@ export abstract class ItemSync { 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): 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 { @@ -279,6 +271,15 @@ export abstract class ItemSync { if (this.marks(acct).has(id) || this.env.store.account(acct).stale.includes(id)) this.marks(acct).set(id, false); } + /** Puts `ids` on `stale` in the batch whose rows make them outdated (see `dependents`). */ + 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 isStale(acct: string, sourceId: string | null): boolean { const id = idInAccount(sourceId, acct); if (!id) return false; @@ -638,10 +639,18 @@ export abstract class ItemSync { if (!kind || !this.inSelection(acct, object)) { if (!found) return null; const meta = found.kind.meta(found.local); - if (found.kind === kind && meta.dirty && !meta.deleted && !meta.isNew) { - // Uploaded first, dropped afterwards: merged like any dirty item, so the upload builds on the server's version. + if (found.kind === kind && (meta.dirty || meta.deleted) && !meta.isNew) { + // Uploaded first, dropped afterwards: merged like any changed item (a deleted one's shadow follows + // too), so the upload builds on the server's version. this.outsideOf(acct).add(refOf(acct, object.id)); - return this.downloadWork(kind, acct, object, found.local, written); + if (!meta.deleted) return this.downloadWork(kind, acct, object, found.local, written); + 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. + this.env.log(`deleted ${refOf(acct, object.id)} outside the selection keeps its shadow`, error); + return null; + } } return this.removeWork(acct, object.id, found, 'outside'); }