diff --git a/lib/__tests__/jmap-mailbox-pagination.test.ts b/lib/__tests__/jmap-mailbox-pagination.test.ts index 60cf0ea4c..4dea54e23 100644 --- a/lib/__tests__/jmap-mailbox-pagination.test.ts +++ b/lib/__tests__/jmap-mailbox-pagination.test.ts @@ -68,7 +68,6 @@ describe('JMAP mailbox pagination', () => { ['missing ids', undefined, 's', []], ['mixed ids', ['third', 4], 's', []], ['duplicate ids', ['parent'], 's', [{ id: 'parent', name: 'Parent' }]], - ['changed state', ['third'], 's2', [{ id: 'third', name: 'Third' }]], ['early empty page', [], 's', []], ['missing object', ['third'], 's', []], ])('rejects %s instead of publishing a partial hierarchy', async (_label, ids, queryState, list) => { @@ -143,16 +142,67 @@ describe('JMAP mailbox pagination', () => { await expect(client.getAllMailboxesWithState()).rejects.toThrow('complete mailbox list'); }); - it('pages when an unbounded get is rejected as tooManyObjects', async () => { + it('pages when an unbounded get is rejected as requestTooLarge (Stalwart 1.0)', async () => { const client = createClient(); const list = [{ id: 'a', name: 'A' }, { id: 'b', name: 'B' }, { id: 'c', name: 'C' }]; serve(client, { primary: list }); vi.mocked(client['request']).mockResolvedValueOnce({ methodResponses: [ - ['error', { type: 'tooManyObjects' }, '0'], + ['error', { type: 'requestTooLarge' }, '0'], ] }); expect((await client.getMailboxes()).map(({ id }) => id)).toEqual(['a', 'b', 'c']); }); + it('pages a shared account whose packed get is rejected as requestTooLarge', async () => { + const client = createClient(2, { primary: { name: 'Self' }, shared: { name: 'Shared' } }); + const shared = [{ id: 'a', name: 'A' }, { id: 'b', name: 'B', parentId: 'a' }, { id: 'c', name: 'C', parentId: 'b' }]; + serve(client, { primary: [{ id: 'inbox', name: 'Inbox' }], shared }); + vi.mocked(client['request']).mockResolvedValueOnce({ methodResponses: [ + ['Mailbox/get', { list: [{ id: 'inbox', name: 'Inbox' }], state: 'm' }, '0'], + ['error', { type: 'requestTooLarge' }, '1'], + ] }); + const result = await client.getAllMailboxesWithState(); + expect(result.mailboxes.map(({ id }) => id)).toEqual(['inbox', 'shared:a', 'shared:b', 'shared:c']); + expect(result.states).toEqual({ primary: 'm', shared: 'm' }); + }); + + it('starts over when the mailbox state moves between pages', async () => { + const client = createClient(); + const page = (ids: string[], queryState: string, total?: number) => ({ methodResponses: [ + ['Mailbox/query', { ids, queryState, ...(total ? { total } : {}) }, 'query'], + ['Mailbox/get', { list: ids.map((id) => ({ id, name: id })), state: queryState }, 'get'], + ] }); + const request = vi.spyOn(client as unknown as RequestTarget, 'request') + .mockResolvedValueOnce({ methodResponses: [['Mailbox/get', { list: [{ id: 'a' }, { id: 'b' }] }, '0']] }) + .mockResolvedValueOnce(page(['a', 'b'], 's1', 3)) + .mockResolvedValueOnce(page(['c'], 's2')) + .mockResolvedValueOnce(page(['a', 'b'], 's2', 3)) + .mockResolvedValueOnce(page(['c'], 's2')); + + const result = await client.getAllMailboxesWithState(); + expect(result.mailboxes.map(({ id }) => id)).toEqual(['a', 'b', 'c']); + expect(result.states).toEqual({ primary: 's2' }); + expect(request).toHaveBeenCalledTimes(5); + }); + + it('gives up when the mailbox state keeps moving', async () => { + const client = createClient(); + vi.spyOn(console, 'error').mockImplementation(() => {}); + let n = 0; + const request = vi.spyOn(client as unknown as RequestTarget, 'request').mockImplementation(async (calls) => { + if (calls[0][0] === 'Mailbox/get') return { methodResponses: [['Mailbox/get', { list: [{ id: 'a' }, { id: 'b' }] }, '0']] }; + const position = calls[0][1].position as number; + const ids = position === 0 ? ['a', 'b'] : ['c']; + const state = `s${n++}`; + return { methodResponses: [ + ['Mailbox/query', { ids, queryState: state, ...(position === 0 ? { total: 3 } : {}) }, 'query'], + ['Mailbox/get', { list: ids.map((id) => ({ id, name: id })), state }, 'get'], + ] }; + }); + + await expect(client.getMailboxes()).rejects.toThrow('changed while it was being loaded'); + expect(request).toHaveBeenCalledTimes(1 + 3 * 2); + }); + it.each([false, true])('respects maxCallsInRequest=1 (shared: %s)', async (shared) => { const client = createClient(2, shared ? { primary: { name: 'Self' }, shared: { name: 'Shared' } } : undefined); Object.assign(client, { capabilities: { 'urn:ietf:params:jmap:core': { maxObjectsInGet: 2, maxCallsInRequest: 1 } } }); diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 1f02ee7df..a5168fc8a 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -603,6 +603,23 @@ const MAILBOX_PROPERTIES = [ "isSubscribed", ] as const; +// A Mailbox/get without ids over maxObjectsInGet: Stalwart 0.16 cuts the list +// at the limit without saying so, 1.0 answers requestTooLarge (RFC 8620 §5.1). +function mailboxListCapped(response: JMAPResponse["methodResponses"][number] | undefined, maxObjects: number): boolean { + if (response?.[0] === "Mailbox/get") return Array.isArray(response[1].list) && response[1].list.length >= maxObjects; + return response?.[0] === "error" && response[1].type === "requestTooLarge"; +} + +// Every new message or flag change moves the Mailbox state (the counts), so a +// paged load is retried a few times before the refresh gives up. +const MAILBOX_TREE_ATTEMPTS = 3; + +class MailboxListChangedError extends Error { + constructor() { + super("Mailbox list changed while it was being loaded"); + } +} + // SieveScript properties (RFC 9661 §2), explicit for the same reason. const SIEVE_SCRIPT_PROPERTIES = ["id", "name", "blobId", "isActive"] as const; @@ -2277,6 +2294,16 @@ export class JMAPClient implements IJMAPClient { * children without their parents. Querying all ids keeps the tree complete. */ private async fetchMailboxTree(accountId: string): Promise { + for (let attempt = 1; ; attempt++) { + try { + return await this.fetchMailboxTreeOnce(accountId); + } catch (error) { + if (!(error instanceof MailboxListChangedError) || attempt >= MAILBOX_TREE_ATTEMPTS) throw error; + } + } + } + + private async fetchMailboxTreeOnce(accountId: string): Promise { const mailboxes: JMAPMailbox[] = []; const seenIds = new Set(); let position = 0; @@ -2335,10 +2362,10 @@ export class JMAPClient implements IJMAPClient { } if (queryState !== undefined && pageQueryState !== queryState) { - throw new Error("Mailbox list changed while it was being loaded"); + throw new MailboxListChangedError(); } if (mailboxState !== undefined && pageMailboxState !== undefined && pageMailboxState !== mailboxState) { - throw new Error("Mailbox list changed while it was being loaded"); + throw new MailboxListChangedError(); } queryState = pageQueryState; if (typeof pageMailboxState === "string") mailboxState = pageMailboxState; @@ -2367,8 +2394,7 @@ export class JMAPClient implements IJMAPClient { ]; let response = await this.request(calls, undefined, { skipOfflineRecord: true }); const first = response.methodResponses?.[0]; - if ((first?.[0] === "Mailbox/get" && first[1].list?.length >= this.getMaxObjectsInGet()) || - (first?.[0] === "error" && first[1].type === "tooManyObjects")) { + if (mailboxListCapped(first, this.getMaxObjectsInGet())) { response = await this.fetchMailboxTree(acctId); } getOfflineHooks()?.record(this, calls, response); @@ -2454,8 +2480,7 @@ export class JMAPClient implements IJMAPClient { let match = response.methodResponses?.find((r) => r[2] === String(i)); // Keep the packed fast path for small accounts. A full-sized result // may be capped, so reload that account through query/get pages. - if ((match?.[0] === "Mailbox/get" && Array.isArray(match[1].list) && match[1].list.length >= maxObjects) || - (match?.[0] === "error" && match[1].type === "tooManyObjects")) { + if (mailboxListCapped(match, maxObjects)) { try { const complete = await this.fetchMailboxTree(accountId); match = complete.methodResponses[0];