Repository navigation
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 78 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // SAFETY: `offset < count`, so the slot holds a live item; the shifts | ||
| // below overwrite it without reading it again. | ||
| unsafe { ptr::drop_in_place(self.peek_item_mut(offset)) }; |
There was a problem hiding this comment.
🟡 ordered_remove_item runs drop_in_place before the shift and before self.count -= 1, so a panicking T::drop unwinds through a fifo whose live range still contains the already-dropped slot — LinearFifo::drop → discard(self.count) then drops it again. discard() explicitly guards against this ('Move head first so a panicking T::drop leaves the fifo consistent'); this sibling path should get the same treatment — e.g. ptr::read the item out, do the shift + self.count -= 1, then drop the local. (Bun ships panic = "abort", so this only bites under cargo test's unwind — flagging for consistency, not blocking.)
Extended reasoning...
What the bug is
In the offset != 0 branch of ordered_remove_item, this PR adds:
unsafe { ptr::drop_in_place(self.peek_item_mut(offset)) };
// ... shift_down_one(...) ...
self.count -= 1;The drop_in_place runs before the shift and before self.count -= 1. If T::drop panics and the panic unwinds, self.count is unchanged and the slot at offset — now containing already-dropped bits — is still inside the live range [head, head+count). Unwinding then runs LinearFifo::drop (also new in this PR), which calls self.discard(self.count), which drop_in_place's the entire live range including that slot again. That's a double drop; for the PR's own Counted test type (which owns a Box<u32>) it would be a double free.
Why this is inconsistent with the same PR's discard()
The PR explicitly designed discard() to be panic-safe on exactly this hazard, with a comment stating the intent:
// Move `head` first so a panicking `T::drop` leaves the fifo
// consistent (the rest of the range leaks, is not dropped twice).
self.move_head(count);
unsafe { ptr::drop_in_place(a); ptr::drop_in_place(b); ... }ordered_remove_item was given the same drop responsibility in this PR but not the same panic guard. Per REVIEW.md "Fix the whole class in the same PR" — the two drop paths added here should be consistent.
Step-by-step proof
Take a LinearFifo<Counted, DynamicBuffer<_>> with head=0, count=3, and suppose Counted::drop for the item at offset 1 panics:
ordered_remove_item(1)—offset != 0, sodrop_in_place(self.peek_item_mut(1))runs.Counted::droppanics. At the panic point,self.count == 3and slot 1 holds a droppedCounted(itsBox<u32>freed).- Unwinding drops the
LinearFifo.Drop::dropseesself.count == 3and callsself.discard(3). discard(3)computes(a, b) = as_mut_slices()covering slots[0, 3), moves head, thenptr::drop_in_place(a)— which drops slot 1 a second time. UB / double free of theBox.
Contrast with discard(): it moves head past the range before dropping, so on a panic during the drop loop the fifo's own Drop sees count == 0 and does nothing.
Impact
Low in practice:
Cargo.tomlsetspanic = "abort"for both dev and release (lines 144, 147), so in the shipped binary a panickingT::dropaborts the process — there is no unwind and no double drop.- The sole non-test caller of
ordered_remove_item(the dev-serverweak_refsfifo insource_map_store.rs) stores a PODWeakRef {u32, u32, i64}with no drop glue, sodrop_in_placeon it is a no-op that cannot panic. - Only
cargo testbuilds unwind (the test harness forces it), and none of the new tests use a panickingDrop.
So this cannot cause a concrete failure today. It's flagged as a nit because (a) the author clearly intended panic-safety here — they wrote the comment in discard() — and (b) the two sibling drop paths added in the same PR should carry the same guard.
How to fix
Match the discard() pattern — make the fifo consistent before running T::drop:
// SAFETY: `offset < count`, so the slot holds a live item; the shifts
// below overwrite it and `count -= 1` drops it from the live range,
// so a panicking `T::drop` unwinds through a consistent fifo.
let removed = unsafe { ptr::read(self.peek_item_mut(offset)) };
// ... existing shift_down_one(...) branches ...
self.count -= 1;
drop(removed);If drop(removed) panics, the fifo has already shifted and decremented count, so LinearFifo::drop drops only the remaining live items — the removed one leaks instead of being dropped twice, mirroring discard()'s guarantee.
|
@robobun adopt |
|
Closed as superseded by #39570. Nothing was pushed to this branch. |
|
Superseded by #39570. The review found a simpler shape: the two Redis queues are the only LinearFifo users with droppable items, and they only need push, pop, front, len and iter, so they become VecDeque. LinearFifo gets a compile-time assert that its item type has no drop glue. No new unsafe. |
|
Standing down here. I did not push to this branch. Two notes from the verification of this PR, for the record. Both are moot in the shape of #39570.
|
…able items (#39570) Replaces #39545. The problem The Valkey client keeps two queues: the offline queue of serialized commands and the in-flight queue of promises. Both were LinearFifo rings. LinearFifo never drops its items. Every consumer of that ring except these two holds bytes, raw pointers or Copy structs, so that was fine there. Here each item owns a JS promise handle and a boxed byte buffer. That leaks on main today. reject_all_pending_commands moves both queues into locals and rejects each item with `?`. When a reject throws, for example during a worker teardown, the function returns early and both locals are dropped with items still inside. Those promises and boxes are never freed. The ASAN tests in #39543 observe exactly this leak. There was a second bug. Two places read the queue with `readable_slice(0)`, which only returns the first contiguous half of a wrapped ring. The auto-pipeline count and the memory estimate both under-counted once the ring had wrapped. What changed The two queue aliases are now std::collections::VecDeque. Every call site is a mechanical rename: init to new, readable_length to len or is_empty, readable_slice(0)[0] to front, write_item to push_back, read_item to pop_front, the two whole-queue scans to iter. Control flow is unchanged. VecDeque drops what is left inside it, so any early return now frees the remaining items. #39543 still fixes the drain loop itself so every promise gets rejected; this PR only makes the early return leak-free. LinearFifo now requires `T: Copy` on all of its impl blocks. The ring never runs item destructors, and the bound states that where cargo check and rust-analyzer see it, before monomorphization. Two consumers needed a derive: FillItem in the lockfile tree builder and RefDataValue in the test runner. Both hold only integers, raw pointers and Copy structs. Every other consumer was already Copy. The per-method `T: Copy` clauses that read, write, unget and peek_item carried are gone with the impl-level bound, and the memmove helper is now slice::copy_within. The header comment states the contract. Visible changes Two, both fixes. Before, the flush wrote the pre-wrap segment of the ring, stayed registered, and wrote the rest only when the event loop woke again. Nothing about the pending flush shortens the poll, so that wake was whatever else happened to fire: a reply, a timer, other I/O. Measured with nothing else live, the tail of a burst left about 80 ms after its head. Now every pipelineable command goes out in one write. estimateShallowMemoryUsageOf counts every queued command's bytes. Before, it counted only the pre-wrap segment. Tests Three new tests in test/js/valkey/reliability/connection-failures.test.ts. One drains and refills the queue so the old ring wrapped, then checks the memory estimate covers all queued bytes. It fails on main (2244 bytes reported for 5000 queued). The other queues 40 commands against a stub that never finishes the handshake, closes, and checks all 40 reject. That one passes on main too and is there to pin the behaviour. The third runs the client in a child process with nothing else live and has a stub count the GETs in the first read after the ring wrapped: main writes 2 of 5 there and the other 3 on a later wake, this branch writes all 5. Not in this PR The drain loop in reject_all_pending_commands still stops at the first throwing reject. #39543 fixes that. The DeferredFailure path when the VM is already stopping still does not settle its promises; that is a gap for the state machine rewrite. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/valkey/reliability/connection-failures.test.ts <!-- robobun:evidence:end --> Two follow-ups outside this PR. #37618 puts an OwnedRef into the MySQL request queue's LinearFifo; with the Copy bound that no longer compiles, and the queue should become a VecDeque the same way, which also removes its manual Drop drain. The Postgres request queue holds raw pointers today and is the next candidate for the same change. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
2 participants
The problem
LinearFifo stores its items in MaybeUninit slots. Rust does not drop MaybeUninit contents. The comment in linear_fifo.rs said that field drop glue covers deinit. That is only true for types with no drop glue. If a fifo of Box or of JS promise handles is dropped with items still queued, the storage is freed but the items are not dropped. They leak.
Both RedisClient queues hold such items. The offline queue holds Entry (a Box<[u8]> and a promise Strong). The in flight queue holds PromisePair (a promise Strong). Today shutdown() and reject_all_pending_commands() drain both queues by hand with read_item() before the queues are dropped, so I did not find a live leak in the Redis client. The leak is one missed drain away, and any new fifo of a droppable type gets it for free. discard() and ordered_remove_item() had the same gap. They moved head past the items or memmoved over them without a drop.
readable_slice(0) returns only the first contiguous run of the ring. Once the ring wraps, the second run is not in the slice. Two RedisClient sites used it as a whole queue scan. The auto pipeline flush counted pipelineable commands from it, so a wrapped queue was flushed in two socket writes instead of one. The memoryCost estimate summed serialized bytes from it, so it under reported a wrapped queue. Both were found while reviewing #34829 and #39193.
What changed
LinearFifo now implements Drop. It drops the live items in ring order, both runs. discard(n) drops the first n items before it moves head. read_item() moves the item out and then advances head without a drop, so nothing is dropped twice. ordered_remove_item() drops the removed item before the shift. Types without drop glue take the same fast path as before.
New accessors: as_slices(), as_mut_slices(), iter() and iter_mut(). They cover all live items across both runs in FIFO order. The two scan sites in valkey.rs and js_valkey.rs now use iter(). The head peeks at send_next_command() and drain() only read element 0 of readable_slice(0). That element is always in the first run, so they are unchanged.
Tests
Unit tests in linear_fifo.rs use a type that counts its drops and owns a Box. They cover drop of a partially read fifo, drop of a wrapped ring, drop of a static ring, discard across the wrap, read_item not dropping, ordered_remove_item on both runs, iter() and iter_mut() on empty, full, wrapped and grown rings, and iter() matching repeated read_item(). The three drop tests fail on main.
Not in this PR
The MaybeUninit accessor rework noted at the top of the file. writable_slice() still hands out &mut [T] over unwritten slots. That needs a signature change in other crates.