fix: retry self-upgrade with binary rename on Linux/WSL to avoid ETXTBSY

pkg/cli/update_extension_check.go

@@ -1,9 +1,13 @@
 package cli
 
 import (
+	"bytes"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
+	"path/filepath"
+	"runtime"
 	"strings"
 
 	"golang.org/x/mod/semver"
@@ -16,12 +20,22 @@ import (
 var updateExtensionCheckLog = logger.New("cli:update_extension_check")
 
 // upgradeExtensionIfOutdated checks if a newer version of the gh-aw extension is available
-// and, if so, upgrades it automatically. Returns true if an upgrade was performed.
+// and, if so, upgrades it automatically.
 //
-// When true is returned the CURRENTLY RUNNING PROCESS still has the old version baked in.
-// The caller should re-launch the freshly-installed binary so that subsequent work
-// (e.g. lock-file compilation) uses the correct new version string.
-func upgradeExtensionIfOutdated(verbose bool) (bool, error) {
+// 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.
+//   - err: non-nil if the upgrade failed.
+//
+// When upgraded is true the CURRENTLY RUNNING PROCESS still has the old version
+// baked in. The caller should re-launch the freshly-installed binary (at
+// installPath) so that subsequent work (e.g. lock-file compilation) uses the
+// correct new version string.
+func upgradeExtensionIfOutdated(verbose bool) (bool, string, error) {
 	currentVersion := GetVersion()
 	updateExtensionCheckLog.Printf("Checking if extension needs upgrade (current: %s)", currentVersion)
 
@@ -31,7 +45,7 @@ func upgradeExtensionIfOutdated(verbose bool) (bool, error) {
 		if verbose {
 			fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Skipping extension upgrade check (development build)"))
 		}
-		return false, nil
+		return false, "", nil
 	}
 
 	// Query GitHub API for latest release
@@ -42,12 +56,12 @@ func upgradeExtensionIfOutdated(verbose bool) (bool, error) {
 		if verbose {
 			fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not check for extension updates: %v", err)))
 		}
-		return false, nil
+		return false, "", nil
 	}
 
 	if latestVersion == "" {
 		updateExtensionCheckLog.Print("Could not determine latest version, skipping upgrade")
-		return false, nil
+		return false, "", nil
 	}
 
 	updateExtensionCheckLog.Printf("Latest version: %s", latestVersion)
@@ -64,7 +78,7 @@ func upgradeExtensionIfOutdated(verbose bool) (bool, error) {
 			if verbose {
 				fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("✓ gh-aw extension is up to date"))
 			}
-			return false, nil
+			return false, "", nil
 		}
 	} else {
 		// Versions are not valid semver; skip unreliable string comparison and
@@ -77,13 +91,122 @@ func upgradeExtensionIfOutdated(verbose bool) (bool, error) {
 	updateExtensionCheckLog.Printf("Upgrading extension from %s to %s", currentVersion, latestVersion)
 	fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Upgrading gh-aw extension from %s to %s...", currentVersion, latestVersion)))
 
-	cmd := exec.Command("gh", "extension", "upgrade", "github/gh-aw")
-	cmd.Stdout = os.Stderr
-	cmd.Stderr = os.Stderr
-	if err := cmd.Run(); err != nil {
-		return false, fmt.Errorf("failed to upgrade gh-aw extension: %w", err)
+	// 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 Linux we buffer the first attempt's output rather than printing it
+	// directly, so that the ETXTBSY 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)
+	firstCmd := exec.Command("gh", "extension", "upgrade", "github/gh-aw")
+	firstCmd.Stdout = firstAttemptOut
+	firstCmd.Stderr = firstAttemptOut
+	firstErr := firstCmd.Run()
+	if firstErr == nil {
+		// First attempt succeeded without any file manipulation.
+		if runtime.GOOS == "linux" {
+			// Replay the buffered output that was not shown during the attempt.
+			_, _ = io.Copy(os.Stderr, &firstAttemptBuf)
+		}
+		fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("✓ gh-aw extension upgraded to "+latestVersion))
+		return true, "", nil
+	}
+
+	// First attempt failed.
+	if runtime.GOOS != "linux" {
+		// On non-Linux systems 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())
+
+	// Resolve the current executable path before renaming; after the rename
+	// os.Executable() returns a "(deleted)"-suffixed path on Linux.
+	var installPath 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)
+		} else {
+			installPath = path
+		}
+	}
+
+	retryCmd := exec.Command("gh", "extension", "upgrade", "github/gh-aw")
+	retryCmd.Stdout = os.Stderr
+	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)
+		}
+		return false, "", fmt.Errorf("failed to upgrade gh-aw extension: %w", retryErr)
+	}
+
+	// Retry succeeded. Clean up the backup.
+	if installPath != "" {
+		cleanupExecutableBackup(installPath)
 	}
 
 	fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("✓ gh-aw extension upgraded to "+latestVersion))
-	return true, nil
+	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.
+func firstAttemptWriter(dst io.Writer, buf *bytes.Buffer) io.Writer {
+	if runtime.GOOS == "linux" {
+		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"
+	if err := os.Rename(exe, backup); err != nil {
+		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
+}
+
+// restoreExecutableBackup renames the exe+".bak" backup back to exe.
+// Called when the upgrade command failed and the new binary was not written.
+func restoreExecutableBackup(installPath string) {
+	backup := installPath + ".bak"
+	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)))
+		} else {
+			updateExtensionCheckLog.Printf("Restored backup %s → %s after failed upgrade", backup, installPath)
+		}
+	} else {
+		// New binary is present (upgrade partially succeeded); just clean up.
+		_ = os.Remove(backup)
+	}
+}
+
+// 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)
+	}
 }

pkg/cli/update_extension_check_test.go

@@ -3,6 +3,10 @@
 package cli
 
 import (
+	"bytes"
+	"os"
+	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -20,10 +24,11 @@ func TestUpgradeExtensionIfOutdated_DevBuild(t *testing.T) {
 
 	// Verify the function exits before making any API calls.
 	// If it did make API calls we'd see a network error in test environments,
-	// but the function must return (false, nil) immediately.
-	upgraded, err := upgradeExtensionIfOutdated(false)
+	// but the function must return (false, "", nil) immediately.
+	upgraded, installPath, err := upgradeExtensionIfOutdated(false)
 	require.NoError(t, err, "Should not return error for dev builds")
 	assert.False(t, upgraded, "Should not report upgrade for dev builds")
+	assert.Empty(t, installPath, "installPath should be empty for dev builds")
 }
 
 func TestUpgradeExtensionIfOutdated_SilentFailureOnAPIError(t *testing.T) {
@@ -37,7 +42,119 @@ func TestUpgradeExtensionIfOutdated_SilentFailureOnAPIError(t *testing.T) {
 	// Use a release version so the API call is attempted
 	SetVersionInfo("v0.1.0")
 
-	upgraded, err := upgradeExtensionIfOutdated(false)
+	upgraded, installPath, err := upgradeExtensionIfOutdated(false)
 	require.NoError(t, err, "Should fail silently on API errors")
 	assert.False(t, upgraded, "Should not report upgrade when API is unreachable")
+	assert.Empty(t, installPath, "installPath should be empty when API is unreachable")
+}
+
+func TestFirstAttemptWriter_Linux(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skip("Linux-only behavior")
+	}
+	var buf bytes.Buffer
+	dst := &bytes.Buffer{}
+	w := firstAttemptWriter(dst, &buf)
+	// On Linux the writer should be the buffer, not dst.
+	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")
+	}
+	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")
+}
+
+func TestRenamePathForUpgrade(t *testing.T) {
+	// Create a temporary file to act as the "executable".
+	dir := t.TempDir()
+	exe := filepath.Join(dir, "gh-aw")
+	require.NoError(t, os.WriteFile(exe, []byte("binary"), 0o755), "Should create temp executable")
+
+	installPath, err := renamePathForUpgrade(exe)
+	require.NoError(t, err, "renamePathForUpgrade should succeed")
+	assert.Equal(t, exe, installPath, "installPath should equal the original exe path")
+
+	// 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")
+	assert.NoError(t, statErr, "Backup file should exist")
+}
+
+func TestRenamePathForUpgrade_NonExistentFile(t *testing.T) {
+	dir := t.TempDir()
+	exe := filepath.Join(dir, "nonexistent")
+
+	_, err := renamePathForUpgrade(exe)
+	assert.Error(t, err, "renamePathForUpgrade should fail for non-existent file")
+}
+
+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"
+
+	require.NoError(t, os.WriteFile(backup, []byte("old binary"), 0o755), "Should create backup")
+
+	restoreExecutableBackup(installPath)
+
+	// Backup should be renamed back to installPath.
+	_, statErr := os.Stat(installPath)
+	require.NoError(t, statErr, "Original executable should be restored")
+
+	// Backup file should be gone.
+	_, statErr = os.Stat(backup)
+	assert.True(t, os.IsNotExist(statErr), "Backup file should have been renamed away")
+}
+
+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"
+
+	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)
+
+	// New binary should still be present.
+	_, statErr := os.Stat(installPath)
+	require.NoError(t, statErr, "New binary should remain intact")
+
+	// Backup should be cleaned up.
+	_, statErr = os.Stat(backup)
+	assert.True(t, os.IsNotExist(statErr), "Backup file should be cleaned up")
+}
+
+func TestCleanupExecutableBackup(t *testing.T) {
+	dir := t.TempDir()
+	installPath := filepath.Join(dir, "gh-aw")
+	backup := installPath + ".bak"
+
+	require.NoError(t, os.WriteFile(backup, []byte("old binary"), 0o755), "Should create backup")
+
+	cleanupExecutableBackup(installPath)
+
+	// Backup file should be removed.
+	_, statErr := os.Stat(backup)
+	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")
+
+	// No panic or error expected.
+	cleanupExecutableBackup(installPath)
 }