Skip to content

Commit 9e768fd

Browse files
glenrgordonclaude
andcommitted
fix: answer the peer when a response payload cannot be encoded
A response payload is attributed to a request id before it is encoded, and json.dumps runs inside transport.send. So any payload the serialiser refuses makes the TypeError escape the response task, no reply is ever written, and the peer waits on that id forever. Two routes reach it today with no hostile input at all: - a handler returning a raw value that is not JSON-native, e.g. {"x": {1, 2}}; a BaseModel result is safe because model_dump(mode="json") coerces, but a raw return value was assigned unchecked; - a handler raising RequestError(..., data=...), whose data is typed Any and passed straight through to the error object. Gate the finished payload once, at the end of _execute_request, with the same json.dumps the transports call. Pydantic's to_jsonable_python would not be a faithful gate here: it coerces a set or bytes rather than rejecting them, so it would pass payloads json.dumps then refuses. The check is deliberately not at the send. A transport failure means the peer is unreachable and propagating it unanswered is correct, which test_response_send_failure_is_not_mapped_to_a_handler_error asserts; an encoding failure means the peer is reachable and the payload is our fault. Catching around transport.send would conflate the two and would convert a transport implementation's own TypeError into a protocol error reported to the peer. That file is untouched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 9853d8b commit 9e768fd

2 files changed

Lines changed: 81 additions & 0 deletions

File tree

‎src/acp/connection.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,16 @@ async def _execute_request(self, message: dict[str, Any]) -> dict[str, Any]:
283283
except Exception:
284284
data = {"details": str(exc)}
285285
payload["error"] = RequestError.internal_error(data).to_error_obj()
286+
# One gate for every payload shape. The response is already attributed to a
287+
# request id, and ``json.dumps`` is what each transport calls, so anything it
288+
# refuses here would otherwise strand the peer inside ``transport.send``, where
289+
# the failure escapes the response task with no handler left to answer.
290+
try:
291+
json.dumps(payload)
292+
except Exception as exc:
293+
logging.exception("Response for method=%s could not be encoded", method, exc_info=exc)
294+
payload.pop("result", None)
295+
payload["error"] = RequestError.internal_error({"details": str(exc)}).to_error_obj()
286296
return payload
287297

288298
async def _run_notification(self, message: dict[str, Any]) -> None:

‎tests/test_rpc.py‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -975,3 +975,74 @@ async def test_connection_init_under_eager_task_factory(server):
975975
await asyncio.sleep(0)
976976
assert conn._disconnected is False
977977
await conn.close()
978+
979+
980+
@pytest.mark.asyncio
981+
async def test_unserialisable_raw_result_answers_the_peer(server):
982+
# Regression: a handler returning a raw (non-BaseModel) value that json.dumps cannot
983+
# encode used to build a payload that died in the transport, so no response line was
984+
# ever written and the peer waited on the request id forever.
985+
async def handler(method: str, params: Any, is_notification: bool) -> Any:
986+
return {"x": {1, 2}}
987+
988+
conn = Connection(handler, server.client_writer, server.client_reader)
989+
990+
req = {"jsonrpc": "2.0", "id": 1, "method": "custom/thing", "params": {}}
991+
server.server_writer.write((json.dumps(req) + "\n").encode())
992+
await server.server_writer.drain()
993+
994+
line = await asyncio.wait_for(server.server_reader.readline(), timeout=1)
995+
resp = json.loads(line)
996+
assert resp["id"] == 1
997+
assert resp["error"]["code"] == -32603
998+
details = resp["error"]["data"]["details"]
999+
assert "set" in details and "JSON serializable" in details
1000+
1001+
await conn.close()
1002+
1003+
1004+
@pytest.mark.asyncio
1005+
async def test_base_model_result_with_a_set_is_coerced_and_still_answers(server):
1006+
# Pins why the BaseModel branch above needs no representability check:
1007+
# model_dump(mode="json") already coerces an Any-typed set into a list. If that ever
1008+
# stops being true, this test says so instead of the check silently becoming too narrow.
1009+
async def handler(method: str, params: Any, is_notification: bool) -> Any:
1010+
return AuthenticateResponse(field_meta={"tags": {1, 2}})
1011+
1012+
conn = Connection(handler, server.client_writer, server.client_reader)
1013+
1014+
req = {"jsonrpc": "2.0", "id": 2, "method": "authenticate", "params": {}}
1015+
server.server_writer.write((json.dumps(req) + "\n").encode())
1016+
await server.server_writer.drain()
1017+
1018+
line = await asyncio.wait_for(server.server_reader.readline(), timeout=1)
1019+
resp = json.loads(line)
1020+
assert resp["id"] == 2
1021+
assert "error" not in resp
1022+
assert sorted(resp["result"]["_meta"]["tags"]) == [1, 2]
1023+
1024+
await conn.close()
1025+
1026+
1027+
@pytest.mark.asyncio
1028+
async def test_unserialisable_request_error_data_answers_the_peer(server):
1029+
# Regression: ``RequestError.data`` is typed ``Any``, so a handler raising a structured
1030+
# protocol error whose data json.dumps cannot encode used to build a payload that died in
1031+
# the transport, leaving no response line and the peer waiting on the request id forever.
1032+
async def handler(method: str, params: Any, is_notification: bool) -> Any:
1033+
raise RequestError(-32000, "boom", data={"x": {1, 2}})
1034+
1035+
conn = Connection(handler, server.client_writer, server.client_reader)
1036+
1037+
req = {"jsonrpc": "2.0", "id": 3, "method": "custom/thing", "params": {}}
1038+
server.server_writer.write((json.dumps(req) + "\n").encode())
1039+
await server.server_writer.drain()
1040+
1041+
line = await asyncio.wait_for(server.server_reader.readline(), timeout=1)
1042+
resp = json.loads(line)
1043+
assert resp["id"] == 3
1044+
assert resp["error"]["code"] == -32603
1045+
details = resp["error"]["data"]["details"]
1046+
assert "set" in details and "JSON serializable" in details
1047+
1048+
await conn.close()

0 commit comments

Comments
 (0)