Skip to content

lock state file writes#7

Merged
skarim merged 6 commits intomainfrom
skarim/state-file-locking
Mar 27, 2026
Merged

lock state file writes#7
skarim merged 6 commits intomainfrom
skarim/state-file-locking

Conversation

@skarim
Copy link
Copy Markdown
Collaborator

@skarim skarim commented Mar 26, 2026

File Locking Implementation:

  • Added a new FileLock type in internal/stack/lock.go with cross-platform support (lock_unix.go, lock_windows.go) to acquire an exclusive lock on the stack file during writes, preventing concurrent modification by multiple processes. Includes a 5-second timeout and robust unlock logic. [1] [2] [3]
  • Introduced unit tests in lock_test.go to verify locking behavior, including concurrent access and lock file persistence.

Stack File Write Changes:

  • Modified Save in internal/stack/stack.go to acquire and release the lock around writes. Added SaveLocked for advanced use cases where the lock is held across multiple operations.

Error Handling and User Feedback:

  • Added a new error type LockError and exit code for lock failures, updating all commands that save the stack file (add.go, init.go, push.go, sync.go, unstack.go) to use a new handleSaveError helper for clearer user messages when a lock cannot be acquired. [1] [2] [3] [4] [5] [6]
  • Updated error codes and documentation to reflect the new lock failure scenario, including README.md and skills/gh-stack/SKILL.md. [1] [2] [3]

Documentation and Comments:

  • Improved code comments and documentation to clarify locking behavior, error handling, and usage patterns for stack file operations.

Best-effort Save in View:

  • Updated view.go to clarify that saving after syncing PR state is best-effort and should not block the user if locking fails.

Copilot AI review requested due to automatic review settings March 26, 2026 00:43
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces cross-process locking for the stack state file to prevent concurrent write races, and standardizes CLI handling/documentation for lock-related failures (exit code 8).

Changes:

  • Add a gitDir-scoped file lock (gh-stack.lock) and make stack.Save() acquire/release it around writes.
  • Add SaveLocked() for atomic Lock + Load + Modify + Save sequences and add new lock-focused tests.
  • Map lock acquisition failures to a dedicated CLI exit code (8) and document it in README/SKILL docs.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
skills/gh-stack/SKILL.md Document exit code 8 semantics (stack locked, 5s timeout).
README.md Add exit code 8 to the public exit-code table.
internal/stack/stack.go Make Save() lock-protected; add SaveLocked() and shared write helper.
internal/stack/lock.go Implement lock acquisition/retry/timeout and lock lifecycle.
internal/stack/lock_unix.go Unix flock implementation for tryLock/unlock.
internal/stack/lock_windows.go Windows LockFileEx implementation for tryLock/unlock.
internal/stack/lock_test.go Add tests validating mutual exclusion and concurrent write serialization.
cmd/utils.go Add ErrLockFailed (exit 8) and handleSaveError() helper.
cmd/add.go Use handleSaveError() when persisting stack changes.
cmd/init.go Use handleSaveError() for save failures.
cmd/push.go Use handleSaveError() for save failures.
cmd/sync.go Use handleSaveError() for save failures (main save path).
cmd/unstack.go Use handleSaveError() for save failures.
cmd/view.go Keep best-effort save after PR sync (now under lock).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/stack/lock.go
Comment thread internal/stack/lock.go
Comment thread internal/stack/stack.go Outdated
Comment thread cmd/utils.go
Comment thread internal/stack/lock_test.go
Comment thread internal/stack/lock_test.go
Comment thread cmd/view.go Outdated
Copy link
Copy Markdown

Copilot AI commented Mar 26, 2026

Warning

This is an internal experiment to assess Copilot's ability to auto-approve PRs. Please 👍 this comment if the assessment below is correct and 👎 if not. Feedback in #f-ccr-auto-approve is appreciated!

Copilot thinks this PR is not ready to approve — see review comments for details.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds cross-process locking around .git/gh-stack persistence to reduce concurrency issues, introduces a dedicated exit code for lock contention, and updates commands/docs accordingly.

Changes:

  • Introduces an advisory OS-level file lock (gh-stack.lock) with timeout/retry logic and tests.
  • Updates stack persistence APIs (Save, SaveNonBlocking, SaveLocked) and routes save failures through a shared handleSaveError (exit code 8 on lock contention).
  • Updates CLI commands and documentation to reflect the new lock-related exit code/behavior.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
skills/gh-stack/SKILL.md Documents new exit code 8 and describes lock timeout behavior.
README.md Adds exit code 8 to the public exit code list.
internal/stack/stack.go Wraps saves with locking; adds non-blocking and “already locked” save variants.
internal/stack/lock.go Implements lock acquisition/retry/timeout and lock error type.
internal/stack/lock_unix.go Unix implementation of non-blocking exclusive advisory locks.
internal/stack/lock_windows.go Windows implementation of non-blocking exclusive locks.
internal/stack/lock_test.go Adds coverage for basic locking behavior and concurrent serialization.
cmd/utils.go Adds ErrLockFailed (exit 8) and handleSaveError to standardize save error handling.
cmd/add.go Uses handleSaveError when persisting stack changes.
cmd/init.go Uses handleSaveError when persisting initial stack state.
cmd/push.go Uses handleSaveError when persisting updated PR/base SHA state.
cmd/sync.go Uses non-blocking save on conflict paths; uses handleSaveError on normal save path.
cmd/unstack.go Uses handleSaveError for save failures.
cmd/merge.go Switches PR-state persistence to non-blocking save.
cmd/rebase.go Switches PR-state persistence to non-blocking save.
cmd/view.go Switches PR-state persistence to non-blocking save.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/stack/stack.go Outdated
Comment thread internal/stack/stack.go
Comment thread internal/stack/lock.go
Comment thread internal/stack/lock_unix.go
Comment thread internal/stack/lock_test.go
Comment thread internal/stack/lock.go Outdated
Comment thread cmd/utils.go
Copy link
Copy Markdown

Copilot AI commented Mar 26, 2026

Warning

This is an internal experiment to assess Copilot's ability to auto-approve PRs. Please 👍 this comment if the assessment below is correct and 👎 if not. Feedback in #f-ccr-auto-approve is appreciated!

Copilot thinks this PR is not ready to approve — see review comments for details.

@skarim skarim marked this pull request as draft March 26, 2026 02:04
@skarim skarim marked this pull request as ready for review March 27, 2026 03:59
@skarim skarim merged commit 32fd36c into main Mar 27, 2026
7 checks passed
@skarim skarim deleted the skarim/state-file-locking branch March 27, 2026 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants