feat(sync): share sync locking across all sync entry-points#333
Conversation
Extract DB-backed lock from scheduled sync into shared module (src/lib/sync-lock.ts) and apply it to manual issue sync and automation sync endpoints. This prevents overlapping concurrent runs across all three sync types (scheduled, manual, automation), addressing race conditions from browser refreshes, cron overlap, or repeated clicks. - New src/lib/sync-lock.ts: acquireLock(syncType) / releaseLock(runId) with stale lock cleanup (>30 min) and transactional double-check - src/app/api/sync/route.ts: acquire lock before issue sync, return 409 if a sync is already running - src/app/api/automation/sync/route.ts: acquire lock before automation sync (single repo or batch), return 409 if locked - src/app/api/sync/scheduled/route.ts: replace inline lock with shared module import, update error message to be sync-type-agnostic - prisma/schema.prisma: update syncType comment to include 'automation'
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: review@https://litellm.jory.dev/v1 (openai)
Recommendation
Approve. This PR correctly addresses the concurrency issues identified in PR 313 by extracting the database-backed locking logic into a shared module and applying it consistently across all sync entry-points (scheduled, manual, and automation).
Change-by-Change Findings
src/lib/sync-lock.ts(Added): Implements a robust, shared locking mechanism using thesyncLocktable. It includes necessary safety features: a 30-minute stale lock cleanup and transactional double-checking to prevent race conditions during lock acquisition.prisma/schema.prisma(Modified): Updates theIssueSyncRun.syncTypeenum/comment to includeautomation, ensuring the audit trail correctly reflects all sync types.src/app/api/automation/sync/route.ts(Modified): Integrates the newacquireLockandreleaseLocklogic. It correctly returns a409 Conflictif a sync is already in progress and uses atry...finallyblock to ensure the lock is released even if the sync fails.src/app/api/sync/route.ts(Modified): Integrates the shared lock for manual issue syncs. It also adds logic to update theIssueSyncRunrecord tocompletedupon successful sync, improving observability.src/app/api/sync/scheduled/route.ts(Modified): Refactors the previously inline lock implementation to use the sharedsync-lock.tsmodule, ensuring the scheduled sync respects the same global lock as other sync types.
Standards Compliance
- Error Handling: Follows the
error instanceof Errorpattern and provides meaningful HTTP status codes (409 for conflicts, 500 for internal errors). - Validation: Validates lock acquisition before proceeding with heavy GitHub/database operations.
- Code Standards: The implementation is internally consistent and follows the repository's pattern of using Prisma for state management.
Linked Issue Fit
- Matches PR 313: The PR directly implements the recommendation to share sync locking/concurrency controls across all sync entry-points to prevent overlapping runs caused by cron overlap, browser refreshes, or repeated clicks.
The manual sync and automation sync routes now use acquireLock() which requires prisma.syncLock and prisma.. Add these mocks to the test fixtures so the tests pass without a real database.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: review@https://litellm.jory.dev/v1 (openai)
Recommendation
Approve. This PR correctly addresses the race conditions identified in issue PR 313 by centralizing the synchronization locking mechanism into a shared module. This ensures that scheduled, manual, and automation syncs cannot run concurrently, preventing overlapping GitHub API calls and database write conflicts.
Change-by-Change Findings
1. Shared Locking Module (src/lib/sync-lock.ts)
- Implementation: Introduced a robust
acquireLockandreleaseLockmechanism using a single-row guard in thesyncLocktable. - Safety: Uses a database transaction to prevent race conditions during lock acquisition and implements a
MAX_AGE_MS(30 minutes) to automatically clear stale locks. - Traceability: The lock acquisition creates an
IssueSyncRunrecord, allowing for better auditability of which sync type holds the lock.
2. API Route Updates
src/app/api/automation/sync/route.ts: Now acquires anautomationlock before proceeding. Correctly uses atry...finallyblock to ensure the lock is released even if the sync fails.src/app/api/sync/route.ts: Now acquires amanuallock. Added logic to update theIssueSyncRunrecord tocompletedupon successful sync.src/app/api/sync/scheduled/route.ts: Refactored to use the shared module instead of the previous inline implementation, ensuring consistency across all sync types.
3. Schema and Testing
prisma/schema.prisma: UpdatedsyncTypeenum/comment to includeautomationfor consistency.- Tests: Added unit tests for the new sync routes (
automation/syncandsync) to verify lock acquisition, 409 Conflict responses, and lock release behavior.
Standards Compliance
- Error Handling: Follows the
error instanceof Errorpattern and returns appropriate HTTP status codes (409 for lock conflicts, 500 for internal errors). - Code Standards: Implementation is internally consistent and follows the established pattern of using
prisma.$transactionfor atomic operations. - Validation: Validates lock status before proceeding with heavy I/O operations (GitHub API calls).
Linked Issue Fit
- Issue PR 313: This PR directly satisfies the requirement to share sync locking/concurrency controls across all sync entry-points, specifically addressing the lack of shared locking between scheduled, manual, and automation syncs.
Fixes #313
Extract DB-backed lock from scheduled sync into shared module
src/lib/sync-lock.tsand apply it to manual issue sync and automation sync endpoints. This prevents overlapping concurrent runs across all three sync types (scheduled, manual, automation), addressing race conditions from browser refreshes, cron overlap, or repeated clicks.Changes
src/lib/sync-lock.ts:acquireLock(syncType)/releaseLock(runId)with stale lock cleanup (>30 min) and transactional double-check