From 8a7f8b0493df12cff8fd34ea8e4da4281274ce90 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Tue, 25 Aug 2026 19:06:59 +0200 Subject: [PATCH] fix: gate first-touch calendar/contacts requests to stop duplicate default calendars #907 Stalwart lazily creates an account's default calendar on the first request that touches it. Bulwark's login fires Calendar/get and CalendarEvent/query concurrently, and on multi-node servers each node can create its own default, leaving users with two identical "Personal" calendars. Hold later calendar and address-book requests per account until the first one settles. --- .../jmap-first-touch-ordering.test.ts | 138 ++++++++++++++++++ lib/jmap/__tests__/first-touch-gate.test.ts | 125 ++++++++++++++++ lib/jmap/client.ts | 45 +++--- lib/jmap/first-touch-gate.ts | 101 +++++++++++++ 4 files changed, 392 insertions(+), 17 deletions(-) create mode 100644 lib/__tests__/jmap-first-touch-ordering.test.ts create mode 100644 lib/jmap/__tests__/first-touch-gate.test.ts create mode 100644 lib/jmap/first-touch-gate.ts diff --git a/lib/__tests__/jmap-first-touch-ordering.test.ts b/lib/__tests__/jmap-first-touch-ordering.test.ts new file mode 100644 index 000000000..a8b9868e7 --- /dev/null +++ b/lib/__tests__/jmap-first-touch-ordering.test.ts @@ -0,0 +1,138 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { JMAPClient } from '../jmap/client'; + +/** + * #907: Stalwart lazily creates an account's default calendar / address book on + * the first request that touches the collection. When Bulwark's login burst + * fires Calendar/get and CalendarEvent/query concurrently against a clustered + * server, each node can create its own default - duplicating the "Personal" + * calendar. The client must therefore not put a second calendar (or contacts) + * request on the wire until the first one for that account has settled, while + * leaving unrelated requests (Mailbox/get) unaffected. + */ + +function makeSession() { + return { + capabilities: { + 'urn:ietf:params:jmap:core': {}, + 'urn:ietf:params:jmap:mail': {}, + 'urn:ietf:params:jmap:calendars': {}, + 'urn:ietf:params:jmap:contacts': {}, + }, + accounts: { + 'acct-1': { + name: 'test', + isPersonal: true, + accountCapabilities: { + 'urn:ietf:params:jmap:mail': {}, + 'urn:ietf:params:jmap:calendars': {}, + 'urn:ietf:params:jmap:contacts': {}, + }, + }, + }, + primaryAccounts: { + 'urn:ietf:params:jmap:mail': 'acct-1', + 'urn:ietf:params:jmap:calendars': 'acct-1', + 'urn:ietf:params:jmap:contacts': 'acct-1', + }, + apiUrl: 'https://mail.example.com/jmap/api', + downloadUrl: 'https://mail.example.com/jmap/download/{accountId}/{blobId}/{name}', + uploadUrl: 'https://mail.example.com/jmap/upload/{accountId}/', + eventSourceUrl: 'https://mail.example.com/jmap/eventsource', + }; +} + +function jsonResponse(body: unknown): Response { + return new Response(JSON.stringify(body), { status: 200, headers: { 'Content-Type': 'application/json' } }); +} + +interface Pending { + methods: string[]; + resolve: (body: unknown) => void; +} + +const flush = () => new Promise((r) => setTimeout(r, 0)); + +describe('JMAPClient first-touch ordering (#907)', () => { + let fetchSpy: ReturnType; + let onWire: Pending[]; + + beforeEach(async () => { + onWire = []; + fetchSpy = vi.spyOn(globalThis, 'fetch'); + }); + + afterEach(() => { + fetchSpy.mockRestore(); + }); + + async function connectedClient(): Promise { + fetchSpy.mockResolvedValueOnce(jsonResponse(makeSession())); + const client = new JMAPClient('https://mail.example.com', 'user@test.com', 'pass123'); + await client.connect(); + // Every API call from here on is parked until the test releases it, in the + // order it reached the (mock) network. + fetchSpy.mockImplementation((_url: string, init?: RequestInit) => { + const body = JSON.parse(String(init?.body ?? '{}')) as { methodCalls?: [string][] }; + return new Promise((resolve) => { + onWire.push({ + methods: (body.methodCalls ?? []).map((c) => c[0]), + resolve: (responseBody) => resolve(jsonResponse(responseBody)), + }); + }); + }); + return client; + } + + const emptyResponse = (method: string) => ({ methodResponses: [[method, { list: [], ids: [] }, '0']] }); + + it('holds CalendarEvent/query until the first Calendar/get for the account has settled', async () => { + const client = await connectedClient(); + + const calendars = client.getAllCalendars(); + const events = client.queryCalendarEvents({ after: '2026-01-01T00:00:00Z', before: '2026-02-01T00:00:00Z' }); + await flush(); + + expect(onWire.map((p) => p.methods[0])).toEqual(['Calendar/get']); + + onWire[0].resolve(emptyResponse('Calendar/get')); + await expect(calendars).resolves.toEqual([]); + await flush(); + + expect(onWire.map((p) => p.methods[0])).toEqual(['Calendar/get', 'CalendarEvent/query']); + onWire[1].resolve(emptyResponse('CalendarEvent/query')); + await expect(events).resolves.toEqual([]); + }); + + it('does not delay unrelated requests, nor contacts behind calendars', async () => { + const client = await connectedClient(); + + const calendars = client.getAllCalendars(); + const mailboxes = client.getMailboxes(); + const books = client.getAddressBooks(); + await flush(); + + expect(onWire.map((p) => p.methods[0]).sort()).toEqual(['AddressBook/get', 'Calendar/get', 'Mailbox/get']); + + for (const p of onWire) p.resolve(emptyResponse(p.methods[0])); + await Promise.all([calendars, mailboxes, books]); + }); + + it('lets calendar requests run concurrently after the first touch', async () => { + const client = await connectedClient(); + + const warm = client.getAllCalendars(); + await flush(); + onWire[0].resolve(emptyResponse('Calendar/get')); + await warm; + onWire = []; + + const a = client.queryCalendarEvents({ after: '2026-01-01T00:00:00Z', before: '2026-02-01T00:00:00Z' }); + const b = client.getAllCalendars(); + await flush(); + + expect(onWire.map((p) => p.methods[0]).sort()).toEqual(['Calendar/get', 'CalendarEvent/query']); + for (const p of onWire) p.resolve(emptyResponse(p.methods[0])); + await Promise.all([a, b]); + }); +}); diff --git a/lib/jmap/__tests__/first-touch-gate.test.ts b/lib/jmap/__tests__/first-touch-gate.test.ts new file mode 100644 index 000000000..864a60c9c --- /dev/null +++ b/lib/jmap/__tests__/first-touch-gate.test.ts @@ -0,0 +1,125 @@ +import { describe, it, expect } from 'vitest'; +import { FirstTouchGate, gateKeysFor } from '../first-touch-gate'; + +type Call = [string, Record, string]; + +function deferred() { + let resolve!: (v: T) => void; + let reject!: (e: unknown) => void; + const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); + return { promise, resolve, reject }; +} + +const flush = () => new Promise((r) => setTimeout(r, 0)); + +const calGet = (accountId = 'a'): Call => ['Calendar/get', { accountId }, '0']; +const evQuery = (accountId = 'a'): Call => ['CalendarEvent/query', { accountId }, '0']; +const bookGet = (accountId = 'a'): Call => ['AddressBook/get', { accountId }, '0']; +const mboxGet = (accountId = 'a'): Call => ['Mailbox/get', { accountId }, '0']; + +describe('gateKeysFor', () => { + it('maps calendar and contact methods to per-account keys', () => { + expect(gateKeysFor([calGet('a'), evQuery('a'), bookGet('b')]).sort()).toEqual([ + 'calendar:a', + 'contacts:b', + ]); + }); + + it('ignores non-gated methods and calls without an accountId', () => { + expect(gateKeysFor([mboxGet(), ['Email/query', { accountId: 'a' }, '0']])).toEqual([]); + expect(gateKeysFor([['Calendar/get', {}, '0']])).toEqual([]); + }); +}); + +describe('FirstTouchGate', () => { + it('holds a second calendar request until the first one for that account settles', async () => { + const gate = new FirstTouchGate(); + const first = deferred(); + const order: string[] = []; + + const p1 = gate.run([calGet()], () => { order.push('send1'); return first.promise; }); + const p2 = gate.run([evQuery()], async () => { order.push('send2'); return 'two'; }); + await flush(); + expect(order).toEqual(['send1']); + + first.resolve('one'); + await expect(p1).resolves.toBe('one'); + await expect(p2).resolves.toBe('two'); + expect(order).toEqual(['send1', 'send2']); + }); + + it('lets requests flow concurrently once the first touch has settled', async () => { + const gate = new FirstTouchGate(); + await gate.run([calGet()], async () => 'warm'); + + const a = deferred(); + const order: string[] = []; + const p1 = gate.run([calGet()], () => { order.push('send1'); return a.promise; }); + const p2 = gate.run([evQuery()], async () => { order.push('send2'); return 'two'; }); + await flush(); + expect(order).toEqual(['send1', 'send2']); + a.resolve('one'); + await Promise.all([p1, p2]); + }); + + it('releases the gate when the first request fails', async () => { + const gate = new FirstTouchGate(); + const first = deferred(); + const p1 = gate.run([calGet()], () => first.promise); + const p2 = gate.run([calGet()], async () => 'two'); + first.reject(new Error('boom')); + await expect(p1).rejects.toThrow('boom'); + await expect(p2).resolves.toBe('two'); + }); + + it('does not gate different accounts or different collections against each other', async () => { + const gate = new FirstTouchGate(); + const first = deferred(); + const order: string[] = []; + const p1 = gate.run([calGet('a')], () => { order.push('cal-a'); return first.promise; }); + const p2 = gate.run([calGet('b')], async () => { order.push('cal-b'); return 'b'; }); + const p3 = gate.run([bookGet('a')], async () => { order.push('book-a'); return 'book'; }); + const p4 = gate.run([mboxGet('a')], async () => { order.push('mbox-a'); return 'mbox'; }); + await flush(); + expect(order).toEqual(['cal-a', 'cal-b', 'book-a', 'mbox-a']); + first.resolve('a'); + await Promise.all([p1, p2, p3, p4]); + }); + + it('a request spanning two collections waits for every in-flight first touch it overlaps', async () => { + const gate = new FirstTouchGate(); + const cal = deferred(); + const book = deferred(); + const order: string[] = []; + const p1 = gate.run([calGet()], () => { order.push('cal'); return cal.promise; }); + const p2 = gate.run([evQuery(), bookGet()], async () => { order.push('mixed'); return 'mixed'; }); + // Nothing is in flight for contacts yet, so this one is the contacts + // first touch and goes out immediately; the mixed request must now wait + // for both. + const p3 = gate.run([bookGet()], () => { order.push('book'); return book.promise; }); + await flush(); + expect(order).toEqual(['cal', 'book']); + + cal.resolve('cal'); + await flush(); + expect(order).toEqual(['cal', 'book']); + + book.resolve('book'); + await Promise.all([p1, p2, p3]); + expect(order).toEqual(['cal', 'book', 'mixed']); + }); + + it('reset() forgets settled keys so the next request is gated again', async () => { + const gate = new FirstTouchGate(); + await gate.run([calGet()], async () => 'warm'); + gate.reset(); + const first = deferred(); + const order: string[] = []; + const p1 = gate.run([calGet()], () => { order.push('1'); return first.promise; }); + const p2 = gate.run([calGet()], async () => { order.push('2'); return '2'; }); + await flush(); + expect(order).toEqual(['1']); + first.resolve('1'); + await Promise.all([p1, p2]); + }); +}); diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 6e6baee5d..5daad835e 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -5,6 +5,7 @@ import type { IJMAPClient, KeywordDiscoveryResult, KeywordInfo, KeywordMigration import { toWildcardQuery } from "./search-utils"; import { batched, itemsPerRequest } from "./request-limits"; import { keywordPointer } from "./patch-pointer"; +import { FirstTouchGate } from "./first-touch-gate"; import { debug } from "@/lib/debug"; import { normalizeCalendarEventLike } from "@/lib/calendar-event-normalization"; import { findTasksOnlyCalendarIds, isTaskLikeObject, type ScannedCalendarObject } from "@/lib/calendar-component-detection"; @@ -636,6 +637,9 @@ export class JMAPClient implements IJMAPClient { private rateLimitCallback: ((rateLimited: boolean, retryAfterMs: number) => void) | null = null; private rateLimitTimeout: NodeJS.Timeout | null = null; private lastRateLimitNoticeAt: number = 0; + // Serialises the first calendar/contacts request per account so Stalwart's + // lazy default-calendar/address-book creation can't run twice (#907). + private firstTouchGate = new FirstTouchGate(); constructor(serverUrl: string, username: string, password: string) { this.serverUrl = serverUrl.replace(/\/$/, ''); @@ -1083,13 +1087,16 @@ export class JMAPClient implements IJMAPClient { methodCalls, }; - const response = await this.authenticatedFetch(this.apiUrl, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify(requestBody), - }); + const apiUrl = this.apiUrl; + const response = await this.firstTouchGate.run(methodCalls, () => + this.authenticatedFetch(apiUrl, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + }, + body: JSON.stringify(requestBody), + }), + ); const responseText = await response.text(); @@ -6700,11 +6707,13 @@ export class JMAPClient implements IJMAPClient { } try { const { using, methodCalls } = this.buildStatePollingRequest(); - const response = await this.authenticatedFetch(this.apiUrl, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ using, methodCalls }), - }); + const response = await this.firstTouchGate.run(methodCalls, () => + this.authenticatedFetch(this.apiUrl, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ using, methodCalls }), + }), + ); if (response.ok) { const data = await response.json(); @@ -6733,11 +6742,13 @@ export class JMAPClient implements IJMAPClient { this.stateCheckInFlight = true; try { const { using, methodCalls } = this.buildStatePollingRequest(); - const response = await this.authenticatedFetch(this.apiUrl, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ using, methodCalls }), - }); + const response = await this.firstTouchGate.run(methodCalls, () => + this.authenticatedFetch(this.apiUrl, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ using, methodCalls }), + }), + ); if (response.ok) { const data = await response.json(); diff --git a/lib/jmap/first-touch-gate.ts b/lib/jmap/first-touch-gate.ts new file mode 100644 index 000000000..0521b9eef --- /dev/null +++ b/lib/jmap/first-touch-gate.ts @@ -0,0 +1,101 @@ +/** + * Serialises the FIRST request that touches a lazily-initialised server + * collection, per account. + * + * Stalwart creates an account's default calendar (and default address book) on + * the first access to an account that has none. On a single node that creation + * is guarded by the node's in-memory resource cache, but in clustered / multi- + * node deployments each node builds its own cache, so concurrent first-touch + * requests that land on different nodes each see "no calendars" and each create + * a default - leaving the user with two identical "Personal" calendars, one of + * which is flagged `isDefault` and can't be removed from the UI (#907). + * + * Bulwark's login fans out `Calendar/get`, `CalendarEvent/query` (and + * `AddressBook/get`, `ContactCard/query`) within a few milliseconds of each + * other, which makes that race trivially winnable. This gate holds every later + * request on a (collection, account) pair until the first one has settled - + * success or failure - after which requests flow with no further coordination. + * Method calls outside the gated collections are never delayed. + */ + +type MethodCall = [string, Record, string]; + +export type GatedCollection = 'calendar' | 'contacts'; + +const COLLECTION_PREFIXES: ReadonlyArray = [ + ['calendar', 'Calendar/'], + ['calendar', 'CalendarEvent/'], + ['calendar', 'CalendarEventNotification/'], + ['contacts', 'AddressBook/'], + ['contacts', 'ContactCard/'], +]; + +function collectionOf(method: string): GatedCollection | null { + for (const [collection, prefix] of COLLECTION_PREFIXES) { + if (method.startsWith(prefix)) return collection; + } + return null; +} + +/** + * The `collection:accountId` keys a request touches. Calls without a string + * accountId are skipped - they'd be rejected by the server anyway. + */ +export function gateKeysFor(methodCalls: ReadonlyArray): string[] { + const keys = new Set(); + for (const call of methodCalls) { + const collection = collectionOf(call[0]); + const accountId = call[1]?.accountId; + if (collection && typeof accountId === 'string' && accountId) { + keys.add(`${collection}:${accountId}`); + } + } + return Array.from(keys); +} + +export class FirstTouchGate { + private settled = new Set(); + private inFlight = new Map>(); + + /** + * Run `send` for `methodCalls`, delaying it while another request is the + * first to touch one of the collections it uses. Resolves/rejects with + * whatever `send` does. + */ + async run(methodCalls: ReadonlyArray, send: () => Promise): Promise { + const pending = gateKeysFor(methodCalls).filter((key) => !this.settled.has(key)); + if (pending.length === 0) return send(); + + const waits = pending + .map((key) => this.inFlight.get(key)) + .filter((p): p is Promise => p !== undefined); + if (waits.length > 0) { + await Promise.all(waits); + // Those keys are settled now; anything still pending belongs to another + // collection/account this request happens to be first on. + return this.run(methodCalls, send); + } + + // First touch for every pending key: run it and let the others queue. + const result = send(); + const done = result.then( + () => undefined, + () => undefined, + ); + for (const key of pending) this.inFlight.set(key, done); + try { + return await result; + } finally { + for (const key of pending) { + this.settled.add(key); + this.inFlight.delete(key); + } + } + } + + /** Forget everything - e.g. after reconnecting as a different account. */ + reset(): void { + this.settled.clear(); + this.inFlight.clear(); + } +}