fix: preserve memory when upgrading an existing database

cmd/codeaf/chatv3_memory_upgrade_test.go

@@ -0,0 +1,147 @@
+package main
+
+import (
+	"database/sql"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/Agent-Field/codeaf/internal/home"
+	"github.com/Agent-Field/codeaf/internal/store"
+)
+
+// The memories table the released build left on disk: no owner column, and an
+// index set the owner-aware build's schema must not assume. It is spelled here
+// rather than read from the store because this test has to keep describing the
+// OLD shape after the current schema moves on.
+const releasedMemoriesDDL = `
+CREATE TABLE memories (
+    id          TEXT PRIMARY KEY,
+    type        TEXT NOT NULL,
+    scope       TEXT NOT NULL,
+    title       TEXT NOT NULL,
+    text        TEXT NOT NULL,
+    tags        TEXT NOT NULL DEFAULT '[]',
+    status      TEXT NOT NULL DEFAULT 'active',
+    use_count   INTEGER NOT NULL DEFAULT 0,
+    miss_count  INTEGER NOT NULL DEFAULT 0,
+    created_seq    INTEGER NOT NULL,
+    updated_seq    INTEGER NOT NULL,
+    source_session TEXT NOT NULL DEFAULT '',
+    source_seq     INTEGER NOT NULL DEFAULT 0
+);
+CREATE INDEX memories_status_updated ON memories (status, updated_seq DESC);
+CREATE INDEX memories_status_scope ON memories (status, scope, updated_seq DESC);
+CREATE VIRTUAL TABLE memories_fts USING fts5(
+    memory_id UNINDEXED,
+    title,
+    text,
+    tags
+);
+`
+
+// THE LAUNCH A PERSON ACTUALLY MAKES. Someone who has been running codeaf for
+// weeks opens the new build, and the start path asks [v3Memory] for their
+// brain. It must not answer nil — which is what it did, printing "memory is off
+// for this session: initialize memories schema: no such column: owner" — and
+// the note they had kept must still be in it.
+func TestLaunchingOnADatabaseFromBeforeOwnersKeepsTheNote(t *testing.T) {
+	fresh := t.TempDir()
+	t.Setenv("HOME", fresh)
+	root := filepath.Join(fresh, ".codeaf")
+	t.Setenv(home.EnvVar, root)
+	if err := os.MkdirAll(root, 0o700); err != nil {
+		t.Fatal(err)
+	}
+
+	// The database the released build wrote, with one note the person kept.
+	raw, err := sql.Open("sqlite", defaultChatDB())
+	if err != nil {
+		t.Fatalf("create the old database: %v", err)
+	}
+	if _, err := raw.Exec(releasedMemoriesDDL); err != nil {
+		t.Fatalf("create the old memories table: %v", err)
+	}
+	if _, err := raw.Exec(`
+		INSERT INTO memories (id, type, scope, title, text, tags, status, use_count,
+		    miss_count, created_seq, updated_seq, source_session, source_seq)
+		VALUES ('mem_kept', 'preference', 'user', 'Keeps the receipts',
+		    'Keeps the receipts for the tax year.', '["finance"]', 'active',
+		    2, 0, 11, 12, 'session-old', 7)`); err != nil {
+		t.Fatalf("insert the old note: %v", err)
+	}
+	// The released build kept its own search view, so the old database carries
+	// one; the launch must leave it answering under the new owner filter.
+	if _, err := raw.Exec(`
+		INSERT INTO memories_fts (memory_id, title, text, tags)
+		SELECT id, title, text, replace(replace(replace(tags, '[', ''), ']', ''), '"', '')
+		FROM memories WHERE id = 'mem_kept' AND status = 'active'`); err != nil {
+		t.Fatalf("index the old note: %v", err)
+	}
+	if err := raw.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	profile := t.TempDir()
+	brain := v3Memory(profile)
+	if brain == nil {
+		t.Fatal("launching on a database from before owners opened no brain")
+	}
+	rows, err := brain.ListMemories([]string{store.OwnerUser}, 20)
+	if err != nil {
+		t.Fatalf("read the person's memories: %v", err)
+	}
+	if len(rows) != 1 {
+		t.Fatalf("the person's shelf = %d rows, want the one they kept", len(rows))
+	}
+	kept := rows[0]
+	if kept.ID != "mem_kept" || kept.Title != "Keeps the receipts" ||
+		kept.Text != "Keeps the receipts for the tax year." {
+		t.Fatalf("the note came back as %+v, want it preserved", kept)
+	}
+	if kept.Owner != store.OwnerUser {
+		t.Fatalf("the note's owner = %q, want %q", kept.Owner, store.OwnerUser)
+	}
+	if kept.SourceSession != "session-old" || kept.SourceSeq != 7 {
+		t.Fatalf("the note's provenance = (%q, %d), want it preserved", kept.SourceSession, kept.SourceSeq)
+	}
+	// Search on the launched brain answers the note the person kept, under the
+	// owner the upgrade gave it.
+	found, err := brain.SearchMemories([]string{store.OwnerUser}, "receipts", 5)
+	if err != nil {
+		t.Fatalf("search the launched brain: %v", err)
+	}
+	if len(found) != 1 || found[0].ID != "mem_kept" {
+		t.Fatalf("search on the launched brain = %v, want the kept note", found)
+	}
+	if err := brain.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	// The owner index must survive the launch, because the retrieval reads are
+	// written against it and the schema can no longer declare it.
+	check, err := sql.Open("sqlite", defaultChatDB())
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer check.Close()
+	var name string
+	if err := check.QueryRow(`SELECT name FROM sqlite_master WHERE type = 'index' AND name = 'memories_owner_active'`).Scan(&name); err != nil {
+		t.Fatalf("the launch did not leave the owner index: %v", err)
+	}
+
+	// And the second launch reads the same note rather than doing the upgrade
+	// twice.
+	brain = v3Memory(profile)
+	if brain == nil {
+		t.Fatal("the second launch opened no brain")
+	}
+	defer brain.Close()
+	again, err := brain.ListMemories([]string{store.OwnerUser}, 20)
+	if err != nil {
+		t.Fatalf("read on the second launch: %v", err)
+	}
+	if len(again) != 1 || again[0].Text != kept.Text {
+		t.Fatalf("the second launch = %+v, want the same note", again)
+	}
+}

docs/changes/unreleased/1787-memory-upgrade.md

@@ -0,0 +1,33 @@
+---
+kind: fixed
+title: a store written before memory owners existed opens again on the next launch
+pr: 1787
+surface: [chat, engine]
+invalidates:
+  - "The memories schema ordered the owner index before the owner column existed, so opening any database written by an earlier release died with `initialize memories schema: no such column: owner` and the launch carried on with no brain. The index is now created beside the column by the owner migration, on a legacy store and a brand new one alike."
+  - "The owner migration returned early the moment the column was present, so an upgrade interrupted between the column and its index would have been left without the index forever. It now ensures the index on every open, whichever shape the store is in."
+  - "A memory written by an older build that is still running against an upgraded store landed with an empty owner, so no owner-filtered read — the router's shortlist, search, a forget match — could ever reach it. Every open now owns empty rows from their scope again (or quarantines them), and a row that already has an owner is never moved."
+---
+
+A database that failed to open under the broken build keeps its notes: the
+failure happens before any memory row is deleted or rewritten, so a line's words,
+its provenance and whether it was still active are all still in the file. There
+is no repair command and no backup to restore — the next open of this build adds
+the missing column, gives every row the owner its old scope proves, creates the
+owner index, and reads the same lines back. A row the old store kept as a project
+cannot prove which project it was, so it also gains the `legacy-project`
+quarantine marker and needs a proven owner before a conversation sees it again;
+nothing else about it is rewritten.
+
+Recovery is a launch of the fixed build, and installing it does not by itself
+change an engine already running. On the machine holding the workspace, updating
+codeaf and reconnecting or opening a conversation there is the whole repair: a
+newer build replaces an older engine as the window connects, busy or not (the
+turn it catches stops where it is and keeps its partial reply), and the new
+engine repairs the file as it opens. A window held by an engine on another
+machine is not reached by that reconnect — update codeaf there, and when its work
+is safe run `codeaf engine --stop --workspace <folder>` there first
+(`codeaf engine --status` names the engine if the folder is unclear). A plain
+launch with no engine holding it repairs the file at its next open. An idle
+engine of the same build may restart to pick up a changed terminal environment; a
+busy one keeps running and is only replaced when a newer build connects.

internal/manual/chat/what-i-remember.md

@@ -152,6 +152,41 @@ that teaches rather than an error. It did not always: a first run once reported
 every reason a file will not open, and a build old enough to print it is a build
 worth replacing.
 
+## It said no such column: owner
+
+That sentence is the second half of `memory is off for this session:`, and it
+means the saved store was written by an older codeaf that the version you just
+opened did not know how to read. It is a mismatch between the file and the
+program, not damage to the file.
+
+The repair is the newer codeaf itself. Its first open of the file adds what the
+old file was missing and reads the same lines back, keeping each note's words,
+where it came from and whether it was still active. A note the old store kept as
+**this project** had no way to name its project, so it also gains the quarantine
+marker `legacy-project` and needs a proven owner before any conversation is
+shown it again. There is nothing to move aside and nothing to restore by hand.
+
+**A window whose conversation is held by an engine keeps talking to that engine
+until a newer build takes its place.** That engine is a process of its own on the
+machine holding the workspace; installing a newer codeaf beside it does not
+change it. So:
+
+- Update codeaf on the machine holding the workspace.
+- Reconnect or open a conversation in that workspace. On that same machine a
+  newer build replaces the older engine as the window connects, busy or not — a
+  turn it catches stops where it is and keeps its partial reply — and the new
+  engine repairs the file as it opens.
+- A window held by an engine on **another** machine is not reached by this
+  machine's reconnect: update codeaf there too, then when its work is safe run
+  `codeaf engine --stop --workspace <that folder>` there and reconnect.
+  `codeaf engine --status` names the engine if you are unsure which folder it is.
+
+A conversation on this machine that no engine is holding is simpler: quit codeaf
+and open it again, and the file is repaired as it opens. An idle engine of the
+same build may restart to pick up a changed terminal environment; a busy one
+keeps running and is only replaced when a newer build connects. Nothing already
+saved is at risk while it waits.
+
 ## Where is everything you remember kept
 
 In `~/.codeaf/graph.db`, one file, made the first time codeaf runs. Memories,

internal/manual/chat_test.go

@@ -1522,6 +1522,11 @@ func TestTheChatManualAnswersTheQuestionsPeopleAsk(t *testing.T) {
 		{"it said memory is off but I never turned it off", "what-i-remember"},
 		{"why does it say could not open graph.db", "what-i-remember"},
 		{"codeaf printed out of memory 14 on startup", "what-i-remember"},
+		// The upgraded-store failure a person saw on the terminal: the exact
+		// sentence has to reach the page that tells them the file is fine and
+		// what to restart.
+		{"it said no such column: owner when codeaf opened", "what-i-remember"},
+		{"memory is off for this session: initialize memories schema: no such column: owner", "what-i-remember"},
 		{"where is my memory file kept on disk", "what-i-remember"},
 		{"can I copy my memories to another machine", "what-i-remember"},
 		{"how do I see what codeaf remembers", "what-i-remember"},

internal/store/memory.go

@@ -214,7 +214,13 @@ CREATE TABLE IF NOT EXISTS memories (
 );
 CREATE INDEX IF NOT EXISTS memories_status_updated ON memories (status, updated_seq DESC);
 CREATE INDEX IF NOT EXISTS memories_status_scope ON memories (status, scope, updated_seq DESC);
-CREATE INDEX IF NOT EXISTS memories_owner_active ON memories (owner, updated_seq DESC) WHERE status = 'active';
+-- THE OWNER INDEX IS NOT HERE. owner is a migration column: a database written
+-- before owners existed already has this table, so CREATE TABLE IF NOT EXISTS
+-- leaves it alone, and an index declared here would be created against a column
+-- that does not exist yet — which is exactly the open that failed with
+-- "no such column: owner" on every store already on disk. It is created by
+-- [migrateMemoriesOwner], beside the column it indexes, on both a legacy store
+-- and a brand new one.
 `
 
 const memoriesFTSSchema = `

internal/store/memory_owner.go

@@ -142,26 +142,45 @@ func ownerFilterSQL(owners []string) (string, []any) {
 // that can do the proving ([Store.RehomeMemories]); everything else stays in
 // quarantine, visible to the person and to nobody's model.
 //
-// It is idempotent — a row that already has an owner is left alone — and it is
-// written in one transaction with the column it backfills.
+// It is idempotent — a row that already has an owner is left alone — and it
+// runs in one transaction on every open, whatever shape the table is in.
+//
+// THE BACKFILL IS NOT ONLY FOR THE OPEN THAT ADDS THE COLUMN, because a
+// pre-owner build can still be RUNNING against a store this build already
+// upgraded: it sees the column, never sets it, and writes its rows with the
+// empty default. Those rows carry a real scope and would be invisible to every
+// owner-filtered read forever, so every open re-runs the same scope-derived
+// mapping, each statement guarded by owner is empty. A row with an owner —
+// quarantined, machine, person, project — is never touched, by this pass or by
+// any later one.
+//
+// The owner index is created here rather than in [memoriesSchema] for the same
+// ordering reason: the schema runs FIRST, against a table that a database
+// written before owners existed already has, so an index declared there asks
+// for a column the migration has not added yet and the open dies with
+// `no such column: owner` (the failure every store already on disk hit).
+// Ensuring it here covers both shapes — the legacy store the backfill just
+// widened, and a store that already carries the column from an earlier or
+// interrupted upgrade.
 func migrateMemoriesOwner(db *sql.DB) error {
 	found, err := tableHasColumn(db, "memories", "owner")
 	if err != nil {
 		return err
 	}
-	if found {
-		return nil
-	}
 	tx, err := db.BeginTx(context.Background(), nil)
 	if err != nil {
 		return fmt.Errorf("migrate memory owners: %w", err)
 	}
 	defer tx.Rollback()
-	if _, err := tx.Exec(`ALTER TABLE memories ADD COLUMN owner TEXT NOT NULL DEFAULT ''`); err != nil {
-		return fmt.Errorf("migrate memory owners: %w", err)
+	if !found {
+		if _, err := tx.Exec(`ALTER TABLE memories ADD COLUMN owner TEXT NOT NULL DEFAULT ''`); err != nil {
+			return fmt.Errorf("migrate memory owners: %w", err)
+		}
 	}
-	// The column lands empty and is filled from scope in one pass each, so no
-	// row is ever left with an owner the validator would refuse.
+	// Every statement below is guarded by owner = '', so this is the same pass
+	// whether it is owning a table that has just been widened or repairing the
+	// rows a still-running pre-owner build wrote into one that was widened
+	// months ago.
 	steps := []struct {
 		owner, scope string
 	}{
@@ -182,7 +201,7 @@ func migrateMemoriesOwner(db *sql.DB) error {
 	if _, err := tx.Exec(`UPDATE memories SET owner = ? WHERE owner = ''`, OwnerLegacyProject); err != nil {
 		return fmt.Errorf("migrate memory owners: %w", err)
 	}
-	if _, err := tx.Exec(`CREATE INDEX IF NOT EXISTS memories_owner_active ON memories (owner, updated_seq DESC) WHERE status = 'active'`); err != nil {
+	if _, err := tx.Exec(memoriesOwnerIndexDDL); err != nil {
 		return fmt.Errorf("migrate memory owners: %w", err)
 	}
 	if err := tx.Commit(); err != nil {
@@ -191,6 +210,12 @@ func migrateMemoriesOwner(db *sql.DB) error {
 	return nil
 }
 
+// memoriesOwnerIndexDDL is the one spelling of the owner-keyed retrieval index.
+// It is a partial index because only active rows are ever retrieved, and it
+// lives beside the column it indexes — see [migrateMemoriesOwner] for why it
+// cannot be declared in [memoriesSchema].
+const memoriesOwnerIndexDDL = `CREATE INDEX IF NOT EXISTS memories_owner_active ON memories (owner, updated_seq DESC) WHERE status = 'active'`
+
 // quarantinedMemory is one legacy project row that cannot prove its owner.
 type quarantinedMemory struct {
 	id   string