Skip to content

fix(extensions): keep SQLite usage capture off the event loop - #4981

Merged
seratch merged 6 commits into
openai:mainfrom
Shy7777:fix/sqlite-usage-capture-event-loop
Sep 25, 2026
Merged

seratch merged 6 commits into
openai:mainfrom
Shy7777:fix/sqlite-usage-capture-event-loop

Conversation

@Shy7777

@Shy7777 Shy7777 commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

This pull request fixes an event-loop stall in AdvancedSQLiteSession.store_run_usage() while preserving branch and turn ownership when session operations overlap.

Summary

Move SQLite turn capture into a worker and keep capture plus the guarded usage write within the existing cancellation handling, so caller cancellation waits for storage to settle. Order capture with appends, pops, and branch deletion on the same session instance: later history changes cannot make an earlier run select a newer or replacement turn. Release that ordering barrier after capture; the existing row-anchor check protects the later write.

Keep each queued append on the branch selected when its call begins, even if a branch switch completes while it waits. Publish branch and clear generation together and capture them as one value, preventing a clear or refresh from combining an old branch with a new generation. Preserve the existing clear/reset behavior and initialize the stored generation when reopening a session.

Order branch deletion behind earlier queued appends so an append cannot recreate a successfully deleted branch. Controlled regressions cover both forced deletion and deletion after a branch switch.

Public APIs and the database schema are unchanged. Independent instances and processes retain the existing database-lock ordering; this change adds no cross-process call-entry ordering or SQLite read transaction.

Test plan

  • Added controlled regressions for lock contention, capture cancellation, branch switching, clear/replacement, reopening, and later append/pop operations; the relevant cases fail before their fixes.
  • Covered queued append targeting on empty and populated branches, coherent clear/refresh publication, queued failure/cancellation, and contention across sequential event loops.
  • All 173 AdvancedSQLiteSession tests passed.
  • Two independent reviews of the complete final diff passed with no findings.
  • The required formatting, lint, Mypy, Pyright, and full test stack passed: 10,860 passed and 60 skipped under the repository test configuration. CI is green on 7927b85b, including native macOS sandbox coverage, Linux Python 3.10–3.14, and Windows Python 3.10/3.13.

Issue number

No existing issue.

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • I've completed two independent reviews of the final diff

Copilot AI lite review requested due to automatic review settings September 12, 2026 10:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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

Copy link
Copy Markdown

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 automated review suggestions for this pull request.

Reviewed commit: 504a400486

ℹ️ 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".

Comment thread src/agents/extensions/memory/advanced_sqlite_session.py Outdated
Comment thread src/agents/extensions/memory/advanced_sqlite_session.py Outdated
@seratch seratch changed the title fix: keep SQLite usage capture off the event loop fix(extensions): keep SQLite usage capture off the event loop Sep 14, 2026

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fix. Moving usage capture off the event loop addresses a real blocking path, but the new await still allows usage to move between turns on the same branch.

While capture is pending, another coroutine can append a turn or pop and replace the original turn. Neither operation changes the clear generation. Capture then selects the newer turn and its anchor, so the write guard accepts the older result’s usage against that turn.

Please preserve the original turn’s attribution across the capture wait without restoring blocking database access. Add controlled regressions for append and pop/replacement during that wait, asserting that the newer turn’s usage remains unchanged. The existing removed/reused-turn tests pause after capture and miss this window.

@Shy7777

Shy7777 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for catching this. You're right: the earlier tests paused after capture and missed the window before it selected a turn.

Fixed in 5a7b58d. Capture now reserves its place before the first await, alongside appends and pops on the same session. Later history changes wait until that capture completes; the existing anchor check still prevents the eventual write from reaching a removed or replacement turn. The turn lookup and usage write both run in workers.

The new append and pop/replacement regressions both reproduced the issue before the fix: the older run overwrote the newer turn's 11 tokens with 80. They now pass and assert that the newer turn's usage stays unchanged. I also checked queued failure/cancellation and session reuse across event loops. All 140 session tests and the full verification stack passed (9,653 tests passed, 78 skipped).

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

Copy link
Copy Markdown

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 automated review suggestions for this pull request.

Reviewed commit: 5a7b58d31b

ℹ️ 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".

Comment thread src/agents/extensions/memory/advanced_sqlite_session.py
@jbeckwith-oai
jbeckwith-oai requested a review from a team as a code owner September 25, 2026 21:30
@seratch
seratch enabled auto-merge (squash) September 25, 2026 21:46

@markstuart-oai markstuart-oai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed e78c8e28bd4bccbe89795319a8d78f08607e00f3 against the current-main merge base ae5803f3ccd59899c14e0ee2f356614933880a68 (the effective two-file change). The off-loop capture and cancellation settlement look sound, but one branch lifecycle race remains: a later queued append can recreate a branch after deletion succeeds.

Source-only review, including the controlled concurrency tests; no local repository workloads were run. Hosted checks on this head: 18 passed, 3 still running.

Comment thread src/agents/extensions/memory/advanced_sqlite_session.py
@seratch
seratch merged commit 8e338e5 into openai:main Sep 25, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants