fix(recovery): fail closed on inspection errors and report leftovers accurately

Signed-off-by: lucannez64 <[email protected]>

.github/workflows/source-check.yml

@@ -38,6 +38,7 @@ jobs:
       - run: python -m unittest discover -s tests -p 'test_close_league_client_in_game.py'
       - run: python -m unittest discover -s tests -p 'test_recover_stale_client.py'
       - run: python -m unittest discover -s tests -p 'test_launchd_orphan_reaping.py'
+      - run: python -m unittest discover -s tests -p 'test_prepare_inspection.py'
       - run: python -m unittest discover -s tests -p 'test_clean_stale_helpers.py'
       - run: python -m unittest discover -s tests -p 'test_hide_league_client_in_game.py'
       - run: python -m unittest discover -s tests -p 'test_client_in_game_tricks.py'

docs/TROUBLESHOOTING.md

@@ -158,10 +158,12 @@ the normal launcher start the product again. It recognizes `LeagueClientUx`,
 is running or when a managed client window is visible, and fails closed when
 X11/Xwayland properties are missing or unreadable. This saves the Darling
 bootstrap but not the Riot Client/CEF startup itself. The stale tree is signalled
-deepest-first so live parents can still reap their own children; anything that is
-left over is reparented to the guest launchd, which reaps untracked zombies
-within a few seconds, so a full runtime restart is not needed to clear them. If no
-window appears afterward, use a full runtime shutdown/restart.
+deepest-first so live parents can still reap their own children. Guest launchd
+periodically reaps untracked orphaned zombie children. Recovery reports the PID,
+state and parent of each process still present after SIGKILL; a surviving live
+process or a zombie owned by another parent may require further investigation.
+If processes remain or no window appears afterward, use a full runtime
+shutdown/restart.
 
 ### Multiple client-in-game helpers
 

scripts/prepare.py

@@ -17,11 +17,11 @@ LFS_URL='lfs.url=https://github.com/darlinghq/darling-swift.git/info/lfs'
 LFS_FILTERS=['-c','filter.lfs.required=true','-c','filter.lfs.smudge=git-lfs smudge -- %f',
              '-c','filter.lfs.process=git-lfs filter-process','-c','filter.lfs.clean=git-lfs clean -- %f']
 def submodules(checkout):
-    out=subprocess.run(['git','-C',str(checkout),'submodule','status','--recursive'],text=True,capture_output=True).stdout
+    out=subprocess.run(['git','-C',str(checkout),'submodule','status','--recursive'],text=True,capture_output=True,check=True).stdout
     return [checkout/line.split()[1] for line in out.splitlines() if len(line.split())>1]
 def pointers(repo):
     """Tracked files still checked out as LFS pointer text instead of their real content."""
-    out=subprocess.run(['git','-C',str(repo),'lfs','ls-files'],text=True,capture_output=True).stdout
+    out=subprocess.run(['git','-C',str(repo),'lfs','ls-files'],text=True,capture_output=True,check=True).stdout
     return [f for f in (line.split(maxsplit=2) for line in out.splitlines()) if len(f)==3 and f[1]=='-']
 def repair(checkout):
     """An interrupted fetch leaves a submodule with a wiped index or pointer text and 'submodule update'
@@ -29,7 +29,7 @@ def repair(checkout):
     for repo in [checkout,*submodules(checkout)]:
         name=repo.relative_to(checkout) or '.'
         if repo!=checkout and subprocess.run(['git','-C',str(repo),'status','--porcelain'],text=True,
-                                             capture_output=True).stdout.strip():
+                                             capture_output=True,check=True).stdout.strip():
             print(f'Repairing the interrupted checkout of {name}',flush=True)
             run(['git','-C',str(repo),'checkout','-f','HEAD'],stdout=subprocess.DEVNULL)
         if pointers(repo):

tests/test_prepare_inspection.py

@@ -0,0 +1,40 @@
+"""Failed Git inspections must stop source preparation."""
+import ast
+from pathlib import Path
+import shutil
+import subprocess
+import tempfile
+import unittest
+
+
+@unittest.skipUnless(shutil.which('git'), 'Git required')
+class PreparationInspectionTests(unittest.TestCase):
+    def setUp(self):
+        tree = ast.parse((Path(__file__).resolve().parents[1] / 'scripts/prepare.py').read_text())
+        definitions = [node for node in tree.body if isinstance(node, ast.FunctionDef)
+                       and node.name in ('submodules', 'pointers', 'repair')]
+        self.namespace = {'subprocess': subprocess}
+        exec(compile(ast.Module(body=definitions, type_ignores=[]), 'prepare.py', 'exec'),
+             self.namespace)
+        self.temporary = tempfile.TemporaryDirectory()
+        self.addCleanup(self.temporary.cleanup)
+        self.root = Path(self.temporary.name)
+
+    def test_failed_submodule_inspection_is_not_an_empty_list(self):
+        with self.assertRaises(subprocess.CalledProcessError):
+            self.namespace['submodules'](self.root)
+
+    def test_failed_lfs_inspection_is_not_an_empty_list(self):
+        with self.assertRaises(subprocess.CalledProcessError):
+            self.namespace['pointers'](self.root)
+
+    def test_failed_status_inspection_stops_repair(self):
+        child = self.root / 'child'
+        child.mkdir()
+        self.namespace.update(submodules=lambda _root: [child], pointers=lambda _repo: [])
+        with self.assertRaises(subprocess.CalledProcessError):
+            self.namespace['repair'](self.root)
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/test_recover_stale_client.py

@@ -149,7 +149,8 @@ class RecoverStaleClientTests(unittest.TestCase):
         result, killed, output = self.run_recover(on_kill=survivor)
         self.assertEqual(result, 0)
         self.assertIn('still present after SIGKILL', output)
-        self.assertIn('parent PID(s): 0', output)
+        self.assertIn('PID 12 state=S parent=0', output)
+        self.assertNotIn('launchd reaps', output)
 
     def test_reused_pid_is_not_reported_as_a_leftover(self):
         folder = self.process(12, 'mldr', [CLIENT], self.client_env())

tools/recover-stale-client.py

@@ -280,11 +280,14 @@ def recover(prefix, proc_root=Path('/proc'), window_check=visible_client_window,
     _signal_tree(prefix, identities, order, signal.SIGKILL, kill, proc_root)
     leftovers = wait_for_exit(prefix, identities, proc_root, sleep)
     if leftovers:
-        parents = sorted({parent_pid(proc_root / str(pid)) for pid in leftovers} - {None})
-        print('recover-stale-client: %d process(es) still present after SIGKILL '
-              '(parent PID(s): %s); the guest launchd reaps untracked zombies within '
-              'seconds' % (len(leftovers), ', '.join(str(p) for p in parents) or 'unknown'),
-              file=out, flush=True)
+        details = [
+            'PID %d state=%s parent=%s' % (
+                pid, process_state(proc_root / str(pid)) or 'unknown',
+                parent_pid(proc_root / str(pid)))
+            for pid in leftovers
+        ]
+        print('recover-stale-client: %d process(es) still present after SIGKILL: %s' %
+              (len(leftovers), '; '.join(details)), file=out, flush=True)
     return 0