fix: bedrock creds refresh #5599
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the BedrockProvider to remove cached AWS client state and instead create a fresh client on every API call. The changes address potential credential expiry issues by ensuring credentials are re-loaded from the environment for each request.
- Removed the
clientfield fromBedrockProviderstruct - Moved AWS client creation from initialization to the
conversemethod - Updated initialization to only validate credentials without storing the client
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let sdk_config = aws_config::load_from_env().await; | ||
| let client = Client::new(&sdk_config); |
There was a problem hiding this comment.
Creating a new AWS SDK config and client on every API call introduces significant overhead. The aws_config::load_from_env() call performs I/O operations to load credentials from various sources (environment variables, config files, instance metadata, etc.). Consider implementing a caching mechanism with credential refresh logic, or leverage the AWS SDK's built-in credential provider caching to avoid this repeated overhead on each request.
There was a problem hiding this comment.
this seems like a fair point although depends on how much overhead - making an LLM call is expensive so maybe it doesn't matter?
There was a problem hiding this comment.
yeah - that was the slop comment left by goose (and copilot is just parrotting the same). I don't really know, but I don't trust this aws stuff very much and if it isn't practically that slow... it may be ok. but does seem extreme (mostly I wanted to see if someone could try it out and see if it worked in the first case). Another option is to somehow trap and re-initialise when needed.
| set_aws_env_vars(config.all_values()); | ||
| set_aws_env_vars(config.all_secrets()); | ||
|
|
||
| // Validate credentials at initialization time by loading config and checking credentials |
There was a problem hiding this comment.
this comment does not help me. adding a targeting comment to 67 saying .provide_redentials() throws an error on invalid credentials maybe?
| let sdk_config = aws_config::load_from_env().await; | ||
| let client = Client::new(&sdk_config); |
There was a problem hiding this comment.
this seems like a fair point although depends on how much overhead - making an LLM call is expensive so maybe it doesn't matter?
Removed comment about validating credentials during initialization.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* main: (33 commits) Fix Claude Code provider to default to Auto mode (#5638) (#5642) Scheduler cleanup (#5571) Better search paths and handling of CLI providers (#5554) docs: description required for "Add Extension" in cli - phase 2 (#5635) Remove some logging (#5631) Use session IDs as task IDs for subagents instead of UUIDs (#5398) Fix the naming (#5628) fix: default tetrate model is broken, replace with haiku-4.5 (#5535) (#5587) Fetch less and use the right SHA (#5621) feat(ui): add custom macOS dock menu with New Window option (#5099) feat: remove hints from recipe prompts (#5622) docs: October 2025 Community All-Stars spotlight, Hacktoberfest edition (#5625) differentiate debug/release in cache key (#5613) Unify subrecipe and subagent execution through shared recipe pipeline (#5082) Standardize CLI argument flags and update documentation (#5516) Release 1.13.0 fix: move goosehints/AGENTS.md handling to goose, and out of developer extension (#5575) fix: add standard context menu items to prevent empty right-click menu (#5616) Bump openapi in prepare-release (#5611) docs: add access control section to Developer tutorial (#5615) ...
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
ok, tested this with timeout with my setup (and refreshing) and it works. |
* main: (83 commits) silence copilot on minor text issues (block#5665) fix: disallow runaway subagent chains (block#5659) chore: remove usage of non-existent env var for log dir (block#5658) clarify agent instructions (block#5655) feat: add check-everything for unified style checks (block#5650) Show errors on failure (block#5643) custom instructions for copilot reviews (block#5646) fix: prevent repeated 404 errors when accessing deleted sessions (block#5644) Flake.nix corrected main (block#5600) fix: goose recipe list can return duplicated entries (block#5645) fix: bedrock creds refresh (block#5599) Fix Claude Code provider to default to Auto mode (block#5638) (block#5642) Scheduler cleanup (block#5571) Better search paths and handling of CLI providers (block#5554) docs: description required for "Add Extension" in cli - phase 2 (block#5635) Remove some logging (block#5631) Use session IDs as task IDs for subagents instead of UUIDs (block#5398) Fix the naming (block#5628) fix: default tetrate model is broken, replace with haiku-4.5 (block#5535) (block#5587) Fetch less and use the right SHA (block#5621) ...
* main: silence copilot on minor text issues (#5665) fix: disallow runaway subagent chains (#5659) chore: remove usage of non-existent env var for log dir (#5658) clarify agent instructions (#5655) feat: add check-everything for unified style checks (#5650) Show errors on failure (#5643) custom instructions for copilot reviews (#5646) fix: prevent repeated 404 errors when accessing deleted sessions (#5644) Flake.nix corrected main (#5600) fix: goose recipe list can return duplicated entries (#5645) fix: bedrock creds refresh (#5599)
Signed-off-by: Blair Allan <[email protected]>
a speculative fix for: #5132
hopefully by loading each time it won't be stale, but not sure of this approach as don't currently have bedrock access or familiarity.