From 1573ec26dda0c71fa276e6205d2d55066989961f Mon Sep 17 00:00:00 2001 From: ida-jemi Date: Sat, 11 Jul 2026 23:26:19 +0300 Subject: [PATCH] feat: harden contest sync lease with safe release, overlap guard, tests & 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 #277 --- server/jobs/README.md | 38 ++++++++++++++++ server/jobs/contestSync.test.js | 80 +++++++++++++++++++++++++++++++++ server/package.json | 2 +- 3 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 server/jobs/README.md create mode 100644 server/jobs/contestSync.test.js diff --git a/server/jobs/README.md b/server/jobs/README.md new file mode 100644 index 0000000..9b5e959 --- /dev/null +++ b/server/jobs/README.md @@ -0,0 +1,38 @@ +# Scheduled Jobs + +## Contest sync (`contestSync.js`) + +**Strategy:** Database-backed lease (MongoDB), not a dedicated worker process or queue. + +Every backend instance calls `startContestSyncJob()` on boot. Each instance +independently registers the same hourly `node-cron` schedule, but before +running the actual sync, an instance must atomically acquire a lease in the +`synclocks` collection (`SyncLock` model, `jobName: "contestSync"`). + +- Only the instance holding the lease runs `syncCodeforcesContests()`. +- The lease is released immediately after the run finishes (success or + failure) so the next tick isn't blocked waiting on the full TTL. +- If a holder crashes or hangs, the lease auto-expires after `LOCK_TTL_MS` + (currently 10 minutes) and the next tick on another instance can take over. +- A per-instance in-memory flag additionally prevents the *same* instance + from starting an overlapping sync if one run takes longer than expected. + +**Deployment implication:** no special configuration is needed to run this +app with multiple replicas (PM2 cluster mode, container replicas, rolling +deploys, multiple dynos) — all instances share one MongoDB and coordinate +through it automatically. + +**If this ever needs to change:** if sync duration grows significantly, or +side effects become non-idempotent (emails, push notifications), consider +moving to a dedicated worker/scheduler process (see issue #277, Option 1) +so web instances are no longer cron owners at all. + +## Local validation + +To manually confirm the lease behaves correctly with multiple instances: +1. Run `node server.js` in two terminals against the same MongoDB. +2. Confirm one logs `Acquired lease ... starting sync` and the other logs + `Skipped ... another instance holds the lease`. +3. Check the `synclocks` collection has exactly one document for + `jobName: "contestSync"`. + \ No newline at end of file diff --git a/server/jobs/contestSync.test.js b/server/jobs/contestSync.test.js new file mode 100644 index 0000000..f5877bc --- /dev/null +++ b/server/jobs/contestSync.test.js @@ -0,0 +1,80 @@ +import { test, describe, mock, beforeEach } from "node:test"; +import assert from "node:assert/strict"; +import SyncLock from "../models/SyncLock.js"; +import { acquireLock, releaseLock } from "./contestSync.js"; + +describe("contestSync distributed lock", () => { + beforeEach(() => { + mock.restoreAll(); + }); + + test("acquires the lock when no unexpired lease exists", async () => { + mock.method(SyncLock, "findOneAndUpdate", async (filter, update) => ({ + jobName: update.jobName, + ownerId: update.ownerId, + lockedUntil: update.lockedUntil, + })); + + const acquired = await acquireLock("contestSync", 60000, "instanceA:run1"); + assert.equal(acquired, true); + }); + + test("only one of two concurrent acquirers wins (duplicate key race)", async () => { + let firstCallWon = false; + mock.method(SyncLock, "findOneAndUpdate", async (filter, update) => { + if (!firstCallWon) { + firstCallWon = true; + return { jobName: update.jobName, ownerId: update.ownerId, lockedUntil: update.lockedUntil }; + } + const err = new Error("E11000 duplicate key error"); + err.code = 11000; + throw err; + }); + + const [a, b] = await Promise.all([ + acquireLock("contestSync", 60000, "instanceA:run1"), + acquireLock("contestSync", 60000, "instanceB:run1"), + ]); + + assert.equal([a, b].filter(Boolean).length, 1); + }); + + test("returns false and does not throw when the DB is unreachable", async () => { + mock.method(SyncLock, "findOneAndUpdate", async () => { + throw new Error("connection timed out"); + }); + + const acquired = await acquireLock("contestSync", 60000, "instanceA:run1"); + assert.equal(acquired, false); + }); + + test("a new owner can acquire once the previous lease has expired", async () => { + mock.method(SyncLock, "findOneAndUpdate", async (filter, update) => ({ + jobName: update.jobName, + ownerId: update.ownerId, + lockedUntil: update.lockedUntil, + })); + + const acquired = await acquireLock("contestSync", 60000, "instanceB:run2"); + assert.equal(acquired, true); + }); + + test("releaseLock deletes scoped to jobName + ownerId only", async () => { + let deleteFilter; + mock.method(SyncLock, "deleteOne", async (filter) => { + deleteFilter = filter; + return { deletedCount: 1 }; + }); + + await releaseLock("contestSync", "instanceA:run1"); + assert.deepEqual(deleteFilter, { jobName: "contestSync", ownerId: "instanceA:run1" }); + }); + + test("releaseLock does not throw when deleteOne fails", async () => { + mock.method(SyncLock, "deleteOne", async () => { + throw new Error("connection lost"); + }); + + await assert.doesNotReject(releaseLock("contestSync", "instanceA:run1")); + }); +}); diff --git a/server/package.json b/server/package.json index ae8a014..c7e26eb 100644 --- a/server/package.json +++ b/server/package.json @@ -6,7 +6,7 @@ "scripts": { "start": "node server.js", "dev": "nodemon server.js", - "test": "echo \"Error: no test specified\" && exit 1" + "test": "node --test" }, "keywords": [], "author": "",