Repository navigation
Memory waterfall drops active messages when two puts race on one session - #23418
goingforstudying-ctrl wants to merge 2 commits into
Conversation
_manage_queue snapshots the active queue, decides how many oldest messages to flush, then archives by position with archive_oldest_messages(n). Two puts landing on the same session at the same time both snapshot before either archives, so the second flush runs against rows the first one already handled and its positional archive ends up dropping messages that were supposed to stay active. aput/aput_messages/aset now run through a per-event-loop asyncio lock, so each waterfall sees the store as the previous one left it. The lock follows the running loop because the sync wrappers spin up a fresh loop per call via asyncio_run.
|
Reproduced the race on main with the gated store: two concurrent |
There was a problem hiding this comment.
Community review. I am not a maintainer, so this does not clear the merge gate.
I read the full diff and Memory._manage_queue / aput / aput_messages / aset in memory.py, and ran the code (llama-index-core at the PR head, Python 3.13, in-memory chat stores only; I did not run a SQL-backed store).
The race is real and the lock fixes the case it targets. The new test_concurrent_aput_does_not_over_archive passes on the PR head and fails with memory.py taken from origin/main. tests/memory and tests/agent on the PR head: 177 passed, 3 skipped.
Three things I found by running it, all with the PR's own StaggeredArchiveChatStore and the seed messages from the test:
-
A copy of the Memory object after any
aputcan no longer be deep-copied._manage_lock_loopholds the event loop, andcopy.deepcopy(memory)andmemory.model_copy(deep=True)raiseTypeError: cannot pickle '_contextvars.Context' objectonce a message has been put under a running loop. Both work on origin/main. A shallowmodel_copy()is fine.pickle.dumps(memory)andmodel_dump()still work (pickle logs "Removing unpickleable private attribute _manage_lock_loop"). I searchedllama_index/core/agent,memoryandworkflowand found no deepcopy of a Memory, so this may only affect user code; I did not search the integrations. It is probably worth keeping the lock out of the instance state (for example aWeakKeyDictionarykeyed by loop) or adding a__deepcopy__. -
The lock is per Memory object and per event loop, so it does not cover two other ways to hit the same race. With the same staggered store and the same seed messages, sequential puts of newA then newB leave
['newA', 'newB']active. On the PR head, both of these still end with an empty active queue (identical to origin/main):- two
Memoryobjects with the samesession_idand the same store, each doing oneaputunderasyncio.gather; - one
Memorydoingaputon the main loop whileasyncio.to_thread(memory.put, ...)runs, sinceasyncio_runthen uses a worker thread with its own event loop and therefore gets a different lock.
The code comment says "per event loop", so this is by design, but the PR title and description read as if concurrent puts on one session are fixed in general. I would say in the description that it covers one Memory object on one loop.
- two
-
Minor:
asettakes the lock butaresetdoes not. I did not test whether that matters.
I did not run a database-backed chat store, other concurrency levels than two puts, or CI.
Ran into this while serving an agent off a shared
Memoryobject in an async server: chunks of the recent conversation kept vanishing even though the token limit was nowhere near hit. Turns out the waterfall isn't safe against two puts landing on the same session at the same time._manage_queuesnapshots the active queue, counts tokens, then flushes witharchive_oldest_messages(n)— positional. When twoaputs overlap, both waterfalls snapshot the queue before either archives. The second one's "oldest n" then lands on rows the first one deliberately kept, so active messages get archived early. With an unlucky interleaving it flushes the whole queue, including the message that was just added.Repro (deterministic, using a store that staggers the two archive calls so the second select runs after the first commit):
Back-to-back puts keep
[seed4, seed5, newA, newB]active. The racy version archives everything — active queue comes back empty.Fix: serialize the waterfall per event loop.
aput/aput_messages/asetnow run through a small asyncio lock, so each manage cycle sees the store as the previous one left it. The lock follows the running loop instead of being created once, because the sync wrappers go throughasyncio_runand a cachedasyncio.Lockwould end up bound to a dead loop on the secondput().One thing I'm not fully sure about: this only serializes within a single
Memoryinstance. Two instances pointed at the samesession_idagainst a shared DB can still race at the store level — doing that properly would need something transactional inAsyncDBChatStore, which felt out of scope.Tests: new regression test forces the interleaving with the gated store and asserts the concurrent result matches sequential puts (fails on main, passes here), plus a guard that sync
put()works across loops.pytest tests/memory/— 87 passed, ruff clean.