Skip to content

fix(recall): cap retained per-filter statistics - #4536

Open
yeahjack wants to merge 12 commits into
rtk-ai:developfrom
yeahjack:fix/recall-stats-row-cap
Open

yeahjack wants to merge 12 commits into
rtk-ai:developfrom
yeahjack:fix/recall-stats-row-cap

Conversation

@yeahjack

Copy link
Copy Markdown
Contributor

Summary

  • Cap recall_stats at 500 (slug, mode) rows across sqlite and tee modes. Dynamic command names currently create unbounded rows even though retained output and tee-read deduplication already have their own limits.
  • Enforce the cap atomically with an AFTER INSERT trigger. Evict the lowest elisions + recalls, then oldest rowid, while protecting the just-inserted row. Repeated-slug upserts keep accumulating counters and do not enter the insert-trigger body. This also works inside an existing caller transaction.
  • Trim existing oversized stores when opened and document that evicted counters are discarded. Returning filters restart their counters; these are bounded calibration statistics, not lifetime totals. Recall output itself is unaffected.

This is a fresh, narrow implementation against current develop, following the maintainer's request for a separate recall_stats cap PR. It does not revive or rebase #3900.

Policy choice

500 is a fixed engineering bound with room for hundreds of filter/mode pairs, independent of the configurable output-entry cap (including when output retention is unlimited). It is not a claimed optimal capacity. Frequency-based eviction favors established filters, and protecting the incoming row lets a newly active filter start accumulating statistics. Both modes retain independent counters and share the same bound.

The trigger keeps insertion and eviction in one SQLite statement, avoiding a count/delete race or a nested-transaction requirement. Opening a legacy oversized table also uses a single count-and-delete statement. No new dependencies, table columns, or user-facing configuration are introduced.

Test plan

  • cargo fmt --all -- --check
  • cargo clippy --all-targets (no warnings)
  • cargo test --all: 4,381 passed across 36 suites, 8 ignored
  • cargo doc --no-deps
  • Six new regression tests cover 1,000 dynamic command slugs, repeated counters and separate modes, incoming-row protection, existing oversized stores, caller-transaction rollback, and four concurrent connections.
  • Mutation check: disabling the trigger makes the dynamic-slug regression fail at 501 rows versus the expected cap of 500; restored implementation passes.

Bounded performance measurement

A local optimized Rust microbenchmark used the production schema, canonicalization, and upsert functions with bundled SQLite 3.45.0 in memory. At 500 initial rows, across seven samples of 2,000 updates:

  • Repeated slug: median 4.6 µs without the trigger, 9.1 µs with it.
  • Distinct dynamic slugs: median 4.8 µs without the trigger, 53.0 µs with it; final rows 2,500 versus 500.

These synthetic measurements show the added retention cost in one local environment, with other builds running. They are not end-to-end CLI startup timings or a claim that this is the fastest possible retention policy.

@rtk-wshm-sync-bot

Copy link
Copy Markdown

wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟡 Risk medium

Summary

Caps the recall_stats table at 500 (slug, mode) rows using an atomic AFTER INSERT SQLite trigger that evicts the least-used row (by elisions+recalls, then oldest rowid) while protecting the newly inserted row. Also trims existing oversized stores on open and updates documentation to clarify that evicted counters are discarded and stats are bounded calibration data, not lifetime totals.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Analyzed automatically by wshm · This is an automated analysis, not a human review.

@yeahjack

Copy link
Copy Markdown
Contributor Author

CI follow-up: Windows exposed the existing best-effort statistics busy timeout when the new contention test issued thousands of durable autocommits. Commit 12da99b keeps the exact counter and row-cap assertions but runs four competing caller-owned IMMEDIATE transactions, with a test-only contention timeout. This tests atomic retention rather than filesystem fsync throughput; production timeout/behavior is unchanged. The focused test and full local fmt/clippy/test gates pass again. Waiting for all checks on the new head before marking ready.

@yeahjack
yeahjack marked this pull request as ready for review October 12, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant