fix: remove hardcoded 180s timeout from REVIEW_FILTER_TASK#474
Conversation
|
✅ OpenCodeReview: No comments generated. Looks good to me. |
The filter task had a hidden inner timeout (ft.Timeout) that always fired before the global OCR_LLM_TIMEOUT (300s) could apply. Removing it lets the task flow through the same two timeout knobs (OCR_LLM_TIMEOUT / --timeout) as every other task type. Also cleaned up unused timeout fields from task_template.json that were never read in production code.
1ec696b to
9989062
Compare
lizhengfeng101
left a comment
There was a problem hiding this comment.
Review
The core change is correct and valuable — removing the inner 180s timeout that raced against the global timeout makes behavior consistent and fixes the reported issue for local LLM users.
However, the cleanup is incomplete:
1. Dead code in internal/diff/relocation.go:33-37
RE_LOCATION_TASK's timeout was also removed from the JSON, but relocation.go still has the identical pattern:
if task.Timeout > 0 {
var cancel context.CancelFunc
ctx, cancel = context.WithTimeout(ctx, time.Duration(task.Timeout)*time.Second)
defer cancel()
}Since all task timeouts are now removed from the template JSON, this is dead code and should be cleaned up for consistency with the agent.go change.
2. (Optional) Struct field residue in internal/config/template/template.go
The Timeout int fields on TaskTemplate (line 57) and LlmConversation (line 217), plus the assignment at line 73, are now effectively unused. If the intent is to fully remove per-task timeouts, these could be cleaned up too — though this can be a follow-up PR.
If the design intent is to preserve the ability for users to set per-task timeouts via custom template files, then the relocation.go code should stay and this should be clarified in the PR description.
TL;DR: Please also remove the same timeout block in relocation.go to keep the codebase consistent. Everything else looks good.
5deebfd to
6c9c839
Compare
|
Thanks for the thorough review, @lizhengfeng101! I've pushed an additional commit that removes the same timeout block from internal/diff/relocation.go as you suggested. The dead code cleanup is done while keeping the Timeout struct fields intact for anyone using custom template files. Let me know if anything else needs tweaking! |
What I changed
I removed the hidden 180s inner timeout on
REVIEW_FILTER_TASKthat wascausing
context deadline exceedederrors for people using local LLMs.Now it uses the same global timeout settings (
OCR_LLM_TIMEOUT/--timeout)as every other task — no more, no less.
Files I touched
internal/agent/agent.goDeleted the
if ft.Timeout > 0 { ... }block inexecuteReviewFilter.The filter task was the only task wrapping its LLM call in an extra
context.WithTimeout— and that 180s deadline was smaller than the300s global timeout, so it always fired first.
internal/config/template/task_template.jsonRemoved
"timeout"from all five tasks. They were dead code — only thefilter task's timeout was ever read in production, and now that's gone too.
internal/config/template/template_test.goUpdated the test to expect
Timeout == 0instead of120.Tests
ok github.com/open-code-review/open-code-review/internal/agent 4.18s
ok github.com/open-code-review/open-code-review/internal/config/template 0.61s
ok github.com/open-code-review/open-code-review/internal/diff 11.76s