valkey: use VecDeque for the command queues; LinearFifo rejects droppable items

Merged
AL
3mo ago
+299-206

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.


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

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.