perf: stop reading every state's JSONL to list states (#219) - #346
Open
ProfSynapse wants to merge 4 commits into
Open
perf: stop reading every state's JSONL to list states (#219)#346ProfSynapse wants to merge 4 commits into
ProfSynapse wants to merge 4 commits into
Conversation
Listing states had to open the JSONL event store once per state, because the only copy of `state.metadata.isArchived` lived inside the snapshot content. Each of those reads parses the entire workspace stream, so the cost was quadratic: measured on a live vault, 200 states cost 200 reads and 180,200 parsed events (~500 ms) on the first list after a restart. This is the schema half of the fix — all four steps of `.claude/skills/nexus-storage/protocols/change-schema.md`: CURRENT_SCHEMA_VERSION, a new MIGRATIONS entry, the column and index in SCHEMA_SQL, and the version literal SCHEMA_SQL stamps into schema_version (without which a fresh install lands on 14 and replays v15 forever). The column is nullable WITH NO DEFAULT in both definitions. `DEFAULT 0` would tell every already-archived state that it is visible again, which is the archive-visibility regression #218 fixed. NULL means "unknown — ask the content", and the read path still honours it. Backfill is split by what each stage can actually reach: - `migrationFn` is synchronous and only sees the database, so it backfills the rows whose stateJson the event applier had already cached. Free, but partial: rows written live by StateRepository have stateJson NULL. - everything else is backfilled at the next init from JSONL, one read per workspace (next commit), never one per state. `description` is backfilled beside it from `context.activeTask`: once the archive flag stops forcing a content read, that fallback is the only thing keeping listStates from reporting "No description" for every state the createState tool ever wrote, since that tool supplies no description at all. src/database/utils/stateContent.ts owns the derivation so the migration, the repository and the event applier cannot drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
StateRepository now derives `isArchived` (and the description fallback) from the snapshot on save, re-derives the flag whenever a content update lands — archiving a state IS a content update, so this is the write that archives anything — and surfaces the column as `undefined` when it is NULL, keeping "unknown" distinguishable from "not archived". `getStates` gains an archive filter answered in SQL. Only an explicit `includeArchived: false` filters; omitting it returns everything, because restore, rename and the createState name-uniqueness check all call getStates with no options and must keep seeing archived states. Rows whose flag is unknown survive the filter either way — hiding a state that is plainly not archived is the failure direction that actually loses data from a list. `backfillDerivedStateMetadata()` fills in rows the v15 migrationFn could not reach (stateJson NULL). It reads each affected workspace stream ONCE and folds it for every state in it: the upgrade cost is O(workspaces), not O(states). Reintroducing a per-state read here would have made the upgrade a smaller copy of the bug. HybridStorageAdapter runs it after init; once every row is known it is a single indexed SELECT that returns nothing, and a failure is non-fatal because the read path still falls back to content. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
`rebuildCache()` deletes the database and rebuilds it from the event store, so anything this applier does not write does not exist afterwards — that is exactly how the notes index tables were destroyed. A denormalized column that only the live write path maintained would come back empty from a rebuild and silently un-archive every state. applyStateSaved now derives isArchived and the description fallback from the event's stateJson, using the same helper as StateRepository so the two paths cannot disagree. applyStateUpdated follows content updates for the flag (archiving is a content update) and keeps its stateJson copy current, so the v15 migrationFn never reads a stale snapshot out of it. An unparseable snapshot leaves the flag NULL rather than asserting "not archived". Verified against the live rig: after a real rebuildCache, the column matched the JSONL content for every row, including a state archived through a state_updated event. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
MemoryService.getStates called adapter.getState for every row purely to learn whether the state was archived. Now it reads the denormalized column, and opens the event store only for a row whose flag is `undefined` — meaning "not backfilled yet", not "not archived". Dropping that fallback is what made archived states reappear in #218, so the fallback is kept and pinned by a test in the regression's original shape (tags present, flag unknowable). The skeleton built from metadata alone now carries state.metadata.isArchived, since both archive filters in the codebase read the flag from there. When content IS fetched it still wins: the stream is the source of truth, the column is a cache of it. listStates and the workspace settings states section pass their includeArchived choice down so SQL can apply it, and keep their in-memory filters as the backstop for un-backfilled rows. Measured in a live vault (headless Obsidian 1.13.7), first list after a restart with the content cache cleared: states | before | after -------|--------------------|------------------- 25 | 31 ms, 25 reads | 0.9 ms, 0 reads 100 | 190 ms, 100 reads | 2.6 ms, 0 reads 200 | 500 ms, 200 reads | 6.1 ms, 0 reads Events parsed on the cold path went from 2,525 / 42,600 / 180,200 to zero, with the archived-state count unchanged at every size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
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.
Closes #219. Schema v14 → v15.
The measured problem
MemoryService.getStatescalledadapter.getStateper row, and each of those parses the entire workspace event stream — so cost was quadratic in events parsed, not linear in states. Warm calls were free via an unbounded content cache, so the whole thing landed on the firstlistStatesafter every restart, which is why it was easy to miss.Before / after
Live headless Obsidian,
readEventsinstrumented, content cache cleared, cold call:Re-measured after rebuilding the rig and cold-starting: 2.0 / 5.0 / 10.1 ms, still zero reads and zero events. Archived counts unchanged (5 / 20 / 40).
Design decisions
Backfill is eager but batched.
migrationFnis synchronous and DB-only, so it cannot read JSONL — it covers rows that have cachedstateJson, and the rest are filled at next init by one read per workspace, not per state. A per-state backfill would have re-paid the exact quadratic cost this PR removes; a purely lazy scheme would never converge, because states are immutable and nothing would trigger the fill.The column is nullable with no default.
DEFAULT 0would silently un-archive every existing state — that is regression #218, and it is why "unknown" and "not archived" have to be distinguishable.descriptionis denormalized too, fromcontext.activeTask.createStatesupplies none, so dropping the content read without this would have made every LLM-authored state list as "No description".A trap caught mid-build
The SQL filter initially changed the default for all callers, which broke
archiveState --restore, rename, and the create-name-uniqueness check — all of which need archived rows visible. Now only an explicitincludeArchived: falsefilters. Pinned by tests.Verification
Both schema paths proven independently, because they fail independently:
rebuildCache()builds fromSCHEMA_SQLalone and early-returns frommigrate()— column and index present, stamped 15, verified twice.state_updated, and the cold read did 0 JSONL reads. This is the path that silently destroyed the notes-index tables earlier today, so it is not a theoretical check.check_schema_consistency.pyexits 0 — all four steps, including theschema_versionstamp literal insideSCHEMA_SQL.npm run buildclean.npx jest tests/unit— 4330 passing. Regression tests fail pre-fix (32 failed / 23 passed; the 23 describe unchanged behaviour).Noted, not fixed
deleteWorkspaceleaves orphanstatesrows and its event stream behind. Pre-existing and unrelated — worth its own issue. Nothing verified on mobile.🤖 Generated with Claude Code
https://claude.ai/code/session_01C6aSoCAS5gNoew6n9qv6DJ
Generated by Claude Code