Skip to content

fix: improve SSE parsing error visibility and propagation#28

Closed
echobt wants to merge 2 commits intomainfrom
fix/sse-error-propagation
Closed

fix: improve SSE parsing error visibility and propagation#28
echobt wants to merge 2 commits intomainfrom
fix/sse-error-propagation

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

This PR improves error handling for SSE (Server-Sent Events) parsing in the Cortex client. Previously, parsing errors were logged at debug level and silently ignored. Now, these errors are logged at warning level for visibility and propagated to callers.

Changes

  • Upgrade SSE parsing error logs from debug to warning level
  • Include data preview in error messages for debugging (truncated to 100 chars)
  • Send ResponseEvent::Error to callers so they can handle parsing failures
  • Use structured logging with named fields for better observability

Verification

cargo check -p cortex-engine
cargo clippy -p cortex-engine

Note: There are pre-existing compilation errors in batch.rs and unified_executor.rs (missing tool_timeout_secs field) that are unrelated to this PR.

@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR implements two improvements to the Cortex codebase: enhanced SSE parsing error handling and centralized timeout constants.

SSE Error Handling

  • SSE parsing errors are now logged at warning level instead of debug level for better visibility
  • Parsing failures now propagate ResponseEvent::Error to callers instead of being silently ignored
  • Error messages include a 100-character data preview for debugging purposes
  • Uses structured logging with named fields (error, data_preview)

Timeout Centralization

  • Created new cortex-common/timeout.rs module with well-documented timeout constants
  • Consolidated timeout values from multiple locations (cortex-exec/runner.rs, cortex-engine/tools/handlers/batch.rs)
  • Added comprehensive unit tests to validate timeout relationships
  • Updated all dependent modules to use centralized constants

Impact

  • Improved error visibility and debugging capabilities for SSE parsing failures
  • Better code maintainability through DRY principle for timeout values
  • No breaking changes to existing functionality

Confidence Score: 5/5

  • This PR is safe to merge with no identified issues
  • Both changes are well-implemented refactorings. The SSE error handling improvement adds proper error propagation without breaking existing behavior, and the timeout centralization follows best practices with comprehensive documentation and tests. All changes maintain backward compatibility.
  • No files require special attention

Important Files Changed

Filename Overview
src/cortex-engine/src/client/cortex.rs Upgraded SSE parsing error logging from debug to warning level and propagated errors to callers
src/cortex-common/src/timeout.rs New module centralizing timeout constants with comprehensive documentation and tests
src/cortex-engine/src/tools/handlers/batch.rs Removed local constant in favor of centralized timeout value from cortex-common
src/cortex-exec/src/runner.rs Replaced local timeout constants with centralized values from cortex-common

Sequence Diagram

sequenceDiagram
    participant Client as CortexClient
    participant SSE as SSE Stream
    participant Parser as JSON Parser
    participant Channel as Response Channel
    participant Caller as Caller/Consumer

    SSE->>Client: SSE Event (data)
    Client->>Parser: serde_json::from_str(event.data)
    
    alt Parsing Success
        Parser-->>Client: Ok(CortexResponseEvent)
        Client->>Client: Match event type
        Client->>Client: Create ResponseEvent
        Client->>Channel: tx.send(Ok(ResponseEvent))
        Channel-->>Caller: Deliver event
    else Parsing Failure
        Parser-->>Client: Err(e)
        Client->>Client: Create data_preview (first 100 chars)
        Client->>Client: tracing::warn!(error, data_preview)
        Client->>Client: Format error message
        Client->>Channel: tx.send(Ok(ResponseEvent::Error))
        Channel-->>Caller: Deliver parsing error
    end
    
    Note over Client,Caller: Before: Errors logged at debug level and ignored<br/>After: Errors logged at warning level and propagated
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Closing as part of PR consolidation. This fix for SSE error handling has been consolidated into PR #73 (consolidated error handling improvements). See #73 for the comprehensive error handling changes.

@echobt echobt closed this Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant