non-blocking saves for non-critical writes

Author: skarim

cmd/merge.go

@@ -44,7 +44,7 @@ func runMerge(cfg *config.Config, target string) error {
 	syncStackPRs(cfg, s)
 
 	// Persist the refreshed PR state.
-	_ = stack.Save(result.GitDir, result.StackFile)
+	stack.SaveNonBlocking(result.GitDir, result.StackFile)
 
 	// Resolve which branch to operate on.
 	var br *stack.BranchRef

cmd/rebase.go

@@ -305,7 +305,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
 
 	syncStackPRs(cfg, s)
 
-	_ = stack.Save(gitDir, sf)
+	stack.SaveNonBlocking(gitDir, sf)
 
 	merged := s.MergedBranches()
 	if len(merged) > 0 {
@@ -496,7 +496,7 @@ func continueRebase(cfg *config.Config, gitDir string) error {
 
 	syncStackPRs(cfg, s)
 
-	_ = stack.Save(gitDir, sf)
+	stack.SaveNonBlocking(gitDir, sf)
 
 	cfg.Printf("All branches in stack rebased locally with %s", s.Trunk.Branch)
 	cfg.Printf("To push up your changes and open/update the stack of PRs, run `%s`",

cmd/sync.go

@@ -221,7 +221,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 		} else {
 			// Persist refreshed PR state even on conflict, then bail out
 			// before pushing or reporting success.
-			_ = stack.Save(gitDir, sf)
+			stack.SaveNonBlocking(gitDir, sf)
 			return ErrConflict
 		}
 	}

cmd/utils.go

@@ -138,7 +138,7 @@ func loadStack(cfg *config.Config, branch string) (*loadStackResult, error) {
 }
 
 // handleSaveError translates a stack.Save error into the appropriate user
-// message and exit error.  Lock failures return ErrLockFailed (exit 8);
+// message and exit error.  Lock contention returns ErrLockFailed (exit 8);
 // other write failures return ErrSilent (exit 1).
 func handleSaveError(cfg *config.Config, err error) error {
 	var lockErr *stack.LockError

cmd/view.go

@@ -52,7 +52,7 @@ func runView(cfg *config.Config, opts *viewOptions) error {
 
 	// Sync PR state and save (best-effort).
 	syncStackPRs(cfg, s)
-	_ = stack.Save(gitDir, sf)
+	stack.SaveNonBlocking(gitDir, sf)
 
 	if opts.asJSON {
 		return viewJSON(cfg, s, currentBranch)

internal/stack/lock.go

@@ -52,9 +52,15 @@ func Lock(gitDir string) (*FileLock, error) {
 		if err == nil {
 			return &FileLock{f: f}, nil
 		}
+		if !isLockBusy(err) {
+			// Unexpected error (e.g. bad fd) — don't retry.
+			f.Close()
+			return nil, fmt.Errorf("locking stack file: %w", err)
+		}
 		if time.Now().After(deadline) {
 			f.Close()
-			return nil, fmt.Errorf("timed out waiting for stack lock after %s — another gh-stack process may be running", LockTimeout)
+			return nil, &LockError{Err: fmt.Errorf(
+				"timed out waiting for stack lock after %s — another gh-stack process may be running", LockTimeout)}
 		}
 		time.Sleep(lockRetryInterval)
 	}

internal/stack/lock_test.go

@@ -1,6 +1,7 @@
 package stack
 
 import (
+	"fmt"
 	"sync"
 	"testing"
 	"time"
@@ -32,9 +33,13 @@ func TestLock_BlocksUntilReleased(t *testing.T) {
 	require.NoError(t, err)
 
 	acquired := make(chan struct{})
+	errCh := make(chan error, 1)
 	go func() {
 		lock2, err := Lock(dir)
-		require.NoError(t, err)
+		if err != nil {
+			errCh <- err
+			return
+		}
 		close(acquired)
 		lock2.Unlock()
 	}()
@@ -43,6 +48,8 @@ func TestLock_BlocksUntilReleased(t *testing.T) {
 	select {
 	case <-acquired:
 		t.Fatal("lock2 acquired while lock1 was still held")
+	case err := <-errCh:
+		t.Fatalf("lock2 failed: %v", err)
 	case <-time.After(300 * time.Millisecond):
 		// expected — lock2 is waiting
 	}
@@ -53,6 +60,8 @@ func TestLock_BlocksUntilReleased(t *testing.T) {
 	select {
 	case <-acquired:
 		// success
+	case err := <-errCh:
+		t.Fatalf("lock2 failed after lock1 released: %v", err)
 	case <-time.After(5 * time.Second):
 		t.Fatal("lock2 did not acquire after lock1 was released")
 	}
@@ -67,24 +76,38 @@ func TestLock_SerializesConcurrentAccess(t *testing.T) {
 
 	// Run 10 concurrent goroutines, each adding a stack under lock.
 	// Uses Lock + Load + SaveLocked for atomic read-modify-write.
+	errCh := make(chan error, 10)
 	var wg sync.WaitGroup
 	for i := 0; i < 10; i++ {
 		wg.Add(1)
 		go func(idx int) {
 			defer wg.Done()
 
 			lock, err := Lock(dir)
-			require.NoError(t, err)
+			if err != nil {
+				errCh <- fmt.Errorf("goroutine %d Lock: %w", idx, err)
+				return
+			}
 			defer lock.Unlock()
 
 			loaded, err := Load(dir)
-			require.NoError(t, err)
+			if err != nil {
+				errCh <- fmt.Errorf("goroutine %d Load: %w", idx, err)
+				return
+			}
 
 			loaded.AddStack(makeStack("main", "branch"))
-			require.NoError(t, SaveLocked(dir, loaded))
+			if err := SaveLocked(dir, loaded); err != nil {
+				errCh <- fmt.Errorf("goroutine %d SaveLocked: %w", idx, err)
+			}
 		}(i)
 	}
 	wg.Wait()
+	close(errCh)
+
+	for err := range errCh {
+		t.Error(err)
+	}
 
 	// All 10 stacks should be present — no lost updates.
 	final, err := Load(dir)

internal/stack/lock_unix.go

@@ -13,6 +13,11 @@ func tryLockFile(f *os.File) error {
 	return syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB)
 }
 
+// isLockBusy reports whether err indicates the lock is held by another process.
+func isLockBusy(err error) bool {
+	return err == syscall.EWOULDBLOCK
+}
+
 func unlockFile(f *os.File) {
 	_ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN)
 }

internal/stack/lock_windows.go

@@ -3,6 +3,7 @@
 package stack
 
 import (
+	"errors"
 	"os"
 
 	"golang.org/x/sys/windows"
@@ -22,6 +23,11 @@ func tryLockFile(f *os.File) error {
 	)
 }
 
+// isLockBusy reports whether err indicates the lock is held by another process.
+func isLockBusy(err error) bool {
+	return errors.Is(err, windows.ERROR_LOCK_VIOLATION)
+}
+
 func unlockFile(f *os.File) {
 	ol := new(windows.Overlapped)
 	_ = windows.UnlockFileEx(

internal/stack/stack.go

@@ -260,15 +260,34 @@ func Load(gitDir string) (*StackFile, error) {
 
 // Save acquires an exclusive lock on the stack file, writes sf as JSON, and
 // releases the lock.  The lock is held only for the duration of the write.
+// Returns *LockError if the lock times out due to contention.
 func Save(gitDir string, sf *StackFile) error {
 	lock, err := Lock(gitDir)
 	if err != nil {
-		return &LockError{Err: err}
+		return err // *LockError for contention, plain error for I/O failures
 	}
 	defer lock.Unlock()
 	return writeStackFile(gitDir, sf)
 }
 
+// SaveNonBlocking attempts to save without blocking.  If another process holds
+// the lock, the save is silently skipped.  Use this for best-effort metadata
+// persistence (e.g. syncing PR state in view).
+func SaveNonBlocking(gitDir string, sf *StackFile) {
+	path := filepath.Join(gitDir, lockFileName)
+	f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0644)
+	if err != nil {
+		return
+	}
+	if tryLockFile(f) != nil {
+		f.Close()
+		return
+	}
+	lock := &FileLock{f: f}
+	defer lock.Unlock()
+	_ = writeStackFile(gitDir, sf)
+}
+
 // SaveLocked writes the stack file without acquiring the lock.  The caller
 // must already hold the lock (via Lock) to protect the write.  Use this when
 // you need an atomic Load-Modify-Save sequence.