refactor(memory): drop dead migration cruft from InitializeAsync#589
Merged
Aaronontheweb merged 4 commits intodevfrom Apr 11, 2026
Merged
refactor(memory): drop dead migration cruft from InitializeAsync#589Aaronontheweb merged 4 commits intodevfrom
Aaronontheweb merged 4 commits intodevfrom
Conversation
The fresh CREATE TABLE schema is already domain-free. The DROP COLUMN / DROP INDEX migration and the DropColumnIfExistsAsync/DropIndexIfExistsAsync helpers were only there for backward compatibility with existing user DBs, which is unnecessary — the store is single-user and the DB is disposable.
Every column being "ensured" in InitializeAsync (memory_class, expires_at, aliases_json, facets_json, slots_json, boundary, audience on both memory_documents and memory_records) is already declared in the CREATE TABLE statements in the same method. On every daemon startup the 14 calls issued 14 redundant PRAGMA table_info scans for columns that always existed on fresh installs. Same reasoning as the DROP COLUMN cleanup: single-user prototype, disposable DB, no backward-compat requirement.
…ema init Five related cleanups surfaced during the /simplify review of the `remove-domain-migration` branch. Each was defensive code that papered over a latent bug or was cleaning up shapes no current write path produces. 1. Delete hygiene UPDATE #1 (turn-completion/ConversationTrace → Never) and the defensive filters/ranker penalties keyed on them. `MemoryCurationPipeline.Extract` early-returns on TurnComplete+!Explicit and only emits "Project Fact:" / "Project Milestone:" titles — nothing currently writes `title='turn-completion'` to memory_documents. 2. Delete hygiene UPDATE #2 (metadata backfill for malformed doc:% titles) plus the pinning test `InitializeAsync_demotes_malformed_auto_recall_ documents_to_searchable` and the `MissingMetadataFacet` constant. 3. Replace the startup FTS rebuild (unconditional DROP + CREATE VIRTUAL TABLE + full INSERT ... SELECT, O(rows) every boot) with `CREATE VIRTUAL TABLE IF NOT EXISTS`. To make this safe, fix the runtime FTS write paths that the rebuild was papering over: - `InsertDocumentFtsAsync` / `InsertRecordFtsAsync` → rename to `UpsertDocumentFtsAsync` / `UpsertRecordFtsAsync`, DELETE-then-INSERT so repeat upserts do not leave duplicate FTS rows - `TombstoneDocumentAsync` now deletes the FTS row - `SupersedeRecordAsync` now deletes the old record's FTS row - Fix two raw-string-literal SQL interpolation bugs (`TombstoneDocumentAsync`, `SupersedeRecordAsync`) where the missing `$` prefix caused `{MemoryUpdateSemantics.*.ToWireValue()}` to be written literally into the column. 4. Unify startup schema ordering. Move `SchemaMigrationHostedService` registration before memory services so its StartAsync runs before `MemoryCurationWorkerService`. Delete the synchronous `memoryStore.InitializeAsync(...).GetAwaiter().GetResult()` at DI registration time. Have the hosted service call both the versioned migrator and `SQLiteMemoryStore.InitializeAsync` in sequence so there is exactly one startup path for schema setup.
Two regressions introduced by the previous cleanup commit, caught by /simplify review: 1. SchemaMigrationHostedService.StartAsync early-returned when the akka persistence provider wasn't SQLite or auto-migrate was disabled, which silently skipped _memoryStore.InitializeAsync(). The memory store always uses SQLite regardless of akka persistence config, so its schema init now runs unconditionally outside the guard. 2. TombstoneDocumentAsync, SupersedeRecordAsync, and UpdateDocumentTextAsync ran the base-table UPDATE/INSERT and the FTS delete/insert as two separate implicit transactions. Before this branch the startup FTS rebuild cleaned up any drift; with the rebuild gone, a crash between the two statements leaves FTS stale (tombstoned doc still searchable, superseded record's old row orphaned). Wrap each path in an explicit BeginTransaction/Commit so both writes land together or neither does.
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
1 participant
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.
Summary
Pure-deletion PR. Removes 92 lines of dead migration scaffolding from
SQLiteMemoryStore.InitializeAsyncthat existed only for backward compatibility with pre-existing user DBs — unnecessary for a single-user prototype with a disposable DB.b20cdc2, 56 lines): Drop theDROP COLUMN domain+ policy-index recreation block + theDropColumnIfExistsAsync/DropIndexIfExistsAsynchelpers.da0adc0, 36 lines): Drop 14EnsureColumnExistsAsynccalls (every column is already in theCREATE TABLEstatements) + the now-orphanedEnsureColumnExistsAsynchelper.Startup-path savings per
InitializeAsyncinvocationtable_infoscans (14 from ensure-column, 4 from drop-column)DROP INDEX IF EXISTSCREATE INDEX IF NOT EXISTS(policy-index recreate)ALTER TABLE ADD COLUMN+ 4ALTER TABLE DROP COLUMNAll were no-ops on fresh DBs, but the PRAGMA/DROP statements ran unconditionally every boot.
Scope — findings flagged but NOT in this PR
/simplifysurfaced three more dead-migration-adjacent blocks in the same method that could be cut with the same reasoning, but each removes defense-in-depth or requires retiring a live contract:turn-completion/ConversationTracedocs toRecallMode=Never. Band-aid forLlmSessionActor.cs:1488writing them withRecallMode=Auto. Recall coordinator already filters at query time. No test pins it.facets_jsonfor malformeddoc:%titles. Pinned byInitializeAsync_demotes_malformed_auto_recall_documents_to_searchabletest.Also noted: memory tables use ad-hoc
CREATE TABLE IF NOT EXISTSinInitializeAsync, while the rest of the daemon uses versioned SQL files viaSchemaMigrator. Two schema paths run against the samenetclaw.dbat different lifecycle stages with no ordering guarantee. Consolidation is a future cleanup.Test plan
dotnet build— cleandotnet test— 1,964 tests passdotnet slopwatch analyze— 0 issues