fix(events): serve a bounded /v1/events read instead of refusing it - #81
Merged
Conversation
added 3 commits
July 26, 2026 18:56
`listFilteredEvents` enumerated the whole `events` table and refused an incomplete enumeration. The refusal is right for an export, but it also hit BOUNDED callers that never asked about the rest of the table: the dashboard events tab sends `?limit=100` and `export events` (MCP) normalizes to `limit=1000`, so both returned a 500 in self-hosted mode once `events` passed the pager's 20_000-row scan cap. `GET /v1/events` already accepts `limit`/`offset` (the generic resource list route clamps and windows them, and the OpenAPI document already declares both), so this is a client-side fix — no route or contract change. - `enumerateSelfHostedRows` takes an optional `need` (the window's far edge) and reports `filled`, kept strictly separate from `complete`: a bounded stop never claims to have seen the whole table. - A bounded read is answered when its window is full, or when the table ended first (then the short result IS the whole tail). It is still refused when it could neither fill the window nor reach the end. An unbounded read passes no `need` and still has to prove completeness — that refusal is unchanged. - The 500-row page clamp is handled by paging, not inherited: every request is capped at the server maximum and the pager pages to the window's far edge, so a window past row 500 comes back FULL rather than silently short. Request size scales by the observed drop ratio so a client-side filter cannot leave the window under-filled either. - `select` maps and filters in one pass, so a row the caller drops does not count toward the window. A single-request bound is also structurally immune to a shifting paging window, which is the second way a `limit=100` read used to fail.
Two defects in the previous commit, both found by re-auditing it. 1. The `/v1` stub returned generic lists in INSERTION order, never applying the resource's declared ORDER BY. A client that windows server-side has to trust the server to order first, so against that stub a correct client looked broken: `events.test.ts > paginates summaries` and `export.test.ts > paginates event exports and honors until filters` both failed, because they seed events oldest-first and expect a newest-first page. CI agreed — same two, no others. The stub now sorts each generic list by the order terms taken from the server's own registry (SELF_HOSTED_RESOURCES + resourceListOrderBy), injected as V1_STUB_LIST_ORDER, so it cannot drift from what production returns. Rotation for the non-total-ORDER-BY emulation now applies to the ordered copy with a cumulative per-resource counter, so that guard keeps working unchanged. Both tests pass again as written — nothing was weakened, and the whole suite is unaffected (2293 pass / 0 fail), i.e. nothing was relying on insertion order. 2. The budget raise for a wide `need` was a self-DoS: `offset` is unbounded at several entry points, so `ceil((offset + limit) / 500) + 1` could authorize millions of synchronous page fetches, and past the server's own 100_000 offset cap those pages cannot even return new rows. The budget is no longer raised. It already reaches 20_000 rows, past every limit a caller can ask for, so only deep-OFFSET windows fall outside it — and those are exactly the reads that should refuse. Deep paging belongs to cursors. Also: a bounded read's dependence on server ordering is now asserted rather than assumed (rows seeded against the declared order must still come back newest-first), and the comment claiming the local sort protects a bounded read against an unordered server is corrected — it cannot; the wrong rows would already be in hand.
…ntly
BLOCKER found by adversarial review of the previous two commits, reproduced and
confirmed a real regression.
De-duplication only ever caught the paging window moving BACKWARD: a row inserted
above the cursor comes back twice. A row DELETED above the cursor moves the window
FORWARD — every unread row slides to a lower offset, the next page starts one row
late, and the skipped row is seen by nobody. No duplicate is ever produced, so
`stable` stayed true and a bounded read returned a full-looking window.
Measured on 1200 events with the window shifting, `listEvents({ limit: 1000 })` --
the `export events` default, two pages -- returned exactly 1000 rows with
duplicates === 0 and no error, silently MISSING evt-000000..evt-000006 (the seven
most recent events) and evt-000507..evt-000513, padded with 14 rows from past the
end of the window. On bb70321 that same read REFUSED, so the bounded path was
strictly less honest than the code it replaced. `/v1/events/{id}` DELETE exists in
the generic registry, so this is reachable in production and not a stub artifact.
Fix: anchor every page after the first on the last row already read -- ask for it
again and require it back. That detects a shift in BOTH directions, which pure
offset paging cannot otherwise do. Once a shift is proven, anchoring stops and the
read continues exactly as an unanchored pager would, so `duplicates` keeps meaning
what it always meant. New `shifted` flag joins `duplicates` in `stable`.
The anchor row costs one row of each page's capacity (a request over the server's
500-row cap would be clamped, and a clamped page is indistinguishable from the end
of the table). Two budget-exhaustion tests therefore assert 298 rows instead of 300
for 3 pages of 100 -- one of them pre-existing. Its intent is unchanged (the budget
ran out, so the count is a lower bound, never a total) and its strength is unchanged
(still an exact count, still complete: false, still pages: 3); only the arithmetic
moved. The refusal-shortfall assertion is now matched by shape rather than a
hard-coded count so it tracks intent instead of page arithmetic.
Also from the review:
- Corrected a false attribution I had propagated from the task description: the
dashboard events tab is NOT a caller of this path. src/server/routes/core.ts
imports listEventSummaries from events.local.js and never reaches events.remote.
The real bounded callers all arrive through src/lib/export.ts (MCP export_events,
CLI `emails export events`, GET /api/export/events) plus library consumers of
src/index.ts, and all are bounded because normalizeEventFilters always supplies a
limit. Comments now say so, and warn against re-adding the dashboard.
- The test named "...single-request bounded read while the paging window shifts" was
a positive control wearing a safety control's name: a single request is immune, but
nothing restricted bounded reads to one request. It now says so, and the multi-page
case has its own test asserting the refusal.
- v1-stub sortForList used a two-way comparator that returned 1 for both cmp(a,b)
and cmp(b,a) when values differed by type but stringified equal; now three-way.
- Documented the limit/offset CLAMP (500 / 100000, never an error) in the OpenAPI
list parameters. It is the undocumented server behavior this client depends on.
Known and NOT fixed, no worse than bb70321: a bounded read whose matching rows are
a short prefix (`since`) still exhausts the budget scanning the rest of the table
rather than stopping once the DESC order proves no later row can match; and a
window whose far edge is past ~20000 refuses, because the budget is deliberately not
scaled by an uncapped offset. Both refuse honestly instead of answering wrongly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes todos task
03a1eff7— a knowingly-landed regression that 500s in production.The defect
src/db/events.remote.tslistFilteredEvents()enumerated the wholeeventstable and then refused an incomplete enumeration. That refusal is correct for an unbounded export and is preserved here. But it also fired for bounded callers, which never asked about the rest of the table.Every caller that reaches this path does so through
src/lib/export.ts, and all of them are bounded becausenormalizeEventFiltersalways supplies a limit (1000 by default, 10 000 max):export_eventsemails export eventsGET /api/export/eventssrc/index.tsSo in self-hosted mode all of them returned a 500 once
eventspassed the pager's scan cap (40 pages x 500 rows = 20 000). It works today only because the table is smaller than that.eventsis the fastest-growing table in the system (one row per delivery, open and click), so it gets there first.Did the route already accept a limit?
Yes — this is a purely client-side fix. No route change, no OpenAPI change, no contract-test change.
GET /v1/eventsis a generic registry resource (SELF_HOSTED_RESOURCES->events), andSelfHostedStore.listResourcealready appliesclampLimit(opts.limit)/clampOffset(opts.offset).listParamsspread into every generic resource'squeryParameters. Verified against the generated document:GET /v1/events->limit, offset, email_id, provider_id, type, recipient.The client simply was not forwarding the bound.
The fix
enumerateSelfHostedRows(src/db/self-hosted-page.ts) gains a bounded mode:need— the window's far edge (offset + limit). The read stops once the window is full.filled— reported separately fromcomplete, and never conflated with it.completestill means "this is the whole set" and is now set only when a short page was actually observed; a bounded stop can never leak into it.select— maps and filters each row in ONE pass. A dropped row is still consumed (offset and duplicate accounting) but does not count toward the window, so a filter the server does not apply cannot leave the window short of the rows the caller asked to keep. Request size scales by the observed drop ratio, so a selective client-side filter costs extra pages rather than a short window.events.remote.tsthen distinguishes the two questions:A bounded read is still refused when it could neither fill its window nor reach the end of the table, because a short page passed off as the requested window silently skips rows — and refused when the window moved while being filled, because a full window assembled out of a moving one is missing rows it will never mention.
The 500-row clamp interaction
Handled by construction, not inherited. The pre-existing mismatch is that
selfHostedListQuerysendslimit = Math.max(1000, limit + offset)while the server clamps to 500, so windows past row 500 under-fill.The events path no longer goes through that helper at all. Every request the pager makes is capped at
SELF_HOSTED_SERVER_PAGE_MAXand it pages to the window's far edge, so a window past row 500 comes back FULL. A test asserts both the identity of the returned rows (evt-000700..evt-000799) and that no request exceeded the cap.selfHostedListQueryitself is not repaired here: it is a shared helper with ~30 call sites across every resource repo, and the correct repair is to move those call sites onto this pager — a separate change with its own blast radius. Nothing in this PR silently inherits it.Second commit: two defects found by re-auditing the first
1. The stub did not order lists. A client that windows server-side must trust the server to order first. The
/v1stub returned generic lists in insertion order, so a correct client looked broken:events.test.ts > paginates summariesandexport.test.ts > paginates event exports and honors until filtersboth failed (they seed events oldest-first and expect a newest-first page). CI agreed — the same two, no others.The stub now sorts each generic list by order terms taken from the server's own registry (
SELF_HOSTED_RESOURCES+resourceListOrderBy), injected asV1_STUB_LIST_ORDER, so it cannot drift from production. The non-total-ORDER-BY rotation guard now applies to the ordered copy with a cumulative per-resource counter and keeps working unchanged.Both tests pass as written — nothing weakened — and the rest of the suite is unaffected, i.e. nothing was relying on insertion order. A new test seeds rows against the declared order and still requires the 100 most recent back, so the client's dependence on server ordering is asserted rather than assumed.
2. The budget raise was a self-DoS. The first commit raised the page budget to
ceil((offset + limit) / 500) + 1.offsetis unbounded at several entry points, so that could authorize millions of synchronous page fetches — and past the server's own 100 000 offset cap those pages cannot even return new rows. The raise is removed. The default budget already reaches 20 000 rows, past everylimita caller can ask for, so only deep-offset windows fall outside it, and those are exactly the reads that should refuse. Deep paging belongs to cursors.Third commit: the blocker — a bounded read could still skip rows silently
Adversarial review found, and I reproduced and confirmed, that commits 1-2 left a real regression.
De-duplication only ever caught the window moving backward (a row inserted above the cursor comes back twice). A row deleted above the cursor moves the window forward: every unread row slides to a lower offset, the next page starts one row late, and the skipped row is seen by nobody. No duplicate is ever produced, so
stablestayed true and a bounded read happily returned a full-looking window.Measured on 1200 events with the window shifting,
listEvents({ limit: 1000 })— theexport eventsdefault, two pages — returned exactly 1000 rows,duplicates === 0, no error, and:evt-000000..evt-000006— the seven most recent events — andevt-000507..evt-000513On
bb70321that same read refused. So the bounded path was strictly less honest than the code it replaced — exactly the class of silent lie this module exists to prevent.DELETE /v1/events/{id}exists in the generic registry, so this is production-reachable, not a stub artifact.Fix: anchor every page after the first on the last row already read — ask for it again and require it back. That detects a shift in both directions, which pure offset paging otherwise cannot. Once a shift is proven, anchoring stops and the read continues exactly as an unanchored pager would, so
duplicateskeeps meaning what it always meant. A newshiftedflag joinsduplicatesinstable.The anchor row costs one row of each page's capacity — a request over the 500-row cap would be clamped, and a clamped page is indistinguishable from the end of the table. Two budget-exhaustion tests therefore assert 298 rows instead of 300 for 3 pages of 100, and one of those is pre-existing. Please scrutinise that one: its intent is unchanged (the budget ran out, so the count is a lower bound and never a total) and its strength is unchanged (still an exact count, still
complete: false, stillpages: 3) — only the arithmetic moved. The refusal-shortfall assertion is now matched by shape rather than a hard-coded count, so it tracks intent instead of page arithmetic.Also from the review: the test named "…single-request bounded read while the paging window shifts" was a positive control wearing a safety control's name — a single request is immune, but nothing restricted bounded reads to one request. It now says so, and the multi-page case has its own test asserting the refusal. The stub's
sortForListcomparator was two-way (returned1for bothcmp(a,b)andcmp(b,a)when values differed by type but stringified equal); now three-way. And thelimit/offsetclamp (500 / 100 000, never an error) is now documented in the OpenAPI list parameters — it is the undocumented server behavior this client depends on.Known, not fixed, and no worse than
bb70321since) still exhausts the budget scanning the rest of the table, instead of stopping once theoccurred_at DESCorder proves no later row can match.offset.Both refuse honestly rather than answering wrongly, and both refused on
bb70321too.Proof: fails on unmodified
mainReverting only the two client source files (
events.remote.ts,self-hosted-page.ts) tobb70321and keeping the final test files and the stub fix: 15 pass / 14 fail. With the fix: 29 pass / 0 fail. The stub fix alone does not make them pass — the client change is load-bearing.The headline failure —
listEventSummaries({ limit: 100 })against a 20 500-row table:On
maina bounded request for 100 rows throws — the production 500. With the fix it returns exactly 100 rows, and they are the 100 most recent (evt-000000..evt-000099), while the unbounded read over the same table still refuses withLOWER BOUND.Also failing on
main, now passing: the one-request-per-bound assertion (3 pages of 500 onmain), the single-request read under a shifting paging window, the multi-page refusal under a forward shift, the forward-shift-with-zero-duplicates pager assertion, and the bounded refusal naming its shortfall counts.Guards that pass on
maintoo — they protect this change from regressing rather than proving the original defect: the 500-clamp window identity, the declared-order dependency, the client-side-filter window fill, and the too-small-table case.Gates
bunx tsc --noEmitcleanbun run buildcleanbun run no-cloud:source24 pass / 0 failbun run no-cloud:packclean (728 text files scanned, no hosted-control-plane markers)HOME, cloud/AWS credentials unset,EMAILS_MODE=local): 2296 pass / 0 fail / 124 skip, against abb70321baseline of 2278 pass / 1 fail / 124 skip (that one failure a 5s-timeout flake indatabase.test.ts, which passes in isolation and passes here). Net +17 tests, all new.LOWER BOUND/partial event listassertions are untouched. The one pre-existing expected value that changed (300 → 298) is called out above with its reason; its intent and strength are unchanged.Version deliberately not bumped and nothing published.