diff --git a/SPECS/ARCHIVE/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait.md b/SPECS/ARCHIVE/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait.md new file mode 100644 index 00000000..7b10f167 --- /dev/null +++ b/SPECS/ARCHIVE/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait.md @@ -0,0 +1,78 @@ +# PRD: FU-P13-T2-1 — Replace run_forever() polling loop with asyncio.Event-based wait + +**Created:** 2026-02-18 +**Priority:** P3 +**Branch:** `codex/feature/FU-P13-T2-1-event-wait-shutdown` +**Status:** PLAN + +--- + +## 1. Problem Statement + +`BrokerDaemon.run_forever()` currently waits for shutdown by polling `asyncio.sleep(0.1)`. This adds up to 100ms latency to stop handling and introduces unnecessary wakeups. + +--- + +## 2. Scope + +### In Scope +- Replace polling-based wait in `run_forever()` with an `asyncio.Event`-driven wait. +- Preserve startup/shutdown semantics and existing lifecycle behavior. +- Keep current external API and tests intact. + +### Out of Scope +- Refactoring broker daemon startup/lock management. +- Changes to proxy or transport behavior. +- New CLI options. + +--- + +## 3. Deliverables + +1. `src/mcpbridge_wrapper/broker/daemon.py` +- Introduce event-based shutdown signaling for `run_forever()`. +- Ensure `stop()` triggers the event and remains safe when called multiple times. + +2. `tests/unit/test_broker_daemon.py` +- Keep existing behavior checks passing. +- Add or adjust assertions if needed to validate event-wait shutdown behavior. + +3. `SPECS/INPROGRESS/FU-P13-T2-1_Validation_Report.md` +- Record quality-gate results and acceptance evidence. + +--- + +## 4. Acceptance Criteria + +- [ ] `run_forever()` responds to stop signal within one event loop tick. +- [ ] Existing `test_run_forever_starts_and_stops` passes without behavioral regressions. +- [ ] Full quality gates pass: + - `pytest` + - `ruff check src/` + - `mypy src/` + - `pytest --cov` (coverage >= 90%) + +--- + +## 5. Dependencies + +- P13-T2 ✅ + +--- + +## 6. Risks and Mitigations + +- **Risk:** Event lifecycle may leak across multiple broker runs. + - **Mitigation:** Reinitialize/reset the event at broker startup boundaries and validate with unit tests. + +--- + +## 7. Validation Plan + +1. Implement event-based wait and stop signaling. +2. Run targeted daemon unit tests. +3. Run required quality gates and record outcomes in the validation report. + +--- +**Archived:** 2026-02-18 +**Verdict:** PASS diff --git a/SPECS/ARCHIVE/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait/FU-P13-T2-1_Validation_Report.md b/SPECS/ARCHIVE/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait/FU-P13-T2-1_Validation_Report.md new file mode 100644 index 00000000..766d31c2 --- /dev/null +++ b/SPECS/ARCHIVE/FU-P13-T2-1_Replace_run_forever_polling_with_Event_wait/FU-P13-T2-1_Validation_Report.md @@ -0,0 +1,76 @@ +# Validation Report: FU-P13-T2-1 + +**Task:** FU-P13-T2-1 — Replace run_forever() polling loop with asyncio.Event-based wait +**Date:** 2026-02-18 +**Branch:** `codex/feature/FU-P13-T2-1-event-wait-shutdown` + +## Scope validated + +- Replaced `run_forever()` fixed-interval polling with event-based waiting. +- Added explicit stop-completion signaling so concurrent stop waiters and `run_forever()` unblock only after shutdown completes. +- Added a unit test confirming `run_forever()` no longer relies on a fixed `0.1s` polling sleep. + +## Quality gates + +### 1) Targeted task tests + +Command: + +```bash +pytest tests/unit/test_broker_daemon.py -q +``` + +Result: **PASS** (`27 passed`) + +### 2) Full test suite + +Command: + +```bash +pytest +``` + +Result: **PASS** (`579 passed, 5 skipped`) + +### 3) Lint + +Command: + +```bash +ruff check src/ +``` + +Result: **PASS** (`All checks passed!`) + +### 4) Type checks + +Command: + +```bash +mypy src/ +``` + +Result: **PASS** (`Success: no issues found in 18 source files`) + +### 5) Coverage + +Command: + +```bash +pytest --cov +``` + +Result: **PASS** (`Total coverage: 92.25%`, threshold: `>= 90%`) + +## Acceptance criteria evidence + +- [x] `run_forever()` responds to stop signal within one event loop tick. + - Evidence: `run_forever()` now waits on `self._stop_event.wait()` instead of sleep polling and exits after `self._stopped_event.wait()` completion. +- [x] Existing `test_run_forever_starts_and_stops` passes without behavioral regressions. + - Evidence: `pytest tests/unit/test_broker_daemon.py -q` includes this test and passed. +- [x] Full quality gates pass. + - Evidence: `pytest`, `ruff check src/`, `mypy src/`, and `pytest --cov` all passed; coverage `92.25%`. + +## Notes + +- Existing `websockets` deprecation warnings in Web UI tests remain unchanged and are unrelated to this task. diff --git a/SPECS/ARCHIVE/INDEX.md b/SPECS/ARCHIVE/INDEX.md index 7c863fcc..a776c092 100644 --- a/SPECS/ARCHIVE/INDEX.md +++ b/SPECS/ARCHIVE/INDEX.md @@ -1,6 +1,6 @@ # mcpbridge-wrapper Tasks Archive -**Last Updated:** 2026-02-18 (FU-P13-T4-2) +**Last Updated:** 2026-02-18 (FU-P13-T2-1) ## Archived Tasks @@ -112,6 +112,7 @@ | P13-T6 | [P13-T6_Document_broker_mode_configuration_migration_and_rollback/](P13-T6_Document_broker_mode_configuration_migration_and_rollback/) | 2026-02-18 | PASS | | 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 | ## Historical Artifacts @@ -187,6 +188,7 @@ | [REVIEW_P13-T6_broker_mode_configuration_migration.md](_Historical/REVIEW_P13-T6_broker_mode_configuration_migration.md) | Review report for P13-T6 | | [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 | ## Archive Log @@ -328,3 +330,5 @@ | 2026-02-18 | FU-P13-T4-1 | Archived REVIEW_FU-P13-T4-1_broker_proxy_loop report | | 2026-02-18 | FU-P13-T4-2 | Archived Implement_or_remove_reconnect_parameter_in_BrokerProxy (PASS) | | 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 | diff --git a/SPECS/ARCHIVE/_Historical/REVIEW_FU-P13-T2-1_event_wait_shutdown.md b/SPECS/ARCHIVE/_Historical/REVIEW_FU-P13-T2-1_event_wait_shutdown.md new file mode 100644 index 00000000..0981a992 --- /dev/null +++ b/SPECS/ARCHIVE/_Historical/REVIEW_FU-P13-T2-1_event_wait_shutdown.md @@ -0,0 +1,31 @@ +## REVIEW REPORT — FU-P13-T2-1 event wait shutdown + +**Scope:** origin/main..HEAD +**Files:** 7 + +### Summary Verdict +- [x] Approve +- [ ] Approve with comments +- [ ] Request changes +- [ ] Block + +### Critical Issues +- None. + +### Secondary Issues +- None. + +### Architectural Notes +- Replacing polling with event waits improves shutdown responsiveness and removes unnecessary wakeups in `run_forever()`. +- Adding `_stopped_event` ensures concurrent `stop()` callers and `run_forever()` wait for full shutdown completion. + +### Tests +- Required quality gates were run during EXECUTE and all passed: + - `pytest` + - `ruff check src/` + - `mypy src/` + - `pytest --cov` (92.25%, threshold 90%) + +### Next Steps +- No actionable review findings. +- FOLLOW-UP step is skipped per FLOW rules. diff --git a/SPECS/INPROGRESS/next.md b/SPECS/INPROGRESS/next.md index eae00e86..21053f35 100644 --- a/SPECS/INPROGRESS/next.md +++ b/SPECS/INPROGRESS/next.md @@ -2,15 +2,15 @@ ## Recently Archived +- 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) -- 2026-02-18 — P13-T3: Implement multi-client transport and JSON-RPC multiplexing (PASS) ## Suggested Next Tasks - P13-T5 follow-up — Complete interactive prompt verification in a desktop session (P1) -- FU-P13-T2-1 — Replace run_forever() polling loop with asyncio.Event-based wait (P3) +- 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) diff --git a/SPECS/Workplan.md b/SPECS/Workplan.md index 481a3e8b..83753a03 100644 --- a/SPECS/Workplan.md +++ b/SPECS/Workplan.md @@ -2007,15 +2007,16 @@ Phase 9 Follow-up Backlog --- -#### FU-P13-T2-1: Replace run_forever() polling loop with asyncio.Event-based wait +#### ✅ FU-P13-T2-1: Replace run_forever() polling loop with asyncio.Event-based wait +- **Status:** ✅ Completed (2026-02-18) - **Type:** Enhancement - **Priority:** P3 - **Discovered:** 2026-02-17 (REVIEW_P13-T2) - **Component:** BrokerDaemon.run_forever() - **Description:** Current implementation uses `asyncio.sleep(0.1)` polling which introduces up to 100ms stop-signal latency. Replace with `asyncio.Event.wait()` for idiomatic zero-latency shutdown. - **Acceptance Criteria:** - - [ ] `run_forever()` responds to stop signal within one event loop tick - - [ ] Existing `test_run_forever_starts_and_stops` passes without change + - [x] `run_forever()` responds to stop signal within one event loop tick + - [x] Existing `test_run_forever_starts_and_stops` passes without change --- diff --git a/src/mcpbridge_wrapper/broker/daemon.py b/src/mcpbridge_wrapper/broker/daemon.py index 9c8b443a..a5842f94 100644 --- a/src/mcpbridge_wrapper/broker/daemon.py +++ b/src/mcpbridge_wrapper/broker/daemon.py @@ -71,6 +71,7 @@ def __init__( self._read_task: asyncio.Task[None] | None = None self._reconnect_attempt: int = 0 self._stop_event: asyncio.Event = asyncio.Event() + self._stopped_event: asyncio.Event = asyncio.Event() # ------------------------------------------------------------------ # Public API @@ -118,6 +119,7 @@ async def start(self) -> None: # Background reader self._stop_event.clear() + self._stopped_event.clear() self._read_task = asyncio.ensure_future(self._read_upstream_loop()) # Start transport (Unix socket server) if provided @@ -130,7 +132,10 @@ async def stop(self) -> None: Drains in-flight requests up to ``config.graceful_shutdown_timeout`` seconds, then terminates the upstream subprocess and removes socket/PID. """ - if self._state in (BrokerState.STOPPED, BrokerState.STOPPING): + if self._state == BrokerState.STOPPED: + return + if self._state == BrokerState.STOPPING: + await self._stopped_event.wait() return self._state = BrokerState.STOPPING @@ -153,25 +158,27 @@ async def stop(self) -> None: timeout=self._config.graceful_shutdown_timeout, ) - # Terminate upstream - if self._upstream is not None and self._upstream.returncode is None: - with contextlib.suppress(Exception): - if self._upstream.stdin is not None: - self._upstream.stdin.close() - try: - await asyncio.wait_for( - self._upstream.wait(), - timeout=self._config.graceful_shutdown_timeout, - ) - except asyncio.TimeoutError: - logger.warning("Upstream did not exit cleanly; killing.") - self._upstream.kill() - await self._upstream.wait() - - # Cleanup files - self._cleanup_files() - self._state = BrokerState.STOPPED - logger.info("Broker STOPPED") + try: + # Terminate upstream + if self._upstream is not None and self._upstream.returncode is None: + with contextlib.suppress(Exception): + if self._upstream.stdin is not None: + self._upstream.stdin.close() + try: + await asyncio.wait_for( + self._upstream.wait(), + timeout=self._config.graceful_shutdown_timeout, + ) + except asyncio.TimeoutError: + logger.warning("Upstream did not exit cleanly; killing.") + self._upstream.kill() + await self._upstream.wait() + finally: + # Always mark shutdown complete so run_forever/stop waiters unblock. + self._cleanup_files() + self._state = BrokerState.STOPPED + self._stopped_event.set() + logger.info("Broker STOPPED") async def run_forever(self) -> None: """Start and block until a shutdown signal is received.""" @@ -194,13 +201,9 @@ def _sync_signal_handler() -> None: with contextlib.suppress(NotImplementedError, RuntimeError): loop.add_signal_handler(sig, _sync_signal_handler) - # Wait until stopped - while self._state not in (BrokerState.STOPPED, BrokerState.STOPPING): - await asyncio.sleep(0.1) - - # Ensure stop completes if STOPPING - if self._state == BrokerState.STOPPING: - await self.stop() + # Wait for shutdown to be requested and fully completed. + await self._stop_event.wait() + await self._stopped_event.wait() # ------------------------------------------------------------------ # Internal helpers diff --git a/tests/unit/test_broker_daemon.py b/tests/unit/test_broker_daemon.py index 6f2450a1..eb8dd8e2 100644 --- a/tests/unit/test_broker_daemon.py +++ b/tests/unit/test_broker_daemon.py @@ -536,6 +536,43 @@ async def _do_stop() -> None: assert daemon.state == BrokerState.STOPPED + @pytest.mark.asyncio + async def test_run_forever_does_not_poll_with_fixed_sleep(self, tmp_path: Path) -> None: + """run_forever waits on events and does not use fixed-interval polling.""" + cfg = _make_config(tmp_path) + daemon = BrokerDaemon(cfg) + + async def _block(*a, **kw) -> bytes: # type: ignore[no-untyped-def] + await daemon._stop_event.wait() + return b"" + + proc = _make_mock_process() + proc.stdout.readline = _block + + sleep_calls: list[float] = [] + original_sleep = asyncio.sleep + + async def _tracked_sleep(delay: float) -> None: + sleep_calls.append(delay) + await original_sleep(delay) + + async def _do_stop() -> None: + await original_sleep(0.05) + await daemon.stop() + + with patch( + "mcpbridge_wrapper.broker.daemon.asyncio.create_subprocess_exec", + new=AsyncMock(return_value=proc), + ), patch( + "mcpbridge_wrapper.broker.daemon.asyncio.sleep", + side_effect=_tracked_sleep, + ): + stopper = asyncio.ensure_future(_do_stop()) + await daemon.run_forever() + await stopper + + assert all(delay != 0.1 for delay in sleep_calls) + # --------------------------------------------------------------------------- # _check_and_clear_stale_lock — edge cases