#576: Move writes to SQLite transactions

[epompeii]

This changeset moves over to using SQLite transactions for write operations: #576

[github-actions]

🤖 Claude Code Review

PR: #681
Base: devel
Head: u/ep/db-transaction
Commit: b95512220767e942de09cba31b66fab8fb8a2269

---

Pull Request Review: SQLite Write Lock Contention Reduction

Overview

This PR reduces SQLite write lock contention through three main strategies:

1. Phase separation — splitting read-modify-write operations into "Phase 1" (reads on read connections) and "Phase 2" (batched writes in a single transaction)
2. last_insert_rowid() — replacing the UUID-lookup-after-insert pattern to avoid extra SELECTs
3. Transaction batching — wrapping related writes in single transactions for atomicity

The PR also introduces DateTime::TEST for deterministic test timestamps, converts test utilities from raw i32 to strong typed IDs, and adds extensive unit tests. Overall the direction is sound and well-tested.

---

Issues

1. last_insert_rowid() declared as Integer (i32) but SQLite returns i64

lib/bencher_schema/src/macros/sql.rs

fn last_insert_rowid() -> diesel::sql_types::Integer;

The code comment acknowledges this is intentional ("overflow is not a concern for this codebase's scale"), but this is a silent truncation risk if any table ever exceeds ~2 billion rows. Consider using BigInt (i64) and converting at the call site.

2. DateTime::now() in production code bypasses Clock abstraction

lib/bencher_schema/src/model/project/threshold/alert.rs — InsertAlert::insert
lib/bencher_schema/src/model/project/plot/mod.rs — new_rank / update_rank

These methods take &mut DbConnection rather than &ApiContext, so the Clock abstraction cannot be used. DateTime::now() is called directly. Per CLAUDE.md, Clock exists on ApiContext precisely to control datetime operations. Consider passing a DateTime parameter instead of calling DateTime::now() internally.

3. Raw i64 in benchmark_unarchive test violates strong-type rule

lib/bencher_schema/src/model/project/benchmark.rs — benchmark_unarchive test

.set(schema::benchmark::archived.eq(Some(1i64)))
// ...
let archived: Option<i64> = schema::benchmark::table

The PR's own test_util.rs provides archive_benchmark and get_benchmark_archived helpers that use DateTime::TEST. This test should use those helpers (or at minimum Option<DateTime> and DateTime::TEST) for consistency with the project rule about strong validated types.

4. DateTime::now() in test code

lib/bencher_schema/src/model/project/plot/mod.rs — plot_creation_inserts_all_components test

let timestamp = bencher_json::DateTime::now();

Should use DateTime::TEST per CLAUDE.md: "All time-based tests should be deterministic."

5. silence_all_empty_returns_zero test doesn't exercise the actual method

lib/bencher_schema/src/model/project/threshold/alert.rs

The test manually replicates the silence logic rather than calling QueryAlert::silence_all(). It should call the actual method to verify the early-return path works correctly in production.

6. last_insert_rowid() contract inconsistency between production and tests

lib/bencher_schema/src/macros/sql.rs says: "Must be called within a transaction"
lib/bencher_schema/src/test_util.rs helpers call it outside transactions

The test file has a justification comment (single-threaded tests), but this contradicts the documented contract. Either relax the contract or wrap test helpers in mini-transactions for consistency.

7. ThresholdModelAction::Remove uses error for unreachable case

lib/bencher_schema/src/model/project/threshold/mod.rs

ThresholdModelAction::Remove => {
    return Err(crate::error::issue_error(
        "Unexpected threshold model removal",
        ...
    ));
},

If this truly cannot happen (the comment says "Cannot happen"), debug_assert!(false, ...) with unreachable!() would be clearer than returning a runtime error.

---

Positive Observations

• Excellent test coverage — thorough unit tests for every changed code path, including edge cases (empty sets, isolation between heads, second-insert-rowid correctness)
• Strong type adoption — systematic conversion from i32 to typed IDs (ThresholdId, MeasureId, etc.) throughout test code
• Consistent struct destructuring — all into_query methods use let Self { ... } = self;, ensuring compiler errors when fields are added
• diesel::QueryResult<T> used consistently instead of Result<T, diesel::result::Error>
• Write methods don't wrap in conn.transaction() — inner methods correctly leave transaction wrapping to callers
• #[expect(...)] used throughout instead of #[allow(...)], with reason strings
• Bug fix in seed_test.rs — removed double cmd.assert().success() that consumed output on first call
• Smoke test improvement — added child.try_wait() to detect early process exit, preventing infinite loops

---

Summary

#	Severity	Issue
1	Low	last_insert_rowid() as i32 risks silent truncation at scale
2	Medium	DateTime::now() in production code bypasses Clock
3	Medium	Raw i64 in benchmark_unarchive test violates strong-type rule
4	Low	DateTime::now() in test should be DateTime::TEST
5	Low	Test replicates logic instead of calling actual method
6	Low	last_insert_rowid() contract inconsistency between docs and tests
7	Low	Runtime error for documented-unreachable code path

The PR is well-structured with high-quality changes. The primary actionable items are #2 (clock bypass in production) and #3 (raw i64 in test).

---

_{Model: claude-opus-4-6}

[github-actions]

Bencher Report

Branch	u/ep/db-transaction
Testbed	ubuntu-22.04

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

• Latency (nanoseconds (ns))

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds flag.

Click to view all benchmark results

Benchmark	Latency	microseconds (µs)
Adapter::Json	📈 view plot ⚠️ NO THRESHOLD	3.82 µs
Adapter::Magic (JSON)	📈 view plot ⚠️ NO THRESHOLD	3.80 µs
Adapter::Magic (Rust)	📈 view plot ⚠️ NO THRESHOLD	25.75 µs
Adapter::Rust	📈 view plot ⚠️ NO THRESHOLD	2.85 µs
Adapter::RustBench	📈 view plot ⚠️ NO THRESHOLD	2.83 µs
head_version_insert/batch/10	📈 view plot ⚠️ NO THRESHOLD	98.56 µs
head_version_insert/batch/100	📈 view plot ⚠️ NO THRESHOLD	235.35 µs
head_version_insert/batch/255	📈 view plot ⚠️ NO THRESHOLD	461.84 µs
head_version_insert/batch/50	📈 view plot ⚠️ NO THRESHOLD	156.40 µs
threshold_query/join/10	📈 view plot ⚠️ NO THRESHOLD	141.16 µs
threshold_query/join/20	📈 view plot ⚠️ NO THRESHOLD	155.01 µs
threshold_query/join/5	📈 view plot ⚠️ NO THRESHOLD	135.82 µs
threshold_query/join/50	📈 view plot ⚠️ NO THRESHOLD	196.15 µs

🐰 View full continuous benchmarking report in Bencher