fix(memory): key OpenViking user-space cache on the resolving client's own snapshot - #94698
nftpoetrist wants to merge 1 commit into
Conversation
…s own snapshot Background writers (on_memory_write, sync_turn) freeze a client via _new_client() on one thread and resolve/use it later. If a config reload swaps self._conn_snapshot in between, _user_space() previously read the live self._conn_snapshot instead of the snapshot the passed-in client was actually built from, and published the OLD client's resolved identity under the NEW connection's cache key — poisoning every subsequent lookup for the new connection with the stale user until the next reload. _new_client() now stamps each client with the snapshot it was built from, and _user_space() keys/publishes the cache against that stamped snapshot when an explicit client is passed, instead of the live self._conn_snapshot.
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head e079fb6b32e5ef7ba04a533d59d84041fffa5d61 against current main 1bbb6e5bce56e721ab685af4cd87df21bbff4d35. I traced the full client-construction/publication surface, every relevant _user_space(...) call shape, the background writer paths, the session-start recall path, the merged #94515 / #93985 / #91998 lineage, and exact-head hosted CI. CI, Docker, and Nix are green, and there were no existing reviews/comments to duplicate.
BLOCKER — the same cross-connection cache poisoning is still reachable through an unstamped published self._client.
This patch binds _conn_snapshot only to clients created by _new_client(). But the provider's live client is constructed directly with _VikingClient(...) in initialization / refresh publication paths, then assigned to self._client alongside self._conn_snapshot; those clients are not stamped by this patch. Meanwhile _read_session_start_memory_parts() captures active_client = client or self._client and then passes that object explicitly to _user_space(active_client, ...). _build_memory_uri() has the same explicit-client shape.
That leaves this production interleaving:
- connection A is live; session-start recall captures
active_client = self._client(A, with no_conn_snapshotattribute); - a profile/config reload publishes live client/snapshot B;
_user_space(active_client)runs on the captured A client;- because A is unstamped, the new fallback reads the live B
self._conn_snapshot; _resolve_user_space(active_client)asks A and resolves A's user;- the publish recheck again reads live B, so it accepts the result and caches
(B_snapshot, A_user); - subsequent B lookups hit that cache and return A's identity without probing B.
So the defect class is not just “background _new_client() clone outlives reload”; it is “a client object can outlive the mutable provider-global identity used to key its result.” The connection identity has to belong to the client/authority carrier everywhere, not only on one construction path.
Required fix: bind the exact settings snapshot to every client that can become/persist as self._client or be passed explicitly to _user_space, ideally through one construction/publication primitive, and have explicit-client cache decisions derive only from that binding. For legacy/hand-wired explicit clients without a proven binding, do not cache their resolved identity under ambient live self._conn_snapshot. An equivalent design that proves client ↔ snapshot identity atomically is also fine.
Please add a regression for the other side of the race: capture the ordinary published A self._client, publish B, then resolve through captured A and prove B's cache cannot be poisoned. The current new test manually does client._conn_snapshot = provider._conn_snapshot, so it also does not exercise the _new_client() stamping that this patch introduces; breaking/removing that stamping would leave this test green. Cover the real construction path as well.
Interlock/credit: this is a valid narrow follow-up to the merged canonical-URI work in #94515, which incorporated the connection-scoped identity work from #93985 and URI work from #91998. It is not a duplicate of those PRs; it closes a race left by that lineage. The broader architectural invariant is the same one those changes were aiming at: a captured OpenViking operation must carry its own connection identity across reloads rather than reconsult ambient provider state.
|
Thanks for the fix. We have ported it to the maintained external Hermes plugin in OpenViking #5261, with your original authorship preserved. A separate follow-up covers retained active clients and adds external-loader regression tests. Both client paths were verified against a real local OpenViking server: the unchanged plugin reproduced the identity mixup, and the fix returned the correct user. All 30 standalone plugin tests pass. The OpenViking PR is now merged. This covers the fix in the OpenViking-maintained plugin as part of the memory-provider migration. |
What does this PR do?
OpenVikingMemoryProvider._user_space()caches the server-resolved current-user identity, keyed onself._conn_snapshot— the live connection identity — regardless of which client object is actually being resolved.Background writers (
on_memory_write,sync_turn) callself._new_client()on one thread to freeze a client from the connection that's live at that moment, then resolve/use that client later (after real network I/O, on a background thread). If a config reload (profile switch, credential rotation) swapsself._conn_snapshotto a different connection while that write is in flight,_user_space()still reads the liveself._conn_snapshotwhen it publishes the cache entry — so the old client's resolved identity gets cached under the new connection's key. Every subsequent identity lookup for the new connection then returns the previous connection's user until the next reload or process restart — a cross-connection memory-identity mixup.Fix
_new_client()now stamps the client it builds with the exact snapshot it was built from (client._conn_snapshot)._user_space()uses that stamped snapshot — not the liveself._conn_snapshot— to key and publish the cache entry when an explicitclientargument is passed. Theself._client/client=Nonepath (unaffected by this race) is unchanged.Regression test
Added
test_user_space_cache_not_poisoned_by_stale_client_across_reload, which freezes a client under connection A, swaps the live connection to B, resolves identity through the frozen A-client, then asserts a fresh resolution against the live B-connection returns B's own identity rather than the cached A identity.Confirmed via mutation testing: with only the test added and the fix reverted, the new test fails with
AssertionError: assert 'alice' == 'bob', reproducing the cache-poisoning cross-user leak. With the fix restored, it passes.Test plan
tests/plugins/memory/test_openviking_provider.py— 63 passedtests/openviking_plugin/test_openviking.py— 59 passed (combined with checkpoint-contract suite)tests/plugins/memory/test_openviking_endpoint_always_blocked.py,tests/plugins/memory/test_openviking_shutdown.py— 11 passed