Skip to content

docs(http_client): add comprehensive timeout hierarchy documentation#86

Closed
echobt wants to merge 1 commit intomainfrom
docs/timeout-constant-naming
Closed

docs(http_client): add comprehensive timeout hierarchy documentation#86
echobt wants to merge 1 commit intomainfrom
docs/timeout-constant-naming

Conversation

@echobt
Copy link
Contributor

@echobt echobt commented Feb 4, 2026

Summary

Adds comprehensive documentation to the HTTP client timeout constants establishing naming conventions and hierarchy for timeout configuration across the codebase.

Problem

Timeout constants are scattered across multiple modules with inconsistent naming:

  • Some use TIMEOUT, others use TIMEOUT_SECS
  • Some use Duration type, others use u64
  • Lack of central documentation about what each timeout controls

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

Testing

  • Verified code compiles with cargo check -p cortex-common

Verification

cargo check -p cortex-common

@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR adds comprehensive documentation to the HTTP client timeout constants in src/cortex-common/src/http_client.rs. The changes are purely documentation improvements with no functional code changes.

Key Improvements:

  • Added a detailed header section explaining the timeout hierarchy and categories (request, connection, read, pool timeouts)
  • Documented naming conventions: constants use SCREAMING_SNAKE_CASE with Duration type, config fields use snake_case with _secs suffix for u64 values
  • Enhanced each timeout constant's documentation with clearer descriptions of their purpose and use cases
  • Positioned this module as the authoritative source for timeout configuration across the codebase

The documentation correctly describes the existing timeout values and their purposes. The PR addresses a legitimate maintenance concern about scattered timeout constants across the codebase (verified in files like api_client.rs and login/server.rs).

Confidence Score: 5/5

  • This PR is completely safe to merge with zero risk
  • Only documentation changes with no code modifications - improves maintainability and establishes clear conventions
  • No files require special attention

Important Files Changed

Filename Overview
src/cortex-common/src/http_client.rs Added comprehensive timeout hierarchy documentation with clear naming conventions and usage guidelines

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant Module as Other Module
    participant HC as http_client.rs
    participant Client as HTTP Client

    Note over HC: Timeout Constants Defined
    Note over HC: DEFAULT_TIMEOUT (30s)
    Note over HC: STREAMING_TIMEOUT (5m)
    Note over HC: HEALTH_CHECK_TIMEOUT (5s)
    Note over HC: POOL_IDLE_TIMEOUT (60s)

    Dev->>Module: Implement HTTP request
    Module->>HC: Import timeout constant
    HC-->>Module: Provides Duration constant
    Module->>HC: Call create_*_client()
    HC->>Client: Configure with timeout
    Client-->>Module: Returns configured client
    Module->>Client: Make HTTP request
    Client-->>Module: Response (within timeout)
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.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@echobt
Copy link
Contributor Author

echobt commented Feb 4, 2026

Closing to consolidate: This timeout documentation will be merged with PR #49 into a consolidated documentation PR for timeout configuration.

@echobt echobt closed this Feb 4, 2026
echobt added a commit that referenced this pull request Feb 4, 2026
## 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
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