fix(memory): make SQLAlchemySession first writes race-safe#2725
fix(memory): make SQLAlchemySession first writes race-safe#2725seratch merged 7 commits intoopenai:mainfrom
Conversation
Head branch was pushed to by a user without write access
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06b4e9c6f2
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6928587b0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 151382e82a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61b7ace6eb
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a12dab769d
ℹ️ 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".
| _table_init_locks: ClassVar[dict[tuple[str, str, str], threading.Lock]] = {} | ||
| _table_init_locks_guard: ClassVar[threading.Lock] = threading.Lock() |
There was a problem hiding this comment.
This lock table only exists inside one Python process. In a common deployment where several workers/processes call SQLAlchemySession.from_url(..., create_tables=True) against the same empty database, each process will still reach metadata.create_all() independently, so the original table already exists/dropped first write race can still happen on first access. The new tests only exercise threads/event loops within a single process, so this fix remains incomplete for multi-worker servers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex For production deployments, create_tables=True is not recommended and this option is useful mainly for local development or demo purposes. So, I think this is fine for now. Can you review again to see if there are any other things to mention?
|
@seratch latest push is green now. I addressed the issues from the earlier review threads and replied inline with the verification details. Codex did leave one more comment about cross-process If you want this PR to cover that too, I can take a look at a DB-native approach. Otherwise I think the current branch is in a good place for your review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a12dab769d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 participants
Summary
SQLAlchemySessionso concurrent first access cannot racecreate_tables=TrueTesting
uv run pytest tests/extensions/memory/test_sqlalchemy_session.pyuv run ruff check src/agents/extensions/memory/sqlalchemy_session.py tests/extensions/memory/test_sqlalchemy_session.pyFixes #2722