feat: harden contest sync lease with safe release, overlap guard, tests & docs - #284
Conversation
…ts & docs - SyncLock now tracks ownerId so releases are scoped to the acquiring instance and can never delete another instance's active lease. - Lease is released immediately after each run instead of relying solely on TTL expiry, reducing unnecessary wait time between ticks. - Added an in-process isRunning guard so a slow sync cannot overlap with the next scheduled tick on the same instance. - Logs now include a run ID and owner ID, and distinguish skipped vs failed vs successful runs. - Added automated tests (node:test) covering concurrent acquisition, DB failure, lease handoff, and scoped release. - Documented the chosen strategy in server/jobs/README.md. Closes kunalverma2512#277
|
@ida-jemi is attempting to deploy a commit to the Kunal Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
🎉 Welcome to CodeLens — Thank You for Your Contribution!Hey @ida-jemi! 👋 We are genuinely excited to have you here. Every single PR — big or small — makes CodeLens better, and yours is no exception. Take a moment to review the checklist below to help us merge your work quickly and smoothly. ✅ Before Requesting a Review
💬 Join Our Community Channel — This is MandatoryBeing part of our communication channel is compulsory for all contributors, not optional. Why join? This is where all important announcements, PR review updates, contribution discussions, and maintainer decisions happen in real time. Contributors who are not in the channel regularly miss critical context and updates, which often leads to duplicated or misaligned work. Staying connected here is what keeps the community strong and your contributions impactful. We are rooting for you! If you have any questions, drop them in the channel or comment right here on this PR. Let's build something great together. 🚀✨ |
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Node tests for ChangesContest sync lock validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/jobs/contestSync.test.js`:
- Around line 51-60: Update the “a new owner can acquire once the previous lease
has expired” test around acquireLock to make the mocked
SyncLock.findOneAndUpdate inspect its filter argument and assert that
lockedUntil uses a $lt predicate with a Date value. Keep the existing successful
acquisition assertion, ensuring the test fails if acquireLock omits the expiry
condition.
- Line 6: Extend the tests around startContestSyncJob to cover successful
synchronization, failed synchronization that still releases the lock, and
skipped execution when the lock is unavailable. Extract or inject the runSync
dependency so each scheduler path can be exercised deterministically, while
retaining the existing acquireLock/releaseLock coverage.
In `@server/jobs/README.md`:
- Around line 15-18: The jobs documentation must not claim safe recovery from
overlap based only on LOCK_TTL_MS and the per-instance isRunning flag. Update
the lease behavior and recovery description to document the implemented
renewal/fencing mechanism, or state and enforce a TTL longer than the maximum
possible run duration before asserting that expired leases can be safely taken
over.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 015c448f-42b3-41c1-b5ca-b978928a1dff
📒 Files selected for processing (3)
server/jobs/README.mdserver/jobs/contestSync.test.jsserver/package.json
…s, cover all scheduler outcomes, fix expiry-predicate test - Extracted runSync into createRunSync() with injectable acquire/ release/renew/syncFn dependencies, enabling deterministic unit tests of the actual scheduling logic (not a reimplementation of it) for all four outcomes: synced, failed-with-release, skipped-locked, skipped-running. - Added renewLock(): while a sync is active, the lease is renewed at half the TTL, scoped to ownerId. A run that legitimately exceeds LOCK_TTL_MS no longer loses its lease to another instance mid-run; only a genuinely crashed/hung holder (which stops renewing) lets the TTL lapse. - Fixed the lease-expiry test, which previously always returned success regardless of the lockedUntil filter — it now captures and asserts the actual query predicate. - Updated README to document the renewal behavior. Addresses CodeRabbit review feedback on this PR
|
Hi @kunalverma2512, Pushed a follow-up commit addressing all three review comments:
All 12 tests pass locally ( |
kunalverma2512
added
documentation
|
@ida-jemi Merging. Phenomenal work. The code quality here is what sets this apart - named constants, explicit CAS validation, scoped error handling, and dependency injection purely for testability. This is not just correct code, it is principled code that the next engineer will understand without a single question. The lease renewal was the last real gap in PR #280 and you closed it precisely. A crashed holder now stops renewing and lets the TTL lapse naturally that is the textbook-correct solution to the hung-sync problem. Beyond correctness, you also gave this codebase its first automated test suite and its first jobs documentation. Both will outlive this PR by years. What makes you an indispensable contributor is not just technical depth but the follow-through you did not stop at "it works." You thought about what breaks it, wrote tests for those exact scenarios, and documented it so the next person inheriting this never has to reverse-engineer your intent. That discipline is rarer than the implementation itself. On more thing I would genuinely love to see your eye applied to the frontend side of CodeLens. Hop onto the Matrix development channel and let us discuss what you could pick up there. You have earned that conversation. Thank you for being exceptional. This is exactly the bar. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
📌 Pull Request Summary
🔗 Related Issue
Closes #277
📝 Description
Changes Made
ownerIdto theSyncLockmodel so a lease is tagged with exactly which instance/run acquired it, allowing safe, scoped release that can never delete another instance's active lease.server/jobs/contestSync.jsto release the lease immediately after each run completes (success or failure) instead of relying solely on TTL expiry, so the next legitimate run doesn't wait unnecessarily.isRunningguard so a single instance cannot start an overlapping sync if a previous run is still in flight when the next scheduled tick fires.server/jobs/contestSync.test.js, using Node's built-innode:testrunner) covering concurrent lock acquisition, DB-unreachable degradation, lease handoff after expiry, and owner-scoped release.server/jobs/README.md.npm test(node --test) since the repo had no test runner configured yet.Motivation
This is a follow-up to #276 covering the full High-severity finding from the review of #269: an in-process cron scheduler has no leader-election or distributed-lock mechanism, so every backend replica independently runs its own schedule. #276 introduced a basic TTL-based lock, but this issue requires the harder production-safety guarantees called out in its acceptance criteria, safe lock release scoped to the correct owner, protection against overlapping runs on the same instance, recovery from a crashed lock holder, and test/documentation coverage, none of which #276 addressed on its own. This PR closes those gaps so the contest sync job is safe to run across any number of replicas without multiplying external API calls, DB writes, or log noise.
🚀 Type of Change
Select all that apply:
🧪 Testing
Verification
Test Details
node --test, covering: successful lock acquisition, concurrent acquirers racing for the same lease (only one wins via duplicate-key handling), gracefulfalsereturn (no throw) when the DB is unreachable, reacquisition after lease handoff, owner-scopedreleaseLockdeletion, andreleaseLocknot throwing on DB failure. All 6 pass.node server.jsinstances against the same MongoDB within that window. Instance 1 loggedAcquired lease ... starting sync→synced 24 contest(s); Instance 2 logged onlySkipped ... another instance holds the lease.with no acquire/sync lines, confirming exclusivity under real concurrency. Delay was removed before committing.synclockscollection holds exactly one document forjobName: "contestSync", and that it is removed immediately after a successful run rather than persisting for the full TTL window.📸 Screenshots / Demo (If Applicable)
Terminal 1 & 2:
npm test:
✅ Checklist
📚 Additional Notes
This PR is based on top of the
fix/contest-sync-distributed-lockbranch from #276 rather thanmain, since it extends that work directly, please merge #276 first (or review this PR's diff against that branch) to avoid seeing unrelated changes. The chosen approach is a MongoDB-backed lease (Option 2 from the issue) rather than a dedicated worker process or queue, since it requires no new infrastructure; the tradeoffs and a migration path to a dedicated worker are documented inserver/jobs/README.mdfor future reference.Summary by CodeRabbit
Documentation
Tests