feat: consolidated streaming and timeout improvements

[echobt]

Summary

This PR consolidates 3 feature PRs for streaming and timeout improvements.

Included PRs:

• feat: add per-chunk timeout for SSE streaming to prevent hangs #25: Add per-chunk timeout for SSE streaming to prevent hangs
• feat: add early JSON validation for tool call arguments #29: Add early JSON validation for tool call arguments
• feat: add per-tool timeout in batch execution #34: Add per-tool timeout in batch execution

Key Changes:

• Added CHUNK_TIMEOUT_SECS constant for SSE streaming (60s)
• Wrapped SSE event iteration with tokio::time::timeout
• Returns descriptive timeout error when chunks are not received
• Added validate_arguments() method to StreamToolCall
• Added is_valid_complete() helper and complete_tool_call_validated() method
• Added DEFAULT_TOOL_TIMEOUT_SECS constant (60 seconds) for batch execution
• Added tool_timeout_secs field to BatchToolArgs for configuration
• Applied individual timeout to each tool execution in execute_parallel()

Files Modified:

• src/cortex-engine/src/client/cortex.rs
• src/cortex-engine/src/streaming.rs
• src/cortex-engine/src/tools/handlers/batch.rs
• src/cortex-engine/src/tools/unified_executor.rs

Closes#25, #29, #34

[greptile-apps]

Greptile Overview

Greptile Summary

This PR consolidates three streaming and timeout improvements from PRs #25, #29, and #34. The changes add per-chunk timeout for SSE streaming (60s), JSON validation methods for tool call arguments, and per-tool timeout for batch execution (60s).

Key improvements:

• Prevents indefinite hangs in SSE streaming by applying a 60-second timeout per chunk
• Adds validation utilities for tool call arguments (validate_arguments(), is_valid_complete(), complete_tool_call_validated())
• Prevents single tools from blocking batch execution by applying individual 60-second timeouts

Issues found:

• The timeout_secs parameter in batch execution is accepted and documented but no longer functional - it's been replaced with tool_timeout_secs but wasn't removed, which could confuse users
• The new validation methods in streaming.rs are not used anywhere in the codebase yet

Recommendations:

• Remove the unused timeout_secs parameter from BatchToolArgs, the tool definition schema, and documentation, OR implement it as an overall batch timeout that works alongside per-tool timeouts
• Consider adding tests for the new validation methods or deferring their implementation until they're needed

Confidence Score: 3/5

• This PR can be merged with caution after addressing the unused timeout_secs parameter issue
• The SSE streaming timeout and batch per-tool timeout implementations are sound and will improve reliability. However, the timeout_secs parameter in batch execution creates confusion by being documented and accepted but not functional. The validation methods in streaming.rs are unused but don't cause harm. The main issue is the misleading API where users might set timeout_secs expecting it to work.
• Pay close attention to src/cortex-engine/src/tools/handlers/batch.rs - the timeout_secs parameter needs to be either removed or implemented

Important Files Changed

Filename	Overview
src/cortex-engine/src/client/cortex.rs	Added per-chunk timeout (60s) to SSE streaming to prevent indefinite hangs when connections stall
src/cortex-engine/src/streaming.rs	Added validation methods for tool call arguments including validate_arguments(), is_valid_complete(), and complete_tool_call_validated()
src/cortex-engine/src/tools/handlers/batch.rs	Added per-tool timeout (60s) for batch execution to prevent single tools from blocking others; renamed parameter from batch timeout to tool timeout
src/cortex-engine/src/tools/unified_executor.rs	Added extraction of tool_timeout_secs parameter from batch arguments to support individual tool timeouts

Sequence Diagram

sequenceDiagram
participant Client
participant CortexClient
participant SSEStream
participant BatchHandler
participant ToolExecutor
Note over CortexClient,SSEStream: SSE Streaming with Per-Chunk Timeout
Client->>CortexClient: create_completion_stream()
CortexClient->>SSEStream: Start SSE stream
loop For each chunk (max 60s wait)
CortexClient->>SSEStream: timeout(60s, stream.next())
alt Chunk received in time
SSEStream-->>CortexClient: Ok(Some(event))
CortexClient->>Client: Send event
else No chunk within 60s
SSEStream-->>CortexClient: Err(Elapsed)
CortexClient->>Client: BackendError("SSE chunk timeout")
CortexClient->>SSEStream: Close stream
end
end
Note over BatchHandler,ToolExecutor: Batch Execution with Per-Tool Timeout
Client->>BatchHandler: execute(BatchToolArgs)
BatchHandler->>BatchHandler: Validate calls
BatchHandler->>BatchHandler: Set tool_timeout (60s default)
par Execute tools in parallel
BatchHandler->>ToolExecutor: timeout(60s, execute_tool("tool1"))
alt Tool completes in time
ToolExecutor-->>BatchHandler: Ok(ToolResult)
else Tool times out
ToolExecutor-->>BatchHandler: Err(Elapsed)
BatchHandler->>BatchHandler: Create timeout error
end
and
BatchHandler->>ToolExecutor: timeout(60s, execute_tool("tool2"))
alt Tool completes in time
ToolExecutor-->>BatchHandler: Ok(ToolResult)
else Tool times out
ToolExecutor-->>BatchHandler: Err(Elapsed)
BatchHandler->>BatchHandler: Create timeout error
end
end
BatchHandler->>BatchHandler: Aggregate results
BatchHandler-->>Client: BatchResult

Loading

[greptile-apps]

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

The timeout_secs parameter is no longer used but still documented and accepted. This could be confusing for users who expect it to control the overall batch timeout. Either:

1. Remove timeout_secs entirely if it's not needed
2. Or implement it as an overall batch timeout in addition to per-tool timeouts

The original PRs merged here indicate #34 specifically addressed "per-tool timeout in batch execution", so this may be intentional, but the timeout_secs field and documentation should be removed if it's not functional.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/cortex-engine/src/tools/handlers/batch.rs
Line: 343:345
Comment:
The `timeout_secs` parameter is no longer used but still documented and accepted. This could be confusing for users who expect it to control the overall batch timeout. Either:
1. Remove `timeout_secs` entirely if it's not needed
2. Or implement it as an overall batch timeout in addition to per-tool timeouts
The original PRs merged here indicate #34 specifically addressed "per-tool timeout in batch execution", so this may be intentional, but the `timeout_secs` field and documentation should be removed if it's not functional.
How can I resolve this? If you propose a fix, please make it concise.