From 0905db14f87d37de2f40a969b7f6e182c8755c19 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 09:15:30 -0500 Subject: [PATCH 01/12] ci: run atmos test race on pull requests The race detector has caught five real data races in this codebase during 2026 (see docs/fixes/), but no workflow ever ran `atmos test race`. Add a job that installs libudev-dev (required to compile the cgo half of github.com/bearsh/hid under CGO_ENABLED=1, the same trap govulncheck documents but works around differently) and runs the full suite under the race detector on every PR, merge-queue entry, and push to main/release branches. Also fixes the race command's description, which said "quick tests" despite always running the full ./... suite. Co-Authored-By: Claude Sonnet 5 --- .atmos.d/test.yaml | 4 +-- .github/workflows/test.yml | 52 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/.atmos.d/test.yaml b/.atmos.d/test.yaml index 8886c3dd6e..136477361b 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 full test 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 full test suite with race detector and shuffled order env: GOTOOLCHAIN: *go_auto_toolchain CGO_ENABLED: "1" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 32953e15b7..f97b4f896b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -595,6 +595,58 @@ 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" + runs-on: ubuntu-latest + # 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. + timeout-minutes: 45 + steps: + - name: Harden Runner + uses: step-security/harden-runner@05e31511f85b41b11d1cf0ef85d0992719546e2c # v2.21.0 + with: + egress-policy: audit + + - 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: | + sudo sed -i 's|http://azure.archive.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list.d/ubuntu.sources + 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 + 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 From b0901e88a80debeec801e23170f771920c1571a3 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 10:51:34 -0500 Subject: [PATCH 02/12] fix(ci): exclude tests/ acceptance suite from atmos test race The new race job timed out: github.com/cloudposse/atmos/tests hung for 11m and tests/testhelpers panicked with "test timed out after 10m0s" inside TestAtmosRunner_buildWithCoverage, stuck waiting on a `go build` subprocess. The `test` job's own matrix comment measures this suite's unsharded runtime at ~90m on Linux (hence its 10-way shard split) -- `atmos test race`'s $(go list ./...) was pulling it in whole. It also shells out to a plain (non -race) `atmos` binary, so racing the driver process caught no races in the binary under test. Exclude ./tests/... from the race run; it's still covered (without race) by the sharded acceptance job. Co-Authored-By: Claude Sonnet 5 --- .atmos.d/test.yaml | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/.atmos.d/test.yaml b/.atmos.d/test.yaml index 136477361b..5154635862 100644 --- a/.atmos.d/test.yaml +++ b/.atmos.d/test.yaml @@ -16,7 +16,7 @@ commands: default: false - name: race type: bool - description: Run the full test suite 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 the full test suite 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,15 @@ 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. + command: go test -race -shuffle=on ${TEST:-$(go list ./... | grep -v '^github.com/cloudposse/atmos/tests')} ${TESTARGS:-} -timeout 10m - 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) From 556229a640526d126719546ec3db8f031128090e Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 12:23:39 -0500 Subject: [PATCH 03/12] fix(ci): resolve race job timeouts and a shuffle-order test bug Second real run of the race job surfaced two more issues: - pkg/toolchain timed out at 10m0s. Its tests install real tool binaries from real registries with no mock seam, and the race command runs the whole ./... suite unsharded and unauthenticated, so every package's network calls compete for the same runner and the same IP-wide GitHub rate limit -- unlike the `test` job's acceptance steps, which already set GITHUB_TOKEN for this reason. Set GITHUB_TOKEN on the race job's step and raise the command's -timeout from 10m to 20m for headroom. - pkg/utils's TestClearInternPool asserted stats without first clearing the package-level intern pool, silently depending on running before any other test interned a string. -shuffle=on randomizes order, so once another test ran first the assertion failed (expected 3, got 17). This is the first time -shuffle=on ran against the full unit-test suite in CI. Fixed by clearing the pool at the start, matching the adjacent TestResetInternStats. See docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md. Co-Authored-By: Claude Sonnet 5 --- .atmos.d/test.yaml | 12 ++- .github/workflows/test.yml | 7 ++ ...026-09-01-race-detector-ci-job-timeouts.md | 77 +++++++++++++++++++ pkg/utils/string_utils_test.go | 7 ++ 4 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md diff --git a/.atmos.d/test.yaml b/.atmos.d/test.yaml index 5154635862..ab76835a7e 100644 --- a/.atmos.d/test.yaml +++ b/.atmos.d/test.yaml @@ -191,7 +191,17 @@ commands: # race coverage on the binary under test. Running it here blew # past every timeout budget with no race-detection value to show # for it. - command: go test -race -shuffle=on ${TEST:-$(go list ./... | grep -v '^github.com/cloudposse/atmos/tests')} ${TESTARGS:-} -timeout 10m + # + # -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. + command: go test -race -shuffle=on ${TEST:-$(go list ./... | grep -v '^github.com/cloudposse/atmos/tests')} ${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 f97b4f896b..93a0a5b2cc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -645,6 +645,13 @@ jobs: 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. 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 0000000000..560660c28a --- /dev/null +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -0,0 +1,77 @@ +# Fix: `[race] full test suite` CI job timeouts and a shuffle-order test-isolation bug + +**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 two real runs, for three separate reasons: the package list included the +CLI acceptance suite (which is 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`. + +## 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: + +1. `$(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. +2. 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. +3. 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. + +## 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`. + +## 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: a local `go test -race -shuffle=on + ./pkg/toolchain/...` run stalled 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 (bounded retry/backoff math, the `test` job's own precedent for + needing `GITHUB_TOKEN`) rather than confirmed by a clean local repro. The next real CI run of + this job is the actual validation and should be checked. + +## Follow-ups + +None. diff --git a/pkg/utils/string_utils_test.go b/pkg/utils/string_utils_test.go index 390448eec0..52e5c182ed 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. From 9f170d6f9c1413d16943c59f526c1a91a80357d2 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 13:54:51 -0500 Subject: [PATCH 04/12] fix: close a real data race in the toolchain's concurrent installer The race job (fixed for timeouts/shuffle-order in the prior commits) caught its first genuine production bug: pkg/toolchain's concurrent batch installer runs pkg/ui/theme.getActiveThemeName() (writes via viper.BindEnv, on every styled render) and pkg/http.GetGitHubTokenFromEnv() (reads via viper.GetString) from separate worker goroutines, both against the process-wide global viper singleton, which has no locking of its own. pkg/config already has a SafeViper mutex-guard for exactly this class of problem (built for the DAG scheduler's concurrent LoadConfig calls), but pkg/http and pkg/ui/theme sit below pkg/config in the import graph and can't reach it without a cycle. Move the guard into a new leaf package, pkg/viperguard, with no Atmos-internal imports; pkg/config.GlobalViper() now delegates to it instead of keeping a second, independent mutex (two separate locks on the same underlying singleton wouldn't exclude each other), and pkg/http/pkg/ui/theme route their global-viper access through it directly. Also fixes a second -shuffle=on test-isolation bug this run surfaced: TestGitHubTokenEnvBinding depended on TestMain's one-time "github-token" env binding surviving every sibling test, but set_test.go's teardownTest() calls viper.Reset(), which discards it. Previously inert (GITHUB_TOKEN was never set in CI before the prior commit); now fixed by re-binding after Reset() and defensively before the assertion that depends on it. See docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md. Co-Authored-By: Claude Sonnet 5 --- ...026-09-01-race-detector-ci-job-timeouts.md | 138 +++++++++++---- pkg/config/global_viper.go | 88 +++------- pkg/http/client.go | 16 +- pkg/toolchain/github_token_test.go | 8 + pkg/toolchain/set_test.go | 7 + pkg/ui/theme/styles.go | 29 ++-- pkg/viperguard/viperguard.go | 161 ++++++++++++++++++ pkg/viperguard/viperguard_test.go | 62 +++++++ 8 files changed, 389 insertions(+), 120 deletions(-) create mode 100644 pkg/viperguard/viperguard.go create mode 100644 pkg/viperguard/viperguard_test.go 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 index 560660c28a..5168387869 100644 --- a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -1,15 +1,18 @@ -# Fix: `[race] full test suite` CI job timeouts and a shuffle-order test-isolation bug +# Fix: `[race] full test suite` CI job — timeouts, a shuffle-order test bug, and a real data race **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 two real runs, for three separate reasons: the package list included the -CLI acceptance suite (which is 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`. +failed on its first three 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, with the first two rounds' fixes in place: the job caught 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. ## Context @@ -17,29 +20,49 @@ test-isolation bug in `pkg/utils`. Wiring it into CI (`.github/workflows/test.yml`) surfaced three latent problems that had never been exercised together before: -1. `$(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. -2. 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. -3. 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. +- `$(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. ## Changes @@ -55,6 +78,27 @@ been exercised together before: `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. ## Validation @@ -64,13 +108,35 @@ been exercised together before: - `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: a local `go test -race -shuffle=on - ./pkg/toolchain/...` run stalled 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 (bounded retry/backoff math, the `test` job's own precedent for - needing `GITHUB_TOKEN`) rather than confirmed by a clean local repro. The next real CI run of - this job is the actual validation and should be checked. +- 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. ## Follow-ups diff --git a/pkg/config/global_viper.go b/pkg/config/global_viper.go index 4e27517bcd..95db6ef347 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/http/client.go b/pkg/http/client.go index 851a0b771f..5bf0ce408b 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/toolchain/github_token_test.go b/pkg/toolchain/github_token_test.go index 1764f99f04..083194b724 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 cc6bc1fde0..993ab31fa9 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 a1c017e968..346557fa8b 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/viperguard/viperguard.go b/pkg/viperguard/viperguard.go new file mode 100644 index 0000000000..e2b486eb0e --- /dev/null +++ b/pkg/viperguard/viperguard.go @@ -0,0 +1,161 @@ +// 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 an explicit value from any source (flag, env, +// config, override) -- unlike a plain Get, it does not count a registered +// default as "set". +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 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 { + 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. +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 0000000000..eec023ab75 --- /dev/null +++ b/pkg/viperguard/viperguard_test.go @@ -0,0 +1,62 @@ +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") +} From cea2c079672ff08559e6865d914906086ff1ebc7 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 15:00:00 -0500 Subject: [PATCH 05/12] fix: two more real races and four shuffle-order test bugs; bump race job runner The race job ran the full suite to completion for the first time and surfaced seven independent failures in one run: - pkg/lsp/server: DocumentManager.Update mutated an existing *Document's fields in place under its own lock, but validateDocument reads them through the returned pointer with no lock held right after -- a second, overlapping didChange for the same URI raced with it. Update now stores a new *Document per version instead of mutating the shared one. - pkg/terraform/cache: nativeWindowsTrustInstall's closure read the package-level installWindowsTrustFunc var from inside a background goroutine that a timeout lets keep running after the caller returns; a test's t.Cleanup reassigning that var raced with it. Now snapshots the func into a local var before the goroutine starts. - pkg/terraform/registry: fakeRegistry's dlHits/verHits counters were incremented from concurrent httptest.Server handler goroutines with no synchronization -- and never read anywhere. Removed them. - pkg/runner/step, pkg/provisioner/backend, pkg/scanners/sarif, pkg/ui/theme: four more -shuffle=on test-isolation bugs, all the same shape as round 3's github-token one -- a test resets/overrides package-level global state in cleanup without restoring it (or, for sarif, uses viper.Set where t.Setenv was needed), silently breaking whatever test the shuffled order runs next. Also: once every timeout was fixed, this job became the slowest check in the PR. go test's package-level concurrency scales with cores, so swap ubuntu-latest for the same RunsOn runner family the build job's linux leg already uses for CPU-heavy Go work. See docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/test.yml | 17 ++- ...026-09-01-race-detector-ci-job-timeouts.md | 110 +++++++++++++++++- pkg/lsp/server/documents.go | 21 +++- pkg/provisioner/backend/azurerm_test.go | 13 ++- pkg/runner/step/output_mode_execution_test.go | 11 ++ pkg/scanners/sarif/normalize_test.go | 15 ++- pkg/terraform/cache/trust_install.go | 14 ++- .../registry/provider_mirror_test.go | 10 +- pkg/ui/theme/styles_test.go | 10 ++ 9 files changed, 193 insertions(+), 28 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 93a0a5b2cc..95a0fe8330 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -610,17 +610,24 @@ jobs: # is unaffected by CGO_ENABLED: Go's FIPS 140-3 module is pure Go, no cgo. race: name: "[race] full test suite" - runs-on: ubuntu-latest + # `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). + # Reuse the same RunsOn "terraform" runner family the build job's linux + # leg already uses for CPU-heavy Go work in this file, rather than + # inventing a new profile. + runs-on: "runs-on=${{github.run_id}}/runner=terraform/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. timeout-minutes: 45 steps: - - name: Harden Runner - uses: step-security/harden-runner@05e31511f85b41b11d1cf0ef85d0992719546e2c # v2.21.0 - with: - egress-policy: audit + # 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 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 index 5168387869..b1cfce2cb9 100644 --- a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -1,18 +1,24 @@ -# Fix: `[race] full test suite` CI job — timeouts, a shuffle-order test bug, and a real data race +# 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 three real runs. Rounds 1–2: the package list included the CLI acceptance +failed on its first four 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, with the first two rounds' fixes in place: the job caught 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. +`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. Also bumped the job's +runner: once every timeout was fixed, this became the slowest check in the PR. ## Context @@ -64,6 +70,55 @@ been exercised together before: 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 @@ -99,6 +154,36 @@ been exercised together before: - `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. ## Validation @@ -137,6 +222,19 @@ been exercised together before: - 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. ## Follow-ups diff --git a/pkg/lsp/server/documents.go b/pkg/lsp/server/documents.go index 1460df1ec1..6318ea62e2 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/provisioner/backend/azurerm_test.go b/pkg/provisioner/backend/azurerm_test.go index 6865c2a5db..a51f8b052d 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 6dc4d1bba1..e7c7e1eac4 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 29873d4159..1b25bb46b2 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 3a07acfc8e..221adcc206 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 2fa61580a1..3c6da15a4a 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/ui/theme/styles_test.go b/pkg/ui/theme/styles_test.go index fe05901570..734ebbd49b 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", From 4a3f9d34cbf23a7edc0e74bf0eb0a65921c06491 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 15:08:27 -0500 Subject: [PATCH 06/12] fix(ci): fix race job runner swap - wrong family, broken apt mirror step The runner-swap commit picked "terraform" without checking its specs: turns out it's an i4i.large (2 cores, 15.7GB RAM) -- fewer cores than ubuntu-latest, a downgrade for this CPU/memory-bound -race workload. Switch to "large" (used by the release job's goreleaser step): an r7a.xlarge, 4 cores, 31GB RAM, double "terraform" on both counts. Separately, the job failed outright before running any tests: its "Install Linux build dependencies" step's `sed -i .../ubuntu.sources` (copied from the floci-go job, which runs on ubuntu-latest) errored with "No such file or directory" -- the RunsOn AMI is Ubuntu 22.04, which has no DEB822 .sources file (a GitHub-hosted 24.04+ image convention). Guard the sed behind a file-existence check so the step works on either runner image. See docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/test.yml | 27 ++++++++++++++----- ...026-09-01-race-detector-ci-job-timeouts.md | 23 +++++++++++++--- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 95a0fe8330..02e490b614 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -612,12 +612,16 @@ jobs: 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). - # Reuse the same RunsOn "terraform" runner family the build job's linux - # leg already uses for CPU-heavy Go work in this file, rather than - # inventing a new profile. - runs-on: "runs-on=${{github.run_id}}/runner=terraform/tag=atmos/extras=s3-cache/private=false" + # 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 @@ -647,7 +651,16 @@ jobs: - name: Install Linux build dependencies (libudev-dev for CGO_ENABLED=1 -- see comment above) run: | - sudo sed -i 's|http://azure.archive.ubuntu.com/ubuntu|http://archive.ubuntu.com/ubuntu|g' /etc/apt/sources.list.d/ubuntu.sources + # 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 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 index b1cfce2cb9..40b26ab890 100644 --- a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -5,7 +5,7 @@ ## Summary The new `[race] full test suite` CI job (added to run `atmos test race` on pull requests) -failed on its first four real runs. Rounds 1–2: the package list included the CLI acceptance +failed on its first five 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 @@ -17,8 +17,10 @@ once, spread across unrelated packages — two real data races (an LSP document- 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. Also bumped the job's -runner: once every timeout was fixed, this became the slowest check in the PR. +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. ## Context @@ -236,6 +238,21 @@ run surfaced seven more independent failures: - 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. + ## Follow-ups None. From 4b4d474f62aa652c3059937adb00447e5e91c2e7 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 16:11:45 -0500 Subject: [PATCH 07/12] fix: close pkg/perf data race and a heatmap-test tracking leak The race job ran to completion for the first time (after fixing its runner) and hit ~20 failing cmd tests plus 24 DATA RACE warnings. 21 of the 24 races traced to one root cause: pkg/perf's "simple stack" fast path (used by the defer perf.Track(...) call at the top of nearly every public function) only verifies goroutine ownership of its shared global stack at call depth 0/1, trusting it at deeper nesting for speed -- a documented "known limitation" where a second goroutine's calls can silently share the stack undetected. Beyond producing wrong metrics (the accepted tradeoff), StackFrame.childTime was a plain time.Duration read/written with no synchronization at all once two goroutines' frames actually interleaved -- a genuine data race. Changed it to atomic.Int64. That doesn't explain why so many otherwise-unrelated cmd tests hit it: perf tracking is off by default (Track is a no-op) unless something enables it. cmd/root_heatmap_test.go's TestDisplayPerformanceHeatmap (both cases) and TestHeatmapNonTTYOutput call perf.EnableTracking(true) directly to exercise the heatmap display but, unlike the well-behaved TestEnableHeatmapIfRequested, never disabled it afterward -- so once any ran under -shuffle=on, tracking stayed on for the rest of the cmd package's test binary, turning every subsequent perf.Track() call live and racy. Added perf.ResetForTesting() (the misleading pre-existing comments already claimed a reset existed) and wired all four heatmap-adjacent tests to enable/reset/disable correctly. One further failure was unrelated: TestUninstallCmd_RunE_MultipleSkills set the force flag via Lookup("force").Value.Set("true"), which updates the value but not pflag's Changed bookkeeping that viper's binding checks for precedence -- unlike every sibling test in the file, which correctly uses Flags().Set(). Fixed to match. See docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md. Co-Authored-By: Claude Sonnet 5 --- cmd/ai/skill/uninstall_test.go | 16 ++-- cmd/cmd_utils_test.go | 5 ++ cmd/root_heatmap_test.go | 16 +++- ...026-09-01-race-detector-ci-job-timeouts.md | 74 ++++++++++++++++++- pkg/perf/perf.go | 39 ++++++++-- 5 files changed, 134 insertions(+), 16 deletions(-) diff --git a/cmd/ai/skill/uninstall_test.go b/cmd/ai/skill/uninstall_test.go index c20457b6b4..4ebb4d830a 100644 --- a/cmd/ai/skill/uninstall_test.go +++ b/cmd/ai/skill/uninstall_test.go @@ -996,11 +996,17 @@ Second test skill. err = os.WriteFile(registryPath, registryData, 0o600) require.NoError(t, err) - // Set force flag. - forceFlag := uninstallCmd.Flags().Lookup("force") - if forceFlag != nil { - _ = forceFlag.Value.Set("true") - } + // 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. + require.NoError(t, uninstallCmd.Flags().Set("force", "true")) + t.Cleanup(func() { + _ = uninstallCmd.Flags().Set("force", "false") + }) // Capture stdout. oldStdout := os.Stdout diff --git a/cmd/cmd_utils_test.go b/cmd/cmd_utils_test.go index 8b22523f7e..d01118e786 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/root_heatmap_test.go b/cmd/root_heatmap_test.go index 4e25d25427..dadf6ccc6d 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/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md index 40b26ab890..62f81397b3 100644 --- a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -5,7 +5,7 @@ ## Summary The new `[race] full test suite` CI job (added to run `atmos test race` on pull requests) -failed on its first five real runs. Rounds 1–2: the package list included the CLI acceptance +failed on its first six 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 @@ -20,7 +20,12 @@ viper.Set-vs-env-var-tracking conflict, and a style cache left seeded with a par 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. +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. ## Context @@ -253,6 +258,71 @@ comparison: its "large" family is an `r7a.xlarge`, 4 cores, 31GB RAM. `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 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. + ## Follow-ups None. diff --git a/pkg/perf/perf.go b/pkg/perf/perf.go index 88ff9d87a6..17e8ecc888 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. From 1fd37105b734443c0b2f28f14ccf337fafa9e8b4 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 17:45:16 -0500 Subject: [PATCH 08/12] fix: upstream bubbles race, widespread --help flag leak, two more resets Another clean-ish run (7 total now) surfaced a smaller set of failures once the previous round's fixes landed: - internal/exec: TestExecuteComponentVendorPullBatch_PullsAllComponentsInOneCall hit a real race inside charmbracelet/bubbles@v1.0.0's progress.Model -- SetPercent's returned tea.Cmd reads m.tag/m.id back off the same *Model pointer when its tick fires, on a different goroutine than the one that can call SetPercent again before that tick lands. This is an upstream bug; can't patch a vendored dependency here, so avoid triggering it instead -- nothing renders the animation without a TTY, so only call SetPercent when m.isTTY. - cmd/init, cmd/scaffold: found the same latent bug in three "--help" integration tests. Cobra's execute() checks the "help" pflag's *current* value on every Execute() call, not whether --help was in that specific invocation's args -- so initCmd.SetArgs([]string{"--help"}) (and four scaffold equivalents) left it permanently true, and every later test that called that command's Execute() got nil back having silently printed help, RunE never called, no matter what args it passed. This is why TestExecuteInit_ArgumentParsing (already fixed once, for a different reason) kept reappearing -- it needed the exact same shuffle order to reproduce both bugs together. - cmd/version, cmd/describe_stacks/dependents (carried over from the round 6 investigation), cmd/validate_editorconfig: two more reset-without-restore leaks (a package-level "format" var, a ciFlagsParser Viper binding lost to another test's viper.Reset()). One failure -- cmd/list's TestListStacksWithOptions_CoverageIntegration -- didn't reproduce on retries with the seed that had just produced it, ruling out simple ordering in favor of genuine goroutine-timing nondeterminism. Left open; see docs/fixes Follow-ups. See docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md. Co-Authored-By: Claude Sonnet 5 --- cmd/describe_dependents_test.go | 8 +- cmd/describe_stacks_test.go | 11 +- cmd/init/init_test.go | 11 ++ cmd/scaffold/scaffold_test.go | 19 +++ cmd/validate_editorconfig_test.go | 7 ++ cmd/version/list_test.go | 9 ++ ...026-09-01-race-detector-ci-job-timeouts.md | 108 +++++++++++++++++- internal/exec/vendor_model.go | 14 ++- 8 files changed, 179 insertions(+), 8 deletions(-) diff --git a/cmd/describe_dependents_test.go b/cmd/describe_dependents_test.go index 82cf579557..f36f5b778c 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 95c20a89e7..aa29615ca4 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/init/init_test.go b/cmd/init/init_test.go index 036b0d29e9..ab95f88bd3 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/scaffold/scaffold_test.go b/cmd/scaffold/scaffold_test.go index efe8c42c7e..1d7d158337 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/validate_editorconfig_test.go b/cmd/validate_editorconfig_test.go index 33a40e1089..d0fb4b6218 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 c61c3dc1f9..f44a096d4a 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 index 62f81397b3..991d1988fc 100644 --- a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -5,7 +5,7 @@ ## Summary The new `[race] full test suite` CI job (added to run `atmos test race` on pull requests) -failed on its first six real runs. Rounds 1–2: the package list included the CLI acceptance +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 @@ -25,7 +25,13 @@ and the job ran to completion for the first time: a single root cause in `pkg/pe 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. +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 @@ -191,6 +197,22 @@ run surfaced seven more independent failures: 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 @@ -258,6 +280,54 @@ comparison: its "large" family is an `r7a.xlarge`, 4 cores, 31GB RAM. `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" @@ -323,6 +393,38 @@ Round 6 validation: 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. +- `cmd/list`'s `TestListStacksWithOptions_CoverageIntegration` failed once locally with + `authentication requires at least one identity configured in atmos.yaml` even though the + `complete` fixture it uses has identities configured and the test normally passes. It did not + reproduce on two follow-up attempts using the exact `-shuffle` seed that produced the original + failure, which rules out a simple test-ordering/leftover-value explanation (those are + seed-deterministic) in favor of genuine goroutine-timing nondeterminism between some earlier + test and this one. Investigated and ruled out: `cmd/list`'s only two `t.Parallel()` tests + (`cmd/list/closure_test.go`) touch no auth/config/viper state; `t.Chdir` (used by + `chdirToCompleteFixture`) has Go's own built-in serialization against concurrent misuse; no + obvious `sync.Once`/singleton pattern in `pkg/auth`'s manager code. Left unfixed. If it recurs + in a future CI run, the next investigation should reach for the actual race-detector output + (this failure carries no `WARNING: DATA RACE` block itself, but the CI log may show one nearby + that this local investigation didn't capture) rather than repeating the same seed-replay + approach that already came up empty twice. diff --git a/internal/exec/vendor_model.go b/internal/exec/vendor_model.go index 0d57daafd2..e6446b4757 100644 --- a/internal/exec/vendor_model.go +++ b/internal/exec/vendor_model.go @@ -329,8 +329,18 @@ 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. Only when rendering to a TTY: charmbracelet/bubbles's + // progress.Model.SetPercent returns a tea.Cmd (nextFrame) whose closure reads + // back m.tag/m.id from the *Model pointer when the 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, as in tests, reliably triggers; upstream bug, not an + // Atmos usage issue). Nothing renders m.progress.View() when there's no TTY + // to show it to, so there's no reason to animate it there either. + var progressCmd tea.Cmd + if m.isTTY { + progressCmd = m.progress.SetPercent(float64(m.index) / float64(len(m.packages))) + } version = grayColor.Render(version) return m, tea.Batch( From eac475d69936fb3ac304d8f453286a99433a3979 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 19:15:57 -0500 Subject: [PATCH 09/12] fix: process-cache leak in pkg/auth, cobra flag Changed leaks, cmd/list identity leak, CodeRabbit findings Round 9 of the race-detector CI incident, plus CodeRabbit review on PR #3022: - pkg/auth: describeWorkflowsCmd's test-added --pager flag panicked ("flag redefined") when a prior test's full Execute() pipeline had already merged RootCmd's persistent --pager flag in; guard with a Lookup check. - pkg/auth/manager_test.go: the process-level credential cache (processCredentialCache) was never reset between Whoami/Authenticate tests that reuse the same provider/identity names, so a passing test's cached credentials leaked into a test asserting authentication failure. Added resetProcessCredentialCache() + t.Cleanup to every affected test. - cmd/list: root-caused and fixed the previously "left open" cmd/list flake (TestListStacksWithOptions_CoverageIntegration and three siblings) -- several tests left a leaked viper "identity" value that a later executor-integration test's identity resolution picked up, tripping an "authentication requires at least one identity configured" error against the intentionally identity-less `complete` fixture. See the fix-log's Round 9 section for the full mechanism. - cmd/terraform/cache/mirror_test.go: TestMirrorCmdRunAll left the --all flag's Changed=true after passing --all, so a later test that never passes --all still observed All=true via viper's flag-precedence binding. - internal/exec/vendor_model.go: closed the remaining edge of the SetPercent data race (flagged by CodeRabbit) by dropping bubbles' animated SetPercent/tea.Tick machinery entirely in favor of a plain percent field rendered with ViewAs -- no shared Model state, no race. - cmd/ai/skill/uninstall_test.go: restore the force flag's original Changed state in cleanup, not just its value (CodeRabbit). - pkg/viperguard: corrected the IsSet doc comment (SetDefault also makes IsSet true), documented View's callback-must-not-call-a-writer restriction, and added test coverage for GetBool/View/viperReaderAdapter (100% package coverage). - .atmos.d/test.yaml: the race command's `go list | grep` pipe could mask a failing go list under mvdan/sh's default (off) pipefail; switched to an explicit set -euo pipefail + variable-assignment form. - .github/workflows/test.yml: bumped the race job's timeout from 45 to 75 minutes -- recent runs finished within a few minutes of the old budget. Co-Authored-By: Claude Sonnet 5 --- .atmos.d/test.yaml | 15 +++++- .github/workflows/test.yml | 9 +++- cmd/ai/skill/uninstall_test.go | 13 ++++- cmd/describe_workflows_test.go | 9 +++- cmd/list/affected_test.go | 13 +++++ cmd/list/instances_test.go | 6 +++ cmd/list/utils_test.go | 8 +++ cmd/terraform/cache/mirror_test.go | 15 ++++++ ...026-09-01-race-detector-ci-job-timeouts.md | 50 +++++++++++++------ internal/exec/vendor_model.go | 32 +++++------- pkg/auth/manager_test.go | 33 ++++++++++++ pkg/viperguard/viperguard.go | 18 ++++--- pkg/viperguard/viperguard_test.go | 39 +++++++++++++++ 13 files changed, 215 insertions(+), 45 deletions(-) diff --git a/.atmos.d/test.yaml b/.atmos.d/test.yaml index ab76835a7e..4ed04c3f83 100644 --- a/.atmos.d/test.yaml +++ b/.atmos.d/test.yaml @@ -201,7 +201,20 @@ commands: # 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. - command: go test -race -shuffle=on ${TEST:-$(go list ./... | grep -v '^github.com/cloudposse/atmos/tests')} ${TESTARGS:-} -timeout 20m + # + # 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 02e490b614..926b18fe8a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -625,8 +625,13 @@ jobs: # 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. - timeout-minutes: 45 + # 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 diff --git a/cmd/ai/skill/uninstall_test.go b/cmd/ai/skill/uninstall_test.go index 4ebb4d830a..bcd85fc43e 100644 --- a/cmd/ai/skill/uninstall_test.go +++ b/cmd/ai/skill/uninstall_test.go @@ -1003,9 +1003,20 @@ Second test skill. // 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") + originalForceValue := forceFlag.Value.String() + originalForceChanged := forceFlag.Changed require.NoError(t, uninstallCmd.Flags().Set("force", "true")) t.Cleanup(func() { - _ = uninstallCmd.Flags().Set("force", "false") + _ = uninstallCmd.Flags().Set("force", originalForceValue) + forceFlag.Changed = originalForceChanged }) // Capture stdout. diff --git a/cmd/describe_workflows_test.go b/cmd/describe_workflows_test.go index 3c3b65fac9..80fa329f7b 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/list/affected_test.go b/cmd/list/affected_test.go index 90850dc8d0..f35b04209e 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 a083f6bb81..11c01ab317 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 a5b9d2e087..c87e4202c8 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/terraform/cache/mirror_test.go b/cmd/terraform/cache/mirror_test.go index 3e69f6795b..5bbe1fd450 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/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md index 991d1988fc..cca70287fd 100644 --- a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -414,17 +414,39 @@ Round 7 validation: ## Follow-ups -- `cmd/list`'s `TestListStacksWithOptions_CoverageIntegration` failed once locally with - `authentication requires at least one identity configured in atmos.yaml` even though the - `complete` fixture it uses has identities configured and the test normally passes. It did not - reproduce on two follow-up attempts using the exact `-shuffle` seed that produced the original - failure, which rules out a simple test-ordering/leftover-value explanation (those are - seed-deterministic) in favor of genuine goroutine-timing nondeterminism between some earlier - test and this one. Investigated and ruled out: `cmd/list`'s only two `t.Parallel()` tests - (`cmd/list/closure_test.go`) touch no auth/config/viper state; `t.Chdir` (used by - `chdirToCompleteFixture`) has Go's own built-in serialization against concurrent misuse; no - obvious `sync.Once`/singleton pattern in `pkg/auth`'s manager code. Left unfixed. If it recurs - in a future CI run, the next investigation should reach for the actual race-detector output - (this failure carries no `WARNING: DATA RACE` block itself, but the CI log may show one nearby - that this local investigation didn't capture) rather than repeating the same seed-replay - approach that already came up empty twice. +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 compiled 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 `-count=3` invocation at the same 300s timeout +hit that budget mid-run and was killed — `cmd/list` under `-race` takes ~90-110s per single pass locally, so +3 consecutive passes need a longer timeout than 300s; that's a local verification-budget artifact, not a +test failure — the goroutine dump it produced was ordinary `t.Parallel()` tests waiting their turn, not a +deadlock.) diff --git a/internal/exec/vendor_model.go b/internal/exec/vendor_model.go index e6446b4757..c08107c8fe 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,22 +324,19 @@ func (m *modelVendor) handleInstalledPkgMsg(msg *installedPkgMsg) (tea.Model, te } } m.index++ - // Update progress bar. Only when rendering to a TTY: charmbracelet/bubbles's - // progress.Model.SetPercent returns a tea.Cmd (nextFrame) whose closure reads - // back m.tag/m.id from the *Model pointer when the 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, as in tests, reliably triggers; upstream bug, not an - // Atmos usage issue). Nothing renders m.progress.View() when there's no TTY - // to show it to, so there's no reason to animate it there either. - var progressCmd tea.Cmd - if m.isTTY { - 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 ) @@ -468,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/pkg/auth/manager_test.go b/pkg/auth/manager_test.go index d444babe09..84016709d5 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/viperguard/viperguard.go b/pkg/viperguard/viperguard.go index e2b486eb0e..58f41ad2b5 100644 --- a/pkg/viperguard/viperguard.go +++ b/pkg/viperguard/viperguard.go @@ -78,9 +78,11 @@ func GetStringSlice(key string) []string { return slices.Clone(viper.GetViper().GetStringSlice(key)) } -// 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 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")() @@ -96,9 +98,8 @@ func IsSet(key string) bool { // 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 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 @@ -152,6 +153,11 @@ func (a viperReaderAdapter) GetStringSlice(key string) []string { // 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")() diff --git a/pkg/viperguard/viperguard_test.go b/pkg/viperguard/viperguard_test.go index eec023ab75..93407bb79d 100644 --- a/pkg/viperguard/viperguard_test.go +++ b/pkg/viperguard/viperguard_test.go @@ -60,3 +60,42 @@ func TestConcurrentBindEnvAndGet(t *testing.T) { 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")) +} From ece8d4317a3d5bda2b55340031e1c8f39a9ac83d Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 19:35:24 -0500 Subject: [PATCH 10/12] docs: fix-log wording nits from CodeRabbit (executed vs compiled, -count=3 caveat) - "compiled test set" -> "executed test set": -run filters which compiled tests execute, it doesn't change what's compiled into the binary. - Make explicit that the -count=3 run is not part of the validation record: it was interrupted by the 300s timeout before finishing, not a pass. Co-Authored-By: Claude Sonnet 5 --- Dockerfile | 2 +- ...026-09-01-race-detector-ci-job-timeouts.md | 13 ++-- website/package.json | 2 + website/pnpm-lock.yaml | 78 ++++++++++--------- 4 files changed, 50 insertions(+), 45 deletions(-) diff --git a/Dockerfile b/Dockerfile index 0be3cd557d..d723c75967 100644 --- a/Dockerfile +++ b/Dockerfile @@ -38,7 +38,7 @@ RUN set -ex; \ # effect for the standalone install method), so pin the package version # explicitly after. curl -1sSLf 'https://raw.githubusercontent.com/opentofu/get.opentofu.org/3d354dfa62d9a36f33df16cdd6bb506ace1e6e2e/static/install-opentofu.sh' | bash -s -- --root-method none --install-method deb; \ - apt-get install -y --allow-downgrades tofu=1.12.6; \ + apt-get install -y --no-install-recommends --allow-downgrades tofu=1.12.6; \ # Install Kustomize binary (required by Helmfile). # Direct download instead of install_kustomize.sh which has known bugs (kubernetes-sigs/kustomize#5562). KUSTOMIZE_VERSION=5.8.1; \ 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 index cca70287fd..748359f4f2 100644 --- a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -423,7 +423,7 @@ and, in a later CI run, three siblings — `TestExecuteListInstancesCmd_TreeForm `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 compiled test set, which changes +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 @@ -445,8 +445,9 @@ 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 `-count=3` invocation at the same 300s timeout -hit that budget mid-run and was killed — `cmd/list` under `-race` takes ~90-110s per single pass locally, so -3 consecutive passes need a longer timeout than 300s; that's a local verification-budget artifact, not a -test failure — the goroutine dump it produced was ordinary `t.Parallel()` tests waiting their turn, not a -deadlock.) +`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. diff --git a/website/package.json b/website/package.json index 89bc4ed7a5..a7fc596e58 100644 --- a/website/package.json +++ b/website/package.json @@ -125,6 +125,8 @@ "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", "shell-quote@^1": "^1.8.4", diff --git a/website/pnpm-lock.yaml b/website/pnpm-lock.yaml index 308ec0dd59..7d30fc093a 100644 --- a/website/pnpm-lock.yaml +++ b/website/pnpm-lock.yaml @@ -29,6 +29,8 @@ 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 shell-quote@^1: ^1.8.4 @@ -1236,13 +1238,13 @@ packages: resolution: {integrity: sha512-mf1LEW0tJLKfWyvn5KdDrhpxHyuxpbNwTIwOYLIvsTffeyOf85j5oIzfG0yosxDgx/sswlqBnESYUcQH0vgZ0g==} engines: {node: '>=18'} peerDependencies: - postcss-selector-parser: ^7.0.0 + postcss-selector-parser: ^7.1.3 '@csstools/selector-specificity@5.0.0': resolution: {integrity: sha512-PCqQV3c4CoVm3kdPhyeZ07VmBRdH2EpMFA/pd9OASpOEC3aXNGoqPDAZ80D0cLpMBxnmk0+yNhGsEx31hq7Gtw==} engines: {node: '>=18'} peerDependencies: - postcss-selector-parser: ^7.0.0 + postcss-selector-parser: ^7.1.3 '@csstools/utilities@2.0.0': resolution: {integrity: sha512-5VdOr0Z71u+Yp3ozOx8T11N703wIFGVRgOWbOZMKgglPJsWA54MRIoMNVMa7shUToIhx5J8vX4sOZgD2XiihiQ==} @@ -5411,12 +5413,12 @@ 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.0: - resolution: {integrity: sha512-8sLjZwK0R+JlxlYcTuVnyT2v+htpdrjDOKuMcOVdYjt52Lh8hWRYpxBPoKx/Zg+bcjc3wx6fmQevMmUztS/ccA==} + postcss-selector-parser@7.1.5: + resolution: {integrity: sha512-KvvtD7SrlBP7dlgkBghEE3r84CABm5SmV2aNcG4oCA+qDnJ/tvKonFVvwWAyyWUEwxuNawdfEAZKP9zM3oZ2Uw==} engines: {node: '>=4'} postcss-sort-media-queries@5.2.0: @@ -7683,9 +7685,9 @@ snapshots: '@csstools/postcss-cascade-layers@5.0.2(postcss@8.5.23)': dependencies: - '@csstools/selector-specificity': 5.0.0(postcss-selector-parser@7.1.0) + '@csstools/selector-specificity': 5.0.0(postcss-selector-parser@7.1.5) postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 '@csstools/postcss-color-function-display-p3-linear@1.0.1(postcss@8.5.23)': dependencies: @@ -7791,9 +7793,9 @@ snapshots: '@csstools/postcss-is-pseudo-class@5.0.3(postcss@8.5.23)': dependencies: - '@csstools/selector-specificity': 5.0.0(postcss-selector-parser@7.1.0) + '@csstools/selector-specificity': 5.0.0(postcss-selector-parser@7.1.5) postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 '@csstools/postcss-light-dark-function@2.0.11(postcss@8.5.23)': dependencies: @@ -7885,7 +7887,7 @@ snapshots: '@csstools/postcss-scope-pseudo-class@4.0.1(postcss@8.5.23)': dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 '@csstools/postcss-sign-functions@1.1.4(postcss@8.5.23)': dependencies: @@ -7918,13 +7920,13 @@ snapshots: dependencies: postcss: 8.5.23 - '@csstools/selector-resolve-nested@3.1.0(postcss-selector-parser@7.1.0)': + '@csstools/selector-resolve-nested@3.1.0(postcss-selector-parser@7.1.5)': dependencies: - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 - '@csstools/selector-specificity@5.0.0(postcss-selector-parser@7.1.0)': + '@csstools/selector-specificity@5.0.0(postcss-selector-parser@7.1.5)': dependencies: - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 '@csstools/utilities@2.0.0(postcss@8.5.23)': dependencies: @@ -10692,7 +10694,7 @@ snapshots: css-blank-pseudo@7.0.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 css-declaration-sorter@7.3.0(postcss@8.5.23): dependencies: @@ -10700,9 +10702,9 @@ snapshots: css-has-pseudo@7.0.3(postcss@8.5.23): dependencies: - '@csstools/selector-specificity': 5.0.0(postcss-selector-parser@7.1.0) + '@csstools/selector-specificity': 5.0.0(postcss-selector-parser@7.1.5) postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-value-parser: 4.2.0 css-loader@6.11.0(webpack@5.108.4(postcss@8.5.23)): @@ -13080,12 +13082,12 @@ snapshots: postcss-attribute-case-insensitive@7.0.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 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): @@ -13151,12 +13153,12 @@ snapshots: '@csstools/css-parser-algorithms': 3.0.5(@csstools/css-tokenizer@3.0.4) '@csstools/css-tokenizer': 3.0.4 postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-dir-pseudo-class@9.0.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-discard-comments@6.0.2(postcss@8.5.23): dependencies: @@ -13177,7 +13179,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: @@ -13189,12 +13191,12 @@ snapshots: postcss-focus-visible@10.0.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-focus-within@9.0.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-font-variant@5.0.0(postcss@8.5.23): dependencies: @@ -13252,7 +13254,7 @@ snapshots: 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: @@ -13276,7 +13278,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: @@ -13286,13 +13288,13 @@ snapshots: dependencies: icss-utils: 5.1.0(postcss@8.5.23) postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-value-parser: 4.2.0 postcss-modules-scope@3.2.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-modules-values@4.0.0(postcss@8.5.23): dependencies: @@ -13301,10 +13303,10 @@ snapshots: postcss-nesting@13.0.2(postcss@8.5.23): dependencies: - '@csstools/selector-resolve-nested': 3.1.0(postcss-selector-parser@7.1.0) - '@csstools/selector-specificity': 5.0.0(postcss-selector-parser@7.1.0) + '@csstools/selector-resolve-nested': 3.1.0(postcss-selector-parser@7.1.5) + '@csstools/selector-specificity': 5.0.0(postcss-selector-parser@7.1.5) postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-normalize-charset@6.0.2(postcss@8.5.23): dependencies: @@ -13449,7 +13451,7 @@ snapshots: postcss-pseudo-class-any-link@10.0.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + postcss-selector-parser: 7.1.5 postcss-reduce-idents@6.0.3(postcss@8.5.23): dependencies: @@ -13474,14 +13476,14 @@ snapshots: postcss-selector-not@8.0.1(postcss@8.5.23): dependencies: postcss: 8.5.23 - postcss-selector-parser: 7.1.0 + 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 - postcss-selector-parser@7.1.0: + postcss-selector-parser@7.1.5: dependencies: cssesc: 3.0.0 util-deprecate: 1.0.2 @@ -13500,7 +13502,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: {} @@ -14360,7 +14362,7 @@ snapshots: dependencies: browserslist: 4.28.5 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: From 331cdb3f245e9b4b8f75b7484f156bb1edf7b679 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 23:26:49 -0500 Subject: [PATCH 11/12] fix: generalize the --help flag leak fix to the whole command tree (Round 11) TestRootHelpFunc_RealTree_UnknownSubcommandErrors reappeared in a full local -race -shuffle=on run of the entire cmd package (461s), this time failing on both "toolchain versions --help" and "terraform bogus-subcommand --help" even though Round 10 already reset both implicated tests' own --help flags. Some other test elsewhere in this large package also drives a real `atmos toolchain --help`/`atmos terraform --help` invocation through RootCmd.ExecuteC(), leaking that flag the same way -- per-test whack-a-mole fixes don't scale to "some other test, somewhere in a 461-second package." Fixed at the root: cmd/testing_helpers_test.go's snapshotRootCmdState/restoreRootCmdState (the mechanism behind every test's NewTestKit(t) call) only ever snapshotted/restored RootCmd's own Flags()/PersistentFlags() -- never any subcommand's, even though a real --help invocation against any subcommand parses onto that command's own FlagSet, a package-level singleton no different from RootCmd's. Added walkCommandTree to recursively visit RootCmd and every reachable command, and used it in both the snapshot and restore paths so every command's flags are captured and restored after each NewTestKit-protected test. cmdStateSnapshot.flags changed from map[string]flagSnapshot to map[*cobra.Command]map[string]flagSnapshot; the three tests in testing_helpers_snapshot_test.go that read the field directly were updated to index by RootCmd explicitly. Also confirmed two separate, pre-existing issues unrelated to this incident (documented in the fix-log's Follow-ups, not fixed here): pkg/provisioner's TestAutoProvisionBackendWritesWarningsToOutputWriter only fails when run in isolation via -run, never as part of the full package (as real CI always runs it); and cmd/root_test.go's two TestApplyCIGitCloneBootstrap_* tests hit a still-unidentified atmosConfig.CI.Enabled leak from some other test in the large cmd package, never seen in any real CI run's failure list across this whole incident. Validation: go build/go vet/golangci-lint clean. go test -race -shuffle=on ./cmd/ -timeout 900s (the entire package, no -run filter, matching the exact shape that exposed the --help leak) completed in 461s with zero failures. A full local run of the entire package set (minus tests/) completed with 390 of 391 packages passing -- the one remaining failure is the separate, already-documented atmosConfig.CI.Enabled leak above, not the --help leak this round fixed. Co-Authored-By: Claude Sonnet 5 --- NOTICE | 26 ++- cmd/ai/skill/install_test.go | 18 +- cmd/ai/skill/ui_test.go | 20 ++ cmd/ci/validate_test.go | 10 +- cmd/root_help_routing_test.go | 19 ++ cmd/testing_helpers_snapshot_test.go | 12 +- cmd/testing_helpers_test.go | 94 +++++--- ...026-09-01-race-detector-ci-job-timeouts.md | 213 ++++++++++++++++++ go.mod | 24 +- go.sum | 46 ++-- internal/tui/utils/utils.go | 31 ++- pkg/auth/identities/aws/credentials_loader.go | 36 ++- .../identities/azure/subscription_test.go | 13 +- pkg/generator/generator_test.go | 17 ++ 14 files changed, 464 insertions(+), 115 deletions(-) diff --git a/NOTICE b/NOTICE index e2dee243e1..2a970644f1 100644 --- a/NOTICE +++ b/NOTICE @@ -327,7 +327,7 @@ APACHE 2.0 LICENSED DEPENDENCIES - github.com/go-logr/logr License: Apache-2.0 - URL: https://github.com/go-logr/logr/blob/v1.4.3/LICENSE + URL: https://github.com/go-logr/logr/blob/v1.4.4/LICENSE - github.com/go-logr/stdr License: Apache-2.0 @@ -663,11 +663,11 @@ APACHE 2.0 LICENSED DEPENDENCIES - go.opentelemetry.io/otel License: Apache-2.0 - URL: https://github.com/open-telemetry/opentelemetry-go/blob/v1.43.0/LICENSE + URL: https://github.com/open-telemetry/opentelemetry-go/blob/v1.46.0/LICENSE - go.opentelemetry.io/otel/metric License: Apache-2.0 - URL: https://github.com/open-telemetry/opentelemetry-go/blob/metric/v1.43.0/metric/LICENSE + URL: https://github.com/open-telemetry/opentelemetry-go/blob/metric/v1.46.0/metric/LICENSE - go.opentelemetry.io/otel/sdk License: Apache-2.0 @@ -679,7 +679,7 @@ APACHE 2.0 LICENSED DEPENDENCIES - go.opentelemetry.io/otel/trace License: Apache-2.0 - URL: https://github.com/open-telemetry/opentelemetry-go/blob/trace/v1.43.0/trace/LICENSE + URL: https://github.com/open-telemetry/opentelemetry-go/blob/trace/v1.46.0/trace/LICENSE - go.opentelemetry.io/proto/otlp License: Apache-2.0 @@ -1058,6 +1058,10 @@ BSD LICENSED DEPENDENCIES License: BSD-3-Clause URL: https://github.com/spf13/pflag/blob/v1.0.10/LICENSE + - github.com/stretchr/testify/internal/difflib + License: BSD-3-Clause + URL: https://github.com/stretchr/testify/blob/v1.12.1/internal/difflib/LICENSE + - github.com/ulikunitz/xz License: BSD-3-Clause URL: https://github.com/ulikunitz/xz/blob/v0.5.15/LICENSE @@ -1084,7 +1088,7 @@ BSD LICENSED DEPENDENCIES - golang.org/x/crypto License: BSD-3-Clause - URL: https://cs.opensource.google/go/x/crypto/+/v0.54.0:LICENSE + URL: https://cs.opensource.google/go/x/crypto/+/v0.55.0:LICENSE - golang.org/x/exp License: BSD-3-Clause @@ -1092,15 +1096,15 @@ BSD LICENSED DEPENDENCIES - golang.org/x/image License: BSD-3-Clause - URL: https://cs.opensource.google/go/x/image/+/v0.43.0:LICENSE + URL: https://cs.opensource.google/go/x/image/+/v0.45.0:LICENSE - golang.org/x/mod/semver License: BSD-3-Clause - URL: https://cs.opensource.google/go/x/mod/+/v0.38.0:LICENSE + URL: https://cs.opensource.google/go/x/mod/+/v0.40.0:LICENSE - golang.org/x/net License: BSD-3-Clause - URL: https://cs.opensource.google/go/x/net/+/v0.57.0:LICENSE + URL: https://cs.opensource.google/go/x/net/+/v0.58.0:LICENSE - golang.org/x/oauth2 License: BSD-3-Clause @@ -1120,7 +1124,7 @@ BSD LICENSED DEPENDENCIES - golang.org/x/text License: BSD-3-Clause - URL: https://cs.opensource.google/go/x/text/+/v0.40.0:LICENSE + URL: https://cs.opensource.google/go/x/text/+/v0.41.0:LICENSE - golang.org/x/time/rate License: BSD-3-Clause @@ -2142,7 +2146,7 @@ MIT LICENSED DEPENDENCIES - github.com/stretchr/testify License: MIT - URL: https://github.com/stretchr/testify/blob/v1.11.1/LICENSE + URL: https://github.com/stretchr/testify/blob/v1.12.1/LICENSE - github.com/subosito/gotenv License: MIT @@ -2250,7 +2254,7 @@ MIT LICENSED DEPENDENCIES - go.yaml.in/yaml/v3 License: MIT - URL: https://github.com/yaml/go-yaml/blob/v3.0.4/LICENSE + URL: https://github.com/yaml/go-yaml/blob/v3.0.5/LICENSE - gopkg.in/yaml.v3 License: MIT diff --git a/cmd/ai/skill/install_test.go b/cmd/ai/skill/install_test.go index 4bbf9eac27..34aa2775c8 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 dc01908050..12012127b1 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/ci/validate_test.go b/cmd/ci/validate_test.go index c426771a3b..86e6f75f9d 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/root_help_routing_test.go b/cmd/root_help_routing_test.go index fb5ed0fc68..9b685c0a74 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/testing_helpers_snapshot_test.go b/cmd/testing_helpers_snapshot_test.go index 5c4408962a..eca802e84f 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 7cf9e3b69c..d6950b7d83 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/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md index 748359f4f2..b2d104cf07 100644 --- a/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md +++ b/docs/fixes/2026-09-01-race-detector-ci-job-timeouts.md @@ -451,3 +451,216 @@ passes finished (`cmd/list` under `-race` takes ~90-110s per single pass locally 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 d386d96f55..b9120a0811 100644 --- a/go.mod +++ b/go.mod @@ -125,7 +125,7 @@ require ( github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 github.com/spf13/viper v1.21.0 - github.com/stretchr/testify v1.11.1 + github.com/stretchr/testify v1.12.1 github.com/terraform-docs/terraform-docs v0.24.0 github.com/tliron/glsp v0.2.2 github.com/versent/saml2aws/v2 v2.36.19 @@ -134,11 +134,11 @@ require ( github.com/zalando/go-keyring v0.2.8 github.com/zclconf/go-cty v1.18.1 go.uber.org/mock v0.6.0 - go.yaml.in/yaml/v3 v3.0.4 + go.yaml.in/yaml/v3 v3.0.5 golang.org/x/oauth2 v0.36.0 golang.org/x/sync v0.22.0 golang.org/x/term v0.45.0 - golang.org/x/text v0.40.0 + 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 @@ -262,7 +262,7 @@ require ( github.com/go-jose/go-jose/v3 v3.0.5 // indirect github.com/go-jose/go-jose/v4 v4.1.4 // indirect github.com/go-logfmt/logfmt v0.6.1 // indirect - github.com/go-logr/logr v1.4.3 // indirect + github.com/go-logr/logr v1.4.4 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-stack/stack v1.8.1 // indirect github.com/goccy/go-json v0.10.6 // indirect @@ -429,11 +429,11 @@ require ( go.opentelemetry.io/contrib/detectors/gcp v1.43.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.43.0 // indirect - go.opentelemetry.io/otel/metric v1.43.0 // indirect + go.opentelemetry.io/otel v1.46.0 // indirect + go.opentelemetry.io/otel/metric v1.46.0 // indirect go.opentelemetry.io/otel/sdk v1.43.0 // indirect go.opentelemetry.io/otel/sdk/metric v1.43.0 // indirect - go.opentelemetry.io/otel/trace v1.43.0 // indirect + go.opentelemetry.io/otel/trace v1.46.0 // indirect go.shabbyrobe.org/gocovmerge v0.0.0-20230507111327-fa4f82cfbf4d // indirect go.uber.org/atomic v1.11.0 // indirect go.yaml.in/yaml/v2 v2.4.4 // indirect @@ -442,13 +442,13 @@ require ( go4.org/netipx v0.0.0-20231129151722-fdeea329fbba // indirect go4.org/unsafe/assume-no-moving-gc v0.0.0-20231121144256-b99613f794b6 // indirect gocloud.dev v0.41.0 // indirect; Pinned: gomplate/v3's s3blob code references s3blob.URLOpener.ConfigProvider, which was removed in gocloud.dev v0.42+. Bump when gomplate/v3 updates or when we migrate the gomplate/v3 usages to gomplate/v4. - golang.org/x/crypto v0.54.0 + golang.org/x/crypto v0.55.0 golang.org/x/exp v0.0.0-20260508232706-74f9aab9d74a // indirect - golang.org/x/mod v0.38.0 // indirect - golang.org/x/net v0.57.0 // indirect + golang.org/x/mod v0.40.0 // indirect + golang.org/x/net v0.58.0 // indirect golang.org/x/sys v0.47.0 golang.org/x/time v0.15.0 // indirect - golang.org/x/tools v0.48.0 // indirect + 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 @@ -488,7 +488,7 @@ require ( github.com/minamijoyo/hcledit v0.2.18 github.com/mxschmitt/playwright-go v0.6100.0 github.com/updatecli/updatecli v0.999.0 - golang.org/x/image v0.43.0 + golang.org/x/image v0.45.0 gopkg.in/op/go-logging.v1 v1.0.0-20160211212156-b2cb9fa56473 ) diff --git a/go.sum b/go.sum index e036174a51..7dbb88dd90 100644 --- a/go.sum +++ b/go.sum @@ -586,8 +586,8 @@ github.com/go-logfmt/logfmt v0.5.0/go.mod h1:wCYkCAKZfumFQihp8CzCvQ3paCTfi41vtzG github.com/go-logfmt/logfmt v0.6.1 h1:4hvbpePJKnIzH1B+8OR/JPbTx37NktoI9LE2QZBBkvE= github.com/go-logfmt/logfmt v0.6.1/go.mod h1:EV2pOAQoZaT1ZXZbqDl5hrymndi4SY9ED9/z6CO0XAk= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= -github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= -github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-logr/logr v1.4.4 h1:tG4xh9yMsRCAiodLVTxyrkzSZ9+o0L1Kg/+cPVcbP/8= +github.com/go-logr/logr v1.4.4/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ= @@ -1357,8 +1357,9 @@ github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o github.com/stretchr/testify v1.8.2/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= -github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= +github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= github.com/subosito/gotenv v1.6.0 h1:9NlTDc1FTs4qu0DDq7AEtTPNw6SVm7uBMsUCUjABIf8= github.com/subosito/gotenv v1.6.0/go.mod h1:Dk4QP5c2W3ibzajGcXpNraDfq2IrhjMIvMSWPKKo0FU= github.com/tchap/go-patricia/v2 v2.3.3 h1:xfNEsODumaEcCcY3gI0hYPZ/PcpVv5ju6RMAhgwZDDc= @@ -1500,8 +1501,8 @@ go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.6 go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.68.0/go.mod h1:Sje3i3MjSPKTSPvVWCaL8ugBzJwik3u4smCjUeuupqg= go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.68.0 h1:CqXxU8VOmDefoh0+ztfGaymYbhdB/tT3zs79QaZTNGY= go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.68.0/go.mod h1:BuhAPThV8PBHBvg8ZzZ/Ok3idOdhWIodywz2xEcRbJo= -go.opentelemetry.io/otel v1.43.0 h1:mYIM03dnh5zfN7HautFE4ieIig9amkNANT+xcVxAj9I= -go.opentelemetry.io/otel v1.43.0/go.mod h1:JuG+u74mvjvcm8vj8pI5XiHy1zDeoCS2LB1spIq7Ay0= +go.opentelemetry.io/otel v1.46.0 h1:FHt5/CDyVxi/8IM1CH7VE/rRgq3kLHa2mSTVMO8AWyc= +go.opentelemetry.io/otel v1.46.0/go.mod h1:Gj3SEScelsNC45tp4nSxRYlS+f5iez7W8XPMCt905kE= go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc v0.18.0 h1:deI9UQMoGFgrg5iLPgzueqFPHevDl+28YKfSpPTI6rY= go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc v0.18.0/go.mod h1:PFx9NgpNUKXdf7J4Q3agRxMs3Y07QhTCVipKmLsMKnU= go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp v0.19.0 h1:HIBTQ3VO5aupLKjC90JgMqpezVXwFuq6Ryjn0/izoag= @@ -1526,16 +1527,16 @@ go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.42.0 h1:s/1iRkCKDfhlh1J go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.42.0/go.mod h1:UI3wi0FXg1Pofb8ZBiBLhtMzgoTm1TYkMvn71fAqDzs= go.opentelemetry.io/otel/log v0.19.0 h1:KUZs/GOsw79TBBMfDWsXS+KZ4g2Ckzksd1ymzsIEbo4= go.opentelemetry.io/otel/log v0.19.0/go.mod h1:5DQYeGmxVIr4n0/BcJvF4upsraHjg6vudJJpnkL6Ipk= -go.opentelemetry.io/otel/metric v1.43.0 h1:d7638QeInOnuwOONPp4JAOGfbCEpYb+K6DVWvdxGzgM= -go.opentelemetry.io/otel/metric v1.43.0/go.mod h1:RDnPtIxvqlgO8GRW18W6Z/4P462ldprJtfxHxyKd2PY= +go.opentelemetry.io/otel/metric v1.46.0 h1:yBnkXvgV7AXFILZc5K6IZe/CBFF3OS7BJ8ov6/lj0K8= +go.opentelemetry.io/otel/metric v1.46.0/go.mod h1:iPmdWqifKUdzziPkvvzIJXITl56fQx2mGM/DHLB3/2o= go.opentelemetry.io/otel/sdk v1.43.0 h1:pi5mE86i5rTeLXqoF/hhiBtUNcrAGHLKQdhg4h4V9Dg= go.opentelemetry.io/otel/sdk v1.43.0/go.mod h1:P+IkVU3iWukmiit/Yf9AWvpyRDlUeBaRg6Y+C58QHzg= go.opentelemetry.io/otel/sdk/log v0.19.0 h1:scYVLqT22D2gqXItnWiocLUKGH9yvkkeql5dBDiXyko= go.opentelemetry.io/otel/sdk/log v0.19.0/go.mod h1:vFBowwXGLlW9AvpuF7bMgnNI95LiW10szrOdvzBHlAg= go.opentelemetry.io/otel/sdk/metric v1.43.0 h1:S88dyqXjJkuBNLeMcVPRFXpRw2fuwdvfCGLEo89fDkw= go.opentelemetry.io/otel/sdk/metric v1.43.0/go.mod h1:C/RJtwSEJ5hzTiUz5pXF1kILHStzb9zFlIEe85bhj6A= -go.opentelemetry.io/otel/trace v1.43.0 h1:BkNrHpup+4k4w+ZZ86CZoHHEkohws8AY+WTX09nk+3A= -go.opentelemetry.io/otel/trace v1.43.0/go.mod h1:/QJhyVBUUswCphDVxq+8mld+AvhXZLhe+8WVFxiFff0= +go.opentelemetry.io/otel/trace v1.46.0 h1:OULy7ccdJnZtJ0UDYFOIGaCmiWzJ8Vi2G/Rsu60qs1c= +go.opentelemetry.io/otel/trace v1.46.0/go.mod h1:J7GAXweO77XSFkB/rmAqk9D6ihszhFjLU+d9WuUxDLI= go.opentelemetry.io/proto/otlp v1.10.0 h1:IQRWgT5srOCYfiWnpqUYz9CVmbO8bFmKcwYxpuCSL2g= go.opentelemetry.io/proto/otlp v1.10.0/go.mod h1:/CV4QoCR/S9yaPj8utp3lvQPoqMtxXdzn7ozvvozVqk= go.shabbyrobe.org/gocovmerge v0.0.0-20230507111327-fa4f82cfbf4d h1:Ns9kd1Rwzw7t0BR8XMphenji4SmIoNZPn8zhYmaVKP8= @@ -1555,8 +1556,9 @@ go.uber.org/zap v1.27.1 h1:08RqriUEv8+ArZRYSTXy1LeBScaMpVSTBhCeaZYfMYc= go.uber.org/zap v1.27.1/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= go.yaml.in/yaml/v2 v2.4.4 h1:tuyd0P+2Ont/d6e2rl3be67goVK4R6deVxCUX5vyPaQ= go.yaml.in/yaml/v2 v2.4.4/go.mod h1:gMZqIpDtDqOfM0uNfy0SkpRhvUryYH0Z6wdMYcacYXQ= -go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= +go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= go.yaml.in/yaml/v4 v4.0.0-rc.3 h1:3h1fjsh1CTAPjW7q/EMe+C8shx5d8ctzZTrLcs/j8Go= go.yaml.in/yaml/v4 v4.0.0-rc.3/go.mod h1:aZqd9kCMsGL7AuUv/m/PvWLdg5sjJsZ4oHDEnfPPfY0= go4.org/intern v0.0.0-20211027215823-ae77deb06f29/go.mod h1:cS2ma+47FKrLPdXFpr7CuxiTW3eyJbWew4qx0qtQWDA= @@ -1581,15 +1583,15 @@ golang.org/x/crypto v0.13.0/go.mod h1:y6Z2r+Rw4iayiXXAIxJIDAJ1zMW4yaTpebo8fPOliY golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8= golang.org/x/crypto v0.31.0/go.mod h1:kDsLvtWBEx7MV9tJOj9bnXsPbxwJQ6csT/x4KIN4Ssk= -golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= -golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= +golang.org/x/crypto v0.55.0 h1:+KWHjbgOaAQ66dh/YlkZKHlz9ZUlq61AFirAR9ntP8M= +golang.org/x/crypto v0.55.0/go.mod h1:uq0V9dE/fzQuJtbnL+2EhWOE63vo164FY8xqEnV9xis= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20260508232706-74f9aab9d74a h1:+3jdDGGB8NGb1Zktc737jlt3/A5f6UlwSzmvqUuufxw= golang.org/x/exp v0.0.0-20260508232706-74f9aab9d74a/go.mod h1:d2fgXJLVs4dYDHUk5lwMIfzRzSrWCfGZb0ZqeLa/Vcw= golang.org/x/exp/typeparams v0.0.0-20260312153236-7ab1446f8b90 h1:cfW8UCYSVdPblxA7qQe3o5Iad55Vsx4BFmuGS9RNOmc= golang.org/x/exp/typeparams v0.0.0-20260312153236-7ab1446f8b90/go.mod h1:PqrXSW65cXDZH0k4IeUbhmg/bcAZDbzNz3byBpKCsXo= -golang.org/x/image v0.43.0 h1:FLxcP4ec2350nTfOC8ysKtqYSIFbk/QGjw1ZHNP4tsY= -golang.org/x/image v0.43.0/go.mod h1:rrpelvGFt+kLPAjPM4HeWPgrl0FtafueU//e5N0qk/Q= +golang.org/x/image v0.45.0 h1:FMb1nTbH5H9vF55SriQHgFw5GnNL9Jg6L25BwXKzhB0= +golang.org/x/image v0.45.0/go.mod h1:n62x/7RqlwXDvGsSU4u6IUTUf6KghUZ9Bt7cG/T9Fx4= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU= golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= @@ -1600,8 +1602,8 @@ golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/mod v0.38.0 h1:MECBjubtXD7yj4HrhIUcywNaGeNVUdfVnxmPajOk4yk= -golang.org/x/mod v0.38.0/go.mod h1:V6Xz0pq8TQ3dGqVQ1FVHuelZpAL0uNhSkk9ogYP3c40= +golang.org/x/mod v0.40.0 h1:hUv+3cXcdRHz08UmSiOob7sadHig73uo5bkXxQ/tvUs= +golang.org/x/mod v0.40.0/go.mod h1:0/weTWkPWGBikyTWAX3dkjVztMmBA5hM0DH6BElSupE= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20181114220301-adae6a3d119a/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -1628,8 +1630,8 @@ golang.org/x/net v0.15.0/go.mod h1:idbUs1IY1+zTqbi8yxTbhexhEEk5ur9LInksu6HrEpk= golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= golang.org/x/net v0.33.0/go.mod h1:HXLR5J+9DxmrqMwG9qjGCxZ+zKXxBru04zlTvWlWuN4= -golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= -golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= +golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= +golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= @@ -1718,8 +1720,8 @@ golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.21.0/go.mod h1:4IBbMaMmOPCJ8SecivzSH54+73PCFmPWxNTLm+vZkEQ= -golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= -golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= +golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= +golang.org/x/text v0.41.0/go.mod h1:jvf1O8ajNzZqhSrQBPbutR/EB83Cc0CFrezNQIwbb5M= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= @@ -1738,8 +1740,8 @@ golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= golang.org/x/tools v0.13.0/go.mod h1:HvlwmtVNQAhOuCjW7xxvovg8wbNq7LwfXh/k7wXUl58= golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk= -golang.org/x/tools v0.48.0 h1:3+hClM1aLL5mjMKm5ovokw9epgRXPuu2tILgismM6RE= -golang.org/x/tools v0.48.0/go.mod h1:08xX0orndb/F7jJxGDicx061tyd5pcMto75YMAXr6lk= +golang.org/x/tools v0.49.0 h1:3NI7VXzL9+1WZD52Dx2ttoPwD5DWrFGpl9mFZDlmisI= +golang.org/x/tools v0.49.0/go.mod h1:SJNXV9DBKT0UbdttsQjbfJlAE/q+y36++zo3uL3N0Oo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/internal/tui/utils/utils.go b/internal/tui/utils/utils.go index 68e74fe28b..d8e45b58e6 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 9e7b9a9243..57bfa2d892 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 2c5c274f11..42500b2e70 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/generator/generator_test.go b/pkg/generator/generator_test.go index e0e366b873..596e4d73ae 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 From 310f20c8e4cc7418735d1bc0ab3424b6111b79e8 Mon Sep 17 00:00:00 2001 From: Erik Osterman Date: Tue, 1 Sep 2026 23:44:52 -0500 Subject: [PATCH 12/12] fix(security): remediate 3 new Dependabot alerts (grpc, browserslist x2) - google.golang.org/grpc v1.82.1 -> v1.83.1 (GHSA covering heap memory exhaustion via HTTP/2 DATA frame fragmentation, patched in 1.83.1). - browserslist (transitive, both the 4.27.x and 4.28.x lines present in the lockfile) -> 4.28.8 via a pnpm override, covering two advisories: unbounded memory growth from unbounded query-result caching, and a crash/prototype-write via untrusted browserslist-stats.json custom stats. All three are minor/patch bumps, allowed by dependabot.yml's major-only ignore rule. Regenerated NOTICE via the new `mage notice:generate` target (this branch's merge from main replaced scripts/generate-notice.sh with tools/noticegen). Co-Authored-By: Claude Sonnet 5 --- NOTICE | 12 ++-- go.mod | 12 ++-- go.sum | 24 +++---- website/package.json | 1 + website/pnpm-lock.yaml | 151 ++++++++++++++++------------------------- 5 files changed, 83 insertions(+), 117 deletions(-) diff --git a/NOTICE b/NOTICE index 48d69b7f85..bab2419bd6 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/go.mod b/go.mod index a2ceaa7672..1e4b99a4ae 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 54b0ce1c97..b362a2c705 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/website/package.json b/website/package.json index a7fc596e58..276700e040 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", diff --git a/website/pnpm-lock.yaml b/website/pnpm-lock.yaml index 0550443589..e75a47d706 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 @@ -916,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'} @@ -2650,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 @@ -2696,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 @@ -2766,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==} @@ -3473,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==} @@ -4826,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: @@ -6359,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==} @@ -6854,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 @@ -6862,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 @@ -7572,8 +7555,6 @@ snapshots: transitivePeerDependencies: - supports-color - '@babel/runtime@7.28.4': {} - '@babel/runtime@7.29.7': {} '@babel/template@7.27.2': @@ -9311,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': {} @@ -9325,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) @@ -9335,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)': @@ -9346,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)': @@ -9357,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)': @@ -9390,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 @@ -9451,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 @@ -9468,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) @@ -9483,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) @@ -9498,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 @@ -9511,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) @@ -9525,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)': @@ -9536,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 @@ -9556,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)': @@ -10252,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 @@ -10313,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: {} @@ -10382,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: {} @@ -10452,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: {} @@ -10654,7 +10629,7 @@ snapshots: core-js-compat@3.46.0: dependencies: - browserslist: 4.27.0 + browserslist: 4.28.8 core-js@2.6.12: {} @@ -10775,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) @@ -10785,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 @@ -11184,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: {} @@ -12840,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: {} @@ -13122,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 @@ -13130,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 @@ -13254,7 +13225,7 @@ 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 @@ -13274,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 @@ -13343,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 @@ -13420,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) @@ -13464,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 @@ -14364,7 +14335,7 @@ 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.4 @@ -14543,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 @@ -14759,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