[codex] cache state DB runtime for repeated lookups#15583
Open
charley-oai wants to merge 1 commit intomainfrom
Open
[codex] cache state DB runtime for repeated lookups#15583charley-oai wants to merge 1 commit intomainfrom
charley-oai wants to merge 1 commit intomainfrom
Conversation
3bd848f to
0d3e0fb
Compare
Collaborator
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d3e0fb47b
ℹ️ 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".
0d3e0fb to
c6e4a50
Compare
Reuse a cached StateRuntime for ad hoc core state DB lookups, but keep state_db::init() on its own runtime so session-owned work does not share a single SQLite pool. Allow no-provider open_if_present() callers to initialize and cache an ad hoc runtime so rollout path fallback and DB read-repair still work, and treat empty-provider cached runtimes as reusable for later ad hoc lookups. Prune dead cached runtimes on insert so one-off sqlite homes do not accumulate stale cache keys for the process lifetime. Co-authored-by: Codex <noreply@openai.com>
c6e4a50 to
de6fb67
Compare
Collaborator
Author
|
@codex review |
Contributor
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
get_state_db()andopen_if_present()so repeated cold metadata/path lookups can reuse an existingStateRuntimestate_db::init()creating a fresh runtime for session-owned work instead of funneling everything through one shared SQLite poolopen_if_present()callers to initialize and cache an ad hoc runtime so rollout-path fallback and DB read-repair still workinit(), and stale-cache pruningWhy
Cold thread metadata lookups were repeatedly calling
StateRuntime::init(), which opens bothstate_5.sqliteandlogs_1.sqlite. On machines with a very largelogs_1.sqlite, that made coldthread/readandthread/resumepaths pay repeated logs DB open/migration costs even when they only needed thread metadata or rollout path lookup.The first version of this change cached too broadly. Each
StateRuntimeuses a SQLite pool capped at 5 connections, so cachingstate_db::init()as well would have risked routing session-owned state DB work from multiple live sessions through one shared 5-connection pool. This PR keeps the caching benefit for ad hoc lookup-style callers while preserving separate runtimes for session-owned paths.The cache also stores
Weak<StateRuntime>values keyed bysqlite_home. Without pruning, dead entries can accumulate if many distinct homes are initialized once and never revisited. Cleaning them up on insert keeps the cache bounded in those cases.Impact
find_thread_path_by_id_str()reaches the no-provideropen_if_present()pathValidation
cargo test -p codex-core rollout::tests::find_thread_path_falls_back_when_db_path_is_stalecargo test -p codex-core rollout::tests::find_thread_path_repairs_missing_db_row_after_filesystem_fallbackcargo test -p codex-core state_db::testsjust fmtjust argument-comment-lintcargo test -p codex-corestill shows unrelated pre-existing failures in other areas; the rollout regressions above are fixedNotes
corechanges.