diff --git a/backends/com_backend.py b/backends/com_backend.py index ea075f3..fee04bc 100644 --- a/backends/com_backend.py +++ b/backends/com_backend.py @@ -4368,6 +4368,22 @@ class ComBackend(AutoCADBackend): def _sync(): doc = _acad_doc() + # `_UNDO _M` below is typed at the command line; with a command or + # prompt already active it would be swallowed as that command's + # input and no mark would exist for the rollback to go back to. + try: + cmd_active = int(doc.GetVariable("CMDACTIVE")) + except Exception as exc: + log.debug("CMDACTIVE read failed, proceeding anyway: %s", exc) + cmd_active = 0 + if cmd_active: + return { + "ok": False, + "error": ( + f"AutoCAD has an active command or prompt (CMDACTIVE={cmd_active}); " + "press ESC in AutoCAD before transaction_begin." + ), + } # R32: record the mark from inside the worker, immediately before # setting it. `_transaction_active = True` used to run only after # `_run` returned, so a timed-out begin left StartUndoMark open in @@ -4376,10 +4392,22 @@ class ComBackend(AutoCADBackend): # exact inverse of the R16 fix transaction_commit already carries. # The abandoned STA thread cannot be cancelled, so the flag has to # be written where the call happens; and it is written *before* - # StartUndoMark because a call that hangs inside it may still land. + # the marks because a call that hangs inside them may still land. # A `_acad_doc()` that fails leaves the flag alone, which is right: # nothing reached AutoCAD. self._transaction_active = True + # Two marks, deliberately. StartUndoMark/EndUndoMark form an undo + # *group* (one UNDO step), not an UNDO Mark: on the live AutoCAD + # 2026 `_UNDO B` with no Mark answers "This will undo everything. + # OK? " and SendCommand blocks at that prompt until a human + # presses ESC (measured: rollback timed out at 60 s, the entities + # stayed, every COM client was rejected meanwhile). The Mark is set + # *before* the group so `_UNDO _B` in transaction_rollback lands + # exactly at the transaction's start — also when the group is + # empty, where `_UNDO 1` would be one step too many. Measured on + # AutoCAD 2026: Mark + group + 2 entities + `_UNDO _B` -> entities + # gone, CMDACTIVE 0, 47 ms; empty group -> nothing else undone. + doc.SendCommand("_.UNDO _M\n") doc.StartUndoMark() return {"ok": True, "message": "Transaction begun (AutoCAD undo mark set)"} @@ -4399,12 +4427,21 @@ class ComBackend(AutoCADBackend): self._transaction_active = False async def transaction_rollback(self) -> dict: + # Without a transaction there is no Mark to go back to, and `_UNDO _B` + # would then prompt "This will undo everything. OK? " and block the + # STA worker at it (see transaction_begin). Refuse the same way the + # headless engine does. + if not self._transaction_active: + return {"ok": False, "error": "No active transaction to rollback"} + def _sync(): doc = _acad_doc() doc.EndUndoMark() - # R16: route UNDO B through the CMDACTIVE-aware sender instead of a raw - # SendCommand that could deadlock if a command/prompt is active. - self._safe_send_command(doc, "_UNDO B") + # R16: route UNDO Back through the CMDACTIVE-aware sender instead of + # a raw SendCommand that could deadlock if a command/prompt is active. + # Back to the Mark transaction_begin set before the group, so this + # never prompts and never undoes anything older than the transaction. + self._safe_send_command(doc, "_.UNDO _B") return {"ok": True, "message": "Transaction rolled back"} try: diff --git a/engineering/pid/spec.py b/engineering/pid/spec.py index da335a6..c864eb2 100644 --- a/engineering/pid/spec.py +++ b/engineering/pid/spec.py @@ -3,6 +3,7 @@ from __future__ import annotations import copy +import logging import math from typing import TYPE_CHECKING @@ -18,6 +19,8 @@ from .symbols import resolve, transform_port if TYPE_CHECKING: from backends.base import AutoCADBackend +log = logging.getLogger(__name__) + EXAMPLE_SPEC: dict = { "sheet": { "layer_set": "pid", @@ -440,7 +443,7 @@ async def run_spec(backend: AutoCADBackend, spec: dict, dry_run: bool = False) - except ValueError as exc: raise ValueError(f"{where}: {exc}") from exc crossings_total += drawn["crossings"] - except BaseException: + except BaseException as exc: # ``Exception`` alone let a client cancellation (``CancelledError`` is # a ``BaseException``) leave the placed symbols in the drawing inside # an open transaction — on COM with the undo mark still open, so every @@ -455,8 +458,32 @@ async def run_spec(backend: AutoCADBackend, spec: dict, dry_run: bool = False) - # transaction, and on COM the undo mark left open while the backend's # flag already said "no transaction". A native ``Task.cancel()`` is # delivered once and clears, so it was never the case that mattered. + # + # Once the rollback body runs after a cancellation it can also *fail* + # (the COM hard timeout, ``_safe_send_command``'s own deadline, an + # ezdxf quarantine refusal). That failure must not replace the + # cancellation: the SDK has already answered the cancelled request, + # and only a cancellation exception is suppressed by its handler — an + # ordinary error makes it respond a second time, which trips + # ``assert not self._completed`` in ``mcp.shared.session`` and kills + # the whole session (measured through the real transport). So after a + # cancellation the rollback error is logged and the cancellation is + # what propagates; after any other error the rollback error propagates + # as before, with the original chained as its context. + cancelled = isinstance(exc, anyio.get_cancelled_exc_class()) with anyio.CancelScope(shield=True): - await backend.transaction_rollback() + try: + await backend.transaction_rollback() + except Exception as rollback_exc: + if not cancelled: + raise + log.error( + "pid_from_spec: rollback after a cancelled request failed (%s: %s); " + "the sheet may be half-drawn - check system_status and " + "transaction_rollback before retrying", + type(rollback_exc).__name__, + rollback_exc, + ) raise await backend.transaction_commit() graph = await build_graph(backend) diff --git a/server.py b/server.py index cec1907..7b14e81 100644 --- a/server.py +++ b/server.py @@ -3988,7 +3988,8 @@ _BATCH_GUARANTEE_NOTES = { "whole document to a temporary DXF, so it scales with drawing size." ), "best_effort_undo": ( - "Rollback ends the AutoCAD undo mark and sends '_UNDO B' to the command line. " + "Rollback ends the AutoCAD undo group and sends '_UNDO _B' (back to the UNDO " + "Mark transaction_begin set) to the command line. " "AutoCAD executes that asynchronously and does not confirm it landed, so this " "is best-effort, NOT atomic. The before/after fingerprint below is the only " "evidence available; treat a mismatch as 'the undo has not landed (yet)' and " @@ -4994,7 +4995,8 @@ async def transaction_commit(ctx: Context = None) -> dict: async def transaction_rollback(ctx: Context = None) -> dict: """Rollback the current transaction to the point of transaction_begin. - COM: Undoes all operations back to the last undo mark. + COM: Undoes all operations back to the UNDO Mark transaction_begin set; + refused (`ok: false`) when no transaction is active. ezdxf: Restores the document from the saved DXF snapshot. WARNING: This is destructive – all changes since transaction_begin are lost. @@ -6839,7 +6841,11 @@ async def pid_from_spec( drawn port-to-port, inside one transaction: any bad item rolls the whole sheet back and the error names it (`lines[2].to`); so does an interruption (a client cancellation mid-run leaves no half-drawn sheet and no open - transaction). A non-finite coordinate, rotation, scale or stub is refused + transaction on either engine — the live one goes back to the UNDO Mark + `transaction_begin` set, so it never waits at an AutoCAD prompt; should + that rollback itself fail, the failure is logged, the cancellation is what + the client sees, and the drawing must be checked before a retry). + A non-finite coordinate, rotation, scale or stub is refused by path before anything is placed. The response carries the graph read back from the drawing and the P&ID critique issues, so the caller sees dangling ends or duplicate tags in the same round trip. `dry_run` returns diff --git a/tests/test_cad_batch.py b/tests/test_cad_batch.py index 815ec61..1278d2e 100644 --- a/tests/test_cad_batch.py +++ b/tests/test_cad_batch.py @@ -33,7 +33,7 @@ the happy path is the easy half: classifier must recover the real cause from the ``__cause__`` chain and never from the message text. * **Atomicity honesty.** ezdxf rolls back by restoring a full DXF snapshot; - COM ends an undo mark and fires ``_UNDO B`` at AutoCAD without ever + COM ends an undo group and fires ``_UNDO _B`` at AutoCAD without ever confirming it landed. The payload must say which one it got, and must not claim a rollback it did not verify. * **dry_run.** Validates against each tool's real JSON Schema without @@ -736,7 +736,7 @@ async def test_a_snapshot_backend_is_reported_as_a_snapshot_guarantee(): async def test_an_undo_mark_backend_is_never_called_atomic(): - """COM ends the undo mark and fires ``_UNDO B`` without confirming it landed.""" + """COM ends the undo group and fires ``_UNDO _B`` without confirming it landed.""" guarantee, note = server._batch_rollback_guarantee(_Caps("undo_mark")) assert guarantee == "best_effort_undo" lowered = note.lower() diff --git a/tests/test_com_sprint3.py b/tests/test_com_sprint3.py index 743ff11..a2cd458 100644 --- a/tests/test_com_sprint3.py +++ b/tests/test_com_sprint3.py @@ -101,7 +101,7 @@ async def test_run_lisp_captures_value_from_users1(monkeypatch): async def test_transaction_begin_sets_flag_and_marks_undo(monkeypatch): doc = _Doc() - doc.StartUndoMark = MagicMock() + doc.StartUndoMark = MagicMock(side_effect=lambda: doc.sent.append("StartUndoMark")) monkeypatch.setattr(cb, "_acad_doc", lambda: doc) b = _backend_no_executor() @@ -109,6 +109,25 @@ async def test_transaction_begin_sets_flag_and_marks_undo(monkeypatch): assert out["ok"] is True assert b._transaction_active is True doc.StartUndoMark.assert_called_once() + # The UNDO Mark goes in *before* the undo group: `_UNDO _B` in the rollback + # returns to it, and a Mark inside the group would be undone with it. + assert doc.sent == ["_.UNDO _M\n", "StartUndoMark"], doc.sent + + +async def test_transaction_begin_refuses_while_a_prompt_is_active(monkeypatch): + """With CMDACTIVE set, SendCommand would feed `_UNDO _M` to the active + command as input and no Mark would exist for the rollback to reach.""" + doc = _Doc() + doc.SetVariable("CMDACTIVE", 1) + doc.StartUndoMark = MagicMock() + monkeypatch.setattr(cb, "_acad_doc", lambda: doc) + + b = _backend_no_executor() + out = await b.transaction_begin() + assert out["ok"] is False and "CMDACTIVE" in out["error"] and "ESC" in out["error"] + assert b._transaction_active is False, "nothing reached AutoCAD" + assert doc.sent == [] + doc.StartUndoMark.assert_not_called() async def test_transaction_commit_clears_flag_on_error(): @@ -124,17 +143,63 @@ async def test_transaction_commit_clears_flag_on_error(): assert b._transaction_active is False # not left stale -async def test_transaction_rollback_uses_safe_send_command(monkeypatch): - doc = _Doc() # idle: CMDACTIVE=0 so _safe_send_command returns promptly +class _MarkingDoc(_Doc): + """An idle doc that keeps AutoCAD's UNDO Mark bookkeeping. + + On the live AutoCAD 2026 `_UNDO B` with no Mark answers "This will undo + everything. OK? " and `SendCommand` blocks at that prompt until a human + presses ESC (measured: the rollback hit the 60 s COM timeout, the entities + stayed, every COM client was rejected meanwhile). `StartUndoMark` / + `EndUndoMark` form an undo *group*, not a Mark. This fake answers the + unmarked Back the way the prompt would end up: as a failure, not a hang. + """ + + def __init__(self): + super().__init__() + self.marks = 0 + self.group_open = False + + def SendCommand(self, cmd): + super().SendCommand(cmd) + if cmd.strip() == "_.UNDO _M": + self.marks += 1 + elif cmd.strip() == "_.UNDO _B": + if self.marks == 0: + raise RuntimeError("prompt: This will undo everything. OK? ") + self.marks -= 1 + + def StartUndoMark(self): + self.group_open = True + + def EndUndoMark(self): + self.group_open = False + + +async def test_transaction_rollback_goes_back_to_the_mark_begin_set(monkeypatch): + doc = _MarkingDoc() # idle: CMDACTIVE=0 so _safe_send_command returns promptly monkeypatch.setattr(cb, "_acad_doc", lambda: doc) b = _backend_no_executor() - b._transaction_active = True + assert (await b.transaction_begin())["ok"] is True out = await b.transaction_rollback() assert out["ok"] is True assert b._transaction_active is False - # UNDO B went through _safe_send_command (which appends a trailing newline). - assert any("_UNDO B" in c for c in doc.sent) + # UNDO Back went through _safe_send_command (which appends a trailing + # newline) and found the Mark begin set: no prompt, group closed, Mark consumed. + assert [c.strip() for c in doc.sent] == ["_.UNDO _M", "_.UNDO _B"] + assert doc.marks == 0 and doc.group_open is False + + +async def test_transaction_rollback_without_a_transaction_is_refused(monkeypatch): + """No transaction means no Mark: an unmarked `_UNDO _B` is the prompt + that strands the STA worker, so it is never sent.""" + doc = _MarkingDoc() + monkeypatch.setattr(cb, "_acad_doc", lambda: doc) + + b = _backend_no_executor() + out = await b.transaction_rollback() + assert out == {"ok": False, "error": "No active transaction to rollback"} + assert doc.sent == [] # ── R13 — offset honors side_x/side_y and deletes the unused copy ─────────── diff --git a/tests/test_com_timeout.py b/tests/test_com_timeout.py index 46cd542..0368171 100644 --- a/tests/test_com_timeout.py +++ b/tests/test_com_timeout.py @@ -167,6 +167,12 @@ async def test_timed_out_transaction_begin_does_not_hide_an_open_undo_mark( marks = [] class _FakeDoc: + def GetVariable(self, name): + return 0 # CMDACTIVE clear + + def SendCommand(self, cmd): + marks.append(cmd.strip()) # the UNDO Mark begin sets first + def StartUndoMark(self): marks.append("start") try: @@ -185,7 +191,7 @@ async def test_timed_out_transaction_begin_does_not_hide_an_open_undo_mark( ) again = await com_backend.transaction_begin() assert again == {"ok": False, "error": "A transaction is already active"} - assert marks == ["start"], "the second begin must not nest another undo mark" + assert marks == ["_.UNDO _M", "start"], "the second begin must not nest another undo mark" finally: release.set() assert finished.wait(5) diff --git a/tests/test_pid_from_spec.py b/tests/test_pid_from_spec.py index 32247df..df9723b 100644 --- a/tests/test_pid_from_spec.py +++ b/tests/test_pid_from_spec.py @@ -226,10 +226,14 @@ async def test_a_cancellation_with_a_com_call_in_flight_still_closes_the_undo_ma With a COM call blocking the worker when the scope is cancelled, the unshielded rollback's ``_run`` was cancelled while queued behind it — so - ``EndUndoMark`` / ``_UNDO B`` never reached AutoCAD — yet its ``finally`` + ``EndUndoMark`` / ``_UNDO _B`` never reached AutoCAD — yet its ``finally`` cleared ``_transaction_active``: ``system_status`` said "no transaction" while the undo mark was open, and the next ``transaction_begin`` nested a second ``StartUndoMark``. + + The order pinned here is the one measured on the live AutoCAD 2026: the + UNDO *Mark* goes in before the undo group, so ``_UNDO _B`` lands at the + transaction's start instead of prompting "This will undo everything". """ import asyncio import time @@ -245,6 +249,8 @@ async def test_a_cancellation_with_a_com_call_in_flight_still_closes_the_undo_ma doc = types.SimpleNamespace( Name="Drawing1.dwg", FullName="", + GetVariable=lambda name: 0, # CMDACTIVE clear + SendCommand=lambda cmd: sent.append(cmd.strip()), StartUndoMark=lambda: sent.append("StartUndoMark"), EndUndoMark=lambda: sent.append("EndUndoMark"), ) @@ -280,10 +286,146 @@ async def test_a_cancellation_with_a_com_call_in_flight_still_closes_the_undo_ma await in_flight.wait() tg.cancel_scope.cancel() - assert sent == ["StartUndoMark", "EndUndoMark", "_UNDO B"], sent + assert sent == ["_.UNDO _M", "StartUndoMark", "EndUndoMark", "_.UNDO _B"], sent assert backend._transaction_active is False # The mark really is closed, so the next transaction is a fresh one. assert (await backend.transaction_begin())["ok"] is True - assert sent[-1] == "StartUndoMark" + assert sent[-2:] == ["_.UNDO _M", "StartUndoMark"] finally: backend._executor.shutdown(wait=True) + + +# ── a rollback that fails after a cancellation must not kill the session ───── + + +async def _cancel_mid_run(run, *, in_flight) -> None: + """Cancel ``run`` through an anyio scope — the SDK's mechanism — once it is + inside ``draw_line``; returns normally only if the cancellation itself is + what left the task (a task group swallows its own cancellation and + re-raises anything else).""" + import anyio + + async with anyio.create_task_group() as tg: + tg.start_soon(run) + await in_flight.wait() + tg.cancel_scope.cancel() + + +async def test_a_rollback_failure_after_a_cancellation_is_logged_not_raised( + backend, monkeypatch, caplog +): + """Now that the shielded rollback body runs after a real cancellation it + can fail (the COM hard timeout, ``_safe_send_command``'s deadline, an ezdxf + quarantine refusal). That failure must not replace the cancellation: the + SDK has already answered the cancelled request and suppresses only a + cancellation exception — an ordinary error makes it respond a second time + and ``assert not self._completed`` kills the whole session.""" + import asyncio + import logging + + from engineering.pid import spec as spec_module + + in_flight = asyncio.Event() + + async def slow_draw_line(*args, **kwargs): + in_flight.set() + await asyncio.sleep(30) + + async def failing_rollback(): + raise RuntimeError("AutoCAD did not respond within 60s") + + monkeypatch.setattr(spec_module, "draw_line", slow_draw_line) + monkeypatch.setattr(backend, "transaction_rollback", failing_rollback) + + with caplog.at_level(logging.ERROR, logger="engineering.pid.spec"): + # With the rollback error escaping, the task group would raise it here. + await _cancel_mid_run(lambda: run_spec(backend, EXAMPLE_SPEC), in_flight=in_flight) + + messages = [r.getMessage() for r in caplog.records if r.name == "engineering.pid.spec"] + assert any("did not respond within 60s" in m and "half-drawn" in m for m in messages), messages + monkeypatch.undo() + assert (await backend.transaction_rollback())["ok"] is True # the test's own cleanup + + +async def test_a_rollback_failure_after_an_ordinary_error_still_propagates(backend, monkeypatch): + """Only a cancellation is special. After a ``ValueError`` the rollback's + own failure is the error the caller gets, with the original chained.""" + from engineering.pid import spec as spec_module + + async def bad_place(backend, section, item): + raise ValueError("placement refused") + + async def failing_rollback(): + raise RuntimeError("AutoCAD did not respond within 60s") + + monkeypatch.setattr(spec_module, "_place", bad_place) + monkeypatch.setattr(backend, "transaction_rollback", failing_rollback) + + with pytest.raises(RuntimeError, match="did not respond") as exc: + await run_spec(backend, EXAMPLE_SPEC) + assert isinstance(exc.value.__context__, ValueError) + assert "equipment[0]: placement refused" in str(exc.value.__context__) + monkeypatch.undo() + assert (await backend.transaction_rollback())["ok"] is True + + +async def test_a_rollback_failure_after_a_client_cancellation_leaves_the_session_alive( + client, monkeypatch +): + """The same through the real transport: ``client.cancel(request id)`` + mid-run, the rollback raises, and the *next* request is still answered. + Before the fix the server tried to respond twice to the cancelled request, + the lowlevel server's ``assert not self._completed`` escaped its task + group, ``system_status`` was never answered and ``Client.__aexit__`` + re-raised the AssertionError.""" + import asyncio + + from fastmcp.exceptions import McpError + + from backends import ezdxf_backend + from engineering.pid import spec as spec_module + + async def slow_draw_line(*args, **kwargs): + await asyncio.sleep(30) + + real_rollback = ezdxf_backend.EzdxfBackend.transaction_rollback + fail = True + calls: list[str] = [] + + async def rollback(self): + calls.append("rollback") + if fail: + raise RuntimeError("simulated rollback failure") + return await real_rollback(self) + + monkeypatch.setattr(spec_module, "draw_line", slow_draw_line) + monkeypatch.setattr(ezdxf_backend.EzdxfBackend, "transaction_rollback", rollback) + + async def depth() -> int: + return (await client.call_tool("system_status", {})).structured_content["transaction_depth"] + + request_id = client.session._request_id + call = asyncio.ensure_future(client.call_tool("pid_from_spec", {"spec": EXAMPLE_SPEC})) + + async def call_sent() -> bool: + return client.session._request_id > request_id + + await _wait_until(call_sent) + await _wait_until(lambda: _is_depth(depth, 1)) + await client.cancel(request_id) + with pytest.raises(McpError, match="cancelled"): + await call + + async def rolled_back() -> bool: + return bool(calls) + + await _wait_until(rolled_back) + # The session is alive: the next request is answered. + assert await asyncio.wait_for(depth(), timeout=5) == 1, "the failed rollback left it open" + fail = False # the test's own cleanup, through the real rollback + assert (await client.call_tool("transaction_rollback", {})).structured_content["ok"] is True + assert await depth() == 0 + + +async def _is_depth(depth, wanted: int) -> bool: + return await depth() == wanted