Skip to content

fix: acquire lock in rr sync to prevent file overwrites during execution#182

Merged
rileyhilliard merged 1 commit intomainfrom
fix/sync-lock-acquisition
Feb 14, 2026
Merged

fix: acquire lock in rr sync to prevent file overwrites during execution#182
rileyhilliard merged 1 commit intomainfrom
fix/sync-lock-acquisition

Conversation

@rileyhilliard
Copy link
Copy Markdown
Owner

@rileyhilliard rileyhilliard commented Feb 14, 2026 •
edited by coderabbitai bot
Loading

Summary

Fixes #181

  • rr sync now acquires a distributed lock before syncing files, preventing overwrites while another rr run or rr <task> is executing on the same host
  • Lock is skipped for dry-run mode (no files modified) and local connections (no remote contention)
  • Added regression test that verifies lock.Acquire is called in sync.go

Before: Connect -> Sync (no lock, could clobber files mid-execution)
After: Connect -> Lock -> Sync -> Release (matches SetupWorkflow pattern)

Test plan

  • rr verify passes (lint + all unit tests)
  • Pre-push hooks pass (lint, tests, coverage)
  • Regression test TestSync_AcquiresLockBeforeSync verifies lock acquisition exists in source
  • Manual: run rr run "sleep 30" in one terminal, then rr sync in another - sync should wait for lock

Summary by CodeRabbit

  • New Features

    • Sync operations now acquire a distributed lock before file synchronization, with a spinner indicating lock acquisition. Sync proceeds only after the lock is held; failures block syncing.
    • Option to skip lock acquisition and automatic skipping for dry-run and local connections. Dry-run preserves project sync config and appends dry-run flags.

  • Tests

    • Added regression test validating lock acquisition and release around sync.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 14, 2026 •
edited
Loading

Warning

Rate limit exceeded

@rileyhilliard has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 43 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Walkthrough

Sync now acquires a distributed lock before performing file synchronization (skipped for dry-run, SkipLock option, or local connections). A new SkipLock option was added. Tests were added to verify the lock acquisition call and release are present in the sync flow.

Changes

Cohort / File(s) Summary
Sync command + lock addition
internal/cli/sync.go		 Adds import of lock subsystem and a SkipLock bool field on SyncOptions. Introduces a new "Acquire lock" phase before sync: determines project/global lock config, displays spinner while acquiring, returns on acquire failure, defers release (non-fatal on release error), reports phase update, then proceeds to sync. Skips lock when DryRun, SkipLock, or local connection. Preserves dry-run/config behavior.
Lock-related tests
internal/cli/sync_test.go		 Adds a compile-time usage check referencing SyncOptions{SkipLock: true} and a regression test TestSync_AcquiresLockBeforeSync that inspects sync.go to ensure lock.Acquire( appears before the sync.Sync call and that lck.Release() is present.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant CLI as CLI (rr sync)
participant Conn as Connection
participant Lock as LockService
participant Sync as SyncEngine
participant Remote as Remote

CLI->>Conn: establish connection
Conn->>CLI: connection ready
CLI->>Lock: Acquire(project/global config)
Lock-->>CLI: lock acquired
CLI->>Sync: perform file sync
Sync->>Remote: transfer files
Remote-->>Sync: ack
CLI->>Lock: Release()
Lock-->>CLI: released

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: adding lock acquisition to rr sync to prevent file overwrites, which directly addresses the bug in the linked issue.
Linked Issues check ✅ Passed The PR fully implements the requirements from issue #181: lock acquisition before sync, lock release after, skipping for dry-run/local connections, and matching the SetupWorkflow pattern.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #181: lock import, SkipLock option, lock phase, and regression test verifying lock acquisition.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/sync-lock-acquisition

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rileyhilliard rileyhilliard force-pushed the fix/sync-lock-acquisition branch from a39d11d to 08275d3 Compare February 14, 2026 04:31
@rileyhilliard
fix: acquire lock in rr sync to prevent file overwrites during execut…
84ee09a 
…ion (#181)

rr sync skipped lock acquisition, allowing it to overwrite remote files
while another rr process was actively executing. Added lock phase between
connect and sync, matching the pattern used by SetupWorkflow. Lock is
skipped for dry-run mode and local connections.
@rileyhilliard rileyhilliard force-pushed the fix/sync-lock-acquisition branch from 08275d3 to 84ee09a Compare February 14, 2026 04:39
@rileyhilliard rileyhilliard merged commit 953e4b0 into main Feb 14, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: rr sync does not acquire lock, can overwrite files during active execution

1 participant

@rileyhilliard