rbrooks/Quest-Board

Watch

Fork 0

"LLM not configured" behaves 8 different ways across 27 call sites, and one of them strands a session in `extracting` forever #287

New issue

Closed

opened 2026-08-05 20:33:49 +00:00 by claude-bot · 3 comments

claude-bot commented

2026-08-05 20:33:49 +00:00

Collaborator

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_config is resolved at 27 call sites across 7 files:

File	Sites
`tasks/reminder_tasks.py`	11
`routers/campaigns.py`	6
`routers/bot.py`	4
`tasks/planning_tasks.py`	2
`routers/sessions.py`	2
`services/lore_service.py`	1
`routers/users.py`	1

There is no shared "resolve or bail" helper, and the None branch was written independently each time. The result is eight distinct behaviours for the same condition:

raise → task fails and retries (reminder_tasks.py:563, :648, :747, lore_service.py:790)
raise → session marked failed, error stored, bot notified (reminder_tasks.py:2036)
log + silent return, no state written (:3219, :3781, :4106, :4258)
write the failure onto the domain row (:3324, :3704)
HTTP 503 (sessions.py:281, campaigns.py:4251, :4293, :5285, :5368, :5414)
return {"skipped": True} (planning_tasks.py:91, :183)
silently degrade to non-LLM string concatenation (lore_service.py:594-595, reached from campaigns.py:2745 passing an unchecked llm_cfg)
silently skip the step entirely (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_extract raises RuntimeError("LLM endpoint not configured.") (reminder_tasks.py:562-563), which is caught by the blanket handler at :588-591 and converted into self.retry(...) with exponential backoff. After max_retries the task dies with MaxRetriesExceededError.

But lore_generation_status was set to extracting back at :488, and nothing on the retry-exhaustion path sets it to failed. So the session sits in extracting indefinitely — the UI shows generation in progress forever, and the GM has no signal that it died. The same shape applies to the matching phase (: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

A 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.
Never leave a lifecycle status mid-flight. Any task that sets extracting/matching/processing must have a terminal-failure path that clears it, including retry exhaustion (Celery's on_failure hook, or an explicit try/finally).
Don't retry a permanent condition. Missing configuration should fail fast and say so; only transport errors deserve backoff.
There is also ~15 lines of identical preamble in every Celery LLM task (asyncio.run wrapper → 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_task decorator 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

A session can never be left in extracting/matching after its task has permanently failed
Missing configuration fails fast rather than burning retries
The not-configured policy at each call site is explicit and documented, even if the behaviours legitimately differ
Test covering retry exhaustion leaving a terminal status

Labels: backend

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_config` is resolved at **27 call sites** across 7 files: | File | Sites | |---|---| | `tasks/reminder_tasks.py` | 11 | | `routers/campaigns.py` | 6 | | `routers/bot.py` | 4 | | `tasks/planning_tasks.py` | 2 | | `routers/sessions.py` | 2 | | `services/lore_service.py` | 1 | | `routers/users.py` | 1 | There is no shared "resolve or bail" helper, and the `None` branch was written independently each time. The result is **eight distinct behaviours** for the same condition: 1. raise → task fails and retries (`reminder_tasks.py:563`, `:648`, `:747`, `lore_service.py:790`) 2. raise → session marked `failed`, error stored, bot notified (`reminder_tasks.py:2036`) 3. log + silent return, no state written (`:3219`, `:3781`, `:4106`, `:4258`) 4. write the failure onto the domain row (`:3324`, `:3704`) 5. HTTP 503 (`sessions.py:281`, `campaigns.py:4251`, `:4293`, `:5285`, `:5368`, `:5414`) 6. return `{"skipped": True}` (`planning_tasks.py:91`, `:183`) 7. silently degrade to non-LLM string concatenation (`lore_service.py:594-595`, reached from `campaigns.py:2745` passing an unchecked `llm_cfg`) 8. silently skip the step entirely (`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_extract` raises `RuntimeError("LLM endpoint not configured.")` (`reminder_tasks.py:562-563`), which is caught by the blanket handler at `:588-591` and converted into `self.retry(...)` with exponential backoff. After `max_retries` the task dies with `MaxRetriesExceededError`. But `lore_generation_status` was set to `extracting` back at `:488`, and **nothing on the retry-exhaustion path sets it to `failed`**. So the session sits in `extracting` indefinitely — the UI shows generation in progress forever, and the GM has no signal that it died. The same shape applies to the `matching` phase (`: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 1. **A `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. 2. **Never leave a lifecycle status mid-flight.** Any task that sets `extracting`/`matching`/`processing` must have a terminal-failure path that clears it, including retry exhaustion (Celery's `on_failure` hook, or an explicit try/finally). 3. **Don't retry a permanent condition.** Missing configuration should fail fast and say so; only transport errors deserve backoff. 4. There is also **~15 lines of identical preamble** in every Celery LLM task (`asyncio.run` wrapper → `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_task` decorator 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 - [ ] A session can never be left in `extracting`/`matching` after its task has permanently failed - [ ] Missing configuration fails fast rather than burning retries - [ ] The not-configured policy at each call site is explicit and documented, even if the behaviours legitimately differ - [ ] Test covering retry exhaustion leaving a terminal status Labels: backend

claude-bot added the

backend

bug

labels

2026-08-05 20:34:26 +00:00

rbrooks referenced this issue from a commit

2026-08-06 00:46:34 +00:00

fix(backend): never leave lore generation stranded mid-flight (#287)

rbrooks referenced this issue from a commit

2026-08-06 00:46:35 +00:00

chore(release): v3.11.2

claude-bot referenced this issue from a pull request that will close it,

2026-08-06 00:46:54 +00:00

v3.11.2 — the rest of the LLM trigger audit (#271, #285, #286, #287, #288) #290

claude-bot closed this issue

2026-08-06 00:58:44 +00:00

claude-bot referenced this issue from a commit

2026-08-06 00:58:45 +00:00

Merge PR #290: v3.11.2 — the rest of the LLM trigger audit (#271, #285, #286, #287, #288)

claude-bot referenced this issue

2026-08-06 13:51:37 +00:00

v3.11.3 — wiki entry summaries were generated as JSON (#285) #292

rbrooks referenced this issue from a commit

2026-08-07 03:44:54 +00:00

fix(backend): reject an LLM response truncated at the output cap (#293)

claude-bot referenced this issue

2026-08-07 03:45:21 +00:00

LLM transport: a truncated response (finish_reason=length) is silently repaired into a partial result #293

claude-bot reopened this issue

2026-08-07 05:44:39 +00:00

claude-bot commented

2026-08-07 05:45:02 +00:00

Author

Collaborator

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.py went 4 → 2).

What v3.11.3 already delivered:

A session can never be left in extracting/matching — on_failure records a terminal status and refuses to clobber one a later phase already reached.
Test covering retry exhaustion — present.
LlmNotConfiguredError exists (settings_service.py:25) with a docstring making the permanent-vs-transient distinction explicit, and reminder_tasks.py:627-629 excludes 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 its None branch.

Current behaviour census:

policy	sites	n
`LlmNotConfiguredError` — fail fast, no retry	`reminder_tasks` 662/751/854	3
plain `RuntimeError` — will burn retries	`reminder_tasks:2142`, `lore_service:886`	2
HTTP 503	`campaigns` ×5, `sessions:282`, `bot.py` ×1	7
log + silent `return`	`reminder_tasks` 3367/3931/4256/4409	4
record failure on the domain row	`reminder_tasks` 3474/3854	2
`{"skipped": True}`	`planning_tasks` 91/182	2
silent degradation	`lore_service:678` via `campaigns:2745`	1
reports config status (legitimately different)	`bot.py` `/meta`, `users.py` test endpoint	2

Two corrections to the original body, from reading the current code:

planning_tasks:75 is not unchecked. The check is simply deferred to :91, and it returns {"skipped": True} consistently with :182. Not a defect.
bot.py /meta and users.py's test endpoint are correctly different. They report whether an LLM is configured rather than consuming one; llm.endpoint_url if llm else None is 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 to existing_body + "\n\n" + new_body when llm_cfg is None — a blind append instead of an LLM merge, with no log and no signal to the GM. campaigns.py:2745 reaches it with an unchecked config.

Worth noting get_llm_config returns None for a malformed endpoint URL too (it swallows the normalize_service_url ValueError), 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 raise LlmNotConfiguredError. get_llm_config stays for the two sites that genuinely want the optional form.
Convert the two plain RuntimeError raises so they stop burning retries.
Make the silent degradation visible.
Document the sanctioned policies and which sites use which, so a new call site has to pick one on purpose.
Tests.

Deliberately deferring the @llm_task decorator (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.

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.py` went 4 → 2). What v3.11.3 already delivered: - [x] **A session can never be left in `extracting`/`matching`** — `on_failure` records a terminal status and refuses to clobber one a later phase already reached. - [x] **Test covering retry exhaustion** — present. - [x] `LlmNotConfiguredError` exists (`settings_service.py:25`) with a docstring making the permanent-vs-transient distinction explicit, and `reminder_tasks.py:627-629` excludes 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 its `None` branch. Current behaviour census: | policy | sites | n | |---|---|---| | `LlmNotConfiguredError` — fail fast, no retry | `reminder_tasks` 662/751/854 | 3 | | plain `RuntimeError` — **will burn retries** | `reminder_tasks:2142`, `lore_service:886` | 2 | | HTTP 503 | `campaigns` ×5, `sessions:282`, `bot.py` ×1 | 7 | | log + silent `return` | `reminder_tasks` 3367/3931/4256/4409 | 4 | | record failure on the domain row | `reminder_tasks` 3474/3854 | 2 | | `{"skipped": True}` | `planning_tasks` 91/182 | 2 | | **silent degradation** | `lore_service:678` via `campaigns:2745` | 1 | | reports config status (legitimately different) | `bot.py` `/meta`, `users.py` test endpoint | 2 | Two corrections to the original body, from reading the current code: - **`planning_tasks:75` is not unchecked.** The check is simply deferred to `:91`, and it returns `{"skipped": True}` consistently with `:182`. Not a defect. - **`bot.py` `/meta` and `users.py`'s test endpoint are correctly different.** They *report* whether an LLM is configured rather than consuming one; `llm.endpoint_url if llm else None` is 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 to `existing_body + "\n\n" + new_body` when `llm_cfg is None` — a blind append instead of an LLM merge, with **no log and no signal to the GM**. `campaigns.py:2745` reaches it with an unchecked config. Worth noting `get_llm_config` returns `None` for a *malformed* endpoint URL too (it swallows the `normalize_service_url` `ValueError`), 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 1. `require_llm_config(db)` — resolve or raise `LlmNotConfiguredError`. `get_llm_config` stays for the two sites that genuinely want the optional form. 2. Convert the two plain `RuntimeError` raises so they stop burning retries. 3. Make the silent degradation visible. 4. Document the sanctioned policies and which sites use which, so a new call site has to pick one on purpose. 5. Tests. **Deliberately deferring the `@llm_task` decorator** (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.

rbrooks referenced this issue from a commit

2026-08-07 05:53:48 +00:00

refactor(backend): one way to resolve "is an LLM configured?" (#287)

claude-bot commented

2026-08-07 05:58:06 +00:00

Author

Collaborator

Harmonisation landed in 0a8910e. CI green. All four acceptance items now met.

Acceptance

A session can never be left in extracting/matching — v3.11.3, via on_failure.
Missing configuration fails fast rather than burning retries — see the correction below; this was already true, and is now uniform.
The not-configured policy at each call site is explicit and documented — four policies documented in settings_service's module docstring, with the non-obvious sites annotated inline.
Test covering retry exhaustion leaving a terminal status — v3.11.3, test_lore_generation_failure.py.

What landed

require_llm_config(db) — resolve or raise the typed, non-retryable LlmNotConfiguredError. 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_proposal was 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.
A malformed endpoint URL was invisible. get_llm_config swallowed normalize_service_url's ValueError, 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:

Neither plain-RuntimeError site was burning retries. process_audio sets max_retries=0; generate_lore_proposals catches the exception and records failed without 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:75 is not an unchecked site (deferred to :91, matching :182), and bot.py's /meta and the admin connectivity test are correctly different — they report whether an LLM is configured rather than consuming one, so None is 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_task decorator — ~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.

Harmonisation landed in `0a8910e`. CI green. All four acceptance items now met. ## Acceptance - [x] **A session can never be left in `extracting`/`matching`** — v3.11.3, via `on_failure`. - [x] **Missing configuration fails fast rather than burning retries** — see the correction below; this was already true, and is now uniform. - [x] **The not-configured policy at each call site is explicit and documented** — four policies documented in `settings_service`'s module docstring, with the non-obvious sites annotated inline. - [x] **Test covering retry exhaustion leaving a terminal status** — v3.11.3, `test_lore_generation_failure.py`. ## What landed `require_llm_config(db)` — resolve or raise the typed, non-retryable `LlmNotConfiguredError`. 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_proposal` was 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. - **A malformed endpoint URL was invisible.** `get_llm_config` swallowed `normalize_service_url`'s `ValueError`, 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: - **Neither plain-`RuntimeError` site was burning retries.** `process_audio` sets `max_retries=0`; `generate_lore_proposals` catches the exception and records `failed` without 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:75` is not an unchecked site** (deferred to `:91`, matching `:182`), and **`bot.py`'s `/meta` and the admin connectivity test are correctly different** — they report whether an LLM is configured rather than consuming one, so `None` is 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_task` decorator** — ~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.

claude-bot closed this issue

2026-08-07 05:58:10 +00:00

claude-bot referenced this issue

2026-08-07 06:00:09 +00:00

An @llm_task decorator for the duplicated Celery task preamble (follow-up to #287) #294

claude-bot commented

2026-08-07 06:00:18 +00:00

Author

Collaborator

Follow-up filed as #294 — the @llm_task decorator, with the duplication re-measured against 0a8910e rather than this issue's original line numbers.

Follow-up filed as #294 — the `@llm_task` decorator, with the duplication re-measured against `0a8910e` rather than this issue's original line numbers.

claude-bot referenced this issue

2026-08-07 06:03:06 +00:00

chore(release): v3.11.4 #295

claude-bot referenced this issue

2026-08-26 15:14:15 +00:00

An @llm_task decorator for the duplicated Celery task preamble (follow-up to #287) #294

rbrooks referenced this issue from a commit

2026-08-26 15:34:26 +00:00

refactor(backend): make the LLM-missing policy structural, not a convention (#294)

claude-bot referenced this issue

2026-08-26 15:34:43 +00:00

An @llm_task decorator for the duplicated Celery task preamble (follow-up to #287) #294