Surface secret fetch retry warnings in job output#3714
Open
Conversation
The buffer logger passed to secrets.FetchSecrets was never flushed, so retry warnings (HTTP 502, 429, TLS timeouts) were silently swallowed. Steps would appear to hang with no explanation. After FetchSecrets returns, we now iterate the buffer's messages and forward any warnings to the shell logger so they appear in the job log. Fixes A-959 Amp-Thread-ID: https://ampcode.com/threads/T-019c78c7-5821-71f5-a011-2fe9f58b9cb4 Co-authored-by: Amp <amp@ampcode.com>
zhming0
requested changes
Feb 23, 2026
Contributor
zhming0
left a comment
zhming0
left a comment
There was a problem hiding this comment.
I can see the rationale 👍🏿 , but I think it might be worth to re-look this implementation in security perspective.
Comment on lines
+918
to
+920
| // Fetch all secrets. We pass secretLogger (a buffer) here because | ||
| // FetchSecrets takes a logger.Logger, but retry warnings within it need | ||
| // to reach the user. We flush those from the buffer afterwards. |
Contributor
There was a problem hiding this comment.
This entire comment read a bit logically odd and misplaced 😅
We pass secretLogger (a buffer) here because FetchSecrets takes a logger.Logger, but retry warnings...
And i think this comment should just be added around the flushing code rather than here.
| if after, ok := strings.CutPrefix(msg, "[warn] "); ok { | ||
| e.shell.Warningf("%s", after) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Is this partially defeating the purpose of using Buffer logger? We haven't mutate redactor at this point, so we are running the risk of exposing secrets to log here.
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
2 participants
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.
Description
The buffer logger (
logger.NewBuffer()) passed tosecrets.FetchSecretswas never flushed, so when secret fetching hit retryable errors (HTTP 502, 429, TLS timeouts), thel.Warn()calls about retries went into the buffer and vanished. Users would see their step appear to hang with no explanation of what was happening.The buffer logger was originally introduced in #3453 to satisfy
api.NewClient's logger requirement and suppress debug HTTP header noise. When retry logic was added in #3706,FetchSecretsreused the same buffer logger without accounting for the fact that it now produces meaningful warnings.After
FetchSecretsreturns, we now iterate the buffer's messages and forward any warnings to the shell logger so they appear in the job output.Context
A-959
Changes
After
FetchSecretscompletes, warning messages from the buffer logger are forwarded to the shell logger. The API client keeps its buffer logger to suppress debug noise.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Disclosures / Credits
Amp wrote the implementation.