fix(lease-broker): wait_ready() polls real connect-readiness not socket-file existence (flaky CI race) #898

jason.woltje · 2026-07-25T21:48:08Z

jason.woltje commented

2026-07-25 21:48:08 +00:00

Root cause

packages/mosaic/src/lease-broker/recovery_b1_adversarial_unittest.py's setUp()
spawns the lease-broker daemon subprocess and calls wait_ready(), which polled
only socket_path.exists(). On a unix domain socket the file is created at
bind() time — before the daemon calls listen(). Under concurrent CI-runner
load, the client's subsequent connect() can race ahead of listen() and get
ConnectionRefusedError: [Errno 111], erroring the whole suite in setUp()
(classic TOCTOU: the file's existence was checked, but the property actually
needed — "accepting connections" — was not).

The fix (exact scalpel)

Only wait_ready() in this one file changed. File-existence polling is
replaced with a real connect() retry loop + timeout: each iteration attempts
an actual socket.connect(socket_path), catches
ConnectionRefusedError/FileNotFoundError, sleeps briefly, and retries until
a connection actually succeeds (then closes it) or the same 5s deadline
elapses (then fails loudly exactly as before, same TimeoutError/RuntimeError
semantics). Readiness is now "the daemon is actually accepting connections,"
not "a file happens to exist," which eliminates the race.

 def wait_ready(process: subprocess.Popen[str], socket_path: Path) -> None:
     deadline = time.monotonic() + 5.0
     while time.monotonic() < deadline:
-        if socket_path.exists():
+        try:
+            with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as probe:
+                probe.settimeout(0.25)
+                probe.connect(str(socket_path))
             return
+        except (ConnectionRefusedError, FileNotFoundError):
+            pass
         if process.poll() is not None:
             output = process.stdout.read() if process.stdout is not None else ""
             raise RuntimeError(f"daemon exited before READY: {output}")
         time.sleep(0.02)
     raise TimeoutError("daemon did not create private socket")

Zero assertion changes, zero daemon changes, zero gate-timeout relaxation.
Verified: git diff origin/main -- packages/mosaic/src/lease-broker/recovery_b1_adversarial_unittest.py | grep -E '^[-+].*assert' is empty.

Note for reviewer (not fixed here, out of scope per issue)

The identical TOCTOU wait_ready() pattern (poll socket_path.exists()) is
also duplicated verbatim in:

packages/mosaic/src/lease-broker/recovery_runtime_unittest.py
docs/compaction-refresh/probes/p5_receipt_replay.py
docs/compaction-refresh/probes/p6_constrained_recovery.py

These were intentionally left untouched per this issue's exact scope (only the
file named in #897). They likely warrant an identical follow-up fix in a
separate PR/issue.

Local verification

Ran the recovery suite standalone (unix-socket based, no Postgres needed) 6
times, all green:

$ python3 -m unittest recovery_b1_adversarial_unittest -v
test_canonical_literal_and_shipped_skill_invocation_are_ungated ... ok
test_every_shell_active_vector_in_every_argv_position_falls_through_to_bash_deny ... ok

Ran 2 tests in 8.1s
OK

(repeated 5 more times, all OK)

No repo-configured Python linter (ruff/flake8/mypy) is present in CI or the
environment for this package; python3 -m py_compile confirms no syntax
errors.

Closes #897

## Root cause `packages/mosaic/src/lease-broker/recovery_b1_adversarial_unittest.py`'s `setUp()` spawns the lease-broker daemon subprocess and calls `wait_ready()`, which polled only `socket_path.exists()`. On a `unix` domain socket the file is created at `bind()` time — *before* the daemon calls `listen()`. Under concurrent CI-runner load, the client's subsequent `connect()` can race ahead of `listen()` and get `ConnectionRefusedError: [Errno 111]`, erroring the whole suite in `setUp()` (classic TOCTOU: the file's existence was checked, but the property actually needed — "accepting connections" — was not). ## The fix (exact scalpel) Only `wait_ready()` in this one file changed. File-existence polling is replaced with a real `connect()` retry loop + timeout: each iteration attempts an actual `socket.connect(socket_path)`, catches `ConnectionRefusedError`/`FileNotFoundError`, sleeps briefly, and retries until a connection actually succeeds (then closes it) or the same 5s deadline elapses (then fails loudly exactly as before, same `TimeoutError`/`RuntimeError` semantics). Readiness is now "the daemon is actually accepting connections," not "a file happens to exist," which eliminates the race. ```diff def wait_ready(process: subprocess.Popen[str], socket_path: Path) -> None: deadline = time.monotonic() + 5.0 while time.monotonic() < deadline: - if socket_path.exists(): + try: + with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as probe: + probe.settimeout(0.25) + probe.connect(str(socket_path)) return + except (ConnectionRefusedError, FileNotFoundError): + pass if process.poll() is not None: output = process.stdout.read() if process.stdout is not None else "" raise RuntimeError(f"daemon exited before READY: {output}") time.sleep(0.02) raise TimeoutError("daemon did not create private socket") ``` **Zero assertion changes, zero daemon changes, zero gate-timeout relaxation.** Verified: `git diff origin/main -- packages/mosaic/src/lease-broker/recovery_b1_adversarial_unittest.py | grep -E '^[-+].*assert'` is empty. ## Note for reviewer (not fixed here, out of scope per issue) The identical TOCTOU `wait_ready()` pattern (poll `socket_path.exists()`) is also duplicated verbatim in: - `packages/mosaic/src/lease-broker/recovery_runtime_unittest.py` - `docs/compaction-refresh/probes/p5_receipt_replay.py` - `docs/compaction-refresh/probes/p6_constrained_recovery.py` These were intentionally left untouched per this issue's exact scope (only the file named in #897). They likely warrant an identical follow-up fix in a separate PR/issue. ## Local verification Ran the recovery suite standalone (unix-socket based, no Postgres needed) 6 times, all green: ``` $ python3 -m unittest recovery_b1_adversarial_unittest -v test_canonical_literal_and_shipped_skill_invocation_are_ungated ... ok test_every_shell_active_vector_in_every_argv_position_falls_through_to_bash_deny ... ok Ran 2 tests in 8.1s OK ``` (repeated 5 more times, all `OK`) No repo-configured Python linter (ruff/flake8/mypy) is present in CI or the environment for this package; `python3 -m py_compile` confirms no syntax errors. Closes #897

jason.woltje added 1 commit 2026-07-25 21:48:09 +00:00

fix(lease-broker): wait_ready() polls real connect-readiness not socket-file existence (flaky CI race)

ci/woodpecker/pr/ci Pipeline was successful

Details

98a20bfaab

setUp() spawned the daemon and polled only for the unix-socket file to
exist before proceeding to connect(). Under concurrent CI-runner load
the socket file can appear (created by bind()) before the daemon calls
listen(), so the client's immediate connect() raced ahead and hit
ConnectionRefusedError: [Errno 111], erroring the whole suite in
setUp().

wait_ready() now attempts a real socket.connect() in a retry loop
(catching ConnectionRefusedError/FileNotFoundError) until a connection
actually succeeds (then closes it) or the same 5s deadline elapses,
at which point it fails exactly as before. Readiness is now "the
daemon is actually accepting connections," not "a file happens to
exist," eliminating the TOCTOU race.

Scope: only wait_ready() in this file changed. Zero assertion changes,
zero daemon/lease-broker source changes, zero gate-timeout relaxation.

Closes #897

jason.woltje commented

2026-07-25 22:09:47 +00:00

Record of Review — PR #898 (wait_ready() flake fix; Closes #897)

VERDICT: APPROVE. Reviewer: independent subagent a34174ada (ms-lead-reviewer), fresh clone, first-principles — ≠ builder (a0c8fbb1, commit-author mosaic-coder).
REVIEWED-HEAD: 98a20bfaaba90fa00de01d4a825ffdc260cd94f6 (unmoved).

Per-criterion (all PASS)

★ MANDATORY zero-assertion-change gate — PASS. git diff origin/main -- recovery_b1_adversarial_unittest.py | grep -E "^[-+].*assert" = EMPTY. Only wait_ready()s readiness probe changed (socket_path.exists() → real socket.connect() in a with context-manager, settimeout(0.25), catch ECONNREFUSED/FileNotFoundError, retry). 5s deadline, daemon-exited-early check, and the exact TimeoutError message byte-unchanged.
Scope — PASS. 1 file, +6/-1; no daemon/source files.
Correctness — PASS. Readiness now = daemon accepting connections (post-listen), not file-exists (post-bind) — the actual TOCTOU fix; probe closes cleanly, own timeout, all failure semantics preserved.
Suite green, no race — PASS. Ran the suite 8x independently, all OK, zero ECONNREFUSED.
Gates/linkage — PASS. firewall 0; body+commit Closes #897; commit-author mosaic-coder ≠ reviewer.
Duplicate-pattern — PASS. recovery_runtime_unittest.py + p5/p6 probes byte-identical to main (still unfixed) — correctly left out of #897 scope; follow-up candidate (Mos-approved).

CI + a correction to the reviewers CI caveat (MS-LEAD verified)

CI TERMINAL-GREEN: Woodpecker pipeline 2017 @ 98a20bfa, all 8 steps success incl test.
The reviewer noted CI test "is vitest-only, never invokes recovery_b1" — this is INCORRECT (I verified from source): the CI test step runs pnpm test → turbo run test → packages/mosaic test = vitest run && pnpm run test:framework-shell, and test:framework-shell chains 11 Python unittests INCLUDING recovery_b1_adversarial_unittest.py. So recovery_b1 DOES run in CI and #898 fixes a REAL CI flake vector (this is what caused #893/#894/#895s test failures).

GO for id-11 + merge at FULL-40 head 98a20bfaaba90fa00de01d4a825ffdc260cd94f6. I relay; Mos stamps id-11 + merges. Non-executor.

# Record of Review — PR #898 (wait_ready() flake fix; Closes #897) **VERDICT: APPROVE.** **Reviewer:** independent subagent `a34174ada` (`ms-lead-reviewer`), fresh clone, first-principles — ≠ builder (`a0c8fbb1`, commit-author `mosaic-coder`). **REVIEWED-HEAD:** `98a20bfaaba90fa00de01d4a825ffdc260cd94f6` (unmoved). ## Per-criterion (all PASS) 1. **★ MANDATORY zero-assertion-change gate — PASS.** `git diff origin/main -- recovery_b1_adversarial_unittest.py | grep -E "^[-+].*assert"` = EMPTY. Only `wait_ready()`s readiness probe changed (`socket_path.exists()` → real `socket.connect()` in a `with` context-manager, `settimeout(0.25)`, catch ECONNREFUSED/FileNotFoundError, retry). 5s deadline, daemon-exited-early check, and the exact `TimeoutError` message byte-unchanged. 2. **Scope — PASS.** 1 file, +6/-1; no daemon/source files. 3. **Correctness — PASS.** Readiness now = daemon accepting connections (post-listen), not file-exists (post-bind) — the actual TOCTOU fix; probe closes cleanly, own timeout, all failure semantics preserved. 4. **Suite green, no race — PASS.** Ran the suite 8x independently, all OK, zero ECONNREFUSED. 5. **Gates/linkage — PASS.** firewall 0; body+commit `Closes #897`; commit-author mosaic-coder ≠ reviewer. 6. **Duplicate-pattern — PASS.** recovery_runtime_unittest.py + p5/p6 probes byte-identical to main (still unfixed) — correctly left out of #897 scope; follow-up candidate (Mos-approved). ## CI + a correction to the reviewers CI caveat (MS-LEAD verified) - **CI TERMINAL-GREEN:** Woodpecker pipeline 2017 @ `98a20bfa`, all 8 steps success incl `test`. - The reviewer noted CI `test` "is vitest-only, never invokes recovery_b1" — **this is INCORRECT (I verified from source):** the CI `test` step runs `pnpm test` → `turbo run test` → `packages/mosaic` `test` = `vitest run && pnpm run test:framework-shell`, and `test:framework-shell` chains 11 Python unittests INCLUDING `recovery_b1_adversarial_unittest.py`. So recovery_b1 DOES run in CI and #898 fixes a REAL CI flake vector (this is what caused #893/#894/#895s `test` failures). **GO for id-11 + merge at FULL-40 head `98a20bfaaba90fa00de01d4a825ffdc260cd94f6`. I relay; Mos stamps id-11 + merges. Non-executor.**

Mos approved these changes 2026-07-25 22:10:14 +00:00

Mos left a comment

First-time contributor

GO — Gate-16 id-11 stamp, pinned to exact FULL head 98a20bfaab.
Basis: verbatim RoR (comment 18803) — independent review APPROVE with the MANDATORY zero-assertion-change gate PASSED (assert-diff empty; only the wait_ready readiness probe changed, file-existence → connect-and-close, same deadline and error message; zero daemon changes; 8x consecutive local clean). CI terminal-green (2017, 8/8). Closes #897. Test-locked lease-broker area handled per the scalpel ruling: the fix changes WHEN the harness starts testing, never WHAT it asserts. MS-LEAD's source-verified refinement noted on record: recovery_b1 IS in the CI chain (reviewer's vitest-only claim corrected), and recovery_runtime_unittest.py carries the same race as a co-equal CI vector — expedited follow-up chartered. Named executor: Mos (id-11), squash pinned.

GO — Gate-16 id-11 stamp, pinned to exact FULL head 98a20bfaaba90fa00de01d4a825ffdc260cd94f6. Basis: verbatim RoR (comment 18803) — independent review APPROVE with the MANDATORY zero-assertion-change gate PASSED (assert-diff empty; only the wait_ready readiness probe changed, file-existence → connect-and-close, same deadline and error message; zero daemon changes; 8x consecutive local clean). CI terminal-green (2017, 8/8). Closes #897. Test-locked lease-broker area handled per the scalpel ruling: the fix changes WHEN the harness starts testing, never WHAT it asserts. MS-LEAD's source-verified refinement noted on record: recovery_b1 IS in the CI chain (reviewer's vitest-only claim corrected), and recovery_runtime_unittest.py carries the same race as a co-equal CI vector — expedited follow-up chartered. Named executor: Mos (id-11), squash pinned.

Mos merged commit 79c8647fd9 into main

2026-07-25 22:10:15 +00:00

Mos deleted branch fix/recovery-b1-wait-ready-race

2026-07-25 22:10:16 +00:00

Mos referenced this issue from a commit

2026-07-25 22:10:17 +00:00

fix(lease-broker): wait_ready() polls real connect-readiness not socket-file existence (flaky CI race) (#898)

jason.woltje referenced this pull request

2026-07-25 22:11:16 +00:00

flaky-test (expedited): apply #898 wait_ready connect-probe to recovery_runtime_unittest.py (co-equal CI vector) #899

jason.woltje referenced this issue from a commit

2026-07-25 22:13:36 +00:00