ci: add race-detector job; fix real data races it caught

.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)

.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

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

cmd/ai/skill/install_test.go

@@ -113,16 +113,7 @@ func TestInstallCmd_ArgsValidation(t *testing.T) {
 // for "atmos ai skill install" with no <source>: 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)
 

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)
+}

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

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")
 }

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()

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

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() {

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{})
 

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()