Skip to content

Commit 9235ab4

Browse files
committed
fix(chat): prevent duplicate event streams from concurrent open
open()/close() did a non-atomic check-then-act on streamRef while being invoked from multiple triggers (login, lifecycle onStart, network reconnect, feature-flag, heartbeat) on the multi-threaded IO scope. Two threads could both observe a null ref and each open a gRPC stream, leaving a second orphaned stream whose reconnect loop kept running. The server rejects the duplicate with ABORTED 'stream already exists' and the two streams knock each other offline in a persistent duel. Serialize open/close under a lock so only one stream can exist, and guard onError to clear only its own ref. Adds a 32-thread concurrency test.
1 parent ecd7c25 commit 9235ab4

2 files changed

Lines changed: 65 additions & 13 deletions

File tree

services/flipcash/src/main/kotlin/com/flipcash/services/controllers/EventStreamingController.kt

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,20 @@ class EventStreamingController @Inject constructor(
2020
private val _chatUpdates = Channel<ChatUpdate>(capacity = Channel.UNLIMITED)
2121
val chatUpdates: Flow<ChatUpdate> = _chatUpdates.receiveAsFlow()
2222

23+
// Guards all reads/writes of [streamRef]. open()/close() are invoked
24+
// concurrently from multiple triggers (login, lifecycle onStart, network
25+
// reconnect, feature-flag, heartbeat) on the multi-threaded IO scope, so
26+
// the check-then-act on streamRef must be atomic — otherwise two threads
27+
// both observe a null ref and each open a stream, producing a second
28+
// orphaned gRPC stream and the server's "ABORTED: stream already exists".
29+
private val lock = Any()
2330
private var streamRef: EventStreamReference? = null
2431

25-
val isConnected: Boolean get() = streamRef != null
32+
val isConnected: Boolean get() = synchronized(lock) { streamRef != null }
2633

27-
val isStreamActive: Boolean get() = streamRef?.isActive == true
34+
val isStreamActive: Boolean get() = synchronized(lock) { streamRef?.isActive == true }
2835

29-
fun open(scope: CoroutineScope): Boolean {
36+
fun open(scope: CoroutineScope): Boolean = synchronized(lock) {
3037
if (streamRef != null) {
3138
trace("EventStreamingController: Stream already open, skipping")
3239
return true
@@ -37,7 +44,8 @@ class EventStreamingController @Inject constructor(
3744
return false
3845
}
3946

40-
streamRef = repository.openEventStream(
47+
lateinit var openedRef: EventStreamReference
48+
openedRef = repository.openEventStream(
4149
scope = scope,
4250
owner = owner,
4351
onEvent = { update ->
@@ -47,15 +55,18 @@ class EventStreamingController @Inject constructor(
4755
onError = { error ->
4856
trace("EventStreamingController: Stream error: ${error.message}")
4957
// Clear the ref so the next heartbeat, lifecycle, or network
50-
// event creates a fresh stream. The framework guarantees this
51-
// fires only once per stream, so no risk of clearing a newer ref.
52-
streamRef = null
58+
// event creates a fresh stream. Only clear if it is still THIS
59+
// stream — never null out a newer ref opened after us.
60+
synchronized(lock) {
61+
if (streamRef === openedRef) streamRef = null
62+
}
5363
},
5464
)
65+
streamRef = openedRef
5566
return true
5667
}
5768

58-
fun close() {
69+
fun close() = synchronized(lock) {
5970
streamRef?.destroy()
6071
streamRef = null
6172
}

services/flipcash/src/test/kotlin/com/flipcash/services/controllers/EventStreamingControllerTest.kt

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,15 @@ import io.mockk.every
1111
import io.mockk.mockk
1212
import io.mockk.verify
1313
import kotlinx.coroutines.CoroutineScope
14+
import kotlinx.coroutines.Dispatchers
1415
import kotlinx.coroutines.ExperimentalCoroutinesApi
1516
import kotlinx.coroutines.test.UnconfinedTestDispatcher
1617
import kotlinx.coroutines.test.runTest
18+
import java.util.concurrent.CountDownLatch
19+
import java.util.concurrent.TimeUnit
20+
import kotlin.concurrent.thread
1721
import kotlin.test.Test
22+
import kotlin.test.assertEquals
1823
import kotlin.test.assertNotNull
1924

2025
@OptIn(ExperimentalCoroutinesApi::class)
@@ -69,21 +74,57 @@ class EventStreamingControllerTest {
6974
fun `chatUpdates flow is accessible`() {
7075
assertNotNull(controller.chatUpdates)
7176
}
77+
78+
@Test
79+
fun `concurrent open calls create only one stream`() {
80+
stubOwner()
81+
val scope = CoroutineScope(Dispatchers.Default)
82+
val threadCount = 32
83+
val startLatch = CountDownLatch(1)
84+
val doneLatch = CountDownLatch(threadCount)
85+
86+
repeat(threadCount) {
87+
thread {
88+
startLatch.await()
89+
controller.open(scope)
90+
doneLatch.countDown()
91+
}
92+
}
93+
94+
// Release all threads at once to maximize the check-then-act race.
95+
startLatch.countDown()
96+
doneLatch.await(5, TimeUnit.SECONDS)
97+
98+
// Exactly one gRPC stream must be opened; opening two produces the
99+
// server-side "ABORTED: stream already exists" duel.
100+
assertEquals(1, repository.openCount)
101+
}
72102
}
73103

74104
private class FakeEventStreamingRepository : EventStreamingRepository {
75-
var opened = false
105+
private val lock = Any()
106+
var openCount = 0
107+
private set
108+
val opened: Boolean get() = openCount > 0
76109
var lastStreamRef: EventStreamReference? = null
110+
private set
77111

78112
override fun openEventStream(
79113
scope: CoroutineScope,
80114
owner: Ed25519.KeyPair,
81115
onEvent: (ChatUpdate) -> Unit,
82116
onError: (Throwable) -> Unit,
83117
): EventStreamReference {
84-
opened = true
85-
val ref = mockk<EventStreamReference>(relaxed = true)
86-
lastStreamRef = ref
87-
return ref
118+
// Widen the controller's check-then-act window so an unsynchronized
119+
// open() lets multiple threads reach here concurrently. Mock creation
120+
// and counting are serialized here so the assertion measures the
121+
// controller's concurrency, not mockk's.
122+
Thread.sleep(20)
123+
return synchronized(lock) {
124+
openCount++
125+
val ref = mockk<EventStreamReference>(relaxed = true)
126+
lastStreamRef = ref
127+
ref
128+
}
88129
}
89130
}

0 commit comments

Comments
 (0)