fix: Add API Timeout - #972
Conversation
This adds an environment variable named "LLM_TIMEOUT" which defaults to "60" (the default of gin.default()). Setting this variable increases the timeout to the API endpoint "generate-suggestions". Increasing this timeout helps with larger texts or slower models, which do not generate an answer within 60s. fixes: icereed#459 fixes: icereed#454 Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
📝 WalkthroughWalkthroughThis PR introduces configurable request timeouts for LLM API calls. Go dependencies are updated (including the new gin-contrib/timeout middleware), the /api/generate-suggestions endpoint is wrapped with timeout enforcement, LLM_TIMEOUT environment variable is parsed during startup, and documentation and troubleshooting guidance are added. ChangesLLM Request Timeout Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
101-119:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpgrade vulnerable transitive modules before merging.
google.golang.org/grpc v1.77.0contains a critical authorization bypass vulnerability (GHSA-p77j-4mvh-x3m3 / CVE-2026-33186) andgo.opentelemetry.io/otel v1.38.0has a remote DoS amplification issue (GHSA-mh2q-q3fh-2475 / CVE-2026-29181). Bump both to patched versions directly or viareplacedirectives while dependencies are being updated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go.mod` around lines 101 - 119, Update the vulnerable transitive modules by bumping google.golang.org/grpc (currently listed as v1.77.0) and go.opentelemetry.io/otel (currently v1.38.0) to their patched releases or add go.mod replace directives pointing to the fixed versions; after updating the versions for google.golang.org/grpc and go.opentelemetry.io/otel, run module resolution (go get/update and go mod tidy) to ensure the dependency graph is re-resolved and the patched transitive versions are used.
🧹 Nitpick comments (1)
main.go (1)
369-371: ⚡ Quick winConsider returning a structured timeout payload for this endpoint.
/api/generate-suggestionsis UI-facing; addingtimeout.WithResponse(...)with a JSON error body will keep client error handling consistent on 408 responses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.go` around lines 369 - 371, The POST route registering "/generate-suggestions" currently uses timeout.New(timeout.WithTimeout(...)) and should also provide a structured JSON timeout response so the UI gets a consistent 408 payload; update the registration to include timeout.WithResponse(...) (alongside timeout.WithTimeout) that returns a 408 status and a JSON body like {"error":"request timeout","code":"timeout","message":"LLM request timed out"} and sets Content-Type application/json so app.generateSuggestionsHandler consumers receive a consistent error shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.go`:
- Around line 693-700: The current LLM_TIMEOUT parsing silently ignores
non-integer values; update the block that reads os.Getenv("LLM_TIMEOUT") and the
strconv.Atoi(...) check so that when err != nil you either log a clear
error/warning (using log.Warnf or log.Fatalf) including the raw env value and
the parse error, or fail fast; ensure the message references the environment
variable name and the offending value and keep the existing non-negative check
and llmTimeout assignment in the branches where parsing succeeds.
In `@README.md`:
- Line 549: Update the README table row for the LLM_TIMEOUT setting: change its
type/description to state "non-negative integer (seconds)" to match runtime
validation and ensure the default-value cell displays `60` with normal table
spacing (not padded or misaligned). Locate the row referencing LLM_TIMEOUT in
the config table and replace the description and default cell formatting so the
entry reads: LLM_TIMEOUT — non-negative integer (seconds) — No — `60`.
---
Outside diff comments:
In `@go.mod`:
- Around line 101-119: Update the vulnerable transitive modules by bumping
google.golang.org/grpc (currently listed as v1.77.0) and
go.opentelemetry.io/otel (currently v1.38.0) to their patched releases or add
go.mod replace directives pointing to the fixed versions; after updating the
versions for google.golang.org/grpc and go.opentelemetry.io/otel, run module
resolution (go get/update and go mod tidy) to ensure the dependency graph is
re-resolved and the patched transitive versions are used.
---
Nitpick comments:
In `@main.go`:
- Around line 369-371: The POST route registering "/generate-suggestions"
currently uses timeout.New(timeout.WithTimeout(...)) and should also provide a
structured JSON timeout response so the UI gets a consistent 408 payload; update
the registration to include timeout.WithResponse(...) (alongside
timeout.WithTimeout) that returns a 408 status and a JSON body like
{"error":"request timeout","code":"timeout","message":"LLM request timed out"}
and sets Content-Type application/json so app.generateSuggestionsHandler
consumers receive a consistent error shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02cb38e6-c0a2-40a3-9efb-8bb288aac91a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
README.mdgo.modmain.go
| if timeout := os.Getenv("LLM_TIMEOUT"); timeout != "" { | ||
| if parsed, err := strconv.Atoi(timeout); err == nil { | ||
| if parsed < 0 { | ||
| log.Fatalf("LLM_TIMEOUT must be non-negative, got: %d", parsed) | ||
| } | ||
| llmTimeout = parsed | ||
| log.Infof("Using LLM timeout: %d", llmTimeout) | ||
| } |
There was a problem hiding this comment.
Fail fast (or at least warn) on non-integer LLM_TIMEOUT values.
If LLM_TIMEOUT is set to a non-numeric value, the code currently ignores it silently and keeps the default. That makes timeout behavior hard to diagnose in production.
Suggested fix
if timeout := os.Getenv("LLM_TIMEOUT"); timeout != "" {
- if parsed, err := strconv.Atoi(timeout); err == nil {
+ if parsed, err := strconv.Atoi(timeout); err == nil {
if parsed < 0 {
log.Fatalf("LLM_TIMEOUT must be non-negative, got: %d", parsed)
}
llmTimeout = parsed
log.Infof("Using LLM timeout: %d", llmTimeout)
+ } else {
+ log.Fatalf("Invalid LLM_TIMEOUT value %q: must be a non-negative integer", timeout)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if timeout := os.Getenv("LLM_TIMEOUT"); timeout != "" { | |
| if parsed, err := strconv.Atoi(timeout); err == nil { | |
| if parsed < 0 { | |
| log.Fatalf("LLM_TIMEOUT must be non-negative, got: %d", parsed) | |
| } | |
| llmTimeout = parsed | |
| log.Infof("Using LLM timeout: %d", llmTimeout) | |
| } | |
| if timeout := os.Getenv("LLM_TIMEOUT"); timeout != "" { | |
| if parsed, err := strconv.Atoi(timeout); err == nil { | |
| if parsed < 0 { | |
| log.Fatalf("LLM_TIMEOUT must be non-negative, got: %d", parsed) | |
| } | |
| llmTimeout = parsed | |
| log.Infof("Using LLM timeout: %d", llmTimeout) | |
| } else { | |
| log.Fatalf("Invalid LLM_TIMEOUT value %q: must be a non-negative integer", timeout) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.go` around lines 693 - 700, The current LLM_TIMEOUT parsing silently
ignores non-integer values; update the block that reads os.Getenv("LLM_TIMEOUT")
and the strconv.Atoi(...) check so that when err != nil you either log a clear
error/warning (using log.Warnf or log.Fatalf) including the raw env value and
the parse error, or fail fast; ensure the message references the environment
variable name and the offending value and keep the existing non-negative check
and llmTimeout assignment in the branches where parsing succeeds.
| | `AUTO_TAG` | Tag for auto processing. | No | paperless-gpt-auto | | ||
| | `LLM_PROVIDER` | AI backend (`openai`, `ollama`, `googleai`, `mistral`, or `anthropic`). | Yes | | | ||
| | `LLM_MODEL` | AI model name (e.g., `gpt-4o`, `mistral-large-latest`, `qwen3:8b`, `claude-sonnet-4-5`). | Yes | | | ||
| | `LLM_TIMEOUT` | timeout to receive an answer from the model in seconds. | No |60 | |
There was a problem hiding this comment.
Align LLM_TIMEOUT docs with runtime validation and fix table cell formatting.
Please document it as a non-negative integer (seconds) and format the default cell as 60 (with normal table spacing) to avoid ambiguity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 549, Update the README table row for the LLM_TIMEOUT
setting: change its type/description to state "non-negative integer (seconds)"
to match runtime validation and ensure the default-value cell displays `60` with
normal table spacing (not padded or misaligned). Locate the row referencing
LLM_TIMEOUT in the config table and replace the description and default cell
formatting so the entry reads: LLM_TIMEOUT — non-negative integer (seconds) — No
— `60`.
icereed
left a comment
icereed
left a comment
There was a problem hiding this comment.
🤖 Automated review — produced by Claude Code running in the maintainer's repo checkout and posted under the maintainer's account, not hand-written by them.
Thanks for this — the problem is real (#937, #1016 and #980 are all the same unbounded-LLM-call failure), but the fix is at the wrong layer and I'd rather not merge it in this shape.
It bounds the wrong thing. timeout.New(...) on the gin route ends the HTTP request after LLM_TIMEOUT. The LLM call itself keeps running, so the failure this is meant to fix — the polling loop wedged behind one stalled generation, recoverable only by restarting the container (#1016) — still happens. The user just gets an error page while the worker stays stuck.
The 60s default would break working setups. Local models routinely exceed it: #1043 measures a correspondent step legitimately taking minutes on a CPU-bound Ollama model with a large correspondent list. Shipping 60s as the default would turn slow-but-working installs into failing ones.
It's also gone stale. POST /generate-suggestions was replaced by the job-based flow (POST /api/jobs/suggestions plus polling) in the #1005 rework, so the wrapped route no longer carries suggestion generation. A request timeout is doubly wrong against that design: the whole point of the job flow is that generation outlives the request.
Where the timeout belongs is the HTTP client used for LLM calls. #998 does exactly that: a per-request timeout on the client, 300s default, <= 0 to disable, wrapping the existing header client so OLLAMA_HEADERS keeps working, with tests. That's the approach that will land.
Two things from your PR that are worth keeping regardless, and I'd welcome as a separate change:
- The troubleshooting line mapping
rate limiter wait failed: context canceledto a timeout cause. That error message is genuinely baffling and the mapping saves people real time. - The point that this needs to be configurable per deployment, not a fixed constant.
Marking as request-changes rather than closing, in case you'd like to redirect it at the client layer — but if #998 lands first this becomes redundant and can be closed.
2 participants
This adds an environment variable named "LLM_TIMEOUT" which defaults to "60" (the default of gin.default()).
Setting this variable increases the timeout to the API endpoint "generate-suggestions".
Increasing this timeout helps with larger texts or slower models, which do not generate an answer within 60s.
fixes: #459
fixes: #454
Summary by CodeRabbit
Release Notes
New Features
Documentation