From b4439ba4b74c23899531232246561ca6d2844966 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 16:46:39 -0300 Subject: [PATCH 01/10] perf(mcp): raise the scan result cap to 100 000 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured rather than guessed. Per result set the cap costs 4.2 MB against 2.1 MB, and the sort at the end of `_run_batched_scan` goes from 10 ms to 21 ms. Wall clock does not move: reaching either ceiling on a real process took 0.02s against 0.03s, because a value dense enough to hit it yields hits far faster than the 30-second budget can spend — the clock binds, not this. Worst case per session is 20 result sets, so 43 MB against 84 MB. The gain is narrow but it is correctness, not convenience: a scan whose true hit count falls between the two ceilings used to come back flagged `partial`, and refining a truncated set can converge on an address that was never in it. Above the new ceiling nothing changes — a scan for `int 0` matches millions and is unreachable at any sane cap. --- PyMemoryEditor/mcp/config.py | 8 +++++++- docs/mcp.md | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/PyMemoryEditor/mcp/config.py b/PyMemoryEditor/mcp/config.py index 536a479..6c3936a 100644 --- a/PyMemoryEditor/mcp/config.py +++ b/PyMemoryEditor/mcp/config.py @@ -46,7 +46,13 @@ #: value (``int`` ``0``, ``100``) legitimately matches millions of addresses; #: keeping them all would blow out the server's memory for a result set no #: refine loop can use anyway. At the cap the scan stops early and says so. -DEFAULT_MAX_SCAN_RESULTS = 50_000 +#: +#: 100 000 costs 4.2 MB per result set and 21 ms to sort, against 2.1 MB and +#: 10 ms at the old 50 000; wall clock is unchanged, since the 30-second +#: budget binds long before the cap does. The gain is that a scan whose true +#: hit count sits between the two stops being flagged ``partial``, and +#: refining a truncated set can converge on an address that was never in it. +DEFAULT_MAX_SCAN_RESULTS = 100_000 #: Wall-clock budget for one scan, in seconds. A full address-space scan of a #: large process takes minutes — long past the point where an MCP client gives diff --git a/docs/mcp.md b/docs/mcp.md index b02896c..b8d025f 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -111,7 +111,7 @@ pymemoryeditor-mcp --allow-any-process # never asks at all (scripts/CI) --max-scan-results N - Addresses one scan may keep. Default 50 000. + Addresses one scan may keep. Default 100 000. --max-scan-seconds S @@ -129,7 +129,7 @@ pymemoryeditor-mcp --allow-any-process # never asks at all (scripts/CI) ToolWhat it does server_infoCapabilities, limits, policy and open sessions. The assistant should call this first. list_processesRunning processes the server is allowed to open. -open_processAttach by pid or name → a session_id. +open_processAttach by pid or name → a session_id. Eight targets can be open at once; past that it refuses rather than rotating one out, since a handle the assistant still holds must not vanish under it. close_processDetach and drop that session's scan results. process_infoBitness, address-space summary, modules and threads. list_memory_regionsPage through the memory map, filtered by permission or backing file. From 2c957da8abe8b774d4dc2a8c8ce1e3335f285ebf Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 16:46:39 -0300 Subject: [PATCH 02/10] fix(mcp): bound the number of processes open at once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SessionStore` accepted any number of sessions — six opens of the same pid gave six sessions, each holding its own handle. Nothing obliges a model to call `close_process`, and a session that is merely forgotten keeps its handle until the server exits, so the store leaked the one resource garbage collection cannot reclaim: a debug handle on Windows, a task port on macOS. Capped at 8, which is far past real use. It refuses rather than evicting, unlike the per-session scan cap: dropping an old result set is safe because a refine loop never looks back, but closing a handle out from under the model is not, and it has no way to detect that. The message names a session to close and lists what is open, since the model cannot see the store. The naive version of this would have caused the problem it prevents. `attach` opens the handle and *then* registers the session, so a store that refuses leaves the handle with nobody holding it — every rejected attach would cost one more handle than having no cap at all. `attach` now closes it before re-raising, and there is a test that fails without it. `server_info` advertises `max_open_sessions` and `max_scans_per_session`, by the principle already stated next to `max_text_bytes`: a limit this project invented should not have to be discovered by tripping the error. Verified by reverting each half: the cap (3 failures), the close (1). --- PyMemoryEditor/mcp/session.py | 29 ++++++++++++- PyMemoryEditor/mcp/toolset.py | 21 +++++++++- tests/mcp_server/test_session.py | 71 +++++++++++++++++++++++++------- tests/mcp_server/test_toolset.py | 28 +++++++++++++ 4 files changed, 131 insertions(+), 18 deletions(-) diff --git a/PyMemoryEditor/mcp/session.py b/PyMemoryEditor/mcp/session.py index 684160a..e0eca8d 100644 --- a/PyMemoryEditor/mcp/session.py +++ b/PyMemoryEditor/mcp/session.py @@ -38,6 +38,12 @@ #: Result sets retained per session before the oldest is evicted. MAX_SCANS_PER_SESSION = 20 +#: Processes one server keeps open at once. Refuses rather than evicting, +#: unlike the scan cap above: a session owns an OS handle, and closing one out +#: from under the model is not something it can detect. Bounds handle +#: exhaustion, since nothing obliges a model to call ``close_process``. +MAX_OPEN_SESSIONS = 8 + class SessionError(Exception): """An unknown or expired session/scan id. @@ -178,8 +184,28 @@ def __init__(self) -> None: self._lock = threading.Lock() def open(self, process: AbstractProcess, pid: int, name: str) -> Session: - """Register an already-opened process and return its session.""" + """Register an already-opened process and return its session. + + :raises SessionError: if :data:`MAX_OPEN_SESSIONS` are already open. + The caller owns the handle it passed in and must close it. + """ with self._lock: + if len(self._sessions) >= MAX_OPEN_SESSIONS: + raise SessionError( + "This server already has %d processes open, which is the " + "limit. Close one you are done with — close_process(%s) " + "frees the oldest — and attach again. Open sessions: %s." + % ( + MAX_OPEN_SESSIONS, + next(iter(self._sessions)), + ", ".join( + "%s (%s, pid %d)" + % (sid, session.name or "?", session.pid) + for sid, session in self._sessions.items() + ), + ) + ) + session_id = "proc-%d" % next(self._session_ids) session = Session( session_id=session_id, process=process, pid=pid, name=name @@ -377,6 +403,7 @@ def host_platform() -> str: __all__ = ( + "MAX_OPEN_SESSIONS", "MAX_SCANS_PER_SESSION", "ScanResult", "Session", diff --git a/PyMemoryEditor/mcp/toolset.py b/PyMemoryEditor/mcp/toolset.py index c5f6c19..03140da 100644 --- a/PyMemoryEditor/mcp/toolset.py +++ b/PyMemoryEditor/mcp/toolset.py @@ -48,8 +48,11 @@ ) from .config import ServerConfig from .session import ( + MAX_OPEN_SESSIONS, + MAX_SCANS_PER_SESSION, ScanResult, Session, + SessionError, SessionStore, batch_regions, host_platform, @@ -599,6 +602,8 @@ def server_info(self) -> Dict[str, Any]: # to. "max_address": format_address(MAX_ADDRESS), "max_offset_magnitude": format_address(MAX_OFFSET_MAGNITUDE), + "max_open_sessions": MAX_OPEN_SESSIONS, + "max_scans_per_session": MAX_SCANS_PER_SESSION, }, "open_sessions": sessions, } @@ -771,7 +776,17 @@ def attach(self, pid: int, name: str) -> Dict[str, Any]: "Could not open pid %d: %s. %s" % (pid, error, _permission_hint()) ) from None - session = self.store.open(process, pid, name) + try: + session = self.store.open(process, pid, name) + except SessionError: + # The handle already exists and the store's cap is checked after + # it does, so refusing without closing leaks the resource the cap + # protects. + try: + process.close() + except Exception: # noqa: BLE001 — target may already be gone + pass + raise result: Dict[str, Any] = { "opened": True, @@ -797,6 +812,10 @@ def open_process(self, pid: int = 0, name: str = "") -> Dict[str, Any]: reopening the same target, since each open costs a handle and resets the cached region map. + At most ``max_open_sessions`` (see ``server_info``) can be open at + once; reaching it is refused, not rotated, so ``close_process`` a + target you are done with. + Attaching to a process the operator has not pre-approved requires the **user's** approval, asked for at the moment you call this. Say which process you want and why; if the request is refused, report that rather diff --git a/tests/mcp_server/test_session.py b/tests/mcp_server/test_session.py index 42c72c8..8734670 100644 --- a/tests/mcp_server/test_session.py +++ b/tests/mcp_server/test_session.py @@ -7,6 +7,7 @@ import pytest from PyMemoryEditor.mcp.session import ( + MAX_OPEN_SESSIONS, MAX_SCANS_PER_SESSION, SessionError, SessionStore, @@ -201,6 +202,54 @@ def test_snapshot_is_address_sorted(self, store): ) +class TestOpenSessionsAreCapped: + """The store used to accept any number of open processes, each holding an + OS handle that only `close_process` releases.""" + + def test_the_cap_is_enforced(self, store): + for _ in range(MAX_OPEN_SESSIONS): + store.open(FakeProcess(), 1, "a") + + with pytest.raises(SessionError) as error: + store.open(FakeProcess(), 1, "a") + + assert str(MAX_OPEN_SESSIONS) in str(error.value) + + def test_the_refusal_says_how_to_recover(self, store): + """The model cannot see the store, so the message has to name an id to + close and list what is open.""" + for index in range(MAX_OPEN_SESSIONS): + store.open(FakeProcess(pid=100 + index), 100 + index, "target%d" % index) + + with pytest.raises(SessionError) as error: + store.open(FakeProcess(), 999, "another") + + message = str(error.value) + assert "close_process" in message + assert "proc-1" in message # the id it is told to close + assert "target0" in message # and what is actually open + assert "pid 100" in message + + def test_closing_one_frees_a_slot(self, store): + ids = [store.open(FakeProcess(), 1, "a").session_id + for _ in range(MAX_OPEN_SESSIONS)] + + store.close(ids[0]) + reaberto = store.open(FakeProcess(), 2, "b") + + assert reaberto.session_id not in ids + assert len(store.sessions) == MAX_OPEN_SESSIONS + + def test_ids_keep_climbing_after_a_close(self, store): + """Reusing an id would let a model holding a stale one address a + different process.""" + primeiro = store.open(FakeProcess(), 1, "a").session_id + store.close(primeiro) + segundo = store.open(FakeProcess(), 2, "b").session_id + + assert primeiro != segundo + + class TestBatchRegions: def _regions(self, sizes): address = 0x1000 @@ -221,28 +270,18 @@ def test_batches_respect_the_byte_budget(self): assert len(batches) == 4 # 3 x 300 bytes, then the 100-byte remainder def test_a_region_larger_than_the_budget_gets_its_own_batch(self): - # It cannot be split without splitting a value across the seam, so the - # budget is a target rather than a guarantee -- but the batch that - # busts it should not be carrying anything else. - # - # This assertion used to be `[2, 1]`, i.e. the oversized region shared - # a batch with the small one before it, under this same name. The name - # was right and the assertion pinned the opposite, so the test was - # documenting the bug it looked like it was guarding against. + # A region can't be split without splitting a value across the seam, + # so the budget is a target -- but the batch that busts it should not + # carry anything else. This asserted `[2, 1]` under the same name, + # i.e. it pinned the opposite of what the name claims. batches = batch_regions(self._regions([10, 5000, 10]), 100) assert [[region.size for region in batch] for batch in batches] == [ [10], [5000], [10] ] def test_a_run_of_small_regions_does_not_ride_along_with_a_large_one(self): - """The case that made the deadline check pointless. - - `batch_regions` exists so `_run_batched_scan` gets a chance to look at - the clock between batches. Without the flush, every small region before - an oversized one joined it: `[10, 10, 10, 5000]` against a 100-byte - budget came back as a *single* batch of four, so the deadline was - checked once for the whole scan -- the one thing the batching is for. - """ + """Without the flush, `[10, 10, 10, 5000]` on a 100-byte budget was one + batch of four, so the deadline was checked once for the whole scan.""" batches = batch_regions(self._regions([10, 10, 10, 5000]), 100) assert len(batches) == 2 diff --git a/tests/mcp_server/test_toolset.py b/tests/mcp_server/test_toolset.py index e74d652..4197abb 100644 --- a/tests/mcp_server/test_toolset.py +++ b/tests/mcp_server/test_toolset.py @@ -1210,6 +1210,34 @@ def test_max_offset_is_still_clamped_at_the_top(self, toolset, session): # --- the one address no parser ever sees --------------------------- # + def test_a_refused_attach_does_not_leak_the_handle( + self, make_toolset, fake_process, monkeypatch + ): + """`attach` opens the handle and *then* registers the session, so a + refusal must close it or the cap leaks what it exists to bound.""" + from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS + + abertos = [] + + def fake_open(**_kwargs): + process = FakeProcess() + abertos.append(process) + return process + + toolset = make_toolset(config()) + monkeypatch.setattr(toolset, "_open_process", fake_open) + + for _ in range(MAX_OPEN_SESSIONS): + toolset.open_process(pid=4242) + + with pytest.raises(SessionError): + toolset.open_process(pid=4242) + + # The last one is the refused attach's, and only it should be closed. + assert len(abertos) == MAX_OPEN_SESSIONS + 1 + assert abertos[-1].closed is True + assert all(process.closed is False for process in abertos[:-1]) + def test_a_chain_resolving_past_64_bits_is_refused_not_returned( self, make_toolset, fake_process ): From 1bc00654e6792b768869d0c2c7373e333299304a Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 18:17:48 -0300 Subject: [PATCH 03/10] test(mcp): address the review on #96 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all correct. **Non-English identifiers.** `reaberto`, `primeiro`, `segundo`, `abertos` — Portuguese names in an all-English codebase, from writing these tests while conversing in Portuguese. Renamed. **A fixture requested and never used.** `test_a_refused_attach_does_not_leak_ the_handle` took `fake_process` and built its own `FakeProcess` instances inside the patched opener, so the parameter did nothing — and `make_toolset` already depends on that fixture, making it redundant twice over. Dropped. (The report also pointed at `test_a_chain_resolving_past_64_bits_is_refused_ not_returned` for the same thing, but that one does use it — `fake_process.resolve_pointer_chain = ...`. Only the one is unused.) **A hardcoded limit in the docs.** `docs/mcp.md` spells out "Eight targets can be open at once", which a constant change would silently invalidate. Keeping the number rather than pointing the reader at `server_info` — a doc that tells you to call an API to learn a limit is worse for a human — and pinning it instead, the same way `test_advertised_widths_are_exactly_the_accepted_widths` pins the widths. `DEFAULT_MAX_SCAN_RESULTS` gets the same guard, since this PR just changed it. Verified by mutating both constants: each fails its own assertion. --- tests/mcp_server/test_parsing.py | 41 ++++++++++++++++++++++++++++++++ tests/mcp_server/test_session.py | 12 +++++----- tests/mcp_server/test_toolset.py | 12 +++++----- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/tests/mcp_server/test_parsing.py b/tests/mcp_server/test_parsing.py index c14ccad..f908a03 100644 --- a/tests/mcp_server/test_parsing.py +++ b/tests/mcp_server/test_parsing.py @@ -630,6 +630,47 @@ def test_the_partition_actually_covers_the_library(self): assert len(lib) + len(mcp) > len(sources) // 2 +class TestTheDocsQuoteTheRealLimits: + """`docs/mcp.md` spells the caps out as words, which a reader wants and a + constant change silently invalidates. Same guard as + `test_advertised_widths_are_exactly_the_accepted_widths`.""" + + WORDS = { + 1: "one", 2: "two", 3: "three", 4: "four", 5: "five", + 6: "six", 7: "seven", 8: "eight", 9: "nine", 10: "ten", + } + + def _mcp_doc(self): + from pathlib import Path + + return ( + Path(__file__).resolve().parents[2] / "docs" / "mcp.md" + ).read_text(encoding="utf-8") + + def test_the_open_session_cap_matches_the_constant(self): + from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS + + doc = self._mcp_doc().lower() + word = self.WORDS[MAX_OPEN_SESSIONS] + + assert "%s targets can be open at once" % word in doc, ( + "docs/mcp.md does not state MAX_OPEN_SESSIONS=%d (%r)" + % (MAX_OPEN_SESSIONS, word) + ) + + def test_the_scan_result_cap_matches_the_default(self): + from PyMemoryEditor.mcp.config import DEFAULT_MAX_SCAN_RESULTS + + doc = self._mcp_doc() + # The docs group digits, as prose should: "100 000", not "100000". + grouped = "{:,}".format(DEFAULT_MAX_SCAN_RESULTS).replace(",", " ") + + assert "Default %s." % grouped in doc, ( + "docs/mcp.md does not state DEFAULT_MAX_SCAN_RESULTS=%s" + % grouped + ) + + class TestServerConfigValidatesItsBounds: """The three numeric bounds were checked at the CLI only. diff --git a/tests/mcp_server/test_session.py b/tests/mcp_server/test_session.py index 8734670..a54e167 100644 --- a/tests/mcp_server/test_session.py +++ b/tests/mcp_server/test_session.py @@ -235,19 +235,19 @@ def test_closing_one_frees_a_slot(self, store): for _ in range(MAX_OPEN_SESSIONS)] store.close(ids[0]) - reaberto = store.open(FakeProcess(), 2, "b") + reopened = store.open(FakeProcess(), 2, "b") - assert reaberto.session_id not in ids + assert reopened.session_id not in ids assert len(store.sessions) == MAX_OPEN_SESSIONS def test_ids_keep_climbing_after_a_close(self, store): """Reusing an id would let a model holding a stale one address a different process.""" - primeiro = store.open(FakeProcess(), 1, "a").session_id - store.close(primeiro) - segundo = store.open(FakeProcess(), 2, "b").session_id + first = store.open(FakeProcess(), 1, "a").session_id + store.close(first) + second = store.open(FakeProcess(), 2, "b").session_id - assert primeiro != segundo + assert first != second class TestBatchRegions: diff --git a/tests/mcp_server/test_toolset.py b/tests/mcp_server/test_toolset.py index 4197abb..5ddbb7c 100644 --- a/tests/mcp_server/test_toolset.py +++ b/tests/mcp_server/test_toolset.py @@ -1211,17 +1211,17 @@ def test_max_offset_is_still_clamped_at_the_top(self, toolset, session): # --- the one address no parser ever sees --------------------------- # def test_a_refused_attach_does_not_leak_the_handle( - self, make_toolset, fake_process, monkeypatch + self, make_toolset, monkeypatch ): """`attach` opens the handle and *then* registers the session, so a refusal must close it or the cap leaks what it exists to bound.""" from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS - abertos = [] + opened = [] def fake_open(**_kwargs): process = FakeProcess() - abertos.append(process) + opened.append(process) return process toolset = make_toolset(config()) @@ -1234,9 +1234,9 @@ def fake_open(**_kwargs): toolset.open_process(pid=4242) # The last one is the refused attach's, and only it should be closed. - assert len(abertos) == MAX_OPEN_SESSIONS + 1 - assert abertos[-1].closed is True - assert all(process.closed is False for process in abertos[:-1]) + assert len(opened) == MAX_OPEN_SESSIONS + 1 + assert opened[-1].closed is True + assert all(process.closed is False for process in opened[:-1]) def test_a_chain_resolving_past_64_bits_is_refused_not_returned( self, make_toolset, fake_process From d8ae874d11d0ca393dd49907119ab98b511763f0 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 19:08:42 -0300 Subject: [PATCH 04/10] fix(mcp): refuse a full server before spending the user's approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both correct. **The cap was enforced after the prompt.** `SessionStore.open` is the guard, and it runs once `attach` has already opened the handle — which is after `_register_open_process` has elicited the user's approval. So the flow was: ask "Allow PyMemoryEditor to attach to game.exe?", user approves, then the call fails because the server is full. An approval is the most expensive step in this server and the one thing it must not spend for nothing. `SessionStore.at_capacity()` is now consulted before the prompt. `open` stays the authority — the two can disagree under a concurrent open, which is why the deep guard remains — but the common case fails fast, and the message names the target so a model does not conclude the pid was wrong. **`WORDS[MAX_OPEN_SESSIONS]` raised KeyError above 10.** Reported by Copilot. The test exists to turn a silent doc/code divergence into an actionable message, and bumping the constant to 12 replaced that with `KeyError: 12` — loud, but saying nothing about what to do. Looked up with `.get` now, with the missing spelling as its own assertion. All three cases verified: 8 passes, 6 says the doc disagrees, 12 says to add the word. Verified by reverting the early check: the prompt happens again and `test_a_full_server_refuses_before_spending_an_approval` fails. --- PyMemoryEditor/mcp/server.py | 14 +++++++++++- PyMemoryEditor/mcp/session.py | 12 ++++++++++ tests/mcp_server/test_parsing.py | 14 +++++++++--- tests/mcp_server/test_server.py | 38 ++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 4 deletions(-) diff --git a/PyMemoryEditor/mcp/server.py b/PyMemoryEditor/mcp/server.py index 26df82e..9539802 100644 --- a/PyMemoryEditor/mcp/server.py +++ b/PyMemoryEditor/mcp/server.py @@ -27,7 +27,7 @@ from .. import __version__ from .config import ServerConfig, parse_args -from .session import SessionError, host_platform +from .session import MAX_OPEN_SESSIONS, SessionError, host_platform from .toolset import MemoryToolset, ToolError if TYPE_CHECKING: # pragma: no cover - import cost avoided at runtime @@ -238,6 +238,18 @@ async def open_process(pid: int = 0, name: str = "", *, ctx: Context) -> Any: if not _can_elicit(ctx): raise SdkToolError(decision.reason) + # Before the prompt, not after. The cap lives in SessionStore.open, + # which runs once the handle is already open -- so without this the + # user was asked to approve an attach, approved it, and then got told + # the server was full. An approval is the most expensive step here and + # the one thing this server must not spend carelessly. + if toolset.store.at_capacity(): + raise SdkToolError( + "Not asking to attach to \"%s\" (pid %d): this server already " + "has %d processes open, which is the limit. Close one with " + "close_process first." % (found_name, found_pid, MAX_OPEN_SESSIONS) + ) + try: answer = await ctx.elicit( message=( diff --git a/PyMemoryEditor/mcp/session.py b/PyMemoryEditor/mcp/session.py index e0eca8d..316b1ce 100644 --- a/PyMemoryEditor/mcp/session.py +++ b/PyMemoryEditor/mcp/session.py @@ -213,6 +213,18 @@ def open(self, process: AbstractProcess, pid: int, name: str) -> Session: self._sessions[session_id] = session return session + def at_capacity(self) -> bool: + """Whether :meth:`open` would refuse right now. + + Lets a caller fail before doing something expensive that the refusal + would waste — the protocol layer asks this before prompting the user, + since spending an approval on an attach that cannot happen is worse + than refusing outright. :meth:`open` stays the authority: this is a + hint, and the two can differ under a concurrent open. + """ + with self._lock: + return len(self._sessions) >= MAX_OPEN_SESSIONS + def get(self, session_id: str) -> Session: """Look up a session, or explain how to obtain a valid id.""" known = "none" diff --git a/tests/mcp_server/test_parsing.py b/tests/mcp_server/test_parsing.py index f908a03..cd42703 100644 --- a/tests/mcp_server/test_parsing.py +++ b/tests/mcp_server/test_parsing.py @@ -650,10 +650,18 @@ def _mcp_doc(self): def test_the_open_session_cap_matches_the_constant(self): from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS - doc = self._mcp_doc().lower() - word = self.WORDS[MAX_OPEN_SESSIONS] + # Looked up, not indexed. This test exists to turn a silent doc/code + # divergence into an actionable message, and `WORDS[12]` would have + # replaced that with `KeyError: 12` — loud, but saying nothing about + # what to do. + word = self.WORDS.get(MAX_OPEN_SESSIONS) + assert word is not None, ( + "MAX_OPEN_SESSIONS is %d and WORDS has no spelling for it. Add " + "one, then update the open_process row in docs/mcp.md to match." + % MAX_OPEN_SESSIONS + ) - assert "%s targets can be open at once" % word in doc, ( + assert "%s targets can be open at once" % word in self._mcp_doc().lower(), ( "docs/mcp.md does not state MAX_OPEN_SESSIONS=%d (%r)" % (MAX_OPEN_SESSIONS, word) ) diff --git a/tests/mcp_server/test_server.py b/tests/mcp_server/test_server.py index 88cd631..701964b 100644 --- a/tests/mcp_server/test_server.py +++ b/tests/mcp_server/test_server.py @@ -437,6 +437,44 @@ def test_refusal_tells_the_model_not_to_go_looking_elsewhere(self): assert "did not approve" in text assert "Do not retry" in text + def test_a_full_server_refuses_before_spending_an_approval(self): + """The cap lives in `SessionStore.open`, which runs after the handle is + already open — so the user used to be asked, approve, and only then be + told the server was full.""" + from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS + + server, toolset, _process = self._build(ServerConfig()) + for _ in range(MAX_OPEN_SESSIONS): + toolset.store.open(FakeProcess(), 4242, "faketarget") + + answerer = Answerer("accept") + + async def run(): + async with connected(server, answerer) as session: + return await session.call_tool("open_process", {"pid": 4242}) + + result = asyncio.run(run()) + + assert result.is_error is True + assert answerer.asked is False, "the user was prompted for an attach that could not happen" + assert "close_process" in result.content[0].text + + def test_the_refusal_names_the_target_it_declined(self): + """A model reads this and must not conclude the pid was the problem.""" + from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS + + server, toolset, _process = self._build(ServerConfig()) + for _ in range(MAX_OPEN_SESSIONS): + toolset.store.open(FakeProcess(), 4242, "faketarget") + + async def run(): + async with connected(server, Answerer("accept")) as session: + return await session.call_tool("open_process", {"pid": 4242}) + + text = asyncio.run(run()).content[0].text + assert "faketarget" in text and "4242" in text + assert str(MAX_OPEN_SESSIONS) in text + def test_cancelling_the_dialog_is_not_approval(self): result, toolset = self._open(ServerConfig(), Answerer("cancel")) assert result.is_error is True From 5d6f9918946b9f266dbaa61f25783ccc649a4422 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 19:33:14 -0300 Subject: [PATCH 05/10] fix(mcp): reap dead sessions, or the cap locks the server out for good MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cap I added in this PR created a way to disable the feature it protects, and a review caught it. Nothing here notices a target exiting. `_rows_for` swallows the read error, `process_info` keeps answering from cached state, and the session stays in the store — a session only leaves via `close_process` or shutdown. Before the cap that was clutter. With a cap that refuses rather than evicts, eight exited processes hold every slot for the life of the server, every `open_process` is refused, and the model cannot even find out which sessions are the dead ones. Reproduced end to end: eight dead targets, ninth attach refused, `process_info("proc-1")` answering as though alive. `SessionStore._reap_dead()` now drops sessions whose pid is gone, called from `open` before it refuses and from `at_capacity` before it reports full. Evicting a *dead* session is the exception that proves the rule about not evicting: its handle is already useless, so nothing is taken away. A full server of live targets still refuses, asserted so the reaper cannot become a back door around the cap. `server_info().open_sessions` gains `alive`, so the model can see them before the store needs the room. **The reaper's first version had a bug of its own,** found by the argument fuzzer going intermittently red: `pid_exists(2**31)` raises `OverflowError` — the value does not fit the platform's pid type — and the reaper runs inside `open`, so it escaped as something other than a `SessionError` and the model would have seen only "Error executing tool". `_looks_alive` treats a pid the OS refuses as dead, which is correct as well as safe: a number the OS will not accept cannot name a running process. Five clean runs of the suite after, against one failure in the run before. **The refusal message no longer says "close the oldest".** The oldest is usually the target the whole session has been refining, and `close_process` discards its scan results too, so following the message literally destroyed the most valuable session. It now lists every session with its scan count, and names the session already holding the requested pid when there is one — a model that lost count was being told the server was full rather than that the target was already open. **The cap's justification was also wrong**, and that is fixed in the comment rather than papered over. It priced one result set and the per-session total (84 MB) while this same PR added the eight-session cap, so the real ceiling on retained addresses goes from ~336 MB to ~672 MB. And "wall clock is unchanged" was true of the one value I measured (`int 0`, dense enough that hits arrive faster than the clock can spend) and not of the change: for a sparser value, collecting twice as many hits means walking further. Also drops the stale `50 000` from `_read_addresses`'s docstring, which is model-facing prompt text this PR had just doubled. Tests pinned liveness with a fixture, because a fake's pid is an arbitrary number: several of the cap tests had been passing only because `pid_exists(1)` is true on a Unix host. --- PyMemoryEditor/mcp/config.py | 25 +++++-- PyMemoryEditor/mcp/session.py | 88 ++++++++++++++++++++-- PyMemoryEditor/mcp/toolset.py | 11 ++- tests/mcp_server/test_server.py | 12 ++- tests/mcp_server/test_session.py | 124 +++++++++++++++++++++++++++++-- tests/mcp_server/test_toolset.py | 5 ++ 6 files changed, 245 insertions(+), 20 deletions(-) diff --git a/PyMemoryEditor/mcp/config.py b/PyMemoryEditor/mcp/config.py index 6c3936a..27ee114 100644 --- a/PyMemoryEditor/mcp/config.py +++ b/PyMemoryEditor/mcp/config.py @@ -47,11 +47,26 @@ #: keeping them all would blow out the server's memory for a result set no #: refine loop can use anyway. At the cap the scan stops early and says so. #: -#: 100 000 costs 4.2 MB per result set and 21 ms to sort, against 2.1 MB and -#: 10 ms at the old 50 000; wall clock is unchanged, since the 30-second -#: budget binds long before the cap does. The gain is that a scan whose true -#: hit count sits between the two stops being flagged ``partial``, and -#: refining a truncated set can converge on an address that was never in it. +#: 100 000 costs 4.2 MB per result set against 2.1 MB, and 21 ms to sort +#: against 10 ms. The figure that matters is not per set, though: a session +#: keeps ``MAX_SCANS_PER_SESSION`` (20) of them and a server keeps +#: ``MAX_OPEN_SESSIONS`` (8) sessions, so the ceiling on retained addresses +#: goes from ~336 MB to ~672 MB. Reaching it needs 160 capped result sets, +#: which a long refine chain across several targets can do. +#: +#: What it buys: a scan whose true hit count falls between the two ceilings +#: stops being flagged ``partial``, and refining a truncated set can converge +#: on an address that was never in it. That case is real but narrow — above +#: the new ceiling nothing changes, and a common value like ``int 0`` matches +#: millions and is out of reach at any sane cap. +#: +#: What it costs in time depends on the value's density, and an earlier +#: version of this comment got that wrong. Measured on ``int 0``, reaching +#: either ceiling took 0.02s against 0.03s — but that is the dense case, where +#: hits arrive faster than the clock can spend. For a sparser value the scan +#: has to walk further to collect twice as many hits, so the time roughly +#: doubles up to the 30-second budget. "Wall clock is unchanged" was true of +#: one measurement, not of the change. DEFAULT_MAX_SCAN_RESULTS = 100_000 #: Wall-clock budget for one scan, in seconds. A full address-space scan of a diff --git a/PyMemoryEditor/mcp/session.py b/PyMemoryEditor/mcp/session.py index 316b1ce..7a59844 100644 --- a/PyMemoryEditor/mcp/session.py +++ b/PyMemoryEditor/mcp/session.py @@ -32,6 +32,7 @@ from typing import Dict, List, Optional, Sequence, Tuple from ..process.abstract import AbstractProcess +from ..process.util import pid_exists from ..process.region import MemoryRegion, MemoryRegionSnapshot @@ -168,6 +169,22 @@ def scan_ids(self) -> Tuple[str, ...]: return tuple(self._scan_order) +def _looks_alive(pid: int) -> bool: + """``pid_exists``, but a pid the OS will not even accept reads as dead. + + The reaper runs inside ``open``, so anything raised here escapes as + something other than a ``SessionError`` and the model is told only + "Error executing tool". ``pid_exists(2**31)`` raises ``OverflowError`` + (the value does not fit the platform's pid type), which the argument + fuzzer found by generating exactly that. A number the OS refuses cannot + name a running process, so treating it as dead is both safe and correct. + """ + try: + return pid_exists(pid) + except (OverflowError, ValueError, OSError): + return False + + class SessionStore: """The server's process handles and their scan results. @@ -189,20 +206,39 @@ def open(self, process: AbstractProcess, pid: int, name: str) -> Session: :raises SessionError: if :data:`MAX_OPEN_SESSIONS` are already open. The caller owns the handle it passed in and must close it. """ + if len(self._sessions) >= MAX_OPEN_SESSIONS: + self._reap_dead() + with self._lock: if len(self._sessions) >= MAX_OPEN_SESSIONS: + # No "close the oldest": the oldest is usually the target the + # whole session has been refining, and close_process discards + # its scan results too. The list is given instead, so the + # choice is made on what each session holds. + existing = [ + sid for sid, s in self._sessions.items() if s.pid == pid + ] + already = ( + " Note that pid %d is already open as %s — reuse that id " + "rather than attaching again." % (pid, existing[0]) + if existing + else "" + ) raise SessionError( "This server already has %d processes open, which is the " - "limit. Close one you are done with — close_process(%s) " - "frees the oldest — and attach again. Open sessions: %s." + "limit, and all of them are live. Close whichever you are " + "done with (close_process also discards that session's " + "scan results) and attach again. Open: %s.%s" % ( MAX_OPEN_SESSIONS, - next(iter(self._sessions)), ", ".join( - "%s (%s, pid %d)" - % (sid, session.name or "?", session.pid) + "%s (%s, pid %d, %d scan%s)" + % (sid, session.name or "?", session.pid, + len(session.scan_ids), + "" if len(session.scan_ids) == 1 else "s") for sid, session in self._sessions.items() ), + already, ) ) @@ -213,6 +249,39 @@ def open(self, process: AbstractProcess, pid: int, name: str) -> Session: self._sessions[session_id] = session return session + def _reap_dead(self) -> List[str]: + """Drop sessions whose target no longer exists. Caller holds no lock. + + The cap refuses rather than evicting, because closing a live handle + out from under the model is something it cannot detect. A *dead* + target is the exception that proves the rule: the handle is already + useless, so dropping it takes nothing away — and without this, eight + exited processes hold every slot forever and `open_process` is refused + for the rest of the server's life, with no way for the model to find + out which sessions are the dead ones (`process_info` answers happily + from cached state). + + A recycled pid reads as alive, which is inherent to asking the OS by + number. + """ + with self._lock: + dead = [ + session_id + for session_id, session in self._sessions.items() + if not _looks_alive(session.pid) + ] + + for session_id in dead: + try: + self.close(session_id) + except SessionError: # closed concurrently + pass + return dead + + def is_alive(self, session_id: str) -> bool: + """Whether this session's target still exists.""" + return _looks_alive(self.get(session_id).pid) + def at_capacity(self) -> bool: """Whether :meth:`open` would refuse right now. @@ -221,7 +290,16 @@ def at_capacity(self) -> bool: since spending an approval on an attach that cannot happen is worse than refusing outright. :meth:`open` stays the authority: this is a hint, and the two can differ under a concurrent open. + + Reaps first, or this would report a server full of exited processes as + full when `open` would have made room. """ + with self._lock: + if len(self._sessions) < MAX_OPEN_SESSIONS: + return False + + self._reap_dead() + with self._lock: return len(self._sessions) >= MAX_OPEN_SESSIONS diff --git a/PyMemoryEditor/mcp/toolset.py b/PyMemoryEditor/mcp/toolset.py index 03140da..686006f 100644 --- a/PyMemoryEditor/mcp/toolset.py +++ b/PyMemoryEditor/mcp/toolset.py @@ -38,7 +38,7 @@ from ..process.abstract import AbstractProcess from ..process.errors import PyMemoryEditorError from ..process.region import MemoryRegion -from ..process.util import get_process_ids_by_name, iter_processes +from ..process.util import get_process_ids_by_name, iter_processes, pid_exists from ..util import ( decode_scan_target, resolve_bufflength, @@ -556,12 +556,18 @@ def server_info(self) -> Dict[str, Any]: reachable, how long a scan may run, and which sessions are already open from earlier in the conversation. """ + # `alive` because a session outlives its target: nothing here notices + # a process exiting, `process_info` keeps answering from cached state, + # and the open-session cap means dead ones would hold slots the model + # cannot identify. The store reaps them when it needs room; this is how + # the model sees them before that. sessions = [ { "session_id": session.session_id, "pid": session.pid, "name": session.name, "scan_ids": list(session.scan_ids), + "alive": pid_exists(session.pid), } for session in self.store.sessions ] @@ -2020,7 +2026,8 @@ def _read_addresses( """Yield ``(address, value | None)`` for each address, in one pass. Uses ``search_by_addresses``, which groups the reads by region so a - 50 000-address refine is a few hundred syscalls rather than 50 000. + refine of a capped result set is a few hundred syscalls rather than one + per address. """ yield from session.process.search_by_addresses( pytype, diff --git a/tests/mcp_server/test_server.py b/tests/mcp_server/test_server.py index 701964b..6181082 100644 --- a/tests/mcp_server/test_server.py +++ b/tests/mcp_server/test_server.py @@ -437,12 +437,16 @@ def test_refusal_tells_the_model_not_to_go_looking_elsewhere(self): assert "did not approve" in text assert "Do not retry" in text - def test_a_full_server_refuses_before_spending_an_approval(self): + def test_a_full_server_refuses_before_spending_an_approval(self, monkeypatch): """The cap lives in `SessionStore.open`, which runs after the handle is already open — so the user used to be asked, approve, and only then be told the server was full.""" + from PyMemoryEditor.mcp import session as session_module from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS + # Fakes carry arbitrary pids, which the store's reaper reads as dead. + monkeypatch.setattr(session_module, "pid_exists", lambda pid: True) + server, toolset, _process = self._build(ServerConfig()) for _ in range(MAX_OPEN_SESSIONS): toolset.store.open(FakeProcess(), 4242, "faketarget") @@ -459,10 +463,14 @@ async def run(): assert answerer.asked is False, "the user was prompted for an attach that could not happen" assert "close_process" in result.content[0].text - def test_the_refusal_names_the_target_it_declined(self): + def test_the_refusal_names_the_target_it_declined(self, monkeypatch): """A model reads this and must not conclude the pid was the problem.""" + from PyMemoryEditor.mcp import session as session_module from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS + # Fakes carry arbitrary pids, which the store's reaper reads as dead. + monkeypatch.setattr(session_module, "pid_exists", lambda pid: True) + server, toolset, _process = self._build(ServerConfig()) for _ in range(MAX_OPEN_SESSIONS): toolset.store.open(FakeProcess(), 4242, "faketarget") diff --git a/tests/mcp_server/test_session.py b/tests/mcp_server/test_session.py index a54e167..6dc4e15 100644 --- a/tests/mcp_server/test_session.py +++ b/tests/mcp_server/test_session.py @@ -6,6 +6,7 @@ import pytest +from PyMemoryEditor.mcp import session as session_module from PyMemoryEditor.mcp.session import ( MAX_OPEN_SESSIONS, MAX_SCANS_PER_SESSION, @@ -202,11 +203,23 @@ def test_snapshot_is_address_sorted(self, store): ) +@pytest.fixture +def all_alive(monkeypatch): + """Pin every session's target as live. + + The store reaps sessions whose pid is gone, and a fake's pid is an + arbitrary number — so without this these tests depend on the host's + process table, and the cap either fires or does not by luck. (Several + passed only because `pid_exists(1)` is true on a Unix host.) + """ + monkeypatch.setattr(session_module, "pid_exists", lambda pid: True) + + class TestOpenSessionsAreCapped: """The store used to accept any number of open processes, each holding an OS handle that only `close_process` releases.""" - def test_the_cap_is_enforced(self, store): + def test_the_cap_is_enforced(self, store, all_alive): for _ in range(MAX_OPEN_SESSIONS): store.open(FakeProcess(), 1, "a") @@ -215,7 +228,7 @@ def test_the_cap_is_enforced(self, store): assert str(MAX_OPEN_SESSIONS) in str(error.value) - def test_the_refusal_says_how_to_recover(self, store): + def test_the_refusal_says_how_to_recover(self, store, all_alive): """The model cannot see the store, so the message has to name an id to close and list what is open.""" for index in range(MAX_OPEN_SESSIONS): @@ -226,11 +239,16 @@ def test_the_refusal_says_how_to_recover(self, store): message = str(error.value) assert "close_process" in message - assert "proc-1" in message # the id it is told to close - assert "target0" in message # and what is actually open + assert "proc-1" in message # every open session is listed + assert "target0" in message assert "pid 100" in message + # No "close the oldest": the oldest is usually the target the whole + # session has been refining, and closing it discards its scans too. + assert "oldest" not in message + # The scan count is what makes the list actionable. + assert "scan" in message - def test_closing_one_frees_a_slot(self, store): + def test_closing_one_frees_a_slot(self, store, all_alive): ids = [store.open(FakeProcess(), 1, "a").session_id for _ in range(MAX_OPEN_SESSIONS)] @@ -240,7 +258,7 @@ def test_closing_one_frees_a_slot(self, store): assert reopened.session_id not in ids assert len(store.sessions) == MAX_OPEN_SESSIONS - def test_ids_keep_climbing_after_a_close(self, store): + def test_ids_keep_climbing_after_a_close(self, store, all_alive): """Reusing an id would let a model holding a stale one address a different process.""" first = store.open(FakeProcess(), 1, "a").session_id @@ -250,6 +268,100 @@ def test_ids_keep_climbing_after_a_close(self, store): assert first != second +class TestDeadSessionsDoNotHoldSlots: + """The lockout the cap created, and the reason it evicts here but nowhere + else. + + Nothing in this server notices a target exiting: `_rows_for` swallows the + read error, `process_info` keeps answering from cached state, and the + session stays in the store. With a cap that refuses rather than evicts, + eight exited processes held every slot for the life of the server and the + model had no way to tell which sessions were the dead ones. + + Evicting a *dead* session is the exception that proves the rule about not + evicting: its handle is already useless, so nothing is taken away. + """ + + def test_a_dead_session_is_reaped_to_make_room(self, store, monkeypatch): + for index in range(MAX_OPEN_SESSIONS): + store.open(FakeProcess(pid=500 + index), 500 + index, "gone%d" % index) + + monkeypatch.setattr(session_module, "pid_exists", lambda pid: pid == 4242) + + # Would have raised before the reaper existed. + session = store.open(FakeProcess(pid=4242), 4242, "live") + + assert session.pid == 4242 + assert [held.pid for held in store.sessions] == [4242], ( + "the eight dead sessions should be gone, not merely joined" + ) + + def test_a_live_session_is_never_reaped(self, store, monkeypatch): + store.open(FakeProcess(pid=4242), 4242, "live") + for index in range(MAX_OPEN_SESSIONS - 1): + store.open(FakeProcess(pid=600 + index), 600 + index, "gone%d" % index) + + monkeypatch.setattr(session_module, "pid_exists", lambda pid: pid == 4242) + store.open(FakeProcess(pid=4243), 4243, "second") + + pids = sorted(s.pid for s in store.sessions) + assert 4242 in pids, "a live session was evicted" + + def test_reaping_closes_the_handle(self, store, monkeypatch): + process = FakeProcess(pid=700) + store.open(process, 700, "gone") + monkeypatch.setattr(session_module, "pid_exists", lambda pid: False) + + store._reap_dead() + + assert process.closed is True, "the handle was dropped without closing" + assert store.sessions == () + + @pytest.mark.parametrize("pid", [2 ** 31, 2 ** 64, -3, 0]) + def test_a_pid_the_os_refuses_reads_as_dead_not_as_a_crash(self, store, pid): + """The reaper runs inside `open`, so anything it raises escapes as + something other than SessionError and the model sees only "Error + executing tool". + + `pid_exists(2**31)` raises OverflowError — the value does not fit the + platform's pid type — which the argument fuzzer found by generating + exactly that. Intermittently, because it also needed the store to be + at capacity at that moment. + """ + from PyMemoryEditor.mcp.session import _looks_alive + + assert _looks_alive(pid) is False + + for index in range(MAX_OPEN_SESSIONS): + store.open(FakeProcess(pid=pid), pid, "weird%d" % index) + + # Must not raise anything but SessionError, and here it makes room. + session = store.open(FakeProcess(pid=pid), pid, "another") + assert session.pid == pid + + def test_a_full_server_of_live_targets_still_refuses(self, store, all_alive): + """The reaper must not become a back door around the cap.""" + for index in range(MAX_OPEN_SESSIONS): + store.open(FakeProcess(pid=800 + index), 800 + index, "live%d" % index) + + with pytest.raises(SessionError) as error: + store.open(FakeProcess(pid=900), 900, "another") + + assert "all of them are live" in str(error.value) + + def test_the_refusal_points_at_an_already_open_pid(self, store, all_alive): + """A model that lost count re-attaches instead of reusing the id.""" + store.open(FakeProcess(pid=4242), 4242, "faketarget") + for index in range(MAX_OPEN_SESSIONS - 1): + store.open(FakeProcess(pid=810 + index), 810 + index, "other%d" % index) + + with pytest.raises(SessionError) as error: + store.open(FakeProcess(pid=4242), 4242, "faketarget") + + message = str(error.value) + assert "already open as proc-1" in message + + class TestBatchRegions: def _regions(self, sizes): address = 0x1000 diff --git a/tests/mcp_server/test_toolset.py b/tests/mcp_server/test_toolset.py index 5ddbb7c..cd0af79 100644 --- a/tests/mcp_server/test_toolset.py +++ b/tests/mcp_server/test_toolset.py @@ -1215,8 +1215,13 @@ def test_a_refused_attach_does_not_leak_the_handle( ): """`attach` opens the handle and *then* registers the session, so a refusal must close it or the cap leaks what it exists to bound.""" + from PyMemoryEditor.mcp import session as session_module from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS + # The store reaps sessions whose pid is gone, and a fake's pid is an + # arbitrary number, so the cap would never fire here otherwise. + monkeypatch.setattr(session_module, "pid_exists", lambda pid: True) + opened = [] def fake_open(**_kwargs): From aa222ac9c60a5accb16ccfca60c79621d1380732 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 19:36:28 -0300 Subject: [PATCH 06/10] fix(mcp): render one capacity refusal, and never as an empty target name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fifth Copilot finding, on the early refusal I added two commits ago. Both halves correct. **It could name an empty target.** `_name_for_pid` returns `""` for a pid missing from the process listing — verified — and the elicitation prompt directly below already guards with `or "unknown"` for exactly that. Mine did not, so it rendered `attach to "" (pid 4242)`. **And it was the less useful of two messages.** `SessionStore.open` lists every open session with its scan count; my early version said only "close one with close_process first". The early path is the one a model actually reaches, so the duplication had made the common case worse. Fixed by removing the duplication rather than copying the text: `SessionStore.capacity_refusal(pid)` renders the message once, returns `None` when there is room, and both callers use it — `open` raises it as the authoritative guard, the protocol layer raises it before prompting. `at_capacity()` is now a thin wrapper over it. Verified by reverting the `or "unknown"`: the new test fails. --- PyMemoryEditor/mcp/server.py | 14 +++-- PyMemoryEditor/mcp/session.py | 97 ++++++++++++++++++--------------- tests/mcp_server/test_server.py | 26 +++++++++ 3 files changed, 87 insertions(+), 50 deletions(-) diff --git a/PyMemoryEditor/mcp/server.py b/PyMemoryEditor/mcp/server.py index 9539802..3b6ddfb 100644 --- a/PyMemoryEditor/mcp/server.py +++ b/PyMemoryEditor/mcp/server.py @@ -27,7 +27,7 @@ from .. import __version__ from .config import ServerConfig, parse_args -from .session import MAX_OPEN_SESSIONS, SessionError, host_platform +from .session import SessionError, host_platform from .toolset import MemoryToolset, ToolError if TYPE_CHECKING: # pragma: no cover - import cost avoided at runtime @@ -243,11 +243,15 @@ async def open_process(pid: int = 0, name: str = "", *, ctx: Context) -> Any: # user was asked to approve an attach, approved it, and then got told # the server was full. An approval is the most expensive step here and # the one thing this server must not spend carelessly. - if toolset.store.at_capacity(): + # + # The store renders the message, rather than this having its own: the + # first version here named neither the open sessions nor their scan + # counts, so the path a model actually hits was the less useful one. + refusal = toolset.store.capacity_refusal(found_pid) + if refusal is not None: raise SdkToolError( - "Not asking to attach to \"%s\" (pid %d): this server already " - "has %d processes open, which is the limit. Close one with " - "close_process first." % (found_name, found_pid, MAX_OPEN_SESSIONS) + 'Not asking to attach to "%s" (pid %d): %s' + % (found_name or "unknown", found_pid, refusal) ) try: diff --git a/PyMemoryEditor/mcp/session.py b/PyMemoryEditor/mcp/session.py index 7a59844..5c89dbc 100644 --- a/PyMemoryEditor/mcp/session.py +++ b/PyMemoryEditor/mcp/session.py @@ -206,42 +206,11 @@ def open(self, process: AbstractProcess, pid: int, name: str) -> Session: :raises SessionError: if :data:`MAX_OPEN_SESSIONS` are already open. The caller owns the handle it passed in and must close it. """ - if len(self._sessions) >= MAX_OPEN_SESSIONS: - self._reap_dead() + refusal = self.capacity_refusal(pid) + if refusal is not None: + raise SessionError(refusal) with self._lock: - if len(self._sessions) >= MAX_OPEN_SESSIONS: - # No "close the oldest": the oldest is usually the target the - # whole session has been refining, and close_process discards - # its scan results too. The list is given instead, so the - # choice is made on what each session holds. - existing = [ - sid for sid, s in self._sessions.items() if s.pid == pid - ] - already = ( - " Note that pid %d is already open as %s — reuse that id " - "rather than attaching again." % (pid, existing[0]) - if existing - else "" - ) - raise SessionError( - "This server already has %d processes open, which is the " - "limit, and all of them are live. Close whichever you are " - "done with (close_process also discards that session's " - "scan results) and attach again. Open: %s.%s" - % ( - MAX_OPEN_SESSIONS, - ", ".join( - "%s (%s, pid %d, %d scan%s)" - % (sid, session.name or "?", session.pid, - len(session.scan_ids), - "" if len(session.scan_ids) == 1 else "s") - for sid, session in self._sessions.items() - ), - already, - ) - ) - session_id = "proc-%d" % next(self._session_ids) session = Session( session_id=session_id, process=process, pid=pid, name=name @@ -282,26 +251,64 @@ def is_alive(self, session_id: str) -> bool: """Whether this session's target still exists.""" return _looks_alive(self.get(session_id).pid) - def at_capacity(self) -> bool: - """Whether :meth:`open` would refuse right now. + def capacity_refusal(self, pid: int = 0) -> Optional[str]: + """The reason :meth:`open` would refuse right now, or ``None``. - Lets a caller fail before doing something expensive that the refusal - would waste — the protocol layer asks this before prompting the user, - since spending an approval on an attach that cannot happen is worse - than refusing outright. :meth:`open` stays the authority: this is a - hint, and the two can differ under a concurrent open. + One message, rendered once. The protocol layer asks this *before* + prompting the user — spending an approval on an attach that cannot + happen is worse than refusing outright — and :meth:`open` raises it as + the authoritative guard. Duplicating the text gave the early path a + worse message than the late one, which is the path a model actually + hits. - Reaps first, or this would report a server full of exited processes as - full when `open` would have made room. + Reaps first, or a server full of exited processes reports as full when + :meth:`open` would have made room. """ with self._lock: if len(self._sessions) < MAX_OPEN_SESSIONS: - return False + return None self._reap_dead() with self._lock: - return len(self._sessions) >= MAX_OPEN_SESSIONS + if len(self._sessions) < MAX_OPEN_SESSIONS: + return None + + # No "close the oldest": the oldest is usually the target the whole + # session has been refining, and close_process discards its scan + # results too. The list is given instead, so the choice is made on + # what each session holds. + existing = [ + sid for sid, session in self._sessions.items() + if session.pid == pid + ] + already = ( + " Note that pid %d is already open as %s — reuse that id " + "rather than attaching again." % (pid, existing[0]) + if existing + else "" + ) + return ( + "This server already has %d processes open, which is the " + "limit, and all of them are live. Close whichever you are " + "done with (close_process also discards that session's scan " + "results) and attach again. Open: %s.%s" + % ( + MAX_OPEN_SESSIONS, + ", ".join( + "%s (%s, pid %d, %d scan%s)" + % (sid, session.name or "?", session.pid, + len(session.scan_ids), + "" if len(session.scan_ids) == 1 else "s") + for sid, session in self._sessions.items() + ), + already, + ) + ) + + def at_capacity(self) -> bool: + """Whether :meth:`open` would refuse right now.""" + return self.capacity_refusal() is not None def get(self, session_id: str) -> Session: """Look up a session, or explain how to obtain a valid id.""" diff --git a/tests/mcp_server/test_server.py b/tests/mcp_server/test_server.py index 6181082..58e56e8 100644 --- a/tests/mcp_server/test_server.py +++ b/tests/mcp_server/test_server.py @@ -482,6 +482,32 @@ async def run(): text = asyncio.run(run()).content[0].text assert "faketarget" in text and "4242" in text assert str(MAX_OPEN_SESSIONS) in text + # The early path renders the store's message, so it is as actionable + # as the late one: every open session, with its scan count. + assert "proc-1" in text and "scan" in text + + def test_an_unnamed_target_is_not_refused_as_an_empty_string(self, monkeypatch): + """`_name_for_pid` returns "" for a pid missing from the listing. + + The elicitation prompt below this already guards with `or "unknown"`; + the refusal did not, so it rendered `attach to "" (pid 4242)`. + """ + from PyMemoryEditor.mcp import session as session_module + from PyMemoryEditor.mcp.session import MAX_OPEN_SESSIONS + + monkeypatch.setattr(session_module, "pid_exists", lambda pid: True) + server, toolset, _process = self._build(ServerConfig()) + monkeypatch.setattr(toolset, "_name_for_pid", lambda pid: "") + for _ in range(MAX_OPEN_SESSIONS): + toolset.store.open(FakeProcess(), 4242, "faketarget") + + async def run(): + async with connected(server, Answerer("accept")) as session: + return await session.call_tool("open_process", {"pid": 4242}) + + text = asyncio.run(run()).content[0].text + assert 'attach to ""' not in text + assert "unknown" in text def test_cancelling_the_dialog_is_not_approval(self): result, toolset = self._open(ServerConfig(), Answerer("cancel")) From 8ee803bff6716f33cd641e0829cb2d520c692e6b Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 19:43:41 -0300 Subject: [PATCH 07/10] fix(mcp): guard server_info's liveness call, and make the cap atomic again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects, one reported and one found reviewing. **`server_info` could crash on an impossible pid.** Reported by Copilot. It rendered `alive` with a bare `pid_exists`, which raises `OverflowError` for a pid outside the platform's range — the same call I had just guarded in the store, written unguarded one file over. Worse there than in the reaper: `server_info` is the tool the instructions tell the model to call first, so a single odd session hid the limits and the policy too, not just the session list. `looks_alive` is now shared rather than duplicated, and the only remaining raw `pid_exists` is inside it. This also explains an instrumentation result that had looked wrong: patching `session.pid_exists` counted zero calls from `server_info`, because `toolset.py` imported the function separately. Two modules, two imports, one guarded. **And the cap stopped being atomic.** Found reviewing my own last commit: the `capacity_refusal` refactor split the check and the insert across two acquisitions of the store's lock, so every racing thread saw room and then every thread inserted. Demonstrated at 12 sessions against a cap of 8 by widening the window; it does not reproduce on its own, which is why the test that guards it injects the same pause — the plain concurrency test passes on the broken implementation, verified by mutation. `_refusal_locked` renders the message with the lock already held, so `open` checks and inserts under one acquisition and reaps outside it. Also drops `SessionStore.is_alive`, which I added in the previous commit and never called. --- PyMemoryEditor/mcp/session.py | 97 ++++++++++++++++++-------------- PyMemoryEditor/mcp/toolset.py | 10 +++- tests/mcp_server/test_session.py | 79 +++++++++++++++++++++++++- tests/mcp_server/test_toolset.py | 21 +++++++ 4 files changed, 162 insertions(+), 45 deletions(-) diff --git a/PyMemoryEditor/mcp/session.py b/PyMemoryEditor/mcp/session.py index 5c89dbc..88445b6 100644 --- a/PyMemoryEditor/mcp/session.py +++ b/PyMemoryEditor/mcp/session.py @@ -169,7 +169,7 @@ def scan_ids(self) -> Tuple[str, ...]: return tuple(self._scan_order) -def _looks_alive(pid: int) -> bool: +def looks_alive(pid: int) -> bool: """``pid_exists``, but a pid the OS will not even accept reads as dead. The reaper runs inside ``open``, so anything raised here escapes as @@ -206,11 +206,18 @@ def open(self, process: AbstractProcess, pid: int, name: str) -> Session: :raises SessionError: if :data:`MAX_OPEN_SESSIONS` are already open. The caller owns the handle it passed in and must close it. """ - refusal = self.capacity_refusal(pid) - if refusal is not None: - raise SessionError(refusal) + with self._lock: + crowded = len(self._sessions) >= MAX_OPEN_SESSIONS + + if crowded: + # Outside the lock: reaping closes handles, which takes it again. + self._reap_dead() with self._lock: + refusal = self._refusal_locked(pid) + if refusal is not None: + raise SessionError(refusal) + session_id = "proc-%d" % next(self._session_ids) session = Session( session_id=session_id, process=process, pid=pid, name=name @@ -237,7 +244,7 @@ def _reap_dead(self) -> List[str]: dead = [ session_id for session_id, session in self._sessions.items() - if not _looks_alive(session.pid) + if not looks_alive(session.pid) ] for session_id in dead: @@ -247,10 +254,6 @@ def _reap_dead(self) -> List[str]: pass return dead - def is_alive(self, session_id: str) -> bool: - """Whether this session's target still exists.""" - return _looks_alive(self.get(session_id).pid) - def capacity_refusal(self, pid: int = 0) -> Optional[str]: """The reason :meth:`open` would refuse right now, or ``None``. @@ -271,40 +274,51 @@ def capacity_refusal(self, pid: int = 0) -> Optional[str]: self._reap_dead() with self._lock: - if len(self._sessions) < MAX_OPEN_SESSIONS: - return None + return self._refusal_locked(pid) - # No "close the oldest": the oldest is usually the target the whole - # session has been refining, and close_process discards its scan - # results too. The list is given instead, so the choice is made on - # what each session holds. - existing = [ - sid for sid, session in self._sessions.items() - if session.pid == pid - ] - already = ( - " Note that pid %d is already open as %s — reuse that id " - "rather than attaching again." % (pid, existing[0]) - if existing - else "" - ) - return ( - "This server already has %d processes open, which is the " - "limit, and all of them are live. Close whichever you are " - "done with (close_process also discards that session's scan " - "results) and attach again. Open: %s.%s" - % ( - MAX_OPEN_SESSIONS, - ", ".join( - "%s (%s, pid %d, %d scan%s)" - % (sid, session.name or "?", session.pid, - len(session.scan_ids), - "" if len(session.scan_ids) == 1 else "s") - for sid, session in self._sessions.items() - ), - already, - ) + def _refusal_locked(self, pid: int) -> Optional[str]: + """Render the refusal, or ``None``. **Caller holds** ``self._lock``. + + Separate from :meth:`capacity_refusal` so :meth:`open` can check and + insert under one acquisition. When the check released the lock and the + insert took it again, the cap could be exceeded: every thread saw room, + then every thread inserted. Demonstrated with 12 concurrent opens + against a cap of 8. + """ + if len(self._sessions) < MAX_OPEN_SESSIONS: + return None + + # No "close the oldest": the oldest is usually the target the whole + # session has been refining, and close_process discards its scan + # results too. The list is given instead, so the choice is made on + # what each session holds. + existing = [ + sid for sid, session in self._sessions.items() + if session.pid == pid + ] + already = ( + " Note that pid %d is already open as %s — reuse that id " + "rather than attaching again." % (pid, existing[0]) + if existing + else "" + ) + return ( + "This server already has %d processes open, which is the " + "limit, and all of them are live. Close whichever you are " + "done with (close_process also discards that session's scan " + "results) and attach again. Open: %s.%s" + % ( + MAX_OPEN_SESSIONS, + ", ".join( + "%s (%s, pid %d, %d scan%s)" + % (sid, session.name or "?", session.pid, + len(session.scan_ids), + "" if len(session.scan_ids) == 1 else "s") + for sid, session in self._sessions.items() + ), + already, ) + ) def at_capacity(self) -> bool: """Whether :meth:`open` would refuse right now.""" @@ -501,6 +515,7 @@ def host_platform() -> str: __all__ = ( "MAX_OPEN_SESSIONS", + "looks_alive", "MAX_SCANS_PER_SESSION", "ScanResult", "Session", diff --git a/PyMemoryEditor/mcp/toolset.py b/PyMemoryEditor/mcp/toolset.py index 686006f..e7facc0 100644 --- a/PyMemoryEditor/mcp/toolset.py +++ b/PyMemoryEditor/mcp/toolset.py @@ -38,7 +38,7 @@ from ..process.abstract import AbstractProcess from ..process.errors import PyMemoryEditorError from ..process.region import MemoryRegion -from ..process.util import get_process_ids_by_name, iter_processes, pid_exists +from ..process.util import get_process_ids_by_name, iter_processes from ..util import ( decode_scan_target, resolve_bufflength, @@ -56,6 +56,7 @@ SessionStore, batch_regions, host_platform, + looks_alive, region_to_dict, ) @@ -567,7 +568,12 @@ def server_info(self) -> Dict[str, Any]: "pid": session.pid, "name": session.name, "scan_ids": list(session.scan_ids), - "alive": pid_exists(session.pid), + # `looks_alive`, not `pid_exists`: the raw call raises + # OverflowError for a pid outside the platform's range, and + # server_info is the tool the instructions tell the model to + # call first — a crash here hides the limits and the policy + # too, not just this field. + "alive": looks_alive(session.pid), } for session in self.store.sessions ] diff --git a/tests/mcp_server/test_session.py b/tests/mcp_server/test_session.py index 6dc4e15..ea195c1 100644 --- a/tests/mcp_server/test_session.py +++ b/tests/mcp_server/test_session.py @@ -268,6 +268,81 @@ def test_ids_keep_climbing_after_a_close(self, store, all_alive): assert first != second +class TestTheCapHoldsUnderConcurrency: + """Tool calls run on a thread pool, so `open` races with itself. + + A refactor split the capacity check and the insert across two acquisitions + of the store's lock: every thread saw room, then every thread inserted. + The window is a few bytecodes wide, so it does not reproduce on its own — + hence the injected pause, which makes a structural defect observable + instead of waiting for luck. + """ + + def test_the_cap_is_not_exceeded_when_opens_race(self, store, all_alive): + import threading + + start = threading.Barrier(MAX_OPEN_SESSIONS * 2) + + def attach(): + start.wait() + try: + store.open(FakeProcess(), 1, "a") + except SessionError: + pass + + threads = [ + threading.Thread(target=attach) + for _ in range(MAX_OPEN_SESSIONS * 2) + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert len(store.sessions) == MAX_OPEN_SESSIONS + + def test_the_cap_holds_even_with_the_window_stretched( + self, store, all_alive, monkeypatch + ): + """The same property with the race window made wide enough to lose. + + Without this, the test above passes on a broken implementation: the + gap between check and insert is too narrow for the scheduler to land + in reliably. + """ + import threading + import time + + original = type(store).capacity_refusal + + def slow(self, pid=0): + result = original(self, pid) + time.sleep(0.02) + return result + + monkeypatch.setattr(type(store), "capacity_refusal", slow) + + start = threading.Barrier(MAX_OPEN_SESSIONS + 4) + + def attach(): + start.wait() + try: + store.open(FakeProcess(), 1, "a") + except SessionError: + pass + + threads = [ + threading.Thread(target=attach) + for _ in range(MAX_OPEN_SESSIONS + 4) + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert len(store.sessions) == MAX_OPEN_SESSIONS + + class TestDeadSessionsDoNotHoldSlots: """The lockout the cap created, and the reason it evicts here but nowhere else. @@ -328,9 +403,9 @@ def test_a_pid_the_os_refuses_reads_as_dead_not_as_a_crash(self, store, pid): exactly that. Intermittently, because it also needed the store to be at capacity at that moment. """ - from PyMemoryEditor.mcp.session import _looks_alive + from PyMemoryEditor.mcp.session import looks_alive - assert _looks_alive(pid) is False + assert looks_alive(pid) is False for index in range(MAX_OPEN_SESSIONS): store.open(FakeProcess(pid=pid), pid, "weird%d" % index) diff --git a/tests/mcp_server/test_toolset.py b/tests/mcp_server/test_toolset.py index cd0af79..9aa0f1e 100644 --- a/tests/mcp_server/test_toolset.py +++ b/tests/mcp_server/test_toolset.py @@ -1210,6 +1210,27 @@ def test_max_offset_is_still_clamped_at_the_top(self, toolset, session): # --- the one address no parser ever sees --------------------------- # + def test_server_info_survives_a_session_with_an_impossible_pid( + self, make_toolset + ): + """`server_info` is the tool the model is told to call first. + + It rendered `alive` with a bare `pid_exists`, which raises + OverflowError for a pid outside the platform's range — so one odd + session took the whole result down, hiding the limits and the policy + as well as the session list. The store had already been guarded for + the same call; this file had not. + """ + toolset = make_toolset(config()) + toolset.store.open(FakeProcess(pid=2 ** 31), 2 ** 31, "impossible") + toolset.store.open(FakeProcess(pid=1), 1, "ordinary") + + sessions = toolset.server_info()["open_sessions"] + + by_pid = {entry["pid"]: entry["alive"] for entry in sessions} + assert by_pid[2 ** 31] is False + assert by_pid[1] is True + def test_a_refused_attach_does_not_leak_the_handle( self, make_toolset, monkeypatch ): From b8ccb27851b2eb206a1a3834f620edd3a6bbde87 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 19:51:11 -0300 Subject: [PATCH 08/10] fix(test): use the test's own pid as the live one, not pid 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught this on Windows alone: `pid_exists(1)` is False there — Windows has no pid 1 (System Idle is 0, System is 4) — so `test_server_info_survives_a_session_with_an_impossible_pid` failed on `assert by_pid[1] is True`. `os.getpid()` is alive by definition on every platform. The commit two before this one said, in its own message, that several cap tests "had been passing only because `pid_exists(1)` is true on a Unix host". I then used pid 1 as the live case in a new test in the same PR. Fourth Windows-only assumption here after the 32 767-character environment variable, `monotonic()`'s ~15.6 ms tick, and the path separator. Verified on Linux in a container as well as locally: 724 passed on both. --- tests/mcp_server/test_toolset.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/mcp_server/test_toolset.py b/tests/mcp_server/test_toolset.py index 9aa0f1e..91c1ff1 100644 --- a/tests/mcp_server/test_toolset.py +++ b/tests/mcp_server/test_toolset.py @@ -1221,15 +1221,23 @@ def test_server_info_survives_a_session_with_an_impossible_pid( as well as the session list. The store had already been guarded for the same call; this file had not. """ + # `os.getpid()`, not pid 1: Windows has no pid 1 (System Idle is 0, + # System is 4), so `pid_exists(1)` is False there and this test failed + # on that runner alone. The test's own process is alive by definition + # everywhere. + import os + + alive_pid = os.getpid() + toolset = make_toolset(config()) toolset.store.open(FakeProcess(pid=2 ** 31), 2 ** 31, "impossible") - toolset.store.open(FakeProcess(pid=1), 1, "ordinary") + toolset.store.open(FakeProcess(pid=alive_pid), alive_pid, "ordinary") sessions = toolset.server_info()["open_sessions"] by_pid = {entry["pid"]: entry["alive"] for entry in sessions} assert by_pid[2 ** 31] is False - assert by_pid[1] is True + assert by_pid[alive_pid] is True def test_a_refused_attach_does_not_leak_the_handle( self, make_toolset, monkeypatch From 046012e86226a83ed9492cb47814f56fc7f5b496 Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 20:01:39 -0300 Subject: [PATCH 09/10] fix(mcp): never refuse without reaping, and actually guard the atomicity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more Copilot findings on the previous commit, both correct. **`open` could refuse without reaping.** It decided whether to reap from a check taken under an earlier acquisition of the lock, so if the store filled between the two it refused with a slot held by an exited process that nobody had looked at. It now loops: judge under the lock, insert if there is room, otherwise reap once and judge again. At most one reap — a second pass finds nothing new, and refusing has to terminate. **And the test guarding the atomicity fix was guarding nothing.** It patched `capacity_refusal` and slept in it to widen the race, but `open` calls `_refusal_locked` — verified by making `capacity_refusal` raise and watching `open` succeed. The sleep never fired. It passed its mutation check only because that mutant happened to reintroduce the `capacity_refusal` call, so the test was validating one historical edit rather than the property. Replaced with two that hold: * `_refusal_locked` asserts the lock is held, so a naked call fails deterministically instead of when the scheduler cooperates. * Acquisitions are counted across one `open`: one for the correct implementation, two when the check releases the lock before inserting. The assertion alone was not enough, and I checked rather than assumed — splitting check from insert still passed it, because each half held the lock on its own. The counting test is what fails on that. 726 tests, green on Linux in a container as well. --- PyMemoryEditor/mcp/session.py | 44 ++++++++---- tests/mcp_server/test_session.py | 118 +++++++++++++++++++++++-------- 2 files changed, 117 insertions(+), 45 deletions(-) diff --git a/PyMemoryEditor/mcp/session.py b/PyMemoryEditor/mcp/session.py index 88445b6..f07ca3c 100644 --- a/PyMemoryEditor/mcp/session.py +++ b/PyMemoryEditor/mcp/session.py @@ -206,24 +206,32 @@ def open(self, process: AbstractProcess, pid: int, name: str) -> Session: :raises SessionError: if :data:`MAX_OPEN_SESSIONS` are already open. The caller owns the handle it passed in and must close it. """ - with self._lock: - crowded = len(self._sessions) >= MAX_OPEN_SESSIONS + # Loop, rather than deciding to reap from a check taken earlier under + # a different acquisition: the store can fill between the two, and then + # a slot held by an exited process would be refused without anyone + # having looked. At most one reap — a second pass finds nothing new, + # and refusing has to terminate. + reaped = False + + while True: + with self._lock: + refusal = self._refusal_locked(pid) + + if refusal is None: + session_id = "proc-%d" % next(self._session_ids) + session = Session( + session_id=session_id, process=process, + pid=pid, name=name, + ) + self._sessions[session_id] = session + return session + + if reaped: + raise SessionError(refusal) - if crowded: # Outside the lock: reaping closes handles, which takes it again. self._reap_dead() - - with self._lock: - refusal = self._refusal_locked(pid) - if refusal is not None: - raise SessionError(refusal) - - session_id = "proc-%d" % next(self._session_ids) - session = Session( - session_id=session_id, process=process, pid=pid, name=name - ) - self._sessions[session_id] = session - return session + reaped = True def _reap_dead(self) -> List[str]: """Drop sessions whose target no longer exists. Caller holds no lock. @@ -285,6 +293,12 @@ def _refusal_locked(self, pid: int) -> Optional[str]: then every thread inserted. Demonstrated with 12 concurrent opens against a cap of 8. """ + # Deterministic, so a refactor that checks capacity outside the lock + # fails here instead of in a timing test. That regression happened + # once: the check and the insert were split across two acquisitions, + # every racing thread saw room, and the cap was exceeded. + assert self._lock.locked(), "capacity must be judged under the lock" + if len(self._sessions) < MAX_OPEN_SESSIONS: return None diff --git a/tests/mcp_server/test_session.py b/tests/mcp_server/test_session.py index ea195c1..ec09efc 100644 --- a/tests/mcp_server/test_session.py +++ b/tests/mcp_server/test_session.py @@ -10,6 +10,7 @@ from PyMemoryEditor.mcp.session import ( MAX_OPEN_SESSIONS, MAX_SCANS_PER_SESSION, + Session, SessionError, SessionStore, batch_regions, @@ -301,46 +302,103 @@ def attach(): assert len(store.sessions) == MAX_OPEN_SESSIONS - def test_the_cap_holds_even_with_the_window_stretched( - self, store, all_alive, monkeypatch - ): - """The same property with the race window made wide enough to lose. + def test_capacity_is_judged_while_the_lock_is_held(self, store, all_alive): + """The property the timing test could not actually probe. + + A previous version of this test patched `capacity_refusal` and slept + in it to widen the race — but `open` calls `_refusal_locked`, not + `capacity_refusal`, so the sleep never fired and the test only + duplicated the one above. It passed its mutation check because that + mutant happened to reintroduce the `capacity_refusal` call. + + Asserted structurally instead: `_refusal_locked` refuses to judge + capacity unless the lock is held, so splitting the check from the + insert fails deterministically rather than when the scheduler + cooperates. + """ + for _ in range(MAX_OPEN_SESSIONS): + store.open(FakeProcess(), 1, "a") + + # Held: this is how `open` calls it, and it answers. + with store._lock: + assert store._refusal_locked(0) is not None + + # Not held: it refuses to answer at all. + with pytest.raises(AssertionError): + store._refusal_locked(0) - Without this, the test above passes on a broken implementation: the - gap between check and insert is too narrow for the scheduler to land - in reliably. + def test_open_judges_and_inserts_under_one_acquisition(self, all_alive): + """The assertion above is not enough on its own. + + It only proves capacity is judged under *an* acquisition, not that the + same one covers the insert — a version that judged under its own lock, + released, and then took the lock again to insert passed it, and that is + exactly the regression this guards. Counting acquisitions separates + them: one for the correct implementation, two for the split. """ import threading - import time - original = type(store).capacity_refusal + class CountingLock: + def __init__(self): + self._real = threading.Lock() + self.acquisitions = 0 - def slow(self, pid=0): - result = original(self, pid) - time.sleep(0.02) - return result + def __enter__(self): + self.acquisitions += 1 + return self._real.__enter__() - monkeypatch.setattr(type(store), "capacity_refusal", slow) + def __exit__(self, *exc): + return self._real.__exit__(*exc) - start = threading.Barrier(MAX_OPEN_SESSIONS + 4) + def locked(self): + return self._real.locked() - def attach(): - start.wait() - try: - store.open(FakeProcess(), 1, "a") - except SessionError: - pass + store = SessionStore() + lock = CountingLock() + store._lock = lock - threads = [ - threading.Thread(target=attach) - for _ in range(MAX_OPEN_SESSIONS + 4) - ] - for thread in threads: - thread.start() - for thread in threads: - thread.join() + store.open(FakeProcess(), 1, "a") - assert len(store.sessions) == MAX_OPEN_SESSIONS + assert lock.acquisitions == 1, ( + "capacity and the insert must share one acquisition; %d means the " + "check released the lock before inserting" % lock.acquisitions + ) + + def test_a_dead_session_is_reaped_even_if_the_store_fills_meanwhile( + self, store, monkeypatch + ): + """`open` used to decide whether to reap from a check taken under an + earlier acquisition of the lock. If the store filled in between, it + refused without anyone having looked for a dead session. + + Simulated by filling the store from inside the first check, which is + the same ordering a concurrent open produces. + """ + alive = {4242} + monkeypatch.setattr(session_module, "pid_exists", lambda pid: pid in alive) + + store.open(FakeProcess(pid=4242), 4242, "live") + + original = type(store)._refusal_locked + calls = [] + + def fill_once(self, pid): + if not calls: + calls.append(pid) + for index in range(MAX_OPEN_SESSIONS - 1): + session_id = "proc-%d" % next(self._session_ids) + self._sessions[session_id] = Session( + session_id=session_id, process=FakeProcess(pid=700 + index), + pid=700 + index, name="gone%d" % index, + ) + return original(self, pid) + + monkeypatch.setattr(type(store), "_refusal_locked", fill_once) + + # Full at the moment of the check, but seven of the eight are dead. + session = store.open(FakeProcess(pid=4243), 4243, "second") + + assert session.pid == 4243 class TestDeadSessionsDoNotHoldSlots: From 6b937dca9547e56e36a55c8449967fb5f64ab28f Mon Sep 17 00:00:00 2001 From: JeanExtreme002 Date: Tue, 8 Sep 2026 20:09:37 -0300 Subject: [PATCH 10/10] fix(mcp): raise the capacity refusal inside the critical section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot again, on the loop from the previous commit, and right. `open` computed the refusal under the lock and raised it after the block, so another thread could `close()` a session in that gap and this caller would be turned away with a message that was already false. The cap was never violated — this is a spurious refusal, not a breach — but the fix is moving two lines up into the `with`, so there is no reason to keep the window. Tested by recording whether the lock is held when the error is constructed, which is the property rather than a timing guess. Reverting the move fails it. --- PyMemoryEditor/mcp/session.py | 8 ++++++-- tests/mcp_server/test_session.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/PyMemoryEditor/mcp/session.py b/PyMemoryEditor/mcp/session.py index f07ca3c..ed80846 100644 --- a/PyMemoryEditor/mcp/session.py +++ b/PyMemoryEditor/mcp/session.py @@ -226,8 +226,12 @@ def open(self, process: AbstractProcess, pid: int, name: str) -> Session: self._sessions[session_id] = session return session - if reaped: - raise SessionError(refusal) + if reaped: + # Raised here, not after the block: computing the refusal + # under the lock and raising it outside leaves a window + # where another thread closes a session, and this caller + # is turned away with a message that is already false. + raise SessionError(refusal) # Outside the lock: reaping closes handles, which takes it again. self._reap_dead() diff --git a/tests/mcp_server/test_session.py b/tests/mcp_server/test_session.py index ec09efc..89117db 100644 --- a/tests/mcp_server/test_session.py +++ b/tests/mcp_server/test_session.py @@ -327,6 +327,37 @@ def test_capacity_is_judged_while_the_lock_is_held(self, store, all_alive): with pytest.raises(AssertionError): store._refusal_locked(0) + def test_the_refusal_is_raised_inside_the_critical_section( + self, store, all_alive, monkeypatch + ): + """Deciding under the lock and raising outside it leaves a window. + + Another thread can close a session in that gap, so the caller is turned + away with a message that is already false — the store has room by the + time it reads it. Probed by recording whether the lock was held at the + moment the error was constructed. + """ + for _ in range(MAX_OPEN_SESSIONS): + store.open(FakeProcess(), 1, "a") + + held = [] + original = session_module.SessionError + + class Probe(original): # type: ignore[misc, valid-type] + def __init__(self, *args): + held.append(store._lock.locked()) + super().__init__(*args) + + monkeypatch.setattr(session_module, "SessionError", Probe) + + with pytest.raises(original): + store.open(FakeProcess(), 2, "b") + + assert held == [True], ( + "the refusal was built after the lock was released, so the store " + "may already have had room" + ) + def test_open_judges_and_inserts_under_one_acquisition(self, all_alive): """The assertion above is not enough on its own.