From e090503d91d6abf93a595e94a739763f1b726944 Mon Sep 17 00:00:00 2001 From: JP Date: Wed, 10 Jun 2026 22:40:54 -0700 Subject: [PATCH] Fix torrent loss on restart, session-shutdown ANR, and player icon overlap Torrents could disappear from the list after an app restart even though their data was still on disk. Three causes, all fixed: - onStop saved resume data on lifecycleScope (cancelled at ON_DESTROY) while onDestroy released the session on the main thread; when release won the race, nothing was saved. A new SessionLifecycleCoordinator now serializes init/save/release on a FIFO queue owned by a process-wide scope, so release can never run before a pending save. - Resume files were written non-atomically; a process kill mid-write truncated previously-good files. Writes now go through tmp + rename, and corrupt files are quarantined as .corrupt and recovered via a magnet re-add (the filename embeds the info-hash). - Resume data was only saved at onStop. It is now also requested eagerly on add_torrent, metadata_received, and torrent_finished alerts. The session-recreate path (save path change) caused an ANR: nativeRelease held the global mutex while libtorrent waited on tracker stop-announces (~27s), blocking a main-thread addMagnet call. nativeRelease now aborts the session under the lock and finishes shutdown on a detached thread via session_proxy, and all native calls moved off the main thread: the repository owns the threading contract, routing every mutating call through the session queue (which also waits for pending init, so calls can no longer hit an uninitialized session). Also fixed: subtitle (CC) and audio-track buttons in the player rendered stacked on top of each other (Box -> Row), Copy magnet silently writing an empty string to the clipboard, and dead background-mode plumbing (onAppBackground/onAppForeground, setBackgroundMode) removed. Tests: 9 JVM unit tests for the coordinator (including a regression test for the save/release race) and 5 instrumented persistence tests (restart round-trip, eager save, quarantine, magnet recovery, removal). --- .../data/TorrentPersistenceTest.kt | 146 +++++++++++++++++ app/src/main/cpp/torrent_jni.cpp | 115 +++++++++++--- .../jpcottin/simpletorrent/MainActivity.kt | 31 ++-- .../simpletorrent/data/DataRepository.kt | 46 +++--- .../data/SessionLifecycleCoordinator.kt | 100 ++++++++++++ .../simpletorrent/data/TorrentManager.kt | 35 +++- .../simpletorrent/ui/main/MainScreen.kt | 16 +- .../ui/main/MainScreenViewModel.kt | 28 ++-- .../simpletorrent/ui/player/PlayerScreen.kt | 9 +- .../data/SessionLifecycleCoordinatorTest.kt | 149 ++++++++++++++++++ .../ui/main/MainScreenViewModelTest.kt | 9 +- 11 files changed, 603 insertions(+), 81 deletions(-) create mode 100644 app/src/androidTest/java/com/jpcottin/simpletorrent/data/TorrentPersistenceTest.kt create mode 100644 app/src/main/java/com/jpcottin/simpletorrent/data/SessionLifecycleCoordinator.kt create mode 100644 app/src/test/java/com/jpcottin/simpletorrent/data/SessionLifecycleCoordinatorTest.kt diff --git a/app/src/androidTest/java/com/jpcottin/simpletorrent/data/TorrentPersistenceTest.kt b/app/src/androidTest/java/com/jpcottin/simpletorrent/data/TorrentPersistenceTest.kt new file mode 100644 index 0000000..b2f5b1d --- /dev/null +++ b/app/src/androidTest/java/com/jpcottin/simpletorrent/data/TorrentPersistenceTest.kt @@ -0,0 +1,146 @@ +package com.jpcottin.simpletorrent.data + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import java.io.File +import kotlin.random.Random + +/** + * End-to-end tests for resume-data persistence in the native layer: torrents must + * survive a session release + re-init (the "app restart" path), resume files must be + * written eagerly without waiting for onStop, and a corrupt resume file must be + * quarantined instead of crashing or silently losing the rest. + */ +@RunWith(AndroidJUnit4::class) +class TorrentPersistenceTest { + + private lateinit var context: Context + private lateinit var stateDir: File + + @Before + fun setUp() { + context = ApplicationProvider.getApplicationContext() + stateDir = File(context.cacheDir, "test_state_${System.nanoTime()}").apply { mkdirs() } + // The native session is a process-wide singleton; make sure each test starts fresh + TorrentManager.nativeRelease() + } + + @After + fun tearDown() { + TorrentManager.nativeRelease() + stateDir.deleteRecursively() + } + + @Test + fun torrentSurvivesSessionRestart() { + TorrentManager.init(context, stateDir.absolutePath) + val hash = seedSampleTorrent() + + TorrentManager.nativeSaveResumeData() + TorrentManager.nativeRelease() + TorrentManager.init(context, stateDir.absolutePath) + + waitFor("torrent $hash restored after restart") { + TorrentManager.getTorrents().any { it.infoHash == hash } + } + } + + @Test + fun resumeFileIsWrittenEagerly_withoutExplicitSave() { + TorrentManager.init(context, stateDir.absolutePath) + val hash = seedSampleTorrent() + + // No nativeSaveResumeData() here: the add_torrent_alert handler must request + // the save on its own. Polling getTorrents() drives the alert loop. + waitFor("eager resume file for $hash") { + TorrentManager.getTorrents() + File(stateDir, "$hash.resume").exists() + } + } + + @Test + fun corruptResumeFileIsQuarantined_andSessionStillStarts() { + // Name is not a valid 40-hex info-hash, so no magnet recovery is possible — + // the file must still be quarantined without crashing the session + val corrupt = File(stateDir, "deadbeef.resume").apply { writeText("not bencoded data") } + + TorrentManager.init(context, stateDir.absolutePath) + + // Session must be functional despite the bad file + TorrentManager.getTorrents() + assertFalse("corrupt file should have been moved aside", corrupt.exists()) + assertTrue( + "corrupt file should be quarantined as .corrupt", + File(stateDir, "deadbeef.resume.corrupt").exists(), + ) + } + + @Test + fun corruptResumeFile_isRecoveredAsMagnet_whenNameIsValidInfoHash() { + val hash = "ab".repeat(20) // valid 40-char hex info-hash + File(stateDir, "$hash.resume").writeText("garbage, not bencoded") + + TorrentManager.init(context, stateDir.absolutePath) + + assertTrue(File(stateDir, "$hash.resume.corrupt").exists()) + // The torrent must come back as a magnet add (fetching metadata) instead + // of silently vanishing + waitFor("recovered torrent $hash in session") { + TorrentManager.getTorrents().any { it.infoHash == hash } + } + } + + @Test + fun removedTorrentDoesNotReappearAfterRestart() { + TorrentManager.init(context, stateDir.absolutePath) + val hash = seedSampleTorrent() + waitFor("resume file for $hash") { + TorrentManager.getTorrents() + File(stateDir, "$hash.resume").exists() + } + + TorrentManager.removeTorrent(hash, deleteFiles = false) + waitFor("torrent $hash removed") { + TorrentManager.getTorrents().none { it.infoHash == hash } + } + assertFalse(File(stateDir, "$hash.resume").exists()) + + TorrentManager.nativeRelease() + TorrentManager.init(context, stateDir.absolutePath) + Thread.sleep(1_000) // give async_add_torrent a chance to (wrongly) restore it + assertTrue(TorrentManager.getTorrents().none { it.infoHash == hash }) + } + + // ── helpers ─────────────────────────────────────────────────────────────── + + /** Creates and seeds a small local torrent, returning its info hash. */ + private fun seedSampleTorrent(): String { + val source = File(context.cacheDir, "sample_${System.nanoTime()}.bin").apply { + writeBytes(Random.nextBytes(64 * 1024)) + } + val result = TorrentManager.createTorrentFrom( + source.absolutePath, + File(context.cacheDir, "torrents").absolutePath, + ) + assertTrue("createTorrentFrom failed: $result", result is CreateTorrentResult.Success) + waitFor("torrent appears in session") { TorrentManager.getTorrents().isNotEmpty() } + return TorrentManager.getTorrents().first().infoHash + } + + private fun waitFor(what: String, timeoutMs: Long = 15_000, condition: () -> Boolean) { + val deadline = System.currentTimeMillis() + timeoutMs + while (System.currentTimeMillis() < deadline) { + if (condition()) return + Thread.sleep(200) + } + fail("Timed out after ${timeoutMs}ms waiting for: $what") + } +} diff --git a/app/src/main/cpp/torrent_jni.cpp b/app/src/main/cpp/torrent_jni.cpp index 2ccd314..244c8c0 100644 --- a/app/src/main/cpp/torrent_jni.cpp +++ b/app/src/main/cpp/torrent_jni.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -96,6 +97,34 @@ static std::string resume_path(const lt::sha1_hash& hash) { return g_state_dir + "/" + sha1_to_hex(hash) + ".resume"; } +// Write atomically (tmp file + rename): a process kill mid-write must never +// truncate a previously-good resume file. +static bool atomic_write(const std::string& path, const char* data, size_t size) { + std::string tmp = path + ".tmp"; + { + std::ofstream f(tmp, std::ios::binary | std::ios::trunc); + if (!f) return false; + f.write(data, static_cast(size)); + f.flush(); + if (!f) { ::remove(tmp.c_str()); return false; } + } + if (::rename(tmp.c_str(), path.c_str()) != 0) { + ::remove(tmp.c_str()); + return false; + } + return true; +} + +static void write_resume_file(const lt::add_torrent_params& params) { + mkdir(g_state_dir.c_str(), 0755); + auto buf = lt::write_resume_data_buf(params); + std::string path = resume_path(params.info_hashes.v1); + if (atomic_write(path, buf.data(), buf.size())) + LOGI("Saved resume: %s", path.c_str()); + else + LOGE("Failed writing resume: %s", path.c_str()); +} + static void load_resume_data() { mkdir(g_state_dir.c_str(), 0755); DIR* dir = opendir(g_state_dir.c_str()); @@ -114,7 +143,25 @@ static void load_resume_data() { lt::error_code ec; lt::add_torrent_params params = lt::read_resume_data(buf, ec); if (ec) { - LOGE("Bad resume file %s: %s", fname.c_str(), ec.message().c_str()); + // Quarantine instead of deleting so the file can be inspected; the + // .corrupt suffix keeps it from being re-parsed on every launch. + LOGE("Bad resume file %s: %s — quarantined as .corrupt", fname.c_str(), ec.message().c_str()); + ::rename(path.c_str(), (path + ".corrupt").c_str()); + // Recovery: the filename embeds the info-hash, so re-add as a magnet. + // libtorrent re-fetches the metadata and rechecks any data already on + // disk, instead of the torrent silently vanishing from the list. + std::string hex = fname.substr(0, fname.size() - 7); + if (hex.size() == 40 && + hex.find_first_not_of("0123456789abcdef") == std::string::npos) { + lt::error_code mec; + lt::add_torrent_params recovered = + lt::parse_magnet_uri("magnet:?xt=urn:btih:" + hex, mec); + if (!mec) { + recovered.save_path = g_save_path; + g_session->async_add_torrent(recovered); + LOGI("Recovering torrent %s via magnet re-add", hex.c_str()); + } + } continue; } // Preserve the save_path from resume data so seeders keep their original path. @@ -145,13 +192,10 @@ static void save_all_resume_data() { g_session->pop_alerts(&alerts); for (auto* a : alerts) { if (auto* rd = lt::alert_cast(a)) { - auto buf = lt::write_resume_data_buf(rd->params); - std::string path = resume_path(rd->params.info_hashes.v1); - std::ofstream f(path, std::ios::binary | std::ios::trunc); - f.write(buf.data(), static_cast(buf.size())); - LOGI("Saved resume: %s", path.c_str()); + write_resume_file(rd->params); --outstanding; - } else if (lt::alert_cast(a)) { + } else if (auto* fail = lt::alert_cast(a)) { + LOGE("save_resume_data failed: %s", fail->error.message().c_str()); --outstanding; } } @@ -229,12 +273,33 @@ Java_com_jpcottin_simpletorrent_data_TorrentManager_nativeInit( JNIEXPORT void JNICALL Java_com_jpcottin_simpletorrent_data_TorrentManager_nativeRelease( JNIEnv* /*env*/, jobject /*thiz*/) { - std::lock_guard lock(g_mutex); - if (!g_session) return; - // Resume data is already saved by nativeSaveResumeData() from onStop. - // Calling save_all_resume_data() here would block the main thread → ANR. - g_session.reset(); - LOGI("Session released"); + // Resume data is saved before release: SessionLifecycleCoordinator sequences + // nativeSaveResumeData() ahead of this call on its FIFO queue. + // + // Shutdown must happen OUTSIDE g_mutex: destroying the session joins + // libtorrent's network thread, which can block for seconds waiting on + // tracker stop-announces. Holding the mutex during that stalls every other + // native call — including ones on the main thread — and caused an ANR. + lt::session_proxy proxy; + { + std::lock_guard lock(g_mutex); + if (!g_session) return; + proxy = g_session->abort(); + g_session.reset(); + } + // Finish the shutdown on a detached thread: waiting on tracker stop-announces + // can take tens of seconds, and a re-init queued behind this call (save path + // change, activity recreation) must not wait for it. + std::thread([p = std::move(proxy)]() mutable { + { + // The proxy must be DESTROYED (not assigned over): only ~session_proxy + // joins the network thread; overwriting it would destroy a joinable + // std::thread and call std::terminate(). + lt::session_proxy local = std::move(p); + } // blocks here until the old session is fully torn down + LOGI("Session shutdown complete"); + }).detach(); + LOGI("Session released (shutdown continuing in background)"); } // Save resume data without destroying the session (safe to call from onStop) @@ -302,6 +367,7 @@ Java_com_jpcottin_simpletorrent_data_TorrentManager_removeTorrent( // Also delete the resume file so it isn't reloaded on next start std::string path = g_state_dir + "/" + hex + ".resume"; ::remove(path.c_str()); + ::remove((path + ".tmp").c_str()); LOGI("Removed: %s deleteFiles=%d", hex.c_str(), (int)deleteFiles); } @@ -314,15 +380,26 @@ Java_com_jpcottin_simpletorrent_data_TorrentManager_getTorrentsJson( std::vector alerts; g_session->pop_alerts(&alerts); - // Process alerts to build error map and save resume data + // Process alerts to build error map and save resume data. + // Resume data is requested eagerly (on add / metadata / finish) so a crash or + // process kill never loses torrents — onStop is only a final catch-all save. std::unordered_map torrent_errors; for (auto* a : alerts) { if (auto* rd = lt::alert_cast(a)) { - // Write resume data to disk - auto buf = lt::write_resume_data_buf(rd->params); - std::string path = resume_path(rd->params.info_hashes.v1); - std::ofstream f(path, std::ios::binary | std::ios::trunc); - if (f) f.write(buf.data(), static_cast(buf.size())); + write_resume_file(rd->params); + } else if (auto* fail = lt::alert_cast(a)) { + LOGE("save_resume_data failed: %s", fail->error.message().c_str()); + } else if (auto* add = lt::alert_cast(a)) { + if (add->error) + LOGE("add torrent failed: %s", add->error.message().c_str()); + else if (add->handle.is_valid()) + add->handle.save_resume_data(lt::torrent_handle::save_info_dict); + } else if (auto* md = lt::alert_cast(a)) { + if (md->handle.is_valid()) + md->handle.save_resume_data(lt::torrent_handle::save_info_dict); + } else if (auto* fin = lt::alert_cast(a)) { + if (fin->handle.is_valid()) + fin->handle.save_resume_data(lt::torrent_handle::save_info_dict); } else if (auto* te = lt::alert_cast(a)) { std::string hash = sha1_to_hex(te->handle.status().info_hashes.v1); torrent_errors[hash] = te->error.message(); diff --git a/app/src/main/java/com/jpcottin/simpletorrent/MainActivity.kt b/app/src/main/java/com/jpcottin/simpletorrent/MainActivity.kt index b3a3bc5..a7051e1 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/MainActivity.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/MainActivity.kt @@ -18,15 +18,22 @@ import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Surface import androidx.compose.ui.Modifier +import com.jpcottin.simpletorrent.data.SessionLifecycleCoordinator import com.jpcottin.simpletorrent.data.TorrentManager import com.jpcottin.simpletorrent.theme.SimpleTorrentTheme class MainActivity : ComponentActivity() { + // All session operations (init/save/release) are sequenced on the coordinator's + // FIFO queue, which outlives this activity. lifecycleScope must NOT be used for + // the resume-data save: it is cancelled at ON_DESTROY, which used to lose data. + private lateinit var session: SessionLifecycleCoordinator + override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) requestAllFilesAccessIfNeeded() - TorrentManager.init(this) + session = TorrentManager.lifecycleCoordinator(this) + session.onCreate() handleTorrentIntent(intent) enableEdgeToEdge() setContent { @@ -41,32 +48,23 @@ class MainActivity : ComponentActivity() { override fun onStart() { super.onStart() // App is becoming visible; restore normal libtorrent tick interval and polling - TorrentManager.nativeSetTickInterval(500) - TorrentManager.isInBackground = false + session.onStart() } override fun onResume() { super.onResume() // Reinit only if the resolved save path changed (e.g. user just granted MANAGE_EXTERNAL_STORAGE) - val newPath = TorrentManager.resolvedSavePath(this) - if (newPath != TorrentManager.currentSavePath) { - TorrentManager.nativeRelease() - TorrentManager.init(this) - } + session.onResume() } override fun onStop() { super.onStop() - // App is going background; slow down libtorrent to save battery - lifecycleScope.launch(Dispatchers.IO) { - TorrentManager.nativeSaveResumeData() - TorrentManager.nativeSetTickInterval(2000) - TorrentManager.isInBackground = true - } + // App is going background; save resume data, then slow down libtorrent to save battery + session.onStop() } override fun onDestroy() { - TorrentManager.nativeRelease() + session.onDestroy() super.onDestroy() } @@ -93,7 +91,8 @@ class MainActivity : ComponentActivity() { val stagedFile = File(stagingDir, name) runCatching { contentResolver.openInputStream(uri)!!.use { it.copyTo(stagedFile.outputStream()) } - TorrentManager.addTorrentFile(stagedFile.absolutePath) + // Queue behind any pending init so the add never hits an uninitialized session + session.withSession { TorrentManager.addTorrentFile(stagedFile.absolutePath) } } } } diff --git a/app/src/main/java/com/jpcottin/simpletorrent/data/DataRepository.kt b/app/src/main/java/com/jpcottin/simpletorrent/data/DataRepository.kt index 672d2cb..a5eea45 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/data/DataRepository.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/data/DataRepository.kt @@ -1,6 +1,5 @@ package com.jpcottin.simpletorrent.data -import kotlin.time.Duration.Companion.seconds import android.util.Log import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay @@ -8,23 +7,29 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.retryWhen -import kotlin.math.min import kotlin.time.Duration.Companion.seconds interface DataRepository { val torrents: Flow> suspend fun addMagnet(uri: String): String - fun pauseTorrent(infoHash: String) - fun resumeTorrent(infoHash: String) - fun removeTorrent(infoHash: String, deleteFiles: Boolean) - fun setSequentialDownload(infoHash: String, enabled: Boolean) + suspend fun pauseTorrent(infoHash: String) + suspend fun resumeTorrent(infoHash: String) + suspend fun removeTorrent(infoHash: String, deleteFiles: Boolean) + suspend fun setSequentialDownload(infoHash: String, enabled: Boolean) suspend fun createTorrentFrom(sourcePath: String, outputDir: String): CreateTorrentResult - fun setBackgroundMode(isBackground: Boolean) } +/** + * Owns the native-boundary threading contract: every mutating call goes through + * [TorrentManager.withSession], which (a) waits for any pending session init so a + * call can never hit an uninitialized session, and (b) executes on the session + * queue's IO scope so the native mutex can never block the caller's thread. + */ class DefaultDataRepository : DataRepository { override val torrents: Flow> = flow { + // Wait for any pending init so the first emission isn't a false empty list + TorrentManager.withSession { } while (true) { emit(TorrentManager.getTorrents()) val delay = if (TorrentManager.isInBackground) 10.seconds else 3.seconds @@ -39,16 +44,21 @@ class DefaultDataRepository : DataRepository { } .flowOn(Dispatchers.IO) - override suspend fun addMagnet(uri: String): String = TorrentManager.addMagnet(uri) - override fun pauseTorrent(infoHash: String) = TorrentManager.pauseTorrent(infoHash) - override fun resumeTorrent(infoHash: String) = TorrentManager.resumeTorrent(infoHash) - override fun removeTorrent(infoHash: String, deleteFiles: Boolean) = - TorrentManager.removeTorrent(infoHash, deleteFiles) - override fun setSequentialDownload(infoHash: String, enabled: Boolean) = - TorrentManager.setSequentialDownload(infoHash, enabled) - override suspend fun createTorrentFrom(sourcePath: String, outputDir: String): CreateTorrentResult = - TorrentManager.createTorrentFrom(sourcePath, outputDir) - override fun setBackgroundMode(isBackground: Boolean) { - TorrentManager.nativeSetTickInterval(if (isBackground) 2000 else 500) + override suspend fun addMagnet(uri: String): String = + TorrentManager.withSession { TorrentManager.addMagnet(uri) } + override suspend fun pauseTorrent(infoHash: String) = + TorrentManager.withSession { TorrentManager.pauseTorrent(infoHash) } + override suspend fun resumeTorrent(infoHash: String) = + TorrentManager.withSession { TorrentManager.resumeTorrent(infoHash) } + override suspend fun removeTorrent(infoHash: String, deleteFiles: Boolean) = + TorrentManager.withSession { TorrentManager.removeTorrent(infoHash, deleteFiles) } + override suspend fun setSequentialDownload(infoHash: String, enabled: Boolean) = + TorrentManager.withSession { TorrentManager.setSequentialDownload(infoHash, enabled) } + + override suspend fun createTorrentFrom(sourcePath: String, outputDir: String): CreateTorrentResult { + // Wait for init, but hash OFF the session queue: hashing a large file can + // take minutes and must not delay a queued onStop resume-data save. + TorrentManager.withSession { } + return TorrentManager.createTorrentFrom(sourcePath, outputDir) } } diff --git a/app/src/main/java/com/jpcottin/simpletorrent/data/SessionLifecycleCoordinator.kt b/app/src/main/java/com/jpcottin/simpletorrent/data/SessionLifecycleCoordinator.kt new file mode 100644 index 0000000..f39c808 --- /dev/null +++ b/app/src/main/java/com/jpcottin/simpletorrent/data/SessionLifecycleCoordinator.kt @@ -0,0 +1,100 @@ +package com.jpcottin.simpletorrent.data + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Deferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.async + +/** + * Abstraction over the native torrent session so the lifecycle sequencing in + * [SessionLifecycleCoordinator] can be unit tested without loading the native library. + */ +interface TorrentSessionControl { + /** True if the resolved save path no longer matches the one the session was started with. */ + fun savePathChanged(): Boolean + fun init() + fun release() + fun saveResumeData() + fun setTickInterval(intervalMs: Int) + fun setBackground(isBackground: Boolean) +} + +/** + * Serializes all native session operations (init / save / release) on a FIFO queue + * owned by a process-wide scope that survives Activity destruction. + * + * This fixes two ways resume data could be silently lost: + * - onStop used to launch the save on lifecycleScope, which is cancelled at + * ON_DESTROY — swiping the app away could cancel the save before it ran. + * - onDestroy used to release the session on the main thread, racing the async + * save; if release won, save_all_resume_data() found no session and wrote nothing. + * + * Lifecycle methods must be called from a single thread (the main thread). + */ +class SessionLifecycleCoordinator( + private val session: TorrentSessionControl, + private val scope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.IO), +) { + private var tail: Deferred<*>? = null + + fun onCreate() { + enqueue { session.init() } + } + + fun onStart() { + enqueue { + session.setTickInterval(FOREGROUND_TICK_MS) + session.setBackground(false) + } + } + + /** Re-creates the session when the save path changed (e.g. storage permission granted). */ + fun onResume() { + enqueue { + if (session.savePathChanged()) { + session.saveResumeData() + session.release() + session.init() + } + } + } + + fun onStop() { + enqueue { + session.saveResumeData() + session.setTickInterval(BACKGROUND_TICK_MS) + session.setBackground(true) + } + } + + fun onDestroy() { + enqueue { session.release() } + } + + /** Runs [block] on the session queue, after any pending init/save/release. */ + suspend fun withSession(block: suspend () -> T): T = enqueue(block).await() + + @Synchronized + private fun enqueue(block: suspend () -> T): Deferred { + val prev = tail + val next = scope.async { + prev?.join() + block() + } + // Drop the reference once the queue drains so the last completed + // Deferred isn't retained indefinitely + next.invokeOnCompletion { + synchronized(this@SessionLifecycleCoordinator) { + if (tail === next) tail = null + } + } + tail = next + return next + } + + companion object { + const val FOREGROUND_TICK_MS = 500 + const val BACKGROUND_TICK_MS = 2000 + } +} diff --git a/app/src/main/java/com/jpcottin/simpletorrent/data/TorrentManager.kt b/app/src/main/java/com/jpcottin/simpletorrent/data/TorrentManager.kt index 2d8de09..03249e4 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/data/TorrentManager.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/data/TorrentManager.kt @@ -72,10 +72,39 @@ object TorrentManager { var currentSavePath: String = "" private set - fun init(context: Context) { + fun init( + context: Context, + stateDir: String = context.filesDir.absolutePath + "/torrent_state", + ) { currentSavePath = resolvedSavePath(context) - val statePath = context.filesDir.absolutePath + "/torrent_state" - nativeInit(currentSavePath, statePath) + nativeInit(currentSavePath, stateDir) + } + + @Volatile + private lateinit var appContext: Context + + /** Returns the process-wide lifecycle coordinator, binding it to the application context. */ + fun lifecycleCoordinator(context: Context): SessionLifecycleCoordinator { + appContext = context.applicationContext + return coordinator + } + + /** + * Runs [block] on the session FIFO queue, after any pending init/save/release. + * This both guarantees the session is initialized and keeps the native call + * off the caller's thread. + */ + suspend fun withSession(block: suspend () -> T): T = coordinator.withSession(block) + + private val coordinator by lazy { + SessionLifecycleCoordinator(object : TorrentSessionControl { + override fun savePathChanged() = resolvedSavePath(appContext) != currentSavePath + override fun init() = init(appContext) + override fun release() = nativeRelease() + override fun saveResumeData() = nativeSaveResumeData() + override fun setTickInterval(intervalMs: Int) = nativeSetTickInterval(intervalMs) + override fun setBackground(isBackground: Boolean) { isInBackground = isBackground } + }) } fun getTorrents(): List = diff --git a/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreen.kt b/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreen.kt index 47e7e63..ebdb1a3 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreen.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreen.kt @@ -6,6 +6,7 @@ import android.content.ClipboardManager import android.content.Context import android.content.Intent import android.net.Uri +import android.widget.Toast import androidx.activity.compose.rememberLauncherForActivityResult import androidx.activity.result.contract.ActivityResultContracts import androidx.compose.animation.AnimatedVisibility @@ -575,9 +576,18 @@ internal fun TorrentCard( text = { Text("Copy magnet") }, onClick = { showShareMenu = false - val magnet = TorrentManager.getMagnetUri(torrent.infoHash) - val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager - clipboard.setPrimaryClip(ClipData.newPlainText("magnet", magnet)) + scope.launch { + // Session queue: waits for init and runs off the main thread + val magnet = TorrentManager.withSession { + TorrentManager.getMagnetUri(torrent.infoHash) + } + if (magnet.isEmpty()) { + Toast.makeText(context, "Magnet link unavailable", Toast.LENGTH_SHORT).show() + } else { + val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager + clipboard.setPrimaryClip(ClipData.newPlainText("magnet", magnet)) + } + } }, ) DropdownMenuItem( diff --git a/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreenViewModel.kt b/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreenViewModel.kt index 8b3b9c1..6e07050 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreenViewModel.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/ui/main/MainScreenViewModel.kt @@ -40,6 +40,8 @@ class MainScreenViewModel( private val _createState = MutableStateFlow(CreateState.Idle) val createState: StateFlow = _createState.asStateFlow() + // Threading note: the repository owns the native-boundary contract — its suspend + // methods run on the session queue, so plain viewModelScope.launch is safe here. fun addMagnet(uri: String) { viewModelScope.launch { val result = repository.addMagnet(uri.trim()) @@ -48,10 +50,18 @@ class MainScreenViewModel( } } } - fun pause(infoHash: String) { repository.pauseTorrent(infoHash) } - fun resume(infoHash: String) { repository.resumeTorrent(infoHash) } - fun remove(infoHash: String, deleteFiles: Boolean) { repository.removeTorrent(infoHash, deleteFiles) } - fun setSequentialDownload(infoHash: String, enabled: Boolean) { repository.setSequentialDownload(infoHash, enabled) } + fun pause(infoHash: String) { + viewModelScope.launch { repository.pauseTorrent(infoHash) } + } + fun resume(infoHash: String) { + viewModelScope.launch { repository.resumeTorrent(infoHash) } + } + fun remove(infoHash: String, deleteFiles: Boolean) { + viewModelScope.launch { repository.removeTorrent(infoHash, deleteFiles) } + } + fun setSequentialDownload(infoHash: String, enabled: Boolean) { + viewModelScope.launch { repository.setSequentialDownload(infoHash, enabled) } + } fun createTorrent(sourcePath: String, outputDir: String) { _createState.value = CreateState.Creating @@ -91,16 +101,6 @@ class MainScreenViewModel( } fun dismissCreateResult() { _createState.value = CreateState.Idle } - - fun onAppBackground() { - com.jpcottin.simpletorrent.data.TorrentManager.isInBackground = true - repository.setBackgroundMode(true) - } - - fun onAppForeground() { - com.jpcottin.simpletorrent.data.TorrentManager.isInBackground = false - repository.setBackgroundMode(false) - } } sealed interface CreateState { diff --git a/app/src/main/java/com/jpcottin/simpletorrent/ui/player/PlayerScreen.kt b/app/src/main/java/com/jpcottin/simpletorrent/ui/player/PlayerScreen.kt index 7a8ee47..2b1faf9 100644 --- a/app/src/main/java/com/jpcottin/simpletorrent/ui/player/PlayerScreen.kt +++ b/app/src/main/java/com/jpcottin/simpletorrent/ui/player/PlayerScreen.kt @@ -4,6 +4,7 @@ import android.content.Context import android.net.Uri import androidx.compose.foundation.background import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.WindowInsets import androidx.compose.foundation.layout.asPaddingValues import androidx.compose.foundation.layout.fillMaxSize @@ -174,12 +175,14 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { ) } if ((subtitles.isNotEmpty() || audioTracks.size > 1) && actualPlayer != null) { - Box( + // Row, not Box: with a Box the subtitle and audio buttons render stacked + // on top of each other, producing an unreadable overlapped icon. + Row( modifier = Modifier .align(Alignment.TopEnd) .padding(WindowInsets.safeDrawing.asPaddingValues()), ) { - if (subtitles.isNotEmpty()) { + if (subtitles.isNotEmpty()) Box { IconButton(onClick = { showSubtitleMenu = !showSubtitleMenu }) { Icon( Icons.Default.ClosedCaption, @@ -213,7 +216,7 @@ fun PlayerScreen(filePath: String, title: String, onBack: () -> Unit) { } } } - if (audioTracks.size > 1) { + if (audioTracks.size > 1) Box { IconButton(onClick = { showAudioMenu = !showAudioMenu }) { Icon( Icons.Default.Audiotrack, diff --git a/app/src/test/java/com/jpcottin/simpletorrent/data/SessionLifecycleCoordinatorTest.kt b/app/src/test/java/com/jpcottin/simpletorrent/data/SessionLifecycleCoordinatorTest.kt new file mode 100644 index 0000000..6789636 --- /dev/null +++ b/app/src/test/java/com/jpcottin/simpletorrent/data/SessionLifecycleCoordinatorTest.kt @@ -0,0 +1,149 @@ +package com.jpcottin.simpletorrent.data + +import junit.framework.TestCase.assertEquals +import junit.framework.TestCase.assertTrue +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Test + +@OptIn(ExperimentalCoroutinesApi::class) +class SessionLifecycleCoordinatorTest { + + private class FakeSession : TorrentSessionControl { + val ops = mutableListOf() + var pathChanged = false + override fun savePathChanged() = pathChanged + override fun init() { ops += "init" } + override fun release() { ops += "release" } + override fun saveResumeData() { ops += "save" } + override fun setTickInterval(intervalMs: Int) { ops += "tick($intervalMs)" } + override fun setBackground(isBackground: Boolean) { ops += "background($isBackground)" } + } + + // ── the disappearing-torrents regression ───────────────────────────────── + + @Test + fun onDestroy_neverReleasesSessionBeforePendingSave() = runTest { + // Swiping the app away fires onStop then onDestroy back to back. The old + // code released the session on the main thread while the save ran async on + // Dispatchers.IO; when release won the race, no resume data was written and + // torrents disappeared on the next launch even though their data was on disk. + val fake = FakeSession() + val coordinator = SessionLifecycleCoordinator(fake, this) + + coordinator.onStop() + coordinator.onDestroy() // enqueued immediately, before the save has run + advanceUntilIdle() + + val saveIndex = fake.ops.indexOf("save") + val releaseIndex = fake.ops.indexOf("release") + assertTrue("save must have run", saveIndex >= 0) + assertTrue("release must have run", releaseIndex >= 0) + assertTrue("release ran before save: ${fake.ops}", saveIndex < releaseIndex) + } + + @Test + fun activityRecreation_releasesOldSessionBeforeInitOfNewOne() = runTest { + val fake = FakeSession() + val coordinator = SessionLifecycleCoordinator(fake, this) + + // Rotation: old activity stops and is destroyed, new one is created + coordinator.onStop() + coordinator.onDestroy() + coordinator.onCreate() + advanceUntilIdle() + + assertEquals( + listOf("save", "tick(2000)", "background(true)", "release", "init"), + fake.ops, + ) + } + + // ── individual lifecycle steps ──────────────────────────────────────────── + + @Test + fun onCreate_initializesSession() = runTest { + val fake = FakeSession() + SessionLifecycleCoordinator(fake, this).onCreate() + advanceUntilIdle() + assertEquals(listOf("init"), fake.ops) + } + + @Test + fun onStart_restoresForegroundTickAndPolling() = runTest { + val fake = FakeSession() + SessionLifecycleCoordinator(fake, this).onStart() + advanceUntilIdle() + assertEquals(listOf("tick(500)", "background(false)"), fake.ops) + } + + @Test + fun onStop_savesResumeDataBeforeEnteringBackgroundMode() = runTest { + val fake = FakeSession() + SessionLifecycleCoordinator(fake, this).onStop() + advanceUntilIdle() + assertEquals(listOf("save", "tick(2000)", "background(true)"), fake.ops) + } + + @Test + fun onResume_doesNothingWhenSavePathUnchanged() = runTest { + val fake = FakeSession() + SessionLifecycleCoordinator(fake, this).onResume() + advanceUntilIdle() + assertTrue(fake.ops.isEmpty()) + } + + @Test + fun onResume_savesBeforeRecreatingSessionWhenSavePathChanged() = runTest { + // The old code released + re-inited without saving first, dropping any + // torrent added since the last save. + val fake = FakeSession().apply { pathChanged = true } + SessionLifecycleCoordinator(fake, this).onResume() + advanceUntilIdle() + assertEquals(listOf("save", "release", "init"), fake.ops) + } + + // ── withSession ─────────────────────────────────────────────────────────── + + @Test + fun withSession_runsAfterPendingInitAndReturnsValue() = runTest { + val fake = FakeSession() + val coordinator = SessionLifecycleCoordinator(fake, this) + + coordinator.onCreate() + val result = coordinator.withSession { + fake.ops += "addTorrent" + "ok:abc123" + } + + assertEquals("ok:abc123", result) + assertEquals(listOf("init", "addTorrent"), fake.ops) + } + + @Test + fun queue_preservesFifoOrderAcrossManyOperations() = runTest { + val fake = FakeSession() + val coordinator = SessionLifecycleCoordinator(fake, this) + + coordinator.onCreate() + coordinator.onStart() + coordinator.onStop() + coordinator.onStart() + coordinator.onStop() + coordinator.onDestroy() + advanceUntilIdle() + + assertEquals( + listOf( + "init", + "tick(500)", "background(false)", + "save", "tick(2000)", "background(true)", + "tick(500)", "background(false)", + "save", "tick(2000)", "background(true)", + "release", + ), + fake.ops, + ) + } +} diff --git a/app/src/test/java/com/jpcottin/simpletorrent/ui/main/MainScreenViewModelTest.kt b/app/src/test/java/com/jpcottin/simpletorrent/ui/main/MainScreenViewModelTest.kt index cf48649..89cf682 100644 --- a/app/src/test/java/com/jpcottin/simpletorrent/ui/main/MainScreenViewModelTest.kt +++ b/app/src/test/java/com/jpcottin/simpletorrent/ui/main/MainScreenViewModelTest.kt @@ -174,12 +174,11 @@ private class FakeRepository( var lastRemovedDeleteFiles: Boolean? = null override suspend fun addMagnet(uri: String): String { lastAddedMagnet = uri; return addMagnetResult } - override fun pauseTorrent(infoHash: String) { lastPausedHash = infoHash } - override fun resumeTorrent(infoHash: String) { lastResumedHash = infoHash } - override fun removeTorrent(infoHash: String, deleteFiles: Boolean) { + override suspend fun pauseTorrent(infoHash: String) { lastPausedHash = infoHash } + override suspend fun resumeTorrent(infoHash: String) { lastResumedHash = infoHash } + override suspend fun removeTorrent(infoHash: String, deleteFiles: Boolean) { lastRemovedHash = infoHash; lastRemovedDeleteFiles = deleteFiles } - override fun setSequentialDownload(infoHash: String, enabled: Boolean) {} + override suspend fun setSequentialDownload(infoHash: String, enabled: Boolean) {} override suspend fun createTorrentFrom(sourcePath: String, outputDir: String) = createResult - override fun setBackgroundMode(isBackground: Boolean) {} }