Part of #1611: request-path panic gate (CI robustness)#1848
Conversation
Introduce the #1611 request-path panic gate: a per-module header that opts each runtime module's production target into a set of panic-class clippy denials (unwrap_used, expect_used, panic, unreachable, todo, unimplemented, indexing_slicing) via cfg_attr(not(test), deny(...)), auto-exempting the module's own test submodule. - scripts/check-panic-gate.sh holds the canonical REQUEST_PATH_MODULES manifest and fails if any gated file is missing or has lost its gate header, so the gate cannot be silently dropped. Panic detection itself is done by clippy in the same lint job. - Wire the manifest check into the existing CI lint job, right after clippy (same workflow as fmt/clippy, no new services). - Add the gate header to the request-path modules that were already clean.
Gate the idempotency middleware and eliminate its production panic sites: - Recover from lock poisoning with unwrap_or_else(PoisonError::into_inner) on the in-memory store's entries/in_flight locks and the deferred-commit lock, mirroring the circuit_breaker idiom, so one panicking lock holder cannot poison shared state for every later request (AC4 contract). - Justify the infallible http::Response builder unwraps (responses built from static status/body) with narrowly scoped allow(clippy::unwrap_used). - Add poisoned_lock_does_not_break_subsequent_requests: poisons an internal lock via a panic in catch_unwind, then asserts a normal store round-trip still succeeds, proving into_inner recovery.
Gate the remaining request-path modules and resolve their panic-class sites:
- Convert lock-poisoning expects to unwrap_or_else(PoisonError::into_inner)
in job, job_tracking, channels, and mail (queue-slot, registry, buffer,
suppression, and metrics locks).
- Justify provably-infallible sites at the narrowest scope with
allow(..., reason = "..."): crypto/RNG and base64 in mail; the reqwest
client build in sync/engine; static 503 responses and future-poll-after-
Ready expects in maintenance/load_shed; bounds-checked indexing in
scheduler, metrics, channels, job's weighted round-robin, and mail's
token decode; the eager SessionLayer misconfiguration expect in session.
- error_page_filter: replace an unchecked find('[').unwrap() with a let-else
fallback and justify the bounds-checked path indexing.
- extract: rewrite prefix_looks_like_markup to find the first non-whitespace
byte without slicing, removing the panic surface entirely.
inbound_mail is intentionally deferred (not gated): its MIME parser has an
extensive untrusted-input indexing surface beyond mechanical justification;
gating it is tracked as follow-up so CI stays green over completeness.
Add a "Request-path panic gate" section describing the #1611 invariant: the definition of the request path and its exempt surfaces (tests, benches, examples, build scripts, autumn-cli, app-author code), the denied clippy lint set and how cfg_attr(not(test), ...) scopes it, enforcement via clippy plus scripts/check-panic-gate.sh in the CI lint job, how to justify an exception or recover poisoned locks, and a note that arithmetic_side_effects remains a deferred follow-up guarded by the saturating-arithmetic idiom and review.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22efa8a3b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && grep -q 'clippy::unwrap_used' "$module" \ | ||
| && grep -q 'clippy::indexing_slicing' "$module" \ |
There was a problem hiding this comment.
If a gated module later drops clippy::panic, clippy::expect_used, clippy::unreachable, clippy::todo, or clippy::unimplemented from its header but keeps unwrap_used and indexing_slicing, this script still exits successfully even though that panic class is no longer enforced by Clippy. That defeats the new “missing or incomplete gate” check for those cases, so please validate the full lint set or match a canonical header rather than only these two lints.
Useful? React with 👍 / 👎.
check-panic-gate.sh now requires every canonical clippy panic lint (unwrap_used, expect_used, panic, unreachable, todo, unimplemented, indexing_slicing) to be present in each gated module's deny header, rather than spot-checking a two-lint subset. A gated module that drops any single panic lint while keeping the others now fails the gate with a message naming the module and the specific missing lint, closing the silent-weakening gap flagged in PR review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7a7a4e09f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for lint in "${REQUIRED_PANIC_LINTS[@]}"; do | ||
| grep -qF "$lint" "$module" \ | ||
| || die "gate header in $module is missing required panic lint '$lint'" |
There was a problem hiding this comment.
In this revision the script checks every lint token, but grep -qF "$lint" "$module" scans the entire module rather than the #![cfg_attr(not(test), deny(...))] header. Modules in this commit now contain legitimate #[allow(clippy::expect_used, ...)] / #[allow(clippy::indexing_slicing, ...)] annotations, so removing one of those lints from the deny header would still satisfy this check while Clippy would stop enforcing that panic class for new production code. Please validate a canonical header block, or otherwise ensure each token is inside the deny list.
Useful? React with 👍 / 👎.
check-panic-gate.sh previously validated each required panic lint with a whole-file `grep -qF "clippy::<lint>"`. Now that gated modules carry legitimate per-site `#[allow(clippy::expect_used, reason = "…")]` / `#[allow(clippy::indexing_slicing, …)]` annotations, a whole-file scan could let such an `#[allow(...)]` spoof a lint that was actually dropped from the module's `#![cfg_attr(not(test), deny(...))]` header — a false pass. Extract the `#![cfg_attr(` … `)]` deny block per module (buffered so an unterminated header fails hard instead of running to EOF) and assert each `clippy::<lint>` from REQUIRED_PANIC_LINTS appears WITHIN that block only. Still require the `autumn-panic-gate:` marker and the deny() opener, and die naming the module plus the specific missing lint. Per PR review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3285bbc4c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[allow( | ||
| clippy::unwrap_used, | ||
| reason = "infallible: response built from stored record status/headers/body" | ||
| )] |
There was a problem hiding this comment.
This exception makes the panic gate accept response_from_record as infallible, but IdempotencyRecord exposes status: u16 and raw (String, Vec<u8>) headers, so a custom or corrupted idempotency store can return an invalid status or header bytes. In that replay path Response::builder() stores the error and the existing .body(...).unwrap() panics during request handling; please validate/skip invalid stored records or return a recovery response instead of exempting this unwrap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5837c55eec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Clippy | ||
| run: cargo clippy --workspace --all-targets -- -D warnings | ||
| - name: Panic-free request-path gate | ||
| run: ./scripts/check-panic-gate.sh |
There was a problem hiding this comment.
I checked the lint job here: this new gate only runs the manifest script after a default-feature cargo clippy, but several files in the manifest are not compiled by default (for example channels is behind ws, mail behind mail, and sync behind offline-sync in autumn/src/lib.rs/autumn/Cargo.toml). In those contexts, adding an unannotated unwrap() to one of those request-path modules still leaves the header intact so this step passes, and rustc builds in later all-features jobs do not enforce Clippy lints; please add a feature/all-features Clippy pass for the gated modules or split feature-specific lint runs.
Useful? React with 👍 / 👎.
2 participants
Before / After
Before: a
.unwrap()/.expect()/panic!added to a request-path module (middleware, extractors, stores, scheduler/jobs, channels) compiled and passed CI silently — nothing enforced the "panic-free request path" invariant that CONTRIBUTING.md already described. A poisoned lock in one request permanently broke framework-owned shared state for every later request.After: production code in 25 request-path modules is gated by clippy restriction lints in the existing CI
lintjob — an unannotated panic-capable call turns CI red. Poison-prone locks now recover instead of propagating (unwrap_or_else(PoisonError::into_inner)), and a regression test proves an induced panic no longer breaks the idempotency store for subsequent requests.How
#[cfg_attr(not(test), deny(clippy::unwrap_used, expect_used, panic, unreachable, todo, unimplemented, indexing_slicing))]on each gated module — scopes the deny to the production (lib) target, auto-exempting tests/benches.scripts/check-panic-gate.shenforces the canonical request-path module manifest (fails if a gated module loses its header); wired as a step in thelintjob alongside fmt/clippy — no new external services.PoisonError::into_inner(the establishedcircuit_breakeridiom); provably-infallible builder/crypto/bounds-checked sites → narrow#[allow(…, reason="…")]; two real fixes (error_page_filterfind('[')→ let-else fallback,extractmarkup sniff rewritten to avoid slicing).Acceptance criteria (#1611)
.unwrap()into a gated module and confirming clippy fails, then reverting.idempotency(the named module, incl. the near-Duration::MAXTTL path already fixed on trunk) + 23 other modules;inbound_maildeferred (below).idempotency::tests::poisoned_lock_does_not_break_subsequent_requests+into_innerconversions.lintjob).Deferred (why "Part of" not "Closes")
inbound_mail.rs— its MIME parser has ~57 index/slice sites over untrusted input; blanket-annotating them as safe would be unsound. Needs a dedicated audit. Header removed; excluded from the manifest.arithmetic_side_effects(unchecked time/duration/integer arithmetic named in AC1) — too noisy to enable now; currently guarded by the saturating-arithmetic idiom (seeidempotency::saturating_deadline) + review.mail/channels/offline-sync/ws) carry the gate header and were burned down + verified with their features enabled, but the default CIlintjob doesn't build those features, so clippy actively enforces them only when built with features. The manifest script still enforces header presence unconditionally.Test evidence
cargo fmt --all -- --checkcleancargo clippy --workspace --all-targets -- -D warningsgreen./scripts/check-panic-gate.sh→panic-gate: 25 request-path modules gatedcargo test -p autumn-web --lib idempotency→ 16 passed (incl. the new poison-recovery test)Generated by Claude Code