Speed up large report deletions

[epompeii]

Problem

Deleting an incident-scale report (hundreds of thousands of cascade rows) holds the single SQLite writer connection for 30+ seconds, blocking all other writes, and generates a WAL burst that Litestream must replicate (the iowait churn during the incident).

Analysis against a copy of the production database showed that every FK cascade level already uses an indexed seek (verified via EXPLAIN bytecode), so no index additions can help. The cost is raw B-tree write volume, with ~80% of the WAL churn coming from UUIDv4 index page scatter: nearly every deleted row dirties its own 4 KiB page in the uuid unique indexes.

Changes

1. Chunked report deletion: DELETE /v0/projects/{project}/reports/{report} now deletes report_benchmark rows in bounded chunks (1024 rows per write statement, cascading to metric, boundary, and alert), releasing the writer lock between chunks so other writers interleave. The final report row delete cascades any stragglers plus the job and rollup rows.
2. UUIDv7 for typed uuids: typed_uuid! now mints time-ordered UUIDv7 instead of random UUIDv4, clustering same-batch rows in the uuid indexes for both inserts and deletes. Stored as hyphenated TEXT, v7 sorts chronologically; mixed v4/v7 rows coexist fine in the UNIQUE indexes, no backfill needed. None of the typed uuids are used as unguessable capabilities, so the reduced entropy (74 vs 122 random bits) and embedded timestamp are acceptable.
3. Writer page cache size: new optional cache_size config on database (KiB, default 64 MiB, previously the 2 MiB SQLite default), applied to the single writer connection only.
4. Index cleanup migration: drops index_report_benchmark_benchmark (strict prefix of index_report_benchmark_benchmark_report) and index_report_benchmark (covered by the UNIQUE(report_id, iteration, benchmark_id) autoindex). Query plans for the report results load, the detector query, and report+benchmark lookups verified unaffected.
5. Drive-by fix: the otel ReportDelete counter sat after the early return for shared versions, so it only counted deletes that also dropped a dangling version; it now increments right after the report row delete.

Validation

Measured against a copy of the production database with a synthesized incident-scale report, replaying the exact statement shape the new code produces:

Total WAL roughly doubles because scattered uuid index pages are re-dirtied across chunk boundaries, but it is spread over a minute instead of one burst. The UUIDv7 change collapses that scatter for new rows going forward.

New integration tests cover report deletion with results (below one chunk, and exactly 2x the chunk size to exercise multiple chunks and the exact-multiple boundary condition).

[github-actions]

🤖 Claude Code Review

PR: #925
Base: devel
Head: claude/report-delete-perf
Commit: 751a7e4cc5f2b7071f7195655e62b99f6436f370

---

Code Review

Reviewed git diff devel...HEAD. The PR bundles four related database performance changes: chunked report deletion, a redundant-index cleanup migration, a configurable writer-connection cache_size PRAGMA, and a UUIDv4 → UUIDv7 switch for index locality. Overall this is high-quality, well-documented, and well-tested work. No blocking issues found.

Strengths

• Excellent inline documentation. The delete_report_results doc comment explains the non-atomic tradeoff, the cascade chain, and why interleaving is safe. The PRAGMA and UUIDv7 comments explain the why, not just the what.
• Good test coverage. reports_delete_chunked deliberately seeds 2 * DELETE_CHUNK_SIZE to exercise the exact-multiple boundary (the loop-termination edge case), and reports_delete_with_results covers the sub-chunk case. Row counts are asserted before and after.
• Correct feature-flag hygiene. Adding uuid v4 to api_run and services/cli is the right fix: those crates call Uuid::new_v4() directly and were previously relying on feature unification from bencher_json, which no longer enables v4.
• Docs and OpenAPI regenerated consistently across all locales, with NonZeroU32 correctly surfacing as minimum: 1.

Verified non-issues

• FK cascade on the writer: confirmed report_benchmark → metric → boundary/alert cascade with ON DELETE CASCADE, and the writer connection runs PRAGMA foreign_keys = ON (via run_migrations), so deleting report_benchmark chunks correctly cascades. The read (auth_conn!) doesn't need FKs.
• SQLite bind limit: eq_any over 1024 ids generates 1024 host params; libsqlite3-sys is bundled (modern SQLite, limit 32766), so no too many SQL variables risk.
• panic! in the new DEFAULT_CACHE_SIZE const: does not trip clippy::panic = "warn" because it's a const context (unlike DEFAULT_CONSOLE_URL, whose #[expect] is needed only because its panic is inside a runtime LazyLock closure). Matches the existing DateTime::TEST precedent.
• cache_size = -{n} correctly maps 65536 → 64 MiB (SQLite negative = KiB), matching the comment.

Minor observations (non-blocking)

1. UUIDv7 embeds a creation timestamp. Report/metric/benchmark UUIDs appear in URLs and API responses; v7 makes millisecond creation time extractable from any UUID. For authorized viewers this is already exposed via created, so there's no new disclosure to unauthorized parties, but a UUID leaked out-of-band (support ticket, screenshot) now also reveals when it was created. Worth a conscious acceptance; almost certainly fine for this platform.

2. OTel counter moved earlier. ReportDelete now increments right after the report row is deleted rather than after version cleanup completes. This is arguably more correct (the report is gone at that point), but it's a slight semantic shift worth noting: a delete that fails during downstream version cleanup now still counts.

3. Error mapping on the read query. delete_report_results maps the report_benchmark id load failure with resource_not_found_err!(Report, ...), though a load failure here is an internal DB error, not a 404. Consistent with surrounding patterns, so acceptable, but semantically the load is a read that "can't not-find."

CLAUDE.md compliance

Clean: TDD followed (tests added), strong typed ids used (ReportBenchmarkId, ReportId::try_from_raw), #[expect(...)] with reasons in test helpers, no emdashes, docs/OpenAPI regenerated, workspace dep versions unchanged, #[cfg(feature = "otel")] gating preserved. The index-cleanup migration's redundancy claims check out (index_report_benchmark_benchmark is a prefix of the (benchmark_id, report_id) index; index_report_benchmark(report_id, benchmark_id) is covered by the UNIQUE(report_id, iteration, benchmark_id) autoindex prefix), and down.sql correctly restores both.

Recommendation: approve. Consider a one-line note in the PR description acknowledging the UUIDv7 timestamp-exposure tradeoff.

---

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

[epompeii]

Addressed the review findings in def06f4:

1. Docs gap: Added the cache_size row to database.mdx in all 9 locales plus example.mdx, updated the modified frontmatter on the server-config content pages, and added a changelog entry under ## Pending v0.6.9.
2. Non-atomic window: Extended the delete_report_results doc comment to cover the crash/partial-state case: the report stays visible with partial results and a stale metric_count_by_report rollup until the delete is retried. This is acceptable because reports are immutable after creation, the delete is idempotent, and the report was already condemned by the caller.
3. UUIDv7 timestamp: Conscious decision, discussed before implementation. The created timestamps are already exposed via the API, and none of the typed uuids act as unguessable capabilities.

Minor items:

• api_run feature placement: The uuid = { workspace = true, features = ["v4"] } line is already in [dev-dependencies] (line 32), not [dependencies]; no change needed.
• Test/const coupling: DELETE_CHUNK_SIZE is now pub (re-exported from api_projects), and reports_delete_chunked computes its row count as 2 * api_projects::DELETE_CHUNK_SIZE, so changing the const can no longer silently defeat the multi-chunk coverage.
• SQLite bind limit: Correct, the bundled libsqlite3-sys is well past 3.32, so the 32766 bind limit applies.

[github-actions]

Bencher Report

Branch	claude/report-delete-perf
Testbed	intel-v1

Click to view all benchmark results

Benchmark	Latency	Benchmark Result microseconds (µs) (Result Δ%)	Upper Boundary microseconds (µs) (Limit %)
Adapter::Json	📈 view plot 🚷 view threshold	4.69 µs (+1.32%) Baseline: 4.63 µs	4.89 µs (95.95%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	4.58 µs (+1.69%) Baseline: 4.50 µs	4.71 µs (97.18%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	25.49 µs (-0.34%) Baseline: 25.58 µs	26.78 µs (95.19%)
Adapter::Rust	📈 view plot 🚷 view threshold	3.52 µs (+0.84%) Baseline: 3.49 µs	3.60 µs (97.65%)
Adapter::RustBench	📈 view plot 🚷 view threshold	3.51 µs (+0.79%) Baseline: 3.49 µs	3.59 µs (97.72%)

🐰 View full continuous benchmarking report in Bencher

[epompeii]

Addressed the round 2 nit in the latest push: cache_size is now Option<NonZeroU32>, so a configured 0 is rejected at config parse time instead of producing PRAGMA cache_size = -0 (no page cache). The OpenAPI spec now carries minimum: 1 and the docs in all 9 locales note the lower bound. There is deliberately no upper bound: sizing the writer cache up is the point of the knob, and server config is trusted admin input.

On the two acks requested: (1) the loss of delete atomicity and (2) the global UUIDv7 timestamp exposure are both deliberate, discussed tradeoffs (see the PR description and the doc comment on delete_report_results).

The resource_not_found_err! mapping on the chunk SELECT matches the existing convention in delete_inner (the report count and version queries map read errors the same way), so leaving that as is for consistency.

[epompeii]

Took the round 4 optional polish in 19ed381: the startup PRAGMA log now includes the resolved cache_size alongside busy_timeout, so operators can confirm their override took effect. The other two observations are intentional: the earlier ReportDelete increment counts the report deletion itself (the version cleanup is best-effort bookkeeping), and the UUIDv7 timestamp exposure is signed off in the PR description and changelog. This should be the final iteration; no further code changes planned.

[epompeii]

Sign-off on concern #1, confirmed with the maintainer before implementation: none of the typed UUIDs are treated as capability secrets. TokenUuid/UserKeyUuid/ProjectKeyUuid are row identifiers; the actual credentials are the separately generated key material and JWTs, which remain untouched (bencher_token and the key generators still use their own random sources). ReportIdempotencyKey is a data-correctness feature, not a security boundary, and the CLI mints it via a raw Uuid::new_v4() call that this PR does not change. Unclaimed organization UUIDs are for demo purposes only, and 74 random bits remain far beyond guessable for that use.

On observation #2: agreed it is a fragility note; the doc comment on delete_report_results names the cascade chain, so a future RESTRICT FK downstream has a breadcrumb to find this loop.

No further changes planned; awaiting the remaining CI jobs.

[epompeii]

Replaces an earlier comment, redacted per repository disclosure policy.

Round 3 recommendation: the UUIDv7 timestamp tradeoff is documented in the PR description (the changelog keeps to user-facing behavior only, per maintainer preference).

On the DELETE_CHUNK_SIZE = 1024 figure: the ~1s per chunk was measured by replaying the exact statement shape against a database copy with a synthesized incident-scale report, with a warm cache. Production storage is slower cold, but the constant is a single tunable and the writer-hold ceiling scales linearly with it if it needs adjusting after observing real deletes.