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) {} }