From 03c6707ca739ce82ccdd6aa6ae437b9d2293ec2a Mon Sep 17 00:00:00 2001 From: parththakkar106 Date: Sun, 16 Aug 2026 21:02:20 +0530 Subject: [PATCH] Write up the embedding-cost fix and the story-tree design Two plan documents from designing the branching story tree. The tree work turned up that memory retrieval fetches the whole bank's embeddings every turn -- 96% of a turn's database traffic -- and that is the one read a tree cannot window, so it lands first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015CYEJKobJ2Re4Dv7qUoSA7 --- plan/13-memory-embedding-cost.md | 127 +++++++++++++++++++++++++++++++ plan/14-phase-story-tree.md | 116 ++++++++++++++++++++++++++++ 2 files changed, 243 insertions(+) create mode 100644 plan/13-memory-embedding-cost.md create mode 100644 plan/14-phase-story-tree.md diff --git a/plan/13-memory-embedding-cost.md b/plan/13-memory-embedding-cost.md new file mode 100644 index 0000000..c0993a3 --- /dev/null +++ b/plan/13-memory-embedding-cost.md @@ -0,0 +1,127 @@ +# 13 — Memory-bank embedding cost (round three of the egress work) + +**Goal:** stop every turn fetching the entire memory bank's embeddings. Measured at +**3,024 KB per turn** on adventure 25 against **129 KB** for everything else a turn +reads — the memory bank is ~96% of a turn's database traffic, and it is fetched fresh +every single turn to pick `memory_top_k = 5` memories. + +Found 2026-08-16 while designing the story tree (see `14-phase-story-tree.md`), because +memory retrieval is the one read a tree **cannot** window — it is long-range recall by +design, so it always spans the full path. That makes this the cost floor of a turn under +the tree, which is why it lands first. + +## The measurement (production, Neon SQL editor) + +```sql +SELECT count(*), avg(json_array_length(embedding))::int, + avg(length(embedding::text))::int, + pg_size_pretty(sum(length(embedding::text))::bigint) +FROM memories WHERE embedding IS NOT NULL; +-- 134 memories | 1536 dims | 30,971 bytes each | 4,053 kB total +``` + +| adventure | active | fetched | per turn | +|---|---|---|---| +| 25 | 100 | 100 | **3,024 kB** | +| 21 | 18 | 18 | 545 kB | +| 12 | 12 | 12 | 363 kB | +| 20 | 4 | 4 | 121 kB | + +Break-even against the rest of a turn is **4.2 memories**, i.e. about action 25. Every +adventure past that is dominated by this. + +`active == fetched` everywhere: nothing has been evicted yet, so the Python-side +`forgotten` filter currently costs nothing. It becomes a real leak the moment eviction +starts. + +## Why round two missed it + +`retrieve_memories` needs `settings.embedding_model`, and embedding providers are +**BYOK-only by construction** — they never touch the demo key. So the public demo never +embeds anything, and the round-two stress harness (which had no embedding model +configured) measured the turn loop with its heaviest read switched off. The 23.0 MB +figure for a 200-action playthrough is the memory-bank-**off** number; with it on, +adventure 25 is closer to 300 MB. + +**Rule going forward: any egress measurement must run with an embedding model set.** + +## Root cause + +`memorybank.py:208` + +```python +candidates = [m for m in adventure.memories if not m.forgotten and m.embedding] +``` + +Walks the relationship, so every memory row for the adventure loads with its embedding. +`embedding` is `Mapped[list]` on a `JSON` column — 1536 floats serialised as text is +~31 KB. Cosine ranking happens in Python (a deliberate choice, documented at +`models.py:144`), so all of it must cross the wire. The comment sized it by **count** +("fine at a few hundred") rather than by **bytes**. + +Not the same bug as migration 36/37 — the column is not a repeating group and there is +no denormalisation to fix. It is a *format* problem plus a *fetch-frequency* problem. + +## Decisions (settled 2026-08-16) + +- **Packed float32, keep 1536 dimensions.** 31 KB → 6 KB, a straight 5x, with **zero + retrieval-quality risk**. Explicitly rejected dropping to 512/768 dims: the size fix + plus the cache makes the extra 3x unnecessary, and it would have meant re-embedding. +- **No re-embedding.** Dimensions are unchanged, so the migration is a pure format + conversion of the 134 existing rows — read the JSON, write packed bytes, no API calls. + One-time 4 MB read. +- **In-process cache alongside the size fix**, not sequenced after it. Turns for one + adventure arrive back-to-back, so a dict keyed by adventure id takes steady-state cost + to ~0. 100 vectors as float32 is 600 KB of RAM — negligible. +- **`memory_bank_capacity` 200 → ~80.** Taken on *quality* grounds as much as cost: + ranking 200 memories to pick 5 dilutes retrieval. Note this will start evicting on + adventure 25 immediately (it sits at 100). +- **Not pgvector.** It is the structural answer and would keep vectors in the database + entirely, but it breaks SQLite dev parity — which the codebase protects deliberately + (`context/history.py:42`, the `replace()`/`trim()` dialect dance). Revisit only if the + bank grows past what Python cosine can handle. + +## Work, in order + +1. **Rebuild the byte-meter harness — in the repo this time.** `backend/tools/dbmeter.py` + + `stress_session.py`. The originals lived outside the repo and are gone. **Default it + to running with an embedding model configured**, since that omission is precisely what + hid this finding. Do this first so every item below is measured, not assumed. +2. **Migration 38 — `memories.embedding_blob` (`LargeBinary`).** Backfill in Python + (`struct.pack(f"<{n}f", *vec)`); the conversion cannot be expressed in portable SQL, so + unlike migration 36/37 this one does pay a one-time 4 MB read. Drop the old JSON column + in a follow-up migration once verified, not in the same one. +3. **Read path.** `retrieve_memories` queries `memories` directly with + `forgotten = false AND embedding_blob IS NOT NULL` in **SQL**, not Python. Unpack with + `struct`/`numpy`. +4. **Vector cache.** Keyed by `adventure_id`, invalidated on memory create, evict and + delete. Must survive the retry/undo paths that prune memories. +5. **Capacity default 200 → 80.** Existing adventures inherit it, so adventure 25 evicts + on its next turn — check that the eviction path is sane at scale before shipping. +6. **Infinite scroll upward in `Play.jsx`** for the remaining 423 KB page load of a + finished adventure — the last open item from round two. Load the newest turns, fetch + older ones as the reader scrolls up. + +## Guardrails to add with this work + +- **Query-count / byte assertions per endpoint**, extending the `test_egress.py` idea: + assert an endpoint issues at most N queries and fetches under X KB against + production-sized fixtures. This class of bug is invisible at ten rows. +- **Explicit column projections on read paths.** List endpoints name the fields they + need rather than loading whole entities, so the next heavy column is opt-**in**. This + is the structural version of what `deferred=True` does by hand. +- **Row-width review rule.** Any new large column justifies itself or goes out-of-line. + `actions` now carries five JSON columns. + +Deliberately **not** taken: moving `context_snapshot` out of the database. It costs +nothing on reads now that it is deferred, and storage is ~$0.02/mo. Revisit only if +backups or storage start to hurt. + +## Verification + +- Harness: one turn on adventure 25, memory bank **on**, before and after. Target is + 3,024 kB → ~600 kB cold, ~0 warm. +- Re-run the round-two shapes with an embedding model configured, so the 200-action + playthrough number is finally honest. +- The existing `test_egress.py` guard must still pass — nothing here should touch the + deferred action columns. diff --git a/plan/14-phase-story-tree.md b/plan/14-phase-story-tree.md new file mode 100644 index 0000000..5881490 --- /dev/null +++ b/plan/14-phase-story-tree.md @@ -0,0 +1,116 @@ +# Phase 14 — Story tree (branching adventures) + +**Goal:** replace the linear action list with a **tree**. A retry becomes a sibling +rather than a rewrite; continuing from one makes it a branch. Players can go back to any +turn, take a different path, and keep both — switching between them freely. + +Depends on `13-memory-embedding-cost.md` shipping first: memory retrieval is the one read +a tree cannot window, so it is the cost floor of a turn under this design. Fix the floor +before building on it. + +## Why (beyond the feature) + +Seven bug classes stop existing, and every one traces to the same root — **the story is a +mutable list**: + +| Bug | Why it goes away | +|---|---| +| Deleting a middle action skips a later one forever (`note_action_removed`) | Cursors become node ids, not positions in a shifting list | +| `prune_dangling_memories` orphaning actions behind the cursor | Nothing is ever removed | +| Editing a summarised action leaves its memory stale forever — **currently unfixed** | Editing makes a new node; the old memory stays correct for the old path | +| The one-turn memory holdback (`settled_story_actions`) | Nothing is mutated, so nothing goes stale — the concept is unnecessary | +| The legacy-cursor no-rewind trap found while fixing that | Same | +| "Anything reading `adventure.actions` during generation must exclude the retried action" — leaked into 4 call sites | The replaced turn is not on your path; it cannot leak | +| `variant_count` / mirrored `text` drifting from `variants` | The denormalisation disappears entirely | + +It also resolves the 1NF violation: `variants` as a JSON repeating group becomes rows. + +## Design decisions (settled 2026-08-16) + +- **Full branching, with UI.** Not the "tree schema, no branch picker" middle option — + branching ships as a feature people use. +- **`branch_id` + `depth` on every node, not parent pointers alone.** Parent pointers + alone mean walking N links to read a story, which throws away the round-two windowing + work. `depth` replaces `index` as the ordering key. +- **A `branches` table with `parent_branch_id` and `fork_depth`.** The fork point is + **stored at fork time, never inferred**. Nothing is copied on a fork — a branch borrows + its ancestors' turns. +- **Lineage cached on the branch row.** `lineage = [(D,∞), (C,45), (B,30), (A,10)]`, + computed once at fork (parent's lineage + one entry). Reads never walk to reconstruct + it. Each ancestor is capped at the `fork_depth` of the branch beneath it. +- **Read the lineage lazily, windowed.** Query the newest few lineage entries, measure, + fetch more only if the context budget is not covered — the exact shape of + `history.window_covering()`. **Clause count is bounded by the context window, not by + fork count**, so a 200-fork story reads as cheaply as a 2-fork one. +- **Promote to a branch only on continue.** Attempts at the tip stay as sibling leaves; + one becomes a branch the moment a turn is played past it. Keeps the lineage chain to + "divergences I built a story on", not "every retry ever" — the difference between a + handful of entries and fifty. +- **Memories and the summary attach to the node that produced them**, found by walking + up. Shared ancestors are shared automatically, so a fork costs nothing and **nothing + needs recreating**. A memory covering depths 37–42 hangs off that branch's node 42 and + is invisible to any path not through it. Generalise the rule: *anything derived attaches + to the node that produced it* — the summary included, so stop storing it per turn. +- **Full lineage for memories, windowed lineage for the story.** Memory retrieval is + long-range recall and cannot be windowed, but memories are sparse (~1 per 6 actions), so + a long OR-clause returning ~33 small rows is fine. Two queries, one lineage. +- **Never auto-prune.** Nothing is deleted without an explicit user action. **This makes + a branch-management UI a hard dependency, not a nice-to-have** — storage grows without + limit otherwise. +- **Story cards stay adventure-wide.** A card invented on branch B shows on branch A. + Already true for undo today (script card mutations are not reverted), so this is a + documented limit, not a regression. Explicitly rejected event-sourcing card changes onto + nodes. +- **State carries over almost free.** `state_before` / `world_state_before` already + snapshot the script scoreboard and RPG world state per action, and `apply_variant()` + already restores them on a switch — a branch switch is the same move. Wrinkle: those are + *before* pictures; a node wants the *after*. And they are NULL on pre-column rows. + +## Schema sketch + +``` +branches(id, adventure_id, parent_branch_id, fork_depth, lineage JSON, created_at) +actions(id, adventure_id, branch_id, depth, type, text, reasoning, + world_delta, state_after, world_state_after, context_snapshot, created_at) +memories(..., branch_id, depth) -- attached to the node that produced it +adventures(..., head_branch_id, head_depth) +``` + +Reading branch C, tip at depth 7, lineage `[(C,7), (B,5), (A,3)]`: + +```sql +SELECT * FROM actions +WHERE (branch_id='C' AND depth <= 7) + OR (branch_id='B' AND depth <= 5) + OR (branch_id='A' AND depth <= 3) +ORDER BY depth DESC LIMIT 32 +``` + +→ `A0 A1 A2 A3 B4 B5 C6 C7`. Depth is a position along *a* path, not a global turn +number — `A4` and `B4` are alternatives, not duplicates. + +## Work + +1. `branches` table, `branch_id`/`depth` on `actions`, `head_*` on `adventures`. +2. Lineage computation + a **single module that owns the branch clause** — same role + `context/history.py` plays today. Every query must go through it; one forgotten clause + shows the wrong story, quietly. +3. `history.py` rewritten against lineage windowing. `window_covering` keeps its shape. +4. Memories/summary attached to nodes; delete the cursor-position machinery + (`position_of_index`, `settled_*`, `note_action_removed`, `_rewind_cursors_to_index`). +5. Sibling storage for un-promoted tip attempts + the promotion step. +6. Node state moves from *before* to *after* snapshots. +7. Migration: every existing adventure becomes branch A; `index` → `depth`; `variants` + entries become sibling nodes; `variant_index` becomes the head pointer. +8. Frontend: branch picker replacing `VariantPager`, plus branch management (rename, + delete, switch) — required, given no auto-pruning. + +## Open + +- **Export/import bundle format** (`routers/adventures.py:1008-1112`) encodes `variants` + and `variantIndex`. Needs a versioned format change and a legacy reader. +- **`retry_of.index` reuse** stops the world-state cooldown clock advancing on a re-run of + the same turn. Whatever replaces it must preserve that. +- **Phased or single migration?** Undecided. A tree touches the memory bank, the context + builder, undo/retry and the UI at once, which argues for phasing behind a flag. +- **What the branch-management UI actually looks like** — unscoped, and it gates release.