Repository navigation
fix(sessions): support MySQL and MariaDB schema creation - #4741
linhongyu510 wants to merge 13 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
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; |
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".
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".
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".
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".
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
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
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".
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".
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 4fb468bb9c and independently ran the live-database verification requested by @seratch. On fresh MySQL 8.0.46 and MariaDB 11.4 servers whose agents database defaulted to latin1/latin1_swedish_ci, SQLAlchemySession(..., create_tables=True) created both session_id columns as VARCHAR(190) CHARACTER SET utf8mb4 COLLATE utf8mb4_bin; the (session_id, created_at) index and cascading FK were present. Add/get/pop/clear all passed on both servers. Case-variant IDs remained distinct. A trailing-space ID was rejected on both PAD SPACE collations before histories could alias. I also pre-created a caller-managed VARCHAR(255) schema, then used the documented create_tables=True path with a 191-character session ID: CRUD passed and the existing column remained VARCHAR(255) on both servers; trailing-space IDs were likewise rejected against that existing PAD SPACE schema. Focused SQLAlchemy session suite: 62 passed. I do not see a remaining blocker on this head.
|
@sylvesterkaczmarek As I've been asking for some time, could you please refrain from proactively posting review comments on other people's PRs? Your feedback can sometimes be helpful, but in many cases, it makes the discussion and progress harder for us to follow. Please comment only when you have new, meaningful feedback that would genuinely help the contributors or maintainers. |
The collation probe previously swallowed inspection failures and returned, which let a trailing-space session_id through on a PAD SPACE column -- the exact silent-merge case the guard exists to prevent. Inspection failures now propagate, and the MySQL 5.7 fallback is keyed on error code 1054 (unknown column) so an unrelated OperationalError is no longer misread as "this server has no PAD_ATTRIBUTE column". The probe only runs for ids that actually end in a space, so the common path is unchanged.
|
@seratch Ran the live MySQL and MariaDB smoke you asked for against this head (now Setup. All three servers were started with
52 checks, 0 failures. On the DDL under a On trailing spaces — worth flagging how the pad attribute is determined, because SELECT 'trailing ' COLLATE utf8mb4_bin = 'trailing' COLLATE utf8mb4_bin;
-- MySQL 8.0.46 -> 1
-- MySQL 5.7.44 -> 1
-- MariaDB 11.8.9 -> 1All three pad, so under Caller-managed schemas — created One defect this surfaced, fixed in Repo suite on this head: Full smoke transcript — MySQL 8.0.46Full smoke transcript — MySQL 5.7.44 (no PAD_ATTRIBUTE column)Full smoke transcript — MariaDB 11.8.9 |
The outer try/except SQLAlchemyError: raise around the collation inspection did nothing -- re-raising is what happens without it. Left in place it implies the failure is being handled, which is the opposite of the contract: an inspection that cannot reach a definitive answer must propagate so the caller retries. Behavior is unchanged; the nested handler for MySQL 5.7's missing PAD_ATTRIBUTE column (error 1054) is untouched, and that is the only case where an inspection failure is legitimately recoverable. tests/extensions/memory/test_sqlalchemy_session.py: 68 passed. ruff check and ruff format --check clean. Signed-off-by: linhongyu510 <linhongyu510@gmail.com>
|
@seratch Here is the actual MySQL and MariaDB smoke result you asked for on 09-07. You were right that compiled DDL does not establish server-side outcomes — running it against live servers turned up things the DDL alone could not show. Ran against real containers, head MySQL 8.0.46 — deliberately started with a non-utf8mb4 server default: 1. Table / index / foreign-key creation — read back from The 2. add / get / pop / clear — all pass; 3. Non-utf8mb4 database default — 4. Distinct session IDs — 5. Trailing-space IDs under 6. Existing caller-managed schema ( MariaDB 11.8.9 — same script, same six sections, identical results including the trailing-space rejection. The harness drops and recreates the database per server, so the run is repeatable — I ran it twice and the output is byte-identical ( Script is at Unit tests on this head: |
The rest of the SQLAlchemySession suite runs against SQLite and mocked
engines, which cannot establish server-side outcomes. This turns the
smoke run requested in review into a test the repo can re-run.
Opt-in, following the OPENAI_RUN_LIVE_* convention already used by
tests/extensions/experimental/hosted_multi_agent/test_live.py: skipped
unless OPENAI_RUN_MYSQL_SESSION_TESTS=1 plus a server URL is set, so the
default suite is unaffected. Each URL is independent -- configuring only
MySQL or only MariaDB skips the other half rather than failing.
Covers the six items from the review:
- table / index / foreign-key creation, read back from
information_schema rather than from the emitted DDL
- add / get / pop / clear
- a non-utf8mb4 (latin1) database default, where the column-level
charset is what has to carry 4-byte characters
- an existing caller-managed schema (create_tables=False)
- case-differing session ids staying distinct under utf8mb4_bin
- trailing-space ids, which utf8mb4_bin compares equal because it is
PAD SPACE in MySQL 8; the pair must not silently share a history
Idempotent by construction: each test creates a uniquely named database
and drops it afterwards, so repeated runs are independent of leftover
rows. Verified by running three times with identical results and no
leftover agents_it_* databases.
Verified locally against MySQL 8.0.46 and MariaDB 11.8.9 (12 passed on
both), with only MySQL configured (6 passed, 6 skipped), and with no
database configured (tests/extensions/memory/ = 328 passed, 14 skipped).
Removing the PAD SPACE rejection from the session turns exactly the
trailing-space case red on both servers and nothing else.
ruff check, ruff format --check and mypy are clean on the new file.
Signed-off-by: linhongyu510 <linhongyu510@gmail.com>
seratch
left a comment
There was a problem hiding this comment.
Thanks for the additional verification. There is one remaining compatibility issue in the MariaDB padding check: utf8mb4_0900_as_cs is an alias for the NO PAD collation utf8mb4_uca1400_nopad_as_cs, but the _nopad_ substring check classifies it as PAD SPACE. This rejects trailing-space session IDs against an existing schema that correctly distinguishes them, including with create_tables=False.
Please determine padding behavior through a server-side comparison using the actual column character set and collation instead of inferring it from the collation name. Add a public CRUD regression against independently provisioned tables using this NO PAD alias, covering both values of create_tables and confirming that "tenant" and "tenant " retain separate histories.
|
Thanks again for your effort here. We won't continue working on this due to our current priorities but may revisit it in the future. |
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