"LLM not configured" behaves 8 different ways across 27 call sites, and one of them strands a session in extracting forever #287
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found in the LLM trigger audit that produced #278 and #279. Two related problems: an inconsistency that makes the system unpredictable, and one concrete stuck-state bug that falls out of it.
The inconsistency
settings_service.get_llm_configis resolved at 27 call sites across 7 files:tasks/reminder_tasks.pyrouters/campaigns.pyrouters/bot.pytasks/planning_tasks.pyrouters/sessions.pyservices/lore_service.pyrouters/users.pyThere is no shared "resolve or bail" helper, and the
Nonebranch was written independently each time. The result is eight distinct behaviours for the same condition:reminder_tasks.py:563,:648,:747,lore_service.py:790)failed, error stored, bot notified (reminder_tasks.py:2036):3219,:3781,:4106,:4258):3324,:3704)sessions.py:281,campaigns.py:4251,:4293,:5285,:5368,:5414){"skipped": True}(planning_tasks.py:91,:183)lore_service.py:594-595, reached fromcampaigns.py:2745passing an uncheckedllm_cfg)sessions.py:339)A GM who hasn't configured an LLM — or whose endpoint is down — cannot predict what any given feature will do. Some fail loudly, some fail into a stored error, some pretend to succeed. #279 was exactly this class of problem (a silent failure indistinguishable from success) and it survived a full release.
The concrete bug this produces
lore_chunk_extractraisesRuntimeError("LLM endpoint not configured.")(reminder_tasks.py:562-563), which is caught by the blanket handler at:588-591and converted intoself.retry(...)with exponential backoff. Aftermax_retriesthe task dies withMaxRetriesExceededError.But
lore_generation_statuswas set toextractingback at:488, and nothing on the retry-exhaustion path sets it tofailed. So the session sits inextractingindefinitely — the UI shows generation in progress forever, and the GM has no signal that it died. The same shape applies to thematchingphase (:672).Retrying a missing configuration is itself questionable: an unconfigured LLM will still be unconfigured 4, 8, and 16 seconds later. It is a permanent condition being handled as a transient one.
Fix direction
with_llm_config(db)helper returning either the config or a typed "not configured" outcome, so each call site makes an explicit, visible choice rather than an accidental one. Roughly three sensible policies: fail the request (503), fail the job and record it on the domain row, or skip with a warning. Any given site should pick one deliberately.extracting/matching/processingmust have a terminal-failure path that clears it, including retry exhaustion (Celery'son_failurehook, or an explicit try/finally).asyncio.runwrapper →task_session()→db.get→ None-check →get_llm_config→ None-branch → local import → try/except log-and-swallow). Five near-identical skeletons at:3190-3260,:3751-3862,:4060-4192,:4228-4338,:3661-3730. An@llm_taskdecorator would delete most of it and make the not-configured policy uniform by construction. Optional, but it is the thing that would stop this drifting again.Acceptance
extracting/matchingafter its task has permanently failedLabels: backend
Reopening. This was closed on 2026-08-06 when the stuck-state bug shipped in v3.11.3, but the harmonisation this issue is named for was never tracked anywhere — so the trail went cold on the larger half. Picking that up now.
Re-survey against current
main(26d63a5)24 call sites, not 27 — the v3.11.x fixes removed three (
bot.pywent 4 → 2).What v3.11.3 already delivered:
extracting/matching—on_failurerecords a terminal status and refuses to clobber one a later phase already reached.LlmNotConfiguredErrorexists (settings_service.py:25) with a docstring making the permanent-vs-transient distinction explicit, andreminder_tasks.py:627-629excludes it from retry.So the typed error and the no-retry rule are real — but they reach only 3 of the 24 sites (the lore chunk pipeline at
:662,:751,:854). Everything else still hand-rolls itsNonebranch.Current behaviour census:
LlmNotConfiguredError— fail fast, no retryreminder_tasks662/751/854RuntimeError— will burn retriesreminder_tasks:2142,lore_service:886campaigns×5,sessions:282,bot.py×1returnreminder_tasks3367/3931/4256/4409reminder_tasks3474/3854{"skipped": True}planning_tasks91/182lore_service:678viacampaigns:2745bot.py/meta,users.pytest endpointTwo corrections to the original body, from reading the current code:
planning_tasks:75is not unchecked. The check is simply deferred to:91, and it returns{"skipped": True}consistently with:182. Not a defect.bot.py/metaandusers.py's test endpoint are correctly different. They report whether an LLM is configured rather than consuming one;llm.endpoint_url if llm else Noneis the right answer there. These should be documented as deliberate, not harmonised away.The one live defect left
approve_lore_proposal(lore_service.py:667-679) falls back toexisting_body + "\n\n" + new_bodywhenllm_cfg is None— a blind append instead of an LLM merge, with no log and no signal to the GM.campaigns.py:2745reaches it with an unchecked config.Worth noting
get_llm_configreturnsNonefor a malformed endpoint URL too (it swallows thenormalize_service_urlValueError), so a typo'd URL degrades silently by the same path. This is the "indistinguishable from success" class that produced #279 and #285.Scope I'm taking
require_llm_config(db)— resolve or raiseLlmNotConfiguredError.get_llm_configstays for the two sites that genuinely want the optional form.RuntimeErrorraises so they stop burning retries.Deliberately deferring the
@llm_taskdecorator (fix direction 4). It is the right long-term answer and I am not doing it in the same change as the behavioural fixes — it rewrites five task skeletons, and bundling a structural refactor with semantic changes makes any regression much harder to attribute. Worth its own issue once this settles.Harmonisation landed in
0a8910e. CI green. All four acceptance items now met.Acceptance
extracting/matching— v3.11.3, viaon_failure.settings_service's module docstring, with the non-obvious sites annotated inline.test_lore_generation_failure.py.What landed
require_llm_config(db)— resolve or raise the typed, non-retryableLlmNotConfiguredError. The three lore-pipeline sites that hand-rolled the identical raise now share it, so the message lives in one place.The four legitimate policies are documented so a new call site has to pick one deliberately rather than invent a ninth behaviour: require it / record the failure on the domain row / skip with a warning / report configuration status. What is explicitly ruled out is producing a worse result and saying nothing.
Two visibility fixes — the only behavioural changes:
approve_lore_proposalwas degrading silently. With no usable LLM it appended both bodies verbatim instead of merging, giving the GM a visibly worse entry that was indistinguishable from a successful merge. The fallback stays — it beats refusing the approval — but it now logs that it happened.get_llm_configswallowednormalize_service_url'sValueError, so a configured but invalid endpoint reported as "not configured" and sent the GM to a settings page that already had a value in it. Now logged: the reason only, never the URL, which is admin-supplied and may embed credentials.896 backend tests (8 new in
test_llm_config_policy.py), ruff check + format clean on the pinned 0.4.4.Corrections to this issue's census
Both from reading the current code rather than trusting the original write-up:
RuntimeErrorsite was burning retries.process_audiosetsmax_retries=0;generate_lore_proposalscatches the exception and recordsfailedwithout retrying. My earlier comment repeated the claim before verifying it. The typed error is still worth having for uniformity — but it fixed no retry bug, because there was none. Acceptance item 2 was already satisfied.planning_tasks:75is not an unchecked site (deferred to:91, matching:182), andbot.py's/metaand the admin connectivity test are correctly different — they report whether an LLM is configured rather than consuming one, soNoneis data there.Net: one real defect, one diagnostic gap, and the rest consistency work. Less dramatic than the title implies — the headline stuck-state bug was already fixed in v3.11.3.
Remaining, not done
Fix direction 4, the
@llm_taskdecorator — ~15 lines of identical preamble across five task skeletons. The issue marks it optional, and it is the thing that would stop this drifting again, but rewriting five task bodies alongside semantic changes would make any regression much harder to attribute. It is not currently tracked anywhere — worth its own issue, which is how this one's larger half got lost in the first place.Closing on acceptance.
Follow-up filed as #294 — the
@llm_taskdecorator, with the duplication re-measured against0a8910erather than this issue's original line numbers.