Add an idempotency key to the run API endpoint

[epompeii]

This changeset adds an idempotency_key to the run API endpoint to prevent the following issue:

1. bencher run sends a Report to the API server
2. The Report is created
3. Before a response can be sent, the server shuts down
4. bencher run retires sending the Report
5. A second Report is created

With this change, the idempotency_key would make it so 5. The original Report is returned

[github-actions]

🤖 Claude Code Review

PR: #760
Base: devel
Head: u/ep/idempotency-key
Commit: 55db88e0864e508a43ab66e85686159cbef53a53

---

Here's my review:

---

PR Review: Add idempotency_key to bencher run

Overall

Clean feature addition that deduplicates retried run submissions using a UUID idempotency key scoped per-project. The migration, schema, API, CLI, and tests are all present and consistent.

Issues

1. CLI always generates a new idempotency key per call (potential design concern)

services/cli/src/bencher/sub/run/mod.rs:283,317 — Both generate_local_report and generate_remote_report call uuid::Uuid::new_v4() fresh each time. This means retrying a CLI invocation (e.g. bencher run ... twice) will produce different keys, so the idempotency check will never fire from the CLI. The feature only works if the same serialized JsonNewRun payload (with the same key) is submitted again — which happens with HTTP-level retries within a single CLI invocation, but not across separate CLI invocations.

Is this the intended scope? If so, the changelog entry "deduplicate retried submissions" is accurate only for in-process retries. If cross-invocation deduplication is desired (e.g., CI reruns), the key would need to be deterministic (derived from branch + hash + testbed + results hash, or user-supplied).

2. Race condition between idempotency check and insert

lib/bencher_schema/src/model/project/report/mod.rs:159-166 — The idempotency check (check_idempotency) runs on a read connection (public_conn!), then the insert happens later inside a write_conn! transaction. Between the read and the write, a concurrent request with the same key could pass the check and attempt to insert, hitting the unique index constraint and returning a 500 instead of the existing report.

Consider catching the unique constraint violation on insert and falling back to a lookup, or moving the check inside the write transaction.

3. idempotency_key variable moves but is used after move

lib/bencher_schema/src/model/project/report/mod.rs:163 — idempotency_key is passed by value into check_idempotency, then used again at line 283 in InsertReport::from_json(idempotency_key, ...). This looks like it should be a compile error unless ReportIdempotencyKey implements Copy. Since it's a UUID newtype wrapping String-backed text, it likely doesn't implement Copy.

Wait — looking more carefully, check_idempotency takes Option<ReportIdempotencyKey> and the outer variable is also Option<ReportIdempotencyKey>. If idempotency_key is Some(...), the function consumes it and returns Some(existing), so the code returns early. If it's None, the function returns Ok(None) and idempotency_key was None at line 283 too. So the only remaining path to line 283 is when the key is Some(...) but no existing report was found — in that case the value was consumed. This would be a compile error. Either ReportIdempotencyKey implements Copy/Clone, or there's a .clone() somewhere not visible in the diff. Worth verifying this compiles.

4. No TTL/expiry on idempotency keys

The idempotency key lives in the report table forever. For a continuous benchmarking platform with high-volume submissions, these nullable columns accumulate indefinitely. Consider whether a cleanup policy or TTL is needed. Not blocking, but worth a tracking issue.

5. No user-scoping on idempotency key

The unique index is (project_id, idempotency_key). Any authenticated user who can submit runs to a project can collide with another user's idempotency key. If user A submits with key X, user B submitting with the same key X to the same project will get user A's report back. Since the key is a random UUIDv4, collision probability is negligible, but the semantics are worth documenting.

Minor

• The down.sql migration correctly drops the index and recreates the table without idempotency_key — looks good.
• The partial unique index (WHERE idempotency_key IS NOT NULL) correctly avoids treating all NULLs as duplicates.
• Tests cover the three main cases (same key, different keys, no key) — solid coverage.
• #[serde(skip_serializing_if = "Option::is_none")] on idempotency_key is good for backward-compatible serialization.
• The diesel::joinable! reorder in schema.rs is cosmetic (alphabetical sort) — fine.

Summary

The core design is sound. The critical item to verify is whether idempotency_key after the check_idempotency call actually compiles (item 3). The race condition (item 2) should be addressed before merge. The CLI always generating a fresh key (item 1) is a design decision that should be explicitly documented or reconsidered depending on the intended use case.

---

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

[github-actions]

Bencher Report

Branch	u/ep/idempotency-key
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.54 µs (-0.11%) Baseline: 4.54 µs	4.66 µs (97.39%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	4.41 µs (-0.12%) Baseline: 4.42 µs	4.49 µs (98.25%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	24.95 µs (-0.42%) Baseline: 25.06 µs	26.41 µs (94.49%)
Adapter::Rust	📈 view plot 🚷 view threshold	3.36 µs (-0.25%) Baseline: 3.37 µs	3.50 µs (96.20%)
Adapter::RustBench	📈 view plot 🚷 view threshold	3.36 µs (-0.29%) Baseline: 3.37 µs	3.53 µs (95.43%)

🐰 View full continuous benchmarking report in Bencher