Skip to content

Review fixes for #115: shutdown order, mux slot retirement, and runtime edge cases - #118

Merged
jthomson04 merged 29 commits into
jthomson04/velo-reliability-cleanupfrom
ai/pr115-mega-review
Oct 5, 2026
Merged

Review fixes for #115: shutdown order, mux slot retirement, and runtime edge cases#118
jthomson04 merged 29 commits into
jthomson04/velo-reliability-cleanupfrom
ai/pr115-mega-review

Conversation

@ryanolson

@ryanolson ryanolson commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR adds 29 commits on top of the head of #115 (f3e0bbe). They come from an 11-pass code review. Each pass had a strict review and an independent pass by a second model. The last pass found no actionable findings. Each correctness fix has a test that failed before the change.

Shutdown

  • The gRPC stream server shutdown waited forever for a connection that never sent the HTTP/2 preface. Now the server drops after a 1 s grace period.

  • AnchorManager::shutdown runs in this order:

    1. It stops the mux tasks.
    2. It takes streams off their slots: SPSC feeds are withdrawn, and MPSC pumps are cancelled.
    3. It removes anchors and cancels local senders.
    4. It joins the mux and retires the slots.

    Steps 1 to 3 do not await. Before this change, three failures were possible:

    • A live reader could end with SenderDropped.
    • A slot close went to a transport that was already torn down, and the send_error was logged at ERROR.
    • A dropped shutdown future left anchors with no feed.

  • IngressRegistry::shutdown clears binds before it walks the slot tables. A concurrent claim cannot leak a slot.

  • stop_mux retires slots only on the first stop.

Runtime edge cases

  • MPSC detach failed after the runtime that made the sender stopped. Now it tries try_send first, then it uses the runtime of the caller.
  • A detach on a thread with no runtime now arms the unattached timeout.
  • Two aborts during thread-local teardown now degrade. They came from Handle::enter and from tokio::spawn on a destroyed context.
  • prebind_anchor no longer panics when it races shutdown. It counts that case as a failed pre-bind.
  • A cancelled SPSC reader pump now stops while its channel is full.
  • A shed fire-and-forget message no longer starts a reply task.

Performance

  • StreamTasks::is_stopped reads an AtomicBool. CancellationToken::is_cancelled, which it used before, takes a mutex. A microbenchmark of the check alone:

    is_cancelled AtomicBool
    1 thread 10.8 ns 0.5 ns
    4 threads 203 ns 1.1 ns

  • UCX no longer clones a flume Sender for each send.

Structure

  • The mux stop logic moved to messenger_mux/lifecycle.rs. mod.rs is now less than 1000 lines.
  • The transports have one stop sequence.
  • The messenger server has one helper for error replies.

Documentation

The Velo::shutdown rustdoc, README.md, and docs/src/concepts/shutdown.md now state two facts:

  • Shutdown does not free memory.
  • A stream with a local sender ends when the application drops or finalizes that sender.

Validation

  • cargo test --all-features --all-targets: 1,751 passed, 0 failed, 2 ignored. NATS and etcd were running. UCX ran with UCX_TLS=tcp.
  • cargo clippy --all-features --no-deps --all-targets -- -D warnings and cargo fmt --check are clean.
  • scripts/check-semver.sh passes: velo 0.18.1 against 0.18.0, and velo-ext 0.5.4.
  • The book builds, and its links pass the check.

Deferred to #116

These items are documented in the code where relevant:

  • The mux still runs while graceful_shutdown tears down the transports.
  • The events teardown-watcher task stays alive when a Messenger is dropped without shutdown.
  • Messenger, its server, and its hub hold references to each other.
  • AnchorManager has no admission gate after shutdown.

Notes

ryanolson added 29 commits October 4, 2026 00:01
@ryanolson
fix(streaming): bound gRPC stream server shutdown when a peer never s…
f6571e6 
…peaks HTTP/2

tonic's graceful shutdown waits for every connection, and a connection
that never finished its HTTP/2 handshake cannot receive GOAWAY. A health
check or a half-open peer therefore held GrpcFrameTransport::shutdown,
and with it Velo::shutdown, open forever. Drop the server after a short
grace period so the listener closes and shutdown returns.

Also move a regression doc comment back onto the test it describes.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): arm the unattached timeout when detach runs off-runtime
5a07a8b 
detach now re-arms the anchor's unattached timeout once the Detached
sentinel is queued. On the fast path that runs on the caller's thread,
and a thread with no runtime (a sender moved to a binding's thread)
could not spawn the timer, so an anchor nobody re-attached was never
reaped. Enter the sender's runtime there.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): let MPSC detach outlive the runtime that made the sender
df02fdb 
detach always spawned its terminal send on the runtime the sender was
attached on. Once that runtime stopped, the task was dropped, detach
returned ChannelClosed with room in the channel, and the consumer saw
neither Detached nor Dropped. Queue the terminal with try_send first,
which commits it at once, and only on a full channel spawn the wait on
the caller's runtime. Drop and the cleanup guard follow the same rule.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
perf(streaming): read the stream-task stop flag without a lock
72c7b17 
CancellationToken::is_cancelled takes a mutex, and every peer and lane
of a mux checks StreamTasks::is_stopped several times per batch. Keep an
AtomicBool next to the token, set under the admission lock in stop().
A microbenchmark of the check alone: 10.8 ns uncontended and 203 ns
with 4 threads for the token, 0.5 ns and 1.1 ns for the flag.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
refactor(streaming): give the mux one stop path in its own module
4eb3ccb 
MuxCore carried a cancel token that was only another name for its
StreamTasks token; read the tasks instead. Stopping the tasks and
closing ingress happened in four places; one stop_mux helper does it
now. Batcher start and the stop path move to messenger_mux/lifecycle.rs,
which brings mod.rs back under 1000 lines.

The batcher keeps its own cancel handle, now documented: a stop that
lands while its loop is polled exits through teardown(true), and tests
use a separate token to drive that exit.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
refactor(transports): one stop sequence for startup failure and shutdown
58bc110 
StartupTransports::stop and VeloBackend::shutdown_now repeated the same
drain, teardown and shutdown steps. Both drain calls are idempotent, so
one helper serves both.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
refactor(messenger): one helper for error replies in the server
449febc 
Four places sent an error response and logged a failure unless it was
an admission refusal. They now share send_error_reply. fail_fast awaits
the reply in the handler task, and the ordered-lane shed path spawns it
on the messenger task tracker instead of an untracked task.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
refactor(ucx): tell connection epochs apart by the admission lock
2fb25be 
ConnHandle held a channel sender only so the pump could recognise its own
epoch on cleanup. The handle is cloned on every send, so that cost a
shared atomic inc and dec each time. The admission Arc is already in the
handle and is unique per epoch, so compare that instead.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): stop the mux before removing anchors at shutdown
2ca31ca 
Velo::shutdown tears down the messenger transports before the anchor
manager. Removing an anchor with a live mux stream closes its slot, and
while the mux still ran that close was batched to the peer: TCP had no
connection left, dialed, found itself cancelled, and failed the frame
as a send error logged at ERROR. Stop the mux first, so the close finds
no batcher and returns.

The new test failed in 2 of 3 runs before the change and passed 20 of
20 after it.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
docs(streaming): name the batcher's admission task as it is spawned now
7ada25a 
Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): do not enter a runtime during thread-local teardown
a90f1f3 
A stream sender dropped from a thread-local destructor can outlive the
thread's tokio context, and Handle::enter panics on a destroyed context;
a panic there aborts the process. Enter the sender's runtime only when
the thread has no runtime at all, through one helper both senders use.
Otherwise the timer is not armed, as with no runtime.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(messenger): start no reply task for a shed fire-and-forget message
2df6fb5 
Moving the shed reply into a tracked task also moved the fire-and-forget
check inside it, so every shed fire-and-forget message spawned a task
and allocated the handler name just to return. Check before spawning:
shedding is the overload path and must stay cheaper than accepting.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): withdraw stream feeds before the mux stops at shutdown
eaf265c 
Stopping the mux first kept shutdown from writing to torn-down
transports, but stopping it retires every ingress slot by injecting
Dropped. A consumer still reading through its direct feed took that as
its sender's and ended with SenderDropped. Withdraw every anchor's feed
first, as AnchorEntry::drop does before it closes a slot; the consumer
then sees its stream end cleanly. Failed 5 of 5 runs before, passes 10
of 10 after.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): arm the MPSC unattached timeout only on a live runtime
a7187b0 
When the last local MPSC sender leaves, its anchor re-arms the
unattached timeout with a bare tokio::spawn. From a thread-local
destructor, after tokio's context is gone, that panics, and a panic
there aborts the process. Guard it with Handle::try_current like the
SPSC timeout; with no runtime the timer does not fire.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): end live MPSC mux streams cleanly at shutdown
d41522f 
Withdrawing SPSC feeds before the mux stops did not cover MPSC, whose
slot records reach the consumer through a pump: the pump forwarded the
Dropped that slot retirement injects, as the sender's own. Cancelling
the pumps first is not enough on its own, since a stopped pump releases
its slot, and a running mux would send that close over the torn-down
transport.

So shutdown stops the mux's tasks first (nothing more is written to a
peer), then takes streams off their slots (SPSC feeds withdrawn, MPSC
pumps cancelled), then retires the slots.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
docs(velo): describe what Velo::shutdown leaves in memory plainly
5c3f1bb 
Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
docs(velo): say what shutdown does to a stream with a local sender
31fc99a 
Shutdown cancels a sender on the same instance: its cancellation token
fires and later sends fail. The reader ends when the application drops
or finalizes that sender; shutdown does not end the reader itself,
because that needs a check on every read. A test pins this for SPSC and
MPSC streams.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): retire mux slots only once shutdown has detached streams
9415e19 
Shutdown stops the mux's tasks, takes every stream off its slot, then
retires the slots. But a batcher parked at an await outside its cancel
arm is dropped by its task wrapper when the stop lands, and its Drop
retired every slot at once, so a reader still on its slot could take
the injected Dropped as its sender's.

Only the first stop of a failed mux retires slots now; shutdown and
MuxCore::drop retire them explicitly. deliver_batch re-closes a late
claim once the slots are retired, keyed on a flag shutdown sets before
retiring, not on the tasks having stopped.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): finish every shutdown state change before its first a…
a03a120 
…wait

AnchorManager::shutdown withdrew feeds before it awaited the mux's
tasks but removed anchors only after. A caller that drops the future
during that await (a timeout around Velo::shutdown) left anchors with no
feed in the registry, and their readers waiting. Remove anchors, cancel
MPSC entries and local senders before the await; the mux is already
stopped, so none of it sends. Document the phases and the two rules
they keep, and update the mux lifecycle module doc to match.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
docs(velo): do not promise that shutdown memory is freed on drop
4591c3b 
The messenger, its server and its dispatcher hold each other, so
dropping the last handle does not free the instance. The book already
says shutdown does not release the ownership graph; the README and the
Velo::shutdown rustdoc now agree with it.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): clear mux binds before retiring slots
6dbb88f 
IngressRegistry::shutdown walked the slot tables and only then cleared
the binds. A batch handler claiming a bind between the two put a live
slot in a table already walked, and that slot was never retired; its
reader waited forever. The last change made this reachable on the
failed-mux path too, where deliver_batch no longer re-closed the claim.

Clear binds first. A claim takes its bind under the peer table's lock,
in a table it inserted before the claim, so it either finds no bind or
leaves a slot the walk still retires. The slots_retired flag and the
late-claim re-close in deliver_batch are no longer needed.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
docs(streaming): state the window before AnchorManager::shutdown stop…
05db259 
…s the mux

The mux runs until AnchorManager::shutdown stops it, which is after
graceful_shutdown has torn the transports down; its batchers can hand
batches to a transport that is going away in that window. Say so rather
than claim nothing is sent.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
docs(streaming): name the mux stop paths in the comments that list them
cf0c64a 
Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
test(velo): share the node and peer wiring of the shutdown tests
cc282eb 
Three shutdown tests each built the same TCP node and the same two-peer setup inline. Move that into two helpers in the test module. The tests keep their assertions, timeouts and comments.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): fail a pre-bind that races mux shutdown instead of pa…
c4abc47 
…nicking

prebind_anchor binds a mux slot and then takes the drain signal the bind
parked. Mux shutdown clears parked drain signals, so a public, sync
prebind_anchor call on another thread could find it gone and panic on
an expect. Release the bind and return None, as a pre-bind that finds
the mux stopped does.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): let a cancelled SPSC reader pump stop while its chann…
0ce261a 
…el is full

A consumer that holds its anchor without reading fills the anchor
channel, and the reader pump then waited on the send without watching
its cancel token. Velo::shutdown cancels the pump and removes the
anchor, but the application still holds the receiver, so the pump kept
its transport receiver and channel alive. Race the send against the
cancel on the full-channel path, as the MPSC pump already does; the
try_send fast path is unchanged.

Also say in the Velo::shutdown doc that reader pumps are cancelled, not
joined.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): stop calling a batcher ended by runtime shutdown an a…
63242f8 
…bort

Nothing aborts a batcher on purpose: one that ends without its teardown
has panicked, which tokio reports, or its runtime shut down before
Velo::shutdown ran. The ERROR claiming an unexpected abort misled the
second case. Log a warning that names both causes.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
test(streaming): pin that a refused stream task drops outside the lock
65cfc8c 
spawn_until_done returns a refused future with the admission lock
released only because a parameter drops after the body's locals. A
batcher's Drop stops the service and takes that lock again, so a change
that dropped the future under the lock would deadlock. Say so at the
return, and add a test whose refused future stops the service from its
Drop; dropping it under the lock fails the test.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@ryanolson
fix(streaming): count a pre-bind that finds the mux stopped as failed
36d4e8c 
The two exits taken when the mux is stopped returned None without the
debug line and the error count every other failed pre-bind records, so
an operator could not tell a shut-down mux from zero-RTT being off.
Record and log them like the rest.

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Oct 5, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository: ai-dynamo/velo/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 5ed1c227-dcc1-4eef-a43f-e05278372032

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@ryanolson
ryanolson requested a review from jthomson04 October 5, 2026 16:11
@ryanolson
ryanolson marked this pull request as ready for review October 5, 2026 16:11
@jthomson04
jthomson04 merged commit 6896e91 into jthomson04/velo-reliability-cleanup Oct 5, 2026
3 checks passed
@jthomson04
jthomson04 deleted the ai/pr115-mega-review branch October 5, 2026 20:34
This was referenced Oct 5, 2026
Merged
Merged
jthomson04 pushed a commit that referenced this pull request Oct 5, 2026
@ryanolson @jthomson04
fix(runtime): preserve shutdown ordering and stream cleanup (#118)
9edcff5 
Preserve shutdown ordering, retire mux slots safely, and handle stream cleanup across runtime shutdown. Consolidate error replies and transport teardown.

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

Signed-off-by: Ryan Olson <rolson@nvidia.com>
@jthomson04 jthomson04 mentioned this pull request Oct 5, 2026
Merged
ryanolson added a commit that referenced this pull request Oct 6, 2026
@jthomson04 @ryanolson
fix(runtime): complete sends and release resources on shutdown (#115)
2c754eb 
* fix(transports): complete queued sends and close failed startup

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* fix(runtime): close owned tasks and preserve stream lifecycles

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* chore(build): trim grpc dependencies and test core configurations

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* test(soak): use explicit drain and bounded response waits

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* fix(velo): stop mux retries when Tokio drops a batcher

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* fix(transports): retire accepted sends and roll back partial startup

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* fix(runtime): preserve ownership and close stream lifecycle races

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* ci(velo): check core libraries without dev dependency features

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* fix(streaming): retire inbound claims that race mux shutdown

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* test(mux): explain retirement regression timing

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* fix(runtime): preserve shutdown ordering and stream cleanup (#118)

Preserve shutdown ordering, retire mux slots safely, and handle stream cleanup across runtime shutdown. Consolidate error replies and transport teardown.

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

Signed-off-by: Ryan Olson <rolson@nvidia.com>

* fix(runtime): close accepted gRPC sockets during shutdown

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* ci: reduce test linker disk use

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

* fix(runtime): join mux sends before transport teardown

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>

---------

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: Ryan Olson <rolson@nvidia.com>
Co-authored-by: Ryan Olson <rolson@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jthomson04 jthomson04 Awaiting requested review from jthomson04

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ryanolson @jthomson04