Session data manager loses concurrent config updates and leaks IntegrityError on concurrent create #1543

Status 2026-10-02. #1677 (merged) made create_or_validate_session accept an existing equal row; the loser of a concurrent create still receives the raw IntegrityError, and update_session_episodic_config is unchanged. Both are step 1 of #1755: INSERT on the primary key with the violation translated, and a one-statement or versioned update.

Summary

SessionDataManagerSQL mutates session rows with read-then-write sequences that take no lock and carry no version, so two servers acting on the same session lose writes. This is not backend-specific: the read takes no lock under PostgreSQL's READ COMMITTED or SQLite's deferred transactions, so both connections read the same value before either writes.

Two distinct problems, both reproduced.

1. Lost update in update_session_episodic_config (data loss, silent)

session_data_manager_sql_impl.py:428-464 reads the whole param_data JSON blob, mutates it in Python, and writes the entire blob back:

session = ...select(SessionConfig).where(session_key == ...)   # read
param_data = dict(session.param_data)
if enabled is not None: param_data["enabled"] = enabled
...
update(SessionConfig).where(session_key == ...).values(param_data=param_data)   # write it all back

Two concurrent calls that change different flags both succeed, and the second commit discards the first one's field. Reproduced with two manager instances (two server processes) against one database:

update results:   ['ok', 'ok']          # A set long_term_memory_enabled=True, B set short_term_memory_enabled=True
final param_data: {"enabled": false, "long_term_memory_enabled": false, "short_term_memory_enabled": true}

long_term_memory_enabled was set to true by a call that returned success, and the value in the database is false. No error is raised and nothing is logged, so the caller believes the change took effect.

This is reachable from three API endpoints (server/api_v2/router.py:938, 992, 1044), so two clients toggling different memory flags on one session is an ordinary sequence, not an exotic one.

2. create_new_session leaks a raw IntegrityError instead of SessionAlreadyExistsError

session_data_manager_sql_impl.py:215-250 does SELECT-then-INSERT with no handler. session_key is the primary key, so the database does arbitrate correctly -- but the loser's exception is never translated:

concurrent create_new_session: ['ok', 'IntegrityError']

There is no IntegrityError handling anywhere in common/session_manager/ or server/api_v2/, so a concurrent create surfaces as an unhandled database error (500) rather than the intended already-exists error. The check-then-insert is a correct fast path; it just needs the race outcome mapped.

This half has already been raised as review feedback on #1539, which renames the method to create_new_session_if_not_exist without changing its body. The idempotency gate on that PR's base branch does not cover it either: the gate adds an equality check inside the if session is not None branch, which two racing workers never enter -- they both see no row and both fall through to the same unguarded insert. It makes a sequential retry idempotent, not a concurrent create.

A fix that reuses the gate: wrap the insert in try/except IntegrityError, and on conflict re-read the row and run the same equality comparison, landing in either the idempotent return or SessionAlreadyExistsError.

Suggested fixes

For 1, any of:

• Make the update atomic in one statement, so the database merges rather than the application (jsonb_set on PostgreSQL, json_set on SQLite), or
• Lock the row for the duration: select(...).with_for_update() before the read, or
• Add a version column and retry on mismatch.

Storing the three flags as real columns instead of fields inside a JSON blob would remove the read-modify-write entirely, since each UPDATE would then touch only the field it owns.

For 2, catch IntegrityError around the insert and raise SessionAlreadyExistsError, keeping the existing SELECT as the fast path.

Note

The same read-then-write-whole-row shape appears in save_short_term_memory (same file, lines 357-403), which does SELECT then UPDATE-or-INSERT with no arbiter. Whoever fixes the above may want to look at it in the same pass.

🤖 Written by Claude Code (Opus 5) on behalf of @edwinyyyu.