Skip to content

fix(backend): index conversation_turn_nodes(last_seen_at) for retention sweep - #14772

Closed
oyi77 wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
oyi77:fix/turn-nodes-last-seen-index-13973
Closed

oyi77 wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
oyi77:fix/turn-nodes-last-seen-index-13973

Conversation

@oyi77

@oyi77 oyi77 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Covers the remaining open item of #13973 (the TRUNCATE removal landed in #14005; the scheduler stagger and this index were deliberately left out).

Problem

cleanupConversationTurnNodes() runs every 6h and deletes in batches via WHERE last_seen_at < ? (deleteFromTableBeforeInBatches). conversation_turn_nodes has indexes on (conversation_id), (parent_id), and (conversation_id, content_hash) — none covers the retention predicate, so every cleanup batch full-scans the table.

Fix

Migration 186_turn_nodes_last_seen_index.sql: CREATE INDEX IF NOT EXISTS idx_turn_nodes_last_seen ON conversation_turn_nodes(last_seen_at). Numbered 186 — clear of the three open PRs currently claiming 187.

Verification

  • New tests/unit/db/migration-186-turn-nodes-last-seen.test.ts: migration applies + idempotent re-run, and EXPLAIN QUERY PLAN on the exact retention predicate uses the index (seeded 500 rows + bound cutoff so the planner can't trivially scan).
  • check-migration-numbering: OK (183 migrations, 0 duplicates). check-file-size: OK both suites. Changelog fragment included.

…on sweep

The 6h cleanup deletes in batches via WHERE last_seen_at < ?, which
full-scanned every batch without an index. Covers the remaining open
item of diegosouzapw#13973 (TRUNCATE removal landed in diegosouzapw#14005).
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for the PR and for pinpointing the missing index — the analysis was right. It turns out #13973 was already fixed on release/v3.8.51 by #14567, whose migration 186_conversation_turn_nodes_last_seen_index.sql creates the very same idx_turn_nodes_last_seen index. Since your file would also be a second migration numbered 186, I'm going to close this as a duplicate. The EXPLAIN QUERY PLAN assertion in your test is a nice idea though — a PR adding it against the existing migration would be welcome.

@oyi77

oyi77 commented Sep 26, 2026

Copy link
Copy Markdown
Contributor Author

Closing as duplicate of #14567 (already merged the identical index via migration 186).

Per your suggestion, the EXPLAIN QUERY PLAN assertion is now in #14868, which pins the existing migration and asserts the retention predicate actually uses the index. Credit for the idea goes to this PR.

@oyi77 oyi77 closed this Sep 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants