Skip to content

fix(reliability): clamp max_concurrent to Semaphore::MAX_PERMITS; add ReliabilityConfig::disabled() (LAB-729) - #50

Merged
27Bslash6 merged 3 commits into
mainfrom
lab-729-panel-followup
Jul 30, 2026
Merged

fix(reliability): clamp max_concurrent to Semaphore::MAX_PERMITS; add ReliabilityConfig::disabled() (LAB-729)#50
27Bslash6 merged 3 commits into
mainfrom
lab-729-panel-followup

Conversation

@27Bslash6

@27Bslash6 27Bslash6 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Applies the surviving findings from the formal expert-panel review of the merged #49 (LAB-729), run 2026-07-28 at high stakes on Ray's request (panel: bug-hunter-supreme, security-specialist, code-craftsman, catchphrase-agent).

Panel verdict on #49: FIX-FIRST → this PR

  • MAJ (bug-hunter, verified by executing it): Semaphore::new(max_concurrent.max(1)) panics when max_concurrent > tokio::sync::Semaphore::MAX_PERMITS (usize::MAX >> 3). usize::MAX is the natural "effectively unbounded" sentinel a user of this public config will reach for, and it crashed inside CacheKitBuilder::build() with tokio's internal panic. Now clamped to [1, MAX_PERMITS], documented on the field next to the existing "0 behaves as 1" note, pinned by limiter_clamps_huge_max_concurrent_instead_of_panicking.
  • MIN (craftsman): the builder's wrap-or-skip gate hand-enumerated each layer's is_some() — the exact shape that shipped a missed-layer bug once already inside feat(reliability): backpressure — bound backend-op concurrency (LAB-729) #49's own history. The check now lives on ReliabilityConfig::is_disabled(), beside the fields, so a future fourth layer cannot silently miss the gate.
  • MIN (craftsman): the documented opt-out was a full struct literal, which breaks every downstream opt-out each time the stack gains a layer (twice now). ReliabilityConfig::disabled() replaces it in the README, builder rustdoc, and preset docs (doctest-covered).

Rejected, with reason

  • MIN (craftsman): dedupe the bounded-poll test helper across macro_tests.rs / reliability_tests.rs. Rejected — the "fix" is a closure-parameterized generic helper in tests/common, which costs more machinery than the eight duplicated test-scaffold lines it removes; the catchphrase reviewer's own pass reached the same conclusion for the sibling test fixtures. Revisit if a third copy appears.

Clean passes

  • security-specialist: NO FINDINGS — verified the secure path still fails closed on a shed (empty fail-open arm at both macro splice sites), shed messages carry no secrets (CWE-532), and the in-process limiter crosses no trust boundary.
  • catchphrase-agent: NO CUTS — "appropriately simple: every layer earns its place"; all three config knobs, the RAII guards, and the test weight ruled load-bearing.

Gates: fmt, clippy -D warnings, 244 tests + 8 doc-tests, 0 failures.

This PR contains only the panel's own prescriptions; the panel report on LAB-729 covers it.

Summary by CodeRabbit

  • New Features
    • Added a convenient option to disable the reliability stack entirely.
    • Reliability settings now consistently determine whether reliability protections are applied.
  • Bug Fixes
    • Prevented failures when configuring extremely large concurrency limits by safely clamping values.
    • Clarified handling of zero and oversized concurrency settings.
  • Documentation
    • Updated examples and guidance to show the recommended way to disable reliability features.
    • Added clarification that a disabled configuration applies no wrapping.

… ReliabilityConfig::disabled() (LAB-729)

Expert-panel follow-up on the merged #49 (panel run 2026-07-28, Ray-requested):

- MAJ (bug-hunter, verified live): tokio's Semaphore::new panics above
  MAX_PERMITS (usize::MAX >> 3), and usize::MAX is the natural
  'effectively unbounded' sentinel a caller will pass — a public config
  value must never panic CacheKitBuilder::build(). Clamp to
  [1, MAX_PERMITS], document on the field, pin with a unit test.
- MIN (craftsman): the builder's wrap-or-skip gate hand-enumerated every
  layer's is_some() and shipped a missed-layer bug once inside #49
  already; the check now lives on ReliabilityConfig::is_disabled(), next
  to the fields, so a future layer can't silently miss the gate.
- MIN (craftsman): the documented opt-out was a full struct literal that
  breaks downstream every time the stack gains a layer (twice now) —
  ReliabilityConfig::disabled() replaces it in README, rustdoc, and the
  preset docs.
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 80c38188-1ea3-4a68-a95b-dfd788d72d08

📥 Commits

Reviewing files that changed from the base of the PR and between 7e87397 and c08fd45.

📒 Files selected for processing (4)
  • README.md
  • crates/cachekit/src/client.rs
  • crates/cachekit/src/intents.rs
  • crates/cachekit/src/reliability.rs

Walkthrough

The reliability configuration now has explicit disabled-state helpers, CacheKit skips reliability wrapping for disabled configurations, and concurrency limits are clamped to Tokio semaphore bounds with test coverage. README and API documentation describe the updated behaviour.

Changes

Reliability configuration and concurrency safeguards

Layer / File(s) Summary
Disabled reliability configuration
crates/cachekit/src/reliability.rs, crates/cachekit/src/client.rs, crates/cachekit/src/intents.rs, README.md
ReliabilityConfig::disabled() and is_disabled() define the opt-out state, which CacheKitBuilder uses to skip reliability wrapping; documentation and examples use the new API.
Concurrency limit clamping
crates/cachekit/src/reliability.rs
Backpressure limits are clamped to valid Tokio semaphore bounds, with documentation and a test for extremely large values.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main changes: clamping max_concurrent and adding ReliabilityConfig::disabled().
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lab-729-panel-followup

Comment @coderabbitai help to get the list of available commands.

@kodus-27b

kodus-27b Bot commented Jul 28, 2026

Copy link
Copy Markdown

Kody Review Complete

Great news! 🎉
No issues were found that match your current review configurations.

Keep up the excellent work! 🚀

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the @kody start-review command at the root of your PR.

  • Validate Business Logic: Ask Kody to validate your code against business rules by adding a comment with the @kody -v business-logic command.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug
Performance
Security
Business Logic

Access your configuration settings here.

@27Bslash6
27Bslash6 enabled auto-merge (squash) July 30, 2026 02:28
@27Bslash6
27Bslash6 merged commit d851797 into main Jul 30, 2026
10 of 11 checks passed
@27Bslash6
27Bslash6 deleted the lab-729-panel-followup branch July 30, 2026 02:30
27Bslash6 added a commit that referenced this pull request Jul 30, 2026
…:disabled() (LAB-1059) (#57)

The CacheKitBuilder::reliability rustdoc was the one opt-out surface #50
missed - it still taught the all-None config prose that
ReliabilityConfig::disabled() replaced in README, preset docs, and the
reliability rustdoc. Same panel MIN, same fix, last surface.
@cachekit-release-bot cachekit-release-bot Bot mentioned this pull request Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant