fix: hold references to the persistence tasks - #3007
Conversation
Every write to the data layer is fired with create_task() and the task is discarded. The event loop only keeps a weak reference, so a task whose sole reference was the create_task() expression can be garbage collected before it reaches the data layer. Unlike a dropped log line, this loses user data: a collected task means a message, step or element is silently never persisted. There is no exception and no log — the write simply never happens, and the failure only shows up later as a thread with missing history. Eight sites across four modules: - context.py update_thread on http context init - element.py create_element - message.py update_step, delete_step, create_step - step.py update_step, delete_step, create_step Each file keeps a module-level set and discards the task in a done callback.
…umentation asyncio only keeps a weak reference to a task created via create_task() or ensure_future(); a discarded reference lets the task be garbage collected before it completes. Chainlit#3007 fixed the persistence call sites in context.py/element.py/message.py/step.py; this covers the remaining sites the same scan turned up, none of which touch the data layer: - emitter.py: the method-queue flush, the new-message persistence dispatch, the first-interaction thread-init dispatch, and the file-element send loop in process_message(). - socket.py: the idle-session clear timer, the on_audio_chunk callback dispatch, and the audio first-interaction thread-init dispatch. - server.py: the action-callback first-interaction thread-init dispatch (the third of three call sites of the same emitter.init_thread(...) pattern, alongside emitter.py and socket.py above). - llama_index/callbacks.py: LlamaIndexCallbackHandler's on_event_start/on_event_end handlers dispatch step.send()/ step.update() from a sync LlamaIndex callback; held via a new per-instance set. - mistralai/__init__.py, openai/__init__.py: the instrumentation on_new_generation callbacks dispatch step.send() the same way. Each file gets its own held-task set, following the pattern Chainlit#3007 established in the files it touched. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Step.__enter__/__exit__ dispatch step.send()/step.update() via create_task() the same way the already-fixed persistence call sites in this file do, but these two were missed by the original scan. Reuse the module's existing _persistence_tasks set. Co-Authored-By: Claude Sonnet 5 <[email protected]>
2cc869e to
cbfcbd1
Compare
|
This PR is stale because it has been open for 14 days with no activity. |
|
@noron12234 Thanks for the contrib. Could you add some unit tests for this functionality? |
Pins the invariant the fix introduces: while a data-layer write is in flight the spawning module holds a strong reference to its task, and the reference is dropped once the task is done. One test per write site, plus the two Step context-manager dispatches and a check that the set is a live registry rather than an accumulator. The tests assert on the delta of each module's task set, not its absolute contents, so the Mock that test_message.py leaves behind when it patches asyncio.create_task cannot influence the result. No test forces a collection and asserts survival: a suspended task is reachable from whatever it awaits, so it survives gc.collect() with or without the fix and such a test would pass against the unfixed code too. Co-Authored-By: Claude Opus 5 <[email protected]>
|
@dokterbob Added in 551e38e — One per write site (3 message, 3 step, 1 element, 1 context), plus the two Shape. The data layer is an Assertions are on the delta of each module's task set, not its absolute contents. What I did not write, and why. I said in the description that a GC test would be flaky by construction. That was the wrong reason — I checked, and the real problem is that it would be vacuous: That's fire-and-forget What is left is the invariant: a strong reference exists for the duration of the write. That does mean the tests reference the private Verification (this time I ran it):
Being straight about the failure mode when reverted: the tests fail with |
The problem
Every write to the data layer is fired and forgotten:
The task is discarded, and the event loop only keeps a weak reference:
For logging or telemetry that would cost a datapoint. Here it costs user data: a collected task means a message, step or element is never written. There is no exception and nothing in the logs — the write simply never happens.
self.persisted = Trueis set regardless, so the object believes it was saved. The failure only surfaces later as a thread with holes in it.Note the
try/exceptdoes not help:create_taskschedules and returns immediately, so an error inside the coroutine never reaches that handler either.Sites
Eight, across four modules — every place a
data_layer.*write is spawned:context.py:98update_threadduring http context initelement.py:215create_elementmessage.py:116update_stepmessage.py:135delete_stepmessage.py:150create_stepstep.py:345update_stepstep.py:370delete_stepstep.py:396create_stepThe change
Each module keeps a set and discards the task in a done callback:
Module-level rather than instance-level because
Message,StepandElementare short-lived — an instance-scoped set would be collected along with the object it was meant to outlive.discardrather thanremoveso a double callback cannot raise. The set stays bounded by the number of in-flight writes.No control flow, ordering or awaited behaviour changes. These writes were fire-and-forget before and remain so; they simply can no longer be collected mid-write.
Verification
I want to be straight about what I did not do: I did not run the test suite, so this is verified by lint and compile only. The change is mechanical — bind the task, register it, deregister on completion — but if you would like the suite run before merging, say so and I will.
No test is added either. The failure is a garbage-collection race, so a test would have to force a GC at a chosen moment and assert a task did not vanish, which is flaky by construction. Happy to add one if you have a shape in mind.
Found with an AST scan for
create_task/ensure_futureresults discarded as bare expression statements (excludingTaskGroup.create_task, which does hold strong references). The scan reports other hits underllama_index/,openai/,mistralai/andsocket.py; those spawn UI-emit and stream work rather than data-layer writes, so I left them out to keep this diff to one concern. Happy to follow up.Summary by cubic
Prevents lost data writes by holding strong references to all persistence tasks. Previously, writes used bare
asyncio.create_task(...)and tasks could be garbage collected before reaching the data layer; now each task is tracked until completion while keeping fire-and-forget semantics._persistence_tasks: set[asyncio.Task]withadd_done_callback(_persistence_tasks.discard)incontext.py,element.py,message.py, andstep.py(includingStep.__enter__/__exit__dispatches ofsend()/update()).backend/tests/test_persistence_tasks.pyto pin the invariant (tasks held during writes and released on completion) and verify the set drains and stays bounded.Written for commit 551e38e. Summary will update on new commits.