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.
This commit is contained in:
Linus Rath
2026-09-27 07:53:36 +02:00
parent fad7b30dbf
commit 2e9e5bb785
4 changed files with 121 additions and 6 deletions
@@ -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([]);
});
});
+37 -5
View File
@@ -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<void> {
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,
+1 -1
View File
@@ -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<boolean> {
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/%'");
+6
View File
@@ -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;