From e2c76d46c720e6060b1850f7b5478c811e0e4cfc Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Fri, 25 Sep 2026 20:45:18 +0200 Subject: [PATCH] fix: stop the app re-reading the same folder changes forever When Mailbox/changes answered with `hasMoreChanges` (Stalwart caps one answer at 5000 changes, other servers earlier), the folder sync applied that page but kept the old state and ran again, which asked for and got the same first page back, without end. The sync now follows `newState` page by page, storing each page's state as it applies it. After five pages, or when a page does not move the state on or can't be calculated, it reads the whole folder list instead, which takes one request whatever changed. --- src/stores/__tests__/email-store.test.ts | 90 ++++++++++++++++++++++++ src/stores/email-store.ts | 82 +++++++++++---------- 2 files changed, 133 insertions(+), 39 deletions(-) diff --git a/src/stores/__tests__/email-store.test.ts b/src/stores/__tests__/email-store.test.ts index b838c4d7..0298fc12 100644 --- a/src/stores/__tests__/email-store.test.ts +++ b/src/stores/__tests__/email-store.test.ts @@ -231,6 +231,96 @@ describe('email-store', () => { expect(useEmailStore.getState().error).toBe('Network error'); }); + + // Stalwart answers at most 5000 changes per Mailbox/changes, with + // hasMoreChanges and an intermediate newState ("r…") to continue from. + describe('Mailbox/changes paged with hasMoreChanges', () => { + const mockGetMailboxChanges = emailApi.getMailboxChanges as ReturnType; + const mockGetMailboxesByIds = emailApi.getMailboxesByIds as ReturnType; + const folder = (id: string, name: string) => ({ id, name, isShared: false }) as any; + const page = (sinceState: string, newState: string, hasMoreChanges: boolean, changes: { + created?: string[]; updated?: string[]; destroyed?: string[]; + } = {}) => ({ + oldState: sinceState, newState, hasMoreChanges, + created: changes.created ?? [], updated: changes.updated ?? [], destroyed: changes.destroyed ?? [], + }); + + beforeEach(() => { + useEmailStore.setState({ + mailboxState: 'sbq', + mailboxes: [folder('a', 'Inbox'), folder('x', 'Old project')], + }); + }); + + it('follows newState page by page, where it used to ask for the first page forever', async () => { + mockGetMailboxChanges + .mockResolvedValueOnce(page('sbq', 'raehae', true, { created: ['n'], updated: ['a'] })) + .mockResolvedValueOnce(page('raehae', 'sca', false, { destroyed: ['x'] })); + mockGetMailboxesByIds.mockResolvedValueOnce({ list: [folder('n', 'New'), folder('a', 'Inbox (renamed)')], state: 'raehae' }); + + await useEmailStore.getState().fetchMailboxes(); + + expect(mockGetMailboxChanges.mock.calls.map(([since]) => since)).toEqual(['sbq', 'raehae']); + expect(mockGetMailboxesByIds).toHaveBeenCalledWith(['n', 'a']); + expect(mockGetMailboxesWithState).not.toHaveBeenCalled(); + const state = useEmailStore.getState(); + expect(state.mailboxState).toBe('sca'); + expect(state.mailboxes.map((m) => [m.id, m.name])).toEqual([['a', 'Inbox (renamed)'], ['n', 'New']]); + }); + + it('stores each page before asking for the next one', async () => { + const seen: Array = []; + mockGetMailboxChanges + .mockResolvedValueOnce(page('sbq', 'raehae', true)) + .mockImplementationOnce(async (since: string) => { + seen.push(useEmailStore.getState().mailboxState); + return page(since, 'sca', false); + }); + + await useEmailStore.getState().fetchMailboxes(); + + expect(seen).toEqual(['raehae']); + expect(useEmailStore.getState().mailboxState).toBe('sca'); + }); + + it('reads the whole folder list after five pages', async () => { + // A sixth call would get the default (null) and still reload, so the + // call count is what shows the cap. + for (let n = 1; n <= 5; n++) { + mockGetMailboxChanges.mockImplementationOnce(async (since: string) => page(since, `r${n}`, true)); + } + mockGetMailboxesWithState.mockResolvedValueOnce({ list: [folder('a', 'Inbox')], state: 'sfull' }); + + await useEmailStore.getState().fetchMailboxes(); + + expect(mockGetMailboxChanges.mock.calls.map(([since]) => since)).toEqual(['sbq', 'r1', 'r2', 'r3', 'r4']); + expect(mockGetMailboxesWithState).toHaveBeenCalledTimes(1); + expect(useEmailStore.getState().mailboxState).toBe('sfull'); + }); + + it('reads the whole folder list when a page does not move the state on', async () => { + mockGetMailboxChanges.mockResolvedValueOnce(page('sbq', 'sbq', true, { updated: ['a'] })); + mockGetMailboxesWithState.mockResolvedValueOnce({ list: [folder('a', 'Inbox')], state: 'sfull' }); + + await useEmailStore.getState().fetchMailboxes(); + + expect(mockGetMailboxChanges).toHaveBeenCalledTimes(1); + expect(useEmailStore.getState().mailboxState).toBe('sfull'); + }); + + it('reads the whole folder list when a later page cannot be calculated', async () => { + mockGetMailboxChanges + .mockResolvedValueOnce(page('sbq', 'raehae', true, { destroyed: ['x'] })) + .mockResolvedValueOnce(null); + mockGetMailboxesWithState.mockResolvedValueOnce({ list: [folder('a', 'Inbox'), folder('z', 'Later')], state: 'sfull' }); + + await useEmailStore.getState().fetchMailboxes(); + + const state = useEmailStore.getState(); + expect(state.mailboxState).toBe('sfull'); + expect(state.mailboxes.map((m) => m.id)).toEqual(['a', 'z']); + }); + }); }); describe('selectMailbox', () => { diff --git a/src/stores/email-store.ts b/src/stores/email-store.ts index ef9098f5..2b8a452a 100644 --- a/src/stores/email-store.ts +++ b/src/stores/email-store.ts @@ -2390,6 +2390,10 @@ function mailboxPath(mailboxes: Mailbox[], mailboxId: string): string | undefine return parts.join(' / '); } +// Pages of Mailbox/changes one folder sync follows before it reads the whole +// folder list instead, which takes one request whatever changed. +const MAX_MAILBOX_CHANGE_ROUNDS = 5; + async function fetchMailboxesImpl(activeAccountId: string): Promise { const get = useEmailStore.getState; const set = useEmailStore.setState; @@ -2405,49 +2409,51 @@ async function fetchMailboxesImpl(activeAccountId: string): Promise { }; try { - let drainAgain = false; - - // Incremental path: ask for just what changed since last time. Fall - // through to a full refetch when the server can't compute the diff or - // we have no previous state to compare against. + // Incremental path: ask for just what changed since last time. The + // server caps one answer (hasMoreChanges; Stalwart at 5000 changes), + // so follow its newState page by page, storing each page's state as it + // is applied: asking again from the old state got the same first page + // back forever. Fall through to a full refetch when the server can't + // compute the diff, when a page doesn't move the state on, after + // MAX_MAILBOX_CHANGE_ROUNDS pages, or with no previous state to compare + // against. let syncedOwn = false; - if (prevState) { - const changes = await getMailboxChanges(prevState); + let sinceState = prevState; + for (let round = 0; sinceState && round < MAX_MAILBOX_CHANGE_ROUNDS; round++) { + const changes = await getMailboxChanges(sinceState); // Bail if the user switched accounts during the await — anything we // set() now would land in the wrong account's bucket. if (get().activeAccountId !== activeAccountId) return; - if (changes) { - syncedOwn = true; - // No changes at all — keep the cached list, just bump the state. - if ( - changes.created.length === 0 && - changes.updated.length === 0 && - changes.destroyed.length === 0 - ) { - set({ mailboxState: changes.newState }); - } else { - const toFetch = [...changes.created, ...changes.updated]; - const fetched = toFetch.length > 0 - ? (await getMailboxesByIds(toFetch)).list - : []; - if (get().activeAccountId !== activeAccountId) return; - const destroyed = new Set(changes.destroyed); - const byId = new Map(); - for (const m of get().mailboxes) { - if (!m.isShared) byId.set(m.id, m); - } - for (const m of fetched) byId.set(m.id, m); - for (const id of destroyed) byId.delete(id); - replaceOwn( - Array.from(byId.values()), - changes.hasMoreChanges ? prevState : changes.newState, - ); - // hasMoreChanges = there are still pending changes past the - // server's response cap. Run the same path again to drain. - drainAgain = changes.hasMoreChanges; + // null → cannotCalculateChanges. Fall through to full. + if (!changes) break; + // No changes at all — keep the cached list, just bump the state. + if ( + changes.created.length === 0 && + changes.updated.length === 0 && + changes.destroyed.length === 0 + ) { + set({ mailboxState: changes.newState }); + } else { + const toFetch = [...changes.created, ...changes.updated]; + const fetched = toFetch.length > 0 + ? (await getMailboxesByIds(toFetch)).list + : []; + if (get().activeAccountId !== activeAccountId) return; + const destroyed = new Set(changes.destroyed); + const byId = new Map(); + for (const m of get().mailboxes) { + if (!m.isShared) byId.set(m.id, m); } + for (const m of fetched) byId.set(m.id, m); + for (const id of destroyed) byId.delete(id); + replaceOwn(Array.from(byId.values()), changes.newState); } - // changes === null → cannotCalculateChanges. Fall through to full. + if (!changes.hasMoreChanges) { + syncedOwn = true; + break; + } + if (!changes.newState || changes.newState === sinceState) break; + sinceState = changes.newState; } if (!syncedOwn) { @@ -2455,8 +2461,6 @@ async function fetchMailboxesImpl(activeAccountId: string): Promise { if (get().activeAccountId !== activeAccountId) return; replaceOwn(list, state); } - - if (drainAgain) void syncMailboxes(activeAccountId, { own: true, shared: false }); } catch (err) { console.warn('[email-store] fetchMailboxes failed:', err); if (get().activeAccountId !== activeAccountId) return;