Problem
HistoryDb::prune_old_entries (crates/core/src/storage/history.rs:223) runs a full-table SELECT COUNT(*) before every retention delete. It is called unconditionally from insert_entry (crates/core/src/storage/history.rs:169), i.e. on every saved history row.
// crates/core/src/storage/history.rs:223
fn prune_old_entries(conn: &Connection, max_rows: i64) -> Result<usize> {
let count: i64 = conn
.query_row(
&format!("SELECT COUNT(*) FROM {HISTORY_TABLE}"), // line 226
[],
|row| row.get(0),
)
.map_err(|e| anyhow!("Failed to count history rows: {}", e))?;
if count <= max_rows { // line 231: early-out
return Ok(0);
}
let deleted = conn
.prepare_cached(&Self::retention_prune_sql())?
.execute(params![max_rows])?;
Ok(deleted)
}The DELETE itself (retention_prune_sql, crates/core/src/storage/history.rs:209) already no-ops under the cap, because its subquery selects the deletion tail via LIMIT -1 OFFSET ?1 (line 215) — when row count <= max_rows the OFFSET consumes all rows and the id IN (...) set is empty:
// crates/core/src/storage/history.rs:209
"DELETE FROM {HISTORY_TABLE} \
WHERE id IN ( \
SELECT id FROM {HISTORY_TABLE} \
ORDER BY created_at DESC, id DESC \
LIMIT -1 OFFSET ?1 \
)"So the COUNT(*) is a redundant precheck guarding a statement that is already safe to run when under cap. The doc comment at insert_entry (crates/core/src/storage/history.rs:164-167) describes this skip as deliberate "avoiding wasted work."
Why it matters
- Runs on the persistence / stop-latency path: one extra
SELECT COUNT(*) per saved row, in the same locked connection as the insert.
- Without an index optimization,
COUNT(*) is an O(N) scan of the history table; it grows with retained rows even though the steady state is "at cap, nothing to delete."
- Low correctness risk to remove — the DELETE's OFFSET tail already encodes the same guard, so removing the count cannot delete rows that are within the cap.
Proposed change
clawpatch direction: delete the count precheck and run the DELETE unconditionally.
- In
prune_old_entries (crates/core/src/storage/history.rs:223), remove the SELECT COUNT(*) query and the if count <= max_rows { return Ok(0); } early-out; return the row count from execute(params![max_rows]) directly.
- Update the
insert_entry doc comment (crates/core/src/storage/history.rs:164-167) to drop the "skips the delete entirely when the row count is at or under the cap" claim.
- Tests:
- Keep
retention_prune_helper_deletes_oldest_tail_over_cap (:440) — proves over-cap still deletes exactly the overflow and keeps the newest max_rows.
- Repurpose, do not delete,
retention_prune_helper_deletes_zero_when_under_cap (:399): it currently asserts the count-skip detail (helper returns Ok(0) and issues no delete). After the change it must still assert the observable invariant — under/at cap, prune_old_entries returns 0 and the row count is unchanged — but stop implying the delete was skipped.
- Coupled SQL-shape test
retention_prune_sql_deletes_offset_tail (:373) is tracked separately; coordinate the change there (see Traceability).
Recommendation / triage
DISCUSS / lean KEEP. This reverses the STOR-1 perf-review decision; the count guard was an intentional early-out. Both the COUNT(*) and the LIMIT -1 OFFSET ?1 delete subquery are ~O(N) index walks under cap, so removing the guard is roughly a wash and makes a DELETE write-statement execute on every insert (extra WAL/transaction work, vs. a read-only count). Recommend keeping unless profiling shows the COUNT(*) actually dominates the insert path. If simplifying, do it together with the coupled SQL-shape-test removal rather than piecemeal.
Acceptance criteria
Traceability
- clawpatch finding ID:
fnd_sig-feat-library-5e03ac577c-544c_4b589bfafd
- Revalidate:
clawpatch revalidate --finding fnd_sig-feat-library-5e03ac577c-544c_4b589bfafd --reasoning-effort high
- Coupled issue: the SQL-shape assertion test
retention_prune_sql_deletes_offset_tail (crates/core/src/storage/history.rs:373). If this change lands, fold the SQL-shape-test cleanup into the same PR; do not remove the count guard independently of that coupling.
Problem
HistoryDb::prune_old_entries(crates/core/src/storage/history.rs:223) runs a full-tableSELECT COUNT(*)before every retention delete. It is called unconditionally frominsert_entry(crates/core/src/storage/history.rs:169), i.e. on every saved history row.The DELETE itself (
retention_prune_sql,crates/core/src/storage/history.rs:209) already no-ops under the cap, because its subquery selects the deletion tail viaLIMIT -1 OFFSET ?1(line 215) — when row count <=max_rowsthe OFFSET consumes all rows and theid IN (...)set is empty:So the
COUNT(*)is a redundant precheck guarding a statement that is already safe to run when under cap. The doc comment atinsert_entry(crates/core/src/storage/history.rs:164-167) describes this skip as deliberate "avoiding wasted work."Why it matters
SELECT COUNT(*)per saved row, in the same locked connection as the insert.COUNT(*)is an O(N) scan of the history table; it grows with retained rows even though the steady state is "at cap, nothing to delete."Proposed change
clawpatch direction: delete the count precheck and run the DELETE unconditionally.
prune_old_entries(crates/core/src/storage/history.rs:223), remove theSELECT COUNT(*)query and theif count <= max_rows { return Ok(0); }early-out; return the row count fromexecute(params![max_rows])directly.insert_entrydoc comment (crates/core/src/storage/history.rs:164-167) to drop the "skips the delete entirely when the row count is at or under the cap" claim.retention_prune_helper_deletes_oldest_tail_over_cap(:440) — proves over-cap still deletes exactly the overflow and keeps the newestmax_rows.retention_prune_helper_deletes_zero_when_under_cap(:399): it currently asserts the count-skip detail (helper returnsOk(0)and issues no delete). After the change it must still assert the observable invariant — under/at cap,prune_old_entriesreturns0and the row count is unchanged — but stop implying the delete was skipped.retention_prune_sql_deletes_offset_tail(:373) is tracked separately; coordinate the change there (see Traceability).Recommendation / triage
DISCUSS / lean KEEP. This reverses the STOR-1 perf-review decision; the count guard was an intentional early-out. Both the
COUNT(*)and theLIMIT -1 OFFSET ?1delete subquery are ~O(N) index walks under cap, so removing the guard is roughly a wash and makes a DELETE write-statement execute on every insert (extra WAL/transaction work, vs. a read-only count). Recommend keeping unless profiling shows theCOUNT(*)actually dominates the insert path. If simplifying, do it together with the coupled SQL-shape-test removal rather than piecemeal.Acceptance criteria
prune_old_entriesissues noSELECT COUNT(*); the DELETE runs unconditionally and relies on the empty OFFSET tail to no-op under cap.max_rows(retention_prune_helper_deletes_oldest_tail_over_cap,:440); under/at cap leaves row count untouched and returns0(retention_prune_helper_deletes_zero_when_under_cap,:399, updated to assert the invariant not the skip).insert_entrydoc comment (:164-167) matches the implemented behavior.HistoryDb.Traceability
fnd_sig-feat-library-5e03ac577c-544c_4b589bfafdclawpatch revalidate --finding fnd_sig-feat-library-5e03ac577c-544c_4b589bfafd --reasoning-effort highretention_prune_sql_deletes_offset_tail(crates/core/src/storage/history.rs:373). If this change lands, fold the SQL-shape-test cleanup into the same PR; do not remove the count guard independently of that coupling.