The sandbox sweep reads its prefixes out of the test sources

Author: dob323

.github/workflows/ci.yml

@@ -230,6 +230,7 @@ jobs:
             tests/test_projects_manifest.py \
             tests/test_public_export.py tests/test_public_scan.py \
             tests/test_release_artifact.py tests/support.py \
+            tests/test_sandbox_sweep.py \
             tests/shard.py \
             tests/test_readme_art.py tests/test_readme_keys.py \
             tools/readme_art_lib.py tools/readme_keys.py

SOURCE.json

@@ -1,5 +1,5 @@
 {
-  "exported_files": 314,
+  "exported_files": 315,
   "schema_version": 1,
-  "source_commit": "299bcac6bd83c71ec515275e8af398af9ef8a4eb"
+  "source_commit": "17c13b2f9b16d0df770a4d01a64620b25686ff69"
 }

public-files.txt

@@ -139,6 +139,7 @@ tests/test_release_artifact.py
 tests/test_release_gc.py
 tests/test_restore_identity.py
 tests/test_sandbox_guard.py
+tests/test_sandbox_sweep.py
 tests/test_session_colors.py
 tests/test_snapshot_contract.py
 tests/test_state_column.py

tests/support.py

@@ -41,10 +41,15 @@ def source_tree_ignore(root: Path = REPO):
     test removes one and copytree lists an entry, then fails to open it, and the
     run dies over a file that was never part of the source.
 
-    c9d85e9 fixed that in one of the three by listing sweep_sandboxes.PREFIXES.
-    That list holds six prefixes; the suite builds a hundred and eighty-three
-    inside the checkout, so the fix covered neither of the two sandboxes that
-    have actually broken a run. Prefixes were never the right shape here.
+    c9d85e9 fixed that in one of the three by borrowing the sweep's list of
+    sandbox prefixes. Six were named against the two hundred and more the suite
+    hands tempfile, so the fix covered neither of the two sandboxes that have
+    actually broken a run. Prefixes were never the right shape here.
+
+    tests/sweep_sandboxes.py no longer carries a list -- it reads the prefixes
+    out of the test sources -- and this rule still does not borrow from it. A
+    copy rule and a delete rule disagree about `.git`: the copy must drop it,
+    the sweep must protect it. Two answers, two constants, no shared edit.
 
     .gitignore settled this for Git already -- `/.*/` with `!/.github/`, under
     the note "Ignore the whole class rather than chase prefixes" -- so this

tests/sweep_sandboxes.py

@@ -15,44 +15,221 @@
 the grace period is touched, so a run in flight -- including a parallel one --
 is never disturbed.
 
+Which names count as a sandbox is READ FROM THE SUITE, never listed here. The
+hand-written list this file used to carry named six of the two hundred and
+more the suite hands tempfile, so most of what a crashed run leaves stayed
+where it was -- including the leaked processes this exists to kill, because it
+decides those by sandbox too. A list somebody has to remember to extend is a
+list that is wrong from the next commit onward. `sandbox_prefixes()` parses the
+test sources instead, and `tests/test_sandbox_sweep.py` fails when a fixture
+writes a prefix that parse cannot read.
+
 Run for its effect, from tests/run. It prints only what it actually removed.
+`--list` prints the same judgements and removes nothing.
 """
 
 from __future__ import annotations
 
+import ast
 import os
 from pathlib import Path
 import shutil
 import signal
+import string
 import sys
 import time
 
 REPO = Path(__file__).resolve().parents[1]
-# Prefixes the fixtures use for their sandboxes, all of them dotted so
-# .gitignore's `/.*/` already keeps them out of a commit.
-PREFIXES = (
-    ".login-",
-    ".commands-",
-    ".msg-cli-",
-    ".account-",
-    ".projects-",
-    # tests/sandbox_guard.py builds one per run and removes it at interpreter
-    # exit; a SIGKILLed run leaves it behind like any other sandbox.
-    ".sandbox-guard-",
-)
+TESTS = REPO / "tests"
+
+# The two calls that make a DIRECTORY under a name tempfile chooses. mkstemp
+# and NamedTemporaryFile are deliberately absent: they leave a file, and this
+# sweep only ever removes directories.
+SANDBOX_CALLS = frozenset({"TemporaryDirectory", "mkdtemp"})
+
+# The characters tempfile draws the random part of a name from
+# (tempfile._RandomNameSequence.characters). A leftover sandbox is a known
+# prefix followed by a run of these and nothing else, which is what separates
+# `.watchdog-g2hiuui4` from a directory somebody deliberately named
+# `.watchdog-notes`. Note the absence of `-`: it keeps a short prefix from
+# claiming a longer sibling, so `.reap-` cannot answer for `.reap-er-x1y2`.
+RANDOM_NAME_CHARACTERS = frozenset(string.ascii_lowercase + string.digits + "_")
+
+# Never removed, whatever the derived prefixes say, and checked before any
+# prefix is consulted.
+#
+# `.git` is first because it is the only entry here whose loss cannot be
+# undone: every commit not yet pushed lives inside it and nothing in the
+# working tree can rebuild it. The other three are the repository's own dotted
+# entries at the root -- source, not litter -- and the same argument applies to
+# each: a mistake about them is not litter left behind, it is source removed.
+# No prefix in the tree reaches any of the four today; this is what holds if
+# one ever does, and tests/test_sandbox_sweep.py proves it by feeding the
+# matcher a prefix that would swallow `.git`.
+#
+# This is NOT `tests.support.SOURCE_ROOT_DOTTED` and must never be replaced by
+# it. That set answers a different question -- what a COPY of this tree must
+# leave behind -- and it deliberately omits `.git`, because a copy carrying
+# history is the bug it was written to stop. Two rules with opposite
+# requirements for the same name; sharing one constant between them puts a
+# deleting rule one edit away from losing the repository.
+NEVER_REMOVE = frozenset({".git", ".github", ".gitignore", ".shellcheckrc"})
+
 # Long enough that no live run is ever in range, short enough that a leak costs
 # one run rather than a week. The override exists so this file can be tested
 # against a sandbox created a moment ago.
 GRACE_SECONDS = int(os.environ.get("SESSION_KIT_SWEEP_GRACE_SECONDS") or 2 * 3600)
 
 
-def stale_sandboxes() -> list[Path]:
+def called_name(node: ast.Call) -> str:
+    """`tempfile.mkdtemp(...)` and `mkdtemp(...)` both answer `mkdtemp`."""
+    function = node.func
+    if isinstance(function, ast.Attribute):
+        return function.attr
+    if isinstance(function, ast.Name):
+        return function.id
+    return ""
+
+
+def string_literal(expression: ast.expr | None) -> str | None:
+    if isinstance(expression, ast.Constant) and isinstance(expression.value, str):
+        return expression.value
+    return None
+
+
+def string_argument(node: ast.Call, keyword: str, position: int) -> str | None:
+    """The literal a call passes for `keyword`, by name or at `position`."""
+    for entry in node.keywords:
+        if entry.arg == keyword:
+            return string_literal(entry.value)
+        if entry.arg is None:
+            # `**kwargs` could carry the prefix; nothing here can read it.
+            return None
+    if position < len(node.args):
+        return string_literal(node.args[position])
+    return None
+
+
+def parse_test_sources() -> dict[Path, ast.Module]:
+    """Every test module that parses, by path.
+
+    A file that does not parse is skipped and said out loud rather than taken
+    as having no sandboxes. Skipping narrows the sweep, which leaves litter;
+    the alternative reading -- treat an unparseable file as empty and carry on
+    silently -- is how the previous list went stale unnoticed.
+    """
+    trees: dict[Path, ast.Module] = {}
+    if not TESTS.is_dir():
+        print(f"tests: no {TESTS}, sweeping nothing", file=sys.stderr)
+        return trees
+    for path in sorted(TESTS.rglob("*.py")):
+        try:
+            source = path.read_text(encoding="utf-8")
+            trees[path] = ast.parse(source, filename=str(path))
+        except (OSError, SyntaxError, UnicodeDecodeError) as error:
+            print(
+                f"tests: cannot read sandbox names from {path}: {error}",
+                file=sys.stderr,
+            )
+    return trees
+
+
+def sandbox_factories(trees: dict[Path, ast.Module]) -> dict[str, tuple[str, int]]:
+    """Helpers that forward a caller's string straight to tempfile.
+
+    `tests/test_worktree_isolation.sandbox(prefix)` is the shape: one function
+    the whole file goes through, so the literals sit at its call sites instead
+    of at the tempfile call. Maps the helper's name to the name and position of
+    the parameter it forwards, so either spelling of a call site can be read.
+    Names are collected across the whole tree because helpers are imported
+    between modules.
+    """
+    factories: dict[str, tuple[str, int]] = {}
+    for tree in trees.values():
+        for node in ast.walk(tree):
+            if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
+                continue
+            parameters = [argument.arg for argument in node.args.args]
+            for inner in ast.walk(node):
+                if not isinstance(inner, ast.Call):
+                    continue
+                if called_name(inner) not in SANDBOX_CALLS:
+                    continue
+                for entry in inner.keywords:
+                    if entry.arg != "prefix":
+                        continue
+                    forwarded = entry.value
+                    if isinstance(forwarded, ast.Name) and forwarded.id in parameters:
+                        factories[node.name] = (
+                            forwarded.id,
+                            parameters.index(forwarded.id),
+                        )
+    return factories
+
+
+def sandbox_prefixes() -> frozenset[str]:
+    """Every prefix the suite can hand tempfile, read from the test sources.
+
+    Call sites are taken whatever their `dir=` argument says. Where a fixture
+    MEANT to put its sandbox is not a property this file can evaluate -- the
+    argument is an expression, and several are read from the environment -- and
+    guessing at it is what a sweep must not do. What makes a removal safe is
+    the name: a prefix from this set followed by tempfile's random characters
+    and nothing else, at the repository root, outside `NEVER_REMOVE`. A prefix
+    belonging to a sandbox that normally lands in /tmp costs nothing here,
+    because no such directory appears at the root; a fixture that moves its
+    sandbox INTO the checkout is covered from the same commit that moves it.
+    """
+    trees = parse_test_sources()
+    factories = sandbox_factories(trees)
+    found: set[str] = set()
+    for tree in trees.values():
+        for node in ast.walk(tree):
+            if not isinstance(node, ast.Call):
+                continue
+            name = called_name(node)
+            if name in SANDBOX_CALLS:
+                # tempfile takes prefix second, after suffix.
+                prefix = string_argument(node, "prefix", 1)
+            elif name in factories:
+                keyword, position = factories[name]
+                prefix = string_argument(node, keyword, position)
+            else:
+                continue
+            if isinstance(prefix, str) and prefix:
+                found.add(prefix)
+    return frozenset(found)
+
+
+def is_a_sandbox_name(name: str, prefixes: frozenset[str]) -> bool:
+    """Could the suite have created a directory called this?
+
+    Protected names are refused before any prefix is looked at, so widening the
+    prefix set can never reach one.
+    """
+    if name in NEVER_REMOVE:
+        return False
+    for prefix in prefixes:
+        if not name.startswith(prefix):
+            continue
+        random_part = name[len(prefix) :]
+        if random_part and set(random_part) <= RANDOM_NAME_CHARACTERS:
+            return True
+    return False
+
+
+def stale_sandboxes(prefixes: frozenset[str]) -> list[Path]:
     cutoff = time.time() - GRACE_SECONDS
     found = []
     for entry in REPO.iterdir():
         if not entry.is_dir() or entry.is_symlink():
             continue
-        if not entry.name.startswith(PREFIXES):
+        if not is_a_sandbox_name(entry.name, prefixes):
+            continue
+        # Nothing outside the checkout, ever. iterdir yields only children of
+        # REPO and the symlinks are already gone, so this cannot point
+        # elsewhere; it is checked anyway because rmtree is what comes next.
+        if entry.resolve().parent != REPO:
             continue
         try:
             if entry.stat().st_mtime > cutoff:
@@ -63,7 +240,7 @@ def stale_sandboxes() -> list[Path]:
     return sorted(found)
 
 
-def in_a_sandbox(cwd: str) -> bool:
+def in_a_sandbox(cwd: str, prefixes: frozenset[str]) -> bool:
     """Is this working directory inside a fixture sandbox, live or deleted?
 
     The kernel appends " (deleted)" once the directory is gone, and gone is the
@@ -78,7 +255,7 @@ def in_a_sandbox(cwd: str) -> bool:
     except ValueError:
         return False
     head = relative.parts[0] if relative.parts else ""
-    return head.startswith(PREFIXES)
+    return is_a_sandbox_name(head, prefixes)
 
 
 def process_age_seconds(pid: str) -> float | None:
@@ -106,7 +283,9 @@ def process_age_seconds(pid: str) -> float | None:
     return max(0.0, uptime - started_ticks / ticks)
 
 
-def leaked_processes(stale: list[Path]) -> list[tuple[int, str]]:
+def leaked_processes(
+    stale: list[Path], prefixes: frozenset[str]
+) -> list[tuple[int, str]]:
     """Same-user pids that cannot belong to a live run, inside a sandbox.
 
     Two ways to be sure. A process sitting in a sandbox this sweep has already
@@ -127,7 +306,7 @@ def leaked_processes(stale: list[Path]) -> list[tuple[int, str]]:
             cwd = os.readlink(f"{base}/cwd")
         except OSError:
             continue
-        if not in_a_sandbox(cwd):
+        if not in_a_sandbox(cwd, prefixes):
             continue
         plain = cwd[: -len(" (deleted)")] if cwd.endswith(" (deleted)") else cwd
         inside_stale = any(
@@ -148,11 +327,19 @@ def leaked_processes(stale: list[Path]) -> list[tuple[int, str]]:
     return out
 
 
-def main() -> int:
-    roots = stale_sandboxes()
-    leaked = leaked_processes(roots)
+def main(argv: list[str] | None = None) -> int:
+    listing = "--list" in (argv if argv is not None else sys.argv[1:])
+    prefixes = sandbox_prefixes()
+    roots = stale_sandboxes(prefixes)
+    leaked = leaked_processes(roots, prefixes)
     if not roots and not leaked:
         return 0
+    if listing:
+        for pid, command in leaked:
+            print(f"tests: would end leaked process {pid} ({command[:60]})")
+        for root in roots:
+            print(f"tests: would remove stale sandbox {root.name}")
+        return 0
     for pid, command in leaked:
         # The process group: a provider CLI runs under `script`, and killing the
         # wrapper alone leaves the CLI holding its conversation.