Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms - #10874
Conversation
…rt storms Loading a session's messages runs `WHERE session_id = ? ORDER BY created_timestamp, id`, but no index covered `created_timestamp` — the only timestamp index is on a different column (`timestamp`). SQLite fell back to an on-disk TEMP B-TREE sort that dragged each row's `content_json` through temp files. With large tool responses stored inline this produced multi-GB write storms that exhausted macOS disk-write limits and stalled goose. Add a composite index serving both the per-session filter and the ordering, in the fresh-schema path and a new migration (v16). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fc81338 to
166b8a5
Compare
* main: docs: add tool shim guide covering when to enable, backends, and troubleshooting (aaif-goose#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (aaif-goose#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (aaif-goose#11015) fix(ui): support remote working directory for external backend (aaif-goose#10827) fix(acp): resume provider-native sessions (aaif-goose#10379) fix (UI): Fix Form/JSON toggle buttons invisible in dark mode on Deeplink Generator (aaif-goose#11077) fix(streaming): skip metadata-only SSE frames instead of failing the turn (aaif-goose#10942) fix(telegram): send responses as rich markdown (aaif-goose#11062) fix(acp): send session setup updates after new and fork session responses (aaif-goose#11018) fix(ui): remove duplicated brace-expansion keys that break every pnpm install (aaif-goose#11071)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58e6f704c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .execute(&mut *tx) | ||
| .await?; | ||
| sqlx::query( | ||
| "CREATE INDEX IF NOT EXISTS idx_messages_session_created ON messages(session_id, created_timestamp, id)", |
There was a problem hiding this comment.
Defer implementation until the issue is Ready
For this upstream change, the reviewed commit message explicitly says issue #11055 is “not yet triaged to Ready,” yet this line begins implementing it. Pause the implementation until the issue reaches Ready and its design, constraints, and verification plan are agreed; proceeding now violates the repository’s contribution workflow.
AGENTS.md reference: AGENTS.md:L9-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
it is in "Ready" status now
* origin/main: fix(conversation): sanitize nested tool responses (#10609) fix(hints): bound recursive file expansion (#10546) fix(providers): drop stale signed thinking blocks after a mid-conversation model switch (#10007) fix(desktop): clarify compact cost display (#11093) Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms (#10874) docs: add tool shim guide covering when to enable, backends, and troubleshooting (#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (#11015) fix(ui): support remote working directory for external backend (#10827)
* origin/main: fix(mcp): prune dead notification subscribers (#11032) chore: remove the extension and tool count suggestion (#10869) feat: compaction in the GDK (#11042) fix(provider): retry transient errors on first stream item before ending turn (#10968) feat(cli): add /new to start a fresh session without restarting (#10767) feat(acp): title new sessions from _meta.sessionTitle (#10712) fix: adjust rmcp::model::Meta ref (#11107) Skip hook loading and lifecycle events for subagents (#10596) Sanitize Unicode tags in Responses output (#10745) fix(conversation): sanitize nested tool responses (#10609) fix(hints): bound recursive file expansion (#10546) fix(providers): drop stale signed thinking blocks after a mid-conversation model switch (#10007) fix(desktop): clarify compact cost display (#11093) Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms (#10874) docs: add tool shim guide covering when to enable, backends, and troubleshooting (#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (#11015) fix(ui): support remote working directory for external backend (#10827)
* main: fix(mcp): prune dead notification subscribers (#11032) chore: remove the extension and tool count suggestion (#10869) feat: compaction in the GDK (#11042) fix(provider): retry transient errors on first stream item before ending turn (#10968) feat(cli): add /new to start a fresh session without restarting (#10767) feat(acp): title new sessions from _meta.sessionTitle (#10712) fix: adjust rmcp::model::Meta ref (#11107) Skip hook loading and lifecycle events for subagents (#10596) Sanitize Unicode tags in Responses output (#10745) fix(conversation): sanitize nested tool responses (#10609) fix(hints): bound recursive file expansion (#10546) fix(providers): drop stale signed thinking blocks after a mid-conversation model switch (#10007) fix(desktop): clarify compact cost display (#11093) Index messages by (session_id, created_timestamp, id) to stop on-disk sort storms (#10874) docs: add tool shim guide covering when to enable, backends, and troubleshooting (#10858) fix(deep-link): route extension/session deep links to regular windows not standalone app windows (#10908) fix(ui): raise chat input z-index so slash menu appears above loading indicator (#11015) fix(ui): support remote working directory for external backend (#10827)
Issue: #11055 — filed for this PR per the contribution workflow in
AGENTS.md; not yet triaged to Ready, so this PR is a proposal attached to it rather than agreed work.Rebased onto
main.Verification after rebase
mainaddedmessages_read_back_in_the_order_they_arrivedin the same block; both tests are kept.main: the read path is stillORDER BY created_timestamp, idand the index is still absent.main's new per-insertSELECT MAX(created_timestamp) FROM messages WHERE session_id = ?inadd_messageis additionally covered by this index.cargo test -p goose --lib session::session_manager— 44 passed.cargo clippy -p goose --lib --tests -- -D warnings— clean.Summary
WHERE session_id = ? ORDER BY created_timestamp, id, but no index coveredcreated_timestamp— the only timestamp index (idx_messages_timestamp) is on a different column (timestamp).TEMP B-TREEsort dragging each row'scontent_jsonthrough temp files. With large tool responses stored inline (observed: a single ~479 MBtoolResponse), this produced multi-GB write storms that exhausted macOS disk-write limits and stalled goose — confirmed by adisk writesresource event (34.36 GB in ~55 min, 100% of samples insqlite3VdbeSorterWrite → vdbeSorterFlushPMA → pwrite).(session_id, created_timestamp, id)so the planner walks the index instead of sorting. Verified the plan goes fromUSE TEMP B-TREE FOR ORDER BY→SEARCH messages USING INDEX idx_messages_session_created. Added in both the fresh-schema path and a new migration (v16).Review Notes
CREATE INDEX IF NOT EXISTSis idempotent; fresh DBs (viacreate_schema) and existing v15 DBs (via migration 16) both covered, mirroringidx_sessions_parent.Decision Log
Hardest decision: Keeping
idx_messages_sessionrather than dropping it. The new composite index(session_id, …)covers any pureWHERE session_id = ?lookup, soidx_messages_sessionis now largely redundant and costs one write per INSERT. I kept it to keep this PR a pure, zero-risk addition — the session-listingLEFT JOINrelies on it, and removing it would change that query plan. Dropping it is a follow-up optimization.Alternatives rejected:
(created_timestamp)index: would help a globalORDER BY created_timestampbut not the dominant per-sessionWHERE session_id = ? ORDER BY created_timestamp, id(still needs a sort after filtering). The composite index covers both filter and ordering.idx_messages_session: less write amplification, but risks the session-listing JOIN planner and is out of scope for a crash fix.Least confident about: One-time migration cost on pathologically large DBs.
CREATE INDEXscans all rows once (it only reads the indexed columns, so the resulting index is tiny — ~3 MB for ~100k rows), but on a ~1GB DB it is a brief lock at next launch. Acceptable and one-time.Test plan
test_messages_session_created_index_avoids_disk_sortasserts the index exists andEXPLAIN QUERY PLANuses it with noTEMP B-TREE(fails without the index).session::session_managersuite (40 tests) passes.cargo fmt,cargo clippy -p goose --all-targets -- -D warningsclean.Note
Fixes the crash mechanism. The oversized
toolResponsethat bloated the DB to 985 MB is a separate data-hygiene issue worth a follow-up (cap/externalize oversized tool results at write time). Existing bloated DBs can be trimmed withVACUUMafter truncating the offending row.Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com