Harden CI (clippy/audit/lint floor) and fix bench latency accuracy#91
Conversation
…h latency accuracy CI (#83): add a clippy -D warnings job and a cargo-audit job, restrict the push trigger to main (pull_request already covers PRs, so this stops PRs running the workflow twice), add a workspace.lints floor (deny warnings, forbid unsafe_code) that every crate inherits via [lints] workspace = true, and switch deploy/kubernetes/{api,worker}.yaml off imagePullPolicy: IfNotPresent on the floating :latest images to Always. Enabling the deny-warnings floor surfaced ~46 pre-existing clippy warnings across sandboxwich-{core,agent,worker,api}. All but two were auto-fixed via `cargo clippy --fix` (derivable Default impls, `&mut *tx` auto-deref, collapsible if-let chains, a stray into_owned, a needless borrow) with no behavior change. The remaining two are silenced at their call site with a comment rather than refactored: renaming the CLI's `Command::Command` variant would break the CLI's public subcommand name, and the too-many-arguments test helper in http_contract.rs would need a params-struct redesign that's out of scope here. Bench (#84): benchmark_http no longer mixes failed-request durations into the p50/p95/p99 sample set (a fast-failing/refused request has a duration that says nothing about service latency and was deflating the reported percentiles). Failures are still counted and shown in the report; the "requests" column continues to reflect total attempted (success + failure). The POST /sandboxes benchmark's report heading now states the actual post-cap requests/concurrency it ran with instead of implying the configured --requests/--concurrency applied unmodified. Verified: cargo build --workspace, cargo test -p sandboxwich-bench, cargo test --workspace --lib --bins, cargo clippy --workspace --all-targets -- -D warnings, and cargo fmt --all -- --check all pass. Also manually ran the bench http command against a local server that fails 50% of requests with an artificial 500ms delay, confirming the reported p50/p95 reflect only the fast successful path. Closes #83 Closes #84 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015gLTjVy8zxQrStUaPEN9tJ
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| while let Some((duration, status)) = rx.recv().await { | ||
| durations.push(duration); | ||
| if !status.is_success() { | ||
| if status.is_success() { | ||
| durations.push(duration); | ||
| } else { | ||
| failures += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Benchmark now excludes failed requests from latency percentiles
The benchmark_http function now only pushes durations of successful requests into the latency sample set (crates/sandboxwich-bench/src/main.rs:647-648). Previously, ALL request durations (including failures) fed the percentile calculations. This means that if failures are fast (e.g. connection refused), the old code would have deflated latency stats, while the new code gives a cleaner picture of actual service latency. However, rps() at line 1132 now reports only successful requests per wall-clock second rather than total throughput — this is a meaningful semantic change to the benchmark report output that could surprise someone comparing reports across versions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| .execute(&mut *tx) | ||
| .await?; | ||
| } | ||
| fetch_sandbox_file_metadata_on_connection(db, &mut *tx, sandbox_id, file_id).await | ||
| fetch_sandbox_file_metadata_on_connection(db, &mut tx, sandbox_id, file_id).await |
There was a problem hiding this comment.
📝 Info: Mixed &mut tx and &mut *tx style within the same function
In upsert_sandbox_file, the PR changes some call sites to &mut tx (lines 4257, 4304) but leaves the inline .execute(&mut *tx) calls at lines 4283 and 4301 unchanged. Both forms are correct (deref coercion handles the former, explicit deref the latter), but the inconsistency within a single function body is worth noting. The clippy lint needless_borrow would not fire on .execute(&mut *tx) because Executor is implemented directly for &mut Transaction as well as &mut AnyConnection, so both paths compile — it's purely a style inconsistency.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # TODO(follow-up): pin to the sha-<12> tag containers.yml already | ||
| # publishes instead of floating :latest, so deploys correlate to a | ||
| # commit. Always at least forces a pull instead of trusting a | ||
| # node's cached (possibly stale) :latest image. | ||
| imagePullPolicy: Always |
There was a problem hiding this comment.
📝 Info: imagePullPolicy change from IfNotPresent to Always has operational cost
Changing imagePullPolicy to Always for the :latest tag (deploy/kubernetes/api.yaml:31, deploy/kubernetes/worker.yaml:79) is correct practice for floating tags (avoids stale cached images), but it means every pod restart will hit the container registry. In air-gapped or rate-limited environments this could cause startup failures. The TODO comments already note that pinning to a SHA-based tag is the proper follow-up.
Was this helpful? React with 👍 or 👎 to provide feedback.
…bench-accuracy # Conflicts: # crates/sandboxwich-agent/src/main.rs # crates/sandboxwich-api/src/main.rs # crates/sandboxwich-api/tests/http_contract.rs
Re-runs clippy --fix across the merged tree so the new -D warnings gate passes, and restores the too_many_arguments allow on a test helper that was dropped when taking main's version during conflict resolution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015gLTjVy8zxQrStUaPEN9tJ
2 participants
Summary
CI (#83) —
.github/workflows/ci.ymlwas thin (fmt/check/test only) and ran on everypushin addition topull_request, doubling CI runs on PR branches. Deploy manifests floated:latestwithIfNotPresent.clippyjob:cargo clippy --workspace --all-targets -- -D warnings.auditjob:cargo auditviataiki-e/install-action@cargo-audit, with--ignore RUSTSEC-2023-0071(rsa timing sidechannel, pulled in transitively throughsqlx-mysqleven though this workspace only enables the postgres/sqlite sqlx features; no fixed version exists upstream — worth revisiting when one ships).pushtrigger tomainonly;pull_requeststill covers PRs, so a PR branch no longer runs the whole workflow twice.[workspace.lints.rust]to the rootCargo.toml(warnings = "deny",unsafe_code = "forbid") and[lints] workspace = truein every crate. The tree has nounsafecode and, after the clippy cleanup below, compiles clean under this floor — verified locally withcargo clippy --workspace --all-targets -- -D warnings.deploy/kubernetes/worker.yamlandapi.yaml: switchedimagePullPolicy: IfNotPresent→Alwayson the three:latest-tagged containers (worker, api, api-migrate job). Noted inline as a TODO: the real fix is pinning to thesha-<12>tagscontainers.ymlalready publishes so a deploy correlates to a commit;Alwaysis the minimal stopgap so a node doesn't keep serving a stale cached:latest.Turning on the lint floor surfaced ~46 pre-existing clippy warnings across
sandboxwich-core,-agent,-worker, and-api(derivableDefaultimpls,&mut *txauto-deref, collapsibleif letchains, a redundant.into_owned(), a needless borrow). All of those were mechanical, behavior-preserving fixes applied viacargo clippy --fixplus acargo fmt. Two were left in place with a targeted#[allow(...)]and an explanatory comment instead of a real fix, since fixing them properly is out of scope here:sandboxwich-cli: theCommand::Commandvariant name — renaming it would rename thesandboxwich command <id>CLI subcommand, a user-facing breaking change.sandboxwich-api/tests/http_contract.rs: a test-only insert helper with 8 arguments — collapsing it into a params struct is a larger test-harness change than belongs in a CI cleanup PR.Bench (#84) —
crates/sandboxwich-bench/src/main.rs:benchmark_httpno longer folds failed-request durations into the p50/p95/p99 sample set. A refused/timed-out/5xx request's duration says nothing about actual service latency, and mixing it in was deflating the reported percentiles while the run still looked "mostly successful." Failures are still counted and reported (a newattemptedfield onBenchSummary= successes + failures keeps the "requests" column showing the total, same as before).POST /sandboxesbenchmark caps atrequests.min(250)/concurrency.min(20)since it creates real rows. The report heading for that section now states the actual post-cap counts (e.g.POST /sandboxes (capped: 250 requests, concurrency 20)) instead of silently running fewer requests than the configured--requests/--concurrencyimplied.Manually verified the bench fix against a local HTTP server that fails 50% of requests with an artificial 500ms delay on the failure path: before the fix this would show p50/mean around 250ms+; after the fix it correctly reports ~7-9ms (the fast successful path) with failures counted separately.
Test plan
cargo build --workspacecargo test -p sandboxwich-bench(3 passed)cargo test --workspace --lib --bins(all unit tests across every crate pass)cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warnings(clean)sandboxwich-bench httpagainst a flaky local server (50% failures, 500ms delay on failures) — confirmed p50/p95 no longer reflect the failure-path latency andfailures/requestscounts are both correctCloses #83
Closes #84
🤖 Generated with Claude Code