diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 9e6d70c..d53cce8 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -125,6 +125,15 @@ Two things make this repo unusual and should shape every finding: pinned standalone binaries — `errcheck`, `gofmt -s`, `goimports`, `ineffassign`, `misspell`, `staticcheck`, plus `deadcode-check.sh`, `file-budget.sh`, `check-style.sh`. Don't infer coverage from that file. +- **The two formatters run via `make fmt-check`, not inline in the workflow** (cli#549), and + they scope to `git ls-files '*.go'` rather than `.`. Both are deliberate: `.` walked untracked + scratch directories, and one definition of the file set is what stops local and CI + disagreeing. `scripts/format.sh` fails closed (exit 2) outside a work tree or on an empty file + list — do not "simplify" either guard away. `run_formatter` returns a status and + writes to a temp file rather than being captured in `$( )`: a function that + `exit`s inside a command substitution ends only the subshell, and the first cut + of this script shipped exactly that false green. `make fmt-selftest` + (`scripts/tests/format-verify.sh`) is the guard; it fails on the old shape. - **`staticcheck` runs `-checks all,-ST1005` deliberately** — do not flag error-string capitalisation or punctuation. It is a tracked, intentional exclusion (cli#279). - `internal/submit/client.go:78` — `InsecureSkipVerify` is intentional for cluster-internal diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ed2b875..3c6110b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -48,15 +48,48 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: shellcheck + dash parse + # NO `apt-get` (cli#534). Both tools are already on `ubuntu-latest`: + # `shellcheck` is preinstalled -- tracebloc/.github's own `quality / shellcheck` + # job, a REQUIRED check in 16 repos, calls `shellcheck --version` with no + # install at all -- and `dash` IS Ubuntu's `/bin/sh`, an essential package. + # + # WHY IT HAD TO GO, and it is not tidiness. This step is the first thing in a + # REQUIRED check, and `apt-get` here had no retry and no time bound of its own, + # so a slow package mirror consumed the whole 10-minute job budget before any + # shell was parsed. Measured on cli#533 -- a workflow-only diff that cannot + # touch installer behaviour -- which failed FOUR consecutive times: + # + # job 96126585157 Installer (shell) failure 10m16s + # 15:34 shellcheck + dash parse <- 10 minutes here, then killed + # + # Nothing after the `apt-get` line ever ran, and the annotation said + # `Installer (shell)` exceeded 10m -- pointing whoever reads it at the + # installer rather than at package fetching. + # + # Removing the dependency beats hardening it: a step that installs nothing + # cannot stall on a mirror, and no retry/timeout wrapper can say that. + # + # THIS PR'S OWN RUN IS THE PROOF. If either tool were absent the step fails + # loudly on the first line, here, before merge -- which is a better check than + # any claim in this comment. run: | - sudo apt-get update -qq && sudo apt-get install -y -qq shellcheck dash + shellcheck --version | head -2 shellcheck --shell=sh --severity=error scripts/install.sh shellcheck --shell=bash --severity=error scripts/check-style.sh shellcheck --shell=bash --severity=error scripts/check-tool-pins.sh + shellcheck --shell=bash --severity=error scripts/format.sh + shellcheck --shell=bash --severity=error scripts/tests/format-verify.sh dash -n scripts/install.sh bash -n scripts/tests/install-verify.sh shellcheck --shell=bash --severity=error scripts/tests/install-ps1-verify.sh bash -n scripts/tests/install-ps1-verify.sh + # format.sh's own fail-closed properties. Formatters are stubbed, so this is + # hermetic and needs no Go toolchain — which is why it lives in this job + # rather than Lint. It exists because the first cut of format.sh reported + # "clean" on a formatter that never ran (#550 review). + - name: Formatter-gate harness (fail-closed / tracked-files scope) + run: bash scripts/tests/format-verify.sh + - name: Verification harness (mandatory cosign / fail-closed) run: bash scripts/tests/install-verify.sh # Same property on Windows (backend#2078). pwsh is preinstalled on the @@ -128,29 +161,18 @@ jobs: go install github.com/kisielk/errcheck@v1.20.0 errcheck ./... - - name: gofmt -s - run: | - drift="$(gofmt -s -l .)" - if [ -n "$drift" ]; then - echo "::error::gofmt -s drift in:" - echo "$drift" | sed 's/^/ /' - echo "::error::run \`make fmt\` to fix" - exit 1 - fi - - # goimports -local: enforce the stdlib / third-party / our-own import - # grouping that .golangci.yml's local-prefixes already declares. gofmt - # doesn't check grouping, so drift accumulated silently until now. - - name: goimports -local - run: | - go install golang.org/x/tools/cmd/goimports@v0.48.0 - drift="$(goimports -local github.com/tracebloc/cli -l .)" - if [ -n "$drift" ]; then - echo "::error::goimports (import grouping) drift in:" - echo "$drift" | sed 's/^/ /' - echo "::error::run \`make fmt\` to fix" - exit 1 - fi + # gofmt -s (simplification) + goimports -local (the stdlib / third-party / + # our-own import grouping that .golangci.yml's local-prefixes declares; + # gofmt does not check grouping). + # + # `make fmt-check`, not an inline copy: both formatters now scope to + # `git ls-files '*.go'` instead of `.` (cli#549), and a second inline copy + # of that scope here is how local and CI start disagreeing about which + # files are gated. It also drops the restated goimports pin — the version + # is declared once, by GOIMPORTS_VERSION in the Makefile, which is what + # check-tool-pins.sh now enforces for this tool too. + - name: gofmt -s + goimports -local + run: make fmt-check - name: ineffassign run: | diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 6b71166..5fb9839 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -73,6 +73,13 @@ jobs: # broke the copy assertions without touching delete.go. The glob # internal/cli/delete*.go deliberately excludes data_delete*.go # (`tracebloc data delete` is a different command, unit-tested). + # + # internal/cli/telemetry*.go is here for the same reason as + # internal/ui, and backend#2314 is the proof: the command-outcome + # event is emitted from main.go AFTER the offboard returns, and its + # spool lives inside the ~/.tracebloc the offboard just deleted — so + # a telemetry change re-created the wiped tree and broke the teardown + # suite's config-dir assertion without touching delete.go at all. filters: | e2e: - '.github/workflows/e2e.yml' @@ -82,6 +89,7 @@ jobs: - 'cmd/**' - 'test/integration/**' - 'internal/cli/delete*.go' + - 'internal/cli/telemetry*.go' - 'internal/nodeboot/**' - 'internal/api/**' - 'internal/config/**' diff --git a/.github/workflows/envelope-contract-drift.yml b/.github/workflows/envelope-contract-drift.yml new file mode 100644 index 0000000..8f18058 --- /dev/null +++ b/.github/workflows/envelope-contract-drift.yml @@ -0,0 +1,147 @@ +name: Envelope contract drift (cross-repo) + +# internal/resources/envelope_contract.json is VENDORED from +# tracebloc/client-runtime (backend#2220, RFC-BACKEND-664 §P0). client-runtime +# owns the training-envelope arithmetic +# (node_sizing.envelope_from_allocatable); this repo, the bash installer and its +# PowerShell twin are readers of it. Before that consolidation the same policy +# was typed out in all three, none derived from the others — and they disagreed: +# set.go ranked candidate nodes (cpu, memory) while the bash installer ranked +# them (memory, cpu), so on a cluster of 8c/16Gi + 4c/32Gi `resources set` and +# the installer anchored on DIFFERENT nodes. +# +# Unlike the installers, Go needs no generator: the contract is embedded verbatim +# with go:embed, so the vendored artifact is byte-identical to upstream and this +# gate is a plain diff. internal/resources/contract_test.go replays the +# contract's golden vectors through MaxRunCores/MaxRunGiB on every PR; this job +# is the other half — it catches the contract itself going stale. +# +# Pin, don't float (scripts/.client-runtime-ref), exactly as this repo already +# does for tracebloc/client and tracebloc/data-ingestors: an unrelated upstream +# commit must not redden every open CLI PR, and the weekly run catches a pin gone +# stale enough to matter. +# +# FAIL-CLOSED. client-runtime is private, so this needs a token GITHUB_TOKEN +# cannot provide; when it cannot read upstream the job FAILS rather than warning +# and exiting 0. A check that never executed must not report as a passing one — +# the activation-phase fail-open cli#536 had to remove from the backend-fixtures +# gate for exactly this reason. + +on: + schedule: + - cron: "0 6 * * 1" # weekly Monday, offset from chart-drift (05:00) + workflow_dispatch: + pull_request: + branches: [develop, main] + paths: + - "internal/resources/**" + - "scripts/.client-runtime-ref" + - ".github/workflows/envelope-contract-drift.yml" + +permissions: + contents: read + +jobs: + envelope-contract: + timeout-minutes: 10 + name: Envelope contract gate (pinned client-runtime ref) + runs-on: ubuntu-latest + steps: + - name: Checkout this CLI ref + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + path: cli + + - name: Resolve the pinned client-runtime ref + id: pin + # First non-comment, non-blank line of scripts/.client-runtime-ref — the + # same convention .client-ref and .data-ingestors-ref use. Shape is + # validated (SHA/branch/tag characters only, no "..") before it reaches + # the checkout action. + run: | + ref="$(grep -vE '^[[:space:]]*(#|$)' cli/scripts/.client-runtime-ref | head -1 | tr -d '[:space:]')" + if [ -z "$ref" ]; then + echo "::error file=scripts/.client-runtime-ref::no ref found — the first non-comment line must be a commit SHA" + exit 1 + fi + if ! printf '%s' "$ref" | grep -qE '^[A-Za-z0-9][A-Za-z0-9._/-]*$' || printf '%s' "$ref" | grep -q '\.\.'; then + echo "::error file=scripts/.client-runtime-ref::invalid ref shape: $ref" + exit 1 + fi + echo "ref=$ref" >> "$GITHUB_OUTPUT" + + - name: Mint a read-only installation token for client-runtime + id: token + # Least privilege per the backend#2157 sweep: named `repositories`, not + # owner-wide, and contents:read only — this job reads two files. + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ secrets.RELEASE_TRAIN_APP_ID }} + private-key: ${{ secrets.RELEASE_TRAIN_APP_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} + repositories: client-runtime + permission-contents: read + + - name: Checkout tracebloc/client-runtime @ pinned ref + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + repository: tracebloc/client-runtime + ref: ${{ steps.pin.outputs.ref }} + token: ${{ steps.token.outputs.token }} + path: client-runtime + persist-credentials: false + + - name: The vendored contract matches upstream, byte for byte + env: + PINNED_REF: ${{ steps.pin.outputs.ref }} + run: | + upstream="client-runtime/envelope_contract.json" + vendored="cli/internal/resources/envelope_contract.json" + if [ ! -f "$upstream" ]; then + echo "::error::$upstream is missing at $PINNED_REF — has the contract moved or been renamed?" + exit 1 + fi + if ! diff -u "$vendored" "$upstream"; then + echo "::error file=internal/resources/envelope_contract.json::the vendored envelope contract has drifted from tracebloc/client-runtime@$PINNED_REF" + echo "" + echo "To adopt the upstream change:" + echo " 1. cp /envelope_contract.json internal/resources/" + echo " 2. update the SHA in scripts/.client-runtime-ref" + echo " 3. go test ./internal/resources/... # the golden vectors WILL have moved" + echo "" + echo "If the overhead or the floors moved, that is a FLEET envelope change" + echo "(backend#2167, RFC-BACKEND-664 L0) — not a re-vendor. Say so on the PR." + exit 1 + fi + echo "vendored contract matches client-runtime@$PINNED_REF" + + - name: Upstream's own goldens are not stale against its own arithmetic + # A vendored contract can match upstream byte-for-byte while UPSTREAM's + # vectors have gone stale against upstream's code — in which case we are + # faithfully mirroring a lie. Re-derive them from client-runtime's own + # generator and require no diff. Pure-python, no cluster, no deps. + run: | + cd client-runtime + if [ ! -f scripts/gen_envelope_vectors.py ]; then + echo "::error::client-runtime@${{ steps.pin.outputs.ref }} has no scripts/gen_envelope_vectors.py — the contract's provenance cannot be verified" + exit 1 + fi + python3 scripts/gen_envelope_vectors.py + if ! git diff --exit-code -- envelope_contract.json; then + echo "::error::client-runtime@${{ steps.pin.outputs.ref }} carries goldens that its own generator does not reproduce." + echo "The pinned ref is not self-consistent — fix it upstream, then re-vendor and re-pin here." + exit 1 + fi + echo "upstream goldens reproduce from upstream code" + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: cli/go.mod + cache: true + cache-dependency-path: cli/go.sum + + - name: The CLI still agrees with the contract it vendored + run: | + cd cli + go test ./internal/resources/... -run 'Contract|Golden|DecisionA|Overhead|FloorText' -v diff --git a/.github/workflows/stale-backlog-caller.yml b/.github/workflows/stale-backlog-caller.yml new file mode 100644 index 0000000..022bb5c --- /dev/null +++ b/.github/workflows/stale-backlog-caller.yml @@ -0,0 +1,33 @@ +name: Close stale backlog issues + +# THIN CALLER (backend#1979). The sweep used to be a 16-way copy of +# `actions/stale`, which could not be given board awareness: eligibility has to +# read the card's `Status`, and a script cannot be maintained as sixteen +# byte-identical copies (backend#1597 item 1). +# +# The reusable it calls replaces `actions/stale` with `stale-backlog.py`, whose +# eligibility is exactly `Backlog` — so a `North Stars` epic, or anything already +# in the pipeline, can no longer be auto-closed by a sweep that could not see +# which column it was in. +# +# NO INPUTS PASSED ON PURPOSE. Every input the callee declares is defaulted +# (`project-number: 2`, `dry-run: false`, `strict: false`, `script-ref: main`), +# and a caller may only pass inputs the `@main` callee declares — passing one it +# does not have kills the run at startup_failure, which is why the callee had to +# land on `main` before these callers could be armed at all. +on: + schedule: + - cron: '0 0 * * 1' # Mondays 00:00 UTC + workflow_dispatch: {} + +# `contents: read` only. The sweep's writes go through the App token minted +# inside the reusable, not through GITHUB_TOKEN — and asking for more here than +# the callee needs would exceed a minimal grant and fail the run at startup with +# no jobs (the same constraint code-quality.yml documents). +permissions: + contents: read + +jobs: + stale: + uses: tracebloc/.github/.github/workflows/stale-backlog.yml@main + secrets: inherit diff --git a/.github/workflows/stale-backlog.yml b/.github/workflows/stale-backlog.yml deleted file mode 100644 index 4e42463..0000000 --- a/.github/workflows/stale-backlog.yml +++ /dev/null @@ -1,33 +0,0 @@ -name: Close stale backlog issues - -on: - schedule: - - cron: '0 0 * * 1' # Mondays 00:00 UTC - workflow_dispatch: {} - -permissions: - issues: write - pull-requests: write - -jobs: - stale: - runs-on: ubuntu-latest - steps: - - uses: actions/stale@4391f3da665fdf50b6810c1a66712fb9ba21aa93 # v11.0.0 - with: - days-before-issue-stale: 42 # 6 weeks of no activity → warning - days-before-issue-close: 14 # +2 weeks of silence → close - stale-issue-label: 'stale' - stale-issue-message: | - 👋 This issue has had no activity for 6 weeks. - - If it's still relevant, please leave a comment with current context (or assign someone). Otherwise it will auto-close in 2 weeks. - - To exempt permanently, add the `keep-open` label. - close-issue-message: | - Closing due to 8+ weeks of inactivity. Please reopen with current context if relevant. - exempt-issue-labels: 'keep-open,blocked' - # PRs: don't auto-stale — branch protection + active reviews govern those - days-before-pr-stale: -1 - days-before-pr-close: -1 - operations-per-run: 50 diff --git a/Makefile b/Makefile index 1600354..89d8fdc 100644 --- a/Makefile +++ b/Makefile @@ -33,7 +33,8 @@ help: @echo " build build ./tracebloc" @echo " install go install ./cmd/tracebloc" @echo - @echo " individual: vet test lint lint-full fmt fmt-check schema-check" + @echo " individual: vet test lint lint-full fmt fmt-check fmt-selftest" + @echo " schema-check" @echo " vulncheck deadcode file-budget check-style clean" @echo " cover cover-integration cover-merge test-integration" @@ -50,7 +51,7 @@ help: # * schema-check — fetches data-ingestors at the pinned ref. # * deadcode — another `go run tool@version` fetch. .PHONY: check -check: vet test-fast fmt-check file-budget check-style check-tool-pins +check: vet test-fast fmt-check fmt-selftest file-budget check-style check-tool-pins @echo "==> check: green (run 'make check-all' for the full CI set)" # check-all: the full PR gate. `ci` is the original name and stays — @@ -132,7 +133,7 @@ GOIMPORTS_VERSION ?= v0.48.0 # which fails on findings since #430. A green `make ci` must imply a green # PR; lint-full's own guard tells you how to install the tool if missing. .PHONY: ci -ci: vet test lint lint-full fmt-check schema-check vulncheck file-budget deadcode check-style check-tool-pins +ci: vet test lint lint-full fmt-check fmt-selftest schema-check vulncheck file-budget deadcode check-style check-tool-pins @echo "==> ci: all green" .PHONY: build @@ -256,29 +257,33 @@ vulncheck: lint-full: $(GO) run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) run +# fmt / fmt-check: gofmt -s (simplification) + goimports -local (import +# grouping: stdlib / third-party / our own — matches .golangci.yml's +# local-prefixes). +# +# Both scope to `git ls-files '*.go'` rather than `.` (cli#549). `.` is the whole +# working TREE, so an untracked scratch directory holding Go files — a nested git +# worktree, a vendored copy, a build sandbox — failed `make check` while every +# tracked file was clean, and `make fmt` then rewrote content the repo does not +# track. build.yml's Lint job calls these same targets, so the file set has one +# definition; see scripts/format.sh for the fail-closed cases. .PHONY: fmt fmt: - gofmt -s -w . - $(GO) run golang.org/x/tools/cmd/goimports@$(GOIMPORTS_VERSION) -local github.com/tracebloc/cli -w . + @GO="$(GO)" GOIMPORTS_VERSION=$(GOIMPORTS_VERSION) ./scripts/format.sh --write -# fmt-check: gofmt -s (simplification) + goimports -local (import grouping: -# stdlib / third-party / our own — matches .golangci.yml's local-prefixes). .PHONY: fmt-check fmt-check: - @diff="$$(gofmt -s -l . 2>/dev/null)"; \ - if [ -n "$$diff" ]; then \ - echo "==> gofmt -s needed on:"; \ - echo "$$diff" | sed 's/^/ /'; \ - echo "==> run \`make fmt\` to fix"; \ - exit 1; \ - fi - @drift="$$($(GO) run golang.org/x/tools/cmd/goimports@$(GOIMPORTS_VERSION) -local github.com/tracebloc/cli -l .)"; \ - if [ -n "$$drift" ]; then \ - echo "==> goimports (import grouping) needed on:"; \ - echo "$$drift" | sed 's/^/ /'; \ - echo "==> run \`make fmt\` to fix"; \ - exit 1; \ - fi + @GO="$(GO)" GOIMPORTS_VERSION=$(GOIMPORTS_VERSION) ./scripts/format.sh --check + +# fmt-selftest: the properties scripts/format.sh must not lose — the formatters +# are stubbed, so it is hermetic and ~6 s. It exists because the FIRST cut of +# format.sh shipped a false green: run_formatter `exit`ed from inside a command +# substitution, which ends only the subshell, so check mode read an empty capture +# and printed "clean" on a formatter that never ran (caught in review on #550). +# A comment cannot hold that shut; this can. +.PHONY: fmt-selftest +fmt-selftest: + @bash scripts/tests/format-verify.sh .PHONY: schema-check schema-check: diff --git a/VERSION b/VERSION index f314d02..ddf1d4a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.9 +0.10.10 diff --git a/docs/cli-navigation.md b/docs/cli-navigation.md index cba1423..6106816 100644 --- a/docs/cli-navigation.md +++ b/docs/cli-navigation.md @@ -138,7 +138,7 @@ flowchart TD FIT{"fits the machine? (+ floors)"} FIT -->|no| re2b(["exit 2 — too big / too small
(macOS: raise Docker Desktop)"]):::fail - FIT -->|"no change"| re0n(["exit 0 — nothing to change"]):::ok + FIT -->|"no change"| re0n(["exit 0 — nothing to change
(may still re-stamp provenance / clear a
phantom GPU — never asks to confirm)"]):::ok FIT -->|ok| CONF{"confirm? (--yes skips)"} CONF -->|"declined / non-TTY, no --yes"| re0c(["exit 0 declined / exit 1 non-TTY"]):::fail CONF -->|yes| PIN{"chart version pinned?"} diff --git a/go.mod b/go.mod index d968e70..034639d 100644 --- a/go.mod +++ b/go.mod @@ -42,9 +42,10 @@ require ( github.com/santhosh-tekuri/jsonschema/v6 v6.0.3 github.com/schollz/progressbar/v3 v3.19.1 github.com/spf13/cobra v1.10.2 + github.com/spf13/pflag v1.0.9 golang.org/x/sys v0.47.0 golang.org/x/term v0.45.0 - golang.org/x/text v0.40.0 + golang.org/x/text v0.41.0 gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.36.3 k8s.io/apimachinery v0.36.3 @@ -76,7 +77,6 @@ require ( github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/rivo/uniseg v0.4.7 // indirect - github.com/spf13/pflag v1.0.9 // indirect github.com/x448/float16 v0.8.4 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect diff --git a/go.sum b/go.sum index 41f0275..273f097 100644 --- a/go.sum +++ b/go.sum @@ -145,8 +145,8 @@ golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= -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.14.0 h1:MRx4UaLrDotUKUdCIqzPC48t1Y9hANFKIRpNx+Te8PI= golang.org/x/time v0.14.0/go.mod h1:eL/Oa2bBBK0TkX57Fyni+NgnyQQN4LitPmob2Hjnqw4= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= diff --git a/internal/cli/auth.go b/internal/cli/auth.go index c4237eb..37b0809 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -411,7 +411,13 @@ func newAuthStatusCmd() *cobra.Command { prof := cfg.Current() p.Section("tracebloc auth") p.Field("status", "signed in") - p.Field("backend", cfg.CurrentEnv) + // sessionEnv, not the raw stored string: this line is the human-facing + // answer to "which backend am I on?", and it must be the same answer + // --check computes and the same one authedClient dials. Printing the + // stored value let `auth status` say `Dev` while every request went to + // dev — a status command that disagrees with the client is worse than + // no status command. + p.Field("backend", sessionEnv(cfg)) if prof.Email != "" { p.Field("account", prof.Email) } @@ -442,7 +448,7 @@ func newAuthStatusCmd() *cobra.Command { // (IsSilentError) so main() prints nothing. // // The target env is resolved exactly like `login` (--env, then $CLIENT_ENV, then -// prod), and must match the signed-in CurrentEnv — otherwise the probe would OK a +// prod), and must match the signed-in env as sessionEnv resolves it — otherwise the probe would OK a // stale session for the wrong backend and the installer would skip the very // `login` that switches env, provisioning into the wrong account (RFC-0001 §10). func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { @@ -454,18 +460,23 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { return &exitError{code: exitFailure} } target := api.ResolveEnv(envFlag) - if !cfg.SignedIn() || cfg.CurrentEnv != target { + // Compare the RESOLVED session env, not the raw cfg.CurrentEnv: target comes + // out of api.ResolveEnv already normalised, so comparing it against the stored + // string made this the one place a `"current_env": "Dev"` config failed a probe + // for the session it is actually signed in to. + signedIn := sessionEnv(cfg) + if !cfg.SignedIn() || signedIn != target { if p.Verbose() { - if cfg.SignedIn() && cfg.CurrentEnv != target { - p.Hintf("Signed in to %q, but this run targets %q — run `tracebloc login`.", cfg.CurrentEnv, target) + if cfg.SignedIn() && signedIn != target { + p.Hintf("Signed in to %q, but this run targets %q — run `tracebloc login`.", signedIn, target) } else { p.Hintf("Not signed in. Run `tracebloc login`.") } } return &exitError{code: exitFailure} } - // Signed in AND CurrentEnv == target: probe it. authedClient() builds the client - // for sessionEnv (== CurrentEnv == target) with the stored token — reuse it and + // Signed in AND the resolved session env == target: probe it. authedClient() + // builds the client for sessionEnv (== the value just compared) with the stored token — reuse it and // discard its message (the exit code is the contract here). client, _, err := authedClient() if err != nil { diff --git a/internal/cli/client.go b/internal/cli/client.go index d7246ee..e5a3f83 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -143,12 +143,26 @@ func clientPrompter() prompter { } // sessionEnv resolves the backend env for the signed-in session: the env saved -// at login, falling back (legacy / empty config) to $CLIENT_ENV then prod. Shared -// by authedClient and logout so every authenticated call — including the revoke -// on sign-out — talks to the host the token was actually issued for. +// at login, falling back (legacy / empty config) to $CLIENT_ENV then prod. +// +// THE ONLY PLACE THAT DERIVES A SESSION ENV FROM A CONFIG. Every caller that +// wants "which backend is this signed-in session on?" — authedClient, logout's +// revoke, `cluster doctor`, `auth status --check`, the telemetry label — goes +// through here, so the answer cannot differ by caller. Reading cfg.CurrentEnv +// directly is the bug this function exists to prevent: it silently drops the +// $CLIENT_ENV fallback, and it skips the normalisation below. +// +// The result is normalised (trimmed, lower-cased) to match api.ResolveEnv, which +// lower-cases both its explicit argument and $CLIENT_ENV. Returning cfg.CurrentEnv +// verbatim made this the one env-resolving function in the CLI whose output was +// not normalised: harmless where the value only reaches api.BaseURL (which +// lower-cases again), but a false negative anywhere the value is COMPARED — a +// config carrying `"current_env": "Dev"` (migrateV1 stores a v1 `env` verbatim, +// and the file is hand-written in fixtures) failed `auth status --check --env dev` +// against a session that works perfectly. func sessionEnv(cfg *config.Config) string { - if cfg.CurrentEnv != "" { - return cfg.CurrentEnv + if e := strings.ToLower(strings.TrimSpace(cfg.CurrentEnv)); e != "" { + return e } return api.ResolveEnv("") } diff --git a/internal/cli/delete.go b/internal/cli/delete.go index f232d88..81f3f3f 100644 --- a/internal/cli/delete.go +++ b/internal/cli/delete.go @@ -331,11 +331,19 @@ func runDelete(ctx context.Context, p *ui.Printer, pr prompter, o deleteOpts) er if o.keepData { p.Infof("Kept local data and config (~/.tracebloc); cleared the active-client pointer — --keep-data.") } else { - if derr := removeHostDataDir(); derr != nil { + if removed, derr := removeHostDataDir(); derr != nil { degraded = true p.Warnf("Couldn't remove local data (%v) — cleared the active-client pointer; "+ "remove the data by hand: rm -rf %s", derr, hostDataDirDisplay()) } else { + // The tree is gone and verified gone. Record it BEFORE printing the + // success line, so nothing later in this process can put it back and + // make that line false — specifically main.go's command-outcome + // telemetry, which runs after this command returns and whose spool + // lives inside the directory just removed (backend#2314). Only on + // the success branch: a failed removal leaves the tree in place, and + // a spool written into a tree that still exists is correct. + markHostStateWiped(removed) p.Successf("Removed local tracebloc data and config.") } } @@ -398,24 +406,29 @@ func renderOffboardSummary(p *ui.Printer, name string, keepData bool) { // $TRACEBLOC_CONFIG_DIR when set — the same resolution config.Dir uses). It goes // through the config package so a test's temp override is honored and the real // ~/.tracebloc is never touched in tests. -func removeHostDataDir() error { +// +// Returns the directory it removed, so the caller can tell the telemetry spool +// not to re-create it on the way out (backend#2314). The path is returned rather +// than re-resolved by the caller because config.Dir() is resolved here, and two +// resolutions of the same thing is how they come to disagree. +func removeHostDataDir() (string, error) { dir, err := config.Dir() if err != nil { - return err + return "", err } if err := osRemoveAll(dir); err != nil { - return err + return "", err } // Verify the directory is actually gone before the caller prints "✔ Removed". // A nil RemoveAll is not proof the tree is absent — a racing writer, a mount, // or a masked partial failure can leave it present — and claiming a clean wipe // we didn't achieve is exactly the offboard-hygiene gap RFC-0003 flags. if _, statErr := osStat(dir); statErr == nil { - return fmt.Errorf("%s still present after removal", dir) + return "", fmt.Errorf("%s still present after removal", dir) } else if !errors.Is(statErr, os.ErrNotExist) { - return fmt.Errorf("verifying removal of %s: %w", dir, statErr) + return "", fmt.Errorf("verifying removal of %s: %w", dir, statErr) } - return nil + return dir, nil } // hostDataDirDisplay is the data dir for a user-facing hint; falls back to the diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index cd3341a..ef3edce 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -111,7 +111,11 @@ func runClusterDoctor( // an error (5xx/403/decode) is a tracebloc-side problem, distinct from a // network failure to reach it at all. Conflating the two would blame the // user's network (and hand them a proxy remedy) for tracebloc's own error. - apiClient := newAPIClient(cfg.CurrentEnv) + // sessionEnv, not cfg.CurrentEnv: the session probe must target the same host + // authedClient would, or `doctor` reports on a backend no other command talks + // to. Reading CurrentEnv directly drops sessionEnv's $CLIENT_ENV fallback and + // its normalisation — a second resolution of the same question. + apiClient := newAPIClient(sessionEnv(cfg)) apiClient.Token = cfg.Current().Token if _, werr := apiClient.WhoAmI(ctx); werr != nil { var ae *api.APIError diff --git a/internal/cli/env_resolution_test.go b/internal/cli/env_resolution_test.go new file mode 100644 index 0000000..af879c7 --- /dev/null +++ b/internal/cli/env_resolution_test.go @@ -0,0 +1,482 @@ +package cli + +// The environment / base-URL resolution family (backend#2171). +// +// The CLI answers "which backend am I talking to?" in several places, and each +// recut of the release train produced one more finding about a site that answered +// it differently from its neighbour: cli#528 (the telemetry label resolved through +// ResolveEnv while the client used the config), #542 (a spool path resolved twice), +// #540 (the label and the sink resolved twice). The pattern is always the same — +// a SECOND resolution of a question already answered — so these tests pin the +// invariants rather than the individual sites: +// +// 1. sessionEnv is the ONLY function that turns a config into a session env, and +// it normalises (trim + lower-case) like api.ResolveEnv does. +// 2. Callers take the resolved value; they never re-derive it. +// +// NOT covered here, deliberately: api.BaseURL's unknown/empty -> prod fail-open. +// That behaviour is shared with the installer's `_backend_url` and diverges from +// client-runtime's controller.py (which refuses), so changing it is a three- +// component decision tracked on backend#2171, not a CLI-local cleanup. + +import ( + "errors" + "fmt" + "go/scanner" + "go/token" + "io/fs" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/telemetry" +) + +// --- 1. sessionEnv is the single, normalising resolution point ---------------- + +// TestSessionEnvNormalisesTheStoredEnv: sessionEnv used to return cfg.CurrentEnv +// verbatim, which made it the one env-resolving function in the CLI whose output +// was not normalised — api.ResolveEnv lower-cases both its argument and +// $CLIENT_ENV, api.BaseURL lower-cases, spoolEnvSlug trims and lower-cases. +// +// Verbatim is invisible where the value only reaches api.BaseURL (which +// lower-cases again) and load-bearing everywhere else: a value that is COMPARED +// (`auth status --check`) or TRIMMED by one consumer and not another (BaseURL does +// not trim; " dev " therefore fell through to PROD). +func TestSessionEnvNormalisesTheStoredEnv(t *testing.T) { + for _, tc := range []struct { + stored string + want string + }{ + {"dev", "dev"}, + {"Dev", "dev"}, // migrateV1 stores a v1 `env` verbatim + {" dev ", "dev"}, // hand-written / fixture config + {"PROD", "prod"}, + {"banana", "banana"}, // unknown is preserved, not coerced — see BaseURL note above + } { + t.Run(tc.stored, func(t *testing.T) { + t.Setenv("CLIENT_ENV", "") + cfg := &config.Config{CurrentEnv: tc.stored} + if got := sessionEnv(cfg); got != tc.want { + t.Fatalf("sessionEnv(current_env=%q) = %q, want %q — sessionEnv must "+ + "normalise like api.ResolveEnv, or its callers disagree about the "+ + "same session", tc.stored, got, tc.want) + } + }) + } +} + +// TestSessionEnvFallsBackToClientEnvOnlyWhenUnset pins the precedence the task +// brief calls load-bearing: config `current_env` BEATS $CLIENT_ENV, and +// $CLIENT_ENV is consulted only when `current_env` is absent. The offboard e2e +// fixture writes `"current_env": "prod"` into its config, so a change that let +// $CLIENT_ENV win would silently repoint that suite. +func TestSessionEnvFallsBackToClientEnvOnlyWhenUnset(t *testing.T) { + t.Setenv("CLIENT_ENV", "dev") + + if got := sessionEnv(&config.Config{CurrentEnv: "prod"}); got != api.EnvProd { + t.Fatalf("sessionEnv(current_env=prod) with CLIENT_ENV=dev = %q, want %q — "+ + "the signed-in env must beat $CLIENT_ENV", got, api.EnvProd) + } + if got := sessionEnv(&config.Config{}); got != api.EnvDev { + t.Fatalf("sessionEnv(no current_env) with CLIENT_ENV=dev = %q, want %q — "+ + "$CLIENT_ENV is the legacy/empty-config fallback", got, api.EnvDev) + } + t.Setenv("CLIENT_ENV", "") + if got := sessionEnv(&config.Config{}); got != api.EnvProd { + t.Fatalf("sessionEnv(no current_env, no CLIENT_ENV) = %q, want %q", got, api.EnvProd) + } +} + +// --- 2. no caller re-derives the session env ---------------------------------- + +// TestClusterDoctorProbesTheSessionEnv: `cluster doctor` built its API client +// from cfg.CurrentEnv directly instead of sessionEnv — a second answer to the +// question authedClient already answers. The whitespace case makes the two +// answers differ for real: api.BaseURL lower-cases but does NOT trim, so +// newAPIClient(" dev ") lands on the prod default while every other +// authenticated command talks to dev-api. +// +// A doctor that probes prod with a dev token reports "session expired" for a +// session that is fine — the worst possible output from a diagnostic. +func TestClusterDoctorProbesTheSessionEnv(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + t.Setenv("CLIENT_ENV", "") + if err := (&config.Config{CurrentEnv: " dev ", Profiles: map[string]*config.Profile{ + " dev ": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + var gotEnv string + orig := newAPIClient + newAPIClient = func(env string) *api.Client { + gotEnv = env + // No BaseURL: the WhoAmI below must fail locally rather than reach any + // real host. What is under test is the env, not the probe's verdict. + return &api.Client{HTTP: &http.Client{Timeout: time.Millisecond}} + } + t.Cleanup(func() { newAPIClient = orig }) + + // HERMETIC BY CONSTRUCTION, and this matters more than it looks. Past the + // session probe, `cluster doctor` loads the real kubeconfig and calls the real + // doctor.Run, whose checkBackendEgress probes backendHost("") — i.e. it issues + // a live GET to https://api.tracebloc.io/. On a developer machine with a real + // k3d cluster that is a genuine production request from a unit test. Failing + // loadClusterFn returns right after the session probe, which is everything + // this test needs: the env is decided before it. + origLoad := loadClusterFn + loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + return nil, errors.New("no cluster (stubbed: keeps this test off the network)") + } + t.Cleanup(func() { loadClusterFn = origLoad }) + + // doctor exits non-zero here (stubbed no-cluster); the assertion is on the env + // it built the client for, which is decided before that. + _, _ = runCmd(t, "cluster", "doctor") + + if gotEnv != api.EnvDev { + t.Fatalf("cluster doctor built its API client for %q, want %q — it must resolve "+ + "through sessionEnv like authedClient, not read cfg.CurrentEnv raw "+ + "(api.BaseURL(%q) is the PROD default, so this probes the wrong backend)", + gotEnv, api.EnvDev, gotEnv) + } +} + +// TestAuthCheckComparesTheResolvedEnv: `auth status --check` compared the raw +// cfg.CurrentEnv against api.ResolveEnv's already-normalised target, so a config +// carrying a non-normalised env failed the probe for the very session it is +// signed in to — and the installer, whose contract is this exit code, would then +// re-run `login` (or skip provisioning) against a session that works. +func TestAuthCheckComparesTheResolvedEnv(t *testing.T) { + probed := false + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/userinfo/" { + probed = true + _, _ = w.Write([]byte(`{"email":"ds@co","account":"Acme"}`)) + } + }) + // withTestBackend isolates the config dir; write a non-normalised env into it. + if err := (&config.Config{CurrentEnv: "Dev", Profiles: map[string]*config.Profile{ + "Dev": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + if _, err := runCmd(t, "auth", "status", "--check", "--env", "dev"); err != nil { + t.Fatalf("--check --env dev must accept a session stored as \"Dev\" "+ + "(sessionEnv resolves it to dev and the client talks to dev-api), got: %v", err) + } + if !probed { + t.Error("the backend was never probed — the env comparison rejected a session " + + "that differs from the target only in case") + } +} + +// TestAuthCheckStillRejectsARealEnvMismatch is the other half: normalising the +// comparison must not make it lenient about the thing it exists to catch. +func TestAuthCheckStillRejectsARealEnvMismatch(t *testing.T) { + probed := false + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/userinfo/" { + probed = true + } + }) + saveSignedIn(t, "tok") // CurrentEnv=dev + if _, err := runCmd(t, "auth", "status", "--check", "--env", "stg"); ExitCodeFromError(err) != 1 { + t.Fatalf("exit code = %d, want 1 — a dev session must not satisfy a stg target", + ExitCodeFromError(err)) + } + if probed { + t.Error("must not probe the backend on a genuine env mismatch") + } +} + +// TestAuthStatusShowsTheEnvTheClientWillUse: `auth status` printed the raw +// stored env as its "backend" field, so the human-facing answer to "which +// backend am I on?" could differ from the one --check computes and the one +// authedClient dials — two answers to the same question, in the same file. +func TestAuthStatusShowsTheEnvTheClientWillUse(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + t.Setenv("CLIENT_ENV", "") + if err := (&config.Config{CurrentEnv: " Dev ", Profiles: map[string]*config.Profile{ + " Dev ": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + out, err := runCmd(t, "auth", "status") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, api.EnvDev) || strings.Contains(out, " Dev ") { + t.Fatalf("auth status reported the stored env verbatim, not the resolved one.\n"+ + "want the %q the client actually dials, got:\n%s", api.EnvDev, out) + } +} + +// TestTheRecordIsLabelledWithTheEnvItWasHanded is the #540 finding. +// +// RecordCommandOutcome resolves the env once and derives BOTH the record's label +// and the sink (spool path + POST destination) from that one value. +// recordCommandOutcome used to call telemetryEnv(signedInEnv()) again for the +// emitter, so the label came from a second, independent config read: two reads +// that merely tend to agree, and disagree the moment a `login` lands between them +// — which is precisely the "labelled stg, posted to prod" leak the comment above +// RecordCommandOutcome claims to prevent. +// +// The invariant is structural, because a race between two config reads is not a +// deterministic test: recordCommandOutcome must label the record with the env it +// was GIVEN. Handing it an env that disagrees with the config on disk is how we +// tell "used the parameter" from "read the config again". +func TestTheRecordIsLabelledWithTheEnvItWasHanded(t *testing.T) { + dir := t.TempDir() + t.Setenv("TRACEBLOC_CONFIG_DIR", dir) + t.Setenv("CLIENT_ENV", "") + // On disk: dev. A second resolution inside recordCommandOutcome would find + // this and label the record "dev". + body := `{"version":2,"current_env":"dev","profiles":{"dev":{"token":"x"}}}` + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + if got := signedInEnv(); got != api.EnvDev { + t.Fatalf("signedInEnv() = %q, want %q — the fixture no longer matches the "+ + "on-disk config layout, so this test would pass vacuously", got, api.EnvDev) + } + + var res map[string]string + sink := telemetry.Sink(func(r map[string]string, _ map[string]any) { res = r }) + + root := NewRootCmd(testBuildInfo()) + // Handed: stg — standing in for "the value the sink was built from", which is + // what the caller resolved before the config changed under it. + if err := recordCommandOutcome( + root, root, testBuildInfo(), 0, time.Second, + func(string) string { return "" }, api.EnvStg, sink, + ); err != nil { + t.Fatal(err) + } + if res == nil { + t.Fatal("nothing was delivered") + } + if got := res["deployment.environment"]; got != api.EnvStg { + t.Fatalf("deployment.environment = %q, want %q — the emitter must be labelled "+ + "with the env it was handed (the one the sink was built from), not with a "+ + "second read of the config", got, api.EnvStg) + } +} + +// --- 3. the guard: no NEW resolution site can appear unnoticed ---------------- + +// resolutionSites is the closed set of files in this MODULE allowed to answer +// "which backend?" from ambient state (the config file or $CLIENT_ENV). +// Everything else must take a resolved env as an argument. +// +// This is the rule the last three recuts each re-litigated one site at a time. It +// is here rather than in a reviewer's head because the failure mode is additive: +// every new site looks locally correct, and only the SECOND one is a bug. +// +// Keys are repo-relative because the walk is repo-wide. It used to walk only +// internal/cli, which left the guard blind in 16 of the 17 packages under +// internal/ — i.e. blind exactly where a new site is most likely to land, in a +// package written by someone who never reads internal/cli (Lukas on #551). +var resolutionSites = map[string]string{ + // The primitive: ResolveEnv is the --env/$CLIENT_ENV/prod chain, and the only + // os.Getenv("CLIENT_ENV") in the module. + "internal/api/client.go": "api.ResolveEnv — the primitive chain, and the only $CLIENT_ENV read", + // The --env FLAG, a different question: the env the human/installer NAMED, + // which login persists (the one cfg.CurrentEnv WRITE) and `auth status --check` + // validates against the session. + "internal/cli/auth.go": "api.ResolveEnv(envFlag) — the explicit --env flag, validated by IsKnownEnv", + // sessionEnv: config current_env, else $CLIENT_ENV, else prod. The one chain + // every session-env consumer in internal/cli resolves through. + "internal/cli/client.go": "sessionEnv — the single config -> session-env resolution", + // Storage. Profiles are keyed by the RAW stored string, so this layer must not + // normalise; it hands the raw value out and sessionEnv normalises it. + "internal/config/config.go": "the on-disk current_env field, its accessors, and the v1 migration", + // The CLUSTER's CLIENT_ENV, read off the jobs-manager Deployment — a + // deliberately different question from this CLI's session env. + "internal/doctor/doctor.go": "the cluster's own CLIENT_ENV, for the egress probe's target host", + // internal/cli/telemetry.go is deliberately ABSENT, and the staleness check + // below is what keeps it that way: telemetryEnv/signedInEnv delegate the whole + // chain to sessionEnv and name no needle, so an entry for it would be inert — + // a licence to re-admit a raw read in the very file whose double resolution + // this PR exists to remove (Bugbot on #551). +} + +// envNeedles are the ways a file reads ambient state to answer "which backend?". +// api.BaseURL/IsKnownEnv are pure mappings over an argument and are deliberately +// absent — they resolve nothing. +// +// Deliberately BROAD (bare identifiers, and CLIENT_ENV unquoted so it matches +// help text too): a false positive is a loud line in a diff, a false negative is +// the bug this guard exists to catch. Fail closed. +var envNeedles = []string{"CurrentEnv", "ResolveEnv", "CLIENT_ENV"} + +// matchesAnyNeedle is THE matcher, called from both directions — the detection +// sweep and the allowlist audit. One function on purpose: two copies of "does +// this file resolve an env?" is the same shape as the two copies of "which env?" +// that this PR exists to remove, and it would let detection and allowlisting +// drift apart exactly where nobody looks. +func matchesAnyNeedle(code string) (string, bool) { + for _, needle := range envNeedles { + if strings.Contains(code, needle) { + return needle, true + } + } + return "", false +} + +// envCodeOf tokenises one file, or reports why it could not. +func envCodeOf(path string) (string, error) { + src, err := os.ReadFile(path) + if err != nil { + return "", err + } + return goCodeTokens(string(src)) +} + +func TestNoNewEnvironmentResolutionSiteAppears(t *testing.T) { + const root = "../.." // this test's package dir -> the module root + + // --- detection: what actually resolves an env, module-wide --------------- + matched := map[string]string{} // repo-relative path -> the needle that hit + walkErr := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + switch d.Name() { + case ".git", ".claude", ".worktrees", "vendor", "node_modules", "testdata": + return fs.SkipDir + } + return nil + } + name := d.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + return nil + } + code, err := envCodeOf(path) + if err != nil { + // "cannot tell" is not "clean": abort rather than read a file whose + // tokenisation failed as a string that matches nothing. + return fmt.Errorf("%s: %w", path, err) + } + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + if needle, ok := matchesAnyNeedle(code); ok { + matched[filepath.ToSlash(rel)] = needle + } + return nil + }) + if walkErr != nil { + t.Fatalf("walking the module: %v", walkErr) + } + + // --- every match must be sanctioned -------------------------------------- + for rel, needle := range matched { + if _, ok := resolutionSites[rel]; !ok { + t.Errorf("%s resolves an environment from ambient state (%q), but is not a "+ + "sanctioned resolution site.\n"+ + "Take the resolved env as an ARGUMENT instead — the CLI must answer "+ + "\"which backend?\" once per invocation and thread it. If this really is "+ + "a new resolution point, add it to resolutionSites with the reason and "+ + "say how it cannot disagree with sessionEnv.", rel, needle) + } + } + + // --- and every sanctioned entry must still EARN its place ---------------- + // + // THE ALLOWLIST MUST STAY A RECORD, NOT BECOME A LICENCE. Asking only whether a + // sanctioned file EXISTS is the defect class this whole PR is about: checking + // the form of a thing instead of the property the form exists to guarantee. An + // entry whose file no longer resolves anything is inert — it verifies nothing, + // while silently pre-approving the next ambient read in that file. My own + // consolidation did exactly that to the telemetry.go entry (Bugbot on #551), + // in the one file whose double resolution this PR removes. + // + // Per ENTRY rather than per suite, so the failure names the entry to delete. + // This also subsumes the "needles went stale" backstop: rename a needle and + // every entry goes inert at once, which is loud and specific rather than a + // single global counter hitting zero. + for rel, why := range resolutionSites { + if _, err := os.Stat(filepath.Join(root, rel)); err != nil { + t.Errorf("resolutionSites lists %s, which cannot be read — stale allowlist", rel) + continue + } + if why == "" { + t.Errorf("resolutionSites[%q] is allowlisted with no reason", rel) + } + if _, ok := matched[rel]; !ok { + t.Errorf("%s is allowlisted as a resolution site but resolves nothing any more "+ + "— drop the entry, or the guard silently permits the next ambient read "+ + "here.", rel) + } + } + + // The one thing the per-entry loop cannot see: an EMPTY allowlist makes it + // vacuous, and a needle rename plus an empty allowlist would then pass in + // silence. Anchor both. + if len(resolutionSites) == 0 || len(matched) == 0 { + t.Fatalf("the guard checked nothing: %d sanctioned entries, %d files matched", + len(resolutionSites), len(matched)) + } +} + +// goCodeTokens renders src as its Go TOKENS, with comments dropped — so the +// guard reads code, never prose (these names are the subject of half the +// comments in the package). +// +// SCANS GO AS GO, because the hand-rolled comment stripper this replaces was +// fail-open on string literals, and Lukas demonstrated both holes on #551: +// the "//" inside a "https://…" literal started a comment that ate the rest of +// the line (needle included), and a "/*" inside a literal like "/*.json" +// swallowed every needle below it to the next "*/" or EOF. Seven non-test files +// in internal/cli already carry an https:// literal, so that was one future line +// away, not a contrived shape. +// +// Literals are KEPT in the output rather than dropped: a needle inside a string +// then reads as a loud false positive, which is cheap — the opposite direction +// fails silently, which is the bug. +// +// SCOPE OF THE ERROR, stated precisely because the first version of this comment +// overclaimed it: go/scanner is LEXICAL, not syntactic, so this returns an error +// only on lexical faults — an unterminated string literal or an unterminated +// /* comment. `func f( {` scans clean and is reported as normal code. That is the +// right guarantee rather than a weak one: the faults it does catch are exactly +// the ones that would desynchronise literal/comment boundaries and hand the +// needles a misread file, which is the failure this function exists to prevent. +// A file that tokenises correctly but does not compile still yields correct +// needles, and `go build` is the check for whether it compiles. +func goCodeTokens(src string) (string, error) { + var fset token.FileSet + var sc scanner.Scanner + scanErrs := 0 + f := fset.AddFile("", fset.Base(), len(src)) + // mode 0: comment tokens are not emitted at all. + sc.Init(f, []byte(src), func(token.Position, string) { scanErrs++ }, 0) + var b strings.Builder + for { + _, tok, lit := sc.Scan() + if tok == token.EOF { + break + } + if lit != "" { + b.WriteString(lit) + } else { + b.WriteString(tok.String()) + } + b.WriteByte(' ') + } + if scanErrs > 0 { + return "", fmt.Errorf("%d scan error(s) — cannot tell what this file reads", scanErrs) + } + return b.String(), nil +} diff --git a/internal/cli/resources.go b/internal/cli/resources.go index c636297..fe54022 100644 --- a/internal/cli/resources.go +++ b/internal/cli/resources.go @@ -154,6 +154,11 @@ func renderResources(ctx context.Context, p *ui.Printer, target *clusterTarget) } else { p.Field("resource env", "(unset — using chart default "+resources.DefaultTraining+")") } + // backend#2220: who chose the ceiling above. Verbose-only — it answers a + // support question ("did someone set this, or did we?"), not one an + // operator needs on every run, and the default view should not grow a + // line for bookkeeping that never changes the numbers. + p.Field("set by", provenanceLine(train.Provenance)) if nodeErr == nil && len(machine.GPU) == 0 { p.Field("gpu", "none detected") } @@ -170,6 +175,24 @@ func renderResources(ctx context.Context, p *ui.Printer, target *clusterTarget) return nil } +// provenanceLine renders RESOURCE_PROVENANCE for humans (backend#2220). +// +// "unknown" gets an explanation rather than the bare word, because the bare word +// invites the wrong conclusion. It does not mean something is broken: it means +// the value predates the marker, and an installer-written size and a deliberate +// `resources set` are indistinguishable once the value differs from the historic +// default. That is why it is treated as a human choice and left alone. +func provenanceLine(provenance string) string { + switch provenance { + case resources.ProvenanceUser: + return "explicitly set (tracebloc resources set)" + case resources.ProvenanceInstaller: + return "sized to this machine at install time" + default: + return "unknown — predates provenance tracking, treated as an explicit choice" + } +} + // machineLine renders the machine-capacity value: "8 CPU · 32 GiB" (+ " · 1 GPU" // when a device is present). func machineLine(m resources.Machine) string { diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index c9f5c27..9c9ecb3 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -238,19 +238,42 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // there is nothing for the fit-check to protect. Sizing an actual CHANGE // is still validated below, before anything mutates. ceilingUnchanged := sameCeiling(desired, current) - if ceilingUnchanged && !phantomGPU { + // backend#2220: an unchanged ceiling is still a HUMAN CHOICE the moment the + // operator runs this command, so it must not exit before BuildEnvSpec stamps + // RESOURCE_PROVENANCE=user. BuildEnvSpec writes the marker unconditionally + // within itself, but this early return could skip it entirely — so an + // installer-sized edge whose operator ran `resources set --max`, or passed + // flags restating the current ceiling, kept `installer`. That is the state + // BuildEnvSpec's own comment calls the most dangerous the marker can be in: + // a deliberate choice wearing the one label that invites a future ladder to + // overwrite it. Caught by Bugbot and confirmed in review on #539. + // + // `unknown` counts as stale too: a pre-marker edge whose operator restates + // the ceiling has now made that size explicit, and recording it as such is + // the honest answer. The cost is one extra apply per edge, exactly once — + // the second run sees `user` and is a clean no-op again. + staleProvenance := current.Provenance != resources.ProvenanceUser + if ceilingUnchanged && !phantomGPU && !staleProvenance { p.Newline() p.Successf("Each training run already uses up to %s — nothing to change.", perRunSize(desired)) return nil } - if ceilingUnchanged { // phantomGPU == true here - // CPU/memory budget is unchanged, but this GPU-less machine's cluster - // still requests a GPU (a stale chart default). Don't treat it as a - // clean no-op — fall through to persist so BuildEnvSpec's explicit-empty - // GPU override lands and clears it; otherwise runs stay unschedulable / - // fall back to CPU while the heartbeat keeps advertising a GPU. + if ceilingUnchanged { + // Same shape as the phantom-GPU case, and for the same reason: the budget + // is unchanged but something else still needs persisting, so this is not + // a clean no-op. Both conditions can hold at once, so both report. p.Newline() - p.Infof("Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule.") + if phantomGPU { + // CPU/memory budget is unchanged, but this GPU-less machine's cluster + // still requests a GPU (a stale chart default). Don't treat it as a + // clean no-op — fall through to persist so BuildEnvSpec's explicit-empty + // GPU override lands and clears it; otherwise runs stay unschedulable / + // fall back to CPU while the heartbeat keeps advertising a GPU. + p.Infof("Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule.") + } + if staleProvenance { + p.Infof("Your CPU and memory budget is unchanged — recording it as your explicit choice so it is never resized automatically.") + } } // (5) Validate + fit-check — ONLY when the ceiling actually CHANGES. An @@ -268,7 +291,27 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // (6) Confirm (unless --yes or --dry-run). One gate for both the flag and // wizard paths; --dry-run mutates nothing so it never needs confirming. - if !req.yes && !req.dryRun { + // + // ceilingUnchanged skips it too, and that is the load-bearing clause: + // the gate guards the CEILING, and an unchanged ceiling has nothing to + // ask about. "Let each training run use up to 4 CPU · 16 GiB?" when the + // answer is already 4 CPU · 16 GiB is a question with one honest answer, + // and off a terminal the gate does not ask at all — it returns exit 1. + // + // cli#546: #539's staleness treatment (backend#2220) sent the unchanged + // ceiling down here for the first time, so `set --cores 4 --memory 16` + // restating the CURRENT ceiling without --yes turned from the documented + // exit-0 no-op ("Exit codes: 0 applied (or nothing to change)" above, and + // the `no change → exit 0` edge in docs/cli-navigation.md that bypasses + // CONF entirely) into exit 1. Nearly every installed edge reads + // `installer` or `unknown`, so that was the whole installed base, not an + // edge case — and the callers that restate a size are scripts (the + // bootstrap, the end-to-end journey), none of which pass --yes for what + // the docs promise is a no-op. The phantom-GPU fall-through (#241) had + // the same shape and is fixed by the same clause: both are bookkeeping + // writes, already announced by the Infof lines above, not budget changes + // an operator needs to sanction. + if !req.yes && !req.dryRun && !ceilingUnchanged { if pr == nil { return &exitError{code: exitFailure, err: fmt.Errorf( "refusing to change the ceiling without confirmation: pass --yes, or run on a terminal")} diff --git a/internal/cli/resources_set_test.go b/internal/cli/resources_set_test.go index 366b6d3..c7f0191 100644 --- a/internal/cli/resources_set_test.go +++ b/internal/cli/resources_set_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "os" "strings" "testing" @@ -41,6 +42,44 @@ func fakeHelm(t *testing.T) *[][]string { return &calls } +// fakeHelmValues is fakeHelm plus the CONTENTS of the `-f` values file, read +// while the upgrade is in flight (helm.Upgrade writes and closes it before +// shelling Runner, and removes it after) — so it is only readable from inside +// the double. +// +// Needed because the existing provenance assertions all go through --dry-run, +// and --dry-run skips the confirmation gate. Proving that a marker lands on a +// path whose bug WAS the gate therefore has to use the real apply path +// (backend#2220). +func fakeHelmValues(t *testing.T) (*[][]string, *string) { + t.Helper() + var values string + calls := fakeHelm(t) + inner := helm.Runner + helm.Runner = func(ctx context.Context, name string, args ...string) (string, error) { + for i, a := range args { + if a == "-f" && i+1 < len(args) { + if b, err := os.ReadFile(args[i+1]); err == nil { + values = string(b) + } + } + } + return inner(ctx, name, args...) + } + return calls, &values +} + +// helmUpgraded reports whether a real `helm upgrade` (not the `--help` capability +// probe) was shelled. +func helmUpgraded(calls [][]string) bool { + for _, c := range calls { + if len(c) >= 3 && c[1] == "upgrade" && c[2] != "--help" { + return true + } + } + return false +} + // runSet drives applyResourcesSet against a fake cluster + prompter, returning the // captured stdout and the error. func runSet(t *testing.T, cs *fake.Clientset, pr prompter, req setReq) (string, error) { @@ -170,6 +209,40 @@ func TestSet_ApplyBuildsHelmArgsAndValues(t *testing.T) { } } +// TestSet_StampsUserProvenance: the apply carries RESOURCE_PROVENANCE=user all +// the way into the values helm is given (backend#2220). +// +// The unit test on BuildEnvSpec proves the map is right; this proves the map +// actually reaches helm. Worth having separately because the failure mode is +// silent: a marker that never lands looks identical to one that landed, and the +// consequence only shows up much later, when a ladder re-derives a size the +// operator had chosen on purpose. +func TestSet_StampsUserProvenance(t *testing.T) { + // Start from an edge the INSTALLER marked — the dangerous case. After a + // human `resources set`, the label must flip to user; leaving it as + // "installer" would advertise a deliberate choice as ours to overwrite. + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=2,memory=8Gi", + "RESOURCE_PROVENANCE": "installer", + }) + out, err := runSet(t, cs, nil, setReq{ + cores: "4", memory: "16", coresSet: true, memSet: true, dryRun: true, yes: true, + }) + if err != nil { + t.Fatalf("dry-run: %v\n%s", err, out) + } + if !strings.Contains(out, "RESOURCE_PROVENANCE") { + t.Errorf("the plan does not mention RESOURCE_PROVENANCE at all:\n%s", out) + } + if !strings.Contains(out, "user") { + t.Errorf("the plan does not stamp the set as a human choice:\n%s", out) + } + // And the envelope still reflects what was asked for (Decision A). + if !strings.Contains(out, "cpu=4,memory=16Gi") { + t.Errorf("plan lost the requested ceiling:\n%s", out) + } +} + // TestSet_KeepsUnsetDimension: `set --cores 4` changes CPU only and KEEPS the // current 8Gi memory (proven via the dry-run plan's resulting values). func TestSet_KeepsUnsetDimension(t *testing.T) { @@ -206,7 +279,15 @@ func TestSet_MaxUsesWholeMachineMinusOverhead(t *testing.T) { // helm upgrade entirely. func TestSet_NoOpSkipsApply(t *testing.T) { calls := fakeHelm(t) - cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"}) + // RESOURCE_PROVENANCE=user is what makes this a CLEAN no-op (backend#2220): + // restating the ceiling on an edge whose size is already recorded as the + // operator's choice leaves nothing at all to persist. Without the marker the + // command must fall through and stamp it — covered by + // TestSet_SameCeilingStampsProvenance below. + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "RESOURCE_PROVENANCE": "user", + }) out, err := runSet(t, cs, nil, setReq{cores: "4", memory: "16", coresSet: true, memSet: true, yes: true}) if err != nil { t.Fatalf("no-op: %v", err) @@ -221,6 +302,188 @@ func TestSet_NoOpSkipsApply(t *testing.T) { } } +// TestSet_SameCeilingStampsProvenance: the hole Bugbot found on #539 and +// saadqbal confirmed — a same-size `resources set` used to return BEFORE +// BuildEnvSpec, so an installer-sized edge kept RESOURCE_PROVENANCE=installer +// even though a human had just chosen that size. +// +// That is the state BuildEnvSpec's own comment calls the most dangerous the +// marker can be in: a deliberate choice wearing the one label that invites a +// future ladder to overwrite it. `resources set` restating the current ceiling +// IS a human choice, so it must persist. +func TestSet_SameCeilingStampsProvenance(t *testing.T) { + for _, tc := range []struct { + name string + provenance string + }{ + {"installer-sized edge", "installer"}, + {"pre-marker edge", ""}, // ParseTraining normalises to unknown + {"junk marker", "banana"}, // ...as does anything unrecognised + } { + t.Run(tc.name, func(t *testing.T) { + calls := fakeHelm(t) + env := map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"} + if tc.provenance != "" { + env["RESOURCE_PROVENANCE"] = tc.provenance + } + cs := csWith("8", "32Gi", env) + out, err := runSet(t, cs, nil, setReq{ + cores: "4", memory: "16", coresSet: true, memSet: true, yes: true, + }) + if err != nil { + t.Fatalf("same-ceiling set: %v\n%s", err, out) + } + upgraded := false + for _, c := range *calls { + if len(c) >= 3 && c[1] == "upgrade" && c[2] != "--help" { + upgraded = true + } + } + if !upgraded { + t.Errorf("a stale %q marker must NOT be a clean no-op — the apply is what stamps `user`:\n%s", + tc.provenance, out) + } + if strings.Contains(out, "nothing to change") { + t.Errorf("must not claim nothing changed while the marker is being corrected:\n%s", out) + } + if !strings.Contains(out, "explicit choice") { + t.Errorf("the reason for the apply should be stated:\n%s", out) + } + }) + } +} + +// TestSet_SameCeilingNeedsNoYes: a same-ceiling restatement must NOT require +// --yes. This is the script-facing half of backend#2220 that #539 broke — and +// the half its tests missed, because every same-ceiling case there passed +// `yes: true`, which is exactly the flag under dispute. +// +// #539 made any non-`user` marker stale, which sent the unchanged-ceiling path +// into the confirmation gate for the first time. Off a terminal that gate does +// not ask, it returns exit 1 — so `resources set --cores 4 --memory 16` +// restating the current ceiling flipped from the documented exit-0 no-op ("0 +// applied (or nothing to change)" in the command's own help; the `no change → +// exit 0` edge in docs/cli-navigation.md, which bypasses CONF entirely) to a +// hard failure. Nearly every installed edge reads `installer` or `unknown`, so +// the blast radius was the installed base, and the callers that restate a size +// without --yes are scripts — the bootstrap and the end-to-end journey — which +// no interactive test exercises. (cli#546) +// +// The assertions are deliberately paired: exit 0 AND the apply still happening. +// Either one alone is satisfiable by the wrong fix — reverting the staleness +// treatment would give exit 0 with no re-stamp, and #539 as merged gives the +// re-stamp only to callers who pass --yes. +func TestSet_SameCeilingNeedsNoYes(t *testing.T) { + for _, tc := range []struct { + name string + provenance string + }{ + {"installer-sized edge", "installer"}, + {"pre-marker edge", ""}, + {"junk marker", "banana"}, + } { + t.Run(tc.name, func(t *testing.T) { + calls, values := fakeHelmValues(t) + env := map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"} + if tc.provenance != "" { + env["RESOURCE_PROVENANCE"] = tc.provenance + } + cs := csWith("8", "32Gi", env) + // pr == nil is a non-TTY script; no `yes` field is set. + out, err := runSet(t, cs, nil, setReq{ + cores: "4", memory: "16", coresSet: true, memSet: true, + }) + if err != nil { + t.Fatalf("restating the current ceiling must not need --yes, got: %v\n%s", err, out) + } + if !helmUpgraded(*calls) { + t.Errorf("the re-stamp must still happen without --yes — otherwise the fix\n"+ + "just reverted backend#2220 for every script:\n%s", out) + } + // And the marker that lands is `user`, read off the values file helm + // was actually handed rather than inferred from the prose. + if !strings.Contains(*values, "RESOURCE_PROVENANCE") || + !strings.Contains(*values, "user") { + t.Errorf("values handed to helm do not stamp RESOURCE_PROVENANCE=user:\n%s", *values) + } + // The numbers are untouched: this is a bookkeeping write, not a resize. + if !strings.Contains(*values, "cpu=4,memory=16Gi") { + t.Errorf("a bookkeeping write must not move the ceiling:\n%s", *values) + } + }) + } + + // A GPU-less machine whose cluster still carries the chart-default + // GPU_REQUESTS reaches the same fall-through for the same reason (#241), and + // was broken by the same missing clause. Scripts must be able to clear it. + t.Run("phantom GPU cleanup needs no --yes", func(t *testing.T) { + calls, values := fakeHelmValues(t) + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "GPU_REQUESTS": "nvidia.com/gpu=1", + "RESOURCE_PROVENANCE": "user", // isolate the phantom GPU as the only reason + }) // no GPU on the node + out, err := runSet(t, cs, nil, setReq{ + cores: "4", memory: "16", coresSet: true, memSet: true, + }) + if err != nil { + t.Fatalf("clearing a phantom GPU must not need --yes, got: %v\n%s", err, out) + } + if !helmUpgraded(*calls) { + t.Errorf("the phantom-GPU cleanup must still apply without --yes:\n%s", out) + } + if !strings.Contains(*values, "GPU_REQUESTS") { + t.Errorf("values must carry the explicit-empty GPU override:\n%s", *values) + } + }) + + // The gate is skipped because the CEILING is unchanged — not because the + // prompter is absent. An operator who would decline still gets the + // bookkeeping write, because there was never a budget question to decline. + // (fakePrompter.Confirm returning false is the only observable proof the + // gate was not entered: entering it would cleanCancel and shell no helm.) + t.Run("not even asked on a terminal", func(t *testing.T) { + calls := fakeHelm(t) + pr := &fakePrompter{confirm: boolPtr(false)} + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "RESOURCE_PROVENANCE": "installer", + }) + out, err := runSet(t, cs, pr, setReq{ + cores: "4", memory: "16", coresSet: true, memSet: true, + }) + if err != nil { + t.Fatalf("same-ceiling set on a terminal: %v\n%s", err, out) + } + if !helmUpgraded(*calls) { + t.Errorf("a declining prompter proves the confirm gate was entered; it must\n"+ + "be skipped when the ceiling is unchanged:\n%s", out) + } + if strings.Contains(out, "nothing was changed") { + t.Errorf("there was no budget question to decline:\n%s", out) + } + }) + + // The narrow-fix guard: a REAL change off a terminal still refuses without + // --yes, so the clause above cannot have disarmed the gate wholesale. + // TestSet_OffTTYWithFlagsNeedsYes covers the same contract from the other + // side; this keeps the pair adjacent to the fix it bounds. + t.Run("a real change still needs --yes", func(t *testing.T) { + calls := fakeHelm(t) + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "RESOURCE_PROVENANCE": "installer", + }) + _, err := runSet(t, cs, nil, setReq{cores: "6", coresSet: true}) + if got := exitCode(t, err); got != 1 { + t.Fatalf("a changed ceiling off a terminal must still exit 1, got %d (%v)", got, err) + } + if helmUpgraded(*calls) { + t.Error("an unconfirmed CHANGE must mutate nothing") + } + }) +} + // TestSet_NoOpEvenWhenCurrentNoLongerFits: restating the ceiling that's already // applied must stay a clean no-op success even when the machine has SHRUNK under // it (smaller Docker Desktop VM, lost node) — the no-op check runs before the @@ -228,7 +491,12 @@ func TestSet_NoOpSkipsApply(t *testing.T) { // change on the same shrunken machine is still fit-checked. func TestSet_NoOpEvenWhenCurrentNoLongerFits(t *testing.T) { // Node 4 CPU / 8 GiB, but the cluster already runs with cpu=8,memory=16Gi. - cur := map[string]string{"RESOURCE_LIMITS": "cpu=8,memory=16Gi"} + // Marked `user` so this stays the clean-no-op case it is testing; the + // stale-marker fall-through has its own test (backend#2220). + cur := map[string]string{ + "RESOURCE_LIMITS": "cpu=8,memory=16Gi", + "RESOURCE_PROVENANCE": "user", + } t.Run("flags restating the current ceiling", func(t *testing.T) { calls := fakeHelm(t) @@ -426,7 +694,14 @@ func TestWizard_GPURowOmittedWhenNoGPU(t *testing.T) { func TestWizard_LeaveAsIs(t *testing.T) { calls := fakeHelm(t) pr := &fakePrompter{answers: map[string]string{"How much may one training run use?": "Leave it as it is"}} - cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"}) + // Marked `user` so "leave it as it is" is the clean no-op this test is about. + // On an edge whose size is NOT yet recorded as the operator's choice, picking + // "leave it as it is" does still persist — the marker is being corrected, and + // TestSet_SameCeilingStampsProvenance covers that (backend#2220). + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "RESOURCE_PROVENANCE": "user", + }) out, err := runSet(t, cs, pr, setReq{}) if err != nil { t.Fatalf("wizard leave: %v", err) diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index df2ff2e..f76f756 100644 --- a/internal/cli/telemetry.go +++ b/internal/cli/telemetry.go @@ -8,12 +8,13 @@ package cli // "terminal event on every path" would then be true only for the handlers // somebody remembered. // -// WHERE THIS STOPS TODAY. The transport is a seam. RFC-BACKEND-1872's Collector -// gateway was replaced on 17 Aug by an ingest endpoint on the backend -// (rfcs#28), which is backend#1905 and does not exist yet — so pendingSink -// returns nil and every event is validated and dropped. That is deliberate: -// validation runs on every build regardless, so a malformed event fails in CI -// wherever the binary was built, and connecting #1905 is one function. +// WHERE THE TRANSPORT LIVES. RFC-BACKEND-1872's Collector gateway was replaced +// on 17 Aug by an ingest endpoint on the backend (rfcs#28, backend#1905), which +// now accepts OTLP/HTTP JSON (backend#2213). Delivery is implemented in +// telemetry_transport.go and the mapping in telemetry_otlp.go (backend#2217); +// this file still owns only WHAT is emitted. Validation runs on every build +// regardless of whether delivery is configured, so a malformed event fails in CI +// wherever the binary was built. import ( "crypto/rand" @@ -95,10 +96,11 @@ func commandPathOf(c *cobra.Command) string { // resolved exactly the way api.BaseURL resolves it — because that is the host // these records are about. The mapping mirrors BaseURL: a known env is itself; a // present-but-unrecognised value is prod, because api.BaseURL routes every -// unknown value to https://api.tracebloc.io (sessionEnv hands cfg.CurrentEnv to -// api.New verbatim). So prod is the accurate label for that population, not a -// guess — and NOT withheld: a misconfigured install that hits prod and fails is -// exactly the run this feature exists to see. +// unknown value to https://api.tracebloc.io (sessionEnv normalises cfg.CurrentEnv +// but does not validate it, so an unrecognised value reaches api.New intact). So +// prod is the accurate label for that population, not a guess — and NOT withheld: +// a misconfigured install that hits prod and fails is exactly the run this +// feature exists to see. // // $CLIENT_ENV is consulted only when there is no signed-in env, matching // sessionEnv: once cfg.CurrentEnv is set the client ignores $CLIENT_ENV, so @@ -107,13 +109,23 @@ func commandPathOf(c *cobra.Command) string { // // NOTE: that api.BaseURL silently routes an unknown env to prod — so an install // believing it is on another backend sends its token there — is a real defect, -// but in client.go, not here; tracked separately. This function must match that -// behaviour until it changes, not diverge from it. +// but in internal/api/client.go, not here. It is shared with the installer's +// `_backend_url` and contradicted by client-runtime's controller.py (which +// refuses), so it is a three-component decision tracked on backend#2171. This +// function must match that behaviour until it changes, not diverge from it. func telemetryEnv(env string) string { resolved := env if resolved == "" { - // Not signed in: $CLIENT_ENV, then the prod default (as sessionEnv does). - resolved = api.ResolveEnv("") + // No stored session: $CLIENT_ENV, then the prod default — resolved BY + // sessionEnv over an empty config rather than by a second copy of its + // fallback, so the precedence chain lives in exactly one function. + // + // NOT REACHABLE FROM PRODUCTION any more: the only production caller passes + // signedInEnv(), which since it delegates to sessionEnv never returns "". + // Kept, and not dead, because telemetryEnv is a pure mapping that the tests + // call directly with "" — and because a mapping that panics or mislabels on + // an empty input would be a worse contract than one that resolves it. + resolved = sessionEnv(&config.Config{}) } if api.IsKnownEnv(resolved) { return strings.ToLower(resolved) @@ -123,14 +135,27 @@ func telemetryEnv(env string) string { return api.EnvProd } -// signedInEnv reads the environment the config points at, best-effort. A -// missing or unreadable config is simply "not signed in". +// signedInEnv resolves the environment the config points at, best-effort. +// +// Delegates to sessionEnv — the same function authedClient and logout resolve +// through — so the record's label is derived from the session env by the same +// code that picks the host the CLI talks to, not by a parallel restatement of +// the rule that can drift from it. +// +// ALWAYS RETURNS A RESOLVED ENV, never "". It used to return "" for a missing or +// unreadable config, and the old comment called that "not signed in"; delegating +// to sessionEnv means that case now resolves through $CLIENT_ENV to prod like any +// other empty config. So "not signed in" no longer names an output — it means +// "resolved from $CLIENT_ENV/prod rather than from a stored session", and the two +// are indistinguishable here by design (the label is about the host, not the +// session). Do NOT write `if signedInEnv() == ""` on the strength of a stale +// reading of this: it cannot fire. func signedInEnv() string { cfg, err := config.Load() if err != nil || cfg == nil { - return "" + cfg = &config.Config{} // unreadable == no stored session } - return cfg.CurrentEnv + return sessionEnv(cfg) } // processInstanceID is the per-PROCESS id §2 asks for off-cluster. @@ -150,13 +175,6 @@ func processInstanceID() string { return hex.EncodeToString(b) } -// pendingSink is the transport seam for backend#1905. -// -// nil means validate-and-drop (telemetry.SetSink's documented contract). When -// the ingest endpoint lands this returns the client that posts to it, and -// nothing else in this file changes. -func pendingSink() telemetry.Sink { return nil } - // RecordCommandOutcome emits the single terminal event for this invocation. // main.go calls it once, after the command tree has returned and before exit. // @@ -164,23 +182,38 @@ func pendingSink() telemetry.Sink { return nil } // was unhappy would be a strictly worse CLI. A malformed event is caught by the // tests below, where it is free. func RecordCommandOutcome(root, executed *cobra.Command, info BuildInfo, exitCode int, elapsed time.Duration) { - _ = recordCommandOutcome(root, executed, info, exitCode, elapsed, os.Getenv, pendingSink()) + // THE ONE RESOLUTION POINT for this invocation. The label, the spool and the + // POST destination are all derived from this single value: two independent + // resolutions is how a record ends up labelled `stg` and posted to prod. + // + // It is resolved here and threaded down as a parameter rather than re-read + // inside recordCommandOutcome, because "both call telemetryEnv(signedInEnv())" + // is not one resolution — it is two config reads that merely tend to agree, + // and a `login` landing between them makes them disagree (Bugbot on #540). + env := telemetryEnv(signedInEnv()) + _ = recordCommandOutcome(root, executed, info, exitCode, elapsed, os.Getenv, env, pendingSink(env)) } -// recordCommandOutcome is RecordCommandOutcome with its two ambient -// dependencies passed in, so the tests drive the real thing. +// recordCommandOutcome is RecordCommandOutcome with its ambient dependencies +// passed in, so the tests drive the real thing. +// +// TAKES THE RESOLVED env, and never resolves one itself — the same rule, and for +// the same reason, as deliver/pendingSink in telemetry_transport.go: the emitter's +// label must be the value the sink was built from, not a second look at the +// config that happens to land on it. func recordCommandOutcome( root, executed *cobra.Command, info BuildInfo, exitCode int, elapsed time.Duration, getenv func(string) string, + env string, sink telemetry.Sink, ) error { if !telemetryEnabled(getenv) { return nil } - emitter := telemetry.New(telemetryEnv(signedInEnv()), info.Version, processInstanceID()) + emitter := telemetry.New(env, info.Version, processInstanceID()) if sink != nil { emitter.SetSink(sink) } diff --git a/internal/cli/telemetry_installer_spool.go b/internal/cli/telemetry_installer_spool.go new file mode 100644 index 0000000..3d6f7e0 --- /dev/null +++ b/internal/cli/telemetry_installer_spool.go @@ -0,0 +1,163 @@ +package cli + +// Draining the INSTALLER's spool — backend#2217, option (b). +// +// THE PROBLEM THIS SOLVES. `scripts/lib/telemetry.sh` produces one contract event +// per install run and spools it, but it cannot deliver: the ingest endpoint needs +// a bearer credential and the installer holds only `TRACEBLOC_CLIENT_ID` / +// `TRACEBLOC_CLIENT_PASSWORD`, a provisioning pair with no exchange for a token. +// It also never reads the CLI's config. So its records had no route at all, and +// #1906 cannot help — that is a pod Collector reading container stdout, and these +// are files on the operator's own machine, often written when no cluster exists. +// +// WHY THE CLI AND NOT THE INSTALLER. The CLI already owns the token (device login +// writes it to `~/.tracebloc/config.json`), and since #542 it already owns a +// spool, a drain loop and the OTLP mapping. Teaching it one more file to read +// makes the CLI the single credential holder and leaves the installer with no +// credential handling at all. The alternative — the installer reading the CLI's +// config — delivers nothing for the failures that matter most: `validate_config` +// and `early_data_dir_guard` run BEFORE provisioning, so there is no token on +// disk yet at the moment those events are written. Those are precisely the +// failures the installer's `$TMPDIR` fallback exists to preserve. +// +// THE FALLBACK PATH IS A GLOB, NOT AN INDEX. `_telemetry_fallback_spool` uses +// `mktemp .../tracebloc-telemetry-XXXXXX`, so the exact name is unpredictable but +// the PATTERN is fixed. Globbing it needs no change to the installer and no new +// shared state — an index file would be a second thing to keep in step, and the +// thing it indexed would still have to exist. +// +// EVERY RECORD IS FILTERED BY ITS OWN ENVIRONMENT, and this is not optional. The +// CLI's own spool is partitioned by env in the FILENAME (see telemetrySpoolPath); +// the installer's is not, and its records carry whatever `CLIENT_ENV` that run +// used. Forwarding them blind would post a `prod`-labelled install failure to +// whichever backend this CLI invocation happens to point at — the exact +// label-versus-destination leak #542's second review finding was about. So a +// record is forwarded only when its `deployment.environment` matches this run's, +// and the rest are left where they are for a future invocation against that env. + +import ( + "os" + "path/filepath" + "strings" +) + +const ( + // installerSpoolGlob matches `_telemetry_fallback_spool`'s mktemp template. + installerSpoolGlob = "tracebloc-telemetry-*" + + // installerDrainMax bounds how many installer records one invocation carries, + // on top of the CLI's own. Deliberately small: the installer's spool caps at + // TB_TELEMETRY_SPOOL_MAX=50, and a `tracebloc login` should not turn into the + // largest request this host has ever sent. + installerDrainMax = 10 +) + +// resourceEnvironment is the attribute the filter reads. Resource scope, so it is +// on the resource map rather than the record's own attributes. +const resourceEnvironment = "deployment.environment" + +// installerSpoolFiles returns every file that may hold installer records. +// +// Ordered predictable-first so a run with both delivers the data-dir spool before +// the scratch files, which is the order they were written in the common case. +// Missing files and unreadable directories are simply absent from the result: +// this runs on the exit path of every command and may never report a problem. +func installerSpoolFiles(getenv func(string) string) []string { + var out []string + + // 1. The data-dir spool, which the installer writes once HOST_DATA_DIR exists. + // Same default the installer uses: $HOST_DATA_DIR, else ~/.tracebloc. + base := strings.TrimSpace(getenv("HOST_DATA_DIR")) + if base == "" { + if home, err := os.UserHomeDir(); err == nil { + base = filepath.Join(home, ".tracebloc") + } + } + if base != "" { + out = append(out, filepath.Join(base, "telemetry", "pending.jsonl")) + } + + // 2. The pre-log fallback files. `_telemetry_fallback_dir` picks $TMPDIR, else + // $HOME, else /tmp — and disqualifies $TMPDIR when the installer is running + // from inside it. From here we cannot tell which it chose, so all three are + // candidates; a glob that matches nothing costs one syscall. + seen := map[string]bool{} + for _, dir := range []string{ + strings.TrimSpace(getenv("TMPDIR")), + strings.TrimSpace(getenv("HOME")), + "/tmp", + } { + dir = strings.TrimRight(dir, "/") + if dir == "" || seen[dir] { + continue + } + seen[dir] = true + matches, err := filepath.Glob(filepath.Join(dir, installerSpoolGlob)) + if err != nil { + continue + } + out = append(out, matches...) + } + return out +} + +// installerRecords reads events for `env` out of the installer's spools. +// +// Returns the matching events and, per file, the events that did NOT match so the +// caller can write them back. A file whose records are all foreign is left +// untouched rather than rewritten — rewriting another component's file to change +// nothing is a needless risk on a path that must never disturb an install. +type installerBatch struct { + // Events for this environment, ready to forward. + events []spooledEvent + // Per source file, the events that stay behind. Only files that actually + // contributed a forwarded event appear here. + remainder map[string][]spooledEvent +} + +func installerRecords(files []string, env string, max int) installerBatch { + batch := installerBatch{remainder: map[string][]spooledEvent{}} + for _, path := range files { + if len(batch.events) >= max { + return batch + } + records := readSpool(path) + if len(records) == 0 { + continue + } + var mine, theirs []spooledEvent + for _, rec := range records { + // A record with no environment is NOT forwarded. The contract omits an + // attribute rather than sending it empty, so an absent environment + // means the emitter could not resolve one — and a record no query can + // filter on is the defect the contract exists to remove. Left in place + // rather than dropped: it is still evidence, just not deliverable. + if rec.Resource[resourceEnvironment] == env && len(batch.events)+len(mine) < max { + mine = append(mine, rec) + continue + } + theirs = append(theirs, rec) + } + if len(mine) == 0 { + continue + } + batch.events = append(batch.events, mine...) + batch.remainder[path] = theirs + } + return batch +} + +// clearInstallerRecords rewrites each source file with only the records that were +// left behind, deleting it when nothing remains. +// +// CALLED ONLY AFTER A SUCCESSFUL DELIVERY. On any failure the files are untouched, +// so the next invocation retries them — the same posture as the CLI's own spool. +// Reuses `writeSpool`, so these files inherit 0600 and the atomic temp+rename. +func clearInstallerRecords(remainder map[string][]spooledEvent) { + for path, keep := range remainder { + // writeSpool removes the file when `keep` is empty, which is right for the + // mktemp fallbacks: the installer writes exactly one record per file and + // never returns to them, so an emptied one is litter. + _ = writeSpool(path, keep) + } +} diff --git a/internal/cli/telemetry_installer_spool_test.go b/internal/cli/telemetry_installer_spool_test.go new file mode 100644 index 0000000..9dedcac --- /dev/null +++ b/internal/cli/telemetry_installer_spool_test.go @@ -0,0 +1,319 @@ +package cli + +// Tests for draining the installer's spool — backend#2217 option (b). +// +// The risky property here is not "does it deliver" but "does it deliver the RIGHT +// records to the RIGHT backend, and leave everything else alone". These files +// belong to another component, so every test that asserts something was sent also +// asserts what was NOT sent and what survived on disk. + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// installerEvent mimics what scripts/lib/telemetry.sh writes: the compact +// {resource, attributes} shape, with the environment on the RESOURCE layer. +func installerEvent(env, instance string, exit int) spooledEvent { + return spooledEvent{ + Resource: map[string]string{ + "service.name": "installer", + "tracebloc.component": "install", + "deployment.environment": env, + "service.instance.id": instance, + }, + Attributes: map[string]any{ + "event.name": "install.run.failed", + "error.type": "preflight", + "tracebloc.install.exit_code": exit, + }, + } +} + +func writeInstallerSpool(t *testing.T, path string, events ...spooledEvent) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + var b strings.Builder + for _, ev := range events { + line, err := json.Marshal(ev) + if err != nil { + t.Fatalf("marshal: %v", err) + } + b.Write(line) + b.WriteString("\n") + } + if err := os.WriteFile(path, []byte(b.String()), 0o600); err != nil { + t.Fatalf("write: %v", err) + } +} + +// ─────────────────────────────────────────────────── locating the spool files + +func TestInstallerSpoolFilesFindsBothShapes(t *testing.T) { + dataDir := t.TempDir() + tmpDir := t.TempDir() + // The predictable data-dir spool. + dataSpool := filepath.Join(dataDir, "telemetry", "pending.jsonl") + writeInstallerSpool(t, dataSpool, installerEvent("prod", "a", 1)) + // A pre-log fallback, whose exact name mktemp chose and we cannot predict. + fallback := filepath.Join(tmpDir, "tracebloc-telemetry-Ab3xY9") + writeInstallerSpool(t, fallback, installerEvent("prod", "b", 2)) + // A file that is NOT ours, in the same directory. + if err := os.WriteFile(filepath.Join(tmpDir, "unrelated.jsonl"), []byte("{}\n"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + getenv := func(k string) string { + switch k { + case "HOST_DATA_DIR": + return dataDir + case "TMPDIR": + return tmpDir + } + return "" + } + files := installerSpoolFiles(getenv) + + var sawData, sawFallback, sawUnrelated bool + for _, f := range files { + switch { + case f == dataSpool: + sawData = true + case f == fallback: + sawFallback = true + case strings.HasSuffix(f, "unrelated.jsonl"): + sawUnrelated = true + } + } + if !sawData { + t.Errorf("the data-dir spool was not found; got %v", files) + } + if !sawFallback { + t.Errorf("the mktemp fallback was not found by glob; got %v", files) + } + if sawUnrelated { + t.Errorf("an unrelated file matched the glob; got %v", files) + } +} + +func TestInstallerSpoolFilesDoesNotDuplicateOneDirectory(t *testing.T) { + dir := t.TempDir() + writeInstallerSpool(t, filepath.Join(dir, "tracebloc-telemetry-Zz1"), installerEvent("prod", "a", 1)) + // TMPDIR and HOME pointing at the same place must not yield the file twice — + // a duplicate would send the same install outcome twice in one batch. + getenv := func(k string) string { + if k == "TMPDIR" || k == "HOME" { + return dir + } + return "" + } + files := installerSpoolFiles(getenv) + count := 0 + for _, f := range files { + if strings.Contains(f, "tracebloc-telemetry-Zz1") { + count++ + } + } + if count != 1 { + t.Errorf("the same fallback file appears %d times in %v", count, files) + } +} + +// ─────────────────────────────────────────────── filtering by environment + +func TestInstallerRecordsOnlyTakesThisEnvironment(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "pending.jsonl") + writeInstallerSpool(t, path, + installerEvent("prod", "prod-run", 1), + installerEvent("stg", "stg-run", 2), + installerEvent("prod", "prod-run-2", 3), + ) + + batch := installerRecords([]string{path}, "prod", installerDrainMax) + + if len(batch.events) != 2 { + t.Fatalf("want the 2 prod records, got %d", len(batch.events)) + } + for _, ev := range batch.events { + if got := ev.Resource["deployment.environment"]; got != "prod" { + t.Errorf("forwarded a %q record while draining prod", got) + } + } + // The stg record must be RETAINED, not dropped: it is deliverable by a later + // invocation against stg, and discarding another environment's evidence is + // not this function's call to make. + keep := batch.remainder[path] + if len(keep) != 1 || keep[0].Resource["deployment.environment"] != "stg" { + t.Errorf("the stg record should be left behind for a later stg run; remainder=%v", keep) + } +} + +func TestInstallerRecordsLeavesRecordsWithNoEnvironment(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "pending.jsonl") + noEnv := installerEvent("prod", "x", 1) + delete(noEnv.Resource, "deployment.environment") + writeInstallerSpool(t, path, noEnv) + + batch := installerRecords([]string{path}, "prod", installerDrainMax) + if len(batch.events) != 0 { + t.Errorf("a record with no environment must not be forwarded; got %d", len(batch.events)) + } + if len(batch.remainder) != 0 { + t.Errorf("a file that contributed nothing must not be scheduled for rewrite; got %v", batch.remainder) + } +} + +func TestInstallerRecordsRespectsTheDrainCap(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "pending.jsonl") + var events []spooledEvent + for i := 0; i < installerDrainMax+7; i++ { + events = append(events, installerEvent("prod", "run", i)) + } + writeInstallerSpool(t, path, events...) + + batch := installerRecords([]string{path}, "prod", installerDrainMax) + if len(batch.events) != installerDrainMax { + t.Fatalf("want exactly %d records, got %d", installerDrainMax, len(batch.events)) + } + // The overflow must survive, or a big installer spool loses records silently. + if got := len(batch.remainder[path]); got != 7 { + t.Errorf("want the 7 uncarried records retained, got %d", got) + } +} + +func TestInstallerRecordsSkipsAFileWithNothingForUs(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "pending.jsonl") + writeInstallerSpool(t, path, installerEvent("stg", "s", 1)) + before, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read: %v", err) + } + + batch := installerRecords([]string{path}, "prod", installerDrainMax) + + // THE PRIMARY ASSERTION IS ON `remainder`, not on the file's bytes. A byte + // comparison is satisfied by a rewrite that happens to produce identical + // content — which is exactly what a rewrite of unchanged records does, so the + // first version of this test passed under the mutation "rewrite files we took + // nothing from". `remainder` is the actual contract: only files that + // contributed a forwarded record may appear in it. + if _, scheduled := batch.remainder[path]; scheduled { + t.Errorf("a file we took nothing from was scheduled for rewrite; remainder=%v", batch.remainder) + } + + clearInstallerRecords(batch.remainder) + + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("the file must not be removed when nothing was taken: %v", err) + } + if string(before) != string(after) { + t.Errorf("a file we took nothing from lost content:\n before %q\n after %q", before, after) + } +} + +// ─────────────────────────────────────────────── end to end through deliver + +func TestDeliverCarriesTheInstallersRecords(t *testing.T) { + ownSpool := withTempConfigDir(t, "prod") + dataDir := t.TempDir() + t.Setenv("HOST_DATA_DIR", dataDir) + t.Setenv("TMPDIR", t.TempDir()) + + installerSpool := filepath.Join(dataDir, "telemetry", "pending.jsonl") + writeInstallerSpool(t, installerSpool, + installerEvent("prod", "installer-prod", 2), + installerEvent("dev", "installer-dev", 3), + ) + + var body string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + body = string(b) + w.WriteHeader(http.StatusAccepted) + })) + defer srv.Close() + + deliver(ownSpool, srv.URL+telemetryIngestPath, "tok", "prod", + event("cli-run", 0), time.Now()) + + if body == "" { + t.Fatal("nothing was POSTed") + } + if !strings.Contains(body, "installer-prod") { + t.Errorf("the installer's prod record was not carried: %s", body) + } + if strings.Contains(body, "installer-dev") { + t.Errorf("a dev-labelled installer record was sent to the prod endpoint: %s", body) + } + if !strings.Contains(body, "cli-run") { + t.Errorf("the CLI's own event was lost: %s", body) + } + // Delivered records are gone; the foreign-env one survives. + left := readSpool(installerSpool) + if len(left) != 1 || left[0].Resource["service.instance.id"] != "installer-dev" { + t.Errorf("after delivery the installer spool should hold only the dev record; got %v", left) + } +} + +func TestDeliverLeavesTheInstallerSpoolAloneOnFailure(t *testing.T) { + ownSpool := withTempConfigDir(t, "prod") + dataDir := t.TempDir() + t.Setenv("HOST_DATA_DIR", dataDir) + t.Setenv("TMPDIR", t.TempDir()) + + installerSpool := filepath.Join(dataDir, "telemetry", "pending.jsonl") + writeInstallerSpool(t, installerSpool, installerEvent("prod", "installer-prod", 2)) + before, err := os.ReadFile(installerSpool) + if err != nil { + t.Fatalf("read: %v", err) + } + + // Unreachable endpoint. + deliver(ownSpool, "http://127.0.0.1:1"+telemetryIngestPath, "tok", "prod", + event("cli-run", 0), time.Now()) + + after, err := os.ReadFile(installerSpool) + if err != nil { + t.Fatalf("the installer spool must survive a failed delivery: %v", err) + } + if string(before) != string(after) { + t.Errorf("a failed delivery modified the installer's spool:\n before %q\n after %q", before, after) + } +} + +func TestDeliverLeavesTheInstallerSpoolAloneWhenNotSignedIn(t *testing.T) { + ownSpool := withTempConfigDir(t, "prod") + dataDir := t.TempDir() + t.Setenv("HOST_DATA_DIR", dataDir) + t.Setenv("TMPDIR", t.TempDir()) + + installerSpool := filepath.Join(dataDir, "telemetry", "pending.jsonl") + writeInstallerSpool(t, installerSpool, installerEvent("prod", "installer-prod", 2)) + before, _ := os.ReadFile(installerSpool) + + // No token: nothing is attempted, so nothing of the installer's is consumed. + deliver(ownSpool, "http://127.0.0.1:1"+telemetryIngestPath, "", "prod", + event("cli-run", 0), time.Now()) + + after, err := os.ReadFile(installerSpool) + if err != nil { + t.Fatalf("the installer spool must survive an unauthenticated run: %v", err) + } + if string(before) != string(after) { + t.Errorf("an unauthenticated run modified the installer's spool") + } +} diff --git a/internal/cli/telemetry_otlp.go b/internal/cli/telemetry_otlp.go new file mode 100644 index 0000000..dfaf1b0 --- /dev/null +++ b/internal/cli/telemetry_otlp.go @@ -0,0 +1,174 @@ +package cli + +// The OTLP/HTTP JSON wire shape, converted AT THE SEAM — backend#2217. +// +// The emitter and the spool keep the compact `{resource, attributes}` shape: +// it is what a human reading a spool file wants, and older spooled files stay +// readable when this mapping changes. Only delivery speaks OTLP. +// +// The receiver is `common/telemetry/otlp.py::parse_export_logs_request` +// (backend#2213). Two of its rules are load-bearing here and are asserted by +// the tests rather than trusted: +// +// 1. ONE `resourceLogs` ENTRY PER EVENT, never one for the batch. +// `service.instance.id` is fresh per run and `service.version` legitimately +// differs between runs, so a drained spool carries several genuinely +// different resources. Collapsing them attributes every event to whichever +// run happened to be first. +// 2. int64 IS A STRING in proto3 JSON. The canonical encoding of 41230 is +// "41230". The receiver takes both, but sending the canonical form means +// this payload is also readable by any stock OTLP consumer. +// +// No `timeUnixNano`. Events arrive late by design once spooling is in play, and +// the #2213 decision deliberately sends no client-side timing — so the receiver +// stamps arrival and "how late" is knowingly invisible. Adding an event clock +// here would be a contract change, not an implementation detail. + +import ( + "encoding/json" + "reflect" + "sort" + "strconv" +) + +// spooledEvent is one emitted occurrence, in the shape the spool stores. +type spooledEvent struct { + Resource map[string]string `json:"resource"` + Attributes map[string]any `json:"attributes"` +} + +type otlpAnyValue struct { + StringValue *string `json:"stringValue,omitempty"` + BoolValue *bool `json:"boolValue,omitempty"` + IntValue *string `json:"intValue,omitempty"` + DoubleValue *float64 `json:"doubleValue,omitempty"` +} + +type otlpAttr struct { + Key string `json:"key"` + Value otlpAnyValue `json:"value"` +} + +type otlpResource struct { + Attributes []otlpAttr `json:"attributes"` +} + +type otlpLogRecord struct { + Attributes []otlpAttr `json:"attributes"` +} + +type otlpScopeLogs struct { + LogRecords []otlpLogRecord `json:"logRecords"` +} + +type otlpResourceLogs struct { + Resource otlpResource `json:"resource"` + ScopeLogs []otlpScopeLogs `json:"scopeLogs"` +} + +type otlpExportLogsServiceRequest struct { + ResourceLogs []otlpResourceLogs `json:"resourceLogs"` +} + +// anyValue renders one attribute value as an OTLP AnyValue. +// +// BY KIND, NOT BY CONCRETE TYPE, and that is not a style choice. The emitter's +// own `checkAttrValue` accepts every scalar KIND via reflection precisely so an +// idiomatic caller can pass a named type (`type Reason string`, or a +// time.Duration through telemetry.Duration). A `switch v := value.(type)` here +// would match the DYNAMIC type, miss those, and drop at the seam exactly the +// values the emitter went out of its way to admit — a silent hole one layer +// below the check that permits them. +func anyValue(value any) (otlpAnyValue, bool) { + // json.Number FIRST, and the ordering is load-bearing rather than tidy. + // json.Number is a NAMED STRING TYPE, so the kind switch below would match + // reflect.String and emit `stringValue` — turning an exit code into a string + // on the drained path, which is a different wrong answer from the float64 one + // this case exists to fix. A value read back from the spool has to be able to + // say whether it was an integer, and only this branch can. + if num, ok := value.(json.Number); ok { + if i, err := num.Int64(); err == nil { + s := strconv.FormatInt(i, 10) + return otlpAnyValue{IntValue: &s}, true + } + if f, err := num.Float64(); err == nil { + return otlpAnyValue{DoubleValue: &f}, true + } + // Neither an integer nor a real: not a number this contract can carry. + return otlpAnyValue{}, false + } + rv := reflect.ValueOf(value) + switch rv.Kind() { + case reflect.String: + s := rv.String() + return otlpAnyValue{StringValue: &s}, true + case reflect.Bool: + b := rv.Bool() + return otlpAnyValue{BoolValue: &b}, true + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + s := strconv.FormatInt(rv.Int(), 10) + return otlpAnyValue{IntValue: &s}, true + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + s := strconv.FormatUint(rv.Uint(), 10) + return otlpAnyValue{IntValue: &s}, true + case reflect.Float32, reflect.Float64: + f := rv.Float() + return otlpAnyValue{DoubleValue: &f}, true + default: + // reflect.Invalid (a nil `any`) lands here too. The emitter already + // omits absent values, so reaching this is a defect upstream rather + // than a value to guess at — dropped, and the batch still ships. + return otlpAnyValue{}, false + } +} + +// attrsFromStrings renders the resource layer. Sorted so a payload is +// byte-stable for a given input, which is what lets the tests assert on it. +func attrsFromStrings(m map[string]string) []otlpAttr { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + out := make([]otlpAttr, 0, len(keys)) + for _, k := range keys { + v := m[k] + out = append(out, otlpAttr{Key: k, Value: otlpAnyValue{StringValue: &v}}) + } + return out +} + +func attrsFromAny(m map[string]any) []otlpAttr { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + out := make([]otlpAttr, 0, len(keys)) + for _, k := range keys { + value, ok := anyValue(m[k]) + if !ok { + continue + } + out = append(out, otlpAttr{Key: k, Value: value}) + } + return out +} + +// otlpPayload renders a batch as one ExportLogsServiceRequest. +func otlpPayload(events []spooledEvent) ([]byte, error) { + doc := otlpExportLogsServiceRequest{ + ResourceLogs: make([]otlpResourceLogs, 0, len(events)), + } + for _, ev := range events { + doc.ResourceLogs = append(doc.ResourceLogs, otlpResourceLogs{ + Resource: otlpResource{Attributes: attrsFromStrings(ev.Resource)}, + ScopeLogs: []otlpScopeLogs{{ + LogRecords: []otlpLogRecord{{ + Attributes: attrsFromAny(ev.Attributes), + }}, + }}, + }) + } + return json.Marshal(doc) +} diff --git a/internal/cli/telemetry_test.go b/internal/cli/telemetry_test.go index 61de040..0505677 100644 --- a/internal/cli/telemetry_test.go +++ b/internal/cli/telemetry_test.go @@ -49,8 +49,13 @@ func captureOutcome( delivered++ }) getenv := func(k string) string { return env[k] } + // Resolve the env the way RecordCommandOutcome does and hand the SAME value + // to the recorder, so these tests exercise the production path end to end + // (config -> label) now that recordCommandOutcome no longer resolves for + // itself. TestTheRecordIsLabelledWithTheEnvItWasHanded pins the threading. if err := recordCommandOutcome( - root, executed, testBuildInfo(), exitCode, 1500*time.Millisecond, getenv, sink, + root, executed, testBuildInfo(), exitCode, 1500*time.Millisecond, getenv, + telemetryEnv(signedInEnv()), sink, ); err != nil { t.Fatalf("recordCommandOutcome: %v", err) } @@ -298,8 +303,8 @@ func TestTheEnvironmentLabelMatchesTheBackend(t *testing.T) { func TestASignedInUnknownEnvIgnoresClientEnv(t *testing.T) { // The bug this pins (Asad, cli#528 review): the client resolves a signed-in - // env via sessionEnv, which returns cfg.CurrentEnv VERBATIM and never consults - // $CLIENT_ENV — so a config on "banana" talks to prod (api.BaseURL default) + // env via sessionEnv, which normalises cfg.CurrentEnv but never falls back to + // $CLIENT_ENV while it is set — so a config on "banana" talks to prod (api.BaseURL default) // regardless of $CLIENT_ENV. The old code resolved the label through // ResolveEnv, which DOES read $CLIENT_ENV, so it filed the run under "dev" // while every request went to prod. The label must be prod, not dev. @@ -356,8 +361,9 @@ func TestTheSignedInEnvironmentWins(t *testing.T) { func TestASignedInUnknownEnvironmentIsLabelledProd(t *testing.T) { // A run signed into an environment the CLI does not recognise talks to prod - // (sessionEnv hands cfg.CurrentEnv to api.New verbatim, api.BaseURL routes the - // unknown value to prod), so its record must be filed under prod — that + // (sessionEnv normalises but does not validate cfg.CurrentEnv, so the unknown + // value reaches api.New and api.BaseURL routes it to prod), so its record must + // be filed under prod — that // failed-install-on-prod run is exactly what this feature exists to capture. dir := t.TempDir() t.Setenv("TRACEBLOC_CONFIG_DIR", dir) diff --git a/internal/cli/telemetry_transport.go b/internal/cli/telemetry_transport.go new file mode 100644 index 0000000..e9d7121 --- /dev/null +++ b/internal/cli/telemetry_transport.go @@ -0,0 +1,465 @@ +package cli + +// Host telemetry transport for the CLI — backend#2217, option (c). +// +// WHAT WAS HERE BEFORE: `pendingSink()` returned nil, so every event was +// validated and dropped. The endpoint now exists (backend#1905/#2213), so this +// file is the delivery half. +// +// THE SHAPE, decided on backend#2217 rather than invented here: one inline POST +// attempt with a ~1s budget, spool to disk on any failure, drain a bounded +// number of pending events on the next invocation. The deciding argument +// against plain fire-and-forget is that IN A SHORT-LIVED CLI, ASYNC DELIVERY IS +// A LIE — a goroutine that outlives main() is killed at exit, so the only honest +// forms of "don't block" are "always block" or "always drop on failure", and +// dropping on failure discards precisely the partition-time events that are the +// most valuable thing the CLI can report. Hence: block briefly, then persist. +// +// Explicit non-goals, so nobody adds them later believing they were forgotten: +// no background daemon, no goroutine outliving the process, no retry loop. +// ONE attempt. The next invocation is the retry. +// +// THE SPOOL KEEPS DROP-OLDEST, AND THAT IS NOT A CONTRADICTION OF D7. D7's +// amended overflow row says drop-NEWEST because `exporterhelper` sheds at the +// entrance and offers nothing else — a platform constraint on the edge +// Collector, not a preference. This spool is our own code and can do what D7 +// originally WANTED, so it does: the newest records describe the incident in +// progress. Same reasoning as `scripts/lib/telemetry.sh`'s `tail -n` trim in the +// installer, which this mirrors deliberately (0600, capped, oldest dropped). +// +// EVERY FAILURE PATH HERE IS SILENT AND RETURNS. A CLI that printed a warning +// because telemetry could not reach the backend would be a worse CLI, and the +// emitter's own rule is that no telemetry path may fail or delay the product. + +import ( + "bufio" + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "os" + "path/filepath" + "strings" + "sync/atomic" + "time" + + "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/telemetry" +) + +const ( + // telemetryIngestPath is the ingest boundary's versioned path + // (backend/metaApi/urls.py). Versioned because a spooled payload written by + // an old CLI can arrive after the backend has moved on. + telemetryIngestPath = "/telemetry/v1/records/" + + // telemetryBudget bounds the WHOLE telemetry step — drain included — not + // each attempt. #2217 sets ~1s per command. + telemetryBudget = time.Second + + // telemetrySpoolMax caps the spool, mirroring the installer's + // TB_TELEMETRY_SPOOL_MAX. Oldest are dropped past it. + telemetrySpoolMax = 50 + + // telemetryDrainMax bounds how many pending events one invocation carries. + // Smaller than the cap on purpose: a full spool must not turn the next + // command into the biggest request the endpoint sees from this host. + telemetryDrainMax = 20 + + // telemetrySpoolReadCap bounds a pathological file. We write this file, so + // it cannot legitimately exceed the cap — but a corrupted or hand-edited one + // must not be able to stall a CLI command. + telemetrySpoolReadCap = 1 << 20 // 1 MiB +) + +// telemetrySpoolPath is where unsent events wait, under the CLI's own config +// directory so it honours $TRACEBLOC_CONFIG_DIR like everything else. +// +// ONE SPOOL PER ENVIRONMENT, and the partition is a correctness requirement +// rather than tidiness. A single host-wide `pending.jsonl` leaks across +// invocations: records queued while signed in to prod get drained by the next +// command — which may be `tracebloc login --env dev` — and POSTed to dev's +// endpoint with dev's token while still carrying `deployment.environment=prod`. +// That is the same label-versus-destination mismatch `deliver` takes a resolved +// URL to prevent, reopened one level up, across invocations instead of inside +// one. (Bugbot on #542; reproduced — a prod-labelled record arrived at a dev +// endpoint before this change.) +// +// The consequence, stated rather than hidden: records for an environment the +// operator never uses again are never delivered. That is the right trade — they +// are capped, and there is usually no token for that environment to deliver them +// with anyway. Delivering them to the WRONG backend is not a better outcome than +// not delivering them. +func telemetrySpoolPath(env string) (string, error) { + dir, err := config.Dir() + if err != nil { + return "", err + } + return filepath.Join(dir, "telemetry", "pending-"+spoolEnvSlug(env)+".jsonl"), nil +} + +// spoolEnvSlug reduces env to something safe in a filename. +// +// The real values are a closed set (dev/stg/prod), so this never fires in +// practice — it exists because a path segment built from a string is a path +// traversal waiting for the day that string stops being closed, and "../.." is a +// worse bug than an ugly filename. Anything unexpected becomes one bucket rather +// than being silently dropped: an undeliverable record is still evidence. +func spoolEnvSlug(env string) string { + var b strings.Builder + for _, r := range strings.ToLower(strings.TrimSpace(env)) { + if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' { + b.WriteRune(r) + } + } + if b.Len() == 0 { + return "unknown" + } + return b.String() +} + +// readSpool returns every parseable event, oldest first. +// +// A malformed line is SKIPPED, not fatal: a torn write costs one record, and +// refusing the file would strand every good record behind it forever. +func readSpool(path string) []spooledEvent { + f, err := os.Open(path) // #nosec G304 -- the CLI's own spool under config.Dir() + if err != nil { + return nil + } + defer func() { _ = f.Close() }() + + var out []spooledEvent + scanner := bufio.NewScanner(io.LimitReader(f, telemetrySpoolReadCap)) + scanner.Buffer(make([]byte, 0, 64*1024), 1<<20) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" { + continue + } + var ev spooledEvent + // UseNumber, NOT json.Unmarshal — and this is the difference between the + // first delivery and every retry sending the same event two ways. + // + // A plain unmarshal decodes every JSON number into float64, so an + // `exit_code` that went out as a canonical `intValue` string on the + // in-memory attempt came back as a float and went out as `doubleValue` on + // the drained one. Same event, two encodings, and the wrong one on the + // path the spool exists for — the partition-time retry. `json.Number` + // defers the choice to `anyValue`, which can then still tell an integer + // from a real. (@saqlainsyed007 and Bugbot on #542; reproduced before + // fixing, and `TestSpoolRoundTripPreservesIntegerEncoding` is the + // regression — the pre-existing integer test worked on the in-memory + // shape only and stayed green through this.) + dec := json.NewDecoder(strings.NewReader(line)) + dec.UseNumber() + if err := dec.Decode(&ev); err != nil { + continue + } + if len(ev.Resource) == 0 && len(ev.Attributes) == 0 { + continue + } + out = append(out, ev) + } + return out +} + +// wipedHostDir records the directory THIS process deliberately removed — the +// `tracebloc delete` offboard on its default (no `--keep-data`) path. Spool +// writes under it are dropped for the rest of the process: see writeSpool. +// +// WHY A LATCH AND NOT A CHECK. main.go emits the command-outcome event AFTER the +// command tree returns, so the offboard has already deleted ~/.tracebloc by the +// time telemetry runs — and telemetry's spool lives INSIDE that tree +// (/telemetry/pending-.jsonl). "Does the dir exist?" is the +// wrong question: it doesn't, and writeSpool's job is to create it. The question +// is whether its absence is a fresh install (spool away) or a wipe the user just +// asked for (don't), and only the offboard knows which. So the offboard says so. +// +// backend#2314: `tracebloc delete` printed "✔ Removed local tracebloc data and +// config." and then the exit-path telemetry write re-created the tree behind it — +// empty when delivery succeeded, and holding an undeliverable event record when +// it didn't (after the wipe there is no token left to deliver with, so deliver +// takes its no-token spool branch every time). An explicit wipe that the CLI +// silently undoes on the way out is a broken promise, and a dropped telemetry +// record is unambiguously the cheaper loss. +// +// IT HOLDS THE WIPED DIRECTORY, NOT JUST A BOOLEAN, and that is about blast +// radius rather than precision for its own sake. A bare "telemetry is off now" +// flag is unscoped: it silences every later writeSpool in the process, including +// one for a path the offboard never touched. It is also permanently sticky in a +// test binary — `delete`'s own unit tests drive the real offboard, so a boolean +// latched there stays latched for every test that runs after it, and three +// unrelated spool tests failed exactly that way while this was being written. +// Recording the path answers the narrower and more useful question: is THIS +// spool inside the tree we removed? +// +// atomic.Value, not a plain string: the sink runs on the exit path while nothing +// else should still be writing, but "should" is not a guarantee and the race +// detector runs in CI. +var wipedHostDir atomic.Value // string + +// markHostStateWiped records the directory this process deliberately removed, so +// the exit-path telemetry write does not resurrect it. Called by the offboard. +func markHostStateWiped(dir string) { wipedHostDir.Store(dir) } + +// insideWipedHostDir reports whether path lies within a directory this process +// deliberately removed. +func insideWipedHostDir(path string) bool { + root, _ := wipedHostDir.Load().(string) + if root == "" { + return false + } + rel, err := filepath.Rel(root, path) + if err != nil { + // Different volumes, so not inside it. + return false + } + // filepath.Rel returns a ".."-prefixed path for anything outside root, and + // no error — so the prefix test, not the error, is what decides this. + return rel == "." || !strings.HasPrefix(rel, "..") +} + +// writeSpool replaces the spool with events, keeping the NEWEST +// telemetrySpoolMax and dropping the oldest past it. +// +// Written to a temp file and renamed, so a crash mid-write cannot leave a +// truncated spool. Two concurrent CLI commands can still lose a record to a +// last-writer-wins rename; that is accepted rather than locked, because a lock +// file on the exit path of every command is a new way for telemetry to hang the +// product, which is the one thing it may not do. +func writeSpool(path string, events []spooledEvent) error { + // The offboard removed the tree this spool lives in. Writing here would + // re-create it — see wipedHostDir (backend#2314). Nothing to remove either: + // the file went with the directory. + if insideWipedHostDir(path) { + return nil + } + if len(events) > telemetrySpoolMax { + events = events[len(events)-telemetrySpoolMax:] + } + dir := filepath.Dir(path) + if len(events) == 0 { + // Nothing to write, so do NOT create the directory on the way to + // deleting a file inside it — the MkdirAll used to run before this + // branch, which re-created a wiped ~/.tracebloc/telemetry/ even on the + // delivered path, where the spool is being emptied rather than filled + // (backend#2314). + err := os.Remove(path) + if err != nil && !os.IsNotExist(err) { + return err + } + return nil + } + if err := os.MkdirAll(dir, 0o700); err != nil { + return err + } + tmp, err := os.CreateTemp(dir, "pending-*.jsonl") + if err != nil { + return err + } + tmpName := tmp.Name() + defer func() { _ = os.Remove(tmpName) }() + + if err := tmp.Chmod(0o600); err != nil { + _ = tmp.Close() + return err + } + enc := json.NewEncoder(tmp) + for _, ev := range events { + if err := enc.Encode(ev); err != nil { + _ = tmp.Close() + return err + } + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmpName, path) +} + +// postOutcome classifies one delivery attempt. +type postOutcome int + +const ( + postDelivered postOutcome = iota // 2xx — the batch is gone + postRetry // spool it; a later run may succeed + postDiscard // permanently unacceptable; stop carrying it +) + +// classifyStatus maps an HTTP status to what to do with the batch. +// +// THE DISCARD CASE IS THE ONE THAT MATTERS. The endpoint answers 400 when a +// whole batch is unparseable. Re-spooling that would wedge the spool: the same +// bad batch would be re-sent by every future command, forever, and would push +// out good records at the cap. A permanent refusal must consume the batch. +// +// 401/403 retry rather than discard because they are a CREDENTIAL state, not a +// payload verdict — an expired token is refreshed by the next `tracebloc login`, +// and the records are still worth sending after it. 408/429 are explicitly +// transient. Everything 5xx is transient by definition. +func classifyStatus(code int) postOutcome { + switch { + case code >= 200 && code < 300: + return postDelivered + case code == http.StatusUnauthorized, code == http.StatusForbidden, + code == http.StatusRequestTimeout, code == http.StatusTooManyRequests: + return postRetry + case code >= 400 && code < 500: + return postDiscard + default: + return postRetry + } +} + +// postBatch makes the single delivery attempt. +func postBatch(ctx context.Context, url, token string, batch []spooledEvent) postOutcome { + payload, err := otlpPayload(batch) + if err != nil { + // Nothing a later run would render differently. + return postDiscard + } + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(payload)) + if err != nil { + return postDiscard + } + req.Header.Set("Content-Type", "application/json") + // Bearer, matching what internal/api already sends and what the endpoint's + // TelemetryIngestAuthentication expects (its ClientAccessTokenAuthentication + // base declares keyword = "Bearer"). The legacy per-user DRF token uses + // keyword "Token"; sending the wrong one 401s silently. + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := http.DefaultClient.Do(req) + if err != nil { + // Timeout, DNS, refused connection — exactly the partition case the + // spool exists for. + return postRetry + } + defer func() { _ = resp.Body.Close() }() + // Drained so the connection can be reused, and bounded so a hostile body + // cannot be read into memory on a path that must stay cheap. + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) + return classifyStatus(resp.StatusCode) +} + +// deliver is the sink body: drain what is pending, add this event, attempt once, +// and persist whatever did not land. +// +// TAKES THE RESOLVED SPOOL PATH AND URL, not an env to resolve for itself. Three +// reasons, and the last two each bit once: a single resolution point means the +// record's label, its spool and its destination cannot disagree; a `deliver` that +// computed api.BaseURL() internally had no seam — its own test posted to +// PRODUCTION; and one that computed the spool path internally drained another +// environment's queue into this one's endpoint (Bugbot on #542). +func deliver(spool, url, token, env string, ev spooledEvent, now time.Time) { + path := spool + pending := readSpool(path) + + // The batch is the OLDEST pending events plus this one. Oldest first so a + // spool that is persistently over the drain limit still makes progress + // through its backlog rather than re-sending its tail. + drained := pending + if len(drained) > telemetryDrainMax { + drained = drained[:telemetryDrainMax] + } + batch := make([]spooledEvent, 0, len(drained)+1) + batch = append(batch, drained...) + batch = append(batch, ev) + + // THE INSTALLER'S RECORDS RIDE ALONG (backend#2217 option b). It produces + // contract events and cannot deliver them — it holds a provisioning pair, not + // a bearer token — so the CLI, which does hold one, carries them. Filtered to + // THIS environment by each record's own `deployment.environment`, because the + // installer's spool is not partitioned by env the way ours is. + // + // Appended AFTER our own, so a full installer spool can never crowd out the + // event this invocation just produced. + installer := installerRecords(installerSpoolFiles(os.Getenv), env, installerDrainMax) + batch = append(batch, installer.events...) + + // No token means not signed in. Spool rather than attempt: the events are + // still worth sending after the next login, and a POST with no credential + // would spend the budget earning a 401. + if token == "" { + _ = writeSpool(path, append(pending, ev)) + // The installer's files are deliberately NOT touched here. Not signed in + // means they were never sent, and they are not ours to discard. + return + } + + ctx, cancel := context.WithDeadline(context.Background(), now.Add(telemetryBudget)) + defer cancel() + + switch postBatch(ctx, url, token, batch) { + case postDelivered, postDiscard: + // Both consume the batch. The difference is only whether the server + // stored it, and neither is a reason to carry it again. + _ = writeSpool(path, pending[len(drained):]) + // The installer's files are only ever touched on a path that CONSUMED + // them. A discard clears them too, for the same reason it clears ours: a + // permanently unparseable batch carried forever wedges every later send, + // and these records are as unparseable as the rest of it. + clearInstallerRecords(installer.remainder) + case postRetry: + _ = writeSpool(path, append(pending, ev)) + } +} + +// telemetryToken reads the bearer token for the CURRENT SESSION, best-effort. +// +// Read at delivery time rather than at startup so a command that signs in can +// deliver its own outcome event. +// +// TAKES NO env, AND THAT IS THE FIX (tracebloc/cli#552). It used to take the +// telemetry label and look the profile up by it — but profiles are keyed on the +// RAW cfg.CurrentEnv, while the label has been through telemetryEnv, which +// lower-cases, trims (via sessionEnv) and remaps anything unrecognised onto +// prod. Whenever those disagreed the lookup did not miss loudly: Profile() +// CREATED an empty profile and returned no token, so delivery took the +// no-token spool path forever while authedClient — reading cfg.Current(), the +// raw key — kept working. The CLI looked signed in and healthy, and outcomes +// simply never arrived. +// +// The parameter was the whole defect: two keys for one concept. Removing it +// makes them impossible to disagree, rather than making them agree today. +// +// The token still goes to the right host. api.BaseURL routes an unrecognised +// env to prod exactly as telemetryEnv does, so the destination the label picks +// is the destination this session's client already uses. (That BaseURL routes +// unknown envs to prod at all is a real defect, tracked across three components +// on backend#2171 — see telemetryEnv's note. It is not this ticket's to change, +// and this fix deliberately does not diverge from that behaviour.) +func telemetryToken() string { + cfg, err := config.Load() + if err != nil || cfg == nil { + return "" + } + return cfg.CurrentToken() +} + +// pendingSink is the transport for backend#2217. +// +// Returns nil — validate-and-drop, telemetry.SetSink's documented contract — +// only when there is nowhere to spool and nothing to post to. Everything else +// is handled inside deliver, silently. +func pendingSink(env string) telemetry.Sink { + // BOTH derived from the same `env`, once. The record's label comes from the + // same value (see RecordCommandOutcome), so label, spool and destination are + // three views of one resolution rather than three chances to disagree. + url := api.BaseURL(env) + telemetryIngestPath + spool, err := telemetrySpoolPath(env) + if err != nil { + // Nowhere to spool and therefore no safe way to retry. Validate-and-drop + // is telemetry.SetSink's documented contract for exactly this. + return nil + } + return func(resource map[string]string, record map[string]any) { + deliver(spool, url, telemetryToken(), env, spooledEvent{ + Resource: resource, + Attributes: record, + }, time.Now()) + } +} diff --git a/internal/cli/telemetry_transport_test.go b/internal/cli/telemetry_transport_test.go new file mode 100644 index 0000000..33ff446 --- /dev/null +++ b/internal/cli/telemetry_transport_test.go @@ -0,0 +1,796 @@ +package cli + +// Tests for the host transport — backend#2217. +// +// Every assertion here is written to FAIL on a specific defect, not to observe +// that the code ran. The review record on this epic (~40 findings across 13 PRs) +// is dominated by one class: a check that cannot fail, wearing a comment saying +// it can. Where a test's whole point is that some value reaches the wire, it +// asserts the value — never merely the absence of an error. + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// event builds a distinguishable event. +func event(instanceID string, exitCode int) spooledEvent { + return spooledEvent{ + Resource: map[string]string{ + "service.name": "cli", + "service.instance.id": instanceID, + "service.version": "1.2.3", + }, + Attributes: map[string]any{ + "event.name": "cli.command.failed", + "error.type": "usage", + "tracebloc.cli.exit_code": exitCode, + "tracebloc.cli.duration_ms": int64(41230), + "tracebloc.cli.interactive": false, + "tracebloc.cli.sampling_rate": 0.5, + }, + } +} + +// decode unmarshals a payload into the receiver's view of it. +func decode(t *testing.T, payload []byte) map[string]any { + t.Helper() + var doc map[string]any + if err := json.Unmarshal(payload, &doc); err != nil { + t.Fatalf("payload is not JSON: %v", err) + } + return doc +} + +// flatten reproduces what the receiver's `_flatten` does: resource attributes, +// then the record's own. Derived from common/telemetry/otlp.py so the assertions +// below are about what the BACKEND will see, not about our own nesting. +func flatten(t *testing.T, entry any) map[string]any { + t.Helper() + e, ok := entry.(map[string]any) + if !ok { + t.Fatalf("resourceLogs entry is not an object: %T", entry) + } + out := map[string]any{} + readAttrs := func(list any) { + items, ok := list.([]any) + if !ok { + return + } + for _, it := range items { + m, ok := it.(map[string]any) + if !ok { + continue + } + key, _ := m["key"].(string) + val, ok := m["value"].(map[string]any) + if !ok || key == "" { + continue + } + for _, kind := range []string{"stringValue", "boolValue", "intValue", "doubleValue"} { + if v, present := val[kind]; present { + out[key] = v + break + } + } + } + } + if res, ok := e["resource"].(map[string]any); ok { + readAttrs(res["attributes"]) + } + scopes, _ := e["scopeLogs"].([]any) + for _, s := range scopes { + sm, ok := s.(map[string]any) + if !ok { + continue + } + recs, _ := sm["logRecords"].([]any) + for _, r := range recs { + rm, ok := r.(map[string]any) + if !ok { + continue + } + readAttrs(rm["attributes"]) + } + } + return out +} + +func resourceLogs(t *testing.T, payload []byte) []any { + t.Helper() + rl, ok := decode(t, payload)["resourceLogs"].([]any) + if !ok { + t.Fatalf("payload has no resourceLogs list: %s", payload) + } + return rl +} + +// ---------------------------------------------------------------- the mapping + +// The rule the receiver's parser calls out by name, and the one a batching +// implementation gets wrong: each event keeps its OWN resource. +func TestEachEventGetsItsOwnResourceLogsEntry(t *testing.T) { + payload, err := otlpPayload([]spooledEvent{event("run-a", 1), event("run-b", 2)}) + if err != nil { + t.Fatalf("otlpPayload: %v", err) + } + entries := resourceLogs(t, payload) + if len(entries) != 2 { + t.Fatalf("want 2 resourceLogs entries (one per event), got %d", len(entries)) + } + // Assert the PAIRING, not just the count: a mapping that emitted two entries + // but copied the first event's resource into both would pass a count check. + got := map[string]any{} + for _, e := range entries { + flat := flatten(t, e) + got[fmt.Sprint(flat["service.instance.id"])] = flat["tracebloc.cli.exit_code"] + } + if got["run-a"] != "1" { + t.Errorf("run-a should carry exit_code 1, got %v (all: %v)", got["run-a"], got) + } + if got["run-b"] != "2" { + t.Errorf("run-b should carry exit_code 2, got %v (all: %v)", got["run-b"], got) + } +} + +// int64 as a JSON STRING is the canonical proto3 encoding. A number here is +// accepted by our receiver but not canonical, so it is asserted. +func TestIntegersAreEncodedAsStrings(t *testing.T) { + payload, err := otlpPayload([]spooledEvent{event("run", 2)}) + if err != nil { + t.Fatalf("otlpPayload: %v", err) + } + flat := flatten(t, resourceLogs(t, payload)[0]) + for _, key := range []string{"tracebloc.cli.exit_code", "tracebloc.cli.duration_ms"} { + v, ok := flat[key].(string) + if !ok { + t.Errorf("%s should be a JSON string (proto3 int64), got %T (%v)", key, flat[key], flat[key]) + continue + } + if v == "" { + t.Errorf("%s encoded as an empty string", key) + } + } + if !strings.Contains(string(payload), `"intValue":"2"`) { + t.Errorf(`payload should contain "intValue":"2"; got %s`, payload) + } +} + +// A bool must not become an int. The receiver reads boolValue BEFORE intValue +// for exactly this reason; the sender must not make it necessary. +func TestBoolsUseBoolValueNotIntValue(t *testing.T) { + payload, err := otlpPayload([]spooledEvent{event("run", 0)}) + if err != nil { + t.Fatalf("otlpPayload: %v", err) + } + flat := flatten(t, resourceLogs(t, payload)[0]) + if v, ok := flat["tracebloc.cli.interactive"].(bool); !ok || v { + t.Errorf("interactive should be boolean false, got %T (%v)", flat["tracebloc.cli.interactive"], flat["tracebloc.cli.interactive"]) + } +} + +// THE DEFECT THIS EXISTS FOR. The emitter admits named scalar types by +// reflect.Kind; a concrete type switch at the seam would drop them silently. +// This test fails if anyValue is ever rewritten as `switch v := value.(type)`. +func TestNamedScalarTypesSurviveTheSeam(t *testing.T) { + type reason string + type attempts int + ev := spooledEvent{ + Resource: map[string]string{"service.name": "cli"}, + Attributes: map[string]any{"tracebloc.cli.reason": reason("quota"), "tracebloc.cli.attempts": attempts(3)}, + } + payload, err := otlpPayload([]spooledEvent{ev}) + if err != nil { + t.Fatalf("otlpPayload: %v", err) + } + flat := flatten(t, resourceLogs(t, payload)[0]) + if flat["tracebloc.cli.reason"] != "quota" { + t.Errorf("named string type dropped at the seam: got %v", flat["tracebloc.cli.reason"]) + } + if flat["tracebloc.cli.attempts"] != "3" { + t.Errorf("named int type dropped at the seam: got %v", flat["tracebloc.cli.attempts"]) + } +} + +// ------------------------------------------------------------------ the spool + +func TestWriteSpoolKeepsNewestAndDropsOldest(t *testing.T) { + path := filepath.Join(t.TempDir(), "telemetry", "pending.jsonl") + events := make([]spooledEvent, 0, telemetrySpoolMax+5) + for i := 0; i < telemetrySpoolMax+5; i++ { + events = append(events, event(fmt.Sprintf("run-%02d", i), i)) + } + if err := writeSpool(path, events); err != nil { + t.Fatalf("writeSpool: %v", err) + } + got := readSpool(path) + if len(got) != telemetrySpoolMax { + t.Fatalf("spool should be capped at %d, got %d", telemetrySpoolMax, len(got)) + } + // DROP-OLDEST: run-00..run-04 are gone, run-05 is now first, and the last + // event survives. Asserting the identities, not the length — a trim that + // kept the WRONG end would also produce 50 records. + if id := got[0].Resource["service.instance.id"]; id != "run-05" { + t.Errorf("oldest survivor should be run-05 (oldest 5 dropped), got %q", id) + } + if id := got[len(got)-1].Resource["service.instance.id"]; id != "run-54" { + t.Errorf("newest event must survive the trim, got %q", id) + } +} + +func TestSpoolFileIsOwnerOnly(t *testing.T) { + path := filepath.Join(t.TempDir(), "telemetry", "pending.jsonl") + if err := writeSpool(path, []spooledEvent{event("run", 0)}); err != nil { + t.Fatalf("writeSpool: %v", err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("spool holds a bearer-token-adjacent payload; want 0600, got %04o", perm) + } +} + +func TestReadSpoolSkipsTornLinesAndKeepsTheRest(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "pending.jsonl") + good, err := json.Marshal(event("run-good", 7)) + if err != nil { + t.Fatalf("marshal: %v", err) + } + body := string(good) + "\n{\"resource\":{\"service.name\":\"cl\n" + string(good) + "\n" + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + got := readSpool(path) + if len(got) != 2 { + t.Fatalf("a torn line must cost one record, not the file; got %d records", len(got)) + } + for _, ev := range got { + if ev.Resource["service.instance.id"] != "run-good" { + t.Errorf("unexpected survivor: %v", ev.Resource) + } + } +} + +func TestWriteSpoolRemovesTheFileWhenEmpty(t *testing.T) { + path := filepath.Join(t.TempDir(), "telemetry", "pending.jsonl") + if err := writeSpool(path, []spooledEvent{event("run", 0)}); err != nil { + t.Fatalf("writeSpool: %v", err) + } + if err := writeSpool(path, nil); err != nil { + t.Fatalf("writeSpool(nil): %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("an empty spool should leave no file behind; stat err = %v", err) + } +} + +// ────────────────────────── the spool must not resurrect a wiped ~/.tracebloc +// +// backend#2314. `tracebloc delete` wipes ~/.tracebloc and prints "✔ Removed +// local tracebloc data and config."; main.go then emits the command-outcome +// event, whose spool lives INSIDE that tree. Both halves of the fix are asserted +// here because they cover different user paths — the empty-spool write is what a +// SUCCESSFUL delivery does (dir came back empty), and the latch is what an +// undelivered event needs (dir came back holding a record). + +// wipedHostState records dir as the offboard's removed tree for one test and +// clears it afterwards. The clear is belt-and-braces: the recorded path is a +// per-test TempDir, so it cannot match another test's spool even if it leaked. +func wipedHostState(t *testing.T, dir string) { + t.Helper() + markHostStateWiped(dir) + t.Cleanup(func() { wipedHostDir.Store("") }) +} + +func TestInsideWipedHostDirOnlyMatchesTheTreeThatWasRemoved(t *testing.T) { + // A sibling directory sharing a name PREFIX is the case a strings.HasPrefix + // check on the raw paths gets wrong: /tmp/a-cfg2 is not inside /tmp/a-cfg. + root := t.TempDir() + wipedHostState(t, filepath.Join(root, "cfg")) + + for _, tc := range []struct { + path string + want bool + }{ + {filepath.Join(root, "cfg"), true}, + {filepath.Join(root, "cfg", "telemetry", "pending-prod.jsonl"), true}, + {filepath.Join(root, "cfg2", "telemetry", "pending-prod.jsonl"), false}, + {filepath.Join(root, "other", "telemetry", "pending-prod.jsonl"), false}, + } { + if got := insideWipedHostDir(tc.path); got != tc.want { + t.Errorf("insideWipedHostDir(%q) = %v, want %v", tc.path, got, tc.want) + } + } +} + +func TestAnUnrelatedSpoolStillWritesAfterAnOffboard(t *testing.T) { + // The scoping that matters in practice: one process offboarded one tree, and + // that must not silence telemetry for a path it never touched. + wipedHostState(t, filepath.Join(t.TempDir(), "gone")) + + path := filepath.Join(t.TempDir(), "telemetry", "pending.jsonl") + if err := writeSpool(path, []spooledEvent{event("run-elsewhere", 0)}); err != nil { + t.Fatalf("writeSpool: %v", err) + } + if got := readSpool(path); len(got) != 1 { + t.Errorf("a spool outside the wiped tree must still be written; got %d records", len(got)) + } +} + +func TestWriteSpoolDoesNotCreateTheDirWhenThereIsNothingToWrite(t *testing.T) { + dir := filepath.Join(t.TempDir(), "telemetry") + path := filepath.Join(dir, "pending.jsonl") + + // The delivered path: the batch landed, so the spool is written EMPTY. It + // must not mkdir on its way to removing a file that isn't there — that is + // what re-created a just-wiped ~/.tracebloc/telemetry/ for every online + // offboard. + if err := writeSpool(path, nil); err != nil { + t.Fatalf("writeSpool(nil): %v", err) + } + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Errorf("writing an empty spool created %s; nothing was written, so nothing should be created (stat err = %v)", dir, err) + } +} + +func TestAWipedHostStateStopsTheSpoolComingBack(t *testing.T) { + dir := filepath.Join(t.TempDir(), "telemetry") + path := filepath.Join(dir, "pending.jsonl") + wipedHostState(t, dir) + + // A REAL event, i.e. the undelivered path — the one the offboard actually + // takes, because the wipe took the token with it and deliver then has + // nothing to post with. + if err := writeSpool(path, []spooledEvent{event("run-after-offboard", 0)}); err != nil { + t.Fatalf("writeSpool after a wipe must be a silent no-op, got: %v", err) + } + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Errorf("the offboard removed this tree; telemetry re-created %s (stat err = %v)", dir, err) + } +} + +// TestOffboardLeavesNothingBehindOnTheNoTokenPath is the end-to-end shape of the +// CI failure: the exact branch `tracebloc delete` reaches on the way out, run +// against a config dir the offboard has already removed. +func TestOffboardLeavesNothingBehindOnTheNoTokenPath(t *testing.T) { + path := withTempConfigDir(t, "prod") + cfgDir := os.Getenv("TRACEBLOC_CONFIG_DIR") + + // Stand where main.go stands: the offboard has run, the tree is gone, and + // the token went with it — so deliver takes its no-token spool branch. + if err := os.RemoveAll(cfgDir); err != nil { + t.Fatalf("simulate the offboard wipe: %v", err) + } + wipedHostState(t, cfgDir) + + deliver(path, "http://127.0.0.1:1"+telemetryIngestPath, "", "prod", event("run-offboard", 0), time.Now()) + + if _, err := os.Stat(cfgDir); !os.IsNotExist(err) { + t.Errorf("`tracebloc delete` promised the tree was removed; the exit-path telemetry write put %s back (stat err = %v)", cfgDir, err) + } +} + +// --------------------------------------------------------------- delivery + +func TestClassifyStatus(t *testing.T) { + cases := map[int]postOutcome{ + 200: postDelivered, 202: postDelivered, + 400: postDiscard, // a permanently unparseable batch must not wedge the spool + 404: postDiscard, + 401: postRetry, // credential state, not a payload verdict + 403: postRetry, + 408: postRetry, + 429: postRetry, + 500: postRetry, 502: postRetry, 503: postRetry, + } + for code, want := range cases { + if got := classifyStatus(code); got != want { + t.Errorf("classifyStatus(%d) = %v, want %v", code, got, want) + } + } +} + +func TestPostBatchSendsBearerAndJSON(t *testing.T) { + var gotAuth, gotType, gotPath string + var gotBody []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth = r.Header.Get("Authorization") + gotType = r.Header.Get("Content-Type") + gotPath = r.URL.Path + gotBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusAccepted) + })) + defer srv.Close() + + out := postBatch(context.Background(), srv.URL+telemetryIngestPath, "tok123", []spooledEvent{event("run", 1)}) + if out != postDelivered { + t.Fatalf("202 should be postDelivered, got %v", out) + } + // "Bearer", not "Token": the endpoint's ClientAccessTokenAuthentication + // declares keyword = "Bearer", and the wrong one 401s silently. + if gotAuth != "Bearer tok123" { + t.Errorf("Authorization = %q, want %q", gotAuth, "Bearer tok123") + } + if gotType != "application/json" { + t.Errorf("Content-Type = %q, want application/json", gotType) + } + if gotPath != telemetryIngestPath { + t.Errorf("path = %q, want %q", gotPath, telemetryIngestPath) + } + if !strings.Contains(string(gotBody), `"resourceLogs"`) { + t.Errorf("body should be an OTLP ExportLogsServiceRequest, got %s", gotBody) + } +} + +// The partition case: the server is unreachable, so the event must be on disk +// afterwards. This is the behaviour option (c) was chosen for. +func TestDeliverSpoolsWhenTheServerIsUnreachable(t *testing.T) { + path := withTempConfigDir(t, "prod") + // A closed listener's address: connection refused, immediately, and no + // request ever leaves the machine. + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + url := srv.URL + telemetryIngestPath + srv.Close() + + deliver(path, url, "tok", "prod", event("run-partition", 3), time.Now()) + + got := readSpool(path) + if len(got) != 1 { + t.Fatalf("an unreachable backend must leave the event spooled; got %d", len(got)) + } + if id := got[0].Resource["service.instance.id"]; id != "run-partition" { + t.Errorf("spooled the wrong event: %q", id) + } +} + +func TestDeliverSpoolsWhenNotSignedIn(t *testing.T) { + path := withTempConfigDir(t, "prod") + deliver(path, "http://127.0.0.1:1"+telemetryIngestPath, "", "prod", event("run-anon", 0), time.Now()) + got := readSpool(path) + if len(got) != 1 { + t.Fatalf("no token must spool rather than post; got %d records", len(got)) + } + if id := got[0].Resource["service.instance.id"]; id != "run-anon" { + t.Errorf("spooled the wrong event: %q", id) + } +} + +func TestDeliverKeepsTheSpoolBoundedAcrossManyFailures(t *testing.T) { + path := withTempConfigDir(t, "prod") + for i := 0; i < telemetrySpoolMax+10; i++ { + deliver(path, "http://127.0.0.1:1"+telemetryIngestPath, "", "prod", event(fmt.Sprintf("run-%02d", i), i), time.Now()) + } + got := readSpool(path) + if len(got) != telemetrySpoolMax { + t.Fatalf("spool must stay capped at %d across repeated failures, got %d", telemetrySpoolMax, len(got)) + } + if id := got[len(got)-1].Resource["service.instance.id"]; id != "run-59" { + t.Errorf("the most recent failure must be retained, got %q", id) + } +} + +// withTempConfigDir points config.Dir() at a temp dir and returns the spool path +// for `env`. +func withTempConfigDir(t *testing.T, env string) string { + t.Helper() + dir := t.TempDir() + t.Setenv("TRACEBLOC_CONFIG_DIR", dir) + path, err := telemetrySpoolPath(env) + if err != nil { + t.Fatalf("telemetrySpoolPath: %v", err) + } + if !strings.HasPrefix(path, dir) { + t.Fatalf("spool path %q should be under the overridden config dir %q", path, dir) + } + return path +} + +// ─────────────────────────────────────────── the spool must not recode numbers + +// THE DEFECT @saqlainsyed007 AND BUGBOT FOUND, as a regression test. +// +// `TestIntegersAreEncodedAsStrings` above operates on the in-memory shape and +// never touches writeSpool/readSpool, so it stayed green while every DRAINED +// record — the partition-time events the spool exists to preserve — encoded +// integers as `doubleValue`. The same event went out two ways, and the wrong way +// on the path the whole design is for. +// +// This asserts the two encodings AGREE, rather than asserting the round-trip +// looks right on its own: a test that only checked the drained payload could be +// satisfied by both paths being wrong together. +func TestSpoolRoundTripPreservesIntegerEncoding(t *testing.T) { + path := filepath.Join(t.TempDir(), "telemetry", "pending.jsonl") + ev := event("run-roundtrip", 2) + + direct, err := otlpPayload([]spooledEvent{ev}) + if err != nil { + t.Fatalf("otlpPayload(in-memory): %v", err) + } + if err := writeSpool(path, []spooledEvent{ev}); err != nil { + t.Fatalf("writeSpool: %v", err) + } + reloaded := readSpool(path) + if len(reloaded) != 1 { + t.Fatalf("expected 1 spooled event back, got %d", len(reloaded)) + } + drained, err := otlpPayload(reloaded) + if err != nil { + t.Fatalf("otlpPayload(drained): %v", err) + } + + // The in-memory path is the reference. Asserted explicitly so a regression + // there cannot make the comparison below pass by both sides breaking. + if !strings.Contains(string(direct), `"intValue":"2"`) { + t.Fatalf("the in-memory reference no longer encodes ints canonically: %s", direct) + } + if !strings.Contains(string(drained), `"intValue":"2"`) { + t.Errorf("a drained record must encode exit_code as intValue, not doubleValue.\n"+ + " in-memory: %s\n drained : %s", direct, drained) + } + // NOT "no doubleValue anywhere" — the fixture legitimately carries + // `sampling_rate: 0.5`, which MUST stay a double. The first draft of this + // test asserted the broad thing and failed on correct output, which would + // have been a test bug reported as a code bug. Assert per-attribute instead. + for _, intAttr := range []string{"tracebloc.cli.exit_code", "tracebloc.cli.duration_ms"} { + if !strings.Contains(string(drained), `"key":"`+intAttr+`","value":{"intValue":"`) { + t.Errorf("%s is not encoded as intValue on the drained path: %s", intAttr, drained) + } + } + + // Field by field, so the failure names WHICH attribute drifted rather than + // leaving a reader to diff two payloads by eye. + directFlat := flatten(t, resourceLogs(t, direct)[0]) + drainedFlat := flatten(t, resourceLogs(t, drained)[0]) + for _, key := range []string{ + "tracebloc.cli.exit_code", + "tracebloc.cli.duration_ms", + "tracebloc.cli.interactive", + "tracebloc.cli.sampling_rate", + "event.name", + "service.instance.id", + } { + if directFlat[key] != drainedFlat[key] { + t.Errorf("%s changed across the spool: in-memory %#v, drained %#v", + key, directFlat[key], drainedFlat[key]) + } + } +} + +// A real (non-integral) number must still come back as a double, or the fix for +// integers would silently truncate every float the contract allows. +func TestSpoolRoundTripKeepsRealsAsDoubles(t *testing.T) { + path := filepath.Join(t.TempDir(), "telemetry", "pending.jsonl") + ev := spooledEvent{ + Resource: map[string]string{"service.name": "cli"}, + Attributes: map[string]any{"tracebloc.cli.sampling_rate": 0.25}, + } + if err := writeSpool(path, []spooledEvent{ev}); err != nil { + t.Fatalf("writeSpool: %v", err) + } + drained, err := otlpPayload(readSpool(path)) + if err != nil { + t.Fatalf("otlpPayload: %v", err) + } + flat := flatten(t, resourceLogs(t, drained)[0]) + if got := flat["tracebloc.cli.sampling_rate"]; got != 0.25 { + t.Errorf("a real must survive the spool as a double; got %#v from %s", got, drained) + } + if strings.Contains(string(drained), `"intValue":"0"`) { + t.Errorf("0.25 was truncated to an integer across the spool: %s", drained) + } +} + +// ───────────────────────────────────── the spool must not cross environments + +// THE LEAK Bugbot FOUND. A single host-wide spool meant records queued against +// one backend were drained by the next invocation against ANOTHER — POSTed to its +// endpoint, with its token, still carrying the first one's +// `deployment.environment`. Reproduced before fixing: a prod-labelled record +// arrived at a dev endpoint. +// +// Asserts BOTH halves, because either alone is satisfiable by a bug: nothing +// prod-labelled reaches the dev endpoint, AND the prod spool still holds its +// record afterwards. A fix that simply discarded the other environment's queue +// would pass the first assertion. +func TestTheSpoolDoesNotLeakAcrossEnvironments(t *testing.T) { + dir := t.TempDir() + t.Setenv("TRACEBLOC_CONFIG_DIR", dir) + + prodSpool, err := telemetrySpoolPath("prod") + if err != nil { + t.Fatalf("telemetrySpoolPath(prod): %v", err) + } + devSpool, err := telemetrySpoolPath("dev") + if err != nil { + t.Fatalf("telemetrySpoolPath(dev): %v", err) + } + if prodSpool == devSpool { + t.Fatalf("prod and dev must not share a spool file; both are %q", prodSpool) + } + + prodEvent := spooledEvent{ + Resource: map[string]string{ + "service.name": "cli", + "deployment.environment": "prod", + "service.instance.id": "prod-run", + }, + Attributes: map[string]any{"event.name": "cli.command.failed", "error.type": "usage"}, + } + // Unreachable endpoint, so it spools against prod. + deliver(prodSpool, "http://127.0.0.1:1"+telemetryIngestPath, "prod-token", "prod", prodEvent, time.Now()) + if got := readSpool(prodSpool); len(got) != 1 { + t.Fatalf("setup: the prod event should be spooled; got %d records", len(got)) + } + + var received string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + received = string(body) + w.WriteHeader(http.StatusAccepted) + })) + defer srv.Close() + + devEvent := spooledEvent{ + Resource: map[string]string{"service.name": "cli", "deployment.environment": "dev"}, + Attributes: map[string]any{"event.name": "cli.command.succeeded"}, + } + deliver(devSpool, srv.URL+telemetryIngestPath, "dev-token", "dev", devEvent, time.Now()) + + if received == "" { + t.Fatal("the dev endpoint received nothing; the dev delivery did not happen") + } + if strings.Contains(received, `"stringValue":"prod"`) || strings.Contains(received, "prod-run") { + t.Errorf("a prod-labelled record was POSTed to the dev endpoint: %s", received) + } + // The other environment's records must still be WAITING, not discarded. + if got := readSpool(prodSpool); len(got) != 1 { + t.Errorf("the prod spool should still hold its record after a dev delivery; got %d", len(got)) + } +} + +func TestSpoolEnvSlugRefusesPathTraversal(t *testing.T) { + // Never reachable with the closed dev/stg/prod set — asserted because a path + // segment built from a string is a traversal waiting for that set to open. + for _, env := range []string{"../../etc", "prod/../dev", "", " ", "PROD"} { + slug := spoolEnvSlug(env) + if strings.ContainsAny(slug, "/.\\") { + t.Errorf("spoolEnvSlug(%q) = %q, which can escape the telemetry directory", env, slug) + } + if slug == "" { + t.Errorf("spoolEnvSlug(%q) returned empty, which would make the spool a directory", env) + } + } + if got := spoolEnvSlug("PROD"); got != "prod" { + t.Errorf("spoolEnvSlug should normalise case; got %q", got) + } +} + +// ───────────────────────────────────────── the token lookup and the profile key + +// THE DEFECT BUGBOT FOUND ON #540, as a regression test — tracebloc/cli#552. +// +// telemetryToken used to take the telemetry LABEL and look the profile up by it. +// Profiles are keyed on the RAW cfg.CurrentEnv; the label has been through +// telemetryEnv, which lower-cases and trims (via sessionEnv) and remaps anything +// unrecognised onto prod. Whenever those disagreed, Profile() CREATED an empty +// profile and returned no token — so delivery took the no-token spool path for +// the rest of time while authedClient, reading the raw key, kept working. No +// error, no retry, no warning: outcomes just stopped arriving. +// +// There was no test for telemetryToken at all, which is why this reached a +// promotion. + +// sessionKeyedAs writes a config.json whose CurrentEnv and profile key are +// exactly `raw` — no normalisation — because the whole point is what happens +// when the raw key and the derived label differ. +// +// Named apart from doctor_test.go's signedInConfig, which hardcodes dev: this +// one exists to control the raw key precisely. +func sessionKeyedAs(t *testing.T, raw, token string) { + t.Helper() + dir := os.Getenv("TRACEBLOC_CONFIG_DIR") + if dir == "" { + t.Fatal("sessionKeyedAs needs TRACEBLOC_CONFIG_DIR set (use withTempConfigDir)") + } + body, err := json.Marshal(map[string]any{ + "version": 2, + "current_env": raw, + "profiles": map[string]any{raw: map[string]any{"token": token}}, + }) + if err != nil { + t.Fatalf("marshal config: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "config.json"), body, 0o600); err != nil { + t.Fatalf("write config: %v", err) + } +} + +func TestTelemetryTokenFindsTheSessionTokenWhateverTheLabel(t *testing.T) { + // Each raw value differs from telemetryEnv(sessionEnv(...)) of it, which is + // precisely when the old two-key lookup missed. The `want` column records + // what the label resolves to, so the divergence is visible in the table + // rather than asserted implicitly. + cases := []struct{ raw, label string }{ + {"acme", "prod"}, // unrecognised → remapped to prod by telemetryEnv + {"STG", "stg"}, // sessionEnv lower-cases + {" dev ", "dev"}, // sessionEnv trims + {"Dev", "dev"}, // migrateV1 stores a v1 env verbatim + {"prod", "prod"}, // the control: raw == label, worked before and must still + } + for _, tc := range cases { + t.Run(tc.raw, func(t *testing.T) { + withTempConfigDir(t, "prod") + sessionKeyedAs(t, tc.raw, "sess-"+tc.label) + + // Pin the divergence itself, so this test still means something if + // telemetryEnv's mapping changes: if these ever match, the case is no + // longer exercising two keys and should be re-chosen. + if got := telemetryEnv(signedInEnv()); got != tc.label { + t.Fatalf("telemetryEnv(signedInEnv()) = %q, want %q — the fixture no longer exercises a divergent label", got, tc.label) + } + if got := telemetryToken(); got != "sess-"+tc.label { + t.Errorf("telemetryToken() = %q, want %q — a session keyed %q must be found however the label resolves", got, "sess-"+tc.label, tc.raw) + } + }) + } +} + +func TestTelemetryTokenIsEmptyWhenThereIsGenuinelyNoToken(t *testing.T) { + // Criterion 5: the no-token spool path must still be reachable. A fix that + // returned something for an unauthenticated CLI would post anonymous events + // and 401 forever. + withTempConfigDir(t, "prod") + if got := telemetryToken(); got != "" { + t.Errorf("no config at all: telemetryToken() = %q, want empty", got) + } + sessionKeyedAs(t, "prod", "") + if got := telemetryToken(); got != "" { + t.Errorf("profile with no token: telemetryToken() = %q, want empty", got) + } +} + +// The composed behaviour the ticket asks for: a signed-in CLI whose label was +// remapped must POST, not spool. The two halves are the real ones — the real +// telemetryToken over a real on-disk config, and the real deliver — with only +// the URL substituted, because api.BaseURL has no test seam and would otherwise +// send this at production. +func TestARemappedSessionPostsRatherThanSpooling(t *testing.T) { + spool := withTempConfigDir(t, "prod") + sessionKeyedAs(t, "acme", "sess-acme") // unknown env → label remaps to prod + + var gotAuth string + var posts int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + posts++ + gotAuth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusAccepted) + })) + defer srv.Close() + + env := telemetryEnv(signedInEnv()) + deliver(spool, srv.URL+telemetryIngestPath, telemetryToken(), env, event("run-remapped", 0), time.Now()) + + if posts != 1 { + t.Fatalf("a signed-in session must POST, got %d requests — this is the spool path, i.e. the bug", posts) + } + // Asserting the VALUE, not merely that a header was sent: the whole defect + // was an empty token reaching the wire as a silent no-op. + if gotAuth != "Bearer sess-acme" { + t.Errorf("Authorization = %q, want %q", gotAuth, "Bearer sess-acme") + } + if got := readSpool(spool); len(got) != 0 { + t.Errorf("a delivered event must leave nothing spooled; got %d", len(got)) + } +} diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index c5feb3b..69adf1b 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -398,6 +398,7 @@ screen. %s/%d are runtime placeholders. "Wrote client id + namespace to %s (no new credential — the existing one stands)." "You don't have permission to %s in this account." "Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule." +"Your CPU and memory budget is unchanged — recording it as your explicit choice so it is never resized automatically." "Your active client is not on the cluster your kubeconfig reaches, and no client here is confirmed. Check your kubeconfig context, then run: %s doctor" "Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create" "Your data is registered as a dataset. View it at https://ai.tracebloc.io/metadata" @@ -673,6 +674,7 @@ screen. %s/%d are runtime placeholders. "service %s/%s has no selector — can't resolve to a Pod for port-forwarding" "session: %s" "set -e\nrm -rf %q\nmkdir -p %q\n/bin/tar -xf - -C %q\nif [ -e %q ]; then mv %q %q; fi\nif ! mv %q %q; then\n if [ -e %q ]; then mv %q %q; fi\n exit 1\nfi\nrm -rf %q\nfind %q -maxdepth 1 \\( -name %q -o -name %q \\) -mmin +60 -exec rm -rf {} + 2>/dev/null || true" +"set by" "setting up jobs-manager port-forward: %w" "sha256[:8]" "shared PVC" diff --git a/internal/config/config.go b/internal/config/config.go index 1e020c7..e4e2410 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -84,6 +84,29 @@ func (c *Config) Current() *Profile { return c.Profile(c.CurrentEnv) } +// CurrentToken returns the stored token for the current env, or "" when there +// is none. +// +// READS WITHOUT CREATING, unlike Current()/Profile(). Profile() stores an empty +// profile for a missing key — correct for the write paths it was built for +// (sign-in mutates the returned pointer then Saves), and wrong for a lookup: +// a read that records its own miss makes the second call look like a hit. +// +// KEYED ON THE RAW CurrentEnv, which is how Profiles is keyed — the same key +// SignedIn() and Current() use. Anything that has been through sessionEnv or +// telemetryEnv has been lower-cased, trimmed, or remapped, and is NOT this key +// (tracebloc/cli#552). +func (c *Config) CurrentToken() string { + if c == nil || c.CurrentEnv == "" { + return "" + } + p := c.Profiles[c.CurrentEnv] + if p == nil { + return "" + } + return p.Token +} + // SignedIn reports whether the current env has a stored token. func (c *Config) SignedIn() bool { if c.CurrentEnv == "" { diff --git a/internal/config/config_coverage_test.go b/internal/config/config_coverage_test.go index c5d1d1f..94fe0ae 100644 --- a/internal/config/config_coverage_test.go +++ b/internal/config/config_coverage_test.go @@ -148,3 +148,58 @@ func TestSave_ErrorBranches(t *testing.T) { } }) } + +// CurrentToken must READ, never record its own miss — tracebloc/cli#552. +// +// Profile() creating an absent profile is correct for the write paths it exists +// for (sign-in mutates the returned pointer, then Saves). As a LOOKUP it is a +// trap: the miss is stored, so the second call finds a profile and looks like a +// hit. Both halves are asserted here, because the distinction between the two +// accessors IS the fix. +func TestCurrentTokenReadsWithoutCreatingAProfile(t *testing.T) { + c := &Config{CurrentEnv: "dev"} + if tok := c.CurrentToken(); tok != "" { + t.Fatalf("no profile should mean no token, got %q", tok) + } + if len(c.Profiles) != 0 { + t.Fatalf("CurrentToken must not create a profile; Profiles = %v", c.Profiles) + } + + // The contrast, so a future refactor cannot quietly route CurrentToken + // through Profile() and still pass the assertion above. + c2 := &Config{CurrentEnv: "dev"} + _ = c2.Profile("dev") + if len(c2.Profiles) != 1 { + t.Fatalf("Profile() is documented to create; it did not, so this test no longer pins the difference") + } +} + +func TestCurrentTokenIsKeyedOnTheRawCurrentEnv(t *testing.T) { + // The keys Profiles is really keyed on are whatever sign-in stored — NOT a + // normalised or remapped form. SignedIn() and Current() both read it raw, + // and CurrentToken must agree with them or callers disagree about whether + // the same config is signed in. + for _, raw := range []string{"dev", "STG", " dev ", "acme"} { + c := &Config{CurrentEnv: raw, Profiles: map[string]*Profile{raw: {Token: "t-" + raw}}} + if got := c.CurrentToken(); got != "t-"+raw { + t.Errorf("CurrentEnv %q: CurrentToken = %q, want %q", raw, got, "t-"+raw) + } + if !c.SignedIn() { + t.Errorf("CurrentEnv %q: SignedIn disagrees with CurrentToken", raw) + } + } +} + +func TestCurrentTokenIsEmptyWhenThereIsGenuinelyNoToken(t *testing.T) { + cases := map[string]*Config{ + "no current env": {Profiles: map[string]*Profile{"dev": {Token: "t"}}}, + "no profiles map": {CurrentEnv: "dev"}, + "profile no token": {CurrentEnv: "dev", Profiles: map[string]*Profile{"dev": {}}}, + "nil config": nil, + } + for name, c := range cases { + if got := c.CurrentToken(); got != "" { + t.Errorf("%s: want empty, got %q", name, got) + } + } +} diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 83ffa66..3c31b0e 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -29,6 +29,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/resources" ) @@ -506,18 +507,25 @@ func checkBackendEgress(ctx context.Context, env map[string]string, probe func(c // backendHost maps CLIENT_ENV to the backend API host, mirroring the edge // runtime's own mapping (controller.py). Unset/unknown defaults to prod, the // chart's CLIENT_ENV default. +// +// DERIVED FROM api.BaseURL, not restated. The env→host mapping used to be a +// second copy of BaseURL's switch living in this package, which is how the two +// drift: the same three hosts written down twice, with nothing that fails when +// only one of them is edited. api.BaseURL already lower-cases, so TrimSpace is +// the only normalisation this adds — a CLIENT_ENV read off a container spec can +// carry surrounding whitespace that a --env flag cannot. +// +// The input is the CLUSTER's CLIENT_ENV (read off the jobs-manager Deployment), +// not this CLI's session env — a deliberately different question, which is why +// this takes a string rather than calling into the session resolution. func backendHost(clientEnv string) string { - // Normalize like the API client (api.ResolveEnv/BaseURL lower-case), so a - // non-lowercase CLIENT_ENV on the edge box doesn't fall through to prod and - // make the doctor probe the wrong backend. - switch strings.ToLower(strings.TrimSpace(clientEnv)) { - case "dev": - return "dev-api.tracebloc.io" - case "stg": - return "stg-api.tracebloc.io" - default: + u, err := url.Parse(api.BaseURL(strings.TrimSpace(clientEnv))) + if err != nil || u.Host == "" { + // Unreachable for BaseURL's closed set of return values; the prod default + // keeps this total rather than returning an empty host into a probe URL. return "api.tracebloc.io" } + return u.Host } // checkRequestsProxy verifies the requests-proxy deployment is present and diff --git a/internal/resources/contract.go b/internal/resources/contract.go new file mode 100644 index 0000000..65f050b --- /dev/null +++ b/internal/resources/contract.go @@ -0,0 +1,99 @@ +package resources + +// contract.go carries the training-envelope contract, vendored from +// tracebloc/client-runtime (backend#2220, RFC-BACKEND-664 §P0). +// +// The question "how much of this machine may ONE training run have" used to be +// answered independently in four places: this package, the bash installer, its +// PowerShell twin, and a fifth 0.75-fraction policy inside client-runtime +// itself. None derived from the others, and two of them disagreed by +// construction — most sharply on the node tie-break, where this package ranked +// candidates (cpu, memory) and the bash installer ranked them (memory, cpu), so +// on a cluster of 8c/16Gi + 4c/32Gi `tracebloc resources set` and the installer +// anchored on DIFFERENT nodes and gave different answers about one machine. +// +// client-runtime now owns the arithmetic (node_sizing.envelope_from_allocatable) +// and its constants live in envelope_contract.json. Unlike the installers, Go +// needs no generator to read it: the file is embedded verbatim at compile time, +// so the vendored artifact is byte-identical to upstream and the cross-repo +// drift gate is a plain diff (.github/workflows/envelope-contract-drift.yml at +// scripts/.client-runtime-ref). +// +// What is NOT changed here is cli#143 Decision A. The number the user sets is +// still the per-run ceiling, written to RESOURCE_* verbatim; Overhead() is still +// a fit-check safety margin that is never subtracted from it. Only the duplicate +// *definition* of these four numbers is gone. + +import ( + _ "embed" + "encoding/json" + "fmt" + "sync" +) + +// contractBytes is the raw contract as vendored. Exposed for the drift test, +// which asserts the embedded bytes still parse and still carry vectors. +// +//go:embed envelope_contract.json +var contractBytes []byte + +// envelopeContract is the decoded shape. Only the fields this package needs are +// named: the anchors and rendering rules are the installers' and jobs-manager's +// business, and decoding them here would invite drift of a different kind. +type envelopeContract struct { + ContractVersion int `json:"contract_version"` + Overhead struct { + CPUMilli int64 `json:"cpu_millicores"` + MemoryBytes int64 `json:"memory_bytes"` + } `json:"overhead"` + Floor struct { + CPUMilli int64 `json:"cpu_millicores"` + MemoryBytes int64 `json:"memory_bytes"` + } `json:"floor"` + Vectors struct { + SingleNode []struct { + Label string `json:"label"` + AllocatableCPU string `json:"allocatable_cpu"` + AllocatableMemory string `json:"allocatable_memory"` + Expected *struct { + CPUMilli int64 `json:"cpu_millicores"` + MemoryBytes int64 `json:"memory_bytes"` + Viable bool `json:"viable"` + RenderGi struct { + CPU string `json:"cpu"` + Memory string `json:"memory"` + } `json:"render_gi"` + } `json:"expected"` + } `json:"single_node"` + } `json:"vectors"` +} + +// mustContract decodes and validates the embedded contract exactly once. +// +// It panics on a malformed contract, the same way regexp.MustCompile does for a +// bad literal pattern: the file is embedded at COMPILE time, so the only way it +// can be invalid is a hand-edit or a botched re-vendor, and that is a broken +// build rather than a runtime condition a user could hit. Silently falling back +// to defaults would be worse than a panic — a default here is a fifth policy, +// which is the whole thing backend#2220 removes. TestContractIsValid keeps the +// panic from ever reaching a release. +var mustContract = sync.OnceValue(func() envelopeContract { + var c envelopeContract + if err := json.Unmarshal(contractBytes, &c); err != nil { + panic(fmt.Sprintf("envelope_contract.json is not valid JSON: %v", err)) + } + if c.ContractVersion < 1 { + panic(fmt.Sprintf("envelope_contract.json has no usable contract_version: %d", c.ContractVersion)) + } + for name, v := range map[string]int64{ + "overhead.cpu_millicores": c.Overhead.CPUMilli, + "overhead.memory_bytes": c.Overhead.MemoryBytes, + "floor.cpu_millicores": c.Floor.CPUMilli, + "floor.memory_bytes": c.Floor.MemoryBytes, + } { + if v <= 0 { + panic(fmt.Sprintf("envelope_contract.json %s must be a positive int, got %d", name, v)) + } + } + return c +}) diff --git a/internal/resources/contract_test.go b/internal/resources/contract_test.go new file mode 100644 index 0000000..7891bd5 --- /dev/null +++ b/internal/resources/contract_test.go @@ -0,0 +1,191 @@ +package resources + +import ( + "encoding/json" + "fmt" + "os" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +// backend#2220. The in-repo half of the ticket's definition of done: a mutation +// to the arithmetic in client-runtime's node_sizing.py must redden tests HERE, +// not just there. Two independent nets, and they fail for different reasons: +// +// - TestGoldenVectorsReplay pins that MaxRunCores/MaxRunGiB still agree with +// the contract's own vectors, so a re-vendored contract whose numbers moved +// cannot land silently. +// - TestDecisionAIsIntact pins cli#143's contract itself, independently of the +// contract file, so re-vendoring cannot quietly turn the overhead into +// something that gets subtracted from the user's number. + +func TestContractIsValid(t *testing.T) { + // The embedded contract is decoded by a panicking MustCompile-style helper, + // which is right for a compile-time asset but must never reach a release. + // This is the test that keeps that promise. + c := mustContract() + if c.ContractVersion < 1 { + t.Fatalf("contract_version = %d, want >= 1", c.ContractVersion) + } + if len(c.Vectors.SingleNode) == 0 { + t.Fatal("the vendored contract carries no single-node vectors — " + + "re-vendor it, or the replay below is asserting nothing") + } + if c.Overhead.CPUMilli != 1000 || c.Overhead.MemoryBytes != 3*gib { + t.Errorf("overhead moved: got %dm / %d bytes, want 1000m / %d bytes. "+ + "If that is intended it is a FLEET envelope change, not a re-vendor — "+ + "see backend#2167 and RFC-BACKEND-664 L0", + c.Overhead.CPUMilli, c.Overhead.MemoryBytes, 3*gib) + } + if c.Floor.CPUMilli != 1000 || c.Floor.MemoryBytes != 2*gib { + t.Errorf("floor moved: got %dm / %d bytes, want 1000m / %d bytes", + c.Floor.CPUMilli, c.Floor.MemoryBytes, 2*gib) + } +} + +func TestEmbeddedContractMatchesTheFileOnDisk(t *testing.T) { + // go:embed snapshots the file at compile time; this catches an edit that was + // made but not rebuilt, and — more usefully — proves the embedded bytes are + // the very bytes the cross-repo drift gate diffs. + onDisk, err := os.ReadFile("envelope_contract.json") + if err != nil { + t.Fatalf("reading envelope_contract.json: %v", err) + } + if string(onDisk) != string(contractBytes) { + t.Error("the embedded contract differs from envelope_contract.json on disk") + } +} + +// node builds a Machine the way LargestReadyNode would, from k8s quantity +// strings, so the vectors are exercised through the real parsing path. +func node(t *testing.T, cpu, mem string) Machine { + t.Helper() + c, err := resource.ParseQuantity(cpu) + if err != nil { + t.Fatalf("bad cpu %q: %v", cpu, err) + } + m, err := resource.ParseQuantity(mem) + if err != nil { + t.Fatalf("bad memory %q: %v", mem, err) + } + return Machine{CPU: c, Mem: m, GPU: map[corev1.ResourceName]resource.Quantity{}} +} + +func TestGoldenVectorsReplay(t *testing.T) { + var failures []string + for _, v := range mustContract().Vectors.SingleNode { + // Vectors whose expected is null are the unparseable cases: the contract + // says "I cannot answer", which k8s quantity parsing rejects long before + // this package sees it. Nothing for MaxRun* to agree with. + if v.Expected == nil { + continue + } + m := node(t, v.AllocatableCPU, v.AllocatableMemory) + + gotCores := MaxRunCores(m) + gotGiB := MaxRunGiB(m) + + // A machine the contract calls non-viable is one this package must clamp + // to zero — MaxRun* returning a positive number for it would be the CLI + // offering the user a ceiling the machine cannot host. + if !v.Expected.Viable { + if gotCores != 0 && gotGiB != 0 { + failures = append(failures, fmt.Sprintf( + "%s (%s/%s): contract says NOT viable, but MaxRunCores=%d MaxRunGiB=%d", + v.Label, v.AllocatableCPU, v.AllocatableMemory, gotCores, gotGiB)) + } + continue + } + + wantCores := int(v.Expected.CPUMilli / 1000) + wantGiB := int(v.Expected.MemoryBytes / gib) + if gotCores != wantCores || gotGiB != wantGiB { + failures = append(failures, fmt.Sprintf( + "%s (%s/%s): want %dc/%dGiB, got %dc/%dGiB", + v.Label, v.AllocatableCPU, v.AllocatableMemory, + wantCores, wantGiB, gotCores, gotGiB)) + } + } + if len(failures) > 0 { + t.Fatalf("MaxRunCores/MaxRunGiB no longer agree with the vendored "+ + "contract's vectors (contract v%d):\n %s\n\n"+ + "Either this package's arithmetic drifted, or the contract was "+ + "re-vendored with different numbers. Both are real changes — do not "+ + "'fix' this by editing the expectations.", + mustContract().ContractVersion, strings.Join(failures, "\n ")) + } +} + +func TestDecisionAIsIntact(t *testing.T) { + // cli#143 Decision A, pinned independently of the contract file: the number + // the user sets IS the per-run ceiling, and the overhead is a fit margin that + // is NEVER subtracted from it. backend#2220 deleted the duplicate derivation + // beside this rule; it did not touch the rule. If a future re-vendor makes + // DeriveTraining start shrinking the user's ask, this is what says so. + cpu := *resource.NewQuantity(6, resource.DecimalSI) + mem := *resource.NewQuantity(24*gib, resource.BinarySI) + + got := DeriveTraining(cpu, mem, "", resource.Quantity{}, false) + if got.CPU.Cmp(cpu) != 0 { + t.Errorf("DeriveTraining shrank the user's CPU: got %s, want %s", + got.CPU.String(), cpu.String()) + } + if got.Mem.Cmp(mem) != 0 { + t.Errorf("DeriveTraining shrank the user's memory: got %s, want %s", + got.Mem.String(), mem.String()) + } + if !got.HasCPUMem { + t.Error("DeriveTraining dropped HasCPUMem") + } + if got.HasGPU { + t.Error("DeriveTraining invented a GPU dimension") + } +} + +func TestOverheadIsStillAFitMarginOnly(t *testing.T) { + // The distinction the contract's own comment insists on: overhead is added to + // what the user asked for when checking fit, never subtracted from it. A + // 6c/24GiB ask must NOT fit a 6c/24GiB node, because the platform still needs + // its cut — that asymmetry is the whole design. + small := node(t, "6", "24Gi") + cpu := *resource.NewQuantity(6, resource.DecimalSI) + mem := *resource.NewQuantity(24*gib, resource.BinarySI) + if FitsNode(small, cpu, mem, "", resource.Quantity{}, false) { + t.Error("a full-node ask fit a node with no room for the platform overhead") + } + + big := node(t, "8", "28Gi") + if !FitsNode(big, cpu, mem, "", resource.Quantity{}, false) { + t.Error("a 6c/24Gi ask did not fit 8c/28Gi, which has room for the overhead") + } +} + +func TestFloorTextMatchesTheContractFloor(t *testing.T) { + // The user-facing strings are hand-written; the floor they describe is not. + // A re-vendor that moved the floor while these strings stayed put would make + // the CLI lie in its error messages. + c := mustContract() + if want := fmt.Sprintf("%d core", c.Floor.CPUMilli/1000); CoreFloorText() != want { + t.Errorf("CoreFloorText() = %q but the contract floor is %q", CoreFloorText(), want) + } + if want := fmt.Sprintf("%d GiB", c.Floor.MemoryBytes/gib); MemFloorText() != want { + t.Errorf("MemFloorText() = %q but the contract floor is %q", MemFloorText(), want) + } +} + +func TestContractJSONIsCanonicalFormatting(t *testing.T) { + // The cross-repo gate is a byte diff, so the vendored file must be the + // upstream bytes — not a re-serialised equivalent. Catches a well-meaning + // editor reformat that would redden the drift job for no real reason. + var probe map[string]json.RawMessage + if err := json.Unmarshal(contractBytes, &probe); err != nil { + t.Fatalf("vendored contract is not a JSON object: %v", err) + } + if !strings.HasSuffix(string(contractBytes), "}\n") { + t.Error("vendored contract should end with a closing brace and one newline, " + + "as client-runtime's generator writes it") + } +} diff --git a/internal/resources/envelope_contract.json b/internal/resources/envelope_contract.json new file mode 100644 index 0000000..4a4ae82 --- /dev/null +++ b/internal/resources/envelope_contract.json @@ -0,0 +1,507 @@ +{ + "contract_version": 1, + "issue": "tracebloc/backend#2220", + "rfc": "RFC-BACKEND-664 \u00a7P0", + "source_of_truth": "client-runtime/node_sizing.py::envelope_from_allocatable", + "regenerate_with": "python3 scripts/gen_envelope_vectors.py", + "description": [ + "The one definition of 'how much of this machine may a single training run", + "have'. Before this file the question was answered independently by the bash", + "installer, its PowerShell twin, the Go CLI, and a fourth 0.75-fraction", + "policy in node_sizing.py itself \u2014 four implementations, none derived from", + "the others, two of which disagreed by construction.", + "", + "The installer speaks bash, its twin speaks PowerShell and the CLI speaks", + "Go, so no consumer can call the Python. Each therefore vendors this file", + "and replays the golden vectors below through its own reader, in its own", + "test suite, drift-gated against this repo at a pinned ref. Mutate the", + "arithmetic without regenerating the vectors and this repo reddens at once;", + "regenerate them and every consumer's gate reddens. That is the ticket's", + "definition of done." + ], + "overhead": { + "cpu_millicores": 1000, + "memory_bytes": 3221225472, + "why": [ + "Reserved for the platform \u2014 kubelet, the k3s server, jobs-manager, the", + "requests proxy, MySQL \u2014 so a run sized to the ceiling still leaves the", + "cluster able to schedule and report it. 1 CPU / 3 GiB is what every live", + "producer already subtracts; keeping it identical is what makes this", + "ticket a no-op on installed edges rather than a fleet envelope change." + ] + }, + "floor": { + "cpu_millicores": 1000, + "memory_bytes": 2147483648, + "why": [ + "Below this a training run is not worth scheduling, so a machine that", + "cannot clear it after overhead is reported NON-VIABLE. It is not a clamp:", + "clamping up is how the 4 GiB WSL2 machine ends up asked to host an 8 GiB", + "pod that stays Pending forever." + ] + }, + "fallback_literal": { + "cpu": "2", + "memory": "8Gi", + "why": [ + "The historical fixed envelope, still hardcoded in six physical places", + "across three repos (_TRAINING_DEFAULT, the ps1's two inline copies, the", + "chart template's two, DEFAULT_JOB_RESOURCES). Recorded here so those", + "become readers too. NOTE it is LARGER than the floor: handing it to a", + "non-viable machine is the sub-8GiB bug, not a safe default." + ] + }, + "anchors": { + "largest": { + "question": "What is the largest run this machine could host?", + "rule": "the single schedulable node maximising (cpu_millicores, memory_bytes) lexicographically", + "used_by": [ + "client installer \u2014 sizing the value it writes", + "cli resources set \u2014 clamping the wizard prompt and the fit check" + ], + "why": [ + "A pod's resources all come from ONE node, so this question is", + "single-node by nature and must never sum across the cluster.", + "The tie-break is itself a consolidation: bash ranked nodes (memory,", + "cpu) and cli's nodeLarger ranked them (cpu, memory), so on a cluster of", + "8c/16Gi + 4c/32Gi the two anchored on different nodes. Nobody chose", + "that; it fell out of two independent implementations." + ] + }, + "every": { + "question": "What envelope fits on EVERY schedulable node?", + "rule": "minimum cpu_millicores and minimum memory_bytes, taken independently", + "used_by": [ + "client-runtime derive path (DERIVE_JOB_ENVELOPE, off by default)" + ], + "why": [ + "An envelope sized to the biggest node cannot schedule on the smallest.", + "Independent minima are deliberately conservative \u2014 a heterogeneous", + "cluster with a small system nodepool degrades toward the floor rather", + "than risking an unschedulable pod." + ] + } + }, + "rendering": { + "Gi": "cpu=,memory=Gi \u2014 installer + CLI surface", + "Mi": "cpu=,memory=Mi \u2014 jobs-manager pod spec", + "rounding": "always floor; a ceiling that rounds up is not a ceiling" + }, + "skipped_nodes": [ + "spec.unschedulable (cordoned)", + "allocatable cpu or memory unparseable" + ], + "vectors": { + "single_node": [ + { + "label": "field-8c-32gi", + "allocatable_cpu": "8", + "allocatable_memory": "32Gi", + "expected": { + "cpu_millicores": 7000, + "memory_bytes": 31138512896, + "viable": true, + "render_gi": { + "cpu": "7", + "memory": "29Gi" + }, + "render_mi": { + "cpu": "7", + "memory": "29696Mi" + } + } + }, + { + "label": "field-16c-64gi", + "allocatable_cpu": "16", + "allocatable_memory": "64Gi", + "expected": { + "cpu_millicores": 15000, + "memory_bytes": 65498251264, + "viable": true, + "render_gi": { + "cpu": "15", + "memory": "61Gi" + }, + "render_mi": { + "cpu": "15", + "memory": "62464Mi" + } + } + }, + { + "label": "field-4c-16gi", + "allocatable_cpu": "4", + "allocatable_memory": "16Gi", + "expected": { + "cpu_millicores": 3000, + "memory_bytes": 13958643712, + "viable": true, + "render_gi": { + "cpu": "3", + "memory": "13Gi" + }, + "render_mi": { + "cpu": "3", + "memory": "13312Mi" + } + } + }, + { + "label": "k8s-millicores-and-mi", + "allocatable_cpu": "15500m", + "allocatable_memory": "63928Mi", + "expected": { + "cpu_millicores": 14000, + "memory_bytes": 63812141056, + "viable": true, + "render_gi": { + "cpu": "14", + "memory": "59Gi" + }, + "render_mi": { + "cpu": "14", + "memory": "60856Mi" + } + } + }, + { + "label": "k8s-raw-bytes", + "allocatable_cpu": "8", + "allocatable_memory": "33285996544", + "expected": { + "cpu_millicores": 7000, + "memory_bytes": 30064771072, + "viable": true, + "render_gi": { + "cpu": "7", + "memory": "28Gi" + }, + "render_mi": { + "cpu": "7", + "memory": "28672Mi" + } + } + }, + { + "label": "laptop-2c-8gi", + "allocatable_cpu": "2", + "allocatable_memory": "8Gi", + "expected": { + "cpu_millicores": 1000, + "memory_bytes": 5368709120, + "viable": true, + "render_gi": { + "cpu": "1", + "memory": "5Gi" + }, + "render_mi": { + "cpu": "1", + "memory": "5120Mi" + } + } + }, + { + "label": "exact-floor", + "allocatable_cpu": "2", + "allocatable_memory": "5Gi", + "expected": { + "cpu_millicores": 1000, + "memory_bytes": 2147483648, + "viable": true, + "render_gi": { + "cpu": "1", + "memory": "2Gi" + }, + "render_mi": { + "cpu": "1", + "memory": "2048Mi" + } + } + }, + { + "label": "one-byte-under-mem-floor", + "allocatable_cpu": "2", + "allocatable_memory": "5368709119", + "expected": { + "cpu_millicores": 1000, + "memory_bytes": 2147483647, + "viable": false, + "render_gi": { + "cpu": "1", + "memory": "1Gi" + }, + "render_mi": { + "cpu": "1", + "memory": "2047Mi" + } + } + }, + { + "label": "one-milli-under-cpu-floor", + "allocatable_cpu": "1999m", + "allocatable_memory": "8Gi", + "expected": { + "cpu_millicores": 0, + "memory_bytes": 5368709120, + "viable": false, + "render_gi": { + "cpu": "0", + "memory": "5Gi" + }, + "render_mi": { + "cpu": "0", + "memory": "5120Mi" + } + } + }, + { + "label": "wsl2-too-small-4gi", + "allocatable_cpu": "4", + "allocatable_memory": "4Gi", + "expected": { + "cpu_millicores": 3000, + "memory_bytes": 1073741824, + "viable": false, + "render_gi": { + "cpu": "3", + "memory": "1Gi" + }, + "render_mi": { + "cpu": "3", + "memory": "1024Mi" + } + } + }, + { + "label": "absurdly-small", + "allocatable_cpu": "500m", + "allocatable_memory": "512Mi", + "expected": { + "cpu_millicores": 0, + "memory_bytes": 0, + "viable": false, + "render_gi": { + "cpu": "0", + "memory": "0Gi" + }, + "render_mi": { + "cpu": "0", + "memory": "0Mi" + } + } + }, + { + "label": "unparseable-cpu", + "allocatable_cpu": "eight", + "allocatable_memory": "8Gi", + "expected": null + }, + { + "label": "unparseable-memory", + "allocatable_cpu": "8", + "allocatable_memory": "lots", + "expected": null + } + ], + "multi_node": [ + { + "label": "heterogeneous-incomparable", + "nodes": [ + { + "cpu": "8", + "memory": "16Gi" + }, + { + "cpu": "4", + "memory": "32Gi" + } + ], + "anchored": { + "largest": { + "allocatable_cpu_millicores": 8000, + "allocatable_memory_bytes": 17179869184, + "expected": { + "cpu_millicores": 7000, + "memory_bytes": 13958643712, + "viable": true, + "render_gi": { + "cpu": "7", + "memory": "13Gi" + }, + "render_mi": { + "cpu": "7", + "memory": "13312Mi" + } + } + }, + "every": { + "allocatable_cpu_millicores": 4000, + "allocatable_memory_bytes": 17179869184, + "expected": { + "cpu_millicores": 3000, + "memory_bytes": 13958643712, + "viable": true, + "render_gi": { + "cpu": "3", + "memory": "13Gi" + }, + "render_mi": { + "cpu": "3", + "memory": "13312Mi" + } + } + } + } + }, + { + "label": "server-plus-small-agent", + "nodes": [ + { + "cpu": "16", + "memory": "64Gi" + }, + { + "cpu": "2", + "memory": "8Gi" + } + ], + "anchored": { + "largest": { + "allocatable_cpu_millicores": 16000, + "allocatable_memory_bytes": 68719476736, + "expected": { + "cpu_millicores": 15000, + "memory_bytes": 65498251264, + "viable": true, + "render_gi": { + "cpu": "15", + "memory": "61Gi" + }, + "render_mi": { + "cpu": "15", + "memory": "62464Mi" + } + } + }, + "every": { + "allocatable_cpu_millicores": 2000, + "allocatable_memory_bytes": 8589934592, + "expected": { + "cpu_millicores": 1000, + "memory_bytes": 5368709120, + "viable": true, + "render_gi": { + "cpu": "1", + "memory": "5Gi" + }, + "render_mi": { + "cpu": "1", + "memory": "5120Mi" + } + } + } + } + }, + { + "label": "identical-pair", + "nodes": [ + { + "cpu": "8", + "memory": "32Gi" + }, + { + "cpu": "8", + "memory": "32Gi" + } + ], + "anchored": { + "largest": { + "allocatable_cpu_millicores": 8000, + "allocatable_memory_bytes": 34359738368, + "expected": { + "cpu_millicores": 7000, + "memory_bytes": 31138512896, + "viable": true, + "render_gi": { + "cpu": "7", + "memory": "29Gi" + }, + "render_mi": { + "cpu": "7", + "memory": "29696Mi" + } + } + }, + "every": { + "allocatable_cpu_millicores": 8000, + "allocatable_memory_bytes": 34359738368, + "expected": { + "cpu_millicores": 7000, + "memory_bytes": 31138512896, + "viable": true, + "render_gi": { + "cpu": "7", + "memory": "29Gi" + }, + "render_mi": { + "cpu": "7", + "memory": "29696Mi" + } + } + } + } + }, + { + "label": "one-cordoned-out", + "nodes": [ + { + "cpu": "16", + "memory": "64Gi", + "unschedulable": true + }, + { + "cpu": "4", + "memory": "16Gi" + } + ], + "anchored": { + "largest": { + "allocatable_cpu_millicores": 4000, + "allocatable_memory_bytes": 17179869184, + "expected": { + "cpu_millicores": 3000, + "memory_bytes": 13958643712, + "viable": true, + "render_gi": { + "cpu": "3", + "memory": "13Gi" + }, + "render_mi": { + "cpu": "3", + "memory": "13312Mi" + } + } + }, + "every": { + "allocatable_cpu_millicores": 4000, + "allocatable_memory_bytes": 17179869184, + "expected": { + "cpu_millicores": 3000, + "memory_bytes": 13958643712, + "viable": true, + "render_gi": { + "cpu": "3", + "memory": "13Gi" + }, + "render_mi": { + "cpu": "3", + "memory": "13312Mi" + } + } + } + } + } + ] + } +} diff --git a/internal/resources/provenance_test.go b/internal/resources/provenance_test.go new file mode 100644 index 0000000..0e0ce49 --- /dev/null +++ b/internal/resources/provenance_test.go @@ -0,0 +1,156 @@ +package resources + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +// backend#2220 scope bullet 4. The marker only works if BOTH writers maintain +// it: the installer stamps installer/unknown, and `resources set` stamps user. +// If this side is missing, an edge the installer marked `installer` and the +// operator then re-sized by hand keeps saying `installer` — a human choice +// wearing the one label that invites a future ladder to overwrite it. That is +// strictly worse than having no marker at all, which is why these are here. + +func TestBuildEnvSpecStampsUserProvenance(t *testing.T) { + cpu := *resource.NewQuantity(6, resource.DecimalSI) + mem := *resource.NewQuantity(24*gib, resource.BinarySI) + + env := BuildEnvSpec(cpu, mem, "", resource.Quantity{}, false) + + if got := env["RESOURCE_PROVENANCE"]; got != ProvenanceUser { + t.Errorf("RESOURCE_PROVENANCE = %q, want %q — `resources set` IS the human choice", + got, ProvenanceUser) + } + // The envelope itself must be untouched by the marker (cli#143 Decision A). + if env["RESOURCE_LIMITS"] != "cpu=6,memory=24Gi" { + t.Errorf("RESOURCE_LIMITS = %q, want cpu=6,memory=24Gi", env["RESOURCE_LIMITS"]) + } + if env["RESOURCE_REQUESTS"] != env["RESOURCE_LIMITS"] { + t.Error("requests and limits diverged — Guaranteed QoS is the chart contract") + } +} + +func TestBuildEnvSpecStampsProvenanceOnTheGPUPathToo(t *testing.T) { + // Every dimension is always written; the marker must not be an exception on + // one branch, or a GPU-enabled `resources set` would leave a stale label. + cpu := *resource.NewQuantity(6, resource.DecimalSI) + mem := *resource.NewQuantity(24*gib, resource.BinarySI) + gpu := *resource.NewQuantity(1, resource.DecimalSI) + + env := BuildEnvSpec(cpu, mem, corev1.ResourceName("nvidia.com/gpu"), gpu, true) + + if got := env["RESOURCE_PROVENANCE"]; got != ProvenanceUser { + t.Errorf("RESOURCE_PROVENANCE = %q on the GPU path, want %q", got, ProvenanceUser) + } +} + +func TestBuildEnvSpecIsUnconditional(t *testing.T) { + // BuildEnvSpec takes no prior state on purpose, so there is no branch on + // which it could omit the key. Omitting it would NOT clear it anyway: the + // apply runs `helm upgrade --reset-then-reuse-values`, which re-applies the + // release's stored values on top of chart defaults, so an omitted key is + // silently re-inherited rather than removed. Same reasoning the GPU + // NoGPUEnvValue carries. + cpu := *resource.NewQuantity(2, resource.DecimalSI) + mem := *resource.NewQuantity(8*gib, resource.BinarySI) + for _, wantGPU := range []bool{true, false} { + env := BuildEnvSpec(cpu, mem, corev1.ResourceName("nvidia.com/gpu"), + *resource.NewQuantity(1, resource.DecimalSI), wantGPU) + if _, ok := env["RESOURCE_PROVENANCE"]; !ok { + t.Errorf("wantGPU=%v: RESOURCE_PROVENANCE missing entirely", wantGPU) + } + } +} + +func TestNormalizeProvenance(t *testing.T) { + cases := map[string]string{ + "installer": ProvenanceInstaller, + "user": ProvenanceUser, + // Everything else is unknown, NEVER a guess. "unknown" is the honest + // answer for a release predating the key, and callers treat it as a human + // choice — guessing "installer" would risk overruling an operator. + "unknown": ProvenanceUnknown, + "": ProvenanceUnknown, + "banana": ProvenanceUnknown, + "Installer": ProvenanceUnknown, // case-sensitive by design: the chart enum is lowercase + "user ": ProvenanceUnknown, // no trimming — a stray space is not a verdict + "future": ProvenanceUnknown, // a value this binary predates + } + for raw, want := range cases { + if got := NormalizeProvenance(raw); got != want { + t.Errorf("NormalizeProvenance(%q) = %q, want %q", raw, got, want) + } + } +} + +func TestParseTrainingReadsProvenance(t *testing.T) { + base := map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"} + + t.Run("explicit user", func(t *testing.T) { + env := map[string]string{} + for k, v := range base { + env[k] = v + } + env["RESOURCE_PROVENANCE"] = "user" + if got := ParseTraining(env).Provenance; got != ProvenanceUser { + t.Errorf("Provenance = %q, want %q", got, ProvenanceUser) + } + }) + + t.Run("explicit installer", func(t *testing.T) { + env := map[string]string{} + for k, v := range base { + env[k] = v + } + env["RESOURCE_PROVENANCE"] = "installer" + if got := ParseTraining(env).Provenance; got != ProvenanceInstaller { + t.Errorf("Provenance = %q, want %q", got, ProvenanceInstaller) + } + }) + + t.Run("a pre-marker release reports unknown, not empty", func(t *testing.T) { + // The shape every edge in the field has today. An empty string here would + // make callers branch on "" and invent their own default — a second + // policy, which is the thing this ticket removes. + if got := ParseTraining(base).Provenance; got != ProvenanceUnknown { + t.Errorf("Provenance = %q, want %q", got, ProvenanceUnknown) + } + }) + + t.Run("provenance does not disturb the numbers", func(t *testing.T) { + env := map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "RESOURCE_PROVENANCE": "user", + } + train := ParseTraining(env) + if !train.HasCPUMem { + t.Fatal("HasCPUMem false") + } + if train.CPU.Value() != 4 || train.Mem.Value() != 16*gib { + t.Errorf("ceiling moved: got %s / %s", train.CPU.String(), train.Mem.String()) + } + }) +} + +func TestRoundTripSetThenRead(t *testing.T) { + // The invariant that matters end to end: what `resources set` writes, the + // read path reports as a human choice. If this ever breaks, the CLI would be + // telling a future ladder it may overwrite a size the operator chose. + cpu := *resource.NewQuantity(6, resource.DecimalSI) + mem := *resource.NewQuantity(24*gib, resource.BinarySI) + + written := BuildEnvSpec(cpu, mem, "", resource.Quantity{}, false) + readBack := ParseTraining(written) + + if readBack.Provenance != ProvenanceUser { + t.Errorf("round trip lost the marker: got %q, want %q", + readBack.Provenance, ProvenanceUser) + } + if readBack.CPU.Cmp(cpu) != 0 || readBack.Mem.Cmp(mem) != 0 { + t.Errorf("round trip changed the ceiling: %s / %s", + readBack.CPU.String(), readBack.Mem.String()) + } +} diff --git a/internal/resources/resources.go b/internal/resources/resources.go index 3f4d8cb..61d918d 100644 --- a/internal/resources/resources.go +++ b/internal/resources/resources.go @@ -56,6 +56,37 @@ type Training struct { GPUName corev1.ResourceName GPU resource.Quantity HasGPU bool + + // Provenance is WHO chose the ceiling above (backend#2220): ProvenanceUser, + // ProvenanceInstaller, or ProvenanceUnknown. It never affects the numbers. + // + // Anything unrecognised — including a release that predates the key — + // normalises to ProvenanceUnknown, which callers MUST treat as a human + // choice. Guessing "installer" for an unattributable value would risk + // overruling an operator who had deliberately set a size, and that risk is + // the entire reason the marker exists. + Provenance string +} + +// Who chose the training envelope. Mirrors env.RESOURCE_PROVENANCE in the +// tracebloc chart (client 1.9.49+); the installer writes the first two, this +// CLI writes ProvenanceUser, and ProvenanceUnknown covers everything else. +const ( + ProvenanceInstaller = "installer" + ProvenanceUser = "user" + ProvenanceUnknown = "unknown" +) + +// NormalizeProvenance maps a raw env value onto the three known states. +// Unrecognised input — empty, junk, a future value this binary predates — is +// ProvenanceUnknown, never a guess. +func NormalizeProvenance(raw string) string { + switch raw { + case ProvenanceInstaller, ProvenanceUser: + return raw + default: + return ProvenanceUnknown + } } // MachineCapacity reports the machine headline ("equipped with …") as the @@ -87,7 +118,12 @@ func ParseTraining(env map[string]string) Training { // chart injects this exact value when the operator set no override. cpu, mem, ok = parseCPUMem(DefaultTraining) } - t := Training{CPU: cpu, Mem: mem, HasCPUMem: ok} + t := Training{ + CPU: cpu, + Mem: mem, + HasCPUMem: ok, + Provenance: NormalizeProvenance(env["RESOURCE_PROVENANCE"]), + } gpuName, gpuQty, gpuOK := parseGPU(firstNonEmpty(env["GPU_LIMITS"], env["GPU_REQUESTS"])) if gpuOK { diff --git a/internal/resources/set.go b/internal/resources/set.go index 8c32230..bd63545 100644 --- a/internal/resources/set.go +++ b/internal/resources/set.go @@ -27,24 +27,34 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) -// overheadCPUMilli / overheadMemBytes are tracebloc's fixed platform reservation -// — the single documented constant the design calls for (~1 core / 3 GiB). It is -// used ONLY as the fit-check safety margin (see FitsNode / MaxRunCores / -// MaxRunGiB); it is never subtracted from the per-run ceiling the user sets. -const ( - overheadCPUMilli = 1000 // ~1 CPU core - overheadMemBytes = 3 * (1 << 30) // 3 GiB - minRunCPUMilli = 1000 // per-run floor: ~1 core - minRunMemBytes = 2 * (1 << 30) // per-run floor: 2 GiB - gib = int64(1) << 30 // bytes per GiB -) +// The platform reservation and the per-run floors. These used to be four +// literals typed out here, and again in client/scripts/lib/install-client-helm.sh, +// and again in client/scripts/install-k8s.ps1 — three copies of the same policy, +// none derived from the others (backend#2220). They now come from the contract +// vendored in contract.go, whose arithmetic lives in client-runtime's +// node_sizing.envelope_from_allocatable. +// +// Functions, not vars, for the same reason Overhead() is a function: a package +// var here would be mutable from anywhere in the process, and the whole point is +// that exactly one place decides these numbers. +// +// Semantics are UNCHANGED. The overhead is still used ONLY as the fit-check +// safety margin (FitsNode / MaxRunCores / MaxRunGiB) and is still never +// subtracted from the per-run ceiling the user sets — cli#143 Decision A. What +// went away is the duplicate definition, not the design. +func overheadCPUMilli() int64 { return mustContract().Overhead.CPUMilli } +func overheadMemBytes() int64 { return mustContract().Overhead.MemoryBytes } +func minRunCPUMilli() int64 { return mustContract().Floor.CPUMilli } +func minRunMemBytes() int64 { return mustContract().Floor.MemoryBytes } + +const gib = int64(1) << 30 // bytes per GiB // Overhead returns tracebloc's fixed platform reservation as quantities. Kept a // function (not exported vars) so callers can't mutate the shared value — a // resource.Quantity's Add mutates its receiver. func Overhead() (cpu, mem resource.Quantity) { - return *resource.NewMilliQuantity(overheadCPUMilli, resource.DecimalSI), - *resource.NewQuantity(overheadMemBytes, resource.BinarySI) + return *resource.NewMilliQuantity(overheadCPUMilli(), resource.DecimalSI), + *resource.NewQuantity(overheadMemBytes(), resource.BinarySI) } // DeriveTraining turns a chosen per-run ceiling into the Training spec written to @@ -129,8 +139,8 @@ func FitsNode(node Machine, cpu, mem resource.Quantity, gpuName corev1.ResourceN // the overhead on this node: floor(nodeCPU - overheadCPU). Never negative. This is // the bound the wizard clamps the "cores" prompt to, so over-asking is impossible. func MaxRunCores(node Machine) int { - milli := node.CPU.MilliValue() - overheadCPUMilli - if milli < minRunCPUMilli { + milli := node.CPU.MilliValue() - overheadCPUMilli() + if milli < minRunCPUMilli() { return 0 } return int(milli / 1000) @@ -139,8 +149,8 @@ func MaxRunCores(node Machine) int { // MaxRunGiB is the largest whole-GiB per-run memory ceiling that still leaves room // for the overhead: floor(nodeMem - overheadMem), in GiB. Never negative. func MaxRunGiB(node Machine) int { - b := node.Mem.Value() - overheadMemBytes - if b < minRunMemBytes { + b := node.Mem.Value() - overheadMemBytes() + if b < minRunMemBytes() { return 0 } return int(b / gib) @@ -162,8 +172,8 @@ func MachineGPU(m Machine) (name corev1.ResourceName, count int64, ok bool) { // BelowCoreFloor / BelowMemFloor enforce the per-run minimum (~1 core / 2 GiB): // a run smaller than this can't hold a training job and is almost always a typo. -func BelowCoreFloor(cpu resource.Quantity) bool { return cpu.MilliValue() < minRunCPUMilli } -func BelowMemFloor(mem resource.Quantity) bool { return mem.Value() < minRunMemBytes } +func BelowCoreFloor(cpu resource.Quantity) bool { return cpu.MilliValue() < minRunCPUMilli() } +func BelowMemFloor(mem resource.Quantity) bool { return mem.Value() < minRunMemBytes() } // CoreFloorText / MemFloorText are the floor values as user-facing strings, for error messages. func CoreFloorText() string { return "1 core" } @@ -198,7 +208,23 @@ const NoGPUEnvValue = "" // always written. func BuildEnvSpec(cpu, mem resource.Quantity, gpuName corev1.ResourceName, gpu resource.Quantity, wantGPU bool) map[string]string { spec := fmt.Sprintf("cpu=%s,memory=%s", cpu.String(), mem.String()) - env := map[string]string{"RESOURCE_REQUESTS": spec, "RESOURCE_LIMITS": spec} + env := map[string]string{ + "RESOURCE_REQUESTS": spec, + "RESOURCE_LIMITS": spec, + // backend#2220: `resources set` IS the human choice, so stamp it. This + // is not cosmetic bookkeeping — it is the difference between a future + // ladder re-deriving an installer-written size and it silently + // overruling a deliberate one. + // + // It must be written unconditionally, and especially when a marker is + // already present: an edge the installer stamped `installer` and the + // operator then re-sized by hand would otherwise keep saying + // `installer`, which is the single most dangerous state the marker can + // be in — a human choice wearing a label that invites overwriting. + // Omitting the key would not clear it either, for the same + // --reset-then-reuse-values reason documented above. + "RESOURCE_PROVENANCE": ProvenanceUser, + } if wantGPU { g := fmt.Sprintf("%s=%d", gpuName, gpu.Value()) env["GPU_LIMITS"], env["GPU_REQUESTS"] = g, g diff --git a/scripts/.client-runtime-ref b/scripts/.client-runtime-ref new file mode 100644 index 0000000..d745d16 --- /dev/null +++ b/scripts/.client-runtime-ref @@ -0,0 +1,12 @@ +# Pinned tracebloc/client-runtime commit that +# internal/resources/envelope_contract.json is vendored from (backend#2220). +# +# Pin, don't float — the same rule scripts/.client-ref and +# scripts/.data-ingestors-ref follow: an unrelated client-runtime commit must +# not redden every open CLI PR. The weekly run catches a pin gone stale. +# +# To adopt an upstream contract change: +# 1. cp /envelope_contract.json internal/resources/ +# 2. update the SHA below +# 3. go test ./internal/resources/... — the golden vectors WILL have moved +6293d15f7025d06bc85886df93456e62767a8d45 diff --git a/scripts/check-tool-pins.sh b/scripts/check-tool-pins.sh index 778b1d2..f5e4ae8 100755 --- a/scripts/check-tool-pins.sh +++ b/scripts/check-tool-pins.sh @@ -32,6 +32,10 @@ cd "$(dirname "$0")/.." || exit 2 # Add a row when a tool moves to a `make` target that CI calls. TOOLS=( "GOVULNCHECK_VERSION:golang.org/x/vuln/cmd/govulncheck" + # cli#549: the Lint job's two inline formatter steps became `make fmt-check`, + # so build.yml no longer holds its own goimports version. This row is what + # keeps that true on the next bump. + "GOIMPORTS_VERSION:golang.org/x/tools/cmd/goimports" ) fail=0 diff --git a/scripts/coverage-floor.sh b/scripts/coverage-floor.sh index 01c9301..d1030f7 100755 --- a/scripts/coverage-floor.sh +++ b/scripts/coverage-floor.sh @@ -48,13 +48,14 @@ for entry in $FLOORS; do # whole token); the awk comparison below then errors on that as bare source # and exits non-zero — which `if awk` reads as "not below floor", prints a # bogus "ok", and turns the ratchet into a silent no-op for that package. - if [ "$pkg" = "$entry" ] || ! printf '%s' "$min" | grep -qE '^[0-9]+$'; then + if [ "$pkg" = "$entry" ] || ! grep -qE '^[0-9]+$' <<<"$min"; then echo "::error::malformed FLOORS entry '$entry' (want 'package:INT') — fix scripts/coverage-floor.sh" >&2 status=1 continue fi line="$(go test -cover "./$pkg/" 2>/dev/null | grep -E 'coverage: [0-9]' || true)" - pct="$(printf '%s\n' "$line" | sed -nE 's/.*coverage: ([0-9]+(\.[0-9]+)?)% of statements.*/\1/p' | head -1)" + pct="$(sed -nE 's/.*coverage: ([0-9]+(\.[0-9]+)?)% of statements.*/\1/p' <<<"$line")" + pct="${pct%%$'\n'*}" if [ -z "$pct" ]; then echo "::error::could not read coverage for ./$pkg/ (did any test run?)" >&2 status=1 diff --git a/scripts/file-budget.sh b/scripts/file-budget.sh index a55ded3..688b5b9 100755 --- a/scripts/file-budget.sh +++ b/scripts/file-budget.sh @@ -34,7 +34,7 @@ for entry in $BUDGETS; do # A malformed entry (no ":max", or a non-integer ceiling) must fail # loudly, not slip through as a silent no-op for that file — same guard, # same reason as coverage-floor.sh. - if [ "$path" = "$entry" ] || ! printf '%s' "$max" | grep -qE '^[0-9]+$'; then + if [ "$path" = "$entry" ] || ! grep -qE '^[0-9]+$' <<<"$max"; then echo "::error::malformed BUDGETS entry '$entry' (want 'path:INT') — fix scripts/file-budget.sh" >&2 status=1 continue diff --git a/scripts/format.sh b/scripts/format.sh new file mode 100755 index 0000000..4558381 --- /dev/null +++ b/scripts/format.sh @@ -0,0 +1,150 @@ +#!/usr/bin/env bash +# ============================================================================= +# format.sh — the gofmt -s + goimports gate, scoped to TRACKED files (cli#549) +# +# Both formatters used to run over `.`, which is the whole working TREE, not the +# repo. `.` includes untracked directories, so any scratch path holding Go files +# — a nested git worktree, a vendored copy, a build sandbox — was reported as +# drift while every tracked file was correctly formatted: +# +# ==> goimports (import grouping) needed on: +# /internal/cli/data.go +# ==> run `make fmt` to fix +# +# CI never saw it (a fresh checkout has no untracked Go files), so it was a +# local-only FALSE failure — and the remedy it printed, `make fmt`, was the +# same bug in write mode: it rewrote files the repo does not track. +# +# The file set is now `git ls-files '*.go'`: exactly what a PR can contain, so +# local and CI cannot disagree about scope. Both call this script. +# +# Usage: +# scripts/format.sh --check report drift, exit 1 if any (make fmt-check) +# scripts/format.sh --write rewrite in place (make fmt) +# +# GO and GOIMPORTS_VERSION come from the environment; the Makefile passes them, +# keeping the version declared once there (backend#1972, check-tool-pins.sh). +# +# FAILS CLOSED (exit 2), because each of these would otherwise be reported as a +# clean pass — the inert-verification class of backend#1729: +# * not inside a git work tree — no way to know what is tracked +# * an EMPTY file list — this module has Go files by construction, so "none +# found" means the query broke, not that the tree is clean. It also matters +# mechanically: bare `gofmt -l` with no path arguments reads STDIN, so an +# unguarded empty list checks nothing and exits 0. +# +# Portable to bash 3.2 (macOS default): no mapfile, no associative arrays. +# ============================================================================= +set -uo pipefail +cd "$(dirname "$0")/.." || exit 2 + +GO="${GO:-go}" +GOIMPORTS_VERSION="${GOIMPORTS_VERSION:-v0.48.0}" +LOCAL_PREFIX="github.com/tracebloc/cli" + +mode="" +case "${1-}" in + --check) mode="check" ;; + --write) mode="write" ;; + *) + echo "usage: scripts/format.sh --check | --write" >&2 + exit 2 + ;; +esac + +git rev-parse --is-inside-work-tree >/dev/null 2>&1 || { + echo "format.sh: not inside a git work tree — cannot tell which files are tracked," >&2 + echo " and formatting the whole directory instead is the bug this replaced." >&2 + exit 2 +} + +# Tracked .go files that EXIST. `git ls-files` reports index entries, so a +# tracked-then-deleted file is still listed; passing it to gofmt is a hard error +# ("no such file or directory") on a tree that is merely mid-edit. +files=() +while IFS= read -r -d '' f; do + [ -f "$f" ] && files+=("$f") +done < <(git ls-files -z -- '*.go') + +if [ ${#files[@]} -eq 0 ]; then + echo "format.sh: git ls-files '*.go' matched no existing tracked file." >&2 + echo " This module has Go files by construction, so that is a broken query," >&2 + echo " not a clean tree. Refusing to report clean." >&2 + exit 2 +fi + +# The formatter's stdout goes to a FILE and run_formatter RETURNS the status; +# it never `exit`s and is never called inside `$( )`. That shape is deliberate and +# it is load-bearing (Bugbot High + @LukasWodka on #550): a function that `exit`s +# from a command substitution ends only the SUBSHELL, so the caller reads an empty +# capture, finds no drift, and prints "clean" — exit 0 on a formatter that never +# ran. Which is precisely the inert-verification failure (backend#1729) this file +# was written to prevent, so it must not be reintroduced here. +# +# scripts/tests/format-verify.sh asserts the propagation with a formatter stubbed +# to fail; keep that harness green rather than trusting this comment. +out_file="" +cleanup() { [ -n "$out_file" ] && rm -f "$out_file"; } +trap cleanup EXIT + +out_file="$(mktemp "${TMPDIR:-/tmp}/format-sh.XXXXXX")" || { + echo "format.sh: mktemp failed — refusing to report clean" >&2 + exit 2 +} + +# xargs, not a bare expansion: the list grows with the repo and a single argv has +# a hard size limit. printf is a builtin, so building the NUL stream is not itself +# subject to that limit. The list is non-empty (guarded above), so the BSD-vs-GNU +# "run once with no arguments" difference cannot bite. +# +# stderr is deliberately NOT redirected. `go run` writes module-download progress +# there, and folding it into the captured stdout would turn a cold cache into +# phantom "drift" filenames. Diagnostics go straight to the terminal instead. +run_formatter() { # run_formatter