Expand CI test coverage for the bare metal runner

[epompeii]

• Test against development deployment with runner
• Remove redundant runner tests

[github-actions]

🤖 Claude Code Review

PR: #686
Base: devel
Head: u/ep/ci-runner
Commit: a2d32b051b27a7cd64cd64a3f76f19d5180d6700

---

Here's my review of this PR:

---

PR Review: Runner Test Consolidation & WebSocket TLS Fix

Summary

This PR does two main things:

1. Consolidates runner integration tests from test_runner into test_api, removing the standalone test_runner test/oci/clean subcommands and the separate CI job
2. Fixes WebSocket TLS cert verification by switching tungstenite to rustls-tls-webpki-roots and installing a rustls crypto provider in the runner binary

Positive Changes

• Significant code reduction (~1000 lines deleted) by removing duplicated test orchestration from test_runner and smoke_test.rs
• Good use of destructuring in TryFrom<TaskRunner> (line 27-33 of runner.rs)
• Proper #[cfg(feature = "plus")] gating throughout
• wait_for_stdout_ready is a clean readiness-probe pattern instead of arbitrary sleeps
• Docker login correctly uses --password-stdin to avoid leaking credentials in process args

Issues

Bug: wait_for_stdout_ready panics instead of returning an error

tasks/test_api/src/task/runner.rs:371-375 — The assert! macro will panic the calling thread instead of returning an anyhow::Result. Since this runs on the main thread, it will abort the entire process without allowing cleanup (the runner daemon won't be killed). Consider returning an error from this function so the caller can handle cleanup gracefully.

// Current: panics, skipping cleanup
assert!(
    start.elapsed() < timeout,
    "Timed out waiting for '{sentinel}' from [{label}]"
);

// Suggested: return Result so caller can clean up
anyhow::ensure!(
    start.elapsed() < timeout,
    "Timed out waiting for '{sentinel}' from [{label}]"
);

This would require changing the function signature to return anyhow::Result<JoinHandle<()>> and having the caller handle it.

Potential Issue: stderr not captured from runner daemon

tasks/test_api/src/task/runner.rs:148-161 — Only stdout is piped for readiness detection. If the runner daemon writes its log output to stderr (common with tracing/log crates), the sentinel "Polling for jobs" would never be found, causing a 30-second timeout. Verify which stream the runner uses for logging.

Minor: runner_child shadowing

tasks/test_api/src/task/runner.rs:149-150 — The mut runner_child variable is declared twice via shadowing:

let mut runner_child = Command::cargo_bin("runner")?;
let mut runner_child = runner_child
    .args([...])
    .spawn()?;

This works but is slightly confusing. Consider using a different name for the Command vs the Child.

CI: continue-on-error: true for runner test

.github/workflows/deploy.yml:21 — The runner test uses continue-on-error: true with a separate "Check Runner Test Result" step. This is fine for a phased rollout, but note that the step ID runner-test must remain stable — if it's renamed, the check step silently becomes a no-op (always passes). Consider adding a comment about this coupling.

test_runner crate: plus feature is now empty

tasks/test_runner/Cargo.toml:11 — plus = [] is an empty feature with no feature flags behind it. The entire parser and task modules are gated on #[cfg(feature = "plus")] in main.rs, but the plus feature doesn't actually enable anything in the dependency tree anymore. This works but the feature is essentially just a compile-time toggle with no downstream effect on dependencies.

Style/CLAUDE.md Compliance

• anyhow usage in tasks/ crates is correct per CLAUDE.md policy
• camino::Utf8Path is used for KVM device check — good
• Workspace dependency shorthand (rustls.workspace = true) is used correctly
• Dockerfile ordering change (moving cargo init --lib stubs before COPY for non-essential crates) looks correct for build caching

Security

• No new security concerns. The token handling via --password-stdin for Docker login is the correct approach.
• The webpki-roots addition for TLS cert verification is a security improvement — the previous __rustls-tls feature was missing root certificate verification for WebSocket connections.

Verdict

The consolidation is clean and the TLS fix is important. The main actionable item is the panic in wait_for_stdout_ready which could leave the runner daemon orphaned on timeout. The stderr vs stdout question for readiness detection should also be verified.

---

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

[github-actions]

Bencher Report

Branch	u/ep/ci-runner
Testbed	ubuntu-22.04

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

• Latency (nanoseconds (ns))

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds flag.

Click to view all benchmark results

Benchmark	Latency	microseconds (µs)
Adapter::Json	📈 view plot ⚠️ NO THRESHOLD	3.55 µs
Adapter::Magic (JSON)	📈 view plot ⚠️ NO THRESHOLD	3.32 µs
Adapter::Magic (Rust)	📈 view plot ⚠️ NO THRESHOLD	22.15 µs
Adapter::Rust	📈 view plot ⚠️ NO THRESHOLD	2.61 µs
Adapter::RustBench	📈 view plot ⚠️ NO THRESHOLD	2.60 µs
head_version_insert/batch/10	📈 view plot ⚠️ NO THRESHOLD	93.67 µs
head_version_insert/batch/100	📈 view plot ⚠️ NO THRESHOLD	236.86 µs
head_version_insert/batch/255	📈 view plot ⚠️ NO THRESHOLD	467.61 µs
head_version_insert/batch/50	📈 view plot ⚠️ NO THRESHOLD	160.34 µs
threshold_query/join/10	📈 view plot ⚠️ NO THRESHOLD	147.61 µs
threshold_query/join/20	📈 view plot ⚠️ NO THRESHOLD	162.38 µs
threshold_query/join/5	📈 view plot ⚠️ NO THRESHOLD	139.09 µs
threshold_query/join/50	📈 view plot ⚠️ NO THRESHOLD	204.50 µs

🐰 View full continuous benchmarking report in Bencher