leaderAwareJob.start()/stop() is not reentrant — repeated cycles permanently and cumulatively monkey-patch the shared leaderElection instance

[prodbycorne]

Overview

makeLeaderAwareJob() in src/jobs/leaderAwareJob.js wraps a background job (priceRefresh, webhookRetryWorker, airdropExpiry) with leader-election coordination by monkey-patching methods directly onto the shared leaderElection instance every time start() is called — and never undoes that patching in stop(). Calling start() more than once across the lifetime of a given leaderElection object (e.g. a stop() followed by a later start(), which is exactly the lifecycle these wrapped jobs otherwise support and that tests like test/leaderElection.test.js plausibly exercise) causes the wrapping to stack, layer upon layer, with no way to return to the original, unwrapped functions.

function start() {
  manualStop = false;
  leadershipLostWhileRunning = false;

  const origIsLeader = leaderElection.isLeader;
  const origTryAcquire = leaderElection.tryAcquire;
  const origRenew = leaderElection.renew;

  let wasLeader = false;
  const checkLeader = () => { /* closes over wasLeader, calls onLeadershipAcquired/onLeadershipLost */ };

  const originalStartRenewLoop = leaderElection.startRenewLoop.bind(leaderElection);
  const originalStopRenewLoop = leaderElection.stopRenewLoop.bind(leaderElection);

  leaderElection.startRenewLoop = () => { originalStartRenewLoop(); ... };
  leaderElection.stopRenewLoop = async () => { await originalStopRenewLoop(); if (underlyingStarted) { job.stop(); underlyingStarted = false; } };

  const checkInterval = setInterval(() => { checkLeader(); }, Math.min(leaderElection.renewIntervalMs || 5000, 2000));
  if (typeof checkInterval.unref === 'function') checkInterval.unref();
  leaderElection._checkInterval = checkInterval;

  setTimeout(() => checkLeader(), 500);
  leaderElection.startRenewLoop();

  const superTryAcquire = leaderElection.tryAcquire;
  leaderElection.tryAcquire = async (...args) => { const result = await superTryAcquire(...args); checkLeader(); return result; };

  const superRenew = leaderElection.renew;
  leaderElection.renew = async (...args) => { const result = await superRenew(...args); checkLeader(); return result; };
}

Every call to start() reassigns leaderElection.startRenewLoop, leaderElection.stopRenewLoop, leaderElection.tryAcquire, and leaderElection.renew to new wrapper functions that close over originalStartRenewLoop/originalStopRenewLoop/superTryAcquire/superRenew captured from whatever leaderElection.* currently held at the time start() was called — which, on a second start() call, is already the previous call's wrapper, not the true original implementation from createLeaderElection(). stop() clears leaderElection._checkInterval and calls leaderElection.stopRenewLoop() (the wrapped version), but at no point does it restore leaderElection.tryAcquire/renew/startRenewLoop/stopRenewLoop to their pre-start() values.

Confirmed consequences of a start() → stop() → start() cycle on the same leaderElection instance:

1. Unbounded closure accumulation. Each start() adds one more layer of wrapping around tryAcquire/renew. After N start/stop cycles, a single tryAcquire() call invokes N nested wrapper functions, each independently calling its own checkLeader() closure (each holding its own now-stale wasLeader variable from whichever cycle created it). This is both a growing memory retention (N stale closures, each capturing leaderElection, job, jobName, logger by reference, never released) and a growing amount of redundant, duplicate work performed per lease operation, with no bound.
2. The shared leaderElection object is irreversibly and increasingly mutated. Anything else holding a reference to the same leaderElection instance (e.g. via getLeaderElection(), which this module explicitly exposes "useful for tests") and calling .tryAcquire()/.renew()/.startRenewLoop()/.stopRenewLoop() directly, bypassing the wrapped job's own start()/stop(), invokes whatever the current (Nth-layer-wrapped) version happens to be — behavior that silently depends on how many times the wrapped job has previously been started and stopped, not on the leaderElection module's own documented contract.
3. There is no unwrap()/restore() path anywhere in this module, so the only way to get back to pristine leaderElection behavior is to construct an entirely new instance via createLeaderElection(jobName) again.

Requirements

• Make start()/stop() reentrant and non-cumulative: either (a) wrap the leaderElection instance in a fresh, disposable adapter object at construction time (not by mutating the shared instance's own methods in place), so repeated start/stop cycles don't stack, or (b) explicitly save the true original method references once (at the point makeLeaderAwareJob() is called, not inside start()), and have stop() restore them, so a subsequent start() re-wraps from the same clean baseline every time.
• Ensure getLeaderElection() returns something whose behavior doesn't silently depend on how many times the wrapping job has been started/stopped previously.
• Add a test that explicitly performs start() → stop() → start() → stop() (at least twice) on the same wrapped job and asserts: (a) leadership-transition callbacks (onLeadershipAcquired/onLeadershipLost) fire exactly once per real transition, not once per accumulated wrapper layer; (b) leaderElection.tryAcquire/renew/startRenewLoop/stopRenewLoop are restored to functionally equivalent (or literally identical) behavior after stop(), verifiable by reference equality or by a call-count instrumentation check.

Acceptance Criteria

• A start() → stop() → start() → stop() cycle (repeated at least 3 times) does not cause the number of checkLeader() invocations per tryAcquire()/renew() call to grow — it stays constant regardless of how many prior start/stop cycles occurred.
• After stop(), leaderElection.tryAcquire/renew/startRenewLoop/stopRenewLoop behave identically to their state immediately after createLeaderElection() was first called (not merely "still work", but demonstrably not accumulating wrapper layers).
• getLeaderElection()'s returned object's behavior does not depend on the wrapped job's start/stop history.
• A new test in test/leaderElection.test.js (or a new file) explicitly covers the repeated start/stop cycle scenario described above.

Additional Notes

More precise references

• src/jobs/leaderAwareJob.js:71-148 (start()): confirmed the entire monkey-patch block runs unconditionally on every call, with no guard against start() having already been called once before on this same leaderElection instance.
• src/jobs/leaderAwareJob.js:94-113: confirmed originalStartRenewLoop/originalStopRenewLoop are captured via leaderElection.startRenewLoop.bind(leaderElection)/leaderElection.stopRenewLoop.bind(leaderElection) — i.e. whatever those properties currently hold at the moment this particular start() call executes, not a reference preserved from module/instance construction time.
• src/jobs/leaderAwareJob.js:135-147: confirmed superTryAcquire/superRenew are captured the same way, immediately before being overwritten — same "captures the previous wrapper, not the true original" issue.
• src/jobs/leaderAwareJob.js:153-169 (stop()): confirmed it clears leaderElection._checkInterval, calls await leaderElection.stopRenewLoop() (the wrapped version, which itself calls job.stop() if underlyingStarted), and stops the underlying job again if still marked started — but never reassigns leaderElection.tryAcquire/renew/startRenewLoop/stopRenewLoop back to anything.
• src/jobs/leaderAwareJob.js:190-192 (getLeaderElection): confirmed this returns the live, mutated leaderElection instance directly (return leaderElection;), not a snapshot or a defensive copy — exposing the accumulating-wrapper state to any caller.

Additional edge cases

• The doc comment at the top of the file (lines 3-27) describes a clean, one-shot wrappedJob.start() / wrappedJob.stop() usage pattern and doesn't mention (or seemingly anticipate) repeated start/stop cycles at all — worth deciding explicitly whether repeated start/stop is actually a supported use case for this module (if src/index.js genuinely only ever calls .start() once per process lifetime and .stop() once during shutdown, the practical blast radius today may be limited to test suites and any future hot-reload/config-reload feature) — but since the module explicitly exposes getLeaderElection() "useful for tests," and test suites very plausibly do start/stop cycles across multiple test cases sharing setup, this is a very plausible path to hit in practice, and worth confirming against test/leaderElection.test.js's actual structure.
• If job.start() throws synchronously inside onLeadershipAcquired() (called from the setInterval/setTimeout-driven checkLeader()), that exception is not caught anywhere in this file and would propagate out of a timer callback — see No process-level unhandledRejection/uncaughtException handlers — a single unexpected throw can crash the process #91 (open, "No process-level unhandledRejection/uncaughtException handlers") for the broader consequence; not re-litigated as its own issue here, but worth being aware of while touching this code.

Implementation sketch

Rather than mutating leaderElection's own properties, wrap it once, at makeLeaderAwareJob() construction time, into a private adapter that start()/stop() operate against:

function makeLeaderAwareJob({ job, jobName, leaderElection, logger }) {
  // Capture true originals exactly once, at construction, never touched again.
  const trueTryAcquire = leaderElection.tryAcquire.bind(leaderElection);
  const trueRenew = leaderElection.renew.bind(leaderElection);
  const trueStartRenewLoop = leaderElection.startRenewLoop.bind(leaderElection);
  const trueStopRenewLoop = leaderElection.stopRenewLoop.bind(leaderElection);
  // ... build checkLeader/onLeadershipAcquired/onLeadershipLost against these true originals,
  // and have start()/stop() install/uninstall a *single* well-defined wrapper layer each time,
  // rather than layering a new one on top of whatever's currently installed.
}

Test/reproduction plan

const leaderElection = createLeaderElection('test_job');
const wrapped = makeLeaderAwareJob({ job: fakeJob, jobName: 'test_job', leaderElection, logger });

wrapped.start(); await wrapped.stop();
const tryAcquireAfterFirstCycle = leaderElection.tryAcquire;
wrapped.start(); await wrapped.stop();
const tryAcquireAfterSecondCycle = leaderElection.tryAcquire;

// Instrument to count checkLeader-equivalent side effects per call, or assert
// tryAcquireAfterFirstCycle and tryAcquireAfterSecondCycle produce the same number
// of onLeadershipAcquired/onLeadershipLost invocations per underlying state transition —
// currently, the second cycle's call chain is one layer deeper than the first's.

Cross-references

• Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 (closed) — "Background jobs... have no leader election — every horizontally-scaled replica runs them independently." This leaderAwareJob.js module is the fix that closed Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98; this issue is a lifecycle bug in that fix itself, not a re-opening of Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98's original concern.