Session 02: retire the tech-debt registry (TD-01..07) + double-review remediation - #26
Merged
Conversation
Extract the timeout + circuit-breaker + reconnect-and-retry composition from IggyClientWrapper::with_reconnect into resilience::run_resilient, generic over the operation, reconnect step, and connection-state check. Behavior-preserving: the wrapper method now binds the executor to its breaker, configured timeout, tracked state, and bounded reconnect. Add the paused-clock test matrix (one test per executor branch) that TD-2026-07-01 required before any further behavioral change to these paths, plus tokio test-util as a dev-only feature to drive it. Resolves TD-2026-07-01.
Entering HalfOpen now grants success_threshold probe tokens (minimum one); each allowed request consumes one and exhausted budgets fail fast, so a recovering server no longer receives an unbounded thundering herd of concurrent probes. Tokens re-grant after open_duration elapses in HalfOpen, guaranteeing a probe whose outcome is never recorded (the non-connection-error path) cannot wedge the breaker. Breaker timing switches from std to tokio Instant (identical in production) so the new behavior is covered by paused-clock tests. Both doc sites that previously claimed limited probes - the circuit_breaker module doc and the IggyClientWrapper struct doc - now describe the implemented token semantics. Resolves TD-2026-07-03.
The header was parsed into a request extension but nothing consumed it;
the documented deadline propagation changed nothing at runtime. Handlers
now extract the parsed RequestTimeout and scope their Iggy access
through AppState::{producer,consumer,iggy}_scoped ->
IggyClientWrapper::with_timeout, a request-scoped view whose operation
deadline is the header value clamped to the global
OPERATION_TIMEOUT_SECS (clients may shorten, never extend). Scoped
views share all Arc'd internals, so circuit-breaker bookkeeping and
reconnect coordination stay global.
Covered end-to-end by an integration test driving send, poll, and
stream-management routes with valid, minimum, and malformed headers.
Resolves TD-2026-07-04.
init_metrics (recorder install + HTTP listener) had zero runtime coverage; a metrics-exporter-prometheus behavior change would have been discovered at production startup. The new test binary installs the exporter on an ephemeral port, records via the public counter/histogram/gauge helpers, scrapes /metrics, and asserts the metric names and labels appear. Own test binary because the recorder install is process-global. Resolves TD-2026-07-05.
Mutable tags (dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2, taiki-e/install-action@<tool>, codecov/codecov-action@v4, rustsec/audit-check@v2, softprops/action-gh-release@v1) let a compromised or force-moved tag execute attacker-controlled code in CI. Every third-party action across the four workflows is now pinned to a 40-char commit SHA with a version comment. Selector-style tags are converted to explicit inputs: dtolnay usages normalize to the master-branch SHA with 'with: toolchain:', and taiki-e/install-action tool tags become 'with: tool:'. Dependabot's github-actions ecosystem (already enabled) bumps SHA pins and their version comments natively. Resolves TD-2026-07-07.
Every documented config key checked against upstream tag server-0.8.0 (shipped core/server/config.toml + config schema sources). Highlights: message_saver is top-level with a 30s default interval; cache_indexes is a string enum and the separate time index is gone; validate_checksum, message_expiry, and max_size moved sections; the disk/S3 archiver section documented a feature removed before 0.8.0 and is now a removed-feature notice; retention is enforced by the default-DISABLED data_maintenance.messages cleaner (previously undocumented, so the guide's retention configs were silently non-functional); size-based deletion happens automatically at 90% of max_size. Env-var table and SDK snippet corrected; guide re-stamped. Resolves TD-2026-07-06.
Registry README gains a Status column reflecting the six records resolved this session. TD-2026-07-02 stays open and parked: crates.io max stable iggy is still 0.10.0 (0.10.1-edge.2 is a pre-release), so the SDK-minor-bump trigger has not fired; dated check note added. CLAUDE.md synced: resilience.rs and metrics_smoke_test.rs in the directory tree, test counts updated to 172 lib / 30 integration / 1 metrics smoke / 18 model.
H1: request-scoped deadlines no longer leak into global machinery. The wrapper holds Arc<Config> plus a separate op_deadline field; with_timeout overrides only the deadline (via the unit-tested clamp_deadline), so the spawned reconnect session and health probes always run at the global operation timeout, and scoped clones are cheap (no Config deep-copy). H2: a timeout is recorded as a circuit-breaker failure only when the deadline is the global one - a client-shortened X-Request-Timeout expiring is not outage evidence, so one client can no longer open the shared circuit for everyone (run_resilient gains a timeout_is_outage_signal parameter, pinned by matrix tests). H3: half-open probe tokens are released when an outcome is deliberately unrecorded (non-connection errors), so completed round-trips cannot starve recovery; CircuitBreaker::release_probe is capped at the granted budget and covered by unit + matrix tests. M1: the retry's breaker-gate bypass is documented in both modules and the now-reachable record_success Open arm is downgraded to debug with an accurate comment. M2: rejections carry a state label (open | half_open) on the metric, half-open rejections and token re-grants are logged, and global-deadline timeouts log at warn before the circuit can open silently. M6: matrix gaps closed (timeout-branch reconnect failure, retry failure recording, concurrent half-open probe race, re-grant boundary, sleep-based tests converted to the paused clock). L1: force_open is a no-op when already Open. L2: rejection bookkeeping extracted to one helper. L3: force_close resets probe fields. L4: the metrics smoke test bounds its HTTP client; the integration header test uses a CI-safe 1000ms deadline.
Round-1 theme M4 (flagged by 5 of 8 review agents): RequestTimeout's public fields let any code bypass from_millis's range validation - a hand-built zero-duration value inserted as an extension would make every scoped Iggy operation fail instantly. The field is now private with from_millis as the sole constructor and a duration() accessor; original_ms is dropped (documented for logging, never logged). RequestTimeoutExt is deleted: zero callers after the scoped-helper wiring, and it advertised a second, unclamped extraction path. Handlers now declare Option<RequestTimeout> directly via a new OptionalFromRequestParts impl, removing the repeated timeout.map(|Extension(t)| t) unwrap at all 13 call sites. Malformed header values now log at warn (a client bug worth surfacing); remaining client-visible feedback is deferred as TD-2026-07-08 with a binding trigger.
partitioning-guide: message_expiry moves to [system.topic]; the inert archive_expired advice and nonexistent delete_oldest_segments switch are removed (0.8.0 deletes oldest sealed segments automatically at 90% of max_size, gated by the default-disabled data_maintenance.messages cleaner) - the guide no longer contradicts the re-validated durable-storage guide. docs/README: archiving blurbs updated for the removed archiver. durable-storage guide: inline caveat that the 0.8.0 saver flushes with fsync:false (previously only in the TD record), the config section formerly named Balanced is renamed High to match the durability table, benchmark figures marked as not re-validated, TOC gains the Sources section. CLAUDE.md: 3x worst-case reconnect caveat and the scoped-timeout breaker exemption. TD-04 resolution reworded to 'propagation smoke test' + clamp unit tests; TD-02's Dependabot mechanism claim corrected (semver-prerelease is not a documented ignore.update-types value). New records: TD-2026-07-08 (client-visible timeout feedback) and TD-2026-07-09 (breaker state enum restructuring), each with a binding trigger; registry table updated.
Full 8-agent suite + independent citation verification (18/18 themes confirmed, 0 discarded). 4 HIGH / 6 MEDIUM themes, all remediated or deferred with binding triggers (TD-2026-07-08, TD-2026-07-09) in the four preceding commits.
with_timeout now clamps against the current view's deadline, making re-scoping shrink-only (round 2 flagged that a scoped view could widen back to the global; latent, no chaining call sites existed). A cfg(test)-only unconnected constructor pins the with_timeout WIRING directly - round 2 mutation-proved that deleting the clamp passed the entire suite when only the free function was tested. New matrix case: a scoped deadline on the disconnected path still reconnects while feeding the breaker nothing. retry_once's breaker recordings now warn like the first-attempt path (the observability asymmetry round 2 caught); out-of-range header values warn like malformed ones; the extractor gets a direct unit test and an accurate absence doc. Breaker: probe_budget() single definition shared by grant and release; force_open logs only when it acts; the module doc notes the retry-gate bypass; stale diagram label removed. Resilience module doc reconciles the false-positives section with the H2 exemption and documents the sole-feeder trade-off (stats refresher, minutes-scale convergence under all-scoped hangs). Docs: durable guide's inline fsync comment no longer contradicts its own caveat; TD-2026-07-09 gains consecutive_successes in the sketch plus the three probe-accounting loosenesses (incl. the cancellation-leak, deferred to the ProbePermit shape); TD-2026-07-08 wording matches the widened warn coverage; round-1 artifact accuracy nits fixed; CLAUDE.md count synced to 183.
Full 8-agent suite on the round-1 remediation: 0 HIGH, 3 MEDIUM (all remediated or deferred with triggers), ~10 LOW fixed in a27b9df. Every round-1 theme independently verified closed, including mutation-testing of the new regression nets.
Merged
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
Works the seven TD records from the session-01 double review, per the session-02 plan. Six resolved, one parked (trigger not fired), two new records filed from this session's own review.
resilience::run_resilient(generic over operation, reconnect, connection check); 20+ paused-clock matrix tests, one per branchsuccess_thresholdbudget,open_durationre-grant anti-wedge); breaker timing on tokioInstant; both doc sites correctedX-Request-Timeoutenforced end-to-end via request-scoped wrapper views, min-clamped to the global timeoutserver-0.8.0; major corrections (archiver removed-feature notice, default-disabled retention cleaner, message_saver defaults)with:inputsDouble review (eng-protocol, full 8-agent suite × 2 rounds)
docs/code-reviews/session-02-round1.md): 4 HIGH / 6 MEDIUM themes, 18/18 claims verification-confirmed. Highlights fixed: request-scoped deadlines leaking into the global reconnect session (op_deadline/Arc<Config>split); client-shortened timeouts opening the shared breaker (one-client DoS — scoped timeouts no longer feed it); half-open probe-token starvation (release_probe);RequestTimeouthardening +OptionalFromRequestPartsextractor.docs/code-reviews/session-02-round2.md): 0 HIGH; regression nets mutation-tested; clamp wiring pinned via a cfg(test) constructor; remaining accounting loosenesses deferred to TD-2026-07-09 with a binding trigger.Test plan
-D warnings, rustdoc-D warningsclean