-
Notifications
You must be signed in to change notification settings - Fork 12
Fix: Apply HTTP client timeouts to prevent infinite hangs #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Fix builder.rs:74: Use Client::builder() with batch_timeout_seconds - Fix huggingface.rs:460: Use Client::builder() with 30s timeout - Prevents infinite hangs during LLM API calls and model downloads Resolves timeout bug documented in octocode_timeout_analysis.md
…ixes Enhanced the timeout bug fixes with: - Better error messages using .context() for HTTP client build failures - Documentation comments explaining why builder pattern is required - Clear warnings that Client::new() does not apply timeouts These improvements help prevent future regressions and make the code more maintainable by documenting the critical timeout requirement. Related to the fix for infinite hangs during LLM GraphRAG operations.
Added two documentation files to support PR submission: 1. PR_DESCRIPTION.md - Detailed technical explanation of the bug and fix - Code quality improvements section - Integration test results from rust-daq codebase - Migration guide for developers - Comprehensive checklist 2. TESTING.md - Complete testing procedures (unit, integration, regression) - Manual testing checklist - Real-world test results - Troubleshooting guide - CI/CD recommendations These documents provide all information needed for PR review and merge. Ready for upstream submission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical bug where octocode's GraphRAG indexing process hangs indefinitely during LLM API calls by properly applying HTTP client timeouts. The root cause was using reqwest::Client::new() instead of the builder pattern, preventing configured timeouts from being applied to HTTP requests.
Key Changes
- Applied client-level timeout configuration to GraphRAG LLM API calls using
Client::builder()pattern - Applied 30-second timeout to HuggingFace model downloads to prevent infinite hangs
- Added inline documentation explaining the importance of using the builder pattern for timeouts
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/indexer/graphrag/builder.rs |
Fixes HTTP client creation for LLM API calls by using builder pattern with configured batch_timeout_seconds timeout |
src/embedding/provider/huggingface.rs |
Fixes HTTP client creation for HuggingFace downloads by using builder pattern with 30-second timeout |
TESTING.md |
Adds comprehensive testing documentation for the timeout fixes, including unit tests, integration tests, and real-world test results |
PR_DESCRIPTION.md |
Provides detailed explanation of the bug, root cause analysis, testing results, and migration guide for developers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Run the complete test suite: | ||
|
|
||
| ```bash | ||
| cd /Users/briansquires/octocode |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hardcoded path (/Users/briansquires/octocode) is environment-specific and should be replaced with a generic placeholder like /path/to/octocode or the relative path reference that's already used elsewhere in the document (e.g., line 49 uses /path/to/test/codebase).
| cd /Users/briansquires/octocode | |
| cd /path/to/octocode |
| Three files use `Client::new()` but apply request-level timeouts correctly: | ||
|
|
||
| ```bash | ||
| cd /Users/briansquires/octocode |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hardcoded path (/Users/briansquires/octocode) is environment-specific and should be replaced with a generic placeholder like /path/to/octocode to make the documentation universally applicable.
Fix: Apply HTTP client timeouts to prevent infinite hangs
Summary
Fixes a critical bug where octocode's GraphRAG indexing process hangs indefinitely during LLM API calls. The root cause is that
reqwest::Clientinstances are created usingClient::new()instead of the builder pattern, which prevents the configuredbatch_timeout_secondsfrom being applied.Problem
When using LLM-powered GraphRAG features (
use_llm = true), the indexing process hangs indefinitely at two points:ai_batch_size > 1)The configuration parameter
graphrag.llm.batch_timeout_secondsis loaded but never applied to the HTTP client, causing requests to wait indefinitely when the LLM provider is slow to respond.Root Cause Analysis
Primary Bug (
src/indexer/graphrag/builder.rs:74)Secondary Bug (
src/embedding/provider/huggingface.rs:460)Reproduction
use_llm = trueandai_batch_size > 1octocode indexon any codebasebatch_timeout_secondsexpiresTesting
Before Fix
ai_batch_size = 1only helped description phaseAfter Fix
Impact
This bug made LLM-powered GraphRAG features unusable for any non-trivial codebase. The fix enables:
Related Documentation
Full technical analysis available at: octocode_timeout_analysis.md
Code Quality Improvements
In addition to the core timeout fixes, this PR includes:
Enhanced Error Handling
.context()error messages to both HTTP client build failuresDocumentation Comments
These comments at both fix locations help prevent future regressions.
Code Verification
Client::new()uses verified (3 instances in commands/ use request-level timeouts correctly)Technical Details
Why Client::new() Doesn't Apply Timeouts
The
reqwest::Client::new()convenience method creates a client with default settings that do not include any timeout. To apply a timeout, you must use the builder pattern:Request-Level vs Client-Level Timeouts
This PR uses client-level timeouts. An alternative approach is request-level timeouts:
The codebase uses both patterns appropriately:
src/commands/where each request may need different timeoutsTest Results
Unit Tests
cargo test --all-featuresResults: 93 passed, 3 failed
The 3 failures are unrelated FastEmbed lock acquisition issues:
test_fastembed_provider_creationtest_fastembed_model_validationtest_fastembed_embedding_generationThese failures are environmental (file lock contention in local cache directory), not caused by the timeout fix changes.
Integration Testing
Real-world test on rust-daq codebase (113 files):
Configuration Tested
Migration Guide for Developers
If you're creating new HTTP clients in octocode:
For LLM/API Calls
For File Downloads
Don't Use Client::new()
Unless you have a specific reason to avoid timeouts (rare), always use the builder pattern.
Related Issues
This fix resolves the core issue documented in:
Checklist