fix(memory): enforce closed state in MongoDBSession - #4172
Conversation
close() released the client but recorded nothing, so every operation after it ran against a released client instead of raising: get_items returned history, add_items wrote, pop_item mutated, clear_session cleared. close() before first use was not terminal, and ping() reported connectivity for a session that had none. Mirror openai#4035 and openai#4109. close() marks the session terminal before releasing the client, and the check goes in _ensure_indexes(), the one path all four operations share. add_items checks before its empty-list fast path, and ping() checks outside its try, which would otherwise swallow the RuntimeError. The flag is a plain boolean rather than the siblings' asyncio.Lock: this class documents that it never uses one, because an asyncio.Lock binds to the event loop that first acquires it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffd5b76c01
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| via :meth:`from_uri`). In that case the session becomes terminal and | ||
| subsequent operations raise ``RuntimeError``. If the client was injected | ||
| externally the caller is responsible for managing its lifecycle and this |
There was a problem hiding this comment.
Update MongoDB lifecycle docs for terminal close
When this newly terminal owned-client branch ships, users reading docs/sessions/index.md still only learn that from_uri(...) closes the AsyncMongoClient, with no mention that subsequent MongoDB session operations and ping() now raise RuntimeError; please update the live MongoDB sessions docs alongside this user-facing lifecycle change so the public guidance matches the runtime behavior.
Useful? React with 👍 / 👎.
Summary
MongoDBSession.close()releases the client but records nothing, so a closed session keeps working. This applies the rule #4035 established and #4109 extended, to the last session backend that does not follow it.#4035's summary states the contract: "SQLiteSession makes close() terminal: it sets
_closedunder the session lock, and every operation re-checks that flag inside the same lock." It fixed Redis and Dapr; #4109 fixedAsyncSQLiteSession.MongoDBSessionis now the only backend left that definesclose()without it —SQLAlchemySession,EncryptedSessionandOpenAIConversationsSessiondefine noclose()at all, so this closes the class.What goes wrong today, for a
from_urisession that owns its client:Every operation runs against a released client instead of raising.
close()before first use is not terminal either, andping()reports connectivity for a session that has none.The fix mirrors #4035 and #4109.
close()marks the session terminal before releasing the client, and the check goes in_ensure_indexes()— the one pathget_items,add_items,pop_itemandclear_sessionall share, so it is a single insertion rather than four.add_itemsalso checks before its empty-list fast path, the same gap #4035 closed for Redis.ping()checks outside itstry, since itsexcept Exceptionwould otherwise swallow theRuntimeError; that comment is lifted fromRedisSession.pingwhich had the identical trap. The injected-client case stays a no-op and stays usable, matchingRedisSession: if you passed the client in, its lifecycle is yours.One judgment call, and it is why this backend was skipped before. The siblings hang the flag on a per-session
asyncio.Lock. This class deliberately has none, and says why:So I did not import that pattern.
close()sets a plain boolean before awaiting the release, which is a single atomic store and needs no lock, and it does not null the client out — so unlikeAsyncSQLiteSessionthere is no attribute for a concurrent operation to trip over, and no_client_releasedflag is needed becauseAsyncMongoClient.close()tolerates repeated calls. That also means a failed or cancelled release leaves the session terminal and a laterclose()simply retries. Adding a lock or a second flag here would be machinery with no requirement behind it.Out of scope, named so it is clear it was seen and not missed: the
session_limit <= 0divergence across backends is untouched — that is a design question you have already declined twice (#3244, #3960), and nothing here needs it settled.Test plan
Six tests added to
tests/extensions/memory/test_mongodb_session.py, all run against unpatched code first:test_closed_operations_raise_runtime_error—get_items,add_items,pop_item,clear_sessiontest_closed_rejects_empty_add_items— the empty-list fast pathtest_close_before_use_is_terminaltest_close_is_idempotenttest_ping_on_closed_session_raisestest_external_client_session_stays_usable_after_close— passes both before and after by design; it exists to hold the ownership branch that did not changeFail-first:
5 failed, 3 passed, 35 deselectedon unpatched code, the five being the new closed-state assertions and the three being the two pre-existingclose()tests plus the ownership test above. After:43 passedin that file.Full stack:
make format— 844 files unchangedmake lint— cleanmake typecheck— pyright 0 errors; mypy clean on.venv_313(593 source files). On the default 3.11 env mypy reports the pre-existingtests/test_run_step_execution.py:1466: Module has no attribute "eager_task_factory", which is unrelated to this diff and present onmain.make tests—6090 passed, 8 skipped+38 passedserial.mainis6084 passed, so +6, which are the six tests added. Zero regressions.One thing I want to flag rather than bury. An earlier full-suite run of mine showed a single failure in
tests/sandbox/test_runtime.py::test_remote_realpath_guard_fails_closed_on_symlink_cycle. It passes in isolation, the whole file passes under-n autoon cleanmain, the full suite is green on cleanmain, and the full suite is green on re-run with this change. So it is intermittent under parallelism and not this diff — but I hit it once, so someone else will, and I would rather say so than quietly re-run until green. It was seen once before, in #4109.I did not run
.agents/skills/code-change-verification/scripts/run.shto completion: it pins typecheck to the default 3.11 env and runs mypy and pyright in parallel with the full suite, which wedges this machine past a sensible time budget. I ran the four stages individually instead, as above, and have left that box unchecked.Issue number
n/a
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR