fix: Reload propagates a failed config refresh instead of swallowing it - #511
Merged
Conversation
Agent.Reload logged a failed ec2:DescribeTags call as a warning and continued as if it had succeeded, so `spored reload` (and therefore `spawn extend`'s SSH-triggered reload) reported success while the daemon kept running on stale, possibly zero-value config. Reload now returns the refresh error, which propagates through spored reload's exit code and extend's SSH invocation. Fixes #505
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Summary
Agent.Reloadlogged a failedec2:DescribeTagscall (RefreshConfig's error) as a warning and continued, adopting whatever stale config the provider already had cached.handleReload/spored reloadthen unconditionally printed "✓ Configuration reloaded successfully", andspawn extend's SSH-triggered reload printed "✓ Configuration reloaded on instance" — both reporting success for an operation that changed nothing on the instance.Reloadnow returns the refresh error directly. This propagates correctly through the existing call chain with no other changes needed:handleReloadalready wraps and returnsReload's error,main()already exits non-zero on aRunEerror, andcmd/extend.go'striggerReloadalready reports the SSH command's exit code as a failure.Found live while diagnosing a field report for #502 — an instance's
spawn extendprinted "✓ Configuration reloaded on instance" whilespored statuson the same instance, moments later, still showedTTL: none — instance will not auto-terminate.Fixes #505.
Test plan
TestReload_SuccessandTestReload_RefreshConfigFailurePropagates— the latter verified to fail without the fix (asserted viagit stashonagent.go) and pass with itgo build ./...,go vet ./...,go test ./pkg/agent/... ./cmd/...all cleangolangci-lint run ./pkg/agent/...— no new findings in touched codeCHANGELOG.mdupdated under## [Unreleased]