perf(tests): bound Notes server descendant cleanup

scripts/verify_notes_browser_runtime.py

@@ -11,6 +11,38 @@ import time
 from urllib.request import urlopen
 
 ROOT = Path(__file__).resolve().parents[1]
+if str(ROOT) not in sys.path:
+    sys.path.insert(0, str(ROOT))
+
+
+def _stop_owned_server(server, identity):
+    """Close only identity-bound descendants, including MCP's new sessions.
+
+    Capture before signalling the parent: an orphaned stderr holder can no
+    longer be attributed by ancestry. Never search by executable name or kill
+    the caller's process group.
+    """
+    from src import process_lifecycle as lifecycle
+
+    descendants = (lifecycle.bind_descendants([identity.pid])
+                   if identity.verdict() == lifecycle.OWNED else [])
+    if os.name == "nt":
+        if identity.verdict() == lifecycle.OWNED:
+            lifecycle.taskkill_tree(identity.pid)
+    else:
+        outcome = lifecycle.terminate_identities(
+            [*(entry.identity for entry in reversed(descendants)), identity],
+            steps=lifecycle.term_kill_steps(10),
+        )
+        if outcome.survivors or outcome.unverified:
+            raise RuntimeError(f"Disposable Notes cleanup could not verify exit: {outcome}")
+    try:
+        logs, _ = server.communicate(timeout=2)
+    except subprocess.TimeoutExpired as error:
+        # Do not turn an unidentified pipe holder into an unbounded wait.
+        server.stdout.close()
+        raise RuntimeError("Disposable Notes output pipe remained open after owned cleanup") from error
+    return logs
 
 
 def main():
@@ -36,7 +68,10 @@ def main():
         server = subprocess.Popen(
             [sys.executable, "-m", "uvicorn", "app:app", "--host", "127.0.0.1", "--port", str(port)],
             cwd=target, env=env, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True,
+            start_new_session=(os.name != "nt"),
         )
+        from src.process_lifecycle import ProcessIdentity
+        server_identity = ProcessIdentity.capture(server.pid)
         result = None
         try:
             origin = f"http://127.0.0.1:{port}"
@@ -62,12 +97,7 @@ def main():
                 print(result.stderr, file=sys.stderr)
             return result.returncode
         finally:
-            server.terminate()
-            try:
-                logs, _ = server.communicate(timeout=10)
-            except subprocess.TimeoutExpired:
-                server.kill()
-                logs, _ = server.communicate()
+            logs = _stop_owned_server(server, server_identity)
             if result is None or result.returncode:
                 print(logs[-8000:], file=sys.stderr)
 

tests/TESTING_STANDARD.md

@@ -99,10 +99,8 @@ cleanup. Specifically:
   `os.environ[...] = ...` that outlives the test.
 - **Current working directory** - never `chdir` without restoring; never assert
   against cwd-relative paths like `./data`. Use a temp workspace helper instead.
-- **Database** - the root `conftest.py` defaults `DATABASE_URL` to a private,
-  worker-owned SQLite file for collection safety and consistent schema across
-  TestClient threads. This fallback is not a per-test data fixture. A test that
-  writes or depends on particular rows must opt
+- **Database** - the root `conftest.py` defaults `DATABASE_URL` to an in-memory
+  SQLite for collection safety. A test that needs a real file-backed DB must opt
   in explicitly via `tests.helpers.sqlite_db.make_temp_sqlite` and bind its
   `SessionLocal` onto the module under test. Do not rely on a persistent
   on-disk DB existing.

tests/conftest.py

@@ -12,17 +12,9 @@ sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 # File-backed databases remain fixture-owned. Cleanup restores the caller.
 # Keep the existing generated environment-reference locations stable.
 _database_environment = pytest.MonkeyPatch()
+_database_environment.setenv("DATABASE_URL", "sqlite:///:memory:")
 from tests.helpers.worker_runtime import bootstrap_runtime, configure_runtime
 _runtime_environment = bootstrap_runtime()
-# A file keeps the collection schema consistent across TestClient threads;
-# SingletonThreadPool can evict the populated in-memory main-thread database.
-# One patcher owns this variable, including restoration of an inherited URL.
-_database_environment.setenv(
-    "DATABASE_URL",
-    f"sqlite:///{_runtime_environment.root / 'collection.db'}"
-    if _runtime_environment is not None else "sqlite:///:memory:",
-)
-_collection_database_engine = None
 
 # Pre-import real heavy modules BEFORE any test file's module-level stubs can
 # replace them with MagicMock. Some test files (e.g. test_llm_core_sanitize_*)
@@ -34,7 +26,6 @@ try:
     import sqlalchemy.orm  # noqa: F401
     import core.database  # noqa: F401
     import src.database
-    _collection_database_engine = core.database.engine
 except ImportError:
     pass  # not installed - the stubs below will handle it
 
@@ -109,10 +100,6 @@ def pytest_configure(config):
     only registers marker names; it imports no production module.
     """
     config.add_cleanup(_database_environment.undo)
-    if _collection_database_engine is not None:
-        # Registered after runtime cleanup, so connections close before the
-        # owned file is removed (also required on Windows).
-        config.add_cleanup(_collection_database_engine.dispose)
 
     import pathlib
     from tests._taxonomy import discover_markers

tests/test_database_test_isolation.py

@@ -85,9 +85,6 @@ def test_actual_tests_restore_process_state_and_ignore_inherited_database(tmp_pa
         import os
         import sys
         import pytest
-        import tempfile
-        from pathlib import Path
-        from sqlalchemy.engine import make_url
 
         def snapshot():
             import core
@@ -110,11 +107,7 @@ def test_actual_tests_restore_process_state_and_ignore_inherited_database(tmp_pa
         class StateGuard:
             def pytest_sessionstart(self):
                 self.initial = snapshot()
-                assert self.initial[0] != inherited_url
-                self.default_path = Path(make_url(self.initial[0]).database)
-                assert self.default_path.is_file()
-                assert self.default_path.name == "collection.db"
-                assert self.default_path.parent == Path(tempfile.gettempdir()).parent
+                assert self.initial[0] == "sqlite:///:memory:"
 
             def pytest_collection_finish(self):
                 assert snapshot() == self.initial, "collection changed database bindings"
@@ -125,11 +118,9 @@ def test_actual_tests_restore_process_state_and_ignore_inherited_database(tmp_pa
                 assert snapshot() == self.initial, "test leaked database or module state"
 
         inherited_url = os.environ["DATABASE_URL"]
-        guard = StateGuard()
         result = pytest.main(["-q", "-p", "no:cacheprovider", *sys.argv[1:]],
-                             plugins=[guard])
+                             plugins=[StateGuard()])
         assert os.environ["DATABASE_URL"] == inherited_url
-        assert not guard.default_path.exists(), "collection database outlived its runtime"
         raise SystemExit(result)
     ''')
     result = subprocess.run(

tests/test_notes_native_cleanup.py

@@ -0,0 +1,43 @@
+"""A detached Notes descendant must not hold output open or kill a bystander."""
+import os
+import runpy
+import subprocess
+import sys
+from pathlib import Path
+
+import pytest
+from src import process_lifecycle as lifecycle
+
+ROOT = Path(__file__).resolve().parents[1]
+
+
+@pytest.mark.skipif(os.name == "nt", reason="POSIX detached-session regression; Windows uses taskkill tree")
+def test_notes_cleanup_reaps_detached_pipe_holder_and_preserves_bystander():
+    stop = runpy.run_path(str(ROOT / "scripts/verify_notes_browser_runtime.py"))["_stop_owned_server"]
+    child = "import time; time.sleep(60)"
+    program = ("import subprocess,sys,time; "
+               f"p=subprocess.Popen([sys.executable,'-c',{child!r}],start_new_session=True); "
+               "print(p.pid,flush=True); time.sleep(60)")
+    with subprocess.Popen([sys.executable, "-c", child]) as bystander:
+        server = subprocess.Popen([sys.executable, "-c", program], stdout=subprocess.PIPE,
+                                  stderr=subprocess.STDOUT, text=True, start_new_session=True)
+        root_identity = lifecycle.ProcessIdentity.capture(server.pid)
+        detached = None
+        try:
+            child_pid = int(server.stdout.readline())
+            detached = lifecycle.ProcessIdentity.capture(child_pid)
+            assert os.getpgid(child_pid) == child_pid
+            stop(server, root_identity)
+            assert server.poll() is not None
+            assert detached.exited()
+            assert bystander.poll() is None, "cleanup touched an unrelated process"
+        finally:
+            # Test-owned fallback cleanup also runs when the negative control
+            # intentionally substitutes the old parent-only teardown.
+            targets = [identity for identity in (detached, root_identity) if identity]
+            lifecycle.terminate_identities(targets, steps=lifecycle.term_kill_steps(1))
+            server.wait(timeout=3)
+            if server.stdout:
+                server.stdout.close()
+            bystander.terminate()
+            bystander.wait(timeout=3)

tests/test_worker_runtime.py

@@ -79,32 +79,3 @@ def test_tmp_path_under_private_runtime_fits_unix_socket(tmp_path):
     # witness. A nested pytest-of-<user> basetemp made it 110 bytes under xdist.
     with socket.socket(socket.AF_UNIX) as sock:
         sock.bind(str(tmp_path / "tmux.sock"))
-
-
-def test_collection_database_survives_thread_connection_churn():
-    """TestClient threads must not lose the collection database or its schema.
-
-    SingletonThreadPool's default five-connection limit used to close the main
-    thread's in-memory database after repeated route clients. Each new thread
-    also saw an empty database. Exercise more threads than that limit and then
-    verify both compatibility factories still see all registered tables.
-    """
-    from concurrent.futures import ThreadPoolExecutor
-    from sqlalchemy import inspect
-    import core.database as database
-    import src.database as compatibility
-
-    expected = set(database.Base.metadata.tables)
-
-    def table_names(factory):
-        with factory() as session:
-            return set(inspect(session.connection()).get_table_names())
-
-    assert {"documents", "model_endpoints", "mcp_servers", "calendars"} <= expected
-    assert expected <= table_names(database.SessionLocal)
-    for _ in range(8):
-        # A fresh executor creates a fresh thread rather than reusing one.
-        with ThreadPoolExecutor(max_workers=1) as executor:
-            assert expected <= executor.submit(table_names, database.SessionLocal).result()
-    assert expected <= table_names(database.SessionLocal)
-    assert expected <= table_names(compatibility.SessionLocal)