Skip to content

Commit 8220479

Browse files
gantipr-gsclaude
andcommitted
fix(replication): recover leadership after a failed leader-lock extend
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 (#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>
1 parent 478422f commit 8220479

3 files changed

Lines changed: 515 additions & 13 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
area: webapp
3+
type: fix
4+
---
5+
6+
Runs replication no longer logs "Cannot extend an already-expired lock" forever after a Redis restart or outage. The leader now re-acquires its lock or steps down once and re-elects, so replication to ClickHouse resumes on its own instead of waiting for a webapp restart.

internal-packages/replication/src/client.test.ts

Lines changed: 360 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { Redis } from "@internal/redis";
12
import { postgresAndRedisTest } from "@internal/testcontainers";
23
import { LogicalReplicationClient } from "./client.js";
34
import { setTimeout } from "timers/promises";
@@ -465,4 +466,363 @@ describe("Replication Client", () => {
465466
await b.shutdown();
466467
}
467468
);
469+
470+
postgresAndRedisTest(
471+
"a failed leader-lock extend re-acquires or steps down instead of looping forever",
472+
async ({ postgresContainer, prisma, redisOptions }) => {
473+
await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`);
474+
475+
const slotName = "lock_recovery_slot";
476+
// Same key prefix the client derives, so raw reads and writes here hit
477+
// the very key Redlock is holding.
478+
const lockKey = `logical-replication-client:${slotName}`;
479+
const redis = new Redis({
480+
...redisOptions,
481+
keyPrefix: `${redisOptions.keyPrefix}logical-replication-client:`,
482+
});
483+
484+
const client = new LogicalReplicationClient({
485+
name: "lock-recovery",
486+
slotName,
487+
publicationName: "lock_recovery_pub",
488+
redisOptions,
489+
table: "TaskRun",
490+
pgConfig: { connectionString: postgresContainer.getConnectionUri() },
491+
resubscribeOnFailure: true,
492+
resubscribeMinDelayMs: 200,
493+
resubscribeMaxDelayMs: 400,
494+
leaderLockTimeoutMs: 2000,
495+
leaderLockExtendIntervalMs: 300,
496+
leaderLockAcquireAdditionalTimeMs: 200,
497+
leaderLockRetryIntervalMs: 100,
498+
});
499+
const elections: boolean[] = [];
500+
const lockErrors: string[] = [];
501+
client.events.on("leaderElection", (won) => elections.push(won));
502+
client.events.on("error", (error) => {
503+
// Only the lock-related failures matter here; the client may surface
504+
// unrelated connection noise on stop/resubscribe.
505+
const message = String((error as Error)?.message ?? error);
506+
if (/already-expired|unable to achieve a quorum|extend/i.test(message)) {
507+
lockErrors.push(message);
508+
}
509+
});
510+
511+
try {
512+
await client.subscribe();
513+
expect(elections).toEqual([true]);
514+
expect(await redis.exists(lockKey)).toBe(1);
515+
516+
// 1. The lock disappears underneath us (Redis restarted, key expired) and
517+
// nobody else wants it. The next extend fails; the client must take the
518+
// lock again and keep streaming — no election flip, no error spam.
519+
await redis.del(lockKey);
520+
await setTimeout(1200); // several heartbeat ticks
521+
expect(await redis.exists(lockKey)).toBe(1);
522+
expect(elections).toEqual([true]);
523+
expect(lockErrors).toHaveLength(0);
524+
expect(client.isStopped).toBe(false);
525+
526+
// 2. Redis stops answering while the key is still ours (a network blip,
527+
// a restart with persistence). Deny the lock scripts so every extend
528+
// and re-acquire fails, but leave the key in place. The client must
529+
// keep streaming under the still-valid TTL, then reclaim the SAME
530+
// lock once Redis answers again — no election flip, no error, no
531+
// stream restart. (ACL rule changes apply to connected clients.)
532+
const valueBefore = await redis.get(lockKey);
533+
await redis.call("ACL", "SETUSER", "default", "-eval", "-evalsha");
534+
try {
535+
await setTimeout(700); // 2-3 failed heartbeat ticks, well inside the 2s TTL
536+
expect(elections).toEqual([true]);
537+
expect(lockErrors).toHaveLength(0);
538+
expect(client.isStopped).toBe(false);
539+
} finally {
540+
await redis.call("ACL", "SETUSER", "default", "+eval", "+evalsha");
541+
}
542+
await setTimeout(900);
543+
expect(await redis.get(lockKey)).toBe(valueBefore);
544+
expect(elections).toEqual([true]);
545+
expect(lockErrors).toHaveLength(0);
546+
expect(client.isStopped).toBe(false);
547+
548+
// 3. Someone else holds the lock when our extend fails. We must step down
549+
// exactly once, then win it back when they let go — the pre-fix client
550+
// logged "Cannot extend an already-expired lock" every tick forever.
551+
await redis.set(lockKey, "someone-else", "PX", 1500);
552+
await setTimeout(1200);
553+
expect(elections).toContain(false);
554+
// Exactly one surfaced failure for the step-down, not one per tick.
555+
expect(lockErrors).toHaveLength(1);
556+
557+
let regained = false;
558+
for (let i = 0; i < 40; i++) {
559+
if (elections.filter((won) => won).length >= 2) {
560+
regained = true;
561+
break;
562+
}
563+
await setTimeout(250);
564+
}
565+
expect(regained).toBe(true);
566+
// Regaining leadership must not have kept emitting extend failures.
567+
expect(lockErrors).toHaveLength(1);
568+
expect(await redis.exists(lockKey)).toBe(1);
569+
} finally {
570+
await client.shutdown();
571+
await redis.quit();
572+
}
573+
}
574+
);
575+
576+
postgresAndRedisTest(
577+
"a client without resubscribe steps down and stops instead of streaming without the lock",
578+
async ({ postgresContainer, prisma, redisOptions }) => {
579+
await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`);
580+
581+
const slotName = "lock_recovery_noresub_slot";
582+
const lockKey = `logical-replication-client:${slotName}`;
583+
const redis = new Redis({
584+
...redisOptions,
585+
keyPrefix: `${redisOptions.keyPrefix}logical-replication-client:`,
586+
});
587+
588+
const client = new LogicalReplicationClient({
589+
name: "lock-recovery-noresub",
590+
slotName,
591+
publicationName: "lock_recovery_noresub_pub",
592+
redisOptions,
593+
table: "TaskRun",
594+
pgConfig: { connectionString: postgresContainer.getConnectionUri() },
595+
leaderLockTimeoutMs: 2000,
596+
leaderLockExtendIntervalMs: 300,
597+
leaderLockAcquireAdditionalTimeMs: 200,
598+
leaderLockRetryIntervalMs: 100,
599+
});
600+
const elections: boolean[] = [];
601+
const lockErrors: string[] = [];
602+
client.events.on("leaderElection", (won) => elections.push(won));
603+
client.events.on("error", (error) => {
604+
const message = String((error as Error)?.message ?? error);
605+
if (/already-expired|unable to achieve a quorum|extend/i.test(message)) {
606+
lockErrors.push(message);
607+
}
608+
});
609+
610+
try {
611+
await client.subscribe();
612+
expect(elections).toEqual([true]);
613+
614+
// Someone else takes the key. With nothing to resubscribe, the pre-fix
615+
// client announced the loss and then kept streaming the slot, lockless,
616+
// until the process died.
617+
await redis.set(lockKey, "someone-else", "PX", 1500);
618+
for (let i = 0; i < 30 && !elections.includes(false); i++) {
619+
await setTimeout(100);
620+
}
621+
await setTimeout(500); // let the pg client finish ending
622+
expect(elections).toEqual([true, false]);
623+
expect(lockErrors).toHaveLength(1);
624+
expect(client.isStopped).toBe(true);
625+
const backends = await prisma.$queryRaw<{ count: bigint }[]>`
626+
SELECT count(*) AS count FROM pg_stat_activity WHERE application_name = 'lock-recovery-noresub'
627+
`;
628+
expect(Number(backends[0].count)).toBe(0);
629+
630+
// The other holder is long gone; nothing on this client contends again.
631+
await setTimeout(2000);
632+
expect(elections).toEqual([true, false]);
633+
expect(lockErrors).toHaveLength(1);
634+
expect(client.isStopped).toBe(true);
635+
expect(await redis.exists(lockKey)).toBe(0);
636+
} finally {
637+
await client.shutdown();
638+
await redis.quit();
639+
}
640+
}
641+
);
642+
643+
postgresAndRedisTest(
644+
"an extend rejection that lands after the lock was replaced starts no recovery",
645+
async ({ postgresContainer, prisma, redisOptions }) => {
646+
await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`);
647+
648+
const slotName = "lock_recovery_stale_slot";
649+
const lockKey = `logical-replication-client:${slotName}`;
650+
const redis = new Redis({
651+
...redisOptions,
652+
keyPrefix: `${redisOptions.keyPrefix}logical-replication-client:`,
653+
});
654+
655+
const client = new LogicalReplicationClient({
656+
name: "lock-recovery-stale",
657+
slotName,
658+
publicationName: "lock_recovery_stale_pub",
659+
redisOptions,
660+
table: "TaskRun",
661+
pgConfig: { connectionString: postgresContainer.getConnectionUri() },
662+
resubscribeOnFailure: true,
663+
resubscribeMinDelayMs: 200,
664+
resubscribeMaxDelayMs: 400,
665+
leaderLockTimeoutMs: 2000,
666+
leaderLockExtendIntervalMs: 300,
667+
leaderLockAcquireAdditionalTimeMs: 200,
668+
leaderLockRetryIntervalMs: 100,
669+
});
670+
const elections: boolean[] = [];
671+
const lockErrors: string[] = [];
672+
client.events.on("leaderElection", (won) => elections.push(won));
673+
client.events.on("error", (error) => {
674+
const message = String((error as Error)?.message ?? error);
675+
if (/already-expired|unable to achieve a quorum|extend/i.test(message)) {
676+
lockErrors.push(message);
677+
}
678+
});
679+
680+
const redlock = client["redlock"];
681+
const realExtend = redlock.extend.bind(redlock);
682+
let openGate: () => void = () => {};
683+
const gate = new Promise<void>((resolve) => (openGate = resolve));
684+
let extendSpy: ReturnType<typeof vi.spyOn> | undefined;
685+
let acquireSpy: ReturnType<typeof vi.spyOn> | undefined;
686+
687+
try {
688+
await client.subscribe();
689+
expect(elections).toEqual([true]);
690+
691+
// Park the first extend after the takeover below; every later one runs
692+
// for real. This is the tick a slow Redis leaves in flight while the next
693+
// tick already fails, recovers, and moves on.
694+
let parked = false;
695+
extendSpy = vi
696+
.spyOn(redlock, "extend")
697+
.mockImplementation(async (...args: Parameters<typeof realExtend>) => {
698+
if (!parked) {
699+
parked = true;
700+
await gate;
701+
}
702+
return realExtend(...args);
703+
});
704+
acquireSpy = vi.spyOn(redlock, "acquire");
705+
706+
// Someone else takes the key: the next real extend fails, the client steps
707+
// down once, and wins the slot back once the other holder expires.
708+
await redis.set(lockKey, "someone-else", "PX", 1500);
709+
let regained = false;
710+
for (let i = 0; i < 60; i++) {
711+
if (elections.filter((won) => won).length >= 2) {
712+
regained = true;
713+
break;
714+
}
715+
await setTimeout(100);
716+
}
717+
expect(regained).toBe(true);
718+
expect(elections.filter((won) => !won)).toHaveLength(1);
719+
expect(lockErrors).toHaveLength(1);
720+
// Re-election fires before the new stream is up; wait until it is, so the
721+
// stale rejection below meets a running client, not one still subscribing.
722+
for (let i = 0; i < 40 && client.isStopped; i++) {
723+
await setTimeout(100);
724+
}
725+
expect(client.isStopped).toBe(false);
726+
727+
// Now the parked extend rejects. Its lock was dropped at the step-down and
728+
// replaced at re-election, so it must not start a recovery: no acquire, no
729+
// second announcement, no error.
730+
const acquiresBefore = acquireSpy.mock.calls.length;
731+
openGate();
732+
await setTimeout(600);
733+
expect(acquireSpy.mock.calls.length).toBe(acquiresBefore);
734+
expect(elections.filter((won) => !won)).toHaveLength(1);
735+
expect(lockErrors).toHaveLength(1);
736+
expect(client.isStopped).toBe(false);
737+
expect(await redis.exists(lockKey)).toBe(1);
738+
} finally {
739+
openGate();
740+
extendSpy?.mockRestore();
741+
acquireSpy?.mockRestore();
742+
await client.shutdown();
743+
await redis.quit();
744+
}
745+
}
746+
);
747+
748+
postgresAndRedisTest(
749+
"shutdown during leader-lock recovery releases the re-acquired lock and stays quiet",
750+
async ({ postgresContainer, prisma, redisOptions }) => {
751+
await prisma.$executeRawUnsafe(`ALTER TABLE public."TaskRun" REPLICA IDENTITY FULL;`);
752+
753+
const slotName = "lock_recovery_shutdown_slot";
754+
const lockKey = `logical-replication-client:${slotName}`;
755+
const redis = new Redis({
756+
...redisOptions,
757+
keyPrefix: `${redisOptions.keyPrefix}logical-replication-client:`,
758+
});
759+
760+
const client = new LogicalReplicationClient({
761+
name: "lock-recovery-shutdown",
762+
slotName,
763+
publicationName: "lock_recovery_shutdown_pub",
764+
redisOptions,
765+
table: "TaskRun",
766+
pgConfig: { connectionString: postgresContainer.getConnectionUri() },
767+
resubscribeOnFailure: true,
768+
resubscribeMinDelayMs: 200,
769+
resubscribeMaxDelayMs: 400,
770+
leaderLockTimeoutMs: 2000,
771+
leaderLockExtendIntervalMs: 300,
772+
leaderLockAcquireAdditionalTimeMs: 200,
773+
leaderLockRetryIntervalMs: 100,
774+
});
775+
const elections: boolean[] = [];
776+
const lockErrors: string[] = [];
777+
client.events.on("leaderElection", (won) => elections.push(won));
778+
client.events.on("error", (error) => {
779+
const message = String((error as Error)?.message ?? error);
780+
if (/already-expired|unable to achieve a quorum|extend/i.test(message)) {
781+
lockErrors.push(message);
782+
}
783+
});
784+
785+
const redlock = client["redlock"];
786+
const realAcquire = redlock.acquire.bind(redlock);
787+
let openGate: () => void = () => {};
788+
const gate = new Promise<void>((resolve) => (openGate = resolve));
789+
let acquireSpy: ReturnType<typeof vi.spyOn> | undefined;
790+
791+
try {
792+
await client.subscribe();
793+
expect(elections).toEqual([true]);
794+
795+
// Hold the recovery's re-acquire open so shutdown() can land while it is
796+
// in flight — the window a Redis that answers slowly opens in production.
797+
acquireSpy = vi
798+
.spyOn(redlock, "acquire")
799+
.mockImplementation(async (...args: Parameters<typeof realAcquire>) => {
800+
await gate;
801+
return realAcquire(...args);
802+
});
803+
804+
// The key vanishes → the next extend fails → recovery calls acquire and parks on the gate.
805+
await redis.del(lockKey);
806+
for (let i = 0; i < 30 && acquireSpy.mock.calls.length === 0; i++) {
807+
await setTimeout(100);
808+
}
809+
expect(acquireSpy.mock.calls.length).toBeGreaterThan(0);
810+
811+
await client.shutdown();
812+
openGate();
813+
await setTimeout(600); // > resubscribeMaxDelayMs: a wrongly scheduled resubscribe would have fired
814+
815+
// stop() owns the teardown: the recovered lock is released, and there is
816+
// no lost-leadership announcement, error, or resubscribe after shutdown.
817+
expect(await redis.exists(lockKey)).toBe(0);
818+
expect(elections).toEqual([true]);
819+
expect(lockErrors).toHaveLength(0);
820+
expect(client.isStopped).toBe(true);
821+
} finally {
822+
acquireSpy?.mockRestore();
823+
await client.shutdown();
824+
await redis.quit();
825+
}
826+
}
827+
);
468828
});

0 commit comments

Comments
 (0)