From a5f5feacaeb22e00c5360297af8318b796762ee0 Mon Sep 17 00:00:00 2001 From: darken Date: Mon, 9 Mar 2026 05:15:04 +0100 Subject: [PATCH] fix(debug): Fix deadlock risk, missing IO dispatcher, and thread safety in session manager Replace sessions.first() inside fsMutex.withLock with synchronous volatile read to avoid potential deadlock. Wrap file deletions in IO dispatcher. Log partial delete failures. Make pendingAutoZips thread-safe via Collections.synchronizedSet. Add test for ext/cache same-basename IDs. --- .../recording/core/DebugSessionManager.kt | 61 ++++++++++--------- .../debug/recording/core/RecorderModule.kt | 8 +++ .../recording/core/DebugSessionManagerTest.kt | 17 ++++++ 3 files changed, 57 insertions(+), 29 deletions(-) diff --git a/app/src/main/java/eu/darken/capod/common/debug/recording/core/DebugSessionManager.kt b/app/src/main/java/eu/darken/capod/common/debug/recording/core/DebugSessionManager.kt index cdeb18df..b1699211 100644 --- a/app/src/main/java/eu/darken/capod/common/debug/recording/core/DebugSessionManager.kt +++ b/app/src/main/java/eu/darken/capod/common/debug/recording/core/DebugSessionManager.kt @@ -15,7 +15,6 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.combine -import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.flow.update import kotlinx.coroutines.CancellationException @@ -43,7 +42,7 @@ class DebugSessionManager @Inject constructor( private val zippingIds = MutableStateFlow>(emptySet()) private val failedZipIds = MutableStateFlow>(emptySet()) private val refreshTrigger = MutableSharedFlow(extraBufferCapacity = 1) - private val pendingAutoZips = mutableSetOf() + private val pendingAutoZips: MutableSet = java.util.Collections.synchronizedSet(mutableSetOf()) val recorderState: Flow get() = recorderModule.state @@ -160,10 +159,11 @@ class DebugSessionManager @Inject constructor( refreshTrigger.tryEmit(Unit) } + private fun activeSessionId(): String? = recorderModule.currentLogDir?.let { deriveSessionId(it) } + suspend fun zipSession(sessionId: String): File = fsMutex.withLock { - val activeRecording = sessions.first().filterIsInstance() - .firstOrNull { it.id == sessionId } - require(activeRecording == null) { "Cannot zip an active recording session" } + // Do NOT call sessions.first() here — deadlock risk with fsMutex + require(activeSessionId() != sessionId) { "Cannot zip an active recording session" } val (dir, existingZip) = findSessionFiles(sessionId) @@ -185,15 +185,19 @@ class DebugSessionManager @Inject constructor( } suspend fun deleteSession(sessionId: String) = fsMutex.withLock { - val currentSessions = sessions.first() - val recording = currentSessions.filterIsInstance() - .firstOrNull { it.id == sessionId } - require(recording == null) { "Cannot delete an active recording session" } + // Do NOT call sessions.first() here — deadlock risk with fsMutex + require(activeSessionId() != sessionId) { "Cannot delete an active recording session" } require(sessionId !in zippingIds.value) { "Cannot delete a session that is being compressed" } - val (dir, zip) = findSessionFiles(sessionId) - dir?.deleteRecursively() - zip?.delete() + withContext(dispatcherProvider.IO) { + val (dir, zip) = findSessionFiles(sessionId) + if (dir?.deleteRecursively() == false) { + log(TAG, WARN) { "Failed to fully delete session dir: ${dir.path}" } + } + if (zip?.delete() == false) { + log(TAG, WARN) { "Failed to delete session zip: ${zip.path}" } + } + } failedZipIds.update { it - sessionId } log(TAG) { "Deleted session: $sessionId" } @@ -201,24 +205,23 @@ class DebugSessionManager @Inject constructor( } suspend fun deleteAllSessions() = fsMutex.withLock { - val activeDir = recorderModule.state.first().currentLogDir + val activeDir = recorderModule.currentLogDir val currentlyZipping = zippingIds.value - for (dir in recorderModule.getLogDirectories()) { - if (!dir.exists()) continue - for (entry in dir.listFiles() ?: emptyArray()) { - if (entry == activeDir) { - log(TAG) { "Skipping active session dir: $entry" } - continue - } - val entryId = deriveSessionId(entry) - if (entryId in currentlyZipping) { - log(TAG) { "Skipping zipping session: $entry" } - continue - } - if (entry.isDirectory) { - entry.deleteRecursively() - } else { - entry.delete() + withContext(dispatcherProvider.IO) { + for (dir in recorderModule.getLogDirectories()) { + if (!dir.exists()) continue + for (entry in dir.listFiles() ?: emptyArray()) { + if (entry == activeDir) { + log(TAG) { "Skipping active session dir: $entry" } + continue + } + val entryId = deriveSessionId(entry) + if (entryId in currentlyZipping) { + log(TAG) { "Skipping zipping session: $entry" } + continue + } + val deleted = if (entry.isDirectory) entry.deleteRecursively() else entry.delete() + if (!deleted) log(TAG, WARN) { "Failed to delete: ${entry.path}" } } } } diff --git a/app/src/main/java/eu/darken/capod/common/debug/recording/core/RecorderModule.kt b/app/src/main/java/eu/darken/capod/common/debug/recording/core/RecorderModule.kt index a37f8e2d..d806d460 100644 --- a/app/src/main/java/eu/darken/capod/common/debug/recording/core/RecorderModule.kt +++ b/app/src/main/java/eu/darken/capod/common/debug/recording/core/RecorderModule.kt @@ -33,6 +33,10 @@ class RecorderModule @Inject constructor( private val installId: InstallId, ) { + @Volatile + internal var currentLogDir: File? = null + private set + private val triggerFile = try { File(context.getExternalFilesDir(null), FORCE_FILE) } catch (e: Exception) { @@ -64,6 +68,8 @@ class RecorderModule @Inject constructor( log(TAG, INFO) { "Build.Fingerprint: ${Build.FINGERPRINT}" } log(TAG, INFO) { "BuildConfig.Versions: ${BuildConfigWrap.VERSION_DESCRIPTION}" } + this@RecorderModule.currentLogDir = sessionDir + copy( recorder = newRecorder, currentLogDir = sessionDir, @@ -76,6 +82,8 @@ class RecorderModule @Inject constructor( log(TAG, ERROR) { "Failed to delete trigger file" } } + this@RecorderModule.currentLogDir = null + copy( recorder = null, currentLogDir = null, diff --git a/app/src/test/java/eu/darken/capod/common/debug/recording/core/DebugSessionManagerTest.kt b/app/src/test/java/eu/darken/capod/common/debug/recording/core/DebugSessionManagerTest.kt index 205e6f72..498b9e90 100644 --- a/app/src/test/java/eu/darken/capod/common/debug/recording/core/DebugSessionManagerTest.kt +++ b/app/src/test/java/eu/darken/capod/common/debug/recording/core/DebugSessionManagerTest.kt @@ -199,6 +199,23 @@ class DebugSessionManagerTest : BaseTest() { // Verify the session IS a Recording — the manager would reject zip/delete calls for this } + @Test + fun `ext and cache sessions with same basename have different IDs`() { + val cacheLogsDir = File(tempDir, "cache/debug/logs").also { it.mkdirs() } + val extDir = File(externalLogsDir, "capod_1.0_1700000000000_abcd1234").also { it.mkdirs() } + File(extDir, "core.log").writeText("external log") + val cacheDir = File(cacheLogsDir, "capod_1.0_1700000000000_abcd1234").also { it.mkdirs() } + File(cacheDir, "core.log").writeText("cache log") + + val sessions = DebugSessionManager.scanSessions( + logDirectories = listOf(externalLogsDir, cacheLogsDir), + ) + + sessions shouldHaveSize 2 + val ids = sessions.map { it.id }.toSet() + ids shouldBe setOf("ext:capod_1.0_1700000000000_abcd1234", "cache:capod_1.0_1700000000000_abcd1234") + } + @Test fun `zipping session should not be deleted`() { val sessionDir = File(externalLogsDir, "capod_1.0_1700000000000_abcd1234").also { it.mkdirs() }