feat(reliability): backpressure — bound backend-op concurrency (LAB-729) - #49
Conversation
…B-729) Adds a ConcurrencyLimiter (tokio::sync::Semaphore + bounded waiting queue) as the outermost layer of the ReliableBackend stack: backpressure(breaker(retry(op))). One permit per logical operation, held across the whole retry sequence — bounds retry amplification, and shed calls never touch breaker counters, so the breaker keeps measuring backend health rather than caller-side overload. Over-limit behaviour is typed and bounded: a saturated limiter admits up to max_queue waiters for at most acquire_timeout each; everyone else is shed with the new non-retryable BackendErrorKind::Backpressure. Defaults mirror cachekit-py's BackpressureController (100 concurrent / 1000 queued / 100 ms), and the production/encrypted/io presets enable it via ReliabilityConfig::default() while minimal stays unbounded — the same preset posture as the LAB-518 retry/breaker layers. Builder gating now also wraps on a backpressure-only config.
…cuitOpen (LAB-729) Review-panel CRIT: the graceful-degradation arm matched is_retryable() || CircuitOpen only, so a Backpressure shed hard-failed #[cachekit]-wrapped functions while the sicker breaker-open state failed open — an availability inversion, and a silent divergence from cachekit-py (whose decorator runs the function uncached on a backpressure rejection). A shed is the same class as CircuitOpen: the call never reached the backend. Failing open adds no work a cold-miss storm doesn't already create — the limiter bounds backend cache ops, not origin executions, and single-flight still dedupes those. secure paths stay fail-closed. Also from the panel: pin the nonzero max_queue admission boundary with a unit test, drop the stray Eq derive on BackpressureConfig (sibling configs are PartialEq only), and document the macro-layer shed behaviour on BackpressureConfig, the macro rustdoc, and the README degradation row.
This comment has been minimized.
This comment has been minimized.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe reliability stack adds default bounded concurrency with queueing and shedding. Backpressure errors are exposed publicly, integrated outside breaker and retry handling, and treated as fail-open conditions by the plain macro path. Tests and documentation cover configuration, composition, cancellation, and shedding. ChangesBackpressure reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CacheMacro
participant ReliableBackend
participant Backpressure
participant BreakerRetry
participant Backend
Caller->>CacheMacro: invoke cached operation
CacheMacro->>ReliableBackend: request backend access
ReliableBackend->>Backpressure: acquire permit
Backpressure-->>ReliableBackend: permit or shed error
ReliableBackend->>BreakerRetry: run admitted operation
BreakerRetry->>Backend: execute backend call
Backend-->>BreakerRetry: response
BreakerRetry-->>ReliableBackend: result
ReliableBackend-->>CacheMacro: result or backpressure error
CacheMacro-->>Caller: cached result or uncached fail-open result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
27Bslash6
changed the title
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/cachekit/src/reliability.rs`:
- Around line 454-528: Update the three Backpressure error messages in
ConcurrencyLimiter::acquire to include the relevant configured limits: max_queue
for queue-full and acquire_timeout for timeout; include max_concurrent as well
if needed to identify the limiter capacity. Preserve the existing shed behavior
and distinguish queue-full, unavailable, and timeout cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 549bd568-b7f3-4564-995e-c77845c7050e
📒 Files selected for processing (9)
README.mdcrates/cachekit-macros/src/lib.rscrates/cachekit/src/client.rscrates/cachekit/src/error.rscrates/cachekit/src/intents.rscrates/cachekit/src/lib.rscrates/cachekit/src/reliability.rscrates/cachekit/tests/macro_tests.rscrates/cachekit/tests/reliability_tests.rs
…ssages, bounded test polls (LAB-729) CodeRabbit: the queue-full and wait-timeout shed messages now carry the configured max_queue / acquire_timeout, so a production incident can tell from the error alone which limit was hit. The three branches stay distinguishable; shed behaviour unchanged. Kody (critical): the saturate-the-limiter tests polled with an equality-terminated spin (== 0), which hangs CI forever if the condition is ever skipped. All four loops (the flagged macro_tests one plus the three same-pattern loops in reliability_tests) now use a relational condition with a 5 s deadline that fails the test fast instead.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@coderabbitai review |
|
@kody start-review |
✅ Action performedReview finished.
|
|
@27Bslash6 — LAB-729 is through the automated review gates and is ready for your signoff / merge. Automated review is clean on the current head (
Every check run is against the current head SHA, not a stale base. Expert panel not run — the crypto/protocol gate was assessed and does not fire. The diff changes no encryption, AAD construction, key derivation, cache-key format, or ByteStorage wire format. The one security-adjacent surface was verified by hand: the macro emits the fail-open arm only in the Companion PRs under the same ticket:
Not merging — that's yours. |
1 participant
Closes LAB-729.
Decision record (LAB-729 gate): build, not rs-specific N/A
src/cachekit/reliability/load_control.py(threading.Semaphore-basedBackpressureController),BackpressureConfig(enabled by default,max_concurrent_requests: 100,queue_size: 1000,timeout: 0.1s), surfaced through the decorator orchestrator with dedicated tests. A fleet-wide N/A was therefore never on the table.unsyncalike) holds unbounded concurrent backend ops in flight —fred's single multiplexed Redis connection buffers unbounded in-flight commands (caller memory), and thereqwestSaaS client grows per-host connections without cap (socket/FD exhaustion). LAB-518's retry loop and LAB-728's SWR refreshes amplify exactly that fan-out under a backend slowdown.Design
ConcurrencyLimiter(tokio::sync::Semaphore+ bounded waiting queue) as the outermost layer of the existingReliableBackenddecorator:backpressure(breaker(retry(op))).CircuitBreaker::try_acquire, so breaker counters and half-open probe slots keep measuring backend health, not caller-side overload (tested withfailure_threshold: 1).as_lockable;healthis unguarded). Tested with cap 1 + concurrent retrying callers.max_queuewaiters for at mostacquire_timeouteach; queue-full and wait-timeout both shed with the new non-retryableBackendErrorKind::Backpressure. Never an unbounded silent queue. Queue accounting is cancel-safe via an RAII drop guard (theProbePermitlesson applied).#[cachekit]plain path fails open onBackpressureexactly likeCircuitOpen— the function runs uncached;securepaths fail closed. (Review-panel catch: the initial diff left sheds hard-failing wrapped functions while the sicker breaker-open state failed open.)Divergences from cachekit-py (stated + justified)
TRANSIENT(queue full) /TIMEOUT(permit timeout) — both retryable thereBackpressurekindCircuitOpenprecedent: "not retryable now")Preset story (mirrors LAB-518 posture)
ReliabilityConfig::default()now includesbackpressure: Some(BackpressureConfig::default())→production/encrypted/ioget it;minimalstays unbounded/off. Builder gating also wraps on a backpressure-only config. Semver: new pub field onReliabilityConfigbreaks exhaustive struct literals; release-pleasebump-minor-pre-majorlands this as 0.6.0, which cargo treats as incompatible with^0.5— no silent behaviour change for existing pins.BackendErrorKindis#[non_exhaustive].Tests
max_seen ≤ Kunder a 32-way burst with cap 4 (plus all 32 complete).max_queue: 0), timed shed (waits outacquire_timeout), nonzero queue-full boundary.macro_fails_open_when_backpressure_sheds).cargo fmt --check,clippy --all-targets -D warnings, full suite 242 passed / 0 failed, MSRV 1.85 check green.Docs (gate)
README (stack table, composition, degradation row, feature table, opt-out example), rustdoc (module docs,
BackpressureConfig, presets, builder, error kind). Companion PRs: protocol matrix flip + decision footnote (cachekit-io/protocol, branchlab-729-matrix-rs-backpressure), docs.cachekit.io corrections (cachekit-io/docs, branchlab-729-docs-backpressure— the concepts page had backpressure throttling inverted, and the rust FAQ still claimed no reliability tier exists).Reviewed pre-push by a 3-agent adversarial panel (bug-hunter / code-craftsman / red-team); the surviving findings (macro fail-closed CRIT, matrix footnote mechanism claims, docs-site contradiction, two minors) are all addressed in this PR set.
Summary by CodeRabbit