Skip to content

docs: consolidated timeout configuration documentation#89

Closed
echobt wants to merge 1 commit intomainfrom
consolidated/timeout-documentation
Closed

docs: consolidated timeout configuration documentation#89
echobt wants to merge 1 commit intomainfrom
consolidated/timeout-documentation

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

This PR consolidates 2 documentation PRs for timeout configuration.

Included PRs:

Key Changes:

  • Added detailed header documentation explaining the timeout hierarchy
  • Documented each timeout constant with its purpose and use case
  • Established naming conventions for constants vs config fields
  • Added timeout hierarchy table for quick reference

Timeout Hierarchy

Use Case Module Constant Value Rationale
Health checks cortex-common/http_client HEALTH_CHECK_TIMEOUT 5s Quick validation
Standard HTTP requests cortex-common/http_client DEFAULT_TIMEOUT 30s Normal API calls
Connection pool idle cortex-common/http_client POOL_IDLE_TIMEOUT 60s DNS refresh
LLM Request (non-streaming) cortex-exec/runner DEFAULT_REQUEST_TIMEOUT_SECS 120s Model inference
LLM Streaming total cortex-common/http_client STREAMING_TIMEOUT 300s Long-running streams
Server request lifecycle cortex-app-server/config request_timeout 300s Full request handling
Per-chunk reads cortex-app-server/config read_timeout 30s Chunk timeout
Graceful shutdown cortex-app-server/config shutdown_timeout 30s Time for cleanup
Entire execution cortex-exec/runner DEFAULT_TIMEOUT_SECS 600s Headless exec limit

Files Modified:

  • src/cortex-common/src/http_client.rs
  • src/cortex-app-server/src/config.rs
  • src/cortex-exec/src/runner.rs

Closes #49, #86

## Summary

This PR consolidates **2 documentation PRs** for timeout configuration.

### Included PRs:
- #49: Add comprehensive timeout configuration documentation
- #86: Add comprehensive timeout hierarchy documentation to HTTP client

### Key Changes:
- Added detailed header documentation explaining the timeout hierarchy
- Documented each timeout constant with its purpose and use case
- Established naming conventions for constants vs config fields
- Added timeout hierarchy table for quick reference

### Timeout Hierarchy

| Use Case                    | Module                      | Constant                    | Value | Rationale |
|-----------------------------|-----------------------------|-----------------------------|-------|-----------|
| Health checks               | cortex-common/http_client   | HEALTH_CHECK_TIMEOUT        | 5s    | Quick validation |
| Standard HTTP requests      | cortex-common/http_client   | DEFAULT_TIMEOUT             | 30s   | Normal API calls |
| Connection pool idle        | cortex-common/http_client   | POOL_IDLE_TIMEOUT           | 60s   | DNS refresh |
| LLM Request (non-streaming) | cortex-exec/runner          | DEFAULT_REQUEST_TIMEOUT_SECS| 120s  | Model inference |
| LLM Streaming total         | cortex-common/http_client   | STREAMING_TIMEOUT           | 300s  | Long-running streams |
| Server request lifecycle    | cortex-app-server/config    | request_timeout             | 300s  | Full request handling |
| Per-chunk reads             | cortex-app-server/config    | read_timeout                | 30s   | Chunk timeout |
| Graceful shutdown           | cortex-app-server/config    | shutdown_timeout            | 30s   | Time for cleanup |
| Entire execution            | cortex-exec/runner          | DEFAULT_TIMEOUT_SECS        | 600s  | Headless exec limit |

### Files Modified:
- src/cortex-common/src/http_client.rs
- src/cortex-app-server/src/config.rs
- src/cortex-exec/src/runner.rs

Closes #49, #86
@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

Consolidated timeout configuration documentation across three core modules, establishing cortex-common/http_client as the authoritative source for timeout hierarchy documentation.

Key Improvements:

  • Added comprehensive timeout hierarchy table in http_client.rs documenting all timeout values (5s to 600s) with their use cases and rationales
  • Established clear naming conventions: SCREAMING_SNAKE_CASE for constants vs snake_case_secs for config fields
  • Added cross-references from config.rs and runner.rs pointing developers to centralized documentation
  • Documented timeout categories (request, connection, read, pool) with best practices
  • Enhanced individual constant documentation with clear explanations of purpose

Documentation Quality:
The documentation is thorough, well-organized, and provides practical guidance for developers adding new timeout configurations. The hierarchy table makes it easy to understand timeout relationships and ensure inner timeouts remain shorter than outer ones.

Confidence Score: 5/5

  • This PR is completely safe to merge - it only adds documentation without modifying any code logic
  • Documentation-only changes with no code modifications. The added documentation is accurate, comprehensive, and properly cross-referenced. All timeout values in the documentation match the actual constant definitions in the code.
  • No files require special attention

Important Files Changed

Filename Overview
src/cortex-common/src/http_client.rs Added comprehensive timeout hierarchy documentation including tables, module-specific timeouts, and recommendations
src/cortex-app-server/src/config.rs Added cross-references to centralized timeout documentation in http_client module
src/cortex-exec/src/runner.rs Enhanced constant documentation with explanations and cross-references to timeout hierarchy

Sequence Diagram

sequenceDiagram
    participant Client
    participant AppServer as cortex-app-server<br/>(config.rs)
    participant Exec as cortex-exec<br/>(runner.rs)
    participant HTTPClient as cortex-common<br/>(http_client.rs)
    participant LLM as LLM Provider

    Note over Client,LLM: Timeout Hierarchy Flow

    Client->>AppServer: Incoming Request
    activate AppServer
    Note right of AppServer: request_timeout: 300s<br/>(Full request lifecycle)
    Note right of AppServer: read_timeout: 30s<br/>(Per-chunk timeout)

    AppServer->>Exec: Execute Headless Session
    activate Exec
    Note right of Exec: DEFAULT_TIMEOUT_SECS: 600s<br/>(Entire exec session)

    loop Multi-turn Conversation (max 10 turns)
        Exec->>HTTPClient: Create Client for LLM Request
        activate HTTPClient
        Note right of HTTPClient: STREAMING_TIMEOUT: 300s<br/>(LLM streaming)
        
        HTTPClient->>LLM: POST /v1/chat/completions
        Note right of Exec: DEFAULT_REQUEST_TIMEOUT_SECS: 120s<br/>(Single LLM request)
        
        LLM-->>HTTPClient: Stream Response Chunks
        Note right of HTTPClient: POOL_IDLE_TIMEOUT: 60s<br/>(DNS re-resolution)
        
        HTTPClient-->>Exec: Aggregated Response
        deactivate HTTPClient
        
        alt Tool Calls Needed
            Exec->>Exec: Execute Tools
            Note right of Exec: Continue conversation
        else Finish Reason: Stop
            Note right of Exec: Conversation complete
        end
    end

    Exec-->>AppServer: Execution Result
    deactivate Exec

    AppServer-->>Client: HTTP Response
    deactivate AppServer

    Note over Client,LLM: Health Check Flow (Fast Path)
    
    Client->>AppServer: GET /health
    activate AppServer
    AppServer->>HTTPClient: Create Health Check Client
    activate HTTPClient
    Note right of HTTPClient: HEALTH_CHECK_TIMEOUT: 5s<br/>(Quick validation)
    HTTPClient->>LLM: Health Check
    LLM-->>HTTPClient: Status
    HTTPClient-->>AppServer: Result
    deactivate HTTPClient
    AppServer-->>Client: 200 OK
    deactivate AppServer

    Note over AppServer: Graceful Shutdown
    AppServer->>AppServer: SIGTERM Received
    Note right of AppServer: shutdown_timeout: 30s<br/>(Cleanup time)
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.

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Closing this PR to consolidate into a single mega-PR combining all refactoring and documentation changes.

@echobt echobt closed this Feb 4, 2026
echobt added a commit that referenced this pull request Feb 4, 2026
This PR consolidates all refactoring and documentation improvements from PRs #77 and #89.

## Refactoring (from PR #77)

### Timeout Constants
- Created centralized cortex_common::timeout module with documented constants
- Replaced scattered magic numbers with well-documented constants
- Added comprehensive documentation for timeout hierarchy

### Code Quality
- Used LazyLock for static regex initialization
- Extracted subagent timeout values as named constants
- Improved compile-time assertions

## Documentation (from PR #89)

### Timeout Hierarchy Documentation
- Added detailed header documentation explaining timeout hierarchy
- Documented each timeout constant with its purpose and use case
- Established naming conventions for constants vs config fields
- Added timeout hierarchy table for quick reference

### Timeout Reference Table

| Use Case                    | Module                      | Constant                    | Value | Rationale |
|-----------------------------|-----------------------------|-----------------------------|-------|-----------|
| Health checks               | cortex-common/http_client   | HEALTH_CHECK_TIMEOUT        | 5s    | Quick validation |
| Standard HTTP requests      | cortex-common/http_client   | DEFAULT_TIMEOUT             | 30s   | Normal API calls |
| Connection pool idle        | cortex-common/http_client   | POOL_IDLE_TIMEOUT           | 60s   | DNS refresh |
| LLM Request (non-streaming) | cortex-exec/runner          | DEFAULT_REQUEST_TIMEOUT_SECS| 120s  | Model inference |
| LLM Streaming total         | cortex-common/http_client   | STREAMING_TIMEOUT           | 300s  | Long-running streams |

## Files Modified
- src/cortex-common/src/timeout.rs (new)
- src/cortex-common/src/http_client.rs
- src/cortex-app-server/src/config.rs
- src/cortex-exec/src/runner.rs
- Multiple other files

Closes #77, #89
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