fix: Tests now pass with RUST_BACKTRACE set #2647
Conversation
This adds a stack trace to error messages, which was causing the string match to miss. Later we should try to make this an error type, so we don't have to string match.
WalkthroughAdjusted RetryManager::next() stream-termination detection to use err.to_string().starts_with(...), triggering stream recreation on matching errors and logging a warning on failure. Tests initialize logging via dynamo_runtime::logging::init() and some test names were prefixed with underscores. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant RM as RetryManager
participant S as Stream
participant L as Logger
C->>RM: next()
RM->>S: poll/receive chunk
alt chunk received
S-->>RM: data
RM-->>C: yield data
else error from stream
S-->>RM: error e
RM->>RM: if e.to_string().startsWith("Stream ended before generation completed")
alt matches
RM->>RM: recreate stream
alt recreation succeeds
RM-->>C: continue streaming
else recreation fails
RM->>L: warn("Failed to recreate stream")
RM-->>C: propagate error
end
else no match
RM-->>C: propagate error
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
lib/llm/src/migration.rs (8)
101-110: Harden stream-termination detection to survive context-wrapped errorsGood move switching to Display (to_string) so backtraces don’t poison comparisons. To make this resilient if upstream adds context (e.g., anyhow context chain), match across the error chain instead of only the top-level message.
Apply this diff:
- if err.to_string().starts_with(STREAM_ERR_MSG) { + if err.chain().any(|e| e.to_string().starts_with(STREAM_ERR_MSG)) {Optional follow-up: hoist
STREAM_ERR_MSGto a module-levelconstso tests can referencesuper::STREAM_ERR_MSGinstead of duplicating the literal. I can provide a patch if you want.
463-469: Initialize tracing once across tests to avoid global re-init
dynamo_runtime::logging::init()likely installs a global subscriber. Re-initializing per test can be a no-op or panic depending on implementation. Make it idempotent in tests and call it via a small helper.Apply this diff here:
- dynamo_runtime::logging::init(); + init_test_logging();Add this once-only helper inside the
#[cfg(test)] mod tests(outside individual tests):use std::sync::Once; static INIT: Once = Once::new(); fn init_test_logging() { INIT.call_once(|| { // Ignore the result if init can fail when already initialized dynamo_runtime::logging::init(); }); }
495-501: Same: use once-only logging init in this test- dynamo_runtime::logging::init(); + init_test_logging();
528-532: Same: use once-only logging init in this test- dynamo_runtime::logging::init(); + init_test_logging();
565-571: Same: use once-only logging init in this test- dynamo_runtime::logging::init(); + init_test_logging();
587-587: Underscore-prefixed test names still run; consider renaming back or marking ignoredLeading underscores don’t disable
#[tokio::test]. If the intent was to keep them running (and they now pass), prefer consistent names without the underscore. If the intent was to skip, use#[ignore].Rename back:
- async fn _test_retry_manager_ongoing_request_migration_indefinite_failure() { + async fn test_retry_manager_ongoing_request_migration_indefinite_failure() {- async fn _test_retry_manager_ongoing_request_migration_indefinite_failure_stream_error() { + async fn test_retry_manager_ongoing_request_migration_indefinite_failure_stream_error() {Also applies to: 635-635
587-590: Also switch this test to once-only logging init- dynamo_runtime::logging::init(); + init_test_logging();
635-640: Also switch this test to once-only logging init- dynamo_runtime::logging::init(); + init_test_logging();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/llm/src/migration.rs(7 hunks)
⏰ 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). (4)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: Build and Test - dynamo
Thanks! Also fix my check to take into account anyhow error chains.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Signed-off-by: Jason Zhou <jasonzho@jasonzho-mlt.client.nvidia.com>
Signed-off-by: Krishnan Prashanth <kprashanth@nvidia.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
This adds a stack trace to error messages, which was causing the string match to miss.
Later we should try to make this an error type, so we don't have to string match.