but: use file locks to prevent multiple background refreshes

[krlvi]

• Added configurable lock scope for inter-process access
• Implemented inter-process locking for background refresh operations

Also re-enables the but journey test. This PR, together with #11719 should eliminate flakiness

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Review	Updated (UTC)
gitbutler-web	Ignored	Preview	Jan 6, 2026 10:23pm

[Copilot]

Pull request overview

This PR implements inter-process locking for background refresh operations to prevent multiple GitButler instances from performing concurrent background refreshes. It introduces a configurable LockScope enum that allows different lock granularities: AllOperations (existing behavior) and BackgroundRefreshOperations (new, scoped locking).

Key Changes

• Added LockScope enum with two variants to enable fine-grained locking control
• Updated try_exclusive_inter_process_access to accept a scope parameter for lock selection
• Implemented background refresh coordination using the new scoped locking mechanism

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/but-core/src/sync.rs	Introduces LockScope enum and updates locking function to support configurable lock scopes with separate lock files
crates/but-ctx/src/access.rs	Updates existing lock acquisition to explicitly use AllOperations scope, maintaining backward compatibility
crates/but/src/command/legacy/refresh.rs	Acquires background refresh lock to prevent concurrent refresh operations
crates/but/src/init.rs	Checks for background refresh lock availability before spawning background refresh process
crates/but-core/tests/core/sync.rs	Adds comprehensive test coverage for lock scope functionality, including exclusivity and scope isolation tests
crates/but-core/tests/core/main.rs	Registers the new sync test module

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

crates/but-ctx/src/access.rs:36

• The LockScope enum should be re-exported in this module's public API since it's now required to use the lock functionality. Users of but-ctx who want to call try_exclusive_inter_process_access directly would need to import LockScope, but it's not currently exported. Add LockScope to the pub use statement on line 33 to maintain a consistent public API.

pub use but_core::sync::{
    LockFile, WorkspaceReadGuard, WorkspaceWriteGuard, WorktreeReadPermission,
    WorktreeWritePermission,
};

[Copilot]

There's a potential race condition here. The lock is checked at line 119-123, then explicitly dropped at line 126, and the background process is spawned at line 128. Between dropping the lock and the child process acquiring it, another process could acquire the BackgroundRefreshOperations lock, defeating the purpose of this check. Consider holding the lock until after the spawn call succeeds, or restructure the logic to avoid this race window.

Suggested change

drop(exclusive_access); // Release the lock immediately so that the new child process can acquire it

[krlvi]

If the lock is held until the spawn succeeds, the child process will not be able to obtain the lock.
This current behavior probably covers our race conditions

[Byron]

I also think this is flaky, but argue that this might be good enough to make tests stable enough.

After all, this really must be fixed on database level using a transaction around all migrations.
But maybe more 'stable' migrations are just a side-effect, and this really is about preventing too many update operations at a time. But even there I argue that the database is the natural spot of synchronisation. We just need access to these features which right now are totally hidden, kind of negating some of the benefits of a database.

[krlvi]

Do you have an idea for a different approach here? Would we want this pr merged?

[Copilot]

Pull request overview

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

[Byron]

Let's merge it, hoping that it fixes the extreme flakiness of the journey test. If not, we can go again.

The test reproduces nicely locally if the test is executed repeatedly. I used this previously:

# The executable is something that `cargo test -p but` prints.
 hyperfine --output inherit 'target/debug/deps/but-4d3970ee7e59a5d8 journey::from_empty'

[krlvi]

Let's merge it, hoping that it fixes the extreme flakiness of the journey test. If not, we can go again.

The test reproduces nicely locally if the test is executed repeatedly. I used this previously:

# The executable is something that `cargo test -p but` prints.

 hyperfine --output inherit 'target/debug/deps/but-4d3970ee7e59a5d8 journey::from_empty'

Does the issue reproduce even with the fix?

[Byron]

Back then I didn't reproduce it locally, but now we also know more as the problem still happens on with that particular test.
It's a bit puzzling also that it seems to have different results depending on running together with other tests, or by itself.
For now I'd tend to ignore the issue, and see if migration improvements solve it or using a file lock around the migration itself like you proposed.