fix(replication): recover leadership after a failed leader-lock extend - #4918
fix(replication): recover leadership after a failed leader-lock extend#4918gantipr-gs wants to merge 1 commit into
Conversation
Redlock never repairs a Lock whose extend failed. While the object's local expiration is still ahead every heartbeat tick just retries the extend; once it passes (a Redis outage longer than the TTL, a pod reschedule) every tick throws "Cannot extend an already-expired lock" locally, without asking Redis again — and kept doing so every interval until the process restarted (triggerdotdev#3428). In our self-hosted deployment, where the single Redis pod is rescheduled roughly daily by node rotation, that meant two error lines every 10s for hours and a paging alert per rotation. The Lock object's state says nothing certain about the key in Redis, so recover from what Redis actually holds, cheapest first: 1. Nobody holds the key: take it again, leadership continues. 2. The key is still ours (the extend landed but its reply was lost, or local and server expiry disagree): rebuild the Lock around the live key and extend it as usual. 3. Someone else holds it: drop the dead lock, emit leaderElection(false) once, tear the attempt down like every other failure path (a non-leader must not keep streaming), and hand recovery to the existing resubscribe path, which contends for the slot with backoff. 4. Redis is unreachable: while the key we last confirmed is inside its TTL nobody else can take it, so keep streaming and retry next tick. Only when that window closes without an answer do we step down as in 3. This is what lets a rotation-length Redis outage pass without interrupting replication. A re-entrancy guard keeps a slow Redis from stacking recoveries across ticks, and each tick hands the handler the Lock it tried to extend: a rejection that lands after a recovery or a teardown has already replaced or dropped that lock is ignored, so a straggling extend can never start a second recovery or announce a second step-down. If stop()/shutdown() lands while a recovery is still talking to Redis, the recovery releases whatever it re-acquired and exits quietly: stop() owns the teardown, so there is no lost leadership to announce and nothing to resubscribe. Tests cover the paths against real Postgres + Redis containers: a deleted key is re-acquired silently; denying the lock scripts via ACL for several ticks (key intact, Redis "unreachable") produces no election flip, no error, and the SAME lock value afterwards; a key held by someone else produces exactly one step-down and a later re-election; a client without resubscribe steps down and stops instead of streaming the slot lockless; an extend rejection that lands after re-election starts no recovery; a shutdown that lands mid-recovery leaves the key free with no election flip or error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
WalkthroughThe replication client records confirmed Redis lock expiration times and routes failed extensions through recovery logic. It can reacquire an unheld lock, reconstruct a lock still owned by the client, continue during a valid Redis outage, or relinquish leadership and resubscribe. Integration tests cover recovery, leadership loss, stale failures, and shutdown cleanup. A change note documents the behavior. Severity of issue fixed: Medium ✨ 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 |
|
Hi @gantipr-gs, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
1 participant
Closes #3428
✅ Checklist
Problem
LogicalReplicationClientrenews its Redis leader lock everyleaderLockExtendIntervalMs. Redlock never repairs aLockwhoseextendfailed: while the object's localexpirationis still ahead each heartbeat tick retries the extend, but once it passes (a Redis outage longer than the TTL, a pod reschedule)Redlock.extendthrowsCannot extend an already-expired lock.locally, without asking Redis again. The heartbeat catch only logged and emittederror, so nothing reset state: the process stayed "leader" in its own eyes, kept the slot, and loggedFailed to extend leader lock+Replication client errorevery interval until it was restarted. That is the loop described in #3428.We hit it on a self-hosted v4 deployment whose single Redis pod is rescheduled roughly daily by node rotation (3–47 s of downtime each time).
Fix
On extend failure,
#recoverLeaderLockrecovers from what Redis actually holds, cheapest first:warnline.Lockaround the live key and extend it as usual.error+leaderElection(false)exactly once, tear the attempt down with#cleanupAttempt()like every other failure path (a non-leader must not keep streaming), and hand off to the existing#scheduleResubscribepath, which contends for the slot with the normal acquire/backoff machinery.leaderLockHeldUntil) is inside its TTL nobody else can take it, so keep streaming and retry next tick. Only when that window closes without an answer do we step down as in 3. This lets a Redis restart of a few seconds pass without interrupting replication.Two guards keep it single-shot. A
leaderLockRecoveringflag stops a slow Redis from stacking recoveries across ticks. Each tick hands the handler theLockit tried to extend, and a rejection that lands after a recovery or teardown has already replaced or dropped that lock is ignored, so a straggling extend can never start a second recovery or announce a second step-down. Ifstop()/shutdown()lands while a recovery is still talking to Redis, the recovery releases whatever it re-acquired and exits quietly.Scope: one commit in
internal-packages/replication/src/client.tsplus tests. No API, env, or dependency changes. The lock TTL/extend interval stay configurable through the existingRUN_REPLICATION_LEADER_LOCK_*env vars.Cost / benefit
This touches leader election for Postgres → ClickHouse replication, so the failure modes worth weighing are: (a) two pods streaming the same slot, and (b) a pod streaming with no lock. Neither is newly possible. Case 1 and 2 only ever re-take or extend a key that is free or already carries our own value, and Redlock's scripts enforce that atomically. Case 3 tears the stream down before resubscribing. Case 4 keeps streaming only while our last confirmed TTL still excludes every other contender, which is the same guarantee the heartbeat relied on before. The pre-fix behavior, by contrast, kept the slot open indefinitely with no lock at all. The benefit is that a self-hosted deployment recovers from a Redis restart on its own instead of needing a pod restart.
Open question for maintainers
We kept recovery in-process rather than exiting the process (option (b) in #3428), because on a single-replica self-host an exit turns every Redis blip into a full replication outage, and here the leader was still the only candidate. If you would also like an exit strategy, it could sit behind an env flag next to the knobs proposed in #3613. Happy to add it in this PR or a follow-up, whichever you prefer.
Testing
All against real Postgres + Redis containers via
postgresAndRedisTestininternal-packages/replication/src/client.test.ts:a failed leader-lock extend re-acquires or steps down instead of looping foreverleaderElectionstays[true], no lock error events;ACL SETUSER default -eval -evalshafor several ticks with the key intact (Redis "unreachable") → no election flip, no error, and the same lock value once Redis answers again;leaderElection(false)and one lock error, then re-election once the other holder expires, with no further errors.a client without resubscribe steps down and stops instead of streaming without the lock→ oneleaderElection(false), one error,isStoppedtrue, zero pg backends for the client, nothing contends again.an extend rejection that lands after the lock was replaced starts no recovery→ parks the firstredlock.extendacross a step-down and re-election, then releases it: noredlock.acquirecall, no second announcement, no error.shutdown during leader-lock recovery releases the re-acquired lock and stays quiet→ gates the recovery'sredlock.acquire, callsshutdown()mid-recovery, opens the gate: key left free, no election flip, no error, no resubscribe.Removing either guard makes its test fail (checked by mutation). Full
client.test.ts(11 tests) passes locally on this branch rebased ontomain;pnpm run typecheckfor the package,oxfmt --check, andoxlintare clean.Changelog
.server-changes/replication-leader-lock-recovery.md(webapp, fix): runs replication no longer logs "Cannot extend an already-expired lock" forever after a Redis restart or outage; the leader re-acquires its lock or steps down once and re-elects.Screenshots
N/A (server-side only).
💯