pyln-testing: replace ephemeral-port-reserve with a filesystem lock approach
What the developer wrote
Authored by daywalker90
pyln-testing: replace ephemeral-port-reserve with a filesystem lock approach
For some reason ports go back into the available pool sooner than what we would expect. I'm not sure why, but it clearly happens.
ephemeral_port_reserve puts them in TIME_WAIT, we can bind to them immediately because of SO_REUSEADDR set,
but then, probably because of node restarts, during a test the port is somehow returned
to a state where ephemeral_port_reserve will return it again immediately for another test.
So we need an additional mechanism for the ports to stay consistent during a test with node restarts.
Our current mechanism was flawed in combination with pytest-xdist since it was tracking per worker, not per machine.
This new filesystem approach should work across pytest workers
Changelog-None
What changed, and why it matters
This commit fixes a flaky testing bug in Core Lightning's Python test helpers. When running many tests in parallel, the old code could accidentally reuse the same network port for two different test nodes, causing test failures or weird node-restart behavior. The fix switches to a file-based lock shared across all test workers on the same machine. It is a test-infrastructure reliability fix, not a fix for a user-facing security vulnerability.
No urgent action required. Treat as a normal test-framework reliability improvement. Reviewers may want to verify that `cleanup_stale_port_locks()` cannot be abused via symlink or stale PID reuse, though the impact is limited to the test environment and the lock directory is world-writable under `/tmp`.
Security signals we found
Race condition in port allocation logic (test-only)
Shared resource coordination fix across parallel workers
Best-effort cleanup of stale lockfiles with broad exception swallowing
Evidence from the diff
The patch replaces a per-process set (unused_port_set protected by threading.Lock) with a filesystem lock directory (/tmp/pyln-testing-ports/<port>.lock) so that reserve_unused_port() and drop_unused_port() coordinate across pytest-xdist workers on the same host. It also adds cleanup_stale_port_locks() to reap lockfiles whose owning PID no longer exists, and refactors BitcoinD cleanup into a kill() method that releases the reserved RPC port. The change is defensive and improves test isolation; it does not patch an exploitable vulnerability in production code.
Changed components
contrib/pyln-testing/pyln/testing/utils.pycontrib/pyln-testing/pyln/testing/fixtures.pyBitcoinD test fixturereserve_unused_port / drop_unused_port helpersInspect captured patch +45 / −24
diff --git a/contrib/pyln-testing/pyln/testing/fixtures.py b/contrib/pyln-testing/pyln/testing/fixtures.py
index 335667cf..2b5556c6 100644
--- a/contrib/pyln-testing/pyln/testing/fixtures.py
+++ b/contrib/pyln-testing/pyln/testing/fixtures.py
@@ -193,13 +193,7 @@ def bitcoind(request, directory, teardown_checks):
yield bitcoind
- try:
- bitcoind.stop()
- except Exception:
- bitcoind.proc.kill()
- bitcoind.proc.wait()
-
- bitcoind.cleanup_files()
+ bitcoind.kill()
class TeardownErrors(object):
diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py
index 30639034..c8c5d6b0 100644
--- a/contrib/pyln-testing/pyln/testing/utils.py
+++ b/contrib/pyln-testing/pyln/testing/utils.py
@@ -14,6 +14,7 @@ from pyln.client import NodeVersion
from pyln.client import Plugin
import ephemeral_port_reserve # type: ignore
+import tempfile
import json
import logging
import lzma
@@ -171,25 +172,46 @@ def get_tx_p2wsh_outnum(bitcoind, tx, amount):
return None
-unused_port_lock = threading.Lock()
-unused_port_set = set()
+_PORT_LOCK_DIR = Path(tempfile.gettempdir()) / "pyln-testing-ports"
+_PORT_LOCK_DIR.mkdir(exist_ok=True)
def reserve_unused_port():
"""Get an unused port: avoids handing out the same port unless it's been
returned"""
- with unused_port_lock:
- while True:
- port = ephemeral_port_reserve.reserve()
- if port not in unused_port_set:
- break
- unused_port_set.add(port)
+ while True:
+ port = ephemeral_port_reserve.reserve()
- return port
+ lock_path = _PORT_LOCK_DIR / f"{port}.lock"
+ try:
+ fd = os.open(str(lock_path), os.O_CREAT | os.O_EXCL | os.O_WRONLY)
+ os.write(fd, str(os.getpid()).encode())
+ os.close(fd)
+ return port
+ except FileExistsError:
+ continue
def drop_unused_port(port):
- unused_port_set.remove(port)
+ if port:
+ lock_path = _PORT_LOCK_DIR / f"{port}.lock"
+ lock_path.unlink(missing_ok=True)
+
+
+def cleanup_stale_port_locks():
+ """Remove lockfiles whose owning process no longer exists."""
+ try:
+ for lock_path in _PORT_LOCK_DIR.glob("*.lock"):
+ try:
+ pid = int(lock_path.read_text())
+ try:
+ os.kill(pid, 0) # signal 0 = existence check, no actual signal
+ except ProcessLookupError:
+ lock_path.unlink(missing_ok=True)
+ except (ValueError, PermissionError, FileNotFoundError):
+ pass
+ except Exception:
+ pass # best-effort, never crash the test run over cleanup
class TailableProc(object):
@@ -290,6 +312,8 @@ class TailableProc(object):
def cleanup_files(self):
"""Ensure files are closed."""
+ cleanup_stale_port_locks()
+
for f in ["stdout_write", "stderr_write", "stdout_read", "stderr_read"]:
try:
getattr(self, f).close()
@@ -459,10 +483,7 @@ class BitcoinD(TailableProc):
TailableProc.__init__(self, bitcoin_dir, verbose=False)
if rpcport is None:
- self.reserved_rpcport = reserve_unused_port()
- rpcport = self.reserved_rpcport
- else:
- self.reserved_rpcport = None
+ rpcport = reserve_unused_port()
self.bitcoin_dir = bitcoin_dir
self.rpcport = rpcport
@@ -499,9 +520,15 @@ class BitcoinD(TailableProc):
self.rpc = SimpleBitcoinProxy(btc_conf_file=self.conf_file)
self.proxies = []
- def __del__(self):
- if self.reserved_rpcport is not None:
- drop_unused_port(self.reserved_rpcport)
+ def kill(self):
+ try:
+ self.stop()
+ except Exception:
+ self.proc.kill()
+ self.proc.wait()
+
+ self.cleanup_files()
+ drop_unused_port(self.rpcport)
def start(self, wallet_file=None):
TailableProc.start(self)
Why this scored 11/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.