Skip to content

fix(session): create inventory tables atomically with schema version - #10586

Merged
filipkujawa merged 1 commit into
mainfrom
investigate/ci-flake-custom-defaults
Jul 20, 2026
Merged

fix(session): create inventory tables atomically with schema version#10586
filipkujawa merged 1 commit into
mainfrom
investigate/ci-flake-custom-defaults

Conversation

@filipkujawa

@filipkujawa filipkujawa commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Problem

test_custom_defaults_save_allows_unlisted_model flaked on main CI (run 29714661659) with a JSON-RPC internal error: no such table: provider_inventory_entries.

Root cause: SessionStorage::create_schema committed the main schema, including the schema_version = CURRENT row, and only then created the provider inventory tables outside the transaction. If schema init is interrupted in that window, the database permanently claims the current schema version while missing the inventory tables, and run_migrations never repairs it.

The ACP tests hit this window reliably enough to flake: the server eagerly spawns storage.pool() init as a detached task, each test's tokio runtime drop cancels it at an await point, and all ACP tests in a binary share one sessions DB.

Reproduced with a 400-iteration cancellation-sweep stress harness: 10/400 iterations failed with the exact CI error before the fix, 0/400 after.

Fix

  • Create the inventory tables inside the same BEGIN IMMEDIATE transaction as the rest of the schema, so all DDL commits atomically with the version row.
  • Remove the now-dead pool-based create_tables variant (the transactional one takes its name) and the unused SessionStorage::create, which stamped the current version without running migrations.

Testing

  • session_manager + inventory unit tests, acp_custom_requests_test (run repeatedly), cargo clippy --all-targets -- -D warnings, and cargo fmt all pass.

@filipkujawa
filipkujawa requested a review from michaelneale July 20, 2026 19:04
Comment thread crates/goose/src/providers/inventory/mod.rs Outdated
Comment thread crates/goose/src/session/session_manager.rs Outdated
Comment thread crates/goose/src/session/session_manager.rs Outdated
Comment thread crates/goose/src/session/session_manager.rs Outdated

@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: 9048814417

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread crates/goose/src/session/session_manager.rs
@filipkujawa
filipkujawa added this pull request to the merge queue Jul 20, 2026
@filipkujawa
filipkujawa removed this pull request from the merge queue due to a manual request Jul 20, 2026
create_schema committed the schema_version row before creating the
provider inventory tables outside the transaction. An interruption in
that window (e.g. the ACP server's eagerly spawned pool-init task being
cancelled on runtime shutdown) left a database claiming the current
schema version with no inventory tables, which run_migrations never
repaired. This surfaced as a flaky "no such table:
provider_inventory_entries" failure in acp_custom_requests_test.

Create the inventory tables inside the same BEGIN IMMEDIATE transaction
and remove the now-dead pool-based variant and the migration-bypassing
SessionStorage::create.
@filipkujawa
filipkujawa force-pushed the investigate/ci-flake-custom-defaults branch from 9048814 to e937ab9 Compare July 20, 2026 19:59
@filipkujawa
filipkujawa added this pull request to the merge queue Jul 20, 2026
Merged via the queue into main with commit d5a8a3f Jul 20, 2026
36 of 38 checks passed
@filipkujawa
filipkujawa deleted the investigate/ci-flake-custom-defaults branch July 20, 2026 21:24
michaelneale added a commit that referenced this pull request Jul 20, 2026
* origin/main: (24 commits)
  fix(session): create inventory tables atomically with schema version (#10586)
  fix(providers): rewrite oneOf to anyOf in tool schemas for OpenAI-compatible backends (#10571)
  fix(evals): report cache-aware Harbor costs (#10430)
  fix(acp): allow custom model as default for non-local providers (#10438)
  fix(config): require absolute goose path roots (#10454)
  chore(deps): bump astral-sh/setup-uv from 8.2.0 to 8.3.2 (#10541)
  fix(permissions): scope smart approval by request (#10457)
  fix(summon): preserve fixed subrecipe values (#10452)
  chore(deps): bump websocket-driver from 0.7.4 to 0.7.5 in /documentation (#10506)
  fix(flatpak): bundle git so hermit can clone its package registry (#10511)
  feat(hooks): pass working_dir to the Stop hook context (#10296)
  chore(deps): bump actions/setup-java from 5.5.0 to 5.6.0 (#10540)
  chore(deps): bump actions/setup-node from 6 to 7 (#10539)
  chore(deps): bump EmbarkStudios/cargo-deny-action from 2.0.20 to 2.1.1 (#10542)
  chore(deps): bump gradle/actions/setup-gradle from 4.4.3 to 6.2.0 (#10543)
  Add declarative Sakana AI provider for the OpenAI-compatible Fugu API (#10357)
  fix(developer): expose AGENT_SESSION_ID to shell commands (#10428)
  Clean up stale documentation audit findings (#10114)
  Restore model interactions viewer (#10205)
  fix(acp): forward image content chunks to client during live session (#10485)
  ...

# Conflicts:
#	crates/goose/src/session/session_manager.rs
leanzero-srl pushed a commit to leanzero-srl/goose-local-edition that referenced this pull request Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants