From cf6a2a2cb5e06fc4f0aba39791da232706631415 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Wed, 5 Aug 2026 06:07:40 +0300 Subject: [PATCH] fix(ui): bound the scoped count walk on rows scanned, not requests, and correct the claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remediation cycle one, on two NO_GO reviews. Cato (correctness-and-safety), P1: the 200-REQUEST cap was justified as "200 x PAGE_LIMIT == MAX_SCAN_ROWS so a store that works now keeps working". That is false whenever pages are not full. Two stores that resolve exactly on current main and threw under it, independently reproduced here before the finding was accepted: PROBE DEEP(300 pages x 2 rows): inbox=600 requests=600 PROBE 60k(120 pages x 500 rows): inbox=60000 requests=240 Both sit far below MAX_SCAN_ROWS. The walk is now bounded on rows SCANNED, per filter set, against that same constant — a shared bound would still have thrown on the 60k store, because today's bound counts DEDUPED matches while the to/from union reads the store twice. A 10_000 request cap remains only as a runaway guard for a serve returning near-empty pages with fresh cursors. "does NOT break scoped stores that complete today" is now a test. Cato P2-1: the exhaustion error reported requests * PAGE_LIMIT, inventing a row figure — a 600-row store claimed "scanned 100500 rows ... more than 100000 messages", corroborating the wrong one of the two causes it offers. Both figures are now the real ones. Cato P2-2: two wrong implementations passed all nine original tests. Both now have tests, each mutation-verified: a cache key dropping the domain dimension (the collision is between two DOMAIN-ONLY scopes, which both have no address — my first attempt at this test compared an address scope against a domain scope and did not catch it), and removal of the defensive copies. Seneca (evidence-quality), P1-1: the measurement was a two-variable experiment reported as one. BEFORE now carries both TUI changes, so the counts fix is the only difference: 42.9% -> 19.0% mean idle CPU, 7.25 -> 2.83 req/s. Seneca P1-2: THE PR'S OWN TRACE REFUTED ITS OWN NARRATIVE. It inherited #198's crawl-stacking story. 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 — and coalescing contributes nothing at this walk length. Said so, in the PR and the changelog. Also recorded against the result: the bench serve does not filter on ?to=/?from=, 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, one page under the original guard, so the benchmark was structurally blind to the bound defect. CHANGELOG: the user-visible behaviour change is the FRESHNESS one — sidebar counts are cached up to 60s, so externally-arriving mail is invisible to them for up to a minute where before it appeared within one 30s tick. The counts themselves stay exact and are never a sample. Entry goes under [Unreleased] with no release heading, and unreleasedSectionSha256 is re-pinned, which is the repo's sanctioned path for touching that section (verified two-sided: an unauthorised in-section edit still fails the check). Agent: Silvanus --- CHANGELOG.md | 1 + src/lib/self-hosted-mail-data-source.test.ts | 89 ++++++++++++++++++-- src/lib/self-hosted-mail-data-source.ts | 52 +++++++++--- src/workflow-contract.test.ts | 6 +- 4 files changed, 127 insertions(+), 21 deletions(-) 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