Process bare metal job results

[epompeii]

This changeset add processing of bare metal job results.
The Report is created initially. Then once the job is completed, results are connected to the Report.

[github-actions]

🤖 Claude Code Review

PR: #680
Base: devel
Head: u/ep/job-results
Commit: 9461f63ff60dfc4ad011674becacab531a2ac3ca

---

Now I have all the results. Let me compile the comprehensive review.

---

Pull Request Review: Multi-Iteration Benchmarking & Job Processing

Scope: 87 files changed, +4,973 / -1,576 lines across 9 commits. Introduces multi-iteration benchmark support, a new Processed job status with two-phase completion, centralized auth header helpers, and UpdateJob refactoring.

---

Critical Issues

1. Old JsonJobOutput blobs in OCI storage will fail to deserialize

Files: lib/bencher_json/src/runner/job.rs, lib/bencher_schema/src/model/runner/job.rs

The old JsonJobOutput had a flat shape (exit_code, stdout, stderr, output, error). The new shape has results: Vec<JsonIterationOutput> (required, no #[serde(default)]). Any jobs stored in OCI with the old format before this deploy will fail to deserialize during reprocess_completed_jobs() at startup, or if fetched via the API.

Recommendation: Add a custom Deserialize impl or #[serde(deserialize_with = "...")] that handles both the old flat format and the new results-based format, or confirm that no old-format blobs can exist in production at deploy time.

---

Medium Issues

2. process_results database writes are not transactional

File: plus/api_runners/src/jobs/websocket.rs:456-527

The handle_completed flow performs multiple database writes (metrics, alerts, report timestamps) without wrapping them in a transaction. A crash mid-processing could leave partial metrics committed. On retry via reprocess_completed_jobs, the adapter would re-process and potentially create duplicate metrics.

Recommendation: Either wrap process_results + report time update in a transaction at the call site, or document that the adapter/report pipeline is idempotent on retry.

3. Iteration(0) silently produces zero iterations

Files: lib/bencher_json/src/project/report.rs, plus/bencher_runner/src/up/job.rs:48, services/cli/src/bencher/sub/run/mod.rs

Iteration(0) is accepted by FromStr and is the Default value. In the runner, job.config.iter.map_or(1, Iteration::as_usize) returns 0 for Some(Iteration(0)), producing an empty results vec. In the CLI, for _ in 0..0 is similarly a no-op. While there is a test scenario for this, it may surprise users.

Recommendation: Consider enforcing >= 1 in Iteration::from_str / TryFrom, or document that 0 means "dry run with no execution."

4. OCI storage failure before process_results can cause data loss

File: plus/api_runners/src/jobs/websocket.rs:495-497

If store_job_output fails, the code continues to process_results. If processing succeeds and transitions to Processed, the output is permanently lost (no stored blob for debugging/auditing). If processing later fails, reprocess_completed_jobs will also fail to find the output.

Recommendation: Consider failing the job if OCI storage fails, or at minimum adding a metric/log when processing succeeds but storage was lost.

---

Low Issues

5. Iteration::as_usize uses as cast instead of From

File: lib/bencher_json/src/project/report.rs

pub fn as_usize(self) -> usize {
    self.0 as usize  // prefer usize::from(self.0)
}

While safe on all Rust tier-1 targets, usize::from(self.0) is more idiomatic and explicitly communicates infallibility.

6. DateTime::now() in tests (pre-existing, partially touched by this PR)

Files: plus/api_runners/tests/jobs/main.rs (2 calls), plus/api_runners/tests/jobs/websocket.rs (4 calls as Clock::Custom seeds)

Per CLAUDE.md, tests should use deterministic time. While pre-existing, the PR touched these files for auth header migration and could have fixed them.

7. Missing #[typeshare] on WebSocket protocol types

File: lib/bencher_json/src/runner/websocket.rs

RunnerMessage, ServerMessage, and CloseReason lack #[typeshare] annotations. If these are intentionally server-only (not REST), this is fine but worth documenting.

8. auth module naming overlap with system::auth

File: lib/bencher_json/src/auth.rs

The new bencher_json::auth module (bearer token helpers) shares a name with bencher_json::system::auth (auth JSON types like JsonLogin, JsonSignup). A doc comment distinguishing them would help.

9. write_conn! used for read-only query

File: lib/bencher_schema/src/model/runner/job.rs:932

reprocess_completed_jobs uses write_conn! for a SELECT. Defensible at startup (no auth context available), but deviates from the macro conventions documented in CLAUDE.md.

---

Positive Observations

• Migration correctness: The job_processed migration correctly re-maps status integers in the right order to avoid collisions (reverse for up, ascending for down). The report_head_index migration adds a useful partial index.
• Strong type discipline: Consistent use of JobId, JobUuid, DateTime, Iteration, BranchId, ReportId, etc. throughout. BTreeMap used for deterministic serialization in output fields.
• UpdateJob refactoring: Named constructors (terminate, heartbeat, start, claim, set_status) and execute_if_status methods are clean and encapsulate diesel patterns well.
• Auth centralization: bencher_json::AUTHORIZATION, bearer_header(), and strip_bearer_token() eliminate scattered string literals across 37+ files. The server.bearer() method was properly removed.
• Error handling: All error types use thiserror. No anyhow or Box<dyn Error> in library/service code. ChannelError has specific variants.
• WebSocket security: Migration from Sec-WebSocket-Protocol subprotocol auth to standard Authorization: Bearer header. CloseReason JSON fits within RFC 6455's 125-byte close frame limit (tested).
• Two-phase completion: Completed -> Processed separation enables crash-safe recovery via reprocess_completed_jobs. TOCTOU races prevented by execute_if_status pattern.
• Test coverage: 8 new tests covering reprocessing, multi-iteration, and adapter failure scenarios. Auth migration is uniform across all 37 test files. #[expect] used correctly over #[allow].
• CLAUDE.md compliance: Destructuring patterns used for TryFrom impls, QueryResult<T> return types, connection macros, camino paths, and clap conventions all followed.
• OpenAPI + TypeScript types correctly regenerated to reflect the new Iteration, JsonIterationOutput, Processed status, and restructured JsonJobOutput.

---

Summary

Priority	Count	Key Items
Critical	1	Old JsonJobOutput OCI blob backwards compatibility
Medium	3	Non-transactional process_results, Iteration(0) semantics, OCI storage failure handling
Low	5	Style/naming/convention items

The code quality is high overall. The UpdateJob refactoring, auth centralization, and two-phase job completion are well-designed. The critical item around old OCI blob deserialization should be resolved before merge if there's any chance of pre-existing stored job output in the old format.

---

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

[github-actions]

Bencher Report

Branch	u/ep/job-results
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.70 µs (+13.28%) Baseline: 3.26 µs	4.42 µs (83.70%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	3.65 µs (+11.60%) Baseline: 3.27 µs	4.38 µs (83.52%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	25.66 µs (+1.79%) Baseline: 25.21 µs	31.28 µs (82.06%)
Adapter::Rust	📈 view plot 🚷 view threshold	2.84 µs (+1.73%) Baseline: 2.80 µs	3.49 µs (81.57%)
Adapter::RustBench	📈 view plot 🚷 view threshold	2.81 µs (+0.46%) Baseline: 2.80 µs	3.47 µs (80.87%)
head_version_insert/batch/10	📈 view plot 🚷 view threshold	100.93 µs (+5.27%) Baseline: 95.88 µs	128.79 µs (78.37%)
head_version_insert/batch/100	📈 view plot 🚷 view threshold	237.41 µs (+2.48%) Baseline: 231.66 µs	270.06 µs (87.91%)
head_version_insert/batch/255	📈 view plot 🚷 view threshold	463.68 µs (+1.61%) Baseline: 456.33 µs	500.96 µs (92.56%)
head_version_insert/batch/50	📈 view plot 🚷 view threshold	160.22 µs (+3.13%) Baseline: 155.35 µs	186.77 µs (85.78%)
threshold_query/join/10	📈 view plot 🚷 view threshold	143.90 µs (+3.81%) Baseline: 138.62 µs	174.67 µs (82.38%)
threshold_query/join/20	📈 view plot 🚷 view threshold	156.05 µs (+2.28%) Baseline: 152.57 µs	188.71 µs (82.69%)
threshold_query/join/5	📈 view plot 🚷 view threshold	135.70 µs (+3.46%) Baseline: 131.16 µs	163.95 µs (82.77%)
threshold_query/join/50	📈 view plot 🚷 view threshold	199.95 µs (+3.50%) Baseline: 193.19 µs	232.94 µs (85.84%)

🐰 View full continuous benchmarking report in Bencher