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: ea238558973158c697022900130b1c3672724caf

---

I have completed a thorough review. Here is my assessment.

Code Review: Speed up large report deletions

Overall this is a well-constructed, well-documented change. The core idea (chunk the report_benchmark cascade delete so the single writer connection is released between batches) is sound, the cascade chain and rollup cleanup check out, and the tests exercise the important boundary. No blocking bugs found. Notes below, roughly by severity.

Correctness (verified, no blockers)

• Chunked delete loop is safe from infinite looping. delete_report_results reads up to DELETE_CHUNK_SIZE ids via auth_conn! then deletes via write_conn! in autocommit. In WAL mode each subsequent read sees the prior committed delete, so the loop converges. All children (report_benchmark → metric → boundary → alert) are ON DELETE CASCADE (confirmed in migrations), so a chunk delete can't hit a RESTRICT FK and spin. Good.
• Rollup cleanup is correct. metric_count_by_report and job_duration_by_report both have report_id … ON DELETE CASCADE, so the final report-row delete cleans them up. The doc comment's "stale rollup until retried" caveat for the mid-loop failure window is accurate.
• Non-atomic delete is documented and acceptable given reports are immutable and the delete is idempotent. Reasonable trade-off, clearly explained in the comment.

UUIDv7 change (typed_uuid.rs) — the highest-impact part

Switching every typed UUID from v4 to v7 is a broad behavioral change bundled into a "report deletion" PR. Worth calling out:

• Timestamp leakage (low severity). v7 embeds the creation time (ms) and drops random entropy from 122 → 74 bits. For Bencher's UUIDs this looks benign: they're resource identifiers behind RBAC, public projects are public by design, and secrets/tokens use Secret/JWT rather than these UUIDs (verified bencher_token, DEFAULT_SECRET_KEY, and CLI idempotency_key all still use v4). No security regression I can find, but the timestamp exposure is a real (minor) new property; confirm no UUID is ever relied on as an unguessable capability.
• WASM is unaffected — the browser build is bencher_valid, which does not pull in bencher_json, so now_v7()'s SystemTime dependency never reaches wasm32. Good.
• Consider splitting the v7 migration into its own PR/commit, or at least noting it prominently in the changelog, since it changes ID semantics platform-wide.

Minor / nits

• Error mapping in delete_report_results: the chunk SELECT maps failures to resource_not_found_err!(Report, …) (404), but the report was just confirmed to exist — a genuine DB error here would surface as a misleading 404. resource_conflict_err! (as used for the delete) would be more accurate. Low impact; may match existing conventions.
• api_run v4 feature: uuid v4 is only used in api_run's tests, so features = ["v4"] could live on a dev-dependency rather than the main dep. Cosmetic.
• Test helpers use raw i32 IDs (seed_result_infra returns (i32, i32, i32, i32)), which is against the CLAUDE.md "strong types even in tests" guidance — though it matches the pre-existing get_project_id/create_test_report helper signatures, so this is consistent with surrounding code rather than new drift.

Standards compliance

• No emdashes; comments explain the "why"; OTel ReportDelete metric preserved (moved earlier, benign). ✅
• Strong type ReportBenchmarkId used for the loaded column. ✅
• openapi.json regenerated for the new cache_size field; server-config docs updated across all locales and the changelog. ✅
• Index-cleanup migration claims verified: index_report_benchmark_benchmark(benchmark_id) is a redundant prefix of index_report_benchmark_benchmark_report(benchmark_id, report_id), and index_report_benchmark(report_id, benchmark_id) is covered by the UNIQUE(report_id, iteration, benchmark_id) autoindex for the report_id-only delete lookup. Both drops are safe; down.sql restores them. ✅
• cache_size PRAGMA correctly writer-only with negative (KiB) value; NonZeroU32 → minimum: 1 in the spec. ✅

Suggestion

The DELETE_CHUNK_SIZE = 1024 is validated "at ~1s per chunk against a production-scale report" — per the repo's public-disclosure policy, that's fine as written (relative terms), just make sure no concrete production figures leak into the eventual PR description or changelog.

Nice work overall. The only thing I'd genuinely push on is decoupling/highlighting the v4→v7 switch, since it silently changes ID semantics for the entire platform.

---

_{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.71 µs (+1.91%) Baseline: 4.62 µs	4.90 µs (96.09%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	4.57 µs (+1.62%) Baseline: 4.50 µs	4.72 µs (96.81%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	25.90 µs (+1.41%) Baseline: 25.54 µs	26.64 µs (97.23%)
Adapter::Rust	📈 view plot 🚷 view threshold	3.54 µs (+1.47%) Baseline: 3.49 µs	3.60 µs (98.24%)
Adapter::RustBench	📈 view plot 🚷 view threshold	3.53 µs (+1.49%) Baseline: 3.48 µs	3.59 µs (98.39%)

🐰 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.