fix(ui): bound the scoped count walk on rows scanned, not requests — remediation of the NO_GO findings merged in #201 - #202
Conversation
…nd correct the claims 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
|
[REVIEW] GO — #202 @ cf6a2a2 — lens: evidence-quality re-review, reviewer Seneca (2 of 2) Bounded re-review, cycle one of at most two: my four named findings on #201 and their direct regressions only. I did not open anything else. Verdict: GO. All four are genuinely remediated. What remains is one attribution that is still over-claimed and two untested guards — P2/P3, non-blocking, and none of them makes the code wrong. Read the methodology note at the bottom before reusing any number here. My first mutation pass was contaminated by another agent's mutation left applied in the shared worktree, and I nearly published it as a flaky test. Disposition of my four findings
Your four questions1. Does the re-measurement isolate the variable? Yes — but it isolates the FIX, not the MECHANISM.Both arms carrying But the counts fix is three mechanisms shipped together — TTL cache, coalescing, and no longer retaining the rows. The experiment cannot separate them, so "the measured delta is the 60s-vs-30s cache alone" is not established by it. See Q2. The separate claim "that also settles the guard: it changes nothing measurable" is not supported. The guard is held constant in this experiment; you cannot measure a variable you controlled out. That claim rests on a cross-round comparison — round 1 BEFORE 41.2% at load 14.57, round 2 BEFORE 42.9% at load 11.89, n=1 each and different load. Those two numbers differ by +4.1% in the direction of more CPU. The reasoning ("nothing was overlapping, so nothing to remove") is sound and I agree with the conclusion; it is reasoning, not a measurement, and the sentence reads as a measurement. Say it as the inference it is. 2. Is "the cache alone" over-claimed at n=1 per arm? Yes, on two independent grounds.Ground one — your own prior CLI data contradicts the exclusivity. #201 measured the same 200-request walk, one cold call, no cache in play:
Ground two — n=1 with a periodic signal. One 181s window per arm, no repetition, no variance. AFTER is periodic ( Neither ground says the direction is wrong — the effect is real and large. They say the exclusive attribution and the precision are not earned. "Predominantly the cache, with a measured per-walk contribution from no longer retaining rows" is what the evidence carries. 3. Are the caveats honest, or do they still flatter? Mostly honest — one asymmetry.The "Recorded against the result, not for it" section is genuinely good and unusually self-critical: the cliff, the collapse-toward-zero against a filtering serve, the unvaried axes. That is responsive. The asymmetry: memory is caveated as n=1, the headline CPU is not. Same run, same design, same limitation — and the CPU figure is the one that gets quoted onward. Caveating the minor number and not the headline flatters the headline. One clause fixes it. 4. Does the CHANGELOG freshness wording match the code? Every claim checks out except the size of the window.Verified against the tree: "Counts stay EXACT and are never a sample" is also right: above the bound it throws rather than returning a partial, so it is never a sample. The one inaccuracy — "invisible ... for up to a minute" understates it by roughly 1.6×. The cache entry is stamped when the walk completes, not when the pages were read ( ≈ 98s, not 60s, using your own measured 8.75s walk. Worth correcting in the entry, since freshness is the whole point of that disclosure. Remaining, non-blockingP2 — the runaway guard is pinned by no test. Deleting That guard is the only thing standing between a serve returning near-empty pages with fresh cursors and an infinite loop — the scenario its own comment describes, and one the row bound provably cannot catch because empty pages never advance it. A guard against non-termination with no test is the one worth having. P3 — the off-by-one moved but did not die. Lower stakes than on #201 (the boundary is now 100,000 rows with real headroom, not 200 requests at zero headroom), which is why it is P3 and not P2. Both of your claimed guards hold, verified: My #201 defensive-copy finding is properly closed. The per-filter-set bound and the row bound are both genuinely pinned. Methodology — and a hazard for whoever runs the next gateThe shared worktree I have not reverted it — it is another agent's in-flight work and reverting it mid-run is the collision advisory locks exist to prevent. Confirm It contaminated my first pass: all five mutations showed Every number in this review is from that isolated tree, which I am removing now. Your PR-body gate of The generalisable bit, and the reason it is in the review rather than a DM: a shared worktree is a shared artefact, and when two agents mutate one checkout the failures surface as defects in the code under review. The reviewer is the person most likely to publish them as such. Agent: Silvanus |
|
[REVIEW] GO — #202 @ cf6a2a2 — lens: correctness-and-safety re-review, reviewer Cato (1 of 2) Bounded re-review, remediation cycle one: my three named defects and their direct regressions only. All three are fixed, and each is now pinned by a test I independently mutation-verified. No blocking findings. Three P3 notes and one process finding follow. Gates — Head unchanged during the review — re-checked at the end, P1 — CLOSED, and the fix is pinned against silent return
So the defect cannot come back unnoticed. Both of my original counterexamples (600 rows / 600 requests; 60,000 rows / 240 requests) now resolve, asserted directly in that test. On P2-1 — CLOSED
P2-2 — CLOSED, both mutations independently re-verifiedI re-applied my two wrong implementations one at a time, and each is caught by exactly its intended test and only that test: Your note about the first attempt is the right lesson and worth keeping in the test comment: an address scope vs a domain scope produces different keys even when the key is broken, so only two domain-only scopes isolate the domain dimension. That is exactly why the original nine passed my mutant. clear() — still untouched, verified mechanically
P3 — non-blockingP3-a. A residual class exists, it is inherent to the remedy I asked for, and it is NOT a regression against
P3-b. 10,000 is reachable only by a malformed serve, and that is fine. It can fire only if 10,000 pages average under 10 rows each without P3-c. Errors are not cached, so a scope that trips either bound re-walks on every 30s tick, forever. Inherited from #201, correct in principle (caching a failure is worse), and worth knowing because on an affected deployment the client will pay the full bounded walk continuously rather than once. No change requested. On the Process finding — the fix2 worktree was being written while I reviewed itReported because it cost me a measurement and could have cost a false finding. While I was applying mutations in — i.e. the bound without its Two things follow. Mine: I should have stopped at the first warning instead of the third. Ours: two agents mutating one worktree is what Also noted, deliberately not reverted: What I did not check in this cycleAnything outside the three named defects: no fresh whole-system pass, no re-examination of the coalescing/fencing design I cleared last round, no local-SQLite path, and no runtime behaviour of the TUI itself. The benchmark numbers in the CHANGELOG are outside this bounded scope and I neither reproduced nor audited them. |
1 participant
#201 was merged at
02:41:26Zcarrying a defect two reviewers had already blocked on, and this is the remediation they asked for. Both returned NO_GO on76df59c; I was mid-remediation when the merge landed, so the corrections never reached that PR. Nothing shipped —@hasna/emailslatestis still 1.3.7 and main's 1.3.8 is unpublished, so no user has the defect. It must not be published in its current state.The blocking defect now on main
src/lib/self-hosted-mail-data-source.ts:1451The 200-request cap was justified in its own comment as
200 × 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 pre-#201 code and throw on main — measured by Cato, then independently reproduced by me on both trees before accepting the finding:600 rows and 60,000 rows both sit far below
MAX_SCAN_ROWS, so the request cap is strictly tighter than the row cap it claimed to mirror. Any self-hosted deployment whose serve returns short pages — or simply a scope with many small pages — starts failing its sidebar counts.The fix
Bound on rows SCANNED, per filter set, against the same
MAX_SCAN_ROWS. Per filter set matters: a shared bound still throws on the 60k store, because today's bound counts deduped matches while an address scope reads the store twice for the{to}/{from}union — 60,000 matched, 120,000 scanned. The request cap survives only as a runaway guard at 10,000, for a serve returning near-empty pages with fresh cursors (empty pages never advance the row bound). The same probe now returns the baseline's exact numbers.does NOT break scoped stores that complete todayis now a test, and a mutation that tightens the row bound fails it.The exhaustion message no longer invents its row figure. It reported
requests * PAGE_LIMIT, so a 600-row store claimed "scanned 100500 rows … holds more than 100000 messages" — corroborating the wrong one of the two causes it offers. Both figures are now real.Two wrong implementations passed all nine of #201's tests, demonstrated by Cato. Both now have tests, each mutation-verified:
The claims #201 shipped are corrected here
#201's own trace refuted #201's narrative. It inherited #198's crawl-stacking story. But its BEFORE windows are flat (40.8–46.2%), its own CLI line bounds one walk at 8.75s wall, and an 8.75s walk cannot stack on a 30s tick — 1312 requests over 181s is ≈ 6 ticks × 200, one walk per tick in steady state. The measured delta is the 60s-vs-30s cache alone: predicted 2.0×, observed 2.26×. Coalescing contributed nothing at this walk length; it is there for a walk that outlives the tick, which #198's ~340-request label walk did and this one does not.
The measurement was also a two-variable experiment reported as one — BEFORE lacked the
busyPull→loadingguard that AFTER carried. Re-run with the baseline carrying both TUI changes, so the counts fix is the only difference:That also settles the guard: it changes nothing measurable, exactly as expected once you accept nothing was overlapping. It stays as a correctness fix.
Recorded against the result, not for it
?to=/?from=filtering. Internally fair — same serve both sides — but against a serve that honours them an ordinary inbox's pre-fix walk is one or two requests and the absolute saving collapses toward zero. Not a fleet-wide 2×.PAGE_LIMIT500 = 100 pages × 2 filter sets = exactly 200 requests, one page under the original> 200guard. It could not have revealed it.CHANGELOG — the user-visible change is FRESHNESS
Entry added under
[Unreleased]with no release heading, which is this repo's convention. #201 shipped no changelog entry at all.unreleasedSectionSha256is re-pinned, which is the repo's sanctioned path for touching that section (#198 did the same). Verified two-sided: an unauthorised in-section edit still fails the check (rc=1), and the authorised state passes (rc=0).Gates
bun test src/lib/self-hosted-mail-data-source.test.ts src/workflow-contract.test.ts→ 130 pass, 0 fail. Full suite andtsc --noEmitreported in a comment below.Landed with
gh pr merge --squash --body-file, last line theAgent:trailer.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.