Docs: record real Proton smoke cleanup verification

CHANGELOG.md

@@ -0,0 +1,11 @@
+# Changelog
+
+## Unreleased
+
+- Fixed headset smoke cleanup reporting success while a smoke-title process
+  could remain running after its files were removed. Cleanup now stops and
+  rechecks processes tied to exact reserved test-title paths, uses PID handles
+  for safe TERM/KILL escalation, and records stopped PIDs. Unrelated titles and
+  shared Wine services are left alone. Verified with local Linux process
+  regressions and a 13/13 real Frame smoke run that stopped five test-owned
+  Proton process PIDs before removing the Windows test title.

docs/testing.md

@@ -164,6 +164,43 @@ does). A cleanup that fails counts as a failed step. Results go to
 screen; it exits 0 when every step passed, 1 if one failed, 2 if the headset
 isn't reachable.
 
+Smoke process cleanup runs before and after title removal and during the
+initial/final leftover sweeps. It targets only the reserved `fc_smoke_arm64`,
+`fc_smoke_x86_64`, `fc_smoke_exe` IDs and their legacy `fc-smoke-` equivalents.
+On the target, `tests/smoke/stop_processes.py` requires the SSH user's uid and
+an exact title directory in `/proc` cwd/exe or an individual absolute command
+argument; similarly named folders and shell command substrings do not count.
+Deleted cwd/exe paths still count, so a process left after file removal can be
+found. It sends SIGTERM, waits up to three seconds, then uses SIGKILL with
+another three-second bound, rescanning for survivors. PID handles prevent PID
+reuse from signalling another process. Linux Python 3.9+ with pidfd support
+is required; missing support, signal errors and survivors fail the
+cleanup step. Stopped PIDs are recorded in the results JSON.
+If the initial leftover cleanup fails, the run skips installs and pairing and
+records the failure instead of overwriting an active test title.
+
+This deliberately does not stop shared Wine/Proton services by name or prefix,
+Steam, SteamVR, process groups or arbitrary descendants. A process that drops
+all title-path evidence cannot safely be attributed and is outside this
+check. Unreadable `/proc` fields provide no ownership evidence; other readable
+fields are still checked, and a wholly unattributable process is left alone.
+Real Frame verification, 2026-10-24, SteamOS build `20261015.6141207`: the
+fixed Python smoke entry point passed all 13 steps under the shared device lock.
+The `fc_smoke_exe` launch ran under Proton Experimental (ARM64), and its
+remove step stopped five test-owned wrapper/Proton PIDs before removing the
+title; the following process sweep found none. The final cleanup found no
+test titles or processes, the test desktop was closed, and the battery
+remained at 99%. The `scripts/frame-smoke.sh` zsh wrapper could not run
+on lxso2 because zsh is absent; `python3 tests/smoke/frame_smoke.py` ran
+the same entry point instead.
+The local regression suite runs real Linux processes in temporary fake title
+directories (including deleted folders and a TERM-resistant wrapper); it checks
+exit, escalation and survival of unrelated processes without SSH or a headset:
+
+```sh
+python3 -m unittest discover -s tests -p test_smoke_cleanup.py -v
+```
+
 `--pair` asks the devkit service to pair a new RSA key, which needs someone
 in the headset to open **Settings → Developer → Pair new host** and approve
 it; the key is checked and then taken out of `authorized_keys` again.

tests/smoke/frame_smoke.py

@@ -13,11 +13,12 @@ BUILD_ID in tests/smoke/results/<time>-<BUILD_ID>.json (git-ignored):
   (someone has to open Settings > Developer > Pair new host and approve it in
   the headset), checked, and taken out of authorized_keys again.
 
-Everything it installs is removed again, also when a step fails: the titles
-(named fc_smoke_*, and leftovers of an interrupted run first), their Steam
+Everything it installs is removed again, also when a step fails: the reserved
+fc_smoke_{arm64,x86_64,exe} titles (and legacy leftovers first), their Steam
 shortcuts, the paired key, and ~/devkit-utils if it wasn't there before (if it
 was, it stays, synced to this checkout as Frame Control always does). A
-cleanup that fails is a failed step.
+cleanup that fails is a failed step. Test-owned processes are stopped and
+rechecked around deletion; shared Proton services are never stopped by name.
 
 Usage: python3 tests/smoke/frame_smoke.py [--pair]     (or scripts/frame-smoke.sh)
 Env:   FRAME_ALIAS (default frame), FRAME_SMOKE_RESULTS (default tests/smoke/results)
@@ -45,11 +46,17 @@ import frame_android  # noqa: E402
 import frame_connect  # noqa: E402
 import frame_titles  # noqa: E402
 import tiny_programs  # noqa: E402
+import stop_processes  # noqa: E402
 
 RESULTS = Path(os.environ.get('FRAME_SMOKE_RESULTS') or HERE / 'results')
 PREFIX = 'fc_smoke_'
-# Earlier runs named titles fc-smoke*, which Steam refused to register but left on disk.
-OLD_PREFIX = 'fc-smoke'
+
+
+def stop_titles(ids):
+    """Run the same bounded, PID-safe helper used by the local process tests."""
+    return json.loads(frame_android.ssh(
+        'python3 - ' + ' '.join(shlex.quote(gid) for gid in sorted(ids)),
+        input=(HERE / 'stop_processes.py').read_text(), timeout=20))
 
 
 class Smoke:
@@ -172,14 +179,17 @@ def title_cycle(smoke, kind, folder):
         return detail
 
     def remove():
+        stopped = stop_titles({gid})
         frame_titles.remove(gid)
+        # A wrapper could have launched a child just as deletion completed.
+        after = stop_titles({gid})
         left = ssh(f'ls -d ~/devkit-game/{gid} ~/devkit-game/{gid}-*.json 2>/dev/null || true').strip()
         if left:
             raise AssertionError(f'left behind: {left}')
         if gid in {t['id'] for t in frame_titles.list_titles()}:
             raise AssertionError(f'{gid} is still listed')
         smoke.installed.discard(gid)
-        return {'removed': gid}
+        return {'removed': gid, 'process_cleanup': [stopped, after]}
 
     if smoke.step(f'{kind}: install', install)['ok']:
         smoke.step(f'{kind}: launch', launch)
@@ -187,15 +197,21 @@ def title_cycle(smoke, kind, folder):
 
 
 def cleanup(smoke):
-    """Remove this run's titles and any fc_smoke_* an interrupted run left: the folder,
-    its json files, and (through steamos-delete) Steam's shortcut. Raises if anything stays."""
+    """Stop reserved smoke processes and remove their titles and shortcuts.
+
+    Legacy fc-smoke-* IDs are included; arbitrary similarly named titles are
+    never removed. Raises if any owned process, file or shortcut stays.
+    """
     problems = []
     try:
-        ids = {t['id'] for t in frame_titles.list_titles() if t['id'].startswith((PREFIX, OLD_PREFIX))}
+        ids = {t['id'] for t in frame_titles.list_titles() if t['id'] in stop_processes.IDS}
     except frame_android.FrameError as e:
         ids = set()
         problems.append(f'could not list titles: {e}')
     ids |= smoke.installed
+    # Scan every reserved ID, even when a prior run removed its folder and
+    # shortcut but left Proton running. Stop before removing any more files.
+    stopped = stop_titles(stop_processes.IDS)
     gone = set()
     for gid in sorted(ids):
         try:
@@ -226,9 +242,10 @@ def cleanup(smoke):
                     smoke.installed.discard(gid)
         except (frame_android.FrameError, AssertionError) as e:
             problems.append(f"syncing Steam's shortcuts: {e}")
+    after = stop_titles(stop_processes.IDS)
     if problems:
         raise AssertionError('; '.join(problems))
-    return {'removed': sorted(ids)}
+    return {'removed': sorted(ids), 'process_cleanup': [stopped, after]}
 
 
 # Runs on the Frame: drop the lines holding one key from authorized_keys, writing a
@@ -307,15 +324,17 @@ def main():
     # Frame Control copies Valve's devkit tools to ~/devkit-utils on the first install
     # (as Valve's client does). If they weren't there before, they go again at the end.
     had_utils = ssh('test -d ~/devkit-utils && echo yes || true').strip() == 'yes'
-    smoke.step('cleanup: leftovers from earlier runs', cleanup, smoke)
+    ready = smoke.step('cleanup: leftovers from earlier runs', cleanup, smoke)['ok']
     folder = tempfile.mkdtemp(prefix='frame-smoke-')
     try:
-        smoke.step('devkit service: properties.json', properties)
-        smoke.step('status', status, build)
-        for kind in ('arm64', 'x86_64', 'exe'):
-            title_cycle(smoke, kind, folder)
-        if args.pair:
-            smoke.step('devkit pairing (throwaway key)', pair, folder)
+        # Do not overwrite a leftover title when safe process cleanup failed.
+        if ready:
+            smoke.step('devkit service: properties.json', properties)
+            smoke.step('status', status, build)
+            for kind in ('arm64', 'x86_64', 'exe'):
+                title_cycle(smoke, kind, folder)
+            if args.pair:
+                smoke.step('devkit pairing (throwaway key)', pair, folder)
     finally:
         smoke.step('cleanup: everything this run installed', cleanup, smoke)
         if not had_utils:

tests/smoke/stop_processes.py

@@ -0,0 +1,91 @@
+"""Linux-only smoke cleanup helper, sent over SSH before deleting test files.
+
+Only exact, reserved smoke title directories establish ownership. Never signal
+by process name, process group, Steam app id, or a shared Wine/Proton prefix.
+"""
+import json
+import os
+from pathlib import Path
+import signal
+import sys
+import time
+
+IDS = frozenset(prefix + kind for prefix in ('fc_smoke_', 'fc-smoke-')
+                for kind in ('arm64', 'x86_64', 'exe'))
+
+
+def owned(pid, roots):
+    """Require our uid and an exact cwd/exe or individual absolute argv path.
+
+    /proc retains a deleted cwd/exe with ' (deleted)' appended. This also lets
+    the final sweep catch an older run whose title was already removed.
+    """
+    proc = Path('/proc') / str(pid)
+    try:
+        if pid == os.getpid() or proc.stat().st_uid != os.getuid():
+            return False
+        state = (proc / 'stat').read_text().rsplit(')', 1)[1].split()[0]
+        if state == 'Z':
+            return False
+        paths = []
+        for key in ('cwd', 'exe'):
+            try:
+                paths.append(os.readlink(proc / key).removesuffix(' (deleted)'))
+            except (FileNotFoundError, PermissionError):
+                pass
+        try:
+            paths += os.fsdecode((proc / 'cmdline').read_bytes()).split('\0')
+        except PermissionError:
+            pass
+        return any(path == root or path.startswith(root + '/')
+                   for path in paths for root in roots)
+    except (FileNotFoundError, ProcessLookupError, PermissionError):
+        return False
+
+
+def candidates(roots):
+    return sorted(int(p.name) for p in Path('/proc').iterdir()
+                  if p.name.isdigit() and owned(int(p.name), roots))
+
+
+def stop(ids, grace=3):
+    if not ids or not set(ids) <= IDS:
+        raise ValueError('only reserved smoke title IDs may be stopped')
+    if not hasattr(os, 'pidfd_open') or not hasattr(signal, 'pidfd_send_signal'):
+        raise RuntimeError('safe smoke cleanup requires Linux Python with pidfd support')
+    roots = [str(Path.home() / 'devkit-game' / gid) for gid in ids]
+    stopped = set()
+    for sig in (signal.SIGTERM, signal.SIGKILL):
+        # Re-scan while waiting: wrappers can spawn a child during shutdown.
+        deadline = time.monotonic() + grace
+        while True:
+            pids = candidates(roots)
+            if not pids:
+                return {'stopped_pids': sorted(stopped)}
+            for pid in pids:
+                try:
+                    fd = os.pidfd_open(pid)
+                except ProcessLookupError:
+                    continue
+                try:
+                    # Open first, then revalidate ownership. The handle cannot
+                    # signal an unrelated process if this numeric PID is reused.
+                    if owned(pid, roots):
+                        try:
+                            signal.pidfd_send_signal(fd, sig)
+                            stopped.add(pid)
+                        except ProcessLookupError:
+                            pass
+                finally:
+                    os.close(fd)
+            if time.monotonic() >= deadline:
+                break
+            time.sleep(0.1)
+    left = candidates(roots)
+    if left:
+        raise RuntimeError(f'smoke processes survived cleanup: {left}')
+    return {'stopped_pids': sorted(stopped)}
+
+
+if __name__ == '__main__':
+    print(json.dumps(stop(sys.argv[1:])))

tests/test_smoke_cleanup.py

@@ -0,0 +1,153 @@
+"""Smoke cleanup against real local Linux processes, never a headset or SSH."""
+import sandbox  # noqa: F401
+import os
+from pathlib import Path
+import shutil
+import shlex
+import signal
+import subprocess
+import sys
+import tempfile
+import unittest
+from unittest import mock
+
+SMOKE = Path(__file__).resolve().parent / 'smoke'
+sys.path.insert(0, str(SMOKE))
+import stop_processes  # noqa: E402
+import frame_smoke  # noqa: E402
+
+
+@unittest.skipUnless(sys.platform == 'linux' and hasattr(os, 'pidfd_open'), 'Linux pidfds required')
+class RealProcesses(unittest.TestCase):
+    def setUp(self):
+        self.tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(self.tmp.cleanup)
+        self.home = Path(self.tmp.name)
+        self.title = self.home / 'devkit-game' / 'fc_smoke_exe'
+        self.title.mkdir(parents=True)
+        self.children = []
+        self.patcher = mock.patch.object(stop_processes.Path, 'home', return_value=self.home)
+        self.patcher.start()
+        self.addCleanup(self.patcher.stop)
+        self.addCleanup(self.reap)
+
+    def reap(self):
+        for child in self.children:
+            if child.poll() is None:
+                child.kill()
+            child.wait(timeout=5)
+
+    def start(self, cwd, *args, ignore=False):
+        code = ('import signal,time; '
+                + ('signal.signal(signal.SIGTERM, signal.SIG_IGN); ' if ignore else '')
+                + 'print("ready", flush=True); time.sleep(60)')
+        child = subprocess.Popen([sys.executable, '-c', code, *args], cwd=cwd,
+                                 stdout=subprocess.PIPE, text=True)
+        self.children.append(child)
+        self.addCleanup(child.stdout.close)
+        self.assertEqual(child.stdout.readline().strip(), 'ready')
+        return child
+
+    def test_deleted_title_process_is_stopped_but_similar_names_survive(self):
+        owned = self.start(self.title)
+        neighbour = self.title.with_name('fc_smoke_exe_other')
+        neighbour.mkdir()
+        other = self.start(neighbour, 'fc_smoke_exe', 'wine', 'proton')
+        shutil.rmtree(self.title)
+        # Reproduces the old success condition: no title remains, yet it runs.
+        self.assertFalse(self.title.exists())
+        self.assertIsNone(owned.poll())
+        def local_target(command, *, input, timeout):
+            return subprocess.run(shlex.split(command), input=input, text=True,
+                                  capture_output=True, check=True, timeout=timeout,
+                                  env={**os.environ, 'HOME': str(self.home)}).stdout
+
+        # Exercise the production SSH command and stdin payload against a local
+        # fake target, including the helper's CLI and JSON result contract.
+        with mock.patch.object(frame_smoke.frame_android, 'ssh', side_effect=local_target):
+            result = frame_smoke.stop_titles({'fc_smoke_exe'})
+        self.assertIn(owned.pid, result['stopped_pids'])
+        self.assertEqual(owned.wait(timeout=5), -signal.SIGTERM)
+        self.assertIsNone(other.poll())
+
+    def test_proton_style_absolute_argument_escalates_without_stopping_shared_wine(self):
+        owned = self.start(self.home, str(self.title / 'fc-smoke-exe.exe'), ignore=True)
+        shared = self.start(self.home, 'wineserver', '-p')
+        quoted = self.start(self.home, 'echo ' + str(self.title / 'fc-smoke-exe.exe'))
+        result = stop_processes.stop(['fc_smoke_exe'], grace=0.2)
+        self.assertIn(owned.pid, result['stopped_pids'])
+        self.assertEqual(owned.wait(timeout=5), -signal.SIGKILL)
+        self.assertIsNone(shared.poll())
+        self.assertIsNone(quoted.poll())
+        self.assertEqual(stop_processes.stop(['fc_smoke_exe']), {'stopped_pids': []})
+
+    def test_unreadable_cwd_does_not_abort_scan_or_hide_readable_argv(self):
+        owned = self.start(self.home, str(self.title / 'fc-smoke-exe.exe'))
+        other = self.start(self.home, 'wineserver')
+        with mock.patch.object(stop_processes.os, 'readlink', side_effect=PermissionError):
+            self.assertTrue(stop_processes.owned(owned.pid, [str(self.title)]))
+            self.assertFalse(stop_processes.owned(other.pid, [str(self.title)]))
+            result = stop_processes.stop(['fc_smoke_exe'], grace=0.2)
+        self.assertIn(owned.pid, result['stopped_pids'])
+        self.assertEqual(owned.wait(timeout=5), -signal.SIGTERM)
+        self.assertIsNone(other.poll())
+
+
+class Safety(unittest.TestCase):
+    def test_non_test_ids_rejected_before_scanning(self):
+        for ids in ([], ['portal2'], ['fc_smoke_exe/../portal2'], ['fc_smoke_custom']):
+            with self.subTest(ids=ids), self.assertRaises(ValueError):
+                stop_processes.stop(ids)
+
+    @unittest.skipUnless(sys.platform == 'linux', 'Linux signalling helper')
+    def test_lost_ownership_after_open_never_signalled(self):
+        with mock.patch.object(stop_processes, 'candidates', side_effect=[[123], [], []]), \
+                mock.patch.object(stop_processes.os, 'pidfd_open', return_value=9, create=True), \
+                mock.patch.object(stop_processes.os, 'close'), \
+                mock.patch.object(stop_processes, 'owned', return_value=False), \
+                mock.patch.object(stop_processes.signal, 'pidfd_send_signal', create=True) as send:
+            self.assertEqual(stop_processes.stop(['fc_smoke_exe'], grace=0), {'stopped_pids': []})
+            send.assert_not_called()
+
+    @unittest.skipUnless(sys.platform == 'linux', 'Linux signalling helper')
+    def test_survivor_is_a_failure(self):
+        with mock.patch.object(stop_processes, 'candidates', return_value=[123]), \
+                mock.patch.object(stop_processes.os, 'pidfd_open', return_value=9, create=True), \
+                mock.patch.object(stop_processes.os, 'close'), \
+                mock.patch.object(stop_processes, 'owned', return_value=True), \
+                mock.patch.object(stop_processes.signal, 'pidfd_send_signal', create=True):
+            with self.assertRaisesRegex(RuntimeError, 'survived cleanup'):
+                stop_processes.stop(['fc_smoke_exe'], grace=0)
+
+    def test_cleanup_scans_even_when_title_already_removed(self):
+        with mock.patch.object(frame_smoke.frame_titles, 'list_titles', return_value=[]), \
+                mock.patch.object(frame_smoke, 'ssh') as ssh, \
+                mock.patch.object(frame_smoke, 'stop_titles', return_value={'stopped_pids': [42]}) as stop:
+            result = frame_smoke.cleanup(frame_smoke.Smoke())
+            self.assertEqual(stop.call_args_list, [mock.call(stop_processes.IDS)] * 2)
+            self.assertEqual(result['process_cleanup'][0]['stopped_pids'], [42])
+            ssh.assert_not_called()
+
+    def test_stop_failure_prevents_file_removal(self):
+        with mock.patch.object(frame_smoke.frame_titles, 'list_titles', return_value=[{'id': 'fc_smoke_exe'}]), \
+                mock.patch.object(frame_smoke, 'ssh') as ssh, \
+                mock.patch.object(frame_smoke, 'stop_titles', side_effect=RuntimeError('still running')):
+            with self.assertRaisesRegex(RuntimeError, 'still running'):
+                frame_smoke.cleanup(frame_smoke.Smoke())
+            ssh.assert_not_called()
+
+    def test_failed_initial_cleanup_prevents_install_and_records_failure(self):
+        with tempfile.TemporaryDirectory() as folder, \
+                mock.patch.object(frame_smoke, 'RESULTS', Path(folder)), \
+                mock.patch.object(frame_smoke.sys, 'argv', ['frame_smoke.py']), \
+                mock.patch.object(frame_smoke.subprocess, 'run', return_value=mock.Mock(returncode=0)), \
+                mock.patch.object(frame_smoke.tempfile, 'mkdtemp', return_value=folder), \
+                mock.patch.object(frame_smoke, 'os_release', return_value={
+                    'BUILD_ID': 'fake', 'VERSION_ID': 'fake', 'VARIANT_ID': 'fake'}), \
+                mock.patch.object(frame_smoke, 'ssh', return_value='yes'), \
+                mock.patch.object(frame_smoke, 'cleanup', side_effect=RuntimeError('cannot stop')), \
+                mock.patch.object(frame_smoke, 'title_cycle') as cycle, \
+                mock.patch('builtins.print'):
+            self.assertEqual(frame_smoke.main(), 1)
+            cycle.assert_not_called()
+            self.assertEqual(len(list(Path(folder).glob('*-fake.json'))), 1)