Repository navigation
[runtime][api] Scope long-term memory operations to the action that obtained the memory set - #1002
Conversation
There was a problem hiding this comment.
Thanks for taking this on @weiqingy.
BTW, while reviewing the change, I noticed that the cross-language job does not provide ACTION_API_KEY like python-it, which caused the Java LTM E2E test to be skipped. I think we can fix this in this PR or in a separate one.
|
|
||
| The current partition key and observation context are copied onto the set | ||
| so that operations submitted to a worker thread stay scoped to the action | ||
| that obtained it. Must be called on the mailbox thread. |
There was a problem hiding this comment.
[P1] Enforce mailbox-thread access for memory-set management
I think get_memory_set and delete_memory_set, as memory-set-level management operations, should only run on the mailbox thread. Otherwise, passing either to durable_execute_async can preserve the same cross-key race. Could we enforce this in both Java and Python, similar to RunnerContextImpl#getResource?
There was a problem hiding this comment.
Good point, done in both languages.
Both management methods now run the same mailbox check getResource uses. The four set-scoped operations stay unchecked on purpose, since being safe on a worker thread is what binding the context to the set buys.
Java needed its own check rather than relying on the bridge, since getMemorySet there never enters Python. On the Python side I passed the checker through the constructor rather than adding one to the RunnerContext ABC, since Java's equivalent sits on the runtime class rather than the API interface. Curious whether you would have put it on the interface instead.
getLongTermMemory is left alone, since capturing the handle on the mailbox thread and using it inside the callable is normal.
This settles the delete_memory_set question without touching its signature. One caveat: on JDK 11 the durable callable runs inline, so the same code will not raise there yet.
| would widen every operation to all keys sharing the job id and set name, which | ||
| for a delete means deleting another key's items. Refuse the operation instead. | ||
| """ | ||
| if memory_set.partition_key is None: |
There was a problem hiding this comment.
[P1] Handle empty partition keys safely
"" is a valid Flink key, but Mem0 ignores a falsy agent_id, dropping partition isolation. Please reject empty keys or encode them into a non-empty, collision-free value. Also use None/null, rather than "", for the unbound state.
There was a problem hiding this comment.
PR description updated.
On refusing rather than encoding: to keep already-stored memories readable the mapping has to leave non-empty keys alone, which leaves no free non-empty value for "" to take. A reserved sentinel just moves which key breaks. Collision-free would mean rewriting every key's agent_id and orphaning what is already stored, so refusing looked like the better trade. Does that match how you would weigh it?
Both None and "" are refused now in both languages, with different messages. The unbound default was "" on both sides, so an unbound Java set already looked bound by the time it reached Python.
Two things worth knowing. On add, mem0 drops agent_id from the stored metadata, so items written under an empty key stay unrecoverable even after the key is fixed. And an existing test was asserting that agent_id="" gets forwarded, so this was pinned as intended behaviour.
|
@wenjin272 Thanks for the review! Good catch, and it turned out to be two jobs: the cross-language one you spotted, and java-it, which runs One limit worth knowing: secrets are withheld from |
|
The That version is the existing Re-triggering it once the current run finishes. |
…btained the set Mem0LongTermMemory holds the partition key and observation context in mutable fields that the mailbox thread overwrites on every context switch. Operations submitted through durable_execute_async run on a worker thread and read those fields themselves, so an operation can be attributed to whichever key the mailbox thread most recently switched to. The key reaches Mem0 as agent_id, which is the only isolation boundary between two keys that share a job id and a memory set name. Bind the partition key, observation id and suppression flag onto the MemorySet when it is created on the mailbox thread, and have add, get, search and delete take them from the set. The Java wrapper forwards into the same Python object and rebuilt the Python set from its name alone, so it now records the context on switch and carries it across the bridge. A set is therefore scoped to one action and must not be reused across actions. Two existing tests reused one across a context switch and expected the new key to apply; they now assert that the set keeps its own key instead. Operating on a set that carries no binding raises rather than proceeding. Mem0 ignores a falsy agent_id instead of matching on it, so an unbound set would widen an operation to every key sharing the job id and set name, which for a delete would remove another key's items. delete_memory_set takes a name rather than a MemorySet and still reads the shared field, which its docstring now records. Generated-by: Claude Code 2.1.228
Mem0LongTermMemoryTest and SkillsIntegrationTest both skip themselves unless ACTION_API_KEY is set, since each needs an OpenAI-compatible chat model. The python-it job passes the secret, but the java-it and cross-language jobs set only LOG_LEVEL, so both Java tests were reported as run while every case was silently skipped. Pass the same secret to those two jobs. Repository secrets are still withheld from pull_request runs on forks, so this restores the coverage for push builds and same-repository pull requests, matching what python-it already gets. Generated-by: Claude Code 2.1.251 (Claude Opus 5)
Mem0 matches on agent_id only when it is truthy, so an empty key drops the filter entirely rather than narrowing to that key. An operation issued under one reaches every key sharing the job id and memory set name, which for a delete removes another key's items, and an add stores its items with no agent_id at all, leaving them unattributed to any key. The empty string is a legal Flink key, so this was reachable by keying a stream on a string field that can be empty. Refuse an empty partition key at the point of use rather than encoding it. Encoding cannot be both collision-free and non-destructive: keeping already stored memories readable requires the mapping to be the identity on non-empty keys, which leaves no unoccupied non-empty value for the empty key to take. Making it collision-free means transforming every key and orphaning every memory already written. Validation stays out of switch_context so that a job carrying an empty key still runs unless it actually uses long-term memory. Distinguish the unbound state from the empty one, since the remedies differ, and make it null in both languages. The Java wrapper defaulted its partition key to the empty string while MemorySet defaulted to null, so an unbound set originating in Java arrived in Python as an empty string and passed both guards. An existing test asserted that an empty agent_id is forwarded to Mem0, pinning the behaviour as intended; it now asserts refusal, and that Mem0 is never reached. Generated-by: Claude Code 2.1.251 (Claude Opus 5)
Binding the partition key onto a MemorySet makes the four set-scoped operations safe to run on a worker thread, but obtaining a set and deleting one still read the key currently in scope. Submitting either through durable_execute_async therefore reproduced the same cross-key attribution one call earlier: the set was stamped, or the delete issued, under whichever key the mailbox thread had most recently switched to. Run the runner context's mailbox thread check in getMemorySet and deleteMemorySet, the same check getResource uses. The four set-scoped operations stay unchecked, since being callable from a worker thread is the purpose of carrying the context on the set. Check in both languages rather than only at the bridge. The Java getMemorySet is a factory that stamps the Java-side fields and never enters Python, so a check on the Python side alone would leave a Java agent unguarded. Python receives the checker through its constructor rather than through a new method on the RunnerContext interface, which has no counterpart on the Java API interface and would not be reachable from a Java caller. delete_memory_set keeps its name-based signature. It has no bound context and applies to the key in scope, which the thread check makes correct, so isolating it no longer requires changing a public signature. Obtaining or deleting a memory set inside a durable callable now raises on Python and on Java under JDK 21 and above. Under JDK 11 the callable runs inline on the mailbox thread, so it does not raise there yet. Generated-by: Claude Code 2.1.251 (Claude Opus 5)
GitHub sets a workflow env var to the empty string when the secret behind it is unavailable, which is what happens on a pull request from a fork. The Java gates tested the variable against null, which an empty string passes, so the tests ran without a usable key and failed instead of skipping. The Python e2e gates test the value for truthiness and were already unaffected. Treat an empty value as absent, matching the Python side. Where a real key is present the tests run as before. Generated-by: Claude Code 2.1.251 (Claude Opus 5)
72ba453 to
4ddbd93
Compare
|
@wenjin272 Thanks for the review. Rebased onto main and pushed. |
…btained the memory set (apache#1002) Generated-by: Claude Code 2.1.251 (Claude Opus 5)
Linked issue: #998
Purpose of change
Mem0LongTermMemoryis shared across partition keys and holds the current key, observation id and suppression flag in mutable fields.switch_contextwrites them on the mailbox thread before each action, whileadd,get,searchanddeletecan run on a worker thread when an action passes them todurable_execute_async, and those operations read the fields themselves. The key they see is whatever the mailbox thread wrote most recently, not the key of the action that submitted the work.The key is the isolation boundary rather than observability metadata: it reaches Mem0 as
agent_id, and two keys sharing a job id and a memory set name are separated byagent_idalone. Reading the wrong one means one key's items land in, or are read from, another key's set. The observation id and suppression flag come from the same fields, so observations can be misattributed independently.This binds the partition key, observation id and suppression flag onto the
MemorySetwhen it is created, and has the four operations take them from the set. The four already receivememory_setas their first parameter, so nothing new is threaded through. A set is consequently scoped to one action and must not be held across actions.Java is covered in the same change rather than separately. Its operations all delegate into the same Python object, and the bridge rebuilt the Python set from its name alone, so once the Python side sources the key from the set, a Java-originated call would arrive without one. The Java wrapper now records the context when it switches and carries it across the bridge, and
to_python_memory_setrequires the key rather than defaulting it.Refusing a key Mem0 cannot isolate
Mem0 tests
agent_idfor truthiness instead of matching on it, so both an unbound key and an empty one would drop the filter entirely and widen an operation to every key sharing the job id and set name, which for a delete would remove another key's items. Anaddunder such a key would be worse than mis-filtered: Mem0 omitsagent_idfrom the stored metadata, leaving the item unattributed to any key.The empty string is a legal Flink key, reachable by keying a stream on a string field that can be empty, so both states are now refused at the point of use. They carry separate messages because the remedies differ. Encoding the empty key instead was rejected: keeping already-stored memories readable requires the mapping to be the identity on non-empty keys, which leaves no unoccupied non-empty value for the empty key to take. Reserving a sentinel only moves which key fails, since a Flink key arrives as
String.valueOf(key)and can be any string. A collision-free encoding would therefore have to transform every key and orphan every memory already written.The unbound state is now
Noneandnullin both languages. Both previously defaulted the memory's own key to the empty string, so neither could distinguish an absent context from the empty key, and the Java wrapper additionally stamped that value onto aMemorySetwhose own default was null, so an unbound set originating in Java arrived in Python looking bound and passed both guards.Validation deliberately stays out of
switch_context, so a job carrying an empty key still runs unless it actually uses long-term memory.Scoping the management operations to the mailbox thread
Binding context onto the set makes the four set-scoped operations safe on a worker thread, but obtaining or deleting a set still reads the key currently in scope. Submitting either to
durable_execute_asynctherefore preserved the same cross-key race one call earlier.getMemorySetanddeleteMemorySetnow run the runner context's mailbox-thread check, the same onegetResourceuses. The four set-scoped operations stay unchecked, since being usable from a worker thread is the point of binding the context to the set.The check fires in Java as well as Python. Java's
getMemorySetis a pure factory that stamps the Java-side fields and never crosses into Python, so a Python-only check would leave Java agents unguarded. Python receives the checker through the constructor rather than through a new method on theRunnerContextABC, because Java'scheckMailboxThreadlives on the runtime class rather than on the API interface, and adding one to the Python ABC would put a user-facing API method there with no Java counterpart.delete_memory_setkeeps its name-based signature. It has no bound context and applies to the key currently in scope, which the mailbox-thread check now makes correct, so closing the gap no longer requires a public signature change. Its documentation records that it can target a different key thanMemorySet.deleteon a same-named set.CI
The cross-language
Mem0LongTermMemoryTestandSkillsIntegrationTestboth skip themselves unlessACTION_API_KEYis set. The python-it job passes the secret; the java-it and cross-language jobs set onlyLOG_LEVEL, so both Java suites went green with every case skipped. Both jobs now receive it. Repository secrets are still withheld frompull_requestruns on forks, so this restores the coverage for push builds and same-repository pull requests, matching what python-it already gets.Tests
test_memory_set_stays_on_its_own_key_after_the_owner_switchestest_observations_stay_with_the_action_that_obtained_the_settest_suppression_follows_the_set_not_the_current_contexttest_unbound_memory_set_is_refused_rather_than_widenedtest_empty_partition_key_is_refused_by_set_operationstest_empty_partition_key_is_refused_by_memory_set_managementtest_memory_set_management_before_any_context_switch_is_refusedtest_memory_set_management_is_refused_off_the_mailbox_threadtest_set_scoped_operations_run_without_the_mailbox_threadtestForwardedSetCarriesTheContextItWasObtainedIntestUnboundSetIsRefusedRatherThanWidenedtestMemorySetIsRefusedBeforeAnyContextSwitchtest_to_python_memory_set_carries_the_action_contexttestEmptyKeyedSetIsRefusedRatherThanWidenedtestMemorySetManagementIsRefusedForAnEmptyKeytestMemorySetManagementIsRefusedOffTheMailboxThreadtestSetScopedOperationsRunWithoutTheMailboxThreadtest_switch_contextandtest_context_switch_changes_observation_owner_and_current_suppressionreused a single set across a context switch and expected the current key to apply. They now assert that a set keeps its own key, and obtain one per context.test_empty_context_key_is_used_consistently_for_mem0_operationsasserted that an emptyagent_idis forwarded to Mem0, recording the widening as intended. It is replaced bytest_empty_partition_key_is_refused_by_set_operations, which asserts refusal and that Mem0 is never reached.Every assertion was checked against a deliberately reverted implementation. Reverting the suppression binding, the observation id binding, any of the refusal guards, the key binding itself, either mailbox check, or the order in which the thread check precedes the key check each leaves at least one test failing.
126 Python tests pass across
api/memory,runtime/memoryandruntime/tests. 80 Java tests pass acrossMemorySetTest, the runtimeMem0LongTermMemoryTest,MemoryRefTest,TestMemoryObservationFlush,ActionTaskContextManagerTestandActionExecutionOperatorTest.ruff checkandspotless:checkare clean, and the touched Python files areruff formatclean.The long-term memory e2e test is gated on
ACTION_API_KEYand needs a Flink cluster plus a local Ollama embedding model, so end-to-end coverage comes from CI on push builds.API
MemorySetgains three fields in both languages, excluded from serialization like the existing long term memory back-reference. Java addssetActionContextand three getters.BaseLongTermMemory.get_memory_set/getMemorySetanddelete_memory_set/deleteMemorySetkeep their signatures and gain a documented per-action, mailbox-thread scope. The runtime bridge helperto_python_memory_setnow requires the partition key; its only caller is the Java wrapper, and a version mismatch fails loudly with a missing-argument error rather than silently.Mem0LongTermMemorygains a mailbox-thread checker parameter in both languages. It is a runtime class rather than public API, and the publicRunnerContextinterface is unchanged in both languages.Existing code that obtains a memory set inside the action using it is unaffected. Code that cached one across actions was already reading whichever key happened to be current, and now raises or stays on its original key instead. Code that obtains or deletes a memory set inside a
DurableCallableor adurable_execute_asynccallable now raises on Python and on Java under JDK 21 and above; under JDK 11 the callable runs inline on the mailbox thread, so it does not raise there yet. A job keyed on a value that can be the empty string now raises when it uses long-term memory, rather than silently sharing one memory set across every key.Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.251 (Claude Opus 5)
The first commit on the branch predates the model half of that convention and carries
Generated-by: Claude Code 2.1.228.