From f0650b05581cfdf844fe7bd50042bd617537b12c Mon Sep 17 00:00:00 2001 From: Maciej Myszkiewicz Date: Sun, 5 Jul 2026 06:45:34 +0200 Subject: [PATCH 1/3] perf(people): batch handle-alias fetch in list(), dropping an N+1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PeopleStore::list() returned every contact with its handle aliases, fetching them with a load_handles call per person inside the row loop — 1 + N queries (SELECT ... FROM handle_aliases WHERE person_id = ? prepared + executed once per contact), all under the store lock. Collect the person rows first, then fetch all aliases in one batched WHERE person_id IN (...) query keyed by person id (batch_handles_conn), windowed at 900 binds to stay under SQLite's default parameter cap. Mirrors the store's existing batch_interactions_for. list() drops from 1 + N to 1 + ceil(N/900) queries and holds the lock for less time. load_handles stays for the single-person get() path; the batch decodes each kind identically. Tests: list_batches_handles_per_person asserts three people (two aliases, none, one) each get exactly their own handles and the alias-less person doesn't inherit a neighbour's — the failure mode a batched IN query risks. --- src/openhuman/people/store.rs | 162 ++++++++++++++++++++++++++++++++-- 1 file changed, 155 insertions(+), 7 deletions(-) diff --git a/src/openhuman/people/store.rs b/src/openhuman/people/store.rs index 6dcac89cb5..f41317d845 100644 --- a/src/openhuman/people/store.rs +++ b/src/openhuman/people/store.rs @@ -17,6 +17,18 @@ use crate::openhuman::people::types::{Handle, Interaction, Person, PersonId}; pub type ConnHandle = Arc>; +/// A decoded `people` row, minus handles (fetched separately, in one batched +/// query, by [`PeopleStore::list`]): `(id, display_name, primary_email, +/// primary_phone, created_at, updated_at)`. +type PersonRow = ( + PersonId, + Option, + Option, + Option, + i64, + i64, +); + /// Process-global handle to the `PeopleStore`, tagged with the workspace it is /// bound to. Controller handlers are free functions with no `&self`, so they /// fetch the store via `get()`. Seeded at core boot and re-bound on active-user @@ -341,23 +353,46 @@ impl PeopleStore { r.get::<_, i64>(5)?, )) })?; - let mut out = Vec::new(); + // Materialise the person rows, then fetch every person's handles in + // one batched `WHERE person_id IN (…)` query instead of a + // `load_handles` round-trip per person (N+1). Both run under the + // same lock, so batching also shortens how long `list` holds it. + // Mirrors `batch_interactions_for`. + let mut people_rows: Vec = Vec::new(); for r in rows { let (id_str, display_name, primary_email, primary_phone, created, updated) = r?; let id = uuid::Uuid::parse_str(&id_str) .map(PersonId) .map_err(|e| rusqlite::Error::InvalidColumnName(e.to_string()))?; - let handles = load_handles(&guard, &id)?; - out.push(Person { + people_rows.push(( id, display_name, primary_email, primary_phone, - handles, - created_at: ts_to_dt(created), - updated_at: ts_to_dt(updated), - }); + created, + updated, + )); } + drop(stmt); + let ids: Vec = people_rows.iter().map(|row| row.0).collect(); + let mut handles_by_id = batch_handles_conn(&guard, &ids)?; + let out = people_rows + .into_iter() + .map( + |(id, display_name, primary_email, primary_phone, created, updated)| { + let handles = handles_by_id.remove(&id).unwrap_or_default(); + Person { + id, + display_name, + primary_email, + primary_phone, + handles, + created_at: ts_to_dt(created), + updated_at: ts_to_dt(updated), + } + }, + ) + .collect(); Ok(out) }) .await @@ -524,6 +559,63 @@ fn load_handles(conn: &Connection, id: &PersonId) -> SqlResult> { Ok(out) } +/// Number of `person_id` binds per handle-batch query. Kept under SQLite's +/// default 999 bound-parameter cap so `list` over an unbounded contact list +/// never overflows a single statement. +const HANDLE_BATCH: usize = 900; + +/// Batched form of [`load_handles`]: fetch handle aliases for many people in +/// `O(ceil(n / HANDLE_BATCH))` queries instead of one per person, keyed by +/// person id. Decodes each `kind` exactly as `load_handles` does, so behaviour +/// is identical — only the round-trip count changes. A person with no aliases +/// is simply absent from the map (callers use `unwrap_or_default`). +fn batch_handles_conn( + conn: &Connection, + ids: &[PersonId], +) -> SqlResult>> { + let mut out: HashMap> = HashMap::new(); + for window in ids.chunks(HANDLE_BATCH) { + if window.is_empty() { + continue; + } + let placeholders = std::iter::repeat("?") + .take(window.len()) + .collect::>() + .join(","); + let sql = format!( + "SELECT person_id, kind, value FROM handle_aliases \ + WHERE person_id IN ({placeholders}) ORDER BY person_id, kind, value" + ); + let id_strings: Vec = window.iter().map(ToString::to_string).collect(); + let mut stmt = conn.prepare(&sql)?; + let rows = stmt.query_map(rusqlite::params_from_iter(id_strings.iter()), |r| { + Ok(( + r.get::<_, String>(0)?, + r.get::<_, String>(1)?, + r.get::<_, String>(2)?, + )) + })?; + for r in rows { + let (pid_str, kind, value) = r?; + let person_id = uuid::Uuid::parse_str(&pid_str) + .map(PersonId) + .map_err(|e| rusqlite::Error::InvalidColumnName(e.to_string()))?; + let h = match kind.as_str() { + "imessage" => Handle::IMessage(value), + "email" => Handle::Email(value), + "display_name" => Handle::DisplayName(value), + other => { + return Err(rusqlite::Error::InvalidColumnName(format!( + "unknown handle kind: {other}" + ))); + } + }; + out.entry(person_id).or_default().push(h); + } + } + Ok(out) +} + fn ts_to_dt(ts: i64) -> DateTime { Utc.timestamp_opt(ts, 0) .single() @@ -568,6 +660,62 @@ mod tests { assert_eq!(list[0].handles.len(), 2); } + #[tokio::test] + async fn list_batches_handles_per_person() { + // Guards the batched handle fetch in `list`: each person must get + // exactly its own aliases and never another person's — the failure + // mode a single `WHERE person_id IN (…)` query risks if rows aren't + // keyed back correctly. A person with no aliases must come back empty, + // not inherit a neighbour's. + let s = PeopleStore::open_in_memory().unwrap(); + let now = Utc::now(); + let mk = |name: &str, email: &str| Person { + id: PersonId::new(), + display_name: Some(name.to_string()), + primary_email: Some(email.to_string()), + primary_phone: None, + handles: vec![], + created_at: now, + updated_at: now, + }; + let alice = mk("Alice", "alice@example.com"); + let bob = mk("Bob", "bob@example.com"); + let carol = mk("Carol", "carol@example.com"); + + s.insert_person( + &alice, + &[ + Handle::Email("alice@example.com".into()), + Handle::DisplayName("Alice".into()), + ], + ) + .await + .unwrap(); + s.insert_person(&bob, &[]).await.unwrap(); + s.insert_person(&carol, &[Handle::IMessage("+15550001".into())]) + .await + .unwrap(); + + let list = s.list().await.unwrap(); + assert_eq!(list.len(), 3); + let by_id: HashMap = list.iter().map(|p| (p.id, p)).collect(); + + assert_eq!( + by_id[&alice.id].handles.len(), + 2, + "alice keeps both aliases" + ); + assert!( + by_id[&bob.id].handles.is_empty(), + "bob has no aliases and must not inherit another person's" + ); + assert_eq!(by_id[&carol.id].handles.len(), 1, "carol keeps her alias"); + assert!( + matches!(by_id[&carol.id].handles[0], Handle::IMessage(_)), + "carol's alias kind is preserved" + ); + } + #[tokio::test] async fn interactions_round_trip() { let s = PeopleStore::open_in_memory().unwrap(); From c329383e203e678bee8c72409537de5bcae73df7 Mon Sep 17 00:00:00 2001 From: Maciej Myszkiewicz Date: Sat, 18 Jul 2026 00:51:19 +0200 Subject: [PATCH 2/3] refactor(people): use repeat_n and drop unreachable empty-window guard in batch_handles_conn - repeat_n("?", n) matches the sibling batch_interactions_for idiom and satisfies clippy::manual_repeat_n (vs repeat().take()). - slice::chunks never yields an empty window (empty ids -> zero windows), so the if window.is_empty() { continue } branch was unreachable; remove it. --- src/openhuman/people/store.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/openhuman/people/store.rs b/src/openhuman/people/store.rs index f41317d845..ba3da7bd4b 100644 --- a/src/openhuman/people/store.rs +++ b/src/openhuman/people/store.rs @@ -575,11 +575,7 @@ fn batch_handles_conn( ) -> SqlResult>> { let mut out: HashMap> = HashMap::new(); for window in ids.chunks(HANDLE_BATCH) { - if window.is_empty() { - continue; - } - let placeholders = std::iter::repeat("?") - .take(window.len()) + let placeholders = std::iter::repeat_n("?", window.len()) .collect::>() .join(","); let sql = format!( From b4840763c49898d360021cb104bd0928208846f0 Mon Sep 17 00:00:00 2001 From: Maciej Myszkiewicz Date: Thu, 23 Jul 2026 16:25:17 +0200 Subject: [PATCH 3/3] fix(people): rename batched-list row alias to DecodedPersonRow main since gained a raw `type PersonRow = (String, ...)` (used by `get()`); this branch added a decoded `type PersonRow = (PersonId, ...)` for `list()`'s batched handle fetch. Merged together they collide at module scope (E0428) and cascade into E0277 (pushing PersonId into a Vec<(String, ...)>). Rename the decoded alias to `DecodedPersonRow` so both coexist; the raw and decoded rows are genuinely different shapes and each is used by exactly one function. --- src/openhuman/people/store.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/openhuman/people/store.rs b/src/openhuman/people/store.rs index ba3da7bd4b..38d6d65fec 100644 --- a/src/openhuman/people/store.rs +++ b/src/openhuman/people/store.rs @@ -20,7 +20,12 @@ pub type ConnHandle = Arc>; /// A decoded `people` row, minus handles (fetched separately, in one batched /// query, by [`PeopleStore::list`]): `(id, display_name, primary_email, /// primary_phone, created_at, updated_at)`. -type PersonRow = ( +/// +/// Distinct from the raw `PersonRow` used by `get()`, whose first element is the +/// undecoded `String` id straight from SQLite; here the id is already parsed to +/// [`PersonId`] so `list()` can collect ids for the batched handle fetch without +/// re-parsing. Kept as a separate alias to avoid an `E0428` name clash. +type DecodedPersonRow = ( PersonId, Option, Option, @@ -358,7 +363,7 @@ impl PeopleStore { // `load_handles` round-trip per person (N+1). Both run under the // same lock, so batching also shortens how long `list` holds it. // Mirrors `batch_interactions_for`. - let mut people_rows: Vec = Vec::new(); + let mut people_rows: Vec = Vec::new(); for r in rows { let (id_str, display_name, primary_email, primary_phone, created, updated) = r?; let id = uuid::Uuid::parse_str(&id_str)