diff --git a/.atmos.d/test.yaml b/.atmos.d/test.yaml index 8886c3dd6e7..4ed04c3f839 100644 --- a/.atmos.d/test.yaml +++ b/.atmos.d/test.yaml @@ -16,7 +16,7 @@ commands: default: false - name: race type: bool - description: Run quick tests with race detector and shuffled order + description: Run the test suite (excluding the tests/ acceptance suite) with race detector and shuffled order default: false - name: generate-mocks type: bool @@ -174,7 +174,7 @@ commands: -D "$GITHUB_WORKSPACE/coverage/shards/shard-{{ .matrix.shard }}" - name: race - description: Run quick tests with race detector and shuffled order + description: Run the test suite (excluding the tests/ acceptance suite) with race detector and shuffled order env: GOTOOLCHAIN: *go_auto_toolchain CGO_ENABLED: "1" @@ -183,7 +183,38 @@ commands: - type: atmos command: build deps - type: shell - command: go test -race -shuffle=on ${TEST:-$(go list ./...)} ${TESTARGS:-} -timeout 10m + # Exclude ./tests/... (the CLI acceptance suite): the `test` job's + # own comment measures its unsharded runtime at ~90m on Linux, + # which is why that job shards it 10-way per OS. It also builds + # and shells out to a plain (non -race) `atmos` binary, so + # instrumenting the driving test process with -race provides no + # race coverage on the binary under test. Running it here blew + # past every timeout budget with no race-detection value to show + # for it. + # + # -timeout 20m (not the default per-package 10m): pkg/toolchain's + # tests install real tool binaries from real registries (no mock + # seam), and this command runs the full ./... suite unsharded, so + # every package's network/registry calls compete for the same + # runner instead of getting a shard's worth of headroom the way + # the sharded `test` job's packages do. -race's CPU/memory + # overhead compounds that contention. 10m wasn't enough headroom + # in CI (pkg/toolchain timed out) even though no single test + # itself hangs -- see docs/fixes for the incident. + # + # set -euo pipefail + the explicit if/assignment (rather than + # embedding the pipe directly in a ${TEST:-...} default + # expansion): a parameter expansion's default value swallows the + # exit status of any command substitution inside it, and mvdan/sh + # (the interpreter for `type: shell` steps) doesn't enable + # pipefail by default -- either one alone would let a failing + # `go list` silently produce an empty/partial package list that + # `grep -v` still exits 0 on, running an incomplete race sweep + # that reports success. + command: >- + set -euo pipefail; + if [ -n "${TEST:-}" ]; then packages="$TEST"; else packages="$(go list ./... | grep -v '^github.com/cloudposse/atmos/tests')"; fi; + go test -race -shuffle=on $packages ${TESTARGS:-} -timeout 20m - name: magefiles description: Run magefiles/ unit tests with coverage (excluded from `go test ./...` by the mage build tag, so CI runs it as a separate step) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 79e1b7c602a..62905babeea 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -612,6 +612,90 @@ jobs: retention-days: 1 compression-level: 0 + # `-race` requires CGO_ENABLED=1 (the detector's runtime is itself a cgo + # library), unlike every other Go build/test path in this repo (CGO_ENABLED=0). + # Enabling cgo pulls in the cgo half of github.com/bearsh/hid (transitive, via + # saml2aws's U2F support) on linux/amd64 -- see hid_libusb_udev_linux.go, which + # links against libudev -- so the compile fails without libudev-dev installed. + # This is the same trap the govulncheck job documents (codeql.yml), but that + # job works around it by forcing CGO_ENABLED=0; that's not an option here since + # it would silently disable the race detector itself. Install libudev-dev + # instead, reusing the floci-go job's apt-get invocation below (same trap, hit + # there because that job also leaves CGO_ENABLED at the runner default). + # + # GOFIPS140=latest (set inside the `race` command itself, .atmos.d/test.yaml) + # is unaffected by CGO_ENABLED: Go's FIPS 140-3 module is pure Go, no cgo. + race: + name: "[race] full test suite" + # `go test`'s package-level concurrency scales with available cores, and + # this job compiles+runs ~400 race-instrumented packages -- the standard + # ubuntu-latest runner's ~4 cores were the bottleneck (this job became + # the slowest check once every other job's own timeout issues were + # fixed). The RunsOn "terraform" family used elsewhere in this file for + # Go work (the build job's linux leg) turned out to be an i4i.large: 2 + # cores, 15.7GB RAM -- fewer cores than ubuntu-latest, a downgrade for + # this CPU/memory-bound workload. "large" (used by the release job's + # goreleaser step) is an r7a.xlarge: 4 cores, 31GB RAM -- double the + # cores and RAM of "terraform", a real upgrade over ubuntu-latest given + # -race's extra memory overhead per test binary. + runs-on: "runs-on=${{github.run_id}}/runner=large/tag=atmos/extras=s3-cache/private=false" + # Race-instrumented binaries run several times slower and use substantially + # more memory than normal test binaries; this runs the entire `./...` suite + # unsharded (unlike the `test` job's acceptance+unit sweep), so budget + # generously. `./cmd/...` alone -- one subtree out of the full package set + # this job actually runs -- took ~45 minutes locally in isolation (see + # docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md); real runs on + # this runner have finished within a few minutes of the previous 45-minute + # budget, leaving too little slack for checkout/setup/apt overhead plus + # natural runner variance. + timeout-minutes: 75 + steps: + # Harden Runner doesn't cover RunsOn self-hosted runners (see the build + # job's linux leg, which skips it the same way); runs-on/action sets up + # this job's runner metadata/labels instead. + - uses: runs-on/action@4e5f72399b6b17f2e79c511c1b38a315a64d22dc # v2.2.0 + + - name: Check out code into the Go module directory + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 + with: + persist-credentials: false + + - name: Set up Go + uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 + with: + go-version-file: "go.mod" + cache: true + + - name: Set up Atmos bootstrap + uses: ./.github/actions/setup-atmos-bootstrap + with: + atmos-version: ${{ env.ATMOS_BOOTSTRAP_VERSION }} + + - name: Install Linux build dependencies (libudev-dev for CGO_ENABLED=1 -- see comment above) + run: | + # The DEB822 /etc/apt/sources.list.d/ubuntu.sources file (and the + # Azure mirror it points at) is a GitHub-hosted-runner-image thing + # (Ubuntu 24.04+); this job now runs on the RunsOn "terraform" + # runner (Ubuntu 22.04, no DEB822 sources file at all -- see + # docs/fixes for the incident where this sed failed outright with + # "No such file or directory"). Only rewrite the mirror if that + # file actually exists, so this step works on either runner image. + if [ -f /etc/apt/sources.list.d/ubuntu.sources ]; then + sudo sed -i 's|http://azure.archive.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list.d/ubuntu.sources + fi + sudo apt-get -o Acquire::Retries=3 -o Acquire::http::Timeout=30 -o Acquire::https::Timeout=30 update + sudo apt-get -o Acquire::Retries=3 -o Acquire::http::Timeout=30 -o Acquire::https::Timeout=30 install -y --no-install-recommends libudev-dev pkg-config + + - name: Run atmos test race + env: + # pkg/toolchain's registry lookups hit the real GitHub API with no + # mock seam; unauthenticated requests share this runner's IP-wide + # 60/hr limit across every package running here, unlike the `test` + # job's acceptance steps, which already set this for the same + # reason (see their own "Use the GitHub token" comment). + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: atmos test race + # Sharding the `test` job fans it out into TEST_SHARD_COUNT x 3 per-leg checks. # Keep the three historical required-check names as compatibility aliases so # branch protection does not need to change. `needs.test.result` aggregates diff --git a/NOTICE b/NOTICE index 48d69b7f85b..bab2419bd6f 100644 --- a/NOTICE +++ b/NOTICE @@ -75,7 +75,7 @@ APACHE 2.0 LICENSED DEPENDENCIES - github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp License: Apache-2.0 - URL: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/detectors/gcp/v1.32.0/detectors/gcp/LICENSE + URL: https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/blob/detectors/gcp/v1.33.0/detectors/gcp/LICENSE - github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric License: Apache-2.0 @@ -575,7 +575,7 @@ APACHE 2.0 LICENSED DEPENDENCIES - github.com/spiffe/go-spiffe/v2 License: Apache-2.0 - URL: https://github.com/spiffe/go-spiffe/blob/v2.6.0/LICENSE + URL: https://github.com/spiffe/go-spiffe/blob/v2.7.0/LICENSE - github.com/tetratelabs/wabin License: Apache-2.0 @@ -651,7 +651,7 @@ APACHE 2.0 LICENSED DEPENDENCIES - go.opentelemetry.io/contrib/detectors/gcp License: Apache-2.0 - URL: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/detectors/gcp/v1.43.0/detectors/gcp/LICENSE + URL: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/detectors/gcp/v1.44.0/detectors/gcp/LICENSE - go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc License: Apache-2.0 @@ -711,15 +711,15 @@ APACHE 2.0 LICENSED DEPENDENCIES - google.golang.org/genproto/googleapis/api License: Apache-2.0 - URL: https://github.com/googleapis/go-genproto/blob/0a33c5d7ca68/googleapis/api/LICENSE + URL: https://github.com/googleapis/go-genproto/blob/3dc84a4a5aaa/googleapis/api/LICENSE - google.golang.org/genproto/googleapis/rpc License: Apache-2.0 - URL: https://github.com/googleapis/go-genproto/blob/0a33c5d7ca68/googleapis/rpc/LICENSE + URL: https://github.com/googleapis/go-genproto/blob/3dc84a4a5aaa/googleapis/rpc/LICENSE - google.golang.org/grpc License: Apache-2.0 - URL: https://github.com/grpc/grpc-go/blob/v1.82.1/LICENSE + URL: https://github.com/grpc/grpc-go/blob/v1.83.1/LICENSE - gopkg.in/ini.v1 License: Apache-2.0 diff --git a/cmd/ai/skill/install_test.go b/cmd/ai/skill/install_test.go index 4bbf9eac27a..34aa2775c8a 100644 --- a/cmd/ai/skill/install_test.go +++ b/cmd/ai/skill/install_test.go @@ -113,16 +113,7 @@ func TestInstallCmd_ArgsValidation(t *testing.T) { // for "atmos ai skill install" with no : it must reach // InstallAllBundled rather than erroring on a missing argument. func TestInstallCmd_RunE_NoArgsInstallsEveryBundledSkill(t *testing.T) { - resetFlags := func() { - forceFlag := installCmd.Flags().Lookup("force") - if forceFlag != nil { - _ = forceFlag.Value.Set("false") - } - yesFlag := installCmd.Flags().Lookup("yes") - if yesFlag != nil { - _ = yesFlag.Value.Set("false") - } - } + resetFlags := func() { resetInstallCmdFlagsForTest(t) } resetFlags() t.Cleanup(resetFlags) @@ -275,12 +266,7 @@ func TestInstallCmd_RunE_PathWithoutDistributionFlagsDoesNotWarn(t *testing.T) { // new, 0 updated) must not claim a location or print the chat hint -- there // is nothing to report either did. func TestInstallCmd_RunE_AlreadyInstalledOmitsLocationWhenNothingInstalled(t *testing.T) { - resetFlags := func() { - yesFlag := installCmd.Flags().Lookup("yes") - if yesFlag != nil { - _ = yesFlag.Value.Set("false") - } - } + resetFlags := func() { resetInstallCmdFlagsForTest(t) } resetFlags() t.Cleanup(resetFlags) diff --git a/cmd/ai/skill/ui_test.go b/cmd/ai/skill/ui_test.go index dc019080504..12012127b18 100644 --- a/cmd/ai/skill/ui_test.go +++ b/cmd/ai/skill/ui_test.go @@ -66,3 +66,23 @@ func resetFlagChangedForTest(t *testing.T, cmd *cobra.Command, name string) { require.NoError(t, flag.Value.Set(flag.DefValue)) flag.Changed = false } + +// resetInstallCmdFlagsForTest resets every flag registered on the +// package-level installCmd singleton (see install.go's init) back to +// default/unchanged. A flag left Changed=true by an earlier test -- whether +// via Flags().Set, which marks Changed, or even via a "reset" helper that +// itself calls Flags().Set to restore a default and so also marks +// Changed=true -- silently affects a later test's own run under +// -shuffle=on, e.g. --client/--scope/--path leaking to change which skills +// are considered already installed or where distribution happens. +func resetInstallCmdFlagsForTest(t *testing.T) { + t.Helper() + + resetFlagChangedForTest(t, installCmd, "force") + resetFlagChangedForTest(t, installCmd, "yes") + resetFlagChangedForTest(t, installCmd, "path") + resetFlagChangedForTest(t, installCmd, "all-clients") + resetFlagChangedForTest(t, installCmd, scopeFlag) + resetFlagChangedForTest(t, installCmd, "global") + resetStringSliceFlagForTest(t, installCmd) +} diff --git a/cmd/ai/skill/uninstall_test.go b/cmd/ai/skill/uninstall_test.go index c20457b6b43..bcd85fc43e6 100644 --- a/cmd/ai/skill/uninstall_test.go +++ b/cmd/ai/skill/uninstall_test.go @@ -996,11 +996,28 @@ Second test skill. err = os.WriteFile(registryPath, registryData, 0o600) require.NoError(t, err) - // Set force flag. + // Set force flag. Use Flags().Set (not Lookup().Value.Set): only the + // former marks the pflag as Changed, which viper's binding checks to + // decide precedence -- uninstall.go reads v.GetBool("force"), not the + // flag directly, so a Value.Set that leaves Changed=false can resolve to + // a stale/default value instead of "true" depending on what ran before + // this test under -shuffle=on. Every other force-flag test in this file + // already uses Flags().Set for this reason. + // + // uninstallCmd is a package-level singleton, so cleanup must restore both + // the original value and Changed state, not just set the value back to + // "false": Flags().Set always marks Changed=true regardless of the value + // passed, so a cleanup that only calls Flags().Set("force", "false") + // would leave Changed=true, making later shuffled tests treat "false" as + // an explicit CLI value instead of falling through to env/config. forceFlag := uninstallCmd.Flags().Lookup("force") - if forceFlag != nil { - _ = forceFlag.Value.Set("true") - } + originalForceValue := forceFlag.Value.String() + originalForceChanged := forceFlag.Changed + require.NoError(t, uninstallCmd.Flags().Set("force", "true")) + t.Cleanup(func() { + _ = uninstallCmd.Flags().Set("force", originalForceValue) + forceFlag.Changed = originalForceChanged + }) // Capture stdout. oldStdout := os.Stdout diff --git a/cmd/ci/validate_test.go b/cmd/ci/validate_test.go index c426771a3b2..86e6f75f9dd 100644 --- a/cmd/ci/validate_test.go +++ b/cmd/ci/validate_test.go @@ -94,7 +94,15 @@ func TestWorkflowValidationErrorOwnsDiagnostics(t *testing.T) { assert.ErrorIs(t, validationErr, errWorkflowValidationFailed) assert.Equal(t, 1, errUtils.GetExitCode(validationErr)) - rendered := errUtils.Format(validationErr, errUtils.DefaultFormatterConfig()) + // MaxLineLength: 0 (DefaultFormatterConfig's zero value) auto-detects from the + // terminal, which varies across CI runners/local dev and can wrap "GitHub + // Actions workflow validation failed" onto two lines right where the substring + // check below expects it on one -- pin a width wide enough that this short + // message never wraps, so the assertion is deterministic regardless of the + // environment's detected terminal width. + cfg := errUtils.DefaultFormatterConfig() + cfg.MaxLineLength = 200 + rendered := errUtils.Format(validationErr, cfg) assert.Contains(t, rendered, "GitHub Actions workflow validation failed") assert.Contains(t, rendered, "actionlint-style diagnostic") } diff --git a/cmd/cmd_utils_test.go b/cmd/cmd_utils_test.go index 8b22523f7e1..d01118e7866 100644 --- a/cmd/cmd_utils_test.go +++ b/cmd/cmd_utils_test.go @@ -397,6 +397,11 @@ func TestEnableHeatmapIfRequested(t *testing.T) { perf.EnableTracking(false) }) + // A prior test that also enables tracking (see cmd/root_heatmap_test.go) + // without resetting the registry would otherwise leave this test's own + // captureHeatmap() assertions sharing state with whatever ran before it. + perf.ResetForTesting() + captureHeatmap := func() string { oldStderr := os.Stderr r, w, err := os.Pipe() diff --git a/cmd/describe_dependents_test.go b/cmd/describe_dependents_test.go index 82cf5795570..f36f5b778c3 100644 --- a/cmd/describe_dependents_test.go +++ b/cmd/describe_dependents_test.go @@ -133,7 +133,13 @@ func TestDescribeDependentsRunnable_InvalidErrorMode(t *testing.T) { t.Setenv("ATMOS_IDENTITY", "") t.Setenv("IDENTITY", "") - errorModeFlag := describeDependentsCmd.Flags().Lookup("error-mode") + // PersistentFlags(), not Flags(): a persistent flag only appears in Flags() + // after cobra's mergePersistentFlags runs, which happens the first time this + // command is actually Execute()'d/ParseFlags()'d -- something that depends on + // which other test happens to run first under -shuffle=on. PersistentFlags() + // is this flag's own FlagSet, populated directly at init() time, so it's + // reliable regardless of execution order. + errorModeFlag := describeDependentsCmd.PersistentFlags().Lookup("error-mode") require.NotNil(t, errorModeFlag, "error-mode flag must be registered on describeDependentsCmd") origValue := errorModeFlag.Value.String() origChanged := errorModeFlag.Changed diff --git a/cmd/describe_stacks_test.go b/cmd/describe_stacks_test.go index 95c20a89e72..aa29615ca4c 100644 --- a/cmd/describe_stacks_test.go +++ b/cmd/describe_stacks_test.go @@ -8,6 +8,7 @@ import ( "github.com/spf13/pflag" "github.com/spf13/viper" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "github.com/cloudposse/atmos/internal/exec" @@ -104,8 +105,14 @@ func TestDescribeStacksRunnable_InvalidErrorMode(t *testing.T) { t.Setenv("ATMOS_IDENTITY", "") t.Setenv("IDENTITY", "") - errorModeFlag := describeStacksCmd.Flags().Lookup("error-mode") - assert.NotNil(t, errorModeFlag, "error-mode flag must be registered on describeStacksCmd") + // PersistentFlags(), not Flags(): a persistent flag only appears in Flags() + // after cobra's mergePersistentFlags runs, which happens the first time this + // command is actually Execute()'d/ParseFlags()'d -- something that depends on + // which other test happens to run first under -shuffle=on. PersistentFlags() + // is this flag's own FlagSet, populated directly at init() time, so it's + // reliable regardless of execution order. + errorModeFlag := describeStacksCmd.PersistentFlags().Lookup("error-mode") + require.NotNil(t, errorModeFlag, "error-mode flag must be registered on describeStacksCmd") origValue := errorModeFlag.Value.String() origChanged := errorModeFlag.Changed t.Cleanup(func() { diff --git a/cmd/describe_workflows_test.go b/cmd/describe_workflows_test.go index 3c3b65fac9c..80fa329f7b6 100644 --- a/cmd/describe_workflows_test.go +++ b/cmd/describe_workflows_test.go @@ -100,7 +100,14 @@ func TestDescribeWorkflows(t *testing.T) { describeWorkflowsMock, ) - describeWorkflowsCmd.Flags().StringP("pager", "p", "", "Specify a pager to use for output (e.g., `less`, `more`)") + // --pager is also a RootCmd persistent flag; under -shuffle=on, a prior test that + // exercised the full Execute() pipeline may have already merged it into this + // command's local FlagSet (cobra's mergePersistentFlags, itself Lookup-guarded). + // Unlike AddFlagSet, StringP's underlying AddFlag panics on a duplicate name, so + // guard it here to keep this test order-independent. + if describeWorkflowsCmd.Flags().Lookup("pager") == nil { + describeWorkflowsCmd.Flags().StringP("pager", "p", "", "Specify a pager to use for output (e.g., `less`, `more`)") + } err := run(describeWorkflowsCmd, []string{}) diff --git a/cmd/init/init_test.go b/cmd/init/init_test.go index 036b0d29e99..ab95f88bd34 100644 --- a/cmd/init/init_test.go +++ b/cmd/init/init_test.go @@ -413,6 +413,17 @@ func TestExecuteInit_TemplateValuesConversion(t *testing.T) { } func TestInitCmd_Integration_Help(t *testing.T) { + // cobra checks the "help" flag's current value on every Execute() call, not + // just whether --help was in this invocation's args -- so leaving it "true" + // leaks into every later test that calls initCmd.Execute() for the rest of + // this package's test binary: Execute() returns nil having printed help + // instead of ever calling RunE, regardless of that later test's own args. + // -shuffle=on can put this test before any of those, so it must restore the + // flag itself; see docs/fixes for the incident. + t.Cleanup(func() { + _ = initCmd.Flags().Set("help", "false") + }) + // Test help output. initCmd.SetArgs([]string{"--help"}) err := initCmd.Execute() diff --git a/cmd/list/affected_test.go b/cmd/list/affected_test.go index 90850dc8d08..f35b04209e7 100644 --- a/cmd/list/affected_test.go +++ b/cmd/list/affected_test.go @@ -145,6 +145,19 @@ func TestAffectedIdentityFlagParsing(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // viper is the global singleton; setupViper's own viper.Reset() + // leaves the "identity" key set for the rest of the test binary + // (e.g. "viper-identity") unless restored here. A later test + // building a fresh cmd with no --identity flag would otherwise + // pick up this leaked value via the same viper.GetString("identity") + // fallback, which is exactly what happened to cmd/list's + // TestListStacksWithOptions_CoverageIntegration and its siblings + // under -shuffle=on: the leaked identity name is non-empty, so + // resolveIdentityName returns it unchecked, and the downstream + // isAuthConfigured check then fails against the (deliberately + // identity-less) `complete` fixture. See docs/fixes. + t.Cleanup(viper.Reset) + cmd := tt.setupCmd() tt.setupViper() v := viper.GetViper() diff --git a/cmd/list/instances_test.go b/cmd/list/instances_test.go index a083f6bb81f..11c01ab317f 100644 --- a/cmd/list/instances_test.go +++ b/cmd/list/instances_test.go @@ -248,6 +248,12 @@ func TestInstancesIdentityFlagLogic(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + // viper is the global singleton; setupViper's viper.Reset() + + // viper.Set("identity", ...) leaks the "identity" key to every + // later test in this binary unless restored here. See + // TestAffectedIdentityFlagParsing's identical cleanup for why. + t.Cleanup(viper.Reset) + tc.setupViper() cmd := tc.setupCmd() diff --git a/cmd/list/utils_test.go b/cmd/list/utils_test.go index a5b9d2e087f..c87e4202c8f 100644 --- a/cmd/list/utils_test.go +++ b/cmd/list/utils_test.go @@ -252,6 +252,13 @@ func TestGetIdentityFromCommand(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + // viper is the global singleton; setupViper's viper.Reset() + + // viper.Set("identity", ...) leaks the "identity" key to every + // later test in this binary unless restored here. See + // TestAffectedIdentityFlagParsing (affected_test.go) for the + // full explanation of the downstream failure this caused. + t.Cleanup(viper.Reset) + tc.setupViper() cmd := tc.setupCmd() result := getIdentityFromCommand(cmd) @@ -263,6 +270,7 @@ func TestGetIdentityFromCommand(t *testing.T) { func TestGetIdentityFromCommand_NormalizesIdentityEnvFalse(t *testing.T) { t.Setenv("ATMOS_IDENTITY", "false") viper.Reset() + t.Cleanup(viper.Reset) viper.SetEnvPrefix("ATMOS") assert.NoError(t, viper.BindEnv(cfg.IdentityFlagName)) diff --git a/cmd/root_heatmap_test.go b/cmd/root_heatmap_test.go index 4e25d254279..dadf6ccc6d5 100644 --- a/cmd/root_heatmap_test.go +++ b/cmd/root_heatmap_test.go @@ -46,8 +46,17 @@ func TestDisplayPerformanceHeatmap(t *testing.T) { t.Run(tt.name, func(t *testing.T) { _ = NewTestKit(t) - // Reset perf registry and enable tracking (P95 is automatically enabled). + // Enable tracking (P95 is automatically enabled). perf.EnableTracking is a + // process-wide flag with no per-test scoping -- left on, every perf.Track() + // call anywhere in the rest of this package's test binary stops being a + // no-op, silently accumulating real metrics into the global registry (and, + // under -shuffle=on, exposing whatever test runs next to real concurrent + // perf.Track traffic) -- see docs/fixes for the incident. ResetForTesting + // clears the registry itself, so this test's own tracked calls aren't + // crowded out of the top-N display by whatever ran before it either. + perf.ResetForTesting() perf.EnableTracking(true) + t.Cleanup(func() { perf.EnableTracking(false) }) // Add some test tracking data. done := perf.Track(nil, "testFunction") @@ -136,8 +145,11 @@ func TestHeatmapFlags(t *testing.T) { func TestHeatmapNonTTYOutput(t *testing.T) { _ = NewTestKit(t) - // Reset perf registry and enable tracking. + // Enable tracking -- see TestDisplayPerformanceHeatmap's comment above for why + // this must be disabled again afterward, and the registry reset too. + perf.ResetForTesting() perf.EnableTracking(true) + t.Cleanup(func() { perf.EnableTracking(false) }) // Add test data. done := perf.Track(nil, "nonTTYTest") diff --git a/cmd/root_help_routing_test.go b/cmd/root_help_routing_test.go index fb5ed0fc684..9b685c0a749 100644 --- a/cmd/root_help_routing_test.go +++ b/cmd/root_help_routing_test.go @@ -235,6 +235,15 @@ func TestRootHelpFunc_RealTree_UnknownSubcommandErrors(t *testing.T) { parentCmd := findChildCommand(RootCmd, tt.parentName) require.NotNilf(t, parentCmd, "RootCmd must have a %q subcommand registered", tt.parentName) + // parentCmd is a package-level singleton NewTestKit does not reach (it + // only snapshots/restores RootCmd's own flags, not nested subcommands'). + // Cobra parses --help onto parentCmd's own FlagSet before this test's + // unknown-subcommand check ever runs, leaving Changed=true there + // afterward; a later test's dispatch on the same command tree (e.g. + // TestRootHelpFunc_RealTree_ValidCasesStillRenderHelp reusing the same + // "version"/"toolchain"/"terraform" commands) would otherwise see that + // leaked state. Reset it here too, symmetric with that test's own reset. + t.Cleanup(func() { _ = parentCmd.Flags().Set("help", "false") }) oldStderr := os.Stderr r, w, pipeErr := os.Pipe() @@ -389,11 +398,21 @@ func TestRootHelpFunc_RealTree_ValidCasesStillRenderHelp(t *testing.T) { target := findChildCommand(RootCmd, tt.parentName) require.NotNilf(t, target, "RootCmd must have a %q subcommand registered", tt.parentName) + // target (and child, below) are package-level singletons NewTestKit does + // not reach: it only snapshots/restores RootCmd's own flags, not nested + // subcommands'. This real --help invocation leaves the flag Changed=true + // on target (and child), which a later test's cobra dispatch on the same + // command tree honors regardless of that later invocation's own args -- + // see TestRootHelpFunc_RealTree_UnknownSubcommandErrors's "toolchain + // versions --help" case, which this leak makes silently succeed instead + // of reporting "unknown command". Reset both after this subtest. + t.Cleanup(func() { _ = target.Flags().Set("help", "false") }) args := []string{tt.parentName} if tt.childName != "" { child := findChildCommand(target, tt.childName) require.NotNilf(t, child, "%q must have a %q subcommand registered", tt.parentName, tt.childName) + t.Cleanup(func() { _ = child.Flags().Set("help", "false") }) args = append(args, tt.childName) } args = append(args, "--help") diff --git a/cmd/scaffold/scaffold_test.go b/cmd/scaffold/scaffold_test.go index efe8c42c7e4..1d7d1583375 100644 --- a/cmd/scaffold/scaffold_test.go +++ b/cmd/scaffold/scaffold_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/spf13/cobra" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -542,7 +543,22 @@ func TestScaffoldGenerateParser_Creation(t *testing.T) { assert.IsType(t, &flags.StandardParser{}, scaffoldGenerateParser) } +// resetHelpFlag restores cmd's "help" flag to unset after a test sets it via +// --help. Cobra checks the flag's current value on every Execute() call, not +// just whether --help was in that invocation's own args, so leaving it "true" +// leaks into any later test that calls the same *cobra.Command's Execute(): +// it returns nil having printed help instead of ever calling RunE, regardless +// of that later test's own args. -shuffle=on can put a --help test before any +// of those; see docs/fixes for the incident. +func resetHelpFlag(t *testing.T, cmd *cobra.Command) { + t.Helper() + t.Cleanup(func() { + _ = cmd.Flags().Set("help", "false") + }) +} + func TestScaffoldCmd_Integration_Help(t *testing.T) { + resetHelpFlag(t, scaffoldCmd) // Test help output for main command scaffoldCmd.SetArgs([]string{"--help"}) err := scaffoldCmd.Execute() @@ -550,6 +566,7 @@ func TestScaffoldCmd_Integration_Help(t *testing.T) { } func TestScaffoldGenerateCmd_Integration_Help(t *testing.T) { + resetHelpFlag(t, scaffoldGenerateCmd) // Test help output for generate subcommand scaffoldGenerateCmd.SetArgs([]string{"--help"}) err := scaffoldGenerateCmd.Execute() @@ -557,6 +574,7 @@ func TestScaffoldGenerateCmd_Integration_Help(t *testing.T) { } func TestScaffoldListCmd_Integration_Help(t *testing.T) { + resetHelpFlag(t, scaffoldListCmd) // Test help output for list subcommand scaffoldListCmd.SetArgs([]string{"--help"}) err := scaffoldListCmd.Execute() @@ -564,6 +582,7 @@ func TestScaffoldListCmd_Integration_Help(t *testing.T) { } func TestScaffoldValidateCmd_Integration_Help(t *testing.T) { + resetHelpFlag(t, scaffoldValidateCmd) // Test help output for validate subcommand scaffoldValidateCmd.SetArgs([]string{"--help"}) err := scaffoldValidateCmd.Execute() diff --git a/cmd/terraform/cache/mirror_test.go b/cmd/terraform/cache/mirror_test.go index 3e69f6795bc..5bbe1fd4500 100644 --- a/cmd/terraform/cache/mirror_test.go +++ b/cmd/terraform/cache/mirror_test.go @@ -47,6 +47,21 @@ func TestMirrorCmdRunAll(t *testing.T) { orig := mirrorRun t.Cleanup(func() { mirrorRun = orig }) + // mirrorCmd is a package-level singleton, and Options.All is read via + // v.GetBool("all") (viper's flag binding), which honors the flag's + // Changed state rather than its value alone. Passing --all here sets + // Changed=true; without restoring it, a later test that never passes + // --all (e.g. TestMirrorCmdRunSingle) would still observe All=true + // under -shuffle=on, since a flag omitted from a later Execute() call + // keeps whatever value/Changed state the previous parse left it in. + allFlag := mirrorCmd.Flags().Lookup("all") + origAllValue := allFlag.Value.String() + origAllChanged := allFlag.Changed + t.Cleanup(func() { + _ = mirrorCmd.Flags().Set("all", origAllValue) + allFlag.Changed = origAllChanged + }) + var got tfmirror.Options mirrorRun = func(o tfmirror.Options) error { got = o diff --git a/cmd/testing_helpers_snapshot_test.go b/cmd/testing_helpers_snapshot_test.go index 5c4408962a8..eca802e84fb 100644 --- a/cmd/testing_helpers_snapshot_test.go +++ b/cmd/testing_helpers_snapshot_test.go @@ -32,12 +32,12 @@ func TestSnapshotRootCmdState(t *testing.T) { require.NoError(t, RootCmd.PersistentFlags().Set("logs-level", "Debug")) }, validateBefore: func(t *testing.T, snapshot *cmdStateSnapshot) { - chdirSnap, exists := snapshot.flags["chdir"] + chdirSnap, exists := snapshot.flags[RootCmd]["chdir"] require.True(t, exists, "Should capture chdir flag") assert.Equal(t, "/tmp/test", chdirSnap.value) assert.True(t, chdirSnap.changed, "Should mark flag as changed") - logsLevelSnap, exists := snapshot.flags["logs-level"] + logsLevelSnap, exists := snapshot.flags[RootCmd]["logs-level"] require.True(t, exists, "Should capture logs-level flag") assert.Equal(t, "Debug", logsLevelSnap.value) }, @@ -51,7 +51,7 @@ func TestSnapshotRootCmdState(t *testing.T) { require.NoError(t, RootCmd.PersistentFlags().Set("chdir", "")) }, validateBefore: func(t *testing.T, snapshot *cmdStateSnapshot) { - chdirSnap, exists := snapshot.flags["chdir"] + chdirSnap, exists := snapshot.flags[RootCmd]["chdir"] require.True(t, exists) assert.True(t, chdirSnap.changed, "Should preserve Changed state even if value is default") }, @@ -65,7 +65,7 @@ func TestSnapshotRootCmdState(t *testing.T) { }, validateBefore: func(t *testing.T, snapshot *cmdStateSnapshot) { // Verify we captured persistent flags. - basePathSnap, exists := snapshot.flags["base-path"] + basePathSnap, exists := snapshot.flags[RootCmd]["base-path"] require.True(t, exists, "Should capture persistent flags") assert.Equal(t, "/custom/base", basePathSnap.value) }, @@ -218,14 +218,14 @@ func TestSnapshotImmutability(t *testing.T) { snapshot := snapshotRootCmdState() // Verify snapshot captured initial state. - chdirSnap := snapshot.flags["chdir"] + chdirSnap := snapshot.flags[RootCmd]["chdir"] assert.Equal(t, "/initial", chdirSnap.value) // Modify RootCmd state. require.NoError(t, RootCmd.PersistentFlags().Set("chdir", "/modified")) // Verify snapshot is unchanged. - chdirSnap = snapshot.flags["chdir"] + chdirSnap = snapshot.flags[RootCmd]["chdir"] assert.Equal(t, "/initial", chdirSnap.value, "Snapshot should preserve initial flag value") // Verify RootCmd has the modified state. diff --git a/cmd/testing_helpers_test.go b/cmd/testing_helpers_test.go index 7cf9e3b69c4..d6950b7d834 100644 --- a/cmd/testing_helpers_test.go +++ b/cmd/testing_helpers_test.go @@ -42,21 +42,38 @@ type flagSnapshot struct { type cmdStateSnapshot struct { args []string osArgs []string - flags map[string]flagSnapshot + flags map[*cobra.Command]map[string]flagSnapshot chdirProcessed bool colorProfile termenv.Profile // Lipgloss color profile openDocsURL func(string) error commands []*cobra.Command // RootCmd.Commands() at snapshot time. } -// snapshotRootCmdState captures the current state of RootCmd including all flag values and I/O streams. -// This allows tests to save state at the beginning and restore it in cleanup via NewTestKit, -// preventing test pollution without needing to maintain a hardcoded list of flags. +// walkCommandTree calls fn for RootCmd and every command reachable from it +// (recursively, through every level of subcommands). Used to snapshot/restore +// flag state across the whole command tree, not just RootCmd's own flags: a +// real invocation of e.g. "atmos toolchain --help" through RootCmd.ExecuteC() +// parses --help onto toolchain's own FlagSet, and that FlagSet is a +// package-level singleton no different from RootCmd's -- left un-reset, it +// leaks into whichever later test's dispatch reaches the same subcommand. See +// docs/fixes for the incident this closes. +func walkCommandTree(root *cobra.Command, fn func(*cobra.Command)) { + fn(root) + for _, c := range root.Commands() { + walkCommandTree(c, fn) + } +} + +// snapshotRootCmdState captures the current state of RootCmd (and every +// subcommand reachable from it) including all flag values and I/O streams. +// This allows tests to save state at the beginning and restore it in cleanup +// via NewTestKit, preventing test pollution without needing to maintain a +// hardcoded list of flags. func snapshotRootCmdState() *cmdStateSnapshot { snapshot := &cmdStateSnapshot{ args: make([]string, len(RootCmd.Flags().Args())), osArgs: make([]string, len(os.Args)), - flags: make(map[string]flagSnapshot), + flags: make(map[*cobra.Command]map[string]flagSnapshot), chdirProcessed: chdirProcessed, colorProfile: lipgloss.ColorProfile(), openDocsURL: openDocsURL, @@ -69,18 +86,21 @@ func snapshotRootCmdState() *cmdStateSnapshot { // Copy os.Args. copy(snapshot.osArgs, os.Args) - // Snapshot all flags (both local and persistent). - snapshotFlags := func(flagSet *pflag.FlagSet) { - flagSet.VisitAll(func(f *pflag.Flag) { - snapshot.flags[f.Name] = flagSnapshot{ - value: f.Value.String(), - changed: f.Changed, - } - }) - } - - snapshotFlags(RootCmd.Flags()) - snapshotFlags(RootCmd.PersistentFlags()) + // Snapshot every command's own flags (both local and persistent). + walkCommandTree(RootCmd, func(c *cobra.Command) { + flags := make(map[string]flagSnapshot) + snapshotFlags := func(flagSet *pflag.FlagSet) { + flagSet.VisitAll(func(f *pflag.Flag) { + flags[f.Name] = flagSnapshot{ + value: f.Value.String(), + changed: f.Changed, + } + }) + } + snapshotFlags(c.Flags()) + snapshotFlags(c.PersistentFlags()) + snapshot.flags[c] = flags + }) return snapshot } @@ -147,10 +167,26 @@ func restoreRootCmdState(snapshot *cmdStateSnapshot) { // Restore chdirProcessed flag. chdirProcessed = snapshot.chdirProcessed - // Restore all flags to their snapshotted values. - restoreFlags := func(flagSet *pflag.FlagSet) { - flagSet.VisitAll(func(f *pflag.Flag) { - if snap, ok := snapshot.flags[f.Name]; ok { + // Remove any command registered on RootCmd since the snapshot was taken + // (e.g. by a test loading real custom commands via InitCliConfig + + // processCustomCommands). Left in place, a later test can collide with + // or silently observe a command from an unrelated, already-finished test. + // Done before the flag walk below so that walk visits exactly the + // commands present in the snapshot. + restoreRootCmdCommands(snapshot.commands) + + // Restore every snapshotted command's flags to their captured values. + restoreFlagsOn := func(c *cobra.Command) { + flags, ok := snapshot.flags[c] + if !ok { + return + } + restoreFlags := func(flagSet *pflag.FlagSet) { + flagSet.VisitAll(func(f *pflag.Flag) { + snap, ok := flags[f.Name] + if !ok { + return + } // StringSlice/StringArray flags need special handling due to append behavior. if f.Value.Type() == "stringSlice" || f.Value.Type() == "stringArray" { restoreStringSliceFlag(f, snap) @@ -159,12 +195,12 @@ func restoreRootCmdState(snapshot *cmdStateSnapshot) { // For other flag types, direct Set() works fine. _ = f.Value.Set(snap.value) f.Changed = snap.changed - } - }) + }) + } + restoreFlags(c.Flags()) + restoreFlags(c.PersistentFlags()) } - - restoreFlags(RootCmd.Flags()) - restoreFlags(RootCmd.PersistentFlags()) + walkCommandTree(RootCmd, restoreFlagsOn) // Restore lipgloss color profile and regenerate theme styles. // This prevents test pollution from color settings. @@ -173,12 +209,6 @@ func restoreRootCmdState(snapshot *cmdStateSnapshot) { // Restore package-level test seams. openDocsURL = snapshot.openDocsURL - - // Remove any command registered on RootCmd since the snapshot was taken - // (e.g. by a test loading real custom commands via InitCliConfig + - // processCustomCommands). Left in place, a later test can collide with - // or silently observe a command from an unrelated, already-finished test. - restoreRootCmdCommands(snapshot.commands) } // restoreRootCmdCommands removes every command currently on RootCmd that diff --git a/cmd/validate_editorconfig_test.go b/cmd/validate_editorconfig_test.go index 33a40e10892..d0fb4b62184 100644 --- a/cmd/validate_editorconfig_test.go +++ b/cmd/validate_editorconfig_test.go @@ -13,6 +13,7 @@ import ( er "github.com/editorconfig-checker/editorconfig-checker/v3/pkg/error" "github.com/editorconfig-checker/editorconfig-checker/v3/pkg/outputformat" "github.com/spf13/cobra" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -576,5 +577,11 @@ func TestEditorConfigCmdCIFlagRegisteredThroughStandardParser(t *testing.T) { assert.Equal(t, "false", flag.DefValue) t.Setenv("ATMOS_CI", "true") + // Re-bind defensively: init() runs this exactly once, but other tests in + // this package call viper.Reset(), which discards it -- leaving this + // assertion dependent on no such test having run first under -shuffle=on. + // BindFlagsToViper is safe to call again (idempotent); see docs/fixes for + // the incident. + require.NoError(t, ciFlagsParser.BindFlagsToViper(editorConfigCmd, viper.GetViper())) assert.True(t, ci.ModeEnabled(&cobra.Command{}), "expected ATMOS_CI env var to resolve through Viper via the standard parser binding") } diff --git a/cmd/version/list_test.go b/cmd/version/list_test.go index c61c3dc1f91..f44a096d4a9 100644 --- a/cmd/version/list_test.go +++ b/cmd/version/list_test.go @@ -365,6 +365,15 @@ func TestListCommand_FormatValidation(t *testing.T) { listOffset = 0 listSince = "" listFormat = tt.format + // listFormat is a package-level var (RunE reads it directly, not + // through the flag), so the "invalid format" case's assignment + // above leaks into whatever test runs next under -shuffle=on -- + // e.g. TestListCommand_ValidationErrors's "invalid since date + // format" subtest failed on this exact stale value instead of + // reaching its own check. Restore the flag's real default ("table", + // see listCmd.Flags().StringVar in list.go) so this test's own + // mutation doesn't outlive it; see docs/fixes for the incident. + t.Cleanup(func() { listFormat = "table" }) cmd := listCmd err := cmd.RunE(cmd, []string{}) diff --git a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md new file mode 100644 index 00000000000..b2d104cf077 --- /dev/null +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -0,0 +1,666 @@ +# Fix: `[race] full test suite` CI job — timeouts, shuffle-order test bugs, and real data races + +**Date:** 2026-09-01 + +## Summary + +The new `[race] full test suite` CI job (added to run `atmos test race` on pull requests) +failed on its first seven real runs. Rounds 1–2: the package list included the CLI acceptance +suite (deliberately sharded elsewhere because it takes ~90 minutes unsharded), +`pkg/toolchain`'s real-network registry tests didn't fit the per-package timeout once running +unsharded and unauthenticated, and `-shuffle=on` exposed a pre-existing test-isolation bug in +`pkg/utils`. Round 3: a genuine production data race in `pkg/toolchain`'s concurrent batch +installer (exactly what this job exists to catch), plus a second `-shuffle=on`-exposed +test-isolation bug, this time in `pkg/toolchain` itself. Round 4, once the job stopped timing +out and started running the full suite to completion: seven more independent failures surfaced at +once, spread across unrelated packages — two real data races (an LSP document-manager race and a +package-var-capture race in a trust-store installer), four more shuffle-order test-isolation bugs +(a reset-without-reinitialize in a test I/O helper, a backend-registry wipe-without-restore, a +viper.Set-vs-env-var-tracking conflict, and a style cache left seeded with a partial scheme), and +one dead/unused test-only field that was itself racing for no reason. Round 5: bumping the job's +runner (once every timeout was fixed, this became the slowest check in the PR) picked the wrong +RunsOn family on the first attempt -- fewer cores than before, and its AMI's older Ubuntu broke an +apt-mirror workaround copied from a GitHub-hosted-runner job. Round 6, once the runner was fixed +and the job ran to completion for the first time: a single root cause in `pkg/perf`'s hot-path +performance-tracking code (used by nearly every function in the codebase) explained 21 of 24 +data races and, once combined with three tests that left tracking permanently enabled, most of +~20 fanned-out `cmd`-package test failures. One further failure in that batch was unrelated: a +pflag `Value.Set` vs `Flags().Set` distinction that silently didn't mark a flag as changed. Round +7: a genuine upstream data race in `charmbracelet/bubbles`'s progress-bar animation, a widespread +`--help`-flag-leak pattern (cobra checks a flag's *current* value on every `Execute()`, not just +whether it was in that call's own args) found in three packages, plus two more +reset-without-restore leaks. One more failure could not be pinned down -- it didn't reproduce +twice with the same `-shuffle` seed, pointing to genuine goroutine-timing nondeterminism rather +than simple test-order dependence -- and is left as an open item. + +## Context + +`atmos test race` (`.atmos.d/test.yaml`) runs `go test -race -shuffle=on $(go list ./...)`. +Wiring it into CI (`.github/workflows/test.yml`) surfaced three latent problems that had never +been exercised together before: + +- `$(go list ./...)` included `github.com/cloudposse/atmos/tests` and `tests/testhelpers` — + the CLI acceptance suite. The `test` job's own matrix comment measures that suite's + unsharded runtime at ~90 minutes on Linux, which is exactly why that job shards it 10 ways + per OS. Run whole inside a single `-timeout 10m` package budget, it panicked with + `test timed out after 10m0s` inside `tests/testhelpers`'s `TestAtmosRunner_buildWithCoverage` + (stuck waiting on a `go build` subprocess) and separately in `tests` itself. It also builds + and shells out to a plain (non-`-race`) `atmos` binary, so instrumenting the driving test + process with `-race` provided no race coverage on the binary under test anyway. +- With `tests/...` excluded, the next run still timed out: `github.com/cloudposse/atmos/pkg/toolchain` + hit `test timed out after 10m0s`. That package's tests install real tool binaries from real + registries (no mock seam — `resolveLatestVersionWithSpinner` calls `NewAquaRegistry()` with + no test-double override) and hit the real GitHub API. Unlike the `test` job's acceptance + steps (which already set `GITHUB_TOKEN` for exactly this reason), the new race job ran every + package unauthenticated and unsharded, so every package's network calls competed for the + same runner and the same IP-wide 60/hr unauthenticated GitHub rate limit, instead of getting + a shard's worth of headroom the way the `test` job's packages do. +- That same run's log also showed a genuine (unrelated) bug: `pkg/utils`'s + `TestClearInternPool` asserted `GetInternStats().Requests == 3` without first clearing the + package-level intern pool, silently relying on running before any other test in the package + interned a string. `-shuffle=on` randomizes test order, so once another test ran first the + assertion failed (`expected: 3, actual: 17`). This is the first time `-shuffle=on` had run + against the full unit-test suite in CI — nothing before now enabled shuffle for anything but + `tests/cli_test.go`'s acceptance suite. +- Round 3, after the above landed: `pkg/toolchain` reported `WARNING: DATA RACE` inside + `TestRunInstallWithNoArgs`, between two of the concurrent batch installer's own worker + goroutines — one calling `pkg/ui/theme.getActiveThemeName()` (writes via `viper.BindEnv`, + called "on demand" on every styled render) and another calling + `pkg/http.GetGitHubTokenFromEnv()` (reads via `viper.GetString`), both against the process-wide + global `viper` singleton. `pkg/config` already has a `SafeViper` mutex-guard for exactly this + class of problem ("spf13/viper has no internal locking of its own", per its own doc comment, + written for the DAG scheduler's concurrent `LoadConfig` calls), but `pkg/http` and + `pkg/ui/theme` sit *below* `pkg/config` in the import graph (confirmed via `go list -deps`: + `pkg/config` already transitively depends on both), so they cannot import `pkg/config` to reach + it without an import cycle — which is exactly why these two call sites were still calling + `viper.*` directly. +- The same round's log also showed `TestGitHubTokenEnvBinding/TestMain_binds_environment_correctly` + failing (`expected: "", actual: ""`) — a second, unrelated `-shuffle=on` test-isolation + bug. `main_test.go`'s `TestMain` binds `"github-token"` to `ATMOS_GITHUB_TOKEN`/`GITHUB_TOKEN` + once at process start; `set_test.go`'s `teardownTest()` calls `viper.Reset()`, which discards + that binding for every test that runs afterward in the same process. Previously this was inert + because neither env var was ever set in CI; adding `GITHUB_TOKEN` in round 2 (above) made the + previously-dormant assertion in `TestGitHubTokenEnvBinding` actually run, and `-shuffle=on` + meant `set_test.go`'s reset could land before it. + +Round 4, once the job ran the full suite to completion instead of timing out partway through, one +run surfaced seven more independent failures: + +- `pkg/runner/step`'s `TestCastHandlerExecuteWithWorkflowRecordsSimulatedSteps` panicked with + `data.InitWriter() must be called before using data package functions`. This package's + `TestMain` initializes `pkg/data`'s global I/O context once for the whole binary, but + `output_mode_execution_test.go`'s `setupOutputModeCapture` helper (used by 3 tests to capture + redirected stdout/stderr) called `iolib.Reset()`/`ui.Reset()`/`data.Reset()` in its `cleanup` + closure to undo its own setup — and never re-initialized afterward, leaving the package-level + I/O context permanently nil for whatever test ran next under `-shuffle=on`. +- `pkg/lsp/server`'s `TestTextDocumentConcurrentOperations` (a test that deliberately drives + concurrent `TextDocumentDidChange` calls) hit a real `WARNING: DATA RACE`: + `DocumentManager.Update` mutated an existing `*Document`'s `Text`/`Version` fields in place + under its own lock, but `Handler.validateDocument` (called synchronously right after `Update` + returns, per that code's own comment) reads those same fields through the returned pointer with + no lock held at all — so a second, overlapping `Update` for the same URI could mutate the exact + struct an earlier caller was still reading. +- `pkg/terraform/cache`'s `TestInstallTrust_WindowsTimeoutsBlockingTrustStore` hit a real + `WARNING: DATA RACE`: `runTrustOperation` runs the install function in a background goroutine + racing a timer, and on timeout returns to the caller while that goroutine keeps running (there's + no context to cancel a plain `func(string) error` with). `nativeWindowsTrustInstall`'s closure + read the package-level `installWindowsTrustFunc` var *inside* that still-running goroutine, + so the test's `t.Cleanup` (restoring the var after the test function returns, well before the + 10-second fake install finishes) raced against it. +- `pkg/terraform/registry`'s `TestProviderMirror_VersionListsAllPlatforms` hit a real + `WARNING: DATA RACE`: its `fakeRegistry` test helper incremented `dlHits`/`verHits` int fields + from `httptest.Server` HTTP handlers, which `net/http` dispatches one goroutine per connection — + concurrent platform-version requests (the test's own point) raced on the plain `int++`. +- `pkg/provisioner/backend`'s `TestAzurermBackendRegisteredInRegistry` failed + (`GetBackendCreate(azurerm)` etc. all nil) — the same "reset without restore" shape as round 3's + `github-token` binding, but for the backend registry: `azurerm.go`'s `init()` registers azurerm + exactly once at process start, and roughly 15 sibling tests in `backend_test.go` call + `ResetRegistryForTesting()`/`resetBackendRegistry()` (many via `t.Cleanup`) to get an empty + registry for their own isolated fixtures. Any of those landing before this test under + `-shuffle=on` leaves the registry permanently empty for the rest of the process. +- `pkg/scanners/sarif`'s `TestHandler_RichTerminalBodyIncludesSourceExcerpt` failed (source + excerpt missing from the rendered output entirely). `normalize_test.go`'s + `TestNormalizeArtifactURIsRewritesNestedSARIFLocations` captured + `viper.GetString(githubWorkspaceViperKey)` and restored it via `viper.Set(...)` in cleanup, on + the assumption that this "restores" the pre-test state. It doesn't: `githubWorkspace()` resolves + this key by binding it to `GITHUB_WORKSPACE` and reading it live via `viper.BindEnv`+`GetString` + on every call; `viper.Set` installs a literal override that outranks the env binding in viper's + precedence and is never cleared by `t.Setenv`. Once that cleanup ran (capturing whatever + `GITHUB_WORKSPACE` happened to be — the real GitHub Actions runner value, in CI), every later + call to `githubWorkspace()` for the rest of the process returned that frozen value regardless of + `t.Setenv("GITHUB_WORKSPACE", "")`, so the excerpt reader looked for the source file under the + real CI workspace path instead of the test's own temp dir and silently found nothing (by + design: `pkg/validation`'s `writeRichDiagnosticSource` is a documented no-op on a read error). + +## Changes + +- `.atmos.d/test.yaml`: exclude `./tests/...` from the `race` command's package list + (`go list ./... | grep -v '^github.com/cloudposse/atmos/tests'`); raise its `-timeout` from + the default 10m to 20m to give unsharded, contention-heavy packages like `pkg/toolchain` + headroom; updated the `--race` flag/subcommand description to match (previously said "quick + tests" despite always running the full suite, then said "full test suite" before this fix + scoped it down to excluding `tests/...`). +- `.github/workflows/test.yml`: added the `race` job (ubuntu-latest, `libudev-dev`/`pkg-config` + installed for the `CGO_ENABLED=1` build — see the job's own comment for that trap); its + "Run atmos test race" step now sets `GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}`, matching the + `test` job's acceptance steps. +- `pkg/utils/string_utils_test.go`: `TestClearInternPool` now calls `ClearInternPool()` before + interning anything, matching the pattern already used by the adjacent `TestResetInternStats`. +- New `pkg/viperguard` package: a leaf package (no Atmos-internal imports) mutex-guarding the + global `viper` singleton's `Set`/`BindEnv`/`GetString`/`GetBool`/`GetStringSlice`/`IsSet`/`View`, + usable from packages below `pkg/config` in the import graph. `pkg/config/global_viper.go`'s + `SafeViper` now delegates every method to `pkg/viperguard` instead of keeping its own + independent mutex — two separate locks guarding the same underlying viper singleton would not + exclude each other, leaving the exact cross-package race this closes. All 8 existing + `cfg.GlobalViper()` call sites (`cmd/terraform/utils.go`, `internal/exec/shell_utils.go`, + `pkg/config/edition.go`, `pkg/config/load.go`, `pkg/auth/profile_fallback.go`) are unchanged — + `SafeViper`'s public method signatures and `ViperReader` (now a type alias) are unchanged. +- `pkg/ui/theme/styles.go`: `getActiveThemeName()`'s `viper.BindEnv`/`IsSet`/`GetString` calls + now go through `pkg/viperguard`. +- `pkg/http/client.go`: `GetGitHubTokenFromEnv()`'s global-instance `viper.GetString("github-token")` + call now goes through `pkg/viperguard`; the caller-supplied-`*viper.Viper` override path (used + in tests to avoid mutating shared global state) is untouched, since that instance is never + shared and was never part of the race. +- `pkg/toolchain/set_test.go`: `teardownTest()` now re-binds `"github-token"` after + `viper.Reset()`, so it no longer leaves the global instance de-bound for whatever test the + shuffled order runs next. +- `pkg/toolchain/github_token_test.go`: `TestMain_binds_environment_correctly` now re-binds + `"github-token"` defensively before asserting on it, rather than assuming `TestMain`'s + one-time binding survived every sibling test that happened to run first. +- `pkg/runner/step/output_mode_execution_test.go`: `setupOutputModeCapture`'s `cleanup` closure + now re-initializes `iolib`/`ui`/`data` against the restored `os.Stdout`/`os.Stderr` after + resetting them, mirroring `TestMain`'s own setup, instead of leaving the package-level I/O + context nil for the rest of the process. +- `.github/workflows/test.yml`: the `race` job now runs on the RunsOn `runner=terraform` family + (the same one the `build` job's linux leg already uses for CPU-heavy Go work), not + `ubuntu-latest`, since `go test`'s package-level concurrency scales with cores and 4 cores was + the bottleneck. `Harden Runner` (doesn't cover RunsOn) is replaced with the same + `runs-on/action` setup step the build job's linux leg uses. +- `pkg/lsp/server/documents.go`: `DocumentManager.Update` now builds a new `*Document` (copying + `URI`/`LanguageID` from the existing entry) instead of mutating the existing struct's fields in + place, so a caller still holding an earlier `Update`/`Open` call's returned pointer keeps + reading a frozen, private snapshot no matter what a later `Update` does to the map. +- `pkg/terraform/cache/trust_install.go`: `nativeWindowsTrustInstall`/`nativeWindowsTrustRemove` + now snapshot `installWindowsTrustFunc`/`removeWindowsTrustFunc` into a local variable before + `runTrustOperation` spawns its background goroutine, so that goroutine only ever touches its own + private copy, never the shared package var a test's `t.Cleanup` might reassign mid-flight. +- `pkg/terraform/registry/provider_mirror_test.go`: removed `fakeRegistry`'s `dlHits`/`verHits` + fields — write-only, never read anywhere in the codebase; deleting the dead counters removes the + race along with the pointless state. +- `pkg/provisioner/backend/azurerm_test.go`: `TestAzurermBackendRegisteredInRegistry` now re-runs + azurerm's four `RegisterBackend*` calls (the same ones `init()` makes) before asserting, instead + of assuming `init()`'s registrations survived every sibling test that resets the registry. +- `pkg/scanners/sarif/normalize_test.go`: `TestNormalizeArtifactURIsRewritesNestedSARIFLocations` + now uses `t.Setenv("GITHUB_WORKSPACE", workspace)` instead of `viper.Set`/`viper.GetString` + capture-and-restore, matching how every other test in this codebase controls this env-bound key. +- `pkg/ui/theme/styles_test.go`: `TestInitializeStyles` now calls `t.Cleanup(InvalidateStyleCache)` + after seeding the package-level style cache with a partial `ColorScheme` (no `Border` set), + matching the sibling `TestComponentLabelStyleCyclesPalette` (log_styles_test.go), which already + does this for the same reason — `TestGetBorderColor` was asserting an empty string. +- `internal/exec/vendor_model.go`: `handleInstalledPkgMsg` now only calls + `m.progress.SetPercent(...)` (and returns its `tea.Cmd`) when `m.isTTY` — working around the + upstream `bubbles` `progress.Model` race documented above, and skipping animation work that + never renders to anyone when there's no TTY regardless. +- `cmd/init/init_test.go`: `TestInitCmd_Integration_Help` now resets `initCmd`'s `help` flag via + `t.Cleanup` after setting it, instead of leaving it `true` for every later test that calls + `initCmd.Execute()`. +- `cmd/scaffold/scaffold_test.go`: added a shared `resetHelpFlag(t, cmd)` helper and applied it to + all four `TestScaffold*Cmd_Integration_Help` tests, same fix as `cmd/init`'s. +- `cmd/version/list_test.go`: `TestListCommand_FormatValidation` now resets the package-level + `listFormat` var to `"table"` (its real flag default) via `t.Cleanup`, instead of leaving it at + `"invalid"` (its last test case's value) for whatever test runs next. +- `cmd/validate_editorconfig_test.go`: `TestEditorConfigCmdCIFlagRegisteredThroughStandardParser` + now re-runs `ciFlagsParser.BindFlagsToViper(editorConfigCmd, viper.GetViper())` (the same call + `init()` makes) before asserting, rather than assuming that one-time binding survived every + sibling test's `viper.Reset()`. + +## Validation + +- `go test -race -shuffle=on ./pkg/utils/... -run 'TestClearInternPool|TestIntern|TestResetInternStats' -v -count=3` + — all pass across 3 shuffled orderings (previously failed under some orderings). +- `go test -race -shuffle=on ./pkg/utils/... -count=1` — full package passes. +- `go build ./...` — clean. +- `python3 -c "import yaml; yaml.safe_load(...)"` and `actionlint .github/workflows/test.yml` + — both workflow/command YAML files parse and lint clean. +- Did not reproduce the `pkg/toolchain` timeout locally (both rounds 2 and 3): a full + `go test -race -shuffle=on ./pkg/toolchain/...` run stalls at near-zero CPU usage in this + sandboxed environment (likely restricted/proxied network egress here, unrelated to GitHub + Actions), so it was killed rather than trusted as a timing signal. The `-timeout 20m` and + `GITHUB_TOKEN` changes are reasoned from the CI log evidence rather than confirmed by a clean + local full-package repro. The next real CI run of this job is the actual validation for that + part of the fix and should be checked. +- `go build ./...` — clean (confirms no import cycle from `pkg/http`/`pkg/ui/theme` importing + `pkg/viperguard`, and `pkg/config`'s delegation compiles). +- `go vet ./pkg/viperguard/... ./pkg/config/... ./pkg/http/... ./pkg/ui/theme/...` — clean. +- `./custom-gcl run --new-from-rev=origin/main` — 0 issues (includes the `lintroller` + `perf.Track` mandate on every new `pkg/viperguard` public function). +- `gofumpt -l` on every changed/new Go file — no output (already formatted). +- New `pkg/viperguard/viperguard_test.go`'s `TestConcurrentBindEnvAndGet` reproduces the exact + shape of the caught race (concurrent `BindEnv` + `GetString` + `Set` + `GetStringSlice`/`IsSet` + against the global singleton) — passes under `go test -race -shuffle=on -count=3`. Sanity-checked + the test actually detects this class of race: a throwaway copy calling bare `viper.BindEnv`/ + `viper.GetString` directly (bypassing `pkg/viperguard`) reliably fails with `WARNING: DATA RACE` + under `-race`; the real test, going through `pkg/viperguard`, does not. +- `go test -race -shuffle=on ./pkg/config/... ./pkg/http/... ./pkg/ui/theme/... -count=1` — all + packages pass, including the pre-existing `pkg/config/global_viper_test.go` suite (proving + `SafeViper`'s delegation preserves its documented locking guarantees: `View` is atomic, + `GetStringSlice` still clones, `ViperReader` still can't be type-asserted back to `*viper.Viper`). +- `go test -race -shuffle=on ./pkg/toolchain/... -run 'TestGitHubTokenEnvBinding' -v -count=3` — + all pass across 3 shuffled orderings (previously failed under some orderings/once `GITHUB_TOKEN` + was actually set). +- Did not get a clean full-package `pkg/toolchain` run locally (see above) to directly confirm + `TestRunInstallWithNoArgs` no longer races; the next real CI run is the actual validation for + that specific test, though `TestConcurrentBindEnvAndGet` exercises the identical race shape. +- Round 4: `go build ./...` and `go vet` clean; `./custom-gcl run --new-from-rev=origin/main` — 0 + issues; `gofumpt -l` — no output on every changed file. +- `go test -race -shuffle=on ./pkg/lsp/server/... -run TestTextDocumentConcurrentOperations -count=5` + and the full package (`-count=1`) — all pass. +- `go test -race -shuffle=on ./pkg/terraform/cache/... -run 'TestInstallTrust|TestRemoveTrust' -count=3` + — all pass (3 full shuffled passes over every install/remove test in the file). +- `go test -race -shuffle=on ./pkg/terraform/registry/... -count=3` — full package passes. +- `go test -race -shuffle=on ./pkg/provisioner/backend/... -count=3` — full package passes. +- `go test -race -shuffle=on ./pkg/scanners/sarif/... -count=3` — full package passes. +- `go test -race -shuffle=on ./pkg/ui/theme/... -count=5` — full package passes. +- `go test -race -shuffle=on ./pkg/runner/step/... -count=3` — full package passes. +- Did not reproduce the runner-swap's actual speedup locally (no access to RunsOn from this + sandbox); the next real CI run is the validation for that change specifically. + +Round 5: the runner swap itself failed immediately, before any tests ran. The "Install Linux +build dependencies" step's `sed -i .../ubuntu.sources` errored with `sed: can't read +/etc/apt/sources.list.d/ubuntu.sources: No such file or directory` (job exit code 2) — the RunsOn +"terraform" runner's AMI is Ubuntu 22.04 (`runs-on-v2.2-ubuntu22-full-x64-...`), which doesn't +have the DEB822 `.sources` file at all (that's a GitHub-hosted-runner-image thing, Ubuntu 24.04+); +this `sed` line was copied from the `floci-go` job, which runs on `ubuntu-latest`. Separately, +the same log's runner-details table showed the "terraform" family is an `i4i.large`: 2 cores, +15.7GB RAM -- *fewer* cores than `ubuntu-latest`, a downgrade for this CPU/memory-bound workload, +not the upgrade intended. Checked the `release` job's `goreleaser` step (also RunsOn) for +comparison: its "large" family is an `r7a.xlarge`, 4 cores, 31GB RAM. + +- `.github/workflows/test.yml`: switched `race`'s `runs-on` from `runner=terraform` to + `runner=large`; guarded the `ubuntu.sources` `sed` behind a `[ -f ... ]` check so the step + works on either runner image instead of erroring outright when the file doesn't exist. + +Round 7, once the round-6 fixes landed and the job ran to completion again with a real 4-core +runner: a fresh run surfaced a smaller but still varied set of failures: + +- `internal/exec`'s `TestExecuteComponentVendorPullBatch_PullsAllComponentsInOneCall` hit a real + `WARNING: DATA RACE` inside `github.com/charmbracelet/bubbles@v1.0.0`'s `progress.Model`: + `SetPercent` mutates `m.tag`/`m.targetPercent` directly and returns a `tea.Cmd` + (`nextFrame`) whose closure reads `m.tag`/`m.id` back off the same `*Model` pointer when the + tick fires -- on bubbletea's own command-execution goroutine, not the model's owning goroutine. + Calling `SetPercent` again (a second package finishing) before that tick fires -- which + completing package installs faster than one animation frame, as this test's mocked installs + do, reliably triggers -- races. This is an upstream library bug, not an Atmos usage defect; + vendoring/patching `bubbles` was out of scope, so the fix avoids triggering it instead: nothing + renders `m.progress.View()`'s animation without a TTY, so `SetPercent` (and the `tea.Cmd` it + returns) is now only called when `m.isTTY`. This closes the CI failure but not every + theoretical production case (a real TTY session installing several same-machine components + faster than one frame could still hit it) -- an upstream fix or report would close it fully. +- Three packages had the *same* latent bug shape, previously undetected because nothing had ever + run their `--help` integration test before another test that also calls the same command's + `Execute()`: cobra's `execute()` checks the `help` pflag's *current* value on every call + (`c.Flags().GetBool("help")`), not whether `-h`/`--help` was in that specific invocation's own + args. `initCmd.SetArgs([]string{"--help"})` (`cmd/init`) and the four + `scaffold*Cmd.SetArgs([]string{"--help"})` calls (`cmd/scaffold`) never reset the flag + afterward, so once any of them ran, every later test in the same package's binary that called + that command's `Execute()` got `nil` back having silently printed help -- `RunE` never ran, no + matter what args that later test passed. This is exactly why `TestExecuteInit_ArgumentParsing` + (fixed already once for a different reason, in round 6) kept reappearing: reproducing it needed + the *same* fixed `-shuffle` seed to confirm, since `--help`-leak and ordering both had to align. + `cmd/root_test.go`'s two `RootCmd.SetArgs([]string{"--help"})` sites don't have this problem -- + they already call `NewTestKit(t)`, which snapshots and restores all of `RootCmd.Flags()`, + including `help`. +- `cmd/version/list_test.go`'s `TestListCommand_ValidationErrors` failed with the wrong error + (`"invalid format: invalid ..."` instead of the expected date-format error) because + `TestListCommand_FormatValidation`'s last case sets the package-level `listFormat` var (which + `listCmd`'s `RunE` reads directly, not through a bound flag) to `"invalid"` and never restores + it, so a later test's own unrelated validation check hit that stale value first. +- `cmd/validate_editorconfig_test.go`'s `TestEditorConfigCmdCIFlagRegisteredThroughStandardParser` + failed for the now-familiar reason: `editorConfigCmd`'s `ciFlagsParser.BindFlagsToViper(...)` + runs once in `init()`; some other test's `viper.Reset()` discards it, and nothing rebinds. +- One more failure, `cmd/list`'s `TestListStacksWithOptions_CoverageIntegration` ("authentication + requires at least one identity configured"), reproduced twice via full-package `-shuffle=on` + scans but did **not** reproduce on either retry using the exact seed that had just produced it + -- ruling out a simple ordering/leftover-value explanation (which would be seed-deterministic) + in favor of genuine goroutine-timing nondeterminism between an earlier test and this one. Ruled + out during investigation: `cmd/list`'s only two `t.Parallel()` tests + (`cmd/list/closure_test.go`) touch no auth/config/viper state at all, and `t.Chdir` (used by + `chdirToCompleteFixture`) has Go's own built-in serialization against concurrent use. Left + unresolved -- see Follow-ups. + +Round 6: with the runner fixed, the job ran to completion (~40 minutes) and failed with ~20 +distinct `cmd` package test failures and 24 `WARNING: DATA RACE` blocks. 21 of the 24 race blocks +traced back to a single root cause: `pkg/perf.finishSimpleStackTracking`, the "simple stack" +performance-tracking fast path used by the `defer perf.Track(...)` call at the top of nearly +every public function repo-wide. `trackWithSimpleStack`'s own comment already documents a "known +limitation" -- it only verifies goroutine ownership of the shared global `simpleStack` at call +depth 0 or 1, "trusting" ownership at deeper nesting for speed, so a second goroutine's calls can +silently start sharing that stack undetected. The resulting cross-goroutine frame mixing wasn't +just producing wrong metrics (the accepted tradeoff) -- `StackFrame.childTime`, read and written +via plain `time.Duration` field access with no synchronization at all, was a genuine, unguarded +data race once two goroutines' frames were actually interleaved on the same stack. + +That still leaves the question of why so many otherwise-unrelated `cmd` tests hit this: perf +tracking is off (`Track` a no-op) unless something calls `perf.EnableTracking(true)`, and normal +test runs never do. `cmd/root_heatmap_test.go`'s `TestDisplayPerformanceHeatmap` (both table-driven +cases) and `TestHeatmapNonTTYOutput` do call it directly to exercise the heatmap display, with +misleading comments claiming to "Reset perf registry" (no such function existed) -- and, unlike +the well-behaved sibling `TestEnableHeatmapIfRequested` (`cmd/cmd_utils_test.go`), never called +`perf.EnableTracking(false)` afterward. Once any of the three ran under `-shuffle=on`, tracking +stayed permanently on for the rest of the `cmd` package's test binary, so every real +`perf.Track()` call in every subsequent test -- hundreds of them, many touching goroutines via +`internal/exec`'s concurrent YAML/stack processing -- became live and exposed to the race above. +`TestEnableHeatmapIfRequested` failed itself for a related but distinct reason: with no registry +reset ever available, its own few tracked calls got crowded out of the heatmap's top-N display by +the (now real) flood of accumulated metrics from whatever ran before it. + +One further failure, `TestUninstallCmd_RunE_MultipleSkills`, was unrelated to the perf issue +entirely: it set the `force` flag via `uninstallCmd.Flags().Lookup("force").Value.Set("true")`, +which updates the flag's value but -- unlike `Flags().Set("force", "true")`, which every other +force-flag test in the same file correctly uses -- does not mark the pflag as `Changed`. Since +`uninstall.go` reads the value through viper (`v.GetBool("force")`, per the flag-handling +mandate), not the raw flag, and viper's precedence favors an explicitly-changed flag, an unmarked +"true" could resolve to whatever unrelated value was left over from a prior test instead, which +under `-shuffle=on` could genuinely be "prompt for confirmation" -- and the test always ran +headless, so that prompt itself errors immediately as impossible. + +- `pkg/perf/perf.go`: `StackFrame.childTime` changed from `time.Duration` to `atomic.Int64` + (nanoseconds), with `.Load()`/`.Add()` at both read/write sites (shared by both the simple-stack + and goroutine-local-stack code paths, which use the same struct). New `ResetForTesting()` clears + the metrics registry, matching what the misleading pre-existing comments already claimed to do. +- `cmd/root_heatmap_test.go`: all three call sites now pair `perf.EnableTracking(true)` with + `t.Cleanup(func() { perf.EnableTracking(false) })` and call `perf.ResetForTesting()` first. +- `cmd/cmd_utils_test.go`: `TestEnableHeatmapIfRequested` now also calls + `perf.ResetForTesting()` before its own assertions, for the same reason. +- `cmd/ai/skill/uninstall_test.go`: `TestUninstallCmd_RunE_MultipleSkills` now sets the force flag + via `Flags().Set("force", "true")` (matching every sibling test in the file) instead of + `Lookup("force").Value.Set("true")`, and resets it to `"false"` via `t.Cleanup`. + +Round 6 validation: + +- `go build ./...`, `go vet ./cmd/... ./pkg/perf/...` — clean. +- `./custom-gcl run --new-from-rev=origin/main` — 0 issues (one `godot` finding on the new + `StackFrame` doc comment, fixed). +- `gofumpt -l` on every changed file — no output. +- `go test -race -shuffle=on ./pkg/perf/... -count=3` — full package passes. +- `go test -race -shuffle=on ./cmd/... -run 'TestEnableHeatmapIfRequested|TestDisplayPerformanceHeatmap|TestHeatmapNonTTYOutput' -count=3` + — passes (exit 0 across the whole `./cmd/...` tree, no FAIL anywhere). +- `go test -race -shuffle=on ./cmd/ai/skill/... -count=3` — full package passes. +- Re-ran the exact set of 18 originally-failing top-level test names (everything from the CI log + except `TestPackerValidateCmd`, a separate, pre-existing environment issue -- `packer init` was + never run, so its plugins aren't installed; unrelated to this incident) across the whole + `./cmd/...` tree with `-race -shuffle=on`: exit 0, no FAIL lines anywhere in ~19000 lines of + output. This is the strongest signal yet that the fan-out is resolved, though (as with every + other round) the actual CI run against the real RunsOn `large` runner is the final check. + +Round 7 validation: + +- `go build ./...`, `go vet ./cmd/...` — clean. +- `./custom-gcl run --new-from-rev=origin/main` — 0 issues (one `godot` finding, fixed). +- `gofumpt -l` on every changed file — no output. +- `go test -race -shuffle=1788300210151906000 ./cmd/init/... -v` (the exact seed that reproduced + the failure) — passes; 25 further `-shuffle=on` scans of the full package — all clean. +- `go test -race -shuffle=on ./cmd/scaffold/...` — clean. +- `go test -race -shuffle=on ./internal/exec/... -run TestExecuteComponentVendorPullBatch -count=5` + — all pass; sanity-checked the fix actually addresses the race by confirming the mechanism + (`SetPercent`'s returned `tea.Cmd` is genuinely what races, per the upstream source read). +- `go test -race -shuffle=on ./cmd/version/... -count=3` and + `go test -race -shuffle=on ./cmd/... -run TestEditorConfigCmdCIFlagRegisteredThroughStandardParser -count=3` + — both clean. +- Full `go test -race -shuffle=on ./cmd/...` (one complete pass, ~45 minutes locally) — one + failure: `cmd/list`'s `TestListStacksWithOptions_CoverageIntegration`, investigated and left + open (see Follow-ups) after it didn't reproduce on retries with the seed that had just produced + it. + +## Follow-ups + +None. + +## Round 9 (resolved `cmd/list` flake) + +The `cmd/list` failure documented in the prior round's Follow-ups (`TestListStacksWithOptions_CoverageIntegration` +and, in a later CI run, three siblings — `TestExecuteListInstancesCmd_TreeFormat`, +`TestExecuteListInstancesCmd_MatrixFormat`, `TestListStacksWithOptions_TreeFormatWithProvenance` — all +failing with `authentication requires at least one identity configured in atmos.yaml`) is a genuine +test-order leak, not goroutine-timing nondeterminism as the previous round concluded (that conclusion was +wrong: retrying with the same `-shuffle` seed via `-run ` narrows the executed test set, which changes +the deterministic order and hides the leak — it doesn't prove the failure is non-order-related). + +Root cause: `cmd/list/affected_test.go`'s `TestAffectedIdentityFlagParsing`, `cmd/list/instances_test.go`'s +`TestInstancesIdentityFlagLogic`, and `cmd/list/utils_test.go`'s `TestGetIdentityFromCommand` and +`TestGetIdentityFromCommand_NormalizesIdentityEnvFalse` all call `viper.Reset()` then +`viper.Set("identity", "viper-identity"|"env-identity"|"no")` against the global viper singleton with no +`t.Cleanup` to restore it. Under `-shuffle=on`, if any of these run before an executor-integration test that +builds a fresh `cmd` (no `--identity` flag set), `getIdentityFromCommand`'s viper fallback +(`cmd/list/utils.go`) picks up the leaked identity name. Since a non-empty `identityName` short-circuits +`resolveIdentityName` (`pkg/auth/manager_helpers.go`) without checking whether auth is even configured, the +leaked value reaches `CreateAndAuthenticateManagerWithAtmosConfigForStack`'s `isAuthConfigured` check — which +then correctly fails, because the `complete` fixture (used by `chdirToCompleteFixture`) has no `auth:` section +at all. `cmd/list/settings_test.go`'s `TestSettingsCmd_RunE_CoverageIntegration` already carried a comment +describing this exact mechanism and worked around it locally (`cmd.Flags().Set("identity", "false")`); the +other executor-integration tests never got the same treatment, which is why only they flaked. + +Fix: added `t.Cleanup(viper.Reset)` to each of the four leaking tests, matching the pattern already used +elsewhere in this package and throughout this incident. + +Validation: `go build ./cmd/list/...`, `go vet ./cmd/list/...` — clean. `go test -race -shuffle=on +./cmd/list/... -timeout 300s`, run twice (each with its own random shuffle order) — both pass, no +`authentication requires at least one identity` failures. A separate `-count=3` invocation at the same +300s timeout is **not** part of this validation record: it was interrupted by the timeout before all three +passes finished (`cmd/list` under `-race` takes ~90-110s per single pass locally, so three consecutive +passes need a longer timeout than 300s) and produced no result, passing or failing — the goroutine dump it +printed was ordinary `t.Parallel()` tests waiting their turn, not a deadlock, but the run itself proves +nothing either way and would need to be rerun with a longer timeout to count as evidence. + +## Round 9 addendum + +Two more shuffle-order bugs were fixed alongside Round 9's `cmd/list` root cause, in the same commit, since +they surfaced in the same CI log and follow the identical pattern: + +- **`cmd/describe_workflows_test.go`'s `TestDescribeWorkflows`** panicked with `workflows flag redefined: + pager`. The test unconditionally calls `describeWorkflowsCmd.Flags().StringP("pager", ...)`; `--pager` is + also a `RootCmd` persistent flag, and cobra's `mergePersistentFlags()` (itself `Lookup`-guarded, unlike a + raw `StringP`/`AddFlag` call) merges it into `describeWorkflowsCmd`'s local `FlagSet` the first time some + *other* test drives the command through the full `Execute()` pipeline. Under `-shuffle=on`, if that other + test runs first, the flag already exists and the direct `StringP` call panics. Fixed by guarding it with a + `Lookup` check first, matching `AddFlagSet`'s own safety. +- **`pkg/auth/manager_test.go`'s `TestManager_Whoami_FallbackAuthenticationFails`** expected an authentication + failure but got a *success* result with credentials from a completely different test's provider. + `pkg/auth/manager_chain.go`'s `processCredentialCache` (a package-level `sync.Map`, intentionally + process-scoped so it doesn't hold data across separate CLI invocations) was never reset between tests that + reuse the same provider/identity names (`"p"`/`"dev"`, used throughout this file) — a passing test's cached + credentials leaked into a later test asserting failure. Fixed by adding `resetProcessCredentialCache()` + + `t.Cleanup(resetProcessCredentialCache)` to the 11 `TestManager_Whoami*`/`TestManager_Authenticate*` tests + that build a `manager` and call `Authenticate`/`Whoami`/`AuthenticateProvider`, matching the pattern already + used in this package's other test files (`manager_chain_process_cache_test.go`, + `manager_ambient_provider_test.go`, `manager_chain_ambient_test.go`). +- **`cmd/terraform/cache/mirror_test.go`'s `TestMirrorCmdRunSingle`** expected `Options.All == false` but got + `true`. `TestMirrorCmdRunAll` (a sibling test) passes `--all`, which cobra parses onto the package-level + `mirrorCmd`'s own `--all` flag with `Changed = true`; `Options.All` is read via `v.GetBool("all")` (viper's + flag binding, which honors `Changed`, not just the flag's default), so a later test that never passes + `--all` still observed `All = true` if it ran after `TestMirrorCmdRunAll` under `-shuffle=on`. Fixed by + capturing the flag's original value and `Changed` state before `TestMirrorCmdRunAll` mutates it, and + restoring both (not just the value) in cleanup — the same pattern CodeRabbit flagged for + `cmd/ai/skill/uninstall_test.go`'s `force` flag in this same PR's review. + +## Round 10 (seven more independent fixes, including one production crash bug) + +The push containing Round 9's fixes produced a new CI run (`33574641692`) with nine `--- FAIL` entries. The +Round 9 addendum fixes above were confirmed resolved (none of those three tests appear in this run's failure +list). Two of the nine need a real `tofu`/`packer` binary the race job's runner doesn't install — +`TestPackerValidateCmd` was already a documented pre-existing gap; `TestRunTerraformMigratePlan_NoMigrationsDirSkipsCleanly` +passed in isolation and across several full-package `-shuffle=on` local reruns (even with `tofu`/`terraform` +removed from `PATH`, which would make an erroneous invocation loud), so its CI-only failure could not be +reproduced or root-caused locally — left open below rather than force a guess-fix. `TestContextWriteRecordsMaskedOutput` +is also left open below: it did not reproduce locally, and its adjacent CI log line looks like unrelated +output interleaved from a concurrently-running package's own test binary rather than genuine contamination +of its own captured stdout. The other six were root-caused and fixed. (A separate CI run, `33576003604`, for +the commit that already contained Round 9's fixes but not yet this round's, independently re-confirmed two of +these: `TestInstallCmd_RunE_AlreadyInstalledOmitsLocationWhenNothingInstalled` reappeared, and its sibling +`TestUserIdentity_LoadCredentials` hit the exact same `us-east-2` symptom as `TestPermissionSetIdentity_LoadCredentials` +below, confirming the `setupAWSEnv` fix covers more than the one test that first exposed it.) + +- **`pkg/auth/identities/aws/permission_set_test.go`'s `TestPermissionSetIdentity_LoadCredentials`** expected + region `us-east-1` (from its own written SSO config file) but got `us-east-2`. Root cause in production + code: `pkg/auth/identities/aws/credentials_loader.go`'s `setupAWSEnv` only added `AWS_REGION` to the + save/restore map when the identity resolved a non-empty region, so when it didn't (this test's identity has + no configured region), any ambient `AWS_REGION` left over from an *earlier* identity's credential load in + the same process was never cleared, and the AWS SDK gives an explicit env var precedence over the shared + config file's per-profile region. Fixed by always tracking `AWS_REGION` in `setupAWSEnv`'s save/restore map + and explicitly `os.Unsetenv`-ing it when the resolved region is empty, instead of leaving it untouched — + this is a real production correctness fix, not just a test-isolation one: region resolution must not depend + on whichever other identity's credentials were loaded earlier in the process. +- **`pkg/auth/identities/azure/subscription_test.go`'s `TestSubscriptionIdentity_PostAuthenticate`** expected + `credentials.json` under a sandboxed `HOME` but got "no such file or directory". `pkg/config/homedir` caches + the resolved home directory across calls; the test sandboxes `HOME` via `t.Setenv` but never called + `homedir.Reset()` + `homedir.DisableCache = true`, so a prior test's cached (real) home directory could + outlive the `t.Setenv` and `SetupFiles` would write somewhere other than the test's temp dir. Fixed with the + same `homedir.Reset()`/`DisableCache`/cleanup pattern already used in `cmd/ai/skill/uninstall_test.go`. +- **`pkg/generator/generator_test.go`'s `TestGenerate`** ("runs single generator by name" subtest) failed with + `generator not found: single` immediately after the subtest assigned that exact generator into the + package-level `registry` var. Root cause: `GetRegistry()` lazily initializes `registry` via `sync.Once` + (`registryOnce`) on its first-ever call in the whole test binary process. Several tests in this file + (`TestGeneratorRegistry`, `TestGenerateAll`, `TestGenerate`, ...) assign `registry` directly and then call a + function (`Register`, `Generate`, `GenerateAll`) that reaches `GetRegistry()` internally; if that call is the + first `GetRegistry()` call in the process, the `Once` fires there and silently overwrites the test's + manually-assigned registry with a fresh empty one, discarding whatever it just registered. Fixed with a + package-level `init()` in the test file that calls `GetRegistry()` once, before any test runs, so the + `Once` is always already settled. +- **`cmd/ci/validate_test.go`'s `TestWorkflowValidationErrorOwnsDiagnostics`** expected the rendered error to + contain the literal string `"GitHub Actions workflow validation failed"` but it didn't. Root cause: + `errUtils.DefaultFormatterConfig()`'s `MaxLineLength` is `0`, which auto-detects wrapping width from the + terminal; the race job's CI runner apparently detects a narrower width than local dev, which wrapped + "validation" and "failed" onto separate lines, breaking the single-line substring match. Fixed by pinning + `MaxLineLength: 200` in this test (wide enough that this short message never wraps), matching the existing + precedent of pinning a fixed width in `errors/examples_test.go` and `errors/formatter_test.go` rather than + relying on auto-detection in a test assertion. +- **`cmd/root_help_routing_test.go`'s `TestRootHelpFunc_RealTree_UnknownSubcommandErrors`** ("toolchain + versions --help" case) expected an "Unknown command" error but got silent success (no panic, exit 0, empty + output). Root cause: `TestRootHelpFunc_RealTree_ValidCasesStillRenderHelp` (a sibling test in the same file) + genuinely invokes `atmos toolchain --help` through the real `RootCmd` tree, which cobra parses onto + `toolchain`'s own `--help` flag; `NewTestKit` does not reach nested subcommands' flags (only `RootCmd`'s + own), so that flag stayed `true` afterward. Cobra's `execute()` checks `helpVal, _ := + c.Flags().GetBool("help")` on *every* call regardless of that call's own args — the same + leaked-`--help`-flag mechanism already fixed for `cmd/init` and `cmd/scaffold` earlier in this incident, this + time on `cmd/toolchain`. Fixed by resetting the invoked command's (and its child's, where applicable) + `--help` flag in both this test and its sibling. +- **`cmd/ai/skill/install_test.go`'s `TestInstallCmd_RunE_AlreadyInstalledOmitsLocationWhenNothingInstalled`** + expected "0 skills installed" on a second run against an already-populated fake `HOME`, but got "52 skills + updated successfully" — and its sibling `TestInstallCmd_RunE_NoArgsInstallsEveryBundledSkill` intermittently + failed the opposite way, missing "skills installed successfully in" from its output. Both tests' + `resetFlags` closures only reset `yes` (and, inconsistently, sometimes `force`) via a direct + `flag.Value.Set("false")` call, which does *not* clear `Changed` (only `Flags().Set` does) and left every + other flag `installCmd` registers (`path`, `client`, `all-clients`, `scope`, `global`) completely untouched. + `installCmd` is a package-level singleton; a later test in this same file leaking any of those flags' + `Changed` state changed which skills the next test's run considered already-installed or where it + distributed them. This file already had the correct pattern established elsewhere + (`resetFlagChangedForTest`, used by `TestInstallCmd_RunE_PathWithClientWarns` and its sibling) but these two + older tests predated it. Fixed by adding a `resetInstallCmdFlagsForTest` helper that resets every flag + `installCmd` registers via the existing `resetFlagChangedForTest`/`resetStringSliceFlagForTest` helpers, and + using it in both tests. + +A seventh fix, found while running the full local suite once with the exact CI command +(`go test -race -shuffle=on $(go list ./... | grep -v '^github.com/cloudposse/atmos/tests') -timeout 20m`), +is a genuine **production crash bug**, not just test isolation: + +- **`internal/tui/utils/utils.go`'s `PrintStyledText`/`PrintStyledTextToSpecifiedOutput`** (used by the + `atmos version` banner and help templates) call `figurine.Write`, which renders via + `github.com/common-nighthawk/go-figure` in *strict* mode (hardcoded `true` inside figurine, not + configurable from Atmos's side). Strict mode's `Slicify` calls `log.Fatal("invalid input.")` — a hard, + unrecoverable `os.Exit`, not a returned error — on the first character outside printable ASCII (`' '` + through `'~'`), which includes a plain `'\n'`. Any styled text containing a newline or control character, + rendered while color is enabled (`--force-color`, `FORCE_COLOR`, `CLICOLOR_FORCE`, or auto-detected color + support), crashes the whole `atmos` process instead of erroring gracefully. `internal/tui/utils/utils_test.go`'s + own `TestPrintStyledText`/`TestPrintStyledTextToSpecifiedOutput` tables already covered "multiline text" and + "text with special characters" cases expecting `wantErr: false`, so this was a real, if narrow, latent + crash — masked locally because it only reproduces when the color-support path is actually taken (this + environment's terminal-color auto-detection is not fully deterministic across otherwise-identical runs, so + the crash surfaced intermittently rather than every time even before this fix). Fixed with a + `sanitizeForFigurine` helper that replaces out-of-range characters with `'?'` before calling + `figurine.Write`, mirroring go-figure's own non-strict fallback behavior (`figure.go`'s `Slicify`: `else { + char = '?' }`) since figurine's strict flag itself can't be turned off from here. Verified directly: forcing + `viper.Set("force-color", true)` and calling `PrintStyledTextToSpecifiedOutput` with `"Line1\nLine2\nLine3"` + now renders successfully instead of crashing. + +Validation for all seven: `go build ./...`, `go vet ./...` — clean. `./custom-gcl run +--new-from-rev=origin/main` — 0 issues. Each fixed package's own tests pass across 3–15 `-race -shuffle=on` +reruns locally (`pkg/auth/identities/aws`, `pkg/auth/identities/azure`, `pkg/generator`, `cmd/ci`, `cmd`, +`cmd/ai/skill`, `internal/tui/utils`). A full local run of the exact CI command across the entire package +set (minus `tests/`, `-timeout 20m`) completed with 390 of 391 testable packages passing; the one failure was +`internal/tui/utils` before this round's fix, now also passing across 15 reruns. + +## Follow-ups + +- `pkg/io/recorder_test.go`'s `TestContextWriteRecordsMaskedOutput` failed once in CI with + `recorder received unmasked output`, and the failing CI log's very next line (unindented, not part of the + test framework's own `--- FAIL` output block) is a `WARN Skipping invalid mask pattern from atmos.yaml` + line whose exact pattern (`[invalid(`) matches a completely unrelated table-driven case in + `pkg/io/masker_test.go`, which builds its own fully-isolated masker and config and cannot reach this + test's global state. Did not reproduce across 5 local `-race -shuffle=on` reruns of the whole `pkg/io` + package. Most likely explanation: the CI log aggregates multiple concurrently-running `go test` package + processes' stdout, and that adjacent line is simply interleaved output from a different package's test + binary, not genuine contamination of this test's own `os.Stdout`-redirecting pipe — but this wasn't + confirmed, so treat the failure itself (not the theory) as still open. If it recurs, capture the CI log + with `##[group]`/timestamps intact (this repo's log fetch already includes per-line timestamps) and check + whether the `pkg/io` package's own timestamp range genuinely contains that warning line, or whether it + falls in a different package's timestamp window. +- `cmd/terraform/migrate`'s `TestRunTerraformMigratePlan_NoMigrationsDirSkipsCleanly` failed once in CI with + `exec: "tofu": executable file not found in $PATH`, even though the test's own comment states it must + succeed "without ever needing a real tfmigrate/opentofu binary." It passed in isolation and across several + full-package `-shuffle=on` reruns locally, including with `tofu`/`terraform` removed from `PATH` (which + would surface an erroneous invocation immediately rather than mask it) — not reproducible locally after + reasonable effort. Left open; if it recurs, capture the exact `-shuffle` seed from the failing CI run and + retry with that seed plus the full, unfiltered package (not `-run`-narrowed, per Round 9's lesson that + narrowing changes the deterministic order). +- `pkg/provisioner/provisioner_test.go`'s `TestAutoProvisionBackendWritesWarningsToOutputWriter` fails + deterministically when run in isolation (`-run `), with or without `-race`/`-shuffle`, and identically + with every change from this whole incident stashed out (verified against the exact commit already on + `origin` before this round). It passes when the full `pkg/provisioner` package runs unfiltered (both local + full-suite runs in this round show it passing), so some other test in that package incidentally provides + setup this test is missing on its own — a real test-hygiene gap, but not one that affects the actual CI + race job (which always runs full packages, never `-run`-narrowed) or this incident. Not fixed here. +- `cmd/root_test.go`'s `TestApplyCIGitCloneBootstrap_CICloneExplicitFalseOptsOut` and + `TestApplyCIGitCloneBootstrap_NoCIProviderDetected` failed in a full local `-race -shuffle=on` run of the + entire package set with `atmosConfig.CI.Enabled` unexpectedly `true` (`assert.False` on that field, not on + `applied` or `tmpConfig.CI.Enabled`, which both passed). `applyCIGitCloneBootstrap` (`cmd/root.go`) only + ever sets the package-level `atmosConfig.CI.Enabled = true` on its "bootstrap applied" branch — the branch + these two tests exercise returns early without touching it — so `atmosConfig.CI.Enabled` was already `true` + *before* either test ran. All three tests that call `applyCIGitCloneBootstrap` directly correctly wrap + themselves in `saveRestoreAtmosConfig(t)` (save-before/restore-after, not reset-to-clean), so the leak's + source is some *other* test elsewhere in the large `cmd` package that sets `atmosConfig.CI.Enabled = true` + (directly, or indirectly via a real `Execute()`/`InitCliConfig()` call that detects a real CI environment + variable) without using that same helper — not identified within this round's time budget. Neither test + appeared in any real CI run's failure list in this incident (rounds 9–11), only in this round's local + full-suite reproductions — left open rather than force a guess-fix across an unbounded search space. + +## Round 11 (the `--help`-flag leak fix, generalized) + +`TestRootHelpFunc_RealTree_UnknownSubcommandErrors` reappeared in a full local `-race -shuffle=on` run of the +*entire* `cmd` package (461s, run directly rather than filtered to just `root_help_routing_test.go`) — this +time both the "toolchain versions --help" *and* "terraform bogus-subcommand --help" cases failed, even though +Round 10 had already reset both implicated tests' own `--help` flags. The `cmd` package has 461 seconds worth +of tests; evidently some *other* test elsewhere in the package also drives a real `atmos toolchain --help` or +`atmos terraform --help` invocation through `RootCmd.ExecuteC()`, leaking that flag the same way, and +per-test whack-a-mole fixes don't scale to "some other test, somewhere in a very large package." + +Fixed at the root instead: `cmd/testing_helpers_test.go`'s `snapshotRootCmdState`/`restoreRootCmdState` +(the mechanism behind every test's `NewTestKit(t)` call) previously only snapshotted and restored `RootCmd`'s +*own* `Flags()`/`PersistentFlags()` — never any subcommand's. A real `--help` invocation against any +subcommand (`toolchain`, `terraform`, `version`, or anything else) parses onto *that command's own* FlagSet, +which is just as much a package-level singleton as `RootCmd`'s, and was never covered. Added +`walkCommandTree`, which recursively visits `RootCmd` and every command reachable from it, and used it in +both the snapshot and restore paths so every command's flags (not just `RootCmd`'s) are captured and put +back after each `NewTestKit`-protected test — closing this leak for the whole command tree at once instead +of one flag-and-command pair at a time. `cmdStateSnapshot.flags` changed from `map[string]flagSnapshot` +(flag name only, ambiguous across commands) to `map[*cobra.Command]map[string]flagSnapshot`; the three +direct-field-access tests in `cmd/testing_helpers_snapshot_test.go` were updated to index by `RootCmd` +explicitly (`snapshot.flags[RootCmd]["chdir"]`, etc.) since they only ever exercised `RootCmd`'s own flags. + +Validation: `go build ./...`, `go vet ./...` — clean. `./custom-gcl run --new-from-rev=origin/main` — 0 +issues. `go test -race -shuffle=on ./cmd/... -run 'TestSnapshotRootCmdState|TestTestKit_|TestRootHelpFunc_RealTree'` +— all pass, including both previously-flaking subtests. A full `go test -race -shuffle=on ./cmd/ -timeout +900s` (the entire package, no `-run` filter, matching the exact shape that exposed this) completed in 461s +with zero failures. A subsequent full local run of the entire package set (minus `tests/`) completed with +390 of 391 packages passing; the one remaining failure (`cmd`, two different tests than the ones this round +fixed) is the separate, still-open `atmosConfig.CI.Enabled` leak documented in Follow-ups — the `--help`-flag +leak this round targeted did not recur. diff --git a/go.mod b/go.mod index a2ceaa76729..1e4b99a4ae6 100644 --- a/go.mod +++ b/go.mod @@ -141,7 +141,7 @@ require ( golang.org/x/text v0.41.0 google.golang.org/api v0.280.0 google.golang.org/genai v1.58.0 - google.golang.org/grpc v1.82.1 + google.golang.org/grpc v1.83.1 gopkg.in/ini.v1 v1.67.3 gopkg.in/yaml.v2 v2.4.0 gopkg.in/yaml.v3 v3.0.1 @@ -171,7 +171,7 @@ require ( github.com/Azure/go-ntlmssp v0.1.1 // indirect github.com/BurntSushi/toml v1.6.0 // indirect github.com/CycloneDX/cyclonedx-go v0.11.0 // indirect - github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.32.0 // indirect + github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.33.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.56.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.56.0 // indirect github.com/Masterminds/goutils v1.1.1 // indirect @@ -394,7 +394,7 @@ require ( github.com/sourcegraph/jsonrpc2 v0.2.1 // indirect github.com/spf13/afero v1.15.0 // indirect github.com/spf13/cast v1.10.0 // indirect - github.com/spiffe/go-spiffe/v2 v2.6.0 // indirect + github.com/spiffe/go-spiffe/v2 v2.7.0 // indirect github.com/stretchr/objx v0.5.3 // indirect github.com/subosito/gotenv v1.6.0 // indirect github.com/tchap/go-patricia/v2 v2.3.3 // indirect @@ -426,7 +426,7 @@ require ( go.etcd.io/bbolt v1.4.3 // indirect go.mongodb.org/mongo-driver v1.17.9 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect - go.opentelemetry.io/contrib/detectors/gcp v1.43.0 // indirect + go.opentelemetry.io/contrib/detectors/gcp v1.44.0 // indirect go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.68.0 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.68.0 // indirect go.opentelemetry.io/otel v1.46.0 // indirect @@ -451,8 +451,8 @@ require ( golang.org/x/tools v0.49.0 // indirect golang.org/x/xerrors v0.0.0-20240903120638-7835f813f4da // indirect google.golang.org/genproto v0.0.0-20260523011958-0a33c5d7ca68 // indirect - google.golang.org/genproto/googleapis/api v0.0.0-20260523011958-0a33c5d7ca68 // indirect - google.golang.org/genproto/googleapis/rpc v0.0.0-20260523011958-0a33c5d7ca68 // indirect + google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect + google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect diff --git a/go.sum b/go.sum index 54b0ce1c976..b362a2c705e 100644 --- a/go.sum +++ b/go.sum @@ -118,8 +118,8 @@ github.com/CycloneDX/cyclonedx-go v0.11.0/go.mod h1:vUvbCXQsEm48OI6oOlanxstwNByX github.com/DATA-DOG/go-sqlmock v1.5.2 h1:OcvFkGmslmlZibjAjaHm3L//6LiuBgolP7OputlJIzU= github.com/DATA-DOG/go-sqlmock v1.5.2/go.mod h1:88MAG/4G7SMwSE3CeA0ZKzrT5CiOU3OJ+JlNzwDqpNU= github.com/DataDog/datadog-go v3.2.0+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.32.0 h1:rIkQfkCOVKc1OiRCNcSDD8ml5RJlZbH/Xsq7lbpynwc= -github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.32.0/go.mod h1:RD2SsorTmYhF6HkTmDw7KmPYQk8OBYwTkuasChwv7R4= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.33.0 h1:l7+6kwRMJNwdCvYdDl7Eax+wzEYHSnNY7zrrfbhDdTA= +github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.33.0/go.mod h1:pJTkW8hEUIIi3Pf65lPZOnn4Y81yCllX6IWk2jNXdkM= github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.56.0 h1:O2sXMyJh8b7devAGdE+163xtRurt0RVpB6DIzX5vGfg= github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.56.0/go.mod h1:hEpiGU18xf70qb3jbTcIggWAiEfX/cOIVc2OTe4OegA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.56.0 h1:ZIT85vKP7LBS84XJ0WdJ3dPOX3iz4j3c0+lpajGQMyo= @@ -1333,8 +1333,8 @@ github.com/spf13/pflag v1.0.10/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3A github.com/spf13/viper v1.4.0/go.mod h1:PTJ7Z/lr49W6bUbkmS1V3by4uWynFiR9p7+dSq/yZzE= github.com/spf13/viper v1.21.0 h1:x5S+0EU27Lbphp4UKm1C+1oQO+rKx36vfCoaVebLFSU= github.com/spf13/viper v1.21.0/go.mod h1:P0lhsswPGWD/1lZJ9ny3fYnVqxiegrlNrEmgLjbTCAY= -github.com/spiffe/go-spiffe/v2 v2.6.0 h1:l+DolpxNWYgruGQVV0xsfeya3CsC7m8iBzDnMpsbLuo= -github.com/spiffe/go-spiffe/v2 v2.6.0/go.mod h1:gm2SeUoMZEtpnzPNs2Csc0D/gX33k1xIx7lEzqblHEs= +github.com/spiffe/go-spiffe/v2 v2.7.0 h1:uXe1MflJoHw58wAUvxVlcM7WpKtijWG7I1UidcGh6g4= +github.com/spiffe/go-spiffe/v2 v2.7.0/go.mod h1:47Q0Q9/AqGha8QLHp+kxpH4Wca7X7EnOtlIJy3mxZ3U= github.com/standard-webhooks/standard-webhooks/libraries v0.0.1 h1:uOfcYT+3QungH6tIGSVCR/Y3KJmgJiHcojJbMTPDZAI= github.com/standard-webhooks/standard-webhooks/libraries v0.0.1/go.mod h1:L1MQhA6x4dn9r007T033lsaZMv9EmBAdXyU/+EF40fo= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= @@ -1493,8 +1493,8 @@ go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= go.opentelemetry.io/contrib/bridges/prometheus v0.68.0 h1:w3zlHYETbDwXyWHZlyyR58ZC39XGi8rAhkBgUgJ9d5w= go.opentelemetry.io/contrib/bridges/prometheus v0.68.0/go.mod h1:GR/mClR2nn7vE8RLwxKjoBNg+QtgdDhRzxVa93koy5o= -go.opentelemetry.io/contrib/detectors/gcp v1.43.0 h1:62yY3dT7/ShwOxzA0RsKRgshBmfElKI4d/Myu2OxDFU= -go.opentelemetry.io/contrib/detectors/gcp v1.43.0/go.mod h1:RyaZMFY7yi1kAs45S6mbFGz8O8rqB0dTY14uzvG4LCs= +go.opentelemetry.io/contrib/detectors/gcp v1.44.0 h1:NmLfL734pJhM0JKaYd2Y28+nY9dPRWYAAbxhRCrKXPw= +go.opentelemetry.io/contrib/detectors/gcp v1.44.0/go.mod h1:tNAsgd8avTGke1+MndXlU5Cru4PQ9Ai/cCNWQv/ZJ/s= go.opentelemetry.io/contrib/exporters/autoexport v0.67.0 h1:4fnRcNpc6YFtG3zsFw9achKn3XgmxPxuMuqIL5rE8e8= go.opentelemetry.io/contrib/exporters/autoexport v0.67.0/go.mod h1:qTvIHMFKoxW7HXg02gm6/Wofhq5p3Ib/A/NNt1EoBSQ= go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.68.0 h1:0Qx7VGBacMm9ZENQ7TnNObTYI4ShC+lHI16seduaxZo= @@ -1765,10 +1765,10 @@ google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55/go.mod h1:DMBHOl98 google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013/go.mod h1:NbSheEEYHJ7i3ixzK3sjbqSGDJWnxyFXZblF3eUsNvo= google.golang.org/genproto v0.0.0-20260523011958-0a33c5d7ca68 h1:cTHF8xtqtBN5sQ4dcoNwOS6FFejvFTkWQbZXsTU3trM= google.golang.org/genproto v0.0.0-20260523011958-0a33c5d7ca68/go.mod h1:RRHjglSYABVCWpQ7USCpdfhcd9t4PkajvVwyynZizTc= -google.golang.org/genproto/googleapis/api v0.0.0-20260523011958-0a33c5d7ca68 h1:WVVw1Nl19li0fMX++FJ3ye1z9+S1N35QODDy5qpnaXw= -google.golang.org/genproto/googleapis/api v0.0.0-20260523011958-0a33c5d7ca68/go.mod h1:1dCETSCY2YKZNXQE3h4fun3TYwF5p8jejRKZgfWAgAY= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260523011958-0a33c5d7ca68 h1:PvEgGJf9C/1u5CHkInMg7UFYYUoiaQmW2LbtH0pjB78= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260523011958-0a33c5d7ca68/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= +google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa h1:Kjn0N0tCrDgiAFW+lGO4JZ3ck44CehvJQMAwj9QF0G8= +google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa/go.mod h1:q4lMZS6kskjT5HvCPrnnypcDPVJqT/f4nfxmkE7gryY= +google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa h1:mZHHdPZl0dbGHCflZgAq/Q468DWVFcU2whhB2KAo8fk= +google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c= google.golang.org/grpc v1.21.0/go.mod h1:oYelfM1adQP15Ek0mdvEgi9Df8B9CZIaU1084ijfRaM= google.golang.org/grpc v1.23.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg= @@ -1776,8 +1776,8 @@ google.golang.org/grpc v1.25.1/go.mod h1:c3i+UQWmh7LiEpx4sFZnkU36qjEYZ0imhYfXVyQ google.golang.org/grpc v1.27.0/go.mod h1:qbnxyOmOxrQa7FizSgH+ReBfzJrCY1pSN7KXBS8abTk= google.golang.org/grpc v1.31.0/go.mod h1:N36X2cJ7JwdamYAgDz+s+rVMFjt3numwzf/HckM8pak= google.golang.org/grpc v1.33.2/go.mod h1:JMHMWHQWaTccqQQlmk3MJZS+GWXOdAesneDmEnv2fbc= -google.golang.org/grpc v1.82.1 h1:NnAxzGRA0677vCa4BUkOAnO5+FfQqVl9iUXeD0IqcGE= -google.golang.org/grpc v1.82.1/go.mod h1:yzTZ1TB1Z3SG+LIYaI+WiE8D5+PZ3ArnrSp8zF3+/ZA= +google.golang.org/grpc v1.83.1 h1:HIO0+BEtBP6soyqvqC8sNUjZ7bTs+0hFQuFF+RAy++Y= +google.golang.org/grpc v1.83.1/go.mod h1:kDyl6SKsiHKt0uylY5gtn5cEjkrIOhQOGDgIc4JGwzQ= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0= google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQJ+fmap5saPgwCLgHXTUD7jkjRqWcaiX5VyM= diff --git a/internal/exec/vendor_model.go b/internal/exec/vendor_model.go index 0d57daafd2d..c08107c8fe2 100644 --- a/internal/exec/vendor_model.go +++ b/internal/exec/vendor_model.go @@ -107,6 +107,7 @@ type modelVendor struct { height int spinner spinner.Model progress progress.Model + percent float64 done bool dryRun bool failedPkg int @@ -268,12 +269,6 @@ func (m *modelVendor) Update(msg tea.Msg) (tea.Model, tea.Cmd) { var cmd tea.Cmd m.spinner, cmd = m.spinner.Update(msg) return m, cmd - case progress.FrameMsg: - newModel, cmd := m.progress.Update(msg) - if newModel, ok := newModel.(progress.Model); ok { - m.progress = newModel - } - return m, cmd } return m, nil } @@ -329,12 +324,19 @@ func (m *modelVendor) handleInstalledPkgMsg(msg *installedPkgMsg) (tea.Model, te } } m.index++ - // Update progress bar - progressCmd := m.progress.SetPercent(float64(m.index) / float64(len(m.packages))) + // Update progress bar. charmbracelet/bubbles's progress.Model.SetPercent + // mutates m.tag and returns a tea.Cmd (nextFrame) whose closure reads + // m.tag/m.id back from the *Model pointer when its tick fires, on + // bubbletea's own command-execution goroutine -- a data race against a + // second SetPercent call landing before that tick fires (which packages + // completing faster than one animation frame reliably triggers; upstream + // bug, not an Atmos usage issue). Track the target percent as a plain + // field instead and render it with ViewAs (no animation, no internal + // Model state, no tea.Cmd), which sidesteps the race entirely. + m.percent = float64(m.index) / float64(len(m.packages)) version = grayColor.Render(version) return m, tea.Batch( - progressCmd, tea.Printf("%s %s %s %s", mark, pkg.Name, version, errMsg), // print message above our program ExecuteInstall(m.packages[m.index], install.InstallOptions{DryRun: m.dryRun}, m.atmosConfig), // download the next package ) @@ -458,7 +460,7 @@ func (m *modelVendor) View() string { pkgCount := fmt.Sprintf(" %*d/%*d", w, m.index, w, n) spin := m.spinner.View() + " " - prog := m.progress.View() + prog := m.progress.ViewAs(m.percent) // effectiveWidth reserves liveLineMargin trailing columns so the rendered line never touches // the terminal's true last column (see liveLineMargin's doc comment). effectiveWidth := max(0, m.width-liveLineMargin) diff --git a/internal/tui/utils/utils.go b/internal/tui/utils/utils.go index 68e74fe28b5..d8e45b58e6e 100644 --- a/internal/tui/utils/utils.go +++ b/internal/tui/utils/utils.go @@ -60,22 +60,45 @@ func PrintStyledText(text string) error { // Check --force-color flag (via Viper). // This allows `atmos version --force-color` to work for screenshot generation. if viper.GetBool("force-color") { - return figurine.Write(iolib.Data, text, AnsiRegularFont) + return figurine.Write(iolib.Data, sanitizeForFigurine(text), AnsiRegularFont) } // Check standard CLICOLOR_FORCE and FORCE_COLOR env vars. if os.Getenv("CLICOLOR_FORCE") != "" || os.Getenv("FORCE_COLOR") != "" { //nolint:forbidigo // Standard terminal env vars - return figurine.Write(iolib.Data, text, AnsiRegularFont) + return figurine.Write(iolib.Data, sanitizeForFigurine(text), AnsiRegularFont) } // Fall back to automatic color detection. // supportscolor automatically detects TTY and other standard environment variables. if supportscolor.Stdout().SupportsColor { - return figurine.Write(iolib.Data, text, AnsiRegularFont) + return figurine.Write(iolib.Data, sanitizeForFigurine(text), AnsiRegularFont) } return nil } +// sanitizeForFigurine replaces every character outside go-figure's supported +// printable-ASCII range (' ' through '~', i.e. no newlines, tabs, or other +// control/non-ASCII characters) with '?', mirroring go-figure's own +// non-strict fallback (figure.go's Slicify: "else { char = '?' }"). The +// figurine library always renders in strict mode, which cannot be +// configured from here and calls log.Fatal -- a hard, unrecoverable process +// exit, not a returned error -- on the first out-of-range character, +// including a plain '\n' in multi-line banner text. Sanitizing first keeps +// this a graceful (if visually imperfect) render instead of taking down the +// whole process. +func sanitizeForFigurine(text string) string { + var b strings.Builder + b.Grow(len(text)) + for _, r := range text { + if r < ' ' || r > '~' { + b.WriteRune('?') + continue + } + b.WriteRune(r) + } + return b.String() +} + func PrintStyledTextToSpecifiedOutput(out io.Writer, text string) error { // Helper to check if a value is truthy // Truthy values: "1", "true" (case-insensitive) - standard Go bool values @@ -112,7 +135,7 @@ func PrintStyledTextToSpecifiedOutput(out io.Writer, text string) error { forceColor := viper.GetBool("force-color") || isTruthy(atmosForceColor) || isTruthy(cliColorForce) || isTruthy(forceColorEnv) if supportscolor.Stdout().SupportsColor || forceColor { // Write to the specified output writer, not os.Stdout - return figurine.Write(out, text, AnsiRegularFont) + return figurine.Write(out, sanitizeForFigurine(text), AnsiRegularFont) } return nil } diff --git a/pkg/auth/identities/aws/credentials_loader.go b/pkg/auth/identities/aws/credentials_loader.go index 9e7b9a92439..57bfa2d8928 100644 --- a/pkg/auth/identities/aws/credentials_loader.go +++ b/pkg/auth/identities/aws/credentials_loader.go @@ -31,7 +31,8 @@ func loadAWSCredentialsFromEnvironment(ctx context.Context, env map[string]strin return nil, err } - log.Debug("Loading AWS credentials from files", + log.Debug( + "Loading AWS credentials from files", "credentials_file", envVars.credsFile, "config_file", envVars.configFile, logKeyProfile, envVars.profile, @@ -48,7 +49,8 @@ func loadAWSCredentialsFromEnvironment(ctx context.Context, env map[string]strin return nil, err } - log.Debug("Successfully loaded AWS credentials from files", + log.Debug( + "Successfully loaded AWS credentials from files", logKeyProfile, envVars.profile, "region", creds.Region, "has_session_token", creds.SessionToken != "", @@ -86,15 +88,21 @@ func extractAWSEnvVars(env map[string]string) (awsEnvVars, error) { } // setupAWSEnv temporarily sets AWS environment variables and returns a cleanup function. +// +// AWS_REGION is always tracked here, even when region is "": the AWS SDK gives an +// explicit AWS_REGION env var precedence over the shared config file's per-profile +// `region` setting, so leaving an ambient AWS_REGION untouched when this identity +// doesn't resolve one would let it silently override the profile's own region -- +// exactly the symptom that made this loader's region resolution depend on whichever +// other identity's credentials were loaded earlier in the same process (see +// docs/fixes for the incident this closes). func setupAWSEnv(credsFile, configFile, profile, region string) func() { originalEnv := make(map[string]string) envVarsToSet := map[string]string{ "AWS_SHARED_CREDENTIALS_FILE": credsFile, "AWS_CONFIG_FILE": configFile, "AWS_PROFILE": profile, - } - if region != "" { - envVarsToSet["AWS_REGION"] = region + "AWS_REGION": region, } // Save original values and set new ones. @@ -102,7 +110,11 @@ func setupAWSEnv(credsFile, configFile, profile, region string) func() { if origValue, exists := os.LookupEnv(key); exists { originalEnv[key] = origValue } - os.Setenv(key, value) + if value != "" { + os.Setenv(key, value) + } else { + os.Unsetenv(key) + } } // Return cleanup function to restore original environment. @@ -154,7 +166,8 @@ func populateExpiration(creds *types.AWSCredentials, awsCreds *aws.Credentials, // Try to read expiration from metadata comment in credentials file. if expiration := readExpirationFromMetadata(credsFile, profile); expiration != "" { creds.Expiration = expiration - log.Debug("Loaded expiration from credentials file metadata", + log.Debug( + "Loaded expiration from credentials file metadata", logKeyProfile, profile, "expiration", expiration, ) @@ -169,7 +182,8 @@ func readExpirationFromMetadata(credentialsPath, profile string) string { // Load the credentials file with comment preservation enabled. cfg, err := awsCloud.LoadINIFile(credentialsPath) if err != nil { - log.Debug("Failed to load credentials file for metadata", + log.Debug( + "Failed to load credentials file for metadata", "path", credentialsPath, "error", err, ) @@ -179,7 +193,8 @@ func readExpirationFromMetadata(credentialsPath, profile string) string { // Get the profile section. section, err := cfg.GetSection(profile) if err != nil { - log.Debug("Profile section not found in credentials file", + log.Debug( + "Profile section not found in credentials file", logKeyProfile, profile, ) return "" @@ -214,7 +229,8 @@ func readExpirationFromMetadata(credentialsPath, profile string) string { if _, err := time.Parse(time.RFC3339, expiration); err == nil { return expiration } - log.Debug("Invalid expiration format in metadata", + log.Debug( + "Invalid expiration format in metadata", "expiration", expiration, "error", err, ) diff --git a/pkg/auth/identities/azure/subscription_test.go b/pkg/auth/identities/azure/subscription_test.go index 2c5c274f113..42500b2e70a 100644 --- a/pkg/auth/identities/azure/subscription_test.go +++ b/pkg/auth/identities/azure/subscription_test.go @@ -14,6 +14,7 @@ import ( errUtils "github.com/cloudposse/atmos/errors" "github.com/cloudposse/atmos/pkg/auth/types" + "github.com/cloudposse/atmos/pkg/config/homedir" "github.com/cloudposse/atmos/pkg/schema" ) @@ -226,10 +227,20 @@ func TestSubscriptionIdentity_GetProviderName(t *testing.T) { } func TestSubscriptionIdentity_PostAuthenticate(t *testing.T) { - // Sandbox HOME so credential files land under a temp dir. + // Sandbox HOME so credential files land under a temp dir. pkg/config/homedir + // caches the resolved home directory across calls; without resetting it and + // disabling the cache, a prior test's cached (real) HOME can outlive this + // t.Setenv, so SetupFiles writes credentials.json somewhere other than + // tmpHome and the os.Stat check below finds nothing there. See docs/fixes. tmpHome := t.TempDir() t.Setenv("HOME", tmpHome) t.Setenv("USERPROFILE", tmpHome) + homedir.Reset() + homedir.DisableCache = true + t.Cleanup(func() { + homedir.Reset() + homedir.DisableCache = false + }) identity := &subscriptionIdentity{ name: "azure-test", diff --git a/pkg/auth/manager_test.go b/pkg/auth/manager_test.go index d444babe094..84016709d50 100644 --- a/pkg/auth/manager_test.go +++ b/pkg/auth/manager_test.go @@ -445,6 +445,9 @@ func TestManager_GetCachedCredentials_Paths(t *testing.T) { } func TestManager_Whoami_WithCachedCredentials(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + // Test that Whoami successfully retrieves cached credentials when available. s := &testStore{data: map[string]any{}, expired: map[string]bool{}} m := &manager{ @@ -471,6 +474,9 @@ func TestManager_Whoami_WithCachedCredentials(t *testing.T) { } func TestManager_Whoami_FallbackAuthenticationFails(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + // Test that Whoami returns error when both GetCachedCredentials and Authenticate fail. // This covers the case where no cached credentials exist and reauthentication also fails. s := &testStore{data: map[string]any{}, expired: map[string]bool{}} @@ -505,6 +511,9 @@ func TestManager_Whoami_FallbackAuthenticationFails(t *testing.T) { } func TestManager_Whoami_FallbackAuthenticationSucceeds(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + // Test that Whoami succeeds via fallback authentication when no cached credentials exist. // This covers the case where provider credentials exist (e.g., in AWS files) and can be used // to derive identity credentials without interactive prompts. @@ -888,6 +897,9 @@ func TestManager_Authenticate_Errors(t *testing.T) { } func TestManager_Authenticate_SuccessFlow(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + s := &testStore{data: map[string]any{}, expired: map[string]bool{}} called := false @@ -917,6 +929,9 @@ func TestManager_Authenticate_SuccessFlow(t *testing.T) { } func TestManager_Authenticate_PostAuthenticatePreservesHints(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + s := &testStore{data: map[string]any{}, expired: map[string]bool{}} postAuthErr := errUtils.Build(errUtils.ErrEmulatorNotRunning). WithHint("Start it with `atmos emulator up aws -s local`."). @@ -942,6 +957,9 @@ func TestManager_Authenticate_PostAuthenticatePreservesHints(t *testing.T) { } func TestManager_Authenticate_UsesCachedTargetCredentials(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + now := ptrTime(time.Now().UTC().Add(30 * time.Minute)) // Pre-seed store with valid creds for target identity. @@ -968,6 +986,9 @@ func TestManager_Authenticate_UsesCachedTargetCredentials(t *testing.T) { } func TestManager_Authenticate_ExpiredCredentials(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + // Create expired credentials. expiredTime := ptrTime(time.Now().UTC().Add(-time.Hour)) @@ -999,6 +1020,9 @@ func TestManager_Authenticate_ExpiredCredentials(t *testing.T) { } func TestManager_Authenticate_PostAuthenticateErrorDoesNotPrint(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + s := &testStore{data: map[string]any{}, expired: map[string]bool{}} m := &manager{ config: &schema.AuthConfig{ @@ -1967,6 +1991,9 @@ func TestManager_SetupAuthLogging_RestoresState(t *testing.T) { } func TestManager_AuthenticateProvider_Success(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + // Create test credentials with expiration. exp := time.Now().Add(time.Hour) creds := &testCreds{exp: &exp} @@ -2005,6 +2032,9 @@ func TestManager_AuthenticateProvider_ProviderNotFound(t *testing.T) { } func TestManager_AuthenticateProvider_CaseInsensitive(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + // Test that provider name lookup is case-insensitive. provider := &testProvider{ name: "Test-Provider", @@ -2031,6 +2061,9 @@ func TestManager_AuthenticateProvider_CaseInsensitive(t *testing.T) { } func TestManager_AuthenticateProvider_AuthenticationFailure(t *testing.T) { + resetProcessCredentialCache() + t.Cleanup(resetProcessCredentialCache) + provider := &testProvider{ name: "test-provider", authErr: fmt.Errorf("authentication failed"), diff --git a/pkg/config/global_viper.go b/pkg/config/global_viper.go index 4e27517bcda..95db6ef347d 100644 --- a/pkg/config/global_viper.go +++ b/pkg/config/global_viper.go @@ -5,9 +5,12 @@ import ( "sync" "github.com/spf13/viper" + + "github.com/cloudposse/atmos/pkg/viperguard" ) -// SafeViper wraps the process-wide global Viper singleton with a mutex. +// SafeViper wraps the process-wide global Viper singleton, delegating every +// method to pkg/viperguard's mutex-guarded functions. // // LoadConfig bridges several config-derived values back into the global Viper // singleton (e.g. profiles.base_path, vendor.update.*, vendor.ci.*) so other @@ -19,36 +22,27 @@ import ( // one per graph node -- under --max-concurrency > 1, so every access to the // singleton must go through GlobalViper() to avoid "concurrent map writes" panics. // -// This applies even to reads/writes of unrelated keys: viper.Set/Get traverse -// and mutate ONE shared underlying map (Viper.override) via deepSearch, and Go -// maps are not safe for any concurrent read/write access, regardless of which -// key each goroutine touches -- a write to "vendor.update.execution.mode" can -// still race with a concurrent read of an unrelated key like "mask". -// -// Deliberately does not cache *viper.Viper in a field: tests (and -// viper.Reset()-calling production paths) replace viper's default instance at -// runtime, so every method re-resolves viper.GetViper() under the lock rather -// than risk diverging from whatever instance is currently "the" global one. -type SafeViper struct { - mu sync.RWMutex -} +// Delegating to pkg/viperguard (rather than guarding with a mutex declared +// here) matters beyond code reuse: pkg/http and pkg/ui/theme also call global +// viper accessors directly (they sit below pkg/config in the dependency +// graph, so they cannot import this package to reach SafeViper without an +// import cycle) and route through pkg/viperguard for the same reason. A +// second, independent mutex declared in this package would not exclude +// pkg/viperguard's callers from those two packages -- two separate locks +// guarding the same underlying viper singleton do not exclude each other -- +// leaving exactly the cross-package data race this type exists to prevent. +type SafeViper struct{} func (s *SafeViper) Set(key string, value any) { - s.mu.Lock() - defer s.mu.Unlock() - viper.GetViper().Set(key, value) + viperguard.Set(key, value) } func (s *SafeViper) GetString(key string) string { - s.mu.RLock() - defer s.mu.RUnlock() - return viper.GetViper().GetString(key) + return viperguard.GetString(key) } func (s *SafeViper) GetBool(key string) bool { - s.mu.RLock() - defer s.mu.RUnlock() - return viper.GetViper().GetBool(key) + return viperguard.GetBool(key) } // GetStringSlice returns a clone of the requested key's string slice: viper's @@ -56,15 +50,11 @@ func (s *SafeViper) GetBool(key string) bool { // than a copy, and handing that out under the lock would let a caller mutate // shared Viper state after the lock is released. func (s *SafeViper) GetStringSlice(key string) []string { - s.mu.RLock() - defer s.mu.RUnlock() - return slices.Clone(viper.GetViper().GetStringSlice(key)) + return viperguard.GetStringSlice(key) } func (s *SafeViper) IsSet(key string) bool { - s.mu.RLock() - defer s.mu.RUnlock() - return viper.GetViper().IsSet(key) + return viperguard.IsSet(key) } // ViperReader exposes only *viper.Viper's read methods. SafeViper.View passes @@ -73,41 +63,7 @@ func (s *SafeViper) IsSet(key string) bool { // against another concurrent View call's reads, or against SafeViper.Set's // write lock, defeating the whole point of View. Extend with more read // methods as callers need them; never add a mutator here. -type ViperReader interface { - // IsSet reports whether key has an explicit value from any source (flag, - // env, config, override) -- unlike a plain Get, it does not count a - // registered default as "set". - IsSet(key string) bool - // GetBool returns key's value coerced to bool. Returns false if unset. - GetBool(key string) bool - // GetString returns key's value coerced to string. Returns "" if unset. - GetString(key string) string - // GetStringSlice returns key's value coerced to []string, cloned so the - // caller cannot mutate Viper's own backing array. Returns nil if unset. - GetStringSlice(key string) []string -} - -// viperReaderAdapter wraps *viper.Viper to satisfy ViperReader without -// exposing the concrete *viper.Viper type to View callbacks. Passing -// *viper.Viper itself through the ViperReader interface would only hide Set -// behind a narrower static type -- Go interfaces retain their dynamic type, -// so a callback could still type-assert the value back to *viper.Viper and -// call Set while holding only View's read lock. Because viperReaderAdapter is -// unexported, code outside this package cannot name it to assert against it, -// so it cannot recover the underlying *viper.Viper this way. -type viperReaderAdapter struct { - v *viper.Viper -} - -func (a viperReaderAdapter) IsSet(key string) bool { return a.v.IsSet(key) } - -func (a viperReaderAdapter) GetBool(key string) bool { return a.v.GetBool(key) } - -func (a viperReaderAdapter) GetString(key string) string { return a.v.GetString(key) } - -func (a viperReaderAdapter) GetStringSlice(key string) []string { - return slices.Clone(a.v.GetStringSlice(key)) -} +type ViperReader = viperguard.ViperReader // View executes fn with a read lock held on the global Viper singleton, // giving fn a consistent snapshot for the whole call. Use this instead of @@ -117,9 +73,7 @@ func (a viperReaderAdapter) GetStringSlice(key string) []string { // concurrent Set() between two separate calls could let the decision combine // one snapshot's presence result with a different snapshot's value. func (s *SafeViper) View(fn func(v ViperReader)) { - s.mu.RLock() - defer s.mu.RUnlock() - fn(viperReaderAdapter{v: viper.GetViper()}) + viperguard.View(fn) } var globalViper = &SafeViper{} diff --git a/pkg/generator/generator_test.go b/pkg/generator/generator_test.go index e0e366b873a..596e4d73ae7 100644 --- a/pkg/generator/generator_test.go +++ b/pkg/generator/generator_test.go @@ -12,6 +12,23 @@ import ( "github.com/cloudposse/atmos/pkg/schema" ) +// init settles registryOnce before any test in this file runs. GetRegistry() +// lazily initializes the package-level registry var via sync.Once on its +// first-ever call in the whole test binary process. Several tests below, +// including TestGeneratorRegistry, TestGenerateAll, and TestGenerate, assign +// registry directly and then call a function that reaches GetRegistry() +// internally (Register, Generate, GenerateAll). Under -shuffle=on, if one of +// those is the first GetRegistry() call in the process, the Once fires there +// and silently overwrites that test's manually-assigned registry with a +// fresh empty one, discarding the generators it just registered. Calling +// GetRegistry() here, before any test runs, guarantees the Once has already +// fired, so every later GetRegistry() call in these tests just returns +// whatever registry currently holds. See docs/fixes for the incident this +// closes. +func init() { + GetRegistry() +} + // testGenerator is a mock generator for testing. type testGenerator struct { name string diff --git a/pkg/http/client.go b/pkg/http/client.go index 851a0b771f4..5bf0ce408bd 100644 --- a/pkg/http/client.go +++ b/pkg/http/client.go @@ -18,6 +18,7 @@ import ( errUtils "github.com/cloudposse/atmos/errors" "github.com/cloudposse/atmos/pkg/perf" + "github.com/cloudposse/atmos/pkg/viperguard" ) const ( @@ -341,13 +342,18 @@ func (t *GitHubAuthenticatedTransport) RoundTrip(req *http.Request) (*http.Respo func GetGitHubTokenFromEnv(v ...*viper.Viper) string { defer perf.Track(nil, "http.GetGitHubTokenFromEnv")() - viperInst := viper.GetViper() + // First try viper (for toolchain commands with --github-token flag). A + // caller-supplied instance isn't shared, so it's read directly; the + // global singleton has no locking of its own and this runs from + // concurrent callers (e.g. the toolchain's concurrent batch installer), + // so that path goes through pkg/viperguard instead. + var token string if len(v) > 0 && v[0] != nil { - viperInst = v[0] + token = v[0].GetString("github-token") + } else { + token = viperguard.GetString("github-token") } - - // First try viper (for toolchain commands with --github-token flag). - if token := viperInst.GetString("github-token"); token != "" { + if token != "" { return token } diff --git a/pkg/lsp/server/documents.go b/pkg/lsp/server/documents.go index 1460df1ec19..6318ea62e2f 100644 --- a/pkg/lsp/server/documents.go +++ b/pkg/lsp/server/documents.go @@ -48,14 +48,29 @@ func (dm *DocumentManager) Update(uri protocol.DocumentUri, version int32, text dm.mu.Lock() defer dm.mu.Unlock() - doc, exists := dm.documents[uri] + existing, exists := dm.documents[uri] if !exists { // Document not open, ignore. return nil } - doc.Version = version - doc.Text = text + // Store a new *Document rather than mutating the existing struct's fields + // in place: callers read the returned pointer's fields (doc.Text, in + // particular) after this method has already unlocked -- e.g. Handler's + // validateDocument, invoked synchronously right after Update in + // TextDocumentDidChange. A second, concurrent Update for the same URI + // (two overlapping didChange notifications) would otherwise mutate the + // very struct an earlier caller is still reading with no lock held. + // Giving each version its own immutable *Document means an older + // caller's pointer stays a consistent snapshot no matter what happens to + // the map afterward. + doc := &Document{ + URI: existing.URI, + LanguageID: existing.LanguageID, + Version: version, + Text: text, + } + dm.documents[uri] = doc return doc } diff --git a/pkg/perf/perf.go b/pkg/perf/perf.go index 88ff9d87a6d..17e8ecc8880 100644 --- a/pkg/perf/perf.go +++ b/pkg/perf/perf.go @@ -43,10 +43,21 @@ type Metric struct { } // StackFrame represents a single frame in the call stack for tracking nested calls. +// +// The childTime field is atomic (nanoseconds), not a plain time.Duration: simple-stack mode +// (trackWithSimpleStack) deliberately trusts goroutine ownership without verifying +// it for every nested call, for speed -- so when a second goroutine's calls do slip +// onto the shared simpleStack undetected (its documented "known limitation"), one +// goroutine's finish reading its own frame's childTime can race with a concurrently +// finishing call elsewhere in the (logically, at that point, shared) stack writing +// to what it resolves as its parent's childTime -- the same field on the same +// frame. That cross-goroutine mixing can still produce logically confused metrics +// (an accepted, pre-existing tradeoff -- see trackWithSimpleStack), but the field +// access itself must not be a data race regardless. type StackFrame struct { functionName string startTime time.Time - childTime time.Duration // Accumulated time spent in child function calls + childTime atomic.Int64 // Accumulated time (ns) spent in child function calls. } // CallStack tracks nested function calls for a single goroutine. @@ -92,6 +103,22 @@ func EnableTracking(enabled bool) { } } +// ResetForTesting clears the metrics registry. Intended for use in tests to +// ensure test isolation: this is process-wide state with no other reset path, +// so a test that enables tracking (and any real Track() calls it triggers, or +// that any other code in the same process makes while tracking happens to be +// on) permanently accumulates into the shared registry for the rest of the +// binary's run otherwise -- e.g. a heatmap-display test's own few tracked +// calls getting crowded out of the top-N display by hundreds of unrelated +// calls from whatever ran before it. Call this alongside EnableTracking, not +// as a replacement for disabling tracking when the test is done. +func ResetForTesting() { + reg.mu.Lock() + defer reg.mu.Unlock() + reg.data = make(map[string]*Metric) + reg.start = time.Now() +} + // UseSimpleTracking enables or disables simple tracking mode. // Simple mode uses a single global call stack (faster, no goroutine ID lookups). // Use false for multi-goroutine scenarios to ensure accurate per-goroutine tracking. @@ -171,7 +198,6 @@ func trackWithSimpleStack(name string, start time.Time) func() { frame := &StackFrame{ functionName: name, startTime: start, - childTime: 0, } simpleStack.push(frame) @@ -201,7 +227,7 @@ func claimSimpleStackOwnership(owner uint64) uint64 { // finishSimpleStackTracking completes tracking for a simple stack frame. func finishSimpleStackTracking(frame *StackFrame, start time.Time, name string) { totalTime := time.Since(start) - selfTime := totalTime - frame.childTime + selfTime := totalTime - time.Duration(frame.childTime.Load()) if selfTime < 0 { selfTime = 0 } @@ -211,7 +237,7 @@ func finishSimpleStackTracking(frame *StackFrame, start time.Time, name string) // If there's a parent frame, add our total time to its child time accumulator. if parent := simpleStack.peek(); parent != nil { - parent.childTime += totalTime + parent.childTime.Add(int64(totalTime)) } // Clear ownership when stack becomes empty. @@ -234,7 +260,6 @@ func trackWithGoroutineLocalStack(name string, start time.Time) func() { frame := &StackFrame{ functionName: name, startTime: start, - childTime: 0, } stack.push(frame) @@ -246,7 +271,7 @@ func trackWithGoroutineLocalStack(name string, start time.Time) func() { // finishGoroutineLocalTracking completes tracking for a goroutine-local stack frame. func finishGoroutineLocalTracking(frame *StackFrame, start time.Time, name string, gid uint64, stack *CallStack) { totalTime := time.Since(start) - selfTime := totalTime - frame.childTime + selfTime := totalTime - time.Duration(frame.childTime.Load()) if selfTime < 0 { selfTime = 0 } @@ -256,7 +281,7 @@ func finishGoroutineLocalTracking(frame *StackFrame, start time.Time, name strin // If there's a parent frame, add our total time to its child time accumulator. if parent := stack.peek(); parent != nil { - parent.childTime += totalTime + parent.childTime.Add(int64(totalTime)) } // Clean up call stack if empty to prevent memory leaks. diff --git a/pkg/provisioner/backend/azurerm_test.go b/pkg/provisioner/backend/azurerm_test.go index 6865c2a5db1..a51f8b052d7 100644 --- a/pkg/provisioner/backend/azurerm_test.go +++ b/pkg/provisioner/backend/azurerm_test.go @@ -625,7 +625,18 @@ func TestNewAzureBackendClient(t *testing.T) { func TestAzurermBackendRegisteredInRegistry(t *testing.T) { // The init() registrations must wire azurerm into the shared backend registry so the - // auto-provision hook and `atmos terraform backend` commands can find it. + // auto-provision hook and `atmos terraform backend` commands can find it. init() only + // runs once for the whole test binary, but many sibling tests in this package call + // ResetRegistryForTesting()/resetBackendRegistry() to get an empty registry for their + // own isolated fixtures -- with -shuffle=on, one of those can run before this test and + // leave the registry empty for the rest of the process. Re-run the same registrations + // init() makes so this test verifies its own claim instead of depending on execution + // order; RegisterBackend* are plain map assignments, safe to call again. + RegisterBackendCreate(backendTypeAzurerm, CreateAzurermBackend) + RegisterBackendDelete(backendTypeAzurerm, DeleteAzurermBackend) + RegisterBackendExists(backendTypeAzurerm, AzurermBackendExists) + RegisterBackendName(backendTypeAzurerm, AzurermBackendName) + assert.NotNil(t, GetBackendCreate(backendTypeAzurerm), "create func registered") assert.NotNil(t, GetBackendDelete(backendTypeAzurerm), "delete func registered") assert.NotNil(t, GetBackendExists(backendTypeAzurerm), "exists func registered") diff --git a/pkg/runner/step/output_mode_execution_test.go b/pkg/runner/step/output_mode_execution_test.go index 6dc4d1bba12..e7c7e1eac49 100644 --- a/pkg/runner/step/output_mode_execution_test.go +++ b/pkg/runner/step/output_mode_execution_test.go @@ -78,6 +78,17 @@ func setupOutputModeCapture(t *testing.T) (*bytes.Buffer, *bytes.Buffer, func()) iolib.Reset() ui.Reset() data.Reset() + + // Re-initialize against the now-restored os.Stdout/os.Stderr, mirroring + // TestMain's own setup: pkg/data's globals are shared package-level + // state for the whole test binary, and leaving them reset would panic + // ("data.InitWriter() must be called before using data package + // functions") in any test that runs after this one and doesn't set up + // its own I/O context first -- exactly what -shuffle=on can surface. + require.NoError(t, iolib.Initialize()) + ioCtx = iolib.GetContext() + ui.InitFormatter(ioCtx) + data.InitWriter(ioCtx) } return stdout, stderr, cleanup diff --git a/pkg/scanners/sarif/normalize_test.go b/pkg/scanners/sarif/normalize_test.go index 29873d4159b..1b25bb46b27 100644 --- a/pkg/scanners/sarif/normalize_test.go +++ b/pkg/scanners/sarif/normalize_test.go @@ -5,7 +5,6 @@ import ( "path/filepath" "testing" - "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -71,9 +70,17 @@ func TestNormalizeArtifactURIsRewritesNestedSARIFLocations(t *testing.T) { require.NoError(t, os.MkdirAll(sourceRoot, 0o755)) require.NoError(t, os.WriteFile(sourceFile, []byte("# target\n"), 0o600)) - previousWorkspace := viper.GetString(githubWorkspaceViperKey) - viper.Set(githubWorkspaceViperKey, workspace) - t.Cleanup(func() { viper.Set(githubWorkspaceViperKey, previousWorkspace) }) + // t.Setenv (not viper.Set): githubWorkspace() resolves this key by binding + // it to the GITHUB_WORKSPACE env var and reading it live on every call, so + // t.Setenv's automatic restore is both correct and sufficient. viper.Set + // installs a literal override that outranks the env binding in viper's + // precedence and is never cleared by t.Setenv -- a prior version of this + // test used exactly that pattern and permanently pinned this key for the + // rest of the process (to whatever value GITHUB_WORKSPACE happened to + // have when this test first ran), silently breaking any later test in + // the same binary that expects t.Setenv("GITHUB_WORKSPACE", ...) to + // control this key -- see docs/fixes for the incident. + t.Setenv("GITHUB_WORKSPACE", workspace) ctx := &scanners.Context{ AtmosConfig: &schema.AtmosConfiguration{ diff --git a/pkg/terraform/cache/trust_install.go b/pkg/terraform/cache/trust_install.go index 3a07acfc8e3..221adcc2066 100644 --- a/pkg/terraform/cache/trust_install.go +++ b/pkg/terraform/cache/trust_install.go @@ -132,14 +132,24 @@ func removeWindowsTrustStore(certPath string) error { } func nativeWindowsTrustInstall(certPath string) error { + // Snapshot the package-level function var before runTrustOperation spawns + // its background goroutine, rather than letting the closure read it live: + // on a timeout, runTrustOperation returns to the caller while that + // goroutine keeps running (there's no context to cancel a plain Go func + // with), and a caller (in practice, a test's t.Cleanup) that reassigns + // installWindowsTrustFunc afterward would race with this goroutine's read + // of it. The goroutine now only ever touches its own private copy. + install := installWindowsTrustFunc return runTrustOperation("Windows trust store install", func() error { - return installWindowsTrustFunc(certPath) + return install(certPath) }) } func nativeWindowsTrustRemove(certPath string) error { + // See nativeWindowsTrustInstall: same snapshot-before-async-use reasoning. + remove := removeWindowsTrustFunc return runTrustOperation("Windows trust store removal", func() error { - return removeWindowsTrustFunc(certPath) + return remove(certPath) }) } diff --git a/pkg/terraform/registry/provider_mirror_test.go b/pkg/terraform/registry/provider_mirror_test.go index 2fa61580a18..3c6da15a4ac 100644 --- a/pkg/terraform/registry/provider_mirror_test.go +++ b/pkg/terraform/registry/provider_mirror_test.go @@ -24,11 +24,9 @@ import ( // fakeRegistry serves the provider registry protocol for one provider, rewriting // service-discovery and download URLs to point at itself. type fakeRegistry struct { - server *httptest.Server - zip []byte - zipSum string - dlHits int - verHits int + server *httptest.Server + zip []byte + zipSum string } func newFakeRegistry(t *testing.T) *fakeRegistry { @@ -42,11 +40,9 @@ func newFakeRegistry(t *testing.T) *fakeRegistry { _, _ = w.Write([]byte(`{"providers.v1":"/v1/providers/","modules.v1":"/v1/modules/"}`)) }) mux.HandleFunc("/v1/providers/hashicorp/aws/versions", func(w http.ResponseWriter, r *http.Request) { - fr.verHits++ _, _ = w.Write([]byte(`{"versions":[{"version":"5.95.0","platforms":[{"os":"linux","arch":"amd64"},{"os":"darwin","arch":"arm64"}]}]}`)) }) mux.HandleFunc("/v1/providers/hashicorp/aws/5.95.0/download/", func(w http.ResponseWriter, r *http.Request) { - fr.dlHits++ // .../download//. seg := strings.Split(strings.TrimPrefix(r.URL.Path, "/v1/providers/hashicorp/aws/5.95.0/download/"), "/") osName, arch := seg[0], seg[1] diff --git a/pkg/toolchain/github_token_test.go b/pkg/toolchain/github_token_test.go index 1764f99f04c..083194b7245 100644 --- a/pkg/toolchain/github_token_test.go +++ b/pkg/toolchain/github_token_test.go @@ -60,6 +60,14 @@ func TestGitHubTokenEnvBinding(t *testing.T) { t.Run("TestMain binds environment correctly", func(t *testing.T) { // This test verifies that the binding in TestMain is working. + // Re-bind defensively: other tests in this package (e.g. set_test.go's + // setupTest/teardownTest) call viper.Reset(), which discards the + // global instance TestMain bound "github-token" into. -shuffle=on + // randomizes test order, so this subtest can no longer assume + // TestMain's one-time binding survived every sibling test that ran + // before it; BindEnv is safe to call again (idempotent). + viper.BindEnv("github-token", "ATMOS_GITHUB_TOKEN", "GITHUB_TOKEN") + // If GITHUB_TOKEN is set in environment, it should be accessible. if envToken := os.Getenv("GITHUB_TOKEN"); envToken != "" { // The global viper instance should have the binding from TestMain. diff --git a/pkg/toolchain/set_test.go b/pkg/toolchain/set_test.go index cc6bc1fde04..993ab31fa9d 100644 --- a/pkg/toolchain/set_test.go +++ b/pkg/toolchain/set_test.go @@ -70,6 +70,13 @@ func setupTest() { func teardownTest() { viper.Reset() + // Restore TestMain's "github-token" env binding: Reset() discards it, and + // leaving it unbound would silently break every test that runs after this + // one in the same process (-shuffle=on means this is not always the last + // test) and expects GITHUB_TOKEN/ATMOS_GITHUB_TOKEN to reach viper -- + // e.g. this package's registry tests falling back to unauthenticated + // GitHub API calls. BindEnv is safe to call again (idempotent). + viper.BindEnv("github-token", "ATMOS_GITHUB_TOKEN", "GITHUB_TOKEN") } // Tests for versionListModel. diff --git a/pkg/ui/theme/styles.go b/pkg/ui/theme/styles.go index a1c017e9682..346557fa8bd 100644 --- a/pkg/ui/theme/styles.go +++ b/pkg/ui/theme/styles.go @@ -4,7 +4,8 @@ import ( "strings" "github.com/charmbracelet/lipgloss" - "github.com/spf13/viper" + + "github.com/cloudposse/atmos/pkg/viperguard" ) // DefaultThemeName is the default theme used when no theme is configured. @@ -425,25 +426,29 @@ func InvalidateStyleCache() { // getActiveThemeName determines the active theme name from configuration or environment. func getActiveThemeName() string { - // Bind environment variables on demand to ensure they're available - // This handles both ATMOS_THEME and THEME as fallbacks - _ = viper.BindEnv("settings.terminal.theme", "ATMOS_THEME", "THEME") - - // Check Viper configuration which now includes bound environment variables - if viper.IsSet("settings.terminal.theme") { - theme := viper.GetString("settings.terminal.theme") + // Bind environment variables on demand to ensure they're available. + // This handles both ATMOS_THEME and THEME as fallbacks. Routed through + // pkg/viperguard (not viper.BindEnv directly): this runs on every styled + // render, including from the toolchain's concurrent batch installer, and + // spf13/viper has no locking of its own -- a bare viper.BindEnv here can + // data-race against any other goroutine's concurrent global-viper access. + _ = viperguard.BindEnv("settings.terminal.theme", "ATMOS_THEME", "THEME") + + // Check Viper configuration which now includes bound environment variables. + if viperguard.IsSet("settings.terminal.theme") { + theme := viperguard.GetString("settings.terminal.theme") if theme != "" { return theme } } - // Check for ATMOS_THEME environment variable directly as fallback - if theme := viper.GetString("ATMOS_THEME"); theme != "" { + // Check for ATMOS_THEME environment variable directly as fallback. + if theme := viperguard.GetString("ATMOS_THEME"); theme != "" { return theme } - // Check for THEME environment variable directly as second fallback - if theme := viper.GetString("THEME"); theme != "" { + // Check for THEME environment variable directly as second fallback. + if theme := viperguard.GetString("THEME"); theme != "" { return theme } diff --git a/pkg/ui/theme/styles_test.go b/pkg/ui/theme/styles_test.go index fe05901570d..734ebbd49b2 100644 --- a/pkg/ui/theme/styles_test.go +++ b/pkg/ui/theme/styles_test.go @@ -48,6 +48,16 @@ func TestGetStyles_NilScheme(t *testing.T) { } func TestInitializeStyles(t *testing.T) { + // InitializeStyles caches this partial scheme (Border and others left at + // their zero value) into the package-level lastColorScheme/CurrentStyles + // globals, shared by every test in this package. Without this cleanup, + // -shuffle=on can run TestGetBorderColor (or any other color getter) + // after this test and have it observe an empty string from the stale + // cache instead of falling back to a real theme -- see docs/fixes for + // the incident. TestComponentLabelStyleCyclesPalette (log_styles_test.go) + // already follows this pattern for the same reason. + t.Cleanup(InvalidateStyleCache) + scheme := &ColorScheme{ Primary: "#0000FF", Success: "#00FF00", diff --git a/pkg/utils/string_utils_test.go b/pkg/utils/string_utils_test.go index 390448eec04..52e5c182ed4 100644 --- a/pkg/utils/string_utils_test.go +++ b/pkg/utils/string_utils_test.go @@ -359,6 +359,13 @@ func TestInternStringsInMap_CommonAtmosKeys(t *testing.T) { // TestClearInternPool tests that clearing the pool works correctly. func TestClearInternPool(t *testing.T) { + // The intern pool and its stats are package-level state shared by every + // test in this package, so start from a known-empty pool rather than + // assuming this test runs before any other test interns a string + // (-shuffle=on runs tests in random order, so that assumption doesn't + // hold in CI's race job). + ClearInternPool() + atmosConfig := &schema.AtmosConfiguration{} // Intern some strings. diff --git a/pkg/viperguard/viperguard.go b/pkg/viperguard/viperguard.go new file mode 100644 index 00000000000..58f41ad2b5d --- /dev/null +++ b/pkg/viperguard/viperguard.go @@ -0,0 +1,167 @@ +// Package viperguard mutex-guards the process-wide global spf13/viper singleton. +// +// spf13/viper has no internal locking of its own: viper.Get*/Set/BindEnv all +// read or mutate one shared underlying map via deepSearch, and Go maps are not +// safe for any concurrent read/write access, regardless of which key each +// goroutine touches -- a BindEnv registering an unrelated key can still race +// with a concurrent Get of a completely different key. Any code that may run +// concurrently with other global-viper access (the DAG scheduler's per-node +// LoadConfig calls, the toolchain's concurrent batch installer, ...) must +// route through this package's functions instead of calling viper.* directly. +// +// This lives in its own leaf package (no imports of any other Atmos package) +// specifically so packages low in the dependency graph -- pkg/http and +// pkg/ui/theme, which pkg/config itself depends on -- can use it without an +// import cycle. pkg/config.GlobalViper() delegates here rather than keeping a +// second, independent mutex: two separate locks guarding the same underlying +// viper singleton would not actually exclude each other, leaving exactly the +// kind of cross-package race this package exists to close. +package viperguard + +import ( + "slices" + "sync" + + "github.com/spf13/viper" + + "github.com/cloudposse/atmos/pkg/perf" +) + +var mu sync.RWMutex + +// Set sets key's value on the global Viper singleton. +func Set(key string, value any) { + defer perf.Track(nil, "viperguard.Set")() + + mu.Lock() + defer mu.Unlock() + viper.GetViper().Set(key, value) +} + +// BindEnv binds a Viper key to one or more environment variable names on the +// global Viper singleton. See viper.BindEnv for the input argument shape. +func BindEnv(input ...string) error { + defer perf.Track(nil, "viperguard.BindEnv")() + + mu.Lock() + defer mu.Unlock() + return viper.BindEnv(input...) +} + +// GetString returns key's value coerced to string. Returns "" if unset. +func GetString(key string) string { + defer perf.Track(nil, "viperguard.GetString")() + + mu.RLock() + defer mu.RUnlock() + return viper.GetViper().GetString(key) +} + +// GetBool returns key's value coerced to bool. Returns false if unset. +func GetBool(key string) bool { + defer perf.Track(nil, "viperguard.GetBool")() + + mu.RLock() + defer mu.RUnlock() + return viper.GetViper().GetBool(key) +} + +// GetStringSlice returns a clone of key's value coerced to []string: viper's +// own GetStringSlice can return its value's existing backing array rather +// than a copy, and handing that out under the lock would let a caller mutate +// shared Viper state after the lock is released. Returns nil if unset. +func GetStringSlice(key string) []string { + defer perf.Track(nil, "viperguard.GetStringSlice")() + + mu.RLock() + defer mu.RUnlock() + return slices.Clone(viper.GetViper().GetStringSlice(key)) +} + +// IsSet reports whether key has a value from any source, including a +// registered default: viper.IsSet's underlying find() also checks +// viper.defaults, so a key registered only via SetDefault also reports true +// here. It cannot distinguish an explicit value (flag, env, config, override) +// from a default; callers needing that distinction need a separate check. +func IsSet(key string) bool { + defer perf.Track(nil, "viperguard.IsSet")() + + mu.RLock() + defer mu.RUnlock() + return viper.GetViper().IsSet(key) +} + +// ViperReader exposes only *viper.Viper's read methods. View passes this (not +// *viper.Viper) to its callback, so the callback cannot call a mutator like +// Set while holding only the read lock -- doing so would race against +// another concurrent View call's reads, or against Set's write lock, +// defeating the whole point of View. Extend with more read methods as +// callers need them; never add a mutator here. +type ViperReader interface { + // IsSet reports whether key has a value from any source, including a + // registered default (see the package-level IsSet's doc comment for why). + IsSet(key string) bool + // GetBool returns key's value coerced to bool. Returns false if unset. + GetBool(key string) bool + // GetString returns key's value coerced to string. Returns "" if unset. + GetString(key string) string + // GetStringSlice returns key's value coerced to []string, cloned so the + // caller cannot mutate Viper's own backing array. Returns nil if unset. + GetStringSlice(key string) []string +} + +// viperReaderAdapter wraps *viper.Viper to satisfy ViperReader without +// exposing the concrete *viper.Viper type to View callbacks. Passing +// *viper.Viper itself through the ViperReader interface would only hide Set +// behind a narrower static type -- Go interfaces retain their dynamic type, +// so a callback could still type-assert the value back to *viper.Viper and +// call Set while holding only View's read lock. Because viperReaderAdapter is +// unexported, code outside this package cannot name it to assert against it, +// so it cannot recover the underlying *viper.Viper this way. +type viperReaderAdapter struct { + v *viper.Viper +} + +func (a viperReaderAdapter) IsSet(key string) bool { + defer perf.Track(nil, "viperguard.viperReaderAdapter.IsSet")() + + return a.v.IsSet(key) +} + +func (a viperReaderAdapter) GetBool(key string) bool { + defer perf.Track(nil, "viperguard.viperReaderAdapter.GetBool")() + + return a.v.GetBool(key) +} + +func (a viperReaderAdapter) GetString(key string) string { + defer perf.Track(nil, "viperguard.viperReaderAdapter.GetString")() + + return a.v.GetString(key) +} + +func (a viperReaderAdapter) GetStringSlice(key string) []string { + defer perf.Track(nil, "viperguard.viperReaderAdapter.GetStringSlice")() + + return slices.Clone(a.v.GetStringSlice(key)) +} + +// View executes fn with a read lock held on the global Viper singleton, +// giving fn a consistent snapshot for the whole call. Use this instead of +// separate Get*/IsSet calls whenever a decision combines more than one read +// (e.g. an IsSet presence check followed by a GetBool value read): each +// individual function in this package locks and unlocks independently, so a +// concurrent Set() between two separate calls could let the decision combine +// one snapshot's presence result with a different snapshot's value. +// +// The callback fn must not call Set, BindEnv, or any other guard writer, +// directly or transitively: mu.RLock is held for the whole call, and those +// writers block on mu.Lock until fn returns, so fn calling one deadlocks +// against itself. +func View(fn func(v ViperReader)) { + defer perf.Track(nil, "viperguard.View")() + + mu.RLock() + defer mu.RUnlock() + fn(viperReaderAdapter{v: viper.GetViper()}) +} diff --git a/pkg/viperguard/viperguard_test.go b/pkg/viperguard/viperguard_test.go new file mode 100644 index 00000000000..93407bb79df --- /dev/null +++ b/pkg/viperguard/viperguard_test.go @@ -0,0 +1,101 @@ +package viperguard_test + +import ( + "sync" + "testing" + + "github.com/spf13/viper" + "github.com/stretchr/testify/assert" + + "github.com/cloudposse/atmos/pkg/viperguard" +) + +// TestConcurrentBindEnvAndGet reproduces the exact shape of a data race caught +// by CI's race-detector job: pkg/ui/theme.getActiveThemeName calling +// viper.BindEnv concurrently with pkg/http.GetGitHubTokenFromEnv calling +// viper.GetString, both against the global viper singleton, from two +// goroutines spawned by the toolchain's concurrent batch installer. Run under +// `go test -race`, this fails if BindEnv/GetString/IsSet/Set/GetStringSlice +// ever go back to calling viper.* directly instead of through this package. +func TestConcurrentBindEnvAndGet(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + t.Setenv("ATMOS_THEME", "dark") + + const iterations = 200 + var wg sync.WaitGroup + wg.Add(4) + + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + _ = viperguard.BindEnv("settings.terminal.theme", "ATMOS_THEME", "THEME") + } + }() + + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + _ = viperguard.GetString("github-token") + } + }() + + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + viperguard.Set("some.unrelated.key", i) + } + }() + + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + _ = viperguard.IsSet("settings.terminal.theme") + _ = viperguard.GetStringSlice("some.slice") + } + }() + + wg.Wait() + + assert.True(t, viperguard.IsSet("settings.terminal.theme"), + "BindEnv must still have taken effect once every goroutine finished") +} + +// TestGetBoolAndView covers the package's GetBool and View, plus every +// viperReaderAdapter method View hands to its callback (IsSet, GetBool, +// GetString, GetStringSlice) -- none of which TestConcurrentBindEnvAndGet +// exercises, since that test only covers the concurrency contract. +func TestGetBoolAndView(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + + viperguard.Set("some.bool.key", true) + viperguard.Set("some.string.key", "hello") + viperguard.Set("some.slice.key", []string{"a", "b"}) + + assert.True(t, viperguard.GetBool("some.bool.key")) + assert.False(t, viperguard.GetBool("unset.bool.key")) + + var viewedBool bool + var viewedString string + var viewedSlice []string + var viewedSet bool + viperguard.View(func(v viperguard.ViperReader) { + viewedBool = v.GetBool("some.bool.key") + viewedString = v.GetString("some.string.key") + viewedSlice = v.GetStringSlice("some.slice.key") + viewedSet = v.IsSet("some.slice.key") + }) + + assert.True(t, viewedBool) + assert.Equal(t, "hello", viewedString) + assert.Equal(t, []string{"a", "b"}, viewedSlice) + assert.True(t, viewedSet) + assert.False(t, viperguard.IsSet("unset.slice.key")) + + // GetStringSlice, both the package-level function and View's adapter, + // must clone Viper's backing array rather than hand it out: mutating the + // returned slice must never corrupt what a later read observes. + viewedSlice[0] = "mutated" + assert.Equal(t, []string{"a", "b"}, viperguard.GetStringSlice("some.slice.key")) +} diff --git a/website/package.json b/website/package.json index 8355bd02ce7..276700e040b 100644 --- a/website/package.json +++ b/website/package.json @@ -106,6 +106,7 @@ "ajv@^6": "^6.14.0", "brace-expansion@^1": "1.1.18", "brace-expansion@^2": "2.1.4", + "browserslist@^4": "^4.28.7", "dompurify@^3": "^3.4.13", "fast-uri@^3": "^3.1.5", "follow-redirects@^1": "^1.16.0", @@ -125,6 +126,7 @@ "path-to-regexp@^0.1": "^0.1.13", "picomatch@^2": "^2.3.2", "postcss@^8": "^8.5.18", + "postcss-selector-parser@^6": "^6.1.3", "postcss-selector-parser@^7": "^7.1.3", "qs@^6": "^6.15.2", "serialize-javascript@^6": "^7.0.5", diff --git a/website/pnpm-lock.yaml b/website/pnpm-lock.yaml index cb15fc62a59..e75a47d706e 100644 --- a/website/pnpm-lock.yaml +++ b/website/pnpm-lock.yaml @@ -10,6 +10,7 @@ overrides: ajv@^6: ^6.14.0 brace-expansion@^1: 1.1.18 brace-expansion@^2: 2.1.4 + browserslist@^4: ^4.28.7 dompurify@^3: ^3.4.13 fast-uri@^3: ^3.1.5 follow-redirects@^1: ^1.16.0 @@ -29,6 +30,7 @@ overrides: path-to-regexp@^0.1: ^0.1.13 picomatch@^2: ^2.3.2 postcss@^8: ^8.5.18 + postcss-selector-parser@^6: ^6.1.3 postcss-selector-parser@^7: ^7.1.3 qs@^6: ^6.15.2 serialize-javascript@^6: ^7.0.5 @@ -915,10 +917,6 @@ packages: peerDependencies: '@babel/core': ^7.29.6 - '@babel/runtime@7.28.4': - resolution: {integrity: sha512-Q/N6JNWvIvPnLDvjlE1OUBLPQHH6l3CltCEsHIujp45zQUSSh8K+gHnaEX45yAT1nyngnINhvWtzN+Nb9D8RAQ==} - engines: {node: '>=6.9.0'} - '@babel/runtime@7.29.7': resolution: {integrity: sha512-Nq8OhGWiZIZGV6hLHoyAKLLcJihP/xFeBMGJoUrxTX2psI8dCifzLhZISFb+VWS3wFMRDmCGw5R+dOySCqPLhw==} engines: {node: '>=6.9.0'} @@ -2649,8 +2647,8 @@ packages: balanced-match@1.0.2: resolution: {integrity: sha512-3oSeUO0TMV67hN1AmbXsK4yaqU7tjiHlbxRDZOpH0KW9+CeX4bRAaX0Anxt0tx2MrpRpWwQaPwIlISEJhYU5Pw==} - baseline-browser-mapping@2.10.43: - resolution: {integrity: sha512-AjYpR78kDWAY3Efj+cDTFH9t9SCoL7OoTp1BOb0mQV7S+6CiLwnWM3FyxhJtdPufDFKzmCSFoUncKjWgJEZTCQ==} + baseline-browser-mapping@2.11.19: + resolution: {integrity: sha512-Grytf1xOxOEMTGRwx6rLGKkTabd4vMg3VrKdj/7joCmV0qgh4QwMMO6xh34YEXQqirAuUdgQGa5orJQQ+69RBw==} engines: {node: '>=6.0.0'} hasBin: true @@ -2695,13 +2693,8 @@ packages: browser-fs-access@0.29.1: resolution: {integrity: sha512-LSvVX5e21LRrXqVMhqtAwj5xPgDb+fXAIH80NsnCQ9xuZPs2xWsOREi24RKgZa1XOiQRbcmVrv87+ulOKsgjxw==} - browserslist@4.27.0: - resolution: {integrity: sha512-AXVQwdhot1eqLihwasPElhX2tAZiBjWdJ9i/Zcj2S6QYIjkx62OKSfnobkriB81C3l4w0rVy3Nt4jaTBltYEpw==} - engines: {node: ^6 || ^7 || ^8 || ^9 || ^10 || ^11 || ^12 || >=13.7} - hasBin: true - - browserslist@4.28.5: - resolution: {integrity: sha512-Cu2E6QejHWzuDMTkuwgpABFgDfZrXLQq5V13YOACZx4mFAG4IwGTbTfHPMr4WtxlHoXSM8FIuRwYYCz5XiabaQ==} + browserslist@4.28.8: + resolution: {integrity: sha512-V2NpofLblG64mfOtSgDhOJESZEGogzDMBv/q+W6oc4LXWP/q75eOXoOaaOu1EOadB9U4Bwx/e0yzbvwKH8zalA==} engines: {node: ^6 || ^7 || ^8 || ^9 || ^10 || ^11 || ^12 || >=13.7} hasBin: true @@ -2765,6 +2758,9 @@ packages: caniuse-lite@1.0.30001805: resolution: {integrity: sha512-52noaS3DubycKSXaU30TwPGIp+POyQSUVa5jBEq3vkRkY0kjyb3LQgvhU6WGyCcyXqVLWO0Cw0Q6BSdD0kUfVA==} + caniuse-lite@1.0.30001810: + resolution: {integrity: sha512-TITQPUkaz+aVk5GL6NhOdwk1aEaNTSDPsGFWrTuhKGtjTF70jL/Oht2W4c6rXUe5fu7Ie19VIahAXHIIiWWNeg==} + canvas-confetti@1.9.4: resolution: {integrity: sha512-yxQbJkAVrFXWNbTUjPqjF7G+g6pDotOUHGbkZq2NELZUMDpiJ85rIEazVb8GTaAptNW2miJAXbs1BtioA251Pw==} @@ -3472,11 +3468,8 @@ packages: ee-first@1.1.1: resolution: {integrity: sha512-WMwm9LhRUo+WUaRN+vRuETqG89IgZphVSNkdFgeb6sS/E4OrDIN7t48CAewSHXc6C8lefD8KKfr5vY61brQlow==} - electron-to-chromium@1.5.239: - resolution: {integrity: sha512-1y5w0Zsq39MSPmEjHjbizvhYoTaulVtivpxkp5q5kaPmQtsK6/2nvAzGRxNMS9DoYySp9PkW0MAQDwU1m764mg==} - - electron-to-chromium@1.5.389: - resolution: {integrity: sha512-cEto7aeOqBfU1D+c5py5pE+ooscKE75JifxLBdFUZsqAxRS6y7kebtxAZvICszSl05gPjYHDTjY+lXpyGvpJbg==} + electron-to-chromium@1.5.415: + resolution: {integrity: sha512-958V+Kbhtgz+SxXeEVKBjrlKRBIDAYvUJfwhjxMZ5S6ut9jAl7l9ZKBkBrvjyjZE36PabLUo2L8kEeV5O4vgJg==} emoji-regex@8.0.0: resolution: {integrity: sha512-MSjYzcWNOA0ewAHpz0MxpYFvwg6yjy1NG3xteoqz644VCo/RPgnr1/GGt+ic3iJTzQ8Eu3TdM14SawnVUmGE6A==} @@ -4825,11 +4818,8 @@ packages: resolution: {integrity: sha512-dRB78srN/l6gqWulah9SrxeYnxeddIG30+GOqK/9OlLVyLg3HPnr6SqOWTWOXKRwC2eGYCkZ59NNuSgvSrpgOA==} engines: {node: ^12.20.0 || ^14.13.1 || >=16.0.0} - node-releases@2.0.26: - resolution: {integrity: sha512-S2M9YimhSjBSvYnlr5/+umAnPHE++ODwt5e2Ij6FoX45HA/s4vHdkDx1eax2pAPeAOqu4s9b7ppahsyEFdVqQA==} - - node-releases@2.0.51: - resolution: {integrity: sha512-wRNIrw4DmVLKQlbgOMdkMx27Wrpzes2hh5Jtbi2bjPd+4wJstWIqP5A+lscnqbm0xxmT5Bpg8Lec5ItEBwx6BQ==} + node-releases@2.0.53: + resolution: {integrity: sha512-D9UOmYG3UH1V+ENW56t5QXBwJw1YEY18ruVeus89Rw+SyIgjPkCO84bRzO3uNIYosJbNwiabWVn48o3uJLjxFQ==} engines: {node: '>=18'} normalize-path@3.0.0: @@ -5416,8 +5406,8 @@ packages: peerDependencies: postcss: ^8.5.18 - postcss-selector-parser@6.1.2: - resolution: {integrity: sha512-Q8qQfPiZ+THO/3ZrOrO0cJJKfpYCagtMUkXbnEfmgUjwXg6z/WBeOyS9APBBPCTSiDV+s4SwQGu8yFsiMRIudg==} + postcss-selector-parser@6.1.4: + resolution: {integrity: sha512-bIoJLOmjCO1S9XdY/DcnR5hJxvrDir1PbGChrzXG3vw0/FOliy/fA3dmdhQ441kah4gKv+TwckGzex6wNS5cnQ==} engines: {node: '>=4'} postcss-selector-parser@7.1.5: @@ -6358,17 +6348,11 @@ packages: resolution: {integrity: sha512-pjy2bYhSsufwWlKwPc+l3cN7+wuJlK6uz0YdJEOlQDbl6jo/YlPi4mb8agUkVC8BF7V8NuzeyPNqRksA3hztKQ==} engines: {node: '>= 0.8'} - update-browserslist-db@1.1.4: - resolution: {integrity: sha512-q0SPT4xyU84saUX+tomz1WLkxUbuaJnR1xWt17M7fJtEJigJeWUNGUqrauFXsHnqev9y9JTRGwk13tFBuKby4A==} + update-browserslist-db@1.3.1: + resolution: {integrity: sha512-ZZ61DsRsOnakl74HAmp3oSN4aXUmEWXf+i/yv0h7tIBfICc3VdrFErQKUUKPgu3AMsTUMbcongALEN4l6GSUrQ==} hasBin: true peerDependencies: - browserslist: '>= 4.21.0' - - update-browserslist-db@1.2.3: - resolution: {integrity: sha512-Js0m9cx+qOgDxo0eMiFGEueWztz+d4+M3rGlmKPT+T4IS/jP4ylw3Nwpu6cpTTP8R1MAC1kF4VbdLt3ARf209w==} - hasBin: true - peerDependencies: - browserslist: '>= 4.21.0' + browserslist: ^4.28.7 update-notifier@6.0.2: resolution: {integrity: sha512-EDxhTEVPZZRLWYcJ4ZXjGFN0oP7qYvbXWzEgRm/Yql4dHX5wDbvh89YHP6PK1lzZJYrMtXUuZZz8XGK+U6U1og==} @@ -6853,7 +6837,7 @@ snapshots: dependencies: '@babel/compat-data': 7.28.5 '@babel/helper-validator-option': 7.27.1 - browserslist: 4.27.0 + browserslist: 4.28.8 lru-cache: 5.1.1 semver: 6.3.1 @@ -6861,7 +6845,7 @@ snapshots: dependencies: '@babel/compat-data': 7.29.7 '@babel/helper-validator-option': 7.29.7 - browserslist: 4.28.5 + browserslist: 4.28.8 lru-cache: 5.1.1 semver: 6.3.1 @@ -7571,8 +7555,6 @@ snapshots: transitivePeerDependencies: - supports-color - '@babel/runtime@7.28.4': {} - '@babel/runtime@7.29.7': {} '@babel/template@7.27.2': @@ -9310,7 +9292,7 @@ snapshots: '@radix-ui/primitive@1.0.0': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/primitive@1.1.1': {} @@ -9324,7 +9306,7 @@ snapshots: '@radix-ui/react-collection@1.0.1(react-dom@18.3.1(react@18.3.1))(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/react-compose-refs': 1.0.0(react@18.3.1) '@radix-ui/react-context': 1.0.0(react@18.3.1) '@radix-ui/react-primitive': 1.0.1(react-dom@18.3.1(react@18.3.1))(react@18.3.1) @@ -9334,7 +9316,7 @@ snapshots: '@radix-ui/react-compose-refs@1.0.0(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 react: 18.3.1 '@radix-ui/react-compose-refs@1.1.1(@types/react@19.2.2)(react@18.3.1)': @@ -9345,7 +9327,7 @@ snapshots: '@radix-ui/react-context@1.0.0(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 react: 18.3.1 '@radix-ui/react-context@1.1.1(@types/react@19.2.2)(react@18.3.1)': @@ -9356,7 +9338,7 @@ snapshots: '@radix-ui/react-direction@1.0.0(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 react: 18.3.1 '@radix-ui/react-dismissable-layer@1.1.5(@types/react@19.2.2)(react-dom@18.3.1(react@18.3.1))(react@18.3.1)': @@ -9389,7 +9371,7 @@ snapshots: '@radix-ui/react-id@1.0.0(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/react-use-layout-effect': 1.0.0(react@18.3.1) react: 18.3.1 @@ -9450,7 +9432,7 @@ snapshots: '@radix-ui/react-presence@1.0.0(react-dom@18.3.1(react@18.3.1))(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/react-compose-refs': 1.0.0(react@18.3.1) '@radix-ui/react-use-layout-effect': 1.0.0(react@18.3.1) react: 18.3.1 @@ -9467,7 +9449,7 @@ snapshots: '@radix-ui/react-primitive@1.0.1(react-dom@18.3.1(react@18.3.1))(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/react-slot': 1.0.1(react@18.3.1) react: 18.3.1 react-dom: 18.3.1(react@18.3.1) @@ -9482,7 +9464,7 @@ snapshots: '@radix-ui/react-roving-focus@1.0.2(react-dom@18.3.1(react@18.3.1))(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/primitive': 1.0.0 '@radix-ui/react-collection': 1.0.1(react-dom@18.3.1(react@18.3.1))(react@18.3.1) '@radix-ui/react-compose-refs': 1.0.0(react@18.3.1) @@ -9497,7 +9479,7 @@ snapshots: '@radix-ui/react-slot@1.0.1(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/react-compose-refs': 1.0.0(react@18.3.1) react: 18.3.1 @@ -9510,7 +9492,7 @@ snapshots: '@radix-ui/react-tabs@1.0.2(react-dom@18.3.1(react@18.3.1))(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/primitive': 1.0.0 '@radix-ui/react-context': 1.0.0(react@18.3.1) '@radix-ui/react-direction': 1.0.0(react@18.3.1) @@ -9524,7 +9506,7 @@ snapshots: '@radix-ui/react-use-callback-ref@1.0.0(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 react: 18.3.1 '@radix-ui/react-use-callback-ref@1.1.0(@types/react@19.2.2)(react@18.3.1)': @@ -9535,7 +9517,7 @@ snapshots: '@radix-ui/react-use-controllable-state@1.0.0(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@radix-ui/react-use-callback-ref': 1.0.0(react@18.3.1) react: 18.3.1 @@ -9555,7 +9537,7 @@ snapshots: '@radix-ui/react-use-layout-effect@1.0.0(react@18.3.1)': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 react: 18.3.1 '@radix-ui/react-use-layout-effect@1.1.0(@types/react@19.2.2)(react@18.3.1)': @@ -10251,7 +10233,7 @@ snapshots: autoprefixer@10.4.21(postcss@8.5.23): dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 caniuse-lite: 1.0.30001805 fraction.js: 4.3.7 normalize-range: 0.1.2 @@ -10312,7 +10294,7 @@ snapshots: balanced-match@1.0.2: {} - baseline-browser-mapping@2.10.43: {} + baseline-browser-mapping@2.11.19: {} batch@0.6.1: {} @@ -10381,21 +10363,13 @@ snapshots: browser-fs-access@0.29.1: {} - browserslist@4.27.0: - dependencies: - baseline-browser-mapping: 2.10.43 - caniuse-lite: 1.0.30001805 - electron-to-chromium: 1.5.239 - node-releases: 2.0.26 - update-browserslist-db: 1.1.4(browserslist@4.27.0) - - browserslist@4.28.5: + browserslist@4.28.8: dependencies: - baseline-browser-mapping: 2.10.43 - caniuse-lite: 1.0.30001805 - electron-to-chromium: 1.5.389 - node-releases: 2.0.51 - update-browserslist-db: 1.2.3(browserslist@4.28.5) + baseline-browser-mapping: 2.11.19 + caniuse-lite: 1.0.30001810 + electron-to-chromium: 1.5.415 + node-releases: 2.0.53 + update-browserslist-db: 1.3.1(browserslist@4.28.8) buffer-from@1.1.2: {} @@ -10451,13 +10425,15 @@ snapshots: caniuse-api@3.0.0: dependencies: - browserslist: 4.28.5 + browserslist: 4.28.8 caniuse-lite: 1.0.30001805 lodash.memoize: 4.1.2 lodash.uniq: 4.5.0 caniuse-lite@1.0.30001805: {} + caniuse-lite@1.0.30001810: {} + canvas-confetti@1.9.4: {} canvas-roundrect-polyfill@0.0.1: {} @@ -10653,7 +10629,7 @@ snapshots: core-js-compat@3.46.0: dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 core-js@2.6.12: {} @@ -10774,7 +10750,7 @@ snapshots: cssnano-preset-advanced@6.1.2(postcss@8.5.23): dependencies: autoprefixer: 10.4.21(postcss@8.5.23) - browserslist: 4.27.0 + browserslist: 4.28.8 cssnano-preset-default: 6.1.2(postcss@8.5.23) postcss: 8.5.23 postcss-discard-unused: 6.0.5(postcss@8.5.23) @@ -10784,7 +10760,7 @@ snapshots: cssnano-preset-default@6.1.2(postcss@8.5.23): dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 css-declaration-sorter: 7.3.0(postcss@8.5.23) cssnano-utils: 4.0.2(postcss@8.5.23) postcss: 8.5.23 @@ -11183,9 +11159,7 @@ snapshots: ee-first@1.1.1: {} - electron-to-chromium@1.5.239: {} - - electron-to-chromium@1.5.389: {} + electron-to-chromium@1.5.415: {} emoji-regex@8.0.0: {} @@ -12839,9 +12813,7 @@ snapshots: fetch-blob: 3.2.0 formdata-polyfill: 4.0.10 - node-releases@2.0.26: {} - - node-releases@2.0.51: {} + node-releases@2.0.53: {} normalize-path@3.0.0: {} @@ -13090,7 +13062,7 @@ snapshots: postcss-calc@9.0.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 6.1.2 + postcss-selector-parser: 6.1.4 postcss-value-parser: 4.2.0 postcss-clamp@4.1.0(postcss@8.5.23): @@ -13121,7 +13093,7 @@ snapshots: postcss-colormin@6.1.0(postcss@8.5.23): dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 caniuse-api: 3.0.0 colord: 2.9.3 postcss: 8.5.23 @@ -13129,7 +13101,7 @@ snapshots: postcss-convert-values@6.1.0(postcss@8.5.23): dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 postcss: 8.5.23 postcss-value-parser: 4.2.0 @@ -13182,7 +13154,7 @@ snapshots: postcss-discard-unused@6.0.5(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 6.1.2 + postcss-selector-parser: 6.1.4 postcss-double-position-gradients@6.0.4(postcss@8.5.23): dependencies: @@ -13253,11 +13225,11 @@ snapshots: postcss-merge-rules@6.1.1(postcss@8.5.23): dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 caniuse-api: 3.0.0 cssnano-utils: 4.0.2(postcss@8.5.23) postcss: 8.5.23 - postcss-selector-parser: 6.1.2 + postcss-selector-parser: 6.1.4 postcss-minify-font-values@6.1.0(postcss@8.5.23): dependencies: @@ -13273,7 +13245,7 @@ snapshots: postcss-minify-params@6.1.0(postcss@8.5.23): dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 cssnano-utils: 4.0.2(postcss@8.5.23) postcss: 8.5.23 postcss-value-parser: 4.2.0 @@ -13281,7 +13253,7 @@ snapshots: postcss-minify-selectors@6.0.4(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 6.1.2 + postcss-selector-parser: 6.1.4 postcss-modules-extract-imports@3.1.0(postcss@8.5.23): dependencies: @@ -13342,7 +13314,7 @@ snapshots: postcss-normalize-unicode@6.1.0(postcss@8.5.23): dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 postcss: 8.5.23 postcss-value-parser: 4.2.0 @@ -13419,7 +13391,7 @@ snapshots: '@csstools/postcss-trigonometric-functions': 4.0.9(postcss@8.5.23) '@csstools/postcss-unset-value': 4.0.0(postcss@8.5.23) autoprefixer: 10.4.21(postcss@8.5.23) - browserslist: 4.27.0 + browserslist: 4.28.8 css-blank-pseudo: 7.0.1(postcss@8.5.23) css-has-pseudo: 7.0.3(postcss@8.5.23) css-prefers-color-scheme: 10.0.0(postcss@8.5.23) @@ -13463,7 +13435,7 @@ snapshots: postcss-reduce-initial@6.1.0(postcss@8.5.23): dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 caniuse-api: 3.0.0 postcss: 8.5.23 @@ -13481,7 +13453,7 @@ snapshots: postcss: 8.5.23 postcss-selector-parser: 7.1.5 - postcss-selector-parser@6.1.2: + postcss-selector-parser@6.1.4: dependencies: cssesc: 3.0.0 util-deprecate: 1.0.2 @@ -13505,7 +13477,7 @@ snapshots: postcss-unique-selectors@6.0.4(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 6.1.2 + postcss-selector-parser: 6.1.4 postcss-value-parser@4.2.0: {} @@ -14363,9 +14335,9 @@ snapshots: stylehacks@6.1.1(postcss@8.5.23): dependencies: - browserslist: 4.28.5 + browserslist: 4.28.8 postcss: 8.5.23 - postcss-selector-parser: 6.1.2 + postcss-selector-parser: 6.1.4 stylis-rule-sheet@0.0.10(stylis@3.5.1): dependencies: @@ -14542,15 +14514,9 @@ snapshots: unpipe@1.0.0: {} - update-browserslist-db@1.1.4(browserslist@4.27.0): - dependencies: - browserslist: 4.27.0 - escalade: 3.2.0 - picocolors: 1.1.1 - - update-browserslist-db@1.2.3(browserslist@4.28.5): + update-browserslist-db@1.3.1(browserslist@4.28.8): dependencies: - browserslist: 4.28.5 + browserslist: 4.28.8 escalade: 3.2.0 picocolors: 1.1.1 @@ -14758,7 +14724,7 @@ snapshots: '@webassemblyjs/wasm-parser': 1.14.1 acorn: 8.18.0 acorn-import-phases: 1.0.4(acorn@8.18.0) - browserslist: 4.28.5 + browserslist: 4.28.8 chrome-trace-event: 1.0.4 enhanced-resolve: 5.24.2 es-module-lexer: 2.3.0