P2-T3: Fix double-spawn race condition when MCP client toggles rapidly

SPECS/ARCHIVE/INDEX.md

@@ -1,11 +1,12 @@
 # mcpbridge-wrapper Tasks Archive
 
-**Last Updated:** 2026-03-01 (P2-T1 archived)
+**Last Updated:** 2026-03-01 (P2-T3 archived)
 
 ## Archived Tasks
 
 | Task ID | Folder | Archived | Verdict |
 |---------|--------|----------|---------|
+| P2-T3 | [P2-T3_Fix_double_spawn_race_condition/](P2-T3_Fix_double_spawn_race_condition/) | 2026-03-01 | PASS |
 | P2-T1 | [P2-T1_Replace_broker_flags_with_single_broker_flag/](P2-T1_Replace_broker_flags_with_single_broker_flag/) | 2026-03-01 | PASS |
 | P2-T2 | [P2-T2_Self-healing_stale_socket_and_PID_file_recovery/](P2-T2_Self-healing_stale_socket_and_PID_file_recovery/) | 2026-03-01 | PASS |
 | BUG-T8 | [BUG-T8_Fix_broker_proxy_bridge_exits_after_first_write_due_to_BaseProtocol_missing_drain_helper/](BUG-T8_Fix_broker_proxy_bridge_exits_after_first_write_due_to_BaseProtocol_missing_drain_helper/) | 2026-03-01 | PASS |
@@ -173,6 +174,7 @@
 
 | File | Description |
 |------|-------------|
+| [REVIEW_P2-T3_spawn_lock.md](_Historical/REVIEW_P2-T3_spawn_lock.md) | Review report for P2-T3 |
 | [REVIEW_P2-T1_broker_flag.md](_Historical/REVIEW_P2-T1_broker_flag.md) | Review report for P2-T1 |
 | [REVIEW_P2-T2_stale_socket_recovery.md](_Historical/REVIEW_P2-T2_stale_socket_recovery.md) | Review report for P2-T2 |
 | [Workplan_0.4.0.md](_Historical/Workplan_0.4.0.md) | Archived workplan snapshot for release 0.4.0 |
@@ -294,6 +296,7 @@
 
 | Date | Task ID | Action |
 |------|---------|--------|
+| 2026-03-01 | P2-T3 | Archived Fix double-spawn race condition when MCP client toggles rapidly (PASS) |
 | 2026-03-01 | P2-T1 | Archived Replace --broker-spawn/--broker-connect with single --broker flag (PASS) |
 | 2026-03-01 | P2-T2 | Archived Self-healing stale socket and PID file recovery (PASS) |
 | 2026-03-01 | WORKPLAN | Archived Workplan_0.4.0.md to _Historical and reset SPECS/Workplan.md |

SPECS/ARCHIVE/P2-T3_Fix_double_spawn_race_condition/P2-T3_Fix_double_spawn_race_condition.md

@@ -0,0 +1,64 @@
+# P2-T3: Fix double-spawn race condition when MCP client toggles rapidly
+
+**Task ID:** P2-T3
+**Status:** In Progress
+**Priority:** P1
+**Branch:** feature/P2-T3-spawn-lock
+**Date:** 2026-03-01
+**Depends on:** P2-T2 ✅
+
+## Problem
+
+When an MCP client (e.g. Zed) toggles the connection off/on rapidly, two proxy processes can start simultaneously. Both enter `_spawn_broker_if_needed`, find no broker running, and each spawns a daemon subprocess. Two competing daemons race to bind the Unix socket:
+- One wins and becomes the real broker.
+- The other crashes (EADDRINUSE or similar).
+- The proxy whose daemon lost gets no broker and shows 0 tools.
+
+The root cause is a TOCTOU (time-of-check-time-of-use) race between the liveness check and the `Popen` call.
+
+## Solution
+
+Add a filesystem-level exclusive lock around the spawn decision in `_spawn_broker_if_needed` using `fcntl.flock`:
+
+1. Open (or create) a lock file at `pid_file.with_suffix(".lock")` — e.g. `~/.mcpbridge_wrapper/broker.lock`.
+2. Acquire `LOCK_EX` via `run_in_executor` (avoids blocking the event loop).
+3. Under the lock, re-check liveness (PID file + socket connect) — the double-check pattern.
+4. If broker is now alive → release lock and return (connect path handles the rest).
+5. If still absent → spawn daemon, poll for socket appearance with lock held.
+6. Lock is released when the `with open(...)` block exits (including on crash — OS releases `flock` on fd close).
+
+### Why `flock` on a separate lock file?
+
+- `flock` requires an open fd; using a dedicated `.lock` file avoids interfering with the PID file's content.
+- `flock(LOCK_EX)` is automatically released when the process dies → no stale-lock cleanup needed.
+- The lock is held only during spawn + socket-poll window (bounded by `connect_timeout`, default 10s).
+
+## Deliverables
+
+### `src/mcpbridge_wrapper/broker/proxy.py`
+- Add `import fcntl` at module level.
+- Refactor `_spawn_broker_if_needed` to:
+  - Derive `lock_file = pid_file.with_suffix(".lock")`.
+  - Ensure parent directory exists with `mkdir(parents=True, exist_ok=True)`.
+  - Use `with open(lock_file, "w") as lock_fd:` + `await loop.run_in_executor(None, fcntl.flock, lock_fd.fileno(), fcntl.LOCK_EX)`.
+  - Move all liveness checks and spawn logic inside the `with` block.
+
+### `tests/unit/test_broker_proxy.py`
+- Add `TestBrokerProxySpawnLock` class with:
+  - `test_spawn_lock_file_created_next_to_pid_file` — lock file at expected path.
+  - `test_spawn_acquires_exclusive_lock` — `fcntl.flock` called with `LOCK_EX`.
+  - `test_second_proxy_skips_spawn_after_first_succeeds` — sequential simulation: second call finds socket alive under lock, skips `Popen`.
+
+## Acceptance Criteria
+
+- [ ] Rapid double-toggle produces exactly one broker daemon (second proxy detects liveness under lock and skips spawn)
+- [ ] Lock is released on proxy exit (including crash) — guaranteed by `flock` kernel semantics
+- [ ] All existing broker proxy tests pass
+- [ ] New lock tests pass
+
+## Implementation Notes
+
+- `fcntl` is Unix/macOS only — acceptable since this project targets macOS exclusively.
+- `run_in_executor(None, ...)` uses the default `ThreadPoolExecutor`; the blocking `flock` call does not block the asyncio event loop.
+- The lock file path (`broker.lock`) is derived from `pid_file` path: `pid_file.with_suffix(".lock")`.
+- No changes needed to `BrokerConfig`, `__main__.py`, or README — purely internal to `proxy.py`.

SPECS/ARCHIVE/P2-T3_Fix_double_spawn_race_condition/P2-T3_Validation_Report.md

@@ -0,0 +1,36 @@
+# P2-T3 Validation Report
+
+**Task:** Fix double-spawn race condition when MCP client toggles rapidly
+**Date:** 2026-03-01
+**Verdict:** PASS
+
+## Acceptance Criteria
+
+- [x] Rapid double-toggle produces exactly one broker daemon — second proxy re-checks liveness under lock and skips Popen (verified by `test_second_proxy_skips_spawn_after_first_succeeds`)
+- [x] Lock is released on proxy exit including crash — guaranteed by `flock` kernel semantics + `with open(...)` context manager (verified by `test_lock_released_on_timeout`)
+- [x] All existing broker proxy tests pass (26/26 in `test_broker_proxy.py`)
+- [x] New lock tests pass (4 new tests in `TestBrokerProxySpawnLock`)
+
+## Changes Made
+
+### `src/mcpbridge_wrapper/broker/proxy.py`
+- Added `import fcntl` at module level.
+- `_spawn_broker_if_needed`: wrapped entire body in `with open(lock_file, "w") as lock_fd:` + `await loop.run_in_executor(None, fcntl.flock, lock_fd.fileno(), fcntl.LOCK_EX)`.
+- Lock file path: `pid_file.with_suffix(".lock")` — e.g. `~/.mcpbridge_wrapper/broker.lock`.
+- Added `lock_file.parent.mkdir(parents=True, exist_ok=True)` for first-run safety.
+- Updated docstring to explain the lock semantics.
+
+### `tests/unit/test_broker_proxy.py`
+- Added `TestBrokerProxySpawnLock` class with 4 tests:
+  - `test_spawn_lock_file_created_next_to_pid_file`
+  - `test_spawn_acquires_exclusive_lock`
+  - `test_second_proxy_skips_spawn_after_first_succeeds`
+  - `test_lock_released_on_timeout`
+
+## Quality Gates
+
+| Gate | Result |
+|------|--------|
+| `pytest tests/unit/` | 682 passed, 2 warnings |
+| `ruff check src/` | All checks passed |
+| `pytest --cov` | 91.43% (≥ 90% required) |

SPECS/ARCHIVE/_Historical/REVIEW_P2-T3_spawn_lock.md

@@ -0,0 +1,39 @@
+## REVIEW REPORT — P2-T3: spawn lock
+
+**Scope:** origin/main..HEAD
+**Files:** 2 changed (src/broker/proxy.py, tests/unit/test_broker_proxy.py)
+
+### Summary Verdict
+- [x] Approve with comments
+- [ ] Request changes
+- [ ] Block
+
+### Critical Issues
+
+None.
+
+### Secondary Issues
+
+- **[Low]** The lock is held across `await asyncio.sleep(0.2)` inside the poll loop. Since `flock` was acquired via `run_in_executor` and is held on a thread-pool fd, the poll loop's `await asyncio.sleep` yields the event loop normally — but other async tasks on the same event loop can still run. This is correct behaviour (the lock serialises OS-level processes, not coroutines), but the docstring could make this clearer for future readers.
+
+- **[Low]** `open(lock_file, "w")` truncates the file on each open. Two concurrent openers both succeed (POSIX open is not exclusive by default) and then the flock serialises them. This is intentional and correct, but "w" mode could confuse reviewers who expect exclusive open. A brief inline comment `# "w" mode is fine; flock(LOCK_EX) serialises concurrent openers` would clarify intent.
+
+- **[Nit]** `import subprocess` inside the `with` block is a deferred import for a module already used elsewhere in the project. Not harmful, but minor inconsistency with the rest of `proxy.py` import style.
+
+### Architectural Notes
+
+- The double-check pattern (check liveness → take lock → re-check liveness) correctly eliminates the TOCTOU window. Any process that wins the lock and finds a live broker immediately returns; any that finds it absent spawns exactly one daemon.
+- `flock` semantics on macOS: automatic release on fd close (including SIGKILL) makes the lock stale-proof. No cleanup file is needed.
+- The lock file `broker.lock` is derived from `pid_file.with_suffix(".lock")` — cleanly co-located with existing runtime files and auto-cleaned when `~/.mcpbridge_wrapper/` is wiped.
+
+### Tests
+
+- 4 new tests in `TestBrokerProxySpawnLock`. All pass.
+- All 682 unit tests pass; no regression.
+- Coverage: 91.43% (≥ 90%). Proxy.py coverage slightly lower due to async path branching — acceptable.
+- No integration test for true concurrent OS-level race; the sequential simulation in `test_second_proxy_skips_spawn_after_first_succeeds` is sufficient for verifying the re-check logic.
+
+### Next Steps
+
+- The "w" mode comment nit is optional; no follow-up task needed.
+- No actionable issues → FOLLOW-UP skipped.

SPECS/INPROGRESS/next.md

@@ -1,17 +1,16 @@
 # No Active Task
 
-**Status:** Idle — P2-T1 archived. Select the next task from `SPECS/Workplan.md`.
+**Status:** Idle — P2-T3 archived. Select the next task from `SPECS/Workplan.md`.
 
 ## Recently Archived
 
+- **P2-T3** — Fix double-spawn race condition when MCP client toggles rapidly (2026-03-01, PASS)
 - **P2-T1** — Replace --broker-spawn/--broker-connect with single --broker flag (2026-03-01, PASS)
 - **P2-T2** — Self-healing stale socket and PID file recovery (2026-03-01, PASS)
 - **BUG-T8** — Fix broker proxy bridge exits after first write due to BaseProtocol missing _drain_helper (2026-03-01, PASS)
 - **P1-T3** — Improve MCP settings examples in README to present broker setup first (2026-03-01, PASS)
-- **P1-T2** — Add Xcode 26.4 known issue release-notes link to README (2026-02-28, PASS)
 
 ## Suggested Next Tasks
 
-- **P2-T3** (P1) — Fix double-spawn race condition when MCP client toggles rapidly (depends on P2-T2 ✅)
 - **P2-T4** (P1) — Surface broker unavailability as JSON-RPC error instead of silent timeout
 - **P2-T5** (P2) — Warn or restart daemon when --web-ui requested but running broker lacks it

SPECS/Workplan.md

@@ -86,17 +86,18 @@ Add new tasks using the canonical template in [TASK_TEMPLATE.md](TASK_TEMPLATE.m
   - [x] Daemon removes `broker.sock` on clean exit and on SIGTERM
   - [x] All existing broker tests pass
 
-#### ⬜️ P2-T3: Fix double-spawn race condition when MCP client toggles rapidly
+#### ✅ P2-T3: Fix double-spawn race condition when MCP client toggles rapidly
+- **Status:** ✅ Completed (2026-03-01)
 - **Description:** When an MCP client (e.g. Zed) toggles the connection off/on quickly, two proxy processes start simultaneously. Both check for a running broker, find none, and both spawn a daemon. Two competing daemons fight over the socket path: one wins, the other crashes. The losing proxy's client gets no broker and shows 0 tools. Fix with a filesystem lock (e.g. `fcntl.flock` on the PID file) so only one spawn attempt proceeds at a time; the second waiter detects the winner's daemon and connects.
 - **Priority:** P1
 - **Dependencies:** P2-T2
 - **Parallelizable:** yes
 - **Outputs/Artifacts:**
   - `src/mcpbridge_wrapper/broker/proxy.py` — spawn lock in `_spawn_broker_if_needed`
 - **Acceptance Criteria:**
-  - [ ] Rapid double-toggle produces exactly one broker daemon, both proxy sessions connect successfully
-  - [ ] Lock is released on proxy exit (including crash)
-  - [ ] All existing broker tests pass
+  - [x] Rapid double-toggle produces exactly one broker daemon, both proxy sessions connect successfully
+  - [x] Lock is released on proxy exit (including crash)
+  - [x] All existing broker tests pass
 
 #### ⬜️ P2-T4: Surface broker unavailability as JSON-RPC error instead of silent timeout
 - **Description:** When the proxy cannot connect to the broker (stale socket, spawn failed, daemon crashed mid-session), the client receives no response and eventually times out — showing "0 tools" or a generic connection error with no actionable message. Instead, the proxy should return a JSON-RPC error response (e.g. code `-32001`, message `"Broker unavailable: <reason>"`) so MCP clients can surface a meaningful error to the user rather than silently hanging.

src/mcpbridge_wrapper/broker/proxy.py

@@ -16,6 +16,7 @@
 
 import asyncio
 import contextlib
+import fcntl
 import logging
 import os
 import socket
@@ -111,68 +112,90 @@ async def run(self) -> None:
     async def _spawn_broker_if_needed(self) -> None:
         """Spawn the broker daemon if not already running.
 
-        Checks the PID file for a live process.  If absent or stale, launches
-        the broker daemon in a detached subprocess and polls the socket path
-        until it appears (up to ``connect_timeout`` seconds).
+        Uses a filesystem exclusive lock (``fcntl.flock``) to prevent two
+        proxy processes from spawning competing daemons simultaneously (the
+        double-spawn race condition that occurs when an MCP client toggles
+        rapidly).  The second proxy waiter acquires the lock only after the
+        first has finished spawning, then re-checks liveness and short-circuits
+        to the connect path if the broker appeared while it was waiting.
+
+        The lock is held for the entire spawn + socket-poll window so that
+        concurrent processes queue rather than race.  It is released
+        automatically when the file descriptor is closed — including on process
+        crash — so no stale-lock cleanup is required.
         """
         pid_file = self._config.pid_file
         socket_path = self._config.socket_path
+        lock_file = pid_file.with_suffix(".lock")
 
-        # Check if broker is already running
-        if pid_file.exists():
-            try:
-                pid = int(pid_file.read_text().strip())
-                os.kill(pid, 0)
-                logger.debug("Broker already running (PID %d)", pid)
-                return
-            except (ValueError, ProcessLookupError, PermissionError):
-                logger.debug("Stale PID file; will spawn broker.")
+        # Ensure the config directory exists before opening the lock file.
+        lock_file.parent.mkdir(parents=True, exist_ok=True)
 
-        # Check if socket already exists and is actually alive.
-        # A stale socket file left after a crash passes exists() but refuses connections.
-        if socket_path.exists():
-            try:
-                with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as s:
-                    s.settimeout(1.0)
-                    s.connect(str(socket_path))
-                # Connection succeeded — broker is alive
-                logger.debug("Broker socket present and accepting connections; skipping spawn.")
-                return
-            except OSError:
-                logger.warning(
-                    "Stale socket found (broker not accepting connections); removing stale files."
-                )
-                socket_path.unlink(missing_ok=True)
-                pid_file.unlink(missing_ok=True)
-                # Fall through to spawn
-
-        logger.info("Spawning broker daemon…")
-        import subprocess
-
-        spawn_args = list(self._spawn_args)
-        if "--broker-daemon" not in spawn_args:
-            spawn_args.insert(0, "--broker-daemon")
-
-        subprocess.Popen(
-            [sys.executable, "-m", "mcpbridge_wrapper", *spawn_args],
-            start_new_session=True,
-            stdin=subprocess.DEVNULL,
-            stdout=subprocess.DEVNULL,
-            stderr=subprocess.DEVNULL,
-        )
-
-        # Poll for socket appearance
         loop = asyncio.get_running_loop()
-        deadline = loop.time() + self._connect_timeout
-        while loop.time() < deadline:
+
+        with open(lock_file, "w") as lock_fd:
+            # Acquire exclusive lock in a thread so the event loop stays free.
+            await loop.run_in_executor(None, fcntl.flock, lock_fd.fileno(), fcntl.LOCK_EX)
+
+            # --- critical section: re-check liveness under lock ---
+            # A concurrent proxy may have spawned the daemon while we waited.
+
+            # Check if broker is already running via PID file.
+            if pid_file.exists():
+                try:
+                    pid = int(pid_file.read_text().strip())
+                    os.kill(pid, 0)
+                    logger.debug("Broker already running (PID %d); skipping spawn.", pid)
+                    return
+                except (ValueError, ProcessLookupError, PermissionError):
+                    logger.debug("Stale PID file; will spawn broker.")
+
+            # Check if socket already exists and is actually alive.
+            # A stale socket file left after a crash passes exists() but refuses connections.
             if socket_path.exists():
-                logger.debug("Broker socket appeared.")
-                return
-            await asyncio.sleep(0.2)
+                try:
+                    with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as s:
+                        s.settimeout(1.0)
+                        s.connect(str(socket_path))
+                    # Connection succeeded — broker is alive.
+                    logger.debug("Broker socket present and accepting connections; skipping spawn.")
+                    return
+                except OSError:
+                    logger.warning(
+                        "Stale socket found (broker not accepting connections);"
+                        " removing stale files."
+                    )
+                    socket_path.unlink(missing_ok=True)
+                    pid_file.unlink(missing_ok=True)
+                    # Fall through to spawn.
+
+            logger.info("Spawning broker daemon…")
+            import subprocess
+
+            spawn_args = list(self._spawn_args)
+            if "--broker-daemon" not in spawn_args:
+                spawn_args.insert(0, "--broker-daemon")
+
+            subprocess.Popen(
+                [sys.executable, "-m", "mcpbridge_wrapper", *spawn_args],
+                start_new_session=True,
+                stdin=subprocess.DEVNULL,
+                stdout=subprocess.DEVNULL,
+                stderr=subprocess.DEVNULL,
+            )
+
+            # Poll for socket appearance while holding the lock so concurrent
+            # proxies wait and then find the broker alive on their re-check.
+            deadline = loop.time() + self._connect_timeout
+            while loop.time() < deadline:
+                if socket_path.exists():
+                    logger.debug("Broker socket appeared.")
+                    return
+                await asyncio.sleep(0.2)
 
-        raise TimeoutError(
-            f"Broker socket did not appear within {self._connect_timeout}s at {socket_path}"
-        )
+            raise TimeoutError(
+                f"Broker socket did not appear within {self._connect_timeout}s at {socket_path}"
+            )
 
     async def _connect_with_timeout(
         self,