diff options
| author | Adam Malczewski <[email protected]> | 2026-05-30 20:06:36 +0900 |
|---|---|---|
| committer | Adam Malczewski <[email protected]> | 2026-05-30 20:06:36 +0900 |
| commit | b19f1aafc43141a865ecd40a813ed3212e77d95e (patch) | |
| tree | 6d7e03f3dee11cd4067f92caf06d317ae2342c2f | |
| parent | 0f39b6f78957aacf206012ad2193d9b0c1940c1f (diff) | |
| download | dispatch-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.md | 95 | ||||
| -rw-r--r-- | gemini-chunk-log-review.md | 90 |
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. |
