From 4cdf645989e980a7a961a9b21bb7afd685b78c81 Mon Sep 17 00:00:00 2001 From: root Date: Wed, 7 Oct 2026 08:26:16 -0400 Subject: [PATCH 1/9] docs: root-cause note for issue #1 Records the investigation before any code changes, from four independent reads of the source. The headline finding is that the SELinux denials in the report are not the cause. Overview hangs because OverviewViewModel.state is a combine() of five flows and one of them, observeCapabilities(), never emits on a cold start: the backing MutableStateFlow is seeded null and exposed through filterNotNull(), and nothing on the launch path writes it. combine publishes nothing until every source has emitted, so the state never leaves its initial value and the screen renders its loading block forever. That also explains why "Probe root" appears to fix it. Navigating to Settings > Access constructs AccessViewModel, whose init refreshes capabilities; the repository is a singleton, so the flow stays non-null for the rest of the process. Merely opening the screen is enough, and relaunching resets it, which is why the hang reproduces every launch. Four secondary defects are recorded with their consequences, including the denial re-read on every poll tick that produced the repeating avc lines, and a genuine pipe deadlock in both shells that makes their timeouts unreachable. --- docs/ISSUE1_ROOT_CAUSE.md | 97 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 docs/ISSUE1_ROOT_CAUSE.md diff --git a/docs/ISSUE1_ROOT_CAUSE.md b/docs/ISSUE1_ROOT_CAUSE.md new file mode 100644 index 0000000..0a10fc8 --- /dev/null +++ b/docs/ISSUE1_ROOT_CAUSE.md @@ -0,0 +1,97 @@ +# 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 + +**Twelve** 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. + +## 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` | From 9f3394841e98772dcdcdce4f2d40773d44f4d4b8 Mon Sep 17 00:00:00 2001 From: root Date: Wed, 7 Oct 2026 08:26:39 -0400 Subject: [PATCH 2/9] fix: make the capabilities flow emit before detection completes This is the fix for the hang in issue #1. observeCapabilities() was capabilities.asStateFlow().filterNotNull(), over a MutableStateFlow seeded with null. The filter swallowed the seed, so the flow stayed silent until refreshCapabilities() happened to run. Twelve view models combine() on it, and combine emits nothing until every source has produced a value, so all twelve sat on their initial state. On Overview that is system == null, which renders LoadingBlock("Reading system state") behind an unconditional early return. The repository now seeds the flow with SystemCapabilities.unknown(SDK_INT), which already existed for exactly this purpose and whose KDoc says so: its statuses map is empty, so every lookup falls back to "Not evaluated on this device" and it renders as honest absence rather than invented capability. With a real first value on the wire, combine resolves immediately and the screen draws its content while detection is still in flight. The first collector kicks that detection once per process via the application scope, so a screen leaving the composition mid-detection cannot cancel work the next screen needs, and a detection that throws leaves the placeholder in place instead of restoring the spinner. refreshCapabilities() also marks the kick as spent, so a caller that beats the first collector does not cause a second evaluation. Detection cannot prompt for root: RootShell.probe() is the only su spawn and its sole caller is the explicit "Probe root" action on the Access screen. --- .../data/repository/SystemRepositoryImpl.kt | 52 +++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) 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 From 2109ed994f282c94a543be2d002c530e7cbc43f6 Mon Sep 17 00:00:00 2001 From: root Date: Wed, 7 Oct 2026 08:39:59 -0400 Subject: [PATCH 3/9] build: keep host-specific Gradle config out of the repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gradle.properties carried the authoring device's own setup: an absolute org.gradle.java.home, an aapt2FromMavenOverride pointing at /usr/bin/aapt2 for an aarch64 host, and heap and daemon caps sized for roughly 1.8 GB of free RAM. Those settings are correct for that machine and wrong everywhere else — a CI runner reading them fails on the first path that only the authoring device has. They are also redundant there. Gradle resolves GRADLE_USER_HOME/gradle.properties ahead of the project file, and that is where this host already defines all of them, so removing them from the repository changes nothing about how it builds locally while making the checkout portable. What stays is the portable half: AndroidX and non-transitive R classes, plus a heap large enough for KSP and Compose in one invocation. A constrained host overrides that downward in its own GRADLE_USER_HOME rather than lowering it in the repository for everyone. --- gradle.properties | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) 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 From 125df5ada3b352f95c6853bc656d9c163ce84b9d Mon Sep 17 00:00:00 2001 From: root Date: Wed, 7 Oct 2026 08:39:59 -0400 Subject: [PATCH 4/9] build: enable R8 and resource shrinking for release by default Release builds had isMinifyEnabled and isShrinkResources both false. The comment explained it as a build-host concession: R8 is memory-hungry and the on-device ARM host cannot run it. That is true, but it meant the shipped artifact was unobfuscated and unshrunk because of where it happened to be compiled, which is a property of the machine leaking into the product. Both now default to on and are driven by a processlens.minify property. The constrained host sets processlens.minify=false in its own GRADLE_USER_HOME, so the opt-out stays on that one machine and every other build, CI included, gets a minified and shrunk release. The keep-rules in proguard-rules.pro were already written for Hilt, Room, Shizuku and Compose, so they now actually apply. Shrinking resources requires shrinking code, so the two are driven by one flag rather than being independently settable into an invalid combination. --- app/build.gradle.kts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) 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" From b70994d12922195a8ee9ae93b53bb9cbdb4d3f37 Mon Sep 17 00:00:00 2001 From: root Date: Wed, 7 Oct 2026 08:40:00 -0400 Subject: [PATCH 5/9] ci: build and upload a debug APK on push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was no .github directory at all, so nothing verified that a change compiled or that the tests passed outside the author's device. The workflow runs the unit tests first and then assembles the debug APK, uploading the APK as an artifact and the test report unconditionally — the report is wanted precisely when the tests fail. Pinned to JDK 17 to match AGP 8.5 and the Kotlin 2.0 toolchain. Concurrency is grouped by ref so a newer push cancels the run it supersedes. It deliberately does not patch any Gradle setting before building; that is only possible because the host-specific configuration no longer lives in the repository. --- .github/workflows/build.yml | 72 +++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 .github/workflows/build.yml 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 From 4673d44545bf78bf5eb56d6a6106fd6983dd7f42 Mon Sep 17 00:00:00 2001 From: root Date: Wed, 7 Oct 2026 08:42:55 -0400 Subject: [PATCH 6/9] docs: correct the consumer count and record the placeholder evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The note said twelve view models combine() on observeCapabilities() and then listed eleven. Eleven is right. Also records a second piece of evidence that the placeholder value was the intended design rather than a convenient fix: AppShellViewModel.summarise() already opens with a total == 0 branch returning "Checking what this device allows...", and that total is the sum of the three statuses counts, so the branch is reachable only from a capabilities object with an empty statuses map — precisely what SystemCapabilities.unknown() produces and what nothing was emitting. The handler, the value and the gate that made both unreachable were all written by the same hand. Notes that the empty statuses map is also the signal a consumer needs to tell "not yet evaluated" from "evaluated, nothing allowed", since the placeholder reports NORMAL access and UNAVAILABLE root before any detection has run. --- docs/ISSUE1_ROOT_CAUSE.md | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/docs/ISSUE1_ROOT_CAUSE.md b/docs/ISSUE1_ROOT_CAUSE.md index 0a10fc8..d85c6c8 100644 --- a/docs/ISSUE1_ROOT_CAUSE.md +++ b/docs/ISSUE1_ROOT_CAUSE.md @@ -59,7 +59,7 @@ the standard observer — yet Overview renders anyway. ## Blast radius -**Twelve** ViewModels `combine()` on `observeCapabilities()` and share the identical gate: +**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`. @@ -77,6 +77,17 @@ safe to render. A grep for `unknown(` across `app/src` finds two hits: the decla 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 | From ac1e50afacd5b9d6d7f17cd6a45aa602d6ecf1c9 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 9 Oct 2026 04:53:33 -0400 Subject: [PATCH 7/9] fix: cache per-metric restrictions so a denied read is attempted once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A refusal is an answer. When SELinux denies /proc/loadavg or the thermal nodes, the kernel returns the same denial to every subsequent identical read, but the sampler asked again every one to ten seconds — one syscall, one audit record and one `avc: denied` line per attempt. A field bug report carried fifty minutes of that spam; nothing was learned after the first. RestrictionCache remembers the first terminal refusal per logical metric and returns the same Observed.Restricted to later callers without touching the filesystem. Only platform-shaped reasons are terminal: PERMISSION_REQUIRED and SAMPLING_DISABLED stay revocable, and Observed.Failed is never memoised so a single bad sample cannot blank a screen for the session. The cache is clearable so a root/Shizuku grant re-opens every read. --- .../processlens/core/system/ProcFsReader.kt | 147 ++++++++++++-- .../core/system/RestrictionCache.kt | 134 +++++++++++++ .../core/system/RestrictionCacheTest.kt | 186 ++++++++++++++++++ 3 files changed, 451 insertions(+), 16 deletions(-) create mode 100644 app/src/main/java/com/processlens/core/system/RestrictionCache.kt create mode 100644 app/src/test/java/com/processlens/core/system/RestrictionCacheTest.kt 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/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/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")) + } +} From 2ab2fc81fd975c65ae1957ffdb48bcb39fe8c628 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 9 Oct 2026 04:54:21 -0400 Subject: [PATCH 8/9] fix: give shell probes a real deadline and preserve a proven root grant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both elevated shells drained stdout to EOF, then stderr, then consulted the timeout — so the timeout was never reached while an `su` sat on an unanswered superuser prompt, and a child that filled its ~64 KiB stderr pipe deadlocked because it never closed stdout. ProcessRunner now drains both pipes concurrently in a scope it owns, enforces the deadline by destroying the process (the only thing that releases a reader parked in a blocking read()), and tears down every stream on every exit path exactly once. Probes run on Dispatchers.IO under a 3s budget; a timeout, denial or absent binary all come back as a non-zero result the caller carries on without. Routing no longer wipes a grant it was just handed: RootShell models its probe result as Observed, invalidate() keeps a proven Value grant while clearing stale negatives, and forgetGrant() drops even a proven grant when the user turns root support off. CompositeSystemObserver and CapabilityDetector drop the restriction cache on an access-level change so a grant re-opens reads. --- .../core/system/CapabilityDetector.kt | 8 + .../core/system/CompositeSystemObserver.kt | 19 +- .../processlens/core/system/ProcessRunner.kt | 448 ++++++++++++++++++ .../com/processlens/core/system/RootShell.kt | 217 +++++++-- .../processlens/core/system/ShizukuShell.kt | 115 +---- .../core/system/ProcessRunnerTest.kt | 222 +++++++++ 6 files changed, 882 insertions(+), 147 deletions(-) create mode 100644 app/src/main/java/com/processlens/core/system/ProcessRunner.kt create mode 100644 app/src/test/java/com/processlens/core/system/ProcessRunnerTest.kt 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/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/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/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) + } + } +} From a978de40cd0cac085c74d7850f5e45ece4662d59 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 9 Oct 2026 04:55:09 -0400 Subject: [PATCH 9/9] fix: make the Overview dashboard always resolve its loading state The permanent "Reading system state" of issue #1 was a starved combine() upstream (fixed in the repository), but it stayed invisible because nothing on this screen questioned a silent source: a thrown collector took the whole flow down without a trace, and the only reaction to "no state yet" was a spinner with no deadline. Three things now hold. A thrown pipeline is folded by .catch into an Observed.Failed the screen renders through the same Section 42 components as any other unreadable value, keeping whatever content was already shown. Collection is restartable via an attempts counter behind flatMapLatest, so the retry the user is offered genuinely re-runs the sources. And the screen bounds its own wait: a spinner gives way to an UnresolvedBlock after a deadline, with a real re-probe. The capability placeholder is kept distinguishable from a probed "no elevated access" so the dashboard never accuses a device of restrictions it has not yet been asked about. --- .../core/designsystem/ScreenScaffold.kt | 91 +++++ .../feature/overview/OverviewScreen.kt | 80 ++++- .../feature/overview/OverviewViewModel.kt | 335 +++++++++++++++++- .../feature/overview/OverviewLoadStateTest.kt | 273 ++++++++++++++ 4 files changed, 761 insertions(+), 18 deletions(-) create mode 100644 app/src/test/java/com/processlens/feature/overview/OverviewLoadStateTest.kt 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/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/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(), + ) + } +}