From 36c77a851fc8d56fdbf95c5d67bcde7b749af6ae Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 6 Oct 2026 23:21:31 -0500 Subject: [PATCH] Close leaked pipes, HTTPError sockets and sqlite handles (#90), v1.1.1 - http_lane: close the HTTPError on every path (retry and re-raise); the error body is read before closing. - cli_lane: `with proc:` so the timed-out/killed path closes its pipes. - acp_lane: close stdin/stdout in the finally after the reap. - open_store: close the connection when the DB key is missing or wrong; die() used to leave it open (new since the issue was filed, found the same way). - tests: close the four sqlite connections the tests themselves leaked. - CI: run the suite with -W always::ResourceWarning and fail on any hit. A finalizer warning is printed, not raised, so -W error cannot do it. The suite went from 28+ ResourceWarnings to 0; 244 tests OK. Closes #90 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015eWZfxrGjWj2QpwdgFwBjR --- .github/workflows/ci.yml | 11 ++++++++- BACKLOG.md | 5 ++-- roundtable | 49 ++++++++++++++++++++++++++++++---------- tests/test_roundtable.py | 9 +++++++- 4 files changed, 58 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dffffc7..bc1a1b5 100644 --- a/.github/workflows/ci.yml +++ b/.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. diff --git a/BACKLOG.md b/BACKLOG.md index a105fb0..a7e6b5f 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -72,10 +72,11 @@ The UI lives in its own private repo, - [x] `--revise latest:` 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 diff --git a/roundtable b/roundtable index 0ff034c..891f82c 100755 --- a/roundtable +++ b/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,6 +2378,14 @@ 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. @@ -2379,7 +2393,7 @@ def http_lane(lane: dict, prompt: str, key: str | None, retries: int) -> dict: 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) diff --git a/tests/test_roundtable.py b/tests/test_roundtable.py index e95516a..ceced3f 100644 --- a/tests/test_roundtable.py +++ b/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):