Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,18 @@ jobs:
run: cargo fmt --all -- --check
- name: Clippy
run: cargo clippy --workspace --all-targets -- -D warnings
- name: Clippy (autumn-web, gated request-path features — panic gate)
# The default-feature Clippy above does not compile the feature-gated
# request-path modules (channels→ws, mail→mail, sync/*→offline-sync,
# session_redis→redis), so their `#![cfg_attr(not(test), deny(clippy::…))]`
# panic-gate lints never fire. Lint them with their features enabled so an
# unannotated `.unwrap()` in those production paths fails CI. (`--all-features`
# is unusable here: `managed-pg-bundled` pulls in `postgresql_embedded`, whose
# build script downloads Postgres binaries; this explicit list covers exactly
# the gated modules with no new services.)
run: cargo clippy -p autumn-web --features "ws,mail,offline-sync,redis" --all-targets -- -D warnings
- name: Panic-free request-path gate
run: ./scripts/check-panic-gate.sh

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Run clippy for feature-gated request modules

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 👍 / 👎.


test:
name: Test (${{ matrix.os }})
Expand Down
78 changes: 78 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,84 @@ The tests capture and print the full `cargo build` / `cargo check`
stdout+stderr on failure, so the breakage is diagnosable directly from the
CI summary.

## Request-path panic gate

Autumn enforces a [#1611][issue-1611] invariant: **request-path modules must not
panic on the production code path.** A request that reaches a runtime module
should never be able to bring the process down through an `unwrap`, `expect`,
`panic!`, out-of-bounds index, or an unfinished `todo!`/`unimplemented!`.

### What counts as "request path"

Per AC2, the gate covers modules that run **per request** or in
**framework-owned background loops** — extractors, form/body decoding, session
and idempotency stores, the scheduler and job queues, channels, and the
per-request middleware stack. These are the files listed in the
`REQUEST_PATH_MODULES` array in `scripts/check-panic-gate.sh`.

Explicitly **exempt** surfaces (a panic there cannot take down a live request):

- `#[cfg(test)]` code, benches, and examples;
- build scripts;
- the `autumn-cli` crate (a short-lived operator tool);
- application-author code (your route handlers are yours to write).

### What the gate checks

Each gated module carries a header that opts its **production** target into a
set of panic-class clippy denials:

```rust
// autumn-panic-gate: request-path module — production code path must be panic-free.
#![cfg_attr(not(test), deny(
clippy::unwrap_used,
clippy::expect_used,
clippy::panic,
clippy::unreachable,
clippy::todo,
clippy::unimplemented,
clippy::indexing_slicing,
))]
```

The `cfg_attr(not(test), …)` scope means the denials apply to the library build
but auto-exempt the module's own `#[cfg(test)] mod tests`. Enforcement happens
in the CI `lint` job (same workflow as `fmt`/`clippy`, no new services):

- `cargo clippy --workspace --all-targets -- -D warnings` fails on any un-justified
panic-class site in a gated module; and
- `scripts/check-panic-gate.sh` verifies every module in the canonical manifest
still exists and still carries the gate header, so the gate cannot be silently
removed.

Run the manifest check locally with `./scripts/check-panic-gate.sh`.

### Justifying an exception

When a site is provably infallible or a misconfiguration you want surfaced
eagerly, annotate it at the **narrowest** scope with a `reason`:

```rust
#[allow(clippy::expect_used, reason = "infallible: HMAC accepts any key length")]
```

For lock poisoning, do **not** annotate — **recover** instead, so one panicking
lock holder can't poison shared state for every later request:

```rust
let guard = self.state.lock().unwrap_or_else(PoisonError::into_inner);
```

This `unwrap_or_else(PoisonError::into_inner)` idiom is the established
lock-poisoning recovery contract (AC4); see `circuit_breaker.rs`.

### Deferred: arithmetic

Unchecked time/duration/integer arithmetic (`clippy::arithmetic_side_effects`)
is a deferred follow-up and is **not** in the automated lint set yet. It is
currently guarded by the saturating-arithmetic idiom (e.g. `saturating_deadline`
in `idempotency.rs`) plus review rather than by the gate.

## Fuzzing

Autumn coverage-guides a set of [cargo-fuzz][cargo-fuzz] (libFuzzer) harnesses
Expand Down
81 changes: 69 additions & 12 deletions autumn/src/channels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,22 @@
//! # // In async context: let msg = rx.recv().await.expect("should receive");
//! ```

// autumn-panic-gate: request-path module — production code path must be panic-free.
// See CONTRIBUTING.md "Request-path panic gate". Justify exceptions with
// #[allow(clippy::<lint>, reason = "…")] at the narrowest scope.
#![cfg_attr(
not(test),
deny(
clippy::unwrap_used,
clippy::expect_used,
clippy::panic,
clippy::unreachable,
clippy::todo,
clippy::unimplemented,
clippy::indexing_slicing,
)
)]

use std::collections::{HashMap, HashSet, VecDeque};
use std::future::Future;
use std::sync::atomic::{AtomicU64, Ordering};
Expand Down Expand Up @@ -253,26 +269,38 @@ struct ChannelMetricCounters {

impl ChannelMetrics {
fn ensure_topic(&self, topic: &str) {
let mut counters = self.counters.lock().expect("channel metrics lock poisoned");
let mut counters = self
.counters
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
counters.entry(topic.to_owned()).or_default();
}

fn record_publish(&self, topic: &str) {
let mut counters = self.counters.lock().expect("channel metrics lock poisoned");
let mut counters = self
.counters
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let stats = counters.entry(topic.to_owned()).or_default();
stats.publishes = stats.publishes.saturating_add(1);
drop(counters);
}

fn record_dropped(&self, topic: &str, count: u64) {
let mut counters = self.counters.lock().expect("channel metrics lock poisoned");
let mut counters = self
.counters
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let stats = counters.entry(topic.to_owned()).or_default();
stats.drops = stats.drops.saturating_add(count);
drop(counters);
}

fn record_lagged(&self, topic: &str, count: u64) {
let mut counters = self.counters.lock().expect("channel metrics lock poisoned");
let mut counters = self
.counters
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let stats = counters.entry(topic.to_owned()).or_default();
stats.lags = stats.lags.saturating_add(count);
drop(counters);
Expand All @@ -281,7 +309,7 @@ impl ChannelMetrics {
fn snapshot(&self) -> HashMap<String, ChannelMetricCounters> {
self.counters
.lock()
.expect("channel metrics lock poisoned")
.unwrap_or_else(std::sync::PoisonError::into_inner)
.clone()
}

Expand All @@ -290,7 +318,10 @@ impl ChannelMetrics {
return;
}

let mut counters = self.counters.lock().expect("channel metrics lock poisoned");
let mut counters = self
.counters
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
counters.retain(|topic, _| !topics.contains(topic));
drop(counters);
}
Expand Down Expand Up @@ -659,7 +690,11 @@ impl LocalChannelsBackend {
}

fn get_or_create_topic(&self, topic: &str) -> Arc<TopicState> {
let mut registry = self.inner.registry.lock().expect("channels lock poisoned");
let mut registry = self
.inner
.registry
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);

#[allow(clippy::option_if_let_else)]
if let Some(state) = registry.get(topic) {
Expand Down Expand Up @@ -699,7 +734,10 @@ impl LocalChannelsBackend {
// every message a resumed subscriber receives is published strictly
// after its snapshot, keeping the replay/live seam gapless. The id is
// assigned even when there are zero receivers so ids stay dense.
let mut replay = state.replay.lock().expect("channel replay lock poisoned");
let mut replay = state
.replay
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let id = replay.next_id;
replay.next_id = replay.next_id.saturating_add(1);
replay.buf.push_back((id, msg.clone()));
Expand Down Expand Up @@ -728,7 +766,10 @@ impl LocalChannelsBackend {
// `rx` can only ever observe messages published strictly after this
// point (seqs `start_seq + 1, start_seq + 2, ...`), none of which are in
// the snapshot below.
let replay_guard = state.replay.lock().expect("channel replay lock poisoned");
let replay_guard = state
.replay
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let rx = state.sender.subscribe();
let start_seq = replay_guard.next_id.saturating_sub(1);
let oldest = replay_guard.buf.front().map(|(seq, _)| *seq);
Expand Down Expand Up @@ -830,7 +871,11 @@ impl ChannelsBackend for LocalChannelsBackend {
}

fn channel_count(&self) -> usize {
let registry = self.inner.registry.lock().expect("channels lock poisoned");
let registry = self
.inner
.registry
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
registry.len()
}

Expand All @@ -839,7 +884,11 @@ impl ChannelsBackend for LocalChannelsBackend {
// topic kept alive only by transient SSE subscribers can lose its
// replay history during a disconnect window (resumable-SSE is in-process
// best-effort — see docs/guide/realtime.md, issue #1356).
let mut registry = self.inner.registry.lock().expect("channels lock poisoned");
let mut registry = self
.inner
.registry
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let mut removed_topics = HashSet::new();
registry.retain(|topic, state| {
// Keep topics with live receivers, or with outstanding keepalive
Expand All @@ -861,7 +910,11 @@ impl ChannelsBackend for LocalChannelsBackend {
// subscribe paths touch metrics before registry, so snapshot must never
// hold the registry mutex while reading metrics.
let subscriber_counts: HashMap<String, usize> = {
let registry = self.inner.registry.lock().expect("channels lock poisoned");
let registry = self
.inner
.registry
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
registry
.iter()
.map(|(topic, state)| (topic.clone(), state.sender.receiver_count()))
Expand Down Expand Up @@ -1188,6 +1241,10 @@ impl InterceptedChannelsBackend {
}

#[cfg(feature = "ws")]
#[allow(
clippy::indexing_slicing,
reason = "idx < interceptors.len() is checked immediately before the index"
)]
fn run_chain(
topic: &str,
msg: &ChannelMessage,
Expand Down
22 changes: 17 additions & 5 deletions autumn/src/extract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,22 @@
//! For the full set of Axum extractors, use
//! `autumn_web::reexports::axum::extract`.

// autumn-panic-gate: request-path module — production code path must be panic-free.
// See CONTRIBUTING.md "Request-path panic gate". Justify exceptions with
// #[allow(clippy::<lint>, reason = "…")] at the narrowest scope.
#![cfg_attr(
not(test),
deny(
clippy::unwrap_used,
clippy::expect_used,
clippy::panic,
clippy::unreachable,
clippy::todo,
clippy::unimplemented,
clippy::indexing_slicing,
)
)]

use axum::extract::{FromRequest, FromRequestParts};
use axum::response::{IntoResponse, Response};

Expand Down Expand Up @@ -284,11 +300,7 @@ fn content_type_essence(raw: &str) -> &str {
#[cfg(feature = "multipart")]
fn prefix_looks_like_markup(prefix: &[u8]) -> bool {
let bytes = prefix.strip_prefix(&[0xEF, 0xBB, 0xBF]).unwrap_or(prefix);
let trimmed = bytes
.iter()
.position(|byte| !byte.is_ascii_whitespace())
.map_or(&[][..], |idx| &bytes[idx..]);
trimmed.first() == Some(&b'<')
bytes.iter().find(|byte| !byte.is_ascii_whitespace()) == Some(&b'<')
}

/// Bound a content-type value before echoing it into an error message, so a
Expand Down
16 changes: 16 additions & 0 deletions autumn/src/form.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,22 @@
//! inline. For a redirect-after-post pattern, serialise `cs.errors()` into
//! the flash store and redirect; restore on the next GET.

// autumn-panic-gate: request-path module — production code path must be panic-free.
// See CONTRIBUTING.md "Request-path panic gate". Justify exceptions with
// #[allow(clippy::<lint>, reason = "…")] at the narrowest scope.
#![cfg_attr(
not(test),
deny(
clippy::unwrap_used,
clippy::expect_used,
clippy::panic,
clippy::unreachable,
clippy::todo,
clippy::unimplemented,
clippy::indexing_slicing,
)
)]

use std::collections::HashMap;

use axum::extract::{FromRequest, Request};
Expand Down
Loading
Loading