Store job output

[epompeii]

Store job output alongside OCI images for a project.

[github-actions]

Bencher Report

Branch	u/ep/output
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.74 µs (+8.82%) Baseline: 3.44 µs	4.64 µs (80.70%)
Adapter::Magic (JSON)	📈 view plot 🚷 view threshold	3.72 µs (+8.05%) Baseline: 3.45 µs	4.59 µs (81.05%)
Adapter::Magic (Rust)	📈 view plot 🚷 view threshold	26.04 µs (+1.08%) Baseline: 25.76 µs	29.53 µs (88.17%)
Adapter::Rust	📈 view plot 🚷 view threshold	3.00 µs (+7.54%) Baseline: 2.79 µs	3.11 µs (96.33%)
Adapter::RustBench	📈 view plot 🚷 view threshold	2.99 µs (+7.32%) Baseline: 2.79 µs	3.11 µs (96.37%)
head_version_insert/batch/10	📈 view plot 🚷 view threshold	95.69 µs (+8.10%) Baseline: 88.52 µs	103.29 µs (92.64%)
head_version_insert/batch/100	📈 view plot 🚷 view threshold	233.23 µs (+3.28%) Baseline: 225.82 µs	241.47 µs (96.59%)
head_version_insert/batch/255	📈 view plot 🚷 view threshold	454.80 µs (+1.17%) Baseline: 449.55 µs	477.54 µs (95.24%)
head_version_insert/batch/50	📈 view plot 🚷 view threshold	157.07 µs (+5.46%) Baseline: 148.94 µs	166.76 µs (94.19%)
threshold_query/join/10	📈 view plot 🚷 view threshold	136.84 µs (+3.18%) Baseline: 132.63 µs	148.26 µs (92.30%)
threshold_query/join/20	📈 view plot 🚷 view threshold	154.08 µs (+4.13%) Baseline: 147.97 µs	164.89 µs (93.44%)
threshold_query/join/5	📈 view plot 🚷 view threshold	131.57 µs (+4.68%) Baseline: 125.69 µs	142.94 µs (92.05%)
threshold_query/join/50	📈 view plot 🚷 view threshold	198.89 µs (+5.08%) Baseline: 189.27 µs	211.02 µs (94.25%)

🐰 View full continuous benchmarking report in Bencher

[github-actions]

🤖 Claude Code Review

PR: #665
Base: devel
Head: u/ep/output
Commit: ff15e22a88279f7a861833bf2801c3ee530d5e86

---

Here's my review:

---

PR Review: Job Output Storage & Schema Changes

Summary

This PR moves job output (stdout, stderr, exit code, error, files) from a database column (exit_code) to blob storage (S3/local filesystem via OciStorage). It introduces a JsonJobOutput struct, stores output on job completion/failure, and retrieves it on single-job GET requests for terminal jobs. Also consolidates test helpers into bencher_api_tests::helpers and replaces Box<dyn Error> with a proper ChannelError enum.

Positives

• Good architectural decision to move potentially large output data out of SQLite and into blob storage. This avoids bloating the DB with unbounded stdout/stderr content.
• Proper error typing: Replacing Box<dyn std::error::Error + Send + Sync> with ChannelError in internal functions follows the project's coding standards. The WebsocketChannelResult boundary still uses Box<dyn Error> which is acceptable since it's a dropshot API requirement.
• Best-effort storage: Output storage failures are logged but don't fail the state transition, which is the right tradeoff for an append-only artifact.
• S3 cache-control: "public, max-age=31536000, immutable" on put is appropriate since terminal job output never changes.
• Test consolidation: Extracting shared helpers (create_test_report, get_project_id, set_job_status, base_timestamp) into bencher_api_tests::helpers reduces duplication.
• Deterministic timestamps: Replacing DateTime::now() with base_timestamp() in tests aligns with the project's requirement for deterministic time-based tests.
• Good test coverage: New tests for the storage layer, backwards compatibility of JSON shapes, single-job GET with/without output, and various job states.

Issues

1. Migration changes status CHECK constraint (Medium)

up.sql:58 changed from CHECK (status >= 0 AND status <= 5) to CHECK (status >= 0). This removes the upper-bound guard, allowing any non-negative integer as a status. While this makes adding future statuses easier, it loses the DB-level validation that prevents invalid status values from being written. Consider whether this was intentional or accidental.

2. ChannelError is missing an OciStorageError variant (Low, but note)

OciStorageError is imported in channel.rs:8 but only used in store_job_output's return type. Since storage errors are handled inline with if let Err(e), this works, but the import may be confusing. Minor point — no code change needed.

3. No size limit on stored job output (Medium)

JsonJobOutput can contain arbitrarily large stdout, stderr, and output HashMap content. The WebSocket max_message_size provides a transport-level bound, but there's no explicit validation on the content size before serializing and storing to S3/local. For S3, this could result in large objects. The design doc mentions relying on max_message_size, which is reasonable, but worth noting as a conscious decision.

4. #[allow(dead_code, unused_imports)] in test common module (Low)

plus/api_runners/tests/common/mod.rs:3 adds unused_imports to the #[allow(...)]. The project standard is #[expect(...)] rather than #[allow(...)]. However, since this is a test common/ module included by multiple test binaries (where different subsets of helpers are used), #[allow(dead_code)] is an existing pattern here and arguably acceptable. Still, if there's a way to use #[expect(dead_code)] it would be preferred.

5. helpers module gated on plus but not oci (Low)

lib/bencher_api_tests/src/lib.rs:36-37: The helpers module is gated on #[cfg(feature = "plus")]. The helpers reference schema::job::* columns which are also behind the plus feature gate, so this is correct. However, api_projects/tests/jobs.rs imports from helpers — ensure the test's Cargo.toml activates the plus feature on bencher_api_tests.

6. Single-job GET does an async storage call in a sync-ish path (Info)

lib/api_projects/src/jobs.rs:238-249: The get_one_inner function fetches output from blob storage for terminal jobs. For S3, this is a network call on each GET. This is fine for single-job retrieval and the design doc explicitly calls out that list endpoints skip this. Just noting that this path adds latency proportional to S3 response time.

7. poll_timeout test uses tokio::time::pause() instead of wall-clock assertion (Good)

The change in plus/api_runners/tests/jobs.rs:2737-2748 replaces a flaky wall-clock timing assertion with tokio::time::pause(). This is a genuine improvement for test determinism.

Style/Standards Compliance

• cargo gen-types was run: openapi.json and bencher.ts are updated consistently.
• CLAUDE.md additions (re: cargo fmt after all changes, Box<dyn Error> guidance) are reasonable documentation improvements.
• Types use strong typing (ProjectUuid, JobUuid, etc.) throughout, per project standards.
• #[serde(skip_serializing_if = "Option::is_none")] on all optional JsonJobOutput fields keeps the wire format clean.

Security

• No new user input parsing vulnerabilities. Job output is written/read by the server based on trusted UUIDs.
• S3 keys use UUIDs from the database, not user-controlled strings — no path traversal risk.
• Local storage uses fs::create_dir_all with UUID-based paths — safe.

Overall

This is a well-structured change. The main item worth discussing is the relaxed status CHECK constraint in the migration (issue #1) and whether the lack of output size validation (issue #3) is acceptable for the deployment context.

---

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