summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorAdam Malczewski <[email protected]>2026-05-30 20:06:36 +0900
committerAdam Malczewski <[email protected]>2026-05-30 20:06:36 +0900
commitb19f1aafc43141a865ecd40a813ed3212e77d95e (patch)
tree6d7e03f3dee11cd4067f92caf06d317ae2342c2f
parent0f39b6f78957aacf206012ad2193d9b0c1940c1f (diff)
downloaddispatch-b19f1aafc43141a865ecd40a813ed3212e77d95e.tar.gz
dispatch-b19f1aafc43141a865ecd40a813ed3212e77d95e.zip
docs(chunks): eviction limitation + gemini review of the chunk-log refactor
- eviction-limitation.md: frontend eviction is whole-message, not per-chunk; options to fully fix later. - gemini-chunk-log-review.md: read-only review of the refactor (cache fix, flat storage, pagination).
-rw-r--r--eviction-limitation.md95
-rw-r--r--gemini-chunk-log-review.md90
2 files changed, 185 insertions, 0 deletions
diff --git a/eviction-limitation.md b/eviction-limitation.md
new file mode 100644
index 0000000..a5b948f
--- /dev/null
+++ b/eviction-limitation.md
@@ -0,0 +1,95 @@
+# Known Limitation: Frontend eviction is whole-message, not per-chunk
+
+Status: **open / deal-with-later.** Documented from the append-only chunk-log
+work (see `plan-chunk-log.md`). The backend is not the problem — this is purely
+a frontend in-memory concern.
+
+## TL;DR
+
+The append-only chunk log made **loading** per-chunk (you fetch the last N
+*chunks*, not N whole turns), but the frontend's in-memory **eviction** still
+drops whole *messages* (turns). A single pathological turn (e.g. the 150-tool-call
+incident — one assistant message holding ~150 chunks) therefore stays resident in
+browser memory in full until it scrolls out of the protected window. On a
+memory-constrained device, one giant turn can still blow the budget.
+
+So: the chunk log helps long *histories* (many normal turns), but does **not**
+yet help a single oversized *turn*.
+
+## What "eviction" is (for the record)
+
+`evictMessages` (`packages/frontend/src/lib/tabs.svelte.ts:345`) trims a tab's
+in-memory `messages` array when its total chunk count exceeds
+`tab.chunkLimit`, to bound browser RAM. It never deletes anything from the DB —
+the `chunks` table is the durable source of truth and evicted content is
+re-fetched on scroll-up via `loadMoreMessages`
+(`tabs.svelte.ts:402`). The active/streaming turn and the last user+assistant
+pair are pinned; eviction is suppressed while the user is scrolled up.
+
+## Root cause
+
+Eviction operates at message granularity and explicitly refuses to trim within a
+message. From `tabs.svelte.ts` (the `evictMessages` comment, ~`:360-372`):
+
+> We never trim chunks from WITHIN a message: messages are the
+> persistence/pagination unit ... Whole-message eviction from the front is the
+> correct granularity.
+
+That comment is now stale in spirit: persistence/pagination is per-**chunk**
+(the flat `chunks` table + `GET /messages` windowing by chunk `seq`), but the
+in-memory store still holds **grouped `ChatMessage[]`** as its source of truth,
+so the smallest evictable unit is a whole turn.
+
+```
+DB / wire: per-chunk ✅ (chunks table, chunk-seq pagination)
+Frontend load: per-chunk ✅ (GET /messages?limit=N windows the chunk log)
+Frontend evict: per-MESSAGE ❌ (tab.messages is grouped; a turn is atomic)
+```
+
+## Why it wasn't fixed in this pass
+
+True per-chunk eviction requires the frontend store's **source of truth to be the
+flat chunk list**, with `messages` derived for rendering (this was P5 in
+`plan-chunk-log.md`). That means:
+
+- store `tab.chunks: ChunkRow[]` (+ the in-flight live turn) instead of
+ `tab.messages: ChatMessage[]`;
+- rewrite every streaming handler (`applyChunkEvent`, `routeSystemEvent`, the
+ `done` / `status` / `statuses` / error paths) to mutate the flat list;
+- derive `messages` via `groupRowsToMessages` for the render layer.
+
+That touches ~10 handler sites in `tabs.svelte.ts` and the ~60 frontend tests in
+`chat-store.test.ts`, all of which currently assert against the grouped
+`tab.messages` model. It was deferred to keep the test suite green and the
+diff bounded. The hard part (flat storage + DB-free `explode`/`group` transforms
+in `packages/core/src/chunks/transform.ts`) is already done and reusable.
+
+## Fix options (later)
+
+1. **Flat-chunk store + derived messages (recommended, the "proper" fix).**
+ Make `tab.chunks: ChunkRow[]` the source of truth; derive `messages` with
+ `groupRowsToMessages` (already shared from
+ `@dispatch/core/src/chunks/transform.js`). Eviction then trims the flat array
+ at chunk granularity; pin the in-flight turn + the tail. Re-fetch on
+ scroll-up by chunk `seq` (already supported). Highest effort (handler +
+ test rewrite), but fully solves it and matches `plan-chunk-log.md` P5.
+
+2. **Partial trim of an oversized message (incremental, lower effort).**
+ Keep the grouped model, but when the *oldest* in-memory message alone exceeds
+ the limit, drop its leading chunks (whole `text`/`thinking`/`tool-batch`
+ units) and record a per-message `oldestChunkSeq` so `loadMoreMessages` can
+ re-hydrate it. Caveat: must keep the message renderable and merge correctly on
+ scroll-up (the `turnId` merge in `loadMoreMessages` already handles the
+ boundary case). A pragmatic stopgap.
+
+3. **Render virtualization (separate concern).**
+ Windowing the *DOM* (only mount visible bubbles) reduces render cost but not
+ the JS-heap cost of holding the chunks. Complementary to 1/2, not a
+ substitute.
+
+## Acceptance check for whichever fix
+
+Load a tab whose history contains one turn with ≫ `chunkLimit` chunks; confirm
+in-memory chunk count stays ≤ `chunkLimit` (± the pinned tail) while scrolled to
+the bottom, and that scrolling up re-hydrates the trimmed chunks of that same
+turn without duplication.
diff --git a/gemini-chunk-log-review.md b/gemini-chunk-log-review.md
new file mode 100644
index 0000000..e04f6c3
--- /dev/null
+++ b/gemini-chunk-log-review.md
@@ -0,0 +1,90 @@
+# Review: Append-Only Chunk-Log Refactor
+
+## Executive Summary
+
+The refactor successfully transitions the codebase from a "message-as-container" model to a flat, append-only **chunk log**. This is a major structural improvement that enables granular pagination and addresses the primary root cause of Anthropic prompt-cache churn by segmenting multi-step turns into stable message pairs.
+
+**Status: Partially Correct.**
+- **Goal 1 (Cache Fix):** **Achieved for happy-path turns**, but **broken for turns involving user interrupts**. The planned "New model" for interrupts (append-only user chunks) was not implemented; the legacy mutation-based stripping remains in `agent.ts`, which continues to bust the cache prefix across steps when an interrupt occurs.
+- **Goal 2 (Flat Storage/Pagination):** **Fully Achieved.** The explode/group transforms are robust, lossless, and handle window boundaries correctly via `turnId` merging in the frontend.
+
+---
+
+## Findings & Questions
+
+### 1. Cache Stability
+**Severity: Should-Fix**
+The implementation of `toModelMessages` (`agent.ts:162`) correctly segments assistant turns into stable `[assistant, tool]` pairs per step. However, the **interrupt logic** (`agent.ts:241`) reintroduces instability.
+- In Step 1 of a turn, an interrupt is marked "freshest" and included in the Step 1 `tool` message.
+- In Step 2, that same Step 1 `tool` message is now "stale" and has the interrupt stripped.
+- **Result:** The serialized content of Step 1 changes between Request 1 and Request 2, shattering the Anthropic cache prefix for everything following the Step 1 text.
+- *Reference:* `packages/core/src/agent/agent.ts:241-247` and `packages/core/src/agent/agent.ts:80-86`.
+
+### 2. Explode/Group Fidelity
+**Severity: Verified Correct**
+The round-trip between `Chunk[]` and `ChunkRow[]` is lossless.
+- `explodeTurn` correctly splits `tool-batch` into paired `tool_call` and `tool_result` rows.
+- `groupRowsToMessages` correctly reconstructs turns using `turnId` and `step`, and gracefully handles orphan `tool_result` rows by creating synthetic entries in the batch. This is vital for pagination.
+- *Reference:* `packages/core/src/chunks/transform.ts`.
+
+### 3. Step Derivation
+**Severity: Verified Correct**
+The assumption that a `tool-batch` marks the end of an LLM step is consistent with the `Agent.run()` loop. The `step` increment in `explodeTurn` (`transform.ts:89`) and the segmentation in `toModelMessages` (`agent.ts:251`) are in sync.
+
+### 4. Migration Safety
+**Severity: Verified Correct**
+The migration in `db/index.ts` correctly detects the legacy `messages` table and performs a one-shot nuke of messages/tabs.
+- It is safe for repeat runs (idempotent check on `sqlite_master`).
+- Tests are safe as they mock `getDatabase()` and use in-memory fakes.
+- *Reference:* `packages/core/src/db/index.ts:106-118`.
+
+### 5. Persistence Correctness
+**Severity: Verified Correct**
+The "Write-on-seal" strategy is correctly implemented.
+- `processMessage` accumulates chunks in memory and flushes exactly once via `flushAssistant()` when the turn settles.
+- The fallback-retry path correctly avoids calling `flushAssistant()` for failed attempts, preventing partial/duplicate turns in the log.
+- *Reference:* `packages/api/src/agent-manager.ts:1081-1085` and `:1168`.
+
+### 6. Rebuild Correctness
+**Severity: Nit**
+`getMessagesForTab` fetches the *entire* chunk log for a tab to rebuild the Agent's in-memory history. For very long conversations (thousands of chunks), this will cause increasing latency and memory pressure whenever an Agent is reconstructed (e.g., on model switch).
+- *Reference:* `packages/core/src/db/chunks.ts:121`.
+
+### 7. Pagination Correctness
+**Severity: Verified Correct**
+Frontend pagination correctly handles turns split across the 50-chunk window.
+- `loadMoreMessages` in `tabs.svelte.ts` detects `turnId` + `role` matches at the boundary and merges the chunks.
+- This ensures that scrolling up restores the "tail" of a turn and prepends the "head" seamlessly.
+- *Reference:* `packages/frontend/src/lib/tabs.svelte.ts:445-467`.
+
+### 8. Interrupt Handling
+**Severity: Blocker (for Caching Goal)**
+The refactor failed to implement the "New model" described in `plan-chunk-log.md` (Section 4).
+- The plan called for interrupts to be appended as their own `user/text` chunks, making history immutable.
+- The implementation instead kept the legacy `[USER INTERRUPT]` string injection and the unstable `stripUserInterruptBlock` logic.
+- This preserves the cache-churn bug for any session involving interrupts.
+- *Reference:* `packages/core/src/agent/agent.ts:162-256`.
+
+### 9. Anthropic Wire Validity
+**Severity: Verified Correct**
+- `applyAnthropicStructuralNormalisations` correctly handles Anthropic's strict requirements, including splitting assistant messages if tool-calls are followed by text (Pass 3) and scrubbing tool IDs (Pass 2).
+- Empty reasoning blocks are stripped to avoid API errors while maintaining the signature in the DB for future turns.
+
+---
+
+## Verified Correct
+The following components were reviewed and found to be implementation-perfect:
+- **`packages/core/src/chunks/transform.ts`**: Pure logic for flattening and re-grouping.
+- **`packages/core/src/db/chunks.ts`**: Monotonic `seq` allocation and pagination queries.
+- **`packages/api/src/routes/tabs.ts`**: Cursor-based history endpoint.
+- **`packages/core/src/agent/agent.ts`**: `applyAnthropicCaching` breakpoint placement.
+
+---
+
+## Recommendations
+
+1. **Fix Interrupt Immutability (High Priority):** Follow the original plan: remove `USER_INTERRUPT_MARKER` injection into tool results. Instead, when an interrupt is dequeued in `Agent.run()`, append it as a new `user` role chunk to the log *after* the current step's tool results. Remove the stripping logic from `toModelMessages`. This makes the prefix 100% stable.
+2. **Explicit Transactions in `appendChunks` (Nit):** Wrap the loop in `appendChunks` (`db/chunks.ts:40`) in an explicit `db.transaction()` to ensure atomicity and improve performance for large turns.
+3. **Optimize History Rebuild (Nit):** Consider limiting the history rebuild in `getOrCreateAgentForTab` to the last N turns or the last M chunks, rather than the entire history, to bound startup time for long-lived tabs.
+4. **Tab-level Lock in `processMessage` (Nit):** Add a primitive lock or a "running" check at the start of `processMessage` to prevent concurrent execution on the same tab, which could otherwise corrupt the `tabAgent` state or the chunk log if two turns are interleaved.
+5. **Update `eviction-limitation.md`**: The document accurately reflects that eviction is still whole-message; this remains a valid technical debt item.