Skip to content

Commit 47091e6

Browse files
Copilotajinkyasraj
andcommitted
fix: prevent race condition in stale-timestamp recovery path
Co-authored-by: ajinkyasraj <145996984+ajinkyasraj@users.noreply.github.com>
1 parent 84b30c6 commit 47091e6

File tree

3 files changed

+66
-5
lines changed

3 files changed

+66
-5
lines changed

package-lock.json

Lines changed: 0 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/octez.connect-transport-matrix/__tests__/communication-client/P2PCommunicationClient.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,64 @@ describe('P2PCommunicationClient', () => {
265265
expect(secondResult.timestamp).toBe(2000)
266266
expect(mockStorage.delete).toHaveBeenCalledWith(StorageKey.MATRIX_SELECTED_NODE)
267267
})
268+
269+
it('handles race condition when multiple callers refresh stale timestamp concurrently', async () => {
270+
mockStorage.get.mockResolvedValue('')
271+
mockStorage.set.mockResolvedValue(undefined)
272+
mockStorage.delete.mockResolvedValue(undefined)
273+
274+
// First getRelayServer call: establish a cached server
275+
const axiosGetMock = axios.get as jest.Mock
276+
axiosGetMock.mockResolvedValue({
277+
data: { region: 'eu', known_servers: ['a'], timestamp: 1000 }
278+
})
279+
280+
await freshClient.getRelayServer()
281+
282+
// Force the localTimestamp to be old so the stale-timestamp refresh path triggers
283+
const relayServerPromise = (freshClient as any).relayServer
284+
if (relayServerPromise) {
285+
const resolved = await relayServerPromise.promise
286+
resolved.localTimestamp = 0
287+
}
288+
289+
// Both concurrent callers will fail their getBeaconInfo calls
290+
// The fix ensures the second caller doesn't orphan the first caller's new promise
291+
let firstCallReachedCatch = false
292+
let secondCallReachedCatch = false
293+
294+
axiosGetMock.mockReset()
295+
296+
// Create a controlled delay to ensure interleaving
297+
axiosGetMock.mockImplementation(() => {
298+
return new Promise((resolve, reject) => {
299+
setTimeout(() => {
300+
reject(new Error('ETIMEDOUT'))
301+
}, 10)
302+
})
303+
})
304+
305+
// Start two concurrent calls
306+
const call1 = freshClient.getRelayServer().catch(() => {
307+
firstCallReachedCatch = true
308+
})
309+
const call2 = freshClient.getRelayServer().catch(() => {
310+
secondCallReachedCatch = true
311+
})
312+
313+
// After both fail, set up successful discovery
314+
await Promise.allSettled([call1, call2])
315+
316+
axiosGetMock.mockResolvedValue({
317+
data: { region: 'us', known_servers: ['b'], timestamp: 3000 }
318+
})
319+
320+
// The third call should succeed and not hang
321+
const thirdResult = await freshClient.getRelayServer()
322+
323+
expect(thirdResult.timestamp).toBe(3000)
324+
expect(firstCallReachedCatch || secondCallReachedCatch).toBe(true)
325+
})
268326
})
269327

270328
describe('updatePeerRoom', () => {

packages/octez.connect-transport-matrix/src/communication-client/P2PCommunicationClient.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,9 @@ export class P2PCommunicationClient extends CommunicationClient {
225225
return { server: relayServer.server, timestamp: relayServer.timestamp }
226226
}
227227

228+
// Capture the current promise reference before async operations
229+
// to prevent race conditions where concurrent callers orphan promises
230+
const currentPromise = this.relayServer
228231
try {
229232
const info = await this.getBeaconInfo(relayServer.server)
230233
this.relayServer.resolve({
@@ -236,8 +239,11 @@ export class P2PCommunicationClient extends CommunicationClient {
236239
} catch (error) {
237240
logger.log('getRelayServer', `cached server ${relayServer.server} is unreachable, resetting`)
238241
await this.storage.delete(StorageKey.MATRIX_SELECTED_NODE).catch((e) => logger.log(e))
239-
this.relayServer = undefined
240-
this.selectedRegion = undefined
242+
// Only reset if this promise instance is still current (not replaced by another caller)
243+
if (this.relayServer === currentPromise) {
244+
this.relayServer = undefined
245+
this.selectedRegion = undefined
246+
}
241247
// Fall through to discovery below
242248
}
243249
}

0 commit comments

Comments
 (0)