Skip to content

Commit 8ec065f

Browse files
authored
fix(table): reclaim a cascade lock a timed-out acquire may have taken (#7680)
A client-side timeout does not mean Redis declined the SET. The command can still be parked in the offline queue and take the lock once the connection completes, leaving the row's cascade held for the full 30s TTL by an owner that already threw — no heartbeat, no release. Every other cell task for that row then reads `contended` and bails on the silent path, so one stalled connection quietly drops later cells rather than just failing the one run. Releasing after a failed acquire is what the Redlock algorithm prescribes: a client that fails to acquire unlocks the instances anyway, including ones it believed it had not locked. Both preconditions the option documents hold here — `ownerId` is the cell task's unique `executionId`, and a throw means `fn` never runs, so the reclaim cannot cut under a caller still doing work. Adds the cascade lock's first tests, covering acquire, contention, reclaim, release on throw, and heartbeat teardown.
1 parent 33f7eef commit 8ec065f

2 files changed

Lines changed: 104 additions & 1 deletion

File tree

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { redisConfigMockFns } from '@sim/testing'
5+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { cascadeLockKey, withCascadeLock } from '@/lib/table/cascade-lock'
7+
8+
const TABLE_ID = 'tbl_1'
9+
const ROW_ID = 'row_1'
10+
const OWNER_ID = 'exec-1'
11+
12+
describe('withCascadeLock', () => {
13+
beforeEach(() => {
14+
vi.clearAllMocks()
15+
redisConfigMockFns.mockAcquireLock.mockResolvedValue(true)
16+
redisConfigMockFns.mockReleaseLock.mockResolvedValue(true)
17+
redisConfigMockFns.mockExtendLock.mockResolvedValue(true)
18+
})
19+
20+
afterEach(() => {
21+
vi.useRealTimers()
22+
})
23+
24+
it('runs the work and releases under the owner that took the lock', async () => {
25+
const fn = vi.fn().mockResolvedValue('done')
26+
27+
await expect(withCascadeLock(TABLE_ID, ROW_ID, OWNER_ID, fn)).resolves.toEqual({
28+
status: 'acquired',
29+
result: 'done',
30+
})
31+
expect(fn).toHaveBeenCalledOnce()
32+
expect(redisConfigMockFns.mockReleaseLock).toHaveBeenCalledWith(
33+
cascadeLockKey(TABLE_ID, ROW_ID),
34+
OWNER_ID
35+
)
36+
})
37+
38+
it('skips the work when another task holds the row', async () => {
39+
redisConfigMockFns.mockAcquireLock.mockResolvedValue(false)
40+
const fn = vi.fn()
41+
42+
await expect(withCascadeLock(TABLE_ID, ROW_ID, OWNER_ID, fn)).resolves.toEqual({
43+
status: 'contended',
44+
})
45+
expect(fn).not.toHaveBeenCalled()
46+
// Nothing was taken, so nothing may be deleted — the holder still owns it.
47+
expect(redisConfigMockFns.mockReleaseLock).not.toHaveBeenCalled()
48+
})
49+
50+
it('reclaims a lock a timed-out acquire may have taken', async () => {
51+
// A client-side timeout does not mean Redis declined the SET: the command
52+
// can still land and hold the row for the full TTL under an owner that
53+
// already threw, silently starving every later cell task for that row.
54+
redisConfigMockFns.mockAcquireLock.mockRejectedValue(new Error('Command timed out'))
55+
const fn = vi.fn()
56+
57+
await expect(withCascadeLock(TABLE_ID, ROW_ID, OWNER_ID, fn)).rejects.toThrow(
58+
'Command timed out'
59+
)
60+
expect(fn).not.toHaveBeenCalled()
61+
expect(redisConfigMockFns.mockAcquireLock).toHaveBeenCalledWith(
62+
cascadeLockKey(TABLE_ID, ROW_ID),
63+
OWNER_ID,
64+
expect.any(Number),
65+
{ reclaimOnFailure: true }
66+
)
67+
})
68+
69+
it('releases the lock when the work throws', async () => {
70+
const fn = vi.fn().mockRejectedValue(new Error('boom'))
71+
72+
await expect(withCascadeLock(TABLE_ID, ROW_ID, OWNER_ID, fn)).rejects.toThrow('boom')
73+
expect(redisConfigMockFns.mockReleaseLock).toHaveBeenCalledWith(
74+
cascadeLockKey(TABLE_ID, ROW_ID),
75+
OWNER_ID
76+
)
77+
})
78+
79+
it('stops the heartbeat once the work settles', async () => {
80+
vi.useFakeTimers()
81+
const fn = vi.fn().mockResolvedValue(undefined)
82+
83+
await withCascadeLock(TABLE_ID, ROW_ID, OWNER_ID, fn)
84+
await vi.advanceTimersByTimeAsync(60_000)
85+
86+
// A heartbeat outliving the work would keep extending a lock nobody holds.
87+
expect(redisConfigMockFns.mockExtendLock).not.toHaveBeenCalled()
88+
})
89+
})

apps/sim/lib/table/cascade-lock.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,21 @@ export async function withCascadeLock<T>(
4040
fn: () => Promise<T>
4141
): Promise<{ status: 'acquired'; result: T } | { status: 'contended' }> {
4242
const key = cascadeLockKey(tableId, rowId)
43-
const acquired = await acquireLock(key, ownerId, LOCK_TTL_SECONDS)
43+
const acquired = await acquireLock(key, ownerId, LOCK_TTL_SECONDS, {
44+
/**
45+
* A client-side timeout does not mean Redis declined the SET — the command
46+
* can still be sitting in the offline queue and take the lock once the
47+
* connection completes, leaving the row's cascade held for the full TTL by
48+
* an owner that already threw, with no heartbeat and no release. Every other
49+
* cell task for that row then reads `contended` and bails on the silent
50+
* path, so one stalled connection quietly drops later cells too.
51+
*
52+
* Both preconditions hold here: `ownerId` is the cell task's `executionId`,
53+
* unique to this holder, and a throw means `fn` never runs, so freeing a
54+
* lock this call may have taken cannot cut under a caller still working.
55+
*/
56+
reclaimOnFailure: true,
57+
})
4458
if (!acquired) return { status: 'contended' }
4559

4660
const heartbeat = setInterval(() => {

0 commit comments

Comments
 (0)