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/CLAUDE.md b/CLAUDE.md index 350e65e..ac671b7 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 (183 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 @@ -711,12 +716,17 @@ 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 (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 ## Deployment Security 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/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/code-reviews/session-02-round1.md b/docs/code-reviews/session-02-round1.md new file mode 100644 index 0000000..16d0eb4 --- /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` (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. + +**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 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). + +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 (see M2). + +**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. 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**. diff --git a/docs/durable-storage-guide.md b/docs/durable-storage-guide.md index 2a8aa59..7f98d17 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 @@ -16,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) --- @@ -27,18 +32,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 +58,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,33 +105,44 @@ 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) -# 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 between save operations (when not using fsync per message) -interval = "1s" +# Interval for running the message saver (default: 30 s) +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: ```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 +156,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 — see the saver fsync caveat above | +| **Performance** | `enforce_fsync=false` everywhere | ~0.1ms P99 | Unbounded (OS-dependent) | ### Performance Benchmarks @@ -152,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 @@ -162,93 +187,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" - -# Cache offset indexes in memory (faster reads) -cache_indexes = true +size = "1 GiB" -# Cache time-based indexes in memory -cache_time_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" -# 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 +306,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 +339,51 @@ client.create_topic( ## Backup and Archiving -Iggy supports archiving expired segments before deletion, either to local disk or S3-compatible cloud storage. +> **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. -### Disk Archiving +Until archiving returns upstream, handle backups at the infrastructure level. -Archive segments to a local directory: +### Filesystem-Level Backup -```toml -[data_maintenance.archiver] -enabled = true -kind = "disk" - -# 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 - -# 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/" +[system.backup] +# Path for storing backup data, relative to system.path (default: "backup") +path = "backup" -# 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 +### Streaming Data Out (Connectors) -``` -1. Segment expires (all messages past retention) - └── segment-0000000000.log - -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 +391,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 +419,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 +433,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 +462,7 @@ Performance ←───────────────────── [system.partition] enforce_fsync = false -[system.partition.message_saver] +[message_saver] enabled = true enforce_fsync = false interval = "5s" @@ -461,14 +472,14 @@ interval = "5s" - Latency: Sub-millisecond - Risk: May lose several seconds of data on crash -#### Balanced (Most Applications) +#### High (Most Applications) ```toml [system.partition] enforce_fsync = true messages_required_to_save = 100 -[system.partition.message_saver] +[message_saver] enabled = true enforce_fsync = true interval = "1s" @@ -485,7 +496,7 @@ interval = "1s" enforce_fsync = true messages_required_to_save = 1 -[system.partition.message_saver] +[message_saver] enabled = true enforce_fsync = true ``` @@ -513,7 +524,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 +532,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 +586,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 +602,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 +627,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 +741,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/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 b74b450..6cc4be3 100644 --- a/docs/tech-debt/README.md +++ b/docs/tech-debt/README.md @@ -3,12 +3,14 @@ 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) | +| [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-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/docs/tech-debt/TD-2026-07-02.md b/docs/tech-debt/TD-2026-07-02.md index 417812b..0524035 100644 --- a/docs/tech-debt/TD-2026-07-02.md +++ b/docs/tech-debt/TD-2026-07-02.md @@ -27,3 +27,13 @@ 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; 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-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/docs/tech-debt/TD-2026-07-04.md b/docs/tech-debt/TD-2026-07-04.md index a85154a..7aba971 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,22 @@ 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. 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-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/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). 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. 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..a62b1a4 --- /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 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. + +## 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/docs/tech-debt/TD-2026-07-09.md b/docs/tech-debt/TD-2026-07-09.md new file mode 100644 index 0000000..fe17ba3 --- /dev/null +++ b/docs/tech-debt/TD-2026-07-09.md @@ -0,0 +1,50 @@ +# 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, 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 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 + +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. diff --git a/src/handlers/messages.rs b/src/handlers/messages.rs index 1bd17e3..a91ca9f 100644 --- a/src/handlers/messages.rs +++ b/src/handlers/messages.rs @@ -21,6 +21,7 @@ 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) .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) .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,7 @@ pub async fn poll_messages( None => params, }; - let response = state.consumer.poll(params).await?; + let response = state.consumer_scoped(timeout).poll(params).await?; Ok(Json(response)) } @@ -209,10 +213,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 +227,7 @@ pub async fn send_message_to( validate_event_type(&payload.event.event_type)?; let response = state - .producer + .producer_scoped(timeout) .send_to( &path.stream, &path.topic, @@ -240,10 +245,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 +274,7 @@ pub async fn poll_messages_from( }; let response = state - .consumer + .consumer_scoped(timeout) .poll_from(&path.stream, &path.topic, params) .await?; diff --git a/src/handlers/streams.rs b/src/handlers/streams.rs index e68f816..bf50ec1 100644 --- a/src/handlers/streams.rs +++ b/src/handlers/streams.rs @@ -5,14 +5,18 @@ 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).list_streams().await?; let stream_infos: Vec = streams .into_iter() @@ -34,15 +38,16 @@ 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).get_stream(&name).await?; let created_at = parse_timestamp_with_context(stream.created_at.as_micros() as i64, "stream", &stream.name); @@ -58,28 +63,33 @@ 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) + .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).delete_stream(&name).await?; Ok(StatusCode::NO_CONTENT) } diff --git a/src/handlers/topics.rs b/src/handlers/topics.rs index 0fb30b0..62fe14e 100644 --- a/src/handlers/topics.rs +++ b/src/handlers/topics.rs @@ -6,6 +6,7 @@ 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); + 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); + 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) .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) .delete_topic(&path.stream, &path.topic) .await?; diff --git a/src/iggy_client/circuit_breaker.rs b/src/iggy_client/circuit_breaker.rs index 959be02..fa2cb12 100644 --- a/src/iggy_client/circuit_breaker.rs +++ b/src/iggy_client/circuit_breaker.rs @@ -16,19 +16,20 @@ //! │ └────┬────┘ │ Fast) │ │ //! │ │ ▲ └────┬────┘ │ //! │ │ │ │ │ -//! │ │ │ success │ timeout expires │ +//! │ │ │ │ timeout expires │ //! │ │ │ ▼ │ //! │ │ │ ┌───────────────┐ │ //! │ │ └─────────────────────────── │ HalfOpen │ │ -//! │ │ success │ (Probe with │ │ -//! │ │ │ one request) │ │ +//! │ │ success_threshold │ (token-limited│ │ +//! │ │ consecutive successes │ probes) │ │ //! │ │ └───────┬───────┘ │ //! │ │ │ │ //! │ │ │ failure │ //! │ │ ▼ │ //! │ │ ┌─────────┐ │ //! │ └───────────────────────────── │ Open │ ◄────────────────┘ -//! │ reset └─────────┘ │ +//! │ (after open_duration + └─────────┘ │ +//! │ successful probes) │ //! └────────────────────────────────────────────────────────────────────┘ //! ``` //! @@ -38,33 +39,55 @@ //! - `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`]. +//! +//! 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 //! 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) //! } //! } //! ``` 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 +153,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 +166,8 @@ impl CircuitBreakerState { opened_at: None, consecutive_failures: 0, consecutive_successes: 0, + half_open_probes_remaining: 0, + half_open_granted_at: None, } } } @@ -177,48 +206,135 @@ 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 && 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 } } } - // 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); + info!( + 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.reject_request("open") + } + 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 { + 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); + } + state.half_open_probes_remaining -= 1; + true } - self.requests_rejected.fetch_add(1, Ordering::Relaxed); - crate::metrics::record_circuit_breaker_rejection(); - return false; } + } + + /// 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.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 + /// 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 + } - true + /// 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.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)"); + } + } } /// Record a successful operation. @@ -249,8 +365,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" + ); } } } @@ -316,15 +439,24 @@ 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); - warn!("Circuit breaker forcibly opened"); + if state.state != CircuitState::Open { + self.open_now(&mut state); + warn!("Circuit breaker forcibly opened"); + } } /// Transition to Open, keeping internal counters and Prometheus metrics @@ -386,7 +518,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); @@ -394,22 +526,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); @@ -423,14 +555,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); @@ -485,4 +617,167 @@ 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); + + // 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(1)).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); + } + + #[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 d68936f..29baa17 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) @@ -58,6 +60,7 @@ mod circuit_breaker; mod connection; mod helpers; mod params; +mod resilience; mod scopeguard; use std::str::FromStr; @@ -138,7 +141,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 /// @@ -167,14 +172,29 @@ 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 (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`. + 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. /// @@ -205,7 +225,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)), }; @@ -371,11 +392,14 @@ 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 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(())) => {} @@ -420,10 +444,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 @@ -434,8 +458,15 @@ 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 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.config.operation_timeout; + let timeout = self.op_deadline; let this = self.clone(); let session = tokio::spawn(async move { this.reconnect().await }); @@ -453,151 +484,33 @@ 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 + /// 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, this view's deadline, tracked connection state, + /// and bounded reconnect session. /// - /// 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. + /// 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>, { - // 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(_) + let timeout_is_outage_signal = self.op_deadline >= self.config.operation_timeout; + resilience::run_resilient( + &self.circuit_breaker, + self.op_deadline, + timeout_is_outage_signal, + || self.state.is_connected(), + || self.reconnect_bounded(), + operation, ) + .await } // ========================================================================= @@ -1107,6 +1020,34 @@ 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, 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(); + // 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 + } + /// Get the current circuit breaker state. pub async fn circuit_breaker_state(&self) -> CircuitState { self.circuit_breaker.state().await @@ -1132,6 +1073,72 @@ 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. + 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 @@ -1195,47 +1202,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..b71e4dd --- /dev/null +++ b/src/iggy_client/resilience.rs @@ -0,0 +1,752 @@ +//! 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 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 +//! +//! 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 — and who feeds the breaker +//! +//! 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; + +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) +/// - `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 +/// - `operation` — the Iggy call; invoked once, plus at most one retry +pub(super) async fn run_resilient( + breaker: &CircuitBreaker, + timeout: Duration, + timeout_is_outage_signal: bool, + 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 { + // 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 rejected the request (current state: {}) - 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, timeout_is_outage_signal, &operation).await + } + Ok(Err(e)) => { + // 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 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 + // 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, timeout_is_outage_signal, &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`]. 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 + 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; + 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; + } + Err(e) + } + 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; + } + 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, + || 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. 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)); + + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + true, + || 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, + || 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, + true, + || 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, + || 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() { + // 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)); + + let op_calls = Arc::clone(&calls); + let result: AppResult = run_resilient( + &breaker, + TIMEOUT, + true, + || 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"); + assert_eq!(breaker.state().await, CircuitState::Open); + } + + #[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, + || 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, + || 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, + || 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, + || 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); + } + + #[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" + ); + } + + #[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 + // ========================================================================= + + #[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 + ); + } + } +} 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/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 0e1b02d..b0be944 100644 --- a/src/middleware/timeout.rs +++ b/src/middleware/timeout.rs @@ -10,19 +10,29 @@ //! X-Request-Timeout: 5000 # 5 seconds in milliseconds //! ``` //! -//! Handlers can then extract this timeout: +//! Handlers extract the parsed value (via the `OptionalFromRequestParts` +//! impl below) and scope their Iggy access to it: //! ```rust,ignore //! async fn handler( -//! timeout: Option>, -//! // ... +//! 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); +//! 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,15 +42,18 @@ //! # Security Considerations //! //! - Minimum and maximum timeout bounds are enforced to prevent abuse -//! - Invalid values are ignored (fall back to server default) -//! - Zero or negative values are rejected +//! - The effective deadline is additionally clamped to the server's global +//! operation timeout — the header can never EXTEND server-side work +//! - 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). /// @@ -59,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 { @@ -78,9 +92,32 @@ 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 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, +{ + 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. @@ -100,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, @@ -108,7 +147,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" ); @@ -118,21 +160,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 { @@ -141,15 +168,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) ); } @@ -158,7 +184,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) ); } @@ -180,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)) + ); + } } 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..5e9fbe9 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..3af0f6e 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -981,6 +981,59 @@ 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); + + // 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", "1000") + .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; diff --git a/tests/metrics_smoke_test.rs b/tests/metrics_smoke_test.rs new file mode 100644 index 0000000..5497544 --- /dev/null +++ b/tests/metrics_smoke_test.rs @@ -0,0 +1,82 @@ +//! 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. + // 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 client.get(&url).send().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}" + ); +}