fix(sessions): support MySQL and MariaDB schema creation - #4741
fix(sessions): support MySQL and MariaDB schema creation#4741linhongyu510 wants to merge 9 commits into
Conversation
Signed-off-by: linhongyu510 <linhongyu510@users.noreply.github.com>
|
The |
|
Follow-up live-database verification completed against a fresh MySQL 8.0.46 instance using SQLAlchemy 2.0.43 and aiomysql 0.3.2. I exercised the actual
The run completed with |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The new MySQL/MariaDB VARCHAR(190) inherits the database’s default collation, which is commonly case-insensitive. Because session_id is the primary key, IDs such as Foo and foo can then collide or resolve as the same session even though SQLite/PostgreSQL distinguish them. Could these columns use a binary/case-sensitive collation and add a two-session case-variance regression?
|
Addressed the case-sensitivity review in
Verification on the updated head:
The change intentionally affects newly auto-created schemas only; |
![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: af3327596c
ℹ️ 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".
| # MySQL-family dialects require a bounded VARCHAR for indexed string columns. | ||
| _MYSQL_SESSION_ID_MAX_LENGTH = 190 | ||
| _SESSION_ID_TYPE = String().with_variant( | ||
| String(_MYSQL_SESSION_ID_MAX_LENGTH, collation="utf8mb4_bin"), |
There was a problem hiding this comment.
On MySQL 5.7, MariaDB, and MySQL 8 when using this PAD SPACE collation, utf8mb4_bin ignores trailing spaces in VARCHAR comparisons. Consequently, ordinary string IDs such as "tenant" and "tenant " resolve to the same primary/foreign-key value, causing separate SQLAlchemySession instances to read and write shared conversation history. Use a representation or collation that preserves full string identity, or reject trailing-space IDs before any write.
AGENTS.md reference: AGENTS.md:L202-L202
Useful? React with 👍 / 👎.
utf8mb4_bin is a PAD SPACE collation, so MySQL and MariaDB ignore trailing spaces when comparing VARCHAR values. "tenant " and "tenant" would resolve to the same primary key and silently share one conversation history, while SQLite keeps them apart - verified by test_session_ids_keep_trailing_spaces_on_sqlite. Switching to a NO PAD collation is not an option here: utf8mb4_0900_bin is MySQL 8.0.4+ only and MariaDB and MySQL 5.7 reject it as an unknown collation, which would undo this PR's MariaDB support. Reject such IDs up front instead, and reject IDs longer than the bounded VARCHAR(190) for the same reason. Only MySQL and MariaDB are checked; other backends keep the unbounded, space-significant column and their existing behavior. Leading and interior spaces stay valid - only trailing spaces are collation-significant.
|
Thanks both — the trailing-space finding is real, and I've pushed a fix in Confirming the finding. MySQL's docs are explicit that the pad attribute for So the same code would keep those two sessions apart on SQLite/PostgreSQL and silently merge them on MySQL — worse than an error, because nothing surfaces. Why I did not switch collation. The obvious fix is a Scope. Only Verification.
One note on my local environment for transparency: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62651f1660
ℹ️ 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".
| else SessionSettings() | ||
| ) | ||
| self._engine = engine | ||
| _validate_mysql_session_id(session_id, engine.dialect.name) |
There was a problem hiding this comment.
When create_tables=False—the documented production default—the caller owns the existing schema, which may validly use a wider session_id column such as VARCHAR(255). This unconditional dialect-based check now rejects a 191-character ID during construction even though that schema accepted it before this change. The MySQL dialect alone does not establish the actual column length, so restrict this validation to schemas the SDK actually creates or otherwise let the caller-managed schema enforce its own constraint.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
The length bound describes only the VARCHAR(190) column this module creates. With create_tables=False the caller owns the schema and may have declared a wider session_id, so the unconditional check rejected IDs that the caller's table stores correctly. Gate the length check on created_schema. Keep the trailing-space rejection unconditional: it follows from the column's collation, not from who created the table, and the resulting merge is silent.
|
Thanks — the Confirming the regression. With The dialect name does not reveal the real column width, so the SDK had no basis for that bound on a schema it did not create. What changed. What I deliberately kept unconditional. The trailing-space rejection still applies for both values of Tests. Added a constructor-level regression for the caller-managed case (the boundary such a caller actually crosses), parametrized the trailing-space test across both Verification: focused SQLAlchemy session suite 63 passed; |
|
One workflow note: the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 359f366098
ℹ️ 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".
| """ | ||
| if dialect_name not in _MYSQL_FAMILY_DIALECTS: | ||
| return | ||
| if session_id != session_id.rstrip(" "): |
There was a problem hiding this comment.
When create_tables=False targets a caller-managed MySQL 8 table using a NO PAD collation such as utf8mb4_0900_bin, "tenant" and "tenant " are distinct, but this dialect-only guard raises before any database access. The error incorrectly assumes every caller-managed table uses the generated utf8mb4_bin collation, breaking schemas that previously supported these IDs; restrict this validation to SDK-created PAD SPACE schemas or inspect the actual column definition.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8bc0d41. You are right that the dialect name cannot establish the real collation, and create_tables=True is not ownership evidence because create_all(checkfirst=True) leaves an existing table unchanged. I removed the dialect-wide trailing-space guard and its private helper rather than adding another heuristic. Caller-managed MySQL/MariaDB schemas (including NO PAD collations) are now left authoritative; the generated DDL remains VARCHAR(190) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin. The replacement regression test exercises the public constructor for mysql/mariadb with both create_tables values and confirms both a 191-character ID and a trailing-space ID are accepted.
| "so this ID would share stored history with " | ||
| f"{session_id.rstrip(' ')!r}." | ||
| ) | ||
| if created_schema and len(session_id) > _MYSQL_SESSION_ID_MAX_LENGTH: |
There was a problem hiding this comment.
When an existing caller-managed MySQL table has session_id VARCHAR(255) and the caller uses create_tables=True, SQLAlchemy's default create_all(checkfirst=True) leaves that table unchanged, but this condition rejects a 191-character ID before the table is inspected. This remains reachable through the documented production pattern in docs/sessions/index.md, which uses create_tables=True with existing databases. Fresh evidence after the earlier review comment is that the current code passes the raw create_tables flag as created_schema, although that flag only requests idempotent creation and does not establish that the SDK created the table.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
The length bound was gated on `create_tables`, but that flag only requests idempotent creation: `create_all` uses `checkfirst`, so it leaves an existing table untouched. `create_tables=True` against an existing database - the pattern `docs/sessions/index.md` recommends for production - therefore does not mean this module generated the column, and a caller whose table declares `session_id VARCHAR(255)` had a valid 191-character ID rejected during construction. Drop the bound entirely rather than trying to infer schema ownership. MySQL authoritatively rejects an over-long value with ERROR 1406 under the strict `sql_mode` that is the modern default, and nothing is persisted before that rejection, so a client-side copy of the check only adds a false negative for wider caller-managed columns. The trailing-space check stays, and no longer depends on schema ownership. Its failure mode is different in kind: the database reports nothing, and two sessions silently end up sharing one conversation history. There is no authoritative rejection to defer to, so it is still rejected before any write. `_MYSQL_SESSION_ID_MAX_LENGTH` remains the width of the generated column. The constructor test now covers both values of `create_tables`, so the `create_tables=True` path that motivated this change is pinned; reverting the fix fails 4 of its cases. Co-authored-by: Claude <noreply@anthropic.com>
|
Both P2 comments are correct, and the second one caught a real hole in my previous attempt. Fixed in
The table is still So I dropped the length bound entirely instead of trying to infer ownership from a wider signal. Inferring it correctly needs the real column definition, which needs an async round trip; the validation runs in the synchronous The trailing-space check stays, and no longer depends on ownership. Its failure mode is different in kind, which is why I did not remove both: there is no authoritative rejection to defer to. The database reports nothing at all, and two sessions silently end up sharing one conversation history — the persistent-corruption case AGENTS.md:140 carves out, and the "must not hide valid history" concern in Verification.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01e48e6f4d
ℹ️ 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".
| # MySQL-family dialects require a bounded VARCHAR for indexed string columns. | ||
| _MYSQL_SESSION_ID_MAX_LENGTH = 190 | ||
| _SESSION_ID_TYPE = String().with_variant( | ||
| String(_MYSQL_SESSION_ID_MAX_LENGTH, collation="utf8mb4_bin"), |
There was a problem hiding this comment.
When MySQL or MariaDB runs without strict SQL mode, this generated VARCHAR(190) does not authoritatively reject a 191-character session_id; it truncates it with a warning. add_items() can therefore persist the parent and messages under the truncated ID, while later queries using the original ID return no history, and two IDs sharing the first 190 characters can write into the same stored session. Fresh evidence after the earlier length-validation comments is that the current revision removes the length check even for a table that was actually created with this 190-character definition; inspect the real column after create_all() and reject oversized IDs before writes when its width is 190, while continuing to permit wider caller-managed columns.
AGENTS.md reference: AGENTS.md:L140-L140
Useful? React with 👍 / 👎.
| # MySQL-family dialects require a bounded VARCHAR for indexed string columns. | ||
| _MYSQL_SESSION_ID_MAX_LENGTH = 190 | ||
| _SESSION_ID_TYPE = String().with_variant( | ||
| String(_MYSQL_SESSION_ID_MAX_LENGTH, collation="utf8mb4_bin"), |
There was a problem hiding this comment.
On a MySQL or MariaDB database whose default character set is not utf8mb4—including a default MySQL 5.7 installation—this emits VARCHAR(190) COLLATE utf8mb4_bin without declaring the column or table character set. The column inherits the database character set, and the server rejects utf8mb4_bin as incompatible with that inherited character set, so create_tables=True still fails during create_all() before either table is available. Declare CHARACTER SET utf8mb4 together with this collation, or use a representation whose collation does not depend on the database default.
Useful? React with 👍 / 👎.
The MySQL variant emitted `VARCHAR(190) COLLATE utf8mb4_bin` with no character
set. A column given only a collation inherits the database character set, and
the server rejects a collation that does not belong to that set with
ERROR 1253 COLLATION 'utf8mb4_bin' is not valid for CHARACTER SET '<set>'
so on an install whose default is not utf8mb4 -- a stock MySQL 5.7 defaults to
latin1 -- create_all() failed before either table existed. Switching to
mysql.VARCHAR lets the charset be declared alongside the collation.
SQLite and PostgreSQL are unaffected; they still receive the unbounded VARCHAR
from the base type. Updated the schema-compilation test to assert the character
set is present, so removing it again fails.
Co-authored-by: Claude <noreply@anthropic.com>
|
Thanks — two new P2s on Taken:
|
Remove the dialect-only trailing-space guard. A MySQL or MariaDB dialect does not reveal the actual collation of an existing session table, and create_tables=True is not proof that the SDK created it because SQLAlchemy's create_all uses checkfirst by default. Caller-managed NO PAD schemas can preserve trailing-space IDs, so rejecting those IDs in the synchronous constructor broke a valid existing setup. Keep the generated utf8mb4_bin schema fix, but leave validation of values against an existing schema to the database that owns it. Replace helper-level tests with public-constructor coverage for both create_tables modes and both MySQL-family dialect names.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Re-reviewed the MySQL/MariaDB case-sensitivity finding. Auto-created session-id columns now use VARCHAR(190) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin, with DDL and case-variant session regressions. My previous blocker is resolved.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
The current head is substantially narrower: it fixes the MySQL-family column types without imposing the earlier constructor-level restrictions on caller-managed schemas.
Before merging, please provide an actual MySQL and MariaDB smoke result for this exact head, covering table/index/foreign-key creation, add/get/pop/clear, a non-utf8mb4 database default, and an existing caller-managed schema. Please also record how distinct session IDs, including trailing-space IDs under utf8mb4_bin, are handled. Compiled DDL alone does not establish those server-side outcomes.
…ions The generated MySQL/MariaDB schema uses utf8mb4_bin, which is PAD SPACE: trailing spaces are insignificant in VARCHAR comparisons, so "tenant" and "tenant " silently resolve to the same primary key and share one conversation history with no error anywhere. 8bc0d41 removed the constructor-side guard because a dialect name cannot distinguish a SDK-created PAD SPACE column from a caller-managed NO PAD column. Move validation into _ensure_tables(), where create_all() has already run and we can query the actual column collation: - MySQL reads PAD_ATTRIBUTE from information_schema.COLLATIONS. - MariaDB does not expose PAD_ATTRIBUTE; its _nopad_ collation family is NO PAD and ordinary collations are PAD SPACE. - Unknown or unavailable collation metadata is left untouched (conservative). - create_tables=False, SQLite, and PostgreSQL bypass validation entirely. - No session-ID length validation is reintroduced. A caller-managed NO PAD schema therefore keeps accepting trailing-space IDs, while the SDK's own generated utf8mb4_bin schema fails fast before any history is written.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b61d2cad1
ℹ️ 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".
| pad_result = await conn.execute( | ||
| sql_text( | ||
| "SELECT PAD_ATTRIBUTE FROM information_schema.COLLATIONS " | ||
| "WHERE COLLATION_NAME = :collation" | ||
| ), |
There was a problem hiding this comment.
On MySQL 5.7, information_schema.COLLATIONS does not expose PAD_ATTRIBUTE, so this query raises an unknown-column error and the broad SQLAlchemyError handler returns without rejecting the ID. Consequently, a generated utf8mb4_bin schema still aliases IDs such as "tenant" and "tenant ", allowing separate sessions to share history. Fresh evidence beyond the earlier trailing-space report is that the newly added metadata query itself is unavailable on a supported target version; use a version-compatible padding determination rather than failing open.
AGENTS.md reference: AGENTS.md:L202-L202
Useful? React with 👍 / 👎.
|
|
||
| async with self._engine.begin() as conn: | ||
| await conn.run_sync(self._metadata.create_all) | ||
| await self._validate_session_id_collation(conn) |
There was a problem hiding this comment.
When an SDK-generated MySQL/MariaDB schema is reopened with the default create_tables=False—for example after an application restart—this is the only call to the collation validator, and _ensure_tables() returns before reaching it. A trailing-space ID can therefore again alias an existing unpadded ID in the generated utf8mb4_bin table and share its history. Fresh evidence after the earlier collation comments is that the implementation can now inspect the actual column, so perform that inspection for trailing-space IDs outside the creation-only branch while continuing to permit caller-managed NO PAD schemas.
AGENTS.md reference: AGENTS.md:L147-L147
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fb468bb9c
ℹ️ 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".
| except SQLAlchemyError: | ||
| return |
There was a problem hiding this comment.
When a trailing-space ID is used and an information_schema or version query raises a transient SQLAlchemyError—or a proxy blocks metadata queries—this handler returns normally, so _ensure_tables() marks _session_id_collation_validated as true. Subsequent CRUD can succeed through another connection against a PAD SPACE table, allowing IDs such as "tenant" and "tenant " to share history permanently without retrying validation. Fresh evidence beyond the earlier MySQL 5.7 comment is that the version-specific fallback now exists, but the outer handler still treats every other inspection failure as successful validation; propagate/retry the error or only cache a definitive result.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
3 participants
This pull request fixes automatic SQLAlchemy session table creation on MySQL and MariaDB. Both dialects reject an unbounded
Stringwhen compiling indexedVARCHARcolumns, soSQLAlchemySession(create_tables=True)currently fails before issuing any DDL.Summary
VARCHAR(190)for bothsession_idcolumns on MySQL and MariaDB.VARCHARtype on PostgreSQL and SQLite.(session_id, created_at)index remains within the traditional 767-byte InnoDB key limit underutf8mb4.create_tables=Trueproves the SDK created an existing table, becausecreate_all(checkfirst=True)leaves it untouched.MetaData.create_all()regression tests for MySQL and MariaDB covering both tables,ON DELETE CASCADE, and the full composite index.Test plan
./.venv/bin/python -m pytest tests/extensions/memory/test_sqlalchemy_session.py -q(52 passed)./.venv/bin/ruff format ...and./.venv/bin/ruff check --fix ...on both changed files (passed).agents/skills/code-change-verification/scripts/run.shlitellm,temporalio, andhttpxdependencies plus existing sandbox type-alias errorsLive database verification
session_idcolumns asVARCHAR(190), composite(session_id, created_at)index, and session clearing verified.innodb_large_prefix=OFF,ROW_FORMAT=COMPACT, andutf8mb4: automatic schema creation and message round-trip succeeded with a 190-character, 760-byte emoji session ID.SHOW CREATE TABLEconfirmedVARCHAR(190), the composite(session_id, created_at)index,ON DELETE CASCADE, and COMPACT row format.information_schemaconfirmed bothsession_idcolumns areVARCHAR(190), the composite index order is correct, the foreign key usesON DELETE CASCADE, and deleting the parent session removed its messages.All runs used ephemeral local containers and required no OpenAI API call.
Issue number
Closes #4740
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR