host: Add shared indexing to some test modules - #5722
Merged
Conversation
Indexing dominates the host suite. setupLocalIndexing wipes the index before every test, so each test rebuilds its fixtures from scratch: render, serialize and write every card. A sweep of the 324 timed test files shows 99 of them average over 2s/test, together 86% of the suite's 167 minutes, and the ones read so far spend that time re-indexing fixtures identical to the previous test's. setupLocalIndexing takes a `reuseIndexAcrossTests` snapshot name. The first test indexes as before and then captures the index with SQLiteAdapter#exportSnapshot; each later test restores it with importSnapshot and builds its realm with `skipBootIndex`, which mounts and serves without indexing. Isolation is kept rather than traded away: importSnapshot replaces every table, so each test starts from an identical pristine index instead of inheriting the previous test's state, and the snapshot is captured before the first test's body runs so nothing a test writes can reach the others. Each test still gets its own app, owner and loader. searchable-search-doc opts in: 43 tests, all passing, 421s -> 68.7s in a browser against a real realm server. Per test after the first: 9751ms -> 1377ms. The option is opt-in per module, and a module that builds more than one realm per test can't use it yet — the snapshot is captured once the first realm finishes indexing, so a second realm's rows would be missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per-test fixture indexing is the largest single line in the host suite: 99 of 324 files average over 2s per test and account for 86% of its 167 minutes. `searchable search doc` proved the shape of the fix at 6.1x (421s -> 68.7s). These are the next six modules by cost that meet the harness's constraints — one realm per test, and the same fixtures for every test in the module. `ai-assistant` (8.40 min, 48 tests, 10.5s/test) opts in directly. The five that share `operator-mode/setup.gts` — operator-mode-ui (7.45), operator-mode-card-chooser (6.38), operator-mode-links (5.63), operator-mode (5.09), create-listing-modal (1.57) — could not opt in at all before this, since the option is an argument to setupLocalIndexing and the helper owns that call. It now forwards one. Each module passes its own snapshot name: the fixtures are identical across the five, but a snapshot alias can only be exported once, and each module captures and restores its own. Left alone: modules building more than one realm per test (the interact-submode family builds four), which need the snapshot to be captured after the first test's last realm rather than its first, and modules building a realm per test with different fixtures each time (serialization, realm-indexing), which have no shared index to reuse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 2h 22m 33s ⏱️ + 1m 6s Results for commit e8320af. ± Comparison against earlier commit 6603b70. Realm Server Test Results 1 files ±0 1 suites ±0 15m 15s ⏱️ +40s Results for commit e8320af. ± Comparison against earlier commit 6603b70. |
backspace
changed the title
backspace
marked this pull request as ready for review
August 7, 2026 22:56
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b6633204b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
store-test.gts is the largest remaining file once the other seven modules are migrated: 7.31m over 73 tests, each rebuilding and reindexing the same fixtures. It meets both constraints — one realm, built in a shared beforeEach ahead of the first test, with fixtures identical for every test. Migrating it lowers the suite's floor as well as its total, since the floor on the slowest shard is the largest single file and this is it. Splitting a file lowers the floor without removing work; this removes the work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review catch: the after hook dropped the `reusableIndex` reference but left the snapshot itself allocated. `exportSnapshot` opens a separate in-memory database and leaves it ATTACHed — `importSnapshot` reads from it — and only `deleteSnapshot` detaches and closes it. The adapter is a page singleton (`globalThis.__sqliteAdapter`) whose `close()` never runs mid-shard, so each opted-in module's indexed fixtures stayed resident for the rest of the shard, growing peak memory module by module. It also held an ATTACH slot, and SQLite caps how many databases one connection can attach — a ceiling adoption would eventually reach, silently until it didn't. So delete the snapshot when the module that captured it ends, which is what the existing `withCachedRealmSetup` teardown already does for its own prefixed snapshots. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI: shard 4, both branches, the same two of store-test's 73 tests — "saving a module from the app rebuilds and reports it" and "a flush record does not survive a session boundary". Both fail on a precondition about the loader, not about index contents: the write didn't flush the module out of the loader, and the flush didn't record it. That is `skipBootIndex` showing through. A test whose index was restored runs against a realm that never booted its index, so everything the boot index does *besides* filling those tables hasn't happened — and these two tests assert on exactly that bookkeeping. The other 71 don't notice. So the eligibility rule I applied was incomplete. One realm and identical fixtures are necessary but not sufficient: the module also has to not care how its index came to exist. Noted on the option, since it isn't visible from a module's setup the way the other two constraints are. store-test remains the suite's largest file and still worth migrating — but by moving its two indexing-dependent tests into their own module first, which is a separate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
backspace
added a commit
that referenced
this pull request
Aug 10, 2026
Review catch (#5722): the multi-realm limitation failed silently, in the worst shape. A module opting in while building two realms per test captured the snapshot after the first realm only. Its first test still passed, because the live database held both. Every later test restored that snapshot, and both realms then took `skipBootIndex`, so the second was mounted and served with nothing indexed for it — a search returning nothing, an "is absent" assertion passing, a list rendering short. Nothing looks broken, and the module doesn't look newly broken either, since test one is fine. The condition is locally detectable without a realm counter: a realm being built while the snapshot is already captured and this test is not the one that restored it is exactly "a realm the snapshot cannot contain". So `captured && !restored` at realm construction, which covers both ways in: auto mode meeting a second realm, and manual mode where captureReusableIndex() ran before the last realm. The message says which, and what to do. It throws on the first test rather than the second, so the failure arrives where the cause is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
backspace
added a commit
that referenced
this pull request
Aug 10, 2026
Review verification (#5722) showed the original comment asserted the conclusion and skipped the dependency. The two operations take their table lists from different places: reset() from `pragma_table_list` on `main`, importSnapshot from the *snapshot's* `sqlite_schema`, DELETEing and refilling only what it finds there. So a table in `main` but absent from the snapshot would never be cleared, and that leak reads as one test seeing another's rows. It holds today because the snapshot is taken from `main` with the schema unchanged in between. The one systematic gap is the `sqlite_%` exclusion, which is free only while no column is AUTOINCREMENT — nothing in config/schema is, so `sqlite_sequence` doesn't exist. A migration adding one would leave restored tests inheriting the previous test's id counter, failing from the second test onward. Comment only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
richardhjtan
approved these changes
Aug 11, 2026
backspace
added a commit
that referenced
this pull request
Aug 11, 2026
Review catch (#5722): the multi-realm limitation failed silently, in the worst shape. A module opting in while building two realms per test captured the snapshot after the first realm only. Its first test still passed, because the live database held both. Every later test restored that snapshot, and both realms then took `skipBootIndex`, so the second was mounted and served with nothing indexed for it — a search returning nothing, an "is absent" assertion passing, a list rendering short. Nothing looks broken, and the module doesn't look newly broken either, since test one is fine. The condition is locally detectable without a realm counter: a realm being built while the snapshot is already captured and this test is not the one that restored it is exactly "a realm the snapshot cannot contain". So `captured && !restored` at realm construction, which covers both ways in: auto mode meeting a second realm, and manual mode where captureReusableIndex() ran before the last realm. The message says which, and what to do. It throws on the first test rather than the second, so the failure arrives where the cause is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
backspace
added a commit
that referenced
this pull request
Aug 11, 2026
Review verification (#5722) showed the original comment asserted the conclusion and skipped the dependency. The two operations take their table lists from different places: reset() from `pragma_table_list` on `main`, importSnapshot from the *snapshot's* `sqlite_schema`, DELETEing and refilling only what it finds there. So a table in `main` but absent from the snapshot would never be cleared, and that leak reads as one test seeing another's rows. It holds today because the snapshot is taken from `main` with the schema unchanged in between. The one systematic gap is the `sqlite_%` exclusion, which is free only while no column is AUTOINCREMENT — nothing in config/schema is, so `sqlite_sequence` doesn't exist. A migration adding one would leave restored tests inheriting the previous test's id counter, failing from the second test onward. Comment only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
This adds a
setupLocalIndexingtest helper that test modules can use to reuse an index across tests. Claude’s estimate is that this makes these modules 66% to 86% faster.