fix(core): reject non-finite values in AimdConfig::validate - #7942
fix(core): reject non-finite values in AimdConfig::validate#7942LuciferYang wants to merge 2 commits into
Conversation
AimdConfig::validate checked its rate fields with sign and ordering comparisons (`<= 0.0`, `min_rate > max_rate`, etc.), all of which are false for NaN, so a NaN or infinity slipped through. These configs are user-reachable: the LANCE_AIMD_* env vars and storage options are parsed with `f64::parse`, which accepts "nan"/"inf". A non-finite rate does not fail loudly. A NaN rate makes the token bucket refill to full on every acquire (`NaN.min(burst)` returns burst), silently disabling throttling; only with a zero burst capacity does it reach `Duration::from_secs_f64(NaN)` and panic. Reject all six f64 fields up front when not finite, with an error naming the field. This also rejects `max_rate = inf`, which previously acted as an undocumented "no ceiling" alias; the documented sentinel for that is `max_rate = 0.0`, which is unaffected.
📝 WalkthroughWalkthroughAIMD configuration validation now rejects NaN and infinite values for controller parameters before applying existing range checks. Parameterized tests cover non-finite inputs and expected validation messages. ChangesAIMD validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-core/src/utils/aimd.rs-350-381 (1)
350-381: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the
InvalidInputvariant as well as the message.These new cases reuse an assertion that only checks
err.to_string(). A regression returning a different error variant with the same text would still pass; assert theInvalidInputvariant and retain the field-specific message check.🤖 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 `@rust/lance-core/src/utils/aimd.rs` around lines 350 - 381, The new non-finite-value cases in the AIMD configuration validation tests only compare error text. Update the shared assertion used by these cases to also match the expected InvalidInput variant, while retaining the existing field-specific message checks from err.to_string().Source: Coding guidelines
🤖 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.
Other comments:
In `@rust/lance-core/src/utils/aimd.rs`:
- Around line 350-381: The new non-finite-value cases in the AIMD configuration
validation tests only compare error text. Update the shared assertion used by
these cases to also match the expected InvalidInput variant, while retaining the
existing field-specific message checks from err.to_string().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 59692e68-51f0-4908-9899-2057ffccb79a
📒 Files selected for processing (1)
rust/lance-core/src/utils/aimd.rs
The validation cases only checked the error message, so a regression returning a different error variant with the same text would still pass. Assert `Error::InvalidInput` alongside the message in the shared test body (covering the pre-existing cases too). Also note in the AimdConfig doc that max_rate must be finite unless it is the 0.0 no-ceiling sentinel.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-core/src/utils/aimd.rs (1)
104-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the input type in the validation error.
The new error includes the field name and value, but not the value’s type. Add
f64so callers receive complete diagnostic context.Suggested fix
- "{name} must be finite, got {value}" + "{name} (f64) must be finite, got {value}"As per coding guidelines, input-validation errors must include full context, including variable names, values, sizes, and types.
🤖 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 `@rust/lance-core/src/utils/aimd.rs` around lines 104 - 123, Update the non-finite-value validation error in the AIMD configuration validation loop to include the input type `f64` alongside the field name and value. Preserve the existing validation behavior and error structure in the loop over initial_rate, min_rate, max_rate, decrease_factor, additive_increment, and throttle_threshold.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@rust/lance-core/src/utils/aimd.rs`:
- Around line 104-123: Update the non-finite-value validation error in the AIMD
configuration validation loop to include the input type `f64` alongside the
field name and value. Preserve the existing validation behavior and error
structure in the loop over initial_rate, min_rate, max_rate, decrease_factor,
additive_increment, and throttle_threshold.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 090ba17d-50b3-495a-9764-72d298b23476
📒 Files selected for processing (1)
rust/lance-core/src/utils/aimd.rs
|
All six fields are |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary
AimdConfig::validatechecks its rate fields with sign and ordering comparisons (initial_rate <= 0.0,min_rate > max_rate,decrease_factor >= 1.0, and so on). Every one of those comparisons isfalseforNaN, so aNaNvalue passes validation untouched;+inflikewise slips through on any field that has no opposing bound (max_rate,additive_increment).These configs are user-reachable. The
LANCE_AIMD_*environment variables and the equivalentstorage_optionskeys are parsed withf64::parse, which accepts"nan","inf", and"infinity"case-insensitively, and the parsed value flows straight intoAimdConfigand thenAimdController::new→validate.Failure mode
A non-finite rate does not fail loudly, which is what makes it worth guarding. With the default burst capacity, a
NaNrate makes the token bucket refill to full on every acquire —(tokens + elapsed * NaN).min(burst)returnsburstbecausef64::mindrops the NaN — so throttling is silently disabled and the only visible symptom is aNaNleaking into rate metrics and logs. Only when the burst capacity is zero does the code reachDuration::from_secs_f64(NaN)and panic.Change
Reject all six
f64fields up front when they are not finite, with an error naming the offending field, before the existing range checks run. This is the common chokepoint for all three ingress paths (env vars, storage options, and direct construction of the publicAimdConfig), so it is more complete than validating at the parse layer.Note for reviewers
This also rejects
max_rate = f64::INFINITY, which previously passed validation and acted as an undocumented "no ceiling" alias. The documented sentinel for no ceiling ismax_rate = 0.0, which is finite and unaffected. No code, test, or configuration in the repository sets any of these fields to a non-finite value.Test plan
Extended the
test_config_validation_rejects_invalidtable with aNaNcase for each of the six fields plus+infoninitial_rateandmax_rate.cargo test -p lance-core --lib utils::aimdandcargo test -p lance-io --lib object_store::throttlepass;cargo fmt --allandcargo clippy -p lance-core --all-targets -- -D warningsare clean.