Push to Bencher Cloud

[epompeii]

• PR Add OCI registry URL to the API server config #676: Add OCI registry URL to the API server config
• PR Add ability to designate a fallback Spec #677: Add ability to designate a fallback Spec

[github-actions]

🤖 Claude Code Review

PR: #679
Base: cloud
Head: devel
Commit: 66754c46a4aafcb59b12bd3a6b7d1c0591e7416a

---

I now have a comprehensive view of both PRs. Here's my review:

---

PR Review: cloud...HEAD (2 commits)

PR #676: Add OCI registry URL to the API server config

PR #677: Add ability to designate a fallback Spec

---

Overall Assessment

Both changes are well-structured, follow project conventions, and have thorough test coverage. A few items worth noting:

Issues & Concerns

1. Dead code: QuerySpec::get_fallback is unused (lib/bencher_schema/src/model/spec/plus.rs)

get_fallback is defined but never called outside of PLAN.md references. This will likely trigger a dead-code warning (or unused lint). It appears to be scaffolding for a future step (Phase B in PLAN.md). Consider adding #[expect(dead_code)] with a comment referencing the planned usage, or deferring the addition until it's needed.

2. PROD_BENCHER_REGISTRY_URL is unused (lib/bencher_json/src/lib.rs)

The PROD_BENCHER_REGISTRY_URL and PROD_BENCHER_REGISTRY_URL_STR constants are declared but never referenced. BENCHER_REGISTRY_URL handles both debug/release via cfg. Same concern as above — either add #[expect(dead_code)] or defer.

3. SQLite trigger and application-level clear are redundant — but safely so (up.sql + specs.rs)

The migration adds enforce_single_fallback_insert and enforce_single_fallback_update triggers as a DB-level safety net. The application code in post_inner and patch_inner calls clear_fallback() before inserting/updating. This belt-and-suspenders approach is fine for correctness, but be aware that if the application-level clear_fallback fails silently, the trigger would abort with a hard "only one spec can be the fallback" error rather than a user-friendly message. This seems acceptable.

4. clear_fallback does a blanket UPDATE ... WHERE fallback IS NOT NULL

This works correctly since the DB invariant guarantees at most one row has fallback IS NOT NULL, but it will update zero rows when there's no current fallback. The execute() return value (number of affected rows) is discarded. This is fine — just noting it for completeness.

5. Migration down.sql drops triggers before table recreation — correct

The down migration properly drops both triggers before recreating the table without the fallback column. This is clean.

Positive Observations

Well-done concurrency control: Both post_inner and patch_inner hold the write lock across the clear + insert/update operations, preventing TOCTOU races on the fallback singleton constraint.

Clock injection: InsertSpec::new and UpdateSpec::new now accept DateTime directly from context.clock.now() instead of calling DateTime::now() internally. This follows the project's test-clock pattern and makes the code deterministic in tests.

Archiving clears fallback: UpdateSpec::new correctly forces fallback = Some(None) when archived == Some(true), ensuring archived specs can't remain the fallback. This is tested by specs_archive_clears_fallback.

Comprehensive tests: ~850 lines of new tests covering: create with/without fallback, replacement semantics, update set/unset, no-change passthrough, archive-clears-fallback, delete-doesn't-auto-promote, and list includes fallback field.

Registry URL changes are consistent: The url field is properly threaded through JsonRegistry → Plus::new() → ApiContext::registry_url with a sensible default_registry_url() fallback based on is_bencher_cloud.

Documentation updated across all 9 languages for the registry config changes.

OpenAPI spec and TypeScript types regenerated — consistent with cargo gen-types output.

Minor / Style

• RegistryDataStore now has a Local variant with proper Sanitize no-op impl — clean.
• JsonRegistry.data_store changed from required to Option<RegistryDataStore> — backward-compatible for existing configs that omit registry entirely (they'll get local storage).
• The PLAN.md diff shows completed tasks removed — good housekeeping.
• Field reordering in JsonSpec (fallback before created/modified, archived after) is a minor API cosmetic change but it's reflected consistently in the OpenAPI spec and TypeScript types.

Summary

The changes are solid. The main actionable item is the unused get_fallback method and PROD_BENCHER_REGISTRY_URL constant — either suppress the lint or defer their addition. Everything else follows project conventions correctly.

---

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

[github-actions]

Bencher Report

Branch	devel
Testbed	ubuntu-22.04

Click to view all benchmark results

Benchmark	Latency	Benchmark Result microseconds (µs) (Result Δ%)	Upper Boundary microseconds (µs) (Limit %)
Adapter::Json	📈 view plot 🚷 view threshold	3.81 µs (+17.19%) Baseline: 3.25 µs	4.39 µs (86.73%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	3.79 µs (+16.16%) Baseline: 3.26 µs	4.36 µs (86.97%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	25.48 µs (+1.22%) Baseline: 25.17 µs	31.36 µs (81.24%)
Adapter::Rust	📈 view plot 🚷 view threshold	2.92 µs (+4.56%) Baseline: 2.79 µs	3.49 µs (83.66%)
Adapter::RustBench	📈 view plot 🚷 view threshold	2.89 µs (+3.65%) Baseline: 2.79 µs	3.48 µs (83.25%)
head_version_insert/batch/10	📈 view plot 🚷 view threshold	103.64 µs (+8.43%) Baseline: 95.58 µs	133.32 µs (77.74%)
head_version_insert/batch/100	📈 view plot 🚷 view threshold	242.65 µs (+4.78%) Baseline: 231.59 µs	276.21 µs (87.85%)
head_version_insert/batch/255	📈 view plot 🚷 view threshold	468.15 µs (+2.49%) Baseline: 456.77 µs	507.81 µs (92.19%)
head_version_insert/batch/50	📈 view plot 🚷 view threshold	163.17 µs (+5.18%) Baseline: 155.14 µs	191.41 µs (85.25%)
threshold_query/join/10	📈 view plot 🚷 view threshold	149.29 µs (+7.79%) Baseline: 138.50 µs	180.36 µs (82.77%)
threshold_query/join/20	📈 view plot 🚷 view threshold	161.53 µs (+5.93%) Baseline: 152.49 µs	194.48 µs (83.06%)
threshold_query/join/5	📈 view plot 🚷 view threshold	140.25 µs (+6.89%) Baseline: 131.21 µs	169.32 µs (82.83%)
threshold_query/join/50	📈 view plot 🚷 view threshold	200.39 µs (+3.80%) Baseline: 193.05 µs	239.19 µs (83.78%)

🐰 View full continuous benchmarking report in Bencher