A ring that ends, and only one thing reading the inbox
Two bugs found by calling a real emulator and watching what it did. An unanswered ring never ended. Dialling and connecting each had a timeout and ringing did not, so a call nobody picked up stayed RINGING until the app was killed. Nothing showed it, because the notification can be brushed off the top of the screen while the state behind it survives, and the phone then answered every later caller "busy" while looking completely idle. A guard now releases it after sixty seconds, deliberately longer than the caller's own forty-five, so this end gives up on its own only when their hang-up never arrived. It hangs up with "timeout", which the far end already renders as "No answer"; an invented word would have shown a flat "Call ended". And the handover was half-built. MainActivity starts the background service when the screen stops and stops it when the screen starts, under a comment reading "exactly one poller at a time". Only the service half existed. The view model's loop kept running beside it, both sessions read the same inbox, and an envelope is consumed once, so whichever asked first won it. When the view model won a call offer the phone rang inside a UI nobody was looking at, and the service, the only half that can post a notification, never saw the call. Seen on a device: the offer arrived, CallManager moved to RINGING, and the only notification posted was the service's own "running" notice. That is why calls did not announce themselves with the app off screen. The loop now stops on ON_STOP and starts again on ON_START, so the comment is finally true. Verified on the emulator against relay1: an unanswered offer at 12:51:07 released itself at 12:52:07 and told the caller, where before it would have refused every call after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
f4c52f18fe
commit
9ca58202a6
@@ -87,6 +87,45 @@ class CallManager(
|
||||
/** Gives up an unanswered outgoing call, like iOS's 45 s dial timeout. */
|
||||
private var dialGuard: Runnable? = null
|
||||
|
||||
/**
|
||||
* Stops an unanswered incoming call ringing forever.
|
||||
*
|
||||
* Every other state had a guard and this one did not, so a call nobody
|
||||
* picked up stayed RINGING until the app was killed. Nothing showed it: the
|
||||
* notification can be swiped off the top of the screen while the state
|
||||
* behind it survives. The next caller then got "busy" from a phone that
|
||||
* looked idle, and its owner had no way to find out why.
|
||||
*
|
||||
* The caller gives up after DIAL_TIMEOUT_MS. This end waits a little longer
|
||||
* than that, so a ring is released by its own timer only when the caller's
|
||||
* hang-up never arrived.
|
||||
*/
|
||||
private var ringGuard: Runnable? = null
|
||||
|
||||
private fun armRingGuard() {
|
||||
cancelRingGuard()
|
||||
val guard =
|
||||
Runnable {
|
||||
if (state == State.RINGING) {
|
||||
android.util.Log.i(
|
||||
"fnmsg-call",
|
||||
"ring expired with no answer, releasing so the next call is not refused",
|
||||
)
|
||||
// "timeout", not a new word: the far end already renders
|
||||
// that as "No answer", which is what this is. An unknown
|
||||
// reason falls through to a flat "Call ended".
|
||||
hangUp(notifyPeer = true, reason = "timeout")
|
||||
}
|
||||
}
|
||||
ringGuard = guard
|
||||
main.postDelayed(guard, RING_TIMEOUT_MS)
|
||||
}
|
||||
|
||||
private fun cancelRingGuard() {
|
||||
ringGuard?.let { main.removeCallbacks(it) }
|
||||
ringGuard = null
|
||||
}
|
||||
|
||||
/**
|
||||
* Gives up a call that was answered but whose media never connected.
|
||||
*/
|
||||
@@ -411,6 +450,7 @@ class CallManager(
|
||||
video = incoming.video,
|
||||
)
|
||||
state = State.RINGING
|
||||
armRingGuard()
|
||||
}
|
||||
|
||||
Content.CallSignal.ANSWER -> {
|
||||
@@ -420,6 +460,7 @@ class CallManager(
|
||||
if (incoming.callId != current?.callId) return
|
||||
dialGuard?.let { main.removeCallbacks(it) }
|
||||
dialGuard = null
|
||||
cancelRingGuard()
|
||||
state = State.CONNECTING
|
||||
armConnectGuard()
|
||||
connection?.setRemoteDescription(
|
||||
@@ -599,6 +640,12 @@ class CallManager(
|
||||
companion object {
|
||||
/** How long an unanswered outgoing call rings before giving up. */
|
||||
const val DIAL_TIMEOUT_MS = 45_000L
|
||||
|
||||
/**
|
||||
* A little past the caller's own patience, so this end releases a ring
|
||||
* only when their hang-up never reached us.
|
||||
*/
|
||||
const val RING_TIMEOUT_MS = 60_000L
|
||||
const val CONNECT_TIMEOUT_MS = 30_000L
|
||||
|
||||
/**
|
||||
|
||||
@@ -168,6 +168,25 @@ class MainActivity : androidx.fragment.app.FragmentActivity() {
|
||||
@Composable
|
||||
private fun Root(model: MessengerViewModel = viewModel()) {
|
||||
val phase by model.phase.collectAsStateWithLifecycle()
|
||||
|
||||
// The other half of the handover above. The service is started when this
|
||||
// screen stops; this stops the loop that would otherwise keep reading the
|
||||
// same inbox beside it. An envelope is consumed once, so two pollers means
|
||||
// one of them loses, and the one that loses is sometimes the only one that
|
||||
// could have rung the phone.
|
||||
val owner = androidx.lifecycle.compose.LocalLifecycleOwner.current
|
||||
DisposableEffect(owner) {
|
||||
val observer =
|
||||
androidx.lifecycle.LifecycleEventObserver { _, event ->
|
||||
when (event) {
|
||||
androidx.lifecycle.Lifecycle.Event.ON_STOP -> model.pausePolling()
|
||||
androidx.lifecycle.Lifecycle.Event.ON_START -> model.resumePolling()
|
||||
else -> Unit
|
||||
}
|
||||
}
|
||||
owner.lifecycle.addObserver(observer)
|
||||
onDispose { owner.lifecycle.removeObserver(observer) }
|
||||
}
|
||||
Scaffold { padding ->
|
||||
val frame = Modifier.fillMaxSize().padding(padding).padding(20.dp)
|
||||
when (val current = phase) {
|
||||
|
||||
@@ -3415,6 +3415,31 @@ class MessengerViewModel(
|
||||
/**
|
||||
* Poll for mail while signed in.
|
||||
*/
|
||||
/**
|
||||
* Stop reading the inbox while the app is off screen.
|
||||
*
|
||||
* `MainActivity` starts the background service on the way out and stops it
|
||||
* on the way back, under a comment reading "exactly one poller at a time".
|
||||
* Only half of that was ever true: the service started, and this loop kept
|
||||
* running beside it. Two sessions then polled the same inbox, and an
|
||||
* envelope is consumed once, so whichever asked first won it.
|
||||
*
|
||||
* When this loop won a call offer the phone rang inside a UI nobody was
|
||||
* looking at, and the service, which is the only half that can post a
|
||||
* notification, never saw the call at all. Observed on a device: an offer
|
||||
* arrived, `CallManager` moved to RINGING, and the only notification posted
|
||||
* was the service's own "running" notice.
|
||||
*/
|
||||
fun pausePolling() {
|
||||
polling?.cancel()
|
||||
polling = null
|
||||
}
|
||||
|
||||
/** Read the inbox again, once the app is back on screen. */
|
||||
fun resumePolling() {
|
||||
if (polling == null && session != null) startPolling()
|
||||
}
|
||||
|
||||
private fun startPolling() {
|
||||
polling?.cancel()
|
||||
// So an incoming call rings, see installCallSignalHandler.
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
package io.flamenet.messenger.app
|
||||
|
||||
import java.io.File
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
/**
|
||||
* Exactly one thing reads the inbox at a time.
|
||||
*
|
||||
* `MainActivity` starts the background service when the screen stops and stops
|
||||
* it when the screen starts, under a comment saying "exactly one poller at a
|
||||
* time". Only half of that was ever true: the service started, and the view
|
||||
* model's loop kept running beside it.
|
||||
*
|
||||
* An envelope is consumed once. With two sessions polling the same inbox
|
||||
* whichever asked first won, and when the view model won a call offer the phone
|
||||
* rang inside a UI nobody was looking at. The service is the only half that can
|
||||
* post a notification, and it never saw the call. Observed on a device: the
|
||||
* offer arrived, `CallManager` went to RINGING, and the only notification
|
||||
* posted was the service's own "running" notice.
|
||||
*/
|
||||
class OnePollerAtATimeTest {
|
||||
private fun source(path: String): String {
|
||||
val file = File("src/main/kotlin/io/flamenet/messenger/app/$path")
|
||||
assertTrue("missing $path", file.exists())
|
||||
return file.readText()
|
||||
}
|
||||
|
||||
/** The service half: started on the way out, stopped on the way back. */
|
||||
@Test
|
||||
fun theServiceHalfIsWired() {
|
||||
val main = source("MainActivity.kt")
|
||||
assertTrue(main.contains("MessengerService.stop(this)"))
|
||||
assertTrue(main.contains("MessengerService.start(this)"))
|
||||
}
|
||||
|
||||
/** The half that was missing. */
|
||||
@Test
|
||||
fun theViewModelHalfIsWired() {
|
||||
val vm = source("MessengerViewModel.kt")
|
||||
assertTrue("nothing could stop the loop", vm.contains("fun pausePolling()"))
|
||||
assertTrue("and nothing could start it again", vm.contains("fun resumePolling()"))
|
||||
assertTrue(
|
||||
"pausing must actually cancel the job, not just drop the reference",
|
||||
vm.contains("polling?.cancel()"),
|
||||
)
|
||||
}
|
||||
|
||||
/** Wired to the lifecycle, not merely defined. */
|
||||
@Test
|
||||
fun theViewModelHalfIsDrivenByTheLifecycle() {
|
||||
val main = source("MainActivity.kt")
|
||||
assertTrue(
|
||||
"ON_STOP must stop the loop",
|
||||
main.contains("Lifecycle.Event.ON_STOP -> model.pausePolling()"),
|
||||
)
|
||||
assertTrue(
|
||||
"ON_START must start it again, or the app comes back deaf",
|
||||
main.contains("Lifecycle.Event.ON_START -> model.resumePolling()"),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Resuming twice must not leave two loops running, which would recreate the
|
||||
* race inside a single process.
|
||||
*/
|
||||
@Test
|
||||
fun resumingTwiceDoesNotStartTwoLoops() {
|
||||
val vm = source("MessengerViewModel.kt")
|
||||
val resume = vm.substringAfter("fun resumePolling()").substringBefore("}")
|
||||
assertTrue(
|
||||
"resume must check the loop is not already running",
|
||||
resume.contains("polling == null"),
|
||||
)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user