diff --git a/android/app/src/main/kotlin/tech/kadence/android/blocker/BlockerEngine.kt b/android/app/src/main/kotlin/tech/kadence/android/blocker/BlockerEngine.kt index 806a9ae..8e67732 100644 --- a/android/app/src/main/kotlin/tech/kadence/android/blocker/BlockerEngine.kt +++ b/android/app/src/main/kotlin/tech/kadence/android/blocker/BlockerEngine.kt @@ -111,6 +111,13 @@ class BlockerEngine( private val configured: () -> Boolean, /** The running interval, if any — `during_work` cannot be decided without it. */ private val timerFacts: () -> IntervalFacts?, + /** + * Wall clock for the two cooldowns, injectable so the tests can step it. + * + * Not `nanoTime`: these windows are seconds long and a dozy phone's monotonic clock + * is not the thing being measured — "has twenty seconds of my day passed" is. + */ + private val now: () -> Long = System::currentTimeMillis, ) { private val _state = MutableStateFlow(BlockerUi()) val state: StateFlow = _state.asStateFlow() @@ -147,15 +154,29 @@ class BlockerEngine( private var leaveForegroundAppFn: (() -> Unit)? = null /** - * Targets we have already dealt with, and when. + * Targets we have already **reported**, and when. * * The same page fires `TYPE_WINDOW_CONTENT_CHANGED` several times a second and the * same app sits in front for minutes; without this the report would fill with one * entry per event. Keyed by `kind:target`, so the cooldowns do not collide. + * + * It throttles the *report*, and nothing else. It used to sit in front of the + * enforcement call as well, which is a different thing entirely: for twenty seconds + * after the first bounce the site or app was simply free, so opening it a second + * time worked. [enforcing] is the only cooldown enforcement consults, and it is + * barely longer than one event burst. */ private val recentlyBlocked = ConcurrentHashMap() + /** When each target was last sent home, so one visit does not fire home twice. */ + private val recentlyEnforced = ConcurrentHashMap() + + // Written from the accessibility service's main-thread callback and from the + // app-poll coroutine both, like everything else in this class. + @Volatile private var lastForeground: String? = null + + @Volatile private var lastBlockedApp: String? = null private var pollJob: Job? = null @@ -204,8 +225,17 @@ class BlockerEngine( // ---- the accessibility service's two entry points ----------------------- /** Wire the service's ability to send the user home while [service] is connected. */ - fun attachCloser(service: BlockerAccessibilityService) { - leaveForegroundAppFn = { service.goHome() } + fun attachCloser(service: BlockerAccessibilityService) = attachCloser { service.goHome() } + + /** + * The same, as a bare function. + * + * Enforcement is the half of this class that was broken, so it has to be reachable + * from a test — and a real [BlockerAccessibilityService] is not something a JVM test + * can bind. + */ + fun attachCloser(closer: () -> Unit) { + leaveForegroundAppFn = closer } fun detachCloser() { @@ -222,6 +252,14 @@ class BlockerEngine( if (!ui.active) return val domains = matchers().first val decision = domains.decide(candidate) as? BlockDecision.Block ?: return + + // Enforce first, and on every visit: the report's cooldown has no business + // deciding whether a blocked site opens. `goHome()` does not close the tab, so + // coming back to it through Recents re-fires the window event and lands here + // again — which is the behaviour anyone would expect from a blocker. + if (enforcing("domain:${decision.target}")) { + leaveForegroundAppFn?.invoke() + } if (!claim("domain:${decision.target}", DOMAIN_COOLDOWN_MS)) return report("domain", decision.target, BlockAction.BLOCKED, browserPackage ?: "browser") @@ -229,7 +267,6 @@ class BlockerEngine( decision.target, "Blocked by `${decision.matchedRule}`.", ) - leaveForegroundAppFn?.invoke() } /** @@ -238,6 +275,11 @@ class BlockerEngine( * Reporting is once per *visit*, not once per poll: [lastBlockedApp] is cleared the * moment the foreground moves anywhere else, so reopening a blocked app counts * again while sitting in it does not. + * + * Enforcement is *not* once per visit, and not on the report's cooldown either: if + * the app is still in front on the next poll, home is pressed again. One + * `GLOBAL_ACTION_HOME` is a request, not a proof — an app that survives it, or that + * the user walks straight back into, must not be left alone for twenty seconds. */ fun onForegroundApp(packageName: String, label: String = "") { if (packageName == context.packageName) { @@ -253,8 +295,19 @@ class BlockerEngine( val decision = matchers().second.decide(packageName, label, ui.killApps) if (decision !is BlockDecision.Block) return - if (packageName == lastBlockedApp) return - lastBlockedApp = packageName + + val eject = decision.action == BlockAction.KILLED + if (eject) { + // Every pass, debounced only against this one visit's duplicate signals. + // Whether the attempt is worth *reporting* is decided below and separately; + // conflating the two is what let the second launch through. + if (enforcing("app:${decision.target}")) leaveForegroundAppFn?.invoke() + } else { + // Warn mode ejects nothing, so there is nothing to repeat: one notice per + // visit, cleared when the foreground moves on. + if (packageName == lastBlockedApp) return + lastBlockedApp = packageName + } if (!claim("app:${decision.target}", APP_COOLDOWN_MS)) return report( @@ -263,9 +316,8 @@ class BlockerEngine( action = decision.action, details = if (label.isBlank()) packageName else "$label ($packageName)", ) - if (decision.action == BlockAction.KILLED) { + if (eject) { announce(decision.target, "Closed while it is on the block list.") - leaveForegroundAppFn?.invoke() } else { announce(decision.target, "On the block list — `kill_blocked_apps` is off.") } @@ -298,6 +350,9 @@ class BlockerEngine( } private suspend fun appLoop() { + // No initial delay: if the guard is already on when the process starts — a + // scheduled window, or an interval running on the desktop — the app in front + // right now is the one that needs looking at. while (scope.isActive) { // Only while enforcing: usage stats are a battery cost worth paying when a // block is possible, and pure waste when it is not. @@ -305,10 +360,22 @@ class BlockerEngine( val pkg = SystemProbes.foregroundPackage(context) if (pkg != null) onForegroundApp(pkg, SystemProbes.appLabel(context, pkg)) } - delay(APP_POLL_MS) + delay(appPollMs()) } } + /** + * How long to wait between usage-stats looks. + * + * The server says how often it wants the app list checked + * (`blocker.app_poll_seconds`); [APP_POLL_MS] is what we use before the first policy + * lands, and the bounds are because this value comes from a document a human edits. + */ + private fun appPollMs(): Long { + val asked = policy?.pollSeconds?.times(1000L) ?: return APP_POLL_MS + return asked.coerceIn(MIN_APP_POLL_MS, MAX_APP_POLL_MS) + } + /** Settings are read once on start and after every edit, not on every poll. */ private suspend fun loadSettings() { if (!configured()) return @@ -445,14 +512,31 @@ class BlockerEngine( * accessibility service's main-thread callback and from the app-poll coroutine at * once, and the only shared fact is a timestamp. */ + /** + * Whether to act on this target now. + * + * Not a cooldown in the [claim] sense — a debounce. Its only job is to stop the + * accessibility event and the usage-stats poll both pressing home for the same + * visit, which would bounce the launcher as well. Short enough that a deliberate + * second attempt always gets through. + */ + private fun enforcing(target: String): Boolean { + val stamp = now() + val previous = recentlyEnforced[target] + if (previous != null && stamp - previous < ENFORCE_DEBOUNCE_MS) return false + recentlyEnforced[target] = stamp + return true + } + private fun claim(target: String, windowMs: Long): Boolean { - val now = System.currentTimeMillis() + val stamp = now() while (true) { val previous = recentlyBlocked[target] when { - previous != null && now - previous < windowMs -> return false - previous == null -> if (recentlyBlocked.putIfAbsent(target, now) == null) return true - recentlyBlocked.replace(target, previous, now) -> return true + previous != null && stamp - previous < windowMs -> return false + previous == null -> + if (recentlyBlocked.putIfAbsent(target, stamp) == null) return true + recentlyBlocked.replace(target, previous, stamp) -> return true } } } @@ -465,9 +549,16 @@ class BlockerEngine( /** Usage stats update in coarse buckets; more often than this buys nothing. */ const val APP_POLL_MS = 4_000L - /** One bounce per target at a time — the URL bar re-fires constantly. */ + /** Nobody's `app_poll_seconds` may spin the loop or let a game run a minute. */ + const val MIN_APP_POLL_MS = 1_000L + const val MAX_APP_POLL_MS = 30_000L + + /** One *report* per target at a time — the URL bar re-fires constantly. */ const val DOMAIN_COOLDOWN_MS = 20_000L const val APP_COOLDOWN_MS = 20_000L + + /** How long one visit's duplicate signals are folded into one bounce. */ + const val ENFORCE_DEBOUNCE_MS = 1_200L } } diff --git a/android/app/src/test/kotlin/tech/kadence/android/BlockerEngineTest.kt b/android/app/src/test/kotlin/tech/kadence/android/BlockerEngineTest.kt new file mode 100644 index 0000000..d05dab4 --- /dev/null +++ b/android/app/src/test/kotlin/tech/kadence/android/BlockerEngineTest.kt @@ -0,0 +1,174 @@ +package tech.kadence.android + +import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.runBlocking +import mockwebserver3.MockResponse +import mockwebserver3.MockWebServer +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import tech.kadence.android.blocker.BlockerEngine +import tech.kadence.android.data.KadenceRepository +import tech.kadence.android.data.net.AuthConfig +import tech.kadence.android.data.net.KadenceApi +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * The guard's enforcement, which is the half that was broken. + * + * One bug, in two places: the twenty-second cooldown that keeps the daily report + * readable was also gating `goHome()`. Blocking worked exactly once per target per + * twenty seconds — open the app or the site again and nothing happened at all, which is + * the whole of "the blocker lets the second launch through". + * + * So these tests step a fake clock and count bounces against reports. They need no + * device and no backend: MockWebServer serves the committed policy fixture, as the rest + * of this suite does. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class BlockerEngineTest { + + private lateinit var server: MockWebServer + private var clock = 1_000_000L + private var bounces = 0 + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined) + + private fun fixture(name: String): String = + checkNotNull(javaClass.classLoader.getResourceAsStream("fixtures/$name")) + .bufferedReader().readText() + + private fun engine(withCloser: Boolean = true): BlockerEngine { + val api = KadenceApi(config = { + object : AuthConfig { + override val baseUrl = server.url("/").toString().trimEnd('/') + override val apiToken = "token" + } + }) + val engine = BlockerEngine( + context = ApplicationProvider.getApplicationContext(), + repo = KadenceRepository(api), + scope = scope, + configured = { true }, + timerFacts = { null }, + now = { clock }, + ) + if (withCloser) engine.attachCloser { bounces++ } + // One poll, so the engine holds the server's "a work interval is running". + runBlocking { engine.refreshNow().join() } + assertTrue(engine.state.value.active, "the fixture says the guard is on") + return engine + } + + @Before + fun setUp() { + server = MockWebServer() + server.start() + server.dispatcher = object : mockwebserver3.Dispatcher() { + override fun dispatch(request: mockwebserver3.RecordedRequest): MockResponse { + val path = request.url.encodedPath + val body = when { + path.endsWith("/api/blocker/policy") -> fixture("blocker_policy_active.json") + path.endsWith("/api/settings") -> fixture("settings.json") + else -> "{}" + } + return MockResponse.Builder() + .code(200) + .addHeader("Content-Type", "application/json") + .body(body) + .build() + } + } + } + + @After + fun tearDown() { + server.close() + } + + @Test + fun `a blocked app is ejected every time it is opened, not once per cooldown`() { + val engine = engine() + + engine.onForegroundApp("com.valvesoftware.steam", "Steam") + assertEquals(1, bounces) + + // Home was pressed, so the launcher is in front: the visit is over. + engine.onForegroundApp("com.android.launcher", "Launcher") + // A second launch, well inside the twenty-second report cooldown. + clock += 3_000 + engine.onForegroundApp("com.valvesoftware.steam", "Steam") + + assertEquals(2, bounces, "the second launch must be ejected too") + } + + @Test + fun `sitting in a blocked app keeps it being ejected`() { + val engine = engine() + + // What the usage-stats poll produces: the same package, over and over. + repeat(4) { + engine.onForegroundApp("com.valvesoftware.steam", "Steam") + clock += 4_000 + } + + assertEquals(4, bounces, "one press of home is a request, not a result") + } + + @Test + fun `one visit's duplicate signals are folded into a single bounce`() { + val engine = engine() + + // The accessibility event and the app poll both land on the same visit. + engine.onForegroundApp("com.valvesoftware.steam", "Steam") + clock += 200 + engine.onForegroundApp("com.valvesoftware.steam", "Steam") + + assertEquals(1, bounces) + } + + @Test + fun `a blocked site is bounced on every visit`() { + val engine = engine() + + engine.onUrlSeen("youtube.com", "com.android.chrome") + clock += 1_500 + engine.onUrlSeen("youtube.com", "com.android.chrome") + + assertEquals(2, bounces, "goHome does not close the tab — coming back must bounce") + } + + @Test + fun `a missing accessibility service does not look like a successful block`() { + val engine = engine(withCloser = false) + + engine.onForegroundApp("com.valvesoftware.steam", "Steam") + engine.onForegroundApp("com.android.launcher", "Launcher") + clock += 3_000 + engine.onForegroundApp("com.valvesoftware.steam", "Steam") + + assertEquals(0, bounces, "nothing to enforce with") + // And when the grant arrives, the very next look enforces. + engine.attachCloser { bounces++ } + clock += 3_000 + engine.onForegroundApp("com.valvesoftware.steam", "Steam") + assertEquals(1, bounces) + } + + @Test + fun `an app that is not on the list is left alone`() { + val engine = engine() + + engine.onForegroundApp("org.gnu.emacs", "Emacs") + engine.onUrlSeen("kadence.timka.su", "com.android.chrome") + + assertEquals(0, bounces) + } +} diff --git a/desktop/kadence_desktop/blocking/manager.py b/desktop/kadence_desktop/blocking/manager.py index fee2823..fdfdb3b 100644 --- a/desktop/kadence_desktop/blocking/manager.py +++ b/desktop/kadence_desktop/blocking/manager.py @@ -25,6 +25,8 @@ machine spends a moment asking a dead port. from __future__ import annotations import logging +import time +from types import ModuleType from typing import Callable from kadence_desktop.blocking import processes @@ -43,17 +45,35 @@ class BlockerManager: notify: Callable[[str, str], None] | None = None, stub: StubResolver | None = None, route: ResolvedRoute | None = None, + procs: ModuleType | None = None, + report_cooldown: float = 20.0, + kill_grace: float = 5.0, ) -> None: self._hosts = HostsBlocker(helper_path) # Injectable so tests can drive the ordering without binding ports or # asking a real systemd-resolved for anything. self._stub = stub if stub is not None else StubResolver() self._route = route if route is not None else ResolvedRoute(helper_path) + # Injectable too, so the app-blocking tests do not have to own a victim + # process — and because `/proc` is not something a test should need. + self._procs = procs if procs is not None else processes self._report = report self._notify = notify self._active = False self._applied: list[str] = [] - self._announced: set[int] = set() + #: The app list and kill/warn flag from the last policy, so the scan + #: timer has something to work from between polls. + self._apps: list[str] = [] + self._kill_apps = True + #: What we have already told you about, keyed by (pid, start time) and + #: stamped with when. It throttles the *report*, never the block — see + #: [_enforce_apps]. + self._reported: dict[tuple[int, int], float] = {} + #: When each process was asked politely to quit, so the next pass can + #: insist. Keyed the same way. + self._asked: dict[tuple[int, int], float] = {} + self.report_cooldown = report_cooldown + self.kill_grace = kill_grace self._redirect_ip = "127.0.0.1" self.enforce_hosts = True self.enforce_apps = True @@ -115,9 +135,10 @@ class BlockerManager: log.info("blocking %s domain(s)", len(domains)) else: self.last_error = self._hosts.last_error - if self.enforce_apps and apps: - self._enforce_apps(apps, kill=bool(policy.get("kill_apps", True))) + self._apps = apps + self._kill_apps = bool(policy.get("kill_apps", True)) self._active = True + self.enforce_apps_now() else: self.release() @@ -165,7 +186,8 @@ class BlockerManager: else: self.last_error = self._hosts.last_error self._active = False - self._announced.clear() + self._reported.clear() + self._asked.clear() # The stub keeps running and keeps its route: the zones are still pointed # at it, so it has to answer — it just forwards everything again. self._stub.set_rules([], [], self._redirect_ip, active=False) @@ -186,18 +208,85 @@ class BlockerManager: # ---- apps -------------------------------------------------------------- - def _enforce_apps(self, patterns: list[str], *, kill: bool) -> None: - for app in processes.scan(patterns): - if app.pid in self._announced: - continue - self._announced.add(app.pid) + def enforce_apps_now(self) -> None: + """One app-blocking pass, from the timer that owns the cadence. - if kill and processes.terminate(app): - self.killed_today += 1 - self._report("app", app.matched, "killed", f"{app.name} (pid {app.pid})") - if self._notify: - self._notify("Closed a distraction", f"{app.name} is blocked during focus.") - else: - self._report("app", app.matched, "warned", f"{app.name} (pid {app.pid})") - if self._notify: - self._notify("That app is blocked", f"{app.name} is on your block list.") + Separate from [sync] because the two halves want different clocks: the + hosts file only changes when the policy does, while a process can be + launched at any moment and the server says how often to look + (`blocker.app_poll_seconds`, arriving as `poll_seconds`). + """ + if not (self._active and self.enforce_apps and self._apps): + return + self._enforce_apps(self._apps, kill=self._kill_apps) + + def _enforce_apps(self, patterns: list[str], *, kill: bool) -> None: + """Close (or report) every blocked process running right now. + + The important word is *every*, on *every* pass. An earlier version kept a + set of pids it had dealt with and skipped them forever, which conflated + two different jobs: not spamming the daily report with one row per poll, + and actually blocking the app. The second launch of a blocked app — or + the first one whose SIGTERM was refused — then went unchallenged for the + rest of the interval, which is the whole bug this docstring exists for. + + So the enforcement runs again each pass and only the *report* is + throttled, per process per [report_cooldown]. "Per process" is + (pid, start time), not pid: a pid alone is reused, and a relaunch + landing on a recycled number must not inherit the old entry. + + The kill escalates across passes rather than inside one: SIGTERM now, + SIGKILL once [kill_grace] has gone by and the process is still there. + That is gentler than the old two-second busy-wait and it never blocks + the thread drawing the countdown. + """ + self._forget_dead() + now = time.monotonic() + for app in self._procs.scan(patterns): + identity = app.identity + if not kill: + self._announce(app, "warned") + continue + + asked_at = self._asked.get(identity) + if asked_at is None: + delivered = self._procs.terminate(app) + self._asked[identity] = now + if delivered: + self.killed_today += 1 + self._announce(app, "killed" if delivered else "warned") + elif now - asked_at >= self.kill_grace: + # It sat through the polite request. Insist, and keep insisting + # one grace period at a time if something refuses the signal. + self._procs.terminate(app, force=True) + self._asked[identity] = now + self._announce(app, "killed") + + def _announce(self, app: "processes.RunningApp", action: str) -> None: + """Report and notify, at most once per process per cooldown.""" + if not self._claim_report(app.identity): + return + details = f"{app.name} (pid {app.pid})" + self._report("app", app.matched, action, details) + if not self._notify: + return + if action == "killed": + self._notify("Closed a distraction", f"{app.name} is blocked during focus.") + else: + self._notify("That app is blocked", f"{app.name} is on your block list.") + + def _claim_report(self, identity: tuple[int, int]) -> bool: + """Whether this process is due a report, and stamp it if so.""" + now = time.monotonic() + last = self._reported.get(identity) + if last is not None and now - last < self.report_cooldown: + return False + self._reported[identity] = now + return True + + def _forget_dead(self) -> None: + """Drop processes that are gone, so an all-day interval cannot leak.""" + for book in (self._reported, self._asked): + for pid, started in list(book): + if not self._procs.alive(pid, started): + del book[(pid, started)] diff --git a/desktop/kadence_desktop/blocking/processes.py b/desktop/kadence_desktop/blocking/processes.py index 993bf47..14e2d42 100644 --- a/desktop/kadence_desktop/blocking/processes.py +++ b/desktop/kadence_desktop/blocking/processes.py @@ -10,6 +10,12 @@ Matching is deliberately forgiving — `steam` catches `steamwebhelper`, and a glob (`steam*`, `*discord*`, `telegram-?`), matched against both the process name and its command line. It never matches ourselves, PID 1, or anything outside the current user's processes. + +Every match carries its **start time** as well as its pid. A pid on its own is +not an identity: Linux reuses them, and the blocker remembers which processes it +has already told you about. Keyed by pid alone, a relaunch that happens to land +on a recycled number would inherit the old entry and be let through — which is +exactly the "it started the second time" this file is here to prevent. """ from __future__ import annotations @@ -17,7 +23,6 @@ from __future__ import annotations import logging import os import signal -import time from fnmatch import fnmatch from dataclasses import dataclass from pathlib import Path @@ -48,6 +53,13 @@ class RunningApp: name: str cmdline: str matched: str + #: Field 22 of `/proc//stat` — clock ticks since boot. With the pid it + #: makes an identity that survives pid reuse. 0 when it could not be read. + start_time: int = 0 + + @property + def identity(self) -> tuple[int, int]: + return (self.pid, self.start_time) def _read(path: Path) -> str: @@ -57,6 +69,34 @@ def _read(path: Path) -> str: return "" +def start_time(pid: int) -> int: + """Clock ticks since boot at which this pid started, 0 if unreadable. + + `comm` sits in `stat` wrapped in parentheses and may itself contain spaces + and parentheses, so the fields are counted from the *last* `)` — the one + place this file cannot naively split on whitespace. + """ + raw = _read(Path(f"/proc/{pid}/stat")) + cut = raw.rfind(")") + if cut < 0: + return 0 + fields = raw[cut + 2 :].split() + # stat's field 3 is `state`, so field 22 is index 19 of what follows comm. + if len(fields) < 20: + return 0 + try: + return int(fields[19]) + except ValueError: + return 0 + + +def alive(pid: int, started: int = 0) -> bool: + """Whether this exact process is still running — not merely this pid.""" + if not Path(f"/proc/{pid}").exists(): + return False + return started == 0 or start_time(pid) == started + + def scan(patterns: list[str]) -> list[RunningApp]: """Every live process of this user matching one of `patterns`.""" wanted = [pattern.strip().lower() for pattern in patterns if pattern.strip()] @@ -89,32 +129,46 @@ def scan(patterns: list[str]) -> list[RunningApp]: match = next((pattern for pattern in wanted if matches(pattern, name, command)), None) if match: - found.append(RunningApp(pid=pid, name=comm, cmdline=cmdline[:200], matched=match)) + found.append( + RunningApp( + pid=pid, + name=comm, + cmdline=cmdline[:200], + matched=match, + start_time=start_time(pid), + ) + ) return found -def terminate(app: RunningApp, *, grace_seconds: float = 2.0) -> bool: - """Ask the process to quit, then insist. True when it is gone.""" +def terminate(app: RunningApp, *, force: bool = False) -> bool: + """Signal the process once. True when the signal landed (or it is gone). + + Deliberately **not** blocking. The previous version sent SIGTERM and then + busy-waited two seconds for the process to die before escalating, on + whichever thread happened to call it — which on the desktop is the one + drawing the countdown. Since the blocker now rescans every few seconds + anyway, the escalation is simply the *next* pass's job (see + `BlockerManager._enforce_apps`), and nothing has to wait for anybody. + + A `PermissionError` on SIGTERM escalates immediately rather than next pass: + a process that refused the polite signal on permission grounds is not about + to quit on its own. + """ try: - os.kill(app.pid, signal.SIGTERM) + os.kill(app.pid, signal.SIGKILL if force else signal.SIGTERM) except ProcessLookupError: return True except PermissionError: - log.warning("not allowed to stop %s (pid %s)", app.name, app.pid) - return False - - deadline = time.monotonic() + grace_seconds - while time.monotonic() < deadline: + if force: + log.warning("not allowed to stop %s (pid %s)", app.name, app.pid) + return False + log.warning("not allowed to ask %s (pid %s) to quit", app.name, app.pid) try: - os.kill(app.pid, 0) + os.kill(app.pid, signal.SIGKILL) except ProcessLookupError: return True - time.sleep(0.1) - - try: - os.kill(app.pid, signal.SIGKILL) - except ProcessLookupError: - return True - except PermissionError: - return False + except PermissionError: + log.warning("not allowed to stop %s (pid %s)", app.name, app.pid) + return False return True diff --git a/desktop/kadence_desktop/controller.py b/desktop/kadence_desktop/controller.py index 13bca46..9d8adb3 100644 --- a/desktop/kadence_desktop/controller.py +++ b/desktop/kadence_desktop/controller.py @@ -138,6 +138,15 @@ class Controller(QObject): self._idle_check.timeout.connect(self._check_idle) self._idle_check.start() + # A blocked app can be launched at any second, so the scan has its own + # clock rather than riding the policy poll — and the server is the one + # that says how fast (`blocker.app_poll_seconds`). It is cheap: a + # `/proc` walk and, at most, one signal per blocked process. + self._app_scan = QTimer(self) + self._app_scan.setInterval(self._app_scan_interval()) + self._app_scan.timeout.connect(self._blocker.enforce_apps_now) + self._app_scan.start() + # ====================================================================== # Properties for QML # ====================================================================== @@ -647,8 +656,24 @@ class Controller(QObject): self._policy = policy if self._config.enforce_blocking: self._blocker.sync(policy) + wanted = self._app_scan_interval() + if self._app_scan.interval() != wanted: + self._app_scan.setInterval(wanted) self.blockerChanged.emit() + def _app_scan_interval(self) -> int: + """Milliseconds between `/proc` scans, as the server asked. + + Clamped because this comes from a settings document a human edits: a + zero would spin, and a minute would let a game run for a minute. + """ + seconds = self._policy.get("poll_seconds") or self._config.poll_seconds + try: + seconds = int(seconds) + except (TypeError, ValueError): + seconds = self._config.poll_seconds + return max(1, min(30, seconds)) * 1000 + def _on_tasks(self, payload: dict[str, Any]) -> None: self._tasks = list(payload.get("tasks", [])) self._task_counts = dict(payload.get("counts", {})) @@ -818,6 +843,7 @@ class Controller(QObject): """Hand everything back: inhibits, DND, the hosts file and the DNS route.""" self._tick.stop() self._idle_check.stop() + self._app_scan.stop() self._screensaver.release() self._dnd.release() self._launcher.clear() diff --git a/desktop/tests/test_app_blocking.py b/desktop/tests/test_app_blocking.py new file mode 100644 index 0000000..c265905 --- /dev/null +++ b/desktop/tests/test_app_blocking.py @@ -0,0 +1,231 @@ +"""Blocking applications: the part that has to work the *second* time. + +The bug these tests exist for: the manager used to remember every pid it had +dealt with and skip it forever, which made one cooldown do two jobs. The daily +report stayed readable — and a blocked app relaunched a moment later, or one +whose first signal was refused, ran untouched for the rest of the interval. + +So the rules pinned here are: + +* enforcement happens on **every** pass, reporting at most once per cooldown; +* a process is identified by (pid, start time), because pids are recycled; +* the kill escalates across passes (SIGTERM, then SIGKILL) and never blocks. + +`FakeProcs` stands in for `/proc`: these tests must not need a victim process, +and the real module is covered by the fact that it reads files this one fakes. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field + +from kadence_desktop.blocking.manager import BlockerManager + + +@dataclass +class FakeApp: + """What `processes.scan` returns, with its own notion of staying alive.""" + + pid: int + name: str + matched: str + start_time: int = 1000 + cmdline: str = "" + #: Which signal it is prepared to ignore: "term", "kill", or neither. + survives: str = "" + + @property + def identity(self) -> tuple[int, int]: + return (self.pid, self.start_time) + + +@dataclass +class FakeProcs: + """A stand-in for `kadence_desktop.blocking.processes`.""" + + running: list[FakeApp] = field(default_factory=list) + signals: list[tuple[int, str]] = field(default_factory=list) + + def scan(self, patterns: list[str]) -> list[FakeApp]: + return [app for app in self.running if app.matched in patterns] + + def terminate(self, app: FakeApp, *, force: bool = False) -> bool: + self.signals.append((app.pid, "kill" if force else "term")) + if force and app.survives == "kill": + return False + if not force and app.survives in ("term", "kill"): + return True # delivered, ignored — exactly what escalation is for + self.running = [item for item in self.running if item.pid != app.pid] + return True + + def alive(self, pid: int, started: int = 0) -> bool: + return any( + app.pid == pid and (started == 0 or app.start_time == started) for app in self.running + ) + + +def policy(*, apps=("steam*",), kill: bool = True, poll_seconds: int = 3) -> dict: + return { + "active": True, + "enabled": True, + "domains": [], + "apps": list(apps), + "kill_apps": kill, + "redirect_ip": "127.0.0.1", + "unexpandable": [], + "allowed": [], + "poll_seconds": poll_seconds, + } + + +def manager(procs: FakeProcs, reports: list, **kwargs) -> BlockerManager: + class NoDns: + running = False + port = 0 + error = None + applied = None + + def start(self) -> bool: + return True + + def stop(self) -> None: + pass + + def set_rules(self, *args, **kwargs) -> None: + pass + + def on(self, *args) -> bool: + return True + + def off(self) -> bool: + return True + + return BlockerManager( + helper_path="/nonexistent/kadence-helper", + report=lambda kind, target, action, details: reports.append((kind, target, action)), + stub=NoDns(), + route=NoDns(), + procs=procs, + **kwargs, + ) + + +def test_the_second_launch_is_closed_too(): + """The bug, stated as a test: relaunching inside the cooldown must not work.""" + procs = FakeProcs([FakeApp(pid=101, name="steam", matched="steam*")]) + reports: list = [] + blocker = manager(procs, reports) + + blocker.sync(policy()) + assert procs.signals == [(101, "term")] + assert procs.running == [] + + # A second launch, a moment later: a new process, a recycled-or-not pid. + procs.running = [FakeApp(pid=101, name="steam", matched="steam*", start_time=2000)] + blocker.enforce_apps_now() + + assert procs.signals == [(101, "term"), (101, "term")] + assert procs.running == [] + + +def test_a_pid_is_not_an_identity(): + """Same pid, same start time is one process; a new start time is another.""" + procs = FakeProcs([FakeApp(pid=7, name="steam", matched="steam*", start_time=42)]) + reports: list = [] + blocker = manager(procs, reports) + + blocker.sync(policy()) + assert reports == [("app", "steam*", "killed")] + + # Reusing the pid *and* the start time cannot happen on a real machine; the + # point is that the bookkeeping keys on both, so this one is throttled... + procs.running = [FakeApp(pid=7, name="steam", matched="steam*", start_time=42)] + blocker.enforce_apps_now() + assert reports == [("app", "steam*", "killed")] + + # ...while a genuinely new process is reported again. + procs.running = [FakeApp(pid=7, name="steam", matched="steam*", start_time=99)] + blocker.enforce_apps_now() + assert reports == [("app", "steam*", "killed"), ("app", "steam*", "killed")] + + +def test_the_report_is_throttled_but_the_block_is_not(): + """One row in the evening report; one signal per pass regardless.""" + stubborn = FakeApp(pid=55, name="steam", matched="steam*", survives="kill") + procs = FakeProcs([stubborn]) + reports: list = [] + blocker = manager(procs, reports, kill_grace=0.0) + + for _ in range(4): + blocker.sync(policy()) + + assert len(reports) == 1, reports + # SIGTERM once, then SIGKILL on every pass after the grace — it is still + # there, so the manager keeps insisting. + assert procs.signals == [(55, "term"), (55, "kill"), (55, "kill"), (55, "kill")] + + +def test_sigkill_only_after_the_grace_period(): + """A well-behaved app gets time to quit before anything is forced.""" + procs = FakeProcs([FakeApp(pid=8, name="steam", matched="steam*", survives="term")]) + reports: list = [] + blocker = manager(procs, reports, kill_grace=600.0) + + blocker.sync(policy()) + blocker.enforce_apps_now() + blocker.enforce_apps_now() + + assert procs.signals == [(8, "term")], "nothing may be forced inside the grace" + + +def test_a_refused_signal_is_retried_rather_than_exempting_the_process(): + """`terminate` saying no must not be a free pass for the interval.""" + procs = FakeProcs([FakeApp(pid=9, name="steam", matched="steam*", survives="kill")]) + reports: list = [] + blocker = manager(procs, reports, kill_grace=0.0, report_cooldown=0.0) + + blocker.sync(policy()) + assert reports == [("app", "steam*", "killed")] + blocker.enforce_apps_now() + + assert procs.signals == [(9, "term"), (9, "kill")] + assert len(reports) == 2 + + +def test_warn_mode_never_kills(): + procs = FakeProcs([FakeApp(pid=11, name="discord", matched="*discord*")]) + reports: list = [] + blocker = manager(procs, reports) + + blocker.sync(policy(apps=("*discord*",), kill=False)) + + assert procs.signals == [] + assert procs.running # still there, by design + assert reports == [("app", "*discord*", "warned")] + assert blocker.killed_today == 0 + + +def test_nothing_is_enforced_once_the_interval_ends(): + procs = FakeProcs([FakeApp(pid=12, name="steam", matched="steam*")]) + reports: list = [] + blocker = manager(procs, reports) + + blocker.sync(policy() | {"active": False}) + blocker.enforce_apps_now() + + assert procs.signals == [] + assert reports == [] + + +def test_the_books_do_not_grow_for_processes_that_are_gone(): + procs = FakeProcs([FakeApp(pid=i, name="steam", matched="steam*") for i in range(20, 25)]) + reports: list = [] + blocker = manager(procs, reports) + + blocker.sync(policy()) + assert len(reports) == 5 + # Everything was closed, so the next pass has nothing left to remember. + blocker.enforce_apps_now() + + assert blocker._reported == {} + assert blocker._asked == {}