From dddb812df49fa4a3c649214df097bd348e4525e7 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 07:06:08 +0200 Subject: [PATCH 1/2] test(selfupdate): exercise real binary self-replacement end to end --- internal/selfupdate/e2e_test.go | 379 ++++++++++++++++++++++++++++++++ 1 file changed, 379 insertions(+) create mode 100644 internal/selfupdate/e2e_test.go diff --git a/internal/selfupdate/e2e_test.go b/internal/selfupdate/e2e_test.go new file mode 100644 index 0000000..8d07a24 --- /dev/null +++ b/internal/selfupdate/e2e_test.go @@ -0,0 +1,379 @@ +//go:build selfupdate_e2e + +// Package selfupdate's end-to-end apply tests. +// +// The rest of apply_test.go replaces inert byte blobs. That proves the +// bookkeeping but never touches the property that actually differs per +// platform: on Windows a running executable cannot be deleted, only renamed, +// which is the whole reason minio/selfupdate swaps with two renames instead of +// writing over the target in place. These tests compile real binaries, execute +// them, and replace them through grant's own apply path while a process is +// still running from the image - so the Windows file-locking semantics are +// genuinely in play. +// +// Build-tagged because they invoke the Go toolchain and take seconds. Run with: +// +// go test -tags=selfupdate_e2e ./internal/selfupdate/ +// +// No network access is required: the fixture is a self-contained module with no +// dependencies, so nothing is downloaded from GitHub. + +package selfupdate + +import ( + "bufio" + "bytes" + "errors" + "io" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + minio "github.com/minio/selfupdate" +) + +const ( + variantA = "VARIANT-A" + variantB = "VARIANT-B" +) + +// fixtureMain is a stand-in for grant: it prints the version baked in at link +// time, and with the "hold" argument stays alive until its stdin is closed. The +// held mode is what keeps the executable image mapped, and therefore locked on +// Windows, while the swap happens. +const fixtureMain = `package main + +import ( + "bufio" + "fmt" + "os" +) + +var version = "unset" + +func main() { + fmt.Println(version) + if len(os.Args) > 1 && os.Args[1] == "hold" { + // Block until the test closes our stdin. Reading to EOF is enough. + _, _ = bufio.NewReader(os.Stdin).ReadString('\n') + } +} +` + +const fixtureMod = "module grantfixture\n\ngo 1.25\n" + +// exeSuffix matches what the real release artifacts use. +func exeSuffix() string { + if runtime.GOOS == "windows" { + return ".exe" + } + return "" +} + +// goTool locates the toolchain used to build the fixtures. +func goTool(t *testing.T) string { + t.Helper() + path, err := exec.LookPath("go") + if err != nil { + t.Skipf("go toolchain not on PATH: %v", err) + } + return path +} + +// buildVariants compiles the fixture twice - once per version string - and +// returns the two binaries as bytes, ready to be written to a target path or +// handed to applyBinaryTo. +func buildVariants(t *testing.T) (a, b []byte) { + t.Helper() + + src := t.TempDir() + if err := os.WriteFile(filepath.Join(src, "main.go"), []byte(fixtureMain), 0o600); err != nil { + t.Fatalf("write fixture main.go: %v", err) + } + if err := os.WriteFile(filepath.Join(src, "go.mod"), []byte(fixtureMod), 0o600); err != nil { + t.Fatalf("write fixture go.mod: %v", err) + } + + build := func(version string) []byte { + out := filepath.Join(t.TempDir(), "fixture"+exeSuffix()) + cmd := exec.CommandContext(t.Context(), goTool(t), + "build", "-o", out, "-ldflags", "-X main.version="+version, ".") + cmd.Dir = src + // GOFLAGS is cleared so a -mod setting from the parent module cannot + // leak in; GOTOOLCHAIN=local keeps the build off the network. + cmd.Env = append(os.Environ(), "GOFLAGS=", "GOTOOLCHAIN=local") + if combined, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("build fixture %s: %v\n%s", version, err, combined) + } + data, err := os.ReadFile(out) + if err != nil { + t.Fatalf("read built fixture %s: %v", version, err) + } + return data + } + + return build(variantA), build(variantB) +} + +// installBinary writes contents to a fresh directory as an executable target, +// mirroring an installed grant. +func installBinary(t *testing.T, contents []byte) string { + t.Helper() + path := filepath.Join(t.TempDir(), "grant"+exeSuffix()) + if err := os.WriteFile(path, contents, 0o755); err != nil { //nolint:gosec // test fixture must be executable + t.Fatalf("install fixture binary: %v", err) + } + return path +} + +// runBinary executes the target and returns the version it printed. +func runBinary(t *testing.T, path string) string { + t.Helper() + out, err := exec.CommandContext(t.Context(), path).Output() + if err != nil { + t.Fatalf("run %s: %v (output %q)", path, err, out) + } + return strings.TrimSpace(string(out)) +} + +// heldProcess is a live process running from the target image. +type heldProcess struct { + version string + exited chan struct{} + release func() +} + +// alive reports whether the process is still running. Without this the "held" +// cases could silently degrade into the idle cases and the test would claim a +// coverage it does not have. +func (h *heldProcess) alive() bool { + select { + case <-h.exited: + return false + default: + return true + } +} + +// holdBinary starts the target in "hold" mode and waits until it has printed +// its version, which proves the image is mapped and the process is live. The +// returned release function shuts it down and waits for it to exit. +// +// On Windows this is what makes the test meaningful: while this process runs, +// the target file cannot be deleted, only renamed - which is exactly the +// constraint the two-rename swap exists to satisfy. +func holdBinary(t *testing.T, path string) *heldProcess { + t.Helper() + + cmd := exec.CommandContext(t.Context(), path, "hold") + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatalf("stdin pipe: %v", err) + } + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatalf("stdout pipe: %v", err) + } + if err := cmd.Start(); err != nil { + t.Fatalf("start held binary: %v", err) + } + + line, readErr := bufio.NewReader(stdout).ReadString('\n') + + exited := make(chan struct{}) + waited := make(chan struct{}) + go func() { + defer close(waited) + _, _ = io.Copy(io.Discard, stdout) + _ = cmd.Wait() + close(exited) + }() + + released := false + h := &heldProcess{version: strings.TrimSpace(line), exited: exited} + h.release = func() { + if released { + return + } + released = true + _ = stdin.Close() + <-waited + } + t.Cleanup(h.release) + + if readErr != nil { + h.release() + t.Fatalf("held binary produced no output: %v", readErr) + } + return h +} + +// assertNoStagedFile pins that the staged replacement never survives, on any +// platform and in any outcome. +func assertNoStagedFile(t *testing.T, target string) { + t.Helper() + if _, err := os.Stat(stagedPath(target)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("staged %s survived the apply (stat err %v)", stagedPath(target), err) + } +} + +// TestSelfUpdateE2EReplacesBinary replaces a binary that has just been executed +// with a different build of itself, and proves the replacement is the thing +// that runs afterwards. The "running during the swap" case is the Windows +// locked-file path. +func TestSelfUpdateE2EReplacesBinary(t *testing.T) { + oldBin, newBin := buildVariants(t) + + tests := []struct { + name string + // hold keeps a process running from the target image across the swap. + hold bool + }{ + {name: "target idle during the swap", hold: false}, + {name: "target running during the swap", hold: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + target := installBinary(t, oldBin) + + // 1. The installed binary is variant A. + if got := runBinary(t, target); got != variantA { + t.Fatalf("before update: %q, want %q", got, variantA) + } + + var held *heldProcess + if tt.hold { + held = holdBinary(t, target) + if held.version != variantA { + t.Fatalf("held process reported %q, want %q", held.version, variantA) + } + } + + // 2. Replace it through grant's own apply path. + if held != nil && !held.alive() { + t.Fatal("held process exited before the swap: the locked-file path was not exercised") + } + if err := applyBinaryTo(newBin, target); err != nil { + t.Fatalf("applyBinaryTo while hold=%v: %v", tt.hold, err) + } + if held != nil { + if !held.alive() { + t.Error("held process exited during the swap: the locked-file path was not exercised") + } + held.release() + } + + // 3. The new binary is what runs now. + if got := runBinary(t, target); got != variantB { + t.Errorf("after update: %q, want %q", got, variantB) + } + + assertNoStagedFile(t, target) + + backup := oldPathFor(target) + _, backupErr := os.Stat(backup) + switch { + case runtime.GOOS == "windows" && tt.hold: + // Documented Windows behaviour, not a test allowance: minio + // cannot os.Remove the backup while a process is still running + // from that image, so it marks it hidden and leaves it. What + // must hold is that it does not accumulate - the next update + // clears it (CommitBinary removes the old path first). + if backupErr != nil { + t.Logf("note: %s was removed even with the image held", backup) + } + if err := applyBinaryTo(oldBin, target); err != nil { + t.Fatalf("second applyBinaryTo: %v", err) + } + if _, err := os.Stat(backup); !errors.Is(err, os.ErrNotExist) { + t.Errorf("%s survived an update performed with nothing running (stat err %v)", backup, err) + } + if got := runBinary(t, target); got != variantA { + t.Errorf("after the second update: %q, want %q", got, variantA) + } + assertNoStagedFile(t, target) + default: + if !errors.Is(backupErr, os.ErrNotExist) { + t.Errorf("%s survived the apply (stat err %v)", backup, backupErr) + } + } + }) + } +} + +// TestSelfUpdateE2ERollsBackRunningBinary drives the real failure path with a +// real executable: the first rename has already moved the running binary aside +// when the second rename fails, so minio must rename it back - and the restored +// file must still be an executable that runs. +func TestSelfUpdateE2ERollsBackRunningBinary(t *testing.T) { + oldBin, newBin := buildVariants(t) + + tests := []struct { + name string + hold bool + }{ + {name: "target idle during the failed swap", hold: false}, + {name: "target running during the failed swap", hold: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + target := installBinary(t, oldBin) + + var held *heldProcess + if tt.hold { + held = holdBinary(t, target) + } + + // Let staging and the first rename succeed, then remove the staged + // file so the second rename fails and rollback has to run. + origCommit := commitFn + defer func() { commitFn = origCommit }() + commitFn = func(opts minio.Options) error { + if err := os.Remove(stagedPath(target)); err != nil { + t.Errorf("remove staged file: %v", err) + } + return origCommit(opts) + } + + if held != nil && !held.alive() { + t.Fatal("held process exited before the swap: the locked-file path was not exercised") + } + err := applyWithOptions(bytes.NewReader(newBin), minio.Options{ + TargetPath: target, + TargetMode: 0o755, + }) + if err == nil { + t.Fatal("expected the commit to fail, got nil") + } + if rbErr := minio.RollbackError(err); rbErr != nil { + t.Fatalf("rollback failed, the binary is gone: %v", rbErr) + } + if strings.Contains(err.Error(), "no longer exists") { + t.Errorf("error wrongly reports an unrecoverable state: %v", err) + } + + if held != nil { + if !held.alive() { + t.Error("held process exited during the swap: the locked-file path was not exercised") + } + held.release() + } + + // The restored binary must still be a working executable, not just + // a file with the right bytes. + if got := runBinary(t, target); got != variantA { + t.Errorf("after rollback: %q, want %q", got, variantA) + } + + assertNoStagedFile(t, target) + if _, statErr := os.Stat(oldPathFor(target)); !errors.Is(statErr, os.ErrNotExist) { + t.Errorf("%s survived the rollback (stat err %v)", oldPathFor(target), statErr) + } + }) + } +} From 6368a543a8da66d0f270341500da0bf8bb282ec3 Mon Sep 17 00:00:00 2001 From: Tim Schindler Date: Fri, 14 Aug 2026 07:07:58 +0200 Subject: [PATCH 2/2] test: exercise binary self-replacement on Windows in CI The windows-latest CI leg added in 0.8.0 only ran go build and go test; it never exercised a binary replacing itself, which is the one part of grant update whose semantics genuinely differ on Windows (a running executable cannot be deleted, only renamed). Add selfupdate_e2e-tagged end-to-end tests that compile two real fixture binaries from a dependency-free module, execute one, and replace it through grant's own applyBinaryTo/applyWithOptions while a process is still running from that image. Cover the success path and the rollback path, and assert the rolled-back binary still executes. No network access is needed, so this does not depend on GitHub releases. Run the step on both ubuntu-latest and windows-latest so the platforms are directly comparable. No production code changed: applyWithOptions and the commitFn seam already existed. --- .github/workflows/ci.yml | 7 +++++++ CHANGELOG.md | 5 +++++ CLAUDE.md | 3 +++ internal/selfupdate/e2e_test.go | 2 +- 4 files changed, 16 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 955fd01..e2f1963 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,6 +42,13 @@ jobs: if: runner.os == 'Windows' run: go test -race ./... -v + # Runs on BOTH legs so the platforms stay comparable. This is the only + # test that replaces a real, running executable, which is where Windows + # and POSIX genuinely differ. It needs no network: the fixture binaries + # are built locally from a dependency-free module. + - name: Self-update end-to-end (binary replaces itself) + run: go test -tags=selfupdate_e2e -run TestSelfUpdateE2E ./internal/selfupdate/ -v + - name: Lint if: runner.os != 'Windows' uses: golangci/golangci-lint-action@v6 diff --git a/CHANGELOG.md b/CHANGELOG.md index bec8b07..de699d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Added + +- End-to-end self-update tests that replace a real, running binary (`internal/selfupdate/e2e_test.go`, build tag `selfupdate_e2e`). They compile two fixture binaries from a dependency-free module, execute one, and swap it through grant's own apply path while a process is still running from that image — so the Windows file-locking semantics behind the two-rename swap are actually exercised, not just the bookkeeping. Success and rollback paths are both covered, and the rolled-back binary is asserted to still run. No network access is required +- CI runs the new self-update end-to-end tests on **both** `ubuntu-latest` and `windows-latest`, closing the gap left by the Windows CI leg added in 0.8.0, which only ran `go build` and `go test` and never exercised a binary replacing itself on Windows + ## [0.8.0] - 2026-08-14 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index 5079d81..b106ab7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -105,6 +105,8 @@ Custom `SCAAccessService` follows SDK conventions: - Apply: `github.com/minio/selfupdate` v0.6.0 owns the staged-file write, the two-rename swap including the Windows path, and rollback — do not hand-roll this. grant adds the `fsync` of the staged file (minio does not sync) plus a best-effort directory sync. Seams: `applyWithOptions`, `prepareFn`, `commitFn` in `internal/selfupdate/apply.go` - **Atomicity, precisely:** each rename is atomic, so the installed binary is never partially written. The *pair* is not: a kill between the two renames, or a failed second rename whose rollback also fails, leaves the binary path absent with `.grant.old`/`.grant.new` beside it. `InterruptedUpdate()` detects that state and `recoveryHint()` prints the `mv` command that fixes it. Do not describe this as fully atomic - `selfUpdater` in `cmd/interfaces.go` is defined over grant-owned types: `UpdateSelf(ctx, current string) (newVersion string, updated bool, err error)` + - **End-to-end apply test:** `internal/selfupdate/e2e_test.go`, build tag `selfupdate_e2e`, run with `go test -tags=selfupdate_e2e ./internal/selfupdate/`. Everything else in `apply_test.go` swaps inert byte blobs; this compiles two real fixture binaries (a dependency-free module built with `-ldflags -X main.version=...`, so no network), executes one, and replaces it through `applyBinaryTo`/`applyWithOptions` **while a process is still running from that image**. That running child is what makes the Windows path real — a running `.exe` cannot be deleted, only renamed. `heldProcess.alive()` asserts the child survived the swap so the held cases cannot silently degrade into the idle cases. Covers success, rollback after a failed second rename (the restored file must still *execute*), and debris + - **Windows leaves `.grant.old` behind, by design:** `minio.CommitBinary` cannot `os.Remove` the backup while a process still runs from that image, so it calls `SetFileAttributesW(FILE_ATTRIBUTE_HIDDEN)` and leaves it. It does not accumulate — the next `CommitBinary` removes the old path before renaming. The e2e test asserts exactly that rather than demanding a debris-free directory on Windows. The staged `.grant.new` must never survive on either platform - `--groups` flag on root command shows only Entra ID groups in the interactive selector - `--group` / `-g` flag on root command for direct group membership elevation (`grant --group "Cloud Admins"`) - Root command unified selector shows both cloud roles and Entra ID groups; groups use `/eligibility/groups` and `/elevate/groups` API endpoints @@ -185,6 +187,7 @@ make clean # Clean build artifacts - `.github/workflows/ci.yml` — `test` job runs as a matrix over `ubuntu-latest` and `windows-latest` with `fail-fast: false` - Windows runners have no GNU make, so that leg runs the equivalent Go commands directly (`go build -trimpath -o grant.exe .`, `go test -race ./... -v`); Linux keeps `make build` / `make test-race`. Keep the two legs in sync when Makefile targets change - `go test -race` works on windows/amd64 because the runner image ships gcc (the race detector needs cgo) +- The `Self-update end-to-end` step runs the `selfupdate_e2e`-tagged tests on **both** legs (no `if:` guard) — it is the only test that replaces a real running executable, and comparing the two platforms is the whole point. It builds its fixtures locally, so it needs no network. Keep it unguarded; guarding it to Linux would defeat its purpose - Lint (`golangci-lint-action`) runs on Linux only — a second pass on Windows adds minutes and finds nothing new - Tests must be OS-portable. Never assert POSIX permission bits without a `runtime.GOOS == "windows"` skip: Go synthesizes `0666`/`0777` for Windows files and `os.Chmod` there only toggles the read-only attribute. Current skips: `internal/config/config_test.go` (`TestLoadConfig_PermissionError`, `TestConfigDir_Error` — chmod 0000 and `HOME`) and `internal/cache/cache_test.go` (`TestSet_FilePermissions`) - Prefer a portable construction over a skip where one exists. To force a write failure, point at a path whose parent component is an existing regular file (`MkdirAll` fails with ENOTDIR on POSIX and ERROR_DIRECTORY on Windows) rather than a hardcoded `/dev/null/...` path, which is an ordinary writable location on Windows diff --git a/internal/selfupdate/e2e_test.go b/internal/selfupdate/e2e_test.go index 8d07a24..a345fec 100644 --- a/internal/selfupdate/e2e_test.go +++ b/internal/selfupdate/e2e_test.go @@ -278,7 +278,7 @@ func TestSelfUpdateE2EReplacesBinary(t *testing.T) { _, backupErr := os.Stat(backup) switch { case runtime.GOOS == "windows" && tt.hold: - // Documented Windows behaviour, not a test allowance: minio + // Documented Windows behavior, not a test allowance: minio // cannot os.Remove the backup while a process is still running // from that image, so it marks it hidden and leaves it. What // must hold is that it does not accumulate - the next update