test: fix flaky TLS end-to-end tests (TOCTOU port race) (#4677)
test_tls_support.py E2E tests have flaked repeatedly across releases. Root cause: _find_free_port() binds port 0, reads the port, CLOSES the socket, then returns — leaving a TOCTOU window where the OS reassigns that port (common under the 9-shard parallel suite + concurrent agents) before server.py binds it. Server fails to bind, _wait_for_server polls a dead process for the full 8s, test times out. Fix (test-only): new _start_and_wait() retries bring-up on a FRESHLY chosen port (defeats the race), _wait_for_server bails the moment proc.poll() shows the subprocess died (fast retry signal, not a slow timeout), timeout bumped 8s->30s for cold imports under load, and server output is captured into the AssertionError on hard failure. Verified deterministic: 4 consecutive clean runs + a run under 50-held-port contention. Co-authored-by: nesquena-hermes <agent@nesquena-hermes>
This commit is contained in:
1 parent
3edbec7c97
commit
de385c2664
2 files changed
+64
-22
No files matched your search
@@ -3,6 +3,12 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
## [v0.51.579] — 2026-06-22 — Release UL (deflake TLS end-to-end tests)
|
||||
|
||||
### Fixed
|
||||
|
||||
- **Test reliability: the TLS end-to-end tests no longer flake.** The `test_tls_support.py` end-to-end tests picked a free port, released it, then started the server — leaving a TOCTOU window where the OS could reassign that port (common under the parallel test suite), so the server failed to bind and the test timed out intermittently. The harness now retries server bring-up on a freshly chosen port, detects a died subprocess immediately instead of polling to the deadline, and surfaces server output on failure. Test-only; no product change.
|
||||
|
||||
## [v0.51.578] — 2026-06-22 — Release UK (preserve reader scroll position across re-render)
|
||||
|
||||
### Fixed
|
||||
|
||||
+58
-22
@@ -43,8 +43,15 @@ def _find_free_port() -> int:
|
||||
|
||||
|
||||
def _wait_for_server(host: str, port: int, use_ssl: bool = False,
|
||||
timeout: float = 8.0) -> bool:
|
||||
"""Poll until the server accepts a connection or times out."""
|
||||
timeout: float = 30.0, proc: "subprocess.Popen | None" = None) -> bool:
|
||||
"""Poll until the server accepts a connection or times out.
|
||||
|
||||
Bumped to 30s because the server subprocess imports the whole app, which can
|
||||
exceed a tight budget under the parallel (9-shard) suite + concurrent agents.
|
||||
If ``proc`` is supplied, bail out early the moment the subprocess has exited
|
||||
(e.g. failed to bind the port) instead of polling a dead process to the
|
||||
deadline — that turns a slow flake into a fast, deterministic retry signal.
|
||||
"""
|
||||
ctx = None
|
||||
if use_ssl:
|
||||
ctx = ssl.create_default_context()
|
||||
@@ -52,6 +59,8 @@ def _wait_for_server(host: str, port: int, use_ssl: bool = False,
|
||||
ctx.verify_mode = ssl.CERT_NONE
|
||||
deadline = time.time() + timeout
|
||||
while time.time() < deadline:
|
||||
if proc is not None and proc.poll() is not None:
|
||||
return False # subprocess died (e.g. port already taken) — don't wait it out
|
||||
try:
|
||||
if use_ssl:
|
||||
c = http.client.HTTPSConnection(host, port, timeout=2, context=ctx)
|
||||
@@ -63,7 +72,7 @@ def _wait_for_server(host: str, port: int, use_ssl: bool = False,
|
||||
c.close()
|
||||
return True
|
||||
except Exception:
|
||||
time.sleep(0.5)
|
||||
time.sleep(0.25)
|
||||
return False
|
||||
|
||||
|
||||
@@ -88,6 +97,45 @@ def _start_server(port: int, cert: str = None, key: str = None) -> subprocess.Po
|
||||
return proc
|
||||
|
||||
|
||||
def _terminate(proc: "subprocess.Popen | None") -> None:
|
||||
if proc is None or proc.poll() is not None:
|
||||
return
|
||||
with suppress(Exception):
|
||||
proc.terminate()
|
||||
try:
|
||||
proc.wait(timeout=5)
|
||||
except subprocess.TimeoutExpired:
|
||||
proc.kill()
|
||||
|
||||
|
||||
def _start_and_wait(use_ssl: bool, cert: str = None, key: str = None,
|
||||
attempts: int = 4) -> subprocess.Popen:
|
||||
"""Start the server and wait until it is reachable, retrying on a fresh port.
|
||||
|
||||
Defeats the _find_free_port() TOCTOU race (the OS can hand the just-released
|
||||
port to another process before server.py binds it, especially under the
|
||||
parallel suite). On a failed bring-up we tear the subprocess down and retry
|
||||
with a NEWLY chosen port. Returns the live Popen, or raises AssertionError
|
||||
with the captured server output after exhausting attempts.
|
||||
"""
|
||||
last_output = ""
|
||||
for _ in range(attempts):
|
||||
port = _find_free_port()
|
||||
proc = _start_server(port, cert=cert, key=key)
|
||||
if _wait_for_server("127.0.0.1", port, use_ssl=use_ssl, proc=proc):
|
||||
proc._test_port = port # type: ignore[attr-defined]
|
||||
return proc
|
||||
# Capture diagnostics before retrying with a fresh port.
|
||||
with suppress(Exception):
|
||||
os.set_blocking(proc.stdout.fileno(), False)
|
||||
last_output = (proc.stdout.read(4000) or "")[:4000]
|
||||
_terminate(proc)
|
||||
raise AssertionError(
|
||||
f"server did not become reachable after {attempts} attempts "
|
||||
f"(use_ssl={use_ssl}); last server output:\n{last_output}"
|
||||
)
|
||||
|
||||
|
||||
# ── Test class ──────────────────────────────────────────────────────────────
|
||||
|
||||
class TestTLSConfigFlag(unittest.TestCase):
|
||||
@@ -162,12 +210,8 @@ class TestTLSEndToEnd(unittest.TestCase):
|
||||
self._proc.kill()
|
||||
|
||||
def test_https_server_responds_to_health(self):
|
||||
port = _find_free_port()
|
||||
self._proc = _start_server(port, cert=self._cert, key=self._key)
|
||||
self.assertTrue(
|
||||
_wait_for_server("127.0.0.1", port, use_ssl=True),
|
||||
"TLS server did not start in time",
|
||||
)
|
||||
self._proc = _start_and_wait(use_ssl=True, cert=self._cert, key=self._key)
|
||||
port = self._proc._test_port
|
||||
ctx = ssl.create_default_context()
|
||||
ctx.check_hostname = False
|
||||
ctx.verify_mode = ssl.CERT_NONE
|
||||
@@ -180,11 +224,8 @@ class TestTLSEndToEnd(unittest.TestCase):
|
||||
conn.close()
|
||||
|
||||
def test_http_without_tls_still_works(self):
|
||||
port = _find_free_port()
|
||||
self._proc = _start_server(port)
|
||||
self.assertTrue(
|
||||
_wait_for_server("127.0.0.1", port, use_ssl=False),
|
||||
)
|
||||
self._proc = _start_and_wait(use_ssl=False)
|
||||
port = self._proc._test_port
|
||||
conn = http.client.HTTPConnection("127.0.0.1", port, timeout=5)
|
||||
conn.request("GET", "/health")
|
||||
resp = conn.getresponse()
|
||||
@@ -196,14 +237,9 @@ class TestTLSEndToEnd(unittest.TestCase):
|
||||
@requires_fcntl
|
||||
def test_tls_startup_failure_fallback_to_http(self):
|
||||
"""Bad cert paths should print a warning and start HTTP anyway."""
|
||||
port = _find_free_port()
|
||||
self._proc = _start_server(
|
||||
port, cert="/nonexistent/cert.pem", key="/nonexistent/key.pem",
|
||||
)
|
||||
# Server should be reachable over plain HTTP even though TLS setup failed
|
||||
self.assertTrue(
|
||||
_wait_for_server("127.0.0.1", port, use_ssl=False),
|
||||
"HTTP fallback server did not start after TLS failure",
|
||||
# Server should be reachable over plain HTTP even though TLS setup failed.
|
||||
self._proc = _start_and_wait(
|
||||
use_ssl=False, cert="/nonexistent/cert.pem", key="/nonexistent/key.pem",
|
||||
)
|
||||
# Confirm TLS warning was printed
|
||||
import fcntl
|
||||
|
||||
Reference in new issue
Block a user