fix(memory): enforce closed state in AsyncSQLiteSession - #4109
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#4035 fixed
RedisSessionandDaprSessionand stated the contract in the process:AsyncSQLiteSessionis the async sibling of the session named there as the reference, it owns the connection it opens, and it never got that treatment. It has no closed state at all:close()drops the connection but records nothing, and_get_connection()recreates a connection wheneverself._connection is None. So a closed session quietly reopens the database on the next call:All four succeed. This is the same failure #4035 describes for Redis ("later operations reconnect"), except SQLite reconnects all the way to a fresh file handle and a fresh WAL, so a caller that closed the session to release the file keeps writing to it.
Two more consequences of there being no flag:
close()before first use is not terminal at all. The earlyreturnfires while_connectionis stillNone, so nothing is recorded and the next operation opens the database normally.close()is not idempotent. Two concurrent calls both pass theis Nonecheck, then serialize on the lock; the second awaitsself._connection.close()after the first set it toNoneand raisesAttributeError: 'NoneType' object has no attribute 'close'. The new idempotency test reproduces this onmain.Fix: mirror
SQLiteSessionand #4035 exactly.close()takes the lock, marks the session terminal, then releases the connection if one was opened._locked_connection()re-checks the flag inside the lock, which coversget_items,add_items,pop_itemandclear_sessionthrough the one path they all share.add_itemsalso checks before its empty-list fast path, becauseadd_items([])would otherwise slip past — the same gap #4035 closed for Redis.The error message follows the existing convention:
RuntimeError("AsyncSQLiteSession is closed").Scope —
MongoDBSessionhas the same gap and is deliberately not in this PR. Itsclose()also records no state, so a session created viafrom_uri(the path that sets_owns_client = True) stays usable after close. I left it out because it is a different mechanism, not the same edit:MongoDBSessionhas no per-sessionasyncio.Lockto hang the flag on — it deliberately uses only athreading.Lock, and only for the class-level init registry — so making close terminal there is a design choice about what to guard rather than a copy of the SQLite idiom. Happy to send that as a follow-up in whichever shape you prefer.I also did not add an ownership branch here. Unlike Redis and Dapr,
AsyncSQLiteSessionhas no injected-connection constructor — it always opens its own connection fromdb_path— so the_owns_clientdistinction from #4035 has nothing to select on and the session is always terminal on close.Test plan
Four tests added to
tests/extensions/memory/test_async_sqlite_session.py, mirroring the ones #4035 added for Redis:test_async_sqlite_session_closed_operations_raise_runtime_errorget_items/add_items/pop_item/clear_sessionall raise afterclose()test_async_sqlite_session_closed_rejects_empty_add_itemsadd_items([])does not bypass the check via the empty-list fast pathtest_async_sqlite_session_close_before_use_is_terminalclose()before the connection is opened still makes the session terminaltest_async_sqlite_session_close_is_idempotentclose()calls are safe no-opsAll four were run against unpatched code first and fail there —
4 failed, 19 deselected, withclose_is_idempotentfailing on theAttributeErrordescribed above rather than on a missing raise. After the fix the file is23 passed(19 existing + 4 new).Full stack from the repo root:
make format— 844 files left unchanged.make lint— All checks passed.make typecheck— pyright0 errors, 0 warnings, 0 informations; mypySuccess: no issues found in 835 source files. Both run asUV_PROJECT_ENVIRONMENT=.venv_313 UV_PYTHON=3.13, since on Python 3.11 the repo has a pre-existing unrelated failure intests/test_run_step_execution.py(asyncio.eager_task_factoryis 3.12+).make tests—6084 passed, 8 skippedparallel plus38 passed, 5 skippedserial. Worth flagging honestly: an earlier run of the same suite showed one failure intests/sandbox/test_runtime.py::test_remote_realpath_guard_fails_closed_on_symlink_cycle. It passes in isolation both onmainand with this change, and the full suite is clean on re-run, so it looks flaky under parallelism and unrelated to this diff, which touches onlyAsyncSQLiteSession.Issue number
None — found by reading the sessions after #4035, so the report is in the summary above.
Checks
.agents/skills/code-change-verification/scripts/run.sh— I ran the four stages it wraps individually instead (results above). The combined script did not finish on this machine: it pins typecheck to the default 3.11 environment and runs mypy and pyright in parallel with the test suite, which I had to kill on a local time budget. That is a local environment limit, not a failing check — every stage passes when run on its own./reviewbefore submitting this PR