From 2e1cb5854806322e142d2402e6bfd4278aed5769 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 27 Sep 2026 07:47:55 +0200 Subject: [PATCH] fix: finish loading or removing a book after an interrupted sync #34 The SyncState's `selected` said which collections were on the device, and changed only when a load or a removal had finished. A book turned off and on again while the device was still removing it looked loaded, so the contacts removed so far never came back; a calendar came back empty. A book turned on and off again while it was still loading never looked loaded, so the contacts loaded so far were never removed. `selected` now names only collections whose rows are all on the device: a collection leaves it before its first row is removed, and the contacts SyncState lists collections that are loaded or removed only in part, so the next run loads them again when they are selected, or removes the rest when they are not. A book that still holds a change waiting for its upload stays in that list instead of `selected`. --- .../__tests__/engine/calendar-flow.test.ts | 9 +- .../__tests__/engine/selection-state.test.ts | 143 ++++++++++++++++++ src/device-sync/engine/calendar-sync.ts | 22 ++- src/device-sync/engine/contacts-sync.ts | 19 ++- src/device-sync/engine/item-sync.ts | 15 +- src/device-sync/engine/sync-state.ts | 8 +- 6 files changed, 201 insertions(+), 15 deletions(-) create mode 100644 src/device-sync/__tests__/engine/selection-state.test.ts diff --git a/src/device-sync/__tests__/engine/calendar-flow.test.ts b/src/device-sync/__tests__/engine/calendar-flow.test.ts index 9025737e..8f391630 100644 --- a/src/device-sync/__tests__/engine/calendar-flow.test.ts +++ b/src/device-sync/__tests__/engine/calendar-flow.test.ts @@ -125,7 +125,14 @@ describe('device sync engine: calendar', () => { expect(report.itemErrors).toEqual([expect.objectContaining({ ref: `a/${review}`, type: 'tooLarge' })]); expect(calendarRow(h, work)).toBeDefined(); expect(titles(h)).toEqual(['Code review']); - expect(h.state(CALENDAR_AUTHORITY)?.accounts.a.selected).toEqual([`a/${h.calendar}`, `a/${work}`]); + // Not complete any more (selected again, it is loaded again); its row is dropped again next run. + expect(h.state(CALENDAR_AUTHORITY)?.accounts.a.selected).toEqual([`a/${h.calendar}`]); + + h.clock.now += 2 * 3_600_000; + await h.run(CALENDAR_AUTHORITY); + + expect(h.server.get('CalendarEvent', 'a', review)).toMatchObject({ title: 'Code review' }); + expect(calendarRow(h, work)).toBeUndefined(); }); it('marks a calendar that mirrors a feed subscription read-only', async () => { diff --git a/src/device-sync/__tests__/engine/selection-state.test.ts b/src/device-sync/__tests__/engine/selection-state.test.ts new file mode 100644 index 00000000..01c683ed --- /dev/null +++ b/src/device-sync/__tests__/engine/selection-state.test.ts @@ -0,0 +1,143 @@ +// SyncState `selected` names the collections whose rows are all on the +// device: a newly selected collection is loaded, a deselected one dropped. +// A load or a drop that stopped half-way must neither hide the collection +// from the next load nor keep its rows from the next drop. + +import { describe, expect, it } from 'vitest'; +import { Events } from '../../android-columns'; +import { calendarPlanner } from '../../calendar/planner'; +import { contactsPlanner } from '../../contacts/planner'; +import { addServerCards, CALENDAR_AUTHORITY, createHarness, RUN_BUDGET, type Harness } from './harness'; + +function real(): Harness { + const h = createHarness(); + h.deps.planners = { contacts: contactsPlanner, calendar: calendarPlanner }; + return h; +} + +/** Runs out of time right after checkpoint `n` of the next run. */ +function outOfTimeAt(h: Harness, n: number): void { + h.checkpoints.count = 0; + h.checkpoints.onCheckpoint = (count) => { + if (count === n) h.clock.now += RUN_BUDGET; + }; +} + +describe('device sync engine: collections loaded or dropped in part', () => { + it('brings every card back when a book whose drop stopped half-way is selected again', async () => { + const h = real(); + const work = h.server.addAddressBook('a', { name: 'Work' }); + addServerCards(h, ['Ada']); + addServerCards(h, Array.from({ length: 120 }, (_, i) => `W${String(i).padStart(3, '0')}`), 'a', work); + await h.run(); + expect(h.contacts()).toHaveLength(121); + h.prefs.contactsSelection[`a/${work}`] = false; + // 'changes', 'deselect' (sync), 'deselect' (chunk 1): out of time after the first chunk went. + outOfTimeAt(h, 3); + expect((await h.run()).outcome).toBe('cancelled'); + h.checkpoints.onCheckpoint = undefined; + const left = h.contacts().length; + expect(left).toBeGreaterThan(1); + expect(left).toBeLessThan(121); + + h.prefs.contactsSelection[`a/${work}`] = true; + expect((await h.run()).outcome).toBe('ok'); + + expect(h.contacts()).toHaveLength(121); + expect(h.state()?.accounts.a.selected.sort()).toEqual([`a/${h.book}`, `a/${work}`].sort()); + }); + + it('finishes the drop of a book in the next run when it stopped half-way', async () => { + const h = real(); + const work = h.server.addAddressBook('a', { name: 'Work' }); + addServerCards(h, ['Ada']); + addServerCards(h, Array.from({ length: 120 }, (_, i) => `W${String(i).padStart(3, '0')}`), 'a', work); + await h.run(); + h.prefs.contactsSelection[`a/${work}`] = false; + outOfTimeAt(h, 3); + expect((await h.run()).outcome).toBe('cancelled'); + h.checkpoints.onCheckpoint = undefined; + + expect((await h.run()).outcome).toBe('ok'); + + expect(h.contacts().map((c) => c.name)).toEqual(['Ada']); + expect(h.state()?.accounts.a.selected).toEqual([`a/${h.book}`]); + }); + + it('keeps a deselected book partial while a dirty card of it waits, and drops it once that is uploaded', async () => { + const h = real(); + const work = h.server.addAddressBook('a', { name: 'Work' }); + addServerCards(h, ['Ada']); + const [boss] = addServerCards(h, ['Boss', 'Colleague'], 'a', work); + await h.run(); + const bossRow = h.contactNamed('Boss')!.id; + const name = h.nameDataRow(bossRow)!; + h.device.user.updateData(Number(name._id), { data1: 'The Boss' }); + h.server.setErrorFor('ContactCard', 'a', boss, { type: 'tooLarge' }); + h.prefs.contactsSelection[`a/${work}`] = false; + + await h.run(); + + expect(h.contacts().map((c) => c.name).sort()).toEqual(['Ada', 'The Boss']); + expect(h.state()?.accounts.a).toMatchObject({ selected: [`a/${h.book}`], partial: [`a/${work}`] }); + + h.clock.now += 2 * 3_600_000; + await h.run(); + + expect(h.server.get('ContactCard', 'a', boss)).toMatchObject({ name: { full: 'The Boss' } }); + expect(h.contacts().map((c) => c.name)).toEqual(['Ada']); + expect(h.state()?.accounts.a).toMatchObject({ selected: [`a/${h.book}`], partial: [] }); + }); + + it('drops the cards of a book deselected again while its load had stopped half-way', async () => { + const h = real(); + const work = h.server.addAddressBook('a', { name: 'Work' }); + h.prefs.contactsSelection[`a/${work}`] = false; + addServerCards(h, ['Ada']); + addServerCards(h, Array.from({ length: 120 }, (_, i) => `W${String(i).padStart(3, '0')}`), 'a', work); + await h.run(); + expect(h.contacts()).toHaveLength(1); + h.prefs.contactsSelection[`a/${work}`] = true; + // 'changes', 'select', 'download' (chunk 1): out of time after the first chunk came. + outOfTimeAt(h, 3); + expect((await h.run()).outcome).toBe('cancelled'); + h.checkpoints.onCheckpoint = undefined; + expect(h.contacts().length).toBeGreaterThan(1); + + h.prefs.contactsSelection[`a/${work}`] = false; + expect((await h.run()).outcome).toBe('ok'); + + expect(h.contacts().map((c) => c.name)).toEqual(['Ada']); + }); + + it('loads the events of a calendar dropped by a stopped run when it is selected again', async () => { + const h = real(); + const holidays = h.server.addCalendar('a', { name: 'Holidays' }); + const sports = h.server.addCalendar('a', { name: 'Sports' }); + for (const [calendar, title] of [[holidays, 'Easter'], [holidays, 'Christmas'], [sports, 'Match']] as const) { + h.server.addEvent('a', { uid: `u-${title}`, title, start: '2026-10-01T09:00:00', duration: 'PT1H', timeZone: 'Europe/Berlin', calendarIds: { [calendar]: true } }); + } + await h.run(CALENDAR_AUTHORITY); + expect(h.events()).toHaveLength(3); + // Both deselected; the run is cancelled once the first calendar's rows are gone. + h.prefs.calendarSelection[`a/${holidays}`] = false; + h.prefs.calendarSelection[`a/${sports}`] = false; + const original = h.deps.isCancelled; + let cancelled = false; + h.deps.isCancelled = async (runId) => { + const left = h.events().length; + if (!cancelled && left > 0 && left < 3) cancelled = true; + return cancelled || original(runId); + }; + expect((await h.run(CALENDAR_AUTHORITY)).outcome).toBe('cancelled'); + h.deps.isCancelled = original; + const titles = () => h.events().map((e) => String(e[Events.TITLE])).sort(); + const dropped = titles().includes('Match') ? holidays : sports; + const expected = dropped === holidays ? ['Christmas', 'Easter'] : ['Match']; + + h.prefs.calendarSelection[`a/${dropped}`] = true; + expect((await h.run(CALENDAR_AUTHORITY)).outcome).toBe('ok'); + + expect(titles()).toEqual(expected); + }); +}); diff --git a/src/device-sync/engine/calendar-sync.ts b/src/device-sync/engine/calendar-sync.ts index fab6327c..1cb3d787 100644 --- a/src/device-sync/engine/calendar-sync.ts +++ b/src/device-sync/engine/calendar-sync.ts @@ -546,7 +546,10 @@ export class CalendarSync extends ItemSync { /** * Deselected calendars lose their rows (the provider deletes their events) * once nothing in them waits for an upload; until then only their clean - * events go. Events that left every synced calendar go when clean. + * events go, and the calendar row is dropped again next run. A calendar + * leaves `selected` before its first row goes: stopped half-way and + * selected again, it is loaded again. Events that left every synced + * calendar go when clean. */ protected async dropOutsideSelection(acct: string): Promise { const synced = new Set(this.syncedCollections(acct).map((id) => collectionKey(acct, id))); @@ -554,12 +557,15 @@ export class CalendarSync extends ItemSync { const rows = this.deleteRows.filter((row) => accountOfRef(this.keyByRow.get(row) ?? null) === acct); const outside = [...(this.outside.get(acct) ?? [])]; if (!deselected.length && !rows.length && !outside.length) return; - const kept = new Set(); + if (deselected.length) { + await this.env.writer.write([], this.tail([acct], (next) => { + const a = accountOf(next, acct); + a.selected = a.selected.filter((key) => !deselected.includes(key)); + })); + } for (const rowId of rows) { await this.env.checkpoints.check('deselect'); - const key = this.keyByRow.get(rowId) as string; - if (await this.dropCalendarRow(rowId)) continue; - kept.add(key); + await this.dropCalendarRow(rowId); } const works: Work[] = []; for (const local of (await this.eventKind.loadByRefs(outside)).values()) { @@ -569,9 +575,11 @@ export class CalendarSync extends ItemSync { const work = this.removeWork(acct, id, { kind: this.eventKind, local }, 'outside'); if (work) works.push(work); } - const selected = [...synced, ...kept]; + // Every synced calendar was loaded before this pass; the others go with their calendar rows. await this.env.writer.write(works, this.tail([acct], (next) => { - accountOf(next, acct).selected = selected; + const a = accountOf(next, acct); + a.selected = [...synced]; + if (a.partial) a.partial = []; })); this.outside.delete(acct); } diff --git a/src/device-sync/engine/contacts-sync.ts b/src/device-sync/engine/contacts-sync.ts index 228443e8..0567e7bc 100644 --- a/src/device-sync/engine/contacts-sync.ts +++ b/src/device-sync/engine/contacts-sync.ts @@ -761,12 +761,15 @@ export class ContactsSync extends ItemSync { /** * After the upload: rows of contacts in no synced address book go, when * clean (a deselected book, or a card moved away on the server). Dirty - * ones stay until their upload succeeds, and keep their book selected. + * ones stay until their upload succeeds, and keep their book `partial`, + * so the next run drops it again. A book is `partial` from before its + * first dropped row until the last: a stop half-way leaves it to load + * again when it is selected again, or to drop again when it is not. */ protected async dropOutsideSelection(acct: string): Promise { const synced = new Set(this.syncedCollections(acct).map((id) => collectionKey(acct, id))); const committed = this.env.store.account(acct); - const deselected = committed.selected.filter((key) => !synced.has(key)); + const deselected = [...new Set([...committed.selected, ...(committed.partial ?? [])])].filter((key) => !synced.has(key)); const outside = [...(this.outside.get(acct) ?? [])]; if (!deselected.length && !outside.length) return; @@ -781,6 +784,11 @@ export class ContactsSync extends ItemSync { else for (const key of meta.collections) if (deselected.includes(key)) kept.add(key); }; if (deselected.length) { + await this.env.writer.write([], this.tail([acct], (next) => { + const a = accountOf(next, acct); + a.selected = a.selected.filter((key) => !deselected.includes(key)); + a.partial = [...new Set([...(a.partial ?? []), ...deselected])]; + })); const groupWorks: Work[] = []; for (const group of await this.loadGroupsWhere('sourceid LIKE ?', [`${acct}/%`])) consider({ kind: this.groupKind, local: group }, groupWorks); await this.env.writer.write(groupWorks); @@ -794,10 +802,11 @@ export class ContactsSync extends ItemSync { } await this.env.writer.write(works); } - const selected = [...synced, ...kept]; - // The book leaves `selected` with its last dropped row; one still holding a dirty item stays. + // Every synced book was loaded before this pass; a book still holding a dirty item stays partial. await this.env.writer.write([], this.tail([acct], (next) => { - accountOf(next, acct).selected = selected; + const a = accountOf(next, acct); + a.selected = [...synced]; + a.partial = [...kept]; })); } else { const works: Work[] = []; diff --git a/src/device-sync/engine/item-sync.ts b/src/device-sync/engine/item-sync.ts index 6b4c4c2c..362d5686 100644 --- a/src/device-sync/engine/item-sync.ts +++ b/src/device-sync/engine/item-sync.ts @@ -467,10 +467,16 @@ export abstract class ItemSync { account.itemsState = from; account.reconcile = null; account.selected = selected; + if (account.partial) account.partial = account.partial.filter((key) => !selected.includes(key)); }); } - /** Collections selected since the rows were written: loaded once the account is up to date. */ + /** + * Collections selected since the rows were written (or loaded or dropped + * only in part): loaded once the account is up to date. A collection is + * `partial` from before its first chunk until its last one makes it + * `selected`, so deselected again after a stop, its rows are dropped. + */ protected async loadNewlySelected(acct: string): Promise { const account = this.env.store.account(acct); if (!account.itemsState || account.reconcile) return; @@ -479,9 +485,16 @@ export abstract class ItemSync { if (this.env.store.account(acct).selected.includes(key)) continue; await this.env.checkpoints.check('select'); const ids = await this.env.jmap.queryAll(this.itemType, acct, { [this.parentFilter]: collection }); + if (!(this.env.store.account(acct).partial ?? []).includes(key)) { + await this.env.writer.write([], this.tail([acct], (next) => { + const a = accountOf(next, acct); + a.partial = [...new Set([...(a.partial ?? []), key])]; + })); + } await this.processIds(acct, ids, [], (next) => { const a = accountOf(next, acct); if (!a.selected.includes(key)) a.selected.push(key); + a.partial = (a.partial ?? []).filter((k) => k !== key); }); } } diff --git a/src/device-sync/engine/sync-state.ts b/src/device-sync/engine/sync-state.ts index 2d07f9ab..99ad9089 100644 --- a/src/device-sync/engine/sync-state.ts +++ b/src/device-sync/engine/sync-state.ts @@ -25,8 +25,13 @@ export interface AccountState { collectionsState: string | null; /** ContactCard / CalendarEvent state the rows describe. */ itemsState: string | null; - /** Collection keys the rows were written for. */ + /** Collection keys whose rows are all on the device. */ selected: string[]; + /** + * Collection keys whose rows may be on the device in part: a load or a drop that has not ended. Selected, such + * a collection is loaded again; deselected, it is dropped again. + */ + partial?: string[]; /** Object ids whose rows could not be written; fetched again at the start of every run, nothing uploads for them meanwhile. */ stale: string[]; /** @@ -86,6 +91,7 @@ function parseAccount(value: unknown): AccountState { stale: strings(a.stale), reconcile: parseReconcile(a.reconcile), }; + if (a.partial !== undefined) out.partial = strings(a.partial); 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);