From cd402a6ce45a35d1d7a4fd3eecd8265aab449bf8 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 27 Aug 2026 12:40:09 -0700 Subject: [PATCH] Make the mock report what changed Two silences, and between them the whole change-reconciliation path was untestable here. `Email/set` never announced anything. A real server pushes a state change after a set and the client acts on it -- `Email/changes`, then the store deciding what to do with the answer. The mock said nothing, so that path simply did not run. And `Email/changes` returned three empty arrays whatever had happened. So even when it was asked, the answer was that nothing had changed. Together they meant every version of the mark-read code has been checked against a server that never reported the change being made. That is how #100 reached production, and why the fix for it could be verified in the message view -- where the flicker partly was -- while whatever remains stayed invisible, because the code that runs when the server answers back has never run here at all. The mock now records what each set created, updated and destroyed against the state it happened in, answers `Email/changes` from that log, and broadcasts afterwards the way Stalwart does. This is a mock change on its own. It fixes nothing and is not meant to: it makes a path testable that was not, which is the prerequisite for finding what is left of #100 rather than guessing at it. I had a theory about `fullIds` eviction and reverted it -- three attempts to reproduce the symptom against this mock failed, which was itself the finding. --- server/src/mock/index.ts | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/server/src/mock/index.ts b/server/src/mock/index.ts index 6d8dd83..0602bb2 100644 --- a/server/src/mock/index.ts +++ b/server/src/mock/index.ts @@ -472,7 +472,22 @@ const handlers: Record = { return { accountId: ACCOUNT, queryState: String(state.n), canCalculateChanges: false, position: pos, ids: list.slice(pos, pos + limit).map((e) => e.id), total: list.length, limit }; }, "Email/get": (a) => genericGet(emails)(a), - "Email/changes": () => ({ accountId: ACCOUNT, oldState: "1", newState: String(state.n), hasMoreChanges: false, created: [], updated: [], destroyed: [] }), + /* + * Real changes, not an empty answer. + * + * This used to return three empty arrays whatever had happened, so the + * client's whole reconciliation path -- `Email/changes`, then deciding what + * to do with what came back -- never ran against the mock. A bug living in + * that path could not be reproduced here at all, which is how one reached + * production and survived being "fixed" once (#100). The log below is what + * the real server can answer from. + */ + "Email/changes": (a) => { + const since = Number(a.sinceState ?? 0); + const relevant = emailChanges.filter((c) => c.state > since); + const pick = (k: "created" | "updated" | "destroyed") => [...new Set(relevant.flatMap((c) => c[k]))]; + return { accountId: ACCOUNT, oldState: String(a.sinceState ?? "1"), newState: String(state.n), hasMoreChanges: false, created: pick("created"), updated: pick("updated"), destroyed: pick("destroyed") }; + }, "Email/set": (a) => { const r = genericSet(emails, "e", (o) => { const bv = (o.bodyValues as Record) ?? {}; @@ -493,6 +508,19 @@ const handlers: Record = { o.blobId = putBlob(`Subject: ${o.subject}\r\n\r\n${bv.text?.value ?? ""}`, "message/rfc822"); })(a); recount(); + nextState(); + recordEmailChange({ + created: Object.values((r.created ?? {}) as Record).map((x) => x.id), + updated: Object.keys((a.update as Obj) ?? {}), + destroyed: (r.destroyed as string[] | undefined) ?? [], + }); + /* A real server pushes a state change after a set, and the client acts on + it -- `Email/changes` runs and the store reconciles what came back. The + mock stayed silent, so that whole path never ran here and a bug living + in it could not be reproduced: marking a message read went round the + server and back on the live instance, and did nothing at all on the mock + (#100). Announced now, the way Stalwart does. */ + broadcast(["Email", "Mailbox", "Thread"]); return r; }, "Email/import": (a) => { const created: Obj = {}; for (const [cid, spec] of Object.entries((a.emails as Obj) ?? {})) { const id = `e${counter++}`; emails.push({ id, blobId: (spec as Obj).blobId, threadId: `t${id}`, mailboxIds: (spec as Obj).mailboxIds, keywords: (spec as Obj).keywords ?? {}, size: 100, receivedAt: new Date().toISOString(), subject: "(imported message)", from: [{ name: null, email: "import@example" }], to: null, preview: "", hasAttachment: false, textBody: [], htmlBody: [], attachments: [], bodyValues: {} }); created[cid] = { id }; } recount(); return setResp({ created }); }, @@ -852,6 +880,14 @@ const session = () => ({ }); const sseClients = new Set(); +/** What changed and when, so `Email/changes` can answer honestly. */ +const emailChanges: Array<{ state: number; created: string[]; updated: string[]; destroyed: string[] }> = []; +function recordEmailChange(change: { created?: string[]; updated?: string[]; destroyed?: string[] }) { + emailChanges.push({ state: state.n, created: change.created ?? [], updated: change.updated ?? [], destroyed: change.destroyed ?? [] }); + // A window is plenty; the client refetches from scratch if it falls behind. + if (emailChanges.length > 200) emailChanges.splice(0, emailChanges.length - 200); +} + function broadcast(types: string[]) { const payload = `event: state\ndata: ${JSON.stringify({ "@type": "StateChange", changed: { [ACCOUNT]: Object.fromEntries(types.map((t) => [t, String(state.n)])) } })}\n\n`; for (const c of sseClients) c.write(payload);