Close leaked pipes, HTTPError sockets and sqlite handles (#90), v1.1.1

.github/workflows/ci.yml

@@ -30,8 +30,17 @@ jobs:
       - name: Install dependencies
         run: python3 -m pip install --quiet cryptography
 
+      # A ResourceWarning raised in a finalizer is printed, never raised, so
+      # `-W error::ResourceWarning` cannot fail the step. Show every one and grep
+      # for it instead: a leaked pipe, socket or sqlite handle fails CI (#90).
       - name: Test suite (no PyYAML -- what a run has)
-        run: python3 -m unittest discover -s tests -v
+        run: |
+          set -o pipefail
+          python3 -W always::ResourceWarning -m unittest discover -s tests -v 2>&1 | tee test.log
+          if grep -n ResourceWarning test.log; then
+            echo "::error::leaked resource -- rerun with PYTHONTRACEMALLOC=10 to see where it was opened"
+            exit 1
+          fi
 
       # The import-yaml tests skip without PyYAML, so run the suite once more
       # with it, on one version, to keep the migration path covered.

BACKLOG.md

@@ -72,10 +72,11 @@ The UI lives in its own private repo,
 - [x] `--revise latest:<non-int>` throws a raw `ValueError` traceback instead of
       the clean, actionable error every other bad input gives
       ([#76](https://github.com/CryptoJones/FlatlineRoundtable/issues/76)) — fixed on fix/open-issues-batch
-- [ ] Lane harnesses leak file descriptors — `acp_lane` / `cli_lane` Popen pipes
+- [x] Lane harnesses leak file descriptors — `acp_lane` / `cli_lane` Popen pipes
       and `http_lane`'s retried `HTTPError` are never closed (28 `ResourceWarning`s,
       all inside `roundtable`, none in the stub; found doing #86)
-      ([#90](https://github.com/CryptoJones/FlatlineRoundtable/issues/90))
+      ([#90](https://github.com/CryptoJones/FlatlineRoundtable/issues/90)) — fixed on fix/lane-fd-leaks (v1.1.1); CI now
+      fails on any `ResourceWarning`
 - [x] Env scrub has no test on the `acp` harness — `child_env()` is shared by
       `cli` and `acp`, but only `cli` asserts a key stays out; drift reopens #22
       ([#77](https://github.com/CryptoJones/FlatlineRoundtable/issues/77)) — fixed on fix/open-issues-batch

roundtable

@@ -96,7 +96,7 @@ from pathlib import Path
 
 # Semantic version. Bump it in the PR that changes behaviour: patch for a fix,
 # minor for a new flag or mode, major for a change that breaks a caller.
-__version__ = "1.1.0"
+__version__ = "1.1.1"
 
 TRANSCRIPT_DIR = Path.home() / ".local" / "share" / "flatline-roundtable" / "transcripts"
 RUN_ID_RE = re.compile(r"[A-Za-z0-9_-][A-Za-z0-9._-]{0,63}")
@@ -859,8 +859,14 @@ def verify_db_key(conn: sqlite3.Connection, key: bytes) -> None:
 def open_store(path: Path) -> tuple[sqlite3.Connection, bytes]:
     """An initialised, migrated store plus its verified key."""
     conn = open_roster(path)
-    key = load_db_key()
-    verify_db_key(conn, key)
+    try:
+        key = load_db_key()
+        verify_db_key(conn, key)
+    except BaseException:
+        # die() is a SystemExit; without this the caller never gets the
+        # connection to close, and a bad or missing key leaks it (#90).
+        conn.close()
+        raise
     return conn, key
 
 
@@ -2372,14 +2378,22 @@ def http_lane(lane: dict, prompt: str, key: str | None, retries: int) -> dict:
         except urllib.error.HTTPError as e:
             retry_after = e.headers.get("Retry-After") if e.headers else None
             transient = e.code >= 500 or e.code == 429
+            # urllib builds the HTTPError around the open response, so it holds a
+            # socket until closed. Nothing outside this handler can close it, and
+            # a 13-lane panel retrying 429s would leak one fd per attempt (#90).
+            # Read the body for the message first, then close on every path.
+            try:
+                err_body = b"" if transient and attempt < retries else e.read()[:200]
+            finally:
+                e.close()
             if transient and attempt < retries:
                 attempt += 1
                 # Honour Retry-After rather than hammering a rate-limited vendor.
                 wait = parse_retry_after(retry_after)
                 time.sleep(min(wait, RETRY_AFTER_CAP) if wait is not None
                            else RETRY_BACKOFF * attempt)
                 continue
-            raise RuntimeError(f"HTTP {e.code}: {e.read()[:200].decode(errors='replace')}") from None
+            raise RuntimeError(f"HTTP {e.code}: {err_body.decode(errors='replace')}") from None
         except urllib.error.URLError as e:
             if attempt < retries:
                 attempt += 1
@@ -2434,15 +2448,18 @@ def cli_lane(lane: dict, prompt: str) -> dict:
         env=env,
         start_new_session=True,
     )
-    try:
-        out, err = proc.communicate(stdin_text, timeout=lane.get("timeout", 300))
-    except subprocess.TimeoutExpired:
+    # `with` closes the pipes on every exit path. communicate() closes them on
+    # success, but not when it times out, so the kill path leaked them (#90).
+    with proc:
         try:
-            os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
-        except (ProcessLookupError, PermissionError):
-            pass
-        proc.wait(timeout=10)
-        raise RuntimeError(f"timed out after {lane.get('timeout', 300)}s (process group killed)") from None
+            out, err = proc.communicate(stdin_text, timeout=lane.get("timeout", 300))
+        except subprocess.TimeoutExpired:
+            try:
+                os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
+            except (ProcessLookupError, PermissionError):
+                pass
+            proc.wait(timeout=10)
+            raise RuntimeError(f"timed out after {lane.get('timeout', 300)}s (process group killed)") from None
 
     if proc.returncode != 0:
         raise RuntimeError(f"{lane['command']} exit {proc.returncode}: {err.strip()[:200]}")
@@ -2650,6 +2667,14 @@ def acp_lane(lane: dict, prompt: str) -> dict:
             proc.wait(timeout=10)
         except (subprocess.TimeoutExpired, OSError):
             pass
+        # The pipes outlive the process unless closed here: one leaked pair per
+        # acp lane (#90). The reader thread may still hold stdout; closing it
+        # under the thread ends its loop, which it already tolerates.
+        for f in (proc.stdin, proc.stdout):
+            try:
+                f.close()
+            except (OSError, ValueError):
+                pass
         shutil.rmtree(workdir, ignore_errors=True)
 
 

tests/test_roundtable.py

@@ -1513,6 +1513,7 @@ def test_init_then_migrate_twice_is_idempotent(self):
             self.assertEqual(r.returncode, 0, r.stderr)
             self.assertIn("already current", r.stdout)
         conn = sqlite3.connect(self.db)
+        self.addCleanup(conn.close)
         self.assertEqual(conn.execute("PRAGMA user_version").fetchone()[0], v1)
         self.assertEqual(v1, rt.SCHEMA_VERSION)
 
@@ -1527,12 +1528,17 @@ def test_concurrent_migrates_do_not_collide(self):
         self.db.parent.mkdir(parents=True, mode=0o700)
         os.close(os.open(self.db, os.O_WRONLY | os.O_CREAT, 0o600))
         holder = rt.open_db(self.db)
+        self.addCleanup(holder.close)
         holder.execute("BEGIN IMMEDIATE")
         errors = []
 
         def racer():
             try:
-                rt.migrate(rt.open_db(self.db))
+                conn = rt.open_db(self.db)
+                try:
+                    rt.migrate(conn)
+                finally:
+                    conn.close()
             except BaseException as e:   # noqa: BLE001 -- surfaced below
                 errors.append(e)
 
@@ -1638,6 +1644,7 @@ def test_verify_runs_before_any_secret_is_read(self):
         """Unit-level: a wrong key must never reach a SELECT on secrets."""
         self.init()
         conn = rt.open_db(self.db)
+        self.addCleanup(conn.close)
         seen = []
         conn.set_trace_callback(seen.append)
         with self.assertRaises(SystemExit):