fix: cache ACP configs by path - #8893
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36d4ff286a
ℹ️ 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".
|
AI comment: This removes the agent-owned config path, so If ACP should always use process-global config, should we remove |
70382ad to
66b8f26
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66b8f26323
ℹ️ 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".
matt2e
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36d2e72982
ℹ️ 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".
| let config = Arc::new(Self::new(config_path, KEYRING_SERVICE)?); | ||
| cache.insert(cache_key, Arc::clone(&config)); |
There was a problem hiding this comment.
Config::for_config_dir now caches a single Config instance per path, but Config::new decides SecretStorage once from GOOSE_DISABLE_KEYRING/config at construction time. After an ACP client updates GOOSE_DISABLE_KEYRING via _goose/config/upsert, later secret RPCs keep using the old backend (keyring vs secrets.yaml) until restart, whereas the previous per-call Config::new(...) path re-evaluated this setting each request. This can make secret reads/writes fail or go to the wrong store immediately after a runtime toggle.
Useful? React with 👍 / 👎.
36d2e72 to
36b5244
Compare
There was a problem hiding this comment.
💡 Codex Review
goose/crates/goose/src/acp/server.rs
Lines 2438 to 2440 in c54d972
Invalidate cached secrets before rebuilding providers
Now that ACP reuses a cached Config handle, this provider rebuild path can read stale secrets_cache entries after out-of-band credential rotation (for example, keyring/secrets file updated while the process stays up). Before this commit, these flows created a fresh Config per request, so model/provider changes saw current secret values. Add config.invalidate_secrets_cache() before create_provider(...) here (and in update_provider) so set_model/provider switches don't continue using stale credentials.
ℹ️ 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".
Share Config handles by normalized config path so ACP custom directories use the same mutex and secrets cache without forcing them through the global singleton. Store the selected handle on GooseAcpAgent and use it for config, secret, and provider/model resolution paths. Add regression coverage for default/global config, custom directory coexistence, path normalization, shared state, and concurrent upserts. Signed-off-by: Matt Toohey <contact@matttoohey.com>
c54d972 to
e240c37
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e240c373ba
ℹ️ 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".
| let config = Arc::new(Self::new(config_path, KEYRING_SERVICE)?); | ||
| cache.insert(cache_key, Arc::clone(&config)); | ||
| Ok(ConfigHandle::Cached(config)) |
There was a problem hiding this comment.
Caching a single Config instance per path here means secret backend selection is frozen at first use, but handle_keyring_fallback_error can later set GOOSE_DISABLE_KEYRING=1 at runtime when keyring access fails. In ACP custom-config deployments that start in keyring mode and then hit that fallback, subsequent requests keep reusing the stale keyring-backed Config instead of switching to file-backed secrets for that config directory, so secret reads/writes continue through the fallback path rather than the intended custom secrets.yaml location. Before this change, recreating Config::new(...) per request re-evaluated the mode and recovered on the next call.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale because it has not had recent activity for 23 days. What happens next?
Thank you for your contribution! 🚀 |
|
This pull request has been automatically closed due to inactivity. Why was this closed?
Want to reopen?
Thank you for your contribution! We appreciate your effort. 🙏 |
2 participants
Now that SACP handles requests concurrently, the previous approach of creating a new Config per request could have race conditions if multiple requests tried to change config.
Summary
This fixes ACP config races by sharing a
Confighandle for each config path instead of creating a new config object per request. The default Goose config directory still usesConfig::global(), while custom ACP config directories use a process-wide cache keyed by the normalizedconfig.yamlpath.Config::for_config_dir(...)andConfigHandleto select either the default global config or a cached custom-path config.GooseAcpAgentand reuse it for session setup, provider/model resolution, config CRUD, and secret requests.AcpServercreation to read Goose mode from the same path-aware config handle.