Merge pull request #7 from github/skarim/state-file-locking

skarim · web-flow · commit 32fd36c5778e · 2026-03-27T00:00:26.000-04:00
lock state file writes
diff --git a/README.md b/README.md
@@ -467,3 +467,4 @@ Compared to the typical workflow, there's no need to name branches, run `git add
 | 5 | Invalid arguments or flags |
 | 6 | Disambiguation required (branch belongs to multiple stacks) |
 | 7 | Rebase already in progress |
+| 8 | Stack is locked by another process |
diff --git a/cmd/add.go b/cmd/add.go
@@ -200,8 +200,7 @@ func runAdd(cfg *config.Config, opts *addOptions, args []string) error {
 	}
 
 	if err := stack.Save(gitDir, sf); err != nil {
-		cfg.Errorf("failed to save stack state: %s", err)
-		return ErrSilent
+		return handleSaveError(cfg, err)
 	}
 
 	// Print summary
diff --git a/cmd/init.go b/cmd/init.go
@@ -327,7 +327,7 @@ func runInit(cfg *config.Config, opts *initOptions) error {
 	syncStackPRs(cfg, &sf.Stacks[len(sf.Stacks)-1])
 
 	if err := stack.Save(gitDir, sf); err != nil {
-		return err
+		return handleSaveError(cfg, err)
 	}
 
 	// Print result
diff --git a/cmd/merge.go b/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
diff --git a/cmd/push.go b/cmd/push.go
@@ -180,8 +180,7 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
 	syncStackPRs(cfg, s)
 
 	if err := stack.Save(gitDir, sf); err != nil {
-		cfg.Errorf("failed to save stack state: %s", err)
-		return ErrSilent
+		return handleSaveError(cfg, err)
 	}
 
 	cfg.Successf("Pushed and synced %d branches", len(s.ActiveBranches()))
diff --git a/cmd/rebase.go b/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`",
diff --git a/cmd/sync.go b/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
 		}
 	}
@@ -288,8 +288,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 	updateBaseSHAs(s)
 
 	if err := stack.Save(gitDir, sf); err != nil {
-		cfg.Errorf("failed to save stack state: %s", err)
-		return ErrSilent
+		return handleSaveError(cfg, err)
 	}
 
 	cfg.Printf("")
diff --git a/cmd/unstack.go b/cmd/unstack.go
@@ -50,8 +50,7 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error {
 	// Remove from local tracking
 	sf.RemoveStackForBranch(target)
 	if err := stack.Save(gitDir, sf); err != nil {
-		cfg.Errorf("failed to save stack state: %s", err)
-		return ErrSilent
+		return handleSaveError(cfg, err)
 	}
 	cfg.Successf("Stack removed from local tracking")
 
diff --git a/cmd/utils.go b/cmd/utils.go
@@ -26,6 +26,7 @@ var (
 	ErrInvalidArgs  = &ExitError{Code: 5} // invalid arguments or flags
 	ErrDisambiguate = &ExitError{Code: 6} // multiple stacks/remotes, can't auto-select
 	ErrRebaseActive = &ExitError{Code: 7} // rebase already in progress
+	ErrLockFailed   = &ExitError{Code: 8} // could not acquire stack file lock
 )
 
 // ExitError is returned by commands to indicate a specific exit code.
@@ -77,6 +78,9 @@ type loadStackResult struct {
 // branch, calls resolveStack (which may prompt for disambiguation), checks for
 // a nil stack, and re-reads the current branch (in case disambiguation caused
 // a checkout).  Errors are printed via cfg and returned.
+//
+// loadStack does NOT acquire the stack file lock.  The lock is acquired
+// automatically by stack.Save() when writing.
 func loadStack(cfg *config.Config, branch string) (*loadStackResult, error) {
 	gitDir, err := git.GitDir()
 	if err != nil {
@@ -133,6 +137,24 @@ func loadStack(cfg *config.Config, branch string) (*loadStackResult, error) {
 	}, nil
 }
 
+// handleSaveError translates a stack.Save error into the appropriate user
+// message and exit error.  Lock contention and stale-file detection both
+// return ErrLockFailed (exit 8); other write failures return ErrSilent (exit 1).
+func handleSaveError(cfg *config.Config, err error) error {
+	var lockErr *stack.LockError
+	if errors.As(err, &lockErr) {
+		cfg.Errorf("another process is currently editing the stack — try again later")
+		return ErrLockFailed
+	}
+	var staleErr *stack.StaleError
+	if errors.As(err, &staleErr) {
+		cfg.Errorf("stack file was modified by another process — please re-run the command")
+		return ErrLockFailed
+	}
+	cfg.Errorf("failed to save stack state: %s", err)
+	return ErrSilent
+}
+
 // resolveStack finds the stack for the given branch, handling ambiguity when
 // a branch (typically a trunk) belongs to multiple stacks. If exactly one
 // stack matches, it is returned directly. If multiple stacks match, the user
diff --git a/cmd/view.go b/cmd/view.go
@@ -50,9 +50,9 @@ func runView(cfg *config.Config, opts *viewOptions) error {
 	s := result.Stack
 	currentBranch := result.CurrentBranch
 
-	// Sync PR state
+	// 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)
diff --git a/internal/stack/lock.go b/internal/stack/lock.go
@@ -0,0 +1,89 @@
+package stack
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"time"
+)
+
+const lockFileName = "gh-stack.lock"
+
+// LockError is returned when the stack file lock cannot be acquired.
+// Callers can check for this with errors.As to distinguish lock failures
+// from other errors.
+type LockError struct {
+	Err error
+}
+
+func (e *LockError) Error() string { return e.Err.Error() }
+func (e *LockError) Unwrap() error { return e.Err }
+
+// StaleError is returned when the stack file was modified on disk since it
+// was loaded.  This indicates another process wrote to the file concurrently.
+// Callers can check for this with errors.As.
+type StaleError struct {
+	Err error
+}
+
+func (e *StaleError) Error() string { return e.Err.Error() }
+func (e *StaleError) Unwrap() error { return e.Err }
+
+// LockTimeout is how long Lock() will wait for the exclusive lock before
+// giving up.  With the lock held only during file writes (milliseconds),
+// this timeout primarily guards against a hung process holding the lock.
+var LockTimeout = 5 * time.Second
+
+// lockRetryInterval is the sleep between non-blocking lock attempts.
+const lockRetryInterval = 100 * time.Millisecond
+
+// FileLock provides an exclusive advisory lock on the stack file to prevent
+// concurrent writes between multiple gh-stack processes.
+type FileLock struct {
+	f *os.File
+}
+
+// Lock acquires an exclusive lock on the stack file in the given git directory.
+// It retries with a non-blocking attempt every 100ms for up to LockTimeout.
+//
+// Most callers should not use Lock directly — stack.Save() acquires the lock
+// automatically.  Use Lock only when you need to hold the lock across multiple
+// operations (e.g. Load-Modify-Save as an atomic unit).
+func Lock(gitDir string) (*FileLock, error) {
+	path := filepath.Join(gitDir, lockFileName)
+	f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0644)
+	if err != nil {
+		return nil, fmt.Errorf("opening lock file: %w", err)
+	}
+
+	deadline := time.Now().Add(LockTimeout)
+	for {
+		err := tryLockFile(f)
+		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, &LockError{Err: fmt.Errorf(
+				"timed out waiting for stack lock after %s — another gh-stack process may be running", LockTimeout)}
+		}
+		time.Sleep(lockRetryInterval)
+	}
+}
+
+// Unlock releases the lock.  The lock file is intentionally left on disk to
+// avoid a race where another process opens the same path, blocks on flock,
+// then wakes up holding a lock on an unlinked inode while a third process
+// creates a new file and locks a different inode.
+func (l *FileLock) Unlock() {
+	if l == nil || l.f == nil {
+		return
+	}
+	unlockFile(l.f)
+	l.f.Close()
+}
diff --git a/internal/stack/lock_test.go b/internal/stack/lock_test.go
diff --git a/internal/stack/lock_unix.go b/internal/stack/lock_unix.go
diff --git a/internal/stack/lock_windows.go b/internal/stack/lock_windows.go
diff --git a/internal/stack/stack.go b/internal/stack/stack.go
diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md

Original file line number	Diff line number	Diff line change
`@@ -200,8 +200,7 @@ func runAdd(cfg config.Config, opts addOptions, args []string) error {`
`200`	`200`	`}`
`201`	`201`
`202`	`202`	`if err := stack.Save(gitDir, sf); err != nil {`
`203`		`- cfg.Errorf("failed to save stack state: %s", err)`
`204`		`- return ErrSilent`
	`203`	`+ return handleSaveError(cfg, err)`
`205`	`204`	`}`
`206`	`205`
`207`	`206`	`// Print summary`
Original file line number	Diff line number	Diff line change
`@@ -327,7 +327,7 @@ func runInit(cfg config.Config, opts initOptions) error {`
`327`	`327`	`syncStackPRs(cfg, &sf.Stacks[len(sf.Stacks)-1])`
`328`	`328`
`329`	`329`	`if err := stack.Save(gitDir, sf); err != nil {`
`330`		`- return err`
	`330`	`+ return handleSaveError(cfg, err)`
`331`	`331`	`}`
`332`	`332`
`333`	`333`	`// Print result`
Original file line number	Diff line number	Diff line change
`@@ -180,8 +180,7 @@ func runPush(cfg config.Config, opts pushOptions) error {`
`180`	`180`	`syncStackPRs(cfg, s)`
`181`	`181`
`182`	`182`	`if err := stack.Save(gitDir, sf); err != nil {`
`183`		`- cfg.Errorf("failed to save stack state: %s", err)`
`184`		`- return ErrSilent`
	`183`	`+ return handleSaveError(cfg, err)`
`185`	`184`	`}`
`186`	`185`
`187`	`186`	`cfg.Successf("Pushed and synced %d branches", len(s.ActiveBranches()))`
Original file line number	Diff line number	Diff line change
`@@ -221,7 +221,7 @@ func runSync(cfg config.Config, opts syncOptions) error {`
`221`	`221`	`} else {`
`222`	`222`	`// Persist refreshed PR state even on conflict, then bail out`
`223`	`223`	`// before pushing or reporting success.`
`224`		`- _ = stack.Save(gitDir, sf)`
	`224`	`+ stack.SaveNonBlocking(gitDir, sf)`
`225`	`225`	`return ErrConflict`
`226`	`226`	`}`
`227`	`227`	`}`
`@@ -288,8 +288,7 @@ func runSync(cfg config.Config, opts syncOptions) error {`
`288`	`288`	`updateBaseSHAs(s)`
`289`	`289`
`290`	`290`	`if err := stack.Save(gitDir, sf); err != nil {`
`291`		`- cfg.Errorf("failed to save stack state: %s", err)`
`292`		`- return ErrSilent`
	`291`	`+ return handleSaveError(cfg, err)`
`293`	`292`	`}`
`294`	`293`
`295`	`294`	`cfg.Printf("")`
Original file line number	Diff line number	Diff line change
`@@ -50,8 +50,7 @@ func runUnstack(cfg config.Config, opts unstackOptions) error {`
`50`	`50`	`// Remove from local tracking`
`51`	`51`	`sf.RemoveStackForBranch(target)`
`52`	`52`	`if err := stack.Save(gitDir, sf); err != nil {`
`53`		`- cfg.Errorf("failed to save stack state: %s", err)`
`54`		`- return ErrSilent`
	`53`	`+ return handleSaveError(cfg, err)`
`55`	`54`	`}`
`56`	`55`	`cfg.Successf("Stack removed from local tracking")`
`57`	`56`