Skip to content

Commit b584700

Browse files
nikomatsakisnikomatsakis
committed
fix: Catch handler errors instead of killing the connection (#131)
Instead of propagating handler errors via ? (which tears down the connection), catch them in dispatch_dispatch and report them back to the remote side: - Requests: send a JSON-RPC error response with the original request id - Notifications: send an out-of-band error notification - The connection stays alive for subsequent messages This is an alternative to the match_request/match_notification API approach. Rather than changing the handler API, errors are caught at the dispatch level — like a web server catching unhandled exceptions. Also migrates parse_message impls from json_cast to json_cast_params so deserialization failures produce -32602 (Invalid params) instead of -32700 (Parse error), which is the correct JSON-RPC error code.
1 parent edfba74 commit b584700

7 files changed

Lines changed: 430 additions & 26 deletions

File tree

‎src/agent-client-protocol-core/src/jsonrpc/incoming_actor.rs‎

Lines changed: 64 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,13 @@ use crate::RoleId;
1313
use crate::UntypedMessage;
1414
use crate::jsonrpc::ConnectionTo;
1515
use crate::jsonrpc::HandleDispatchFrom;
16+
use crate::jsonrpc::OutgoingMessage;
1617
use crate::jsonrpc::ReplyMessage;
1718
use crate::jsonrpc::Responder;
1819
use crate::jsonrpc::ResponseRouter;
1920
use crate::jsonrpc::dynamic_handler::DynHandleDispatchFrom;
2021
use crate::jsonrpc::dynamic_handler::DynamicHandlerMessage;
22+
use crate::jsonrpc::outgoing_actor::send_raw_message;
2123

2224
use crate::role::Role;
2325

@@ -93,20 +95,25 @@ pub(super) async fn incoming_protocol_actor<Counterpart: Role>(
9395
let mut new_pending_messages = vec![];
9496
for pending_message in pending_messages {
9597
tracing::trace!(method = pending_message.method(), handler = ?handler.dyn_describe_chain(), "Retrying message");
98+
let id = pending_message.id();
9699
match handler
97100
.dyn_handle_dispatch_from(pending_message, connection.clone())
98-
.await?
101+
.await
99102
{
100-
Handled::Yes => {
103+
Ok(Handled::Yes) => {
101104
tracing::trace!("Message handled");
102105
}
103-
Handled::No {
106+
Ok(Handled::No {
104107
message: m,
105108
retry: _,
106-
} => {
109+
}) => {
107110
tracing::trace!(method = m.method(), handler = ?handler.dyn_describe_chain(), "Message not handled");
108111
new_pending_messages.push(m);
109112
}
113+
Err(err) => {
114+
tracing::warn!(?err, handler = ?handler.dyn_describe_chain(), "Dynamic handler errored on pending message, reporting back");
115+
report_handler_error(connection, id, err)?;
116+
}
110117
}
111118
}
112119
pending_messages = new_pending_messages;
@@ -253,55 +260,69 @@ async fn dispatch_dispatch<Counterpart: Role>(
253260
tracing::trace!(handler = ?handler.describe_chain(), "Attempting handler chain");
254261
match handler
255262
.handle_dispatch_from(dispatch, connection.clone())
256-
.await?
263+
.await
257264
{
258-
Handled::Yes => {
265+
Ok(Handled::Yes) => {
259266
tracing::trace!(?method, ?id, handler = ?handler.describe_chain(), "Handler accepted message");
260267
return Ok(());
261268
}
262269

263-
Handled::No { message: m, retry } => {
270+
Ok(Handled::No { message: m, retry }) => {
264271
tracing::trace!(?method, ?id, handler = ?handler.describe_chain(), "Handler declined message");
265272
dispatch = m;
266273
retry_any |= retry;
267274
}
275+
276+
Err(err) => {
277+
tracing::warn!(?method, ?id, ?err, handler = ?handler.describe_chain(), "Handler errored, reporting back to remote");
278+
return report_handler_error(connection, id, err);
279+
}
268280
}
269281

270282
// Next, apply any dynamic handlers.
271283
for dynamic_handler in dynamic_handlers.values_mut() {
272284
tracing::trace!(handler = ?dynamic_handler.dyn_describe_chain(), "Attempting dynamic handler");
273285
match dynamic_handler
274286
.dyn_handle_dispatch_from(dispatch, connection.clone())
275-
.await?
287+
.await
276288
{
277-
Handled::Yes => {
289+
Ok(Handled::Yes) => {
278290
tracing::trace!(?method, ?id, handler = ?dynamic_handler.dyn_describe_chain(), "Dynamic handler accepted message");
279291
return Ok(());
280292
}
281293

282-
Handled::No { message: m, retry } => {
294+
Ok(Handled::No { message: m, retry }) => {
283295
tracing::trace!(?method, ?id, handler = ?dynamic_handler.dyn_describe_chain(), "Dynamic handler declined message");
284296
retry_any |= retry;
285297
dispatch = m;
286298
}
299+
300+
Err(err) => {
301+
tracing::warn!(?method, ?id, ?err, handler = ?dynamic_handler.dyn_describe_chain(), "Dynamic handler errored, reporting back to remote");
302+
return report_handler_error(connection, id, err);
303+
}
287304
}
288305
}
289306

290307
// Finally, apply the default handler for the role.
291308
tracing::trace!(role = ?counterpart, "Attempting default handler");
292309
match counterpart
293310
.default_handle_dispatch_from(dispatch, connection.clone())
294-
.await?
311+
.await
295312
{
296-
Handled::Yes => {
313+
Ok(Handled::Yes) => {
297314
tracing::trace!(?method, handler = "default", "Role accepted message");
298315
return Ok(());
299316
}
300-
Handled::No { message: m, retry } => {
317+
Ok(Handled::No { message: m, retry }) => {
301318
tracing::trace!(?method, handler = "default", "Role declined message");
302319
dispatch = m;
303320
retry_any |= retry;
304321
}
322+
Err(err) => {
323+
tracing::warn!(?method, ?id, ?err, handler = "default", "Default handler errored, reporting back to remote");
324+
return report_handler_error(connection, id, err);
325+
}
305326
}
306327

307328
// If the message was never handled, check whether the retry flag was set.
@@ -330,3 +351,33 @@ async fn dispatch_dispatch<Counterpart: Role>(
330351
}
331352
}
332353
}
354+
355+
/// When a handler returns an error, report it back to the remote side instead
356+
/// of propagating it and tearing down the connection.
357+
///
358+
/// For requests (which have an id), sends a JSON-RPC error response.
359+
/// For notifications (no id), sends an out-of-band error notification.
360+
/// For responses, forwards the error to the local awaiter.
361+
fn report_handler_error<Counterpart: Role>(
362+
connection: &ConnectionTo<Counterpart>,
363+
id: Option<serde_json::Value>,
364+
error: crate::Error,
365+
) -> Result<(), crate::Error> {
366+
match id {
367+
Some(id) => {
368+
// Request: send error response with the original request id
369+
let jsonrpc_id = serde_json::from_value(id).unwrap_or(jsonrpcmsg::Id::Null);
370+
send_raw_message(
371+
&connection.message_tx,
372+
OutgoingMessage::Response {
373+
id: jsonrpc_id,
374+
response: Err(error),
375+
},
376+
)
377+
}
378+
None => {
379+
// Notification or response without id: send error notification
380+
connection.send_error_notification(error)
381+
}
382+
}
383+
}

‎src/agent-client-protocol-core/src/schema/mod.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ macro_rules! impl_jsonrpc_request {
3636
if method != $method {
3737
return Err($crate::Error::method_not_found());
3838
}
39-
$crate::util::json_cast(params)
39+
$crate::util::json_cast_params(params)
4040
}
4141
}
4242

@@ -84,7 +84,7 @@ macro_rules! impl_jsonrpc_notification {
8484
if method != $method {
8585
return Err($crate::Error::method_not_found());
8686
}
87-
$crate::util::json_cast(params)
87+
$crate::util::json_cast_params(params)
8888
}
8989
}
9090

@@ -133,10 +133,10 @@ macro_rules! impl_jsonrpc_request_enum {
133133
params: &impl serde::Serialize,
134134
) -> Result<Self, $crate::Error> {
135135
match method {
136-
$( $(#[$meta])* $method => $crate::util::json_cast(params).map(Self::$variant), )*
136+
$( $(#[$meta])* $method => $crate::util::json_cast_params(params).map(Self::$variant), )*
137137
_ => {
138138
if let Some(custom_method) = method.strip_prefix('_') {
139-
$crate::util::json_cast(params).map(
139+
$crate::util::json_cast_params(params).map(
140140
|ext_req: $crate::schema::ExtRequest| {
141141
Self::$ext_variant($crate::schema::ExtRequest::new(
142142
custom_method.to_string(),
@@ -196,10 +196,10 @@ macro_rules! impl_jsonrpc_notification_enum {
196196
params: &impl serde::Serialize,
197197
) -> Result<Self, $crate::Error> {
198198
match method {
199-
$( $(#[$meta])* $method => $crate::util::json_cast(params).map(Self::$variant), )*
199+
$( $(#[$meta])* $method => $crate::util::json_cast_params(params).map(Self::$variant), )*
200200
_ => {
201201
if let Some(custom_method) = method.strip_prefix('_') {
202-
$crate::util::json_cast(params).map(
202+
$crate::util::json_cast_params(params).map(
203203
|ext_notif: $crate::schema::ExtNotification| {
204204
Self::$ext_variant($crate::schema::ExtNotification::new(
205205
custom_method.to_string(),

‎src/agent-client-protocol-core/src/schema/proxy_protocol.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ impl<M: JsonRpcMessage> JsonRpcMessage for SuccessorMessage<M> {
5050
if method != METHOD_SUCCESSOR_MESSAGE {
5151
return Err(crate::Error::method_not_found());
5252
}
53-
let outer = crate::util::json_cast::<_, SuccessorMessage<UntypedMessage>>(params)?;
53+
let outer = crate::util::json_cast_params::<_, SuccessorMessage<UntypedMessage>>(params)?;
5454
if !M::matches_method(&outer.message.method) {
5555
return Err(crate::Error::method_not_found());
5656
}
@@ -161,7 +161,7 @@ impl<M: JsonRpcMessage> JsonRpcMessage for McpOverAcpMessage<M> {
161161
if method != METHOD_MCP_MESSAGE {
162162
return Err(crate::Error::method_not_found());
163163
}
164-
let outer = crate::util::json_cast::<_, McpOverAcpMessage<UntypedMessage>>(params)?;
164+
let outer = crate::util::json_cast_params::<_, McpOverAcpMessage<UntypedMessage>>(params)?;
165165
if !M::matches_method(&outer.message.method) {
166166
return Err(crate::Error::method_not_found());
167167
}

‎src/agent-client-protocol-core/src/util.rs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,33 @@ where
3030
Ok(m)
3131
}
3232

33+
/// Cast incoming request/notification params into a typed payload.
34+
///
35+
/// Like [`json_cast`], but deserialization failures become
36+
/// [`Error::invalid_params`](`crate::Error::invalid_params`) (`-32602`)
37+
/// instead of a parse error, which is the correct JSON-RPC error code for
38+
/// malformed method parameters.
39+
pub fn json_cast_params<N, M>(params: N) -> Result<M, crate::Error>
40+
where
41+
N: serde::Serialize,
42+
M: serde::de::DeserializeOwned,
43+
{
44+
let json = serde_json::to_value(params).map_err(|e| {
45+
crate::Error::internal_error().data(serde_json::json!({
46+
"error": e.to_string(),
47+
"phase": "serialization"
48+
}))
49+
})?;
50+
let m = serde_json::from_value(json.clone()).map_err(|e| {
51+
crate::Error::invalid_params().data(serde_json::json!({
52+
"error": e.to_string(),
53+
"json": json,
54+
"phase": "deserialization"
55+
}))
56+
})?;
57+
Ok(m)
58+
}
59+
3360
/// Creates an internal error with the given message
3461
pub fn internal_error(message: impl ToString) -> crate::Error {
3562
crate::Error::internal_error().data(message.to_string())

0 commit comments

Comments
 (0)