feat(api): add bounded admission control - #4467
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
/ok to test ed10f57 |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4467.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/api-core/src/cfg/file.rs (1)
3951-4023: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider the repository table-test helpers for these validation cases.
The three new tests map inputs to
Resultoutcomes. This file already importscheck_valuesandscenarios!. A single table would replace the hand-rolledZeroOutalias, the manual loop, and the duplicated upper-bound helper.Example shape:
scenarios!( run = |config: ApiAdmissionControlConfig| config.validate().map_err(|error| error.to_string()); "bounds are validated only when enabled" { /* cases */ } );As per coding guidelines: "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results. Use scenarios! with Outcome for Result-returning operations, value_scenarios! for total operations, and direct check_cases/check_values when macros obscure the table."
🤖 Prompt for 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. In `@crates/api-core/src/cfg/file.rs` around lines 3951 - 4023, Refactor the admission-control validation tests around api_admission_control_only_validates_bounds_when_enabled and the upper-bound tests into a single scenarios! table using the imported check_values and Outcome patterns. Replace the ZeroOut alias, manual iteration, and assert_api_admission_semaphore_bound helper with table cases that cover disabled zero bounds, zero-value validation errors, and Tokio semaphore maximum acceptance/rejection while preserving field and maximum checks.Source: Coding guidelines
crates/api-core/src/admission.rs (1)
606-612: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the bounded yield loop with a deterministic wait.
The loop yields at most 100 times and then asserts the occupancy. Scheduler variation can exhaust the budget before the pending task reaches the semaphore, which makes the test flaky in CI. A
tokio::time::timeoutaround aNotify, or a timeout around the occupancy poll, removes the fixed iteration count.♻️ Proposed change to remove the fixed iteration budget
- for _ in 0..100 { - if controller.occupancy() == (0, 0) { - break; - } - tokio::task::yield_now().await; - } - assert_eq!(controller.occupancy(), (0, 0)); + tokio::time::timeout(Duration::from_secs(5), async { + while controller.occupancy() != (0, 0) { + tokio::task::yield_now().await; + } + }) + .await + .expect("pending request should occupy the queue slot");🤖 Prompt for 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. In `@crates/api-core/src/admission.rs` around lines 606 - 612, Replace the fixed 100-iteration yield loop after the controller operation with a deterministic asynchronous wait, using a tokio::time::timeout around an appropriate Notify or occupancy-poll future. Ensure the wait completes when controller.occupancy() reaches (0, 0), then retain the final assertion and fail clearly if the timeout expires.
🤖 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.
Nitpick comments:
In `@crates/api-core/src/admission.rs`:
- Around line 606-612: Replace the fixed 100-iteration yield loop after the
controller operation with a deterministic asynchronous wait, using a
tokio::time::timeout around an appropriate Notify or occupancy-poll future.
Ensure the wait completes when controller.occupancy() reaches (0, 0), then
retain the final assertion and fail clearly if the timeout expires.
In `@crates/api-core/src/cfg/file.rs`:
- Around line 3951-4023: Refactor the admission-control validation tests around
api_admission_control_only_validates_bounds_when_enabled and the upper-bound
tests into a single scenarios! table using the imported check_values and Outcome
patterns. Replace the ZeroOut alias, manual iteration, and
assert_api_admission_semaphore_bound helper with table cases that cover disabled
zero bounds, zero-value validation errors, and Tokio semaphore maximum
acceptance/rejection while preserving field and maximum checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9395923a-9161-4595-a068-eb5778a1ed58
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
book/src/configuration/configurability.mdcrates/api-core/Cargo.tomlcrates/api-core/src/admission.rscrates/api-core/src/cfg/README.mdcrates/api-core/src/cfg/file.rscrates/api-core/src/cfg/load.rscrates/api-core/src/handlers/scout_stream.rscrates/api-core/src/lib.rscrates/api-core/src/listener.rscrates/api-core/src/test_support/default_config.rsdocs/observability/core_metrics.md
1 participant
Adds bounded admission control for Forge gRPC and admin HTTP requests. It limits executing and pending work before handlers access the database, returns transport-appropriate overload responses, and releases permits when handler execution completes.
Also prevents stalled Scout streams from holding capacity indefinitely by timing out while waiting for the initial
Initmessage.Related issues
Closes #4212
Related: #4215
Type of Change
Breaking Changes
Testing
Additional Notes