test(sync-lock): cover the DB-backed global mutex#523
Conversation
sync-lock.ts (shared by /api/sync, /api/sync/scheduled, /api/automation/sync) had no tests despite being a concurrency primitive. Cover acquire-when-free, conflict-when-held, stale-lock reclaim at the 30-min boundary, the in-transaction race re-check, and release. Closes #506
…tches) Addresses #506 review note: releaseLock runs in callers' finally{} even when acquire failed or the row is already gone — deleteMany matches nothing and must not throw.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
PR PR 523 Review: test(sync-lock): cover the DB-backed global mutex
Recommendation: Approve
This PR adds comprehensive unit tests for src/lib/sync-lock.ts, the DB-backed global mutex shared by /api/sync, /api/sync/scheduled, and /api/automation/sync. The implementation is well-structured and fully addresses the linked issue PR 506.
Change-by-Change Findings
| File | Change | Assessment |
|---|---|---|
src/lib/sync-lock.test.ts |
New 93-line test file | ✅ Correct |
Test coverage verified:
- Acquire when free —
acquires when no lock is heldcreates both theIssueSyncRunandsyncLockrows, confirming the full happy path. - Conflict on fresh lock —
conflicts when a fresh lock is heldverifies 409-equivalent behavior ({ locked: false }) and confirms$transactionis never called (early bail-out). - Stale lock reclaim —
reclaims a stale lock (>30 min) then acquirestests theMAX_AGE_MS = 30 * 60 * 1000boundary, mocking the outer check to see the stale lock and the in-tx re-check to see it cleared. - In-transaction race —
conflicts when another writer wins the race inside the transactionverifies the double-check pattern inside$transactionthrowsalready_lockedwhen a concurrent writer grabs the row first. - Conditional release —
deletes only this run's lock rowandsafe no-op when no matching row existsconfirmreleaseLockusesdeleteManywith bothid: "global"andsyncRunIdconstraints.
Sources
| Source | Evidence |
|---|---|
Tool: read_file (src/lib/sync-lock.ts) |
Verified the production code under test — MAX_AGE_MS = 30 * 60 * 1000, acquireLock with outer check + in-tx re-check, releaseLock with deleteMany conditional, already_locked error message. |
Tool: git_grep (vi.hoisted) |
Confirmed the vi.hoisted() pattern is the established convention across 40+ test files in this repo. |
Tool: git_log (src/lib/sync-lock.test.ts) |
Two commits: initial coverage addition and the release-on-any-path follow-up. |
| CI Check Results | All 5 checks (Lint, Tests, Build, Typecheck, Docker Build) completed with success. |
Standards Compliance
| Standard | Status | Notes |
|---|---|---|
Mocking pattern (vi.hoisted + vi.mock) |
✅ | Consistent with existing test files (40+ examples in codebase). |
Error handling pattern (err instanceof Error) |
✅ | Uses err instanceof Error && err.message === "already_locked" in production code; tests exercise this path. |
| Prisma mock structure | ✅ | mocks.syncLock.findUnique, create, delete, deleteMany and mocks.issueSyncRun.create match the actual Prisma client calls. |
Test isolation (vi.clearAllMocks in beforeEach) |
✅ | Properly resets mocks between tests. |
Linked Issue Fit
Issue PR 506 specified:
- ✅ Test acquire-when-free → "acquires when no lock is held"
- ✅ Test acquire-when-held (409) → "conflicts when a fresh lock is held"
- ✅ Test stale-lock reclaim at 30-min boundary → "reclaims a stale lock (>30 min) then acquires"
- ✅ Test release-on-failure → Both
releaseLocktests verify safe release-on-any-path viadeleteMany
The PR matches the issue exactly. The issue body notes "Effort: M" and this single-file, 93-line addition is appropriately scoped.
Evidence Provider Findings
No evidence providers configured. N/A.
Tool Harness Findings
The 4-tool plan completed successfully (read_file, git_log, git_grep, git_status_short). All returned ok status.
Unknowns / Needs Verification
None. All required verification was completed:
- Production code read and verified against tests
- Mock patterns confirmed against existing test conventions
- CI results confirm all checks pass
- Linked issue acceptance criteria fully covered
Conclusion
This PR is a clean, well-scoped test addition that fully addresses issue PR 506. The tests are thorough, use repository-standard patterns, and all CI checks pass. Approve.
Summary
src/lib/sync-lock.ts(previously untested) — the DB mutex shared by/api/sync,/api/sync/scheduled, and/api/automation/sync.Covers: acquire-when-free (creates run + lock row), conflict-when-fresh-lock-held (bails before the tx), stale-lock reclaim at the 30-min boundary, the in-transaction race re-check (another writer wins → conflict), and
releaseLockdeleting only the matching run's row.Verification
vitest run src/lib/sync-lock.test.ts→ 5 passing.tsc→ 0;eslint→ clean.Closes #506 · part of #498