refactor(database): hand the sweeps a client instead of a singleton - #183
Conversation
Four modules under `src/main` reached SQLite by importing the `database`
value out of `src/database/index.ts`, which builds a client against
`process.cwd()/squeal.sqlite3` the moment the module is evaluated. Every
other service goes through `AppDatabase`, whose header says so in as many
words — "never through a direct `@/database` import".
What that cost was not hypothetical. The co-located test files could not
name the database they were testing against, so they mocked the module
instead, and the mock had to be registered before the module under test was
imported. That ordering is invisible in an import list, so it needed a
paragraph in `api-test-helper.ts` explaining it, a separate test
(`database-mock-import-order.test.ts`) making the mistake on purpose to keep
the paragraph honest, and a comment above the imports in each file.
`retention.test.ts` had to mock both sweeps outright, and said why in a
comment naming this issue.
The client is now an argument:
deleteExpiredQueries(client, retentionDays?)
deleteExpiredSpans(client, options?)
markInterruptedQueries(client)
writeSpans(client, records) // the default is gone
`writeSpans` already took a client; what it also took was the singleton as a
default, so a caller could write to the wrong database by saying nothing.
Both production callers were already passing one. Removing the default is
enforced by the compiler rather than by a test, and that is not a figure of
speech: with it gone, a call that names only its records no longer
type-checks, which is the whole of the guarantee. Putting the default back is
a type-valid edit that no test can reject, so nothing here pretends
otherwise.
It also swaps its two arguments, so that all four read the same way round.
That is churn on its own, and it is here because this is the commit that
already changes the signature — and because `tsc` names every call site, so
the swap costs one compile rather than a search.
`RetentionLive` binds the acquired service and passes `appDatabase.client`
into the sweeps, and `boot.ts` does the same for the reconciliation. The type
is spelled `typeof database` rather than the `AppDatabaseClient` alias, so
`src/main` gains no dependency on `src/server`, and the import is type-only
— nothing under `src/main` evaluates `src/database/index.ts` any more.
TDD. The tests the issue asked for are the ones that were impossible to
write: run a sweep against database A while B exists, and assert B is
untouched. Four such cases:
× sweeps the database it is given and no other (queries)
× sweeps the database it is given and no other (spans)
× marks the database it is given and no other (reconciliation)
× sweeps the client the app database built (the layer)
Worth being exact about what that red proves. For the three co-located
files it is a signature-change red: against the parent's production code the
new call binds a drizzle instance where a number was expected and every case
in the file dies on `no such table`. It shows the tests are wired to the new
shape, not that the old code swept the wrong database — the old code had no
second database to get wrong. Only the layer case is a behavioural red.
Nor do the B halves of those three carry much on their own. B is a second
in-memory database no production caller can reach, so "and B was left alone"
is a property of a database nothing has a handle to; what pins the sweeps is
the A half plus the mutants below. The layer case is the one that is new all
the way through, because there the client under test is the one the app
database actually built.
Two more cases cover the seams the first pass left open:
- `boot.ts` was the one changed call site no test reached. A boot that
reconciled the singleton compiled and passed the entire suite.
`boot.test.ts` seeds an unfinished query through the client the layer
built and asserts boot marked that one.
- `in-memory-database.ts` is what every test needing an app database now
depends on for its isolation, and nothing asserted anything about it. It
is now held down on both properties the callers lean on: every table the
schema declares, and a database of its own on every call.
`makeTestAppDatabase` is rewritten over `createInMemoryDatabase` rather than
repeating it, so there is one implementation of "empty app database" and one
set of tests for it.
The four rewritten files import `api-test-helper` not at all, so the
import-order comments repeating its paragraph go with them. That left
`spans-table.test.ts` as its only caller, and it never imported `@/database`
— it read the mock's database straight off `getTestDatabase()`, so the
`vi.mock` and the ordering rule it enforced were doing nothing for it. It
takes a database from `createInMemoryDatabase` now, which is the same thing
without the module mock, and `api-test-helper.ts` goes, along with
`api-test-helper.test.ts` and `database-mock-import-order.test.ts` — both of
which existed only to keep the deleted paragraph honest.
The two `vi.mock` calls in `retention.test.ts` stay. They are not there for
the singleton: that file is testing the fiber around the sweeps — the order,
the schedule, and what a failure does to both — so the sweeps are mocked to
something that resolves on command. The comment above them says that now,
instead of naming this issue.
Mutation: 9 defects died — each of the three sweeps restored to the
singleton (the pre-change code exactly), the schedule handing over the
singleton, the schedule giving the client to only one of its two sweeps,
boot reconciling the singleton, the writer writing to a database of its own,
and the test helper either skipping the schema or handing every caller the
same database. A tenth, omitting the client at a call site, is rejected by
`tsc`; `trace-store.test.ts` also fails on it at runtime, so vitest catches
that one too. 2 non-defects survived: renaming the parameter, and reading the
options before the client.
Closes #85
|
React Doctor found 1 new issue in 1 file · 1 warning · score 90 / 100 (Great) · 1 fixed · vs 1 warning
Reviewed by React Doctor for commit |
React Doctor reads the second await as an independent one that should be gathered into a `Promise.all`. It cannot be: the libsql driver is synchronous and SQLite serializes writes, so the two DELETEs queue whatever this code asks for. Measured over 40,000 spans the two forms are within noise, 22-26ms either way, and both leave the same rows behind. The rule it comes from is about request paths -- "your users wait twice as long" -- and this is the daily retention fiber, where nobody is waiting at all. Recording that at the site so the next reader does not have to re-measure it.
| `) | ||
|
|
||
| const overCap = await database.run(sql` | ||
| const overCap = await client.run(sql` |
There was a problem hiding this comment.
Not applying this one — I think it is a false positive here, and I measured it rather than assuming.
The two DELETEs cannot overlap. drizzle(databaseFilePath) builds a local libsql client, whose driver is synchronous and runs on the main process's event loop, and SQLite serializes writes on a connection regardless. So Promise.all describes a concurrency the database will not honour — the statements queue either way.
Measured on an in-memory app database with 40,000 spans, three runs each:
| run 1 | run 2 | run 3 | |
|---|---|---|---|
| sequential | 24ms | 22ms | 22ms |
Promise.all |
23ms | 22ms | 26ms |
Within noise, and both forms leave the same 2,000 rows behind and report the same 18,000 deleted.
The other half is context: the rule's premise is "your users wait twice as long", but nothing is waiting on this. deleteExpiredSpans has one caller — the daily retention fiber in src/server/retention.ts, which sweeps once at boot and then once every 24 hours. That file already documents the synchronous-driver constraint at L29 as the reason it carries no per-sweep timeout either.
I have added a comment at the call site (55b4c11) recording the measurement, so the next reader does not have to re-derive it.
Four modules under
src/main— the two retention sweeps, the queryreconciliation, and the span writer — reached SQLite by importing the
databasevalue out ofsrc/database/index.ts, which builds a client againstprocess.cwd()/squeal.sqlite3the moment the module is evaluated. Every otherservice goes through
AppDatabase, whose header says so in as many words.The client is an argument now:
Nothing under
src/mainevaluatessrc/database/index.tsany more — theremaining references are all
import type, and the one value import left inthe repository is
src/server/services/app-database.ts, which is the intendedowner.
What it buys
The tests the issue asked for are the ones the singleton made impossible: run
a sweep against database A while B exists, and assert B was left alone. Four
such cases, plus two covering the seams the first pass left open (
boot.tswas a changed call site no test reached;
in-memory-database.tsis what everytest now leans on for isolation and nothing asserted anything about it).
Deleted with them:
src/test/api-test-helper.ts, its self-test, anddatabase-mock-import-order.test.ts. Those existed to make the "register thevi.mockbefore importing the module under test" hazard survivable and to keepthe paragraph explaining it honest.
grep -rn "vi.mock('@/database"nowreturns nothing repo-wide, so the hazard is gone rather than documented.
The two
vi.mockcalls inretention.test.tsstay: that file tests the fiberaround the sweeps — order, schedule, and what a failure does to both — so it
needs sweeps that resolve on command. The comment above them says that now,
instead of naming this issue.
Verification
Full suite 1416 passed, 2 failed — both pre-existing on
main(
sqlite-adapter.test.ts > connects and executes SELECT 1, and theDatabaseForm.test.tsxpost-change-verdict flake). Both typecheck projectsexit 0; ESLint 0 errors.
Mutation: 9 defects killed, including each of the three sweeps restored to the
singleton (the pre-change code exactly), the schedule giving the client to only
one of its two sweeps, boot reconciling the singleton, the writer writing to a
database of its own, and the test helper either skipping the schema or handing
every caller the same database. A tenth — omitting the client at a call site —
is rejected by
tscand also failstrace-store.test.tsat runtime. Twonon-defects survived (renaming a parameter, reading the options before the
client).
Reviewed with fresh context, which re-ran all ten independently. One thing it
raised and I am carrying forward rather than fixing here: restoring the
writeSpansdefault is a type-valid, test-invisible edit, so ano-restricted-importsrule naming@/databaseoutsideapp-database.tswould make the invariant enforceable rather than conventional. Mutants 1-3 and
6 show the suite rejects every realistic regression today, so that is
belt-and-braces, not a gap.
Closes #85