fix: add MongoDB-based distributed lock to contest sync cron job - #280
Conversation
Prevents duplicate execution when multiple server instances run concurrently (e.g. horizontal scaling). Uses an atomic findOneAndUpdate on a new SyncLock collection so only one instance per hourly tick runs syncCodeforcesContests(); lock failures degrade gracefully instead of crashing. Fixes kunalverma2512#276
|
@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. 🚀✨ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe contest synchronization job now uses a MongoDB-backed expiring lock. Startup initializes the lock model, and each run acquires and releases the lock while preventing overlapping local executions. ChangesContest sync distributed locking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant ContestSyncJob
participant SyncLock
participant ContestService
Server->>ContestSyncJob: Await job initialization
ContestSyncJob->>SyncLock: Initialize indexes
ContestSyncJob->>SyncLock: Acquire expired lease
SyncLock-->>ContestSyncJob: Grant or deny ownership
ContestSyncJob->>ContestService: Sync contests when granted
ContestSyncJob->>SyncLock: Release owned lease
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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: 1
🤖 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/models/SyncLock.js`:
- Around line 12-20: Update the startup flow in startContestSyncJob() so it
awaits SyncLock.init() (or syncIndexes()) after connectDB() and before
scheduling the cron, ensuring the unique jobName index exists before any lock
acquisition.
🪄 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
Run ID: de5cddac-43ee-4b68-85ca-6f10e7906652
📒 Files selected for processing (2)
server/jobs/contestSync.jsserver/models/SyncLock.js
|
Please perform a COMPLETE production-grade review of this PR. This PR fixes a high-severity distributed systems issue related to the contest sync cron job by introducing a MongoDB-based distributed locking mechanism. This is NOT a feature review. This is a correctness, concurrency, and production reliability review. Please review every changed file and every changed line of code:
Review this as if you are the senior backend engineer responsible for approving a production deployment. 1. Distributed Lock CorrectnessVerify whether the locking algorithm is actually correct. Specifically check:
If you find even a theoretical race condition, explain it in detail. 2. Failure ScenariosReview every failure path. Examples:
Explain exactly what happens in every case. 3. Lock LifecycleCarefully inspect:
Verify there is no path that leaves the system permanently locked or permanently unlocked. 4. Cron Job ReviewReview contestSync.js completely. Check:
Verify that skipped executions behave exactly as intended. 5. MongoDB Model ReviewReview SyncLock.js in depth. Check:
Suggest improvements if applicable. 6. Production ReadinessThink beyond the current deployment. Would this implementation still be correct if the application runs:
If not, explain exactly why. 7. Edge CasesLook specifically for hidden production bugs such as:
8. Code QualityReview whether the implementation follows professional backend engineering standards. Evaluate:
Point out any code smell. 9. Alternative DesignsIf there is a more robust production approach, explain it. Examples:
Explain the trade-offs. 10. Testing ReviewDetermine whether the contributor has tested enough. Suggest additional tests for:
Mention any missing integration or concurrency tests. 11. Review Output FormatFor EVERY issue found, provide:
Do not limit the review to style or syntax. Focus on correctness, concurrency, distributed systems, production stability, fault tolerance, scalability, and long-term maintainability. Finally answer these questions explicitly:
Please be extremely critical. Assume this code will run in production for years and look for subtle bugs that are easy to miss during normal code review. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "=== contestSync.js ==="
fd contestSync.js -x cat -n {}
echo "=== SyncLock.js ==="
fd SyncLock.js -x cat -n {}
echo "=== server.js ==="
fd server.js --max-depth 2 -x cat -n {}
echo "=== search for cron usage ==="
rg -n "cron|schedule|setInterval" --type=js -g '!node_modules'
echo "=== search usages of startContestSyncJob ==="
rg -n "startContestSyncJob" --type=jsLength of output: 4811 🏁 Script executed: #!/bin/bash
set -e
fd package.json --max-depth 2 -x cat -n {} \;
echo "=== mongoose version specifically ==="
rg -n '"mongoose"' server/package.json 2>/dev/null || fd package.json -x grep -n mongoose {}
echo "=== node-cron version ==="
rg -n '"node-cron"' server/package.json 2>/dev/null || fd package.json -x grep -n node-cron {}
echo "=== ContestService.syncCodeforcesContests ==="
fd service.js -p contests -x cat -n {}Length of output: 3183 🏁 Script executed: #!/bin/bash
set -e
fd -a "service.js" server/modules
cat -n server/modules/contests/service.js 2>/dev/null || fd service.js server/modules -x cat -n {}Length of output: 4405 🏁 Script executed: #!/bin/bash
fd repository.js server/modules/contests -x cat -n {}Length of output: 3719
1. Distributed Lock CorrectnessCan two instances acquire the lock simultaneously? No. The mechanism is sound, but it's implicit rather than explicit:
Since MongoDB serializes writes on the same document/unique key at the storage-engine level, this does provide correct mutual exclusion — no split-brain, no simultaneous acquisition. However, this is a fragile way to express a CAS: it depends entirely on error-code sniffing rather than checking the returned document. If a future refactor changes the filter shape, adds retryable-write semantics, or a driver upgrade alters error wrapping, this silently breaks. Recommend an explicit check: const result = await SyncLock.findOneAndUpdate(
{ jobName, lockedUntil: { $lt: now } },
{ jobName, lockedUntil: new Date(now.getTime() + ttlMs) },
{ upsert: true, returnDocument: "after" }
);
return Boolean(result); // defensive, in addition to the catch blockSeverity: Medium — File: 2. Failure Scenarios
3. Lock Lifecycle
No path leaves the system permanently locked (TTL bounds it) or permanently unlocked (unique index + filter always converge on a single winner). Verdict: lifecycle is safe but coarse — it trades instant release/renewal for simplicity. 4. Cron Job Review (
|
| Environment | Correct? | Notes |
|---|---|---|
| 2-5 Node instances | Yes | Confirmed correct via the unique-index CAS mechanism. |
| PM2 cluster mode | Yes, with a caveat | Each PM2 worker is a separate process with its own clock/event loop — same guarantees apply. |
| Kubernetes / Railway / Render replicas | Yes, with a caveat | The clock-drift issue (Section 7) becomes more relevant in multi-node/multi-AZ deployments where NTP sync isn't guaranteed to sub-second precision. |
| Docker Swarm | Yes | Same as above. |
| Future autoscaling | Mostly yes | As replica count grows, contention on the single SyncLock doc increases proportionally (more E11000 "losers" per tick), but this is cheap — no bottleneck concern at any realistic scale for an hourly job. |
Caveat that applies to all of the above: the correctness of the entire scheme hinges on the unique index on jobName actually existing in the target Mongo deployment (see Section 5) — this is an operational dependency that isn't verified/enforced anywhere beyond SyncLock.init().
7. Edge Cases
- Clock skew (real, unaddressed):
nowinacquireLockis each Node process's localDate.now(). If instance A's clock is fast, it may consider a lock expired sooner than it should be (from the setter's perspective), or if the setter's clock was fast when writinglockedUntil, other instances may see the lock as valid longer than the 5-minute intent. This is not catastrophic here (worst case: a slightly early/late reacquisition on an idempotent job), but for a "run for years" system, prefer server-side time via an update pipeline (update: [{ $set: { lockedUntil: { $add: ["$$NOW", ttlMs] } } }]) so all time math happens on the Mongo server's single clock, eliminating app-to-app clock skew entirely. - Lock expiry timing / long-running sync: as discussed, TTL (5 min) < tick interval (60 min) normally provides ample margin, but there is no hard timeout on the Codeforces HTTP call (
cfGetContestList) — an indefinitely hanging request would hold the lock in a "logically running but not renewing" state past its TTL, allowing another instance (or the same one on a later tick) to start a concurrent run. Recommend adding an explicitaxiostimeout tocfGetContestListand/or anAbortControllerfor the sync call. - Duplicate cron ticks / lost updates / deadlocks / infinite lock ownership: none observed — the TTL bound rules out infinite ownership and deadlocks.
- Partial failures:
bulkWrite({ ordered: false })inContestRepository.bulkUpsertContestsmeans one bad doc doesn't block the rest — good; combined with the idempotent upsert key, partial/duplicate runs self-heal on the next tick.
8. Code Quality
- Naming and comments are good — the docstrings on
acquireLockand theSyncLockSchemaexplain intent clearly. - Code smell: the unused/uninspected result of
findOneAndUpdatecombined with thereturnDocument: "after"option that is never read (Lines 20-24) — either use the returned doc for a defensive check or drop the option, since as written it does nothing. - Separation of concerns is reasonable: lock logic lives entirely in
contestSync.js/SyncLock.js, sync logic stays inContestService. - Logging quality is decent but lacks a per-instance identifier for correlating across replicas in production log aggregators.
- No defensive timeout on the external HTTP call driving the whole lock-duration assumption.
9. Alternative Designs (trade-offs)
- Lease renewal (heartbeat): an interval that re-extends
lockedUntilevery N seconds while the sync runs would eliminate the "TTL expires mid-sync" risk entirely, at the cost of slightly more code and one extra periodic write. Recommended given how cheap it'd be to add. - Owner token + compare-and-delete release: store a random
ownerIdper acquisition andfindOneAndDelete({ jobName, ownerId })on completion to release the lock immediately rather than waiting out the TTL. Improves responsiveness for manual/ad-hoc reruns but is unnecessary complexity for a fixed hourly job. - Redis Redlock: explicitly avoided per the linked issue's objective ("avoid requiring new infrastructure such as Redis") — correct call for this scale.
- Platform-native cron (e.g., Kubernetes CronJob) instead of in-process node-cron: removes the need for app-level locking altogether by only ever running one Job pod, but couples the sync to infra config rather than app code — a bigger architectural change, probably out of scope here.
- Leader election (e.g., via Mongo-backed leader lease held for the process lifetime rather than per-tick): would let only the leader instance run any scheduled job, generalizing beyond just contest sync, at the cost of more moving parts.
Given the scope (single hourly job, no existing infra), the current MongoDB-upsert-lock approach is a reasonable, low-complexity choice — the main gap is the missing renewal/heartbeat for long-running syncs.
10. Testing Review
Manual two-instance testing (per PR images) validates the primary happy path (one syncs, one skips) and lock persistence across restarts. Missing/recommended tests:
- Unit test for
acquireLockmockingSyncLock.findOneAndUpdateto throw{ code: 11000 }→ assertfalsereturned, no crash. - Unit test for
acquireLockmocking a generic Mongo error (e.g.,MongoNetworkError) → assertfalsereturned and a warning is logged, not a throw. - Integration test using
mongodb-memory-server(or similar) with two concurrent calls toacquireLockfor the samejobName→ assert exactly one resolves "acquired" and the doc'slockedUntilis set once. - Test for lock expiry: manually set
lockedUntilin the past, callacquireLockagain, assert it succeeds (renewal path). - Test for
SyncLock.init()actually creating the unique index (guards against silentautoIndex: falseregressions). - Stress/race test: fire N (e.g., 20) concurrent
acquireLockcalls for the same freshjobNameand assert exactly one true, N-1 false, and no unhandled exceptions. - No integration test exists exercising the actual
runSyncskip-logging path or the immediate-run-on-boot path (Line 60).
Answers to your explicit questions
- Is this distributed locking implementation actually correct? Yes, for the core mutual-exclusion guarantee at a given moment in time — verified by manual testing and confirmed algorithmically via MongoDB's per-document write serialization + unique index conflict.
- Is there any remaining race condition? Not a simultaneous-acquisition race, but there is a real overlap-on-expiry gap: if a sync outlives the 5-minute TTL (no heartbeat/renewal), a second instance (or the same instance's next tick) can start concurrently. Downstream writes are idempotent, so this degrades to wasted work rather than data corruption.
- Is it safe under horizontal scaling? Yes, conditional on the unique index on
SyncLock.jobNameactually existing in production (verifyautoIndexsettings). - Could duplicate executions still happen? Yes, in the narrow long-running-sync-exceeds-TTL scenario described above — but "duplicate" here means duplicate work, not duplicate/corrupted data, thanks to
ContestRepository.bulkUpsertContests's idempotent upsert key. - Are there any edge cases that remain unsolved? Clock skew across app instances (no server-side time source used), no HTTP timeout bounding the sync duration relative to the lock TTL, and dependence on
autoIndexfor the unique index. - Is this PR production-ready? Yes, for the stated scope and current scale (single hourly job, small number of replicas) — it correctly solves the duplicate-sync problem described in the linked issue.
- Must anything be fixed before merging? Nothing blocking; the following are strongly recommended as fast follow-ups: (a) add an HTTP timeout to
cfGetContestListso lock duration assumptions hold, (b) explicitly verify/enforce the unique index exists regardless ofautoIndex, (c) optionally add a defensive check on thefindOneAndUpdateresult. - Overall merge confidence: 8/10.
- Overall production reliability score: 7/10 (would be 9/10 with a heartbeat/lease-renewal mechanism and a bounded HTTP timeout on the sync call).
✅ Action performed
Review finished.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server.js`:
- Line 11: Update startContestSyncJob() invocation in startServer() to catch
SyncLock.init() failures, log the error, and leave contest synchronization
disabled while allowing app.listen() to continue. Preserve normal job startup
when lock initialization succeeds.
🪄 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
Run ID: 2db160d1-2d5e-4098-8bc0-04a1c3b5e575
📒 Files selected for processing (2)
server/jobs/contestSync.jsserver/server.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/jobs/contestSync.js
|
@ida-jemi Please see coderabbitai comment please do a proper reasoning and confirm me ones done that things are finally ready to merge or not NOTE : please provide a report for the reasoning you will do about any further changes you will be doing and why and how it improves the code and overall work Thanks |
…e, and handle SyncLock init failure gracefully - Fixes a syntax error from a bad merge (duplicate const runSync). - acquireLock/releaseLock now use an ownerId per acquisition so only the owning instance can release its own lease, and release happens immediately after each run instead of waiting out the full TTL. - startContestSyncJob() now catches SyncLock.init() failures so a background job init problem can't prevent app.listen() from running. Addresses CodeRabbit review feedback on PR kunalverma2512#280
|
@kunalverma2512 I've reviewed the comments and made a few changes: (a) Startup could crash on a background-job failure
Fix:
(b)
|
| Item | CodeRabbit's verdict | Why it's reasonable to leave for a follow-up |
|---|---|---|
| No HTTP timeout on the Codeforces API call | Recommended fast-follow | This belongs in ContestService/cfGetContestList, not the locking implementation. It's a separate concern in a different file and is better handled in its own focused PR rather than expanding the scope of this one. |
| Clock drift across instances | "Not catastrophic" (explicitly marked as non-blocking) | The worst-case outcome is a slightly early or late lock reacquisition on an already-idempotent job. A more robust solution, using MongoDB server time (e.g., via an aggregation pipeline update), is a reasonable future enhancement, but it's not urgent. |
| No lease renewal / heartbeat | "Real gap" but degrades to wasted work rather than data corruption | Since the sync operation is idempotent, the impact is limited. This PR addresses the concern differently by releasing the lock immediately after each run and adding an in-process isRunning guard, which substantially reduces the overlap window without introducing heartbeat complexity. |
autoIndex might be disabled, undermining the unique index |
"Please verify" | Verified directly by checking server/config/db.js. The configuration never modifies autoIndex, so Mongoose's default behavior (autoIndex: true) remains in effect. Confirmed to be a non-issue. |
Please do review the changes and if there are any more changes or improvements to be made pls do let me know
|
Hi @kunalverma2512, just following up since it's been about a week. Whenever you get a chance, could you take a look at the fixes I pushed addressing CodeRabbit's review (startup-crash handling on SyncLock.init() failure, and the explicit lock-ownership check)? Happy to make any further changes if you have feedback. Just wanted to bump this in case it got buried. Thanks! |
|
@ida-jemi Hi Thanks for keeping hope and I'll comeback soon Till then you can work on thinking that what more you can grow and make its issue and directly come up with PR Currently I am occupied a lot so I hope you'll understand |
|
@ida-jemi — Merging this. Genuinely well done. 🎉 I want to take a moment to actually say this properly: this was not a The core locking logic is correct and production-safe for our current I am going to keep two things out of this PR intentionally and open Follow-up 1 — HTTP timeout on the Codeforces API call Why these matter even though they are not blockers today: Follow-up 1 is the only scenario where duplicate execution can still Follow-up 2 matters the moment we run more than one replica. Logs from Both follow-ups are small and well-scoped. I will open them as labelled One small tip before you go: your written reasoning in the contributor Looking forward to seeing what you build next. |
|
@coderabbitai create new issue This is a follow-up to PR #280 which introduced a MongoDB-based distributed The lock uses a fixed 5-minute TTL. The problem is there is no HTTP timeout Please create a well-structured GitHub issue covering the full failure chain, |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh label list --repo kunalverma2512/CodeLens --limit 100Length of output: 1843 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="kunalverma2512/CodeLens"
body_file="$(mktemp)"
cat >"$body_file" <<'EOF'
## Summary
Follow-up to `#280`: bound the Codeforces API request used by contest synchronization so a hung HTTP response cannot outlive the MongoDB distributed-lock lease and permit a concurrent sync on another application instance.
## Failure chain
1. `startContestSyncJob()` acquires the `contestSync` MongoDB lease with a fixed five-minute TTL.
2. The job invokes `cfGetContestList` to fetch Codeforces contests.
3. If Codeforces accepts the TCP connection but never sends a response, the request has no application-level timeout and can remain pending until the host/OS network timeout (potentially 10–30 minutes).
4. While the original process is still awaiting that request, the five-minute lease expires.
5. A different replica sees the lease as expired, successfully acquires it, and starts a second `syncCodeforcesContests()` execution.
6. Both processes can then perform the same sync concurrently.
The database upserts are idempotent, so this should not corrupt contest records. However, it defeats the PR `#280` guarantee of one synchronization execution per scheduled interval and causes duplicate external API traffic and avoidable database work.
## Why `isRunning` does not prevent this
`isRunning` is process-local. It blocks a second `runSync` call only within the instance that is already waiting on the hung request. It is not stored in MongoDB and is not shared with PM2 workers, containers, Kubernetes pods, or other application replicas. Once the distributed lease expires, another process has no visibility into the first process's `isRunning` state and can legally acquire the lock.
## Affected areas
- `server/modules/contests/service.js` — `cfGetContestList` / Codeforces HTTP request configuration
- `server/jobs/contestSync.js` — the existing fixed five-minute distributed-lock lease relies on sync work being bounded
## Required changes
- Configure an explicit, finite HTTP timeout for the Codeforces request (15 seconds is the initial target proposed during review).
- Ensure timeout failures reject through the existing sync error-handling path, are logged with actionable context, and do not crash the server.
- Preserve the existing behavior that releases the owner-scoped lock in `finally`.
- Document the timeout choice and ensure it is configurable if project conventions support environment-based HTTP settings.
## Acceptance criteria
- [ ] A Codeforces request that does not return within the configured timeout fails predictably rather than waiting for OS-level TCP timeouts.
- [ ] The failed sync is logged and the owner-scoped distributed lock is released through the existing cleanup path.
- [ ] A hung/simulated slow request cannot remain active beyond the five-minute lock lease; the configured timeout must remain safely below that lease.
- [ ] Add or update tests covering the timeout/failure path and verifying that a later sync attempt can proceed.
- [ ] Verify normal successful Codeforces synchronization remains unchanged.
## References
- Follow-up to PR `#280`: https://github.com/kunalverma2512/CodeLens/pull/280
- Requested in CodeRabbit review comment: https://github.com/kunalverma2512/CodeLens/pull/280#issuecomment-4952346075
- Requested by `@kunalverma2512`
EOF
issue_url="$(gh issue create \
--repo "$repo" \
--title "fix(contest-sync): add a timeout to Codeforces API requests" \
--body-file "$body_file" \
--label "bug" \
--label "backend" \
--label "type:bug" \
--label "level:advanced" \
--assignee "kunalverma2512")"
printf '%s\n' "$issue_url"Length of output: 3836 The Comment image 1 shows a server instance completing the Codeforces contest sync; the Comment image 2 shows another instance correctly skipping because the distributed lock is held; the Comment image 3 shows the persisted
It includes the cross-process failure chain, why |
|
@coderabbitai create new issue This is a follow-up to PR #280 which introduced multi-instance awareness The logging in contestSync.js still uses a static prefix with no per-instance Please create a well-structured GitHub issue explaining the observability gap |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh label list --repo kunalverma2512/CodeLens --limit 100Length of output: 1843 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="kunalverma2512/CodeLens"
body_file="$(mktemp)"
cat >"$body_file" <<'EOF'
## Summary
Follow-up to `#280`: add a stable per-instance identifier to contest-sync logs so activity from multiple application replicas can be correlated in a shared log stream.
PR `#280` made the contest sync execution-safe across instances using a MongoDB-backed distributed lock. Its logs still use a static prefix such as `[Contest Sync]`, however. In a multi-replica deployment, this does not identify which process acquired the lock, completed a sync, skipped because another replica owns the lease, or emitted an error.
## Observability gap
When multiple instances run concurrently—such as PM2 cluster workers, Render/Railway replicas, Kubernetes pods, or Docker Swarm tasks—their stdout/stderr is commonly aggregated. Without an instance identifier, log lines are indistinguishable across processes.
For example, an operator can see:
- `[Contest Sync] Synced 24 Codeforces contest(s).`
- `[Contest Sync] Skipped - another instance holds the lock.`
but cannot determine which replica performed the sync and which replica skipped. This prevents reconstructing ownership and execution flow during an incident.
## Production impact
The lack of instance-level context makes the following scenarios unnecessarily difficult to debug:
- Investigating whether the distributed lock is consistently selecting one executor across replicas.
- Identifying the replica that produced sync failures, slow requests, or database errors.
- Correlating application logs with a specific Kubernetes pod, container, PM2 worker, host, or platform instance.
- Diagnosing restart loops or deployments where a newly started replica repeatedly attempts the immediate startup sync.
- Determining whether unexpected duplicate work came from the same process or separate replicas.
The lock remains correct, but incident response and post-incident diagnosis are impaired as horizontal scaling increases.
## Affected areas
- `server/jobs/contestSync.js` — contest-sync success, skip, lock-acquisition-failure, and error logs.
- Server configuration/environment handling — source or derive a stable identifier for the running process/replica.
## Required changes
- Add an instance identifier to every contest-sync log line.
- Prefer a configurable environment value when the deployment platform provides one; otherwise use a safe generated process-local fallback established at startup.
- Use a consistent log format for successful runs, lock contention/skips, acquisition failures, release failures, and sync errors.
- Ensure the identifier is safe to expose in logs and does not include secrets.
- Keep the locking behavior and existing sync semantics unchanged.
## Acceptance criteria
- [ ] Every contest-sync log emitted by `server/jobs/contestSync.js` includes a non-empty instance identifier.
- [ ] Two concurrently running instances produce distinguishable sync and skip log entries.
- [ ] The identifier remains consistent for the lifetime of a process.
- [ ] An environment-provided identifier can be used when available, with a documented fallback when it is absent.
- [ ] No credentials, connection strings, or other sensitive configuration values are logged.
- [ ] Existing contest-sync scheduling, distributed-lock acquisition, owner-scoped release, and error handling remain unchanged.
- [ ] Add or update tests for identifier selection and representative log output where the project's test conventions support it.
## References
- Follow-up to PR `#280`: https://github.com/kunalverma2512/CodeLens/pull/280
- Requesting comment: https://github.com/kunalverma2512/CodeLens/pull/280#issuecomment-4952346075
- Requested by `@kunalverma2512`
EOF
gh issue create \
--repo "$repo" \
--title "enhancement(contest-sync): add per-instance identifiers to distributed sync logs" \
--body-file "$body_file" \
--label "enhancement" \
--label "backend" \
--label "code-quality" \
--label "level:advanced" \
--assignee "kunalverma2512"Length of output: 4241 The PR description image 1 shows one server instance successfully completing the contest sync:
It is marked as a follow-up to Labels applied: |
kunalverma2512
added
enhancement
|
Quick reminder - please keep the development channel updated This channel exists specifically for this so the whole community Looking forward to seeing your updates here! 🙌 |
2 participants
📌 Pull Request Summary
🔗 Related Issue
Closes #276
📝 Description
Provide a clear and concise summary of the changes made in this pull request.
Changes Made
SyncLockmongoose model (server/models/SyncLock.js) representing a distributed lock document keyed byjobName, with alockedUntilexpiry timestamp.server/jobs/contestSync.jsto atomically acquire the lock viafindOneAndUpdatebefore runningsyncCodeforcesContests(). Only the instance that successfully acquires the lock runs the sync; all other instances skip that tick and log a message instead.server/server.js-startContestSyncJob()is still called the same way; the guard lives inside the job itself.Motivation
startContestSyncJob()schedules an hourlynode-cronjob entirely in-process, with no coordination between multiple running instances. As reported by CodeRabbit during review of PR #269, once this app is horizontally scaled (multiple dynos/replicas/PM2 cluster workers), every instance would independently fire its own timer and run the full Codeforces sync at the same wall-clock minute, hitting the Codeforces API N times instead of once, and multiplying DB write load for zero benefit. This PR introduces a MongoDB-based distributed lock (using an existing dependency, no new infra) so that exactly one instance executes the sync per scheduled tick, regardless of instance count.🚀 Type of Change
Select all that apply:
🧪 Testing
Verification
Test Details
Ran two
node server.jsinstances concurrently against the same MongoDB Atlas cluster to simulate horizontal scaling:[Contest Sync] Synced 24 Codeforces contest(s).while Terminal 2 logged[Contest Sync] Skipped - another instance holds the lock., confirming exactly one instance executes the sync per tick.synclockscollection in MongoDB Atlas contains a single lock document per job (jobName: "contestSync") with alockedUntilexpiry ~5 minutes ahead ofcreatedAt.📸 Screenshots / Demo (If Applicable)
Terminal 1:
Terminal 2:
✅ Checklist
📚 Additional Notes
Chose the MongoDB-based leader lock approach (option 1 from the issue) since MongoDB is already a project dependency and
findOneAndUpdateprovides the atomicity needed to avoid a read-then-write race condition between instances, no new infrastructure (e.g. Redis) required. Also fixed thefindOneAndUpdatedeprecation warning (new: true→returnDocument: "after") encountered while implementing this.Summary by CodeRabbit