FU-P13-T2-2: Move PID file write to after successful upstream launch

SPECS/ARCHIVE/FU-P13-T2-2_Move_PID_file_write_to_after_successful_upstream_launch/FU-P13-T2-2_Move_PID_file_write_to_after_successful_upstream_launch.md

@@ -0,0 +1,70 @@
+# PRD: FU-P13-T2-2 — Move PID file write to after successful upstream launch
+
+**Task ID:** FU-P13-T2-2  
+**Priority:** P3  
+**Phase:** Phase 13: Persistent Broker & Shared Xcode Session  
+**Dependencies:** P13-T2  
+**Status:** Planned
+
+## Objective
+
+Eliminate a startup race in `BrokerDaemon.start()` where the PID file can be written before
+`_launch_upstream()` succeeds. If startup fails between these operations, the wrapper can leave a
+live-process PID lock that blocks future starts. The fix is to move PID-file persistence so it
+runs only after upstream launch has completed successfully.
+
+## Success Criteria
+
+- PID file is created only after `_launch_upstream()` succeeds.
+- Failure paths during upstream launch do not leave a PID file behind.
+- Existing stale-lock behavior and cleanup flows remain unchanged.
+- Current broker daemon tests remain green, and coverage for this behavior is explicit.
+
+## Acceptance Tests
+
+1. Existing test coverage for successful startup still passes.
+2. Existing test coverage for startup failures still passes.
+3. Add or update a unit test that proves no PID file is written when `_launch_upstream()` raises.
+4. Verify lock/PID cleanup behavior remains correct after stop or failed start.
+
+## Test-First Plan
+
+1. Inspect `tests/unit/test_broker_daemon.py` around startup/lifecycle tests and identify current
+   assertions for PID file timing.
+2. Add/adjust a failing test asserting PID file write happens after successful upstream launch and
+   does not happen on launch failure.
+3. Run focused tests for broker daemon to confirm the new test fails before implementation.
+
+## Implementation Plan
+
+### Phase 1: Validate current startup order
+- **Inputs:** `src/mcpbridge_wrapper/broker/daemon.py`, existing broker daemon tests.
+- **Outputs:** Confirmed execution order and concrete edit target in `start()`.
+- **Verification:** Readable call order documented in PR notes.
+
+### Phase 2: Move PID write after upstream launch
+- **Inputs:** `BrokerDaemon.start()`.
+- **Outputs:** Refactored startup sequence with unchanged external behavior.
+- **Verification:** New/updated test passes and no regressions in daemon lifecycle tests.
+
+### Phase 3: Run required quality gates + report
+- **Inputs:** Updated source and tests.
+- **Outputs:** Passing `pytest`, `ruff check src/`, `mypy src/`, and `pytest --cov` results plus
+  `SPECS/INPROGRESS/FU-P13-T2-2_Validation_Report.md`.
+- **Verification:** Command outputs captured with PASS/FAIL summary and coverage percentage.
+
+## Risks and Mitigations
+
+- **Risk:** Changing startup ordering could break assumptions in lock handling.
+  **Mitigation:** Keep lock acquisition timing unchanged and rely on existing stale-lock tests.
+- **Risk:** Test fixtures may mock PID writes loosely.
+  **Mitigation:** Add explicit assertions around PID path existence and write invocation order.
+
+## Notes
+
+- If behavior changes are not user-visible, no documentation updates are required.
+- If startup semantics or troubleshooting guidance changes, update `docs/troubleshooting.md`.
+
+---
+**Archived:** 2026-02-18
+**Verdict:** PASS

SPECS/ARCHIVE/FU-P13-T2-2_Move_PID_file_write_to_after_successful_upstream_launch/FU-P13-T2-2_Validation_Report.md

@@ -0,0 +1,44 @@
+# Validation Report — FU-P13-T2-2
+
+**Task:** FU-P13-T2-2 — Move PID file write to after successful upstream launch  
+**Date:** 2026-02-18  
+**Verdict:** PASS
+
+## Scope
+
+- Updated startup ordering in `BrokerDaemon.start()` so PID file is written only
+  after `_launch_upstream()` succeeds.
+- Added regression test for launch-failure path to ensure no PID lock file is
+  created when upstream start fails.
+
+## Test-First Evidence
+
+1. Added test: `test_start_does_not_write_pid_file_when_launch_fails`.
+2. Ran before implementation:
+   - `pytest tests/unit/test_broker_daemon.py -k "start_does_not_write_pid_file_when_launch_fails" -q`
+   - Result: **FAIL** (PID file existed unexpectedly).
+3. Implemented startup-order fix.
+4. Re-ran focused test and broker daemon suite:
+   - Focused test: **PASS**
+   - `pytest tests/unit/test_broker_daemon.py -q`: **28 passed**.
+
+## Required Quality Gates
+
+- `pytest -q`  
+  Result: **PASS** (`580 passed, 5 skipped, 2 warnings`)
+- `ruff check src/`  
+  Result: **PASS** (`All checks passed!`)
+- `mypy src/`  
+  Result: **PASS** (`Success: no issues found in 18 source files`)
+- `pytest --cov=src/mcpbridge_wrapper --cov-report=term-missing`  
+  Result: **PASS** (`580 passed, 5 skipped`, total coverage **92.25%**, threshold 90%)
+
+## Acceptance Criteria Status
+
+- [x] PID file is written only after `_launch_upstream()` succeeds.
+- [x] Stale-lock/startup tests continue to pass.
+
+## Notes
+
+- Two existing third-party deprecation warnings were observed during full test
+  runs (`websockets`/`uvicorn`) and are unrelated to this task.

SPECS/ARCHIVE/INDEX.md

@@ -1,6 +1,6 @@
 # mcpbridge-wrapper Tasks Archive
 
-**Last Updated:** 2026-02-18 (FU-P13-T2-1)
+**Last Updated:** 2026-02-18 (REVIEW_FU-P13-T2-2)
 
 ## Archived Tasks
 
@@ -113,6 +113,7 @@
 | FU-P13-T4-1 | [FU-P13-T4-1_Fix_asyncio_get_event_loop_deprecation_in_BrokerProxy/](FU-P13-T4-1_Fix_asyncio_get_event_loop_deprecation_in_BrokerProxy/) | 2026-02-18 | PASS |
 | FU-P13-T4-2 | [FU-P13-T4-2_Implement_or_remove_reconnect_parameter_in_BrokerProxy/](FU-P13-T4-2_Implement_or_remove_reconnect_parameter_in_BrokerProxy/) | 2026-02-18 | PASS |
 | FU-P13-T2-1 | [FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait/](FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait/) | 2026-02-18 | PASS |
+| FU-P13-T2-2 | [FU-P13-T2-2_Move_PID_file_write_to_after_successful_upstream_launch/](FU-P13-T2-2_Move_PID_file_write_to_after_successful_upstream_launch/) | 2026-02-18 | PASS |
 
 ## Historical Artifacts
 
@@ -189,6 +190,7 @@
 | [REVIEW_FU-P13-T4-1_broker_proxy_loop.md](_Historical/REVIEW_FU-P13-T4-1_broker_proxy_loop.md) | Review report for FU-P13-T4-1 |
 | [REVIEW_FU-P13-T4-2_broker_proxy_reconnect.md](_Historical/REVIEW_FU-P13-T4-2_broker_proxy_reconnect.md) | Review report for FU-P13-T4-2 |
 | [REVIEW_FU-P13-T2-1_event_wait_shutdown.md](_Historical/REVIEW_FU-P13-T2-1_event_wait_shutdown.md) | Review report for FU-P13-T2-1 |
+| [REVIEW_FU-P13-T2-2_pid_write_order.md](_Historical/REVIEW_FU-P13-T2-2_pid_write_order.md) | Review report for FU-P13-T2-2 |
 
 ## Archive Log
 
@@ -332,3 +334,5 @@
 | 2026-02-18 | FU-P13-T4-2 | Archived REVIEW_FU-P13-T4-2_broker_proxy_reconnect report |
 | 2026-02-18 | FU-P13-T2-1 | Archived Replace_run_forever_polling_with_Event_wait (PASS) |
 | 2026-02-18 | FU-P13-T2-1 | Archived REVIEW_FU-P13-T2-1_event_wait_shutdown report |
+| 2026-02-18 | FU-P13-T2-2 | Archived Move_PID_file_write_to_after_successful_upstream_launch (PASS) |
+| 2026-02-18 | FU-P13-T2-2 | Archived REVIEW_FU-P13-T2-2_pid_write_order report |

SPECS/ARCHIVE/_Historical/REVIEW_FU-P13-T2-2_pid_write_order.md

@@ -0,0 +1,33 @@
+## REVIEW REPORT — FU-P13-T2-2 pid write order
+
+**Scope:** origin/main..HEAD  
+**Files:** 7
+
+### Summary Verdict
+- [x] Approve
+- [ ] Approve with comments
+- [ ] Request changes
+- [ ] Block
+
+### Critical Issues
+- None.
+
+### Secondary Issues
+- None.
+
+### Architectural Notes
+- The startup sequence in `BrokerDaemon.start()` now aligns with lock-file safety expectations:
+  lock check → upstream launch → PID write. This removes the stale-lock window called out in the
+  follow-up without changing lifecycle state transitions.
+
+### Tests
+- Added regression test `test_start_does_not_write_pid_file_when_launch_fails`.
+- Required gates executed and passing:
+  - `pytest -q`
+  - `ruff check src/`
+  - `mypy src/`
+  - `pytest --cov=src/mcpbridge_wrapper --cov-report=term-missing`
+- Coverage remains above threshold (92.25% ≥ 90%).
+
+### Next Steps
+- No actionable findings. FOLLOW-UP is skipped for this review.

SPECS/INPROGRESS/next.md

@@ -2,15 +2,15 @@
 
 ## Recently Archived
 
+- 2026-02-18 — FU-P13-T2-2: Move PID file write to after successful upstream launch (PASS)
 - 2026-02-18 — FU-P13-T2-1: Replace run_forever() polling loop with asyncio.Event-based wait (PASS)
 - 2026-02-18 — FU-P13-T4-2: Implement or remove reconnect parameter in BrokerProxy (PASS)
 - 2026-02-18 — FU-P13-T4-1: Fix asyncio.get_event_loop() deprecation in BrokerProxy (PASS)
 - 2026-02-18 — P13-T6: Document broker mode configuration, migration, and rollback (PASS)
 - 2026-02-18 — P13-T5: Validate prompt reduction and multi-client stability (PARTIAL)
-- 2026-02-18 — P13-T4: Add stdio proxy mode for compatibility with existing MCP clients (PASS)
 
 ## Suggested Next Tasks
 
 - P13-T5 follow-up — Complete interactive prompt verification in a desktop session (P1)
-- FU-P13-T2-2 — Move PID file write to after successful upstream launch (P3)
 - FU-BUG-T7-1 — Cap `pending_methods` map to guard against unbounded growth (P3)
+- FU-P12-T1-1 — Remove or document `MCPInitializeParams` in schemas (P3)

SPECS/Workplan.md

@@ -2020,15 +2020,16 @@ Phase 9 Follow-up Backlog
 
 ---
 
-#### FU-P13-T2-2: Move PID file write to after successful upstream launch
+#### ✅ FU-P13-T2-2: Move PID file write to after successful upstream launch
+- **Status:** ✅ Completed (2026-02-18)
 - **Type:** Robustness
 - **Priority:** P3
 - **Discovered:** 2026-02-17 (REVIEW_P13-T2)
 - **Component:** BrokerDaemon.start()
 - **Description:** PID file is currently written before upstream subprocess is launched. A crash between write and launch leaves a live-PID lock that blocks future starts until the owning process dies. Move the write to after successful launch.
 - **Acceptance Criteria:**
-  - [ ] PID file is written only after `_launch_upstream()` succeeds
-  - [ ] Stale-lock tests continue to pass
+  - [x] PID file is written only after `_launch_upstream()` succeeds
+  - [x] Stale-lock tests continue to pass
 
 ---
 

src/mcpbridge_wrapper/broker/daemon.py

@@ -95,7 +95,7 @@ def status(self) -> dict[str, Any]:
         }
 
     async def start(self) -> None:
-        """Start the broker: validate lock, write PID file, launch upstream.
+        """Start the broker: validate lock, launch upstream, then write PID file.
 
         Raises:
             RuntimeError: If another broker instance is already running (live PID found).
@@ -105,12 +105,13 @@ async def start(self) -> None:
         # Stale-lock / duplicate-instance check
         self._check_and_clear_stale_lock()
 
-        # Write own PID
+        # Launch upstream subprocess
+        await self._launch_upstream()
+
+        # Persist PID only after upstream launch succeeds.
         self._config.pid_file.write_text(str(os.getpid()))
         logger.debug("PID file written: %s", self._config.pid_file)
 
-        # Launch upstream subprocess
-        await self._launch_upstream()
         self._state = BrokerState.READY
         logger.info(
             "Broker READY (upstream PID %s)",

tests/unit/test_broker_daemon.py

@@ -117,6 +117,23 @@ async def test_start_writes_pid_file(self, tmp_path: Path) -> None:
         if daemon._read_task and not daemon._read_task.done():
             daemon._read_task.cancel()
 
+    @pytest.mark.asyncio
+    async def test_start_does_not_write_pid_file_when_launch_fails(
+        self,
+        tmp_path: Path,
+    ) -> None:
+        cfg = _make_config(tmp_path)
+        daemon = BrokerDaemon(cfg)
+
+        with patch.object(
+            daemon,
+            "_launch_upstream",
+            new=AsyncMock(side_effect=OSError("launch failed")),
+        ), pytest.raises(OSError, match="launch failed"):
+            await daemon.start()
+
+        assert not cfg.pid_file.exists()
+
     @pytest.mark.asyncio
     async def test_start_creates_data_dir(self, tmp_path: Path) -> None:
         nested = tmp_path / "a" / "b" / "c"