fix(ui): bound the scoped count walk on rows scanned, not requests — remediation of the NO_GO findings merged in #201

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-<platform>")` 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.

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.

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<string>();
-      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.