-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(memory): make SQLAlchemySession.pop_item atomic #4206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| from collections.abc import Iterable, Sequence | ||
| from contextlib import asynccontextmanager | ||
| from datetime import datetime, timedelta | ||
| from pathlib import Path | ||
| from typing import Any, cast | ||
|
|
||
| import pytest | ||
|
|
@@ -990,3 +991,39 @@ async def test_runner_with_session_settings_override(agent: Agent): | |
| history_items = [item for item in last_input if item.get("content") != "New question"] | ||
| # Should have 2 history items (last two from the 10 we added) | ||
| assert len(history_items) == 2 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_concurrent_pop_item_hands_each_item_to_one_caller( | ||
| agent: Agent, tmp_path: Path | ||
| ) -> None: | ||
| """pop_item removes an item, so concurrent callers must never receive the same one. | ||
|
|
||
| pop_item selects the newest id, reads its payload, then deletes it in three separate | ||
| statements. Concurrent callers can therefore select the same id before either delete | ||
| lands, and both return that item while the rows they never claimed stay in the store. | ||
| """ | ||
| item_count = 40 | ||
| session_id = "concurrent_pop" | ||
| session = SQLAlchemySession.from_url( | ||
| session_id, | ||
| url=f"sqlite+aiosqlite:///{(tmp_path / 'concurrent_pop.db').as_posix()}", | ||
| create_tables=True, | ||
| ) | ||
|
|
||
| await session.add_items( | ||
| cast( | ||
| list[TResponseInputItem], | ||
| [{"role": "user", "content": f"m{index}"} for index in range(item_count)], | ||
| ) | ||
| ) | ||
|
|
||
| popped = await asyncio.gather(*[session.pop_item() for _ in range(item_count)]) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With the SQLite URL used in this test, running 40 real writers concurrently can raise AGENTS.md reference: AGENTS.md:L169-L169 Useful? React with 👍 / 👎. |
||
| contents = [item.get("content") for item in popped if item is not None] | ||
|
|
||
| assert len(contents) == item_count | ||
| # Each item belongs to exactly one caller. | ||
| assert len(set(contents)) == item_count | ||
| assert sorted(cast(list[str], contents)) == sorted(f"m{index}" for index in range(item_count)) | ||
| # And nothing is left behind by a pop that returned an item it did not delete. | ||
| assert await session.get_items() == [] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When two concurrent
pop_item()calls run under MySQL/InnoDB's default REPEATABLE READ isolation, the loser can enter a tight loop here: the transaction snapshot keeps selecting the same tail row that another transaction already deleted,DELETEreturnsrowcount == 0, and thiscontinuerepeats without ever seeing the next item. SinceSQLAlchemySessionsupportsmysql+aiomysql, retry the pop in a new transaction or take a locking/current read before looping.AGENTS.md reference: AGENTS.md:L134-L134
Useful? React with 👍 / 👎.