Skip to content

Commit af511d8

Browse files
ostermanclaudeatmos-pro[bot]
authored
feat: stack-level retry defaults + registry-retry docs + CI flakes (#2987)
* docs: document GOAWAY provider-registry retry scenario Extend the component retry docs with the HTTP/2 GOAWAY provider-registry failure mode and cross-link with the terraform cache docs, so users know `retry:` (not the registry cache) is what recovers a batch of components that all hit a transient registry connectivity blip at once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: rewrite GOAWAY retry docs in ASD-STE100 style Simplify the new provider-registry-failure prose from the previous commit into short, active, single-idea sentences per ASD-STE100 conventions, with no loss of information. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: add retry-policy mixin use-case, fix stale mixins link - Add a "Mixins for Retry Policies" use-case to howto/mixins.mdx, showing how overrides.retry shares one retry policy across components that don't have a common base component. - Fix howto/mixins.mdx's "Learn Design Pattern" link: it pointed to /design-patterns/component-catalog/with-mixins, which was repurposed into the Component Archetypes page and explicitly says "This is NOT Mixins". Point it at the actual mixins design-pattern page instead. - Document `retry` as a supported overrides.* field in component-overrides.mdx (already implemented in internal/exec/stack_processor_process_stacks_helpers_overrides.go but undocumented). - Cross-link retry.mdx <-> howto/mixins.mdx and add a reciprocal link from the mixins design-pattern page back to component retry and component overrides. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: cover overrides.retry end-to-end via the mixin-overrides fixture Extend TestDescribeComponentWithOverridesSection and the atmos-overrides-section fixture with a retry block, proving the exact scenario documented for sharing a retry policy via a mixin: a `terraform.overrides.retry` block in an imported file applies to components imported after it (test3), and correctly does not apply when imported after the component (test2) — matching the existing file-scoping behavior already proven for `overrides.vars`. This closes the gap between the unit-level merge-precedence tests in stack_processor_merge_test.go (which prove overrides wins in isolation) and an end-to-end proof that overrides.retry flows through real import resolution the same way overrides.vars does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fix): qualify retry batch-recovery claim in retry.mdx "Any of the three lets the whole batch recover on its own" overstated the guarantee. Each subprocess retry loop is independent and bounded by its own max_attempts/max_elapsed_time (already documented in "How it works"), so a component whose registry outage outlasts its own budget still fails even with retry configured. Clarify that recovery is per-component and budget-bound, and that a long enough outage can still fail part or all of a batch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: wire overrides.provision into component overrides merge `provision` is a real per-component section (workdir/target delivery settings for helmfile/kubernetes/helm/terraform/packer components) but was never extracted by processComponentOverrides, so overrides.provision silently no-op'd instead of erroring or applying. Wire it in the same way retry was: extract it (gated by supportsSourceProvision, matching the merge's existing gate), add a dedicated sentinel error for a malformed value, and merge it in as the highest-precedence layer (global -> base -> component -> overrides), consistent with every other overrides.* field. Document `provision` in the "What You Can Override" list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): widen shrunk kubernetesReadyTimeout to stop CI flake TestManager_WaitKubernetesReady_RetriesUntilReady failed on the windows CI shard: "missing call(s) to *emulator.MockRuntime.Exec" for the retry loop's second attempt. Root cause is test timing, not a product bug — waitKubernetesReady correctly bounds by deadline, but the test's 50ms shrunk kubernetesReadyTimeout has to survive two real gomock-backed attempts, and the failing run took 0.31s total (300ms+ for a single mocked attempt is plausible under CI scheduler contention), so the deadline expired before the loop's second iteration ever ran. Widen the shrunk timeout to 2s. Poll interval stays at 1ms, so a passing run still finishes in low single-digit milliseconds — this only adds headroom against CI jitter, not requirement laxity. Verified with `go test -run TestManager_WaitKubernetesReady_RetriesUntilReady -count=5`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): retry copyRepoWithRetry on Windows file-lock errors too TestDescribeAffectedWithDependentsStackFilterYamlFunctions failed on the windows CI shard: "The process cannot access the file because another process has locked a portion of the file" reading a shared fixture's terraform.tfstate mid-copy. copyRepoWithRetry already retries when a source file vanishes mid-copy (git background housekeeping racing the walk), but only checked os.IsNotExist. It copies the live, shared repo tree, so on Windows another concurrently running test can legitimately hold one of those files open at the exact moment this copy walks it -- Windows enforces mandatory file locking far more strictly than Unix, so the read fails outright instead of racing cleanly. Same class of issue already handled for `git worktree remove` in pkg/git/worktree.go via string matching on the OS error text; apply the same idiom here. Added TestIsTransientRepoCopyError covering both this file's known transient causes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: convert provision-overrides test to table-driven form Addresses CodeRabbit review comment on PR #2987: the three provision-override scenarios (valid, non-map error, unsupported component type) had repeated setup boilerplate. Consolidate into a single table-driven test, matching the existing convention already used by the very next function in this file (TestProcessComponentInheritance). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: use errors.Is for missing-file detection in copy-retry helper Addresses CodeRabbit review comment on PR #2987: os.IsNotExist does not reliably unwrap wrapped errors (it predates errors.Is and only special-cases raw *PathError/*LinkError/*SyscallError). Replace with errors.Is(err, os.ErrNotExist) in isTransientRepoCopyError so a wrapped missing-file error from a future otiai10/copy version (or any other wrapping layer) is still classified as transient. Converted TestIsTransientRepoCopyError to table-driven form per the same review comment, and added a case covering a wrapped os.ErrNotExist to guard against regressing back to os.IsNotExist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): classify closed-pipe errors as clean shutdown in session helper TestRunSessionDefaultsNilOptions failed on Windows CI ("RunSession with nil opts: exit status 1") well before its context timeout (5.8s of a 10s budget), so the earlier 3s->10s timeout bump for this test was treating the wrong symptom -- the child helper process itself was exiting 1, not timing out. runAsciicastSessionHelper (the test-binary-as-fake-shell used by session tests) exited 1 on any stdin read error other than a literal io.EOF. finishSession's ordinary teardown sends EOT then closes the input pipe; under Windows CI load that race can surface as io.ErrClosedPipe (or an "input/output error") instead of a plain io.EOF, which the helper had never seen before this change. The codebase already classifies exactly this error set as an expected clean shutdown for the parent's stdout-read loop via isExpectedSessionReadError (session.go) -- reuse it here instead of duplicating a narrower, incorrect check. Verified with `go test ./pkg/asciicast/... -run TestRunSession -count=5`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat: support stack-root global retry, like metadata/hooks/provision `retry:` was the only major component section without a stack-level global default. metadata, hooks, generate, source, and provision all support a stack-manifest-root block that cascades into every component of that type, via a Global*Section layer threaded through ComponentProcessorOptions and merged first (lowest precedence) in mergeComponentConfigurations. retry never got the same treatment, so the only ways to share a retry policy across a stack were per- component, an abstract base component, or the overrides.retry mixin trick. Add a `retry:` stack-manifest-root block, following the exact pattern already established for global metadata: - Read and validate `config[retry]` in ProcessStackConfig (must be a map; unlike metadata, every RetryConfig field is meaningful at global scope, so no field allowlist is needed). - Thread GlobalComponentRetry through ComponentProcessorOptions and wire it into all six built-in component-type constructions (terraform, helmfile, packer, ansible, kubernetes, helm). - Merge it as the new lowest-precedence layer in the retry merge: global -> base component -> concrete component -> overrides. - Also merge it into custom (non-built-in) component types in the builtInTypes passthrough loop, mirroring how global metadata is merged there. - Add ErrInvalidGlobalRetrySection and register `retry` as a root property in the atmos/manifest and stacks/stack-config JSON schemas (reusing the existing #/definitions/retry). Verified end-to-end with a live build: `atmos describe stacks` shows a component with no local retry inheriting the global policy, and a component with a local `max_attempts` override still inheriting global's `conditions` list (deep-merge, not wholesale replacement). Note: `atmos describe component`'s JSON/YAML output has its own fixed key allowlist that has never included `retry` at all -- even a component's own directly-set retry doesn't show there. Pre-existing, unrelated to this change; the real execution path (internal/exec/utils.go's ComponentRetrySection) reads the merged config directly and is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: document stack-root global retry defaults Document the new stack-manifest-root retry: block added in the previous commit: - retry.mdx: new "Stack-Level Defaults" section with the precedence order (stack-root -> base component -> concrete component -> overrides) and an example. Updated the "share a retry policy across a batch" list from three to four options, leading with the stack-root block since it's the simplest for the whole-stack case. - howto/mixins.mdx: added a tip pointing at the stack-root option for readers who only need the retry policy on every component in a stack -- the mixin/overrides.retry trick documented there remains the right tool when the policy should apply to only part of a stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: add changelog post and roadmap entry for stack-level retry Required by the pull-request skill for a minor-labeled feature PR. - website/blog/2026-08-24-stack-level-retry-defaults.mdx: problem-first changelog post per the changelog skill's template (Problem/Fix/How to Use It/Get Involved), tagged enhancement, authored by osterman. - website/src/data/roadmap.js: new shipped milestone under the CI/CD Simplification initiative (same initiative as the original component-level retry milestone), linked to the new changelog slug and the retry docs' new #stack-level-defaults anchor. Progress stays at 95% (20/21 shipped, same rounded value as before). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fix): correct retry Inheritance example, document opt-out, broaden scope Three fixes to the component retry docs found during a field-test pass on the stack-level retry feature: - The "## Inheritance" example used `metadata.component: base/network` to demonstrate config inheritance, but that key only selects which Terraform source a component uses -- it does not inherit config. `metadata.inherits: [base/network]` is the correct mechanism; verified live that the original example produced retry: null (no inheritance at all) while metadata.inherits correctly inherits the base policy. Also corrected the same example's claim that `conditions` "is appended to" the base under default settings -- default list_merge_strategy is replace, so conditions actually replaces unless the user opts into list_merge_strategy: append. - Documented that retry merges as a deep merge like every other section, so `retry: {}` does NOT disable an inherited policy (verified live), and that `retry: !unset` is rejected outright (tracked separately in #2994, a general Atmos gap affecting every typed section, not retry-specific). The supported opt-out is `max_attempts: 1`. - Broadened the intro/scope language ahead of extending retry execution to Helmfile, Packer, and Ansible (previously terraform-only both in implementation and in how the docs read). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat: extend component retry to Helmfile, Packer, and Ansible Component retry (retry.conditions matched against captured subprocess output, with backoff) was wired up for terraform only, even though the underlying mechanism was already fully generic -- a field-test pass found retry.mdx documenting it as applying to "every component in the stack" via the new stack-level defaults, while helmfile/kubernetes components in that same stack silently got no protection at all. Kubernetes and native Helm are out of scope here: they call Go SDKs directly with no subprocess to capture/pattern-match, and would need a different (err.Error()-based) retry mechanism. Tracked as a follow-up, not implemented in this change. - Export executeShellCommandWithRetry -> ExecuteShellCommandWithRetry in internal/exec, so pkg/component/ansible (a different package, already importing internal/exec for ExecuteShellCommand) can call it directly -- no new abstraction, just visibility. - Harden it: a caller-supplied stdout/stderr capture option (e.g. helmfile's NodeHooks.After buffer) previously got silently replaced by retry's own capture buffer when both were configured (last ShellCommandOption wins). Now composed via io.MultiWriter so both receive the full output. Extracted the composition logic into composeRetryCaptureWriters, which also fixed a funlen lint finding. - Wire Helmfile (internal/exec/helmfile.go), Packer (internal/exec/packer.go), and Ansible (pkg/component/ansible/executor.go) to the same wrapper terraform uses. Extracted each call site into its own small execute*CommandWithRetry function (matching the existing executeMainTerraformCommand pattern) so retry wiring is directly unit-testable without standing up each command's full stack-processing preamble or requiring a real binary. - Bundle the shared (allArgsAndFlags, componentPath, envVars) trio into a new retryExecParams struct for the Helmfile/Packer wrappers, resolving an argument-limit lint finding; switched all three wrappers to a pointer atmosConfig param, resolving a gocritic hugeParam finding. - Extended internal/exec's TestMain (and added one to pkg/component/ansible, which had none) so _ATMOS_TEST_EXIT_ONE can combine with _ATMOS_TEST_STDOUT/_ATMOS_TEST_STDERR to simulate a matching/non-matching transient failure -- lets retry-wiring tests use the test binary itself as a fake terraform/helmfile/packer/ ansible-playbook command, no real binaries needed. - Tests prove the wiring end-to-end through each real call chain (not just the shared helper in isolation): matching errors retry to max_attempts, non-matching errors fail fast on the first attempt (asserted via an invocation-count file), and helmfile's NodeHooks capture keeps receiving output when retry is also active. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: add changelog post and roadmap entry for retry's Helmfile/Packer/Ansible extension Separate from the stack-level-retry-defaults post -- this is a distinct capability (which component types retry works for at all, not how it's scoped within a stack) and deserves its own changelog entry per the roadmap skill's one-milestone-per-shipped-capability convention. Progress stays at 95% (21/22 shipped in the ci-cd initiative, same rounded value as before). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit findings on retry PR Honors settings.list_merge_strategy for retry.conditions merges (both the built-in and custom component-type paths), asserts the configured retry count in the Helmfile/Packer/Ansible matching-error tests instead of only checking the final error, fails loudly instead of discarding counter-file I/O errors in the shared test fixtures, adds trailing periods to two comments, and corrects the retry-precedence wording in both changelog posts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: scope stack-root retry claims to supported component types Corrects four remaining "every component" claims (retry.mdx x2, the retry blog post, and the mixins how-to) flagged by CodeRabbit's follow-up review — they contradicted the doc's own "Supported component types" section, which excludes native Kubernetes and native Helm. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: rewrite retry blog posts in plain, consistent language Rewrites both retry changelog posts in short, active-voice sentences (ASD-STE100 style) and bumps their dates to today's publish date (filenames renamed to match). Also fixes two framing problems flagged as AI-cliche/imprecise: - stack-level-retry-defaults: drops the "it's not X, it's Y" contrast and the "hits all at once" framing in favor of the real point -- every component in a stack shares a registry, so they all benefit from the same retry policy. - retry-helmfile-packer-ansible: drops the "stack rarely runs one kind of component" framing in favor of the actual motivation -- CI is inherently flaky, retrying resolves it, and this pattern already works well for Terraform, so it now extends to the other component types for consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: rewrite stack-level-retry-defaults intro in natural prose Replaces the choppy, fragment-heavy intro with full sentences that open on the real pain (CI is flaky, retries fix it, this already works well for Terraform) instead of a single contrived registry-blip example, then lead into this post's actual news: define the retry policy once at the stack level instead of per component. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * [autocommit] formatting fixes * fix(ci): add missing shellescape override to stop flaky NOTICE diffs al.essio.dev/pkg/shellescape is a vanity-import-path module that go-licenses resolves non-deterministically -- some runs return its real GitHub LICENSE URL, others return "Unknown". That flip-flop is exactly what the existing REPO_OVERRIDES deterministic-URL mechanism in generate-notice.sh was built to prevent, but this module was missing from the list, so the "Review Dependency Licenses" CI check failed whenever a run's committed NOTICE didn't match that run's resolution. Adds the override and confirms two consecutive regenerations now produce an identical NOTICE file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: differentiate the two retry blog post intros Both posts' intros had converged on the same "CI is flaky, Atmos already handles this for Terraform, now X" template, making two distinct features read as copy-paste of each other. Gives each post its own real hook instead: - stack-level-retry-defaults: leads with the repetition/DRY problem (sharing one retry policy across a stack's components) -- what this post's feature actually changes. - retry-helmfile-packer-ansible: leads with the coverage-gap problem (Terraform already recovered from transient errors, Helmfile/Packer/ Ansible in the same pipeline didn't) -- what this post's feature actually changes. Also trims each post's "The Problem" section where it had started repeating the new intro's own examples verbatim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate Dependabot alert #278 (postcss-selector-parser DoS) postcss-selector-parser < 6.1.3 and < 7.1.0 (< 7.1.3) allow uncontrolled AST recursion in toString(), a low-severity DoS (GHSA-w9m9-85wc-3x92 / CVE-2026-9358). Pins both major-version lines to their patched releases (6.1.3, 7.1.3) via pnpm.overrides, since the vulnerable package is only pulled in transitively. 39 other open CodeQL/Semgrep alerts on the repo were reviewed but none match this repo's one established safe-fix pattern (go/allocation-size-overflow), so per the security-remediate skill's conservative rule they're left for manual review rather than guessed at. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate Dependabot alerts #279, #281, #282 - google.golang.org/grpc: bump v1.82.1 -> v1.83.2, fixing GHSA-vp52-pcj8-j9qc (heap memory exhaustion via HTTP/2 DATA frame fragmentation, <= 1.83.0). - browserslist: pin transitive dependency to ^4.28.7 via pnpm.overrides, fixing GHSA-c83g-rgw3-j3cx (unbounded memory growth) and GHSA-73wf-gq98-2v4g (uncaught crash via untrusted stats file), both affecting <= 4.28.6. Alert #280 (postcss-selector-parser) was already fixed on this branch by an earlier commit; it stays "open" on GitHub only because it's scoped to the default branch's dependency graph and will auto-close once this branch merges. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: atmos-pro[bot] <173522224+atmos-pro[bot]@users.noreply.github.com>
1 parent 5438c87 commit af511d8

40 files changed

Lines changed: 1309 additions & 193 deletions

NOTICE

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -719,7 +719,7 @@ APACHE 2.0 LICENSED DEPENDENCIES
719719

720720
- google.golang.org/grpc
721721
License: Apache-2.0
722-
URL: https://github.com/grpc/grpc-go/blob/v1.83.1/LICENSE
722+
URL: https://github.com/grpc/grpc-go/blob/v1.83.2/LICENSE
723723

724724
- gopkg.in/ini.v1
725725
License: Apache-2.0

errors/errors.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -524,6 +524,7 @@ var (
524524
ErrInvalidComponentOverridesRequiredVersion = errors.New("invalid component overrides required_version attribute")
525525
ErrInvalidComponentOverridesHooks = errors.New("invalid component overrides hooks section")
526526
ErrInvalidComponentOverridesGenerate = errors.New("invalid component overrides generate section")
527+
ErrInvalidComponentOverridesProvision = errors.New("invalid component overrides provision section")
527528
ErrInvalidComponentOverridesFlags = errors.New("invalid component overrides flags section")
528529
ErrInvalidComponentAttribute = errors.New("invalid component attribute")
529530
ErrInvalidComponentMetadataComponent = errors.New("invalid component metadata.component attribute")
@@ -655,6 +656,7 @@ var (
655656
ErrInvalidAuthSection = errors.New("invalid auth section")
656657
ErrInvalidGlobalMetadataSection = errors.New("invalid metadata section")
657658
ErrGlobalMetadataFieldNotAllowed = errors.New("metadata field is not allowed at global (stack-wide) scope")
659+
ErrInvalidGlobalRetrySection = errors.New("invalid retry section")
658660
ErrInvalidImportSection = errors.New("invalid import section")
659661
ErrInvalidImport = errors.New("invalid import")
660662
ErrInvalidRemoteImport = errors.New("invalid remote import")

go.mod

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

go.sum

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/exec/describe_affected_test.go

Lines changed: 62 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package exec
22

33
import (
44
"encoding/json"
5+
"errors"
6+
"fmt"
57
"net/http"
68
"net/http/httptest"
79
"os"
@@ -291,7 +293,7 @@ func shouldSkipRepoCopyPath(src string) bool {
291293
// housekeeping (e.g. an automatic repack writes tmp_pack_*/tmp_idx_*/tmp_rev_* files and
292294
// renames or removes them within milliseconds) -- the otiai10/copy directory walk stats every
293295
// entry it lists, so it can observe one of these files mid-flight and fail with
294-
// "no such file or directory" (os.IsNotExist).
296+
// "no such file or directory" (os.ErrNotExist, possibly wrapped).
295297
// - fixture files locked by another concurrently running test's terraform process (e.g. a
296298
// terraform.tfstate under tests/fixtures/scenarios/plan-diff held open mid-plan/apply), which
297299
// on Windows surfaces as a sharing/lock violation rather than IsNotExist.
@@ -319,12 +321,17 @@ func copyRepoWithRetry(t *testing.T, src, dest string, opts *cp.Options) error {
319321
}
320322

321323
// isTransientRepoCopyError reports whether err is expected to resolve on its own shortly, and so
322-
// is worth retrying rather than failing the test outright. Windows reports a locked file via its
323-
// error message text rather than a portable sentinel/errno, so this matches on that text the same
324-
// way pkg/git/worktree.go's isTransientWorktreeRemoveError does for transient worktree-removal
325-
// errors.
324+
// is worth retrying rather than failing the test outright. See copyRepoWithRetry's doc comment
325+
// for the two known causes. Windows reports a locked file via its error message text rather than
326+
// a portable sentinel/errno, so this matches on that text the same way pkg/git/worktree.go's
327+
// isTransientWorktreeRemoveError does for transient worktree-removal errors.
326328
func isTransientRepoCopyError(err error) bool {
327-
if os.IsNotExist(err) {
329+
if err == nil {
330+
return false
331+
}
332+
// errors.Is (not os.IsNotExist) so a wrapped os.ErrNotExist is still recognized --
333+
// os.IsNotExist does not reliably unwrap.
334+
if errors.Is(err, os.ErrNotExist) {
328335
return true
329336
}
330337
lower := strings.ToLower(err.Error())
@@ -340,6 +347,55 @@ func isTransientRepoCopyError(err error) bool {
340347
return false
341348
}
342349

350+
func TestIsTransientRepoCopyError(t *testing.T) {
351+
tests := []struct {
352+
name string
353+
err error
354+
want bool
355+
}{
356+
{
357+
name: "nil error is not transient",
358+
err: nil,
359+
want: false,
360+
},
361+
{
362+
name: "not-exist error is transient",
363+
err: os.ErrNotExist,
364+
want: true,
365+
},
366+
{
367+
name: "wrapped not-exist error is transient",
368+
// os.IsNotExist does not reliably unwrap; errors.Is does. This case
369+
// guards against a regression back to os.IsNotExist.
370+
err: fmt.Errorf("copy failed: %w", os.ErrNotExist),
371+
want: true,
372+
},
373+
{
374+
name: "windows lock violation is transient",
375+
// Exact error text observed on Windows CI (ERROR_LOCK_VIOLATION).
376+
err: errors.New(`read ..\..\tests\fixtures\scenarios\hooks-test\components\terraform\hook-and-store\terraform.tfstate.d\test-component2\terraform.tfstate: The process cannot access the file because another process has locked a portion of the file.`),
377+
want: true,
378+
},
379+
{
380+
name: "windows sharing violation is transient",
381+
// ERROR_SHARING_VIOLATION wording, distinct from the lock-violation text above.
382+
err: errors.New(`open foo.txt: The process cannot access the file because it is being used by another process.`),
383+
want: true,
384+
},
385+
{
386+
name: "unrelated error is not transient",
387+
err: errors.New("permission denied"),
388+
want: false,
389+
},
390+
}
391+
392+
for _, tt := range tests {
393+
t.Run(tt.name, func(t *testing.T) {
394+
assert.Equal(t, tt.want, isTransientRepoCopyError(tt.err))
395+
})
396+
}
397+
}
398+
343399
// setupDescribeAffectedTest sets up the test environment for describe affected tests.
344400
func setupDescribeAffectedTest(t *testing.T) (atmosConfig schema.AtmosConfiguration, repoPath, componentPath string) {
345401
t.Helper()

internal/exec/describe_component_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,9 @@ func TestDescribeComponentWithOverridesSection(t *testing.T) {
469469
assert.Contains(t, y, "b: b")
470470
assert.Contains(t, y, "c: c")
471471
assert.Contains(t, y, "d: d")
472+
// `catalog/overrides` (with its `retry` override) is imported after `catalog/c1`,
473+
// so it must not apply — same file-scoping rule as the `vars` override above.
474+
assert.NotContains(t, y, "retry:")
472475

473476
// `test3`
474477
res, err = ExecuteDescribeComponent(&ExecuteDescribeComponentParams{
@@ -487,6 +490,11 @@ func TestDescribeComponentWithOverridesSection(t *testing.T) {
487490
assert.Contains(t, y, "b: b-overridden")
488491
assert.Contains(t, y, "c: c")
489492
assert.Contains(t, y, "d: d")
493+
// `catalog/overrides` (with its `retry` override) is imported before `catalog/c1`,
494+
// so the retry policy from the mixin-style overrides file must apply here — the
495+
// same mechanism documented for sharing a `retry` policy via a mixin.
496+
assert.Contains(t, y, "max_attempts: 5")
497+
assert.Contains(t, y, "- /GOAWAY/")
490498
}
491499

492500
func TestDescribeComponent_Packer(t *testing.T) {

internal/exec/helmfile.go

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -454,16 +454,11 @@ func ExecuteHelmfile(info schema.ConfigAndStacksInfo) error {
454454
// toolchain-installed helmfile (under the install path, not the system PATH)
455455
// is found — mirroring the `version` subcommand above. Falls back to the bare
456456
// command name when no toolchain dependency provides it.
457-
err = ExecuteShellCommand(
458-
atmosConfig,
459-
tenv.Resolve(info.Command),
460-
allArgsAndFlags,
461-
componentPath,
462-
envVars,
463-
info.DryRun,
464-
info.RedirectStdErr,
465-
shellOpts...,
466-
)
457+
err = executeHelmfileCommandWithRetry(&atmosConfig, &info, tenv, retryExecParams{
458+
allArgsAndFlags: allArgsAndFlags,
459+
componentPath: componentPath,
460+
envVars: envVars,
461+
}, shellOpts...)
467462
if info.NodeHooks != nil {
468463
if afterErr := info.NodeHooks.After(context.Background(), &info, stdoutBuf.String()+stderrBuf.String(), err); afterErr != nil && err == nil {
469464
err = afterErr
@@ -482,6 +477,37 @@ func ExecuteHelmfile(info schema.ConfigAndStacksInfo) error {
482477
return nil
483478
}
484479

480+
// executeHelmfileCommandWithRetry runs the resolved helmfile subcommand through
481+
// ExecuteShellCommandWithRetry. Extracted from ExecuteHelmfile so the retry wiring can
482+
// be unit-tested directly with a fake invoke, without standing up ExecuteHelmfile's full
483+
// stack-processing/auth/toolchain preamble or requiring a real helmfile binary.
484+
func executeHelmfileCommandWithRetry(
485+
atmosConfig *schema.AtmosConfiguration,
486+
info *schema.ConfigAndStacksInfo,
487+
tenv *dependencies.ToolchainEnvironment,
488+
params retryExecParams,
489+
shellOpts ...ShellCommandOption,
490+
) error {
491+
return ExecuteShellCommandWithRetry(
492+
atmosConfig,
493+
info,
494+
info.SubCommand,
495+
func(o ...ShellCommandOption) error {
496+
return ExecuteShellCommand(
497+
*atmosConfig,
498+
tenv.Resolve(info.Command),
499+
params.allArgsAndFlags,
500+
params.componentPath,
501+
params.envVars,
502+
info.DryRun,
503+
info.RedirectStdErr,
504+
o...,
505+
)
506+
},
507+
shellOpts...,
508+
)
509+
}
510+
485511
// renderAndDeliver is a seam over helmfile.RenderAndDeliver so the inline
486512
// call-site can be unit-tested without invoking the helmfile binary.
487513
var renderAndDeliver = helmfile.RenderAndDeliver

internal/exec/helmfile_test.go

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package exec
22

33
import (
4+
"bytes"
45
"context"
56
"errors"
67
"os"
@@ -325,3 +326,114 @@ func TestExecuteHelmfileNodeHooks_AfterErrorDroppedWhenExecAlreadyFailed(t *test
325326
assert.NotErrorIs(t, err, afterErr, "the after-hook error must be dropped when the exec error already won")
326327
assert.Error(t, nodeHooks.afterExecErr, "After must have been called with the real (non-nil) exec error")
327328
}
329+
330+
// TestExecuteHelmfileCommandWithRetry_MatchingError_Retries proves the retry wiring added
331+
// to ExecuteHelmfile actually triggers through the real call chain
332+
// (executeHelmfileCommandWithRetry -> ExecuteShellCommandWithRetry -> ExecuteShellCommand),
333+
// not just that the shared helper works in isolation. Uses the test binary itself as the
334+
// "helmfile" command (cross-platform, no real helmfile install needed) via the
335+
// _ATMOS_TEST_EXIT_ONE/_ATMOS_TEST_STDERR TestMain gate.
336+
func TestExecuteHelmfileCommandWithRetry_MatchingError_Retries(t *testing.T) {
337+
exePath, err := os.Executable()
338+
require.NoError(t, err)
339+
340+
counterFile := filepath.Join(t.TempDir(), "counter")
341+
342+
info := &schema.ConfigAndStacksInfo{
343+
Command: exePath,
344+
SubCommand: "sync",
345+
ComponentRetrySection: &schema.RetryConfig{
346+
MaxAttempts: intPtr(3),
347+
Conditions: []string{"/Bad Gateway/"},
348+
},
349+
}
350+
envVars := []string{
351+
"_ATMOS_TEST_COUNTER_FILE=" + counterFile,
352+
"_ATMOS_TEST_EXIT_ONE=1",
353+
"_ATMOS_TEST_STDERR=Error: 502 Bad Gateway returned",
354+
}
355+
356+
atmosConfig := schema.AtmosConfiguration{}
357+
err = executeHelmfileCommandWithRetry(&atmosConfig, info, nil, retryExecParams{
358+
allArgsAndFlags: []string{"sync"},
359+
componentPath: t.TempDir(),
360+
envVars: envVars,
361+
})
362+
require.Error(t, err, "all 3 attempts fail in this fixture, so the final error must propagate")
363+
364+
counterBytes, readErr := os.ReadFile(counterFile)
365+
require.NoError(t, readErr)
366+
assert.Len(t, counterBytes, 3, "all 3 configured attempts must execute before the final error propagates")
367+
}
368+
369+
// TestExecuteHelmfileCommandWithRetry_NonMatchingError_FailsFast proves a real helmfile
370+
// failure whose output does not match `conditions` is NOT retried through the real call
371+
// chain -- the counter file lets us assert exactly one subprocess invocation happened.
372+
func TestExecuteHelmfileCommandWithRetry_NonMatchingError_FailsFast(t *testing.T) {
373+
exePath, err := os.Executable()
374+
require.NoError(t, err)
375+
376+
counterFile := filepath.Join(t.TempDir(), "counter")
377+
378+
info := &schema.ConfigAndStacksInfo{
379+
Command: exePath,
380+
SubCommand: "sync",
381+
ComponentRetrySection: &schema.RetryConfig{
382+
MaxAttempts: intPtr(3),
383+
Conditions: []string{"/Bad Gateway/"},
384+
},
385+
}
386+
envVars := []string{
387+
"_ATMOS_TEST_COUNTER_FILE=" + counterFile,
388+
"_ATMOS_TEST_EXIT_ONE=1",
389+
"_ATMOS_TEST_STDERR=permission denied",
390+
}
391+
392+
atmosConfig := schema.AtmosConfiguration{}
393+
err = executeHelmfileCommandWithRetry(&atmosConfig, info, nil, retryExecParams{
394+
allArgsAndFlags: []string{"sync"},
395+
componentPath: t.TempDir(),
396+
envVars: envVars,
397+
})
398+
require.Error(t, err)
399+
400+
counterBytes, readErr := os.ReadFile(counterFile)
401+
require.NoError(t, readErr)
402+
assert.Len(t, counterBytes, 1, "non-matching error must fail fast on the first attempt")
403+
}
404+
405+
// TestExecuteHelmfileCommandWithRetry_ComposesWithNodeHooksCapture proves that
406+
// executeHelmfileCommandWithRetry's caller-supplied NodeHooks capture buffers (shellOpts)
407+
// still receive output when retry is also configured -- guards the
408+
// ExecuteShellCommandWithRetry MultiWriter composition fix at the actual helmfile call site,
409+
// not just in the shared helper's own unit tests.
410+
func TestExecuteHelmfileCommandWithRetry_ComposesWithNodeHooksCapture(t *testing.T) {
411+
exePath, err := os.Executable()
412+
require.NoError(t, err)
413+
414+
info := &schema.ConfigAndStacksInfo{
415+
Command: exePath,
416+
SubCommand: "sync",
417+
ComponentRetrySection: &schema.RetryConfig{
418+
MaxAttempts: intPtr(2),
419+
Conditions: []string{"/Bad Gateway/"},
420+
},
421+
}
422+
envVars := []string{
423+
"_ATMOS_TEST_EXIT_ONE=1",
424+
"_ATMOS_TEST_STDERR=502 Bad Gateway",
425+
}
426+
427+
var nodeHooksStderr bytes.Buffer
428+
shellOpts := []ShellCommandOption{WithStderrCapture(&nodeHooksStderr)}
429+
430+
atmosConfig := schema.AtmosConfiguration{}
431+
err = executeHelmfileCommandWithRetry(&atmosConfig, info, nil, retryExecParams{
432+
allArgsAndFlags: []string{"sync"},
433+
componentPath: t.TempDir(),
434+
envVars: envVars,
435+
}, shellOpts...)
436+
require.Error(t, err)
437+
assert.Contains(t, nodeHooksStderr.String(), "502 Bad Gateway",
438+
"the NodeHooks-style caller capture buffer must still receive output when retry is also active")
439+
}

internal/exec/packer.go

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -301,14 +301,42 @@ func ExecutePacker(
301301
// "executable file not found in $PATH", because exec.Command resolves the
302302
// binary via the process's real PATH at call time, not via the PATH=...
303303
// entry later added to envVars.
304-
return executePackerShellCommand(
304+
return executePackerCommandWithRetry(&atmosConfig, info, tenv, retryExecParams{
305+
allArgsAndFlags: allArgsAndFlags,
306+
componentPath: componentPath,
307+
envVars: envVars,
308+
})
309+
}
310+
311+
// executePackerCommandWithRetry runs the resolved packer subcommand through
312+
// ExecuteShellCommandWithRetry. Extracted from ExecutePacker so the retry wiring can
313+
// be unit-tested directly with a fake invoke, without standing up ExecutePacker's full
314+
// stack-processing/toolchain preamble or requiring a real packer binary. The inner call
315+
// goes through the executePackerShellCommand seam (not ExecuteShellCommand directly) so
316+
// auth-credential-injection tests that swap that seam still intercept the real subprocess
317+
// invocation, retry or not.
318+
func executePackerCommandWithRetry(
319+
atmosConfig *schema.AtmosConfiguration,
320+
info *schema.ConfigAndStacksInfo,
321+
tenv *dependencies.ToolchainEnvironment,
322+
params retryExecParams,
323+
) error {
324+
return ExecuteShellCommandWithRetry(
305325
atmosConfig,
306-
tenv.Resolve(info.Command),
307-
allArgsAndFlags,
308-
componentPath,
309-
envVars,
310-
info.DryRun,
311-
info.RedirectStdErr,
326+
info,
327+
info.SubCommand,
328+
func(o ...ShellCommandOption) error {
329+
return executePackerShellCommand(
330+
*atmosConfig,
331+
tenv.Resolve(info.Command),
332+
params.allArgsAndFlags,
333+
params.componentPath,
334+
params.envVars,
335+
info.DryRun,
336+
info.RedirectStdErr,
337+
o...,
338+
)
339+
},
312340
WithEnvironment(info.SanitizedEnv),
313341
)
314342
}

0 commit comments

Comments
 (0)