Production-readiness for open-source launch (GitHub + crates.io) - #1
Conversation
Hardens fleet-router across correctness, robustness, operability, packaging, CI/CD, security, and documentation for a public release. Reliability & correctness: - Atomic, underflow-safe connection counter; ConnectionGuard decrements synchronously on drop (no spawn, no runtime-handle dependency). - Saturating block-number arithmetic (no overflow/panic on upstream data). - Failover no longer silently drops client frames: buffers and replays them after the socket swap; forwards status/keep-alive frames during failover so clients never stall. - Upstream ABI handshake retries against another upstream and skips control frames instead of dropping the client. - Non-advancing upstreams are flagged stale and deprioritized for routing. Operability: - Structured logging via tracing (RUST_LOG; default info). - SIGTERM + SIGINT graceful shutdown with bounded drain and client close frames. - Config validation with actionable errors; `config init` no longer panics. - Connection cap, handshake/idle timeouts, configurable max message size. - Optional /health, /ready, /metrics (Prometheus) HTTP endpoint. Packaging & release: - crates.io metadata, MSRV 1.85, single-line description, lean `include` list, committed Cargo.lock, CLI version from CARGO_PKG_VERSION. - mock-ship marked publish=false. - CI installs clang/libclang, caches deps, runs clippy -D warnings, an MSRV check, cargo-deny, and a Docker build; least-privilege permissions. - Tag-driven release workflow (crates.io + Linux binary + GHCR image). - Hardened Dockerfile (non-root user, HEALTHCHECK, --locked, rust 1.85). Docs & community: - Rewritten README (prerequisites, architecture, full config reference, observability, deployment, security & limitations). - CONTRIBUTING, SECURITY, CODE_OF_CONDUCT, CHANGELOG, issue/PR templates. Tests: added operational tests (config validation + metrics endpoint); made the range-aware routing tests deterministic. Verified on Linux (Docker): cargo fmt --check, clippy --locked -D warnings, full test suite, release build, MSRV 1.85, cargo-deny, cargo publish --dry-run.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant updates to fleet-router, including configuration validation, structured logging via tracing, graceful shutdown, connection backpressure, and an optional HTTP health/metrics endpoint. It also refactors the load-balancing logic to support range-aware, least-connections routing with stale-upstream deprioritization, and enhances failover resilience by buffering client frames. The review feedback highlights a critical race condition in the failover buffering logic that could lead to lost or out-of-order messages, as well as two Slowloris Denial of Service (DoS) vulnerabilities in the handshake and metrics server loops due to resetting timeouts on individual read operations instead of wrapping the entire loops.
| drop(sw); | ||
| let mut p = pending.lock().await; | ||
| if p.len() < MAX_PENDING_FRAMES { | ||
| p.push_back(msg); | ||
| } else { | ||
| tracing::warn!("pending client-frame buffer full; dropping frame"); | ||
| } |
There was a problem hiding this comment.
There is a subtle race condition here. By dropping the sw lock before acquiring the pending lock, the main loop can acquire server_writer, swap the socket, and flush pending before this failed message is actually pushed to pending. This results in the failed message being pushed after the flush, leaving it stranded in pending and causing out-of-order or lost messages. Removing the explicit drop(sw); ensures that pending is locked and the message is buffered while sw is still held, preventing the main loop from swapping and flushing prematurely.
There was a problem hiding this comment.
Valid — fixed in 1ae6cdb. The drop(sw) created exactly that window: the failover loop could acquire server_writer, swap, and flush pending before this frame was enqueued, stranding it. Now server_writer is held across the pending push (lock order stays writer→pending, matching the flush path, so no deadlock).
| loop { | ||
| let next = if handshake_timeout_ms > 0 { | ||
| match timeout(Duration::from_millis(handshake_timeout_ms), ws.next()).await { | ||
| Ok(n) => n, | ||
| Err(_) => return None, // handshake timed out | ||
| } | ||
| } else { | ||
| ws.next().await | ||
| }; | ||
| match next { | ||
| Some(Ok(Message::Text(text))) => return Some(text.to_string()), | ||
| Some(Ok(Message::Ping(_))) | Some(Ok(Message::Pong(_))) => continue, | ||
| _ => return None, | ||
| } | ||
| } |
There was a problem hiding this comment.
Applying the timeout to each individual ws.next() call inside the loop allows a slow or malicious upstream to keep the handshake alive indefinitely by sending periodic Ping/Pong frames or slow bytes, bypassing the handshake timeout. Wrapping the entire handshake loop in a single global timeout ensures that the connection is closed if the handshake does not complete within the specified limit.
There was a problem hiding this comment.
Valid — fixed in 1ae6cdb. The whole ABI-read loop is now wrapped in a single timeout(handshake_timeout_ms, ...), so a peer dripping Ping/Pong frames can no longer keep the handshake alive past the deadline.
| let mut len = 0; | ||
| while len < buf.len() { | ||
| let n = match timeout(Duration::from_secs(5), stream.read(&mut buf[len..])).await { | ||
| Ok(Ok(0)) => break, // EOF | ||
| Ok(Ok(n)) => n, // read some bytes | ||
| Ok(Err(e)) => return Err(e), | ||
| Err(_) => break, // read timeout | ||
| }; | ||
| len += n; | ||
| if buf[..len].windows(4).any(|w| w == b"\r\n\r\n") || buf[..len].contains(&b'\n') { | ||
| break; // we have the request line (and possibly headers) | ||
| } | ||
| } |
There was a problem hiding this comment.
Applying the timeout to each individual stream.read call resets the timer on every read. A slow client can send 1 byte every 4 seconds and keep the connection open indefinitely, leading to Slowloris resource exhaustion on the metrics port. Wrapping the entire request reading loop in a single global timeout completely mitigates this vulnerability.
There was a problem hiding this comment.
Valid — fixed in 1ae6cdb. The request-head read is now wrapped in a single overall timeout(5s, ...) instead of a per-read timer, mitigating the Slowloris vector on the metrics port.
…r-buffer race Three issues raised by automated review (gemini-code-assist): - connection_handler: keep holding `server_writer` while buffering a failed client frame into `pending`, so the failover loop cannot acquire the writer, swap the socket, and flush `pending` before the frame is enqueued (which would strand/reorder it). - connection_handler: apply a single overall timeout to the upstream ABI handshake instead of per-frame, so a peer dripping Ping/Pong frames cannot keep the handshake alive past the deadline. - health: apply a single overall timeout to the metrics request-head read instead of resetting it per read (anti-Slowloris on the metrics port).
1 participant
Overview
Hardens
fleet-routerfor a public open-source launch (public GitHub repo + crates.io release). Driven by a multi-dimension production-readiness audit; this PR fixes all blocker/high findings and most medium ones, and adds the docs, CI/CD, and packaging needed to ship.All changes verified on Linux via Docker (the crate can't build on Windows —
rs_abieosis Linux-only):cargo fmt --check,clippy --locked -D warnings, full test suite (deterministic), release build, MSRV 1.85,cargo-deny, andcargo publish --dry-run.Reliability & correctness
ConnectionGuarddecrements synchronously on drop (no spawn / runtime-handle dependency).Operability
tracing(RUST_LOG, defaultinfo).SIGTERM+SIGINTgraceful shutdown with a bounded drain and client close frames.config initno longer panics./health,/ready,/metrics(Prometheus) HTTP endpoint (src/health.rs).Packaging & release
includelist, committedCargo.lock, CLI version fromCARGO_PKG_VERSION.mock-shipmarkedpublish = false.clang/libclang, caches deps, runsclippy -D warnings, an MSRV check,cargo-deny, and a Docker build; least-privilegepermissions. Redundantcargo-build.ymlremoved.release.yml(crates.io publish + Linux binary + GHCR image).Dockerfile(non-root user,HEALTHCHECK,--locked, Rust 1.85), port reconciled to 17000.Docs & community
README.md(prerequisites, architecture, full config reference, observability, deployment, security & limitations).CONTRIBUTING.md,SECURITY.md,CODE_OF_CONDUCT.md,CHANGELOG.md, issue templates, and PR template.Tests
tests/operational.rs(config validation + metrics endpoint).Review guide
src/connection_handler.rs,src/main.rs,src/models.rs,src/tasks.rs,src/functions.rs.src/health.rs,deny.toml,.github/workflows/release.yml.tests/*andmock-ship/*diffs arecargo fmtnormalization (whitespace only).Before merging / launching (maintainer actions)
CARGO_REGISTRY_TOKENrepo secret (crates.io API token) forrelease.yml.v0.2.0to trigger the release workflow.Deferred (low-severity, documented follow-ups)
zcdfallible conversions (audit verified these panics aren't reachable from untrusted network input today).wss://-to-upstream TLS.