fix: Integration tests fixes

[keivenchang]

Overview:

Fix namespace collision issues in integration tests and correct dynamo_system_ namespace handling.

Details:

• Fix namespace collision in component hierarchy
• Correct dynamo_system_ namespace in metrics system
• Update HTTP server test expectations
• Improve soak test configuration

Where should the reviewer start?

lib/runtime/src/metrics.rs - Core metrics namespace changes
lib/runtime/src/http_server.rs - HTTP server test fix

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

None

Summary by CodeRabbit

• Tests

  • Improved test clarity by updating namespace, component, and event names in event publishing and subscribing tests.
  • Enhanced metrics tests to use more descriptive namespace and event names, and updated expected output for metric naming conventions.
  • Added request counting, configurable response delay, and refined logging to the soak integration test, with environment-driven behavior for backend processing.

• Style / Documentation

  • Added documentation comments explaining the purpose and behavior of the soak integration test.

[copy-pr-bot]

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]

Walkthrough

This update revises test cases and metric naming conventions across several modules. It introduces more descriptive and distinct namespace, component, and event names in tests, adjusts metric registry naming from "http_server" to "dynamo", modifies expected Prometheus output strings, and enhances the soak test with request counting, delayed response simulation, and improved logging.

Changes

Cohort / File(s)	Change Summary
Component Event Tests lib/runtime/src/component/component.rs	Updated test event publishing/subscribing to use unique namespace/component/event names; moved subscription from namespace to component level.
Namespace Event Tests lib/runtime/src/component/namespace.rs	Changed test namespace and event names to be more descriptive in publish/subscribe tests.
HTTP Metrics Naming lib/runtime/src/http_server.rs	Changed metrics namespace from "http_server" to "dynamo"; updated metric names and Prometheus output expectations accordingly.
Metrics Test Data lib/runtime/src/metrics.rs	Updated test for invalid namespace to use special characters; adjusted expected Prometheus output for correct metric name prefixes and ordering.
Soak Test Enhancements lib/runtime/tests/soak.rs	Added backend request counting, configurable response delay, environment-driven behavior, improved routing, and refined progress logging in soak test.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Ingress
    participant Backend (RequestHandler)
    loop For each batch of requests
        Client->>Ingress: Send request batch
        Ingress->>Backend (RequestHandler): Forward request
        alt Normal processing
            Backend (RequestHandler)-->>Ingress: Immediate response stream
        else Delayed processing
            Backend (RequestHandler)-->>Ingress: Delayed response stream (100ms per yield)
        end
        Ingress-->>Client: Return streaming response
    end
    Client->>Backend (RequestHandler): Query backend_counter (after test)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Possibly related PRs

• feat: add a hierarchical Prometheus MetricsRegistry trait for DistributedRuntime, Namespace, Components, and Endpoint #2008: Adds MetricsRegistry trait implementations for DistributedRuntime, Namespace, Component, and Endpoint, modifying similar files and types as this PR, particularly around component and namespace behaviors and metrics.

Poem

In the warren where metrics gleam,
Namespaces now are clear and keen.
The backend counts each hopping quest,
With streams that pause or run their best.
Soak tests thump with data bright—
A rabbit’s joy in code’s delight! 🐇✨

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 095ea3e and d1feb8e.

📒 Files selected for processing (5)

• lib/runtime/src/component/component.rs (1 hunks)
• lib/runtime/src/component/namespace.rs (1 hunks)
• lib/runtime/src/http_server.rs (3 hunks)
• lib/runtime/src/metrics.rs (4 hunks)
• lib/runtime/tests/soak.rs (7 hunks)

🧰 Additional context used

🧠 Learnings (6)

📓 Common learnings

Learnt from: oandreeva-nv
PR: ai-dynamo/dynamo#1195
File: lib/llm/tests/block_manager.rs:260-275
Timestamp: 2025-05-29T21:46:45.112Z
Learning: The team prefers to evaluate performance-related changes like channel capacity limits during dedicated performance testing rather than implementing them immediately, especially for components like DynamoEventManager where the performance impact needs careful measurement.

lib/runtime/src/component/namespace.rs (4)

Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.

Learnt from: kthui
PR: #1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:07:24.843Z
Learning: The codebase uses async-nats version 0.40, not the older nats crate. Error handling should use async_nats::error::Error variants, not nats::Error variants.

Learnt from: oandreeva-nv
PR: #1195
File: lib/llm/tests/block_manager.rs:150-152
Timestamp: 2025-06-02T19:37:27.666Z
Learning: In Rust/Tokio applications, when background tasks use channels for communication, dropping the sender automatically signals task termination when the receiver gets None. The start_batching_publisher function in lib/llm/tests/block_manager.rs demonstrates this pattern: when the KVBMDynamoRuntimeComponent is dropped, its batch_tx sender is dropped, causing rx.recv() to return None, which triggers cleanup and task termination.

Learnt from: alec-flowers
PR: #1181
File: lib/llm/src/kv_router/publisher.rs:379-425
Timestamp: 2025-05-29T00:02:35.018Z
Learning: In lib/llm/src/kv_router/publisher.rs, the functions create_stored_blocks and create_stored_block_from_parts are correctly implemented and not problematic duplications of existing functionality elsewhere in the codebase.

lib/runtime/src/http_server.rs (1)

Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.

lib/runtime/src/metrics.rs (1)

Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.

lib/runtime/src/component/component.rs (5)

Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.

Learnt from: oandreeva-nv
PR: #1195
File: lib/llm/tests/block_manager.rs:150-152
Timestamp: 2025-06-02T19:37:27.666Z
Learning: In Rust/Tokio applications, when background tasks use channels for communication, dropping the sender automatically signals task termination when the receiver gets None. The start_batching_publisher function in lib/llm/tests/block_manager.rs demonstrates this pattern: when the KVBMDynamoRuntimeComponent is dropped, its batch_tx sender is dropped, causing rx.recv() to return None, which triggers cleanup and task termination.

Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.

Learnt from: alec-flowers
PR: #1181
File: lib/llm/src/kv_router/publisher.rs:379-425
Timestamp: 2025-05-29T00:02:35.018Z
Learning: In lib/llm/src/kv_router/publisher.rs, the functions create_stored_blocks and create_stored_block_from_parts are correctly implemented and not problematic duplications of existing functionality elsewhere in the codebase.

Learnt from: kthui
PR: #1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:07:24.843Z
Learning: The codebase uses async-nats version 0.40, not the older nats crate. Error handling should use async_nats::error::Error variants, not nats::Error variants.

lib/runtime/tests/soak.rs (9)

Learnt from: ryanolson
PR: #1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from Send + Sync + Debug to Send + Debug because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.

Learnt from: grahamking
PR: #1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.

Learnt from: kthui
PR: #1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:07:24.843Z
Learning: The codebase uses async-nats version 0.40, not the older nats crate. Error handling should use async_nats::error::Error variants, not nats::Error variants.

Learnt from: PeaBrane
PR: #1392
File: lib/llm/src/kv_router/scoring.rs:35-46
Timestamp: 2025-06-05T01:02:15.318Z
Learning: In lib/llm/src/kv_router/scoring.rs, PeaBrane prefers panic-based early failure over Result-based error handling for the worker_id() method to catch invalid data early during development.

Learnt from: oandreeva-nv
PR: #1195
File: lib/llm/tests/block_manager.rs:150-152
Timestamp: 2025-06-02T19:37:27.666Z
Learning: In Rust/Tokio applications, when background tasks use channels for communication, dropping the sender automatically signals task termination when the receiver gets None. The start_batching_publisher function in lib/llm/tests/block_manager.rs demonstrates this pattern: when the KVBMDynamoRuntimeComponent is dropped, its batch_tx sender is dropped, causing rx.recv() to return None, which triggers cleanup and task termination.

Learnt from: PeaBrane
PR: #1285
File: lib/llm/src/kv_router/scoring.rs:58-63
Timestamp: 2025-05-30T06:38:09.630Z
Learning: In lib/llm/src/kv_router/scoring.rs, the user prefers to keep the panic behavior when calculating load_avg and variance with empty endpoints rather than adding guards for division by zero. They want the code to fail fast on this error condition.

Learnt from: PeaBrane
PR: #1236
File: lib/llm/src/mocker/engine.rs:140-161
Timestamp: 2025-06-17T00:50:44.845Z
Learning: In Rust async code, when an Arc<Mutex<_>> is used solely to transfer ownership of a resource (like a channel receiver) into a spawned task rather than for sharing between multiple tasks, holding the mutex lock across an await is not problematic since there's no actual contention.

Learnt from: ryanolson
PR: #1093
File: lib/llm/src/block_manager/block/registry.rs:98-122
Timestamp: 2025-05-29T06:20:12.901Z
Learning: In lib/llm/src/block_manager/block/registry.rs, the background task spawned for handling unregister notifications uses detached concurrency by design. The JoinHandle is intentionally not stored as this represents a reasonable architectural tradeoff for a long-running cleanup task.

Learnt from: jthomson04
PR: #1429
File: lib/runtime/src/utils/leader_worker_barrier.rs:69-72
Timestamp: 2025-06-08T03:12:03.985Z
Learning: In the leader-worker barrier implementation in lib/runtime/src/utils/leader_worker_barrier.rs, the wait_for_key_count function correctly uses exact equality (==) instead of greater-than-or-equal (>=) because worker IDs must be unique (enforced by etcd create-only operations), ensuring exactly the expected number of workers can register.

🔇 Additional comments (11)

lib/runtime/src/metrics.rs (2)

800-822: Good improvement to the invalid namespace test case.

The change from "test-namespace" to "@@123" is a better test case because it tests the edge case where a namespace consisting entirely of invalid characters gets sanitized to an empty string, which should fail metric creation.

---

929-937: Correct ordering of metrics in expected output.

The reordering of testintcounter metric lines to appear with the testnamespace_ prefix is correct and aligns with the actual Prometheus metric naming convention where namespace prefixes are prepended to metric names.

Also applies to: 1031-1033

lib/runtime/src/http_server.rs (2)

78-78: Consistent namespace and metric naming.

The changes correctly standardize the namespace to "dynamo" and adjust the metric name from "dynamo_uptime_seconds" to "system_uptime_seconds" to avoid redundancy. The final metric name "dynamo_system_uptime_seconds" properly reflects the namespace prefix.

Also applies to: 101-101

---

335-337: Test expectations correctly updated.

The test output properly reflects the new metric naming convention with namespace="dynamo" label.

lib/runtime/src/component/namespace.rs (1)

102-103: Improved test clarity with descriptive names.

The changes to use specific namespace names ("test_namespace_publish", "test_namespace_subscribe") and event names ("test_event") improve test clarity and help with test isolation.

Also applies to: 111-119

lib/runtime/src/component/component.rs (1)

92-94: Enhanced test specificity and clarity.

The changes improve the tests by:

1. Using descriptive namespace and component names
2. Moving subscription to the component level, which properly tests component-specific event handling
3. Maintaining consistency with the namespace test improvements

Also applies to: 102-111

lib/runtime/tests/soak.rs (5)

16-20: Good documentation addition.

The comments clearly explain the purpose of the soak test.

---

69-81: Well-structured enhancements for observability and configurability.

The additions provide:

1. Request counting via atomic counter for thread-safe tracking
2. Configurable response behavior through environment variable
3. Delayed processing option to test queue saturation scenarios

Also applies to: 88-109

---

145-146: Significant reduction in default batch size needs clarification.

The default batch_load was reduced from 10,000 to 100, which is a 100x reduction. This seems like it could significantly impact the soak test's ability to stress the system.

Was this change intentional? If so, what motivated reducing the load by such a large factor? This could affect the test's ability to identify performance issues under high load.

---

156-159: Good use of PushRouter for proper request distribution.

Converting the client to use PushRouter improves the test by ensuring requests are properly distributed across backend instances.

---

191-193: Reduced logging frequency improves readability.

Printing progress every 1000 requests instead of every batch reduces log noise while still providing adequate progress tracking.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[keivenchang]

Combined. Please review the next PR, thanks.