mirror of
https://github.com/bulwarkmail/webmail.git
synced 2026-11-02 08:15:55 +00:00
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.
This commit is contained in:
@@ -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<void>((r) => setTimeout(r, 0));
|
||||
|
||||
describe('JMAPClient first-touch ordering (#907)', () => {
|
||||
let fetchSpy: ReturnType<typeof vi.spyOn>;
|
||||
let onWire: Pending[];
|
||||
|
||||
beforeEach(async () => {
|
||||
onWire = [];
|
||||
fetchSpy = vi.spyOn(globalThis, 'fetch');
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
fetchSpy.mockRestore();
|
||||
});
|
||||
|
||||
async function connectedClient(): Promise<JMAPClient> {
|
||||
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<Response>((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]);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,125 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { FirstTouchGate, gateKeysFor } from '../first-touch-gate';
|
||||
|
||||
type Call = [string, Record<string, unknown>, string];
|
||||
|
||||
function deferred<T = void>() {
|
||||
let resolve!: (v: T) => void;
|
||||
let reject!: (e: unknown) => void;
|
||||
const promise = new Promise<T>((res, rej) => { resolve = res; reject = rej; });
|
||||
return { promise, resolve, reject };
|
||||
}
|
||||
|
||||
const flush = () => new Promise<void>((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<string>();
|
||||
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<string>();
|
||||
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<string>();
|
||||
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<string>();
|
||||
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<string>();
|
||||
const book = deferred<string>();
|
||||
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<string>();
|
||||
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]);
|
||||
});
|
||||
});
|
||||
+28
-17
@@ -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();
|
||||
|
||||
@@ -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, unknown>, string];
|
||||
|
||||
export type GatedCollection = 'calendar' | 'contacts';
|
||||
|
||||
const COLLECTION_PREFIXES: ReadonlyArray<readonly [GatedCollection, string]> = [
|
||||
['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<MethodCall>): string[] {
|
||||
const keys = new Set<string>();
|
||||
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<string>();
|
||||
private inFlight = new Map<string, Promise<void>>();
|
||||
|
||||
/**
|
||||
* 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<T>(methodCalls: ReadonlyArray<MethodCall>, send: () => Promise<T>): Promise<T> {
|
||||
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<void> => 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();
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user