fix(core): bound SlotBackoff retry sleeps and fix slot overflow - #7883
Conversation
SlotBackoff doubled its slot count every attempt with no ceiling, so the `slot_i * unit` product (a u32 multiply widened to u64 only afterward) could overflow at high attempt counts, and the sleep grew without limit. This is reachable on the default commit-retry path (20 retries): once the first attempt takes a few seconds, `unit` is large enough that a deep retry overflows — a debug panic, or in release a wrapped, wrong sleep. Cap the slot count at MAX_SLOTS (128), which already exceeds any realistic number of concurrent committers, and widen the multiply before it runs. Capping the slot count rather than the resulting duration preserves the uniform-slot spreading that keeps contending writers from colliding; a duration clamp would pile every high-attempt writer onto the same instant. Every backoff is now bounded by `(MAX_SLOTS - 1) * unit`. The cap is proportional to `unit`, so a slow first attempt can still produce a multi-minute single sleep. Bounding wall-clock regardless of `unit` needs a timeout around the commit-path sleep (as the write-retry path already has); that is a separate change.
📝 WalkthroughWalkthrough
ChangesSlot Backoff
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/backoff.rs-156-156 (1)
156-156: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent
attemptfrom wrapping.After
u32::MAXcalls, this panics in debug builds and wraps to zero in release builds, restarting the low-slot distribution. Use a non-wrapping increment that preserves the capped regime, or make overflow explicit through a fallible API.Proposed compatible fix
- self.attempt += 1; + self.attempt = self.attempt.saturating_add(1);As per coding guidelines, “Use
checked_addandchecked_mulinstead ofwrapping_addandwrapping_mulfor counters and IDs, and return an error on overflow.”🤖 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/backoff.rs` at line 156, Update the attempt increment in the backoff implementation to prevent u32 overflow after reaching the maximum value. Use a non-wrapping or checked increment that preserves the capped backoff behavior, and propagate an explicit error if the API is made fallible; keep the existing distribution logic unchanged.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/backoff.rs`:
- Line 156: Update the attempt increment in the backoff implementation to
prevent u32 overflow after reaching the maximum value. Use a non-wrapping or
checked increment that preserves the capped backoff behavior, and propagate an
explicit error if the API is made fallible; keep the existing distribution logic
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 5d0d2e76-080f-4c21-9a23-dbaa8c9d3342
📒 Files selected for processing (1)
rust/lance-core/src/utils/backoff.rs
The `attempt` counter used a plain `+= 1`, which would panic in debug and wrap to zero in release after u32::MAX calls, restarting the low-slot distribution. Saturate it, matching the `saturating_add` already used for the exponent a few lines above. Not reachable in practice, but keeps the counter monotonic and consistent with the rest of the function.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/lance-core/src/utils/backoff.rs (2)
151-154: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new slot ceiling in the public API docs.
The implementation now caps the slot count at
MAX_SLOTS, but theSlotBackoffdocumentation should explicitly describe this limit so its exponential-growth description remains accurate.As per coding guidelines:
**/*.{rs,md,rst}requires public APIs to document semantics with synchronized examples and links.🤖 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/backoff.rs` around lines 151 - 154, Update the public documentation for SlotBackoff to state that exponential slot growth is capped at MAX_SLOTS, and synchronize any associated example or link required by the API documentation guidelines. Keep the implementation unchanged.Source: Coding guidelines
261-284: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake this overflow regression test deterministic. The current loop depends on
SmallRng::from_os_rng()eventually picking a non-zero slot within 200 draws, so it can fail independently of the overflow fix. Seed the RNG or test the slot-multiplication path directly.🤖 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/backoff.rs` around lines 261 - 284, Make test_slot_backoff_large_unit_does_not_overflow deterministic by removing its dependence on SlotBackoff’s OS-seeded random draws. Seed the RNG used by SlotBackoff or directly exercise the slot-multiplication path with a known non-zero slot, while preserving assertions that the widened product does not wrap below unit.
🤖 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/backoff.rs`:
- Line 156: Add a focused unit test for the backoff type that initializes
attempt to u32::MAX, calls next_backoff(), and asserts attempt remains u32::MAX,
directly covering saturating counter behavior.
---
Outside diff comments:
In `@rust/lance-core/src/utils/backoff.rs`:
- Around line 151-154: Update the public documentation for SlotBackoff to state
that exponential slot growth is capped at MAX_SLOTS, and synchronize any
associated example or link required by the API documentation guidelines. Keep
the implementation unchanged.
- Around line 261-284: Make test_slot_backoff_large_unit_does_not_overflow
deterministic by removing its dependence on SlotBackoff’s OS-seeded random
draws. Seed the RNG used by SlotBackoff or directly exercise the
slot-multiplication path with a known non-zero slot, while preserving assertions
that the widened product does not wrap below unit.
🪄 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: 1b2d6390-971b-41cf-ac40-ca699bed334a
📒 Files selected for processing (1)
rust/lance-core/src/utils/backoff.rs
Directly exercise the saturating counter: set attempt to u32::MAX, call next_backoff, and assert it stays put. Fails if the increment reverts to `+= 1` (debug panic / release wrap).
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/backoff.rs (1)
260-285: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake the overflow regression test deterministic.
This test only detects a reverted
u32multiplication when a slot greater than 1 is drawn; slot 1 still producesu32::MAX. Seed the RNG or otherwise force a known slot so the test cannot pass or fail based on random draws.🤖 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/backoff.rs` around lines 260 - 285, Make test_slot_backoff_large_unit_does_not_overflow deterministic by controlling SlotBackoff’s RNG or slot selection so a slot greater than 1 is guaranteed during the test. Preserve the overflow assertion while removing reliance on random draws and eliminate the saw_nonzero loop/state if the forced slot makes it unnecessary.
🤖 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/backoff.rs`:
- Around line 260-285: Make test_slot_backoff_large_unit_does_not_overflow
deterministic by controlling SlotBackoff’s RNG or slot selection so a slot
greater than 1 is guaranteed during the test. Preserve the overflow assertion
while removing reliance on random draws and eliminate the saw_nonzero loop/state
if the forced slot makes it unnecessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: de4a3112-92fd-469e-a392-6cc604d094ce
📒 Files selected for processing (1)
rust/lance-core/src/utils/backoff.rs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Seed the RNG so the drawn slots are reproducible, and assert the backoff is an exact multiple of unit (a wrapped u32 product is not) instead of a lower bound that a slot of 1 would satisfy. Removes the reliance on random draws flagged in review.
`cargo clippy --all-targets` flags `field_reassign_with_default` when a field is assigned after `Default::default()`. Build the SlotBackoff with a struct literal (`..Default::default()`) instead.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance-core/src/utils/backoff.rs (1)
261-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the cap boundary explicitly.
This verifies the upper bound and overflow handling, but not the promised transition where attempts 0–4 retain their original ranges and attempt 5 is the first capped range. Add deterministic assertions around attempts 4 and 5 to catch an off-by-one in the cap or exponent calculation.
As per coding guidelines, every bugfix and feature must have corresponding tests.
🤖 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/backoff.rs` around lines 261 - 277, Extend the backoff tests around SlotBackoff to deterministically assert the uncapped range for attempt 4 and the first capped range for attempt 5. Use a controlled RNG or fixed inputs so the assertions verify the exact transition and detect off-by-one errors in the cap or exponent calculation, while retaining the existing overflow and upper-bound checks.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.
Nitpick comments:
In `@rust/lance-core/src/utils/backoff.rs`:
- Around line 261-277: Extend the backoff tests around SlotBackoff to
deterministically assert the uncapped range for attempt 4 and the first capped
range for attempt 5. Use a controlled RNG or fixed inputs so the assertions
verify the exact transition and detect off-by-one errors in the cap or exponent
calculation, while retaining the existing overflow and upper-bound checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 8deb8285-91b6-4bc8-90dc-fa4704167d91
📒 Files selected for processing (1)
rust/lance-core/src/utils/backoff.rs
|
Thanks! |
|
Thank you @wjones127 |
Summary
SlotBackoff::next_backoffdoubled the slot count on every attempt with no ceiling, and computed the sleep as(slot_i * self.unit) as u64— au32 * u32multiply that widens tou64only after the product is formed. Two problems followed:slot_i * unitproduct overflowsu32: a debug-build panic, or in release a wrapped, wrong sleep duration.This is reachable on the default commit-retry path (20 retries).
unitis set to the first attempt's latency plus 10% (commit.rs), so once the first commit takes a few seconds, a deep retry drivesslot_iinto the range where the product overflows.Change
Cap the slot count at
MAX_SLOTS(128) and widen the operands tou64before multiplying. 128 slots already exceeds any realistic number of concurrent committers, so further doubling only lengthens the wait without reducing collisions. Every backoff is now bounded by(MAX_SLOTS - 1) * unit.Capping the slot count rather than the resulting duration is deliberate:
SlotBackoffspreads contending writers across random slots so they don't collide, and a duration clamp would map every high-attempt writer onto the same instant, recreating the thundering herd the type exists to prevent. A fixed 128-slot grid keeps writers uniformly distributed. Attempts 0-4 are unchanged (their slot counts are already below 128); the cap only affects attempt 5 and beyond, where the extra slots bought no throughput anyway (commit throughput is one perunitregardless of slot count).Follow-up (out of scope)
The cap is proportional to
unit, not absolute, so a slow first attempt can still produce a multi-minute single sleep. The commit-path sleep has no timeout wrapper, unlike the write-retry path which bounds its sleep against a deadline. Bounding wall-clock regardless ofunitis tracked separately in #7882.Test plan
test_slot_backoff(existing) — low-attempt slot distributions, unchanged.test_slot_backoff_high_attempt_is_bounded— every backoff stays within(MAX_SLOTS - 1) * unitacross many high attempts; fails if the cap is removed.test_slot_backoff_large_unit_does_not_overflow— withunit = u32::MAX, asserts a lower bound that a revertedu32multiply would violate in release (wraps to a smaller value) as well as debug (panics).cargo fmt --allandcargo clippy -p lance-core --tests -- -D warningsare clean.