Skip to content

fix(event-plane): stop retired ZMQ subscriber pumps#11730

Open
zhongdaor-nv wants to merge 4 commits into
mainfrom
codex/dis-2415-zmq-sub-pump-lifecycle
Open

fix(event-plane): stop retired ZMQ subscriber pumps#11730
zhongdaor-nv wants to merge 4 commits into
mainfrom
codex/dis-2415-zmq-sub-pump-lifecycle

Conversation

@zhongdaor-nv

@zhongdaor-nv zhongdaor-nv commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • PR #11246 (share one ZMQ context) exposed a pre-existing SUB socket leak
  • Stop the ZMQ subscriber socket pump when its final transport or subscription-stream owner is dropped, so retired publishers release their SUB sockets during churn.
  • Keep returned subscription streams owning the pump lifecycle, preserving broker-mode behavior when the temporary transport leaves scope.
  • Make DynamicSubscriber cancellation interrupt an idle discovery watch and propagate cancellation to backend-owned watch tasks.

The root cause was an ordinary Tokio JoinHandle: dropping ZmqSubTransport detached the pump while the pump continued to own the SUB socket. After the event plane moved to one shared ZMQ context, retained sockets accumulated in that finite context and eventually surfaced as EMFILE / "Too many open files".

Fixes DIS-2415.

Validation

  • cargo fmt --all -- --check
  • cargo clippy -p dynamo-runtime --lib --tests -- -D warnings
  • cargo test -p dynamo-runtime transports::event_plane --lib (25 passed)
  • Graham-style review of the final Rust diff (no findings)

Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
@github-actions github-actions Bot added the fix label Jul 15, 2026
@zhongdaor-nv zhongdaor-nv marked this pull request as ready for review July 15, 2026 21:15
@zhongdaor-nv zhongdaor-nv requested a review from a team as a code owner July 15, 2026 21:15

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f82fc24e-4ce7-42e7-8a9c-5b43c7b081af

📥 Commits

Reviewing files that changed from the base of the PR and between 65fba25 and 9cfedf1.

📒 Files selected for processing (2)
  • lib/runtime/src/transports/event_plane/dynamic_subscriber.rs
  • lib/runtime/src/transports/event_plane/zmq_transport.rs

Walkthrough

Changes

Event-plane lifecycle handling

Layer / File(s) Summary
Discovery cancellation propagation
lib/runtime/src/transports/event_plane/dynamic_subscriber.rs
DynamicSubscriber passes its cancellation token to discovery, stops watching through tokio::select!, and adds coverage for cancelling an idle discovery stream.
Socket-pump ownership
lib/runtime/src/transports/event_plane/zmq_transport.rs
ZmqSubTransport uses shared AbortOnDropHandle ownership so subscriptions retain the socket pump and its final owner stops it. Tests cover transport and stream drop behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description omits required template sections like Overview, Details, Reviewer start, and Related Issues. Add the missing sections and include either a Closes/Relates issue reference or the confirmed no-issue checkbox path.
Docstring Coverage ⚠️ Warning Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: stopping retired ZMQ subscriber pumps.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

// Pass cancellation through so the discovery backend can stop any
// task that it owns in addition to the consumer loop below.
let mut watch_stream = match discovery
.list_and_watch(query.clone(), Some(cancel_token.clone()))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing DynamicSubscriber's standalone token into KVStoreDiscovery replaces the backend's runtime-primary cancellation token, so runtime shutdown no longer stops the underlying KV watch task while the subscriber stream is still retained. Fix: pass a token linked to the runtime shutdown token, not an independent token.

🤖 AI Fix

Modify lib/runtime/src/transports/event_plane/dynamic_subscriber.rs DynamicSubscriber to add with_cancel_token(discovery: Arc<dyn Discovery>, query: DiscoveryQuery, topic: String, cancel_token: CancellationToken) -> Self, have new call it with CancellationToken::new(), and update lib/runtime/src/transports/event_plane/mod.rs EventSubscriber::new_internal direct ZMQ branch to call DynamicSubscriber::with_cancel_token(discovery, query, topic.clone(), drt.primary_token().child_token()).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 4a57350

@datadog-official

datadog-official Bot commented Jul 15, 2026

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

🚦 2 Pipeline jobs failed

PR | dynamo-runtime / test / sequential cuda13.0, arm64   View in Datadog   GitHub Actions

PR | dynamo-status-check   View in Datadog   GitHub Actions

ℹ️ Info

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 41.66% (-5.20%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: bf71dd0 | Docs | Give us feedback!

Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
@ai-dynamo ai-dynamo deleted a comment from rzdao Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants