Repository navigation
fix(memory): preserve encrypted SQLite history on wrong-key pops - #5083
jbeckwith-oai merged 9 commits into
Conversation
karaaslanz
left a comment
There was a problem hiding this comment.
One concurrency edge still leaves the destructive-loss bug reachable.
karaaslanz
left a comment
There was a problem hiding this comment.
The original peek→pop TOCTOU is fixed, but the compensating restore still has a narrower ordering race.
seratch
left a comment
There was a problem hiding this comment.
Thanks for separating authentication failure from expiry. Before merging, please replace the separate destructive pop and compensating append with an operation that authenticates and conditionally removes the same record atomically at the backend boundary.
Please also replace the sequential append test with controlled interleavings covering an append and a clear during the operation, asserting final history order and that cleared records cannot reappear. The current test appends only after restoration has completed, so it does not exercise the race.
…://github.com/1aifanatic/openai-agents-python into fix/encrypted-session-authenticate-before-pop
The merge-base changed after approval.
The merge-base changed after approval.
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed native encrypted SQLite pop against both prior feedback threads. The same transaction now claims and authenticates the exact tail row before committing. InvalidToken rolls back rather than peeking or appending a replacement row, so identity and order survive a wrong key even during concurrent append/clear. Repeated cancellation waits for the worker to finish rollback. Authenticated expired tails still drain; overridden, wrapped and non-SQLite pops keep their original delegation behavior. The narrower atomic guarantee is accurately stated on pop_item.
The broad GitHub comparison includes 19 upstream main commits. I compared those files against pinned main: only encrypt_session.py, sqlite_session.py and test_encrypt_session.py differ, and reviewed that entire owned change with surrounding code.
All 21 exact-head hosted checks succeeded, including Python/Windows tests, native macOS, packaged contracts, lint and typecheck. The new tests pause at actual SQLite authentication and check write locking, row/order preservation, expiry, cancellation and custom-session delegation. I did not run local tests. No blocking findings.
The merge-base changed after approval.
|
Thanks for reviewing and merging #5083. I'd like to continue contributing, and I understand the current collaborator-only PR policy. Do maintainers ever invite external contributors to submit fixes for specific issues, or should I focus on bug reports and minimal reproductions? If there is a path to becoming a collaborator, I'd appreciate guidance on the expectations. |
This pull request fixes destructive wrong-key pops for native SQLite-backed encrypted sessions. Authentication runs inside the transaction that claims the row, so failure preserves ciphertext and row order while authenticated expired items retain their existing cleanup behavior.
Other backends, custom pop implementations and compaction wrappers retain their released delegation behavior. The new atomic authentication guarantee is limited to native SQLite; other backends retain the existing wrong-key limitation.
Test plan
Issue number
Addresses the native SQLite case in #5005. Other backends remain follow-up work.
Checks