|
| 1 | +# P2-T2: Self-healing stale socket and PID file recovery |
| 2 | + |
| 3 | +**Status:** In Progress |
| 4 | +**Priority:** P0 |
| 5 | +**Branch:** feature/P2-T2-stale-socket-recovery |
| 6 | +**Created:** 2026-03-01 |
| 7 | + |
| 8 | +--- |
| 9 | + |
| 10 | +## Problem Statement |
| 11 | + |
| 12 | +When the broker daemon crashes or is killed, it leaves `broker.sock` and `broker.pid` on disk. The proxy's `_spawn_broker_if_needed` checks `socket_path.exists()` and skips spawning if the socket file is present — even if no process is listening. This silently blocks all future broker mode sessions until the user manually deletes the files. |
| 13 | + |
| 14 | +--- |
| 15 | + |
| 16 | +## Root Cause |
| 17 | + |
| 18 | +In `proxy.py` → `_spawn_broker_if_needed`, line 131-133: |
| 19 | + |
| 20 | +```python |
| 21 | +# Check if socket already exists (race condition: broker started without PID file yet) |
| 22 | +if socket_path.exists(): |
| 23 | + logger.debug("Broker socket already present; skipping spawn.") |
| 24 | + return |
| 25 | +``` |
| 26 | + |
| 27 | +Existence check (`Path.exists()`) does not verify whether anything is actually listening on the socket. A stale socket file left after a crash passes the existence check and prevents spawn. |
| 28 | + |
| 29 | +--- |
| 30 | + |
| 31 | +## Solution |
| 32 | + |
| 33 | +### 1. `proxy.py` — Liveness check in `_spawn_broker_if_needed` |
| 34 | + |
| 35 | +Replace the plain existence check with a socket connect attempt: |
| 36 | + |
| 37 | +```python |
| 38 | +if socket_path.exists(): |
| 39 | + import socket as _socket |
| 40 | + try: |
| 41 | + with _socket.socket(_socket.AF_UNIX, _socket.SOCK_STREAM) as s: |
| 42 | + s.settimeout(1.0) |
| 43 | + s.connect(str(socket_path)) |
| 44 | + # Connection succeeded → broker is alive |
| 45 | + logger.debug("Broker socket present and accepting connections; skipping spawn.") |
| 46 | + return |
| 47 | + except ConnectionRefusedError: |
| 48 | + # Stale socket — broker is not listening |
| 49 | + logger.warning( |
| 50 | + "Stale socket found (broker not accepting connections); removing stale files." |
| 51 | + ) |
| 52 | + socket_path.unlink(missing_ok=True) |
| 53 | + pid_file.unlink(missing_ok=True) |
| 54 | + # Fall through to spawn |
| 55 | +``` |
| 56 | + |
| 57 | +This ensures: |
| 58 | +- If broker is alive (socket accepts connections): skip spawn (no change in behaviour) |
| 59 | +- If broker is dead (socket refuses connections): remove stale files and proceed with spawn |
| 60 | +- `FileNotFoundError` and other OS errors during connect are suppressed — treated as "not alive" (also falls through to spawn path) |
| 61 | + |
| 62 | +### 2. `daemon.py` — `atexit` cleanup on daemon exit |
| 63 | + |
| 64 | +The daemon already removes files via `_cleanup_files()` in `stop()`, and `stop()` is called by the SIGTERM/SIGINT handlers in `run_forever()`. However, if the Python interpreter exits abnormally (e.g., unhandled exception, explicit `sys.exit()`), cleanup may be skipped. |
| 65 | + |
| 66 | +Add an `atexit` registration in `start()`: |
| 67 | + |
| 68 | +```python |
| 69 | +import atexit |
| 70 | +atexit.register(self._cleanup_files) |
| 71 | +``` |
| 72 | + |
| 73 | +This ensures `_cleanup_files()` runs even for abnormal exits (excluding SIGKILL, which cannot be intercepted). |
| 74 | + |
| 75 | +--- |
| 76 | + |
| 77 | +## Files to Change |
| 78 | + |
| 79 | +| File | Change | |
| 80 | +|------|--------| |
| 81 | +| `src/mcpbridge_wrapper/broker/proxy.py` | Replace existence-only socket check with connect-based liveness check | |
| 82 | +| `src/mcpbridge_wrapper/broker/daemon.py` | Register `atexit` cleanup in `start()` | |
| 83 | + |
| 84 | +--- |
| 85 | + |
| 86 | +## Tests to Add |
| 87 | + |
| 88 | +In `tests/unit/test_broker_proxy.py` — new class `TestBrokerProxyStaleSocket`: |
| 89 | + |
| 90 | +1. **`test_stale_socket_triggers_spawn`** — socket file exists but connect raises `ConnectionRefusedError`; verify `Popen` is called and stale files are removed. |
| 91 | +2. **`test_live_socket_skips_spawn`** — socket file exists and connect succeeds; verify `Popen` is NOT called. |
| 92 | +3. **`test_stale_socket_with_stale_pid_file_triggers_spawn`** — both files exist, connect raises `ConnectionRefusedError`; verify both files are removed and spawn proceeds. |
| 93 | + |
| 94 | +In `tests/unit/test_broker_daemon.py` — new class `TestBrokerDaemonAtExit`: |
| 95 | + |
| 96 | +4. **`test_atexit_registered_after_start`** — after `daemon.start()`, `atexit` registry includes `_cleanup_files`. |
| 97 | + |
| 98 | +--- |
| 99 | + |
| 100 | +## Acceptance Criteria |
| 101 | + |
| 102 | +- [ ] After broker crash, next `--broker-spawn` session auto-recovers without manual file removal |
| 103 | +- [ ] Liveness check uses `connect()` not `exists()` |
| 104 | +- [ ] Daemon registers `atexit` cleanup on `start()` |
| 105 | +- [ ] All existing broker tests pass |
| 106 | +- [ ] New tests cover the stale-socket scenario and atexit registration |
| 107 | +- [ ] `ruff check src/` passes |
| 108 | +- [ ] `mypy src/` passes (if configured) |
| 109 | +- [ ] Coverage ≥ 90% |
| 110 | + |
| 111 | +--- |
| 112 | + |
| 113 | +## Dependencies |
| 114 | + |
| 115 | +None — can be implemented independently. |
0 commit comments