feat(settings): make LLM timeout configurable (#776)#783
Conversation
|
hey @Frankli9986 i tested your PR, grate job but in summery:
I was testing these changes on ollama running qwen3:8b model and they wroked fine for me |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
cubic flagged the 1-hour improve/preview timeout as a DoS exposure with no compensating controls (PR srbhr#783, review on 2026-05-28). Add a process- local asyncio.Semaphore that caps concurrent tailoring jobs at 4 and rejects excess callers fast with HTTP 503 instead of letting them queue against the long inner timeout. The cap pairs with the existing timeout to bound resource usage: - Inner LLM timeout (Settings) controls per-stage budget. - 1-hour wait_for is the safety net for legitimately slow local models. - Semaphore (this commit) limits how many of those run concurrently. Excess callers get 503 with a 'try again' message instead of stacking up and exhausting workers. Adds 4 unit tests covering the semaphore's acquire/saturate/recover behaviour. Co-Authored-By: cubic-dev-ai[bot] <175864597+cubic-dev-ai[bot]@users.noreply.github.com>
Frankli9986
force-pushed
the
fix-776-configurable-timeout
branch
from
60379a6 to
2b05a36
Compare
|
Updated this PR — rebased onto latest 1. @rami-shalhoub's review (5/24) — fully integrated, thanks for the careful patch and end-to-end test on
You're credited as 2. @cubic-dev-ai's P1 (5/28) — addressed in a separate commit. The concern was that raising the
Includes 4 unit tests covering acquire / saturate / recover behaviour. @rami-shalhoub feel free to re-test on Ollama whenever you have a moment — the integrated version should behave identically to your patch on the happy path and add a 503 only when 4+ tailorings hit the same instance simultaneously. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ) | ||
| return safe | ||
|
|
||
| FALLBACK_MAX_TOKENS = 4096 |
There was a problem hiding this comment.
CRITICAL: Duplicated function and constant will cause runtime TypeError.
The new get_safe_max_tokens function with the fall_back parameter was added at line 737, but the old version (without fall_back) was not removed and remains at line 783. In Python, the later definition shadows the earlier one. As a result, the call in parser.py:167 (get_safe_max_tokens(model_name, fall_back=8192)) will raise a TypeError: get_safe_max_tokens() got an unexpected keyword argument 'fall_back' at runtime.
Remove the old FALLBACK_MAX_TOKENS = 4096 and the old get_safe_max_tokens function definition (lines 781-825).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Persist empty string on clear so the gpt-5 auto-migration doesn't | ||
| # re-fire on next get_llm_config() call. | ||
| stored["reasoning_effort"] = request.reasoning_effort | ||
| if request.timeout_seconds is not None: |
There was a problem hiding this comment.
WARNING: Users cannot clear the timeout_seconds configuration.
When the frontend clears the timeout input, it sends timeout_seconds: null. The backend check if request.timeout_seconds is not None: evaluates to False, so the old timeout value in stored is never cleared or updated. Once a user sets a timeout, they cannot reset it to the default via the UI.
Unconditionally assign the value (e.g., stored["timeout_seconds"] = request.timeout_seconds) or use Pydantic's model_fields_set to distinguish between "omitted" and "explicitly set to null".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ), | ||
| timeout=240.0, # 4-minute hard limit | ||
| ) | ||
| await asyncio.wait_for(_improve_preview_semaphore.acquire(), timeout=0) |
There was a problem hiding this comment.
SUGGESTION: Use explicit .locked() check for non-blocking semaphore acquire.
While asyncio.wait_for(..., timeout=0) works to simulate a non-blocking acquire, it can be less readable. The more explicit and standard pattern is to check .locked() first.
| await asyncio.wait_for(_improve_preview_semaphore.acquire(), timeout=0) | |
| if _improve_preview_semaphore.locked(): | |
| raise asyncio.TimeoutError() | |
| await _improve_preview_semaphore.acquire() |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| FALLBACK_MAX_TOKENS = 4096 | ||
|
|
||
| def get_safe_max_tokens(model_name: str, requested: int = DEFAULT_JSON_MAX_TOKENS, fall_back:int = FALLBACK_MAX_TOKENS) -> int: |
There was a problem hiding this comment.
SUGGESTION: Missing space in type hint.
There is a missing space after the colon in the fall_back type hint.
| def get_safe_max_tokens(model_name: str, requested: int = DEFAULT_JSON_MAX_TOKENS, fall_back:int = FALLBACK_MAX_TOKENS) -> int: | |
| def get_safe_max_tokens(model_name: str, requested: int = DEFAULT_JSON_MAX_TOKENS, fall_back: int = FALLBACK_MAX_TOKENS) -> int: |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Previous Issues (all resolved ✓)
Files Reviewed (17 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 2b05a36)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2b05a36)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by qwen3.7-plus-20260602 · 1,793,754 tokens |
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (7 files)
|
Frankli9986
force-pushed
the
fix-776-configurable-timeout
branch
from
2b05a36 to
b27f29d
Compare
|
Hi team, I've rebased this PR onto the latest Fixes applied
cubic-dev-ai P1 comment review (comment ID 3314887978) New commit: Please re-review when convenient. |
- Allow users to configure LLM timeout_seconds (30-600s) via settings. - Add improve/preview concurrency semaphore with explicit .locked() guard. - Remove duplicated get_safe_max_tokens definition that caused TypeError. - Allow null timeout_seconds to clear the persisted config value. - Fix fall_back type-hint spacing.
Frankli9986
force-pushed
the
fix-776-configurable-timeout
branch
from
b27f29d to
11f1e2a
Compare
|
Rebased on latest
Regarding the cubic-dev-ai P1 comment about the 1-hour New head: |
|
Quick follow-up: the branch was updated after my earlier push. The current head is
PR is now showing as mergeable. |
|
@kilo-code-bot / maintainers — the review findings from the last pass have been addressed and the branch is now rebased on the latest Specifically:
The PR is currently |
| The response includes resume_preview data but leaves resume_id null. | ||
| """ | ||
| resume = await db.get_resume(request.resume_id) | ||
| resume = db.get_resume(request.resume_id) |
There was a problem hiding this comment.
CRITICAL: All db calls missing await — returns coroutine objects instead of results
All database methods in database.py are async def (e.g. get_resume, update_resume, get_job, list_resumes, create_resume, get_master_resume, create_improvement, get_improvement_by_tailored_resume, update_job). await was removed from every call in this file.
resume = db.get_resume(request.resume_id) returns a coroutine object, not a dict. Since coroutine objects are always truthy, if not resume: will never trigger the 404 — every endpoint silently passes validation and then fails on attribute access (e.g. resume["resume_id"] on a coroutine raises TypeError).
This affects ~30+ db calls across this file. All must be re-prefixed with await.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| cleaned = (request.api_base or "").strip() | ||
| stored["api_base"] = cleaned or None | ||
| if request.api_key is not None: | ||
| stored["api_key"] = request.api_key |
There was a problem hiding this comment.
WARNING: API key stored in plaintext config.json — security regression
The replaced load_config_file() injected decrypted keys and save_config_file() stripped them before writing (per CLAUDE.md: "keys live ONLY in the encrypted api_keys SQLite table"). The new _load_config() does raw json.loads() and _save_config() does raw json.dumps(), bypassing the encryption layer entirely.
Combined with this line (stored["api_key"] = request.api_key), the API key will be written to config.json in plaintext. This contradicts the documented security architecture and exposes secrets on disk.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| except ValidationError: | ||
| canonical = data # not a full resume payload; hash as-is | ||
| normalized = _normalize_payload(canonical) | ||
| """Hash canonicalized improved data for preview/confirm validation.""" |
There was a problem hiding this comment.
WARNING: Removed ResumeData.model_validate() canonicalization — risks preview/confirm hash mismatch (HTTP 400)
The previous implementation ran data through ResumeData.model_validate(data).model_dump() before hashing. This ensured that preview and confirm produced identical hashes even when one path had fields set to schema defaults that the other omitted.
Without canonicalization, if improve/preview hashes the raw dict and improve/confirm hashes the .model_dump() output (which includes defaulted fields), the same resume content can produce different hashes — reopening the "preview hash mismatch" bug the original code specifically fixed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| detail="Confirmation required. Pass confirm=RESET_ALL_DATA in request body.", | ||
| ) | ||
| await db.reset_database() | ||
| db.reset_database() |
There was a problem hiding this comment.
WARNING: db.reset_database() is async def but not awaited — same pattern as the resumes.py db calls.
The method is declared async def reset_database(self) at database.py:749. Without await, the database will never actually be reset — the returned coroutine is silently discarded, and the endpoint returns a success message while no data is deleted.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
2 participants
Closes #776
Summary
Users running local LLMs (e.g. Mistral via Ollama) frequently hit the hard-coded 120s/180s timeout. This PR makes the timeout user-configurable via the Settings UI.
Changes
Backend
LLMConfigRequest/LLMConfigResponse: add optionaltimeout_secondsfield (validated 30–600)routers/config.py: read and persisttimeout_secondsfrom/to config.jsonllm.py:_calculate_timeout()acceptsbase_timeout_override;completionandjsonoperations use it;health_checkkeeps its 30s defaultFrontend
lib/api/config.ts:timeout_secondsadded toLLMConfigandLLMConfigUpdateen,es,ja,pt-BR,zhBehavior
Summary by cubic
Make LLM request timeouts configurable in Settings for slow or local models; applies to both completions and JSON. Tailoring respects this setting, raises outer limits, and caps concurrency to avoid overload. Closes #776.
New Features
timeout_seconds(30–600s;nullclears) inLLMConfig;_calculate_timeout()accepts override; used bycompleteandcomplete_json; defaults (120/180s) when unset; override still scales with token/provider factors; health checks stay 30s.CancelledErrorreturns 499.LLMConfig/LLMConfigUpdate; tailor page loads config and callssetImproveTimeoutMs; resume API caches improve-timeout; Nextexperimental.proxyTimeoutset to 3_600_000.Bug Fixes
get_safe_max_tokensthat caused a TypeError; parser now usesget_safe_max_tokens(..., fall_back=8192).timeout_secondsby sendingnull; minor type-hint cleanup forfall_back.Written for commit 11f1e2a. Summary will update on new commits.