chore(deps): Iggy SDK 0.10, RUSTSEC patches, and double-review remediation - #23
Merged
Conversation
- bytes 1.11.0 -> 1.12.0 (RUSTSEC-2026-0007, BytesMut::reserve overflow) - time 0.3.44 -> 0.3.53 (RUSTSEC-2026-0009, DoS via stack exhaustion) - quinn-proto 0.11.13 -> 0.11.15 (RUSTSEC-2026-0037, QUIC DoS) - rustls-webpki 0.103.8 -> 0.103.13 (RUSTSEC-2026-0049, CRL matching) - aws-lc-rs 1.15.1 -> 1.17.1, aws-lc-sys 0.34 -> 0.42 (RUSTSEC-2026-0044/0045/0046/0047/0048) - rkyv 0.7.45 -> 0.7.46 (RUSTSEC-2026-0001, UB in Arc/Rc from_value) Remaining astral-tokio-tar advisories are dev-dependency-only (testcontainers 0.26) and are resolved by the testcontainers 0.27 bump in the next commit. Fixes #13, fixes #14, fixes #15, fixes #16, fixes #17, fixes #18, fixes #19, fixes #20, fixes #21, fixes #22
- tower-http 0.6 -> 0.7 - rand 0.9 -> 0.10 (rand::rng().random() -> rand::random() free function) - metrics-exporter-prometheus 0.16 -> 0.18 - testcontainers 0.26 -> 0.27 (dev; drops vulnerable astral-tokio-tar 0.5.6 and unmaintained rustls-pemfile via bollard 0.20) - reqwest 0.12 -> 0.13 (dev) - version floors raised: tokio 1.52, uuid 1.23, rust_decimal 1.42 cargo audit now reports zero vulnerabilities.
- iggy 0.8.0 -> 0.10.0 (latest stable; Client trait API unchanged, no source changes required) - pin apache/iggy image to 0.8.0 (the server release paired with the 0.10 SDK) in docker-compose.yaml and integration tests instead of floating :latest, for reproducible builds and CI runs - verified end-to-end: all 24 integration tests green against server 0.8.0 via testcontainers
- advisories: drop deprecated vulnerability/unmaintained/yanked lint-level keys (removed in cargo-deny 0.14; vulnerabilities and yanked crates now error by default); set unmaintained = "all" - drop obsolete ignores: RUSTSEC-2024-0384 (instant no longer in tree) and RUSTSEC-2025-0134 (rustls-pemfile dropped by bollard 0.20 via testcontainers 0.27) - licenses: allow Unicode-3.0 (ICU crates) and CDLA-Permissive-2.0 (webpki-roots CA bundle pulled by iggy 0.10 TLS support) cargo deny check: advisories ok, bans ok, licenses ok, sources ok
- README/CLAUDE.md: correct Iggy versions (server 0.8.0, SDK 0.10.0) and refresh dependency tables (governor 0.10, tower-http 0.7, tokio 1.52, rust_decimal 1.42, metrics-exporter-prometheus 0.18, testcontainers 0.27) - CLAUDE.md: document the SDK integration decision (Client trait vs high-level IggyProducer/IggyConsumer) for the HTTP gateway - CHANGELOG.md: record security patches and dependency updates
Review round 1, themes B and E (flagged by 4 of 8 agents): - run 'cargo deny check advisories licenses' instead of licenses-only, drop continue-on-error, and include the job in ci-success — the freshly migrated deny.toml advisories section previously gated nothing in CI - pin extended-tests stress server to apache/iggy:0.8.0, in lockstep with docker-compose.yaml and the integration tests
Review round 1, themes C, D, F and the doc half of G (verified by the step-4.5 pass; README table rows flagged by 4 of 8 agents): - default app port 3000 -> 8000 (config.rs, .env.example): the old default collided with the Iggy server's HTTP API under the documented compose quick start; README/CLAUDE.md/docs curl examples now target 8000 for the app and keep 3000 only for the Iggy server - README: unit-test count 93 -> 130, uuid 1.23 / testcontainers 0.27 table rows, iggy_client.rs -> iggy_client/ layout, integration-test instructions rewritten (testcontainers auto-spins the server; the old '-- --ignored' command ran zero tests) - CLAUDE.md: middleware order corrected (Request ID before Timeout), poll_with_params -> poll_messages, module-path references fixed - architecture.md: poll_with_params -> poll_messages in sequence diagram - deny.toml: comment now states the real cargo-deny floor (0.18.2) and correct default severities; pruned dead MPL-2.0/Unicode-DFS-2016 allowances (license-not-encountered warnings) - routes.rs: inverted layer-order comment corrected (rate limit runs first, not last) - iggy_client/mod.rs: 'Consumer Groups' heading corrected to standalone consumer offsets semantics - tests: stale 'edge server' comment and module-path reference fixed - durable-storage guide re-stamped for server 0.8.0 - handlers/messages.rs: partition_id doc default corrected to 0 - CHANGELOG: astral-tokio-tar 'removed' -> 'upgraded to patched 0.6.x', advisory attribution corrected to aws-lc-sys, new entries recorded
Review round 1, themes A, I, J (root cause confirmed independently by 4 of 8 agents and the verification pass): the wrapper's connection-error variants had zero producers, so reconnection, breaker failures, and the connected flag could never engage; SDK 0.10's default-on transport reconnection additionally swallows most mid-op failures into blocking retries. - classify_iggy_error(): map SDK connection variants (Disconnected, NotConnected, StaleClient, ClientShutdown, ConnectionClosed, TcpError, CannotEstablishConnection) into the wrapper's connection-aware errors at every network call site - health_check(): live ping bounded by the operation timeout, driving ConnectionState; the background health task now probes instead of reading a latched flag, keeping /health and /ready truthful - record circuit-breaker failure on first-attempt timeouts: with the SDK blocking internally, timeouts are the primary outage signal - reconnect(): shutdown() the old client before swapping (prevents the SDK 0.10 detached-heartbeat leak reviving zombie connections), reset the attempt counter per session (no permanent-failure latch), bound each connect attempt, saturating backoff arithmetic with clamped exponent, jitter applied before the max-delay cap - reconnect_bounded(): request-path reconnections and follower waits are now bounded by the operation timeout - with_reconnect(): duplicated 20-line retry block extracted into retry_once() - new(): initial connect bounded by the operation timeout (previously hung forever when the server was down, contradicting its docs) - ensure_stream/ensure_topic: lookup errors are no longer swallowed; losing a concurrent creation race (NameAlreadyExists) is treated as success instead of crash-looping the process - scopeguard: de-genericized (the value channel was dead weight) - module docs and CLAUDE.md now describe the two-layer resilience design (SDK transport reconnection + wrapper policy) accurately Tests: +9 (error classification, backoff bounds/overflow/floor/cap, scopeguard early-return); 139 lib + 24 integration green.
…alid clients Review round 1, theme H (silent-failure-hunter H3/H4/H5, type-design F2, confirmed by verification pass): - TRUSTED_PROXIES is now actually enforced: forwarded headers are only honored when the direct peer (via ConnectInfo) is inside a trusted range; untrusted peers are keyed by their real peer address, so rotating spoofed X-Forwarded-For values no longer bypasses rate limiting or brute-force tracking. Previously is_trusted() had zero call sites and the documented validation was a silent no-op. - invalid TRUSTED_PROXIES entries fail startup with the new RateLimitError::InvalidTrustedProxyCidr instead of silently degrading to trust-all on a typo - auth brute-force limiter now meters FAILURES only: the old code consumed a token on every request before validation, capping valid-key clients at ~10 req/min/IP (and the whole service when clients shared the 'unknown' bucket); the limiter also now uses trusted-proxy-aware IP extraction - server started with into_make_service_with_connect_info (main and test harness) so the peer address is available to the middleware - invalid CORS_ALLOWED_ORIGINS entries are logged instead of silently dropped; an all-invalid (fail-closed) list warns loudly - serve-error path now runs state.shutdown() so background tasks are awaited on both exit paths Tests: +6 (trusted/untrusted peer, missing connect-info fallback, valid-key-never-throttled, failure-budget throttling with valid-key recovery); 145 lib + 24 integration green.
Review round 1, theme G (silent-failure-hunter H2, consistency #9): the entire metrics module was dead code - the exporter was never installed, no record_* function had a call site, and Prometheus scraped a port the app never served. - main.rs: install the Prometheus exporter on METRICS_PORT at startup; a bind failure now fails startup (silently missing metrics would defeat alerting), METRICS_PORT=0 disables it explicitly - wire the natural call sites: message send/poll counters in the services, reconnect attempts, circuit-breaker opens/rejections and state gauge, connection-status gauge from the live health probe - prometheus.yml: scrape app:9090 (the METRICS_PORT listener) instead of app:8000 where no /metrics route exists; compose now sets METRICS_PORT=9090 and maps it to host 9091 (9090 is Prometheus) - circuit breaker: failures recorded while already Open no longer refresh opened_at (stragglers were extending the open window and delaying recovery) [architect finding 9] - producer/consumer services: send/send_batch/poll now delegate to their *_to/_from variants (removes duplicated counter+response blocks) [simplifier finding 4]
Review round 1, theme K (type-design F3/F4/F7, silent-failure M5):
- GET /messages?count=0 now returns 400 via validate_poll_count
instead of the misleading 500 the SDK's InvalidMessagesCount mapped
to (the count survived .min(max_count) untouched)
- to_identifier() uses Identifier::named explicitly: str::try_into
reinterpreted all-digit names ('42') as numeric server-assigned IDs,
silently targeting a different resource - dangerous on DELETE; the
docstring no longer claims the SDK enforces the charset invariant
(that lives in validation.rs)
- removed PollMessagesRequest: dead API surface consumed only by
tests, documenting a 'None = all partitions' capability nothing
implements
- test fixture: a server-task panic now fails fast with a clear
message instead of being masked as a generic 60-attempt timeout
(oneshot Closed is now distinguished from Empty)
Tests: +3 (all-digit-name identity, poll-count validation); suite now
147 lib + 18 model + 24 integration, all green.
…t sync Review round 1, themes L and triage closure: - new integration test pinning PollingStrategy::next + auto_commit: a second offset-less poll must not re-deliver committed messages (the biggest unpinned SDK semantic in the 0.10 bump) - new integration test pinning Partitioning::messages_key_str: all messages sharing a key land in exactly one partition - docs/tech-debt/ registry created with six records (TD-2026-07-01 through -06), each with a binding trigger, covering the review's deferred findings: with_reconnect test matrix, DiagnosticEvents integration, half-open probe limiting, X-Request-Timeout enforcement, metrics smoke test, durable-storage re-validation - CLAUDE.md: X-Request-Timeout section now states honestly that the header is parsed but not yet enforced (TD-2026-07-04) - test counts synced everywhere: 147 lib / 26 integration / 18 model - CHANGELOG: Fixed section summarizing the review remediation
Review round 2 (consistency lens): the round-1 port standardization missed several documents, falsifying the CHANGELOG claim that all docs agree on 8000. - README/CLAUDE.md config tables: PORT default 3000 -> 8000 - architecture.md: docker run mapping and Kubernetes liveness/readiness probes 3000 -> 8000 (the old manifest yielded CrashLoopBackOff) - rustdoc curl examples (messages, auth, request_id handlers/middleware) and health.rs probe example: 3000 -> 8000 - CHANGELOG: merged duplicate Unreleased section headings (Keep a Changelog violation from prepending) and deleted the stale Security block claiming deny.toml ignores this branch actually pruned - Dockerfile: EXPOSE 9090 (metrics listener) and install curl, which HEALTHCHECK invokes but the runtime image never contained - README/CLAUDE.md observability sections now document the app's own /metrics endpoint (host 9091 under compose) - middleware diagrams (routes.rs, README, CLAUDE.md) include the Timeout-extract layer in correct order - README: error table gains the 503 connection/circuit/timeout variants this branch made producible; directory listings gain utils.rs, metrics.rs, handlers/util.rs; .env.example claim softened
…nfig Review round 2 (logic-security F1/F2 MEDIUM, type-design R2-1, plus converging dead-surface findings from three lenses): - honored X-Forwarded-For chains are now resolved with the rightmost-untrusted rule (walk right-to-left, skip trusted-range hops, first untrusted address wins) and parsed as IpAddr, falling back to the peer address on unparseable chains. The previous first-entry choice let an attacker behind an APPENDING proxy (the nginx/ALB/ingress default) rotate spoofed first entries and keep bypassing per-IP limits - exactly the attack the round-1 fix targeted; garbage values can also no longer mint arbitrary limiter keys - X-Real-IP from a trusted peer is honored only if it parses as an IP - missing-ConnectInfo fallback warns once per process instead of per-request per-middleware (latent log flood) - RateLimitLayer::with_trusted_proxies now takes the shared Arc<TrustedProxyConfig> - the config is parsed exactly once and genuinely shared by auth and rate limiting, as the routes.rs comment claimed; RateLimitError is #[non_exhaustive] - removed dead surface: TrustedProxyConfig::is_trusted(&str) (tests ported to is_trusted_ip), UNKNOWN_IP/extract_client_ip re-exports, classify_iggy_error un-exported (fallback contract too easy to violate publicly), stale auth.rs comment; extract_client_ip is now the documented header-only fallback that the validated variant delegates to - ip.rs module docs/diagram rewritten for the new flow; CLAUDE.md documents the rightmost-untrusted semantics Tests: +9 (appending-proxy spoof defeat, multi-hop trusted skip, all-trusted chain, unparseable chain/garbage X-Real-IP fallbacks, invalid-CIDR fail-fast, IPv6 containment, /0 prefix); 156 lib + 26 integration green
Review round 2 (silent-failure R2-1..R2-8, simplifier, test-analyzer): Resilience: - failed/timed-out reconnect attempts now shutdown() the new client before retrying - the same zombie-heartbeat leak class round 1 fixed for the success path; per-attempt connect bound halved so failure exits reach the cleanup arms before reconnect_bounded's outer deadline aborts the session mid-connect - old-client shutdown failure now logs at warn (it exists to kill the heartbeat; silence at debug hid the exact failure it prevents) - health task logs the recovery transition at info and repeat outages at warn (operators no longer infer recovery from silence) Observability: - metrics exporter starts BEFORE the Iggy client so startup-window reconnect/breaker metrics are not dropped by the no-op recorder; gauges seeded at startup (absent-series was indistinguishable from healthy); Config::metrics_addr() is the single source of truth (honors HOST; main.rs no longer builds its own address) and the orphaned try_init_metrics is gone - send/poll duration histograms are now recorded, and failed sends/batches record status=failure (outages previously looked like traffic stopping); the never-wired request-duration histogram is removed rather than documented-but-empty - circuit breaker: open_now() unifies the three Open transitions; force_open now emits the open counter/gauge and force_close resets the gauge (Prometheus could silently drift from internal counters) CI: deny job runs 'cargo deny check' (all sections - bans and sources were configured but never checked) Tests (+7 unit, +3 integration; suite 158 lib / 29 integration): - invalid-CIDR fail-fast, IPv6/zero-prefix CIDR containment - backoff max-below-floor and zero-max edges - auth budgets derived from constants; per-IP failure-bucket isolation - wire-level spoofed-XFF rotation cannot bypass the rate limiter (secure fixture now runs with TRUSTED_PROXIES enforcement on) - count=0 returns 400 on both poll routes - initialize_defaults idempotence + live health_check() true path - semantic tests poll-until-deadline instead of fixed sleeps; secure fixture gets the same fast-fail on server-task death
- docs/code-reviews/session-01-round2.md: provenance (including the two lens failures on session limits), verification results for every round-1 fix, disposition table for all round-2 findings - TD-2026-07-01: records that its own trigger fired in-session, accepted explicitly and re-armed - test counts synced to 158 lib / 29 integration / 18 model
…lete) The two review agents lost to the session limit delivered after reset. Architect (HIGH + 2 MEDIUM, all fixed): - reconnect sessions now run in a SPAWNED task awaited under the caller's timeout: dropping the reconnect future at its commit-point awaits (connect-success -> swap/shutdown) leaked a fully-connected client whose detached SDK heartbeat could never be shut down, with a realistic trigger (health-probe read guard delaying the write lock past the deadline); JoinHandle drop does not cancel, so the session now completes in the background and later callers join as followers - wait_for_reconnection had a genuine lost-wakeup race: tokio Notify registers on first poll, not creation, and notify_waiters stores no permit - now enable()d before the is_reconnecting check (the old comment claimed the opposite semantics) - error classification extended to all four transports the connection string supports (QuicError, HttpError, EmptyResponse, the WebSocket family) - QUIC/HTTP/WS deployments would otherwise resurrect the round-1 dead-reconnect-path bug; HttpResponseError deliberately excluded (server answered = application error) with a pinning test - RECONNECT_MAX_DELAY_MS=0 now rejected at startup (zero-delay spin) - health_check lock interaction and with_reconnect worst-case latency (3x operation timeout) and breaker false-positive mode documented Comment-analyzer (13/13 round-1 prose fixes verified; new ones fixed): - governor burst semantics corrected in four places: allow_burst REPLACES bucket capacity, it is not additive above RPS - stale rate_limit module doc (claimed log-only enforcement), auth guarantee doc now cites the rightmost-untrusted rule, non-compiling init_metrics example, License-check naming, IP-extraction bullets - round-1 artifact corrected with bracketed round-2 annotations (the 'disable SDK reconnection' plan that proved impossible, and the phantom Theme-H TD deferral that was actually implemented) - routes.rs diagram gains the Body Limit layer; TD-01/TD-03 updated Round-2 artifact addendum records the late reports and dispositions. Gates: 159 lib + 29 integration + 18 model green, clippy -D warnings clean, fmt clean.
The re-enabled CI (the workflow had been auto-disabled for repo inactivity, which is why no PR run had ever exercised this branch) surfaced two failures: - MSRV: iggy 0.10's compio-buf 0.8 uses MaybeUninit slice APIs stabilized in Rust 1.93 and declares no rust-version, so the earlier metadata-based MSRV sweep could not see it and cargo cannot enforce it at resolution time. Verified empirically: 1.90/1.91/1.92 fail, 1.93.0 compiles and passes all 159 lib tests. rust-version, CI matrix, README badge/prerequisites, and CLAUDE.md updated; recorded as Breaking in the CHANGELOG. A downgrade was not possible: iggy_common 0.10 hard-requires the compio 0.18 line. - docs: public extract_client_ip_with_validation doc linked the private rightmost_untrusted_xff (rustdoc::private-intra-doc-links under -D warnings); demoted to plain code formatting. Verified with RUSTDOCFLAGS='-D warnings' cargo doc locally.
The re-enabled weekly Extended Tests failed on main: the io_uring-based server (0.6+) panics with 'Cannot create runtime: Operation not permitted' under the default container seccomp profile. The GitHub services block predates the io_uring rewrite and never gained the flag that docker-compose (privileged: true) and the testcontainers fixture (.with_privileged(true)) already carry. Documentation Coverage and all other extended jobs passed.
Reproduced the health failure locally against apache/iggy:0.8.0: the image ships no curl (health-cmd could never succeed) AND the server binds 127.0.0.1 by default, so the runner could not reach 8090 either. - healthcheck switched to the bundled 'iggy ping' CLI (verified in the local container; mirrors docker-compose's healthcheck) - env sets 0.0.0.0 bind addresses and iggy/iggy root credentials, matching the stress step's connection string and compose
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full dependency/SDK refresh plus a two-round, eight-lens code review of the entire repo, remediated in 17 chained commits. Each commit closes one theme; review artifacts live in
docs/code-reviews/session-01-round{1,2}.mdand deferred items carry binding-trigger records indocs/tech-debt/.Security
bytes,time,quinn-proto,rustls-webpki, 5x AWS-LC,rkyv);cargo auditreports zero vulnerabilitiestestcontainers0.27 drops vulnerableastral-tokio-tar0.5.x and unmaintainedrustls-pemfilefrom the dev treeTRUSTED_PROXIESis now actually enforced: peer-address gating viaConnectInfo+ rightmost-untrustedX-Forwarded-Forresolution (robust against appending proxies); invalid CIDRs fail startupDependencies
apache/iggy:0.8.0everywhere (compose, integration tests, weekly stress CI)Review findings fixed (highlights)
/health//readylied during outages) — now: classified errors across all four transports, livepinghealth probes, cancellation-safe spawned reconnect sessions, saturating backoff, leak-free client swapscount=0polls return 400 instead of 500; all-digit names ("42") no longer silently target numeric server IDswait_for_reconnection(tokioNotifyregistration semantics)Breaking
Test plan
-D warnings, fmt,cargo audit,cargo deny checkall cleanNote: this PR intentionally exceeds the size guideline — it carries a security refresh plus the full double-review remediation as one reviewed, gated unit; the 17-commit chain is structured for commit-by-commit review.
Fixes #13, fixes #14, fixes #15, fixes #16, fixes #17, fixes #18, fixes #19, fixes #20, fixes #21, fixes #22