diff --git a/backend/app/models.py b/backend/app/models.py index 3654729..cfe930e 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -134,6 +134,12 @@ class Adventure(Base): story_cards: Mapped[list["StoryCard"]] = relationship( back_populates="adventure", cascade="all, delete-orphan" ) + # Every action of the adventure — that is, every *branch's*. Not the story + # being played, and re-ordering it by depth would not make it one: the + # collection is the tree, and a path is a selection out of it. Anything + # showing a reader a story goes through `context.history`, which goes + # through the branch clause. What is left here is ownership and the + # delete-orphan cascade, which are facts about the adventure. actions: Mapped[list["Action"]] = relationship( back_populates="adventure", cascade="all, delete-orphan", diff --git a/backend/app/routers/adventures.py b/backend/app/routers/adventures.py index bc48323..dcf67d1 100644 --- a/backend/app/routers/adventures.py +++ b/backend/app/routers/adventures.py @@ -1144,6 +1144,12 @@ def export_adventure( # Export is the one read that genuinely wants every attempt, so it asks for # the deferred `variants` column up front — iterating adv.actions instead # would lazy-load it one row at a time. + # + # And the one read deliberately left un-pathed: a backup wants the whole + # adventure, not the branch its owner happens to be standing on. `index` + # orders it because the v1 bundle is a flat list keyed on index and its + # reader has no idea branches exist — which is exactly why SP6 replaces the + # format rather than quietly widening this query. exported_actions = ( db.query(models.Action) .filter(models.Action.adventure_id == adv.id) diff --git a/backend/app/tree.py b/backend/app/tree.py index c30cf33..4e7dc0d 100644 --- a/backend/app/tree.py +++ b/backend/app/tree.py @@ -88,15 +88,21 @@ def head_branch(db: Session, adventure: models.Adventure) -> models.Branch: def place_action( - db: Session, adventure: models.Adventure, action: models.Action + db: Session, + adventure: models.Adventure, + action: models.Action, + branch: models.Branch | None = None, ) -> models.Branch: """Put `action` on the head branch and move the head to it. `depth` follows `index` while the two coexist. They have to agree: a read ordering by depth and a cursor counting in index space are describing the same story, and SP2 swaps one for the other under everything at once. + + `branch` is the head, already resolved, for a caller placing several nodes + at once — see `place_new_nodes` for why that is worth a parameter. """ - branch = head_branch(db, adventure) + branch = branch or head_branch(db, adventure) action.branch_id = branch.id if action.depth is None: action.depth = action.index @@ -107,7 +113,10 @@ def place_action( def place_memory( - db: Session, adventure: models.Adventure, memory: models.Memory + db: Session, + adventure: models.Adventure, + memory: models.Memory, + branch: models.Branch | None = None, ) -> models.Branch: """Attach a memory to the node that produced it. @@ -115,7 +124,7 @@ def place_memory( that node's depth. A hand-written memory summarises nothing, so its depth stays NULL and it belongs to the adventure rather than to a path. """ - branch = head_branch(db, adventure) + branch = branch or head_branch(db, adventure) memory.branch_id = branch.id if memory.depth is None and memory.source_end is not None: memory.depth = memory.source_end @@ -135,7 +144,15 @@ def place_new_nodes(session: Session) -> None: Nodes whose adventure has not been inserted yet are left alone: there is no id to hang a branch off, and an Action needs `adventure_id` to be written at all, so the case does not arise from any writer we have. + + The head is resolved once per adventure per flush, and held in `heads` for + the length of the call. That is not just saving a dictionary lookup: the + identity map holds *weak* references, so a branch row nobody keeps a strong + reference to is collected between two nodes and read back from the database + for the next one. Resolving per node turned a fixture writing two hundred + actions in one flush into two hundred SELECTs on `branches`. """ + heads: dict[int, models.Branch] = {} for obj in list(session.new): if isinstance(obj, models.Action): place = place_action @@ -146,8 +163,12 @@ def place_new_nodes(session: Session) -> None: if obj.branch_id is not None or obj.adventure_id is None: continue adventure = session.get(models.Adventure, obj.adventure_id) - if adventure is not None: - place(session, adventure, obj) + if adventure is None: + continue + head = heads.get(adventure.id) + if head is None: + head = heads[adventure.id] = head_branch(session, adventure) + place(session, adventure, obj, head) def refresh_head(db: Session, adventure: models.Adventure) -> None: diff --git a/backend/tests/test_branch_clause.py b/backend/tests/test_branch_clause.py index 7bc48ea..0f1fe54 100644 --- a/backend/tests/test_branch_clause.py +++ b/backend/tests/test_branch_clause.py @@ -350,6 +350,28 @@ def test_a_memory_written_without_a_branch_is_placed_anyway(forked): assert memory.depth == 3 +def test_placing_a_flush_of_nodes_reads_the_branch_once(forked, emitted_sql): + """The guard resolves the head once per flush, not once per node. + + The identity map holds weak references, so a branch row nobody keeps a + strong reference to is collected between two nodes and read back for the + next one. Writing two hundred actions in one flush was two hundred SELECTs + on `branches` before the head was hoisted out of the loop, and nothing + about the result would have told you. + """ + db, adventure, _ = forked + emitted_sql.clear() + for i in range(50): + db.add(models.Action( + adventure_id=adventure.id, index=500 + i, type="do", text=f"bulk {i}" + )) + db.commit() + branch_reads = [s for s in emitted_sql if s.startswith("SELECT") and "FROM branches" in s] + assert len(branch_reads) <= 2, ( + f"{len(branch_reads)} reads of `branches` to place 50 nodes" + ) + + def test_an_adventure_with_no_branch_at_all_reads_as_empty(forked): """The loud version of a missing branch: nothing, rather than everything. diff --git a/plan/14-phase-story-tree.md b/plan/14-phase-story-tree.md index c2a5193..8e873ce 100644 --- a/plan/14-phase-story-tree.md +++ b/plan/14-phase-story-tree.md @@ -293,6 +293,52 @@ doc's own example reads back as `A0 A1 A2 A3 B4 B5 C6 C7`, and that a sibling's invisible. Egress: a 20-fork fixture costs within a small factor of a 1-fork one — clause count is bounded by the context window, not by fork count. +**Done, 2026-08-17** (branch `sp2-branch-clause`). **317 tests green**, the 297 from SP1 +plus 20 in `test_branch_clause.py`. The baseline contract passes **unmodified**, which +was the pass condition. Six things worth not rediscovering: + +- **The contract forced the write side, not the read side.** The SP0 baseline writes its + actions straight to the database and must pass unmodified — so "every writer calls + `place_action`" could not be the invariant, because the baseline is a writer and does + not. Neither did any of the eleven other test fixtures. The alternative was a read + tolerant of a NULL branch, which is the quiet-wrong-story failure this subphase exists + to make impossible. So the session enforces it instead: `tree.place_new_nodes` runs + from `Session.before_flush` and places anything unplaced, registered in `models.py` so + that importing the models arms it. The call sites keep their explicit calls — a node + placed at the call site is placed *before* the code around it reads the row back. +- **A branch could no longer be created with a flush.** `root_branch` did + `db.add(); db.flush()` to get the id its lineage names, and a nested flush inside + `before_flush` raises. It inserts through Core and reads the row back — same + transaction, three statements, once per adventure ever. +- **The identity map holds weak references, and it cost 25 % of the suite.** Resolving + the head branch per node re-read the `branches` row for every node in the flush, because + nothing held a strong reference between two calls: 201 SELECTs to write 200 actions, + and 36 s → 45 s on the same 297 tests. The head is now resolved once per adventure per + flush (2 SELECTs), which put the suite back at 38.8 s *with* 20 more tests. Pinned by a + test, because the symptom is only ever a stopwatch. +- **The index screen is the one read scoped by head branch rather than by lineage.** + `_latest_narration` picks one row per adventure for a hundred adventures at once, and a + lineage clause each would put hundreds of OR-terms on that query. The two answers differ + only for a branch with no nodes of its own, which cannot exist — a branch is created by + playing a turn onto it. +- **Two reads are deliberately left un-pathed**, both documented where they live. + `max_action_index` allocates the legacy `index`, which must stay adventure-wide or two + branches issue the same number; and export is a flat v1 bundle whose reader has no idea + branches exist, which is why SP6 replaces the format rather than widening the query. + A third is a known divergence, not a decision: the index screen's `action_count` counts + the tree, and will overstate a branched story until SP5. +- **`Adventure.actions` was left ordered by `index` on purpose.** Ordering the collection + by depth would not make it a story — it is every branch's actions, and a path is a + selection out of it. What the relationship is still for is ownership and the + delete-orphan cascade. + +Measured: a story forked **20 times reads its newest 32-action window for 3,178 B against +the 2,961 B an unforked story of the same length costs (1.07×)**, naming one branch of its +22 lineage entries. The pre-tree 600-action `--keep` fixture was migrated and then driven +over HTTP end to end: index **1,840 B**, page load **64,149 B** — the same shapes as +before the phase — and scrolling to the start took 9 pages and saw all 600 actions exactly +once. + ### SP3 — Memories and summary attach to nodes Cursors stop being positions in a shifting list. `memory_cursor`/`summary_cursor` become diff --git a/plan/STATUS.md b/plan/STATUS.md index 1a0d1e1..e366a3e 100644 --- a/plan/STATUS.md +++ b/plan/STATUS.md @@ -78,26 +78,29 @@ needed; nothing requires reading a row of anyone's story. ## Pick up here -**`plan/14-phase-story-tree.md`, SP2 — the branch clause.** SP0 (the regression contract -and the `--rich` fixture) and SP1 (schema, migration, and the writer that keeps new rows -on the tree) are done and green; nothing is deployed yet. SP2 is where the reads move -onto `(branch_id, depth)`, and where the highest-risk line in the whole phase lives: -`history._from_memory()` slices `adventure.actions`, which under a tree is *every -branch's* actions rather than the path. Make that shortcut branch-aware or delete it. +**`plan/14-phase-story-tree.md`, SP3 — memories and the summary attach to nodes.** SP0 +(the regression contract and the `--rich` fixture), SP1 (schema, migration, and the writer +that keeps new rows on the tree) and SP2 (the branch clause: every action read now selects +on `(branch_id, depth)` through `app/context/lineage.py`) are done and green; nothing is +deployed yet. SP3 turns `memory_cursor`/`summary_cursor` from positions in a shifting list +into node anchors, and deletes the cursor-position machinery that goes with them — +`position_of_index`, `note_action_removed`, `_rewind_cursors_to_index`, +`prune_dangling_memories`. **`settled_story_actions` and the holdback stay until SP4**: +they exist because retry mutates a row in place, and retry stops doing that in SP4, not +in SP3. Deleting them early reopens the exact bug they were written for. **The schema is live in code but not on production.** When SP1 ships, the deploy needs one `VACUUM FULL actions;` on the direct (non-`-pooler`) endpoint afterwards — it rewrites every row. See the 144 MB lesson at the top of this file. -Two things from the egress work are worth carrying into it: +Two things to carry into it: -- **Paging already anticipates the tree.** `action_window` in `routers/adventures.py` - anchors on an action id and orders by comparing `Action.index`, never by treating - index as a position. A branch changes which actions are on the path, not how two of - them order, so the anchor survives; anything counting offsets would not. -- **Weigh new columns in bytes.** A tree adds parent/branch columns to `actions`, which - is already the table that fills the disk. `tests/test_egress.py` has byte ceilings - now — they will tell you. +- **Every action read goes through `context/lineage.py`.** A memory read has to as well — + the same `Path` builds a clause over `Memory` — and it reads the *full* lineage, not a + window: retrieval is long-range recall and cannot be windowed. It stays affordable + because memories are sparse, so assert the byte cost on a deep fork. +- **Weigh new columns in bytes.** `actions` is already the table that fills the disk. + `tests/test_egress.py` has byte ceilings now — they will tell you. **After any migration that rewrites `actions`:** one `VACUUM FULL actions;`. That is the lesson of the 144 MB above — a rewrite doubles the table and only a `VACUUM FULL` gives @@ -109,6 +112,36 @@ drive it before rewriting it. --- +## What happened on 2026-08-17, part five — the tree, SP2 + +Every read of an action now goes through one module. `app/context/lineage.py` turns a +branch's stored lineage into the OR-of-ranges that is "this story", and history, paging, +the newest-action lookups, the index screen and the scripting history API all select +through it. Ordering moved from `index` to `depth`. **317 tests green**, and the SP0 +baseline still passes unmodified, which was the pass condition. Branch `sp2-branch-clause`. + +Three things to carry forward: + +- **A read-side invariant needs a write-side floor.** From SP2 a row without a branch is a + row no read can see, and it fails by *disappearing*. Wiring every writer was not enough, + because the SP0 baseline and eleven other fixtures write actions straight to the database + and never call `place_action` — and the baseline may not be edited. `tree.place_new_nodes` + now runs from `Session.before_flush`, so nothing can be written unplaced. That is a + better invariant than the one SP1 shipped, and it was the contract that forced it. +- **The SQLAlchemy identity map is weak, and that is a performance cliff.** Resolving the + head branch once per node re-read the row from the database for every node in a flush — + 201 SELECTs to write 200 actions, and a 25 % slower suite (36 s → 45 s). Nothing about + the results changed; only a stopwatch could see it. Hoist the lookup out of the loop and + hold the reference for the length of the call. Now pinned by a test. +- **Clause count is bounded by the window, and it is now measured.** A story forked 20 + times reads its newest 32 actions naming *one* branch, for 1.07× what an unforked story + of the same length costs. Reading the tail widens the lineage only when a deleted action + leaves the estimate short. + +The 600-action `--keep` fixture — a genuine pre-tree database — was migrated and then +driven over HTTP: index 1,840 B, page load 64,149 B (both unchanged), and scrolling to the +start took 9 pages and saw every action exactly once. + ## What happened on 2026-08-17, part four — the tree, SP0 and SP1 No behaviour change, and none intended: a linear story is a tree with one branch, so