perf(tests): bound Notes server descendant cleanup

package-lock.json

@@ -6,15 +6,15 @@
     "": {
       "name": "odysseus",
       "devDependencies": {
-        "@antithesishq/bombadil": "^0.7.10",
-        "@playwright/test": "^1.64.1",
+        "@antithesishq/bombadil": "^0.7.0",
+        "@playwright/test": "^1.62.1",
         "acorn": "8.19.0"
       }
     },
     "node_modules/@antithesishq/bombadil": {
-      "version": "0.7.10",
-      "resolved": "https://registry.npmjs.org/@antithesishq/bombadil/-/bombadil-0.7.10.tgz",
-      "integrity": "sha512-71aLKQBB3GNDBQZXU6MCKbt99dl9I+/pjg0EfGD9eCKXDIqpKPXD2rFH6yCJude37RGtnKq6hZFfZhuTp+Tykg==",
+      "version": "0.7.0",
+      "resolved": "https://registry.npmjs.org/@antithesishq/bombadil/-/bombadil-0.7.0.tgz",
+      "integrity": "sha512-alJmnphJ/iUoL5mCsnV3DwtajGy/sEQ3NJJCiMhgjqXshSq2BUtAs0vqdXEiiSkB8HbsOX5CLrAcaogYdwfAJg==",
       "dev": true,
       "license": "MIT",
       "bin": {
@@ -22,13 +22,13 @@
       }
     },
     "node_modules/@playwright/test": {
-      "version": "1.64.1",
-      "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.64.1.tgz",
-      "integrity": "sha512-IaYGml/nSOiZWROfPtuIL7neuu9qDOZ3TNvNljUIrhDMxiGTgPjzj/g9p9RxpS2tSepPDRY+UwAaEeiKtUvIAg==",
+      "version": "1.62.1",
+      "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.62.1.tgz",
+      "integrity": "sha512-DTcUc8qii+cpHvtOwggMtBRMjKZHXYWdw8syRYu2vtzuq4Wxphqq4NfCs5Zt44L6mA8rfDfj+PHnxFc/FeK6mQ==",
       "dev": true,
       "license": "Apache-2.0",
       "dependencies": {
-        "playwright": "1.64.1"
+        "playwright": "1.62.1"
       },
       "bin": {
         "playwright": "cli.js"
@@ -50,26 +50,44 @@
         "node": ">=0.4.0"
       }
     },
+    "node_modules/fsevents": {
+      "version": "2.3.2",
+      "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz",
+      "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==",
+      "dev": true,
+      "hasInstallScript": true,
+      "license": "MIT",
+      "optional": true,
+      "os": [
+        "darwin"
+      ],
+      "engines": {
+        "node": "^8.16.0 || ^10.6.0 || >=11.0.0"
+      }
+    },
     "node_modules/playwright": {
-      "version": "1.64.1",
-      "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.64.1.tgz",
-      "integrity": "sha512-HCrVhHr5QQuwYoswEUGTb7gGp522yflbv/ST8nF4sFWGuLjQCt17Q/EubpGGSNKYn1tsTCPZxqHhHCfVURu7UA==",
+      "version": "1.62.1",
+      "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.62.1.tgz",
+      "integrity": "sha512-0M+L3LAD8/nm554LOla9Ayx0j0tmFZ0FBcoQ7F1VuVHpM/XpiC8RcDzBQB8W5+hA8L22THxELzeF+2WcUzvcLg==",
       "dev": true,
       "license": "Apache-2.0",
       "dependencies": {
-        "playwright-core": "1.64.1"
+        "playwright-core": "1.62.1"
       },
       "bin": {
         "playwright": "cli.js"
       },
       "engines": {
         "node": ">=20"
+      },
+      "optionalDependencies": {
+        "fsevents": "2.3.2"
       }
     },
     "node_modules/playwright-core": {
-      "version": "1.64.1",
-      "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.64.1.tgz",
-      "integrity": "sha512-BC4s8y6SEVA6Q1rSxZTto8hI1KUI3xXlHGZgvGqP4IqPdI/a6FX9H29IDNz7vKo9l21b6LfYosScPL6M3xxyDQ==",
+      "version": "1.62.1",
+      "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.62.1.tgz",
+      "integrity": "sha512-wPYSwEBJY9GHraISXqyqtx0na0LpO3XEX7jNDhntbex7tzUS7kLnZsOlFruFJB4Hi/rhDMjXGqHewDZ68nYZVw==",
       "dev": true,
       "license": "Apache-2.0",
       "bin": {

package.json

@@ -12,8 +12,8 @@
     "test:photo-editor:webkit": "PHOTO_EDITOR_E2E_BROWSER=webkit playwright test --config tests/e2e/playwright.config.js"
   },
   "devDependencies": {
-    "@antithesishq/bombadil": "^0.7.10",
-    "@playwright/test": "^1.64.1",
+    "@antithesishq/bombadil": "^0.7.0",
+    "@playwright/test": "^1.62.1",
     "acorn": "8.19.0"
   }
 }

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/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)