perf(daemon): redesign spaceTaskMessages.byTask.compact on conversation turns [#2338] - #2361
perf(daemon): redesign spaceTaskMessages.byTask.compact on conversation turns [#2338]#2361lsm wants to merge 2 commits into
Conversation
…on turns [#2338] Replace the result-bounded turn model (6 window passes over every task message + a dead turnUserMessageId forward-fill; ~1.3s warm on a 46k-message task) with a materialized, global-per-task conversation_turn_index. - Migration 170: add conversation_turn_index (stored, backfilled once via temp table + UPDATE…FROM) and idx_sdk_messages_task_turn. Maintained at insert — an anchor (message_type='user' AND is_renderable=1) opens a new turn (MAX+1), others inherit the current turn. Rewind-safe: rewind deletes-the-future then appends, so MAX-over-survivors self-corrects. - Rewrite the compact feed + active roster on a new conversation-turn base CTE that reads the column and pushes a recent-100-turn cap into the sdk_messages scan. Per segment the compact feed keeps the anchor + a summary (assistant text → thinking → last 3 tool_use) + the result; the active roster is the conversation turn holding the session's most recent agent row (running iff that row is not a result). - Fixes the mid-turn-user-message swallow bug: every renderable user message is now an always-visible conversation anchor. Measured on a 46.2k-message task (reproduces the issue's 633ms base-CTE cost): compact 1.3s → 23ms, active-turn 0.8s → 20ms — both well under the 100ms target. Legacy byTask/full + its base CTE are left untouched. Design + benchmark evidence: docs/design/2338-compact-conversation-turns.md. Compact/active-turn tests rewritten to the new semantics.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES — CI is red (P0, blocking). Two P2 behavior/doc items below.
P0 — Red CI: 5 runtime test files have an inline sdk_messages schema missing conversation_turn_index
This PR adds conversation_turn_index to the INSERT in SDKMessageRepository.saveSDKMessage / saveUserMessage / saveHyperNeoActionMessage, but five test files build their sdk_messages table with a hand-written inline CREATE TABLE that omits the new column:
tests/unit/5-space/runtime/prompt-too-long-recovery.test.ts:46tests/unit/5-space/runtime/prompt-too-long-replay.test.tstests/unit/5-space/runtime/space-runtime-stalled-recovery.test.tstests/unit/5-space/runtime/space-runtime-tick-loop.test.tstests/unit/5-space/runtime/space-runtime.test.ts
Each calls runMigrations(db) before CREATE TABLE IF NOT EXISTS sdk_messages (...). Migration 170's tableExists('sdk_messages') guard therefore no-ops (the table doesn't exist yet at migration time), so the column is never added — and the inline CREATE doesn't include it either. Every repository INSERT then throws SqliteError: table sdk_messages has no column named conversation_turn_index, the overflow messages never persist, and /compact is never injected → 21+ assertions fail across the five files. Both Daemon Unit Tests (5-space-runtime-a) and the All Tests Pass gate are red on this PR's head (74225e43).
The PR updated space-test-db.ts, hidden-subtype-idle-detection.test.ts, and sdk-message-repository.test.ts but missed these five. Same inline-schema pattern, latent (didn't fail only because consumers mock the repo): tests/unit/helpers/space-agent-schema.ts:142.
Fix: add conversation_turn_index INTEGER to each inline sdk_messages schema — or, better, point these fixtures at createTables/createSpaceTables so they can't drift on the next schema change.
P2 — GitHub PR-activity rows are dropped from the compact feed (behavior change)
The new compact selected_ids selects only user-anchors / terminal results / non-hook system rows / per-segment assistant summaries. GitHub-event rows (messageType='github_pr_activity', kind='github') match none of these branches, so they are now excluded from spaceTaskMessages.byTask.compact entirely. Legacy compact could surface them via the non-terminal tail. If github activity should appear inline in the compact task thread, add a selected_ids branch for kind='github'; otherwise confirm it's intended (it still appears in the activity panel + the full byTask feed). See anchored comment.
P2 — "load-more param" is documented but not implemented (doc/code mismatch)
The design doc and the SQL doc-comments (here + line 2205) promise "older turns load via the load-more param," but the compact query is registered paramCount: 1 (taskId only) and the recent-turn window is hardcoded to SPACE_TASK_MESSAGES_COMPACT_RECENT_TURNS = 100. There is no load-more parameter, so on tasks with >100 conversation turns (e.g. the 42k-message task in the issue) older turns — including early system:init rows — are silently unreachable in compact (the full view still returns them). Either implement the param or correct the doc to state the cap.
What looks good
- Anchor definition is consistent across the migration backfill, insert-time
resolveConversationTurnIndex, and the query predicate (message_type='user' AND is_renderable=1). VerifiedcomputeIsRenderablereturns 0 for anyuserrow containing atool_resultblock, so tool_result echoes correctly do not start turns. - Dropping
turnUserMessageId/turnHiddenMessageCountfrom compact is frontend-safe.useTurnBlocksgroups by session-switch + result-boundary (turn blockid = firstMsgUuid ?? sessionId-startTime, notturnId);MinimalThreadFeedmatches the active roster on(sessionId, turnIndex)— still emitted; andturnHiddenMessageCountis not rendered anywhere in non-test frontend code. - Migration 170 backfill is correct: turn 0 for pre-anchor rows, per-task partitioning, NULL for no-task rows, idempotent, and the sargable index is asserted by its own test.
Once the P0 schema drift is fixed and CI is green, the two P2s can be addressed or consciously deferred.
… load-more doc Review feedback on PR #2361: - P0 (red CI): add conversation_turn_index to the hand-written inline sdk_messages schemas in the 5 runtime test files + helpers (space-agent-schema). They run runMigrations before a CREATE TABLE IF NOT EXISTS sdk_messages, so migration 170's column was never added and every repo INSERT threw "no column named conversation_turn_index" — overflow messages never persisted, /compact never injected (21+ failures). - P2: surface GitHub activity rows in byTask.compact again. selected_ids had no branch matching kind='github', so PR/review/CI events vanished from the compact feed (legacy compact surfaced them via the non-terminal tail, and mapRow + the frontend still classify kind='github'). Add the branch; rows sit outside the turn model (turnIndex NULL) but are sparse and state-filtered. - P2: the load-more param was documented (constant docblock, compact docblock, design doc §Performance) but never implemented (paramCount stays 1, recent window hardcoded to 100). Correct the docs to state the cap; older history is reachable via the unbounded byTask (full) feed. Tests: new github-in-compact case; full runtime + migration-170 suites green.
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE — all three prior findings addressed and verified; CI fully green (0 failures).
Round-1 findings — resolved
- P0 (red CI — missing
conversation_turn_indexin inline test schemas): fixed. The column was added to all 14 test files that hand-write asdk_messagesschema +helpers/space-agent-schema.ts(not just the 5 that were failing — the latent-risk files too).Daemon Unit Tests (5-space-runtime-a)now passes; the previously-throwing repo inserts no longer break overflow recovery. - P2 (GitHub events dropped from compact): fixed.
SELECT id FROM joined WHERE kind = 'github'added toselected_ids— github activity rows sit outside the turn model (turnIndexNULL, sparse/state-filtered) but are surfaced again, matching legacy behavior. New test asserts a routed PR event appears in compact withkind='github'/messageType='github_pr_activity'in chronological position. - P2 (load-more documented but unimplemented): fixed honestly — all three doc sites (constant docblock, compact SQL docblock, design doc §Performance) now state the 100-turn cap explicitly and point to the unbounded
byTask(full) feed as the drill-in surface, rather than claiming a param that was never wired.
CI status (head 5f7a9d953)
All required checks pass: 5-space-runtime-a, Lint/Knip/Format/Typecheck (so bun run check incl. db-schema-parity is clean), Web Tests, both migration shards, and the rest. Both inline review threads are resolved (0 unresolved).
One transient: 4-space-migrations-a initially failed on a single migration-34 test timing out (5000ms ceiling, ran 13s) — an unrelated older migration (status CHECK constraints) this PR doesn't touch, and the shard passed on the prior commit. Confirmed a flake: migration-34 + migration-170 pass locally (17/17, ~6s), and a rerun of the job went green (2m36s).
Still sound (re-verified this round)
Anchor definition consistent across backfill/insert/query; dropping turnUserMessageId/turnHiddenMessageCount is frontend-safe (useTurnBlocks groups by session/result-boundary, MinimalThreadFeed matches on turnIndex, hidden-count not rendered); migration 170 backfill correct + idempotent + sargable.
Ship it.
1 participant
Closes #2338.
What & why
Opening a heavy task thread (~42k messages) cost ~633 ms warm, CPU-bound: the compact feed ran 6 window passes over every task message to compute a result-bounded turn model, plus recomputed it on every 250 ms-debounced re-eval. It also swallowed user messages sent mid-turn (only the first per result-turn was pinned).
This replaces it with a materialized, global-per-task
conversation_turn_indexand a conversation-turn model.conversation_turn_index(stored, backfilled once) +idx_sdk_messages_task_turn. Maintained at insert — an anchor (message_type='user' AND is_renderable=1) opens a new turn; rewind-safe via the append-only invariant.byTask/full feed). Per segment the feed keeps anchor + summary (assistant text → thinking → last 3 tool_use) + result; GitHub activity rows (PR/review/CI) are kept alongside the thread, outside the turn model (noturnIndex, sparse + state-filtered); the roster is the conversation turn holding the session's most recent agent row.Design + benchmark evidence:
docs/design/2338-compact-conversation-turns.md.Measurement (46.2k-message task, reproduces the issue's 633 ms base-CTE cost)
Both under the 100 ms target. Legacy
byTask/full is left untouched.Notes for review
turnIndexviaSUM(isTerminal)window) and the deadturnUserMessageIdforward-fill are removed from the compact + active-turn paths (theid-vs-rowidtiebreak inconsistency and the forward-fill-over-tool_result bug go with them).byTask/full keeps the legacy base CTE.