diff --git a/SPECS/ARCHIVE/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth.md b/SPECS/ARCHIVE/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth.md new file mode 100644 index 00000000..0b004408 --- /dev/null +++ b/SPECS/ARCHIVE/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth.md @@ -0,0 +1,95 @@ +# PRD: FU-P12-T1-5 — Cap `_clients` dict and prune `client_identities` to prevent unbounded growth + +**Created:** 2026-02-19 +**Priority:** P2 +**Branch:** `codex/feature/FU-P12-T1-5-cap-clients-and-prune-identities` +**Status:** PLAN + +--- + +## 1. Problem Statement + +`MetricsCollector._clients` and the shared SQLite `client_identities` table both +grow without bounds as new `(name, version)` values appear over time. This can +increase memory use and table size indefinitely for long-running wrapper +processes. + +--- + +## 2. Scope + +### In Scope +- Add a soft capacity limit to in-memory `_clients` by evicting oldest entries + based on `last_seen`. +- Add shared-store pruning for stale `client_identities` records on write. +- Add tests that prove capacity enforcement and shared-store pruning behavior. + +### Out of Scope +- Dashboard UI layout changes. +- Changes to request metrics retention in `request_logs`. +- Adding new user-facing configuration flags. + +--- + +## 3. Deliverables + +1. In-memory client identity cap +- `src/mcpbridge_wrapper/webui/metrics.py` +- Enforce max entries (50) and evict least-recently-seen client identities. + +2. Shared-store stale identity pruning +- `src/mcpbridge_wrapper/webui/shared_metrics.py` +- Prune stale `client_identities` rows during identity writes using `last_seen`. + +3. Test coverage +- `tests/unit/webui/test_metrics.py` +- `tests/unit/webui/test_shared_metrics.py` +- Cover eviction and pruning while preserving existing multi-client behavior. + +4. Validation artifact +- `SPECS/INPROGRESS/FU-P12-T1-5_Validation_Report.md` + +--- + +## 4. Acceptance Criteria + +- [ ] `_clients` dict does not exceed configured cap (50 entries). +- [ ] Oldest entries are evicted first by `last_seen`. +- [ ] Stale `client_identities` rows are pruned on write in shared mode. +- [ ] Existing multi-client behavior remains intact. +- [ ] `pytest` passes. +- [ ] `ruff check src/` passes. +- [ ] `mypy src/` passes. +- [ ] `pytest --cov` reports coverage >= 90%. + +--- + +## 5. Dependencies + +- FU-P12-T1-3 ✅ + +--- + +## 6. Risks and Mitigations + +- **Risk:** Aggressive pruning could remove identities still relevant to recent + dashboard activity. + - **Mitigation:** Use a conservative retention window and prune only clearly + stale entries. + +- **Risk:** Eviction ordering bugs could remove newer clients first. + - **Mitigation:** Derive eviction order directly from tracked `last_seen` and + validate with unit tests. + +--- + +## 7. Validation Plan + +1. Add in-memory cap + eviction in `MetricsCollector`. +2. Add shared-store identity pruning logic on writes. +3. Update and run tests, then run full quality gates. + + +--- +**Archived:** 2026-02-19 +**Verdict:** PASS diff --git a/SPECS/ARCHIVE/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth/FU-P12-T1-5_Validation_Report.md b/SPECS/ARCHIVE/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth/FU-P12-T1-5_Validation_Report.md new file mode 100644 index 00000000..6994696e --- /dev/null +++ b/SPECS/ARCHIVE/FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth/FU-P12-T1-5_Validation_Report.md @@ -0,0 +1,43 @@ +# Validation Report — FU-P12-T1-5 + +**Task:** FU-P12-T1-5 — Cap `_clients` dict and prune `client_identities` to prevent unbounded growth +**Date:** 2026-02-19 +**Verdict:** PASS + +## Scope + +- Added an in-memory client identity cap for `MetricsCollector` with oldest + entry eviction based on `last_seen`. +- Added shared SQLite pruning for stale `client_identities` rows during + `set_client_info` writes. +- Added unit tests for eviction ordering and stale identity pruning. + +## Files Changed + +- `src/mcpbridge_wrapper/webui/metrics.py` +- `src/mcpbridge_wrapper/webui/shared_metrics.py` +- `tests/unit/webui/test_metrics.py` +- `tests/unit/webui/test_shared_metrics.py` + +## Required Quality Gates + +- `pytest` + - Result: **PASS** (`593 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` + - Result: **PASS** (`593 passed, 5 skipped, 2 warnings`; total coverage **92.18%**, threshold 90%) + +## Acceptance Criteria Status + +- [x] `_clients` dict never exceeds the configured cap. +- [x] Stale `client_identities` rows are pruned on write. +- [x] Existing multi-client dashboard behavior is preserved. +- [x] `pytest` suite remains green. + +## Notes + +- Existing third-party deprecation warnings from `websockets` / `uvicorn` were + observed during test runs and are unrelated to this task. diff --git a/SPECS/ARCHIVE/INDEX.md b/SPECS/ARCHIVE/INDEX.md index e0da166d..8d7314f0 100644 --- a/SPECS/ARCHIVE/INDEX.md +++ b/SPECS/ARCHIVE/INDEX.md @@ -1,6 +1,6 @@ # mcpbridge-wrapper Tasks Archive -**Last Updated:** 2026-02-19 (REVIEW_fu_p12_t1_4_in_flight_shared_metrics_archived) +**Last Updated:** 2026-02-19 (REVIEW_fu_p12_t1_5_client_identity_retention_archived) ## Archived Tasks @@ -119,6 +119,7 @@ | FU-P12-T1-2 | [FU-P12-T1-2_Add_code_comment_clarifying_stdin-only_client_capture_in_on_request/](FU-P12-T1-2_Add_code_comment_clarifying_stdin-only_client_capture_in_on_request/) | 2026-02-18 | PASS | | FU-P12-T1-3 | [FU-P12-T1-3_Show_multi-client_widgets_in_Web_UI_instead_of_single_overwritten_active_client/](FU-P12-T1-3_Show_multi-client_widgets_in_Web_UI_instead_of_single_overwritten_active_client/) | 2026-02-18 | PASS | | FU-P12-T1-4 | [FU-P12-T1-4_Make_IN_FLIGHT_KPI_reflect_real_in_flight_requests_in_shared_metrics_mode/](FU-P12-T1-4_Make_IN_FLIGHT_KPI_reflect_real_in_flight_requests_in_shared_metrics_mode/) | 2026-02-19 | PASS | +| FU-P12-T1-5 | [FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth/](FU-P12-T1-5_Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth/) | 2026-02-19 | PASS | | FU-P12-T3-2 | [FU-P12-T3-2_Add_error_code_column_to_audit_CSV_export/](FU-P12-T3-2_Add_error_code_column_to_audit_CSV_export/) | 2026-02-19 | PASS | ## Historical Artifacts @@ -202,6 +203,7 @@ | [REVIEW_FU-P12-T1-2_stdin_capture_comment.md](_Historical/REVIEW_FU-P12-T1-2_stdin_capture_comment.md) | Review report for FU-P12-T1-2 | | [REVIEW_FU-P12-T1-3_multi_client_widgets.md](_Historical/REVIEW_FU-P12-T1-3_multi_client_widgets.md) | Review report for FU-P12-T1-3 | | [REVIEW_FU-P12-T1-4_in_flight_shared_metrics.md](_Historical/REVIEW_FU-P12-T1-4_in_flight_shared_metrics.md) | Review report for FU-P12-T1-4 | +| [REVIEW_FU-P12-T1-5_client_identity_retention.md](_Historical/REVIEW_FU-P12-T1-5_client_identity_retention.md) | Review report for FU-P12-T1-5 | | [REVIEW_FU-P12-T3-2_error_code_csv_export.md](_Historical/REVIEW_FU-P12-T3-2_error_code_csv_export.md) | Review report for FU-P12-T3-2 | ## Archive Log @@ -358,5 +360,7 @@ | 2026-02-18 | FU-P12-T1-3 | Archived REVIEW_FU-P12-T1-3_multi_client_widgets report | | 2026-02-19 | FU-P12-T1-4 | Archived Make_IN_FLIGHT_KPI_reflect_real_in_flight_requests_in_shared_metrics_mode (PASS) | | 2026-02-19 | FU-P12-T1-4 | Archived REVIEW_FU-P12-T1-4_in_flight_shared_metrics report | +| 2026-02-19 | FU-P12-T1-5 | Archived Cap_clients_dict_and_prune_client_identities_to_prevent_unbounded_growth (PASS) | +| 2026-02-19 | FU-P12-T1-5 | Archived REVIEW_FU-P12-T1-5_client_identity_retention report | | 2026-02-19 | FU-P12-T3-2 | Archived Add_error_code_column_to_audit_CSV_export (PASS) | | 2026-02-19 | FU-P12-T3-2 | Archived REVIEW_FU-P12-T3-2_error_code_csv_export report | diff --git a/SPECS/ARCHIVE/_Historical/REVIEW_FU-P12-T1-5_client_identity_retention.md b/SPECS/ARCHIVE/_Historical/REVIEW_FU-P12-T1-5_client_identity_retention.md new file mode 100644 index 00000000..aad6fddc --- /dev/null +++ b/SPECS/ARCHIVE/_Historical/REVIEW_FU-P12-T1-5_client_identity_retention.md @@ -0,0 +1,33 @@ +## REVIEW REPORT — FU-P12-T1-5 client identity retention + +**Scope:** origin/main..HEAD +**Files:** 9 + +### Summary Verdict +- [x] Approve +- [ ] Approve with comments +- [ ] Request changes +- [ ] Block + +### Critical Issues +- None. + +### Secondary Issues +- None. + +### Architectural Notes +- `MetricsCollector` now bounds in-memory client identity growth with + deterministic oldest-first eviction by `last_seen`. +- `SharedMetricsStore` now prunes stale `client_identities` rows on each client + info write, preventing unbounded accumulation in long-lived shared DB usage. + +### Tests +- Quality gates rerun and passing: + - `pytest` (`593 passed, 5 skipped, 2 warnings`) + - `ruff check src/` (`All checks passed!`) + - `mypy src/` (`Success: no issues found in 18 source files`) + - `pytest --cov` (`92.18%`, threshold `>=90%`) + +### Next Steps +- No actionable follow-up items identified. +- FOLLOW-UP step can be skipped for this task. diff --git a/SPECS/INPROGRESS/next.md b/SPECS/INPROGRESS/next.md index 17a61b40..b099b699 100644 --- a/SPECS/INPROGRESS/next.md +++ b/SPECS/INPROGRESS/next.md @@ -2,15 +2,15 @@ ## Recently Archived +- 2026-02-19 — FU-P12-T1-5: Cap `_clients` dict and prune `client_identities` to prevent unbounded growth (PASS) - 2026-02-19 — FU-P12-T1-4: Make `IN FLIGHT` KPI reflect real in-flight requests in shared-metrics mode (PASS) - 2026-02-19 — FU-P12-T3-2: Add `error_code` column to audit CSV export (PASS) - 2026-02-18 — FU-P12-T1-3: Show multi-client widgets in Web UI instead of single overwritten active client (PASS) - 2026-02-18 — FU-P12-T1-2: Add code comment clarifying stdin-only client capture in `on_request` (PASS) - 2026-02-18 — FU-P12-T1-1: Remove or document `MCPInitializeParams` in schemas (PASS) -- 2026-02-18 — FU-BUG-T7-1: Cap `pending_methods` map to guard against unbounded growth (PASS) ## Suggested Next Tasks - P13-T5 follow-up — Complete interactive prompt verification in a desktop session (P1) -- FU-P12-T1-5 — Cap `_clients` dict and prune `client_identities` to prevent unbounded growth (P2) - FU-P12-T1-6 — Uniform HTML escaping in `renderClientWidgets` (P3) +- FU-P12-T3-1 — Document unused `error_message` parameter in `MetricsCollector.record_response` (P3) diff --git a/SPECS/Workplan.md b/SPECS/Workplan.md index 5ea3d003..2765e1f7 100644 --- a/SPECS/Workplan.md +++ b/SPECS/Workplan.md @@ -2250,7 +2250,8 @@ Phase 9 Follow-up Backlog --- -#### ⬜️ FU-P12-T1-5: Cap `_clients` dict and prune `client_identities` to prevent unbounded growth +#### ✅ FU-P12-T1-5: Cap `_clients` dict and prune `client_identities` to prevent unbounded growth +- **Status:** ✅ Completed (2026-02-19) - **Description:** The in-memory `_clients` dict in `MetricsCollector` and the `client_identities` SQLite table in `SharedMetricsStore` grow without limit — every unique `(name, version)` pair adds an entry that is never evicted. Add a soft cap (e.g. 50 entries, evict oldest by `last_seen`) to `_clients`, and add a `WHERE last_seen > ?` pruning clause for `client_identities` on write. This aligns with the project pattern established by FU-BUG-T7-1 (`pending_methods` cap). - **Priority:** P2 - **Dependencies:** FU-P12-T1-3 @@ -2260,10 +2261,10 @@ Phase 9 Follow-up Backlog - Updated `src/mcpbridge_wrapper/webui/shared_metrics.py` — pruning old `client_identities` rows - Updated tests covering eviction behavior - **Acceptance Criteria:** - - [ ] `_clients` dict never exceeds the configured cap - - [ ] Stale `client_identities` rows are pruned on write - - [ ] Existing multi-client dashboard behavior is preserved - - [ ] `pytest` suite remains green + - [x] `_clients` dict never exceeds the configured cap + - [x] Stale `client_identities` rows are pruned on write + - [x] Existing multi-client dashboard behavior is preserved + - [x] `pytest` suite remains green --- diff --git a/src/mcpbridge_wrapper/webui/metrics.py b/src/mcpbridge_wrapper/webui/metrics.py index 35464431..aa3c0eed 100644 --- a/src/mcpbridge_wrapper/webui/metrics.py +++ b/src/mcpbridge_wrapper/webui/metrics.py @@ -10,6 +10,8 @@ from collections import deque from typing import Any, Deque, Dict, List, Optional, Tuple +MAX_CLIENT_IDENTITIES = 50 + def categorize_error(code: Optional[int]) -> str: """Categorize a JSON-RPC error code into a severity bucket. @@ -48,16 +50,23 @@ class MetricsCollector: max_datapoints: Maximum number of data points to retain per metric. """ - def __init__(self, window_seconds: int = 3600, max_datapoints: int = 3600) -> None: + def __init__( + self, + window_seconds: int = 3600, + max_datapoints: int = 3600, + max_clients: int = MAX_CLIENT_IDENTITIES, + ) -> None: """Initialize the metrics collector. Args: window_seconds: Rolling window duration in seconds. max_datapoints: Maximum data points retained per time-series. + max_clients: Maximum number of client identities to retain in memory. """ self._lock = threading.Lock() self._window_seconds = window_seconds self._max_datapoints = max_datapoints + self._max_clients = max_clients # Counters self._total_requests: int = 0 @@ -111,6 +120,25 @@ def set_client_info(self, name: str, version: str) -> None: else: existing["last_seen"] = now existing["initialize_count"] = existing["initialize_count"] + 1 + self._prune_clients_if_needed(now) + + def _prune_clients_if_needed(self, now: float) -> None: + """Trim client identities to the configured max size.""" + if len(self._clients) <= self._max_clients: + return + + # Evict oldest entries by last_seen, keeping ties deterministic by key. + oldest_first = sorted( + self._clients.items(), + key=lambda item: ( + float(item[1].get("last_seen", now)), + item[0][0], + item[0][1], + ), + ) + overflow = len(self._clients) - self._max_clients + for key, _ in oldest_first[:overflow]: + self._clients.pop(key, None) def record_request(self, tool_name: str, request_id: Optional[str] = None) -> None: """Record an incoming request for a tool. diff --git a/src/mcpbridge_wrapper/webui/shared_metrics.py b/src/mcpbridge_wrapper/webui/shared_metrics.py index a2bcbed5..ab5573ca 100644 --- a/src/mcpbridge_wrapper/webui/shared_metrics.py +++ b/src/mcpbridge_wrapper/webui/shared_metrics.py @@ -16,6 +16,7 @@ # Default database location DEFAULT_DB_PATH = Path.home() / ".cache" / "mcpbridge-wrapper" / "metrics.db" +CLIENT_IDENTITIES_RETENTION_SECONDS = 7 * 24 * 60 * 60 class SharedMetricsStore: @@ -222,6 +223,12 @@ def set_client_info(self, name: str, version: str) -> None: initialize_count=client_identities.initialize_count + 1""", (name, version, now), ) + cutoff = now - CLIENT_IDENTITIES_RETENTION_SECONDS + conn.execute( + """DELETE FROM client_identities + WHERE last_seen <= ?""", + (cutoff,), + ) def get_summary(self, window_seconds: int = 3600) -> Dict[str, Any]: """Get aggregated metrics summary. diff --git a/tests/unit/webui/test_metrics.py b/tests/unit/webui/test_metrics.py index ac7b5fc4..619327d2 100644 --- a/tests/unit/webui/test_metrics.py +++ b/tests/unit/webui/test_metrics.py @@ -242,6 +242,41 @@ def test_set_client_info_increments_initialize_count(self): assert len(summary["clients"]) == 1 assert summary["clients"][0]["initialize_count"] == 2 + def test_set_client_info_caps_identity_history(self): + """Client identity history is capped and evicts oldest entries first.""" + metrics = MetricsCollector(max_clients=3) + with patch("time.time", side_effect=[1.0, 2.0, 3.0, 4.0]): + metrics.set_client_info("A", "1") + metrics.set_client_info("B", "1") + metrics.set_client_info("C", "1") + metrics.set_client_info("D", "1") + + summary = metrics.get_summary() + identities = {(client["name"], client["version"]) for client in summary["clients"]} + assert len(summary["clients"]) == 3 + assert ("A", "1") not in identities + assert ("B", "1") in identities + assert ("C", "1") in identities + assert ("D", "1") in identities + + def test_set_client_info_refresh_prevents_recent_client_eviction(self): + """Refreshing a client updates last_seen and avoids oldest-first eviction.""" + metrics = MetricsCollector(max_clients=3) + with patch("time.time", side_effect=[1.0, 2.0, 3.0, 4.0, 5.0]): + metrics.set_client_info("A", "1") + metrics.set_client_info("B", "1") + metrics.set_client_info("C", "1") + metrics.set_client_info("A", "1") + metrics.set_client_info("D", "1") + + summary = metrics.get_summary() + identities = {(client["name"], client["version"]) for client in summary["clients"]} + assert len(summary["clients"]) == 3 + assert ("A", "1") in identities + assert ("B", "1") not in identities + assert ("C", "1") in identities + assert ("D", "1") in identities + def test_reset_clears_client_info(self): """Test that reset() clears client info back to 'unknown'.""" metrics = MetricsCollector() diff --git a/tests/unit/webui/test_shared_metrics.py b/tests/unit/webui/test_shared_metrics.py index 2029240a..bf53f696 100644 --- a/tests/unit/webui/test_shared_metrics.py +++ b/tests/unit/webui/test_shared_metrics.py @@ -4,7 +4,10 @@ import pytest -from mcpbridge_wrapper.webui.shared_metrics import SharedMetricsStore +from mcpbridge_wrapper.webui.shared_metrics import ( + CLIENT_IDENTITIES_RETENTION_SECONDS, + SharedMetricsStore, +) class TestSharedMetricsStore: @@ -254,6 +257,34 @@ def test_set_client_info_same_client_increments_count(self, store): assert len(summary["clients"]) == 1 assert summary["clients"][0]["initialize_count"] == 2 + def test_set_client_info_prunes_stale_client_identities(self, store): + """set_client_info removes stale client identities beyond retention.""" + now = time.time() + stale_time = now - CLIENT_IDENTITIES_RETENTION_SECONDS - 10.0 + fresh_time = now - 5.0 + + with store._transaction() as conn: + conn.execute( + """INSERT INTO client_identities + (client_name, client_version, last_seen, initialize_count) + VALUES (?, ?, ?, ?)""", + ("stale", "0.1", stale_time, 1), + ) + conn.execute( + """INSERT INTO client_identities + (client_name, client_version, last_seen, initialize_count) + VALUES (?, ?, ?, ?)""", + ("fresh", "1.0", fresh_time, 1), + ) + + store.set_client_info("Cursor", "1.2.3") + summary = store.get_summary() + identities = {(client["name"], client["version"]) for client in summary["clients"]} + + assert ("stale", "0.1") not in identities + assert ("fresh", "1.0") in identities + assert ("Cursor", "1.2.3") in identities + def test_reset_clears_client_info(self, store): """Test that reset() clears client info back to 'unknown'.""" store.set_client_info("Cursor", "1.2.3")