Page large finished turns by stored byte size - #1199
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request for security, code quality, architecture, performance, and practical behavior. |
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: This change splits one large finished turn into smaller pages. That reduces server memory use during a timeline load.
I found three important problems.
-
High: delegation closure defeats the byte limit. The byte cut skips the existing parent-crossing guard. Parent closure then fetches all descendants without sequence bounds. A delegated fixture loaded 32.7 MiB of event data with a 4 MiB limit. The live route returned a 16.5 MiB response. Please page nested rows or keep the cut outside parent spans.
-
High: one newest oversized event makes the thread unopenable. I stored a newest 4.19 MiB tool input. The latest timeline route returned a non-retryable 413 before any row rendered. The read limit does not shorten tool arguments, file data, or user text. Please return a bounded placeholder page for one oversized event.
-
Medium: the client loses the newest slice of a finished turn. The API returned all 650 commands across eight pages. After browser auto-load, the UI showed 567 commands. Expansion ended at command 566, so commands 567 through 649 were inaccessible. Please preserve completion state and use distinct row identities for page slices.
The architecture scan found one useful refactor. getStoredTimelineWindowEventDataBytes scans the same window before the floor query scans it again. The floor query already returns fits. One query can handle all three results.
The security review found no SQL injection or cross-thread cursor access. Drizzle binds the raw prepared-query parameters.
Validation passed:
- Server timeline tests: 17 passed.
- Database event tests: 70 passed.
- Turbo type checks passed for
@bb/serverand@bb/db. - The development app and timeline route started successfully.
- Browser automation loaded the eight-page finished-turn fixture and reproduced the missing newest slice.
I left inline comments on the three important findings. I did not approve or request changes.
|
Addressed all SlopCop findings in 48daf8b.
The original 1,305-event fixture returned eight HTTP 200 pages under a 180 MiB heap. All 650 tool calls remained available. App and database suites passed. The server suite had only the known local file-mode skill-tree failure. |
48daf8b to
4268b9a
Compare
Reproduction
A finished turn with 1,305 events and 32,686,106 stored bytes exhausted a 180 MiB V8 heap. The timeline request returned an empty reply, and the server restarted.
Root cause
The 1,500-event budget did not limit stored bytes. Finished turns also kept one summary segment, so the server loaded and decoded the complete 32 MiB turn.
Fix
Measure stored UTF-8 bytes before row allocation. Move the sequence floor until the newest suffix fits. Return a truncated page with an older cursor. This changes timeline pagination semantics because one finished summary can now span several sequence pages.
The limit is 4 MiB. A 16 MiB latest page failed the heap test. A later 8 MiB nested page also failed. At 4 MiB, eight HTTP 200 pages retained all 650 tool calls under a 180 MiB heap.
A single event over 4 MiB cannot fit in a smaller window. The server now returns HTTP 200 with a safe placeholder. It gives the client an older cursor when earlier events exist. This replaces the previous permanent 413 response.
Tests cover byte floors, delegated children, finished-turn page identity, client page merge, and the single-event placeholder. All touched package type checks passed. All relevant tests passed. The full server suite has only the known local file-mode skill-tree failure.
Fixes #1129