Skip to content

fix(services): detach the turn's status callback when the turn ends - #264

Merged
dovvnloading merged 1 commit into
mainfrom
fix/status-callback-does-not-outlive-its-turn
Sep 7, 2026
Merged

fix(services): detach the turn's status callback when the turn ends#264
dovvnloading merged 1 commit into
mainfrom
fix/status-callback-does-not-outlive-its-turn

Conversation

@dovvnloading

Copy link
Copy Markdown
Owner

Reviewing note: the diff looks large and is almost entirely indentation. git diff -w shows the eleven lines that actually changed.

The problem

GenerationService.generate installs a progress callback on the engine and never removes it:

engine.set_status_callback(
    lambda message: self._publish(sink, snapshot, "loading_model", message)
)

SynthesisAgent.set_status_callback forwards straight through to the chat client — and the lifetimes do not match:

# app_factory.py -- one client for the whole process
routing_chat_client = RoutingChatClient(OllamaChatClient(client), llamacpp_chat_client)

# ...a new engine per turn, sharing it
engine_factory=lambda snapshot: SynthesisAgent(..., routing_chat_client, ...)

So a per-turn callback ends up parked on a process-wide object, with two consequences.

It pins the turn. The lambda closes over sink and snapshot, so the finished turn's snapshot stays referenced for the life of the process — attachments included, up to the 24 MB a single message may carry, held long after the answer was delivered.

It misattributes progress. generate_chat_title builds a fresh engine and installs no callback of its own, so it runs against whatever the previous turn left behind:

def generate_chat_title(self, snapshot, response):
    engine = self._engine_factory(snapshot)     # no set_status_callback
    return engine.generate_chat_title(...)

A model load during titling therefore publishes a loading_model event against a job that has already completed.

The fix

The install is paired with a finally that detaches it, and the protocol documents None as meaning exactly that.

Verified directly against a stand-in for the shared client:

after generate, shared client callback: None
snapshot still reachable from the client: False

Verification

Two tests, because the failure path is the one that matters most — a turn that raised used to leave its callback installed, and that is precisely when a stale callback would misattribute the next runtime message:

  • a completed turn leaves no callback on the shared client
  • a turn that raises ModelOperationError leaves none either

Both fail against the unfixed code. Each asserts a callback was installed before asserting it was removed, so neither can be satisfied by an engine that simply never received one.

Check Result
python -m pytest -q 909 passed (907 on main + 2 new)
python -m mypy clean, 79 source files
python -m ruff check backend tests tools main.py app_factory.py clean

Run on Python 3.14, one of the versions in the compatibility matrix.

Compatibility and rollback

set_status_callback now accepts None on the protocol and the shipped double; LlamaCppChatClient already did, so nothing downstream changes. Progress reporting during a turn is untouched — the callback is installed at the same point and removed only once the turn is over. No API contract, stored data, or migration. Reverting the commit restores the previous behaviour exactly.

Limits

This makes the callback's lifetime match the turn's. It does not give generate_chat_title or translate_text progress reporting of their own — they still run without one, so a model load during titling now reports nothing rather than reporting it against the wrong job. Silence is the correct half of that trade here; wiring real progress into those calls is a feature.

The retained-snapshot cost was bounded at one turn (each new turn replaced the previous callback), so this is a correctness and footprint fix rather than an unbounded leak.

🤖 Generated with Claude Code

`GenerationService.generate` installs a progress callback on the engine and
never removed it. `SynthesisAgent.set_status_callback` forwards straight to the
chat client -- and `app_factory` builds exactly one `RoutingChatClient` for the
process, while `engine_factory` builds a `SynthesisAgent` per turn. The
per-turn callback therefore stayed on a process-wide object.

It closes over `sink` and `snapshot`, so the finished turn's snapshot stayed
referenced for the life of the process, attachments included -- up to the 24 MB
a single message may carry, held long after the answer was delivered.

It also misattributed progress. `generate_chat_title` builds a fresh engine and
installs no callback of its own, so it runs against whatever the previous turn
left behind: a model load during titling published a "loading_model" event
against a job that had already completed.

The install is now paired with a `finally` that detaches it, and the protocol
documents `None` as meaning exactly that. The failure path matters as much as
the success one -- a turn that raised used to leave its callback installed --
so both are covered.

The diff looks large and is almost entirely indentation: `git diff -w` shows
the eleven lines that actually changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dovvnloading
dovvnloading merged commit 1e313f0 into main Sep 7, 2026
7 checks passed
@dovvnloading
dovvnloading deleted the fix/status-callback-does-not-outlive-its-turn branch September 7, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant