From a28ce888ca45b49b747b2836770d680d01084899 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:14:08 -0700 Subject: [PATCH 01/15] refactor(iggy_client): extract resilience executor + test matrix Extract the timeout + circuit-breaker + reconnect-and-retry composition from IggyClientWrapper::with_reconnect into resilience::run_resilient, generic over the operation, reconnect step, and connection-state check. Behavior-preserving: the wrapper method now binds the executor to its breaker, configured timeout, tracked state, and bounded reconnect. Add the paused-clock test matrix (one test per executor branch) that TD-2026-07-01 required before any further behavioral change to these paths, plus tokio test-util as a dev-only feature to drive it. Resolves TD-2026-07-01. --- Cargo.toml | 4 + docs/tech-debt/TD-2026-07-01.md | 16 +- src/iggy_client/mod.rs | 196 +----------- src/iggy_client/resilience.rs | 532 ++++++++++++++++++++++++++++++++ 4 files changed, 566 insertions(+), 182 deletions(-) create mode 100644 src/iggy_client/resilience.rs diff --git a/Cargo.toml b/Cargo.toml index f04911e..5750653 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,6 +70,10 @@ metrics-exporter-prometheus = { version = "0.18", default-features = false, feat [dev-dependencies] reqwest = { version = "0.13", features = ["json"] } testcontainers = "0.27" +# test-util unlocks tokio::time::pause()/advance() for the paused-clock +# resilience matrix (TD-2026-07-01); dev-only so production builds are +# unaffected. +tokio = { version = "1.52", features = ["full", "test-util"] } # ============================================================================= # Lints Configuration diff --git a/docs/tech-debt/TD-2026-07-01.md b/docs/tech-debt/TD-2026-07-01.md index c91628b..d274b41 100644 --- a/docs/tech-debt/TD-2026-07-01.md +++ b/docs/tech-debt/TD-2026-07-01.md @@ -1,7 +1,7 @@ # TD-2026-07-01: `with_reconnect` composition test matrix **Source:** Review session 01, Round 1 — pr-test-analyzer finding #2 (HIGH). -**Status:** open +**Status:** resolved (session 02) ## Problem @@ -43,3 +43,17 @@ behavioral change (reconnect sessions now run in a spawned task for cancellation safety; `wait_for_reconnection` gained `enable()` registration). Same disposition, same re-armed trigger - the spawned-task shape also makes the future matrix EASIER (sessions are now addressable via JoinHandle). + +## Resolution (session 02) + +The retry/breaker/timeout composition was extracted into +`src/iggy_client/resilience.rs` as `run_resilient`, generic over the +operation, the reconnect step, and the connection-state check — no +test-only constructor, no live server needed. `with_reconnect` now +delegates to it. The paused-clock matrix (`tokio::test(start_paused = +true)`, one test per branch) covers: breaker-open fast-fail, +timeout-while-connected (no reconnect), timeout-while-disconnected → +reconnect + retry, connection-error → reconnect + single retry (success +and failure, proving no retry loop), non-connection error → breaker +untouched, reconnect failure propagation, and retry-path breaker +bookkeeping (timeout records failure, non-connection error does not). diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index d68936f..2832148 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -58,6 +58,7 @@ mod circuit_breaker; mod connection; mod helpers; mod params; +mod resilience; mod scopeguard; use std::str::FromStr; @@ -453,151 +454,26 @@ impl IggyClientWrapper { /// Execute an operation with automatic reconnection on connection failure. /// - /// This is the core resilience mechanism. Features: - /// - **Circuit Breaker**: Fail fast when service is known to be unavailable - /// - **Timeout**: All operations are bounded by `config.operation_timeout` - /// - **Retry**: On connection failure, attempts reconnect and retries once - /// - /// # Worst-case latency - /// - /// A request on the reconnect path can take up to 3x the operation - /// timeout (first attempt + bounded reconnect + retry) - bounded, but - /// well above the typical single-operation expectation. - /// - /// # Breaker false positives - /// - /// Timeouts count as breaker failures because the SDK's internal - /// reconnection swallows outages into blocking retries (a timeout is - /// often the only signal). The cost: N consecutive merely-SLOW - /// operations (including the background stats refresher's) can open the - /// circuit without a real outage. Failures must be consecutive - any - /// success resets the count - which bounds the risk. - /// - /// # Circuit Breaker Integration - /// - /// Before attempting the operation, the circuit breaker is checked: - /// - If **Open**: Returns `CircuitOpen` error immediately (fail fast) - /// - If **Closed** or **HalfOpen**: Proceeds with the operation - /// - /// After the operation, the circuit breaker is updated: - /// - Success: Records success (may close circuit if in HalfOpen) - /// - Failure: Records failure (may open circuit if threshold exceeded) - /// - /// # Timeout vs Disconnection - /// - /// The function differentiates between: - /// - **Connection errors**: Trigger reconnection and retry - /// - **Timeouts**: Only trigger reconnection if connection state shows disconnected; - /// otherwise return timeout error (slow operation != broken connection) - /// - /// This prevents unnecessary reconnection attempts under high latency conditions. + /// This is the core resilience mechanism: circuit-breaker gate, timeout + /// bound, and reconnect-plus-single-retry on classified connection + /// errors. The composition itself lives in [`resilience::run_resilient`] + /// (see its module docs for the full semantics, worst-case latency, and + /// breaker false-positive analysis); this method binds it to the + /// wrapper's breaker, configured operation timeout, tracked connection + /// state, and bounded reconnect session. async fn with_reconnect(&self, operation: F) -> AppResult where F: Fn() -> Fut, Fut: std::future::Future>, { - // Check circuit breaker before attempting operation - if !self.circuit_breaker.allow_request().await { - let state = self.circuit_breaker.state().await; - return Err(AppError::CircuitOpen(format!( - "Circuit breaker is {} - service temporarily unavailable", - state - ))); - } - - let timeout_duration = self.config.operation_timeout; - - // First attempt with timeout - let result = tokio::time::timeout(timeout_duration, operation()).await; - - match result { - Ok(Ok(value)) => { - self.circuit_breaker.record_success().await; - Ok(value) - } - Ok(Err(e)) if Self::is_connection_error(&e) => { - self.circuit_breaker.record_failure().await; - warn!(error = %e, "Operation failed due to connection error, attempting reconnect"); - self.reconnect_bounded().await?; - self.retry_once(&operation).await - } - Ok(Err(e)) => { - // Non-connection error - don't record as circuit breaker failure - Err(e) - } - Err(_) => { - // Timeout on first attempt. The SDK's internal transport - // reconnection swallows most mid-operation connection failures - // into blocking retries, so a timeout is often the only outage - // signal we get - record it as a circuit-breaker failure. - self.circuit_breaker.record_failure().await; - - // Only reconnect if we have evidence the connection is - // actually lost (the background health check drives this - // flag via live pings). A timeout alone could just be a slow - // operation. - if !self.state.is_connected() { - warn!( - timeout = ?timeout_duration, - "Operation timed out and connection state is disconnected, attempting reconnect" - ); - self.reconnect_bounded().await?; - self.retry_once(&operation).await - } else { - debug!( - timeout = ?timeout_duration, - "Operation timed out but connection state is healthy, not reconnecting" - ); - Err(AppError::OperationTimeout(format!( - "Operation timed out after {:?}", - timeout_duration - ))) - } - } - } - } - - /// Single post-reconnect retry with timeout and circuit-breaker - /// bookkeeping. Shared by both reconnect paths of [`Self::with_reconnect`]. - async fn retry_once(&self, operation: &F) -> AppResult - where - F: Fn() -> Fut, - Fut: std::future::Future>, - { - let timeout_duration = self.config.operation_timeout; - match tokio::time::timeout(timeout_duration, operation()).await { - Ok(Ok(value)) => { - self.circuit_breaker.record_success().await; - Ok(value) - } - Ok(Err(e)) => { - if Self::is_connection_error(&e) { - self.circuit_breaker.record_failure().await; - } - Err(e) - } - Err(_) => { - self.circuit_breaker.record_failure().await; - Err(AppError::OperationTimeout(format!( - "Operation timed out after {:?} on retry", - timeout_duration - ))) - } - } - } - - /// Check if an error is a connection-related error that warrants reconnection. - /// - /// Uses explicit pattern matching on error variants rather than string matching, - /// which is more reliable and maintainable. Any error indicating connection - /// issues should trigger a reconnection attempt. - fn is_connection_error(error: &AppError) -> bool { - matches!( - error, - AppError::ConnectionFailed(_) - | AppError::Disconnected(_) - | AppError::ConnectionReset(_) + resilience::run_resilient( + &self.circuit_breaker, + self.config.operation_timeout, + || self.state.is_connected(), + || self.reconnect_bounded(), + operation, ) + .await } // ========================================================================= @@ -1196,46 +1072,4 @@ mod tests { } } - #[test] - fn test_is_connection_error_connection_failed() { - let error = AppError::ConnectionFailed("test".to_string()); - assert!(IggyClientWrapper::is_connection_error(&error)); - } - - #[test] - fn test_is_connection_error_disconnected() { - let error = AppError::Disconnected("connection lost".to_string()); - assert!(IggyClientWrapper::is_connection_error(&error)); - } - - #[test] - fn test_is_connection_error_connection_reset() { - let error = AppError::ConnectionReset("reset by peer".to_string()); - assert!(IggyClientWrapper::is_connection_error(&error)); - } - - #[test] - fn test_is_connection_error_unrelated_errors() { - // These errors should NOT trigger reconnection - let test_cases = vec![ - AppError::BadRequest("invalid input".to_string()), - AppError::NotFound("resource missing".to_string()), - AppError::Internal("internal error".to_string()), - AppError::StreamError("stream issue".to_string()), - AppError::TopicError("topic issue".to_string()), - AppError::SendError("send failed".to_string()), - AppError::PollError("poll failed".to_string()), - AppError::ConfigError("config issue".to_string()), - AppError::OperationTimeout("timed out".to_string()), - AppError::CircuitOpen("circuit open".to_string()), - ]; - - for error in test_cases { - assert!( - !IggyClientWrapper::is_connection_error(&error), - "Error {:?} should not be treated as connection error", - error - ); - } - } } diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs new file mode 100644 index 0000000..43f5b0e --- /dev/null +++ b/src/iggy_client/resilience.rs @@ -0,0 +1,532 @@ +//! Resilience executor: the timeout + circuit-breaker + reconnect-and-retry +//! composition applied to every Iggy operation. +//! +//! Extracted from `IggyClientWrapper::with_reconnect` so the composition can +//! be exercised against fake operations and a fake reconnect step (no live +//! server), with `tokio::time::pause()` driving the timeout branches — see +//! the test matrix at the bottom of this file (TD-2026-07-01). +//! +//! # Composition semantics +//! +//! 1. **Circuit breaker gate**: if the breaker rejects the request, fail +//! fast with `CircuitOpen` without running the operation. +//! 2. **First attempt**, bounded by `timeout`. +//! 3. **Classified connection error** → record breaker failure, run the +//! `reconnect` step, then retry the operation exactly once. +//! 4. **Non-connection error** → returned as-is; the breaker is NOT touched +//! (bad requests must not open the circuit). +//! 5. **Timeout** → recorded as a breaker failure (the SDK's internal +//! transport reconnection swallows most outages into blocking retries, so +//! a timeout is often the only outage signal). Reconnect + single retry +//! happen only if `is_connected` reports the connection as lost — a +//! timeout alone could just be a slow operation. +//! +//! # Worst-case latency +//! +//! A request on the reconnect path can take up to 3x `timeout` (first +//! attempt + bounded reconnect + retry) — bounded, but well above the +//! typical single-operation expectation. +//! +//! # Breaker false positives +//! +//! Timeouts count as breaker failures, so N consecutive merely-SLOW +//! operations (including the background stats refresher's) can open the +//! circuit without a real outage. Failures must be consecutive — any +//! success resets the count — which bounds the risk. + +use std::time::Duration; + +use tracing::{debug, warn}; + +use super::circuit_breaker::CircuitBreaker; +use crate::error::{AppError, AppResult}; + +/// Check if an error is a connection-related error that warrants reconnection. +/// +/// Uses explicit pattern matching on error variants rather than string +/// matching, which is more reliable and maintainable. +pub(super) fn is_connection_error(error: &AppError) -> bool { + matches!( + error, + AppError::ConnectionFailed(_) + | AppError::Disconnected(_) + | AppError::ConnectionReset(_) + ) +} + +/// Execute `operation` under the full resilience composition. +/// +/// Generic over its collaborators so the composition is testable without a +/// live server: +/// +/// - `breaker` — gates the request and records the outcome +/// - `timeout` — per-attempt bound (both the first attempt and the retry) +/// - `is_connected` — consulted only on the timeout branch, to distinguish +/// "slow operation" from "lost connection" +/// - `reconnect` — the bounded reconnect step; invoked at most once +/// - `operation` — the Iggy call; invoked once, plus at most one retry +pub(super) async fn run_resilient( + breaker: &CircuitBreaker, + timeout: Duration, + is_connected: C, + reconnect: R, + operation: F, +) -> AppResult +where + F: Fn() -> Fut, + Fut: Future>, + C: FnOnce() -> bool, + R: FnOnce() -> RFut, + RFut: Future>, +{ + // Check circuit breaker before attempting operation + if !breaker.allow_request().await { + let state = breaker.state().await; + return Err(AppError::CircuitOpen(format!( + "Circuit breaker is {} - service temporarily unavailable", + state + ))); + } + + // First attempt with timeout + match tokio::time::timeout(timeout, operation()).await { + Ok(Ok(value)) => { + breaker.record_success().await; + Ok(value) + } + Ok(Err(e)) if is_connection_error(&e) => { + breaker.record_failure().await; + warn!(error = %e, "Operation failed due to connection error, attempting reconnect"); + reconnect().await?; + retry_once(breaker, timeout, &operation).await + } + Ok(Err(e)) => { + // Non-connection error - don't record as circuit breaker failure + Err(e) + } + Err(_) => { + // Timeout on first attempt. The SDK's internal transport + // reconnection swallows most mid-operation connection failures + // into blocking retries, so a timeout is often the only outage + // signal we get - record it as a circuit-breaker failure. + breaker.record_failure().await; + + // Only reconnect if we have evidence the connection is actually + // lost (the background health check drives this flag via live + // pings). A timeout alone could just be a slow operation. + if !is_connected() { + warn!( + timeout = ?timeout, + "Operation timed out and connection state is disconnected, attempting reconnect" + ); + reconnect().await?; + retry_once(breaker, timeout, &operation).await + } else { + debug!( + timeout = ?timeout, + "Operation timed out but connection state is healthy, not reconnecting" + ); + Err(AppError::OperationTimeout(format!( + "Operation timed out after {:?}", + timeout + ))) + } + } + } +} + +/// Single post-reconnect retry with timeout and circuit-breaker bookkeeping. +/// Shared by both reconnect paths of [`run_resilient`]. +async fn retry_once( + breaker: &CircuitBreaker, + timeout: Duration, + operation: &F, +) -> AppResult +where + F: Fn() -> Fut, + Fut: Future>, +{ + match tokio::time::timeout(timeout, operation()).await { + Ok(Ok(value)) => { + breaker.record_success().await; + Ok(value) + } + Ok(Err(e)) => { + if is_connection_error(&e) { + breaker.record_failure().await; + } + Err(e) + } + Err(_) => { + breaker.record_failure().await; + Err(AppError::OperationTimeout(format!( + "Operation timed out after {:?} on retry", + timeout + ))) + } + } +} + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] +mod tests { + use std::sync::Arc; + use std::sync::atomic::{AtomicU32, Ordering}; + + use super::super::circuit_breaker::{CircuitBreakerConfig, CircuitState}; + use super::*; + + const TIMEOUT: Duration = Duration::from_secs(5); + + fn breaker_with(failure_threshold: u32) -> CircuitBreaker { + CircuitBreaker::new(CircuitBreakerConfig::new( + failure_threshold, + 1, + Duration::from_secs(30), + )) + } + + /// Fake reconnect step that counts invocations and returns `result`. + fn fake_reconnect( + counter: &Arc, + result: AppResult<()>, + ) -> impl FnOnce() -> std::future::Ready> { + let counter = Arc::clone(counter); + move || { + counter.fetch_add(1, Ordering::SeqCst); + std::future::ready(result) + } + } + + // ========================================================================= + // TD-2026-07-01 composition matrix - one test per branch of the executor + // ========================================================================= + + #[tokio::test(start_paused = true)] + async fn breaker_open_fails_fast_without_running_operation() { + let breaker = breaker_with(5); + breaker.force_open().await; + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Ok(42) + } + }, + ) + .await; + + assert!(matches!(result, Err(AppError::CircuitOpen(_)))); + assert_eq!(calls.load(Ordering::SeqCst), 0, "operation must not run"); + assert_eq!(reconnects.load(Ordering::SeqCst), 0); + assert_eq!(breaker.requests_rejected(), 1); + } + + #[tokio::test(start_paused = true)] + async fn success_passes_through_and_records_breaker_success() { + // Enter HalfOpen (success_threshold = 1) so record_success is + // observable: the single success must close the circuit. The breaker + // stamps opened_at with std Instant (not tokio's pausable clock), so + // a zero open_duration is what makes the Open->HalfOpen transition + // immediate and deterministic here. + let breaker = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::ZERO)); + breaker.force_open().await; + let reconnects = Arc::new(AtomicU32::new(0)); + + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, + fake_reconnect(&reconnects, Ok(())), + || async { Ok(42) }, + ) + .await; + + assert_eq!(result.unwrap(), 42); + assert_eq!(breaker.state().await, CircuitState::Closed); + assert_eq!(reconnects.load(Ordering::SeqCst), 0); + } + + #[tokio::test(start_paused = true)] + async fn timeout_while_connected_returns_timeout_without_reconnecting() { + let breaker = breaker_with(1); + let reconnects = Arc::new(AtomicU32::new(0)); + + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, // connected: slow operation, not an outage + fake_reconnect(&reconnects, Ok(())), + || async { std::future::pending().await }, + ) + .await; + + assert!(matches!(result, Err(AppError::OperationTimeout(_)))); + assert_eq!(reconnects.load(Ordering::SeqCst), 0, "must not reconnect"); + // The timeout still counts as a breaker failure (threshold 1 -> Open). + assert_eq!(breaker.state().await, CircuitState::Open); + } + + #[tokio::test(start_paused = true)] + async fn timeout_while_disconnected_reconnects_and_retry_succeeds() { + let breaker = breaker_with(5); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || false, // health checks say the connection is lost + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + if calls.fetch_add(1, Ordering::SeqCst) == 0 { + std::future::pending().await + } else { + Ok(7) + } + } + }, + ) + .await; + + assert_eq!(result.unwrap(), 7); + assert_eq!(reconnects.load(Ordering::SeqCst), 1); + assert_eq!(calls.load(Ordering::SeqCst), 2, "exactly one retry"); + } + + #[tokio::test(start_paused = true)] + async fn connection_error_reconnects_and_retry_succeeds() { + let breaker = breaker_with(5); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + if calls.fetch_add(1, Ordering::SeqCst) == 0 { + Err(AppError::ConnectionFailed("first attempt".into())) + } else { + Ok(7) + } + } + }, + ) + .await; + + assert_eq!(result.unwrap(), 7); + assert_eq!(reconnects.load(Ordering::SeqCst), 1); + assert_eq!(calls.load(Ordering::SeqCst), 2); + // Failure then success: the success resets the consecutive count. + assert_eq!(breaker.state().await, CircuitState::Closed); + } + + #[tokio::test(start_paused = true)] + async fn connection_error_on_retry_does_not_loop() { + let breaker = breaker_with(5); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Err(AppError::ConnectionFailed("still down".into())) + } + }, + ) + .await; + + assert!(matches!(result, Err(AppError::ConnectionFailed(_)))); + assert_eq!(calls.load(Ordering::SeqCst), 2, "single retry, no loop"); + assert_eq!(reconnects.load(Ordering::SeqCst), 1, "single reconnect"); + } + + #[tokio::test(start_paused = true)] + async fn non_connection_error_leaves_breaker_untouched() { + // failure_threshold = 1: a single recorded failure would open the + // circuit, so Closed-after proves record_failure was never called. + let breaker = breaker_with(1); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Err(AppError::BadRequest("invalid input".into())) + } + }, + ) + .await; + + assert!(matches!(result, Err(AppError::BadRequest(_)))); + assert_eq!(breaker.state().await, CircuitState::Closed); + assert_eq!(calls.load(Ordering::SeqCst), 1, "no retry"); + assert_eq!(reconnects.load(Ordering::SeqCst), 0, "no reconnect"); + } + + #[tokio::test(start_paused = true)] + async fn reconnect_failure_propagates_without_retry() { + let breaker = breaker_with(5); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, + fake_reconnect( + &reconnects, + Err(AppError::ConnectionFailed("reconnect exhausted".into())), + ), + move || { + let calls = Arc::clone(&op_calls); + async move { + calls.fetch_add(1, Ordering::SeqCst); + Err(AppError::ConnectionFailed("first attempt".into())) + } + }, + ) + .await; + + assert!( + matches!(&result, Err(AppError::ConnectionFailed(msg)) if msg.contains("reconnect exhausted")) + ); + assert_eq!(calls.load(Ordering::SeqCst), 1, "no retry after failed reconnect"); + assert_eq!(reconnects.load(Ordering::SeqCst), 1); + } + + #[tokio::test(start_paused = true)] + async fn timeout_on_retry_records_breaker_failure() { + // Threshold 2: first-attempt connection error + retry timeout are the + // two consecutive failures that open the circuit. + let breaker = breaker_with(2); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + if calls.fetch_add(1, Ordering::SeqCst) == 0 { + Err(AppError::ConnectionFailed("first attempt".into())) + } else { + std::future::pending().await + } + } + }, + ) + .await; + + assert!( + matches!(&result, Err(AppError::OperationTimeout(msg)) if msg.contains("on retry")) + ); + assert_eq!(breaker.state().await, CircuitState::Open); + } + + #[tokio::test(start_paused = true)] + async fn non_connection_error_on_retry_skips_breaker_failure() { + // Threshold 2: only the first-attempt connection error is recorded; + // the retry's BadRequest must not be, so the circuit stays Closed. + let breaker = breaker_with(2); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + || true, + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + if calls.fetch_add(1, Ordering::SeqCst) == 0 { + Err(AppError::ConnectionFailed("first attempt".into())) + } else { + Err(AppError::BadRequest("rejected by server".into())) + } + } + }, + ) + .await; + + assert!(matches!(result, Err(AppError::BadRequest(_)))); + assert_eq!(breaker.state().await, CircuitState::Closed); + } + + // ========================================================================= + // Error classifier + // ========================================================================= + + #[test] + fn connection_variants_are_connection_errors() { + for error in [ + AppError::ConnectionFailed("test".to_string()), + AppError::Disconnected("connection lost".to_string()), + AppError::ConnectionReset("reset by peer".to_string()), + ] { + assert!(is_connection_error(&error), "{error:?}"); + } + } + + #[test] + fn non_connection_variants_are_not_connection_errors() { + let test_cases = vec![ + AppError::BadRequest("invalid input".to_string()), + AppError::NotFound("resource missing".to_string()), + AppError::Internal("internal error".to_string()), + AppError::StreamError("stream issue".to_string()), + AppError::TopicError("topic issue".to_string()), + AppError::SendError("send failed".to_string()), + AppError::PollError("poll failed".to_string()), + AppError::ConfigError("config issue".to_string()), + AppError::OperationTimeout("timed out".to_string()), + AppError::CircuitOpen("circuit open".to_string()), + ]; + + for error in test_cases { + assert!( + !is_connection_error(&error), + "Error {:?} should not be treated as connection error", + error + ); + } + } +} From 1e371564705ad220211ebda41c9920d6758991f6 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:17:19 -0700 Subject: [PATCH 02/15] feat(iggy_client): token-limit half-open circuit-breaker probes Entering HalfOpen now grants success_threshold probe tokens (minimum one); each allowed request consumes one and exhausted budgets fail fast, so a recovering server no longer receives an unbounded thundering herd of concurrent probes. Tokens re-grant after open_duration elapses in HalfOpen, guaranteeing a probe whose outcome is never recorded (the non-connection-error path) cannot wedge the breaker. Breaker timing switches from std to tokio Instant (identical in production) so the new behavior is covered by paused-clock tests. Both doc sites that previously claimed limited probes - the circuit_breaker module doc and the IggyClientWrapper struct doc - now describe the implemented token semantics. Resolves TD-2026-07-03. --- docs/tech-debt/TD-2026-07-03.md | 14 +- src/iggy_client/circuit_breaker.rs | 206 ++++++++++++++++++++++++++--- src/iggy_client/mod.rs | 4 +- src/iggy_client/resilience.rs | 6 +- 4 files changed, 203 insertions(+), 27 deletions(-) diff --git a/docs/tech-debt/TD-2026-07-03.md b/docs/tech-debt/TD-2026-07-03.md index eee33c2..925b89c 100644 --- a/docs/tech-debt/TD-2026-07-03.md +++ b/docs/tech-debt/TD-2026-07-03.md @@ -1,7 +1,7 @@ # TD-2026-07-03: Circuit-breaker half-open probe limiting **Source:** Review session 01, Round 1 — code-architect #9, type-design F5 (LOW). -**Status:** open +**Status:** resolved (session 02) ## Problem @@ -17,3 +17,15 @@ multi-probe behavior — both `circuit_breaker.rs` module docs AND the `IggyClientWrapper` struct doc in `iggy_client/mod.rs` claim limited probes) at the first production incident involving breaker recovery, or whenever breaker configuration is next extended. + +## Resolution (session 02) + +Implemented token-limited probing: entering HalfOpen grants +`success_threshold` probe tokens (minimum one), each `allow_request()` +consumes one, and exhausted budgets reject with a breaker rejection. +Tokens re-grant after `open_duration` elapses in HalfOpen so a probe whose +outcome is never recorded (non-connection errors touch neither counter) +cannot wedge the breaker. Both doc sites (circuit_breaker.rs module doc +diagram/section AND the `IggyClientWrapper` struct doc) now describe the +implemented token-limited behavior. Breaker timing moved from std to +tokio `Instant` so the limiting is covered by paused-clock tests. diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 959be02..94a1f91 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -20,8 +20,8 @@ //! │ │ │ ▼ │ //! │ │ │ ┌───────────────┐ │ //! │ │ └─────────────────────────── │ HalfOpen │ │ -//! │ │ success │ (Probe with │ │ -//! │ │ │ one request) │ │ +//! │ │ success │ (token-limited│ │ +//! │ │ │ probes) │ │ //! │ │ └───────┬───────┘ │ //! │ │ │ │ //! │ │ │ failure │ @@ -38,6 +38,15 @@ //! - `success_threshold`: Number of consecutive successes in half-open to close //! - `open_duration`: How long to stay open before trying half-open //! +//! # Half-Open Probe Limiting +//! +//! Entering half-open grants `success_threshold` probe tokens (minimum one); +//! each allowed request consumes one, and requests beyond the budget are +//! rejected so a recovering server never receives a thundering herd of +//! probes. Tokens re-grant after `open_duration` elapses in half-open, +//! guaranteeing the breaker cannot wedge if a probe's outcome is never +//! recorded. See [`CircuitBreaker::allow_request`]. +//! //! # Usage //! //! ```rust,ignore @@ -62,9 +71,12 @@ //! ``` use std::sync::atomic::{AtomicU32, AtomicU64, Ordering}; -use std::time::{Duration, Instant}; +use std::time::Duration; use tokio::sync::RwLock; +// tokio's Instant (a thin wrapper over std's) so breaker timing follows the +// pausable test clock; identical behavior in production. +use tokio::time::Instant; use tracing::{debug, info, warn}; /// Circuit breaker state. @@ -130,6 +142,10 @@ struct CircuitBreakerState { consecutive_failures: u32, /// Number of consecutive successes (in half-open state). consecutive_successes: u32, + /// Probe tokens remaining in the current half-open window. + half_open_probes_remaining: u32, + /// When the current half-open probe window was granted (for re-grant). + half_open_granted_at: Option, } impl CircuitBreakerState { @@ -139,6 +155,8 @@ impl CircuitBreakerState { opened_at: None, consecutive_failures: 0, consecutive_successes: 0, + half_open_probes_remaining: 0, + half_open_granted_at: None, } } } @@ -177,14 +195,30 @@ impl CircuitBreaker { /// /// - **Closed**: Always allows requests /// - **Open**: Rejects requests; transitions to HalfOpen after timeout - /// - **HalfOpen**: Allows requests (for probing) + /// - **HalfOpen**: Allows a token-limited number of probes (see below) + /// + /// # Half-open probe limiting + /// + /// Entering HalfOpen grants `success_threshold` probe tokens (at least + /// one) — exactly the number of successes needed to close the circuit. + /// Each allowed request consumes a token; with no tokens left, requests + /// are rejected, which caps the probe load on a recovering server + /// instead of letting every concurrent caller through at once. + /// + /// Tokens re-grant after `open_duration` elapses in HalfOpen. This is + /// the anti-wedge guarantee: a probe whose outcome is never recorded + /// (e.g. the operation failed with a non-connection error, which by + /// design touches neither breaker counter) would otherwise leave the + /// breaker half-open with zero tokens forever. pub async fn allow_request(&self) -> bool { - // First, check with a read lock for the common case + // First, check with a read lock for the common cases that don't + // mutate state (Closed passes, still-Open rejects). { let state = self.state.read().await; match state.state { CircuitState::Closed => return true, - CircuitState::HalfOpen => return true, + // Consuming a probe token requires the write lock below. + CircuitState::HalfOpen => {} CircuitState::Open => { // Check if timeout has expired if let Some(opened_at) = state.opened_at @@ -199,26 +233,61 @@ impl CircuitBreaker { } } - // Need write lock to transition from Open to HalfOpen + // Write lock: Open -> HalfOpen transition, or HalfOpen token use. let mut state = self.state.write().await; - // Re-check state in case another task already transitioned - if state.state == CircuitState::Open { - if let Some(opened_at) = state.opened_at - && opened_at.elapsed() >= self.config.open_duration - { - state.state = CircuitState::HalfOpen; - state.consecutive_successes = 0; - crate::metrics::set_circuit_breaker_state(1); - info!("Circuit breaker transitioning from Open to HalfOpen"); - return true; + match state.state { + // Another task closed the circuit while we waited for the lock. + CircuitState::Closed => true, + CircuitState::Open => { + if let Some(opened_at) = state.opened_at + && opened_at.elapsed() >= self.config.open_duration + { + state.state = CircuitState::HalfOpen; + state.consecutive_successes = 0; + self.grant_probe_tokens(&mut state); + // The transitioning caller takes the first probe token. + state.half_open_probes_remaining -= 1; + crate::metrics::set_circuit_breaker_state(1); + info!( + probes = state.half_open_probes_remaining + 1, + "Circuit breaker transitioning from Open to HalfOpen" + ); + return true; + } + self.requests_rejected.fetch_add(1, Ordering::Relaxed); + crate::metrics::record_circuit_breaker_rejection(); + false + } + CircuitState::HalfOpen => { + if state.half_open_probes_remaining == 0 { + // Re-grant after open_duration so leaked probes cannot + // wedge the breaker in HalfOpen (see doc above). + let window_expired = state + .half_open_granted_at + .is_none_or(|granted| granted.elapsed() >= self.config.open_duration); + if !window_expired { + self.requests_rejected.fetch_add(1, Ordering::Relaxed); + crate::metrics::record_circuit_breaker_rejection(); + return false; + } + self.grant_probe_tokens(&mut state); + debug!("Circuit breaker re-granted half-open probe tokens"); + } + state.half_open_probes_remaining -= 1; + true } - self.requests_rejected.fetch_add(1, Ordering::Relaxed); - crate::metrics::record_circuit_breaker_rejection(); - return false; } + } - true + /// Grant a fresh window of half-open probe tokens. + /// + /// `success_threshold` tokens (at least one, so a zero threshold cannot + /// deadlock the breaker) — exactly enough probes to close the circuit + /// if all of them succeed. + fn grant_probe_tokens(&self, state: &mut CircuitBreakerState) { + state.half_open_probes_remaining = self.config.success_threshold.max(1); + state.half_open_granted_at = Some(Instant::now()); } /// Record a successful operation. @@ -485,4 +554,99 @@ mod tests { assert_eq!(cb.state().await, CircuitState::Open); assert!(!cb.allow_request().await); } + + // ========================================================================= + // Half-open probe limiting (TD-2026-07-03) + // ========================================================================= + + #[tokio::test(start_paused = true)] + async fn test_half_open_limits_probes_to_success_threshold() { + let config = CircuitBreakerConfig::new(1, 2, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + cb.record_failure().await; + assert_eq!(cb.state().await, CircuitState::Open); + tokio::time::advance(Duration::from_secs(30)).await; + + // success_threshold = 2 probe tokens: two callers pass, the third + // is rejected instead of piling onto the recovering server. + assert!(cb.allow_request().await); + assert!(cb.allow_request().await); + assert_eq!(cb.state().await, CircuitState::HalfOpen); + + let rejected_before = cb.requests_rejected(); + assert!(!cb.allow_request().await); + assert_eq!(cb.requests_rejected(), rejected_before + 1); + } + + #[tokio::test(start_paused = true)] + async fn test_half_open_probe_tokens_regrant_after_open_duration() { + let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + cb.record_failure().await; + tokio::time::advance(Duration::from_secs(30)).await; + + // Single token consumed by the transitioning caller; its outcome is + // never recorded (the leaked-probe case) so the breaker sits in + // HalfOpen with zero tokens. + assert!(cb.allow_request().await); + assert!(!cb.allow_request().await); + + // The re-grant window keeps the breaker from wedging permanently. + tokio::time::advance(Duration::from_secs(30)).await; + assert!(cb.allow_request().await); + assert_eq!(cb.state().await, CircuitState::HalfOpen); + } + + #[tokio::test(start_paused = true)] + async fn test_half_open_reentry_grants_fresh_probe_tokens() { + let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + cb.record_failure().await; + tokio::time::advance(Duration::from_secs(30)).await; + + // Consume the only token, then fail the probe: back to Open. + assert!(cb.allow_request().await); + cb.record_failure().await; + assert_eq!(cb.state().await, CircuitState::Open); + + // Next half-open entry starts with a fresh token budget. + tokio::time::advance(Duration::from_secs(30)).await; + assert!(cb.allow_request().await); + assert_eq!(cb.state().await, CircuitState::HalfOpen); + } + + #[tokio::test(start_paused = true)] + async fn test_half_open_recovery_within_probe_budget() { + // The token budget equals success_threshold, so a healthy server + // can be probed back to Closed without any rejection in between. + let config = CircuitBreakerConfig::new(1, 2, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + cb.record_failure().await; + tokio::time::advance(Duration::from_secs(30)).await; + + assert!(cb.allow_request().await); + cb.record_success().await; + assert!(cb.allow_request().await); + cb.record_success().await; + + assert_eq!(cb.state().await, CircuitState::Closed); + assert!(cb.allow_request().await); + } + + #[tokio::test(start_paused = true)] + async fn test_half_open_zero_success_threshold_still_grants_a_probe() { + // Degenerate config: success_threshold = 0 must not deadlock the + // breaker with a zero-token grant. + let config = CircuitBreakerConfig::new(1, 0, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + cb.record_failure().await; + tokio::time::advance(Duration::from_secs(30)).await; + + assert!(cb.allow_request().await); + } } diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index 2832148..666d0a6 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -139,7 +139,9 @@ fn backoff_delay_ms(attempt: u32, base_ms: u64, max_ms: u64, jitter_unit: f64) - /// The client includes a circuit breaker that prevents request pile-up during outages: /// - **Closed** (normal): All requests pass through /// - **Open** (failing): Requests fail fast without attempting the operation -/// - **Half-Open** (recovery): Limited requests allowed to test if service recovered +/// - **Half-Open** (recovery): Probes limited to `success_threshold` tokens +/// per `open_duration` window; excess requests fail fast (see +/// `circuit_breaker` module docs for the token re-grant rules) /// /// # Performance Considerations /// diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index 43f5b0e..d450843 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -234,10 +234,8 @@ mod tests { #[tokio::test(start_paused = true)] async fn success_passes_through_and_records_breaker_success() { // Enter HalfOpen (success_threshold = 1) so record_success is - // observable: the single success must close the circuit. The breaker - // stamps opened_at with std Instant (not tokio's pausable clock), so - // a zero open_duration is what makes the Open->HalfOpen transition - // immediate and deterministic here. + // observable: the single success must close the circuit. Zero + // open_duration makes the Open->HalfOpen transition immediate. let breaker = CircuitBreaker::new(CircuitBreakerConfig::new(1, 1, Duration::ZERO)); breaker.force_open().await; let reconnects = Arc::new(AtomicU32::new(0)); From 3db35ba74d294d34c063176953843db6cf904727 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:22:28 -0700 Subject: [PATCH 03/15] feat(middleware): enforce X-Request-Timeout in the Iggy call path The header was parsed into a request extension but nothing consumed it; the documented deadline propagation changed nothing at runtime. Handlers now extract the parsed RequestTimeout and scope their Iggy access through AppState::{producer,consumer,iggy}_scoped -> IggyClientWrapper::with_timeout, a request-scoped view whose operation deadline is the header value clamped to the global OPERATION_TIMEOUT_SECS (clients may shorten, never extend). Scoped views share all Arc'd internals, so circuit-breaker bookkeeping and reconnect coordination stay global. Covered end-to-end by an integration test driving send, poll, and stream-management routes with valid, minimum, and malformed headers. Resolves TD-2026-07-04. --- CLAUDE.md | 14 +++++---- docs/tech-debt/TD-2026-07-04.md | 17 ++++++++++- src/handlers/messages.rs | 33 +++++++++++++-------- src/handlers/streams.rs | 39 ++++++++++++++++++------- src/handlers/topics.rs | 32 +++++++++++--------- src/iggy_client/mod.rs | 21 +++++++++++++ src/middleware/timeout.rs | 23 +++++++++++---- src/services/consumer.rs | 12 ++++++++ src/services/producer.rs | 12 ++++++++ src/state.rs | 34 +++++++++++++++++++++ tests/integration_tests.rs | 52 +++++++++++++++++++++++++++++++++ 11 files changed, 240 insertions(+), 49 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 350e65e..b312371 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -711,12 +711,14 @@ Request → Rate Limit → Auth → Request ID → Timeout → Tracing → CORS ### Request Timeout (`src/middleware/timeout.rs`) - Clients can specify `X-Request-Timeout: ` header -- Bounded: 100ms minimum, 5 minutes maximum -- Stored in request extensions for handler use -- `RequestTimeoutExt` trait for easy extraction in handlers -- **Note**: currently parsed and stored only — no handler consumes it yet, so - all Iggy operations are bounded by the global `OPERATION_TIMEOUT_SECS`. - Enforcement is tracked in `docs/tech-debt/TD-2026-07-04.md`. +- Bounded: 100ms minimum, 5 minutes maximum (header parse acceptance) +- Enforced end-to-end: all Iggy-touching handlers scope their client via + `AppState::{producer,consumer,iggy}_scoped` → + `IggyClientWrapper::with_timeout`, bounding every operation attempt by + the request deadline +- The effective deadline is clamped to the global `OPERATION_TIMEOUT_SECS` + — clients may shorten a request's bound, never extend it +- Requests without the header use the global timeout unchanged ## Deployment Security diff --git a/docs/tech-debt/TD-2026-07-04.md b/docs/tech-debt/TD-2026-07-04.md index a85154a..d0027fc 100644 --- a/docs/tech-debt/TD-2026-07-04.md +++ b/docs/tech-debt/TD-2026-07-04.md @@ -1,7 +1,7 @@ # TD-2026-07-04: X-Request-Timeout enforcement **Source:** Review session 01, Round 1 — silent-failure-hunter M4 (MEDIUM). -**Status:** open +**Status:** resolved (session 02) ## Problem @@ -18,3 +18,18 @@ Before the header is advertised in any client-facing documentation beyond CLAUDE.md (README API reference, OpenAPI, etc.), either wire `effective_timeout` into the handler → wrapper call path (per-request bound passed to `with_reconnect`) or delete the middleware and the claim. + +## Resolution (session 02) + +Maxim chose wiring over deletion. The header now propagates end-to-end: +the timeout middleware's parsed `RequestTimeout` is extracted by every +Iggy-touching handler (13 across messages/streams/topics) and passed +through `AppState::{producer_scoped, consumer_scoped, iggy_scoped}` into +`IggyClientWrapper::with_timeout` — a request-scoped view of the wrapper +whose operation deadline is the header value clamped to the global +`OPERATION_TIMEOUT_SECS` (clients may shorten, never extend). All Arc'd +internals are shared, so breaker bookkeeping and reconnect coordination +stay global. Covered by an end-to-end integration test +(`test_request_timeout_header_is_honored_end_to_end`) plus the existing +middleware parse-bound unit tests; CLAUDE.md and the module docs now +describe the enforced behavior. diff --git a/src/handlers/messages.rs b/src/handlers/messages.rs index 1bd17e3..92fc404 100644 --- a/src/handlers/messages.rs +++ b/src/handlers/messages.rs @@ -13,14 +13,15 @@ //! - `BATCH_MAX_SIZE` - Maximum messages per batch send (default: 1000) //! - `POLL_MAX_COUNT` - Maximum messages per poll (default: 100) -use axum::Json; -use axum::extract::{Path, Query, State}; +use axum::extract::{Extension, Path, Query, State}; use axum::http::StatusCode; +use axum::Json; use serde::Deserialize; use tracing::instrument; use crate::error::{AppError, AppResult}; use crate::iggy_client::PollParams; +use crate::middleware::RequestTimeout; use crate::models::{Event, PollMessagesResponse, SendMessageRequest, SendMessageResponse}; use crate::state::AppState; use crate::validation::{ @@ -43,16 +44,17 @@ use crate::validation::{ /// "partition_key": "optional-key" /// } /// ``` -#[instrument(skip(state, payload))] +#[instrument(skip(state, timeout, payload))] pub async fn send_message( State(state): State, + timeout: Option>, Json(payload): Json, ) -> AppResult<(StatusCode, Json)> { // Validate event type before processing validate_event_type(&payload.event.event_type)?; let response = state - .producer + .producer_scoped(timeout.map(|Extension(t)| t)) .send(&payload.event, payload.partition_key.as_deref()) .await?; @@ -90,9 +92,10 @@ pub struct SendBatchRequest { /// "partition_key": "optional-key" /// } /// ``` -#[instrument(skip(state, payload), fields(batch_size = payload.events.len()))] +#[instrument(skip(state, timeout, payload), fields(batch_size = payload.events.len()))] pub async fn send_batch( State(state): State, + timeout: Option>, Json(payload): Json, ) -> AppResult<(StatusCode, Json>)> { let max_batch_size = state.config.batch_max_size; @@ -118,7 +121,7 @@ pub async fn send_batch( } let responses = state - .producer + .producer_scoped(timeout.map(|Extension(t)| t)) .send_batch(&payload.events, payload.partition_key.as_deref()) .await?; @@ -167,9 +170,10 @@ fn default_count() -> u32 { /// ```bash /// curl "http://localhost:8000/messages?partition_id=1&count=10&offset=0" /// ``` -#[instrument(skip(state))] +#[instrument(skip(state, timeout))] pub async fn poll_messages( State(state): State, + timeout: Option>, Query(query): Query, ) -> AppResult> { // Validate poll parameters @@ -189,7 +193,10 @@ pub async fn poll_messages( None => params, }; - let response = state.consumer.poll(params).await?; + let response = state + .consumer_scoped(timeout.map(|Extension(t)| t)) + .poll(params) + .await?; Ok(Json(response)) } @@ -209,10 +216,11 @@ pub struct StreamTopicPath { /// /// - `stream` - Target stream name /// - `topic` - Target topic name -#[instrument(skip(state, payload))] +#[instrument(skip(state, timeout, payload))] pub async fn send_message_to( State(state): State, Path(path): Path, + timeout: Option>, Json(payload): Json, ) -> AppResult<(StatusCode, Json)> { // Validate path parameters before use @@ -222,7 +230,7 @@ pub async fn send_message_to( validate_event_type(&payload.event.event_type)?; let response = state - .producer + .producer_scoped(timeout.map(|Extension(t)| t)) .send_to( &path.stream, &path.topic, @@ -240,10 +248,11 @@ pub async fn send_message_to( /// /// - `stream` - Source stream name /// - `topic` - Source topic name -#[instrument(skip(state))] +#[instrument(skip(state, timeout))] pub async fn poll_messages_from( State(state): State, Path(path): Path, + timeout: Option>, Query(query): Query, ) -> AppResult> { // Validate path parameters before use @@ -268,7 +277,7 @@ pub async fn poll_messages_from( }; let response = state - .consumer + .consumer_scoped(timeout.map(|Extension(t)| t)) .poll_from(&path.stream, &path.topic, params) .await?; diff --git a/src/handlers/streams.rs b/src/handlers/streams.rs index e68f816..1b62426 100644 --- a/src/handlers/streams.rs +++ b/src/handlers/streams.rs @@ -1,18 +1,25 @@ use axum::Json; -use axum::extract::{Path, State}; +use axum::extract::{Extension, Path, State}; use axum::http::StatusCode; use tracing::instrument; use super::util::parse_timestamp_with_context; use crate::error::AppResult; +use crate::middleware::RequestTimeout; use crate::models::{CreateStreamRequest, StreamInfo}; use crate::state::AppState; use crate::validation::validate_resource_name; /// List all streams. -#[instrument(skip(state))] -pub async fn list_streams(State(state): State) -> AppResult>> { - let streams = state.iggy_client.list_streams().await?; +#[instrument(skip(state, timeout))] +pub async fn list_streams( + State(state): State, + timeout: Option>, +) -> AppResult>> { + let streams = state + .iggy_scoped(timeout.map(|Extension(t)| t)) + .list_streams() + .await?; let stream_infos: Vec = streams .into_iter() @@ -34,15 +41,19 @@ pub async fn list_streams(State(state): State) -> AppResult, Path(name): Path, + timeout: Option>, ) -> AppResult> { // Validate path parameter before use validate_resource_name(&name, "Stream")?; - let stream = state.iggy_client.get_stream(&name).await?; + let stream = state + .iggy_scoped(timeout.map(|Extension(t)| t)) + .get_stream(&name) + .await?; let created_at = parse_timestamp_with_context(stream.created_at.as_micros() as i64, "stream", &stream.name); @@ -58,28 +69,36 @@ pub async fn get_stream( } /// Create a new stream. -#[instrument(skip(state))] +#[instrument(skip(state, timeout, payload))] pub async fn create_stream( State(state): State, + timeout: Option>, Json(payload): Json, ) -> AppResult { validate_resource_name(&payload.name, "Stream")?; - state.iggy_client.create_stream(&payload.name).await?; + state + .iggy_scoped(timeout.map(|Extension(t)| t)) + .create_stream(&payload.name) + .await?; Ok(StatusCode::CREATED) } /// Delete a stream by name. -#[instrument(skip(state))] +#[instrument(skip(state, timeout))] pub async fn delete_stream( State(state): State, Path(name): Path, + timeout: Option>, ) -> AppResult { // Validate path parameter before use validate_resource_name(&name, "Stream")?; - state.iggy_client.delete_stream(&name).await?; + state + .iggy_scoped(timeout.map(|Extension(t)| t)) + .delete_stream(&name) + .await?; Ok(StatusCode::NO_CONTENT) } diff --git a/src/handlers/topics.rs b/src/handlers/topics.rs index 0fb30b0..a8798da 100644 --- a/src/handlers/topics.rs +++ b/src/handlers/topics.rs @@ -1,11 +1,12 @@ use axum::Json; -use axum::extract::{Path, State}; +use axum::extract::{Extension, Path, State}; use axum::http::StatusCode; use serde::Deserialize; use tracing::instrument; use super::util::parse_timestamp_with_context; use crate::error::AppResult; +use crate::middleware::RequestTimeout; use crate::models::{CreateTopicRequest, TopicInfo}; use crate::state::AppState; use crate::validation::{validate_partition_count, validate_resource_name}; @@ -24,17 +25,19 @@ pub struct TopicPath { } /// List all topics in a stream. -#[instrument(skip(state))] +#[instrument(skip(state, timeout))] pub async fn list_topics( State(state): State, Path(path): Path, + timeout: Option>, ) -> AppResult>> { // Validate path parameter before use validate_resource_name(&path.stream, "Stream")?; - let topics = state.iggy_client.list_topics(&path.stream).await?; + let client = state.iggy_scoped(timeout.map(|Extension(t)| t)); + let topics = client.list_topics(&path.stream).await?; - let stream_details = state.iggy_client.get_stream(&path.stream).await?; + let stream_details = client.get_stream(&path.stream).await?; let topic_infos: Vec = topics .into_iter() .map(|t| { @@ -56,21 +59,20 @@ pub async fn list_topics( } /// Get a specific topic by name. -#[instrument(skip(state))] +#[instrument(skip(state, timeout))] pub async fn get_topic( State(state): State, Path(path): Path, + timeout: Option>, ) -> AppResult> { // Validate path parameters before use validate_resource_name(&path.stream, "Stream")?; validate_resource_name(&path.topic, "Topic")?; - let topic = state - .iggy_client - .get_topic(&path.stream, &path.topic) - .await?; + let client = state.iggy_scoped(timeout.map(|Extension(t)| t)); + let topic = client.get_topic(&path.stream, &path.topic).await?; - let stream_details = state.iggy_client.get_stream(&path.stream).await?; + let stream_details = client.get_stream(&path.stream).await?; let created_at = parse_timestamp_with_context(topic.created_at.as_micros() as i64, "topic", &topic.name); @@ -86,10 +88,11 @@ pub async fn get_topic( } /// Create a new topic in a stream. -#[instrument(skip(state))] +#[instrument(skip(state, timeout, payload))] pub async fn create_topic( State(state): State, Path(path): Path, + timeout: Option>, Json(payload): Json, ) -> AppResult { // Validate path parameter before use @@ -99,7 +102,7 @@ pub async fn create_topic( validate_partition_count(payload.partitions, "Topic")?; state - .iggy_client + .iggy_scoped(timeout.map(|Extension(t)| t)) .create_topic(&path.stream, &payload.name, payload.partitions) .await?; @@ -107,17 +110,18 @@ pub async fn create_topic( } /// Delete a topic from a stream. -#[instrument(skip(state))] +#[instrument(skip(state, timeout))] pub async fn delete_topic( State(state): State, Path(path): Path, + timeout: Option>, ) -> AppResult { // Validate path parameters before use validate_resource_name(&path.stream, "Stream")?; validate_resource_name(&path.topic, "Topic")?; state - .iggy_client + .iggy_scoped(timeout.map(|Extension(t)| t)) .delete_topic(&path.stream, &path.topic) .await?; diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index 666d0a6..09413cc 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -985,6 +985,27 @@ impl IggyClientWrapper { &self.config } + /// Return a view of this wrapper whose operations are bounded by + /// `timeout` instead of the configured `OPERATION_TIMEOUT_SECS`. + /// + /// This is how the `X-Request-Timeout` header propagates into the Iggy + /// call path: handlers scope the client to the request's deadline. The + /// bound is clamped to the configured global timeout — a client may + /// SHORTEN the deadline, never extend it. + /// + /// All `Arc`'d internals (SDK client, connection state, circuit + /// breaker) are shared with the parent, so breaker bookkeeping and + /// reconnection coordination stay global; only the deadline changes. + /// A reconnect session triggered under a scoped view has its caller + /// wait bounded by the same shortened deadline (the session itself + /// continues in the background — see `reconnect_bounded`). + #[must_use] + pub fn with_timeout(&self, timeout: Duration) -> Self { + let mut scoped = self.clone(); + scoped.config.operation_timeout = timeout.min(self.config.operation_timeout); + scoped + } + /// Get the current circuit breaker state. pub async fn circuit_breaker_state(&self) -> CircuitState { self.circuit_breaker.state().await diff --git a/src/middleware/timeout.rs b/src/middleware/timeout.rs index 0e1b02d..bf14e6f 100644 --- a/src/middleware/timeout.rs +++ b/src/middleware/timeout.rs @@ -10,19 +10,28 @@ //! X-Request-Timeout: 5000 # 5 seconds in milliseconds //! ``` //! -//! Handlers can then extract this timeout: +//! Handlers extract the parsed value and scope their Iggy access to it: //! ```rust,ignore //! async fn handler( +//! State(state): State, //! timeout: Option>, -//! // ... //! ) -> impl IntoResponse { -//! let effective_timeout = timeout -//! .map(|t| t.0.duration) -//! .unwrap_or(default_timeout); -//! // Use effective_timeout for operations +//! // Iggy operations bounded by the request deadline (clamped to the +//! // configured global OPERATION_TIMEOUT_SECS — clients may shorten, +//! // never extend). +//! let client = state.iggy_scoped(timeout.map(|Extension(t)| t)); +//! client.list_streams().await //! } //! ``` //! +//! # Enforcement path +//! +//! All Iggy-touching handlers pass the extracted timeout through +//! `AppState::{producer_scoped, consumer_scoped, iggy_scoped}` into +//! `IggyClientWrapper::with_timeout`, which bounds every operation attempt +//! (and the caller's wait on any reconnect) by the request deadline. +//! Requests without the header use the global `OPERATION_TIMEOUT_SECS`. +//! //! # Benefits //! //! - Prevents wasted work when clients have already given up @@ -32,6 +41,8 @@ //! # Security Considerations //! //! - Minimum and maximum timeout bounds are enforced to prevent abuse +//! - The effective deadline is additionally clamped to the server's global +//! operation timeout — the header can never EXTEND server-side work //! - Invalid values are ignored (fall back to server default) //! - Zero or negative values are rejected diff --git a/src/services/consumer.rs b/src/services/consumer.rs index 8a92cae..8273ea6 100644 --- a/src/services/consumer.rs +++ b/src/services/consumer.rs @@ -48,6 +48,18 @@ impl ConsumerService { } } + /// Return a view of this service whose Iggy operations are bounded by + /// `timeout` (clamped to the configured global — see + /// [`IggyClientWrapper::with_timeout`]). The consumed-messages counter + /// is shared with the parent, so stats stay global. + #[must_use] + pub fn with_timeout(&self, timeout: std::time::Duration) -> Self { + Self { + client: self.client.with_timeout(timeout), + messages_consumed: Arc::clone(&self.messages_consumed), + } + } + /// Poll messages from the default stream and topic. /// /// # Arguments diff --git a/src/services/producer.rs b/src/services/producer.rs index 3011ca5..936f3e2 100644 --- a/src/services/producer.rs +++ b/src/services/producer.rs @@ -36,6 +36,18 @@ impl ProducerService { } } + /// Return a view of this service whose Iggy operations are bounded by + /// `timeout` (clamped to the configured global — see + /// [`IggyClientWrapper::with_timeout`]). The sent-messages counter is + /// shared with the parent, so stats stay global. + #[must_use] + pub fn with_timeout(&self, timeout: std::time::Duration) -> Self { + Self { + client: self.client.with_timeout(timeout), + messages_sent: Arc::clone(&self.messages_sent), + } + } + /// Send an event to the default stream and topic. #[instrument(skip(self, event), fields(event_id = %event.id))] pub async fn send( diff --git a/src/state.rs b/src/state.rs index 94becea..c4f63ce 100644 --- a/src/state.rs +++ b/src/state.rs @@ -30,6 +30,7 @@ use tracing::{debug, info, trace, warn}; use crate::config::Config; use crate::iggy_client::IggyClientWrapper; +use crate::middleware::RequestTimeout; use crate::services::{ConsumerService, ProducerService}; /// Cached statistics for efficient `/stats` endpoint. @@ -134,6 +135,39 @@ impl AppState { state } + // ========================================================================= + // Request-scoped views (X-Request-Timeout propagation) + // ========================================================================= + // + // Handlers pass the optional client-supplied `RequestTimeout` (parsed by + // the timeout middleware) and receive a service view whose Iggy + // operations are bounded by that deadline, clamped to the configured + // global. Without a header the shared instance is used as-is. + + /// Producer scoped to the request's effective timeout. + pub fn producer_scoped(&self, timeout: Option) -> ProducerService { + match timeout { + Some(t) => self.producer.with_timeout(t.duration), + None => self.producer.clone(), + } + } + + /// Consumer scoped to the request's effective timeout. + pub fn consumer_scoped(&self, timeout: Option) -> ConsumerService { + match timeout { + Some(t) => self.consumer.with_timeout(t.duration), + None => self.consumer.clone(), + } + } + + /// Iggy client scoped to the request's effective timeout. + pub fn iggy_scoped(&self, timeout: Option) -> IggyClientWrapper { + match timeout { + Some(t) => self.iggy_client.with_timeout(t.duration), + None => self.iggy_client.clone(), + } + } + /// Get the current cached statistics. /// /// Returns the cached stats without blocking. If the cache is empty, diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 385bd7c..a5028a9 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -981,6 +981,58 @@ async fn test_message_with_partition_key() { assert!(response.status().is_success()); } +/// X-Request-Timeout propagation (TD-2026-07-04): a valid header must flow +/// through the scoped producer/consumer path and still serve the request; a +/// malformed header is ignored (falls back to the global operation timeout) +/// rather than rejected. +#[tokio::test] +async fn test_request_timeout_header_is_honored_end_to_end() { + let fixture = TestFixture::new().await; + + // Valid header on a send: bounded by 5s instead of the 30s global. + let response = fixture + .client + .post(fixture.url("/messages")) + .header("X-Request-Timeout", "5000") + .json(&generic_event("request-timeout-send", None)) + .send() + .await + .expect("Send with timeout header failed"); + assert_eq!(response.status(), 201); + + // Valid-but-minimum header on a poll: 100ms is plenty against a local + // container, and exercises the scoped consumer path. + let response = fixture + .client + .get(fixture.url("/messages?partition_id=0&count=1&offset=0")) + .header("X-Request-Timeout", "100") + .send() + .await + .expect("Poll with timeout header failed"); + assert!(response.status().is_success()); + + // Malformed header: ignored, request served under the global timeout. + let response = fixture + .client + .post(fixture.url("/messages")) + .header("X-Request-Timeout", "not-a-number") + .json(&generic_event("request-timeout-malformed", None)) + .send() + .await + .expect("Send with malformed timeout header failed"); + assert_eq!(response.status(), 201); + + // Scoped stream management path (iggy_scoped). + let response = fixture + .client + .get(fixture.url("/streams")) + .header("X-Request-Timeout", "2000") + .send() + .await + .expect("List streams with timeout header failed"); + assert!(response.status().is_success()); +} + #[tokio::test] async fn test_send_to_specific_stream_topic() { let fixture = TestFixture::new().await; From 33e608940207e70b69faa5c297933db5103b1548 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:23:26 -0700 Subject: [PATCH 04/15] test(metrics): add Prometheus exporter smoke test init_metrics (recorder install + HTTP listener) had zero runtime coverage; a metrics-exporter-prometheus behavior change would have been discovered at production startup. The new test binary installs the exporter on an ephemeral port, records via the public counter/histogram/gauge helpers, scrapes /metrics, and asserts the metric names and labels appear. Own test binary because the recorder install is process-global. Resolves TD-2026-07-05. --- docs/tech-debt/TD-2026-07-05.md | 12 +++++- tests/metrics_smoke_test.rs | 76 +++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 tests/metrics_smoke_test.rs diff --git a/docs/tech-debt/TD-2026-07-05.md b/docs/tech-debt/TD-2026-07-05.md index 70974bc..16f05b6 100644 --- a/docs/tech-debt/TD-2026-07-05.md +++ b/docs/tech-debt/TD-2026-07-05.md @@ -1,7 +1,7 @@ # TD-2026-07-05: Metrics exporter smoke test **Source:** Review session 01, Round 1 — pr-test-analyzer #7 (MEDIUM). -**Status:** open +**Status:** resolved (session 02) ## Problem @@ -16,3 +16,13 @@ panic without a recorder installed. At the next metrics-exporter-prometheus major/minor bump, add one test that calls `init_metrics` on an ephemeral port, records a counter, and fetches `/metrics` asserting the metric name appears. + +## Resolution (session 02) + +Resolved ahead of the trigger (scheduled by the session-02 kickoff as a +quick win). `tests/metrics_smoke_test.rs` — its own integration-test +binary, so the process-global recorder install cannot collide with other +tests — calls `init_metrics` on an ephemeral port, records through the +public counter/histogram/gauge helpers, scrapes `/metrics` over HTTP +(with startup retry), and asserts all three metric names and the counter +labels appear in the exposition output. diff --git a/tests/metrics_smoke_test.rs b/tests/metrics_smoke_test.rs new file mode 100644 index 0000000..c82f7e9 --- /dev/null +++ b/tests/metrics_smoke_test.rs @@ -0,0 +1,76 @@ +//! Smoke test for the Prometheus metrics exporter (TD-2026-07-05). +//! +//! `init_metrics` installs a process-global recorder, so this lives in its +//! own integration-test binary (own process) where the single install +//! cannot collide with any other test. +//! +//! Run with: `cargo test --test metrics_smoke_test` +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use std::net::{SocketAddr, TcpListener}; +use std::time::Duration; + +use iggy_sample::metrics; + +/// Reserve an ephemeral loopback port, then release it for the exporter. +/// +/// A small TOCTOU window exists between drop and rebind; acceptable for a +/// test, and the same pattern the app integration tests use. +fn ephemeral_addr() -> SocketAddr { + TcpListener::bind("127.0.0.1:0") + .expect("bind ephemeral port") + .local_addr() + .expect("local addr") +} + +#[tokio::test] +async fn init_metrics_serves_recorded_counter_over_http() { + let addr = ephemeral_addr(); + + metrics::init_metrics(addr).expect("Prometheus exporter must install and bind"); + + // Record through the public helpers (counter, histogram, gauge) so a + // metrics-exporter-prometheus behavior change in any register path is + // caught here instead of at production startup. + metrics::record_message_sent("smoke-stream", "smoke-topic", "success"); + metrics::record_send_duration("smoke-stream", "smoke-topic", 0.042); + metrics::set_connection_status(true); + + // Scrape /metrics, retrying briefly while the listener task comes up. + let url = format!("http://{addr}/metrics"); + let deadline = tokio::time::Instant::now() + Duration::from_secs(10); + let body = loop { + match reqwest::get(&url).await { + Ok(response) => { + assert!( + response.status().is_success(), + "scrape failed: {}", + response.status() + ); + break response.text().await.expect("scrape body"); + } + Err(e) if tokio::time::Instant::now() < deadline => { + eprintln!("exporter not up yet ({e}), retrying"); + tokio::time::sleep(Duration::from_millis(100)).await; + } + Err(e) => panic!("exporter never came up on {url}: {e}"), + } + }; + + assert!( + body.contains(metrics::names::MESSAGES_SENT_TOTAL), + "counter missing from scrape:\n{body}" + ); + assert!( + body.contains(r#"stream="smoke-stream""#), + "counter labels missing from scrape:\n{body}" + ); + assert!( + body.contains(metrics::names::SEND_DURATION_SECONDS), + "histogram missing from scrape:\n{body}" + ); + assert!( + body.contains(metrics::names::CONNECTION_STATUS), + "gauge missing from scrape:\n{body}" + ); +} From 69fd81c8ea6dda2efd9ae05cc384de6592c5a8ae Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:26:56 -0700 Subject: [PATCH 05/15] ci: pin third-party GitHub Actions to full commit SHAs Mutable tags (dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2, taiki-e/install-action@, codecov/codecov-action@v4, rustsec/audit-check@v2, softprops/action-gh-release@v1) let a compromised or force-moved tag execute attacker-controlled code in CI. Every third-party action across the four workflows is now pinned to a 40-char commit SHA with a version comment. Selector-style tags are converted to explicit inputs: dtolnay usages normalize to the master-branch SHA with 'with: toolchain:', and taiki-e/install-action tool tags become 'with: tool:'. Dependabot's github-actions ecosystem (already enabled) bumps SHA pins and their version comments natively. Resolves TD-2026-07-07. --- .github/workflows/ci.yml | 47 ++++++++++++++++++---------- .github/workflows/extended-tests.yml | 35 ++++++++++++++------- .github/workflows/pr.yml | 25 ++++++++++----- .github/workflows/release.yml | 19 ++++++----- docs/tech-debt/TD-2026-07-07.md | 24 +++++++++++++- 5 files changed, 105 insertions(+), 45 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 222fd38..f0bec0e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,8 +34,9 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@nightly + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master with: + toolchain: nightly components: rustfmt - name: Check formatting run: cargo fmt --all -- --check @@ -48,10 +49,11 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master with: + toolchain: stable components: clippy - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Run Clippy run: cargo clippy --all-targets --all-features -- -D warnings @@ -73,10 +75,10 @@ jobs: rust: "1.93.0" steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@master + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master with: toolchain: ${{ matrix.rust }} - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 with: key: ${{ matrix.os }}-${{ matrix.rust }} @@ -98,8 +100,10 @@ jobs: needs: [fmt, clippy] steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Run integration tests run: cargo test --test '*' --all-features @@ -116,19 +120,22 @@ jobs: needs: [test] steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master with: + toolchain: stable components: llvm-tools-preview - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Install cargo-llvm-cov - uses: taiki-e/install-action@cargo-llvm-cov + uses: taiki-e/install-action@4684b8405694ae9dd42c9f39ba901a70ae83f4a3 # v2.82.9 + with: + tool: cargo-llvm-cov - name: Generate coverage report run: cargo llvm-cov --all-features --workspace --lcov --output-path lcov.info - name: Upload coverage to Codecov - uses: codecov/codecov-action@v4 + uses: codecov/codecov-action@b9fd7d16f6d7d1b5d2bec1a2887e65ceed900238 # v4.6.0 with: files: lcov.info fail_ci_if_error: false @@ -143,8 +150,10 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Build documentation run: cargo doc --no-deps --all-features @@ -163,7 +172,7 @@ jobs: issues: write steps: - uses: actions/checkout@v4 - - uses: rustsec/audit-check@v2 + - uses: rustsec/audit-check@69366f33c96575abad1ee0dba8212993eecbe998 # v2.0.0 with: token: ${{ secrets.GITHUB_TOKEN }} @@ -175,11 +184,15 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Install cargo-deny - uses: taiki-e/install-action@cargo-deny + uses: taiki-e/install-action@4684b8405694ae9dd42c9f39ba901a70ae83f4a3 # v2.82.9 + with: + tool: cargo-deny - name: Check dependency policy (advisories, bans, licenses, sources) run: cargo deny check diff --git a/.github/workflows/extended-tests.yml b/.github/workflows/extended-tests.yml index 7ea9db4..2db3430 100644 --- a/.github/workflows/extended-tests.yml +++ b/.github/workflows/extended-tests.yml @@ -41,8 +41,10 @@ jobs: if: ${{ github.event_name == 'schedule' || github.event.inputs.run_benchmarks == 'true' }} steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Install criterion dependencies run: | @@ -98,8 +100,10 @@ jobs: --health-retries 10 steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Wait for Iggy to be ready run: | @@ -133,8 +137,10 @@ jobs: if: ${{ github.event_name == 'schedule' || github.event.inputs.run_memory_check == 'true' }} steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Install Valgrind run: | @@ -172,10 +178,11 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@nightly + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master with: + toolchain: nightly components: miri - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Setup Miri run: cargo miri setup @@ -205,8 +212,10 @@ jobs: - "--no-default-features" steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 with: key: features-${{ matrix.features }} @@ -224,8 +233,10 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@nightly - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: nightly + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Check documentation coverage run: | diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 4766214..c0049eb 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -106,8 +106,10 @@ jobs: with: fetch-depth: 0 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Check for missing documentation run: | @@ -137,11 +139,15 @@ jobs: with: fetch-depth: 0 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Install cargo-semver-checks - uses: taiki-e/install-action@cargo-semver-checks + uses: taiki-e/install-action@4684b8405694ae9dd42c9f39ba901a70ae83f4a3 # v2.82.9 + with: + tool: cargo-semver-checks - name: Check for breaking changes run: cargo semver-checks check-release @@ -189,13 +195,16 @@ jobs: with: fetch-depth: 0 - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master with: + toolchain: stable components: llvm-tools-preview - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Install cargo-llvm-cov - uses: taiki-e/install-action@cargo-llvm-cov + uses: taiki-e/install-action@4684b8405694ae9dd42c9f39ba901a70ae83f4a3 # v2.82.9 + with: + tool: cargo-llvm-cov - name: Generate coverage summary run: | diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c2cd486..ebed9e0 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -99,11 +99,12 @@ jobs: steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master with: + toolchain: stable targets: ${{ matrix.target }} - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 with: key: release-${{ matrix.target }} @@ -185,7 +186,7 @@ jobs: echo "Generated changelog with $(wc -l < CHANGELOG.md) entries" - name: Create release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@de2c0eb89ae2a093876385947365aca7b0e5f844 # v1 with: name: Release v${{ needs.validate.outputs.version }} body_path: CHANGELOG.md @@ -204,8 +205,10 @@ jobs: if: needs.validate.outputs.is_prerelease == 'false' steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Publish to crates.io run: cargo publish --token ${{ secrets.CRATES_IO_TOKEN }} @@ -226,8 +229,10 @@ jobs: url: ${{ steps.deployment.outputs.page_url }} steps: - uses: actions/checkout@v4 - - uses: dtolnay/rust-toolchain@stable - - uses: Swatinem/rust-cache@v2 + - uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master + with: + toolchain: stable + - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 - name: Build documentation run: cargo doc --no-deps --all-features diff --git a/docs/tech-debt/TD-2026-07-07.md b/docs/tech-debt/TD-2026-07-07.md index 8db3804..89d9b31 100644 --- a/docs/tech-debt/TD-2026-07-07.md +++ b/docs/tech-debt/TD-2026-07-07.md @@ -1,7 +1,7 @@ # TD-2026-07-07: Pin third-party GitHub Actions to commit SHAs **Source:** automated security review on the v0.2.0 release PR (MEDIUM). -**Status:** open +**Status:** resolved (session 02) ## Problem @@ -26,3 +26,25 @@ full 40-char commit SHA with a version comment (`uses: owner/action@ # vX`), and enable Dependabot's `github-actions` ecosystem updates to keep the pins fresh (already configured for version updates — verify it bumps SHA pins too). + +## Resolution (session 02) + +Trigger fired ahead of this session: PR #25 (`fix(ci): correct release +binary name`) was a CI-focused change that landed after this record was +written. All third-party actions across the four workflows are now +pinned to full 40-char commit SHAs with a version comment: + +- `dtolnay/rust-toolchain@fa04a145… # master` — all usages normalized to + the master-branch SHA with an explicit `with: toolchain:` input (the + branch name was previously doing double duty as the toolchain selector) +- `Swatinem/rust-cache@e18b4977… # v2` +- `taiki-e/install-action@4684b840… # v2.82.9` — tool-name tags converted + to `with: tool:` inputs +- `codecov/codecov-action@b9fd7d16… # v4.6.0` +- `rustsec/audit-check@69366f33… # v2.0.0` +- `softprops/action-gh-release@de2c0eb8… # v1` + +First-party `actions/*` steps remain tag-pinned (GitHub-owned, out of +this record's third-party scope). Dependabot's `github-actions` +ecosystem (already configured, weekly) updates SHA pins and their +version comments natively, keeping the pins fresh. From 9363b8c4ef969efd1531482267efdf77a63526ad Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:41:18 -0700 Subject: [PATCH 06/15] style: apply rustfmt to session-02 changes --- src/handlers/messages.rs | 2 +- src/iggy_client/resilience.rs | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/handlers/messages.rs b/src/handlers/messages.rs index 92fc404..902f915 100644 --- a/src/handlers/messages.rs +++ b/src/handlers/messages.rs @@ -13,9 +13,9 @@ //! - `BATCH_MAX_SIZE` - Maximum messages per batch send (default: 1000) //! - `POLL_MAX_COUNT` - Maximum messages per poll (default: 100) +use axum::Json; use axum::extract::{Extension, Path, Query, State}; use axum::http::StatusCode; -use axum::Json; use serde::Deserialize; use tracing::instrument; diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index d450843..57a6b0e 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -48,9 +48,7 @@ use crate::error::{AppError, AppResult}; pub(super) fn is_connection_error(error: &AppError) -> bool { matches!( error, - AppError::ConnectionFailed(_) - | AppError::Disconnected(_) - | AppError::ConnectionReset(_) + AppError::ConnectionFailed(_) | AppError::Disconnected(_) | AppError::ConnectionReset(_) ) } @@ -421,7 +419,11 @@ mod tests { assert!( matches!(&result, Err(AppError::ConnectionFailed(msg)) if msg.contains("reconnect exhausted")) ); - assert_eq!(calls.load(Ordering::SeqCst), 1, "no retry after failed reconnect"); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "no retry after failed reconnect" + ); assert_eq!(reconnects.load(Ordering::SeqCst), 1); } From 69c1e47d7bd597929e705812446622aab9d99947 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:41:18 -0700 Subject: [PATCH 07/15] docs(iggy_client): complete the module-structure doc list --- src/iggy_client/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index 09413cc..547ec4e 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -25,9 +25,11 @@ //! //! # Module Structure //! +//! - `circuit_breaker` - Fail-fast state machine with token-limited probing //! - `connection` - Connection state tracking for reconnection coordination //! - `params` - Parameter types like `PollParams` //! - `helpers` - Utility functions for identifier conversion and jitter +//! - `resilience` - Timeout/breaker/reconnect-retry composition (`run_resilient`) //! - `scopeguard` - RAII guard for cleanup on drop //! //! # Connection Resilience (two layers) @@ -1094,5 +1096,4 @@ mod tests { ); } } - } From b424a559d4f709eb34e5e3339f08fac2702c80a7 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:41:18 -0700 Subject: [PATCH 08/15] docs(guides): re-validate durable-storage guide against server 0.8.0 Every documented config key checked against upstream tag server-0.8.0 (shipped core/server/config.toml + config schema sources). Highlights: message_saver is top-level with a 30s default interval; cache_indexes is a string enum and the separate time index is gone; validate_checksum, message_expiry, and max_size moved sections; the disk/S3 archiver section documented a feature removed before 0.8.0 and is now a removed-feature notice; retention is enforced by the default-DISABLED data_maintenance.messages cleaner (previously undocumented, so the guide's retention configs were silently non-functional); size-based deletion happens automatically at 90% of max_size. Env-var table and SDK snippet corrected; guide re-stamped. Resolves TD-2026-07-06. --- docs/durable-storage-guide.md | 472 ++++++++++++++++---------------- docs/tech-debt/TD-2026-07-06.md | 34 ++- 2 files changed, 276 insertions(+), 230 deletions(-) diff --git a/docs/durable-storage-guide.md b/docs/durable-storage-guide.md index 2a8aa59..87915a2 100644 --- a/docs/durable-storage-guide.md +++ b/docs/durable-storage-guide.md @@ -2,6 +2,10 @@ A comprehensive guide to configuring Apache Iggy for durable, production-ready message storage. +> **Validated key-by-key against Apache Iggy server 0.8.0** (upstream tag `server-0.8.0`, +> shipped default config `core/server/config.toml` and config sources in +> `core/configs/src/server_config/`) on 2026-07-05. + --- ## Table of Contents @@ -27,18 +31,21 @@ Iggy uses an **append-only log** as its core storage abstraction. This is the sa ``` Topic: "orders" (3 partitions) -├── Partition 0/ -│ ├── segment-0000000000.log (messages 0-999,999) -│ ├── segment-0000000000.idx (offset index) -│ ├── segment-0000000000.tidx (time index) -│ ├── segment-0001000000.log (messages 1,000,000-1,999,999) +├── partitions/0/ +│ ├── 00000000000000000000.log (messages from offset 0) +│ ├── 00000000000000000000.index (offset + timestamp index) +│ ├── 00000000000001000000.log (messages from offset 1,000,000) │ └── ... -├── Partition 1/ +├── partitions/1/ │ └── ... -└── Partition 2/ +└── partitions/2/ └── ... ``` +Segment files are named by their 20-digit zero-padded start offset. Each segment +has a single `.index` companion file covering both positional and time-based lookups +(older Iggy versions used separate `.idx`/`.tidx` files). + ### Key Characteristics | Property | Description | @@ -50,7 +57,9 @@ Topic: "orders" (3 partitions) ### Storage Location -By default, all data is persisted under the `local_data` directory. Configure the path in `server.toml`: +By default, all data is persisted under the `local_data` directory. Configure +the path in the server's `config.toml` (server 0.8.0 embeds a default config; +point `IGGY_CONFIG_PATH` at your own file to override it): ```toml [system] @@ -95,20 +104,21 @@ With fsync enabled: #### 1. Message Saver Configuration -The `message_saver` section controls background persistence: +The top-level `[message_saver]` section controls background persistence +(defaults shown are the shipped 0.8.0 defaults): ```toml -[system.partition.message_saver] -# Enable background message saving +[message_saver] +# Enable the background process that saves buffered data to disk enabled = true # Force synchronous writes to disk -# true = durable but slower (data guaranteed on disk before ack) +# true = durable but slower (data guaranteed on disk) # false = faster but data may be lost on crash enforce_fsync = true -# Interval between save operations (when not using fsync per message) -interval = "1s" +# Interval for running the message saver (default: 30 s) +interval = "30 s" ``` #### 2. Per-Message fsync (Maximum Durability) @@ -117,11 +127,15 @@ For the strongest durability guarantee, force fsync after every message: ```toml [system.partition] -# Force fsync on every partition write +# Force fsync on every partition write (default: false) enforce_fsync = true -# Flush after each message (set to 1 for maximum durability) +# Flush after each message (set to 1 for maximum durability; default: 1024) messages_required_to_save = 1 + +# Companion size threshold - a save is also triggered once buffered +# messages reach this size (default: "1 MiB") +size_of_messages_required_to_save = "1 MiB" ``` Or via environment variables: @@ -135,10 +149,10 @@ export IGGY_SYSTEM_PARTITION_MESSAGES_REQUIRED_TO_SAVE=1 | Level | Configuration | Latency | Data Loss Risk | |-------|---------------|---------|----------------| -| **Maximum** | `enforce_fsync=true`, `messages_required_to_save=1` | ~9ms P99 | None (survives power loss) | -| **High** | `enforce_fsync=true`, `messages_required_to_save=100` | ~1-2ms P99 | Up to 100 messages | -| **Balanced** | `enforce_fsync=true`, `interval="1s"` | ~0.5ms P99 | Up to 1 second of data | -| **Performance** | `enforce_fsync=false` | ~0.1ms P99 | Unbounded (OS-dependent) | +| **Maximum** | `[system.partition]` `enforce_fsync=true`, `messages_required_to_save=1` | ~9ms P99 | None (survives power loss) | +| **High** | `[system.partition]` `enforce_fsync=true`, `messages_required_to_save=100` | ~1-2ms P99 | Up to 100 messages | +| **Balanced** | `[system.partition]` `enforce_fsync=false` + `[message_saver]` `enforce_fsync=true`, `interval="1s"` | ~0.5ms P99 | Up to 1 saver interval of data | +| **Performance** | `enforce_fsync=false` everywhere | ~0.1ms P99 | Unbounded (OS-dependent) | ### Performance Benchmarks @@ -162,93 +176,112 @@ Iggy stores messages in **segments**, which are physical files on disk. ```toml [system.segment] -# Maximum segment size before rolling to new segment +# Soft limit for segment size before rolling to a new segment (default: "1 GiB") +# Hard maximum is 1 GiB, and the size must be a multiple of 512 B. # Larger = fewer files, potentially slower recovery # Smaller = more files, faster recovery, more overhead -size = "1 GB" +size = "1 GiB" -# Cache offset indexes in memory (faster reads) -cache_indexes = true +# Index caching strategy (single .index file covers offset + time lookups): +# "all" (or true) = keep all indexes in memory (fastest, most memory) +# "open_segment" = cache indexes only for the currently open segment (default) +# "none" (or false) = always read indexes from disk (least memory) +cache_indexes = "open_segment" -# Cache time-based indexes in memory -cache_time_indexes = true - -# Validate checksums when loading segments (integrity check) +[system.partition] +# Validate checksums (CRC) when loading data (integrity check; default: false) validate_checksum = true ``` +> **Changed in 0.8.0**: `cache_indexes` is now a string enum (`"all"`, +> `"open_segment"`, `"none"`) rather than a boolean, the separate +> `cache_time_indexes` option was removed (one combined index file), and +> `validate_checksum` lives under `[system.partition]`, not `[system.segment]`. + ### Segment Lifecycle ``` 1. ACTIVE: Current segment receiving writes - └── segment-0000000000.log (< size limit) + └── 00000000000000000000.log (< size limit) -2. ROLLED: Size limit reached, new segment created - ├── segment-0000000000.log (closed, read-only) - └── segment-0001000000.log (now active) +2. SEALED: Size limit reached, new segment created + ├── 00000000000000000000.log (closed, read-only) + └── 00000000000001000000.log (now active) 3. EXPIRED: All messages in segment past retention - └── segment-0000000000.log (eligible for deletion/archival) + └── 00000000000000000000.log (eligible for deletion) -4. ARCHIVED/DELETED: Based on configuration - └── (moved to archive location or deleted) +4. DELETED: Removed by the message cleaner + └── (the active segment is always protected) ``` ### Index Files -Each segment has associated index files: +Each segment has one associated index file: | File | Purpose | |------|---------| | `.log` | Message data (binary format) | -| `.idx` | Offset index (find message by offset) | -| `.tidx` | Time index (find message by timestamp) | +| `.index` | Combined index (find message by offset or timestamp) | ### Checksum Validation Enable checksum validation to detect data corruption: ```toml -[system.segment] -validate_checksum = true # Validates on segment load +[system.partition] +validate_checksum = true # Validates CRCs when loading data (default: false) ``` -This adds overhead during segment loading but catches corruption early. +This adds overhead during data loading but catches corruption early. --- ## Data Retention Policies -Iggy supports both time-based and size-based retention. +Iggy supports both time-based and size-based retention. Server-wide defaults +for new topics live under `[system.topic]`; both policies can be active +simultaneously and can be overridden per-topic via `CreateTopic`/`UpdateTopic`. + +> **Important**: retention is enforced by the background **message cleaner**, +> which is **disabled by default** in 0.8.0. Enable it or expired data will +> never be deleted: +> +> ```toml +> [data_maintenance.messages] +> cleaner_enabled = true # default: false +> interval = "1 m" # how often the cleaner runs (default: "1 m") +> ``` ### Time-Based Retention (Message Expiry) Messages are deleted after a specified duration: ```toml -[system.segment] +[system.topic] # Human-readable format message_expiry = "7 days" # 7 days message_expiry = "2 days 4 hours" # 2 days + 4 hours message_expiry = "none" # Never expire (default) ``` -Expiry is evaluated per-segment. The entire segment is removed when all messages within it have expired. +Expiry is evaluated per-segment: a sealed segment is removed once all messages +within it have expired. The active (open) segment is always protected. ### Size-Based Retention (Max Topic Size) Limit the total size of a topic: ```toml -[topic] -# Maximum topic size across all partitions -max_topic_size = "10 GB" - -# Delete oldest segments when limit reached -delete_oldest_segments = true +[system.topic] +# Maximum topic size; "unlimited" or "0" = no size limit (default: "unlimited") +max_size = "10 GiB" ``` -When the limit is reached, the oldest segments are deleted in whole-segment increments to make room for new messages. +When a topic reaches **90% of `max_size`**, the oldest sealed segments are +deleted in whole-segment increments to make room for new messages. There is no +separate `delete_oldest_segments` switch in 0.8.0 — this behavior is automatic +whenever `max_size` is set and the message cleaner is enabled. ### Retention Strategy by Use Case @@ -262,31 +295,32 @@ When the limit is reached, the oldest segments are deleted in whole-segment incr ### Configuring Per-Topic Retention -When creating topics via the SDK: +When creating topics via the SDK (signature as of iggy 0.10.0): ```rust use iggy::prelude::*; +use std::str::FromStr; -// 7-day retention +// 7-day retention with a 10 GiB size cap client.create_topic( - "mystream", - "events", - 3, // partitions - None, - None, - Some(IggyExpiry::ExpireDuration(IggyDuration::from_str("7days")?)), - Some(IggyByteSize::from_str("10GB")?), // max size + &"mystream".try_into()?, // stream_id: &Identifier + "events", // topic name + 3, // partitions_count + CompressionAlgorithm::default(), // compression ("none") + None, // replication_factor + IggyExpiry::ExpireDuration(IggyDuration::from_str("7days")?), + MaxTopicSize::Custom(IggyByteSize::from_str("10GiB")?), ).await?; // Never expire (event sourcing) client.create_topic( - "mystream", + &"mystream".try_into()?, "event-store", 10, + CompressionAlgorithm::default(), None, - None, - Some(IggyExpiry::NeverExpire), - None, // no size limit + IggyExpiry::NeverExpire, + MaxTopicSize::Unlimited, // no size limit ).await?; ``` @@ -294,91 +328,51 @@ client.create_topic( ## Backup and Archiving -Iggy supports archiving expired segments before deletion, either to local disk or S3-compatible cloud storage. - -### Disk Archiving +> **Removed in 0.8.0**: the built-in segment archiver +> (`[data_maintenance.archiver]` with `disk` and `s3` backends) existed in the +> legacy 0.4.x server line but was **removed** in the io_uring server rewrite +> (0.7.x/0.8.x). Server 0.8.0 ships **no built-in archiving to disk or S3**. +> A `[system.segment] archive_expired` flag (default `false`) is still +> accepted by the config parser, but no archiver backend exists in 0.8.0, so +> the flag has no effect — expired segments are simply deleted by the message +> cleaner. -Archive segments to a local directory: +Until archiving returns upstream, handle backups at the infrastructure level. -```toml -[data_maintenance.archiver] -enabled = true -kind = "disk" +### Filesystem-Level Backup -# Archive expired segments before deletion -archive_expired = true +The entire server state lives under `system.path` (default `local_data`), so +any file-level backup tool works. For a consistent snapshot, either stop the +server or use filesystem/volume snapshots (LVM, ZFS, EBS): -# Local path for archived data -path = "/var/lib/iggy/archive" +```bash +# Simple sync of the data directory to S3 (run against a snapshot, +# or accept that the active segment may be mid-write) +aws s3 sync /var/lib/iggy/data s3://iggy-backups/production/iggy/ ``` -### S3-Compatible Cloud Storage +### Backup Directory -Archive to AWS S3, MinIO, or any S3-compatible storage: +Iggy reserves a backup location inside the data directory, used by the server +for compatibility conversions (not a general-purpose archiver): ```toml -[data_maintenance.archiver] -enabled = true -kind = "s3" - -# Archive expired segments before deletion -archive_expired = true +[system.backup] +# Path for storing backup data, relative to system.path (default: "backup") +path = "backup" -# S3 configuration -[data_maintenance.archiver.s3] -# AWS credentials (or use IAM roles/environment) -access_key_id = "your-access-key" -secret_access_key = "your-secret-key" - -# S3 bucket and optional prefix -bucket = "iggy-backups" -key_prefix = "production/iggy/" - -# Region -region = "us-east-1" - -# For non-AWS S3 (MinIO, etc.) -endpoint = "https://minio.example.com" - -# Temporary staging directory for uploads -tmp_upload_dir = "/tmp/iggy-upload" +[system.backup.compatibility] +# Subpath for converted segment data after compatibility conversion +path = "compatibility" ``` -### Archive Workflow - -``` -1. Segment expires (all messages past retention) - └── segment-0000000000.log +### Streaming Data Out (Connectors) -2. If archiver enabled: - ├── Upload to S3: s3://bucket/prefix/stream/topic/partition/segment-0000000000.log - └── Verify upload success - -3. After successful archive: - └── Delete local segment (if archive_expired = true) - -4. If archiver disabled: - └── Delete segment immediately -``` - -### MinIO Example (Self-Hosted S3) - -For on-premises deployments using MinIO: - -```toml -[data_maintenance.archiver] -enabled = true -kind = "s3" -archive_expired = true - -[data_maintenance.archiver.s3] -access_key_id = "minioadmin" -secret_access_key = "minioadmin" -bucket = "iggy-archive" -endpoint = "http://minio:9000" -region = "us-east-1" # Required even for MinIO -tmp_upload_dir = "/tmp/iggy-s3-staging" -``` +For continuous off-server copies of message data, the Iggy **connectors +runtime** (a separate component from the server) can sink topics to external +systems (e.g., PostgreSQL, Elasticsearch, Iceberg, Quickwit). This is a +replication-style pipeline rather than a segment archive, but it is the +supported way to keep an external durable copy with 0.8.0. --- @@ -386,15 +380,22 @@ tmp_upload_dir = "/tmp/iggy-s3-staging" ### Automatic State Recovery -If metadata becomes corrupted but segment files are intact, Iggy can rebuild: +The config schema retains a recovery flag: ```toml [system.recovery] -# Recreate stream/topic/partition metadata from existing data files +# Recreate streams/topics/partitions if expected data for existing state +# is missing (default: false) recreate_missing_state = true ``` -This scans the data directory and rebuilds the logical structure from physical segment files. +> **Caveat (0.8.0)**: this key is accepted by the config parser, but as of +> server 0.8.0 it is not referenced anywhere in the server code — it is a +> holdover from the legacy server line and currently has **no effect**. The +> 0.8.0 server performs its own crash-recovery on startup (offset clamping, +> checksum-validated loading when `validate_checksum` is enabled), but it does +> not rebuild missing metadata from raw segment files. Do not rely on this +> flag; rely on backups. ### Manual Recovery Steps @@ -407,14 +408,13 @@ This scans the data directory and rebuilds the logical structure from physical s find /var/lib/iggy/data -name "*.log" -size 0 ``` -4. **Restore from backup** (if using archiver): +4. **Restore from backup** (if syncing the data directory to S3): ```bash aws s3 sync s3://iggy-backups/production/iggy/ /var/lib/iggy/data/ ``` -5. **Restart with recovery enabled**: +5. **Restart and verify**: ```bash - export IGGY_SYSTEM_RECOVERY_RECREATE_MISSING_STATE=true iggy-server ``` @@ -422,9 +422,9 @@ This scans the data directory and rebuilds the logical structure from physical s | Measure | Configuration | Purpose | |---------|---------------|---------| -| Checksums | `validate_checksum = true` | Detect corruption on read | +| Checksums | `[system.partition]` `validate_checksum = true` | Detect corruption on load | | fsync | `enforce_fsync = true` | Prevent partial writes | -| S3 backup | `archiver.enabled = true` | Off-site redundancy | +| S3 backup | (infrastructure, e.g. `aws s3 sync`) | Off-site redundancy | | EBS snapshots | (infrastructure) | Point-in-time recovery | --- @@ -451,7 +451,7 @@ Performance ←───────────────────── [system.partition] enforce_fsync = false -[system.partition.message_saver] +[message_saver] enabled = true enforce_fsync = false interval = "5s" @@ -468,7 +468,7 @@ interval = "5s" enforce_fsync = true messages_required_to_save = 100 -[system.partition.message_saver] +[message_saver] enabled = true enforce_fsync = true interval = "1s" @@ -485,7 +485,7 @@ interval = "1s" enforce_fsync = true messages_required_to_save = 1 -[system.partition.message_saver] +[message_saver] enabled = true enforce_fsync = true ``` @@ -513,7 +513,7 @@ fsync performance depends heavily on storage: ### Recommended Production Configuration ```toml -# server.toml - Production Configuration +# config.toml - Production Configuration (Iggy server 0.8.0) [system] path = "/var/lib/iggy/data" @@ -521,39 +521,36 @@ path = "/var/lib/iggy/data" [system.partition] enforce_fsync = true messages_required_to_save = 100 # Balance: durability with reasonable latency +validate_checksum = true -[system.partition.message_saver] +[message_saver] enabled = true enforce_fsync = true interval = "1s" [system.segment] -size = "1 GB" -cache_indexes = true -cache_time_indexes = true -validate_checksum = true -message_expiry = "7 days" # Adjust per your needs +size = "1 GiB" +cache_indexes = "open_segment" -[system.recovery] -recreate_missing_state = true +[system.topic] +message_expiry = "7 days" # Adjust per your needs +max_size = "unlimited" # Or e.g. "50 GiB" -[data_maintenance.archiver] -enabled = true -kind = "s3" -archive_expired = true - -[data_maintenance.archiver.s3] -bucket = "your-iggy-backups" -region = "us-east-1" -key_prefix = "production/" -tmp_upload_dir = "/tmp/iggy-upload" +# Required for retention to be enforced (disabled by default!) +[data_maintenance.messages] +cleaner_enabled = true +interval = "1 m" ``` +> **Note**: the built-in S3/disk archiver from older Iggy versions does not +> exist in 0.8.0 — see [Backup and Archiving](#backup-and-archiving) for +> infrastructure-level alternatives. + ### Infrastructure Checklist - [ ] **Storage**: Use NVMe SSD or provisioned IOPS EBS - [ ] **Filesystem**: ext4 or XFS with `noatime` mount option -- [ ] **Backup**: Enable S3 archiver or EBS snapshots +- [ ] **Backup**: Sync the data directory to S3 and/or use EBS snapshots (no built-in archiver in 0.8.0) - [ ] **Monitoring**: Alert on disk space, segment count, backup failures - [ ] **Capacity**: Plan for 2x expected data volume - [ ] **Recovery testing**: Regularly test restore from backups @@ -578,12 +575,15 @@ As of Iggy server v0.8.0 (April 2026): ### Single-Node Architecture -Iggy currently runs as a **single node only**. There is no built-in replication or clustering. +Iggy 0.8.0 is effectively a **single-node** system for production purposes. +The 0.8.0 config does introduce an **experimental `[cluster]` section** +(disabled by default) with node/peer definitions, but clustered replication is +still under active development and not production-ready in this release. **Implications:** - No automatic failover - Single point of failure for availability -- Rely on S3 archiving + infrastructure (EBS snapshots) for redundancy +- Rely on filesystem backups + infrastructure (EBS snapshots) for redundancy **Roadmap:** - Clustering via Viewstamped Replication is planned for future releases @@ -591,22 +591,22 @@ Iggy currently runs as a **single node only**. There is no built-in replication ### Mitigation Strategies -Until clustering is available: +Until clustering is production-ready: -1. **S3 Archiving**: Archive all data to durable cloud storage +1. **S3 backup**: Periodically sync the data directory to durable cloud storage 2. **EBS Snapshots**: If on AWS, use automated EBS snapshots -3. **Cold standby**: Keep a replica populated from S3 archives +3. **Cold standby**: Keep a replica populated from S3 backups 4. **Infrastructure HA**: Use container orchestration for process restarts ``` -Primary ──writes──→ Iggy ──archives──→ S3 +Primary ──writes──→ Iggy ──s3 sync──→ S3 │ └── EBS Snapshot (every hour) - + On failure: 1. Launch new instance 2. Attach latest EBS snapshot (or restore from S3) -3. Start Iggy with recreate_missing_state = true +3. Start Iggy 4. Resume operations (some data loss possible) ``` @@ -616,85 +616,97 @@ On failure: ### Environment Variables -All configuration can be set via environment variables using the pattern `IGGY_
_`: +All configuration can be set via environment variables using the pattern +`IGGY_` — the TOML path uppercased with `_` separators (e.g. +`system.partition.enforce_fsync` → `IGGY_SYSTEM_PARTITION_ENFORCE_FSYNC`). +The 0.8.0 server generates these mappings at compile time, so only real config +keys are accepted. A custom config file path can be given via +`IGGY_CONFIG_PATH` (otherwise the embedded default `core/server/config.toml` +is used). | Variable | Default | Description | |----------|---------|-------------| | `IGGY_SYSTEM_PATH` | `local_data` | Data directory | -| `IGGY_SYSTEM_PARTITION_ENFORCE_FSYNC` | `false` | Enable fsync on writes | -| `IGGY_SYSTEM_PARTITION_MESSAGES_REQUIRED_TO_SAVE` | `10000` | Messages before flush | -| `IGGY_SYSTEM_SEGMENT_SIZE` | `1073741824` | Segment size in bytes (1GB) | -| `IGGY_SYSTEM_SEGMENT_CACHE_INDEXES` | `true` | Cache offset indexes | -| `IGGY_SYSTEM_SEGMENT_VALIDATE_CHECKSUM` | `false` | Validate on load | -| `IGGY_SYSTEM_SEGMENT_MESSAGE_EXPIRY` | `none` | Default message TTL | -| `IGGY_SYSTEM_RECOVERY_RECREATE_MISSING_STATE` | `false` | Auto-rebuild metadata | -| `IGGY_DATA_MAINTENANCE_ARCHIVER_ENABLED` | `false` | Enable archiving | -| `IGGY_DATA_MAINTENANCE_ARCHIVER_KIND` | `disk` | `disk` or `s3` | - -### Complete server.toml Template +| `IGGY_SYSTEM_PARTITION_ENFORCE_FSYNC` | `false` | Enable fsync on partition writes | +| `IGGY_SYSTEM_PARTITION_MESSAGES_REQUIRED_TO_SAVE` | `1024` | Buffered messages before flush | +| `IGGY_SYSTEM_PARTITION_SIZE_OF_MESSAGES_REQUIRED_TO_SAVE` | `1 MiB` | Buffered size before flush | +| `IGGY_SYSTEM_PARTITION_VALIDATE_CHECKSUM` | `false` | Validate CRCs on load | +| `IGGY_SYSTEM_SEGMENT_SIZE` | `1 GiB` | Segment size (max 1 GiB, multiple of 512 B) | +| `IGGY_SYSTEM_SEGMENT_CACHE_INDEXES` | `open_segment` | `all`, `open_segment`, or `none` | +| `IGGY_SYSTEM_TOPIC_MESSAGE_EXPIRY` | `none` | Default message TTL for new topics | +| `IGGY_SYSTEM_TOPIC_MAX_SIZE` | `unlimited` | Default max topic size | +| `IGGY_MESSAGE_SAVER_ENABLED` | `true` | Background message saver | +| `IGGY_MESSAGE_SAVER_ENFORCE_FSYNC` | `true` | fsync on background saves | +| `IGGY_MESSAGE_SAVER_INTERVAL` | `30 s` | Saver interval | +| `IGGY_DATA_MAINTENANCE_MESSAGES_CLEANER_ENABLED` | `false` | Retention cleaner (enable for expiry/size limits!) | +| `IGGY_DATA_MAINTENANCE_MESSAGES_INTERVAL` | `1 m` | Cleaner interval | +| `IGGY_SYSTEM_STATE_ENFORCE_FSYNC` | `false` | fsync on metadata state updates | +| `IGGY_SYSTEM_RECOVERY_RECREATE_MISSING_STATE` | `false` | Accepted but has no effect in 0.8.0 | + +### Complete config.toml Template (Durability-Related Sections) ```toml -# Apache Iggy Server Configuration -# Full reference for durable storage settings +# Apache Iggy Server 0.8.0 Configuration +# Durable-storage-related sections; values shown are recommended +# production settings (shipped defaults noted in comments) [system] -# Base path for all data +# Base path for all data (default: "local_data") path = "local_data" +[system.state] +# fsync on metadata state updates (default: false) +enforce_fsync = false + [system.partition] -# Synchronous writes for durability +# Synchronous writes for durability (default: false) enforce_fsync = true -# Messages to buffer before flushing (1 = immediate, higher = batched) +# Count of buffered messages before triggering a save +# (1 = immediate, higher = batched; soft limit; default: 1024) messages_required_to_save = 100 -[system.partition.message_saver] -# Background persistence +# Size of buffered messages before triggering a save (default: "1 MiB") +size_of_messages_required_to_save = "1 MiB" + +# Validate CRC checksums when loading data (default: false) +validate_checksum = true + +[message_saver] +# Background persistence (defaults: enabled = true, enforce_fsync = true, +# interval = "30 s") enabled = true enforce_fsync = true interval = "1s" [system.segment] -# Max segment size (bytes or human-readable) -size = "1 GB" +# Soft segment size limit; max 1 GiB, multiple of 512 B (default: "1 GiB") +size = "1 GiB" -# Index caching for performance -cache_indexes = true -cache_time_indexes = true +# Index caching: "all" | "open_segment" | "none" (default: "open_segment") +cache_indexes = "open_segment" -# Data integrity -validate_checksum = true +# Parsed but non-functional in 0.8.0 (no archiver backend exists) +archive_expired = false -# Default message retention (overridable per-topic) +[system.topic] +# Default message retention for new topics (default: "none" = keep forever) message_expiry = "7 days" -[system.recovery] -# Rebuild metadata from segment files if missing -recreate_missing_state = true +# Default max topic size; oldest sealed segments deleted at 90% of this +# (default: "unlimited") +max_size = "unlimited" -[data_maintenance.archiver] -# Enable archiving of expired segments -enabled = true +# Retention enforcement - MUST be enabled for expiry/size limits to apply +[data_maintenance.messages] +# (default: false) +cleaner_enabled = true +# (default: "1 m") +interval = "1 m" -# "disk" or "s3" -kind = "s3" - -# Archive before deleting expired segments -archive_expired = true - -# Disk archiver settings (if kind = "disk") -[data_maintenance.archiver.disk] -path = "/var/lib/iggy/archive" - -# S3 archiver settings (if kind = "s3") -[data_maintenance.archiver.s3] -access_key_id = "" # Or use IAM roles -secret_access_key = "" # Or use IAM roles -bucket = "iggy-backups" -key_prefix = "production/" -region = "us-east-1" -endpoint = "" # Custom endpoint for MinIO, etc. -tmp_upload_dir = "/tmp/iggy-upload" +[system.recovery] +# Accepted by the parser but has no effect in server 0.8.0 (default: false) +recreate_missing_state = false ``` --- @@ -718,5 +730,7 @@ tmp_upload_dir = "/tmp/iggy-upload" --- -*Last updated: July 2026* -*Iggy server version: 0.8.0* +*Last updated: 2026-07-05* +*Iggy server version: 0.8.0 — every configuration key in this guide was +validated key-by-key against the upstream `server-0.8.0` tag +(`core/server/config.toml` + `core/configs/src/server_config/`).* diff --git a/docs/tech-debt/TD-2026-07-06.md b/docs/tech-debt/TD-2026-07-06.md index 5f27630..4a68735 100644 --- a/docs/tech-debt/TD-2026-07-06.md +++ b/docs/tech-debt/TD-2026-07-06.md @@ -1,7 +1,7 @@ # TD-2026-07-06: Durable-storage guide config re-validation **Source:** Review session 01, Round 1 — consistency #10 (LOW). -**Status:** open +**Status:** resolved (session 02) ## Problem @@ -14,3 +14,35 @@ server 0.6.0 and have not been re-verified key-by-key against 0.8.0. At the next server image bump past the 0.8.x line, re-validate every documented config key against the shipped server config and re-stamp. + +## Resolution (session 02) + +Validated key-by-key against upstream tag `server-0.8.0` (shipped default +config `core/server/config.toml` + config schema sources in +`core/configs/src/server_config/`, behavior cross-checked in +`core/server/src/`). Major corrections to the guide: + +- `[system.partition.message_saver]` → top-level `[message_saver]`; default + interval is 30 s (not 1 s); `messages_required_to_save` default is 1024 + (not 10000), with the `size_of_messages_required_to_save` companion added +- `cache_indexes` is a string enum (`"all"`/`"open_segment"`/`"none"`, + default `"open_segment"`); `cache_time_indexes` removed (single combined + `.index` file per segment; file naming corrected to 20-digit offsets) +- `validate_checksum` lives under `[system.partition]`; `message_expiry` + and `max_size` live under `[system.topic]` +- The entire `[data_maintenance.archiver]` disk/S3 section documented a + feature REMOVED before 0.8.0 — rewritten as a removed-feature notice with + real alternatives; `archive_expired` documented as parsed-but-inert +- Retention (expiry + size) is enforced by the `[data_maintenance.messages]` + cleaner, which defaults to DISABLED — previously undocumented, making the + guide's retention configs silently non-functional as written +- Size-based deletion is automatic at 90% of `max_size` on sealed segments + (`delete_oldest_segments` no longer exists) +- Env-var table corrected to the `ConfigEnv` mapping; SDK snippet corrected + to the real 0.10.0 `create_topic` signature + +Left as-is (explicitly unverified): Iggy's published latency benchmark +numbers, and the deeper runtime semantics of `message_saver.enforce_fsync` +(0.8.0's saver task sends `FlushUnsavedBuffer { fsync: false }` while +logging the setting — documented at the level of the upstream config +comments only). From 3ba1030600226df3076e8f02757c91c8eb41a59b Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 18:41:18 -0700 Subject: [PATCH 09/15] docs(tech-debt): session-02 registry sync and test-count update Registry README gains a Status column reflecting the six records resolved this session. TD-2026-07-02 stays open and parked: crates.io max stable iggy is still 0.10.0 (0.10.1-edge.2 is a pre-release), so the SDK-minor-bump trigger has not fired; dated check note added. CLAUDE.md synced: resilience.rs and metrics_smoke_test.rs in the directory tree, test counts updated to 172 lib / 30 integration / 1 metrics smoke / 18 model. --- CLAUDE.md | 9 +++++++-- docs/tech-debt/README.md | 18 +++++++++--------- docs/tech-debt/TD-2026-07-02.md | 8 ++++++++ 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b312371..2e0cd1b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -106,6 +106,7 @@ src/ │ ├── connection.rs # Connection state management │ ├── helpers.rs # Utility functions │ ├── params.rs # PollParams builder +│ ├── resilience.rs # Timeout/breaker/retry composition (run_resilient) │ └── scopeguard.rs # Scope guard utilities ├── validation.rs # Input validation utilities ├── middleware/ @@ -135,6 +136,7 @@ tests/ ├── integration_tests.rs # End-to-end API tests with testcontainers │ ├── Standard fixture tests (basic CRUD, messages) │ └── Security boundary tests (auth, rate limiting) +├── metrics_smoke_test.rs # Prometheus exporter smoke test (own process) └── model_tests.rs # Unit tests for models fuzz/ @@ -427,12 +429,15 @@ environment: ### Running Tests ```bash -# Unit tests (159 tests) +# Unit tests (172 tests) cargo test --lib -# Integration tests (29 tests, requires Docker for testcontainers) +# Integration tests (30 tests, requires Docker for testcontainers) cargo test --test integration_tests +# Metrics exporter smoke test (1 test) +cargo test --test metrics_smoke_test + # Model tests (18 tests) cargo test --test model_tests diff --git a/docs/tech-debt/README.md b/docs/tech-debt/README.md index b74b450..1b1ddd0 100644 --- a/docs/tech-debt/README.md +++ b/docs/tech-debt/README.md @@ -3,12 +3,12 @@ Deferred findings from code reviews, each with a binding trigger — the condition under which the record MUST be resolved (not "someday"). -| ID | Title | Source | Trigger | -|----|-------|--------|---------| -| [TD-2026-07-01](TD-2026-07-01.md) | `with_reconnect` composition test matrix | Review session 01 (tests #2) | Any behavioral change to `with_reconnect`/`retry_once` | -| [TD-2026-07-02](TD-2026-07-02.md) | DiagnosticEvents-driven connection state | Review session 01 (architect #2) | Next iggy SDK minor bump | -| [TD-2026-07-03](TD-2026-07-03.md) | Half-open probe limiting | Review session 01 (architect #9, types F5) | First production incident involving breaker recovery, or breaker config exposure | -| [TD-2026-07-04](TD-2026-07-04.md) | X-Request-Timeout enforcement | Review session 01 (silentfail M4) | Before advertising the header in any client-facing docs beyond CLAUDE.md | -| [TD-2026-07-05](TD-2026-07-05.md) | Metrics exporter smoke test | Review session 01 (tests #7) | Next metrics-exporter-prometheus major/minor bump | -| [TD-2026-07-06](TD-2026-07-06.md) | Durable-storage guide config re-validation | Review session 01 (consistency #10) | Next server image bump past 0.8.x | -| [TD-2026-07-07](TD-2026-07-07.md) | Pin third-party GitHub Actions to commit SHAs | Security review on v0.2.0 release PR | Next CI-focused change, or any new repo secret | +| ID | Title | Source | Trigger | Status | +|----|-------|--------|---------|--------| +| [TD-2026-07-01](TD-2026-07-01.md) | `with_reconnect` composition test matrix | Review session 01 (tests #2) | Any behavioral change to `with_reconnect`/`retry_once` | resolved (session 02) | +| [TD-2026-07-02](TD-2026-07-02.md) | DiagnosticEvents-driven connection state | Review session 01 (architect #2) | Next iggy SDK minor bump | open (parked; trigger re-checked 2026-07-05, not fired) | +| [TD-2026-07-03](TD-2026-07-03.md) | Half-open probe limiting | Review session 01 (architect #9, types F5) | First production incident involving breaker recovery, or breaker config exposure | resolved (session 02) | +| [TD-2026-07-04](TD-2026-07-04.md) | X-Request-Timeout enforcement | Review session 01 (silentfail M4) | Before advertising the header in any client-facing docs beyond CLAUDE.md | resolved (session 02) | +| [TD-2026-07-05](TD-2026-07-05.md) | Metrics exporter smoke test | Review session 01 (tests #7) | Next metrics-exporter-prometheus major/minor bump | resolved (session 02) | +| [TD-2026-07-06](TD-2026-07-06.md) | Durable-storage guide config re-validation | Review session 01 (consistency #10) | Next server image bump past 0.8.x | resolved (session 02) | +| [TD-2026-07-07](TD-2026-07-07.md) | Pin third-party GitHub Actions to commit SHAs | Security review on v0.2.0 release PR | Next CI-focused change, or any new repo secret | resolved (session 02) | diff --git a/docs/tech-debt/TD-2026-07-02.md b/docs/tech-debt/TD-2026-07-02.md index 417812b..d1963e8 100644 --- a/docs/tech-debt/TD-2026-07-02.md +++ b/docs/tech-debt/TD-2026-07-02.md @@ -27,3 +27,11 @@ to drive `ConnectionState`/breaker directly, and re-check whether the SDK has made internal reconnection configurable from connection strings (if so, decide once whether resilience policy lives in the SDK or the wrapper, and delete the loser). + +## Trigger check (session 02, 2026-07-05) + +crates.io shows max stable `iggy` at **0.10.0** (newest is `0.10.1-edge.2`, +a pre-release excluded by the trigger and by Dependabot's prerelease +ignore rule). The "next SDK minor bump" trigger has NOT fired; the record +stays open and parked. Re-run the check at the next Dependabot `iggy` +proposal. From e95fe1adfaa790c16ad48692169470d621afbea2 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 19:07:41 -0700 Subject: [PATCH 10/15] fix(iggy_client): remediate round-1 resilience findings H1: request-scoped deadlines no longer leak into global machinery. The wrapper holds Arc plus a separate op_deadline field; with_timeout overrides only the deadline (via the unit-tested clamp_deadline), so the spawned reconnect session and health probes always run at the global operation timeout, and scoped clones are cheap (no Config deep-copy). H2: a timeout is recorded as a circuit-breaker failure only when the deadline is the global one - a client-shortened X-Request-Timeout expiring is not outage evidence, so one client can no longer open the shared circuit for everyone (run_resilient gains a timeout_is_outage_signal parameter, pinned by matrix tests). H3: half-open probe tokens are released when an outcome is deliberately unrecorded (non-connection errors), so completed round-trips cannot starve recovery; CircuitBreaker::release_probe is capped at the granted budget and covered by unit + matrix tests. M1: the retry's breaker-gate bypass is documented in both modules and the now-reachable record_success Open arm is downgraded to debug with an accurate comment. M2: rejections carry a state label (open | half_open) on the metric, half-open rejections and token re-grants are logged, and global-deadline timeouts log at warn before the circuit can open silently. M6: matrix gaps closed (timeout-branch reconnect failure, retry failure recording, concurrent half-open probe race, re-grant boundary, sleep-based tests converted to the paused clock). L1: force_open is a no-op when already Open. L2: rejection bookkeeping extracted to one helper. L3: force_close resets probe fields. L4: the metrics smoke test bounds its HTTP client; the integration header test uses a CI-safe 1000ms deadline. --- src/iggy_client/circuit_breaker.rs | 184 ++++++++++++++++++++++----- src/iggy_client/mod.rs | 98 +++++++++++---- src/iggy_client/resilience.rs | 192 ++++++++++++++++++++++++++--- src/metrics.rs | 10 +- tests/integration_tests.rs | 7 +- tests/metrics_smoke_test.rs | 8 +- 6 files changed, 422 insertions(+), 77 deletions(-) diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 94a1f91..8a1747c 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -20,15 +20,16 @@ //! │ │ │ ▼ │ //! │ │ │ ┌───────────────┐ │ //! │ │ └─────────────────────────── │ HalfOpen │ │ -//! │ │ success │ (token-limited│ │ -//! │ │ │ probes) │ │ +//! │ │ success_threshold │ (token-limited│ │ +//! │ │ consecutive successes │ probes) │ │ //! │ │ └───────┬───────┘ │ //! │ │ │ │ //! │ │ │ failure │ //! │ │ ▼ │ //! │ │ ┌─────────┐ │ //! │ └───────────────────────────── │ Open │ ◄────────────────┘ -//! │ reset └─────────┘ │ +//! │ (after open_duration + └─────────┘ │ +//! │ successful probes) │ //! └────────────────────────────────────────────────────────────────────┘ //! ``` //! @@ -53,18 +54,24 @@ //! let cb = CircuitBreaker::new(CircuitBreakerConfig::default()); //! //! // Check if request should be allowed -//! if !cb.allow_request() { +//! if !cb.allow_request().await { //! return Err(AppError::CircuitOpen); //! } //! -//! // Execute the operation +//! // Execute the operation. Only CONNECTION-CLASS outcomes feed the +//! // breaker (see `resilience::run_resilient` for the real composition): //! match operation().await { //! Ok(result) => { -//! cb.record_success(); +//! cb.record_success().await; //! Ok(result) //! } +//! Err(e) if is_connection_error(&e) => { +//! cb.record_failure().await; +//! Err(e) +//! } +//! // Other errors record neither; release any half-open probe token. //! Err(e) => { -//! cb.record_failure(); +//! cb.release_probe().await; //! Err(e) //! } //! } @@ -224,9 +231,7 @@ impl CircuitBreaker { if let Some(opened_at) = state.opened_at && opened_at.elapsed() < self.config.open_duration { - self.requests_rejected.fetch_add(1, Ordering::Relaxed); - crate::metrics::record_circuit_breaker_rejection(); - return false; + return self.reject_request("open"); } // Timeout expired - need to transition to half-open } @@ -246,18 +251,16 @@ impl CircuitBreaker { state.state = CircuitState::HalfOpen; state.consecutive_successes = 0; self.grant_probe_tokens(&mut state); - // The transitioning caller takes the first probe token. - state.half_open_probes_remaining -= 1; - crate::metrics::set_circuit_breaker_state(1); info!( - probes = state.half_open_probes_remaining + 1, + probes = state.half_open_probes_remaining, "Circuit breaker transitioning from Open to HalfOpen" ); + // The transitioning caller takes the first probe token. + state.half_open_probes_remaining -= 1; + crate::metrics::set_circuit_breaker_state(1); return true; } - self.requests_rejected.fetch_add(1, Ordering::Relaxed); - crate::metrics::record_circuit_breaker_rejection(); - false + self.reject_request("open") } CircuitState::HalfOpen => { if state.half_open_probes_remaining == 0 { @@ -267,12 +270,15 @@ impl CircuitBreaker { .half_open_granted_at .is_none_or(|granted| granted.elapsed() >= self.config.open_duration); if !window_expired { - self.requests_rejected.fetch_add(1, Ordering::Relaxed); - crate::metrics::record_circuit_breaker_rejection(); - return false; + debug!( + "Circuit breaker rejected request: half-open probe budget exhausted" + ); + return self.reject_request("half_open"); } + // info: a full probe window elapsed without a recorded + // outcome - recovery is stalling, not progressing. + info!("Circuit breaker re-granted half-open probe tokens"); self.grant_probe_tokens(&mut state); - debug!("Circuit breaker re-granted half-open probe tokens"); } state.half_open_probes_remaining -= 1; true @@ -290,6 +296,36 @@ impl CircuitBreaker { state.half_open_granted_at = Some(Instant::now()); } + /// Record a rejection (counter + state-labeled metric) and return `false`. + /// + /// Single site for the bookkeeping so no rejection path can forget the + /// metrics half; the label lets operators distinguish "circuit is open" + /// from "half-open probe budget exhausted" — materially different + /// situations. + fn reject_request(&self, state_label: &'static str) -> bool { + self.requests_rejected.fetch_add(1, Ordering::Relaxed); + crate::metrics::record_circuit_breaker_rejection(state_label); + false + } + + /// Hand back a half-open probe token whose outcome was deliberately not + /// recorded (non-connection errors touch neither breaker counter). + /// + /// Without this, probes completing with e.g. `NotFound` — a full + /// round-trip proving transport health — would permanently consume + /// tokens and starve recovery until the re-grant window. No-op outside + /// HalfOpen; capped at the granted budget. + pub(super) async fn release_probe(&self) { + let mut state = self.state.write().await; + if state.state == CircuitState::HalfOpen { + let cap = self.config.success_threshold.max(1); + if state.half_open_probes_remaining < cap { + state.half_open_probes_remaining += 1; + debug!("Circuit breaker released a half-open probe token (outcome not recorded)"); + } + } + } + /// Record a successful operation. /// /// In HalfOpen state, consecutive successes can close the circuit. @@ -318,8 +354,15 @@ impl CircuitBreaker { } } CircuitState::Open => { - // Shouldn't happen - requests are rejected in Open state - warn!("Unexpected success recorded in Open state"); + // Reachable through legitimate interleavings: a half-open + // probe (or its post-reconnect retry, which bypasses the + // gate) can complete successfully after another probe's + // failure reopened the circuit. The success is deliberately + // discarded - recovery restarts from the next half-open + // window's probes. + debug!( + "Success recorded while Open (in-flight probe finished after reopen); discarded" + ); } } } @@ -385,14 +428,23 @@ impl CircuitBreaker { state.opened_at = None; state.consecutive_failures = 0; state.consecutive_successes = 0; + // Hygiene: half-open probe fields are re-granted on every HalfOpen + // entry, but stale values should not outlive a manual reset. + state.half_open_probes_remaining = 0; + state.half_open_granted_at = None; crate::metrics::set_circuit_breaker_state(0); info!("Circuit breaker forcibly closed"); } /// Force the circuit to open (for testing or manual intervention). + /// + /// A no-op when already Open, preserving the no-refresh policy for + /// `opened_at` (see `record_failure`) and keeping `times_opened` honest. pub async fn force_open(&self) { let mut state = self.state.write().await; - self.open_now(&mut state); + if state.state != CircuitState::Open { + self.open_now(&mut state); + } warn!("Circuit breaker forcibly opened"); } @@ -455,7 +507,7 @@ mod tests { assert_eq!(cb.requests_rejected(), 1); } - #[tokio::test] + #[tokio::test(start_paused = true)] async fn test_circuit_transitions_to_half_open() { let config = CircuitBreakerConfig::new(1, 1, Duration::from_millis(10)); let cb = CircuitBreaker::new(config); @@ -463,22 +515,22 @@ mod tests { cb.record_failure().await; assert_eq!(cb.state().await, CircuitState::Open); - // Wait for timeout - tokio::time::sleep(Duration::from_millis(20)).await; + // Advance the paused clock past the open window + tokio::time::advance(Duration::from_millis(20)).await; // Should allow request and transition to half-open assert!(cb.allow_request().await); assert_eq!(cb.state().await, CircuitState::HalfOpen); } - #[tokio::test] + #[tokio::test(start_paused = true)] async fn test_circuit_closes_after_success_in_half_open() { let config = CircuitBreakerConfig::new(1, 2, Duration::from_millis(10)); let cb = CircuitBreaker::new(config); // Open the circuit cb.record_failure().await; - tokio::time::sleep(Duration::from_millis(20)).await; + tokio::time::advance(Duration::from_millis(20)).await; // Transition to half-open assert!(cb.allow_request().await); @@ -492,14 +544,14 @@ mod tests { assert_eq!(cb.state().await, CircuitState::Closed); } - #[tokio::test] + #[tokio::test(start_paused = true)] async fn test_circuit_reopens_on_failure_in_half_open() { let config = CircuitBreakerConfig::new(1, 2, Duration::from_millis(10)); let cb = CircuitBreaker::new(config); // Open the circuit cb.record_failure().await; - tokio::time::sleep(Duration::from_millis(20)).await; + tokio::time::advance(Duration::from_millis(20)).await; // Transition to half-open assert!(cb.allow_request().await); @@ -593,8 +645,13 @@ mod tests { assert!(cb.allow_request().await); assert!(!cb.allow_request().await); + // Just below the window boundary the budget must stay exhausted - + // an unconditional re-grant would defeat the probe cap entirely. + tokio::time::advance(Duration::from_secs(29)).await; + assert!(!cb.allow_request().await); + // The re-grant window keeps the breaker from wedging permanently. - tokio::time::advance(Duration::from_secs(30)).await; + tokio::time::advance(Duration::from_secs(1)).await; assert!(cb.allow_request().await); assert_eq!(cb.state().await, CircuitState::HalfOpen); } @@ -649,4 +706,67 @@ mod tests { assert!(cb.allow_request().await); } + + #[tokio::test(start_paused = true)] + async fn test_half_open_concurrent_probes_admit_exactly_the_budget() { + // Two callers race allow_request at the Open->HalfOpen boundary with + // a single-token budget: exactly one may pass. On the deterministic + // paused single-thread runtime, join! interleaves both futures + // through the same write-lock protocol the production path uses. + let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + cb.record_failure().await; + tokio::time::advance(Duration::from_secs(30)).await; + + let (a, b) = tokio::join!(cb.allow_request(), cb.allow_request()); + assert!( + a ^ b, + "exactly one of two racing probes may pass, got ({a}, {b})" + ); + assert_eq!(cb.state().await, CircuitState::HalfOpen); + } + + #[tokio::test(start_paused = true)] + async fn test_release_probe_returns_token_capped_at_budget() { + let config = CircuitBreakerConfig::new(1, 1, Duration::from_secs(30)); + let cb = CircuitBreaker::new(config); + + // No-op while Closed. + cb.release_probe().await; + assert!(cb.allow_request().await); + + cb.record_failure().await; + tokio::time::advance(Duration::from_secs(30)).await; + + // Consume the only token, release it, and it must admit again. + assert!(cb.allow_request().await); + assert!(!cb.allow_request().await); + cb.release_probe().await; + assert!(cb.allow_request().await); + + // Releases never exceed the granted budget (single token here). + cb.release_probe().await; + cb.release_probe().await; + assert!(cb.allow_request().await); + assert!( + !cb.allow_request().await, + "budget cap must hold after over-release" + ); + } + + #[tokio::test] + async fn test_force_open_when_already_open_does_not_double_count() { + let cb = CircuitBreaker::default(); + + cb.force_open().await; + cb.force_open().await; + + assert_eq!(cb.state().await, CircuitState::Open); + assert_eq!( + cb.times_opened(), + 1, + "repeat force_open must not inflate the counter" + ); + } } diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index 547ec4e..ea4bc97 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -172,14 +172,28 @@ pub struct IggyClientWrapper { /// The underlying Iggy client (behind RwLock for reconnection) /// See struct-level docs for performance considerations. client: Arc>, - /// Application configuration - config: Config, + /// Application configuration (shared; never mutated after construction) + config: Arc, + /// Deadline applied to operations issued through THIS view. + /// + /// Equals `config.operation_timeout` on the root wrapper; request-scoped + /// views created by [`Self::with_timeout`] carry a shorter value. Kept + /// separate from `config` so a per-request deadline can never leak into + /// process-global machinery (reconnect sessions, health probes), which + /// always reads `config.operation_timeout`. + op_deadline: Duration, /// Connection state tracking state: Arc, /// Circuit breaker for fail-fast during outages circuit_breaker: Arc, } +/// Clamp a requested per-request deadline to the configured global timeout: +/// a client may SHORTEN the deadline, never extend it. +fn clamp_deadline(requested: Duration, global: Duration) -> Duration { + requested.min(global) +} + impl IggyClientWrapper { /// Create a new Iggy client wrapper from configuration. /// @@ -210,7 +224,8 @@ impl IggyClientWrapper { let wrapper = Self { client: Arc::new(RwLock::new(client)), - config, + op_deadline: config.operation_timeout, + config: Arc::new(config), state: Arc::new(ConnectionState::new()), circuit_breaker: Arc::new(CircuitBreaker::new(circuit_breaker_config)), }; @@ -376,11 +391,13 @@ impl IggyClientWrapper { match IggyClient::from_connection_string(&self.config.iggy_connection_string) { Ok(new_client) => { // Bound the connect: the SDK's internal reconnection would - // otherwise retry inside connect() indefinitely. The bound - // is HALF the operation timeout so a failed attempt exits - // through the cleanup arms below before reconnect_bounded's - // outer deadline aborts the whole session (an abort drops - // the client mid-connect with no chance to shut it down). + // otherwise retry inside connect() indefinitely. HALF the + // GLOBAL operation timeout (never a request-scoped + // deadline — see `with_timeout`) so that when a caller is + // still waiting on this session, a failed attempt exits + // through the cleanup arms below and reports before the + // caller's own deadline elapses, instead of the caller + // timing out mid-attempt and reading a stale outcome. let attempt_timeout = self.config.operation_timeout / 2; match tokio::time::timeout(attempt_timeout, new_client.connect()).await { Ok(Ok(())) => {} @@ -425,10 +442,10 @@ impl IggyClientWrapper { } } - /// Reconnect, bounded by the operation timeout from the caller's view. + /// Reconnect, bounded by this view's deadline from the caller's side. /// /// The session runs in a SPAWNED task and the caller awaits its - /// `JoinHandle` under the timeout. This is a cancellation-safety + /// `JoinHandle` under the deadline. This is a cancellation-safety /// requirement, not a style choice: dropping the reconnect future at /// its commit-point awaits (between `connect()` succeeding and the /// swap/shutdown completing) would leak a fully-connected client whose @@ -439,8 +456,13 @@ impl IggyClientWrapper { /// keeps running in the background and completes (or fails) cleanly; /// the caller just reports the timeout, and waiters on a later attempt /// join the still-running session as followers. + /// + /// Only the caller's WAIT uses the (possibly request-scoped) + /// `op_deadline`; the session itself always runs at the global + /// `config.operation_timeout` (see `reconnect`), so a short-deadline + /// request can never cripple the shared reconnect session. async fn reconnect_bounded(&self) -> AppResult<()> { - let timeout = self.config.operation_timeout; + let timeout = self.op_deadline; let this = self.clone(); let session = tokio::spawn(async move { this.reconnect().await }); @@ -463,16 +485,23 @@ impl IggyClientWrapper { /// errors. The composition itself lives in [`resilience::run_resilient`] /// (see its module docs for the full semantics, worst-case latency, and /// breaker false-positive analysis); this method binds it to the - /// wrapper's breaker, configured operation timeout, tracked connection - /// state, and bounded reconnect session. + /// wrapper's breaker, this view's deadline, tracked connection state, + /// and bounded reconnect session. + /// + /// A timeout counts as a circuit-breaker failure only when this view + /// runs at the global deadline: a client-shortened deadline expiring + /// says nothing about an outage, and letting it feed the shared breaker + /// would let one client open the circuit for everyone. async fn with_reconnect(&self, operation: F) -> AppResult where F: Fn() -> Fut, Fut: std::future::Future>, { + let timeout_is_outage_signal = self.op_deadline >= self.config.operation_timeout; resilience::run_resilient( &self.circuit_breaker, - self.config.operation_timeout, + self.op_deadline, + timeout_is_outage_signal, || self.state.is_connected(), || self.reconnect_bounded(), operation, @@ -995,16 +1024,20 @@ impl IggyClientWrapper { /// bound is clamped to the configured global timeout — a client may /// SHORTEN the deadline, never extend it. /// - /// All `Arc`'d internals (SDK client, connection state, circuit - /// breaker) are shared with the parent, so breaker bookkeeping and - /// reconnection coordination stay global; only the deadline changes. - /// A reconnect session triggered under a scoped view has its caller - /// wait bounded by the same shortened deadline (the session itself - /// continues in the background — see `reconnect_bounded`). + /// All `Arc`'d internals (SDK client, config, connection state, circuit + /// breaker) are shared with the parent; the scoped view carries only a + /// different `op_deadline`, so the clone is cheap and process-global + /// machinery is untouched: a reconnect session triggered under a scoped + /// view has its caller wait bounded by the shortened deadline, but the + /// session itself (and health probes) always run at the global + /// `config.operation_timeout`. Timeouts observed under a shortened + /// deadline are NOT recorded as circuit-breaker failures (see + /// `with_reconnect`) — a client-chosen deadline expiring is not an + /// outage signal. #[must_use] pub fn with_timeout(&self, timeout: Duration) -> Self { let mut scoped = self.clone(); - scoped.config.operation_timeout = timeout.min(self.config.operation_timeout); + scoped.op_deadline = clamp_deadline(timeout, self.config.operation_timeout); scoped } @@ -1033,6 +1066,29 @@ impl IggyClientWrapper { mod tests { use super::*; + #[test] + fn test_clamp_deadline_shortens() { + // A client may shorten the deadline below the global bound. + assert_eq!( + clamp_deadline(Duration::from_millis(100), Duration::from_secs(30)), + Duration::from_millis(100) + ); + } + + #[test] + fn test_clamp_deadline_never_extends() { + // TD-2026-07-04 security property: the header (max 5 min) can never + // extend server-side work beyond the global operation timeout. + assert_eq!( + clamp_deadline(Duration::from_secs(300), Duration::from_secs(30)), + Duration::from_secs(30) + ); + assert_eq!( + clamp_deadline(Duration::from_secs(30), Duration::from_secs(30)), + Duration::from_secs(30) + ); + } + #[test] fn test_backoff_first_attempt_is_base_delay() { // Zero jitter offset happens at jitter_unit = 0.5 diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index 57a6b0e..06cbf70 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -13,13 +13,31 @@ //! 2. **First attempt**, bounded by `timeout`. //! 3. **Classified connection error** → record breaker failure, run the //! `reconnect` step, then retry the operation exactly once. -//! 4. **Non-connection error** → returned as-is; the breaker is NOT touched -//! (bad requests must not open the circuit). -//! 5. **Timeout** → recorded as a breaker failure (the SDK's internal -//! transport reconnection swallows most outages into blocking retries, so -//! a timeout is often the only outage signal). Reconnect + single retry -//! happen only if `is_connected` reports the connection as lost — a -//! timeout alone could just be a slow operation. +//! 4. **Non-connection error** → returned as-is; the breaker records +//! neither success nor failure (bad requests must not open the circuit), +//! but any half-open probe token the request consumed is RELEASED so an +//! unrecorded outcome cannot starve recovery (see +//! `CircuitBreaker::release_probe`). +//! 5. **Timeout** → recorded as a breaker failure only when +//! `timeout_is_outage_signal` is true, i.e. the deadline is the global +//! operation timeout (the SDK's internal transport reconnection swallows +//! most outages into blocking retries, so such a timeout is often the +//! only outage signal). A client-shortened `X-Request-Timeout` deadline +//! expiring says nothing about an outage and must not feed the shared +//! breaker — otherwise one client could open the circuit for everyone. +//! Reconnect + single retry happen only if `is_connected` reports the +//! connection as lost — a timeout alone could just be a slow operation. +//! +//! # Retry and the breaker gate +//! +//! The single post-reconnect retry deliberately does NOT re-pass the +//! breaker gate: a request whose reconnect just succeeded gets its one +//! retry even if the first attempt's recorded failure opened the circuit. +//! In half-open this means one probe token can cover up to two server +//! operations (attempt + retry); the retry's outcome is still recorded, so +//! the breaker converges on real evidence. A retry success that lands after +//! another probe's failure reopened the circuit is discarded (see +//! `CircuitBreaker::record_success`). //! //! # Worst-case latency //! @@ -59,6 +77,8 @@ pub(super) fn is_connection_error(error: &AppError) -> bool { /// /// - `breaker` — gates the request and records the outcome /// - `timeout` — per-attempt bound (both the first attempt and the retry) +/// - `timeout_is_outage_signal` — whether an expired `timeout` may be +/// recorded as a breaker failure (false for client-shortened deadlines) /// - `is_connected` — consulted only on the timeout branch, to distinguish /// "slow operation" from "lost connection" /// - `reconnect` — the bounded reconnect step; invoked at most once @@ -66,6 +86,7 @@ pub(super) fn is_connection_error(error: &AppError) -> bool { pub(super) async fn run_resilient( breaker: &CircuitBreaker, timeout: Duration, + timeout_is_outage_signal: bool, is_connected: C, reconnect: R, operation: F, @@ -79,9 +100,12 @@ where { // Check circuit breaker before attempting operation if !breaker.allow_request().await { + // The state is re-read after the rejection, so under a concurrent + // transition it reports the CURRENT state, not necessarily the one + // that rejected the request. let state = breaker.state().await; return Err(AppError::CircuitOpen(format!( - "Circuit breaker is {} - service temporarily unavailable", + "Circuit breaker rejected the request (current state: {}) - service temporarily unavailable", state ))); } @@ -96,18 +120,35 @@ where breaker.record_failure().await; warn!(error = %e, "Operation failed due to connection error, attempting reconnect"); reconnect().await?; - retry_once(breaker, timeout, &operation).await + retry_once(breaker, timeout, timeout_is_outage_signal, &operation).await } Ok(Err(e)) => { - // Non-connection error - don't record as circuit breaker failure + // Non-connection error - record neither success nor failure, + // but hand back any half-open probe token this request consumed + // so an unrecorded outcome cannot starve recovery. + breaker.release_probe().await; Err(e) } Err(_) => { // Timeout on first attempt. The SDK's internal transport // reconnection swallows most mid-operation connection failures - // into blocking retries, so a timeout is often the only outage - // signal we get - record it as a circuit-breaker failure. - breaker.record_failure().await; + // into blocking retries, so a GLOBAL-deadline timeout is often + // the only outage signal we get - record it as a breaker + // failure. A client-shortened deadline expiring is not outage + // evidence; hand back any consumed probe token instead. + if timeout_is_outage_signal { + breaker.record_failure().await; + warn!( + timeout = ?timeout, + "Operation timed out at the global deadline (recorded as circuit-breaker failure)" + ); + } else { + breaker.release_probe().await; + debug!( + timeout = ?timeout, + "Operation timed out at a client-scoped deadline (not a breaker failure)" + ); + } // Only reconnect if we have evidence the connection is actually // lost (the background health check drives this flag via live @@ -118,7 +159,7 @@ where "Operation timed out and connection state is disconnected, attempting reconnect" ); reconnect().await?; - retry_once(breaker, timeout, &operation).await + retry_once(breaker, timeout, timeout_is_outage_signal, &operation).await } else { debug!( timeout = ?timeout, @@ -134,10 +175,13 @@ where } /// Single post-reconnect retry with timeout and circuit-breaker bookkeeping. -/// Shared by both reconnect paths of [`run_resilient`]. +/// Shared by both reconnect paths of [`run_resilient`]. Deliberately does +/// not re-pass the breaker gate (see module docs, "Retry and the breaker +/// gate"). async fn retry_once( breaker: &CircuitBreaker, timeout: Duration, + timeout_is_outage_signal: bool, operation: &F, ) -> AppResult where @@ -152,11 +196,18 @@ where Ok(Err(e)) => { if is_connection_error(&e) { breaker.record_failure().await; + } else { + // Unrecorded outcome: hand back any consumed probe token. + breaker.release_probe().await; } Err(e) } Err(_) => { - breaker.record_failure().await; + if timeout_is_outage_signal { + breaker.record_failure().await; + } else { + breaker.release_probe().await; + } Err(AppError::OperationTimeout(format!( "Operation timed out after {:?} on retry", timeout @@ -211,6 +262,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, fake_reconnect(&reconnects, Ok(())), move || { @@ -241,6 +293,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, fake_reconnect(&reconnects, Ok(())), || async { Ok(42) }, @@ -260,6 +313,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, // connected: slow operation, not an outage fake_reconnect(&reconnects, Ok(())), || async { std::future::pending().await }, @@ -282,6 +336,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || false, // health checks say the connection is lost fake_reconnect(&reconnects, Ok(())), move || { @@ -312,6 +367,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, fake_reconnect(&reconnects, Ok(())), move || { @@ -336,7 +392,10 @@ mod tests { #[tokio::test(start_paused = true)] async fn connection_error_on_retry_does_not_loop() { - let breaker = breaker_with(5); + // Threshold 2: the first-attempt and retry connection errors are the + // two consecutive recorded failures - the retry's record_failure is + // load-bearing (the circuit must end Open). + let breaker = breaker_with(2); let calls = Arc::new(AtomicU32::new(0)); let reconnects = Arc::new(AtomicU32::new(0)); @@ -344,6 +403,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, fake_reconnect(&reconnects, Ok(())), move || { @@ -359,6 +419,7 @@ mod tests { assert!(matches!(result, Err(AppError::ConnectionFailed(_)))); assert_eq!(calls.load(Ordering::SeqCst), 2, "single retry, no loop"); assert_eq!(reconnects.load(Ordering::SeqCst), 1, "single reconnect"); + assert_eq!(breaker.state().await, CircuitState::Open); } #[tokio::test(start_paused = true)] @@ -373,6 +434,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, fake_reconnect(&reconnects, Ok(())), move || { @@ -401,6 +463,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, fake_reconnect( &reconnects, @@ -439,6 +502,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, fake_reconnect(&reconnects, Ok(())), move || { @@ -472,6 +536,7 @@ mod tests { let result: AppResult = run_resilient( &breaker, TIMEOUT, + true, || true, fake_reconnect(&reconnects, Ok(())), move || { @@ -491,6 +556,99 @@ mod tests { assert_eq!(breaker.state().await, CircuitState::Closed); } + #[tokio::test(start_paused = true)] + async fn scoped_timeout_is_not_a_breaker_failure() { + // H2 (session-02 round 1): a client-shortened deadline expiring says + // nothing about an outage; with outage-signal false the breaker must + // stay Closed even at failure_threshold 1. + let breaker = breaker_with(1); + let reconnects = Arc::new(AtomicU32::new(0)); + + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + false, // client-scoped deadline + || true, + fake_reconnect(&reconnects, Ok(())), + || async { std::future::pending().await }, + ) + .await; + + assert!(matches!(result, Err(AppError::OperationTimeout(_)))); + assert_eq!(breaker.state().await, CircuitState::Closed); + assert_eq!(reconnects.load(Ordering::SeqCst), 0); + } + + #[tokio::test(start_paused = true)] + async fn timeout_branch_reconnect_failure_propagates() { + // The timeout-while-disconnected branch has its own `reconnect().await?`; + // its failure must propagate without a retry. + let breaker = breaker_with(5); + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + true, + || false, // disconnected + fake_reconnect( + &reconnects, + Err(AppError::ConnectionFailed("reconnect exhausted".into())), + ), + move || { + let calls = Arc::clone(&op_calls); + async move { + calls.fetch_add(1, Ordering::SeqCst); + std::future::pending().await + } + }, + ) + .await; + + assert!( + matches!(&result, Err(AppError::ConnectionFailed(msg)) if msg.contains("reconnect exhausted")) + ); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "no retry after failed reconnect" + ); + assert_eq!(reconnects.load(Ordering::SeqCst), 1); + } + + #[tokio::test(start_paused = true)] + async fn half_open_probe_token_released_on_non_connection_error() { + // H3 (session-02 round 1): a probe completing with a non-connection + // error records neither counter but must hand its token back, or + // recovery starves until the re-grant window. + let breaker = breaker_with(1); // success_threshold 1 => single token + breaker.record_failure().await; + assert_eq!(breaker.state().await, CircuitState::Open); + tokio::time::advance(Duration::from_secs(30)).await; + + let reconnects = Arc::new(AtomicU32::new(0)); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + true, + || true, + fake_reconnect(&reconnects, Ok(())), + || async { Err(AppError::NotFound("no such topic".into())) }, + ) + .await; + + assert!(matches!(result, Err(AppError::NotFound(_)))); + assert_eq!(breaker.state().await, CircuitState::HalfOpen); + // Without release_probe the single token would be gone and this + // would be rejected until the re-grant window. + assert!( + breaker.allow_request().await, + "released token must admit the next probe" + ); + } + // ========================================================================= // Error classifier // ========================================================================= diff --git a/src/metrics.rs b/src/metrics.rs index 9ec39cb..99bcd59 100644 --- a/src/metrics.rs +++ b/src/metrics.rs @@ -10,7 +10,7 @@ //! - `iggy_messages_polled_total` - Total messages polled (with labels: stream, topic) //! - `iggy_connection_reconnects_total` - Total reconnection attempts //! - `iggy_circuit_breaker_opens_total` - Times the circuit breaker opened -//! - `iggy_circuit_breaker_rejections_total` - Requests rejected by circuit breaker +//! - `iggy_circuit_breaker_rejections_total` - Requests rejected by circuit breaker (label: state = open | half_open) //! //! ## Histograms //! - `iggy_send_duration_seconds` - Message send duration @@ -147,8 +147,12 @@ pub fn record_circuit_breaker_open() { } /// Record circuit breaker rejection. -pub fn record_circuit_breaker_rejection() { - counter!(names::CIRCUIT_BREAKER_REJECTIONS_TOTAL).increment(1); +/// +/// `state` labels which breaker state rejected the request (`"open"` or +/// `"half_open"`), so operators can distinguish a hard-open circuit from an +/// exhausted half-open probe budget during recovery. +pub fn record_circuit_breaker_rejection(state: &'static str) { + counter!(names::CIRCUIT_BREAKER_REJECTIONS_TOTAL, "state" => state).increment(1); } // ============================================================================= diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index a5028a9..3af0f6e 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -1000,12 +1000,13 @@ async fn test_request_timeout_header_is_honored_end_to_end() { .expect("Send with timeout header failed"); assert_eq!(response.status(), 201); - // Valid-but-minimum header on a poll: 100ms is plenty against a local - // container, and exercises the scoped consumer path. + // Short-but-CI-safe header on a poll exercises the scoped consumer + // path (the literal 100ms minimum invites flakes on saturated hosts; + // the parse bounds are unit-tested in the middleware). let response = fixture .client .get(fixture.url("/messages?partition_id=0&count=1&offset=0")) - .header("X-Request-Timeout", "100") + .header("X-Request-Timeout", "1000") .send() .await .expect("Poll with timeout header failed"); diff --git a/tests/metrics_smoke_test.rs b/tests/metrics_smoke_test.rs index c82f7e9..5497544 100644 --- a/tests/metrics_smoke_test.rs +++ b/tests/metrics_smoke_test.rs @@ -37,10 +37,16 @@ async fn init_metrics_serves_recorded_counter_over_http() { metrics::set_connection_status(true); // Scrape /metrics, retrying briefly while the listener task comes up. + // The per-request timeout keeps an accepted-but-unanswered connection + // from hanging the test past the outer deadline. + let client = reqwest::Client::builder() + .timeout(Duration::from_secs(2)) + .build() + .expect("http client"); let url = format!("http://{addr}/metrics"); let deadline = tokio::time::Instant::now() + Duration::from_secs(10); let body = loop { - match reqwest::get(&url).await { + match client.get(&url).send().await { Ok(response) => { assert!( response.status().is_success(), From 94e13e9b008a25ff8efe9cfceb3442e0d908e53c Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 19:09:15 -0700 Subject: [PATCH 11/15] refactor(middleware): harden RequestTimeout, drop dead trait Round-1 theme M4 (flagged by 5 of 8 review agents): RequestTimeout's public fields let any code bypass from_millis's range validation - a hand-built zero-duration value inserted as an extension would make every scoped Iggy operation fail instantly. The field is now private with from_millis as the sole constructor and a duration() accessor; original_ms is dropped (documented for logging, never logged). RequestTimeoutExt is deleted: zero callers after the scoped-helper wiring, and it advertised a second, unclamped extraction path. Handlers now declare Option directly via a new OptionalFromRequestParts impl, removing the repeated timeout.map(|Extension(t)| t) unwrap at all 13 call sites. Malformed header values now log at warn (a client bug worth surfacing); remaining client-visible feedback is deferred as TD-2026-07-08 with a binding trigger. --- docs/tech-debt/TD-2026-07-08.md | 29 +++++++++++++ src/handlers/messages.rs | 25 +++++------ src/handlers/streams.rs | 27 ++++-------- src/handlers/topics.rs | 18 ++++---- src/middleware/mod.rs | 2 +- src/middleware/timeout.rs | 75 +++++++++++++++++++-------------- src/state.rs | 6 +-- 7 files changed, 105 insertions(+), 77 deletions(-) create mode 100644 docs/tech-debt/TD-2026-07-08.md diff --git a/docs/tech-debt/TD-2026-07-08.md b/docs/tech-debt/TD-2026-07-08.md new file mode 100644 index 0000000..452dc53 --- /dev/null +++ b/docs/tech-debt/TD-2026-07-08.md @@ -0,0 +1,29 @@ +# TD-2026-07-08: Client-visible feedback for X-Request-Timeout + +**Source:** Review session 02, Round 1 — silent-failure-hunter (MEDIUM, theme M3). +**Status:** open + +## Problem + +Two client-facing silences remain in the timeout-header path: + +1. A request whose valid short deadline fires receives the generic 504 + ("Operation timed out. Please try again.") — indistinguishable from + genuine server slowness. Nothing in the response says the deadline was + the client's own header. +2. The effective deadline is never echoed back, so a client cannot confirm + its header was honored (malformed headers are warn-logged server-side + as of session 02, but the client still sees nothing). + +## Mitigations in place (session 02) + +Malformed header values log at `warn!`; the middleware docs and CLAUDE.md +describe the ignore-and-fall-back behavior accurately; server-side traces +carry the timeout duration. + +## Binding trigger + +Before the header is documented in any external API reference (README API +section, OpenAPI spec), add BOTH: an `X-Effective-Timeout` response header +echoing the enforced deadline, and timeout error details that name the +client-requested deadline when one was in effect. diff --git a/src/handlers/messages.rs b/src/handlers/messages.rs index 902f915..a91ca9f 100644 --- a/src/handlers/messages.rs +++ b/src/handlers/messages.rs @@ -14,7 +14,7 @@ //! - `POLL_MAX_COUNT` - Maximum messages per poll (default: 100) use axum::Json; -use axum::extract::{Extension, Path, Query, State}; +use axum::extract::{Path, Query, State}; use axum::http::StatusCode; use serde::Deserialize; use tracing::instrument; @@ -47,14 +47,14 @@ use crate::validation::{ #[instrument(skip(state, timeout, payload))] pub async fn send_message( State(state): State, - timeout: Option>, + timeout: Option, Json(payload): Json, ) -> AppResult<(StatusCode, Json)> { // Validate event type before processing validate_event_type(&payload.event.event_type)?; let response = state - .producer_scoped(timeout.map(|Extension(t)| t)) + .producer_scoped(timeout) .send(&payload.event, payload.partition_key.as_deref()) .await?; @@ -95,7 +95,7 @@ pub struct SendBatchRequest { #[instrument(skip(state, timeout, payload), fields(batch_size = payload.events.len()))] pub async fn send_batch( State(state): State, - timeout: Option>, + timeout: Option, Json(payload): Json, ) -> AppResult<(StatusCode, Json>)> { let max_batch_size = state.config.batch_max_size; @@ -121,7 +121,7 @@ pub async fn send_batch( } let responses = state - .producer_scoped(timeout.map(|Extension(t)| t)) + .producer_scoped(timeout) .send_batch(&payload.events, payload.partition_key.as_deref()) .await?; @@ -173,7 +173,7 @@ fn default_count() -> u32 { #[instrument(skip(state, timeout))] pub async fn poll_messages( State(state): State, - timeout: Option>, + timeout: Option, Query(query): Query, ) -> AppResult> { // Validate poll parameters @@ -193,10 +193,7 @@ pub async fn poll_messages( None => params, }; - let response = state - .consumer_scoped(timeout.map(|Extension(t)| t)) - .poll(params) - .await?; + let response = state.consumer_scoped(timeout).poll(params).await?; Ok(Json(response)) } @@ -220,7 +217,7 @@ pub struct StreamTopicPath { pub async fn send_message_to( State(state): State, Path(path): Path, - timeout: Option>, + timeout: Option, Json(payload): Json, ) -> AppResult<(StatusCode, Json)> { // Validate path parameters before use @@ -230,7 +227,7 @@ pub async fn send_message_to( validate_event_type(&payload.event.event_type)?; let response = state - .producer_scoped(timeout.map(|Extension(t)| t)) + .producer_scoped(timeout) .send_to( &path.stream, &path.topic, @@ -252,7 +249,7 @@ pub async fn send_message_to( pub async fn poll_messages_from( State(state): State, Path(path): Path, - timeout: Option>, + timeout: Option, Query(query): Query, ) -> AppResult> { // Validate path parameters before use @@ -277,7 +274,7 @@ pub async fn poll_messages_from( }; let response = state - .consumer_scoped(timeout.map(|Extension(t)| t)) + .consumer_scoped(timeout) .poll_from(&path.stream, &path.topic, params) .await?; diff --git a/src/handlers/streams.rs b/src/handlers/streams.rs index 1b62426..bf50ec1 100644 --- a/src/handlers/streams.rs +++ b/src/handlers/streams.rs @@ -1,5 +1,5 @@ use axum::Json; -use axum::extract::{Extension, Path, State}; +use axum::extract::{Path, State}; use axum::http::StatusCode; use tracing::instrument; @@ -14,12 +14,9 @@ use crate::validation::validate_resource_name; #[instrument(skip(state, timeout))] pub async fn list_streams( State(state): State, - timeout: Option>, + timeout: Option, ) -> AppResult>> { - let streams = state - .iggy_scoped(timeout.map(|Extension(t)| t)) - .list_streams() - .await?; + let streams = state.iggy_scoped(timeout).list_streams().await?; let stream_infos: Vec = streams .into_iter() @@ -45,15 +42,12 @@ pub async fn list_streams( pub async fn get_stream( State(state): State, Path(name): Path, - timeout: Option>, + timeout: Option, ) -> AppResult> { // Validate path parameter before use validate_resource_name(&name, "Stream")?; - let stream = state - .iggy_scoped(timeout.map(|Extension(t)| t)) - .get_stream(&name) - .await?; + let stream = state.iggy_scoped(timeout).get_stream(&name).await?; let created_at = parse_timestamp_with_context(stream.created_at.as_micros() as i64, "stream", &stream.name); @@ -72,13 +66,13 @@ pub async fn get_stream( #[instrument(skip(state, timeout, payload))] pub async fn create_stream( State(state): State, - timeout: Option>, + timeout: Option, Json(payload): Json, ) -> AppResult { validate_resource_name(&payload.name, "Stream")?; state - .iggy_scoped(timeout.map(|Extension(t)| t)) + .iggy_scoped(timeout) .create_stream(&payload.name) .await?; @@ -90,15 +84,12 @@ pub async fn create_stream( pub async fn delete_stream( State(state): State, Path(name): Path, - timeout: Option>, + timeout: Option, ) -> AppResult { // Validate path parameter before use validate_resource_name(&name, "Stream")?; - state - .iggy_scoped(timeout.map(|Extension(t)| t)) - .delete_stream(&name) - .await?; + state.iggy_scoped(timeout).delete_stream(&name).await?; Ok(StatusCode::NO_CONTENT) } diff --git a/src/handlers/topics.rs b/src/handlers/topics.rs index a8798da..62fe14e 100644 --- a/src/handlers/topics.rs +++ b/src/handlers/topics.rs @@ -1,5 +1,5 @@ use axum::Json; -use axum::extract::{Extension, Path, State}; +use axum::extract::{Path, State}; use axum::http::StatusCode; use serde::Deserialize; use tracing::instrument; @@ -29,12 +29,12 @@ pub struct TopicPath { pub async fn list_topics( State(state): State, Path(path): Path, - timeout: Option>, + timeout: Option, ) -> AppResult>> { // Validate path parameter before use validate_resource_name(&path.stream, "Stream")?; - let client = state.iggy_scoped(timeout.map(|Extension(t)| t)); + let client = state.iggy_scoped(timeout); let topics = client.list_topics(&path.stream).await?; let stream_details = client.get_stream(&path.stream).await?; @@ -63,13 +63,13 @@ pub async fn list_topics( pub async fn get_topic( State(state): State, Path(path): Path, - timeout: Option>, + timeout: Option, ) -> AppResult> { // Validate path parameters before use validate_resource_name(&path.stream, "Stream")?; validate_resource_name(&path.topic, "Topic")?; - let client = state.iggy_scoped(timeout.map(|Extension(t)| t)); + let client = state.iggy_scoped(timeout); let topic = client.get_topic(&path.stream, &path.topic).await?; let stream_details = client.get_stream(&path.stream).await?; @@ -92,7 +92,7 @@ pub async fn get_topic( pub async fn create_topic( State(state): State, Path(path): Path, - timeout: Option>, + timeout: Option, Json(payload): Json, ) -> AppResult { // Validate path parameter before use @@ -102,7 +102,7 @@ pub async fn create_topic( validate_partition_count(payload.partitions, "Topic")?; state - .iggy_scoped(timeout.map(|Extension(t)| t)) + .iggy_scoped(timeout) .create_topic(&path.stream, &payload.name, payload.partitions) .await?; @@ -114,14 +114,14 @@ pub async fn create_topic( pub async fn delete_topic( State(state): State, Path(path): Path, - timeout: Option>, + timeout: Option, ) -> AppResult { // Validate path parameters before use validate_resource_name(&path.stream, "Stream")?; validate_resource_name(&path.topic, "Topic")?; state - .iggy_scoped(timeout.map(|Extension(t)| t)) + .iggy_scoped(timeout) .delete_topic(&path.stream, &path.topic) .await?; diff --git a/src/middleware/mod.rs b/src/middleware/mod.rs index d5a21db..421622c 100644 --- a/src/middleware/mod.rs +++ b/src/middleware/mod.rs @@ -36,5 +36,5 @@ pub use rate_limit::{RateLimitError, RateLimitLayer, TrustedProxyConfig}; pub use request_id::RequestIdLayer; pub use timeout::{ MAX_REQUEST_TIMEOUT_MS, MIN_REQUEST_TIMEOUT_MS, REQUEST_TIMEOUT_HEADER, RequestTimeout, - RequestTimeoutExt, extract_request_timeout, + extract_request_timeout, }; diff --git a/src/middleware/timeout.rs b/src/middleware/timeout.rs index bf14e6f..091eaf3 100644 --- a/src/middleware/timeout.rs +++ b/src/middleware/timeout.rs @@ -10,16 +10,17 @@ //! X-Request-Timeout: 5000 # 5 seconds in milliseconds //! ``` //! -//! Handlers extract the parsed value and scope their Iggy access to it: +//! Handlers extract the parsed value (via the `OptionalFromRequestParts` +//! impl below) and scope their Iggy access to it: //! ```rust,ignore //! async fn handler( //! State(state): State, -//! timeout: Option>, +//! timeout: Option, //! ) -> impl IntoResponse { //! // Iggy operations bounded by the request deadline (clamped to the //! // configured global OPERATION_TIMEOUT_SECS — clients may shorten, //! // never extend). -//! let client = state.iggy_scoped(timeout.map(|Extension(t)| t)); +//! let client = state.iggy_scoped(timeout); //! client.list_streams().await //! } //! ``` @@ -43,15 +44,16 @@ //! - Minimum and maximum timeout bounds are enforced to prevent abuse //! - The effective deadline is additionally clamped to the server's global //! operation timeout — the header can never EXTEND server-side work -//! - Invalid values are ignored (fall back to server default) -//! - Zero or negative values are rejected +//! - Invalid, zero, or out-of-range values are ignored: the request +//! proceeds under the global timeout (negative values never parse as +//! `u64` and are likewise ignored) use std::time::Duration; use axum::extract::Request; use axum::middleware::Next; use axum::response::Response; -use tracing::debug; +use tracing::{debug, warn}; /// Minimum allowed request timeout (100ms). /// @@ -70,13 +72,14 @@ pub const REQUEST_TIMEOUT_HEADER: &str = "x-request-timeout"; /// Extracted request timeout from client header. /// -/// This is stored in request extensions and can be extracted by handlers. +/// Stored in request extensions by the middleware; handlers receive it via +/// the `Option` extractor. The field is private so +/// [`RequestTimeout::from_millis`] is the only constructor — a value outside +/// `[MIN_REQUEST_TIMEOUT_MS, MAX_REQUEST_TIMEOUT_MS]` is unrepresentable. #[derive(Debug, Clone, Copy)] pub struct RequestTimeout { - /// The timeout duration specified by the client. - pub duration: Duration, - /// The original value from the header (for logging). - pub original_ms: u64, + /// The timeout duration specified by the client (range-validated). + duration: Duration, } impl RequestTimeout { @@ -89,9 +92,30 @@ impl RequestTimeout { } Some(Self { duration: Duration::from_millis(ms), - original_ms: ms, }) } + + /// The client-requested timeout duration. + pub fn duration(&self) -> Duration { + self.duration + } +} + +/// Lets handlers declare `timeout: Option` directly instead +/// of unwrapping `Option>` at every call site. +/// Absence simply means the client sent no (valid) `X-Request-Timeout`. +impl axum::extract::OptionalFromRequestParts for RequestTimeout +where + S: Send + Sync, +{ + type Rejection = std::convert::Infallible; + + async fn from_request_parts( + parts: &mut axum::http::request::Parts, + _state: &S, + ) -> Result, Self::Rejection> { + Ok(parts.extensions.get::().copied()) + } } /// Middleware that extracts and validates the `X-Request-Timeout` header. @@ -119,7 +143,10 @@ pub async fn extract_request_timeout(mut request: Request, next: Next) -> Respon ); } } else { - debug!( + // warn: a malformed header is a client bug - the caller believes + // a deadline is in effect that is not (TD-2026-07-08 tracks + // echoing the effective deadline back to the client). + warn!( value = value_str, "Invalid X-Request-Timeout header value, ignoring" ); @@ -129,21 +156,6 @@ pub async fn extract_request_timeout(mut request: Request, next: Next) -> Respon next.run(request).await } -/// Extension trait for extracting request timeout from request extensions. -pub trait RequestTimeoutExt { - /// Get the client-specified timeout, or fall back to the provided default. - fn effective_timeout(&self, default: Duration) -> Duration; -} - -impl RequestTimeoutExt for axum::http::Request { - fn effective_timeout(&self, default: Duration) -> Duration { - self.extensions() - .get::() - .map(|t| t.duration) - .unwrap_or(default) - } -} - #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { @@ -152,15 +164,14 @@ mod tests { #[test] fn test_request_timeout_from_millis_valid() { let timeout = RequestTimeout::from_millis(5000).unwrap(); - assert_eq!(timeout.duration, Duration::from_millis(5000)); - assert_eq!(timeout.original_ms, 5000); + assert_eq!(timeout.duration(), Duration::from_millis(5000)); } #[test] fn test_request_timeout_from_millis_minimum() { let timeout = RequestTimeout::from_millis(MIN_REQUEST_TIMEOUT_MS).unwrap(); assert_eq!( - timeout.duration, + timeout.duration(), Duration::from_millis(MIN_REQUEST_TIMEOUT_MS) ); } @@ -169,7 +180,7 @@ mod tests { fn test_request_timeout_from_millis_maximum() { let timeout = RequestTimeout::from_millis(MAX_REQUEST_TIMEOUT_MS).unwrap(); assert_eq!( - timeout.duration, + timeout.duration(), Duration::from_millis(MAX_REQUEST_TIMEOUT_MS) ); } diff --git a/src/state.rs b/src/state.rs index c4f63ce..5e9fbe9 100644 --- a/src/state.rs +++ b/src/state.rs @@ -147,7 +147,7 @@ impl AppState { /// Producer scoped to the request's effective timeout. pub fn producer_scoped(&self, timeout: Option) -> ProducerService { match timeout { - Some(t) => self.producer.with_timeout(t.duration), + Some(t) => self.producer.with_timeout(t.duration()), None => self.producer.clone(), } } @@ -155,7 +155,7 @@ impl AppState { /// Consumer scoped to the request's effective timeout. pub fn consumer_scoped(&self, timeout: Option) -> ConsumerService { match timeout { - Some(t) => self.consumer.with_timeout(t.duration), + Some(t) => self.consumer.with_timeout(t.duration()), None => self.consumer.clone(), } } @@ -163,7 +163,7 @@ impl AppState { /// Iggy client scoped to the request's effective timeout. pub fn iggy_scoped(&self, timeout: Option) -> IggyClientWrapper { match timeout { - Some(t) => self.iggy_client.with_timeout(t.duration), + Some(t) => self.iggy_client.with_timeout(t.duration()), None => self.iggy_client.clone(), } } From 1456b3df737412d50970ae4ba9e814825af8b01b Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 19:11:19 -0700 Subject: [PATCH 12/15] docs: apply round-1 documentation corrections (M5) and file TD-08/TD-09 partitioning-guide: message_expiry moves to [system.topic]; the inert archive_expired advice and nonexistent delete_oldest_segments switch are removed (0.8.0 deletes oldest sealed segments automatically at 90% of max_size, gated by the default-disabled data_maintenance.messages cleaner) - the guide no longer contradicts the re-validated durable-storage guide. docs/README: archiving blurbs updated for the removed archiver. durable-storage guide: inline caveat that the 0.8.0 saver flushes with fsync:false (previously only in the TD record), the config section formerly named Balanced is renamed High to match the durability table, benchmark figures marked as not re-validated, TOC gains the Sources section. CLAUDE.md: 3x worst-case reconnect caveat and the scoped-timeout breaker exemption. TD-04 resolution reworded to 'propagation smoke test' + clamp unit tests; TD-02's Dependabot mechanism claim corrected (semver-prerelease is not a documented ignore.update-types value). New records: TD-2026-07-08 (client-visible timeout feedback) and TD-2026-07-09 (breaker state enum restructuring), each with a binding trigger; registry table updated. --- CLAUDE.md | 5 ++++- docs/README.md | 4 ++-- docs/durable-storage-guide.md | 15 +++++++++++-- docs/partitioning-guide.md | 27 +++++++++++------------ docs/tech-debt/README.md | 2 ++ docs/tech-debt/TD-2026-07-02.md | 10 +++++---- docs/tech-debt/TD-2026-07-04.md | 12 +++++++---- docs/tech-debt/TD-2026-07-09.md | 38 +++++++++++++++++++++++++++++++++ 8 files changed, 86 insertions(+), 27 deletions(-) create mode 100644 docs/tech-debt/TD-2026-07-09.md diff --git a/CLAUDE.md b/CLAUDE.md index 2e0cd1b..1d1109a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -720,7 +720,10 @@ Request → Rate Limit → Auth → Request ID → Timeout → Tracing → CORS - Enforced end-to-end: all Iggy-touching handlers scope their client via `AppState::{producer,consumer,iggy}_scoped` → `IggyClientWrapper::with_timeout`, bounding every operation attempt by - the request deadline + the request deadline (worst case ~3x the deadline on the reconnect + path: first attempt + bounded reconnect wait + single retry) +- Timeouts under a client-shortened deadline are NOT circuit-breaker + failures (a client's short deadline expiring is not outage evidence) - The effective deadline is clamped to the global `OPERATION_TIMEOUT_SECS` — clients may shorten a request's bound, never extend it - Requests without the header use the global timeout unchanged diff --git a/docs/README.md b/docs/README.md index 633c5e7..9b8e1a2 100644 --- a/docs/README.md +++ b/docs/README.md @@ -8,7 +8,7 @@ This directory contains comprehensive guides for understanding and working with |-------|-------------| | [Event-Driven Architecture Guide](guide.md) | Core concepts, streams/topics/partitions, consumer groups, error handling patterns, and production patterns (outbox, saga, idempotency) | | [Partitioning Guide](partitioning-guide.md) | Deep dive into partition keys, ordering guarantees, and partition selection strategies | -| [Durable Storage Guide](durable-storage-guide.md) | Storage architecture, fsync configuration, backup/archiving to S3, recovery procedures, and production recommendations | +| [Durable Storage Guide](durable-storage-guide.md) | Storage architecture, fsync configuration, retention, backup strategies (the built-in archiver was removed before server 0.8.0), recovery procedures, and production recommendations | | [Structured Concurrency](structured-concurrency.md) | Task lifecycle management, cancellation tokens, and graceful shutdown patterns | ## Quick Navigation @@ -26,7 +26,7 @@ This directory contains comprehensive guides for understanding and working with **Data Durability** - [Durability Configuration](durable-storage-guide.md#durability-configuration) - fsync settings and trade-offs -- [Backup and Archiving](durable-storage-guide.md#backup-and-archiving) - S3 and disk archival +- [Backup and Archiving](durable-storage-guide.md#backup-and-archiving) - backup strategies (built-in archiver removed in 0.8.0) - [Recovery](durable-storage-guide.md#recovery-and-data-integrity) - Disaster recovery procedures **Message Ordering** diff --git a/docs/durable-storage-guide.md b/docs/durable-storage-guide.md index 87915a2..fd2fbf1 100644 --- a/docs/durable-storage-guide.md +++ b/docs/durable-storage-guide.md @@ -20,6 +20,7 @@ A comprehensive guide to configuring Apache Iggy for durable, production-ready m 8. [Production Recommendations](#production-recommendations) 9. [Current Limitations](#current-limitations) 10. [Configuration Reference](#configuration-reference) +11. [Sources and Further Reading](#sources-and-further-reading) --- @@ -121,6 +122,12 @@ enforce_fsync = true interval = "30 s" ``` +> **Caveat (verified against 0.8.0 sources):** the saver task in server +> 0.8.0 issues its periodic flush with `fsync: false` while merely logging +> the `enforce_fsync` setting, so treat saver-based durability as +> UNVERIFIED at this version — if you need a hard on-disk guarantee, use +> per-message fsync (`[system.partition] enforce_fsync = true`) below. + #### 2. Per-Message fsync (Maximum Durability) For the strongest durability guarantee, force fsync after every message: @@ -151,7 +158,7 @@ export IGGY_SYSTEM_PARTITION_MESSAGES_REQUIRED_TO_SAVE=1 |-------|---------------|---------|----------------| | **Maximum** | `[system.partition]` `enforce_fsync=true`, `messages_required_to_save=1` | ~9ms P99 | None (survives power loss) | | **High** | `[system.partition]` `enforce_fsync=true`, `messages_required_to_save=100` | ~1-2ms P99 | Up to 100 messages | -| **Balanced** | `[system.partition]` `enforce_fsync=false` + `[message_saver]` `enforce_fsync=true`, `interval="1s"` | ~0.5ms P99 | Up to 1 saver interval of data | +| **Balanced** | `[system.partition]` `enforce_fsync=false` + `[message_saver]` `enforce_fsync=true`, `interval="1s"` | ~0.5ms P99 | Up to 1 saver interval — see the saver fsync caveat above | | **Performance** | `enforce_fsync=false` everywhere | ~0.1ms P99 | Unbounded (OS-dependent) | ### Performance Benchmarks @@ -166,6 +173,10 @@ With fsync-per-message enabled, Iggy achieves impressive durability performance: Single-digit millisecond latencies at P9999 for a fully durable workload is excellent performance, competitive with or exceeding many other streaming platforms. +> These figures are Iggy's published benchmark numbers; they were NOT +> re-measured as part of this guide's 0.8.0 key-by-key validation (which +> covers configuration keys and behaviors only). + --- ## Segment Management @@ -461,7 +472,7 @@ interval = "5s" - Latency: Sub-millisecond - Risk: May lose several seconds of data on crash -#### Balanced (Most Applications) +#### High (Most Applications) ```toml [system.partition] diff --git a/docs/partitioning-guide.md b/docs/partitioning-guide.md index 7e157c8..9bcdc9f 100644 --- a/docs/partitioning-guide.md +++ b/docs/partitioning-guide.md @@ -708,16 +708,6 @@ size_of_messages_required_to_save = "1 MiB" # minimum: 512 B # Smaller = more files, faster deletion; Larger = fewer files, better sequential I/O size = "1 GiB" # maximum: 1 GiB -# Message expiration time (retention policy) -# "none" = keep forever -# Time format: "7 days", "24 hours", "30 minutes" -message_expiry = "none" - -# What happens when segments expire -# true = move to archive directory -# false = delete permanently -archive_expired = false - # Index caching strategy # "all" = cache all indexes (fastest reads, most memory) # "open_segment" = cache only active segment (balanced) @@ -738,11 +728,20 @@ server_confirmation = "wait" # "unlimited" or size like "100 GiB" max_size = "unlimited" -# Auto-delete oldest segments when max_size is reached -# Only takes effect if max_size is set -delete_oldest_segments = false +# Message expiration time (retention policy) +# "none" = keep forever +# Time format: "7 days", "24 hours", "30 minutes" +message_expiry = "none" ``` +Size-based cleanup is automatic in server 0.8.0: once a topic reaches 90% of +`max_size`, the oldest **sealed** segments are deleted (there is no separate +`delete_oldest_segments` switch). Note that BOTH expiry- and size-based +retention are enforced by the `[data_maintenance.messages]` cleaner, which +is **disabled by default** — see the +[durable-storage guide](durable-storage-guide.md#data-retention-policies) +for the full retention configuration. + ### Message Deduplication ```toml @@ -765,7 +764,7 @@ expiry = "1 m" | **Maximum throughput** | `enforce_fsync = false`, `messages_required_to_save = 5000` | | **Low memory** | `cache_indexes = "none"`, smaller `messages_required_to_save` | | **Fast reads** | `cache_indexes = "all"` | -| **Auto-cleanup** | `message_expiry = "7 days"`, `delete_oldest_segments = true` | +| **Auto-cleanup** | `[system.topic] message_expiry = "7 days"` + `[data_maintenance.messages] cleaner_enabled = true` | --- diff --git a/docs/tech-debt/README.md b/docs/tech-debt/README.md index 1b1ddd0..6cc4be3 100644 --- a/docs/tech-debt/README.md +++ b/docs/tech-debt/README.md @@ -12,3 +12,5 @@ condition under which the record MUST be resolved (not "someday"). | [TD-2026-07-05](TD-2026-07-05.md) | Metrics exporter smoke test | Review session 01 (tests #7) | Next metrics-exporter-prometheus major/minor bump | resolved (session 02) | | [TD-2026-07-06](TD-2026-07-06.md) | Durable-storage guide config re-validation | Review session 01 (consistency #10) | Next server image bump past 0.8.x | resolved (session 02) | | [TD-2026-07-07](TD-2026-07-07.md) | Pin third-party GitHub Actions to commit SHAs | Security review on v0.2.0 release PR | Next CI-focused change, or any new repo secret | resolved (session 02) | +| [TD-2026-07-08](TD-2026-07-08.md) | Client-visible feedback for X-Request-Timeout | Review session 02 (silentfail M3) | Before documenting the header in any external API reference | open | +| [TD-2026-07-09](TD-2026-07-09.md) | Breaker per-state data as enum with payloads | Review session 02 (types MEDIUM) | Next behavioral change to CircuitBreakerState/allow_request | open | diff --git a/docs/tech-debt/TD-2026-07-02.md b/docs/tech-debt/TD-2026-07-02.md index d1963e8..0524035 100644 --- a/docs/tech-debt/TD-2026-07-02.md +++ b/docs/tech-debt/TD-2026-07-02.md @@ -31,7 +31,9 @@ delete the loser). ## Trigger check (session 02, 2026-07-05) crates.io shows max stable `iggy` at **0.10.0** (newest is `0.10.1-edge.2`, -a pre-release excluded by the trigger and by Dependabot's prerelease -ignore rule). The "next SDK minor bump" trigger has NOT fired; the record -stays open and parked. Re-run the check at the next Dependabot `iggy` -proposal. +a pre-release, excluded by the trigger; Dependabot does not propose +pre-release upgrades from a stable version by default — note the +`version-update:semver-prerelease` entry in dependabot.yml is not among +GitHub's documented `ignore.update-types` values and is likely inert). +The "next SDK minor bump" trigger has NOT fired; the record stays open +and parked. Re-run the check at the next Dependabot `iggy` proposal. diff --git a/docs/tech-debt/TD-2026-07-04.md b/docs/tech-debt/TD-2026-07-04.md index d0027fc..7aba971 100644 --- a/docs/tech-debt/TD-2026-07-04.md +++ b/docs/tech-debt/TD-2026-07-04.md @@ -29,7 +29,11 @@ through `AppState::{producer_scoped, consumer_scoped, iggy_scoped}` into whose operation deadline is the header value clamped to the global `OPERATION_TIMEOUT_SECS` (clients may shorten, never extend). All Arc'd internals are shared, so breaker bookkeeping and reconnect coordination -stay global. Covered by an end-to-end integration test -(`test_request_timeout_header_is_honored_end_to_end`) plus the existing -middleware parse-bound unit tests; CLAUDE.md and the module docs now -describe the enforced behavior. +stay global. Coverage: the clamp itself is unit-tested directly +(`clamp_deadline`, both directions), deadline ENFORCEMENT is pinned at the +`run_resilient` level by the paused-clock matrix, and +`test_request_timeout_header_is_honored_end_to_end` is a propagation SMOKE +test (it exercises the wired path but would not fail if the header were +silently dropped — noted by the session-02 Round 1 review); CLAUDE.md and +the module docs describe the enforced behavior including the ~3x +worst-case reconnect path. diff --git a/docs/tech-debt/TD-2026-07-09.md b/docs/tech-debt/TD-2026-07-09.md new file mode 100644 index 0000000..d25a34b --- /dev/null +++ b/docs/tech-debt/TD-2026-07-09.md @@ -0,0 +1,38 @@ +# TD-2026-07-09: Circuit-breaker per-state data as an enum with payloads + +**Source:** Review session 02, Round 1 — type-design-analyzer (MEDIUM). +**Status:** open + +## Problem + +`CircuitBreakerState` holds its invariants by convention across multiple +mutation sites instead of in the type: `opened_at` is meaningful only in +Open, `half_open_probes_remaining`/`half_open_granted_at` only in HalfOpen, +`consecutive_failures` only in Closed. The flat struct makes stale-field +states representable (session 02 added `force_close` resets and an +`is_none_or` guard as mitigations), and every new transition must remember +the convention. + +## Ideal shape (recorded by the review) + +Move per-state data into the internal state enum — `Open { opened_at }`, +`HalfOpen { probes_remaining, granted_at }`, `Closed { consecutive_failures }` +— keeping the public data-less `CircuitState` as a projection for +`state()`. That deletes both `Option`s, the `is_none_or` guard, and the +reset conventions in one move; all mutation already happens under the +write lock, so the churn is local to `circuit_breaker.rs`. + +## Why deferred + +Session 02 already reworked the breaker twice (token limiting + Round 1 +remediation); the current invariants are lock-serialized, comment-documented, +and pinned by 15 unit tests including concurrency and degenerate-config +cases. The enum refactor is shape-only with no behavioral delta and +deserves its own change with the full matrix re-run. + +## Binding trigger + +The next behavioral change to `CircuitBreakerState` or `allow_request` +(any new state, counter, or transition rule) MUST land as/with the +enum-with-payloads restructuring rather than adding another +convention-maintained field. From 1fc8837bcbd5bd887e62a7407b9c5405444f0c12 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 19:11:31 -0700 Subject: [PATCH 13/15] docs(code-reviews): session-02 round-1 review artifact Full 8-agent suite + independent citation verification (18/18 themes confirmed, 0 discarded). 4 HIGH / 6 MEDIUM themes, all remediated or deferred with binding triggers (TD-2026-07-08, TD-2026-07-09) in the four preceding commits. --- docs/code-reviews/session-02-round1.md | 219 +++++++++++++++++++++++++ 1 file changed, 219 insertions(+) create mode 100644 docs/code-reviews/session-02-round1.md diff --git a/docs/code-reviews/session-02-round1.md b/docs/code-reviews/session-02-round1.md new file mode 100644 index 0000000..5ed3c35 --- /dev/null +++ b/docs/code-reviews/session-02-round1.md @@ -0,0 +1,219 @@ +# Session 02 — Code Review, Round 1 + +**Target:** branch `tech-debt/session-02` vs `main` (10 commits, 27 files) — resilience +executor extraction + paused-clock matrix, half-open probe token limiting, +X-Request-Timeout end-to-end enforcement, metrics exporter smoke test, SHA-pinned +workflows, durable-storage guide re-validation, TD registry updates. + +**Provenance:** config: full (Fable 5, high effort, feature-dev + pr-review-toolkit +installed; all 8 agents + verifier pinned to the strongest model; attested by Maxim +via the step-0 gate). Agent fallbacks: none — all eight roster agents ran as their +native types. Verification tally: 18 themes verified, **18 kept / 0 discarded** +(one sub-point of theme 10 trimmed as overstated: `messages_required_to_save` +placement is consistent between the two guides). + +Reviewers cited in brackets: [consistency] general-purpose, [architect] +feature-dev:code-architect, [reviewer] feature-dev:code-reviewer, [types] +type-design-analyzer, [silent] silent-failure-hunter, [comments] comment-analyzer, +[tests] pr-test-analyzer, [simplifier] code-simplifier. + +--- + +## HIGH + +### H1. Request-scoped timeout leaks into the process-global reconnect session +[architect, types, silent, reviewer, comments — 5/8 agents] +`src/iggy_client/mod.rs:1005-1008` (`with_timeout`), `:444-445` (`reconnect_bounded` +spawns from the scoped clone), `:384` (`attempt_timeout = operation_timeout / 2`). + +`with_timeout` expresses the request deadline by mutating `config.operation_timeout` +on a clone, but that field is overloaded: it is also the reconnect session's +per-attempt connect bound. A request scoped to the 100ms header minimum that trips +the reconnect path spawns the **singleton** reconnect session (gated by the +Arc-shared `ConnectionState::start_reconnecting`) with a 50ms connect bound and, at +the default infinite retry setting, that session can loop forever unable to complete +TCP+auth on anything but localhost — while every other caller joins it as a +follower. One short-deadline client during an outage quietly cripples app-level +reconnection process-wide. `health_check` on a scoped clone has the same latent +issue. The `with_timeout` doc claim "only the deadline changes" is falsified. + +**Remediation:** separate the per-request deadline from wrapper configuration — +`config: Arc` plus a distinct `op_deadline: Duration` field read only by +`with_reconnect`/the caller's `reconnect_bounded` wait; `reconnect()` and +`health_check` always read the global `config.operation_timeout`. Also eliminates +the per-request deep `Config` clone (strings incl. credentials) flagged separately +[types LOW, architect MEDIUM]. + +### H2. Client-shortened timeouts feed the shared circuit breaker — one-client DoS +[silent] +`src/iggy_client/resilience.rs:105-110` with `mod.rs` (Arc-shared breaker). + +Every timeout records a breaker failure, and the timeout is now client-controlled +via `X-Request-Timeout` while the breaker is shared across scoped views. +`failure_threshold` (default 5) consecutive requests with a 100ms header against an +operation that legitimately takes longer open the **global** circuit: all clients +get 503 for `open_duration`, repeatable indefinitely. The timeout-as-outage-signal +rationale only holds for the *global* deadline. + +**Remediation:** pass deadline provenance into `run_resilient`; record a timeout as +a breaker failure only when the deadline was NOT client-shortened. Covered by new +matrix tests. + +### H3. Half-open probe tokens leak on non-connection outcomes — starvation against a healthy server +[silent HIGH, architect LOW] +`src/iggy_client/resilience.rs:101-104`, `:152-156`; `circuit_breaker.rs:262-278`. + +An admitted half-open probe whose operation completes with a non-connection error +(BadRequest, NotFound, …) records neither success nor failure — the token is +consumed and never returned, although a completed round-trip is transport-health +evidence. If the first `success_threshold` callers after recovery hit such errors, +all traffic is rejected 503 for a full `open_duration`, and the cycle can repeat +indefinitely. During that window there is no log above debug and rejections share +one unlabeled counter with open-state rejections (H5). + +**Remediation:** release the probe token when the outcome is deliberately +unrecorded (`CircuitBreaker::release_probe()` called from both non-connection +arms); do NOT record success (local errors like validation failures never touched +the server, so treating them as transport success would falsely close the circuit). + +### H4. The clamp and the scoped wiring are untested — the headline TD-04 property has no regression net +[tests HIGH, consistency MEDIUM] +`src/iggy_client/mod.rs:1005-1008`; `tests/integration_tests.rs:989-1034`. + +All four integration-test assertions pass identically if the header is silently +dropped; nothing anywhere tests `timeout.min(global)` — the security property that +clients can shorten but never extend (300s header max vs 30s global = 10x extension +if the `.min` regresses). Enforcement of the timeout *parameter* is well covered at +the `run_resilient` level; the untested surface is the clamp + wiring. + +**Remediation:** extract the clamp into a free function with direct unit tests +(both directions); soften TD-04's "covered by an end-to-end test" wording to +"propagation smoke test". (Falls out naturally from the H1 refactor.) + +--- + +## MEDIUM + +### M1. Post-reconnect retry bypasses the breaker gate and token accounting +[architect] +`src/iggy_client/resilience.rs:138-166`; `circuit_breaker.rs:320-323`. + +`retry_once` never calls `allow_request`: a half-open probe's retry is a second +server operation on one token (2x the documented probe budget), and when the first +probe's failure has reopened the circuit, the retry still executes while Open — its +success lands in `record_success`'s Open arm, proving the "Shouldn't happen" comment +and `warn!` wrong (also reachable via the legitimate two-probe race [silent, reviewer]). +**Disposition:** documented as intentional (single bounded retry after a successful +reconnect is the feature; re-gating would fail requests whose reconnect just +succeeded). The Open-arm log is downgraded and reworded; both module docs note the +bypass explicitly. + +### M2. Observability gaps in the new breaker/timeout paths +[silent, tests] +- Half-open rejections indistinguishable from open-state rejections in metrics + (`metrics.rs:150-152` single unlabeled counter) and emit no log line. +- Token re-grant logs only `debug!` although it means a probe window produced no + recorded outcome. +- The timeout-while-connected branch records a breaker failure at `debug!` only — + the circuit can march to open with zero operator-visible output at + `RUST_LOG=info`. +- The `CircuitOpen` error message re-reads state after rejection and can report a + stale value ("Circuit breaker is closed - temporarily unavailable"). + +**Remediation:** state-labeled rejection metric + rejection/re-grant logging; +timeout branch logs `warn!` when the deadline is the outage-relevant global one; +error message reworded to "current state". + +### M3. Client-facing silences in the timeout header path +[silent] +`src/middleware/timeout.rs:106-127`; `src/error.rs`. +Malformed headers are dropped at `debug!` (client believes a deadline is in effect); +a fired client deadline returns the generic 504 indistinguishable from server +slowness. **Disposition:** partially remediated (warn-level log for malformed +headers); response-header echo / error-detail enrichment deferred — recorded as +TD-2026-07-08 with a binding trigger. + +### M4. `RequestTimeout` invariants unenforced + dead `RequestTimeoutExt` + 14x extension boilerplate +[types, simplifier, architect, comments, consistency — 5/8 agents] +`src/middleware/timeout.rs:74-95`, `:132-145`; 14 handler sites. +Public fields bypass `from_millis` validation (a zero-duration extension would +poison every op); `original_ms` is never logged; `RequestTimeoutExt` has zero +callers and advertises an unclamped second extraction path; every handler repeats +`timeout.map(|Extension(t)| t)`. +**Remediation:** private fields + `duration()` accessor, drop `original_ms`, delete +the trait + re-export, implement axum 0.8 `OptionalFromRequestParts` so handlers +take `Option` directly. + +### M5. Documentation drift and durability overclaims +[consistency, comments] +- `docs/partitioning-guide.md:696-768` still teaches the pre-0.8.0 schema + (`message_expiry`/`archive_expired` under `[system.segment]`, + `delete_oldest_segments`) now contradicting the re-validated storage guide. +- `docs/README.md:11,29` still advertises built-in S3/disk archiving. +- `docs/durable-storage-guide.md` states `enforce_fsync = true` ⇒ "data guaranteed + on disk" while TD-2026-07-06 itself records the 0.8.0 saver flushes with + `fsync: false`; two different configs both named "Balanced"; benchmark table + carries no not-re-validated note; TOC omits the Sources section. +- `CLAUDE.md` timeout section omits the up-to-3x worst-case caveat. +- `src/iggy_client/mod.rs:378-383` reconnect comment contradicts + `reconnect_bounded`'s no-abort design. +- `circuit_breaker.rs:13-32,52-71` module example misses `.await`s and records + every error as failure; diagram arrows mislabeled. +- `timeout.rs:46-47` "zero or negative values are rejected" — they are ignored. + +**Remediation:** all corrected in Round 1 remediation. + +### M6. Test-matrix gaps +[tests] +Timeout-branch reconnect failure untested; `retry_once` connection-error breaker +recording unasserted (deleting the `record_failure` passes the suite); no +concurrency test races two `allow_request` callers in HalfOpen; re-grant boundary +untested just-below the window. **Remediation:** all four added; legacy sleep-based +breaker tests converted to paused clock while the file is hot [tests LOW]. + +--- + +## LOW + +- **L1** `force_open` while already Open refreshes `opened_at`/inflates + `times_opened`, contradicting the no-refresh policy [architect] — guarded. +- **L2** Rejection bookkeeping triplicated in `allow_request`; HalfOpen transition + log computes `probes = remaining + 1` after decrementing [simplifier] — helper + extracted; log moved before decrement. +- **L3** `force_close` leaves stale half-open token fields (benign; every HalfOpen + entry re-grants) [tests, types] — reset added for hygiene. +- **L4** Metrics smoke test: `reqwest::get` unbounded per-request; integration test + uses the literal 100ms minimum as a live bound (CI flake risk) [tests, silent] — + client timeout added; header raised to 1000ms. +- **L5** TD-2026-07-02 note cites `version-update:semver-prerelease`, which is not + a documented Dependabot `ignore.update-types` value [consistency, verified + against GitHub docs] — note reworded (pre-existing `dependabot.yml` entry left; + it is inert and Dependabot does not propose prereleases from stable regardless). +- **L6** `CircuitBreakerConfig` accepts `success_threshold = 0` and the `max(1)` + patch lives at the grant site rather than the constructor [types] — accepted + as-is: fields are `pub` so constructor validation is bypassable anyway; the grant + site is the single consumer needing the floor, and the degenerate case is pinned + by a regression test. +- **L7** Resilience test-matrix op-counter scaffolding repeats 8x [simplifier] — + accepted: agent's own verdict is that standalone-readable tests are a feature; + no clever helper forced. + +## Not defects (verified, no action) + +- `release.yml` retains the crates.io publish job against `publish = false` + [reviewer, consistency flagged] — explicit session-01 decision (commit `2b58c23` + "keep the crates.io publish job; publish=false is the gate"); prior decision, not + a defect. +- Probe-token arithmetic underflow, `allow_request` read/write-path race handling, + clamp direction, error-taxonomy preservation, workflow SHA pins (all six + spot-verified upstream), test-count claims, TD-record accuracy — verified clean + by multiple agents. + +## Ratings (type-design lens) + +`run_resilient` signature 9/9/9/9 (encapsulation/invariants/usefulness/enforcement) +— "at-most-once reconnect and no-retry-loop are compile-time facts". Scoped +`with_timeout` views 4/3/8/4 pre-remediation (the H1 refactor is the fix). +`CircuitBreakerState` 8/4/8/6 — enum-with-payload restructuring noted as the ideal +shape; deferred (see TD-2026-07-09) since all mutation is already lock-serialized +and Round 1 adds probe-release + reset hygiene instead. From c4485c7e9fd8c752ec205c52d0f48730d1b6fd38 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 19:22:44 -0700 Subject: [PATCH 14/15] fix(iggy_client): apply round-2 findings with_timeout now clamps against the current view's deadline, making re-scoping shrink-only (round 2 flagged that a scoped view could widen back to the global; latent, no chaining call sites existed). A cfg(test)-only unconnected constructor pins the with_timeout WIRING directly - round 2 mutation-proved that deleting the clamp passed the entire suite when only the free function was tested. New matrix case: a scoped deadline on the disconnected path still reconnects while feeding the breaker nothing. retry_once's breaker recordings now warn like the first-attempt path (the observability asymmetry round 2 caught); out-of-range header values warn like malformed ones; the extractor gets a direct unit test and an accurate absence doc. Breaker: probe_budget() single definition shared by grant and release; force_open logs only when it acts; the module doc notes the retry-gate bypass; stale diagram label removed. Resilience module doc reconciles the false-positives section with the H2 exemption and documents the sole-feeder trade-off (stats refresher, minutes-scale convergence under all-scoped hangs). Docs: durable guide's inline fsync comment no longer contradicts its own caveat; TD-2026-07-09 gains consecutive_successes in the sketch plus the three probe-accounting loosenesses (incl. the cancellation-leak, deferred to the ProbePermit shape); TD-2026-07-08 wording matches the widened warn coverage; round-1 artifact accuracy nits fixed; CLAUDE.md count synced to 183. --- CLAUDE.md | 2 +- docs/code-reviews/session-02-round1.md | 6 +-- docs/durable-storage-guide.md | 6 +-- docs/tech-debt/TD-2026-07-08.md | 2 +- docs/tech-debt/TD-2026-07-09.md | 26 +++++++--- src/iggy_client/circuit_breaker.rs | 19 +++++-- src/iggy_client/mod.rs | 70 +++++++++++++++++++++---- src/iggy_client/resilience.rs | 72 ++++++++++++++++++++++++-- src/middleware/timeout.rs | 32 +++++++++++- 9 files changed, 199 insertions(+), 36 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1d1109a..ac671b7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -429,7 +429,7 @@ environment: ### Running Tests ```bash -# Unit tests (172 tests) +# Unit tests (183 tests) cargo test --lib # Integration tests (30 tests, requires Docker for testcontainers) diff --git a/docs/code-reviews/session-02-round1.md b/docs/code-reviews/session-02-round1.md index 5ed3c35..16d0eb4 100644 --- a/docs/code-reviews/session-02-round1.md +++ b/docs/code-reviews/session-02-round1.md @@ -1,6 +1,6 @@ # Session 02 — Code Review, Round 1 -**Target:** branch `tech-debt/session-02` vs `main` (10 commits, 27 files) — resilience +**Target:** branch `tech-debt/session-02` vs `main` (9 commits, 27 files) — resilience executor extraction + paused-clock matrix, half-open probe token limiting, X-Request-Timeout end-to-end enforcement, metrics exporter smoke test, SHA-pinned workflows, durable-storage guide re-validation, TD registry updates. @@ -8,7 +8,7 @@ workflows, durable-storage guide re-validation, TD registry updates. **Provenance:** config: full (Fable 5, high effort, feature-dev + pr-review-toolkit installed; all 8 agents + verifier pinned to the strongest model; attested by Maxim via the step-0 gate). Agent fallbacks: none — all eight roster agents ran as their -native types. Verification tally: 18 themes verified, **18 kept / 0 discarded** +native types. Verification tally: 18 claims verified, **18 kept / 0 discarded** (one sub-point of theme 10 trimmed as overstated: `messages_required_to_save` placement is consistent between the two guides). @@ -69,7 +69,7 @@ consumed and never returned, although a completed round-trip is transport-health evidence. If the first `success_threshold` callers after recovery hit such errors, all traffic is rejected 503 for a full `open_duration`, and the cycle can repeat indefinitely. During that window there is no log above debug and rejections share -one unlabeled counter with open-state rejections (H5). +one unlabeled counter with open-state rejections (see M2). **Remediation:** release the probe token when the outcome is deliberately unrecorded (`CircuitBreaker::release_probe()` called from both non-connection diff --git a/docs/durable-storage-guide.md b/docs/durable-storage-guide.md index fd2fbf1..7f98d17 100644 --- a/docs/durable-storage-guide.md +++ b/docs/durable-storage-guide.md @@ -113,9 +113,9 @@ The top-level `[message_saver]` section controls background persistence # Enable the background process that saves buffered data to disk enabled = true -# Force synchronous writes to disk -# true = durable but slower (data guaranteed on disk) -# false = faster but data may be lost on crash +# Force synchronous writes to disk on each save +# (see the caveat below: the 0.8.0 saver's actual flush behavior does not +# honor this setting - treat it as intent, not a guarantee) enforce_fsync = true # Interval for running the message saver (default: 30 s) diff --git a/docs/tech-debt/TD-2026-07-08.md b/docs/tech-debt/TD-2026-07-08.md index 452dc53..a62b1a4 100644 --- a/docs/tech-debt/TD-2026-07-08.md +++ b/docs/tech-debt/TD-2026-07-08.md @@ -17,7 +17,7 @@ Two client-facing silences remain in the timeout-header path: ## Mitigations in place (session 02) -Malformed header values log at `warn!`; the middleware docs and CLAUDE.md +Malformed AND out-of-range header values log at `warn!`; the middleware docs and CLAUDE.md describe the ignore-and-fall-back behavior accurately; server-side traces carry the timeout duration. diff --git a/docs/tech-debt/TD-2026-07-09.md b/docs/tech-debt/TD-2026-07-09.md index d25a34b..fe17ba3 100644 --- a/docs/tech-debt/TD-2026-07-09.md +++ b/docs/tech-debt/TD-2026-07-09.md @@ -16,18 +16,30 @@ the convention. ## Ideal shape (recorded by the review) Move per-state data into the internal state enum — `Open { opened_at }`, -`HalfOpen { probes_remaining, granted_at }`, `Closed { consecutive_failures }` -— keeping the public data-less `CircuitState` as a projection for -`state()`. That deletes both `Option`s, the `is_none_or` guard, and the -reset conventions in one move; all mutation already happens under the -write lock, so the churn is local to `circuit_breaker.rs`. +`HalfOpen { probes_remaining, granted_at, consecutive_successes }`, +`Closed { consecutive_failures }` — keeping the public data-less +`CircuitState` as a projection for `state()`. That deletes both `Option`s, +the `is_none_or` guard, and the reset conventions in one move; all +mutation already happens under the write lock, so the churn is local to +`circuit_breaker.rs`. + +The same restructuring is the right home for probe-token OWNERSHIP (an +RAII `ProbePermit` guard): the round-2 review identified three bounded +accounting loosenesses the current design accepts — a request admitted +while Closed can phantom-release a token it never consumed; a straggler +release after the window expired lets the next caller re-grant +immediately (budget-per-window is a soft bound); and a request future +dropped mid-probe (client disconnect) leaks its token until the +`open_duration` re-grant self-heals it (logged at info). All are capped +by the budget and the re-grant window; a permit guard would make them +unrepresentable. ## Why deferred Session 02 already reworked the breaker twice (token limiting + Round 1 remediation); the current invariants are lock-serialized, comment-documented, -and pinned by 15 unit tests including concurrency and degenerate-config -cases. The enum refactor is shape-only with no behavioral delta and +and pinned by the breaker unit-test suite (17+ tests) including concurrency +and degenerate-config cases. The enum refactor is shape-only with no behavioral delta and deserves its own change with the full matrix re-run. ## Binding trigger diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 8a1747c..fa2cb12 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -16,7 +16,7 @@ //! │ └────┬────┘ │ Fast) │ │ //! │ │ ▲ └────┬────┘ │ //! │ │ │ │ │ -//! │ │ │ success │ timeout expires │ +//! │ │ │ │ timeout expires │ //! │ │ │ ▼ │ //! │ │ │ ┌───────────────┐ │ //! │ │ └─────────────────────────── │ HalfOpen │ │ @@ -48,6 +48,10 @@ //! guaranteeing the breaker cannot wedge if a probe's outcome is never //! recorded. See [`CircuitBreaker::allow_request`]. //! +//! One consumed token can cover up to two server operations: the +//! post-reconnect retry in `resilience::run_resilient` deliberately does +//! not re-pass this gate (see that module's "Retry and the breaker gate"). +//! //! # Usage //! //! ```rust,ignore @@ -292,10 +296,17 @@ impl CircuitBreaker { /// deadlock the breaker) — exactly enough probes to close the circuit /// if all of them succeed. fn grant_probe_tokens(&self, state: &mut CircuitBreakerState) { - state.half_open_probes_remaining = self.config.success_threshold.max(1); + state.half_open_probes_remaining = self.probe_budget(); state.half_open_granted_at = Some(Instant::now()); } + /// Half-open probe budget: `success_threshold`, floored at one so a + /// zero threshold cannot deadlock the breaker. Single definition shared + /// by the grant and release paths so the cap cannot drift. + fn probe_budget(&self) -> u32 { + self.config.success_threshold.max(1) + } + /// Record a rejection (counter + state-labeled metric) and return `false`. /// /// Single site for the bookkeeping so no rejection path can forget the @@ -318,7 +329,7 @@ impl CircuitBreaker { pub(super) async fn release_probe(&self) { let mut state = self.state.write().await; if state.state == CircuitState::HalfOpen { - let cap = self.config.success_threshold.max(1); + let cap = self.probe_budget(); if state.half_open_probes_remaining < cap { state.half_open_probes_remaining += 1; debug!("Circuit breaker released a half-open probe token (outcome not recorded)"); @@ -444,8 +455,8 @@ impl CircuitBreaker { let mut state = self.state.write().await; if state.state != CircuitState::Open { self.open_now(&mut state); + warn!("Circuit breaker forcibly opened"); } - warn!("Circuit breaker forcibly opened"); } /// Transition to Open, keeping internal counters and Prometheus metrics diff --git a/src/iggy_client/mod.rs b/src/iggy_client/mod.rs index ea4bc97..29baa17 100644 --- a/src/iggy_client/mod.rs +++ b/src/iggy_client/mod.rs @@ -177,7 +177,8 @@ pub struct IggyClientWrapper { /// Deadline applied to operations issued through THIS view. /// /// Equals `config.operation_timeout` on the root wrapper; request-scoped - /// views created by [`Self::with_timeout`] carry a shorter value. Kept + /// views created by [`Self::with_timeout`] carry a shorter (or equal, + /// when the header matches the global) value. Kept /// separate from `config` so a per-request deadline can never leak into /// process-global machinery (reconnect sessions, health probes), which /// always reads `config.operation_timeout`. @@ -393,11 +394,12 @@ impl IggyClientWrapper { // Bound the connect: the SDK's internal reconnection would // otherwise retry inside connect() indefinitely. HALF the // GLOBAL operation timeout (never a request-scoped - // deadline — see `with_timeout`) so that when a caller is - // still waiting on this session, a failed attempt exits - // through the cleanup arms below and reports before the - // caller's own deadline elapses, instead of the caller - // timing out mid-attempt and reading a stale outcome. + // deadline — see `with_timeout`) so that a caller waiting + // at the global deadline sees a failed attempt exit + // through the cleanup arms below before its own deadline + // elapses. (Callers on a SHORTER scoped deadline may + // still time out mid-attempt; they just report the + // timeout while the session continues.) let attempt_timeout = self.config.operation_timeout / 2; match tokio::time::timeout(attempt_timeout, new_client.connect()).await { Ok(Ok(())) => {} @@ -458,9 +460,11 @@ impl IggyClientWrapper { /// join the still-running session as followers. /// /// Only the caller's WAIT uses the (possibly request-scoped) - /// `op_deadline`; the session itself always runs at the global - /// `config.operation_timeout` (see `reconnect`), so a short-deadline - /// request can never cripple the shared reconnect session. + /// `op_deadline`; the session itself derives its per-attempt connect + /// bound from the global `config.operation_timeout` (see `reconnect`) + /// and, at the default infinite-retry setting, is unbounded as a whole + /// — so a short-deadline request can never cripple the shared + /// reconnect session. async fn reconnect_bounded(&self) -> AppResult<()> { let timeout = self.op_deadline; let this = self.clone(); @@ -1037,7 +1041,10 @@ impl IggyClientWrapper { #[must_use] pub fn with_timeout(&self, timeout: Duration) -> Self { let mut scoped = self.clone(); - scoped.op_deadline = clamp_deadline(timeout, self.config.operation_timeout); + // Clamp against THIS view's deadline (not just the global) so + // re-scoping is shrink-only: a view scoped to 1s can never be + // widened back toward the global by a second with_timeout call. + scoped.op_deadline = clamp_deadline(timeout, self.op_deadline); scoped } @@ -1066,6 +1073,49 @@ impl IggyClientWrapper { mod tests { use super::*; + /// Build a wrapper without connecting (from_connection_string only + /// parses). Lets the tests below pin the with_timeout WIRING — the + /// round-2 review mutation-proved that deleting the clamp passed the + /// entire suite when only the free function was tested. + #[allow(clippy::expect_used)] + fn unconnected_wrapper() -> IggyClientWrapper { + let config = Config::default(); + let client = IggyClient::from_connection_string(&config.iggy_connection_string) + .expect("default connection string parses"); + IggyClientWrapper { + client: Arc::new(RwLock::new(client)), + op_deadline: config.operation_timeout, + config: Arc::new(config), + state: Arc::new(ConnectionState::new()), + circuit_breaker: Arc::new(CircuitBreaker::default()), + } + } + + #[test] + fn test_with_timeout_wiring_clamps_and_is_shrink_only() { + let root = unconnected_wrapper(); + let global = root.config.operation_timeout; + assert_eq!(root.op_deadline, global, "root runs at the global deadline"); + + // Shorten: honored. + let scoped = root.with_timeout(Duration::from_millis(100)); + assert_eq!(scoped.op_deadline, Duration::from_millis(100)); + + // Extend: clamped to the global (TD-2026-07-04 security property, + // now pinned at the wiring level, not just the free function). + let extended = root.with_timeout(global + Duration::from_secs(270)); + assert_eq!(extended.op_deadline, global); + + // Re-scoping a scoped view is shrink-only: it can never widen back + // toward the global. + let rescoped = scoped.with_timeout(Duration::from_secs(10)); + assert_eq!(rescoped.op_deadline, Duration::from_millis(100)); + + // The outage-signal predicate derives correctly from the wiring. + assert!(root.op_deadline >= root.config.operation_timeout); + assert!(scoped.op_deadline < scoped.config.operation_timeout); + } + #[test] fn test_clamp_deadline_shortens() { // A client may shorten the deadline below the global bound. diff --git a/src/iggy_client/resilience.rs b/src/iggy_client/resilience.rs index 06cbf70..b71e4dd 100644 --- a/src/iggy_client/resilience.rs +++ b/src/iggy_client/resilience.rs @@ -45,12 +45,30 @@ //! attempt + bounded reconnect + retry) — bounded, but well above the //! typical single-operation expectation. //! -//! # Breaker false positives +//! # Breaker false positives — and who feeds the breaker //! -//! Timeouts count as breaker failures, so N consecutive merely-SLOW -//! operations (including the background stats refresher's) can open the -//! circuit without a real outage. Failures must be consecutive — any -//! success resets the count — which bounds the risk. +//! GLOBAL-deadline timeouts count as breaker failures, so N consecutive +//! merely-SLOW operations (including the background stats refresher's) can +//! open the circuit without a real outage. Failures must be consecutive — +//! any success resets the count — which bounds the risk. (Client-scoped +//! deadlines are exempt per semantics #5, which SHRINKS this +//! false-positive surface.) +//! +//! The flip side of that exemption: during a pure-hang outage where ALL +//! request traffic carries short client deadlines, the only guaranteed +//! breaker feeder is the background stats refresher, which runs on the +//! root (global-deadline) wrapper — health-check pings keep `is_connected` +//! truthful but never touch the breaker. Convergence to Open is therefore +//! minutes-scale (≈ failure_threshold × (global timeout + refresh +//! interval)) instead of seconds. Scoped CONNECTION errors still record +//! immediately. This is the accepted trade against the one-client-DoS the +//! exemption prevents. +//! +//! Note also that a single request may release its probe token twice +//! (scoped first-attempt timeout, then an unrecorded retry outcome); the +//! budget cap bounds the effect to transiently admitting one extra +//! concurrent probe — the same order of slack as the documented +//! retry-gate bypass. use std::time::Duration; @@ -196,6 +214,7 @@ where Ok(Err(e)) => { if is_connection_error(&e) { breaker.record_failure().await; + warn!(error = %e, "Retry failed with a connection error (recorded as breaker failure)"); } else { // Unrecorded outcome: hand back any consumed probe token. breaker.release_probe().await; @@ -205,6 +224,10 @@ where Err(_) => { if timeout_is_outage_signal { breaker.record_failure().await; + warn!( + timeout = ?timeout, + "Retry timed out at the global deadline (recorded as breaker failure)" + ); } else { breaker.release_probe().await; } @@ -649,6 +672,45 @@ mod tests { ); } + #[tokio::test(start_paused = true)] + async fn scoped_timeout_while_disconnected_still_reconnects_without_breaker_failures() { + // The H2 exemption suppresses breaker RECORDING for scoped + // deadlines, not RECOVERY: a scoped request finding the connection + // down must still trigger the reconnect, and neither its + // first-attempt timeout nor its retry timeout may feed the breaker. + let breaker = breaker_with(1); // any recorded failure would open it + let calls = Arc::new(AtomicU32::new(0)); + let reconnects = Arc::new(AtomicU32::new(0)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + false, // client-scoped deadline + || false, // health checks say the connection is lost + fake_reconnect(&reconnects, Ok(())), + move || { + let calls = Arc::clone(&op_calls); + async move { + calls.fetch_add(1, Ordering::SeqCst); + std::future::pending().await + } + }, + ) + .await; + + assert!( + matches!(&result, Err(AppError::OperationTimeout(msg)) if msg.contains("on retry")) + ); + assert_eq!(reconnects.load(Ordering::SeqCst), 1, "recovery still fires"); + assert_eq!(calls.load(Ordering::SeqCst), 2, "single retry"); + assert_eq!( + breaker.state().await, + CircuitState::Closed, + "scoped timeouts on both attempts must not feed the breaker" + ); + } + // ========================================================================= // Error classifier // ========================================================================= diff --git a/src/middleware/timeout.rs b/src/middleware/timeout.rs index 091eaf3..b0be944 100644 --- a/src/middleware/timeout.rs +++ b/src/middleware/timeout.rs @@ -103,7 +103,9 @@ impl RequestTimeout { /// Lets handlers declare `timeout: Option` directly instead /// of unwrapping `Option>` at every call site. -/// Absence simply means the client sent no (valid) `X-Request-Timeout`. +/// Absence means the client sent no (valid) `X-Request-Timeout` — or the +/// route is not behind the timeout middleware, which degrades safely to +/// the global deadline. impl axum::extract::OptionalFromRequestParts for RequestTimeout where S: Send + Sync, @@ -135,7 +137,9 @@ pub async fn extract_request_timeout(mut request: Request, next: Next) -> Respon ); request.extensions_mut().insert(timeout); } else { - debug!( + // Same silence class as a malformed value: the caller + // believes a deadline is in effect that is not. + warn!( timeout_ms = ms, min = MIN_REQUEST_TIMEOUT_MS, max = MAX_REQUEST_TIMEOUT_MS, @@ -202,4 +206,28 @@ mod tests { let timeout = RequestTimeout::from_millis(0); assert!(timeout.is_none()); } + + #[tokio::test] + async fn test_optional_extractor_reads_extension_presence() { + use axum::extract::OptionalFromRequestParts; + + let (mut parts, ()) = axum::http::Request::new(()).into_parts(); + + let absent = + >::from_request_parts(&mut parts, &()) + .await + .unwrap(); + assert!(absent.is_none(), "no extension => None (global timeout)"); + + let inserted = RequestTimeout::from_millis(5000).unwrap(); + parts.extensions.insert(inserted); + let present = + >::from_request_parts(&mut parts, &()) + .await + .unwrap(); + assert_eq!( + present.map(|t| t.duration()), + Some(Duration::from_millis(5000)) + ); + } } From 0139aff5e5308644205946d74e1a6576e625bb38 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 5 Jul 2026 19:23:40 -0700 Subject: [PATCH 15/15] docs(code-reviews): session-02 round-2 review artifact Full 8-agent suite on the round-1 remediation: 0 HIGH, 3 MEDIUM (all remediated or deferred with triggers), ~10 LOW fixed in a27b9df. Every round-1 theme independently verified closed, including mutation-testing of the new regression nets. --- docs/code-reviews/session-02-round2.md | 108 +++++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 docs/code-reviews/session-02-round2.md diff --git a/docs/code-reviews/session-02-round2.md b/docs/code-reviews/session-02-round2.md new file mode 100644 index 0000000..6edd939 --- /dev/null +++ b/docs/code-reviews/session-02-round2.md @@ -0,0 +1,108 @@ +# Session 02 — Code Review, Round 2 + +**Target:** the Round 1 remediation (`997c2d0..HEAD`: resilience fixes, RequestTimeout +hardening, doc corrections, TD-08/09) plus re-verification of every Round 1 theme +against `main...HEAD`. + +**Provenance:** config: full (Fable 5, high effort, feature-dev + pr-review-toolkit; +all 8 agents pinned to the strongest model; step-0 gate attested by Maxim in Round 1, +same session). Agent fallbacks: none. Verification: Round 2 findings were verified by +the reporting agents against the live tree (multiple agents re-read every cited line; +the test analyzer **mutation-tested** the new regression nets); no separate +verification pass was run because every kept finding carries in-report evidence +quotes and the two cross-agent MEDIUMs were confirmed by independent agents. + +**Headline: 0 HIGH, 3 MEDIUM, ~10 LOW. All eight agents independently verified every +Round 1 theme (H1–H4, M1–M6, L1–L5) genuinely closed** — including an exhaustive +reader audit of `op_deadline` vs `config.operation_timeout` (architect), a +five-arm outcome table proving no scoped timeout can feed the breaker (reviewer), +and mutation kills on each new H2/H3/M6 test (tests). + +--- + +## MEDIUM (all remediated in `a27b9df` unless noted) + +### R2-M1. Stale CLAUDE.md unit-test count [consistency, comments] +The remediation added lib tests after the 54c318c sync (172 → 180 at review time). +**Fixed:** synced to the final 183 after the Round 2 additions. + +### R2-M2. The clamp *wiring* was invisible to the suite — mutation-proven [tests] +Deleting the `.min` clamp from `with_timeout` passed all 180 tests; only the free +function was pinned. **Fixed:** a `#[cfg(test)]` unconnected constructor (parse-only, +no server) enables `test_with_timeout_wiring_clamps_and_is_shrink_only`, which pins +shorten/extend/re-scope behavior and the outage-signal derivation at the wiring +level. + +### R2-M3. Probe token leaks when the request future is cancelled [silent] +Hyper drops handler futures on client disconnect; a probe dropped between +`allow_request` and its outcome releases nothing. Bounded and self-healing: the +`open_duration` re-grant window recovers it and now logs at `info!`. +**Disposition:** deferred into TD-2026-07-09 (the RAII `ProbePermit` guard belongs +with the enum-with-payloads restructuring), which now records this plus the +phantom-release and stale-window loosenesses explicitly. + +## LOW (remediated in `a27b9df`) + +- `with_timeout` re-scoping could widen a scoped view back toward the global + (latent; no chaining call sites) [simplifier, types, architect — 3/8 agents] → + clamps against the current view's deadline; shrink-only pinned by the wiring test. +- `retry_once` recorded breaker failures silently — the M2 asymmetry half-fixed + [simplifier] → warn on both retry recording arms. +- Out-of-range header values logged `debug!` while malformed logged `warn!` — same + silence class [types, comments] → both warn; TD-08 wording updated. +- Probe budget expression duplicated between grant and release [types] → + `probe_budget()` single definition. +- `force_open` warned even on the no-op path [architect, comments] → logs only when + it acts. +- Resilience "Breaker false positives" doc contradicted the H2 exemption; nobody + documented that the stats refresher is the SOLE guaranteed breaker feeder under + all-scoped pure-hang outages (minutes-scale convergence — accepted trade) + [comments, silent, architect] → module-doc section rewritten; double-release slack + noted there too. +- `attempt_timeout` comment over-generalized to scoped callers; `op_deadline` / + `reconnect_bounded` doc precision; extractor absence doc; stale diagram label + [comments, silent] → all corrected. +- Scoped+disconnected path and `retry_once`'s scoped arms untested [tests F2/F3] → + new matrix case `scoped_timeout_while_disconnected_still_reconnects_without_breaker_failures`. +- `OptionalFromRequestParts` extractor unpinned [tests F4] → direct unit test + (present/absent). +- TD-09 sketch omitted `consecutive_successes`; test-count claim imprecise [types, + consistency] → completed. +- Round 1 artifact accuracy (9 commits not 10; "claims" not "themes"; phantom "H5" + ref; M1 "both module docs" made true by adding the bypass note to the + circuit_breaker module doc; durable guide's inline fsync comment contradicted its + own caveat) [comments, consistency] → all corrected. + +## Accepted without action (on record) + +- **Commit subjects vs commitlint**: five subjects exceed the configured 72-char + `header-max-length` [reviewer]. CI enforces only a `type(scope): subject` regex, + so the gate passes; subjects were shortened by history rewrite before the PR + (branch unpushed). The config/CI divergence itself is noted here — the pr.yml + check does not run commitlint against `.commitlintrc.json`. +- **Warn burst at outage onset** (~RPS x global-timeout one-time burst as in-flight + requests time out) [silent] — bounded, one-time, and each warn corresponds to a + real 504. +- **Probe cap is soft against deliberate half-open abuse** (scoped/NotFound traffic + gets tokens released) [reviewer] — the deliberate H3 anti-starvation trade; rate + limiting bounds it. +- **`timeout_is_outage_signal: bool`** kept over a provenance enum [types verdict: + "acceptable, at the threshold"] — derived once at the single production call site; + tripwire recorded: convert if `run_resilient` grows another parameter or flag. +- **Test-run anomaly** [silent]: four half-open tests failed once in a sibling + agent's first run — explained: the test-analyzer agent was mutation-testing the + shared tree concurrently (its report documents mutate-run-revert cycles); 35 + subsequent runs and the orchestrator's post-review run are green at HEAD. + +## Round 1 themes — closure verification + +| Theme | Verdict | Verified by | +|-------|---------|-------------| +| H1 op_deadline/Arc split | closed, exhaustive reader audit | architect, reviewer, types, consistency | +| H2 scoped-timeout breaker exemption | closed, five-arm outcome table | reviewer, architect, silent, tests (mutation) | +| H3 probe release | closed, all arms + ordering (release precedes reconnect `?`) | silent, architect, tests (mutation) | +| H4 clamp tests | closed at fn level; wiring gap found and closed this round | tests | +| M1–M6, L1–L5 | closed as dispositioned | all eight | + +Re-ratings (type-design lens): scoped views 4/3/8/4 → **9/8/9/8**; +`RequestTimeout` → **9/9/9/9**; `run_resilient` holds **9/9/9/9**.