fix: Truncate HttpError message to first 8192 characters

[siclait]

Overview:

Prior to this fix, instantiating an HttpError with a long error message would yield a ValueError that doesn't include any of the original message, making it more difficult to trace the source of the issue.

With this fix, we truncate the message instead to maintain some context.

Details:

A few changes:

• Truncates the HttpError message field in the constructor
• Adds tests for message truncation and passing a non-str value for message
• Simplifies the logic for extracting code and message from the py_err and drops the name=HttpError check.

Where should the reviewer start?

The truncation logic is in exceptions.py

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

N/A

Summary by CodeRabbit

• Bug Fixes

  • Enhanced HTTP error handling with stricter input validation for error codes and messages.
  • Implemented automatic truncation of excessively long error messages to improve performance and stability.

• Tests

  • Added test coverage for HTTP error validation and message truncation behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[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.

[github-actions]

👋 Hi siclait! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

[coderabbitai]

Walkthrough

The changes enhance HttpError exception handling across Python and Rust bindings by introducing stricter validation for HTTP error codes and messages, implementing message truncation at the Python layer, and replacing duck-typing with PyO3's typed extraction mechanism in the Rust layer for improved robustness.

Changes

Cohort / File(s)	Summary
Python Exception Implementation lib/bindings/python/src/dynamo/llm/exceptions.py	Added input validation requiring code to be an int within 0–599 range and message to be a str; implemented runtime message truncation at 8192 characters with warning; added private constant _MAX_MESSAGE_LENGTH.
Rust Bindings Enhancement lib/bindings/python/rust/http.rs	Introduced private struct HttpError with code: u16 and message: String fields derived from FromPyObject; replaced brittle duck-typing with type-safe PyO3 ABI-compatible extraction; normalizes extracted messages and converts to Rust http_error::HttpError.
Test Coverage lib/bindings/python/tests/test_http_error.py	Added validation tests for invalid message type (non-string input) and long message truncation (8192 character limit); verified error formatting with truncation behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Rust bindings (http.rs): Requires understanding PyO3 ABI-stable extraction semantics and verification that the typed extraction correctly replaces the previous duck-typing logic without introducing edge cases.
• Python validation (exceptions.py): Straightforward input validation and truncation logic; verify message normalization behavior aligns with Rust expectations.
• Test coverage: Verify test assertions correctly validate truncation boundaries and error messaging behavior across the Python–Rust boundary.

Poem

🐰 Errors once slippery, now held with care,
Each code and message validated fair,
Duck-types replaced by types so true,
From Python's realm to Rust's stable brew,
Eight-thousand-one-hundred-ninety-two bounds await!
Exceptions now robust—robust, and great! ✨

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: truncating HttpError messages to 8192 characters, which is directly reflected in the file changes and PR objectives.
Description check	✅ Passed	The description follows the template structure with Overview, Details, and Where to start sections. All required sections are present and adequately filled with relevant information.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

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

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

lib/bindings/python/tests/test_http_error.py (1)

4-5: Pipeline failure: isort hook modified imports.

The pre-commit isort hook failed, indicating import ordering was corrected automatically. Please commit the isort changes.

🧹 Nitpick comments (3)

lib/bindings/python/rust/http.rs (1)

162-164: Outdated comment: refers to duck typing but code now uses typed extraction.

The comment describes the old approach. Consider updating it to reflect the current typed extraction via FromPyObject.

Suggested fix

-                        // With the Stable ABI, we can't subclass Python's built-in exceptions in PyO3, so instead we
-                        // implement the exception in Python and use duck typing / assume that it's an HttpError if
-                        // the code and message are present.
+                        // With the Stable ABI, we can't subclass Python's built-in exceptions in PyO3, so we
+                        // implement the exception in Python and extract the code and message fields directly.

lib/bindings/python/src/dynamo/llm/exceptions.py (2)

11-14: Consider explicit bool rejection.

isinstance(True, int) returns True in Python since bool is a subclass of int. This means HttpError(True, "msg") would pass validation with code=1. If this is unintended, add an explicit check.

Optional fix if bool should be rejected

-        if not isinstance(code, int):
+        if not isinstance(code, int) or isinstance(code, bool):
             raise ValueError("HttpError status code must be an integer")

---

22-26: Prefer logging.warning() over print() for library code.

Using print() in library code can interfere with application output and isn't configurable. Consider using the logging module instead.

Suggested fix

+import logging
+
+logger = logging.getLogger(__name__)
+
 _MAX_MESSAGE_LENGTH = 8192

         if len(message) > _MAX_MESSAGE_LENGTH:
-            print(
+            logger.warning(
                 f"HttpError message length {len(message)} exceeds max length {_MAX_MESSAGE_LENGTH}, truncating..."
             )
             message = message[: (_MAX_MESSAGE_LENGTH - 3)] + "..."

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 676ce43 and 616b10b.

📒 Files selected for processing (3)

• lib/bindings/python/rust/http.rs (2 hunks)
• lib/bindings/python/src/dynamo/llm/exceptions.py (1 hunks)
• lib/bindings/python/tests/test_http_error.py (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: PeaBrane
Repo: ai-dynamo/dynamo PR: 3155
File: components/backends/vllm/src/dynamo/vllm/main.py:228-233
Timestamp: 2025-09-21T01:40:52.456Z
Learning: In the dynamo codebase, error handling for distributed runtime client initialization (like runtime.namespace().component().endpoint().client()) is handled at the Rust level in the distributed runtime bindings, so Python-level try/catch blocks are not needed and would be redundant.

📚 Learning: 2025-08-21T17:23:02.836Z

Learnt from: michaelfeil
Repo: ai-dynamo/dynamo PR: 2591
File: lib/bindings/python/rust/http.rs:0-0
Timestamp: 2025-08-21T17:23:02.836Z
Learning: In lib/bindings/python/rust/http.rs, the enable_endpoint method uses EndpointType::all() to dynamically support all available endpoint types with case-insensitive matching, which is more maintainable than hardcoded match statements for endpoint type mappings.

Applied to files:

• lib/bindings/python/rust/http.rs

📚 Learning: 2025-06-13T22:07:24.843Z

Learnt from: kthui
Repo: ai-dynamo/dynamo 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.

Applied to files:

• lib/bindings/python/rust/http.rs

🧬 Code graph analysis (2)

lib/bindings/python/rust/http.rs (1)

lib/bindings/python/src/dynamo/llm/exceptions.py (1)

• HttpError (9-31)

lib/bindings/python/tests/test_http_error.py (1)

lib/bindings/python/src/dynamo/llm/exceptions.py (1)

• HttpError (9-31)

🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/5020/merge) by siclait.

lib/bindings/python/tests/test_http_error.py

[error] 1-1: isort hook failed: files were modified by this hook. The pre-commit hook updated the file to satisfy import sorting.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

• GitHub Check: clippy (lib/bindings/python)
• GitHub Check: tests (lib/runtime/examples)
• GitHub Check: tests (.)
• GitHub Check: clippy (launch/dynamo-run)
• GitHub Check: tests (launch/dynamo-run)
• GitHub Check: tests (lib/bindings/python)
• GitHub Check: clippy (.)
• GitHub Check: Build and Test - dynamo

🔇 Additional comments (6)

lib/bindings/python/rust/http.rs (2)

141-145: LGTM!

The HttpError struct with FromPyObject derive cleanly replaces the previous duck-typing approach. The field types correctly match the Python-side HttpError attributes.

---

165-171: LGTM!

The typed extraction via FromPyObject is cleaner and more robust than the previous attribute-by-attribute extraction. The newline/carriage return sanitization for SSE compatibility is correctly preserved.

lib/bindings/python/tests/test_http_error.py (2)

22-25: LGTM!

Good test case validating that non-string message types raise ValueError. Using bytes as the invalid type is a sensible edge case since it's a common confusion with str.

---

28-35: LGTM!

The truncation test correctly validates:

• Message length is capped at 8192 characters
• The truncated content (first 8189 chars) is present in the string representation
• The trailing character "B" is excluded

lib/bindings/python/src/dynamo/llm/exceptions.py (2)

6-6: LGTM!

Good choice defining the max length as a module-level constant for maintainability. The 8KB limit is reasonable for error messages.

---

10-31: Overall implementation looks solid.

The validation and truncation logic correctly addresses the PR objective of preventing ValueError from omitting the original message. The truncation preserves context while staying within bounds.

[rmccorm4]

/ok to test e88b6c8

[itay]

We definitely should return better error messages, but I am actually curious what is the case where we create an HttpError that is >8K in size?

[rmccorm4]

Syncing with latest main to pull in this commit to fix vllm arm failure: siclait@676ce43

[rmccorm4]

/ok to test c0d460d

[siclait]

We definitely should return better error messages, but I am actually curious what is the case where we create an HttpError that is >8K in size?

That's a good question. I just saw the ValueError — I'll find out what the cause was.

[kthui]

LGTM! Thanks for improving the error message handling.

[rmccorm4]

@siclait I'm going to merge this to avoid it getting stale or have merge conflicts over the holidays since it seems like a net win regardless, but please do follow up on the use case / scenario as it would be good to get to the bottom of as well:

We definitely should return better error messages, but I am actually curious what is the case where we create an HttpError that is >8K in size?

That's a good question. I just saw the ValueError — I'll find out what the cause was.