From 2e9e5bb7855ad96eaab75b0fd609de1fc66a5397 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 27 Sep 2026 07:45:24 +0200 Subject: [PATCH] fix: remove items the server deleted right after this device created them #34 After an upload the stored state stays where the download left it, and the engine relied on /changes to report its own creates back. /changes leaves out an object that was created and destroyed after the state it is asked from (Stalwart does, RFC 8620 allows it), so an event or contact created on the device and deleted on the server before the next download stayed on the device for good. The SyncState now lists the objects our uploads created, written with their identity, and the next run fetches them first: one that is gone loses its rows, an unchanged one writes nothing. An upload-only sync runs in full while the list is not empty. --- .../__tests__/engine/own-creates.test.ts | 77 +++++++++++++++++++ src/device-sync/engine/item-sync.ts | 42 ++++++++-- src/device-sync/engine/run.ts | 2 +- src/device-sync/engine/sync-state.ts | 6 ++ 4 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 src/device-sync/__tests__/engine/own-creates.test.ts diff --git a/src/device-sync/__tests__/engine/own-creates.test.ts b/src/device-sync/__tests__/engine/own-creates.test.ts new file mode 100644 index 00000000..76eb8c94 --- /dev/null +++ b/src/device-sync/__tests__/engine/own-creates.test.ts @@ -0,0 +1,77 @@ +// Objects our uploads created. The stored state stays where the download left +// it, and `/changes` omits an object created and destroyed after its +// `sinceState` (Stalwart does, RFC 8620 allows it): such an object must still +// leave the device when another client destroys it before the next download. + +import { describe, expect, it } from 'vitest'; +import { calendarPlanner } from '../../calendar/planner'; +import { contactsPlanner } from '../../contacts/planner'; +import { addDeviceContact, addServerCards, CALENDAR_AUTHORITY, createHarness, type Harness } from './harness'; + +function real(): Harness { + const h = createHarness(); + h.deps.planners = { contacts: contactsPlanner, calendar: calendarPlanner }; + return h; +} + +describe('device sync engine: objects our uploads created', () => { + it('removes a device-created contact that another client destroyed before the next download', async () => { + const h = real(); + addServerCards(h, ['Ada Lovelace']); + await h.run(); + const id = addDeviceContact(h, 'Hedy Lamarr'); + expect((await h.run()).stats.uploaded.created).toBe(1); + const hedy = h.contacts().find((c) => c.id === id)!.sourceId!.split('/')[1]; + h.server.serverDestroy('ContactCard', 'a', hedy); + + const report = await h.run(); + + expect(report).toMatchObject({ outcome: 'ok', stats: { downloaded: { deleted: 1 } } }); + expect(h.contacts().map((c) => c.name)).toEqual(['Ada Lovelace']); + expect(h.state()?.accounts.a.created ?? []).toEqual([]); + }); + + it('removes a device-created event that another client destroyed before the next download', async () => { + const h = real(); + await h.run(CALENDAR_AUTHORITY); + const rowId = Number(h.device.rows('calendars')[0]._id); + const id = h.device.user.insertEvent(rowId, { title: 'Dentist', dtstart: Date.UTC(2026, 9, 1, 8), dtend: Date.UTC(2026, 9, 1, 9), eventTimezone: 'Europe/Berlin' }); + expect((await h.run(CALENDAR_AUTHORITY)).stats.uploaded.created).toBe(1); + const created = String(h.events().find((e) => Number(e._id) === id)!._sync_id).split('/')[1]; + h.server.serverDestroy('CalendarEvent', 'a', created); + + const report = await h.run(CALENDAR_AUTHORITY); + + expect(report).toMatchObject({ outcome: 'ok', stats: { downloaded: { deleted: 1 } } }); + expect(h.events()).toEqual([]); + }); + + it('fetches a created object once, and writes nothing when it is unchanged', async () => { + const h = real(); + await h.run(); + addDeviceContact(h, 'Hedy Lamarr'); + await h.run(); + expect(h.state()?.accounts.a.created).toHaveLength(1); + + const before = h.batches.log.length; + expect((await h.run()).outcome).toBe('ok'); + + expect(h.state()?.accounts.a.created).toEqual([]); + expect(h.batches.log.slice(before).flat().filter((op) => op.op !== 'syncState')).toEqual([]); + expect(h.contacts().map((c) => c.name)).toEqual(['Hedy Lamarr']); + }); + + it('turns an upload-only sync into a full one while a created object waits to be checked', async () => { + const h = real(); + await h.run(); + const id = addDeviceContact(h, 'Hedy Lamarr'); + await h.run(); + const hedy = h.contacts().find((c) => c.id === id)!.sourceId!.split('/')[1]; + h.server.serverDestroy('ContactCard', 'a', hedy); + + // Android's upload sync 30 s after an app wrote anything, with nothing dirty in this account. + expect((await h.run(undefined, { upload: true })).outcome).toBe('ok'); + + expect(h.contacts()).toEqual([]); + }); +}); diff --git a/src/device-sync/engine/item-sync.ts b/src/device-sync/engine/item-sync.ts index 4bda226a..b0eae42d 100644 --- a/src/device-sync/engine/item-sync.ts +++ b/src/device-sync/engine/item-sync.ts @@ -44,6 +44,19 @@ function both(first: StateChange | undefined, second: StateChange | undefined): }; } +/** The work with one more state change, also after a re-plan. */ +function withState(work: Work, change: StateChange): Work { + const replan = work.replan; + return { + ...work, + state: both(work.state, change), + replan: replan && (async () => { + const next = await replan(); + return next && withState(next, change); + }), + }; +} + /** An item's upload, on its way to the server. */ export interface Pending { held: Held; @@ -324,10 +337,20 @@ export abstract class ItemSync { // ── Downloads ── - /** Objects put on `stale` by an earlier run: fetched again first (invariant 10). */ + /** + * Objects put on `stale` by an earlier run (invariant 10), and objects our + * uploads created since the stored state (`/changes` from it omits one that + * was created and destroyed since): fetched again first. + */ protected async refetchStale(acct: string): Promise { - const ids = [...this.env.store.account(acct).stale]; - if (ids.length) await this.processIds(acct, ids, [], () => undefined); + const account = this.env.store.account(acct); + const created = account.created ?? []; + const ids = [...new Set([...account.stale, ...created])]; + if (!ids.length) return; + await this.processIds(acct, ids, [], (next) => { + const a = accountOf(next, acct); + a.created = (a.created ?? []).filter((id) => !created.includes(id)); + }); } /** `/changes` pages from the stored state; a full reconcile without one or after `cannotCalculateChanges`. */ @@ -1070,7 +1093,8 @@ export abstract class ItemSync { } await this.beforePlanning(acct, [object]); const work = this.downloadWork(p.held.kind, acct, object, p.held.local, null); - return work ? [work] : []; + // Our own earlier create: remembered like one of this run. + return work ? [withState(work, this.rememberCreated(acct, id))] : []; } /** Sends the items' actions in `/set` calls and settles each item once all its actions have an answer. */ @@ -1319,7 +1343,7 @@ export abstract class ItemSync { this.markStale(acct, server.id); this.env.report.itemError({ ref: refOf(acct, server.id), side: 'upload', type: reason, description: message }); }; - const state = this.listing(kind, acct, server.id, true); + const state = both(this.listing(kind, acct, server.id, true), created ? this.rememberCreated(acct, server.id) : undefined); return { group: withExtra(plan.ops), state, @@ -1329,6 +1353,14 @@ export abstract class ItemSync { }; } + /** An object our upload created is remembered with its identity until the next run fetched it (see `refetchStale`). */ + private rememberCreated(acct: string, id: string): StateChange { + return (next) => { + const account = accountOf(next, acct); + account.created = [...new Set([...(account.created ?? []), id])]; + }; + } + private poisonWork(p: Pending, error: SetErrorShape): Work { const marker = nextMarker(p.meta.poison, p.fingerprint, error, this.env.now(), { firstMs: this.env.tuning.poisonBackoffMs, diff --git a/src/device-sync/engine/run.ts b/src/device-sync/engine/run.ts index 39c06178..883be9c5 100644 --- a/src/device-sync/engine/run.ts +++ b/src/device-sync/engine/run.ts @@ -48,7 +48,7 @@ function itemTypeOf(authority: Authority): string { /** Rows waiting for an upload (dirty, deleted, new), or state work a run must do whatever its extras. */ export async function hasLocalWork(reader: ProviderReader, authority: Authority, state: SyncState): Promise { for (const account of Object.values(state.accounts)) { - if (account.reconcile || account.stale.length) return true; + if (account.reconcile || account.stale.length || account.created?.length) return true; } if (authority === CALENDAR_AUTHORITY) { const rows = await reader.rows('events', ['_id'], "dirty = 1 OR deleted = 1 OR _sync_id IS NULL OR _sync_id LIKE '~pending/%'"); diff --git a/src/device-sync/engine/sync-state.ts b/src/device-sync/engine/sync-state.ts index 46e1e73a..2d07f9ab 100644 --- a/src/device-sync/engine/sync-state.ts +++ b/src/device-sync/engine/sync-state.ts @@ -29,6 +29,11 @@ export interface AccountState { selected: string[]; /** Object ids whose rows could not be written; fetched again at the start of every run, nothing uploads for them meanwhile. */ stale: string[]; + /** + * Objects our uploads created after `itemsState`: fetched again at the start of the next run, since `/changes` + * omits an object created and destroyed after its `sinceState`. + */ + created?: string[]; reconcile: ReconcileMarker | null; /** Contacts: refs of the group cards present on the device (a hard-deleted group is found by its absence). */ groups?: string[]; @@ -81,6 +86,7 @@ function parseAccount(value: unknown): AccountState { stale: strings(a.stale), reconcile: parseReconcile(a.reconcile), }; + if (a.created !== undefined) out.created = strings(a.created); if (a.groups !== undefined) out.groups = strings(a.groups); if (a.taskOnly !== undefined) out.taskOnly = strings(a.taskOnly); return out;