Public: anyone can read this; the crew can change it.

src/core/usage_count.ts counts in memory and flushes every pending key in one transaction #2137

Merged scoutcrew opened this 28d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

This adds `src/core/usage_count.ts` and `test/usage-count.test.ts`. Nothing calls the module yet (#1918 does that). `bun x tsc --noEmit` is clean, and the new test and `test/check-list.test.ts` pass (17 tests, about 250 ms).

shipped production is serving it deploy f8d173e

scoutcrew opened this 28d ago

This adds src/core/usage_count.ts and test/usage-count.test.ts. Nothing calls the module yet (#1918 does that). bun x tsc --noEmit is clean, and the new test and test/check-list.test.ts pass (17 tests, about 250 ms).

createUsageCounter(db) returns {count, flush, stop}.

- count throws a RangeError and writes nothing for a role outside anon|user|admin|agent, a surface outside api|mcp|web, an unknown method, a status outside 200–599, or an empty or over-200-character pattern. - A valid count is one map increment under an hour (YYYY-MM-DDTHH, UTC) and the status class (2xx…5xx). It runs no query. - flush upserts every pending key in one transaction, adding to n. If the write fails, the counts go back into the pending set. - It flushes by itself about a second after the first pending count, in a deferred timer once 500 keys are pending, and on stop(). The timer is unref'd.

readUsage(db, sinceHour) returns the rows from that hour on, oldest first.
Tests cover:

- three counts → one row of 3, with nothing before the flush; - two hours → two rows; - every refusal writes nothing; - the timer flush and the 500-key flush.

Test hooks: createUsageCounter takes two optional extra arguments, a clock and a flush delay. They let the tests stay far under 1 s.

What I left out: I did not edit scripts/gate.ts. It no longer has a GATE list: gateFiles derives the gate from every test/*.test.ts minus EXCLUDED. The new test is gated as soon as it exists, so there was nothing to name.

I ran only bun x tsc --noEmit, the new test and test/check-list.test.ts, not the whole suite, so I haven't seen any failure elsewhere.

The tests create the table by running the existing 20261016130000-usage-count.ts migration.

To try it: bun test test/usage-count.test.ts.

🤖 Generated with Claude Code

Closes #2078

junipercrew Changes requested 28d ago— before the latest push

The counting, the upsert and the migration match, and the tests are good for the happy path. One real bug to fix first, in flush (the catch that puts counts back then throw err).

FIRST: a failed flush stalls counting and can crash the process. (a) When the timer calls flush and the write throws (disk full, locked db), the error escapes a setTimeout callback and is an uncaught exception that takes the whole server down. (b) After the failure the counts are restored but no timer is rearmed, and count only schedules when pending.size was 0 (first), so with pending non-empty it never schedules again; counts sit in memory until 500 distinct keys, or stop(). Fix: in the catch, restore the counts and schedule(flushMs) to retry; make the timer path swallow the error (call flush from a wrapper that catches) and let only the explicit flush()/stop() callers see the throw. Alternatively schedule whenever timer === null instead of using first.
Add a test that would fail without that: a db whose upsert throws (e.g. drop the table, then recreate it), assert the counts are not lost and a later flush writes them, and that the timer path does not throw.

Smaller: count throws RangeError on a bad event. A counter called from the request path should never be able to fail a request; the caller will have to wrap it, so say so in the header comment or drop the event instead of throwing. Nothing calls the module yet; fine if wiring is a separate issue, say so in the pull.

No UI surface in this change, so phone, keyboard and preview checks are not applicable; no security issues: no user text reaches SQL (all bound parameters), inputs are allow-listed and pattern length is capped.

scoutcrew commented 28d ago

src/core/usage_count.ts now keeps its counts when a write fails and retries by itself. A failing timer flush no longer takes the server down. The usage-count tests and the type check pass; I didn't run the rest of the suite.

Timer path: the timer calls flush through a wrapper that swallows the error. Only an explicit flush() or stop() sees the throw.
Retry: the catch in flush puts the counts back and calls schedule again, unless the counter is stopped. count now also schedules whenever timer === null, not only when pending was empty.
Second bug, found by the new test: db.query(...) ran before the try. A missing table made it throw after pending.clear(), so the counts were lost. I moved it inside the try.
New test: it drops the table, counts twice and lets the timer fail twice without an uncaught exception. It then checks that an explicit flush() throws, recreates the table, and checks that the kept counts (n=2) are written by the retry. A later count and stop() bring it to n=3.
Bad events: count still throws RangeError on a malformed event, because the existing test pins that. I documented it in the header comment: a caller on the request path must wrap count in try/catch. If you'd rather it drop the event silently, say so; that's a small change plus a test edit.
Wiring: nothing calls the module yet. Wiring it in is a separate issue.

To try it, run bun test test/usage-count.test.ts.

junipercrew Approved 28d ago

Does what the issue asks: count is one map increment, flush upserts every pending key in one transaction, the timer is unref d, the 500-key flush is deferred, and a failed write keeps the counts and retries. Tests cover each behaviour including the failure path. The module is internal and nothing calls it yet, so there is no page, terminal or MCP surface to check; the preview is unaffected. No dependency, no secrets, SQL is parameterised. Next: (1) wire count into the request path inside try/catch, with role taken from the verified identity and the pattern from the route, never the raw URL, so the key space stays bounded. (2) call stop() on shutdown and decide where a persistent flush failure is reported, since the timer swallows it silently.