Skip to content

Conversation

habema
Copy link
Contributor

@habema habema commented Sep 4, 2025

Resolves #1385

This PR introduces AdvancedSQLiteSession as a drop-in replacement for SQLiteSession with enhanced conversation management capabilities. Addresses #1385 and builds on the foundation from #1474.

Key Features

  • Structured queries: Get conversations by turns, tool usage analytics, etc.
  • Usage tracking: Turn-level token usage with detailed breakdowns
  • Conversation branching: Soft deletion allows "undoing" parts of conversations and continuing from any point

New Table Schema

-- Message structure with soft deletion
message_structure (
    id, session_id, message_id, message_type, sequence_number,
    user_turn_number, tool_name, is_active, created_at, deactivated_at
)

-- Turn-level usage tracking  
turn_usage (
    id, session_id, user_turn_number, requests, input_tokens, 
    output_tokens, total_tokens, input_tokens_details, output_tokens_details
)

Usage Pattern

Currently requires manual usage storage:

result = await Runner.run(agent, "Hello", session=session)
await session.store_run_usage(result)  # Manual step

Question: Should usage storage be automatic?

Currently users need to manually call store_run_usage() after each run to get the analytics features (turn-level token tracking, usage breakdowns, etc.). It works fine but could be better UX-wise.

However, since this is an extension feature, it shouldn't require changes to the core SDK (run.py). Making it automatic could be quite a complex change - as it could need to patch the runner itself, and potentially break the clean separation between core and extensions.

What's the preference here - keep the explicit manual approach or explore automatic storage despite the complexity?

Testing Notes

Tests skip in old_versions_test due to sqlalchemy dependency in the same directory (same issue as #1357). The advanced session itself doesn't need sqlalchemy, but the import path requires it.

@habema habema changed the title Feat: Add StructuredSQLiteSession with conversation branching & usage tracking Feat: Add AdvancedSQLiteSession with conversation branching & usage tracking Sep 4, 2025
@seratch seratch added enhancement New feature or request feature:sessions labels Sep 5, 2025
@seratch
Copy link
Member

seratch commented Sep 12, 2025

@codex Can you review this PR? Share both good things and rooms for improvements.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review: Here are some suggestions.

Reply with @codex fix comments to fix any unresolved comments.

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, or 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 fix this CI failure" or "@codex address that feedback".

Comment on lines 351 to 354
def _update_sync():
conn = self._get_connection()

# Serialize token details as JSON

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Guard turn usage writes with session lock

The detailed turn-level usage tracking is helpful, however _update_turn_usage_internal acquires the shared SQLite connection and performs writes without using self._lock. For in-memory sessions that share a single connection, two concurrent calls to store_run_usage() or an overlap with add_items() can execute on the same connection from different threads, producing sqlite3.ProgrammingError/OperationalError (“database is locked”). Base session methods wrap all access in the lock for this reason; usage writes should do the same or otherwise ensure only one thread uses the connection at a time.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid, but SQLiteSession needs to reflect this as well

@habema
Copy link
Contributor Author

habema commented Sep 13, 2025

I added the active_only param to usage methods. Instead of modifying turn_usage table, I kept all usage data intact (for cost accounting) but added SQL JOINs to filter by active messages when needed.

For asyncio.Lock, I kept threading.Lock to match base class. Changing it in AdvancedSQLiteSession only will require overriding all base class methods, so the asyncio refactor should be a separate PR that touches both classes.

@habema
Copy link
Contributor Author

habema commented Sep 13, 2025

I ran into a bug in my implementation of reactivate_from_turn(), which uncovered deeper issues. I replaced the broken soft deletion approach with actual branch-based conversation management.

Key changes:

  • Added branch_id and branch_turn_number columns to schema, removed is_active/deactivated_at
  • Replaced deactivate_from_turn()/reactivate_from_turn() with create_branch_from_turn() that properly copies conversation history
  • Fixed the original bug where reactivating would affect both original and new branches
  • Added proper branch switching with switch_to_branch() and list_branches()
  • Updated unique constraints to include branch_id so turn numbers can overlap between branches

Now each branch is truly isolated - no more weird cross-branch contamination when doing operations.

I understand this might require some extensive review, so take your time.

@habema habema requested a review from seratch September 14, 2025 10:25
@habema
Copy link
Contributor Author

habema commented Sep 22, 2025

@seratch awaiting your review

@seratch
Copy link
Member

seratch commented Sep 22, 2025

@habema Thanks for being patient here. I didn't have enough time for intensively reviewing this one today, but I will do tomorrow!

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Left a few minor comments

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this!

@seratch seratch merged commit 4f54878 into openai:main Sep 24, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature:sessions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Database schema for conversations could be better
2 participants