Skip to content

fix(memory): preserve encrypted SQLite history on wrong-key pops - #5083

Merged
jbeckwith-oai merged 9 commits into
openai:mainfrom
1aifanatic:fix/encrypted-session-authenticate-before-pop
Sep 25, 2026
Merged

jbeckwith-oai merged 9 commits into
openai:mainfrom
1aifanatic:fix/encrypted-session-authenticate-before-pop

Conversation

@1aifanatic

@1aifanatic 1aifanatic commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Wrong-key recovery, expired tails, row identity, concurrent append/clear and repeated cancellation.
  • Valid-key SQLAlchemy, compaction-wrapper, custom SQLite and context-aware session pops.
  • Two independent reviews completed without findings.
  • Required verification script passed: format, lint, mypy, pyright; 10753 parallel tests and 77 serial tests passed (56 parallel and 4 serial skips).

Issue number

Addresses the native SQLite case in #5005. Other backends remain follow-up work.

Checks

  • Added focused regression tests.
  • Ran the required code-change-verification script.
  • Confirmed every verification step passed.
  • Completed two independent reviews of the final implementation.

@1aifanatic
1aifanatic requested review from a team, rm-openai and seratch as code owners September 18, 2026 03:30

@karaaslanz karaaslanz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concurrency edge still leaves the destructive-loss bug reachable.

Comment thread src/agents/extensions/memory/encrypt_session.py Outdated

@karaaslanz karaaslanz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original peek→pop TOCTOU is fixed, but the compensating restore still has a narrower ordering race.

Comment thread src/agents/extensions/memory/encrypt_session.py Outdated
@seratch seratch changed the title fix(memory): authenticate encrypted history before destructive pop fix(extensions): authenticate encrypted history before destructive pop Sep 20, 2026

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jbeckwith-oai jbeckwith-oai changed the title fix(extensions): authenticate encrypted history before destructive pop fix(memory): preserve encrypted SQLite history on wrong-key pops Sep 25, 2026
dpiet-oai
dpiet-oai previously approved these changes Sep 25, 2026
@1aifanatic
1aifanatic dismissed dpiet-oai’s stale review September 25, 2026 20:11

The merge-base changed after approval.

dpiet-oai
dpiet-oai previously approved these changes Sep 25, 2026
@1aifanatic
1aifanatic dismissed dpiet-oai’s stale review September 25, 2026 20:16

The merge-base changed after approval.

markstuart-oai
markstuart-oai previously approved these changes Sep 25, 2026

@markstuart-oai markstuart-oai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@1aifanatic
1aifanatic dismissed markstuart-oai’s stale review September 25, 2026 20:23

The merge-base changed after approval.

@seratch seratch added this to the 0.22.x milestone Sep 25, 2026
seratch
seratch previously approved these changes Sep 25, 2026
@seratch
seratch enabled auto-merge (squash) September 25, 2026 20:27
@jbeckwith-oai
jbeckwith-oai merged commit 0f87da2 into openai:main Sep 25, 2026
21 checks passed
@github-actions github-actions Bot mentioned this pull request Sep 28, 2026
@1aifanatic

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants