Skip to content

fix(advanced-sqlite-session): re-raise structure metadata write failures (fixes #3348)#3508

Open
LeSingh1 wants to merge 1 commit into
openai:mainfrom
LeSingh1:fix/sqlite-add-items-error-propagation
Open

fix(advanced-sqlite-session): re-raise structure metadata write failures (fixes #3348)#3508
LeSingh1 wants to merge 1 commit into
openai:mainfrom
LeSingh1:fix/sqlite-add-items-error-propagation

Conversation

@LeSingh1
Copy link
Copy Markdown
Contributor

Fixes #3348.

Bug

AdvancedSQLiteSession.add_items() writes rows to the base messages table, commits them, and only then writes message_structure rows. If the structure metadata write fails, the previous implementation logs the error and returns without raising:

self._insert_items(conn, items)
conn.commit()  # base rows persisted here
try:
    self._insert_structure_metadata(conn, items)
    conn.commit()
except Exception as e:
    conn.rollback()
    self._logger.error(...)
    try:
        self._cleanup_orphaned_messages_sync(conn)
        ...
    except Exception as cleanup_error:
        ...
    # no `raise` -> add_items returns None as if it succeeded

So a caller can observe a successful add_items() call even though subsequent get_items() calls join through message_structure and see nothing. The session looks empty, the caller has no way to know whether to retry, and if cleanup also fails the base table keeps invisible orphan rows.

Fix

Re-raise the original exception at the end of the outer except, after the orphan cleanup attempt. The cleanup path still runs first, so the on-disk state stays internally consistent (no orphaned base rows), and the caller now learns the call failed:

                    except Exception as cleanup_error:
                        conn.rollback()
                        self._logger.error(...)
                    # Re-raise so callers see the failure.
                    raise

One-line behavioral change.

Verification

Added test_add_items_propagates_structure_metadata_errors in tests/extensions/memory/test_advanced_sqlite_session.py. The test subclasses AdvancedSQLiteSession to make _insert_structure_metadata raise RuntimeError, calls add_items(), and asserts:

  1. add_items() propagates the RuntimeError to the caller.
  2. After the orphan cleanup runs, get_items() returns [] (no invisible orphan rows in the base table).

Local A/B:

without fix: FAILED ... Failed: DID NOT RAISE <class 'RuntimeError'>
with fix:    PASSED (37/37 tests in test_advanced_sqlite_session.py)

make format, make lint, and make typecheck on the touched files are all clean.

Scope

4-line behavior change in src/agents/extensions/memory/advanced_sqlite_session.py plus 29 lines of test coverage. No public API change.

AdvancedSQLiteSession.add_items() commits the base messages table
write, then writes message_structure rows. If the structure write
fails, the previous implementation logged the error and returned
successfully without raising. Subsequent get_items() calls join
through message_structure and see nothing, so the caller could
observe a 'successful' add_items() followed by missing reads with
no way to know whether to retry.

Re-raise the original exception after the orphan cleanup attempt
so the caller learns add_items() did not succeed. The cleanup path
that deletes the orphaned base rows still runs first, so the
on-disk state stays internally consistent.

Fixes openai#3348.
@seratch seratch added duplicate This issue or pull request already exists feature:sessions labels May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists feature:sessions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AdvancedSQLiteSession.add_items can report success after structure metadata failure

2 participants