Retain large event outputs, render generated images, and migrate legacy rows - #2920
Retain large event outputs, render generated images, and migrate legacy rows#2920ymichael wants to merge 9 commits into
Conversation
214378b to
1f46348
Compare
ymichael
changed the title
|
@slopcop The PR description has been rewritten to lead with the user-facing benefits and benchmark results, with the detailed lifecycle, validation, and raw artifacts kept in expandable sections. |
|
🚨 SLOP COP 🚨 · I am the Slop Cop, and I am reviewing this change now. I will check security, code quality, performance, architecture, and the main user flow. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| runEventLoopWorkSync("sweep:retained-event-output-expiry:delete", () => | ||
| deleteExpiredRetainedEventOutputs(deps.db, { | ||
| expiredAtOrBefore: now, | ||
| limit: RETAINED_EVENT_OUTPUT_EXPIRY_BATCH_SIZE, |
There was a problem hiding this comment.
🚨 slopcop/review — The expiry worker removes only one full output each ten-second sweep.
That permits 8,640 removals each day. A faster write rate creates an endless backlog. Expired outputs can remain as plaintext past seven days. Please use a bounded row or time budget, and yield between delete groups.
There was a problem hiding this comment.
Fixed in 28f2efe. The 10-second expiry sweep now performs up to 256 one-row deletes, yielding with setImmediate after every successful delete and stopping early when empty. The regression seeds two expired sidecars and observes both deletion plus the intermediate one-row state between yields; it failed before this fix and now passes.
| }); | ||
| const hydratedEventRows = | ||
| detailsInlineOutputLimit === null | ||
| ? hydrateRetainedEventOutputRows(db, eventRowsWithBackgroundTaskState) |
There was a problem hiding this comment.
🚨 slopcop/review — This byte check occurs after the code loads every full sidecar.
The earlier floor sees only previews. One large result can cause an unlimited database read, JSON parse, and JSON write before fallback. Please query lengths first, then hydrate only values that fit the 4 MiB budget.
There was a problem hiding this comment.
Fixed in 28f2efe. Bounded timeline detail hydration now preflights sidecar byte metadata before selecting values. If raw bytes already exceed the 4 MiB budget it returns previews without materializing the full strings; otherwise it checks JSON-escaped size and hydrates only when the completed response fits. The new 5 MiB regression stays previewed at a 4 MiB budget and hydrates exactly at 8 MiB.
| json_type(data, ?) = 'text' | ||
| AND json_type(data, ?) IS NULL | ||
| AND json_extract(data, ?) = ? | ||
| AND length(json_extract(data, ?)) > ? |
There was a problem hiding this comment.
🚨 slopcop/review — This migration size check does not match the new-write size check.
SQLite counts Unicode code points. JavaScript counts UTF-16 units. Thus, 20,000 emoji count as 20,000 here and 40,000 in JavaScript. New writes retain this output, but the legacy migration skips it forever. Please use one unit and add an astral Unicode test.
There was a problem hiding this comment.
Fixed in 28f2efe. The SQL predicate is now only a cheap UTF-8-byte candidate filter; prepareCompletedEventOutputData remains the canonical JavaScript UTF-16 threshold check. False-positive multibyte candidates advance the cursor safely. A 20,000-emoji value (40,000 JS UTF-16 units) now migrates and hydrates byte-identically.
| let migratedBytes = 0; | ||
| let migratedRows = 0; | ||
| const startedAt = performance.now(); | ||
| while (migratedRows < expectedRows) { |
There was a problem hiding this comment.
🚨 slopcop/review — This loop measures continuous advances, not the production drain time.
Production stops after 64 advances and waits for the next ten-second sweep. Therefore, 4,000 rows need at least 63 sweeps, or approximately 10.5 minutes. The PR reports 4.6–5.5 seconds. Please benchmark the real schedule or correct the stated drain time.
There was a problem hiding this comment.
Fixed in 28f2efe. The harness now reports both continuous active work and a production projection using the actual 10-second cadence, one foundation call versus at most 64 bounded advances per sweep. It also adds four 4 MiB single-row paths. For 4,000 rows / 147 MB, bounded migration uses about 5.1 s active work but about 630 s from startup; standard idle p95 occupancy is 69.779 -> 0.378 ms and the 4 MiB-row p95 is 23.347 -> 4.308 ms. Raw artifacts and the corrected comparison are linked in the PR.
| return { inserted: false }; | ||
| } | ||
| if (prepared.retainedOutput !== null) { | ||
| insertPreparedRetainedEventOutput(db, { |
There was a problem hiding this comment.
🚨 slopcop/review — This helper can save the preview without the retained full output.
insertEvents accepts a plain connection. If this second write fails, the preview remains. A retry ignores the existing event and never restores the sidecar. Please make both writes atomic, or require a transaction. Add a failure test for the sidecar write.
There was a problem hiding this comment.
Fixed in 28f2efe. insertEvents now accepts DbConnection and wraps the complete event batch plus retained-sidecar inserts in one immediate transaction; notification happens only after commit. The regression installs a real SQLite BEFORE INSERT trigger that rejects the sidecar insert and verifies that no preview event is left behind. It failed before the transaction and passes now.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: This PR moves large completed outputs into a side table for seven days. Normal timeline reads receive small previews. Explicit reads can still receive the full output. A background job converts old rows in small steps.
I found five material issues.
- High — Expiry cannot keep pace. The worker removes one output every ten seconds. It can remove only 8,640 rows each day. A faster write rate keeps plaintext outputs past seven days and grows the database.
- High — The 4 MiB guard runs too late. Detail reads load and rebuild all full values before the size check. One very large result can block the server or exhaust memory.
- Medium — Unicode size rules differ. SQLite counts code points, but JavaScript counts UTF-16 units. The migration can skip legacy emoji-heavy outputs that new writes retain.
- Medium — The drain benchmark does not model production. It runs advances continuously. Production runs 64 advances every ten seconds. A 4,000-row backlog needs at least 10.5 minutes, not 4.6–5.5 seconds. The 36 KiB fixture also does not test a very large single row.
- Medium — One exported write path is not atomic.
insertEventscan save a preview before a sidecar write fails. A retry ignores the event and cannot restore the full output.
The architecture scan found a smaller maintenance issue. The output paths exist in both retained-event-outputs.ts and event-output-truncation.ts. The retention module and sweep module also import each other. Move the policy constants and target contract into retained-event-output.ts. Derive both path lists from that contract.
Verification completed:
- The full
@bb/dbsuite passed: 455 tests. - The five focused server files passed: 55 tests.
- The
@bb/dband@bb/serverTurbo typechecks passed. - The local app loaded through doobie. The project view and prompt interface worked without console errors.
- The current GitHub checks are green.
The core design is useful, but these limits need correction before the retention promise is reliable.
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
28f2efe to
193561b
Compare
|
@slopcop Follow-up: current |
| if (cursor.lastCreatedAt === 0 && cursor.lastEventId === "") { | ||
| return emptyCompletedEventOutputMigrationResult("idle", 0); | ||
| } | ||
| advanceCompletedEventOutputMigrationCursor(db, { |
There was a problem hiding this comment.
🚨 slopcop/review — A completed migration restarts forever.
This reset makes the next 10-second sweep scan from the first event again.
After migration completes, each sweep can still run 64 scans of 250 rows and evaluate JSON conditions.
Please store a completed-pass state and reduce later scans.
Reset that state after an import or a policy change.
There was a problem hiding this comment.
Fixed in b73deba. A full cursor pass now persists an explicit completed state; ordinary 10-second sweeps then return with zero scanned rows. A bounded daily wrap catches out-of-band/imported legacy rows inserted behind the cursor, and a cursor-version change restarts immediately. The regression covers completion, a no-scan follow-up, and discovery of a behind-cursor row after the rescan interval.
|
|
||
| const expiresAt = args.createdAt + COMPLETED_EVENT_OUTPUT_RETENTION_MS; | ||
| const truncation = isJsonObject(existingTruncation) ? existingTruncation : {}; | ||
| item[target.outputPath] = truncateOutput(value); |
There was a problem hiding this comment.
🚨 slopcop/review — The output size becomes incorrect for retained values.
This assignment replaces a 50,000-character output with a 4,173-character storage preview.
The timeline preview then uses row.output.length as outputPreview.totalChars.
The UI can report 4,173 characters while the retained detail contains 50,000 characters.
Please carry truncation.originalLength into the timeline row and use it for totalChars.
There was a problem hiding this comment.
Fixed in b73deba. Timeline projection now derives the authoritative original output length from the completed-event truncation metadata and preserves it when applying an additional timeline preview. The normal and detail timeline regressions use a 5 MiB retained value and assert bounded preview text plus the full original totalChars.
| } | ||
| const remainingDataBytes = maxDataBytes - storedDataBytes; | ||
| if ( | ||
| retainedOutputSizeTotal(rawSizes, rowCountsByEventId) > remainingDataBytes |
There was a problem hiding this comment.
🚨 slopcop/review — The byte check rejects retained output that fits the limit.
The check adds the retained value size to storedDataBytes.
It does not subtract the storage preview that hydration replaces.
I reproduced this with a 40,000-character retained value.
The hydrated rows used 45,489 bytes, but this function returned the 9,805-byte previews at that exact limit.
Please calculate the projected hydrated JSON size before the decision.
There was a problem hiding this comment.
Fixed in b73deba. The preflight now computes the exact projected serialized JSON byte count: it removes the stored preview and truncation metadata from the small event template, then adds SQLite octet_length(json_quote(value)) metadata for the retained string. It still does not select/materialize the full value before acceptance. The regression proves an exact-limit response hydrates and a limit one byte smaller stays a preview.
| if (!result.threadId) { | ||
| throw new Error("Migrated completed output has no thread"); | ||
| } | ||
| deps.hub.notifyThread(result.threadId, ["history-rewritten"]); |
There was a problem hiding this comment.
🚨 slopcop/review — The migration can cause repeated client refreshes.
Each migrated row sends an immediate history-rewritten message.
A sweep can send 64 messages, and one thread can receive many messages.
The client handles this change immediately and invalidates several queries.
Please collect changed thread IDs and send one message for each thread after the sweep.
There was a problem hiding this comment.
Fixed in b73deba. The periodic sweep now collects affected thread IDs in a Set and emits cache/history invalidation once per thread in finally, so a later advance failure cannot lose invalidation. The regression migrates two rows belonging to one thread and now expects exactly one history-rewritten notification.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
This PR moves large completed outputs into a second database table. Normal reads keep a short preview. Detail reads keep full text for seven days. A background job converts old rows in small steps.
I found four medium issues:
- The timeline can report the storage preview length as the full output length.
- The detail byte check can reject a full output that fits its limit.
- The migration restarts a complete scan every ten seconds forever.
- The migration can send many immediate refresh messages for one thread.
I left line comments with the evidence and proposed corrections.
The security review found no access, privacy, retention, or transaction defect. The central target list also removes path duplication cleanly. I found no other required architecture refactor.
The database suite passed 458 tests. The focused server suites passed 36 tests. The security checks passed 13 focused database tests.
I also tested the real application with Doobie. A 40,000-character command stored a 4.8 KB preview and a full retained value. The detail view restored all 40,000 characters.
GitHub currently reports a merge conflict with main.
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
| types: args.types, | ||
| }); | ||
| return rows.map((row) => parseStoredEventRow(row)); | ||
| return hydrateRetainedEventOutputRows(db, rows).map((row) => |
There was a problem hiding this comment.
🚨 slopcop/review — This raw read has no output byte limit.
The route accepts any numeric row limit. This line loads every full sidecar before JSON serialization.
Twenty 1 MiB outputs took 286 ms locally. One request can use unbounded time and memory.
Please set a row limit and a total-byte limit, or stream bounded pages.
There was a problem hiding this comment.
Fixed in 6aba4c1. The raw event-list route now clamps pages to 100 rows and performs the exact projected hydrated-data byte preflight before selecting any full sidecar value. Pages over 8 MiB return 413; callers can use the existing sequence/limit pagination, and a smaller page still returns the byte-identical retained value. The same byte guard protects /events/wait. The in-memory integration regressions cover a requested 1,000-row page being capped at 100 and a 9 MiB retained page returning 413 while a one-row page hydrates exactly; both failed before this fix and now pass.
193561b to
b73deba
Compare
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary
This change moves large completed outputs into a separate database table for seven days.
Normal timeline pages keep small previews. A background job converts old database rows in small steps.
Findings
I found five medium issues. I added one new inline comment and did not repeat four open comments.
- Raw event reads can load unlimited full outputs into memory.
- Timeline previews report the storage preview length instead of the full output length.
- The detail byte check rejects some full outputs that fit the response limit.
- The migration cursor restarts after each complete pass and scans all completed history forever.
- Migration notices can cause 64 immediate client refreshes for one thread in one sweep.
I found no security defect. The central output-target contract also removes the old path-list duplication.
Verification
- The database suite passed all 460 tests.
- The database and server typechecks passed.
- All current GitHub checks passed.
- A browser check returned the exact 50,026-character retained output without storage-only truncation data.
git diff --checkpassed.
Please address the five findings before merge.
|
@slopcop The four follow-up findings are addressed on |
b73deba to
6aba4c1
Compare
|
@slopcop The additional raw-read finding is now addressed on |
|
|
||
| function truncateOutput(value: string): string { | ||
| return ( | ||
| value.slice(0, COMPLETED_EVENT_OUTPUT_RETAINED_HEAD_CHARS) + |
There was a problem hiding this comment.
🚨 slopcop/review — Preserve Unicode characters at both preview boundaries
slice() works on UTF-16 code units. A boundary can divide an emoji or another astral character. The stored preview then contains an unpaired surrogate. After retention expires, UTF-8 output replaces that character with U+FFFD permanently. I reproduced this with 2,047 ASCII characters followed by 😀; value.slice(0, 2048).isWellFormed() returned false.
Please move each boundary when it falls between a high and low surrogate. Add tests for both the head and tail boundaries.
There was a problem hiding this comment.
Fixed in 9e58d94. The retained preview now detects a head or tail boundary between a UTF-16 high/low surrogate pair and moves that boundary inward, so the preview never introduces an unpaired surrogate. The in-memory SQLite regression places an emoji across each boundary and verifies both stored previews remain well formed after round-tripping through SQLite; it failed before this fix and passes now.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary
This change moves large completed outputs from busy event rows into a separate table for seven days.
Normal timeline pages use small previews. Detail and raw reads can restore the full text while it remains available.
A background job moves old rows in small steps, which reduces server pauses.
Findings
I found six open issues. Four already had open SlopCop comments, and I added two new comments.
- High:
bb thread log --json --allstops after 100 events. The server caps a page at 100 rows, but the CLI expects 1,000 rows. - Medium: The migration reads overlapping 250-row windows. A focused benchmark scanned 125,500 rows to move 1,000 rows.
- Medium: The second timeline preview can split an emoji. The result contains an unpaired UTF-16 surrogate and can show a broken character.
- Medium: The interface reports expired output as too large. After retention ends, the output does not exist and cannot load.
- Medium: The Unicode boundary fix stores incorrect preview lengths. A direct check reported 2,048 units when the stored head had 2,047 units.
- Low: The benchmark artifact records private machine details. It includes the host name and the absolute repository path.
Architecture
The shared output-target contract removes the old path duplication. I found no need for a larger storage refactor.
The two preview functions should share one safe UTF-16 boundary helper. This change would prevent the current second-preview defect and later drift.
The benchmark also repeats the production cadence and advance limit. Inputs or one shared source would keep its projections accurate.
Verification
- A browser test ran a real Codex command with 40,000 output characters.
- The database stored a 6,843-character preview and a sidecar.
- The expanded interface restored all 40,000 characters without a preview notice.
- The CLI test used 162 events and returned only sequences 1 through 100.
- The focused database checks passed 15 of 15 tests and 40 of 40 tests.
- The 4 MiB migration benchmark measured approximately 4.9 ms at p95 for one synchronous step.
- All GitHub checks pass, and GitHub reports that the pull request can merge.
The security review found no access, injection, cross-thread, expiry, path, or transaction defect.
I recommend corrections for the high and medium issues before merge.
4e71ff6 to
1eba401
Compare
|
@slopcop All six findings from the latest review are addressed on head |
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request now. I will check security, code quality, architecture, performance, and the main user flow. |
| afterSeq: parseOptionalInteger(query.afterSeq, "afterSeq"), | ||
| beforeSeq: parseOptionalInteger(query.beforeSeq, "beforeSeq"), | ||
| limit: parseOptionalInteger(query.limit, "limit") ?? 100, | ||
| limit: parseBoundedPositiveOptionalInteger({ |
There was a problem hiding this comment.
🚨 slopcop/review — Do not silently cap the public event list
This route previously accepted a larger limit.
The response is an array without page data, and the contract still accepts any positive integer.
A client can request 1,000 rows, receive 100 rows, and incorrectly stop because the result appears complete.
Reject values above 100, or add page data that tells clients to continue.
There was a problem hiding this comment.
Fixed in f89baf1. The public query schema now rejects limit values above the shared 100-row page size with HTTP 400, so callers never mistake a silently truncated response for the final page. The CLI continues to paginate 100-row requests when all events are requested.
| const previews = retainedOutputPreviewsByCallId( | ||
| events, | ||
| availablePreview, | ||
| Date.now(), |
There was a problem hiding this comment.
🚨 slopcop/review — Do not cache a time-based retention state without an expiry
Availability uses Date.now() during the build, but the cache key does not include time.
The expiry sweep deletes sidecars without a thread notification.
An unchanged thread can show available after the retained output expires.
Add a cache expiry, compute availability after cache access, or notify affected threads during deletion.
There was a problem hiding this comment.
Fixed in f89baf1. Expiry selection now returns the affected thread IDs, and the sweep coalesces a history-rewritten notification per thread after deletion, including on the error path. That invalidates timeline and projection caches when time-based full-output availability changes.
| if (!target) { | ||
| throw new Error("Expected completed output migration target"); | ||
| } | ||
| const result = runEventLoopWorkSync( |
There was a problem hiding this comment.
🚨 slopcop/review — Move unlimited output work off the server event loop
This synchronous frame parses, serializes, and writes one output with no byte limit.
The included benchmark used 16 MiB outputs.
It measured 146.9 ms median delay and 366.1 ms maximum timer delay.
The row limit cannot limit work for one large row.
Use a worker or a byte limit that prevents a long server stall.
There was a problem hiding this comment.
Fixed in f89baf1 with a byte bound while preserving the explicit no-worker and main-loop constraint. Candidate selection short-circuits before JSON parsing when serialized event data exceeds 8 MiB; that legacy row remains byte-identical inline, the cursor advances past it, and later bounded rows still migrate. The new-write sidecar path is unchanged. A focused 8 MiB regression covers skip-and-continue behavior.
| } | ||
| const state = getCompletedEventOutputScanState(db, args); | ||
| const cursor = state.cursor; | ||
| if (cursor.lastCreatedAt === COMPLETED_EVENT_OUTPUT_MIGRATION_COMPLETED_AT) { |
There was a problem hiding this comment.
🚨 slopcop/review — Avoid a full history rescan every day
After completion, this code resets each target cursor to the first event every 24 hours.
New events already create sidecars when the server writes them.
One million completed rows need at least 625 seconds at the current scan limit.
Keep a permanent migration cursor, or start a bounded rescan only after an import.
There was a problem hiding this comment.
Fixed in f89baf1. Completion is now permanent for the cursor version; the daily wrap was removed, so an already-drained database does zero history scans on later sweeps. A future import path must deliberately bump or restart the cursor or invoke explicit maintenance rather than imposing a permanent daily rescan on every database.
| updatedAt: args.migratedAt, | ||
| } | ||
| : window; | ||
| const scanRows = |
There was a problem hiding this comment.
🚨 slopcop/review — Remove the unused production count query
This COUNT runs on each advance to fill scanRows.
The server does not read scanRows.
The benchmark saw 12,000 statements for 2,000 migrated rows, and this query supplied 2,000 statements.
Remove the field from production, or collect it through benchmark instrumentation.
There was a problem hiding this comment.
Fixed in f89baf1. The per-advance count query and its types were removed. The scanRows metric now records only rows actually fetched when opening a persisted 250-row window and is zero while reusing that window. In the refreshed 4,000-row drain this removed 3,984 statements, from 24,000 to 20,016, while scan rows remained 4,000.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary
This PR changes how BB stores large output from completed commands and tools.
BB keeps a short preview in the event row. It keeps the full output in a separate row for seven days.
A small periodic job moves old event rows to this format. The UI and CLI can then request the full output when necessary.
Review results
I found five issues:
- The event route silently reduces larger client limits to 100 rows. The response has no page data.
- The timeline cache can show full output as available after the retention period ends.
- One 16 MiB legacy output can block the server event loop for hundreds of milliseconds.
- Each completed migration starts a full history scan again after 24 hours.
- Each migration step runs a count query that the server does not use.
I left one inline comment for each issue. I recommend that the author resolve them before merge.
Architecture and security
I found no authorization bypass, cross-thread lookup, SQL injection risk, or broken transaction boundary.
I found no useful duplicate code to remove. The new UTF-16 helper has different rules from the existing byte-window helper.
Checks
- The database suite passed all 463 tests.
- The changed application test passed all 6 tests.
- The focused server run passed 139 of 140 tests. The one time-based assertion passed its isolated rerun.
- The affected timeline tests passed their focused rerun after they exceeded limits in the full concurrent run.
- Type checks passed for the database, server, application, and CLI packages.
- A live doobie check created a thread with 40,000 output characters.
- The database stored a 4,870-character preview and the full retained value.
- The UI loaded and displayed the complete output after the row opened.
- A 16 MiB benchmark measured 146.9 ms median delay and 366.1 ms maximum timer delay.
I posted a comment review. I did not use an approval or a change-request state.
f89baf1 to
c9ed7ed
Compare
ymichael
changed the title
76aa2ab to
07fa5fd
Compare
|
@slopcop The PR has been rebased onto current main, the two Drizzle migrations have been regenerated as a single migration, and the rebased CI is green. Ready for another look. |
|
🚨 SLOP COP 🚨 · I’m reviewing this now. I’m checking security, general code quality and architecture, performance, and the end-to-end behavior in parallel, then I’ll leave one consolidated review with the findings. |
| outputPath: RetainedEventOutputPath, | ||
| ): number { | ||
| const payload: unknown = JSON.parse(data); | ||
| if (!isJsonObject(payload) || !isJsonObject(payload.item)) { |
There was a problem hiding this comment.
🚨 slopcop/review — High: migrated legacy image sidecars crash bounded hydration
prepareLegacyImageGenerationOutputData stores the hydratable item under rawEvent.params.item, but this byte preflight only accepts payload.item. I reproduced a valid legacy image migration followed by canHydrateRetainedEventOutputRowsWithinDataByteLimit() throwing Retained output event payload is not an item object. The raw events route and turn-detail reads call this preflight, so affected threads return HTTP 500 until the retained value expires. Please pass the row into this calculation and reuse hydratableOutputItem(row, payload), then cover both the bounded-hydration helper and public routes with a migrated legacy envelope.
| ...row, | ||
| output: buildTimelineOutputPreview(row.output), | ||
| outputPreview: { totalChars: row.output.length }, | ||
| outputPreview: row.outputPreview ?? { |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: the new output-availability state is omitted from row memo signatures
This makes outputPreview.experimental_fullOutputAvailability user-visible state, but command/tool render signatures still omit both it and totalChars. A history-rewritten refetch after migration or expiry can therefore return a new row at the same sequence whose signature compares equal, causing the memoized component to keep the old availability/detail state (including a previously loaded full value after retention expiry). Please add the preview fields to the command/tool signatures and cover the available → retention-expired transition in the signature/memo tests.
| const page = await sdk.threads.events.list({ | ||
| threadId, | ||
| limit: String(THREAD_LOG_ALL_EVENTS_PAGE_SIZE), | ||
| limit: String(THREAD_EVENT_LIST_PAGE_SIZE), |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: bb thread log --all cannot page output-heavy histories
The raw event endpoint now intentionally returns event_data_too_large when one hydrated page exceeds 8 MiB, but --all always asks for 100 rows and has no retry with a smaller page. I reproduced an output-heavy thread returning 413 for limit=100, after which bb thread log --all --json aborts instead of printing the thread. Please make this path byte-aware (for example, catch the typed error and reduce the page size until it fits) and add a CLI regression with several retained outputs totaling more than 8 MiB.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: this PR moves large command, tool, search, fetch, and generated-image results out of the normal timeline record. The timeline keeps a small preview so everyday reads stay fast, while explicit detail/raw reads can recover the full value for seven days. It also teaches the app to show generated-image activity and incrementally migrates older databases without one long event-loop stall.
I found three issues that should be fixed before merge:
- High — migrated legacy image outputs can make reads return 500. The migration stores old Codex image results under
rawEvent.params.item, but the response-size preflight only understandspayload.item. A valid migrated row therefore throws before hydration on raw-event and turn-detail paths. This is also a low-severity request-level availability/security issue because one provider-produced legacy image can keep those reads broken until retention expiry. - Medium — retention changes can leave stale output UI mounted. Command and tool render signatures do not include the new full-output availability or character count. After a history rewrite, React can treat
availableandretention-expiredrows as identical and retain stale details. - Medium —
bb thread log --alldoes not honor the new byte-bounded pagination contract. It always requests 100 raw events. If retained outputs push that page over 8 MiB, the endpoint returns 413 and the CLI aborts instead of retrying with a smaller page.
Architecture note: the central retained-output target table is a good consolidation. The two incremental migration drivers in packages/db/src/data/sweeps.ts now duplicate most of their cursor/window/update machinery; a shared internal driver with strategy-specific selectors/preparers would reduce drift, but I consider that optional once the correctness issues above are covered.
Performance review found no additional regressions. The partial indexes, bounded response hydration, cursor windows, expiry index, and yields are all exercised; a local migration benchmark measured 36 KiB rows at 0.40 ms p50 / 0.96 ms p95 synchronous work per advance and 1 MiB rows at 1.31 ms p50 (2.60 ms max).
Verification on 07fa5fd0: @bb/db passed 469/469, @bb/client-core 253/253, @bb/cli 530/530, and 52 focused server tests passed. A full server run passed 2,214 tests with three unrelated five-second timeouts. In the live dev app, I created a real thread, inserted a 4 MiB generated-image completion through the DB data layer, confirmed the stored event shrank to 4,531 bytes with the exact 4,194,328-byte value retained in the sidecar, and used Chrome/doobie to expand the turn. The UI rendered a compact non-expandable Generated image row and did not put the encoded image prefix in the DOM. I separately reproduced the 8 MiB raw-page failure and the CLI abort.
1 participant
Human comments
What was wrong
Large completed outputs lived directly in
events.data, so ordinary timeline reads loaded and parsed megabytes that the UI did not need. Image-generation completions were worse: they were stored as unhandled provider envelopes with multi-megabyte encoded results, so a useful “Generated image” event could exceed the 4 MiB timeline limit and render as an unexplained generic placeholder. Existing databases also removed inline output in one bulk synchronous sweep, which could stall the Node event loop.What changed
User-facing result
setImmediateyield. Cursor/window progress survives restart.Output lifecycle
events.data. The exact JSON-encoded string goes intoretained_event_outputs.event.createdAt + 7 days. The preview then becomes authoritative and sidecars are deleted in bounded, yielding sweeps.The authoritative output contract now has five pairs:
commandExecution.aggregatedOutput,imageGeneration.result,toolCall.result,webFetch.resultText, andwebSearch.resultText. The existing four pairs and all sidecar semantics are unchanged; no caller redeclares the list.Generated-image boundary
imageGenerationitems into the existing provider-bridge delta union.imageGenerationitem. The provider-bridge envelope remains protocol version 2 because this is additive vocabulary; its grammar snapshot and protocol documentation are updated.provider/unhandleditem/completedimage-generation envelope shape, preserves that envelope, and sidecarizes only its nestedresult.0113_puzzling_black_knight.sqlcreates the sidecar table and expiry index and adds the partial(created_at, id)index used by legacy image scans. It was generated once from the final combined schema.Everything remains on the Node main event loop; this PR adds no worker.
What the measurements say
For the original four output paths, bounded migration trades total drain time for responsiveness. On 36 KiB rows, p95 synchronous occupancy fell from 110.23 ms to 1.59 ms idle and from 247.96 ms to 2.47 ms under controlled CPU load. A 4,000-row / 147 MB backlog takes about 10.5 minutes on production cadence, with a yield between rows.
The generated-image benchmark matches the observed production population: 39 rows across 5 threads, including four 4,555,236-character results, for 56,720,944 result bytes. The exact parent cannot drain this backlog. HEAD drains it in 40 advances (39 migrations plus cursor completion):
Each image run migrates 39 rows / 56,720,944 bytes, scans 39 rows, executes 239 statements, and performs 40 ticks / 40 yields. The largest stored event falls from 4,555,638 bytes to 4,713 bytes. The database grows from 57,524,224 to 57,831,424 bytes while retained values coexist with previews (+307,200 bytes, about 0.53%). Expected and hydrated full-result SHA-256 are identical:
825d4731616b8bb2a9caae08316fc48f95eb7846cc692dce062a428bbeb25d89.Benchmark baselines and raw artifacts
New-write/read sidecar benchmark
BEFORE is inline baseline
eeaaa3e8db7b3aeb3c4ab46873816c84cb6ea513; AFTER is sidecar runtimeb78bf6038f15c081c8274f3e8141ccc6afa8f5b2. Foundation head259f937f7b327ea73f56b69ace6ed35645c8045eonly adds the target export and has the same runtime/schema behavior. Same harness, Apple M4 Max, Node v22.23.1, 12 warmups + 60 read iterations, 8 warmups + 40 write iterations.Artifacts: comparison, inline JSON, sidecar JSON, harness, SHA-256 manifest.
Original four-path legacy migration benchmark
BEFORE is foundation
259f937f7b327ea73f56b69ace6ed35645c8045e; AFTER is migration runtimef89baf1d0384d3f59cba5e655a1a65d083160a51. Same harness, two warmups, 10 measured iterations, Apple M4 Max, Node v22.23.1. CPU runs add one continuously busy child; migration stays on the main loop.Every 4,000-row drain migrated 147,456,000 bytes. Persisted windows reduced scan rows from 875,500 to 4,000 (99.5%). Expected, hydrated, and raw-event hashes match.
Artifacts: comparison, BEFORE idle, AFTER idle, BEFORE CPU, AFTER CPU, SHA-256 manifest.
Legacy generated-image benchmark
BEFORE is exact pre-extension head
c9ed7ed4a4f1d88a1cca1b26bfac54a06e5c4c18; AFTER runtime iscf98aab80691b9d39dc908707fa063618314d6f7. The rebased runtime patch is range-diff equivalent; only the host protocol number and generated migration packaging changed. Same harness and 39-row dataset, two warmups, 10 measured iterations per load condition.Artifacts: comparison, harness, BEFORE idle, BEFORE CPU, AFTER idle, AFTER CPU, SHA-256 manifest.
Exact combined file list (70 files)
Residual tradeoffs
How you verified
Focused regressions use real in-memory SQLite without mocks and fail on the relevant parent behavior:
Generated imagerow without exposing its result or showing the generic oversized placeholder;Validation on rebased head
07fa5fd0ea:@bb/db: 469/469 tests passed, including migration replay, snapshot-chain, query-plan, cursor, and sidecar regressions;origin/main's0112state;mainremoved that inventory.Fixes: no linked issue.