From f1f762c228f871a314c98d8f87706ff970485056 Mon Sep 17 00:00:00 2001 From: John Ellis Date: Mon, 24 Aug 2026 12:55:23 -0700 Subject: [PATCH] Empty a folder in batches the server will accept Emptying Deleted Items back-referenced one Email/query straight into one Email/set, so every id in the folder arrived in a single call. Stalwart refuses the whole call over maxObjectsInSet with requestTooLarge, which left a folder of 5192 messages impossible to empty at all. Walk the folder a page at a time instead: the filter re-runs each pass, so the next page is whatever is still there. A pass that destroys nothing stops the loop and reports the server's error rather than spinning. The same defect sat in two neighbours. Delete-forever on a large selection sent every id in one Email/set, and mark-all-read fed up to 5000 ids into an Email/get that only echoed them back - past maxObjectsInGet, and for no gain, since the query already returned them. Both now page through the same ceiling, read from the session rather than hardcoded. The mock advertised maxObjectsInSet but never enforced it, so none of this could fail in a test. It now rejects oversized get and set calls the way Stalwart does. While here, restrict emptying to Deleted Items. It was offered on Junk too, where a permanent one-shot clear is harder to justify; Junk is now select-all plus Delete, which takes the batched path. --- server/src/mock/index.ts | 27 ++++- web/src/jmap/client.ts | 7 +- web/src/store/__tests__/empty-mailbox.test.ts | 110 +++++++++++++++++ web/src/store/mail.ts | 113 +++++++++++++----- web/src/views/mail/MailboxTree.tsx | 2 +- web/src/views/mail/MessageList.tsx | 4 +- 6 files changed, 228 insertions(+), 35 deletions(-) create mode 100644 web/src/store/__tests__/empty-mailbox.test.ts diff --git a/server/src/mock/index.ts b/server/src/mock/index.ts index 8a37405..5a0ba10 100644 --- a/server/src/mock/index.ts +++ b/server/src/mock/index.ts @@ -254,6 +254,30 @@ class MethodError extends Error { } } +const MAX_OBJECTS = 500; + +/** + * Stalwart refuses a whole method call that carries more objects than it will + * process at once - it does not quietly handle the first 500. Enforce the same + * ceiling the session advertises, so an unbatched client fails here too. + */ +function enforceLimits(name: string, args: Obj): void { + const tooLarge = () => { + throw new MethodError("requestTooLarge", "The number of ids requested by the client exceeds the maximum number the server is willing to process in a single method call."); + }; + if (name.endsWith("/get")) { + const ids = args.ids as unknown[] | null | undefined; + if (Array.isArray(ids) && ids.length > MAX_OBJECTS) tooLarge(); + } + if (name.endsWith("/set")) { + const n = + Object.keys((args.create as Obj) ?? {}).length + + Object.keys((args.update as Obj) ?? {}).length + + ((args.destroy as unknown[] | undefined)?.length ?? 0); + if (n > MAX_OBJECTS) tooLarge(); + } +} + const setResp = (extra: Obj = {}): Obj => ({ accountId: ACCOUNT, oldState: "1", newState: nextState(), created: {}, updated: {}, destroyed: [], ...extra }); function genericGet(list: Obj[]) { @@ -520,7 +544,7 @@ function readBody(req: IncomingMessage): Promise { } const session = () => ({ - capabilities: { "urn:ietf:params:jmap:core": { maxSizeUpload: 50000000, maxConcurrentUpload: 4, maxSizeRequest: 10000000, maxConcurrentRequests: 4, maxCallsInRequest: 16, maxObjectsInGet: 500, maxObjectsInSet: 500, collationAlgorithms: ["i;ascii-casemap"] }, "urn:ietf:params:jmap:mail": {}, "urn:ietf:params:jmap:submission": {}, "urn:ietf:params:jmap:vacationresponse": {}, "urn:ietf:params:jmap:sieve": { implementation: "mock" }, "urn:ietf:params:jmap:calendars": {}, "urn:ietf:params:jmap:calendars:parse": {}, "urn:ietf:params:jmap:contacts": {}, "urn:ietf:params:jmap:contacts:parse": {}, "urn:ietf:params:jmap:principals": {}, "urn:ietf:params:jmap:principals:availability": {}, "urn:ietf:params:jmap:quota": {}, "urn:ietf:params:jmap:blob": {}, "urn:ietf:params:jmap:filenode": {}, ...(LEGACY ? {} : { "urn:stalwart:jmap": {} }) }, + capabilities: { "urn:ietf:params:jmap:core": { maxSizeUpload: 50000000, maxConcurrentUpload: 4, maxSizeRequest: 10000000, maxConcurrentRequests: 4, maxCallsInRequest: 16, maxObjectsInGet: MAX_OBJECTS, maxObjectsInSet: MAX_OBJECTS, collationAlgorithms: ["i;ascii-casemap"] }, "urn:ietf:params:jmap:mail": {}, "urn:ietf:params:jmap:submission": {}, "urn:ietf:params:jmap:vacationresponse": {}, "urn:ietf:params:jmap:sieve": { implementation: "mock" }, "urn:ietf:params:jmap:calendars": {}, "urn:ietf:params:jmap:calendars:parse": {}, "urn:ietf:params:jmap:contacts": {}, "urn:ietf:params:jmap:contacts:parse": {}, "urn:ietf:params:jmap:principals": {}, "urn:ietf:params:jmap:principals:availability": {}, "urn:ietf:params:jmap:quota": {}, "urn:ietf:params:jmap:blob": {}, "urn:ietf:params:jmap:filenode": {}, ...(LEGACY ? {} : { "urn:stalwart:jmap": {} }) }, accounts: { [ACCOUNT]: { name: USER, isPersonal: true, isReadOnly: false, accountCapabilities: { "urn:ietf:params:jmap:mail": {}, "urn:ietf:params:jmap:submission": {}, "urn:ietf:params:jmap:vacationresponse": {}, "urn:ietf:params:jmap:sieve": {}, "urn:ietf:params:jmap:calendars": {}, "urn:ietf:params:jmap:contacts": {}, "urn:ietf:params:jmap:principals": {}, "urn:ietf:params:jmap:quota": {}, "urn:ietf:params:jmap:filenode": {} } } }, primaryAccounts: { ...Object.fromEntries(["mail", "submission", "vacationresponse", "sieve", "calendars", "contacts", "principals", "quota", "filenode", "blob"].map((c) => [`urn:ietf:params:jmap:${c}`, ACCOUNT])), ...(LEGACY ? {} : { "urn:stalwart:jmap": ACCOUNT }) }, username: USER, @@ -596,6 +620,7 @@ export const server = createServer(async (req, res) => { if (!h || (LEGACY && name.startsWith("x:"))) { responses.push(["error", { type: "unknownMethod" }, id]); continue; } try { const args = resolveRefs(rawArgs, responses); + enforceLimits(name, args); const r = h(args); responses.push([name, r as Obj, id]); if (name.endsWith("/set") || name.endsWith("/import")) touched.add(name.split("/")[0]!); diff --git a/web/src/jmap/client.ts b/web/src/jmap/client.ts index d23c117..f84a85a 100644 --- a/web/src/jmap/client.ts +++ b/web/src/jmap/client.ts @@ -104,6 +104,11 @@ export class JmapClient { return core?.maxObjectsInGet ?? 500; } + get maxObjectsInSet(): number { + const core = this.session?.capabilities[CAP.core] as { maxObjectsInSet?: number } | undefined; + return core?.maxObjectsInSet ?? 500; + } + get maxSizeUpload(): number { const core = this.session?.capabilities[CAP.core] as { maxSizeUpload?: number } | undefined; return core?.maxSizeUpload ?? 50_000_000; @@ -358,7 +363,7 @@ export function ref(resultOf: string, name: string, path: string): ResultRef { return { resultOf, name, path }; } -/** Chunk ids for /get calls to respect maxObjectsInGet. */ +/** Chunk ids so a /get or /set call stays under the server's per-call maximum. */ export function chunk(arr: T[], size: number): T[][] { const out: T[][] = []; for (let i = 0; i < arr.length; i += size) out.push(arr.slice(i, i + size)); diff --git a/web/src/store/__tests__/empty-mailbox.test.ts b/web/src/store/__tests__/empty-mailbox.test.ts new file mode 100644 index 0000000..1f9598e --- /dev/null +++ b/web/src/store/__tests__/empty-mailbox.test.ts @@ -0,0 +1,110 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { CAP, client } from "@/jmap/client"; +import { useMail } from "@/store/mail"; +import { useToasts } from "@/ui/toast"; +import type { JmapSession } from "@/jmap/types"; + +/** + * Emptying a full folder used to back-reference one Email/query straight into + * one Email/set, so every id in the folder arrived in a single call. Stalwart + * refuses the whole call over `maxObjectsInSet` with `requestTooLarge` — a + * Deleted Items with 5192 messages in it could not be emptied at all. + */ + +const TRASH = "mbTrash"; +const MAX = 500; + +interface Call { + name: string; + args: Record; + id: string; +} + +/** A server that holds `count` messages and enforces MAX objects per call. */ +function server(count: number, opts: { refuseDestroy?: boolean } = {}) { + const live = new Set(Array.from({ length: count }, (_, i) => `e${i}`)); + const destroyBatches: number[] = []; + const fetchMock = vi.fn(async (_url: string, init: RequestInit) => { + const body = JSON.parse(init.body as string) as { methodCalls: [string, Record, string][] }; + const methodResponses = body.methodCalls.map(([name, args, id]: [string, Record, string]) => { + if (name === "Email/query" && (args.filter as { inMailbox?: string })?.inMailbox === TRASH) { + const limit = Math.min((args.limit as number) ?? 50, MAX); + return [name, { accountId: "a1", queryState: "q", canCalculateChanges: false, position: 0, ids: [...live].slice(0, limit), total: live.size }, id]; + } + if (name === "Email/set" && Array.isArray(args.destroy)) { + const destroy = args.destroy as string[]; + destroyBatches.push(destroy.length); + if (destroy.length > MAX) return ["error", { type: "requestTooLarge", description: "The number of ids requested by the client exceeds the maximum number the server is willing to process in a single method call." }, id]; + if (opts.refuseDestroy) return [name, { accountId: "a1", oldState: "1", newState: "2", destroyed: [], notDestroyed: Object.fromEntries(destroy.map((x) => [x, { type: "forbidden", description: "no" }])) }, id]; + for (const x of destroy) live.delete(x); + return [name, { accountId: "a1", oldState: "1", newState: "2", destroyed: destroy, notDestroyed: {} }, id]; + } + // Everything the store refreshes afterwards; shape fits get and query. + return [name, { accountId: "a1", state: "1", list: [], notFound: [], ids: [], total: 0, queryState: "q", position: 0, canCalculateChanges: false }, id]; + }) as unknown as Call[]; + return { ok: true, status: 200, json: async () => ({ methodResponses, sessionState: "1" }) } as Response; + }); + vi.stubGlobal("fetch", fetchMock); + return { live, destroyBatches, fetchMock }; +} + +const messages = () => useToasts.getState().toasts.map((t) => t.message); + +beforeEach(() => { + client.session = { + capabilities: { [CAP.core]: { maxObjectsInGet: MAX, maxObjectsInSet: MAX }, [CAP.mail]: {} }, + accounts: {}, + primaryAccounts: {}, + state: "s1", + } as unknown as JmapSession; + useMail.setState({ accountId: "a1", mailboxes: { [TRASH]: { id: TRASH, role: "trash", name: "Deleted Items" } } as never, list: null, emails: {} }); + useToasts.setState({ toasts: [] }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe("emptyMailbox", () => { + it("deletes a folder larger than maxObjectsInSet, one accepted batch at a time", async () => { + const s = server(5192); + await useMail.getState().emptyMailbox(TRASH); + expect(Math.max(...s.destroyBatches)).toBeLessThanOrEqual(MAX); + expect(s.live.size).toBe(0); + expect(messages()).toContain("Deleted 5192 messages"); + }); + + it("needs no batching when the folder already fits in one call", async () => { + const s = server(12); + await useMail.getState().emptyMailbox(TRASH); + expect(s.destroyBatches).toEqual([12]); + expect(messages()).toContain("Deleted 12 messages"); + }); + + it("refuses any folder that is not Deleted Items", async () => { + const s = server(5192); + useMail.setState({ mailboxes: { ...useMail.getState().mailboxes, mbJunk: { id: "mbJunk", role: "junk", name: "Junk" } } as never }); + await useMail.getState().emptyMailbox("mbJunk"); + expect(s.destroyBatches).toEqual([]); + expect(s.live.size).toBe(5192); + expect(messages()).toContain("Only Deleted Items can be emptied."); + }); + + it("stops instead of looping when the server destroys nothing", async () => { + const s = server(5192, { refuseDestroy: true }); + await useMail.getState().emptyMailbox(TRASH); + expect(s.destroyBatches).toHaveLength(1); + expect(messages().some((m) => m.startsWith("Could not empty folder"))).toBe(true); + }); +}); + +describe("destroy", () => { + it("splits a selection bigger than maxObjectsInSet across calls", async () => { + const s = server(1200); + await useMail.getState().destroy(Array.from({ length: 1200 }, (_, i) => `e${i}`)); + expect(s.destroyBatches).toEqual([MAX, MAX, 200]); + expect(s.live.size).toBe(0); + expect(messages()).toContain("1200 messages deleted forever"); + }); +}); diff --git a/web/src/store/mail.ts b/web/src/store/mail.ts index 5955768..0968403 100644 --- a/web/src/store/mail.ts +++ b/web/src/store/mail.ts @@ -11,6 +11,7 @@ import type { MailboxRole, QueryResponse, Quota, + SetError, SetResponse, Thread, VacationResponse, @@ -517,8 +518,8 @@ export const useMail = create((set, get) => ({ return { emails: next, selected: {} }; }); try { - const res = await client.call("Email/set", { accountId, destroy: ids }); - const failed = Object.keys(res.notDestroyed ?? {}); + const { notDestroyed } = await destroyEmails(accountId, ids); + const failed = Object.keys(notDestroyed); if (failed.length) toast.error(`${failed.length} message(s) could not be deleted`); else toast.show(`${ids.length === 1 ? "Message" : `${ids.length} messages`} deleted forever`); void get().loadMailboxes(); @@ -555,19 +556,44 @@ export const useMail = create((set, get) => ({ async emptyMailbox(mailboxId) { const accountId = get().accountId; if (!accountId) return; + // Emptying is permanent and covers the whole folder at once, so it is + // offered for Deleted Items alone. The menus hide it elsewhere; this is + // the guard that makes that true of the action itself. + if (mailboxId !== get().roleId("trash")) { + toast.error("Only Deleted Items can be emptied."); + return; + } + // A folder can hold far more messages than the server will destroy in one + // call, so walk it a page at a time instead of back-referencing one huge + // query into one Email/set. Each pass re-runs the filter, so the next page + // is simply whatever is still in the folder. + const page = client.maxObjectsInSet; + let deleted = 0; + let progress: number | null = null; try { - const res = await client.chain([ - ["Email/query", { accountId, filter: { inMailbox: mailboxId }, limit: 5000 }, "q"], - ["Email/set", { accountId, "#destroy": { resultOf: "q", name: "Email/query", path: "/ids" } }, "s"], - ]); - const s = res.get("s")?.[0] as unknown as SetResponse; - const n = s.destroyed?.length ?? 0; - toast.show(`Deleted ${n} message${n === 1 ? "" : "s"}`); + for (;;) { + const q = await client.call("Email/query", { accountId, filter: { inMailbox: mailboxId }, limit: page }); + if (!q.ids.length) break; + if (progress === null && (q.total ?? q.ids.length) > page) { + progress = toast.show("Emptying folder…", { duration: 0 }); + } + const { destroyed, notDestroyed } = await destroyEmails(accountId, q.ids); + deleted += destroyed.length; + // Nothing went through: the rest is undeletable, and looping again + // would ask for the same ids forever. + if (!destroyed.length) { + const [, err] = Object.entries(notDestroyed)[0] ?? []; + throw new Error(err ? setErrorMessage(err) : "the server refused to delete these messages"); + } + } + toast.show(`Deleted ${deleted} message${deleted === 1 ? "" : "s"}`); set({ list: get().list ? { ...get().list!, ids: get().list!.mailboxId === mailboxId ? [] : get().list!.ids, total: 0 } : null }); + } catch (err) { + toast.error(`Could not empty folder: ${(err as Error).message}${deleted ? ` (${deleted} deleted first)` : ""}`); + } finally { + if (progress !== null) toast.dismiss(progress); void get().loadMailboxes(); void get().refreshList(); - } catch (err) { - toast.error(`Could not empty folder: ${(err as Error).message}`); } }, @@ -590,32 +616,40 @@ export const useMail = create((set, get) => ({ const accountId = get().accountId; if (!accountId) return; const boxes = includeChildren ? get().descendantMailboxIds(mailboxId) : [mailboxId]; + // The ids the query returns are all we need; asking Email/get to echo them + // back only risks blowing past maxObjectsInGet on a very full folder. + const page = client.maxObjectsInSet; const unreadIn = async (filter: EmailFilter): Promise => { - const res = await client.chain([ - ["Email/query", { accountId, filter, limit: 5000 }, "q"], - ["Email/get", { accountId, "#ids": { resultOf: "q", name: "Email/query", path: "/ids" }, properties: ["id"] }, "g"], - ]); - return ((res.get("g")?.[0] as unknown as GetResponse).list ?? []).map((e) => e.id); + const res = await client.call("Email/query", { accountId, filter, limit: page }); + return res.ids; + }; + const nextUnread = async (): Promise => { + if (boxes.length === 1) return unreadIn({ inMailbox: boxes[0]!, notKeyword: "$seen" }); + try { + return await unreadIn({ operator: "AND", conditions: [{ notKeyword: "$seen" }, { operator: "OR", conditions: boxes.map((id) => ({ inMailbox: id })) }] }); + } catch { + // Server without filter-operator support: one query per folder. + const per = await Promise.all(boxes.map((id) => unreadIn({ inMailbox: id, notKeyword: "$seen" }).catch(() => [] as Id[]))); + return [...new Set(per.flat())]; + } }; try { - let ids: Id[]; - if (boxes.length === 1) { - ids = await unreadIn({ inMailbox: boxes[0]!, notKeyword: "$seen" }); - } else { - try { - ids = await unreadIn({ operator: "AND", conditions: [{ notKeyword: "$seen" }, { operator: "OR", conditions: boxes.map((id) => ({ inMailbox: id })) }] }); - } catch { - // Server without filter-operator support: one query per folder. - const per = await Promise.all(boxes.map((id) => unreadIn({ inMailbox: id, notKeyword: "$seen" }).catch(() => [] as Id[]))); - ids = [...new Set(per.flat())]; - } + // One page per pass; the ones just marked drop out of the filter, so a + // repeated head id means the last pass changed nothing and we stop. + let marked = 0; + let lastHead: Id | null = null; + for (;;) { + const ids = await nextUnread(); + if (!ids.length || ids[0] === lastHead) break; + lastHead = ids[0]!; + await get().markRead(ids, true); + marked += ids.length; } - if (!ids.length) { + if (!marked) { toast.show("Nothing unread here"); return; } - await get().markRead(ids, true); - toast.success(`Marked ${ids.length} message${ids.length === 1 ? "" : "s"} as read${includeChildren && boxes.length > 1 ? ` in ${boxes.length} folders` : ""}`); + toast.success(`Marked ${marked} message${marked === 1 ? "" : "s"} as read${includeChildren && boxes.length > 1 ? ` in ${boxes.length} folders` : ""}`); void get().loadMailboxes(); } catch (err) { toast.error(`Could not mark as read: ${(err as Error).message}`); @@ -860,9 +894,26 @@ async function runQuery(accountId: Id, q: ListQuery, position: number, limit: nu return { ids: query.ids, total: query.total ?? query.ids.length, queryState: query.queryState }; } +/** + * Destroy emails in batches the server will accept. + * + * Handing Email/set more ids than `maxObjectsInSet` fails the whole call with + * requestTooLarge — nothing is deleted — so split first and merge the results. + */ +async function destroyEmails(accountId: Id, ids: Id[]): Promise<{ destroyed: Id[]; notDestroyed: Record }> { + const destroyed: Id[] = []; + const notDestroyed: Record = {}; + for (const part of chunk(ids, client.maxObjectsInSet)) { + const res = await client.call("Email/set", { accountId, destroy: part }); + destroyed.push(...(res.destroyed ?? [])); + Object.assign(notDestroyed, res.notDestroyed ?? {}); + } + return { destroyed, notDestroyed }; +} + async function setEmails(accountId: Id, update: Record>) { const ids = Object.keys(update); - for (const part of chunk(ids, 400)) { + for (const part of chunk(ids, client.maxObjectsInSet)) { const sub: Record> = {}; for (const id of part) sub[id] = update[id]!; const res = await client.call("Email/set", { accountId, update: sub }); diff --git a/web/src/views/mail/MailboxTree.tsx b/web/src/views/mail/MailboxTree.tsx index f80514f..b1485d9 100644 --- a/web/src/views/mail/MailboxTree.tsx +++ b/web/src/views/mail/MailboxTree.tsx @@ -261,7 +261,7 @@ function MailboxMenu({ mailbox: m, onCreateChild, onShare }: { mailbox: Mailbox; : } label={m.isSubscribed ? "Hide from list" : "Show in list"} onClick={() => void useMail.getState().updateMailbox(m.id, { isSubscribed: !m.isSubscribed })} disabled={m.role === "inbox"} /> } label="Share…" onClick={onShare} /> - {(m.role === "trash" || m.role === "junk") && } label="Empty folder" onClick={() => void empty()} danger />} + {m.role === "trash" && } label="Empty folder" onClick={() => void empty()} danger />} } label="Delete folder" onClick={() => void remove()} danger disabled={isSpecial || !m.myRights.mayDelete} /> ); diff --git a/web/src/views/mail/MessageList.tsx b/web/src/views/mail/MessageList.tsx index 54b1325..8b256bb 100644 --- a/web/src/views/mail/MessageList.tsx +++ b/web/src/views/mail/MessageList.tsx @@ -74,6 +74,8 @@ export function MessageList({ title, list, openThreadId, focusId, setFocusId, on const selCount = Object.keys(selected).length; const mailbox = mailboxId ? mailboxes[mailboxId] : undefined; const isTrashOrJunk = mailbox?.role === "trash" || mailbox?.role === "junk"; + // Emptying in one action is for Deleted Items only; Junk is cleared by hand. + const isTrash = mailbox?.role === "trash"; const isDrafts = mailbox?.role === "drafts"; const rowHeight = twoLine ? (settings.density === "compact" ? 56 : settings.density === "comfortable" ? 78 : 66) : settings.density === "compact" ? 36 : settings.density === "comfortable" ? 52 : 44; @@ -198,7 +200,7 @@ export function MessageList({ title, list, openThreadId, focusId, setFocusId, on } label="Select all" onClick={selectAll} /> } label="Mark all as read" onClick={() => mailboxId && void useMail.getState().markMailboxRead(mailboxId)} disabled={!mailboxId} /> - {isTrashOrJunk && ( + {isTrash && ( <>