From 507c8ac3179f924ed6208123264309b3208f0165 Mon Sep 17 00:00:00 2001 From: YoursFunny Date: Mon, 21 Sep 2026 13:52:43 +0800 Subject: [PATCH] perf: index the link-cache prune, evict chats with no live prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the 300 s sweep did the hard way: - The link cache is pruned by `created_at` (`DELETE FROM link_cache WHERE created_at < ?`) and had no index on it, so every sweep scanned the whole table — every post sent inside the TTL window, which is up to a week of them — while the `url` primary key served none of it. A new migration (appended; migration 1 is frozen and already shipped) creates the index, and the upgrade test now asserts it exists after an upgrade. - `ChatStore::prune_expired` only ever *looked* at chats that had an expired edit-before-forward record, so a chat with no prompt at all — the common case: every chat that ever sent a message or ran a command — stayed in the cache and in the per-chat lock map for the process lifetime. The candidate set now includes chats holding no records, which is what the eviction below was written for; the DB keeps the row, so the next use costs one SELECT (pinned by a new test that also shows the durable settings come back). Deliberately *not* done: skipping the write in `ChatStore::set` when the state is unchanged. Comparing against the cached copy would skip a serialize plus a blocking DB round trip for a no-op update — but every one of the 13 `update` callers mutates something, so the no-op case is a user repeating an identical command, and the same comparison would also skip the write that repairs a row whose earlier write failed. A rare saving against a rare repair, and the write is what makes the cache a cache rather than a source of truth. Verified: the new eviction test fails without the candidate change (checked by reverting it) and passes with it; 118 bot tests and 91 x-media tests pass. `cargo fmt --check`, `cargo clippy --workspace --all-targets --locked -- -D warnings` and `cargo test --workspace --locked` clean. --- crates/xmedia-bot/src/db.rs | 17 ++++++++ crates/xmedia-bot/src/state.rs | 72 ++++++++++++++++++++++++++++++---- 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/crates/xmedia-bot/src/db.rs b/crates/xmedia-bot/src/db.rs index 78794d9..1a42e06 100644 --- a/crates/xmedia-bot/src/db.rs +++ b/crates/xmedia-bot/src/db.rs @@ -139,6 +139,11 @@ const MIGRATIONS: &[&str] = &[ // written by its former holder — which used to duplicate a send or drop // the new holder's retry state, silently. "ALTER TABLE tasks ADD COLUMN lease_token TEXT", + // 2: the 300 s sweep prunes the link cache by `created_at` + // (`DELETE FROM link_cache WHERE created_at < ?`). Without an index that + // is a full scan of every post sent inside the TTL window — up to a week + // of them — on every sweep; the `url` primary key cannot serve it. + "CREATE INDEX IF NOT EXISTS idx_link_cache_created_at ON link_cache(created_at)", ]; /// Brings an existing database up to [`MIGRATIONS`]. Idempotent: a database @@ -285,6 +290,18 @@ mod tests { ) .unwrap(); assert_eq!(payload, "{\"chat_id\":1}", "rows survive the upgrade"); + // The link-cache prune's index arrives with the migrations (the + // baseline schema has none): without it every sweep scans the + // whole table. + let index: i64 = conn + .query_row( + "SELECT COUNT(*) FROM sqlite_master \ + WHERE type = 'index' AND name = 'idx_link_cache_created_at'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(index, 1, "the migration's index must exist"); Ok(()) }) .await diff --git a/crates/xmedia-bot/src/state.rs b/crates/xmedia-bot/src/state.rs index a44d095..fa44efd 100644 --- a/crates/xmedia-bot/src/state.rs +++ b/crates/xmedia-bot/src/state.rs @@ -131,18 +131,24 @@ impl ChatStore { pub async fn prune_expired(&self, ttl: Duration) -> Vec<(i64, i64)> { let now = unix_now(); let ttl_secs = ttl.as_secs() as i64; - // Chats that may have an expired record, from a cache snapshot; the - // pruning itself re-reads and writes under the per-chat lock below - // (see the eviction note). Takes no lock of its own, so a chat - // appearing later is simply picked up by the next sweep. + // Chats worth looking at, from a cache snapshot: the ones with an + // expired record, plus the ones holding no record at all. The latter + // used to be left alone for the process lifetime — every chat that ever + // sent a message or ran a command stayed in the cache and in the + // per-chat lock map — even though a chat with no live prompt is exactly + // what the eviction below is for. The pruning itself re-reads and + // writes under the per-chat lock below; taking no lock here means a + // chat appearing later is simply picked up by the next sweep. let candidates: Vec = { let cache = self.cache.lock(); cache .iter() .filter(|(_, data)| { - data.edit_message - .values() - .any(|entry| entry.created_at + ttl_secs <= now) + data.edit_message.is_empty() + || data + .edit_message + .values() + .any(|entry| entry.created_at + ttl_secs <= now) }) .map(|(chat_id, _)| *chat_id) .collect() @@ -265,6 +271,58 @@ mod tests { ); } + #[tokio::test] + async fn an_idle_chat_is_evicted_and_its_state_reloads() { + let dir = tempfile::tempdir().unwrap(); + let pool = crate::db::open_store(dir.path().join("e.db").to_str().unwrap()).unwrap(); + let store = ChatStore::new(pool); + // Durable settings and no prompt at all: this chat used to sit in the + // cache (and in the per-chat lock map) for the process lifetime, + // because the sweep only ever looked at chats with an *expired* record. + store + .update(9, |data| { + data.forward_channel_id = Some(-100); + data.message_format.insert("twitter".into(), "{url}".into()); + }) + .await; + assert!(store.cache.lock().contains_key(&9)); + + let removed = store.prune_expired(Duration::from_secs(60)).await; + + assert!(removed.is_empty(), "nothing had expired"); + assert!( + !store.cache.lock().contains_key(&9), + "a chat with no live prompt must leave the cache" + ); + assert!(!store.locks.lock().contains_key(&9), "…and its lock"); + // The DB kept the row, so the next use reloads everything it held. + let data = store.get(9).await; + assert_eq!(data.forward_channel_id, Some(-100)); + assert_eq!( + data.message_format.get("twitter").map(String::as_str), + Some("{url}") + ); + } + + #[tokio::test] + async fn a_live_prompt_keeps_its_chat_cached() { + let dir = tempfile::tempdir().unwrap(); + let pool = crate::db::open_store(dir.path().join("k.db").to_str().unwrap()).unwrap(); + let store = ChatStore::new(pool); + store + .update(10, |data| { + data.edit_message.insert(1, edit_entry(10, unix_now())); + }) + .await; + + store.prune_expired(Duration::from_secs(3600)).await; + + assert!( + store.cache.lock().contains_key(&10), + "a live prompt holds its chat in the cache" + ); + } + #[tokio::test] async fn prune_eviction_keeps_the_persisted_state() { // Every record expires → the chat is evicted from the cache; the