diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml new file mode 100644 index 0000000..ab45441 --- /dev/null +++ b/.github/workflows/build.yml @@ -0,0 +1,72 @@ +name: Build + +# A debug APK on every push and pull request, kept as an artifact so a build can +# be installed without a local toolchain. +# +# Nothing here overrides the project's Gradle settings. That is deliberate: +# gradle.properties holds only portable configuration, and anything specific to a +# machine (JDK location, an aapt2 override for a non-x86_64 host, heap caps) lives +# in that machine's GRADLE_USER_HOME. A workflow that had to patch the repository's +# build settings before it could build would be a sign the repository was carrying +# someone's local setup. + +on: + push: + branches: ['**'] + pull_request: + workflow_dispatch: + +permissions: + contents: read + +concurrency: + # A newer push to the same ref makes the in-flight run redundant. + group: build-${{ github.ref }} + cancel-in-progress: true + +jobs: + build: + runs-on: ubuntu-latest + timeout-minutes: 45 + + steps: + - uses: actions/checkout@v4 + + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + # AGP 8.5 targets JDK 17; a newer JDK is not a safe substitute here + # because the Kotlin 2.0 compiler's bundled tooling cannot parse it. + java-version: '17' + distribution: 'temurin' + + - name: Set up Gradle + uses: gradle/actions/setup-gradle@v4 + + - name: Unit tests + # Runs before the APK so a failing test fails the job on its own evidence + # rather than being attributed to packaging. + run: ./gradlew --no-daemon testDebugUnitTest + + - name: Upload test report + # Wanted precisely when the previous step failed, which is why this is + # not conditional on success. + if: always() + uses: actions/upload-artifact@v4 + with: + name: unit-test-report + path: app/build/reports/tests/testDebugUnitTest + if-no-files-found: ignore + retention-days: 14 + + - name: Assemble debug APK + run: ./gradlew --no-daemon assembleDebug + + - name: Upload debug APK + uses: actions/upload-artifact@v4 + with: + name: processlens-debug-apk + path: app/build/outputs/apk/debug/*.apk + # An empty artifact that silently succeeds would be worse than a failure. + if-no-files-found: error + retention-days: 14 diff --git a/app/build.gradle.kts b/app/build.gradle.kts index be680d0..bcc5754 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -43,6 +43,16 @@ android { } } + // R8 and resource shrinking are on for release by default. An unminified, + // unobfuscated release APK is a shipping defect rather than a build-host + // preference, so the default does not bend to the host that happens to be + // building. The on-device ARM host this project is often built on cannot run + // R8 inside its ~1.8 GB of free RAM, so it opts out in GRADLE_USER_HOME with + // `processlens.minify=false`, which keeps the opt-out on that one machine and + // leaves every other build — CI included — minified and shrunk. + val minifyRelease = (project.findProperty("processlens.minify") as String?) + ?.toBooleanStrictOrNull() ?: true + buildTypes { debug { isMinifyEnabled = false @@ -50,11 +60,9 @@ android { versionNameSuffix = "-debug" } release { - // R8 stays off: full minification is memory-hungry on an on-device ARM - // build host with ~1.8 GB free. Keep-rules are retained so it can be - // switched on when building on a workstation. - isMinifyEnabled = false - isShrinkResources = false + // Resource shrinking requires code shrinking, so the two move together. + isMinifyEnabled = minifyRelease + isShrinkResources = minifyRelease proguardFiles( getDefaultProguardFile("proguard-android-optimize.txt"), "proguard-rules.pro" diff --git a/app/src/main/java/com/processlens/core/designsystem/ScreenScaffold.kt b/app/src/main/java/com/processlens/core/designsystem/ScreenScaffold.kt index e87b0c5..b343499 100644 --- a/app/src/main/java/com/processlens/core/designsystem/ScreenScaffold.kt +++ b/app/src/main/java/com/processlens/core/designsystem/ScreenScaffold.kt @@ -25,6 +25,8 @@ import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.foundation.verticalScroll import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.outlined.ArrowBack +import androidx.compose.material.icons.outlined.Info +import androidx.compose.material.icons.outlined.Refresh import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.Icon import androidx.compose.material3.MaterialTheme @@ -36,6 +38,7 @@ import androidx.compose.ui.draw.clip import androidx.compose.ui.graphics.vector.ImageVector import androidx.compose.ui.semantics.contentDescription import androidx.compose.ui.semantics.semantics +import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp @@ -198,6 +201,11 @@ fun ScreenList( * Used only for the genuinely unknown-duration first read. Refresh ticks do not show * one — a spinner appearing twice a second is worse than a value that updates in * place, and it hides the fact that the previous reading is still valid. + * + * A spinner is a claim that an answer is on its way, and it expires. Issue #1 was this + * composable left on screen indefinitely by a caller with no deadline, which is not a + * fault of the indicator but of showing it unconditionally: a screen that can reach + * "nothing yet" must bound how long it says so and then switch to [UnresolvedBlock]. */ @Composable fun LoadingBlock(modifier: Modifier = Modifier, label: String = "Reading system state") { @@ -223,6 +231,89 @@ fun LoadingBlock(modifier: Modifier = Modifier, label: String = "Reading system } } +/** + * What a screen shows when a bounded wait ran out (Sections 42, 48). + * + * The counterpart to [LoadingBlock], and the state this app did not have. Issue #1 + * presented as a hang, but the reason it survived to a release is that the symptom was + * indistinguishable from slowness: the header rendered, the title rendered, the one + * thing below them turned forever, and nothing in the UI was capable of saying "this is + * not coming". So every screen that can wait for a first reading now has somewhere to + * land — what was being waited for, why the waiting stopped, and a control that does + * something about it. + * + * [onRetry] has no default and there is no overload without it. A screen that can reach + * this state with no way out is the exact defect this exists to prevent, so the type + * system refuses to let a caller build one. + * + * [detail] goes through [ExpandableDetail] rather than into [explanation], keeping the + * Section 48 split intact: prose a user can act on first, the mechanical cause behind a + * disclosure for the reader who wants it. + */ +@Composable +fun UnresolvedBlock( + title: String, + explanation: String, + onRetry: () -> Unit, + modifier: Modifier = Modifier, + retryLabel: String = "Try again", + isRetrying: Boolean = false, + detail: String? = null, + icon: ImageVector = Icons.Outlined.Info, +) { + Column( + modifier = modifier + .fillMaxWidth() + .heightIn(min = 120.dp) + .padding(vertical = 12.dp) + .semantics { contentDescription = title }, + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.Center, + ) { + Icon( + icon, + contentDescription = null, + modifier = Modifier.size(Dimens.iconLarge), + tint = MaterialTheme.colorScheme.onSurfaceVariant, + ) + Spacer(Modifier.height(10.dp)) + Text( + title, + style = MaterialTheme.typography.titleSmall, + color = MaterialTheme.colorScheme.onSurface, + textAlign = TextAlign.Center, + ) + Spacer(Modifier.height(6.dp)) + Text( + explanation, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + ) + Spacer(Modifier.height(12.dp)) + if (isRetrying) { + // The control stays put and goes busy rather than being swapped for a bare + // spinner. Replacing it is what makes a retry that is working look like a + // retry that did nothing, which is the complaint this whole state answers. + Row(verticalAlignment = Alignment.CenterVertically) { + CircularProgressIndicator( + modifier = Modifier.size(14.dp), + strokeWidth = 2.dp, + color = ProcessLensTheme.accent.base, + ) + Spacer(Modifier.width(8.dp)) + ActionText(retryLabel, onClick = onRetry, enabled = false) + } + } else { + ActionText(retryLabel, onClick = onRetry, icon = Icons.Outlined.Refresh) + } + if (!detail.isNullOrBlank()) { + Spacer(Modifier.height(8.dp)) + ExpandableDetail(summary = "Technical details", detail = detail) + } + } +} + /** Fixed-height spacer used to separate sections without a divider. */ @Composable fun SectionGap(modifier: Modifier = Modifier) { diff --git a/app/src/main/java/com/processlens/core/system/CapabilityDetector.kt b/app/src/main/java/com/processlens/core/system/CapabilityDetector.kt index b87834a..1fbdf65 100644 --- a/app/src/main/java/com/processlens/core/system/CapabilityDetector.kt +++ b/app/src/main/java/com/processlens/core/system/CapabilityDetector.kt @@ -65,6 +65,14 @@ class CapabilityDetector @Inject constructor( */ suspend fun detect(): SystemCapabilities = withContext(io) { val api = Build.VERSION.SDK_INT + // A capability refresh is the one question "what can this app read *now*", + // and it is the only caller that ever asks. A denial is terminal only for + // the privileges that earned it, so any remembered refusal from a lower + // access level is dropped before the probes below re-attempt every read — + // otherwise a freshly granted Shizuku or root session would keep answering + // the normal-app denial it cached minutes earlier (see + // [ProcFsReader.invalidateRestrictions]). + procFs.invalidateRestrictions() val shizukuState = shizuku.state() val rootState = root.state() val access = when { diff --git a/app/src/main/java/com/processlens/core/system/CompositeSystemObserver.kt b/app/src/main/java/com/processlens/core/system/CompositeSystemObserver.kt index a0b71da..203a760 100644 --- a/app/src/main/java/com/processlens/core/system/CompositeSystemObserver.kt +++ b/app/src/main/java/com/processlens/core/system/CompositeSystemObserver.kt @@ -40,6 +40,7 @@ class CompositeSystemObserver @Inject constructor( private val standard: StandardAndroidObserver, private val shizuku: ShizukuShell, private val root: RootShell, + private val procFs: ProcFsReader, private val packages: PackageInspector, private val cpuSamplerFactory: CpuSamplerFactory, private val samplingPolicy: SamplingPolicy, @@ -80,15 +81,31 @@ class CompositeSystemObserver @Inject constructor( suspend fun applySettings(settings: UserSettings) = routeLock.withLock { samplingPolicy.apply(settings) val changed = allowShizuku != settings.shizukuEnabled || allowRoot != settings.rootEnabled + val rootDisabled = allowRoot && !settings.rootEnabled allowShizuku = settings.shizukuEnabled allowRoot = settings.rootEnabled - if (changed) active = null + if (changed) { + active = null + // A proven grant survives `invalidate`, by design (defect 4). But the + // user turning root support *off* is the one case where that proof must + // not be carried forward: re-enabling it later has to consult their + // superuser manager afresh rather than silently elevating on a grant + // recorded under the old setting. + if (rootDisabled) root.forgetGrant() + // The access level may now differ, so any refusal remembered under the + // old route is stale — the next capability refresh re-probes everything. + procFs.invalidateRestrictions() + } } /** Forces re-resolution — after a permission grant, or a manual refresh. */ suspend fun invalidate() = routeLock.withLock { active = null root.invalidate() + // A manual refresh is exactly the moment to re-ask what the current access + // level can read: a denial cached before the user granted Shizuku or root + // would otherwise outlive the grant that lifts it. + procFs.invalidateRestrictions() } /** diff --git a/app/src/main/java/com/processlens/core/system/ProcFsReader.kt b/app/src/main/java/com/processlens/core/system/ProcFsReader.kt index 1261f4c..27aa807 100644 --- a/app/src/main/java/com/processlens/core/system/ProcFsReader.kt +++ b/app/src/main/java/com/processlens/core/system/ProcFsReader.kt @@ -1,5 +1,7 @@ package com.processlens.core.system +import android.util.Log +import com.processlens.core.common.AccessLevel import com.processlens.core.common.DataSource import com.processlens.core.common.Observed import com.processlens.core.common.Precision @@ -21,10 +23,26 @@ import java.io.File * All calls are blocking file I/O and must run on an IO dispatcher — the caller * is responsible for that, which keeps this class a pure, testable adapter with * an injectable [root] for unit tests. + * + * ## Device-wide metrics are read once per refusal, not once per sample + * + * A refused read is already terminal here — [readFile] maps a denial to an + * [Observed.Restricted] and nothing in this class retries — but *terminal* and + * *remembered* are different things, and only the first was true. The sampler + * re-asks every one to ten seconds, so a device whose policy denies `/proc/stat` + * or `/sys/class/thermal` answered the same denial thousands of times per session + * and wrote an audit line for each. + * + * The six device-wide metrics below therefore go through [restrictions], which + * hands back the first refusal without touching the filesystem again. Per-process + * reads deliberately do **not**: their keys would be paths containing a PID, the + * map would grow without bound on a busy device, and — worse — a PID the kernel + * recycles would inherit the verdict passed on the process that used to own it. */ class ProcFsReader( private val root: File = File("/proc"), private val sysRoot: File = File("/sys"), + private val restrictions: RestrictionCache = RestrictionCache(), ) { /** @@ -42,8 +60,8 @@ class ProcFsReader( * Aggregate jiffy counters from the first line of `/proc/stat`. * Returns [Observed.Restricted] on the very common SELinux denial. */ - fun readSystemCpuTimes(): Observed = readFirstLine(File(root, "stat")).let { line -> - when (line) { + fun readSystemCpuTimes(): Observed = sessionMemo(METRIC_PROC_STAT) { + when (val line = readFirstLine(File(root, "stat"))) { is Observed.Value -> parseCpuLine(line.value) ?.let { Observed.of(it, DataSource.PROC_FS) } ?: Observed.Failed("Could not parse /proc/stat") @@ -52,10 +70,15 @@ class ProcFsReader( } } - /** Per-core lines (`cpu0`, `cpu1`, …) from `/proc/stat`. */ - fun readPerCoreCpuTimes(): Observed> { - val text = readFile(File(root, "stat")) - return when (text) { + /** + * Per-core lines (`cpu0`, `cpu1`, …) from `/proc/stat`. + * + * Shares [METRIC_PROC_STAT] with [readSystemCpuTimes]: the two parse different + * things out of the same file, and a policy that refuses the file refuses it for + * both, so one memo and one log line cover the pair. + */ + fun readPerCoreCpuTimes(): Observed> = sessionMemo(METRIC_PROC_STAT) { + when (val text = readFile(File(root, "stat"))) { is Observed.Value -> { val cores = text.value.lineSequence() .filter { it.startsWith("cpu") && it.length > 3 && it[3].isDigit() } @@ -96,8 +119,8 @@ class ProcFsReader( ) } - fun readLoadAverage(): Observed { - return when (val line = readFirstLine(File(root, "loadavg"))) { + fun readLoadAverage(): Observed = sessionMemo(METRIC_LOADAVG) { + when (val line = readFirstLine(File(root, "loadavg"))) { is Observed.Value -> { val p = line.value.trim().split(WHITESPACE) val one = p.getOrNull(0)?.toFloatOrNull() @@ -117,8 +140,8 @@ class ProcFsReader( // ------------------------------------------------------------------- memory /** Parses `/proc/meminfo` into its KiB key/value pairs. */ - fun readMemInfo(): Observed> { - return when (val text = readFile(File(root, "meminfo"))) { + fun readMemInfo(): Observed> = sessionMemo(METRIC_MEMINFO) { + when (val text = readFile(File(root, "meminfo"))) { is Observed.Value -> { val map = HashMap(64) text.value.lineSequence().forEach { line -> @@ -147,9 +170,15 @@ class ProcFsReader( * PIDs currently visible in `/proc`. Under `hidepid=2` (API 29+) this is just * our own process; the caller compares the result against the expected count * to decide whether a full process list is possible at all. + * + * Note what is and is not memoised: a refusal to list `/proc` at all is a + * device-wide fact worth remembering, while a *short* list is a successful read + * and is taken again every sample, because processes start and stop. */ - fun listVisiblePids(): Observed> = Observed.catching(DataSource.PROC_FS) { - root.list()?.mapNotNull { it.toIntOrNull() }?.sorted() ?: emptyList() + fun listVisiblePids(): Observed> = sessionMemo(METRIC_PROC_LISTING) { + Observed.catching(DataSource.PROC_FS) { + root.list()?.mapNotNull { it.toIntOrNull() }?.sorted() ?: emptyList() + } } /** @@ -173,6 +202,26 @@ class ProcFsReader( companion object { private val WHITESPACE = Regex("\\s+") + private const val TAG = "ProcessLens" + + /** + * Metric names for the session memo (see [RestrictionCache]). + * + * Deliberately the canonical device paths rather than the injected [root] and + * [sysRoot]: a test pointing the reader at a temporary directory memoises and + * logs under the same names a phone does, and the log line stays readable by + * someone who has only the bug report in front of them. + * + * One key per *metric*, which is not always one key per file — the two + * `/proc/stat` readers share theirs — and never a key containing a PID. + */ + const val METRIC_PROC_STAT = "/proc/stat" + const val METRIC_LOADAVG = "/proc/loadavg" + const val METRIC_MEMINFO = "/proc/meminfo" + const val METRIC_PROC_LISTING = "/proc process listing" + const val METRIC_CPUFREQ = "/sys cpufreq nodes" + const val METRIC_THERMAL = "/sys thermal zones" + /** * Parses one `/proc//stat` line. Exposed on the companion because the * elevated observer reads the same text through a shell, and two copies of @@ -280,8 +329,16 @@ class ProcFsReader( * Per-core scaling frequencies from `/sys/devices/system/cpu/cpuN/cpufreq`. * An offline core has no readable `scaling_cur_freq`, which is reported as * `isOnline = false` rather than 0 Hz. + * + * Three files per core, so an eight-core device that hides cpufreq was paying + * twenty-four refused opens every sample. There is no public API for core + * frequency on any API level, so sysfs is the only path — which makes + * remembering the refusal the only saving available. */ - fun readCoreFrequencies(coreCount: Int): Observed> { + fun readCoreFrequencies(coreCount: Int): Observed> = + sessionMemo(METRIC_CPUFREQ) { scanCoreFrequencies(coreCount) } + + private fun scanCoreFrequencies(coreCount: Int): Observed> { val out = ArrayList(coreCount) var anyReadable = false for (i in 0 until coreCount) { @@ -309,11 +366,32 @@ class ProcFsReader( * First plausible thermal zone reading. Zone naming is entirely OEM-specific, * so zones are filtered by type name and the value is treated as milli-degrees * when it is implausibly large for deci-degrees. + * + * This is the walk the bug report caught denying itself fifty minutes of audit + * lines, and it is the one metric here with a public alternative — except that + * `PowerManager`'s thermal API reports a throttling severity rather than a + * temperature, so it cannot answer this question. [StandardAndroidObserver] + * consults it for what it *can* honestly say when this comes back refused. */ - fun readCpuTemperature(): Observed { - val zones = File(sysRoot, "class/thermal").listFiles() + fun readCpuTemperature(): Observed = sessionMemo(METRIC_THERMAL) { scanThermalZones() } + + private fun scanThermalZones(): Observed { + val thermalRoot = File(sysRoot, "class/thermal") + val zones = thermalRoot.listFiles() ?.filter { it.name.startsWith("thermal_zone") } - ?: return Observed.notPresent("No thermal zones exposed") + ?: return if (thermalRoot.exists()) { + // `listFiles()` answers null for "there is no such directory" and for + // "you may not look in it" alike. Where the directory is visible but + // unlistable the second is what happened, and that is a restriction an + // elevated shell lifts — reporting it as "this device has no thermal + // zones" would describe the wrong device. + Observed.platform( + "${thermalRoot.path} exists but cannot be listed by this app", + AccessLevel.SHIZUKU, + ) + } else { + Observed.notPresent("No thermal zones exposed") + } for (zone in zones) { val type = readFirstLine(File(zone, "type")).let { @@ -338,6 +416,43 @@ class ProcFsReader( return Observed.notPresent("No CPU thermal zone could be read") } + // -------------------------------------------------------- terminal refusals + + /** + * Runs [read] unless [key] is already known to be refused, and spends that + * metric's single log line the first time it is. + * + * The log line is composed here rather than relayed: `detail` is text this class + * wrote, so no kernel audit string, shell output or raw `errno` message reaches + * logcat through it (Section 48). `Log.i` rather than `Log.w` because a + * restriction is documented platform behaviour, not a fault — the same + * distinction the UI draws with a padlock instead of a warning triangle. + */ + private fun sessionMemo(key: String, read: () -> Observed): Observed { + val outcome = restrictions.attempt(key, read) + if (outcome is Observed.Restricted && restrictions.shouldAnnounce(key)) { + Log.i( + TAG, + "$key is not readable at this access level and will not be re-read " + + "this session: ${outcome.detail}", + ) + } + return outcome + } + + /** + * Forgets every remembered refusal. + * + * Must be called whenever the app's access level may have changed, because a + * denial is permanent only for the privileges that earned it: the same + * `/proc/stat` that is refused to a normal app is read freely through a root or + * Shizuku shell (Sections 27, 28). [StandardAndroidObserver.getCapabilities] + * calls this before it probes, which covers both the first evaluation and every + * re-evaluation forced by a grant, because a capability refresh is the only + * thing that ever asks what this app can read *now*. + */ + fun invalidateRestrictions() = restrictions.clear() + // -------------------------------------------------------------------- utils /** diff --git a/app/src/main/java/com/processlens/core/system/ProcessRunner.kt b/app/src/main/java/com/processlens/core/system/ProcessRunner.kt new file mode 100644 index 0000000..be27f92 --- /dev/null +++ b/app/src/main/java/com/processlens/core/system/ProcessRunner.kt @@ -0,0 +1,448 @@ +package com.processlens.core.system + +import com.processlens.core.common.AccessLevel +import com.processlens.core.common.IoDispatcher +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineName +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.launch +import kotlinx.coroutines.runInterruptible +import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull +import java.io.ByteArrayOutputStream +import java.io.Closeable +import java.io.InputStream +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Starts the operating-system process for an argv. + * + * The single seam in this file that touches the OS, and the reason [ProcessRunner] + * is testable without a rooted device: a unit test supplies a starter returning a + * fake [Process] — one that denies, one that never exits, one that succeeds — and + * drives every path below with no `su` binary, no Shizuku binder and no Android. + * + * `null` and a thrown exception are different answers and are reported + * differently. A throw means the shell could not be invoked at all (`su` is not + * on PATH, the binder died); `null` means the shell exists but declined to hand + * back a process, which is what Shizuku's reflective `newProcess` does when its + * hidden API has moved. + */ +fun interface ProcessStarter { + fun start(argv: List): Process? +} + +/** + * Runs one external process to completion under a real deadline (requirement 4 of + * the issue #1 spec, Section 28). + * + * Both shells used to carry the identical defect this class exists to remove: + * they read stdout to EOF, *then* stderr to EOF, and only *then* consulted the + * timeout. ShizukuShell's comment there was half right — it understood that the + * pipes must be drained before the wait — and wrong about draining them one after + * the other. Two things followed, and both were reachable in the field: + * + * - The timeout was unreachable. An `su` parked on an unanswered Magisk prompt + * blocked in the first `read()` forever, not for the nominal ten seconds. + * - It was a genuine deadlock, not merely a slow path. A child that fills the + * ~64 KiB stderr pipe blocks in `write()`, so it never closes stdout, so the + * stdout read never reaches EOF, so stderr is never drained. Each side waits + * for the other. + * + * ### Why the drains are not children of the caller + * + * The trap underneath all of this: **a blocking `InputStream.read()` does not + * observe coroutine cancellation.** `withTimeout` cannot interrupt a thread parked + * in `read()`; it would cancel the coroutine, then suspend forever waiting for an + * uncancellable child to finish, and on `Dispatchers.IO` enough parked threads is + * a failure in its own right. The only thing that releases that reader is closing + * the pipe, which [Process.destroy] does by killing the writer. + * + * So the deadline here is not implemented by abandoning the readers. It is + * implemented by *destroying the process*, which makes the readers finish. The + * drains therefore run in a scope this function owns rather than in the caller's + * job: a caller who gives up must not be made to wait for a thread in `read()`, + * and [ProcessSession] guarantees the destroy-and-close that releases it happens + * on every exit path — success, timeout, denial, throw, cancellation — exactly + * once. + * + * Output is captured into a [StreamSink] rather than returned by the drain + * coroutine, so bytes the child had already produced survive even when the drain + * itself is abandoned. Reporting "" there would be discarding evidence, which is + * the opposite of what Section 42 asks for. + */ +@Singleton +class ProcessRunner @Inject constructor( + @IoDispatcher private val io: CoroutineDispatcher, +) { + + /** + * Starts [argv], waits at most [timeoutMillis] for it, and returns whatever it + * said. + * + * Always runs on the injected IO dispatcher, so a caller cannot accidentally + * put a blocking `su` on the main thread or on the computation pool + * (Section 43). [starter] is the last parameter so call sites read as a + * trailing lambda; it is invoked exactly once and inside the IO context. + * + * Never throws for a process-level failure — a refusal, a deadline or an + * absent binary all come back as a non-zero [ShellResult] — because every + * caller's answer to all three is the same: carry on without the shell. + * Cancellation of the caller is the one thing that does propagate. + */ + suspend fun execute( + argv: List, + accessLevel: AccessLevel, + timeoutMillis: Long = ElevatedShell.DEFAULT_TIMEOUT, + maxOutputBytes: Int = MAX_OUTPUT_BYTES, + maxErrorBytes: Int = MAX_ERROR_BYTES, + starter: ProcessStarter, + ): ShellResult { + if (argv.isEmpty()) return ShellResult.failure("Empty command", accessLevel) + + return withContext(io) { + val process = try { + starter.start(argv) + } catch (t: Throwable) { + return@withContext ShellResult.failure(describe(t), accessLevel) + } ?: return@withContext ShellResult.failure( + "This shell could not start a process on this device.", + accessLevel, + ) + + val session = ProcessSession(process) + val stdout = StreamSink(maxOutputBytes) + val stderr = StreamSink(maxErrorBytes) + + // Owned, parentless scope. See the class KDoc: these two coroutines can + // park in an uncancellable read(), so they must not be able to hold the + // caller's job open. + val drains = CoroutineScope(io + SupervisorJob() + CoroutineName(DRAIN_SCOPE_NAME)) + try { + // Before anything else, and not just hygiene: none of the + // DiagnosticCommands read stdin, but `su` itself may, and a child + // waiting on a pipe nobody will ever write to is a hang with no + // deadline attached. Closing it hands over EOF immediately. + session.closeStdin() + + val stdoutDrain = drains.launch { stdout.drain(process.inputStream) } + val stderrDrain = drains.launch { stderr.drain(process.errorStream) } + + // Concurrent with both drains. runInterruptible is what makes the + // outer deadline real: waitForTimeout blocks, and without thread + // interruption withTimeoutOrNull could not return until it chose to. + val finished = withTimeoutOrNull(timeoutMillis) { + runInterruptible { process.waitForTimeout(timeoutMillis) } + } ?: false + + if (!finished) { + // This, not the cancellation, is the timeout. Killing the child + // closes the write ends, which is the only thing that lets the + // two readers above unwind. + session.tearDown() + } + + // On the happy path the child has already exited, so EOF is coming + // and the drains are allowed the full command budget to finish + // reading a multi-megabyte dump. On the timeout path they have only + // a short grace window, because by then the pipes are already closed + // and anything still blocked is a Process implementation whose + // destroy() did not take — Shizuku's is a remote binder proxy. + val drainBudget = if (finished) timeoutMillis else DRAIN_GRACE_MILLIS + joinWithin(drainBudget, stdoutDrain, stderrDrain) + + val out = stdout.text() + val err = stderr.text() + val code = session.exitCodeOrNull() + + when { + !finished -> ShellResult( + EXIT_TIMED_OUT, + out, + combine("Command timed out after $timeoutMillis ms", err), + accessLevel, + ) + // waitForTimeout said it finished but the process will not report + // a status. Trusting either answer would be guessing, so this is + // reported as the same kind of non-result as a deadline. + code == null -> ShellResult( + EXIT_TIMED_OUT, + out, + combine("The shell did not report an exit status.", err), + accessLevel, + ) + else -> ShellResult(code, out, err, accessLevel) + } + } finally { + // Every exit path, cancellation included. Both calls are + // non-suspending, which is why they still run when the caller's job + // is already cancelled. + session.tearDown() + drains.cancel() + } + } + } + + companion object { + /** + * Reported when the process could not be started at all. The same value + * [ShellResult.failure] uses, which is where start failures come from. + */ + const val EXIT_NOT_STARTED = -1 + + /** + * Reported when the child never exited on its own and was destroyed, or + * exited without a readable status. + * + * Distinct from [EXIT_NOT_STARTED] so a caller can tell "the superuser + * prompt went unanswered" from "there is no `su` here" — [RootShell] needs + * exactly that distinction to decide whether it has learned anything. Both + * are negative, which a real wait status (0..255) can never be. + */ + const val EXIT_TIMED_OUT = -2 + + /** + * Caps on captured output. `dumpsys batterystats` can exceed 10 MB on a + * device that has been up for weeks; the parsers only need the header + * sections, and an unbounded read would be a genuine OOM risk on the + * low-memory devices this app targets. + */ + const val MAX_OUTPUT_BYTES = 4 * 1024 * 1024 + const val MAX_ERROR_BYTES = 64 * 1024 + + /** + * How long a drain is given *after* the process has been destroyed. + * + * Short on purpose. By this point the child is killed and all three pipes + * are closed, so a reader that is still blocked is pathological rather than + * busy, and the sink already holds whatever it captured. Waiting longer + * would only move the hang from the shell into the caller. + */ + const val DRAIN_GRACE_MILLIS = 500L + } +} + +// The mechanical constants live at file scope rather than in the companion because +// StreamSink and waitForTimeout below need them, and a private companion member is +// visible only inside its own class — not to the rest of the file. + +private const val DRAIN_SCOPE_NAME = "process-drain" + +/** Poll interval for the [waitForTimeout] fallback. */ +private const val POLL_INTERVAL_MILLIS = 50L + +private const val CHUNK_BYTES = 16 * 1024 +private const val INITIAL_BUFFER_BYTES = 64 * 1024 + +/** + * Owns one [Process] and the three pipes hanging off it. + * + * Exists so that "destroy the process and close the streams exactly once, on + * every exit path" is a property of one small class instead of a rule repeated in + * five `catch` blocks. The guards are atomic because teardown is reachable from + * the timeout branch and from `finally` on the same run. + * + * Teardown here is not tidying. [Process.destroy] is the mechanism by which the + * deadline in [ProcessRunner.execute] takes effect at all: killing the writer is + * what gives the blocked readers their EOF. + */ +private class ProcessSession(private val process: Process) { + + private val stdinClosed = AtomicBoolean(false) + private val tornDown = AtomicBoolean(false) + + /** Closes the parent's write end, giving an `su` that reads stdin its EOF. */ + fun closeStdin() { + if (stdinClosed.compareAndSet(false, true)) { + closeQuietly(process.outputStream) + } + } + + /** + * Kills the child, then closes all three pipes. Idempotent. + * + * Destroy comes first and the order matters: on Linux, closing a descriptor + * another thread is already blocked reading does not wake that thread, whereas + * killing the writer does. The closes that follow are descriptor hygiene, not + * the release mechanism. + */ + fun tearDown() { + if (!tornDown.compareAndSet(false, true)) return + try { + process.destroy() + } catch (ignored: Throwable) { + // A remote proxy may refuse to be destroyed. Nothing further to try. + } + closeStdin() + closeQuietly(process.inputStream) + closeQuietly(process.errorStream) + } + + /** The real exit status, or null when the process has not actually exited. */ + fun exitCodeOrNull(): Int? = try { + process.exitValue() + } catch (notYet: IllegalThreadStateException) { + null + } catch (t: Throwable) { + // A Shizuku proxy that cannot answer. Null means "unknown", never zero: + // a fabricated success here would be read downstream as a working command. + null + } +} + +/** + * A bounded accumulator for one pipe. + * + * Deliberately separate from the coroutine that fills it. On the timeout path the + * reader may still be parked when the runner stops waiting for it, and the bytes + * it already captured are real output — `ps` output that arrived before the hang + * is as true as `ps` output that arrived before an exit. Keeping the buffer + * outside the coroutine is what lets the runner read a partial capture instead of + * reporting nothing (Section 42). + * + * [ByteArrayOutputStream]'s own methods are synchronised, and only [drain] writes + * [captured], so [truncated] is the single field that genuinely crosses threads. + */ +private class StreamSink(private val maxBytes: Int) { + + private val buffer = ByteArrayOutputStream(minOf(maxBytes, INITIAL_BUFFER_BYTES)) + + /** Touched only by the draining coroutine. */ + private var captured = 0 + + @Volatile + private var truncated = false + + /** Reads to EOF, to the byte cap, or until the pipe is closed underneath us. */ + fun drain(stream: InputStream) { + val chunk = ByteArray(CHUNK_BYTES) + try { + while (true) { + val read = stream.read(chunk) + if (read <= 0) break + val allowed = minOf(read, maxBytes - captured) + if (allowed > 0) { + buffer.write(chunk, 0, allowed) + captured += allowed + } + if (captured >= maxBytes) { + truncated = true + break + } + } + } catch (t: Throwable) { + // The expected ending on the timeout path, not an anomaly: the runner + // destroyed the process and closed this pipe precisely to land here. + // Whatever arrived first is kept. + } + } + + /** + * What was captured. Truncation is marked inline so a parser cannot mistake a + * cut-off dump for a complete one. + */ + fun text(): String = try { + val body = buffer.toString("UTF-8") + if (truncated) body + "\n[output truncated at $maxBytes bytes]\n" else body + } catch (t: Throwable) { + "" + } +} + +/** + * Reads a stream to text with a hard byte cap, never throwing. + * + * Kept as a top-level function in this package, as it was in ShizukuShell.kt, so + * both shells and anything added beside them share one implementation. Two + * behaviours changed when it moved here, both deliberate: + * + * - It no longer closes the stream. [ProcessRunner] owns the pipes and closes + * each exactly once; a second close from here would make "exactly once" a claim + * rather than a fact. + * - A failure part-way through returns the bytes already captured instead of "", + * for the reason given on [StreamSink]. + * + * This is the synchronous form, for a caller holding a stream and nothing else. + * [ProcessRunner] does not use it: it needs the sink itself so it can read a + * partial capture from a drain that never finished. + */ +internal fun InputStream.readAllTextSafely(maxBytes: Int): String = + StreamSink(maxBytes).let { sink -> + sink.drain(this) + sink.text() + } + +/** + * `Process.waitFor(timeout, unit)` is API 26+, which matches this app's minSdk, + * but the `Process` returned by Shizuku is a remote proxy whose implementation may + * not honour it. Falling back to polling `exitValue()` keeps a hung command from + * blocking the caller forever. + * + * Never throws, so a caller does not have to distinguish "not finished" from + * "could not tell" — both answers lead to the same place, destroying the process. + */ +internal fun Process.waitForTimeout(timeoutMillis: Long): Boolean { + try { + return waitFor(timeoutMillis, TimeUnit.MILLISECONDS) + } catch (interrupted: InterruptedException) { + // `runInterruptible` interrupts this thread when the coroutine deadline + // fires. Reported as "not finished", which is true, and the flag is + // restored so the dispatcher's own bookkeeping is not left lying — this + // runs on a pooled IO thread that will be handed to someone else. + Thread.currentThread().interrupt() + return false + } catch (t: Throwable) { + val deadline = System.currentTimeMillis() + timeoutMillis + while (System.currentTimeMillis() < deadline) { + try { + exitValue() + return true + } catch (notYet: IllegalThreadStateException) { + try { + Thread.sleep(POLL_INTERVAL_MILLIS) + } catch (ie: InterruptedException) { + Thread.currentThread().interrupt() + return false + } + } catch (t2: Throwable) { + // The proxy cannot answer either. "Not finished" sends the caller + // down the destroy path rather than trusting an unknown status. + return false + } + } + return false + } +} + +/** + * Joins [jobs] but never for longer than [millis]. + * + * `join()` on a coroutine blocked in `read()` does not return when the job is + * cancelled — the job stays in its cancelling state until the body actually comes + * back — so the bound is not optional. + */ +private suspend fun joinWithin(millis: Long, vararg jobs: Job) { + withTimeoutOrNull(millis) { + jobs.forEach { it.join() } + } +} + +private fun closeQuietly(closeable: Closeable?) { + try { + closeable?.close() + } catch (ignored: Throwable) { + } +} + +/** Matches the detail wording the shells already use for a thrown failure. */ +private fun describe(t: Throwable): String = t.message ?: t::class.java.simpleName + +/** Keeps a drained stderr alongside an explanation instead of replacing it. */ +private fun combine(detail: String, stderr: String): String = + listOf(detail, stderr.trim()).filter { it.isNotBlank() }.joinToString("\n") diff --git a/app/src/main/java/com/processlens/core/system/RestrictionCache.kt b/app/src/main/java/com/processlens/core/system/RestrictionCache.kt new file mode 100644 index 0000000..88ba4de --- /dev/null +++ b/app/src/main/java/com/processlens/core/system/RestrictionCache.kt @@ -0,0 +1,134 @@ +package com.processlens.core.system + +import com.processlens.core.common.Observed +import com.processlens.core.common.RestrictionReason +import java.util.concurrent.ConcurrentHashMap + +/** + * Remembers which reads the sandbox has already refused, so each one is attempted + * once per session instead of once per sample. + * + * A refusal is an answer. When SELinux policy denies `/sys/class/thermal`, the + * kernel gives the same answer to the next identical read and to the ten thousand + * after it — but the sampler asks again every one to ten seconds (Section 6), and + * every attempt costs a syscall, an audit record, and a line of `avc: denied` in + * the device log. One bug report against this app carried fifty minutes of them. + * Nothing was learned after the first. + * + * So the first refusal is kept and later callers get it back without touching the + * filesystem. Section 43 — ProcessLens must not become the problem it investigates + * — is why; Section 42 is what makes it safe, because what comes back out is the + * same [Observed.Restricted] the real read produced, carrying the same reason to + * the same "Not available" block. Nothing is substituted and nothing is invented. + * + * ## What is deliberately not remembered + * + * [Observed.Failed] never is. A parse that failed, or a file that happened to be + * empty, is a fault rather than a policy: `/proc/loadavg` yielding nothing once + * says nothing about the next read, and memoising it would turn a single bad + * sample into a permanently blank screen — the opposite failure to the one this + * class exists to fix. + * + * Nor is every restriction. Only the reasons that describe the *platform* are + * terminal. [RestrictionReason.PERMISSION_REQUIRED] is one dialog away from being + * false, and [RestrictionReason.SAMPLING_DISABLED] is the user's own switch, which + * they can flip back between two ticks (Section 40); treating either as settled + * would leave a value unavailable after the user had already fixed it. + * + * ## Why it must be clearable + * + * A denial is permanent only at the access level that earned it. `/proc/stat` is + * refused to a normal app and read freely through a root shell, so the moment the + * user grants root or Shizuku (Sections 27, 28) every memo here describes a device + * the app no longer is. [clear] exists for that transition, and `ProcFsReader` + * exposes it so the capability re-evaluation — the one moment the app stops + * assuming and asks what it can read *now* — can drop the lot before it probes. + * + * Plain Kotlin on purpose: no `android.*`, no context, nothing that needs a device. + * This is session state that decides whether a screen says "Not available", it is + * cheap to get subtly wrong, and it is worth being able to test on the JVM. + */ +class RestrictionCache { + + /** + * Keys are logical metric names, supplied by the caller rather than derived + * from the paths it read, so a unit test with a temporary `/proc` root memoises + * and reports under exactly the names a real device does. + */ + private val refusals = ConcurrentHashMap() + + /** Keys whose one permitted log line has been spent. See [shouldAnnounce]. */ + private val announced = ConcurrentHashMap() + + /** + * Returns the remembered refusal for [key], or performs [read] and remembers it + * if it turns out to be a terminal one. + * + * [read] is not invoked at all once a refusal is remembered, which is the whole + * point: the memo has to remove the filesystem access, not merely hide its + * result. Thread-safe but not atomic — two samplers racing on a cold key both + * read, and both arrive at the same answer, which is cheaper than holding a lock + * across file I/O. + */ + fun attempt(key: String, read: () -> Observed): Observed { + refusals[key]?.let { return it } + val outcome = read() + if (outcome is Observed.Restricted && outcome.isTerminalForThisSession()) { + refusals[key] = outcome + } + return outcome + } + + /** The remembered refusal for [key], or null if it has not been refused yet. */ + fun refusalFor(key: String): Observed.Restricted? = refusals[key] + + fun isRefused(key: String): Boolean = refusals.containsKey(key) + + /** Metric names currently known to be refused. Order is not defined. */ + val refusedKeys: Set get() = refusals.keys.toSet() + + /** + * True the first time it is asked about [key] and false ever afterwards, which + * is how "at most one log line per metric per session" is enforced in one place + * rather than at each of the call sites that might want to log. + * + * Note that [clear] does not reset this. The budget is per session, not per + * access level: a user who grants root and has the same path refused again does + * not need to be told twice, and the line was never for them anyway — it is for + * whoever reads the log of a bug report. + */ + fun shouldAnnounce(key: String): Boolean = announced.putIfAbsent(key, true) == null + + /** + * Forgets every refusal, so the next read of each metric reaches the filesystem + * again. + * + * Called when the app's access level may have changed. Clearing too eagerly + * merely costs the reads this class was added to avoid; not clearing at all + * would mean a user who granted root kept seeing "Not available" for everything + * ProcessLens had given up on beforehand, which is a worse bug than the one + * being fixed. + */ + fun clear() { + refusals.clear() + } + + /** + * Whether this restriction will still be true at the next tick. + * + * Exhaustive on purpose: a new [RestrictionReason] should fail to compile here + * and force an answer, because defaulting a new reason to "terminal" would + * silently memoise something revocable. + */ + private fun Observed.Restricted.isTerminalForThisSession(): Boolean = when (reason) { + RestrictionReason.PLATFORM_RESTRICTED, + RestrictionReason.NOT_PRESENT_ON_DEVICE, + RestrictionReason.NOT_SUPPORTED_ON_API_LEVEL, + RestrictionReason.REQUIRES_ELEVATED_ACCESS, + -> true + + RestrictionReason.PERMISSION_REQUIRED, + RestrictionReason.SAMPLING_DISABLED, + -> false + } +} diff --git a/app/src/main/java/com/processlens/core/system/RootShell.kt b/app/src/main/java/com/processlens/core/system/RootShell.kt index 96a0ce6..97ba5c4 100644 --- a/app/src/main/java/com/processlens/core/system/RootShell.kt +++ b/app/src/main/java/com/processlens/core/system/RootShell.kt @@ -1,11 +1,15 @@ package com.processlens.core.system import com.processlens.core.common.AccessLevel +import com.processlens.core.common.DataSource import com.processlens.core.common.IoDispatcher +import com.processlens.core.common.Observed +import com.processlens.core.common.RestrictionReason import com.processlens.domain.model.RootState import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.withContext import java.io.File +import java.util.concurrent.atomic.AtomicReference import javax.inject.Inject import javax.inject.Singleton @@ -20,16 +24,55 @@ import javax.inject.Singleton * * The probe result is cached: re-running `su` on every capability refresh would * spam the superuser log and, on some managers, re-prompt the user. + * + * ### What defect 4 was + * + * That cache used to be one nullable [RootState] field and [invalidate] used to + * null it. Routing then wiped a grant it had just been handed: + * `AccessViewModel.probeRoot()` set GRANTED, its own `refreshCapabilitiesOnly()` + * called `invalidateAccess()` → `CompositeSystemObserver.invalidate()` → + * `invalidate()` here, and by the time `resolve()` looked, the field was null + * again. `state()` answered BINARY_PRESENT, whose `isUsable` is false, and the + * standard observer stayed in place — so root never engaged no matter how many + * times the user granted it. + * + * The cause was conflating two different nothings behind one null: "we have never + * checked" and "forget what we learned". Those are now distinct, and so is a + * refusal — see [probed]. */ @Singleton class RootShell @Inject constructor( @IoDispatcher private val io: CoroutineDispatcher, + private val runner: ProcessRunner, ) : ElevatedShell { override val accessLevel: AccessLevel = AccessLevel.ROOT - @Volatile - private var probedState: RootState? = null + /** + * What the last real `su` execution established, or null when none has run. + * + * Modelled as [Observed] rather than as a nullable [RootState] because the + * states routing has to tell apart are exactly the ones [Observed] already + * exists to express, and because collapsing them is what caused defect 4: + * + * - `null` — never checked. A blank slate, and the only thing [invalidate] + * is allowed to produce. + * - [Observed.Value] — `su` ran and reported uid 0. The proof is recorded as + * what it literally is: a reading whose [DataSource] is a root shell. This + * is the one state that survives [invalidate]. + * - [Observed.Restricted] — the superuser manager answered, and the answer + * was no. A decision about access, not a fault, which is what `Restricted` + * means everywhere else in this codebase. + * - [Observed.Failed] — the probe itself broke: `su` never answered inside + * the deadline, or could not be started. + * + * [AtomicReference] rather than `@Volatile` so [invalidate] can decide and + * write as one step. The window is small — a probe and the invalidation that + * follows it run sequentially in `AccessViewModel` — but it is the exact + * window the defect lived in, and a compare-and-set closes it rather than + * narrowing it. + */ + private val probed = AtomicReference?>(null) /** Cheap, synchronous: does an `su` binary exist anywhere standard? */ fun hasBinary(): Boolean = SU_PATHS.any { path -> @@ -40,38 +83,107 @@ class RootShell @Inject constructor( } } - /** Cached state. Returns [RootState.BINARY_PRESENT] until [probe] has run. */ - fun state(): RootState = probedState ?: if (hasBinary()) { - RootState.BINARY_PRESENT - } else { - RootState.UNAVAILABLE + /** + * Cached state. Returns [RootState.BINARY_PRESENT] until [probe] has run. + * + * Consults [probed] first and only falls back to [hasBinary], so the common + * case is a field read. The fallback is a handful of `stat` calls and never a + * process spawn — requirement 5 of the issue spec depends on that, because + * `CompositeSystemObserver.resolve()` reaches this method on the path that + * produces the app's first frame. + */ + fun state(): RootState = when (val proof = probed.get()) { + null -> if (hasBinary()) RootState.BINARY_PRESENT else RootState.UNAVAILABLE + is Observed.Value -> proof.value + is Observed.Restricted -> when (proof.reason) { + RestrictionReason.NOT_PRESENT_ON_DEVICE -> RootState.UNAVAILABLE + else -> RootState.DENIED + } + // A probe that never got an answer is reported as a refusal when the + // binary is there. Not a guess: the Access screen's own wording for + // DENIED is "denied or timed out", so the two already share a bucket in + // the only place a user reads them. + is Observed.Failed -> if (hasBinary()) RootState.DENIED else RootState.UNAVAILABLE } /** * Actually attempts elevation. This may show the superuser prompt, so it is * only called when the user has enabled root support in Settings — never * speculatively at startup. + * + * Always re-runs `su`, including over a cached grant. That is how a revoked + * grant becomes discoverable: tapping "Probe root" again is a genuine + * re-check, and a manager that has since withdrawn the grant produces a + * refusal here that overwrites the proof. */ suspend fun probe(): RootState = withContext(io) { if (!hasBinary()) { - probedState = RootState.UNAVAILABLE + // Cached rather than recomputed, so the route resolution that follows + // does not repeat the filesystem walk. + probed.set(Observed.notPresent("No su binary is present on this device.")) return@withContext RootState.UNAVAILABLE } - val result = execute(DiagnosticCommand.Probe.argv, timeoutMillis = 10_000L) + + val result = execute(DiagnosticCommand.Probe.argv, PROBE_TIMEOUT_MILLIS) val granted = result.isSuccess && result.stdout.contains("uid=0") - val newState = when { - granted -> RootState.GRANTED - // A manager that denies returns non-zero quickly, often with nothing - // on stdout. Distinguish that from "no binary at all". - else -> RootState.DENIED - } - probedState = newState - newState + + probed.set( + when { + granted -> Observed.of(RootState.GRANTED, DataSource.SHELL_ROOT) + + result.exitCode == ProcessRunner.EXIT_TIMED_OUT -> Observed.Failed( + "su did not answer within $PROBE_TIMEOUT_MILLIS ms.", + result.stderr.trim().take(DETAIL_LIMIT).takeIf { it.isNotBlank() }, + ) + + // A manager that denies returns non-zero quickly, often with + // nothing on stdout. Distinguish that from "no binary at all". + else -> Observed.Restricted( + RestrictionReason.REQUIRES_ELEVATED_ACCESS, + AccessLevel.ROOT, + result.stderr.trim().take(DETAIL_LIMIT).takeIf { it.isNotBlank() } + ?: "su exited with ${result.exitCode} without reporting uid 0.", + ) + }, + ) + state() } - /** Forgets the cached probe so a settings change can re-evaluate. */ + /** + * Forgets stale conclusions so a routing change can re-evaluate — and keeps a + * grant that a real `su` execution proved. + * + * This is the fix for defect 4 and the asymmetry is the whole point. A + * negative is worth forgetting: the user may have granted root in their + * manager since, and `AccessViewModel` already states the principle — "a + * stale 'denied' is as misleading as a stale 'granted'". A proven positive is + * not, because routing is re-evaluated immediately after a probe and dropping + * the proof mid-sequence is precisely what kept root from ever engaging. + * + * Keeping it does not make a revoked grant permanent. Two things still clear + * it: [probe], which always re-runs `su`, and [forgetGrant], which routing + * calls when the user withdraws root support. + */ fun invalidate() { - probedState = null + val current = probed.get() + if (current is Observed.Value) return + // Compare-and-set, not set: if a probe proved a grant between the read + // above and here, that proof is newer than this decision and wins. + probed.compareAndSet(current, null) + } + + /** + * Forgets everything, a proven grant included, so the next [probe] has to + * earn it again. + * + * Separate from [invalidate] because it is a different question. Routing asks + * "has anything about the route changed?"; this answers "the user has + * withdrawn root support, so nothing we proved under the old settings should + * be carried forward". Resurrecting an old grant when root is switched back + * on would elevate without the user's manager being consulted again. + */ + fun forgetGrant() { + probed.set(null) } override suspend fun isAvailable(): Boolean = state().isUsable @@ -83,6 +195,18 @@ class RootShell @Inject constructor( * single-quote escaping, so a package name containing shell metacharacters * cannot break out of its argument position. Only the enumerated * [DiagnosticCommand]s ever reach here, and none of them mutate state. + * + * The deadline, the concurrent drain of both pipes and the destroy-on-every- + * path teardown all live in [ProcessRunner]; see its KDoc for why reading the + * two pipes in sequence — which this used to do — was a deadlock and not + * merely slow. + * + * [timeoutMillis] stays the caller's to choose, and deliberately so: the + * three-second figure in requirement 4 is about *probes*, and `ElevatedObserver` + * legitimately asks for ten to twenty seconds to pull `dumpsys batterystats` + * off a device that has been up for weeks. Capping those at three would turn + * working screens into empty ones. Only [probe] is pinned, to + * [PROBE_TIMEOUT_MILLIS]. */ override suspend fun execute(argv: List, timeoutMillis: Long): ShellResult = withContext(io) { @@ -93,30 +217,16 @@ class RootShell @Inject constructor( return@withContext ShellResult.failure("No su binary on this device", accessLevel) } - var process: Process? = null - try { - val command = argv.joinToString(" ") { shellQuote(it) } - process = ProcessBuilder("su", "-c", command) + val command = argv.joinToString(" ") { shellQuote(it) } + runner.execute(argv, accessLevel, timeoutMillis) { + // redirectErrorStream(false) is the default, and it is spelled out + // because it is load-bearing: the two pipes are kept separate so a + // denial message on stderr is never mistaken for command output, + // and keeping them separate is what makes a concurrent drain + // necessary in the first place. + ProcessBuilder(SU, "-c", command) .redirectErrorStream(false) .start() - - val out = process.inputStream.readAllTextSafely(MAX_OUTPUT_BYTES) - val err = process.errorStream.readAllTextSafely(MAX_ERROR_BYTES) - - if (!process.waitForTimeout(timeoutMillis)) { - process.destroy() - return@withContext ShellResult( - -1, out, "Command timed out after $timeoutMillis ms", accessLevel, - ) - } - ShellResult(process.exitValue(), out, err, accessLevel) - } catch (t: Throwable) { - ShellResult.failure(t.message ?: t::class.java.simpleName, accessLevel) - } finally { - try { - process?.destroy() - } catch (ignored: Throwable) { - } } } @@ -128,6 +238,29 @@ class RootShell @Inject constructor( "'" + arg.replace("'", "'\\''") + "'" companion object { + /** + * Budget for the access probe (requirement 4 of the issue #1 spec). + * + * Three seconds, down from the ten this used to pass — and ten was never + * real anyway, because the old sequential drain meant the timeout was + * never consulted until the first `read()` returned. + * + * Three seconds is shorter than a human takes to answer a Magisk prompt, + * and that trade is deliberate: a probe is a question about access, not + * the act of gaining it, and nothing in the app may block on it + * (requirement 5). A prompt that is still on screen when this expires + * costs the user one more tap — the grant their manager records is + * persistent, so the second `su` returns immediately. The alternative is a + * dialog nobody is looking at holding an IO thread and a screen's refresh + * for as long as the phone is in a pocket. + */ + const val PROBE_TIMEOUT_MILLIS = 3_000L + + private const val SU = "su" + + /** Matches the detail length `ElevatedObserver` keeps from a failed shell. */ + private const val DETAIL_LIMIT = 200 + private val SU_PATHS = listOf( "/system/bin/su", "/system/xbin/su", @@ -137,7 +270,5 @@ class RootShell @Inject constructor( "/vendor/bin/su", "/debug_ramdisk/su", ) - private const val MAX_OUTPUT_BYTES = 4 * 1024 * 1024 - private const val MAX_ERROR_BYTES = 64 * 1024 } } diff --git a/app/src/main/java/com/processlens/core/system/ShizukuShell.kt b/app/src/main/java/com/processlens/core/system/ShizukuShell.kt index 9a66119..c788569 100644 --- a/app/src/main/java/com/processlens/core/system/ShizukuShell.kt +++ b/app/src/main/java/com/processlens/core/system/ShizukuShell.kt @@ -9,8 +9,6 @@ import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.withContext import rikka.shizuku.Shizuku -import java.io.ByteArrayOutputStream -import java.io.InputStream import javax.inject.Inject import javax.inject.Singleton @@ -31,6 +29,7 @@ import javax.inject.Singleton class ShizukuShell @Inject constructor( @ApplicationContext private val context: Context, @IoDispatcher private val io: CoroutineDispatcher, + private val runner: ProcessRunner, ) : ElevatedShell { override val accessLevel: AccessLevel = AccessLevel.SHIZUKU @@ -107,7 +106,15 @@ class ShizukuShell @Inject constructor( * `Shizuku.newProcess` is a hidden API reached by reflection inside the * Shizuku library, so it is wrapped defensively: a signature change in a * future Shizuku release degrades this to "unavailable" rather than crashing - * the app. + * the app. A `null` from [newProcess] — the shell declined to hand back a + * process — is passed straight through to [ProcessRunner], which reports it as + * a start failure rather than a crash. + * + * The deadline, the concurrent drain of both pipes and the destroy-on-every- + * path teardown all live in [ProcessRunner]. This used to drain stdout to EOF + * and only then stderr, which both missed the timeout entirely (the first + * blocking `read()` on an unanswered prompt never returned) and could deadlock + * against a child filling its stderr pipe. See [ProcessRunner]'s KDoc. */ override suspend fun execute(argv: List, timeoutMillis: Long): ShellResult = withContext(io) { @@ -117,41 +124,8 @@ class ShizukuShell @Inject constructor( accessLevel, ) } - var process: Process? = null - try { - process = newProcess(argv.toTypedArray()) - ?: return@withContext ShellResult.failure( - "Shizuku could not start a process on this device.", - accessLevel, - ) - - // Drain both pipes before waiting: a command like `dumpsys - // batterystats` produces megabytes, and a full pipe buffer would - // deadlock a waitFor() that has not been read from. - val out = process.inputStream.readAllTextSafely(MAX_OUTPUT_BYTES) - val err = process.errorStream.readAllTextSafely(MAX_ERROR_BYTES) - - val finished = process.waitForTimeout(timeoutMillis) - if (!finished) { - process.destroy() - return@withContext ShellResult( - exitCode = -1, - stdout = out, - stderr = "Command timed out after ${timeoutMillis} ms", - accessLevel = accessLevel, - ) - } - ShellResult(process.exitValue(), out, err, accessLevel) - } catch (t: Throwable) { - ShellResult.failure( - t.message ?: t::class.java.simpleName, - accessLevel, - ) - } finally { - try { - process?.destroy() - } catch (ignored: Throwable) { - } + runner.execute(argv, accessLevel, timeoutMillis) { command -> + newProcess(command.toTypedArray()) } } @@ -175,70 +149,5 @@ class ShizukuShell @Inject constructor( "moe.shizuku.privileged.api", "moe.shizuku.manager", ) - - /** - * Caps on captured output. `dumpsys batterystats` can exceed 10 MB on a - * device that has been up for weeks; the parsers only need the header - * sections, and an unbounded read would be a genuine OOM risk on the - * low-memory devices this app targets. - */ - private const val MAX_OUTPUT_BYTES = 4 * 1024 * 1024 - private const val MAX_ERROR_BYTES = 64 * 1024 - } -} - -/** - * Reads a stream to text with a hard byte cap, never throwing. Truncation is - * marked inline so a parser cannot mistake a cut-off dump for a complete one. - */ -internal fun InputStream.readAllTextSafely(maxBytes: Int): String = try { - use { stream -> - val buffer = ByteArrayOutputStream(minOf(maxBytes, 64 * 1024)) - val chunk = ByteArray(16 * 1024) - var total = 0 - while (true) { - val read = stream.read(chunk) - if (read <= 0) break - val allowed = minOf(read, maxBytes - total) - if (allowed > 0) { - buffer.write(chunk, 0, allowed) - total += allowed - } - if (total >= maxBytes) { - buffer.write("\n[output truncated at $maxBytes bytes]\n".toByteArray()) - break - } - } - buffer.toString("UTF-8") - } -} catch (t: Throwable) { - "" -} - -/** - * `Process.waitFor(timeout, unit)` is API 26+, which matches this app's minSdk, - * but the `Process` returned by Shizuku is a remote proxy whose implementation - * may not honour it. Falling back to polling `exitValue()` keeps a hung command - * from blocking the caller forever. - */ -internal fun Process.waitForTimeout(timeoutMillis: Long): Boolean { - try { - return waitFor(timeoutMillis, java.util.concurrent.TimeUnit.MILLISECONDS) - } catch (t: Throwable) { - val deadline = System.currentTimeMillis() + timeoutMillis - while (System.currentTimeMillis() < deadline) { - try { - exitValue() - return true - } catch (notYet: IllegalThreadStateException) { - try { - Thread.sleep(50) - } catch (ie: InterruptedException) { - Thread.currentThread().interrupt() - return false - } - } - } - return false } } diff --git a/app/src/main/java/com/processlens/data/repository/SystemRepositoryImpl.kt b/app/src/main/java/com/processlens/data/repository/SystemRepositoryImpl.kt index 5ead222..7727eab 100644 --- a/app/src/main/java/com/processlens/data/repository/SystemRepositoryImpl.kt +++ b/app/src/main/java/com/processlens/data/repository/SystemRepositoryImpl.kt @@ -1,5 +1,7 @@ package com.processlens.data.repository +import android.os.Build +import com.processlens.core.common.ApplicationScope import com.processlens.core.common.DefaultDispatcher import com.processlens.core.common.Observed import com.processlens.core.system.CompositeSystemObserver @@ -18,17 +20,20 @@ import com.processlens.domain.repository.SystemRepository import com.processlens.domain.repository.SystemState import com.processlens.domain.usecase.ProcessListAssembler import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.onStart +import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext +import java.util.concurrent.atomic.AtomicBoolean import javax.inject.Inject import javax.inject.Singleton @@ -46,10 +51,27 @@ class SystemRepositoryImpl @Inject constructor( private val observer: CompositeSystemObserver, private val settings: SettingsRepository, @DefaultDispatcher private val computation: CoroutineDispatcher, + @ApplicationScope private val appScope: CoroutineScope, ) : SystemRepository { - private val capabilities = MutableStateFlow(null) + /** + * Seeded with a renderable placeholder rather than `null`. + * + * This flow feeds a `combine()` in twelve view models, and `combine` publishes + * nothing until *every* source has emitted at least once. A `null` seed behind a + * `filterNotNull()` therefore held every one of those screens on its initial + * state until something happened to call [refreshCapabilities] — which nothing on + * the cold-start path did, so Overview showed "Reading system state" until the + * user wandered into Settings → Access and incidentally constructed the view model + * that refreshes it (issue #1). + * + * [SystemCapabilities.unknown] exists for exactly this: every `get()` on it falls + * back to "Not evaluated on this device", so it renders as honest absence rather + * than as fabricated capability. + */ + private val capabilities = MutableStateFlow(SystemCapabilities.unknown(Build.VERSION.SDK_INT)) private val capabilityLock = Mutex() + private val detectionStarted = AtomicBoolean(false) /** * Polls at the user's chosen interval (Section 6: 1/2/5/10 s or manual). @@ -108,10 +130,34 @@ class SystemRepositoryImpl @Inject constructor( lastKnownProcessCount = count } + /** + * Emits the placeholder immediately, then the real evaluation when it lands. + * + * The first collector triggers detection; the result is shared by every later + * collector because this repository is a `@Singleton`. Detection runs on the + * application scope rather than the collector's, so a screen that leaves the + * composition mid-detection does not cancel the work the next screen needs. + */ override fun observeCapabilities(): Flow = - capabilities.asStateFlow().filterNotNull().distinctUntilChanged() + capabilities.asStateFlow() + .onStart { kickFirstDetection() } + .distinctUntilChanged() + + /** + * Runs the first capability evaluation once per process, without blocking the + * collector. Failure is swallowed deliberately: the placeholder is already on the + * wire, so a detection that throws degrades the UI to "not evaluated" instead of + * restoring the permanent spinner this replaced. + */ + private fun kickFirstDetection() { + if (!detectionStarted.compareAndSet(false, true)) return + appScope.launch { runCatching { refreshCapabilities() } } + } override suspend fun refreshCapabilities(): SystemCapabilities = capabilityLock.withLock { + // Mark detection as done even when a caller beat the first collector to it, + // so the kick cannot queue a redundant second evaluation. + detectionStarted.set(true) val detected = observer.getCapabilities() capabilities.value = detected detected diff --git a/app/src/main/java/com/processlens/feature/overview/OverviewScreen.kt b/app/src/main/java/com/processlens/feature/overview/OverviewScreen.kt index db891a4..db4a728 100644 --- a/app/src/main/java/com/processlens/feature/overview/OverviewScreen.kt +++ b/app/src/main/java/com/processlens/feature/overview/OverviewScreen.kt @@ -24,7 +24,11 @@ import androidx.compose.material.icons.outlined.ViewList import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.unit.dp @@ -53,6 +57,7 @@ import com.processlens.core.designsystem.ScreenHeader import com.processlens.core.designsystem.SectionHeader import com.processlens.core.designsystem.SegmentedBar import com.processlens.core.designsystem.StatTile +import com.processlens.core.designsystem.UnresolvedBlock import com.processlens.core.designsystem.loadColor import com.processlens.domain.model.EventSeverity import com.processlens.domain.model.FavoriteType @@ -128,7 +133,28 @@ fun OverviewScreen( val system = state.system if (system == null) { - LoadingBlock() + // A spinner is only an honest answer for as long as a sample might still + // be arriving silently. Past the deadline — or the moment the pipeline + // reports a failure — the screen stops turning and explains itself, with a + // retry that actually restarts the sources (issue #1, Sections 42/48). + var deadlineElapsed by remember { mutableStateOf(false) } + LaunchedEffect(state.loadFailure, state.isRevalidating) { + deadlineElapsed = false + kotlinx.coroutines.delay(FIRST_SAMPLE_DEADLINE_MILLIS) + deadlineElapsed = true + } + when (state.loadPhase(deadlineElapsed)) { + OverviewLoadPhase.WAITING -> LoadingBlock() + OverviewLoadPhase.UNRESOLVED -> UnresolvedBlock( + title = unresolvedTitle(state.loadFailure), + explanation = unresolvedExplanation(state.loadFailure), + onRetry = viewModel::revalidateAccess, + isRetrying = state.isRevalidating, + detail = state.loadFailure?.detail, + ) + // Can't happen while system == null, but the when is exhaustive. + OverviewLoadPhase.CONTENT -> LoadingBlock() + } return@Column } @@ -141,6 +167,18 @@ fun OverviewScreen( ) } + // Non-blocking: the dashboard is already drawn above and below it. Only + // offered once the capability matrix has really been evaluated, so it never + // flashes against the startup placeholder, and never on a device already + // running through Shizuku or root (Section 42, requirement 6). + if (showElevatedAccessNotice(state.capabilities, state.isAccessNoticeDismissed)) { + ElevatedAccessNotice( + capabilities = state.capabilities!!, + onOpenAccess = onOpenAccess, + onDismiss = viewModel::dismissAccessNotice, + ) + } + if (state.isRecording) { RecordingBanner( onOpen = { state.activeInvestigationId?.let(onOpenTimeline) ?: onOpenInvestigate() }, @@ -281,6 +319,46 @@ private fun RecordingBanner(onOpen: () -> Unit) { ) } +/** + * The non-blocking "running without elevated access" notice (requirement 6). + * + * Deliberately an INFO banner, not a warning: normal access is the expected state on + * most devices and nothing is broken. It states how many probed capabilities an + * elevated shell would improve — a figure that comes from the capability matrix the + * device actually produced, never a guess — and offers two outs: open the Access + * screen, or dismiss it for this screen's lifetime. + */ +@Composable +private fun ElevatedAccessNotice( + capabilities: com.processlens.domain.model.SystemCapabilities, + onOpenAccess: () -> Unit, + onDismiss: () -> Unit, +) { + val unlockable = countUnlockable(capabilities) + val levels = unlockedByLevels(capabilities) + .joinToString(" or ") { it.label } + .ifBlank { "Shizuku or root" } + NoticeBanner( + text = if (unlockable > 0) { + "ProcessLens is running without elevated access. $unlockable " + + (if (unlockable == 1) "capability" else "capabilities") + + " on this device would read more fully with $levels. Everything else is " + + "shown; nothing is being withheld." + } else { + "ProcessLens is running without elevated access. Everything this device " + + "exposes to apps is already shown." + }, + severity = EventSeverity.INFO, + action = { + Row(verticalAlignment = Alignment.CenterVertically) { + ActionText("Set up access", onClick = onOpenAccess, icon = Icons.Outlined.Bolt) + Spacer(Modifier.width(8.dp)) + ActionText("Dismiss", onClick = onDismiss) + } + }, + ) +} + @Composable private fun StatGrid( system: SystemState, diff --git a/app/src/main/java/com/processlens/feature/overview/OverviewViewModel.kt b/app/src/main/java/com/processlens/feature/overview/OverviewViewModel.kt index b71b108..c355de8 100644 --- a/app/src/main/java/com/processlens/feature/overview/OverviewViewModel.kt +++ b/app/src/main/java/com/processlens/feature/overview/OverviewViewModel.kt @@ -2,8 +2,10 @@ package com.processlens.feature.overview import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import com.processlens.core.common.AccessLevel import com.processlens.core.common.Observed import com.processlens.core.common.valueOrNull +import com.processlens.domain.model.Availability import com.processlens.domain.model.Favorite import com.processlens.domain.model.InvestigationEvent import com.processlens.domain.model.SystemCapabilities @@ -14,11 +16,14 @@ import com.processlens.domain.repository.SettingsRepository import com.processlens.domain.repository.SystemRepository import com.processlens.domain.repository.SystemState import dagger.hilt.android.lifecycle.HiltViewModel +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch @@ -32,13 +37,28 @@ import javax.inject.Inject * screen observed while it was open — there is no back-filling and no interpolation. * A sample whose CPU reading was restricted is stored as `null` and drawn as a gap * (Section 42). + * + * The other thing this class now guarantees is that its state **resolves**. Issue #1 + * was a starved `combine()` upstream, fixed in the repository, but the reason it was + * invisible for so long is that nothing here ever questioned a silent source: a + * five-arity `combine` publishes nothing until every input has spoken, a thrown + * collector took the whole flow down without a trace, and the screen's only reaction + * to "no state yet" was a spinner with no deadline. Three things therefore hold here + * now, and the dashboard depends on all three: + * + * - a throw lands in [State.loadFailure] as an [Observed.Failed] rather than ending + * the screen's story; + * - collection is restartable, so the retry the user is offered is a real one; + * - the capability placeholder is distinguishable from a real "no elevated access" + * answer, so the dashboard does not accuse the device of restrictions it has not + * yet been asked about. */ @HiltViewModel class OverviewViewModel @Inject constructor( private val systemRepository: SystemRepository, private val settingsRepository: SettingsRepository, - investigationRepository: InvestigationRepository, - favoritesRepository: FavoritesRepository, + private val investigationRepository: InvestigationRepository, + private val favoritesRepository: FavoritesRepository, ) : ViewModel() { data class State( @@ -54,6 +74,19 @@ class OverviewViewModel @Inject constructor( val memoryHistory: List = emptyList(), val isRefreshing: Boolean = false, val error: String? = null, + /** + * Set when the state pipeline itself threw. + * + * [Observed.Failed] rather than a bare `String` on purpose: the screen then + * renders it through the same Section 42 components as any other unreadable + * value, reason and technical-details expander included, instead of growing a + * second vocabulary for "this went wrong" that only the dashboard speaks. + */ + val loadFailure: Observed.Failed? = null, + /** True while [revalidateAccess] is in flight, so a re-probe looks like one. */ + val isRevalidating: Boolean = false, + /** The user has closed the elevated-access notice for this screen's lifetime. */ + val isAccessNoticeDismissed: Boolean = false, ) { val isFirstLoad: Boolean get() = system == null } @@ -82,8 +115,73 @@ class OverviewViewModel @Inject constructor( private val refreshing = MutableStateFlow(false) private val errors = MutableStateFlow(null) + private val notices = MutableStateFlow(Notices()) + + /** The bits of state the user's own actions own, kept in one flow so the chain + * of `combine` operators below does not grow an operator per boolean. */ + private data class Notices( + val isRevalidating: Boolean = false, + val accessNoticeDismissed: Boolean = false, + ) - val state: StateFlow = combine( + /** + * Collection is restartable (issue #1). + * + * The `catch` in [observeDashboardState] resolves a thrown pipeline into something + * the screen can render — but a caught flow is a *finished* flow. `stateIn` keeps + * the last value and nothing upstream ever runs again, so a retry control wired to + * a finished pipeline is the same dead end wearing a different hat. Bumping this + * counter cancels whatever is left of the previous collection and starts the + * sources from scratch, which is what makes the retry on the unresolved state and + * the re-probe in the access notice do something a user can observe. + */ + private val attempts = MutableStateFlow(0) + + /** + * The last state the pipeline managed to build. + * + * Read only by the `catch`, so a failure that arrives after content was already on + * screen degrades to "the figures you can see, plus the error" instead of throwing + * the dashboard back to an empty frame. + */ + private val lastBuilt = MutableStateFlow(State()) + + @OptIn(ExperimentalCoroutinesApi::class) + val state: StateFlow = attempts + .flatMapLatest { observeDashboardState() } + .combine(refreshing) { state, isRefreshing -> + state.copy(isRefreshing = isRefreshing) + }.combine(errors) { state, error -> + state.copy(error = error) + }.combine(notices) { state, notice -> + state.copy( + isRevalidating = notice.isRevalidating, + isAccessNoticeDismissed = notice.accessNoticeDismissed, + ) + }.stateIn( + scope = viewModelScope, + // The polling flow is cold: this stops it when the screen leaves and lets it + // survive a rotation without a restart. + started = SharingStarted.WhileSubscribed(5_000), + initialValue = State(), + ) + + /** + * One collection of the five sources, ending in a state the screen can always draw. + * + * The three user-driven flows (refresh, errors, notices) are deliberately *outside* + * this function: they must not be cancelled and rebuilt when [attempts] restarts + * the sources, or a retry would silently clear the very error message that prompted + * it. + * + * The `catch` is the point of the whole thing. Before it there was no `catch` and no + * `try` around any flow collection anywhere in this app, which means a throwing + * upstream — an OEM `/proc` read that raises something `ProcFsReader` does not + * expect, a database query that fails mid-collection — took this flow down in + * silence and left the screen on `initialValue`, i.e. the permanent "Reading system + * state" of issue #1. Swallowing it would have been worse than the crash. + */ + private fun observeDashboardState(): Flow = combine( systemRepository.observeSystemState().onEachRecordHistory(), systemRepository.observeCapabilities(), settingsRepository.observe(), @@ -105,17 +203,16 @@ class OverviewViewModel @Inject constructor( cpuHistory = hist.cpu, memoryHistory = hist.memory, ) - }.combine(refreshing) { state, isRefreshing -> - state.copy(isRefreshing = isRefreshing) - }.combine(errors) { state, error -> - state.copy(error = error) - }.stateIn( - scope = viewModelScope, - // The polling flow is cold: this stops it when the screen leaves and lets it - // survive a rotation without a restart. - started = SharingStarted.WhileSubscribed(5_000), - initialValue = State(), - ) + }.onEach { built -> + lastBuilt.value = built + }.catch { cause -> + // Both channels, on purpose. `errors` is the existing banner and is what the + // user reads; `loadFailure` is the structured copy the unresolved state needs + // and the only one that survives a tap on "Dismiss". + errors.value = cause.message?.takeIf { it.isNotBlank() } + ?: "The dashboard stopped receiving system state" + emit(loadFailureState(lastBuilt.value, cause)) + } private data class Quint( val system: SystemState, @@ -157,11 +254,36 @@ class OverviewViewModel @Inject constructor( } } - /** Re-probes access after the user grants Shizuku or changes a setting. */ + /** + * Re-probes access, and re-collects everything that depends on the answer. + * + * This method existed before issue #1 was filed and had no callers — which is a + * large part of why the hang was permanent, because it is the one action that + * writes the capability flow the dashboard was waiting on. It now backs two + * controls: the retry on the unresolved state, and the re-probe in the access + * notice. + * + * Three things it does that the dead version did not. It reports progress, because + * a probe that takes a second and says nothing is indistinguishable from a dead + * button. It surfaces its own failure through the existing banner rather than + * dropping it in a `runCatching`. And it bumps [attempts] on the way out, so a tap + * after the pipeline has already terminated restarts the sources instead of + * politely doing nothing. + */ fun revalidateAccess() { + if (notices.value.isRevalidating) return + notices.value = notices.value.copy(isRevalidating = true) viewModelScope.launch { - runCatching { systemRepository.invalidateAccess() } - runCatching { systemRepository.refreshCapabilities() } + try { + systemRepository.invalidateAccess() + systemRepository.refreshCapabilities() + errors.value = null + } catch (t: Throwable) { + errors.value = t.message ?: "Could not re-probe this device's access level" + } finally { + notices.value = notices.value.copy(isRevalidating = false) + attempts.value += 1 + } } } @@ -169,12 +291,191 @@ class OverviewViewModel @Inject constructor( errors.value = null } + /** + * Closes the elevated-access notice. + * + * Held here rather than in the composition so it survives a rotation: a banner that + * reappears every time the device turns is not dismissible in any sense the user + * recognises. It is deliberately *not* persisted — the honest scope of the dismissal + * is "I have read this", not "never tell me about this device again", and the next + * launch re-probes anyway. + */ + fun dismissAccessNotice() { + notices.value = notices.value.copy(accessNoticeDismissed = true) + } + private companion object { const val RECENT_EVENT_LIMIT = 6 const val FAVORITE_LIMIT = 6 } } +/** + * How long the dashboard waits for its first sample before it stops pretending to be + * busy and starts explaining itself. + * + * Not a timeout on anything: nothing is cancelled when it expires, and a sample that + * arrives late still renders. It only bounds how long a *spinner* is an honest answer. + * Four seconds is comfortably longer than a healthy cold read (the repository's poll + * loop emits before its first `delay`, so the first sample lands in well under a + * second) and short enough that a user has not yet decided the app is broken. + */ +const val FIRST_SAMPLE_DEADLINE_MILLIS = 4_000L + +/** What the dashboard draws where its content goes. */ +enum class OverviewLoadPhase { + /** There is a sample. Draw the dashboard. */ + CONTENT, + + /** No sample yet, and the wait is still young enough for a spinner to be true. */ + WAITING, + + /** No sample, and no longer any reason to expect one silently. Explain and offer out. */ + UNRESOLVED, +} + +/** + * The loading decision, as a function rather than a shape in a composable. + * + * Deliberately not an [Observed]: that type models a *reading* that is absent and the + * reason it is absent, and "the first sample has not arrived yet" is neither a reading + * nor a restriction. Pretending otherwise would mean inventing an `Observed` case to + * mean "pending", which would weaken the one type in this app that is never allowed to + * be vague. + * + * [deadlineElapsed] comes from the screen because only the composition knows how long + * it has been on screen. Everything else is state, which is why this is testable + * without a Compose harness. + */ +fun OverviewViewModel.State.loadPhase(deadlineElapsed: Boolean): OverviewLoadPhase = when { + system != null -> OverviewLoadPhase.CONTENT + loadFailure != null || deadlineElapsed -> OverviewLoadPhase.UNRESOLVED + else -> OverviewLoadPhase.WAITING +} + +/** + * Folds a thrown pipeline into a renderable state. + * + * Pure, and takes the previous state so the dashboard keeps whatever it had: a failure + * three minutes into a session should cost the user the trend they were reading, not + * the whole screen. [Observed.Failed] rather than [Observed.Restricted] because this is + * a fault, not a platform limit — Section 42 draws that line, and labelling a crashed + * collector "restricted by Android" would blame the device for the app's own bug. + */ +fun loadFailureState(previous: OverviewViewModel.State, cause: Throwable): OverviewViewModel.State = + previous.copy( + isRefreshing = false, + loadFailure = Observed.Failed( + detail = cause.message?.takeIf { it.isNotBlank() } + ?: cause::class.java.simpleName, + cause = cause::class.java.name, + ), + ) + +/** Heading for the unresolved state. Separate from the prose so both are testable. */ +fun unresolvedTitle(failure: Observed.Failed?): String = + if (failure != null) "System state could not be read" else "No reading has arrived yet" + +/** + * The prose under [unresolvedTitle]. + * + * Both branches say the same load-bearing thing in different words: nothing is being + * hidden. A reader who has watched a spinner has every reason to assume the app knows + * something it is not saying, and the only cure is to state that there is no sample at + * all (Section 42). + */ +fun unresolvedExplanation(failure: Observed.Failed?): String = if (failure != null) { + "The flow this dashboard reads from stopped with an error, so there is no sample to " + + "show. Nothing is being withheld: the reading was never taken." +} else { + "Nothing arrived within ${FIRST_SAMPLE_DEADLINE_MILLIS / 1_000} seconds. The figures " + + "are not being withheld — no sample has reached this screen at all. Re-probing " + + "this device re-reads its access level and starts the sampling again." +} + +/** + * True once the capability matrix holds a real answer. + * + * This distinction is load-bearing and easy to lose. `SystemCapabilities.unknown()` — + * the placeholder the repository seeds its flow with so that twelve `combine`s resolve + * on the first frame — reports `rootState = UNAVAILABLE` and `accessLevel = NORMAL`, + * which is indistinguishable from a device that was probed and genuinely has neither. + * Reading it as a confirmed "no root" would flash "running without elevated access" on + * every single launch, including on rooted devices, a second before the real answer + * lands. + * + * What the placeholder *does* leave behind is an empty `statuses` map: every `get()` + * falls through to "Not evaluated on this device", and no detector ever produces an + * empty matrix. Emptiness is therefore the signal, and it is the only one available + * without adding a state to a model this screen does not own. + */ +fun isCapabilityMatrixEvaluated(capabilities: SystemCapabilities?): Boolean = + capabilities != null && capabilities.statuses.isNotEmpty() + +/** + * Whether to offer the non-blocking "no elevated access" notice. + * + * Gated on [isCapabilityMatrixEvaluated] so it cannot fire against the placeholder, and + * on `NORMAL` rather than on `rootState` alone: a device running through Shizuku is not + * "without elevated access", and telling its user to go and find root would be wrong as + * well as annoying. + */ +fun showElevatedAccessNotice( + capabilities: SystemCapabilities?, + isDismissed: Boolean, +): Boolean = !isDismissed && + isCapabilityMatrixEvaluated(capabilities) && + capabilities?.accessLevel == AccessLevel.NORMAL + +/** + * The elevated levels that would expose at least one of [observations]. + * + * Reads each restriction's own [Observed.Restricted.unlockedBy], which is exactly what + * that field is for, so the hint is the data layer's claim rather than the screen's + * guess. A figure this kernel simply does not publish carries `unlockedBy = null` by + * construction and therefore contributes nothing: Section 42 forbids telling a user + * that root would produce a sensor their hardware does not have. + * + * `NORMAL` is filtered out because "normal access would expose this" is not a thing to + * suggest to someone already running at normal access. + */ +fun unlockedByLevels(vararg observations: Observed<*>): List = observations + .filterIsInstance() + .mapNotNull { it.unlockedBy } + .filter { it != AccessLevel.NORMAL } + .distinct() + .sortedBy { it.rank } + +/** + * The same question asked of the probed capability matrix. + * + * The per-metric answer above is the precise one and wins where it exists, but most + * platform refusals cannot name a level honestly at the point of the read — whether a + * shell would actually succeed is only knowable by asking one. The matrix *has* asked, + * which is why `CapabilityStatus.unlockedBy` is populated where `Observed` is not, and + * why the notice's wording comes from here. + */ +fun unlockedByLevels(capabilities: SystemCapabilities): List = capabilities + .statuses + .values + .asSequence() + .filter { it.availability != Availability.FULL } + .mapNotNull { it.unlockedBy } + .filter { it != AccessLevel.NORMAL } + .distinct() + .sortedBy { it.rank } + .toList() + +/** + * How many probed capabilities elevated access would actually improve. + * + * Counts `LIMITED` as well as `UNAVAILABLE`: a partial answer that a shell would + * complete is a real gain, and reporting only the total refusals would undersell the + * offer. Counts nothing that names no level, so the number cannot exceed what is true. + */ +fun countUnlockable(capabilities: SystemCapabilities): Int = capabilities.statuses.values + .count { it.availability != Availability.FULL && it.unlockedBy != null } + /** * Convenience for the dashboard's "how sure are we?" line: the number of headline * figures that are real readings rather than restrictions. diff --git a/app/src/test/java/com/processlens/core/system/ProcessRunnerTest.kt b/app/src/test/java/com/processlens/core/system/ProcessRunnerTest.kt new file mode 100644 index 0000000..b0ee723 --- /dev/null +++ b/app/src/test/java/com/processlens/core/system/ProcessRunnerTest.kt @@ -0,0 +1,222 @@ +package com.processlens.core.system + +import com.processlens.core.common.AccessLevel +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import java.io.ByteArrayInputStream +import java.io.ByteArrayOutputStream +import java.io.InputStream +import java.io.OutputStream +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import org.junit.Test + +/** + * Tests for the real-deadline process runner (requirement 4 of the issue #1 spec). + * + * The seam that makes this a JVM test with no device: [ProcessStarter]. Every path + * the field hit — `su` missing, `su` denied, `su` parked on an unanswered prompt, + * `su` granting — is driven here by a [FakeProcess] standing in for the OS process, + * so the timeout, the destroy-on-every-path teardown and the concurrent drain are + * exercised against code that never touches a shell. + * + * These use [runBlocking] on a real dispatcher rather than virtual time on purpose: + * the whole point of the class is that a blocking `read()` and a wall-clock deadline + * behave correctly together, and virtual time would hide exactly the interaction + * being tested. + */ +class ProcessRunnerTest { + + private val runner = ProcessRunner(Dispatchers.IO) + + private fun run( + argv: List = listOf("id"), + timeoutMillis: Long = 1_000L, + starter: ProcessStarter, + ): ShellResult = runBlocking { + runner.execute(argv, AccessLevel.ROOT, timeoutMillis, starter = starter) + } + + // ---------------------------------------------------------------- su succeeds + + @Test + fun `a process that exits zero reports success and its stdout`() { + val result = run { FakeProcess(stdout = "uid=0(root) gid=0(root)\n", exitCode = 0) } + + assertTrue(result.isSuccess) + assertEquals(0, result.exitCode) + assertTrue(result.stdout.contains("uid=0")) + assertEquals(AccessLevel.ROOT, result.accessLevel) + } + + @Test + fun `both pipes are drained and kept separate`() { + val result = run { + FakeProcess(stdout = "the answer\n", stderr = "a warning\n", exitCode = 0) + } + + assertTrue(result.stdout.contains("the answer")) + assertTrue(result.stderr.contains("a warning")) + assertFalse("stderr must not leak into stdout", result.stdout.contains("a warning")) + } + + // ----------------------------------------------------------------- su denied + + @Test + fun `a non-zero exit is a failure that carries the exit code and stderr`() { + val result = run { + FakeProcess(stderr = "Permission denied\n", exitCode = 1) + } + + assertFalse(result.isSuccess) + assertEquals(1, result.exitCode) + assertTrue(result.stderr.contains("Permission denied")) + } + + // ---------------------------------------------------------------- su missing + + @Test + fun `a starter that returns null is a start failure, not a crash`() { + val result = run { null } + + assertFalse(result.isSuccess) + assertEquals(ProcessRunner.EXIT_NOT_STARTED, result.exitCode) + assertTrue(result.stderr.contains("could not start")) + } + + @Test + fun `a thrown starter is reported as a failure with its message`() { + val result = run { throw java.io.IOException("No such file or directory") } + + assertFalse(result.isSuccess) + assertEquals(ProcessRunner.EXIT_NOT_STARTED, result.exitCode) + assertTrue(result.stderr.contains("No such file or directory")) + } + + @Test + fun `an empty argv never starts anything`() { + var started = false + val result = run(argv = emptyList()) { + started = true + FakeProcess(exitCode = 0) + } + + assertFalse(started) + assertFalse(result.isSuccess) + } + + // ------------------------------------------------------------------ su hangs + + @Test + fun `a process that never exits is timed out and destroyed`() { + val process = FakeProcess(exitCode = 0, hang = true) + + val started = System.currentTimeMillis() + val result = run(timeoutMillis = 200L) { process } + val elapsed = System.currentTimeMillis() - started + + assertEquals(ProcessRunner.EXIT_TIMED_OUT, result.exitCode) + assertFalse(result.isSuccess) + assertTrue("the deadline is real, not merely a label", result.stderr.contains("timed out")) + assertTrue("the child must be destroyed to release the readers", process.wasDestroyed()) + assertTrue( + "the call must return near the deadline, not hang: ${elapsed}ms", + elapsed < 2_000L, + ) + } + + @Test + fun `output produced before a hang survives the timeout`() { + val process = FakeProcess( + stdoutStream = PrefixThenBlockStream("partial output\n"), + exitCode = 0, + hang = true, + ) + + val result = run(timeoutMillis = 200L) { process } + + assertEquals(ProcessRunner.EXIT_TIMED_OUT, result.exitCode) + assertTrue( + "bytes captured before the hang are evidence, not discarded", + result.stdout.contains("partial output"), + ) + } + + /** + * A fake OS process. + * + * [hang] parks it: [exitValue] throws until [destroy] is called, so the real + * `Process.waitFor(timeout, unit)` polling loop returns false at the deadline — + * which is precisely the "`su` on an unanswered prompt" case. + */ + private class FakeProcess( + stdout: String = "", + stderr: String = "", + private val exitCode: Int = 0, + private val hang: Boolean = false, + stdoutStream: InputStream? = null, + ) : Process() { + + private val out: InputStream = stdoutStream ?: ByteArrayInputStream(stdout.toByteArray()) + private val err: InputStream = ByteArrayInputStream(stderr.toByteArray()) + private val sink = ByteArrayOutputStream() + private val destroyed = AtomicBoolean(false) + + fun wasDestroyed(): Boolean = destroyed.get() + + override fun getOutputStream(): OutputStream = sink + override fun getInputStream(): InputStream = out + override fun getErrorStream(): InputStream = err + + override fun waitFor(): Int { + while (hang && !destroyed.get()) Thread.sleep(10) + return exitValue() + } + + override fun exitValue(): Int { + if (hang && !destroyed.get()) throw IllegalThreadStateException("still running") + return if (destroyed.get() && hang) 143 else exitCode + } + + override fun destroy() { + if (destroyed.compareAndSet(false, true)) { + runCatching { out.close() } + runCatching { err.close() } + } + } + + override fun waitFor(timeout: Long, unit: TimeUnit): Boolean = + super.waitFor(timeout, unit) + } + + /** + * Yields a prefix, then blocks until closed — the shape of a pipe whose child is + * still alive but no longer producing. [close] (which the runner calls when it + * destroys the process) is what lets the blocked read throw and the drain end. + */ + private class PrefixThenBlockStream(prefix: String) : InputStream() { + private val head = ByteArrayInputStream(prefix.toByteArray()) + private val closed = AtomicBoolean(false) + + override fun read(): Int { + val b = head.read() + if (b != -1) return b + while (!closed.get()) Thread.sleep(10) + throw java.io.IOException("stream closed") + } + + override fun read(b: ByteArray, off: Int, len: Int): Int { + val n = head.read(b, off, len) + if (n != -1) return n + while (!closed.get()) Thread.sleep(10) + throw java.io.IOException("stream closed") + } + + override fun close() { + closed.set(true) + } + } +} diff --git a/app/src/test/java/com/processlens/core/system/RestrictionCacheTest.kt b/app/src/test/java/com/processlens/core/system/RestrictionCacheTest.kt new file mode 100644 index 0000000..d37dcee --- /dev/null +++ b/app/src/test/java/com/processlens/core/system/RestrictionCacheTest.kt @@ -0,0 +1,186 @@ +package com.processlens.core.system + +import com.processlens.core.common.AccessLevel +import com.processlens.core.common.DataSource +import com.processlens.core.common.Observed +import com.processlens.core.common.RestrictionReason +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test +import java.util.concurrent.atomic.AtomicInteger + +/** + * Tests for the negative cache (requirement 1 of the issue #1 spec: a denied read is + * attempted once per session, not once per sample). + * + * Plain JVM: [RestrictionCache] is deliberately free of `android.*`, so the exact + * memoisation that decides whether a screen keeps saying "Not available" is checked + * here rather than on a device. + */ +class RestrictionCacheTest { + + private fun restricted(reason: RestrictionReason): Observed.Restricted = + Observed.Restricted(reason, AccessLevel.SHIZUKU, "denied") + + private fun value(v: T): Observed = Observed.of(v, DataSource.PROC_FS) + + @Test + fun `a terminal refusal is remembered and the read is not repeated`() { + val cache = RestrictionCache() + val calls = AtomicInteger(0) + + val read = { + calls.incrementAndGet() + restricted(RestrictionReason.PLATFORM_RESTRICTED) + } + + val first = cache.attempt("cpu", read) + val second = cache.attempt("cpu", read) + + assertEquals("the second attempt must not touch the filesystem", 1, calls.get()) + assertSame("the remembered refusal is handed back verbatim", first, second) + assertTrue(cache.isRefused("cpu")) + } + + @Test + fun `every platform reason is terminal`() { + val terminal = listOf( + RestrictionReason.PLATFORM_RESTRICTED, + RestrictionReason.NOT_PRESENT_ON_DEVICE, + RestrictionReason.NOT_SUPPORTED_ON_API_LEVEL, + RestrictionReason.REQUIRES_ELEVATED_ACCESS, + ) + for (reason in terminal) { + val cache = RestrictionCache() + cache.attempt("k") { restricted(reason) } + assertTrue("$reason should be cached", cache.isRefused("k")) + } + } + + @Test + fun `a permission refusal is not remembered because one dialog revokes it`() { + val cache = RestrictionCache() + val calls = AtomicInteger(0) + + repeat(2) { + cache.attempt("usage") { + calls.incrementAndGet() + restricted(RestrictionReason.PERMISSION_REQUIRED) + } + } + + assertEquals("a permission refusal is re-read every time", 2, calls.get()) + assertFalse(cache.isRefused("usage")) + assertNull(cache.refusalFor("usage")) + } + + @Test + fun `a sampling-disabled refusal is not remembered because the user owns the switch`() { + val cache = RestrictionCache() + val calls = AtomicInteger(0) + + repeat(3) { + cache.attempt("cpu") { + calls.incrementAndGet() + restricted(RestrictionReason.SAMPLING_DISABLED) + } + } + + assertEquals(3, calls.get()) + assertFalse(cache.isRefused("cpu")) + } + + @Test + fun `a failed read is never memoised so one bad sample is not permanent`() { + val cache = RestrictionCache() + val calls = AtomicInteger(0) + + repeat(2) { + cache.attempt("loadavg") { + calls.incrementAndGet() + Observed.Failed("empty file", "ParseException") + } + } + + assertEquals("a fault is retried, not remembered", 2, calls.get()) + assertFalse(cache.isRefused("loadavg")) + } + + @Test + fun `a successful read is never memoised`() { + val cache = RestrictionCache() + val calls = AtomicInteger(0) + + repeat(2) { + cache.attempt("cpu") { + calls.incrementAndGet() + value(42) + } + } + + assertEquals(2, calls.get()) + assertFalse(cache.isRefused("cpu")) + } + + @Test + fun `different metrics are remembered independently`() { + val cache = RestrictionCache() + + cache.attempt("thermal") { restricted(RestrictionReason.PLATFORM_RESTRICTED) } + cache.attempt("meminfo") { value(100L) } + + assertEquals(setOf("thermal"), cache.refusedKeys) + } + + @Test + fun `clear forgets every refusal so the next read reaches the filesystem again`() { + val cache = RestrictionCache() + val calls = AtomicInteger(0) + val read = { + calls.incrementAndGet() + restricted(RestrictionReason.PLATFORM_RESTRICTED) + } + + cache.attempt("cpu", read) + assertTrue(cache.isRefused("cpu")) + + cache.clear() + + assertFalse(cache.isRefused("cpu")) + cache.attempt("cpu", read) + assertEquals("clearing re-opens the read", 2, calls.get()) + } + + @Test + fun `shouldAnnounce is true once per key then false forever`() { + val cache = RestrictionCache() + + assertTrue(cache.shouldAnnounce("cpu")) + assertFalse(cache.shouldAnnounce("cpu")) + assertFalse(cache.shouldAnnounce("cpu")) + assertTrue("a different metric gets its own one line", cache.shouldAnnounce("thermal")) + } + + @Test + fun `clear does not reset the announcement budget which is per session`() { + val cache = RestrictionCache() + + assertTrue(cache.shouldAnnounce("cpu")) + cache.clear() + + assertFalse("the log budget outlives an access-level change", cache.shouldAnnounce("cpu")) + } + + @Test + fun `refusalFor returns the exact Observed that the read produced`() { + val cache = RestrictionCache() + val produced = restricted(RestrictionReason.REQUIRES_ELEVATED_ACCESS) + + cache.attempt("pss") { produced } + + assertSame(produced, cache.refusalFor("pss")) + } +} diff --git a/app/src/test/java/com/processlens/feature/overview/OverviewLoadStateTest.kt b/app/src/test/java/com/processlens/feature/overview/OverviewLoadStateTest.kt new file mode 100644 index 0000000..b1d5697 --- /dev/null +++ b/app/src/test/java/com/processlens/feature/overview/OverviewLoadStateTest.kt @@ -0,0 +1,273 @@ +package com.processlens.feature.overview + +import com.processlens.core.common.AccessLevel +import com.processlens.core.common.Observed +import com.processlens.core.common.RestrictionReason +import com.processlens.domain.model.Availability +import com.processlens.domain.model.Capability +import com.processlens.domain.model.CapabilityStatus +import com.processlens.domain.model.RootState +import com.processlens.domain.model.ShizukuState +import com.processlens.domain.model.SystemCapabilities +import com.processlens.testing.Fixtures +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Tests for the loading state machine (requirement 5 of the issue #1 spec: the + * dashboard must always resolve — it must never sit forever on "Reading system + * state"). + * + * These are pure-function tests, which is the design: the decisions that used to + * live inside a composable with an undeadlined spinner are now functions over + * [OverviewViewModel.State], so the guarantee can be asserted without a Compose + * harness or a device. + */ +class OverviewLoadStateTest { + + private fun state( + system: com.processlens.domain.repository.SystemState? = null, + loadFailure: Observed.Failed? = null, + capabilities: SystemCapabilities? = null, + ) = OverviewViewModel.State( + system = system, + loadFailure = loadFailure, + capabilities = capabilities, + ) + + private fun caps( + access: AccessLevel, + statuses: Map = mapOf( + Capability.CPU_OVERALL to CapabilityStatus( + Capability.CPU_OVERALL, + Availability.FULL, + "read from /proc/stat", + ), + ), + ): SystemCapabilities = SystemCapabilities( + apiLevel = 34, + accessLevel = access, + shizukuState = ShizukuState.NOT_INSTALLED, + rootState = if (access == AccessLevel.ROOT) RootState.GRANTED else RootState.UNAVAILABLE, + hasUsageAccess = false, + hasPhoneStatePermission = false, + hasNotificationPermission = false, + isBatteryOptimisationIgnored = false, + statuses = statuses, + ) + + // ------------------------------------------------- the three sampling outcomes + + @Test + fun `all readings succeed - the dashboard shows content`() { + val s = state(system = Fixtures.systemState()) + + assertEquals(OverviewLoadPhase.CONTENT, s.loadPhase(deadlineElapsed = false)) + assertEquals(OverviewLoadPhase.CONTENT, s.loadPhase(deadlineElapsed = true)) + } + + @Test + fun `some readings are denied - the dashboard still shows content`() { + // CPU and battery temperature restricted, the rest readable: the exact + // shape of the reporter's device, where thermal and loadavg were denied. + val s = state( + system = Fixtures.systemState( + cpuPercent = Observed.platform("SELinux denied /proc/stat", AccessLevel.SHIZUKU), + batteryTemperature = Observed.platform("thermal zone denied", AccessLevel.ROOT), + ), + ) + + assertEquals( + "one restricted metric must never block the whole screen", + OverviewLoadPhase.CONTENT, + s.loadPhase(deadlineElapsed = true), + ) + } + + @Test + fun `every reading is denied - the dashboard still resolves to content`() { + val denied = Observed.platform("SELinux enforcing", AccessLevel.ROOT) + val s = state( + system = Fixtures.systemState( + cpuPercent = denied, + batteryTemperature = denied, + rxBytes = denied, + txBytes = denied, + ownCpuPercent = denied, + ownMemoryBytes = denied, + ), + ) + + assertEquals( + "a device that denies everything shows per-metric 'Not available', never a spinner", + OverviewLoadPhase.CONTENT, + s.loadPhase(deadlineElapsed = true), + ) + } + + // ------------------------------------------------- the loading always resolves + + @Test + fun `before the deadline with no sample it is honest to show a spinner`() { + assertEquals(OverviewLoadPhase.WAITING, state().loadPhase(deadlineElapsed = false)) + } + + @Test + fun `after the deadline with no sample the spinner gives way to an explanation`() { + assertEquals( + "this is the fix for the permanent 'Reading system state' hang", + OverviewLoadPhase.UNRESOLVED, + state().loadPhase(deadlineElapsed = true), + ) + } + + @Test + fun `a thrown pipeline resolves immediately, without waiting out the deadline`() { + val s = state(loadFailure = Observed.Failed("boom", "IllegalStateException")) + + assertEquals(OverviewLoadPhase.UNRESOLVED, s.loadPhase(deadlineElapsed = false)) + } + + // ----------------------------------------------------------- loadFailureState + + @Test + fun `a failure keeps whatever content was already on screen`() { + val previous = OverviewViewModel.State( + system = Fixtures.systemState(), + cpuHistory = listOf(10f, 20f, 30f), + ) + + val folded = loadFailureState(previous, IllegalStateException("db closed")) + + assertEquals("the trend the user was reading is not thrown away", previous.system, folded.system) + assertEquals(listOf(10f, 20f, 30f), folded.cpuHistory) + assertEquals("db closed", folded.loadFailure?.detail) + assertFalse(folded.isRefreshing) + } + + @Test + fun `a failure with no message falls back to the exception type`() { + val folded = loadFailureState(OverviewViewModel.State(), NullPointerException()) + + assertEquals("NullPointerException", folded.loadFailure?.detail) + } + + // ------------------------------------------------------------- unresolved copy + + @Test + fun `the unresolved title names a failure as a failure and a wait as a wait`() { + assertEquals("No reading has arrived yet", unresolvedTitle(null)) + assertEquals( + "System state could not be read", + unresolvedTitle(Observed.Failed("x")), + ) + } + + @Test + fun `both unresolved explanations insist that nothing is being withheld`() { + assertTrue(unresolvedExplanation(null).contains("not being withheld")) + assertTrue(unresolvedExplanation(Observed.Failed("x")).contains("Nothing is being withheld")) + } + + // --------------------------------------------------- capability matrix guard + + @Test + fun `the placeholder matrix is not treated as an evaluated answer`() { + assertFalse(isCapabilityMatrixEvaluated(null)) + assertFalse( + "unknown() seeds an empty matrix so twelve combines resolve on frame one", + isCapabilityMatrixEvaluated(SystemCapabilities.unknown(34)), + ) + } + + @Test + fun `a matrix with any row is treated as evaluated`() { + assertTrue(isCapabilityMatrixEvaluated(caps(AccessLevel.NORMAL))) + } + + // --------------------------------------------------- elevated-access notice + + @Test + fun `the access notice never fires against the placeholder`() { + assertFalse(showElevatedAccessNotice(SystemCapabilities.unknown(34), isDismissed = false)) + } + + @Test + fun `the access notice fires for a probed device at normal access`() { + assertTrue(showElevatedAccessNotice(caps(AccessLevel.NORMAL), isDismissed = false)) + } + + @Test + fun `the access notice does not fire for a device already running elevated`() { + assertFalse(showElevatedAccessNotice(caps(AccessLevel.SHIZUKU), isDismissed = false)) + assertFalse(showElevatedAccessNotice(caps(AccessLevel.ROOT), isDismissed = false)) + } + + @Test + fun `a dismissed notice stays dismissed`() { + assertFalse(showElevatedAccessNotice(caps(AccessLevel.NORMAL), isDismissed = true)) + } + + // ----------------------------------------------------------- unlockedByLevels + + @Test + fun `unlockedByLevels reads each restriction's own claim and drops the un-unlockable`() { + val levels = unlockedByLevels( + Observed.of(1, com.processlens.core.common.DataSource.PROC_FS), + Observed.Restricted(RestrictionReason.PLATFORM_RESTRICTED, AccessLevel.SHIZUKU), + Observed.Restricted(RestrictionReason.NOT_PRESENT_ON_DEVICE, null), + Observed.Restricted(RestrictionReason.PERMISSION_REQUIRED, AccessLevel.NORMAL), + Observed.Restricted(RestrictionReason.REQUIRES_ELEVATED_ACCESS, AccessLevel.ROOT), + ) + + assertEquals(listOf(AccessLevel.SHIZUKU, AccessLevel.ROOT), levels) + } + + @Test + fun `unlockedByLevels over the matrix counts only improvable rows that name a level`() { + val caps = caps( + AccessLevel.NORMAL, + statuses = mapOf( + Capability.CPU_OVERALL to CapabilityStatus( + Capability.CPU_OVERALL, Availability.FULL, "ok", + ), + Capability.BATTERY_PER_APP to CapabilityStatus( + Capability.BATTERY_PER_APP, Availability.UNAVAILABLE, "needs a shell", + unlockedBy = AccessLevel.SHIZUKU, + ), + Capability.MEMORY_PER_PROCESS to CapabilityStatus( + Capability.MEMORY_PER_PROCESS, Availability.LIMITED, "partial", + unlockedBy = AccessLevel.SHIZUKU, + ), + Capability.CPU_TEMPERATURE to CapabilityStatus( + Capability.CPU_TEMPERATURE, Availability.UNAVAILABLE, + "no thermal zone", unlockedBy = null, + ), + ), + ) + + assertEquals(listOf(AccessLevel.SHIZUKU), unlockedByLevels(caps)) + assertEquals( + "LIMITED and UNAVAILABLE both count, the un-unlockable row does not", + 2, + countUnlockable(caps), + ) + } + + @Test + fun `nothing is unlockable when every row is full`() { + assertEquals(0, countUnlockable(caps(AccessLevel.ROOT))) + assertTrue(unlockedByLevels(caps(AccessLevel.ROOT)).isEmpty()) + } + + @Test + fun `a value carries no unlock suggestion`() { + assertNull( + unlockedByLevels(Observed.of(5, com.processlens.core.common.DataSource.PROC_FS)) + .firstOrNull(), + ) + } +} diff --git a/docs/ISSUE1_ROOT_CAUSE.md b/docs/ISSUE1_ROOT_CAUSE.md new file mode 100644 index 0000000..d85c6c8 --- /dev/null +++ b/docs/ISSUE1_ROOT_CAUSE.md @@ -0,0 +1,108 @@ +# Issue #1 — "Not loading until probed SU: Reading system state" + +Root-cause note. Written before the fix, from four independent reads of the source. + +## Summary + +**The SELinux denials are not the cause.** They are real, and they are a second defect worth +fixing, but the hang happens on every device regardless of them. + +Overview hangs because `OverviewViewModel.state` is a `combine()` of five flows, and one of those +flows — `observeCapabilities()` — never emits on a cold start. `kotlinx` `combine` is all-or-nothing: +it publishes nothing until *every* source has emitted at least once. So the state never leaves +`initialValue = State()`, where `system == null`, and `OverviewScreen` renders `LoadingBlock()` +— default label `"Reading system state"` — forever. + +The reporter's own hypothesis ("the non-root collector treats denied reads as not-ready and retries +forever") is **refuted**. Every `/proc` and `/sys` read funnels through `ProcFsReader.readFile`, +which catches `SecurityException` and EACCES-bearing `IOException` and returns a terminal +`Observed.Restricted`. No read site retries, blocks, or asserts non-null. A denied `loadavg` and a +denied thermal zone still produce a complete `SystemState`. Those samples were being computed +correctly the whole time — and silently discarded by the starved `combine`. + +## The causal chain + +1. `SystemRepositoryImpl.kt:51` — `private val capabilities = MutableStateFlow(null)` +2. `SystemRepositoryImpl.kt:111-112` — `observeCapabilities() = capabilities.asStateFlow().filterNotNull().distinctUntilChanged()`. + `filterNotNull()` swallows the seeded `null`, so the flow is **silent until something writes the field**. +3. The only writer is `refreshCapabilities()` (`:114-118`). **Nothing on the cold-start path calls it.** + `ProcessLensApplication` only installs the crash handler; `MainActivity.onCreate` only calls + `setContent`; `OverviewViewModel` has no `init {}` block; `ProcessLensNavHost` has no `LaunchedEffect`. + All ten real call sites are user-initiated actions on the Access, Capabilities, Network or Settings screens. +4. `OverviewViewModel.kt:86-92` — a 5-arity `combine` whose second source is that silent flow. The four + chained `.combine` operators for favourites, history, refreshing and errors are all *downstream* of + the gate, so their emissions cannot produce output either. +5. `OverviewViewModel.kt:112-118` — `stateIn(..., initialValue = State())` therefore publishes `State()` + once and never again. `State.system` is `null`; `isFirstLoad` is defined as `system == null` (`:58`). +6. `OverviewScreen.kt:129-133` — `if (system == null) { LoadingBlock(); return@Column }`. An + unconditional gate with no deadline, no error branch and no retry affordance. + `LoadingBlock`'s default label is `"Reading system state"` (`ScreenScaffold.kt:203`). + +`ScreenHeader` is composed *above* that early return, which is why the title still shows and the app +looks alive rather than frozen — matching "no crash, no FATAL EXCEPTION". + +## Why "Probe root" appears to fix it + +It is not the root grant. Navigating to Settings → Access constructs `AccessViewModel`, whose +`init { refresh() }` (`AccessViewModel.kt:104-106, 127-132`) calls `invalidateAccess()` then +`refreshCapabilities()`. That writes `capabilities.value`, and because `SystemRepositoryImpl` is a +`@Singleton` the flow stays non-null for the rest of the process — so Overview's `combine` finally +receives its missing input and the dashboard renders. **Merely opening the Access screen is enough; +the button is incidental.** Relaunching resets the in-memory singleton to `null`, which is exactly why +the hang reproduces on every launch. + +Proof that root is not involved: `probeRoot()` sets `probedState = GRANTED`, then its own +`refreshCapabilitiesOnly()` calls `invalidateAccess()` → `CompositeSystemObserver.invalidate()` → +`RootShell.invalidate()`, which wipes the grant (`RootShell.kt:73-75`) *before* routing is +re-evaluated. `resolve()` then sees `BINARY_PRESENT.isUsable == false` (`Capability.kt:188`) and keeps +the standard observer — yet Overview renders anyway. + +## Blast radius + +**Eleven** ViewModels `combine()` on `observeCapabilities()` and share the identical gate: +`AccessViewModel:79`, `AppDetailViewModel:88`, `BatteryViewModel:84`, `CapabilitiesViewModel:69`, +`CpuViewModel:55`, `InvestigateViewModel:131`, `MemoryViewModel:49`, `NetworkViewModel:85`, +`OverviewViewModel:88`, `ProcessDetailViewModel:82`, `AppShellViewModel:48`. + +`AppShellViewModel` gates the theme and the first-run onboarding overlay app-wide; it survives only +because its `State()` default carries real defaults. `CapabilitiesViewModel` is the one screen whose +entire job is showing capabilities. Overview is simply the screen the user lands on first. + +## The fix was already written and never wired up + +`SystemCapabilities.unknown(apiLevel)` exists at `Capability.kt:158-170`. Its KDoc reads verbatim +*"Used only as an initial UI state before the first real evaluation"*, and `statuses = emptyMap()` +makes every `get()` fall back to the `"Not evaluated on this device."` status, so it is designed to be +safe to render. A grep for `unknown(` across `app/src` finds two hits: the declaration, and one +androidTest. **Zero production callers.** `OverviewViewModel.revalidateAccess()` (`:161-166`) — which +would have broken the deadlock — likewise has zero callers and is dead code. + +A second piece of evidence that the placeholder is the intended design: `AppShellViewModel.summarise()` +already opens with `if (total == 0) return "Checking what this device allows…"`, and `total` is the sum +of the three `statuses` counts. That branch is reachable *only* from a capabilities object with an empty +`statuses` map — which is exactly what `unknown()` produces and what nothing was ever emitting. The +author wrote the handler for this state, wrote the value for this state, and then gated the flow so +neither could ever be used. + +The empty-`statuses` signal is also how a consumer tells "not yet evaluated" apart from "evaluated, +and this device allows nothing" — a distinction the no-root banner depends on, since `unknown()` +reports `accessLevel = NORMAL` and `rootState = UNAVAILABLE` before any detection has run. + +## Secondary defects found + +| # | Defect | Location | Consequence | +|---|---|---|---| +| 2 | No session memo of a permanent denial. The 2 s poll re-reads `/proc/loadavg` and re-walks `/sys/class/thermal` on **every tick**. | `SystemRepositoryImpl.kt:61-73`, `ProcFsReader.kt:99-115, 309-338` | The 50 minutes of repeating `avc: denied` in the report. Wasted work and log spam, not a hang. The flow runs on the injected `Dispatchers.Default`, whose workers are named `DefaultDispatcher-worker-N` — truncated to `comm="DefaultDispatch"` in the denial lines. | +| 3 | Both shells drain stdout to EOF, *then* stderr, *then* consult the timeout. | `RootShell.kt:103-111`, `ShizukuShell.kt:131-143` | `timeoutMillis` is unreachable. A child that fills the 64 KiB stderr pipe blocks on write, so stdout never reaches EOF and the read never returns — a genuine deadlock. An `su` awaiting an unanswered Magisk prompt blocks forever, not for 10 s. | +| 4 | `invalidateAccess()` wipes a just-obtained root grant before routing re-evaluates. | `RootShell.kt:73-75` via `CompositeSystemObserver.kt:89-92` | Root never actually engages, even after a successful grant. | +| 5 | No `.catch {}` anywhere in the codebase; no try/catch around any flow collection. | all ViewModels | A throwing collector would fail silently and leave the same permanent spinner. | + +## Fix plan + +| Item | Requirement | Files | +|---|---|---| +| F1 | R2, R5 — loading always resolves | `data/repository/SystemRepositoryImpl.kt` | +| F2 | R1, R3, R7 — terminal per-metric unavailability, public APIs, one log line per metric | new `core/system/RestrictionCache.kt`, `core/system/ProcFsReader.kt`, `core/system/StandardAndroidObserver.kt` | +| F3 | R4 — real timeout, concurrent drain, destroy on timeout | new `core/system/ProcessRunner.kt`, `core/system/RootShell.kt`, `core/system/ShizukuShell.kt` | +| F4 | defect 4 — keep a proven grant across a route invalidation | `core/system/CompositeSystemObserver.kt` | +| F5 | R6 — N/A wording, no-root banner, reachable re-probe | `feature/overview/OverviewScreen.kt`, `feature/overview/OverviewViewModel.kt`, `core/designsystem/ScreenScaffold.kt` | diff --git a/gradle.properties b/gradle.properties index 61825b5..90631d1 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,22 +1,18 @@ -# ---- Memory-conservative config for an on-device aarch64 build host ---- -# The Kotlin compiler runs in-process so a second daemon does not compete for RAM. -org.gradle.jvmargs=-Xmx1536m -XX:MaxMetaspaceSize=512m -Dfile.encoding=UTF-8 -org.gradle.daemon=false -org.gradle.parallel=false -org.gradle.caching=false -org.gradle.configureondemand=false +# Portable project settings only. +# +# Host-specific configuration — the JDK location, an aapt2 override for a build +# host that is not x86_64, heap caps, the daemon and the Kotlin compiler strategy +# — belongs in GRADLE_USER_HOME/gradle.properties (~/.gradle/gradle.properties), +# which Gradle resolves ahead of this file. Keeping it out of the repository is +# what lets one checkout build both on a CI runner and on an on-device ARM host +# without either one having to edit the other's settings. It used to live here, +# and a CI runner reading it would have failed on the first line that named a +# path only the authoring device has. -kotlin.compiler.execution.strategy=in-process - -# ---- Toolchain pin ---- -# This host's default JDK is newer than the Kotlin 2.0.x compiler's bundled -# IntelliJ utilities can parse. AGP 8.5 targets JDK 17 anyway, so pin explicitly. -org.gradle.java.home=/usr/lib/jvm/java-17-openjdk-arm64 - -# ---- aarch64 build-host fix ---- -# AGP downloads an x86_64 aapt2 from Maven, which cannot execute on an ARM device. -# Point it at the native aapt2 that ships with the distro's android-sdk packages. -android.aapt2FromMavenOverride=/usr/bin/aapt2 +# AGP needs more than Gradle's default heap to run KSP and Compose in one +# invocation. A memory-constrained host should override this downward in +# GRADLE_USER_HOME, not lower it here for everyone. +org.gradle.jvmargs=-Xmx3g -XX:MaxMetaspaceSize=768m -Dfile.encoding=UTF-8 android.useAndroidX=true android.nonTransitiveRClass=true