From 6f076a79d3356afadbeb9f7c2b794354a00ed0b7 Mon Sep 17 00:00:00 2001 From: M57 <41645758+Hy4ri@users.noreply.github.com> Date: Thu, 17 Sep 2026 00:33:57 +0300 Subject: [PATCH] fix(kanban): preserve transfer cleanup and screen lifecycle (#1137) --- .../ui/kanban/KanbanLifecycleTest.kt | 81 +++++++++++++ .../ui/kanban/KanbanExportState.kt | 10 ++ .../ui/kanban/KanbanImportCleanup.kt | 26 +++++ .../ui/kanban/KanbanRuntimeState.kt | 26 ++++- .../hermescontrol/ui/kanban/KanbanScreen.kt | 80 ++++++++----- .../ui/kanban/KanbanImportCleanupTest.kt | 108 ++++++++++++++++++ 6 files changed, 295 insertions(+), 36 deletions(-) create mode 100644 app/src/androidTest/java/com/m57/hermescontrol/ui/kanban/KanbanLifecycleTest.kt create mode 100644 app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanExportState.kt create mode 100644 app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanImportCleanup.kt create mode 100644 app/src/test/java/com/m57/hermescontrol/ui/kanban/KanbanImportCleanupTest.kt diff --git a/app/src/androidTest/java/com/m57/hermescontrol/ui/kanban/KanbanLifecycleTest.kt b/app/src/androidTest/java/com/m57/hermescontrol/ui/kanban/KanbanLifecycleTest.kt new file mode 100644 index 00000000..c17eeba3 --- /dev/null +++ b/app/src/androidTest/java/com/m57/hermescontrol/ui/kanban/KanbanLifecycleTest.kt @@ -0,0 +1,81 @@ +package com.m57.hermescontrol.ui.kanban + +import androidx.compose.material3.Text +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.runtime.MutableState +import androidx.compose.runtime.mutableStateOf +import androidx.compose.ui.test.assertTextEquals +import androidx.compose.ui.test.junit4.StateRestorationTester +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onNodeWithText +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.LifecycleRegistry +import androidx.lifecycle.compose.LocalLifecycleOwner +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith + +@RunWith(AndroidJUnit4::class) +class KanbanLifecycleTest { + @get:Rule + val compose = createComposeRule() + + @Test + fun pendingExportSurvivesSavedStateRestoration() { + val restoration = StateRestorationTester(compose) + lateinit var path: MutableState + restoration.setContent { + path = rememberPendingKanbanExportPath() + } + compose.runOnIdle { path.value = "/cache/board.tar.gz" } + restoration.emulateSavedInstanceStateRestore() + compose.runOnIdle { assertEquals("/cache/board.tar.gz", path.value) } + } + + @Test + fun tickerStopsInBackgroundRefreshesOnResumeAndStopsOnDisposal() { + lateinit var registry: LifecycleRegistry + val owner = + object : LifecycleOwner { + override val lifecycle: Lifecycle get() = registry + } + compose.runOnUiThread { + registry = LifecycleRegistry(owner) + registry.currentState = Lifecycle.State.STARTED + } + var now = 100L + val visible = mutableStateOf(true) + var displayed = 0L + compose.mainClock.autoAdvance = false + compose.setContent { + CompositionLocalProvider(LocalLifecycleOwner provides owner) { + if (visible.value) { + displayed = rememberKanbanNowSeconds(true) { now } + Text("$displayed") + } + } + } + compose.mainClock.advanceTimeBy(32) + compose.onNodeWithText("100").assertTextEquals("100") + compose.runOnUiThread { now = 105L } + compose.mainClock.advanceTimeBy(5_100) + compose.onNodeWithText("105").assertTextEquals("105") + compose.runOnUiThread { + registry.currentState = Lifecycle.State.CREATED + now = 200L + } + compose.mainClock.advanceTimeBy(10_100) + compose.onNodeWithText("105").assertTextEquals("105") + compose.runOnUiThread { registry.currentState = Lifecycle.State.STARTED } + compose.mainClock.advanceTimeBy(64) + compose.onNodeWithText("200").assertTextEquals("200") + compose.runOnUiThread { visible.value = false } + compose.mainClock.advanceTimeBy(64) + compose.runOnUiThread { now = 300L } + compose.mainClock.advanceTimeBy(10_100) + compose.runOnIdle { assertEquals(200L, displayed) } + } +} diff --git a/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanExportState.kt b/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanExportState.kt new file mode 100644 index 00000000..aab46350 --- /dev/null +++ b/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanExportState.kt @@ -0,0 +1,10 @@ +package com.m57.hermescontrol.ui.kanban + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.MutableState +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.saveable.rememberSaveable + +/** Keep the cache path while the external document picker recreates the Activity (#1137). */ +@Composable +internal fun rememberPendingKanbanExportPath(): MutableState = rememberSaveable { mutableStateOf(null) } diff --git a/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanImportCleanup.kt b/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanImportCleanup.kt new file mode 100644 index 00000000..865f6488 --- /dev/null +++ b/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanImportCleanup.kt @@ -0,0 +1,26 @@ +package com.m57.hermescontrol.ui.kanban + +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeout + +/** Own only confirmed uploads; cancellation must not strand their temporary archive (#1137). */ +internal suspend fun withKanbanImportCleanup( + cleanup: suspend () -> Unit, + onCleanupFailure: (Exception) -> Unit, + dispatcher: CoroutineDispatcher = Dispatchers.IO, + operation: suspend () -> T, +): T = + try { + operation() + } finally { + withContext(NonCancellable + dispatcher) { + try { + withTimeout(10_000L) { cleanup() } + } catch (e: Exception) { + onCleanupFailure(e) + } + } + } diff --git a/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanRuntimeState.kt b/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanRuntimeState.kt index d9acd1f3..e47c5a19 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanRuntimeState.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanRuntimeState.kt @@ -2,8 +2,12 @@ package com.m57.hermescontrol.ui.kanban import androidx.compose.runtime.Composable import androidx.compose.runtime.produceState +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.compose.LocalLifecycleOwner +import androidx.lifecycle.repeatOnLifecycle import com.m57.hermescontrol.data.model.KanbanTask import kotlinx.coroutines.delay +import kotlinx.coroutines.isActive enum class KanbanArcState { QUEUED, @@ -54,13 +58,23 @@ object KanbanRuntimeHelper { /** One coarse clock per board screen, rather than one coroutine per task card. */ @Composable -fun rememberKanbanNowSeconds(enabled: Boolean): Long = - produceState( - initialValue = System.currentTimeMillis() / 1000L, +fun rememberKanbanNowSeconds( + enabled: Boolean, + clock: () -> Long = { System.currentTimeMillis() / 1000L }, +): Long { + val lifecycle = LocalLifecycleOwner.current.lifecycle + return produceState( + initialValue = clock(), key1 = enabled, + key2 = lifecycle, ) { - while (enabled) { - delay(5_000L) - value = System.currentTimeMillis() / 1000L + if (enabled) { + lifecycle.repeatOnLifecycle(Lifecycle.State.STARTED) { + while (isActive) { + value = clock() + delay(5_000L) + } + } } }.value +} diff --git a/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanScreen.kt b/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanScreen.kt index d5274829..d64d5f75 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanScreen.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/kanban/KanbanScreen.kt @@ -90,13 +90,16 @@ import com.m57.hermescontrol.ui.kanban.components.KanbanFilterSheet import com.m57.hermescontrol.ui.kanban.components.KanbanOrchestrationDialog import com.m57.hermescontrol.ui.kanban.components.KanbanTaskCard import com.m57.hermescontrol.util.StreamingUriRequestBody +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.MultipartBody import okhttp3.RequestBody.Companion.toRequestBody import java.io.File +import kotlin.coroutines.coroutineContext private const val DEFAULT_COLUMN = "todo" @@ -159,26 +162,27 @@ fun KanbanScreen( var showOrchestrationDialog by remember { mutableStateOf(false) } // Board Transfer (Export / Import) - var pendingExportPath by remember { mutableStateOf(null) } + var pendingExportPath by rememberPendingKanbanExportPath() val exportLauncher = rememberLauncherForActivityResult( contract = ActivityResultContracts.CreateDocument("application/gzip"), ) { uri -> - if (uri != null && pendingExportPath != null) { + if (uri != null) { coroutineScope.launch { try { - val cachedFile = File(pendingExportPath!!) - if (cachedFile.exists()) { - withContext(Dispatchers.IO) { - context.contentResolver.openOutputStream(uri)?.use { output -> - cachedFile.inputStream().use { input -> - input.copyTo(output) - } - } ?: error("Unable to open selected destination") - } - viewModel.showToast("Board exported successfully") + val cachedFile = File(checkNotNull(pendingExportPath) { "No pending board export" }) + withContext(Dispatchers.IO) { + check(cachedFile.isFile) { "Cached board export is missing; export the board again" } + context.contentResolver.openOutputStream(uri)?.use { output -> + cachedFile.inputStream().use { input -> + input.copyTo(output) + } + } ?: error("Unable to open selected destination") } + viewModel.showToast("Board exported successfully") + } catch (e: CancellationException) { + throw e } catch (e: Exception) { viewModel.showToast("Failed to save export: ${e.message}") } finally { @@ -230,39 +234,55 @@ fun KanbanScreen( contentLength = contentLength, ) val filePart = MultipartBody.Part.createFormData("file", fileName, requestBody) + // Retrofit streams the body on OkHttp's worker; avoid losing a confirmed + // response to withContext's prompt cancellation on the return hop (#1137). val uploadResult = - withContext(Dispatchers.IO) { - safeApiCall { - ApiClient.hermesApi.uploadManagedFileStream(pathBody, overwriteBody, filePart) - } + safeApiCall { + ApiClient.hermesApi.uploadManagedFileStream(pathBody, overwriteBody, filePart) } when (uploadResult) { is NetworkResult.Success -> { val serverPath = uploadResult.data.entry?.path ?: uploadResult.data.path ?: targetPath - val importResult = viewModel.importBoard(serverPath) - when (importResult) { - is NetworkResult.Success -> { - val res = importResult.data - var msg = "Board '${res.name}' imported" - if (res.renamed) msg += " (as ${res.board})" - viewModel.showToast(msg) - viewModel.loadBoards() - } + withKanbanImportCleanup( + cleanup = { + val response = + ApiClient.hermesApi.deleteManagedFile( + ManagedFileDelete(serverPath), + ) + check( + response.isSuccessful, + ) { "Archive cleanup failed (HTTP ${response.code()})" } + }, + onCleanupFailure = { + viewModel.showToast("Temporary import archive cleanup failed: ${it.message}") + }, + ) { + val importResult = viewModel.importBoard(serverPath) + coroutineContext.ensureActive() + when (importResult) { + is NetworkResult.Success -> { + val res = importResult.data + var msg = "Board '${res.name}' imported" + if (res.renamed) msg += " (as ${res.board})" + viewModel.showToast(msg) + viewModel.loadBoards() + } - is NetworkResult.Failure -> { - viewModel.showToast("Import failed: ${importResult.error.message}") + is NetworkResult.Failure -> { + viewModel.showToast("Import failed: ${importResult.error.message}") + } } } - runCatching { - ApiClient.hermesApi.deleteManagedFile(ManagedFileDelete(serverPath)) - } } is NetworkResult.Failure -> { + coroutineContext.ensureActive() viewModel.showToast("Failed to upload archive: ${uploadResult.error.message}") } } + } catch (e: CancellationException) { + throw e } catch (e: Exception) { viewModel.showToast("Import failed: ${e.message}") } diff --git a/app/src/test/java/com/m57/hermescontrol/ui/kanban/KanbanImportCleanupTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/kanban/KanbanImportCleanupTest.kt new file mode 100644 index 00000000..13f93af7 --- /dev/null +++ b/app/src/test/java/com/m57/hermescontrol/ui/kanban/KanbanImportCleanupTest.kt @@ -0,0 +1,108 @@ +package com.m57.hermescontrol.ui.kanban + +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test + +@OptIn(ExperimentalCoroutinesApi::class) +class KanbanImportCleanupTest { + @Test + fun cleansAfterSuccess() = + runTest { + var cleaned = false + val result = + withKanbanImportCleanup( + cleanup = { cleaned = true }, + onCleanupFailure = { throw AssertionError(it) }, + dispatcher = StandardTestDispatcher(testScheduler), + ) { "imported" } + assertEquals("imported", result) + assertTrue(cleaned) + } + + @Test + fun cleansAfterImportFailureWithoutMaskingIt() = + runTest { + val failure = IllegalStateException("import failed") + var cleaned = false + try { + withKanbanImportCleanup( + cleanup = { cleaned = true }, + onCleanupFailure = { throw AssertionError(it) }, + dispatcher = StandardTestDispatcher(testScheduler), + ) { throw failure } + } catch (e: IllegalStateException) { + assertSame(failure, e) + } + assertTrue(cleaned) + } + + @Test + fun cancellationWaitsForSuspendingCleanupAndRemainsCancelled() = + runTest { + var cleaned = false + var cancellationPropagated = false + val job = + launch { + try { + withKanbanImportCleanup( + cleanup = { + delay(100) + cleaned = true + }, + onCleanupFailure = { throw AssertionError(it) }, + dispatcher = StandardTestDispatcher(testScheduler), + ) { awaitCancellation() } + } catch (e: CancellationException) { + cancellationPropagated = true + throw e + } + } + runCurrent() + job.cancel() + advanceUntilIdle() + assertTrue(cleaned) + assertTrue(cancellationPropagated) + assertTrue(job.isCancelled) + } + + @Test + fun cleanupFailureIsReportedWithoutMaskingImportResult() = + runTest { + val failure = IllegalStateException("HTTP 500") + var reported: Exception? = null + val result = + withKanbanImportCleanup( + cleanup = { throw failure }, + onCleanupFailure = { reported = it }, + dispatcher = StandardTestDispatcher(testScheduler), + ) { "imported" } + assertEquals("imported", result) + // Coroutine stack-trace recovery can copy exceptions across dispatcher boundaries. + assertEquals(failure.message, reported?.message) + assertTrue(reported is IllegalStateException) + } + + @Test + fun cleanupIsBounded() = + runTest { + var reported = false + withKanbanImportCleanup( + cleanup = { awaitCancellation() }, + onCleanupFailure = { reported = true }, + dispatcher = StandardTestDispatcher(testScheduler), + ) { Unit } + assertTrue(reported) + assertEquals(10_000L, testScheduler.currentTime) + } +}