diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bd287fb..e7ab5c9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to `@hasna/emails` are documented here. ## [Unreleased] +- **fix(ui): the OTHER half of the idle spin — scoped folder counts crawled the mailbox on every 30s tick, and the sidebar's inbox picker threw while still committing its state.** The previous entry closed `listLabelSummaries` and said plainly that `mailboxCounts` -> `scanScopeRows` was still uncached and un-coalesced on the same tick; this closes it. That walk followed the cursor chain **twice** for an address (the `{to}`/`{from}` union) with no bound that could fire: its only limit, `seen.size > MAX_SCAN_ROWS`, counts rows MATCHED, which a serve ignoring `?to=`/`?from=` barely grows. It is now bounded on rows **scanned, per filter set** against that same `MAX_SCAN_ROWS`, TTL-cached (`SCOPED_COUNT_TTL_MS = 60_000`, above the 30s refresh) keyed PER SCOPE, and coalesced behind a generation fence. The budget deliberately does NOT live in `scanScopeRows`, which is also the destructive `clear()` preflight — a cap or TTL there would make `clear()` delete a partial or stale subset while reporting a plausible count, and that is a regression test. **A first attempt capped REQUESTS at 200 on the argument that `200 x PAGE_LIMIT == MAX_SCAN_ROWS`; adversarial review measured two stores that resolve today and threw under it** (300 pages x 2 rows = 600 rows / 600 requests; 120 pages x 500 rows = 60,000 rows / 240 requests), which is why the bound counts rows. Also fixed: `setAddress` committed `selectedAddressId` BEFORE `persistSetting`, which throws in self-hosted mode, so a user was pinned to one inbox by an action that visibly failed and the reload never ran; and the 30s refresh guarded on `busyPull`, a field never set true anywhere, now `loading`. **USER-VISIBLE BEHAVIOUR CHANGE — FRESHNESS: the sidebar folder counts are cached for up to 60s, so mail arriving from OUTSIDE this client is now invisible to them for up to a minute, where before it appeared within one 30s tick.** `invalidate()` covers only this client's own writes, so your own read/star/archive/delete still update the counts immediately. The counts themselves are still EXACT and are never a sample — unlike the label tally above, which is. Measured on the real pty path with a 50,000-message store, an idle `sleep` negative control at `delta_ticks=0`, and the ONLY difference between the two builds being this fix: mean idle CPU **42.9% -> 19.0%** over a single 181s window, requests **7.25/s -> 2.83/s**, and the shape goes from flat to periodic. **What that delta is NOT: it is not the crawl-stacking the previous entry describes.** The BEFORE windows are flat (40.8-46.2%), one walk measures 8.75s wall, and an 8.75s walk cannot stack on a 30s tick — 1312 requests over 181s is ~6 ticks x 200, one walk per tick in steady state. The measured delta is the 60s-vs-30s **cache** alone (predicted 2.0x, observed 2.26x); coalescing contributed nothing here and is there for a walk that outlives the tick. **Two limits on that number, both against it:** the bench serve does not implement `?to=`/`?from=` filtering, so against a serve that HONOURS them an ordinary inbox's walk is one or two requests and the absolute saving collapses toward zero; and the bench store sat at exactly 200 requests (50,000 / 500 x 2 filter sets), one page under the original `> 200` guard, so the benchmark was structurally blind to the bound defect review found. Real CLI, same path: maxrss 486,672 kB -> 236,952 kB with byte-identical counts. - **fix(ui): `emails ui` burned ~92% of a core while idle — a sidebar label list was crawling the entire mailbox, repeatedly, with the crawls stacking up.** Measured on station01 against 1.3.6 under a pty with zero interaction: 40.9% -> 66.0% -> 91.7% -> 92.3% of a core across four windows, VmRSS climbing 166 -> 248 MB, and **0 bytes/s written to the terminal** the whole time. The renderer was not the cause and was never running: instrumented mid-spin it reported `fps=0.0 raf/s=0.0 isRunning=false controlState=idle liveReq=0`. Per-thread CPU put 33.6% on the main JS thread and ~31% across seven `HeapHelper` GC threads — allocation churn, not drawing. The source was `SelfHostedMailDataSource.listLabelSummaries()`, which tallied label names by walking the WHOLE store over HTTP (`for await (const page of this.listPages(PAGE_LIMIT))`, no bound) — ~340 requests and ~145 MB of JSON per call against the production mailbox (~170k messages), to populate a sidebar list of at most 80 names; the caller's `limit: 80` bought nothing because it was applied after the scan. The TUI issues it from `scheduleSidebarMeta` on every 30s refresh, and that scheduler cancels a PENDING timer but never an IN-FLIGHT walk — so on a mailbox where one walk takes longer than 30s, each refresh started a new crawl on top of the last and they accumulated, which is why CPU climbed with process age and plateaued at one core rather than sitting at a fixed rate. The walk is now bounded (`MAX_LABEL_SCAN_REQUESTS = 10`, i.e. 5,000 rows — the same budget `src/cli/tui/data.remote.ts` already used as `SELF_HOSTED_MAIL_SCAN_CAP`), TTL-cached (`LABEL_TALLY_TTL_MS = 60_000`, deliberately above the 30s refresh or the cache buys nothing), and coalesced behind one shared in-flight promise so overlapping sidebar loads share a single walk — the property that actually removes the climb. `invalidate()` drops the tally, since labelling changes it. Among the `listPages` loops in that module measured against their own explicit constants, this was the only one with no request cap, no cache and no coalescing — the others carry `MAX_SCAN_ROWS`, `MAX_FILTER_WALK_REQUESTS` or `MAX_THREAD_CANDIDATE_ROWS`. **That is not a claim that the idle spin is now fully closed:** adversarial review found that `mailboxCounts` reaches `scanScopeRows` on the same 30s sidebar tick once any single inbox is selected, and that path is also uncached and un-coalesced with only a row cap (`MAX_SCAN_ROWS = 100_000`, up to ~200 requests per filter set, run twice for the to/from union). It is pre-existing, out of scope here, and tracked separately. **Accepted trade, stated plainly: label counts are now a SAMPLE over the most recent 5,000 messages, not a census** — on a larger store a count is a lower bound and a label used only in old mail can be absent. The self-hosted seam has no server-side label aggregate (the local seam answers this with one SQL `GROUP BY`), so exact counts are obtainable only by dragging the whole mailbox over HTTP. Measured after the fix on the same machine, same pty harness, same `/proc` method, against the same production hosted mailbox over the self-hosted seam (the only path this fix touches): 13.1% / 5.2% / 14.2% / 4.7% across the identical four windows, with no climb and flat RSS. **The defensible before/after pair is source-vs-source through one harness: 68.8% -> 4.4%.** The 92.3% figure was taken against the *installed 1.3.6 dist bundle* rather than this branch's source, so 92.3% -> 4.7% spans two build artefacts and is quoted here as an order-of-magnitude indication only, not as a controlled comparison. The controlled measurement of the mechanism itself is the request count against a 340-page store: **340 requests / 118.2 MB before, 10 requests / 3.4 MB after.** - **test(ui): seven hermetic regression tests pin the budget, the coalescing and the cache lifecycle.** THREE of them measure the pathology directly, and fail against the pre-fix implementation with its own numbers: a single `listLabelSummaries()` call issues **200** requests over a 200-page store, three concurrent calls issue **600**, and a repeat call issues **400** instead of reusing 10. A fourth **passes before and after by design** — it holds the normal path (a store inside the budget still returns exact counts with `search`/`limit` applied) so the budget cannot be satisfied by returning nothing; it is an anti-vacuity guard, not a regression test, and is described that way rather than counted among the failing three. Adversarial review then proved those four pinned the cache only as "a cache exists" — an implementation with an infinite TTL and no invalidation passed all of them, which would freeze the sidebar counts forever — so three more pin the cache's lifecycle: that it expires, that a write drops it, and that a write landing MID-WALK is fenced. Each was mutation-tested: the infinite-TTL/no-invalidation implementation fails two of them, and removing the generation fence fails the third. - **fix(ui): `emails ui` could not start in any real terminal — the packaged runtime loaded a foreign OpenTUI native library.** 1.3.4 exited immediately with `Failed to initialize OpenTUI render library: Symbol "createEventSink" not found in .../@opentui/core-linux-arm64/libopentui.so`. `@opentui/core` loads its prebuilt renderer with a bare `import("@opentui/core-")` from inside its own module, so the version-matched prebuilt in `@opentui/core/node_modules/` is what answers. `scripts/build-tui-runtime.ts` inlined core into `dist/cli/ui-runtime-bundle.js` while listing the eight platform packages as **external**, which moved that import to `dist/cli/` — it then resolved against the installed package's *parents*, never saw core's own prebuilt, and bound to whatever copy the install had hoisted (here `0.1.105`, an ABI predating the symbol core calls). The install was version-correct throughout; only the loaded `.so` was wrong. `@opentui/core` is now external — it is already a declared runtime dependency, and keeping it in `node_modules` keeps the JS and the library it `dlopen`s in one dependency tree. `web-tree-sitter` and `bun-ffi-structs` were dropped from the same list for the same reason: both are core's dependencies, neither is declared by `@hasna/emails`, so externalising them pointed at unowned copies too. Declaring the eight platform packages as our own `optionalDependencies` was rejected — it copies upstream's platform matrix into this manifest and rots on every core bump, while leaving the resolution anchor wrong. `patchBundledNativeAssetPath()` is gone with the bundling it patched around, and the runtime bundle drops from 3.4 MB to 2.1 MB. diff --git a/src/lib/self-hosted-mail-data-source.test.ts b/src/lib/self-hosted-mail-data-source.test.ts index 8dad9a73..5ae132e4 100644 --- a/src/lib/self-hosted-mail-data-source.test.ts +++ b/src/lib/self-hosted-mail-data-source.test.ts @@ -2901,21 +2901,47 @@ describe("SelfHostedMailDataSource — scoped mailboxCounts scan budget", () => // Comfortably inside the budget, so the walk completes and counts stay exact. const SHALLOW_PAGES = 40; - it("bounds the scoped walk instead of following the chain to its end", async () => { - const serve = scopedDeepServe(DEEP_PAGES); + it("bounds the scoped walk on rows scanned instead of following the chain to its end", async () => { + // 220 x 500 = 110_000 rows, so ONE filter set already exceeds MAX_SCAN_ROWS. + const serve = scopedDeepServe(220, { rowsPerPage: 500 }); const ds = new SelfHostedMailDataSource({ baseUrl: "https://emails.example/v1", apiKey: "test-key", fetchImpl: serve.fetchImpl, }); - // Pre-fix this resolves after 600 requests (300 pages x the to/from union). - // Fixed, it fails closed at the budget and says why, exactly as the sibling + // Pre-fix the only bound counted MATCHED rows via `seen.size`. Fixed, the + // walk fails closed on rows SCANNED and says why, as the sibling // filtered-list walk already does (filterWalkExhausted). await expect(ds.mailboxCounts({ source: source(SCOPED) })).rejects.toThrow(/scoped folder counts/i); - expect(serve.requests.length).toBeLessThanOrEqual(201); - expect(serve.requests.length).toBeLessThan(DEEP_PAGES); - }); + // It stops as soon as the row bound trips rather than draining the chain. + expect(serve.requests.length).toBeLessThanOrEqual(210); + // 110_000 synthetic rows: slow to build, so it needs more than the 5s default. + }, 60_000); + + it("does NOT break scoped stores that complete today with many small pages", async () => { + // THE REGRESSION GUARD FOR THE BOUND ITSELF. An earlier revision capped this + // walk at 200 REQUESTS; adversarial review measured two stores that resolve + // exactly on the pre-fix code and threw under that cap. Both sit far below + // MAX_SCAN_ROWS, which is why the bound counts rows and not requests. + const deep = scopedDeepServe(300, { rowsPerPage: 2 }); + const dsDeep = new SelfHostedMailDataSource({ + baseUrl: "https://emails.example/v1", + apiKey: "test-key", + fetchImpl: deep.fetchImpl, + }); + const deepCounts = await dsDeep.mailboxCounts({ source: source(SCOPED) }); + expect(deepCounts.inbox).toBe(600); + expect(deep.requests.length).toBeGreaterThan(200); + + const wide = scopedDeepServe(120, { rowsPerPage: 500 }); + const dsWide = new SelfHostedMailDataSource({ + baseUrl: "https://emails.example/v1", + apiKey: "test-key", + fetchImpl: wide.fetchImpl, + }); + expect((await dsWide.mailboxCounts({ source: source(SCOPED) })).inbox).toBe(60_000); + }, 60_000); it("coalesces concurrent scoped counts onto one walk", async () => { const serve = scopedDeepServe(SHALLOW_PAGES); @@ -2980,6 +3006,55 @@ describe("SelfHostedMailDataSource — scoped mailboxCounts scan budget", () => expect(other.inbox).toBe(0); }); + it("keeps two DIFFERENT domain scopes off each other's cached counts", async () => { + // The scope has two dimensions and a key built from the ADDRESS alone passes + // every other test here — because two domain-only scopes both have no + // address, so they collapse onto one key and the second is served the + // first's numbers. Adversarial review demonstrated exactly that, so the + // domain dimension needs a case where it is the ONLY thing that differs. + const serve = compactCursorServe(new Map([ + ["", { + messages: [ + v1("1", { to_addrs: ["a@alpha.test"] }), + v1("2", { to_addrs: ["b@beta.test"] }), + v1("3", { to_addrs: ["c@beta.test"] }), + v1("4", { to_addrs: ["d@beta.test"] }), + ], + nextCursor: null, + }], + ])); + const ds = new SelfHostedMailDataSource({ + baseUrl: "https://emails.example/v1", + apiKey: "test-key", + fetchImpl: serve.fetchImpl, + }); + + const alpha = await ds.mailboxCounts({ source: { domain: "alpha.test" } }); + const beta = await ds.mailboxCounts({ source: { domain: "beta.test" } }); + + expect(alpha.inbox).toBe(1); + expect(beta.inbox).toBe(3); + }); + + it("hands every caller its own counts object, so one caller cannot poison the cache", async () => { + // MailboxCounts is a plain mutable record and the cached entry outlives the + // call. Without a copy on the way in and out, a caller that adjusts what it + // was given silently rewrites what the next caller is served — which every + // other test here misses, because they never mutate the result. + const serve = scopedDeepServe(3); + const ds = new SelfHostedMailDataSource({ + baseUrl: "https://emails.example/v1", + apiKey: "test-key", + fetchImpl: serve.fetchImpl, + }); + + const first = await ds.mailboxCounts({ source: source(SCOPED) }); + expect(first.inbox).toBe(6); + first.inbox = 999_999; + + expect((await ds.mailboxCounts({ source: source(SCOPED) })).inbox).toBe(6); + }); + it("still returns exact scoped counts for a store inside the budget", async () => { // Anti-vacuity guard: the budget must not be satisfiable by counting nothing, // and scoping must still exclude mail addressed elsewhere. diff --git a/src/lib/self-hosted-mail-data-source.ts b/src/lib/self-hosted-mail-data-source.ts index b620c2de..5cdec0aa 100644 --- a/src/lib/self-hosted-mail-data-source.ts +++ b/src/lib/self-hosted-mail-data-source.ts @@ -273,20 +273,40 @@ const LABEL_TALLY_TTL_MS = 60_000; // Unlike the label tally, these counts are PER SCOPE — one inbox's numbers must // never be served for another — so the cache is keyed rather than store-wide. // -// One shared budget for the whole call across both filter sets, mirroring -// listFilteredMailboxPage. 200 x PAGE_LIMIT is 100_000 rows: the same worst case -// as MAX_SCAN_ROWS, which is what bounds this path today, so a store that works -// now keeps working — the change is that the bound now counts rows SCANNED -// rather than rows matched, which is what makes it reachable at all. -const MAX_SCOPED_COUNT_REQUESTS = 200; +// THE BOUND IS ON ROWS, NOT REQUESTS, and that distinction was measured rather +// than reasoned. An earlier revision capped this walk at 200 REQUESTS on the +// argument that 200 x PAGE_LIMIT is MAX_SCAN_ROWS "so a store that works now +// keeps working". That is FALSE whenever pages are not full: adversarial review +// ran two stores that resolve exactly on the current code and would have thrown +// under the request cap — +// +// 300 pages x 2 rows -> inbox=600 requests=600 +// 120 pages x 500 rows -> inbox=60000 requests=240 +// +// Both sit far below MAX_SCAN_ROWS, so a request cap is strictly tighter than +// the row cap it claimed to mirror. The walk is therefore bounded on rows +// actually SCANNED, against the same MAX_SCAN_ROWS constant that bounds this +// path today. The only change in kind is that today's bound counts rows MATCHED +// (`seen.size`), which a serve ignoring ?to=/?from= barely grows — that is what +// made the walk unbounded in practice. +// +// The request cap below is a runaway guard only. It cannot fire before the row +// bound unless a serve averages under ten rows per page, and it exists so that a +// serve handing back near-empty pages with fresh cursors terminates instead of +// spinning forever (empty pages never advance the row bound). +const MAX_SCOPED_COUNT_REQUESTS = 10_000; // Must exceed the TUI's 30s sidebar refresh, for the same reason as the tally. const SCOPED_COUNT_TTL_MS = 60_000; -function scopedCountWalkExhausted(scannedRows: number): Error { +// Both figures are the REAL ones. An earlier revision reported +// `requests * PAGE_LIMIT`, which invented a row count — a 600-row store claimed +// "scanned 100500 rows … holds more than 100000 messages" and so corroborated +// the wrong one of the two causes it offers. +function scopedCountWalkExhausted(scannedRows: number, requests: number): Error { return new Error( `self-hosted emails: scoped folder counts scanned ${scannedRows} rows over ` - + `${MAX_SCOPED_COUNT_REQUESTS} requests without completing. Either this server ignored ` - + "the GET /v1/messages ?to=/?from= recipient filters, or this one address holds more " + + `${requests} requests without completing. Either this server ignored the ` + + "GET /v1/messages ?to=/?from= recipient filters, or this one address holds more " + `than ${MAX_SCAN_ROWS} messages — upgrade the emails-serve deployment, or scope the ` + "read to a domain instead of an address.", ); @@ -1444,11 +1464,21 @@ export class SelfHostedMailDataSource implements MailDataSource { const walk = (async () => { const counts = emptyCounts(); const seen = new Set(); - let requests = 0; for (const filters of scopeServerFilterSets(scope)) { + // PER FILTER SET, not across the union. An address scope reads the same + // store twice (?to= then ?from=) and today's bound counts DEDUPED + // matches, so a store of 60_000 rows read twice is 60_000 against that + // bound and would be 120_000 against a shared one — which would have + // thrown on a store that completes today. Each set gets the same + // MAX_SCAN_ROWS headroom the single-scan path already has. + let requests = 0; + let scannedRows = 0; for await (const page of this.listPages(PAGE_LIMIT, filters)) { requests += 1; - if (requests > MAX_SCOPED_COUNT_REQUESTS) throw scopedCountWalkExhausted(requests * PAGE_LIMIT); + scannedRows += page.length; + if (scannedRows > MAX_SCAN_ROWS || requests > MAX_SCOPED_COUNT_REQUESTS) { + throw scopedCountWalkExhausted(scannedRows, requests); + } for (const message of page) { // An address scope is a union of two server reads, so the same // message can arrive twice; count it once. diff --git a/src/workflow-contract.test.ts b/src/workflow-contract.test.ts index 96ecd89b..53971a4b 100644 --- a/src/workflow-contract.test.ts +++ b/src/workflow-contract.test.ts @@ -6,12 +6,12 @@ import { join } from "node:path"; const workflowDir = join(import.meta.dir, "..", ".github", "workflows"); const repositoryRoot = join(import.meta.dir, ".."); const packageProvenanceWorkflowSha256 = "706c636d7b60059f6e8ce52229bfb723c0c9a2c61cb4a462b3d6ead24a46232f"; -// Re-pinned when the [Unreleased] section gains the `emails ui` label-scan fix -// (task be9b3bb0). This constant is a tripwire, not a formality: it makes every +// Re-pinned when the [Unreleased] section gains the scoped-folder-count fix +// (task 90e98ccc). This constant is a tripwire, not a formality: it makes every // change to that section an explicit, reviewable edit, so an entry cannot drift // into a shipped release section unnoticed. Recompute with the same // markdownSection()/textSha256() pair this file already uses. -const unreleasedSectionSha256 = "5d81b6a062a47677aed260da62d98c37c00ad782db5bf162a7ad665e1afdc9f1"; +const unreleasedSectionSha256 = "8c74151a5bd7ffcf2307c3ea8ffd30a0ebebad42360831221df9df8160d664d3"; const release132Section = `## 1.3.2 (2026-07-26) - fail closed on malformed JSON, wrong response envelopes, and missing required