diff --git a/app/src/main/java/com/bitchat/android/mesh/MeshConnectionTracker.kt b/app/src/main/java/com/bitchat/android/mesh/MeshConnectionTracker.kt index 0dd6a552e..e2d52765c 100644 --- a/app/src/main/java/com/bitchat/android/mesh/MeshConnectionTracker.kt +++ b/app/src/main/java/com/bitchat/android/mesh/MeshConnectionTracker.kt @@ -3,6 +3,7 @@ package com.bitchat.android.mesh import android.util.Log import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.launch import java.util.concurrent.ConcurrentHashMap @@ -43,6 +44,7 @@ abstract class MeshConnectionTracker( protected val pendingConnections = ConcurrentHashMap() private var isActive = false + private var cleanupJob: Job? = null /** * Start the tracker and its cleanup loop @@ -119,7 +121,9 @@ abstract class MeshConnectionTracker( abstract fun getConnectionCount(): Int private fun startPeriodicCleanup() { - scope.launch { + // A stop/start can complete while the previous loop is in delay(); one loop must remain. + cleanupJob?.cancel() + cleanupJob = scope.launch { while (isActive) { try { delay(CLEANUP_INTERVAL) diff --git a/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareController.kt b/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareController.kt index d5a694f15..f59fb2061 100644 --- a/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareController.kt +++ b/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareController.kt @@ -14,6 +14,7 @@ import kotlinx.coroutines.launch import androidx.annotation.VisibleForTesting import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicLong /** * WifiAwareController manages lifecycle and debug surfacing for the WifiAwareMeshService. @@ -30,6 +31,9 @@ object WifiAwareController { private var starting = false private val restartInFlight = AtomicBoolean(false) private val restartRequested = AtomicBoolean(false) + // Monotonic. A deadline rather than a delay, so a request coalesced into a running loop + // still holds that loop back; the latest deadline requested wins. + private val restartNotBeforeMs = AtomicLong(Long.MIN_VALUE) private var awareReceiverRegistered = false private var lastBlockedReason: String? = null @@ -249,13 +253,22 @@ object WifiAwareController { // publishes anyway — resurrecting NAN while the hotspot owns the radio. // Ordering holds because acquireHotspotLease() increments before calling // stop(): either we see the flag here, or stop() sees our published service. + // + // stop() withdraws an instance by clearing `service`. A reused instance it took + // during this attempt is being torn down and must not be published again. + var retainedForRetry = false val published = synchronized(lifecycleLock) { - val canPublish = !heldForHotspot() && startedService.isRunning() + val withdrawn = reusableService != null && service !== reusableService + val canPublish = !heldForHotspot() && !withdrawn && startedService.isRunning() if (canPublish) { service = startedService _running.value = true } else { - if (service === startedService) service = null + if (heldForHotspot() && service === startedService) service = null + // A reused instance can defer its start while recovery cleanup finishes. + // Keep it for the next attempt: it was never shut down, so replacing it + // would orphan it next to a second instance. + retainedForRetry = service === startedService _running.value = false } canPublish @@ -268,7 +281,7 @@ object WifiAwareController { } else { // stopServices() can block, so keep it out of the lock. val heldForHotspot = heldForHotspot() - if (heldForHotspot || reusableService == null) { + if (!retainedForRetry) { try { startedService.stopServices() } catch (_: Exception) { } } if (heldForHotspot) { @@ -310,6 +323,21 @@ object WifiAwareController { } } + /** + * Records that the published service lost its sessions but was not shut down. + * + * Unlike [onServiceStopped] this keeps the reference: the instance still owns its scope + * and core, and [startIfPossible] resumes it. Clearing [running] is what lets + * [restartIfStillEnabled] act. + */ + internal fun onServiceInterrupted(interruptedService: WifiAwareMeshService) { + synchronized(lifecycleLock) { + if (service !== interruptedService) return + _running.value = false + clearDebugSnapshot() + } + } + /** * Schedules a restart of the Wi-Fi Aware transport. Concurrent requests are coalesced into * a single in-flight loop that retries with backoff. This is important because a single fixed @@ -322,6 +350,9 @@ object WifiAwareController { // burning attempts against the hold — would otherwise be dropped, and the old // loop would exhaust MAX_RESTART_ATTEMPTS without ever seeing the cleared hold, // leaving Aware down despite the user's setting. + restartNotBeforeMs.accumulateAndGet(monotonicNowMs() + delayMs) { current, requested -> + maxOf(current, requested) + } restartRequested.set(true) if (!restartInFlight.compareAndSet(false, true)) { @@ -330,13 +361,16 @@ object WifiAwareController { } scope.launch { try { - if (delayMs > 0L) delay(delayMs) + awaitRestartDeadline() do { // Consume the request before working, so anything arriving during // the attempts below schedules another pass. restartRequested.set(false) var attempt = 0 while (_enabled.value && !_running.value && attempt < MAX_RESTART_ATTEMPTS) { + // A failure answered during the previous attempt may have moved this. + awaitRestartDeadline() + if (!_enabled.value || _running.value) break val ctx = appContext if (ctx != null && !refreshSupportStatus(ctx).supported) break startIfPossible() @@ -358,6 +392,20 @@ object WifiAwareController { } } + private fun monotonicNowMs(): Long = System.nanoTime() / 1_000_000L + + /** Re-reads the deadline after each wait, since a coalesced request can extend it. */ + private suspend fun awaitRestartDeadline() { + while (true) { + val remainingMs = restartNotBeforeMs.get().let { deadline -> + val now = monotonicNowMs() + if (deadline <= now) 0L else deadline - now + } + if (remainingMs == 0L) return + delay(remainingMs) + } + } + /** * Listens for system Wi-Fi Aware availability changes. Aware can flip off/on at runtime * (Wi-Fi toggling, hotspot/SoftAP, location changes); without this we would only recover on diff --git a/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareMeshService.kt b/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareMeshService.kt index 93aabb17b..1dd8244dc 100644 --- a/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareMeshService.kt +++ b/app/src/main/java/com/bitchat/android/wifi-aware/WifiAwareMeshService.kt @@ -77,6 +77,19 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor private const val CLIENT_SOCKET_ATTEMPTS = 3 private const val CLIENT_ROLE_REVERSAL_FAILURES = 3 private const val ROLE_REVERSAL_PREFIX = "ROLE_SERVER:" + private const val RECOVERY_RESTART_DELAY_MS = 500L + private const val ATTACH_RETRY_BASE_MS = 3_000L + private const val ATTACH_RETRY_MAX_MS = 60_000L + + /** + * Delay before re-attaching after [consecutiveFailures] failed attaches in a row. + * + * A start counts as running until its attach is answered, so the controller's attempt + * limit never trips on repeated attach failures. This keeps them off the radio instead. + */ + internal fun attachRetryDelayMs(consecutiveFailures: Int): Long = + (ATTACH_RETRY_BASE_MS shl (consecutiveFailures - 1).coerceIn(0, 5)) + .coerceAtMost(ATTACH_RETRY_MAX_MS) } // Core crypto/services @@ -105,6 +118,7 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor @Volatile private var isActive = false @Volatile private var recoveryInProgress = false private val sessionGeneration = AtomicInteger(0) + private val consecutiveAttachFailures = AtomicInteger(0) // Delegate override var delegate: WifiAwareMeshDelegate? = null @@ -363,6 +377,7 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor return } wifiAwareSession = session + consecutiveAttachFailures.set(0) Log.i(TAG, "Wi-Fi Aware attached; starting publish & subscribe") // PUBLISH (server role) @@ -493,9 +508,10 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor override fun onAttachFailed() { if (!isCurrentSession(generation)) return Log.e(TAG, "Wi-Fi Aware attach failed") - handleUnexpectedStop(generation) + val retryDelayMs = attachRetryDelayMs(consecutiveAttachFailures.incrementAndGet()) + handleUnexpectedStop(generation, retryDelayMs) if (com.bitchat.android.wifiaware.WifiAwareController.enabled.value) { - com.bitchat.android.wifiaware.WifiAwareController.restartIfStillEnabled(3000) + com.bitchat.android.wifiaware.WifiAwareController.restartIfStillEnabled(retryDelayMs) } } @@ -516,7 +532,7 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor meshCore.startCore() com.bitchat.android.service.MeshServiceHolder.startSharedGossip("WIFI") - startPeriodicConnectionMaintenance() + startPeriodicConnectionMaintenance(generation) connectionTracker.start() } @@ -569,13 +585,16 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor return generation == sessionGeneration.get() && isActive } - private fun handleUnexpectedStop(generation: Int) { + private fun handleUnexpectedStop(generation: Int, restartDelayMs: Long = RECOVERY_RESTART_DELAY_MS) { if (generation != sessionGeneration.get()) return if (!isActive) { return } recoveryInProgress = true isActive = false + // Must follow isActive = false so a concurrent publish in the controller is either + // refused or overwritten here. Must precede the restart requests made by callers. + com.bitchat.android.wifiaware.WifiAwareController.onServiceInterrupted(this) TransportBridgeService.unregister("WIFI") com.bitchat.android.service.MeshServiceHolder.stopSharedGossip("WIFI") try { com.bitchat.android.services.AppStateStore.clearTransportPeers("WIFI") } catch (_: Exception) { } @@ -590,7 +609,8 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor try { oldPublishSession?.close() } catch (_: Exception) { } try { oldSubscribeSession?.close() } catch (_: Exception) { } try { oldWifiAwareSession?.close() } catch (_: Exception) { } - if (generation == sessionGeneration.get() && !isActive) { + // Qualified: a bare isActive here is the coroutine's, which is always true. + if (generation == sessionGeneration.get() && !this@WifiAwareMeshService.isActive) { if (publishSession === oldPublishSession) publishSession = null if (subscribeSession === oldSubscribeSession) subscribeSession = null if (wifiAwareSession === oldWifiAwareSession) wifiAwareSession = null @@ -605,7 +625,7 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor // Recovery cleanup is done; nudge a restart now that startServices() will no // longer be deferred by recoveryInProgress. The controller coalesces requests. if (com.bitchat.android.wifiaware.WifiAwareController.enabled.value) { - com.bitchat.android.wifiaware.WifiAwareController.restartIfStillEnabled(500) + com.bitchat.android.wifiaware.WifiAwareController.restartIfStillEnabled(restartDelayMs) } } } @@ -645,13 +665,15 @@ class WifiAwareMeshService(private val context: Context) : MeshService, Transpor /** * Periodic active maintenance: retries connections to discovered but unconnected peers. */ - private fun startPeriodicConnectionMaintenance() { + private fun startPeriodicConnectionMaintenance(generation: Int) { serviceScope.launch { - while (isActive) { + // Scoped to the session: a restart can finish while this loop is in delay(), and + // an isActive check alone would leave it running next to the new session's loop. + while (isCurrentSession(generation)) { try { val schedule = powerManager.profile.value.wifiAware delay(schedule.connectionMaintenanceMs) - if (!isActive) break + if (!isCurrentSession(generation)) break val now = System.currentTimeMillis() diff --git a/app/src/test/kotlin/com/bitchat/android/wifiaware/WifiAwareUnexpectedStopRestartTest.kt b/app/src/test/kotlin/com/bitchat/android/wifiaware/WifiAwareUnexpectedStopRestartTest.kt new file mode 100644 index 000000000..68afe9fcc --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/wifiaware/WifiAwareUnexpectedStopRestartTest.kt @@ -0,0 +1,219 @@ +package com.bitchat.android.wifiaware + +import android.app.Application +import android.content.Context +import android.content.ContextWrapper +import android.content.pm.PackageManager +import android.location.LocationManager +import android.net.wifi.aware.AttachCallback +import android.net.wifi.aware.WifiAwareManager +import android.os.Handler +import androidx.test.core.app.ApplicationProvider +import com.bitchat.android.crypto.EncryptionService +import com.bitchat.android.mesh.BluetoothMeshService +import com.bitchat.android.mesh.FragmentingPacketSender +import com.bitchat.android.mesh.MeshCore +import com.bitchat.android.service.MeshServiceHolder +import com.bitchat.android.ui.NotificationManager +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.ArgumentCaptor +import org.mockito.MockedConstruction +import org.mockito.Mockito +import org.mockito.kotlin.any +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config + +/** + * The radio can end an Aware session at any time, and the service also ends its own + * sessions to refresh discovery. Each of those stops has to come back on its own: the + * user's setting says Aware is on, and no Activity resume may be coming. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33]) +class WifiAwareUnexpectedStopRestartTest { + + private val awareManager: WifiAwareManager = mock() + private lateinit var context: Context + private val constructionStubs = mutableListOf>() + + @Before + fun setUp() { + // Robolectric has no AndroidKeyStore, and these collaborators are not under test. + constructionStubs += Mockito.mockConstruction(EncryptionService::class.java) { mock, _ -> + whenever(mock.getIdentityFingerprint()).thenReturn(SYNTHETIC_FINGERPRINT) + } + constructionStubs += Mockito.mockConstruction(BluetoothMeshService::class.java) + constructionStubs += Mockito.mockConstruction(NotificationManager::class.java) + constructionStubs += Mockito.mockConstruction(MeshCore::class.java) + constructionStubs += Mockito.mockConstruction(FragmentingPacketSender::class.java) + val app = ApplicationProvider.getApplicationContext() + shadowOf(app.packageManager).setSystemFeature(PackageManager.FEATURE_WIFI_AWARE, true) + shadowOf(app.getSystemService(LocationManager::class.java)).setLocationEnabled(true) + shadowOf(app).grantPermissions(android.Manifest.permission.NEARBY_WIFI_DEVICES) + whenever(awareManager.isAvailable).thenReturn(true) + context = object : ContextWrapper(app) { + override fun getApplicationContext(): Context = this + override fun getSystemService(name: String): Any? = + if (name == Context.WIFI_AWARE_SERVICE) awareManager else super.getSystemService(name) + } + WifiAwareController.resetHotspotLeasesForTest() + } + + @After + fun tearDown() { + WifiAwareController.setEnabled(false) + WifiAwareController.resetHotspotLeasesForTest() + MeshServiceHolder.clear() + constructionStubs.forEach { it.close() } + } + + @Test + fun `an unexpected session end re-attaches the same service without an outside start`() { + WifiAwareController.initialize(context, enabledByDefault = true) + val service = WifiAwareController.getService()!! + assertTrue(WifiAwareController.running.value) + + val callback = ArgumentCaptor.forClass(AttachCallback::class.java) + Mockito.verify(awareManager).attach(callback.capture(), any()) + + callback.value.onAwareSessionTerminated() + + assertFalse( + "the controller must not report a stopped transport as running", + WifiAwareController.running.value + ) + runBlocking { + withTimeout(RESTART_TIMEOUT_MS) { WifiAwareController.running.first { it } } + } + Mockito.verify(awareManager, Mockito.times(2)).attach(any(), any()) + assertSame( + "recovery reuses the instance rather than orphaning it", + service, + WifiAwareController.getService() + ) + } + + @Test + fun `a restart deferred by recovery cleanup keeps the instance for the next attempt`() { + withStubbedService { constructed, _ -> + val service = constructed.constructed().single() + WifiAwareController.onServiceInterrupted(service) + + // The stub stays not-running through startServices(), as a deferred start does. + WifiAwareController.startIfPossible() + + assertFalse(WifiAwareController.running.value) + assertSame(service, WifiAwareController.getService()) + assertEquals(1, constructed.constructed().size) + verify(service, never()).stopServices() + } + } + + @Test + fun `a stop racing a reused start is not undone by the start`() { + withStubbedService { constructed, isRunning -> + val service = constructed.constructed().single() + WifiAwareController.onServiceInterrupted(service) + whenever(service.startServices()).thenAnswer { + WifiAwareController.stop() + isRunning.set(true) + } + + WifiAwareController.startIfPossible() + + assertFalse(WifiAwareController.running.value) + assertNull(WifiAwareController.getService()) + } + } + + @Test + fun `an interruption from a replaced instance leaves the current one running`() { + withStubbedService(initiallyRunning = true) { constructed, _ -> + val current = constructed.constructed().single() + + WifiAwareController.onServiceInterrupted(mock()) + + assertTrue(WifiAwareController.running.value) + assertSame(current, WifiAwareController.getService()) + } + } + + @Test + fun `a cooldown requested during a restart attempt delays the next attempt`() { + withStubbedService { constructed, isRunning -> + val service = constructed.constructed().single() + WifiAwareController.onServiceInterrupted(service) + val attemptTimesNs = CopyOnWriteArrayList() + whenever(service.startServices()).thenAnswer { + attemptTimesNs += System.nanoTime() + if (attemptTimesNs.size == 1) { + // An attach failure answered before the attempt finishes publishing. + WifiAwareController.restartIfStillEnabled(COALESCED_COOLDOWN_MS) + } else { + // Ends the loop so it cannot run into the next test. + isRunning.set(true) + } + } + + WifiAwareController.restartIfStillEnabled() + runBlocking { + withTimeout(RESTART_TIMEOUT_MS) { WifiAwareController.running.first { it } } + } + + val gapMs = TimeUnit.NANOSECONDS.toMillis(attemptTimesNs[1] - attemptTimesNs[0]) + assertTrue( + "next attempt came after ${gapMs}ms, inside the ${COALESCED_COOLDOWN_MS}ms cooldown", + gapMs >= COALESCED_COOLDOWN_MS + ) + } + } + + @Test + fun `repeated attach failures back off to a capped retry interval`() { + val delays = (1..8).map(WifiAwareMeshService::attachRetryDelayMs) + + assertEquals(listOf(3_000L, 6_000L, 12_000L, 24_000L, 48_000L, 60_000L, 60_000L, 60_000L), delays) + } + + private fun withStubbedService( + initiallyRunning: Boolean = false, + block: (MockedConstruction, AtomicBoolean) -> Unit + ) { + // Starts out running so the controller publishes it, as a healthy first start does. + val isRunning = AtomicBoolean(true) + Mockito.mockConstruction(WifiAwareMeshService::class.java) { mock, _ -> + whenever(mock.isRunning()).thenAnswer { isRunning.get() } + }.use { constructed -> + WifiAwareController.initialize(context, enabledByDefault = true) + isRunning.set(initiallyRunning) + block(constructed, isRunning) + } + } + + private companion object { + // Longer than the service's restart delay plus one controller retry. + const val RESTART_TIMEOUT_MS = 20_000L + // Longer than the controller's fixed retry delay, so ignoring it is detectable. + const val COALESCED_COOLDOWN_MS = 4_500L + const val SYNTHETIC_FINGERPRINT = "00112233445566778899aabbccddeeff" + } +}