Skip to content

fix(runtime): give event publishers unique discovery identities - #11014

Merged
zhongdaor-nv merged 5 commits into
mainfrom
fix/event-publisher-identity
Jul 6, 2026
Merged

fix(runtime): give event publishers unique discovery identities#11014
zhongdaor-nv merged 5 commits into
mainfrom
fix/event-publisher-identity

Conversation

@zhongdaor-nv

@zhongdaor-nv zhongdaor-nv commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Give every EventPublisher a fork-safe, per-incarnation publisher ID and use the same ID in both EventEnvelope and event-channel discovery.
  • Keep the existing four-part discovery key and serialized DiscoveryInstance::EventChannel shape while allowing multiple publishers for the same process/component/topic.
  • Track ZMQ publisher connections by full DiscoveryInstanceId and ignore unrelated topics.
  • Keep asynchronous discovery unregister work alive through graceful-shutdown Phase 2.
  • Make discovery registration terminology unambiguous: conversion uses a documented default_instance_id, registration code logs only the final instance_id, and EventPublisher logs use publisher_id. The old public with_instance_id method remains as a compatibility alias.
  • Add direct ZMQ regression coverage for two same-topic publishers and drop-to-immediate-recreate, including a fresh identity/sequence space, eventual discovery state, and continued delivery from both surviving and recreated publishers.
  • Remove the mocker E2E autouse fixture that forced NATS, so nondurable mocker coverage exercises the default ZMQ event plane.

The root cause was that each publisher bound its own ZMQ endpoint and owned its own sequence counter, but all publishers in one process registered with the process-level discovery ID. Their discovery keys therefore collided, subscribers only connected to one endpoint, and dropping any colliding publisher could remove the shared registration.

This also makes (publisher_id, sequence) valid for broker deduplication across multiple publishers and prevents a late unregister from an old publisher incarnation from removing its replacement. The wire schema and discovery path structure are unchanged. The public Rust DiscoverySpec::EventChannel variant now requires publisher_id.

Validation

  • Rebased onto origin/main at 2469510c7d6 and resolved the event-plane conflict while retaining both main's cached NATS subject path and this change's publisher lifecycle handling.
  • cargo fmt --all
  • git diff --check
  • cargo test -p dynamo-runtime transports::event_plane:: --lib — 18 passed
  • Drop-to-recreate direct-ZMQ regression repeated 20 times without failure
  • cargo test -p dynamo-runtime --lib — 433 passed, 2 ignored
  • cargo clippy -p dynamo-runtime --lib --tests -- -D warnings
  • cargo check -p dynamo-llm -p dynamo-backend-common
  • maturin develop --uv
  • pytest -q -s 'tests/router/test_router_e2e_with_mockers.py::test_router_decisions[nats_core-tcp]' — 1 passed in 40.75s with the default ZMQ event plane, two workers, and dp_size=4
  • pytest -q -s 'tests/router/test_router_e2e_with_mockers.py::test_router_decisions_disagg_round_robin_prefill_dp_rank[no_bootstrap-prefill_first]' — 1 passed in 80.35s with the default ZMQ event plane
  • pytest -q -s 'tests/router/test_router_e2e_with_mockers.py::test_router_decisions[jetstream-tcp]' — 1 passed in 40.64s, confirming durable coverage still opts into NATS
  • pre-commit run --files on all changed Rust and Python files

Summary by CodeRabbit

  • New Features

    • Event channel registrations now use a distinct publisher identity, improving how multiple publishers on the same topic are tracked.
    • Subscribers now connect only to matching ZMQ topics, reducing unrelated connections.
  • Bug Fixes

    • Fixed unregister and logging behavior so the correct instance is reported and removed.
    • Improved publisher shutdown handling so one publisher can be stopped without affecting others.

@zhongdaor-nv
zhongdaor-nv temporarily deployed to external_collaborator June 27, 2026 04:49 — with GitHub Actions Inactive
@github-actions github-actions Bot added the fix label Jun 27, 2026
@datadog-official

datadog-official Bot commented Jun 27, 2026

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

🚦 1 Pipeline job failed

Docs link check | lychee   View in Datadog   GitHub Actions

ℹ️ Info

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 42.21% (-9.87%)

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

@zhongdaor-nv
zhongdaor-nv temporarily deployed to external_collaborator June 27, 2026 05:22 — with GitHub Actions Inactive
@zhongdaor-nv
zhongdaor-nv marked this pull request as ready for review June 27, 2026 09:22
@zhongdaor-nv
zhongdaor-nv requested review from a team as code owners June 27, 2026 09:22
@zhongdaor-nv
zhongdaor-nv requested a review from a team June 27, 2026 09:22

@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 found 1 potential issue.

Open in Devin Review

Comment thread tests/router/test_router_e2e_with_mockers.py
@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The PR splits discovery ownership from event-channel publisher identity, threads the new publisher id through event publisher registration and ZMQ subscriber matching, updates discovery logging, adds shutdown-safe unregistration, expands ZMQ integration coverage, and removes a router mocker environment override.

Changes

Discovery and event-plane identity flow

Layer / File(s) Summary
Discovery identity contract
lib/runtime/src/discovery/mod.rs
DiscoverySpec::EventChannel now carries publisher_id, and with_instance_id distinguishes owner ids from publisher ids when building DiscoveryInstance values.
Discovery registration ids and logs
lib/runtime/src/discovery/kube.rs, lib/runtime/src/discovery/kv_store.rs
Kube and KV-store discovery registrations now capture owner and registered instance ids separately, and unregister logging uses the instance being removed.
EventPublisher identity and shutdown
lib/runtime/src/transports/event_plane/mod.rs
EventPublisher generates a random publisher_id, includes it in NATS and ZMQ direct discovery specs, and registers a shutdown guard during async unregistration.
ZMQ subscriber matching
lib/runtime/src/transports/event_plane/dynamic_subscriber.rs
DynamicSubscriber::start_zmq tracks active endpoints by DiscoveryInstanceId, filters add/remove events by topic, and the extractor tests cover topic and transport matching.
ZMQ multi-publisher test
lib/runtime/src/transports/event_plane/mod.rs
A Tokio test covers two ZMQ publishers on the same topic, independent discovery and delivery, and unregistering one publisher while the other remains active.

Router mocker harness

Layer / File(s) Summary
Remove NATS pinning fixture
tests/router/test_router_e2e_with_mockers.py
The mocker e2e router fixture that forced DYN_EVENT_PLANE to nats for etcd-backed tests was removed.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The PR description is detailed, but it omits the required template sections, especially Where should reviewer start? and Related Issues. Add the missing template sections and include a related issue reference or the no-issue checkbox, plus a reviewer start point.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly captures the main runtime change: assigning unique discovery identities to event publishers.
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.

Comment thread lib/runtime/src/transports/event_plane/mod.rs
zhongdaor-nv and others added 4 commits July 2, 2026 22:44
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Dropping an EventPublisher schedules an async discovery unregister and
holds a GracefulShutdownTracker guard so Runtime::shutdown Phase 2 waits
for it. Add a regression test that drops a publisher, runs the real
shutdown sequence, and asserts the guard is taken synchronously on drop,
released once unregister finishes, and that the publisher is gone from
discovery before Phase 3 cancels the main token.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@zhongdaor-nv
zhongdaor-nv merged commit c9f9629 into main Jul 6, 2026
98 of 99 checks passed
@zhongdaor-nv
zhongdaor-nv deleted the fix/event-publisher-identity branch July 6, 2026 18:17
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