Skip to content

chore(ingest): take the crate to zero clippy warnings under the new lint set - #523

Merged
JeremyFunk merged 1 commit into
mainfrom
chore/ingest-clippy-clean
Aug 19, 2026
Merged

chore(ingest): take the crate to zero clippy warnings under the new lint set#523
JeremyFunk merged 1 commit into
mainfrom
chore/ingest-clippy-clean

Conversation

@JeremyFunk

@JeremyFunkJeremyFunk commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Turns on a strict clippy lint set for apps/ingest and takes the crate to zero
warnings under it. Self-contained against main — the lint config
(apps/ingest/Cargo.toml[lints.rust]/[lints.clippy] plus a root
clippy.toml) ships in this PR.

cargo clippy --all-targets: 735 unique warnings → 0. cargo test: 152
passed, the same count as main.

What was mechanical

cargo clippy --fix --all-targets covered ~570: str_to_string,
map_unwrap_or, use_self, uninlined_format_args, unreachable_pub,
manual_let_else, unused_qualifications, redundant_closure_for_method_calls,
duration_suboptimal_units.

What needed a decision

Casts — reviewed individually rather than blanket-allowed:

SiteCall
encode_wal_frameFolded the three width checks into the try_froms that needed them, and added the missing row_count guard — a usize row count over u32::MAX was silently truncating into the WAL header. The only real truncation bug in the set.
Duration → u64/i64 millistry_from(..).unwrap_or(MAX) behind a duration_millis helper; current_time_millis now returns the i64 both callers were casting to anyway.
decompressed_lenDropped the 64 KiB stack array — io::copy into io::sink() already returns the byte count.
Shard routing, sampler threshold, format_sample_rate, billable_gb, OTel int→f64 points, load-test rate mathCorrect as written. Each carries an #[expect(..., reason = "...")] naming the invariant.

let _ = (46 sites) — became drop(..), behaviour-preserving. Two exceptions where the discarded value was worth having:

  • CloudflareConnectorResolver::record_success/record_failure returned a Resultno caller read, so a failed connector-health write vanished. Now infallible, logged at debug.
  • monitor_process in the load generator stops when its receiver is gone.

Other deliberate behaviour changes:

  • ReplayBlobStore gets a hand-written Debug. missing_debug_implementations wanted a derive, which would have put the S3 signing credentials one {:?} from a log line.
  • pace uses saturating_sub instead of unwrap() on checked_sub.
  • hex / uri_encode_path / hex_prefix build from a nibble table instead of a format! per byte.
  • build_logs_payload no longer returns a Result it never failed with.

Two needless_pass_by_value fixes needed an explicit drop.gzip and
rows_to_frames used to consume their argument and free it. Borrowing instead
left the uncompressed export batch (up to INGEST_BATCH_MAX_BYTES, default
4 MiB) alive across the export retry loop — which runs up to 20 attempts with
backoff, i.e. minutes during an upstream outage, once per in-flight lane — and
left the row buffers alive across the WAL append/fsync. Both are now dropped
where the old signatures dropped them. Caught by a reviewing agent, not by any
benchmark.

Structure — the 22 functions tripping too_many_lines/cognitive_complexity
keep their shape under #[expect(..., reason = "...")]. They are request
handlers, protocol encoders, retry loops and scenario tests where the length is
one linear pass; splitting them would thread a dozen locals through helpers.
That refactor should be made on purpose, not as a side effect of a lint sweep —
the #[expect]s make each one individually revisitable.

Interaction with the hotpath profiler

  • telemetry::HttpClient became a pub use ... as re-export instead of a
    pub type alias. unused_qualifications wanted
    hotpath::wrap::reqwest::Client shortened to Client, which would silently
    break the --features hotpath build where those are different types.
    (#[expect] misbehaves on a type alias here — it suppresses the lint and then
    reports itself unfulfilled — so the re-export is the honest fix.)
  • main's two #[allow(clippy::too_many_arguments)] became #[expect] with
    reasons, since allow_attributes is denied.

Scope of the zero-warning claim: default features, which is what CI builds
and what ships. cargo clippy --features hotpath still reports 32 — 24
large_futures from the profiler's own instrumentation of the request handlers
(newly visible because this PR enables pedantic, not newly introduced), and
8 #[expect]s that go unfulfilled because #[hotpath::measure] rewrites those
bodies so the complexity lints no longer fire. Silencing the first group would
mean Box::pin-ing request handlers, a real perf change that does not belong
in a lint PR.

Verification

  • cargo clippy --all-targets → 0 warnings; cargo test → 152 passed (identical to main)
  • CI Cargo test and load benchmark passes on the pinned Rust 1.94.1
  • Every hunk of the diff (389 total) was audited for unintended behaviour change.
    r2.rs's SigV4 helpers were checked by exhaustive differential test — all 256
    byte values through hex, every Unicode scalar through uri_encode_path
    byte-identical to baseline. encode_wal_frame's new guard was shown
    unreachable for any input that previously encoded.
  • Criterion ingest_accept benchmarks, 7 alternating rounds per tree:
    +0.6 % / +0.2 % median, against 4–9 % run-to-run variance of the same
    binary. decompressed_len measured 32 % faster on small replay chunks and a
    wash on large ones; r2::hex 30× faster.

Formatting

rustfmt ran only on the files this touched, and main's pre-existing drift was
restored afterwards, so the diff carries no unrelated reformatting —
metrics.rs still has exactly the 5 unformatted hunks it had before.

🤖 Generated with Claude Code

@JeremyFunk
JeremyFunkforce-pushed the chore/ingest-clippy-clean branch from db6aff4 to 63b2e58CompareAugust 19, 2026 09:14
@JeremyFunk
JeremyFunk changed the base branch from ai2/02-session-write to mainAugust 19, 2026 09:14
…int set
735 unique warnings on `cargo clippy --all-targets`, now none. `cargo clippy
--fix` covered ~570 of them (`str_to_string`, `map_unwrap_or`, `use_self`,
`uninlined_format_args`, `unreachable_pub`, `manual_let_else`, …); the rest
were reviewed one at a time.
The casts were the part worth reading. Most were already sound and now say so
with a bounded conversion instead of `as`:
- `encode_wal_frame` folds its three width checks into the `try_from`s that
needed them, and gains the row-count guard it was missing — the only real
truncation bug in the set.
- Duration/timestamp narrowing goes through `try_from(..).unwrap_or(MAX)`
behind a `duration_millis` helper; `current_time_millis` returns the `i64`
both its callers were casting to.
- `decompressed_len` hands its 64 KiB scratch buffer to `io::copy`.
- The casts that are correct as written — saturating float→int in the sampler,
counters widening to f64 for rate math — carry an `#[expect]` naming the
invariant.
Behaviour changes, all small and deliberate:
- `CloudflareConnectorResolver::record_success`/`record_failure` no longer
return a `Result` no caller read; a failed health write is logged at debug
instead of vanishing into `let _ =`.
- `ReplayBlobStore` gets a hand-written `Debug` so the signing credentials
cannot reach a log line.
- `monitor_process` in the load generator stops when its receiver is gone.
- `pace` uses `saturating_sub` rather than an `unwrap` on `checked_sub`.
Two of the by-value → by-reference parameter changes needed an explicit
`drop`. `gzip` and `rows_to_frames` used to consume their argument and free it;
borrowing instead left the uncompressed export batch (up to
INGEST_BATCH_MAX_BYTES) alive across the export retry loop, which runs for
minutes during an upstream outage, and left the row buffers alive across the
WAL append. Both are now dropped at the point the old signatures dropped them.
The 22 functions tripping `too_many_lines`/`cognitive_complexity` keep their
shape under `#[expect(..., reason = "...")]`: request handlers, protocol
encoders, retry loops and scenario tests, where the length is one linear pass
and splitting it would only thread locals through helpers. Breaking them up is
a refactor to make on purpose, not a side effect of a lint sweep.
`telemetry::HttpClient` becomes a `pub use ... as` re-export rather than a
`pub type` alias: `unused_qualifications` wanted the hotpath path shortened to
`Client`, which would silently break `--features hotpath` where those are
different types.
rustfmt was run only on the files this touched, and the crate's pre-existing
formatting drift was restored afterwards so the diff carries no unrelated
reformatting.
152 tests still pass, the same count as main.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@JeremyFunk
JeremyFunkforce-pushed the chore/ingest-clippy-clean branch from 63b2e58 to f904c18CompareAugust 19, 2026 09:31
@JeremyFunk
JeremyFunk merged commit 9a9989f into mainAug 19, 2026
23 of 25 checks passed
@JeremyFunk
JeremyFunk deleted the chore/ingest-clippy-clean branch August 19, 2026 09:51
@github-actions

Copy link
Copy Markdown

🍁 Maple PR preview

Note

Preview resources were removed when this pull request closed.

Final commit f904c18 · View workflow run

JeremyFunk added a commit that referenced this pull request Aug 19, 2026
The clippy cleanup (#523) followed the pedantic suggestion to construct
Durations with from_mins/from_hours - APIs that need rust >= 1.91. CI
tests build with the mise-pinned 1.94.1 and passed, but the Railway
Dockerfile and the deploy/probe workflows all build in rust:1.88 images,
so every ingest deploy since #523 fails with E0658 duration_constructors
(main.rs:1470, :1570, :2181).
- Bump rust:1.88 -> rust:1.94-bookworm in the Dockerfile (pinned to
bookworm explicitly so the build glibc can't drift past the
debian:bookworm-slim runner stage) and in deploy-prd, deploy-stg and
aws-probe.
- Pin msrv = "1.94" in clippy.toml so suggestion lints can't propose
APIs newer than the deploy toolchain again.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@JeremyFunk