fix(core): validate CPU/IO env vars with a diagnosable error#7856
fix(core): validate CPU/IO env vars with a diagnosable error#7856LuciferYang wants to merge 2 commits into
Conversation
`LANCE_CPU_THREADS` and `LANCE_IO_CORE_RESERVATION` were parsed with `.parse().unwrap()`. A non-integer value panicked inside the `LazyLock` init with a bare `ParseIntError` naming neither the variable nor the cause. Worse, `LANCE_CPU_THREADS=0` parsed fine, then flowed into `max_blocking_threads(0)`, which asserts inside tokio on the first compute task — a confusing panic far from the misconfiguration. Route both through `parse_env_usize`, which trims surrounding whitespace, names the offending variable in the error, and rejects values below a per-variable minimum: `LANCE_CPU_THREADS` must be at least 1, while `LANCE_IO_CORE_RESERVATION` still allows 0 (reserve no cores for IO). Bad values still fail fast, now with an actionable message. The variables feed process-global `LazyLock`s that read once and are read in parallel by other tests, so the pure parser is unit-tested directly rather than by mutating the environment.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCPU and I/O environment-variable parsing now uses a shared validator that trims input, enforces minimum values, reports named errors, and is covered by unit tests. ChangesEnvironment variable parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
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/tokio.rs-24-25 (1)
24-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-Unicode environment values
std::env::vartreatsVarError::NotUnicodeas an error, but bothLANCE_CPU_THREADSandLANCE_IO_CORE_RESERVATIONcurrently handle it as if the variable were unset. Match onlyNotPresentas absence and return a descriptive error forNotUnicodeinstead of silently falling back to the defaults.🤖 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/tokio.rs` around lines 24 - 25, Update the environment-variable handling in the CPU-thread and IO-core reservation logic around LANCE_CPU_THREADS and LANCE_IO_CORE_RESERVATION to distinguish VarError::NotPresent from VarError::NotUnicode. Continue using defaults only when variables are absent, and return or panic with a descriptive error containing the affected variable name when its value is not valid Unicode.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.
Inline comments:
In `@rust/lance-core/src/utils/tokio.rs`:
- Around line 63-69: Add a Rust doc comment to the public static
IO_CORE_RESERVATION documenting the LANCE_IO_CORE_RESERVATION environment
variable, default value of 2, accepted nonnegative values including 0, and panic
behavior for invalid input. Explain its relationship to
get_num_compute_intensive_cpus, include a compiling example, and link to the
related API symbols and environment-variable documentation.
- Around line 24-25: Update the runtime initialization flow around the
LANCE_CPU_THREADS and corresponding second parse_env_usize call sites to return
a typed error instead of invoking panic!. Propagate the Result through the
initialization boundary, preserving the environment variable name and raw value
in the contextual error.
---
Other comments:
In `@rust/lance-core/src/utils/tokio.rs`:
- Around line 24-25: Update the environment-variable handling in the CPU-thread
and IO-core reservation logic around LANCE_CPU_THREADS and
LANCE_IO_CORE_RESERVATION to distinguish VarError::NotPresent from
VarError::NotUnicode. Continue using defaults only when variables are absent,
and return or panic with a descriptive error containing the affected variable
name when its value is not valid Unicode.
🪄 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: QUIET
Plan: Pro Plus
Run ID: d34fff57-b221-44a6-a052-55fd8f450fde
📒 Files selected for processing (1)
rust/lance-core/src/utils/tokio.rs
Document the `LANCE_IO_CORE_RESERVATION` variable, the default of 2, that 0 is allowed, the invalid-input behavior, and the relationship to get_num_compute_intensive_cpus.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Summary
LANCE_CPU_THREADSandLANCE_IO_CORE_RESERVATIONwere parsed with.parse().unwrap(). Two problems followed from a misconfigured value:LazyLockinitializer with a bareParseIntErrorthat named neither the variable nor the cause. A stray quote or trailing space (common in.envfiles, Docker, and k8s manifests) was enough to trigger it, and the process stayed panicked because the poisonedLazyLockre-panics on every subsequent access.LANCE_CPU_THREADS=0parsed fine, then flowed intocreate_runtime'smax_blocking_threads(0), which asserts inside tokio (Max blocking threads cannot be set to 0) on the first compute task. The panic surfaced deep in tokio, far from the actual misconfiguration, with no hint that the CPU-threads variable was at fault.Both variables are documented as user-tunable in the performance guide, so a hand-set bad value is a realistic, reachable failure — not a corrupt-input edge case.
Change
Route both through a small pure helper,
parse_env_usize(name, raw, min), which trims surrounding whitespace, names the offending variable in the error, and rejects values below a per-variable minimum.LANCE_CPU_THREADSmust be at least 1 (the floor tokio requires);LANCE_IO_CORE_RESERVATIONkeeps allowing 0, meaning reserve no cores for IO. Unset still defaults to 2. Bad values still fail fast — the contract is unchanged — but now with an actionable message instead of a bareParseIntErroror an unrelated tokio assertion.The only inputs newly rejected are ones that never worked:
LANCE_CPU_THREADS=0(previously a guaranteed tokio assertion) and values that already panicked as unparseable. Whitespace-padded values that used to panic now parse, so the change only widens the set of accepted configurations aside from those two.Test plan
The variables feed process-global
LazyLocks that read once and are read in parallel by other tests, so the environment cannot be mutated reliably from a test. The pure parser is unit-tested directly instead:parses_valid_value_and_trims_surrounding_whitespacerejects_non_integer_naming_the_variablerejects_value_below_minimum(theLANCE_CPU_THREADS=0case)allows_zero_when_minimum_is_zero(theLANCE_IO_CORE_RESERVATION=0case)cargo fmt --allandcargo clippy -p lance-core --tests -- -D warningsare clean.