diff --git a/.github/workflows/upgrade-test.yml b/.github/workflows/upgrade-test.yml new file mode 100644 index 00000000000..82eef828282 --- /dev/null +++ b/.github/workflows/upgrade-test.yml @@ -0,0 +1,92 @@ +name: Extension Upgrade Test + +on: + workflow_dispatch: {} + pull_request: + paths: + - 'pkg/cli/update_extension_check.go' + - '.github/workflows/upgrade-test.yml' + +permissions: + contents: read + +jobs: + upgrade-path-test: + name: Extension upgrade on ${{ matrix.os }} + runs-on: ${{ matrix.os }} + timeout-minutes: 10 + permissions: + contents: read + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Find two most recent stable releases + id: releases + shell: bash + run: | + # Use /releases/latest to get n (mirrors getLatestRelease() logic: non-prerelease only). + LATEST=$(gh api repos/github/gh-aw/releases/latest --jq '.tag_name') + + # Get the second most recent non-draft, non-prerelease release (n-1). + # per_page=50 is ample headroom given this project's release cadence. + PREVIOUS=$(gh api "repos/github/gh-aw/releases?per_page=50" \ + --jq '[.[] | select(.prerelease == false and .draft == false)][1].tag_name') + + if [ -z "$LATEST" ] || [ -z "$PREVIOUS" ] || [ "$LATEST" = "null" ] || [ "$PREVIOUS" = "null" ]; then + echo "❌ Could not find two stable releases (latest=$LATEST, previous=$PREVIOUS)" + exit 1 + fi + if [ "$LATEST" = "$PREVIOUS" ]; then + echo "❌ Only one stable release found; cannot test upgrade path" + exit 1 + fi + + echo "latest=$LATEST" >> "$GITHUB_OUTPUT" + echo "previous=$PREVIOUS" >> "$GITHUB_OUTPUT" + echo "Latest stable: $LATEST" + echo "Previous stable: $PREVIOUS" + + - name: Install previous version (n-1) + shell: bash + env: + PREVIOUS: ${{ steps.releases.outputs.previous }} + run: | + echo "Installing gh-aw $PREVIOUS ..." + gh extension install github/gh-aw --pin "$PREVIOUS" --force + + # Extract version string (stable releases always have the simple vX.Y.Z form). + INSTALLED=$(gh aw version 2>&1 | grep -oE 'v[0-9]+\.[0-9]+\.[0-9]+' | head -1) + echo "Installed: $INSTALLED" + [ "$INSTALLED" = "$PREVIOUS" ] || { echo "❌ Expected $PREVIOUS, got $INSTALLED"; exit 1; } + echo "✅ n-1 installed: $INSTALLED" + + - name: Run gh aw upgrade (exercises the self-upgrade code path) + shell: bash + env: + LATEST: ${{ steps.releases.outputs.latest }} + run: | + # gh aw upgrade calls upgradeExtensionIfOutdated which: + # 1. Detects current ($PREVIOUS) < latest ($LATEST) + # 2. Runs `gh extension upgrade github/gh-aw` + # 3. On Windows: binary is in use → rename+retry workaround is triggered + # 4. Re-launches the freshly installed binary with --skip-extension-upgrade + # --no-fix keeps the test focused: skips codemods, action updates, and compilation. + gh aw upgrade --no-fix + + - name: Verify version after upgrade + shell: bash + env: + LATEST: ${{ steps.releases.outputs.latest }} + run: | + INSTALLED=$(gh aw version 2>&1 | grep -oE 'v[0-9]+\.[0-9]+\.[0-9]+' | head -1) + echo "Installed: $INSTALLED" + echo "Expected: $LATEST" + [ "$INSTALLED" = "$LATEST" ] || { echo "❌ Version mismatch: got $INSTALLED, expected $LATEST"; exit 1; } + echo "✅ Upgrade succeeded: $INSTALLED" diff --git a/pkg/cli/update_extension_check.go b/pkg/cli/update_extension_check.go index 25e903631be..4ac1222a396 100644 --- a/pkg/cli/update_extension_check.go +++ b/pkg/cli/update_extension_check.go @@ -24,11 +24,11 @@ var updateExtensionCheckLog = logger.New("cli:update_extension_check") // // Returns: // - upgraded: true if an upgrade was performed. -// - installPath: on Linux, the resolved path where the new binary was installed -// (captured before any rename so the caller can relaunch the new binary from -// the correct path even after os.Executable() starts returning a "(deleted)" -// suffix). Empty string on non-Linux systems or when the path cannot be -// determined. +// - installPath: on Linux or Windows, the resolved path where the new binary +// was installed (captured before any rename so the caller can relaunch the +// new binary from the correct path; on Linux os.Executable() may return a +// "(deleted)"-suffixed path after the rename). Empty string on other systems +// or when the path cannot be determined. // - err: non-nil if the upgrade failed. // // When upgraded is true the CURRENTLY RUNNING PROCESS still has the old version @@ -92,13 +92,13 @@ func upgradeExtensionIfOutdated(verbose bool) (bool, string, error) { fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Upgrading gh-aw extension from %s to %s...", currentVersion, latestVersion))) // First attempt: run the upgrade without touching the filesystem. - // On most systems (and on Linux when there is no in-use binary conflict) - // this will succeed. On Linux with WSL the kernel may return ETXTBSY when - // gh tries to open the currently-executing binary for writing; in that case - // we fall through to the rename+retry path below. + // On most systems this will succeed. On Linux with WSL the kernel may + // return ETXTBSY when gh tries to open the currently-executing binary for + // writing; on Windows the OS returns "Access is denied" for the same + // reason. In both cases we fall through to the rename+retry path below. // - // On Linux we buffer the first attempt's output rather than printing it - // directly, so that the ETXTBSY error message is suppressed when the + // On Linux and Windows we buffer the first attempt's output rather than + // printing it directly, so that the error message is suppressed when the // rename+retry path succeeds and the user is not shown a confusing failure. var firstAttemptBuf bytes.Buffer firstAttemptOut := firstAttemptWriter(os.Stderr, &firstAttemptBuf) @@ -108,7 +108,7 @@ func upgradeExtensionIfOutdated(verbose bool) (bool, string, error) { firstErr := firstCmd.Run() if firstErr == nil { // First attempt succeeded without any file manipulation. - if runtime.GOOS == "linux" { + if needsRenameWorkaround() { // Replay the buffered output that was not shown during the attempt. _, _ = io.Copy(os.Stderr, &firstAttemptBuf) } @@ -117,27 +117,32 @@ func upgradeExtensionIfOutdated(verbose bool) (bool, string, error) { } // First attempt failed. - if runtime.GOOS != "linux" { - // On non-Linux systems there is nothing more to try. + if !needsRenameWorkaround() { + // On platforms other than Linux and Windows there is nothing more to try. return false, "", fmt.Errorf("failed to upgrade gh-aw extension: %w", firstErr) } - // On Linux the failure is likely ETXTBSY. Log the first attempt's output - // at debug level and attempt the rename+retry workaround. - updateExtensionCheckLog.Printf("First upgrade attempt failed (likely ETXTBSY); retrying with rename workaround. First attempt output: %s", firstAttemptBuf.String()) + // On Linux the failure is likely ETXTBSY; on Windows it is likely + // "Access is denied". Both arise because the OS prevents overwriting a + // running binary. Attempt the rename+retry workaround: rename the + // currently-running binary away to free up its path, then retry the + // upgrade so that gh can write the new binary at the original location. + updateExtensionCheckLog.Printf("First upgrade attempt failed (likely locked binary); retrying with rename workaround. First attempt output: %s", firstAttemptBuf.String()) // Resolve the current executable path before renaming; after the rename // os.Executable() returns a "(deleted)"-suffixed path on Linux. var installPath string + var backupPath string if exe, exeErr := os.Executable(); exeErr == nil { if resolved, resolveErr := filepath.EvalSymlinks(exe); resolveErr == nil { exe = resolved } - if path, renameErr := renamePathForUpgrade(exe); renameErr != nil { - // Rename failed; the retry will likely fail again with ETXTBSY. - updateExtensionCheckLog.Printf("Could not rename executable for retry (upgrade will likely fail with ETXTBSY): %v", renameErr) + if iPath, bPath, renameErr := renamePathForUpgrade(exe); renameErr != nil { + // Rename failed; the retry will likely fail again. + updateExtensionCheckLog.Printf("Could not rename executable for retry (upgrade will likely fail): %v", renameErr) } else { - installPath = path + installPath = iPath + backupPath = bPath } } @@ -146,67 +151,107 @@ func upgradeExtensionIfOutdated(verbose bool) (bool, string, error) { retryCmd.Stderr = os.Stderr if retryErr := retryCmd.Run(); retryErr != nil { // Retry also failed. Restore the backup so the user still has gh-aw. - if installPath != "" { - restoreExecutableBackup(installPath) + if backupPath != "" { + restoreExecutableBackup(installPath, backupPath) + } + if runtime.GOOS == "windows" && isWindowsLockError(firstAttemptBuf.String(), retryErr) { + // On Windows, self-upgrade may not be possible while the binary is + // running. Guide the user to upgrade manually from a separate shell. + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("On Windows, gh-aw cannot self-upgrade while it is running.")) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Please upgrade manually by running one of the following:")) + fmt.Fprintln(os.Stderr, " gh extension upgrade gh-aw") + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("If that does not work, try reinstalling:")) + fmt.Fprintln(os.Stderr, " gh extension remove gh-aw") + fmt.Fprintln(os.Stderr, " gh extension install github/gh-aw") } return false, "", fmt.Errorf("failed to upgrade gh-aw extension: %w", retryErr) } // Retry succeeded. Clean up the backup. - if installPath != "" { - cleanupExecutableBackup(installPath) + if backupPath != "" { + cleanupExecutableBackup(backupPath) } fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("✓ gh-aw extension upgraded to "+latestVersion)) return true, installPath, nil } -// firstAttemptWriter returns a writer that buffers output on Linux (so that -// ETXTBSY error messages from a failed first upgrade attempt can be suppressed -// when the rename+retry workaround succeeds) and writes directly to dst on -// other platforms. +// needsRenameWorkaround reports whether the current platform requires the +// rename+retry workaround when upgrading the running binary. +// +// On Linux, overwriting a running binary returns ETXTBSY. +// On Windows, the same operation returns "Access is denied". +// Both errors are resolved by renaming the current binary away first. +func needsRenameWorkaround() bool { + return runtime.GOOS == "linux" || runtime.GOOS == "windows" +} + +// firstAttemptWriter returns a writer that buffers output on platforms that +// use the rename+retry workaround (Linux and Windows), so that error messages +// from a failed first upgrade attempt are suppressed when the retry succeeds. +// On other platforms it writes directly to dst. func firstAttemptWriter(dst io.Writer, buf *bytes.Buffer) io.Writer { - if runtime.GOOS == "linux" { + if needsRenameWorkaround() { return buf } return dst } -// renamePathForUpgrade renames the binary at exe to exe+".bak", freeing the -// original path for the new binary to be written by gh extension upgrade. -// Returns exe (the install path) so the caller can relaunch the new binary and -// restore the backup if the upgrade fails. -func renamePathForUpgrade(exe string) (string, error) { - backup := exe + ".bak" +// renamePathForUpgrade renames the binary at exe to a PID-qualified backup +// path (exe+"..bak"), freeing the original path for the new binary to be +// written by gh extension upgrade. Using a PID-qualified name ensures each +// invocation gets a unique backup so that a failed cleanup (e.g. Windows cannot +// remove a running binary) does not cause the destination to already exist on +// a subsequent upgrade attempt. +// Returns the install path (exe) and the backup path so the caller can +// relaunch the new binary and restore or clean up the backup. +func renamePathForUpgrade(exe string) (string, string, error) { + backup := fmt.Sprintf("%s.%d.bak", exe, os.Getpid()) if err := os.Rename(exe, backup); err != nil { - return "", fmt.Errorf("could not rename %s → %s: %w", exe, backup, err) + return "", "", fmt.Errorf("could not rename %s → %s: %w", exe, backup, err) } - updateExtensionCheckLog.Printf("Renamed %s → %s to avoid ETXTBSY on Linux", exe, backup) - return exe, nil + updateExtensionCheckLog.Printf("Renamed %s → %s to free path for upgrade", exe, backup) + return exe, backup, nil } -// restoreExecutableBackup renames the exe+".bak" backup back to exe. +// restoreExecutableBackup renames backupPath back to installPath. // Called when the upgrade command failed and the new binary was not written. -func restoreExecutableBackup(installPath string) { - backup := installPath + ".bak" +func restoreExecutableBackup(installPath, backupPath string) { if _, statErr := os.Stat(installPath); os.IsNotExist(statErr) { // New binary was not installed; restore the backup. - if renErr := os.Rename(backup, installPath); renErr != nil { - updateExtensionCheckLog.Printf("could not restore backup %s → %s: %v", backup, installPath, renErr) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(fmt.Sprintf("Failed to restore gh-aw backup after upgrade failure. Manually rename %s to %s to recover.", backup, installPath))) + if renErr := os.Rename(backupPath, installPath); renErr != nil { + updateExtensionCheckLog.Printf("could not restore backup %s → %s: %v", backupPath, installPath, renErr) + fmt.Fprintln(os.Stderr, console.FormatErrorMessage(fmt.Sprintf("Failed to restore gh-aw backup after upgrade failure. Manually rename %s to %s to recover.", backupPath, installPath))) } else { - updateExtensionCheckLog.Printf("Restored backup %s → %s after failed upgrade", backup, installPath) + updateExtensionCheckLog.Printf("Restored backup %s → %s after failed upgrade", backupPath, installPath) } } else { // New binary is present (upgrade partially succeeded); just clean up. - _ = os.Remove(backup) + _ = os.Remove(backupPath) } } -// cleanupExecutableBackup removes the exe+".bak" backup after a successful upgrade. -func cleanupExecutableBackup(installPath string) { - backup := installPath + ".bak" - if err := os.Remove(backup); err != nil && !os.IsNotExist(err) { - updateExtensionCheckLog.Printf("Could not remove backup %s: %v", backup, err) +// cleanupExecutableBackup removes backupPath after a successful upgrade. +func cleanupExecutableBackup(backupPath string) { + if err := os.Remove(backupPath); err != nil && !os.IsNotExist(err) { + updateExtensionCheckLog.Printf("Could not remove backup %s: %v", backupPath, err) + } +} + +// isWindowsLockError reports whether the output or error from an upgrade +// attempt indicate a Windows file-locking issue (the running-binary-lock +// symptom). Only when a lock error is detected should the Windows-specific +// self-upgrade guidance be shown; other failures should propagate the +// underlying error message instead. +func isWindowsLockError(output string, err error) bool { + lockMsgs := []string{"Access is denied", "The process cannot access the file"} + for _, msg := range lockMsgs { + if strings.Contains(output, msg) { + return true + } + if err != nil && strings.Contains(err.Error(), msg) { + return true + } } + return false } diff --git a/pkg/cli/update_extension_check_test.go b/pkg/cli/update_extension_check_test.go index 1f0ac7c4612..ba54278b69b 100644 --- a/pkg/cli/update_extension_check_test.go +++ b/pkg/cli/update_extension_check_test.go @@ -4,6 +4,7 @@ package cli import ( "bytes" + "errors" "os" "path/filepath" "runtime" @@ -59,15 +60,32 @@ func TestFirstAttemptWriter_Linux(t *testing.T) { assert.Equal(t, &buf, w, "firstAttemptWriter should return the buffer on Linux") } -func TestFirstAttemptWriter_NonLinux(t *testing.T) { - if runtime.GOOS == "linux" { - t.Skip("Non-Linux behavior only") +func TestFirstAttemptWriter_Windows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("Windows-only behavior") } var buf bytes.Buffer dst := &bytes.Buffer{} w := firstAttemptWriter(dst, &buf) - // On non-Linux the writer should be dst. - assert.Equal(t, dst, w, "firstAttemptWriter should return dst on non-Linux") + // On Windows the writer should be the buffer (rename+retry workaround). + assert.Equal(t, &buf, w, "firstAttemptWriter should return the buffer on Windows") +} + +func TestFirstAttemptWriter_NonLinuxNonWindows(t *testing.T) { + if runtime.GOOS == "linux" || runtime.GOOS == "windows" { + t.Skip("Non-Linux/non-Windows behavior only") + } + var buf bytes.Buffer + dst := &bytes.Buffer{} + w := firstAttemptWriter(dst, &buf) + // On other platforms the writer should be dst. + assert.Equal(t, dst, w, "firstAttemptWriter should return dst on non-Linux/non-Windows") +} + +func TestNeedsRenameWorkaround(t *testing.T) { + result := needsRenameWorkaround() + expected := runtime.GOOS == "linux" || runtime.GOOS == "windows" + assert.Equal(t, expected, result, "needsRenameWorkaround should return true only on Linux and Windows") } func TestRenamePathForUpgrade(t *testing.T) { @@ -76,16 +94,18 @@ func TestRenamePathForUpgrade(t *testing.T) { exe := filepath.Join(dir, "gh-aw") require.NoError(t, os.WriteFile(exe, []byte("binary"), 0o755), "Should create temp executable") - installPath, err := renamePathForUpgrade(exe) + installPath, backupPath, err := renamePathForUpgrade(exe) require.NoError(t, err, "renamePathForUpgrade should succeed") assert.Equal(t, exe, installPath, "installPath should equal the original exe path") + assert.NotEmpty(t, backupPath, "backupPath should be non-empty") + assert.Contains(t, backupPath, ".bak", "backupPath should have .bak suffix") // The original path should no longer exist. _, statErr := os.Stat(exe) assert.True(t, os.IsNotExist(statErr), "Original executable should have been renamed away") - // The backup should exist. - _, statErr = os.Stat(exe + ".bak") + // The backup should exist at the returned path. + _, statErr = os.Stat(backupPath) assert.NoError(t, statErr, "Backup file should exist") } @@ -93,7 +113,7 @@ func TestRenamePathForUpgrade_NonExistentFile(t *testing.T) { dir := t.TempDir() exe := filepath.Join(dir, "nonexistent") - _, err := renamePathForUpgrade(exe) + _, _, err := renamePathForUpgrade(exe) assert.Error(t, err, "renamePathForUpgrade should fail for non-existent file") } @@ -101,11 +121,11 @@ func TestRestoreExecutableBackup_NoNewBinary(t *testing.T) { // Simulate: backup exists, new binary was NOT written (upgrade failed). dir := t.TempDir() installPath := filepath.Join(dir, "gh-aw") - backup := installPath + ".bak" + backup := installPath + ".99999.bak" require.NoError(t, os.WriteFile(backup, []byte("old binary"), 0o755), "Should create backup") - restoreExecutableBackup(installPath) + restoreExecutableBackup(installPath, backup) // Backup should be renamed back to installPath. _, statErr := os.Stat(installPath) @@ -120,12 +140,12 @@ func TestRestoreExecutableBackup_NewBinaryPresent(t *testing.T) { // Simulate: both backup and new binary exist (upgrade partially succeeded). dir := t.TempDir() installPath := filepath.Join(dir, "gh-aw") - backup := installPath + ".bak" + backup := installPath + ".99999.bak" require.NoError(t, os.WriteFile(installPath, []byte("new binary"), 0o755), "Should create new binary") require.NoError(t, os.WriteFile(backup, []byte("old binary"), 0o755), "Should create backup") - restoreExecutableBackup(installPath) + restoreExecutableBackup(installPath, backup) // New binary should still be present. _, statErr := os.Stat(installPath) @@ -138,23 +158,69 @@ func TestRestoreExecutableBackup_NewBinaryPresent(t *testing.T) { func TestCleanupExecutableBackup(t *testing.T) { dir := t.TempDir() - installPath := filepath.Join(dir, "gh-aw") - backup := installPath + ".bak" + backupPath := filepath.Join(dir, "gh-aw.99999.bak") - require.NoError(t, os.WriteFile(backup, []byte("old binary"), 0o755), "Should create backup") + require.NoError(t, os.WriteFile(backupPath, []byte("old binary"), 0o755), "Should create backup") - cleanupExecutableBackup(installPath) + cleanupExecutableBackup(backupPath) // Backup file should be removed. - _, statErr := os.Stat(backup) + _, statErr := os.Stat(backupPath) assert.True(t, os.IsNotExist(statErr), "Backup file should be removed after cleanup") } func TestCleanupExecutableBackup_NoBackup(t *testing.T) { // Should not fail if backup doesn't exist. dir := t.TempDir() - installPath := filepath.Join(dir, "gh-aw") + backupPath := filepath.Join(dir, "gh-aw.99999.bak") // No panic or error expected. - cleanupExecutableBackup(installPath) + cleanupExecutableBackup(backupPath) +} + +func TestIsWindowsLockError(t *testing.T) { + tests := []struct { + name string + output string + err error + expected bool + }{ + { + name: "access denied in output", + output: "gh: Access is denied.\n", + err: nil, + expected: true, + }, + { + name: "sharing violation in output", + output: "The process cannot access the file because it is being used by another process.", + err: nil, + expected: true, + }, + { + name: "access denied in error", + output: "", + err: errors.New("exit status 1: Access is denied"), + expected: true, + }, + { + name: "unrelated error", + output: "gh: 401 Unauthorized", + err: errors.New("exit status 1"), + expected: false, + }, + { + name: "empty output and nil error", + output: "", + err: nil, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isWindowsLockError(tt.output, tt.err) + assert.Equal(t, tt.expected, result, "isWindowsLockError result mismatch") + }) + } }