Skip to content

Commit a30cc7a

Browse files
Enhance writing logic for ai_queries sqlite db table (warpdotdev#12484)
## Description <!-- Please remember to add your design buddy onto the PR for review, if it contains any UI changes! --> We don't have write cap for ai_queries (which could be dangerous), but we have [read cap for ai_queries](https://github.com/warpdotdev/warp/blob/917041b78aeed1c3e62e4be463f9c8694ee2c695/app/src/persistence/block_list.rs#L96), and those empty input lines [got filtered out](https://github.com/warpdotdev/warp/blob/917041b78aeed1c3e62e4be463f9c8694ee2c695/app/src/ai/blocklist/history_model.rs#L2015-L2023) before feeding downstream. Therefore, we get much less number of rows than expected. (85% of them are filtered out from local check with below sqlite queries) This PR aims to fix above experiences: - added write cap for ai_queries based on FIFO - skipped writing to table if input was empty The goal is without touching user local storage, the ai_queries will be filled by non-empty records by FIFO eviction policy eventually. ## Context when running ``` DB="$HOME/Library/Group Containers/2BBY89MBSN.dev.warp/Library/Application Support/dev.warp.Warp-Dev/warp.sqlite" # Total row count sqlite3 "$DB" "SELECT COUNT(*) FROM ai_queries;" # Rows with empty input (treats whitespace-only as empty) sqlite3 "$DB" "SELECT COUNT(*) FROM ai_queries WHERE TRIM(input) = '[]';" ``` i observed 16745 (out of 18144 total) are empty input rows. Those rows are written in ai_queries table, but they got filtered out upon [reading ](https://github.com/warpdotdev/warp/blob/b34f4ecbdffba385d53d71161514b91aa4c8dad7/app/src/ai/blocklist/history_model.rs#L2015-L2023)(so we never used those rows and this is a waste of resource) ## Linked Issue <!-- Link the GitHub issue this PR addresses. Before opening this PR, please confirm: --> - [ ] The linked issue is labeled `ready-to-spec` or `ready-to-implement`. - [ ] Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). ## Testing <!-- How did you test this change? What automated tests did you add? If you didn't add any new tests, what's your justification for not adding any? Manual testing is required for changes that can be manually tested, and almost all changes can be manually tested. If your change can be manually tested, please include screenshots or a screen recording that show it working end to end. You can run the app locally using `./script/run` - see WARP.md for more details on how to get set up. --> - [x] I have manually tested my changes locally with `./script/run` ### Screenshots / Videos <!-- Attach screenshots or a short video demonstrating the change, where appropriate. Remove this section if it is not relevant to your PR. --> ## Agent Mode - [ ] Warp Agent Mode - This PR was created via Warp's AI Agent Mode <!-- ## Changelog Entries for Stable The entries below will be used when constructing a soft-copy of the stable release changelog. Leave blank or remove the lines if no entry in the stable changelog is needed. Entries should be on the same line, without the `{{` `}}` brackets. You can use multiple lines, even of the same type. The valid suffixes are: - NEW-FEATURE: for new, relatively sizable features. Features listed here will likely have docs / social media posts / marketing launches associated with them, so use sparingly. - IMPROVEMENT: for new functionality of existing features. - BUG-FIX: for fixes related to known bugs or regressions. - IMAGE: the image specified by the URL (hosted on GCP) will be added to Dev & Preview releases. For Stable releases, see the pinned doc in the #release Slack channel. - OZ: Oz-related updates. Use `CHANGELOG-OZ`. At most 4 Oz updates are shown in-app per release. - NONE: Explicitly opt out of changelog inclusion. Use `CHANGELOG-NONE` for PRs that should never appear in the changelog (e.g. refactors, internal tooling, CI changes). This prevents the changelog agent from inferring an entry. CHANGELOG-NEW-FEATURE: {{text goes here...}} CHANGELOG-IMPROVEMENT: {{text goes here...}} CHANGELOG-BUG-FIX: {{text goes here...}} CHANGELOG-BUG-FIX: {{more text goes here...}} CHANGELOG-IMAGE: {{GCP-hosted URL goes here...}} CHANGELOG-OZ: {{text goes here...}} CHANGELOG-NONE -->
1 parent 5bc232d commit a30cc7a

3 files changed

Lines changed: 252 additions & 5 deletions

File tree

app/src/pane_group/pane/terminal_pane.rs

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2180,13 +2180,21 @@ fn handle_ai_history_event(
21802180
return;
21812181
}
21822182

2183+
// Only query-bearing inputs (e.g. user queries) are persisted for
2184+
// up-arrow history. Early return and skip writing for exchanges that
2185+
// carry empty input.
2186+
let inputs: Vec<_> = exchange
2187+
.input
2188+
.iter()
2189+
.filter_map(|input| PersistedAIInputType::try_from(input).ok())
2190+
.collect();
2191+
if inputs.is_empty() {
2192+
return;
2193+
}
2194+
21832195
let persisted_query = PersistedAIInput {
21842196
start_ts: exchange.start_time,
2185-
inputs: exchange
2186-
.input
2187-
.iter()
2188-
.filter_map(|input| PersistedAIInputType::try_from(input).ok())
2189-
.collect(),
2197+
inputs,
21902198
exchange_id: exchange.id,
21912199
conversation_id: *conversation_id,
21922200
output_status: AIQueryHistoryOutputStatus::from(&exchange.output_status),

app/src/persistence/block_list.rs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,15 +106,53 @@ pub(super) fn read_ai_queries(
106106
.collect_vec())
107107
}
108108

109+
const AI_QUERIES_COUNT_LIMIT: i64 = 10_000;
110+
109111
pub(super) fn upsert_ai_query(
110112
conn: &mut SqliteConnection,
111113
query: Arc<PersistedAIInput>,
114+
) -> anyhow::Result<()> {
115+
upsert_ai_query_with_limit(conn, query, AI_QUERIES_COUNT_LIMIT)
116+
}
117+
118+
/// Upserts an AI query while keeping the `ai_queries` table capped at `limit` rows by evicting
119+
/// the oldest queries (FIFO by `id`). Split out from [`upsert_ai_query`] so tests can exercise the
120+
/// eviction path with a small limit instead of inserting `AI_QUERIES_COUNT_LIMIT` rows.
121+
fn upsert_ai_query_with_limit(
122+
conn: &mut SqliteConnection,
123+
query: Arc<PersistedAIInput>,
124+
limit: i64,
112125
) -> anyhow::Result<()> {
113126
use schema::ai_queries::dsl::*;
114127

115128
let new_ai_query = NewAIQuery::try_from(query.as_ref())?;
116129

117130
Ok(conn.transaction::<_, Error, _>(|conn| {
131+
// Only a genuinely new exchange grows the table.
132+
let is_new_exchange = ai_queries
133+
.filter(exchange_id.eq(&new_ai_query.exchange_id))
134+
.count()
135+
.first::<i64>(conn)?
136+
== 0;
137+
if is_new_exchange {
138+
let query_count: i64 = ai_queries.count().first(conn)?;
139+
// add 1 because we are about to insert a new row.
140+
let diff = query_count - limit + 1;
141+
if diff > 0 {
142+
// Find the oldest row to keep and evict everything older (FIFO).
143+
let last_kept_id: Option<i32> = ai_queries
144+
.select(id)
145+
.order(id.asc())
146+
.offset(diff)
147+
.limit(1)
148+
.first(conn)
149+
.optional()?;
150+
if let Some(last_kept_id) = last_kept_id {
151+
diesel::delete(ai_queries.filter(id.lt(last_kept_id))).execute(conn)?;
152+
}
153+
}
154+
}
155+
118156
diesel::insert_into(ai_queries)
119157
.values(&new_ai_query)
120158
.on_conflict(exchange_id)
@@ -292,3 +330,7 @@ pub(super) fn delete_ai_conversation(
292330

293331
Ok(())
294332
}
333+
334+
#[cfg(test)]
335+
#[path = "block_list_tests.rs"]
336+
mod tests;
Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
//! Unit tests for the `ai_queries` persistence layer in [`super`].
2+
//!
3+
//! Covers the FIFO eviction cap added to [`super::upsert_ai_query`] and the empty-input filter
4+
//! that drives the persistence skip in `handle_ai_history_event`.
5+
6+
use std::sync::Arc;
7+
8+
use chrono::Local;
9+
use diesel::sqlite::SqliteConnection;
10+
use diesel::{Connection, ExpressionMethods, QueryDsl, RunQueryDsl};
11+
use diesel_migrations::MigrationHarness;
12+
13+
use super::upsert_ai_query_with_limit;
14+
use crate::ai::agent::conversation::AIConversationId;
15+
use crate::ai::agent::{AIAgentExchangeId, AIAgentInput, UserQueryMode};
16+
use crate::ai::blocklist::{AIQueryHistoryOutputStatus, PersistedAIInput, PersistedAIInputType};
17+
use crate::ai::llms::LLMId;
18+
19+
/// Builds an in-memory SQLite database with all migrations applied.
20+
fn test_connection() -> SqliteConnection {
21+
let mut conn =
22+
SqliteConnection::establish(":memory:").expect("in-memory sqlite connection should open");
23+
conn.run_pending_migrations(::persistence::MIGRATIONS)
24+
.expect("migrations should run");
25+
conn
26+
}
27+
28+
/// Builds a query-bearing [`PersistedAIInput`] with a fresh, unique `exchange_id`.
29+
fn make_query(text: &str) -> Arc<PersistedAIInput> {
30+
Arc::new(PersistedAIInput {
31+
exchange_id: AIAgentExchangeId::new(),
32+
conversation_id: AIConversationId::new(),
33+
start_ts: Local::now(),
34+
inputs: vec![PersistedAIInputType::Query {
35+
text: text.to_string(),
36+
context: Default::default(),
37+
referenced_attachments: Default::default(),
38+
}],
39+
output_status: AIQueryHistoryOutputStatus::Completed,
40+
working_directory: None,
41+
model_id: LLMId::from("test-model"),
42+
coding_model_id: LLMId::from("test-coding-model"),
43+
})
44+
}
45+
46+
fn ai_query_count(conn: &mut SqliteConnection) -> i64 {
47+
use crate::persistence::schema::ai_queries::dsl::ai_queries;
48+
ai_queries
49+
.count()
50+
.first(conn)
51+
.expect("count query should succeed")
52+
}
53+
54+
/// Returns the persisted `exchange_id`s ordered by `id` ascending (i.e. insertion / FIFO order).
55+
fn remaining_exchange_ids(conn: &mut SqliteConnection) -> Vec<String> {
56+
use crate::persistence::schema::ai_queries::dsl::{ai_queries, exchange_id, id};
57+
ai_queries
58+
.select(exchange_id)
59+
.order(id.asc())
60+
.load::<String>(conn)
61+
.expect("load query should succeed")
62+
}
63+
64+
fn input_json_for_exchange(conn: &mut SqliteConnection, exchange: &str) -> String {
65+
use crate::persistence::schema::ai_queries::dsl::{ai_queries, exchange_id, input};
66+
ai_queries
67+
.filter(exchange_id.eq(exchange))
68+
.select(input)
69+
.first::<String>(conn)
70+
.expect("row for exchange should exist")
71+
}
72+
73+
#[test]
74+
fn upsert_ai_query_caps_table_and_evicts_oldest_first() {
75+
let mut conn = test_connection();
76+
let limit = 3;
77+
78+
// Insert five distinct exchanges into a table capped at three.
79+
let queries: Vec<Arc<PersistedAIInput>> =
80+
(0..5).map(|i| make_query(&format!("q{i}"))).collect();
81+
let exchange_ids: Vec<String> = queries.iter().map(|q| q.exchange_id.to_string()).collect();
82+
83+
for query in &queries {
84+
upsert_ai_query_with_limit(&mut conn, query.clone(), limit).expect("upsert should succeed");
85+
}
86+
87+
// The table never exceeds the limit.
88+
assert_eq!(ai_query_count(&mut conn), limit);
89+
90+
// The two oldest (q0, q1) are evicted; the three newest remain in insertion order.
91+
assert_eq!(
92+
remaining_exchange_ids(&mut conn),
93+
exchange_ids[2..].to_vec()
94+
);
95+
}
96+
97+
#[test]
98+
fn upsert_ai_query_stays_below_limit_without_evicting() {
99+
let mut conn = test_connection();
100+
let limit = 3;
101+
102+
// Filling exactly up to the limit should not evict anything.
103+
let queries: Vec<Arc<PersistedAIInput>> =
104+
(0..3).map(|i| make_query(&format!("q{i}"))).collect();
105+
let exchange_ids: Vec<String> = queries.iter().map(|q| q.exchange_id.to_string()).collect();
106+
107+
for query in &queries {
108+
upsert_ai_query_with_limit(&mut conn, query.clone(), limit).expect("upsert should succeed");
109+
}
110+
111+
assert_eq!(ai_query_count(&mut conn), limit);
112+
assert_eq!(remaining_exchange_ids(&mut conn), exchange_ids);
113+
}
114+
115+
#[test]
116+
fn upsert_ai_query_updates_existing_exchange_without_evicting() {
117+
let mut conn = test_connection();
118+
let limit = 2;
119+
120+
// Fill the table to its limit with two distinct exchanges.
121+
let first = make_query("first");
122+
let second = make_query("second");
123+
upsert_ai_query_with_limit(&mut conn, first.clone(), limit).expect("upsert should succeed");
124+
upsert_ai_query_with_limit(&mut conn, second.clone(), limit).expect("upsert should succeed");
125+
assert_eq!(ai_query_count(&mut conn), limit);
126+
127+
// Re-upsert the oldest exchange (same `exchange_id`) repeatedly. Because this is an update of
128+
// an existing exchange rather than a new one, it must update in place and never evict.
129+
let updated_first = Arc::new(PersistedAIInput {
130+
inputs: vec![PersistedAIInputType::Query {
131+
text: "first-updated".to_string(),
132+
context: Default::default(),
133+
referenced_attachments: Default::default(),
134+
}],
135+
..(*first).clone()
136+
});
137+
for _ in 0..5 {
138+
upsert_ai_query_with_limit(&mut conn, updated_first.clone(), limit)
139+
.expect("upsert should succeed");
140+
}
141+
142+
// Still exactly two rows, and both original exchanges survive (the oldest was not evicted).
143+
assert_eq!(ai_query_count(&mut conn), limit);
144+
assert_eq!(
145+
remaining_exchange_ids(&mut conn),
146+
vec![
147+
first.exchange_id.to_string(),
148+
second.exchange_id.to_string()
149+
]
150+
);
151+
152+
// The in-place update took effect.
153+
let input_json = input_json_for_exchange(&mut conn, &first.exchange_id.to_string());
154+
assert!(
155+
input_json.contains("first-updated"),
156+
"existing row should have been updated in place, got: {input_json}"
157+
);
158+
}
159+
160+
#[test]
161+
fn empty_input_skip_filters_out_non_query_inputs() {
162+
// Mirrors the filter in `handle_ai_history_event`: only query-bearing inputs are persisted.
163+
// An exchange whose inputs are all non-query types collapses to an empty `inputs` vec, which
164+
// is the exact condition that skips persistence.
165+
let user_query = AIAgentInput::UserQuery {
166+
query: "hello".to_string(),
167+
context: Default::default(),
168+
static_query_type: None,
169+
referenced_attachments: Default::default(),
170+
user_query_mode: UserQueryMode::default(),
171+
running_command: None,
172+
intended_agent: None,
173+
};
174+
let non_query = AIAgentInput::ResumeConversation {
175+
context: Default::default(),
176+
};
177+
178+
// A query input is persistable; a non-query input is not.
179+
assert!(PersistedAIInputType::try_from(&user_query).is_ok());
180+
assert!(PersistedAIInputType::try_from(&non_query).is_err());
181+
182+
// An exchange carrying only non-query inputs collapses to empty -> skipped.
183+
let only_non_query = [non_query];
184+
let persisted: Vec<_> = only_non_query
185+
.iter()
186+
.filter_map(|input| PersistedAIInputType::try_from(input).ok())
187+
.collect();
188+
assert!(persisted.is_empty());
189+
190+
// An exchange carrying a query input is persisted.
191+
let with_query = [user_query];
192+
let persisted: Vec<_> = with_query
193+
.iter()
194+
.filter_map(|input| PersistedAIInputType::try_from(input).ok())
195+
.collect();
196+
assert_eq!(persisted.len(), 1);
197+
}

0 commit comments

Comments
 (0)