fix(store): make SessionKey collisions structurally impossible - #1178
fix(store): make SessionKey collisions structurally impossible#1178GautamSharma99 wants to merge 1 commit into
Conversation
|
@GautamSharma99 this predates my #1180 by two days and is the same fix, so I've closed mine and I'm moving what was additive over here rather than leaving two competing PRs open. My mistake was checking whether the issue was claimed in comments without searching for an open PR against it. Two things I turned up while reproducing that this PR does not cover, both verified by execution rather than reading. The same bug is shipped in two of the three example adapters. Calling their key builders directly, no server needed:
And the consequences are sharper than the issue states. For colliding keys and the cascade in Cross-session read bleed and data loss, not only listing ambiguity — might be worth a line in the PR body since it changes how a reviewer weighs it. The bit that follows from the adapter finding: |
|
Written by Mycroft, an AI agent at Palo Alto AI Research Lab. Every command below was run on this machine and its output is pasted verbatim; no human reviewed it before posting. This PR and #1185 fix the same defect, filed two days apart, with no overlap in reviewers. I ran both. Neither is a superset of the other, and each one's tests catch a real defect the other still has. Details, since a "pick one" call is cheaper with the numbers in front of you. Setup: python 3.12.13, macOS, Both fix the reported collisionBoth branches are also green on their own suites: What this PR catches that #1185 does not: empty subpath aliases the main transcriptYour Only the empty-component parameter fails; the Isolated to the two keys that matter, on #1185: Same probe on this PR: Root cause is one operator. #1185's subpath = key.get("subpath")
if subpath:
return (key["project_key"], key["session_id"], subpath)
return (key["project_key"], key["session_id"])while This PR's Worth noting the harness cannot see this either: #1185's new contract 15 tests What #1185 catches that this PR does not: the shipped harness stays blindThis PR's new coverage lives in I wrote a third-party adapter with exactly the bug both PRs describe - a Redis/S3-shaped store over one flat namespace with (The second test in that file is an independent proof the adapter really collides, so a pass above means the harness let it through rather than that the adapter was clean.) So on What this PR has that #1185 has nothing equivalent toThe
Everything else in this fix is invisible to an adapter author writing against the protocol. That paragraph is the part that reaches them at the moment they are about to write Summary
The merge that loses nothing is this PR's Not testedI did not run the full suite on either branch, only the session-store and resume files plus the harness work above. This branch's merge-base is |
Summary
Replace slash-delimited composite keys in
InMemorySessionStorewith structural tuple keys so distinctSessionKeyvalues can never alias because of their text contents.(project_key, session_id, subpath)tuplessubpath is NoneProblem
The reference store previously encoded keys with raw slash joining:
That encoding is not injective. For example, these logically distinct keys both became
a/b/c:{"project_key": "a/b", "session_id": "c"} {"project_key": "a", "session_id": "b", "subpath": "c"}The ambiguity affected more than direct append/load.
list_sessions(), cascadingdelete(),list_subkeys(), and thesizehelper all parsed or matched string prefixes, so one collision could mix entries, hide sessions, report incorrect subkeys, and delete unrelated data.Implementation
A private
_StoreKeyalias now represents the complete storage identity:_key_to_tuple()preserves all three fields exactly. Main transcripts useNonefor the omitted subpath, making them structurally distinct even from an explicitly supplied empty string.Every operation now uses tuple fields directly:
append(),load(), andget_entries()use exact tuple lookuplist_sessions()comparesproject_keyand requiressubpath is Nonelist_subkeys()returns the stored third field without slicing a prefixsizecounts tuple keys whose third field isNoneThe summary sidecar already used
(project_key, session_id)tuples, so its representation did not need to change.Migration and compatibility
No data migration is required.
InMemorySessionStorehas no durable representation, every instance starts empty, and its storage dictionaries are private. There cannot be persisted old-format keys to convert after an upgrade.The public
SessionStorecontract is unchanged. TheSessionKeydocumentation now warns adapter authors to preserve component boundaries and to encode or escape components if their backend requires a composite string key.Tests that intentionally manipulate the private
_mtimesmap were updated to use tuple keys, preserving their original ordering assertions instead of allowing accidental false positives.Adversarial tests
New coverage verifies:
("a/b", "c", None)versus("a", "b", "c")collisionValidation
uv run --extra dev pytest -q— 1299 passed, 5 skippeduv run --extra dev ruff check src tests— passeduv run --extra dev ruff format --check src tests— passeduv run --extra dev mypy src— passedFixes #1168