Skip to content

Commit 4c05f63

Browse files
committed
fix(mcp): throw instead of falling open when refresh lock wait exceeds deadline
When the Redis refresh lock can't be acquired within REFRESH_MAX_WAIT_MS the previous code ran fn() uncoordinated — but another process can still be holding the lock (watchdog-extended) and refreshing the same OAuth row, recreating the exact race the lock prevents. Throw on deadline. The caller can retry; the Redis-down branch remains the only path that runs fn() uncoordinated (no coordination is possible there).
1 parent 6d67cb7 commit 4c05f63

2 files changed

Lines changed: 27 additions & 2 deletions

File tree

apps/sim/lib/mcp/oauth/storage.test.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,25 @@ describe('withMcpOauthRefreshLock', () => {
202202
expect(keys).toContain('mcp:oauth:refresh:row-b')
203203
})
204204

205+
it('throws when the lock is held longer than the max wait (does not race)', async () => {
206+
vi.useFakeTimers()
207+
try {
208+
// Acquire always fails — another process holds the lock with watchdog extension.
209+
mockAcquireLock.mockResolvedValue(false)
210+
const fn = vi.fn(async () => 'should-not-run')
211+
212+
const pending = withMcpOauthRefreshLock('row-deadline', fn)
213+
// Attach the rejection expectation before draining so Vitest doesn't see
214+
// an unhandled rejection while timers advance.
215+
const assertion = expect(pending).rejects.toThrow(/held longer than/)
216+
await vi.advanceTimersByTimeAsync(31_000)
217+
await assertion
218+
expect(fn).not.toHaveBeenCalled()
219+
} finally {
220+
vi.useRealTimers()
221+
}
222+
})
223+
205224
it('extends the lock TTL while fn() is running so long refreshes do not lose the lock', async () => {
206225
vi.useFakeTimers()
207226
try {

apps/sim/lib/mcp/oauth/storage.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -308,8 +308,14 @@ async function runWithRedisMutex<T>(
308308
}
309309

310310
if (Date.now() >= deadline) {
311-
logger.warn('Refresh lock wait timed out, running uncoordinated', { rowId })
312-
return fn()
311+
// Lock still held by another process AND its watchdog is keeping it
312+
// alive — falling open would let us refresh concurrently and race the
313+
// rotating refresh token. Throw and let the caller decide whether to
314+
// retry; the Redis-down path remains the only branch that runs `fn()`
315+
// uncoordinated (no coordination available there).
316+
throw new Error(
317+
`MCP OAuth refresh lock for ${rowId} held longer than ${REFRESH_MAX_WAIT_MS}ms`
318+
)
313319
}
314320
await sleep(REFRESH_POLL_INTERVAL_MS)
315321
}

0 commit comments

Comments
 (0)