Skip to content

Commit 17c2a51

Browse files
authored
Merge pull request #1486 from DeusData/fix/1375-oversized-mcp-response
fix(daemon): answer an oversized MCP reply with an error, not process death (#1375)
2 parents 28aaa39 + 5efa574 commit 17c2a51

3 files changed

Lines changed: 169 additions & 3 deletions

File tree

‎src/daemon/application.c‎

Lines changed: 86 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2340,6 +2340,70 @@ static cbm_daemon_runtime_application_status_t application_set_context(
23402340
return CBM_DAEMON_RUNTIME_APPLICATION_OK;
23412341
}
23422342

2343+
/* Build the JSON-RPC error that replaces a reply too large to frame.
2344+
*
2345+
* Says the two numbers a caller needs to act — what the reply measured and what
2346+
* fits — because the alternative the reporter lived through was a silent exit
2347+
* with nothing to go on. The advice is concrete for the same reason: "narrow the
2348+
* projection or add LIMIT" is actionable, "internal error" is not.
2349+
*
2350+
* `request` may be NULL when the message did not parse; the response then omits
2351+
* the id, which is what JSON-RPC requires for an unidentifiable request. */
2352+
static char *application_oversized_response_error(const cbm_jsonrpc_request_t *request,
2353+
size_t response_length) {
2354+
char message[CBM_SZ_256];
2355+
(void)snprintf(message, sizeof(message),
2356+
"response too large: %zu bytes exceeds the %u byte transport limit; "
2357+
"narrow the projection, add LIMIT, or paginate",
2358+
response_length, (unsigned)CBM_DAEMON_RUNTIME_APPLICATION_PAYLOAD_MAX);
2359+
2360+
yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL);
2361+
if (!doc) {
2362+
return NULL;
2363+
}
2364+
yyjson_mut_val *err = yyjson_mut_obj(doc);
2365+
yyjson_mut_doc_set_root(doc, err);
2366+
/* -32603 (internal error) is the closest standard code: the request was
2367+
* well-formed and the failure is server-side. */
2368+
yyjson_mut_obj_add_int(doc, err, "code", -32603);
2369+
yyjson_mut_obj_add_str(doc, err, "message", message);
2370+
char *error_json = yyjson_mut_write(doc, 0, NULL);
2371+
yyjson_mut_doc_free(doc);
2372+
if (!error_json) {
2373+
return NULL;
2374+
}
2375+
2376+
cbm_jsonrpc_response_t response = {
2377+
.id = request && request->has_id ? request->id : 0,
2378+
.id_str = request ? request->id_str : NULL,
2379+
.error_json = error_json,
2380+
.error_code = -32603,
2381+
};
2382+
char *encoded = cbm_jsonrpc_format_response(&response);
2383+
free(error_json);
2384+
return encoded;
2385+
}
2386+
2387+
/* Decide whether `response` can be framed, substituting the error when it
2388+
* cannot. Owns `response` either way and returns the reply to send, or NULL
2389+
* only on allocation failure. This is ONE function on purpose: the bug was the
2390+
* DECISION (an oversized reply travelling on as if it were fine), so the
2391+
* decision and the substitution have to be testable together — a test that
2392+
* only builds the error passes even with the size check removed. */
2393+
static char *application_framable_response(char *response, const cbm_jsonrpc_request_t *request) {
2394+
if (!response || strlen(response) <= CBM_DAEMON_RUNTIME_APPLICATION_PAYLOAD_MAX) {
2395+
return response;
2396+
}
2397+
char *replacement = application_oversized_response_error(request, strlen(response));
2398+
free(response);
2399+
return replacement;
2400+
}
2401+
2402+
char *cbm_daemon_application_framable_response_for_test(char *response,
2403+
const cbm_jsonrpc_request_t *request) {
2404+
return application_framable_response(response, request);
2405+
}
2406+
23432407
static cbm_daemon_runtime_application_status_t application_mcp_request(
23442408
cbm_daemon_application_session_t *session, const uint8_t *request, uint32_t request_length,
23452409
uint8_t **response_out, uint32_t *response_length_out) {
@@ -2365,18 +2429,37 @@ static cbm_daemon_runtime_application_status_t application_mcp_request(
23652429
} else if (tool_request && response) {
23662430
application_update_notice_inject(session, &response);
23672431
}
2368-
if (parsed_ok) {
2369-
cbm_jsonrpc_request_free(&parsed);
2370-
}
23712432
if (response) {
23722433
size_t response_length = strlen(response);
23732434
if (response_length > UINT32_MAX) {
2435+
if (parsed_ok) {
2436+
cbm_jsonrpc_request_free(&parsed);
2437+
}
23742438
free(response);
23752439
return CBM_DAEMON_RUNTIME_APPLICATION_HANDLER_ERROR;
23762440
}
2441+
/* A reply larger than one frame has to become a JSON-RPC error HERE,
2442+
* while the request id is still in hand. Handing it to the transport
2443+
* instead is indistinguishable from a dead socket at the frontend
2444+
* worker, which responds by _Exit()ing the process — right for a broken
2445+
* transport, a fatal over-reaction to a large answer. Under an MCP host
2446+
* that does not respawn the server, that took every tool on it out for
2447+
* the rest of the session, leaving exit=1 and an empty stderr to debug
2448+
* from (#1375). */
2449+
response = application_framable_response(response, parsed_ok ? &parsed : NULL);
2450+
if (!response) {
2451+
if (parsed_ok) {
2452+
cbm_jsonrpc_request_free(&parsed);
2453+
}
2454+
return CBM_DAEMON_RUNTIME_APPLICATION_HANDLER_ERROR;
2455+
}
2456+
response_length = strlen(response);
23772457
*response_out = (uint8_t *)response;
23782458
*response_length_out = (uint32_t)response_length;
23792459
}
2460+
if (parsed_ok) {
2461+
cbm_jsonrpc_request_free(&parsed);
2462+
}
23802463
application_refresh_watch(session);
23812464
return CBM_DAEMON_RUNTIME_APPLICATION_OK;
23822465
}

‎src/daemon/application_internal.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,10 @@
55
#define CBM_DAEMON_APPLICATION_INTERNAL_H
66

77
#include "daemon/application.h"
8+
#include "mcp/mcp.h" /* cbm_jsonrpc_request_t */
89

910
#include <stdbool.h>
11+
#include <stddef.h>
1012

1113
/* Read-only test diagnostic for a live session. The caller must serialize
1214
* this check with request, cancellation, and close callbacks. */
@@ -35,4 +37,12 @@ int cbm_daemon_application_background_initializes_for_test(void);
3537
* that a request QUEUED rather than erroring or starting. */
3638
int cbm_daemon_application_busy_queue_waits_for_test(void);
3739

40+
/* Build the JSON-RPC error substituted for a reply too large to frame (#1375).
41+
* Exposed because the alternative — driving a real >10 MiB reply — needs a
42+
* ~20k-node index, a fixture cost the unit suite should not carry. The
43+
* end-to-end behaviour is covered by the repro in the issue. Caller frees.
44+
* `request` may be NULL (unparseable message → no id echoed). */
45+
char *cbm_daemon_application_framable_response_for_test(char *response,
46+
const cbm_jsonrpc_request_t *request);
47+
3848
#endif /* CBM_DAEMON_APPLICATION_INTERNAL_H */

‎tests/test_daemon_application.c‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4870,7 +4870,80 @@ TEST(daemon_application_rejects_clean_exit_when_process_tree_is_not_contained) {
48704870
PASS();
48714871
}
48724872

4873+
/* #1375: a reply too large to frame must become a JSON-RPC ERROR, not a
4874+
* transport failure.
4875+
*
4876+
* Why this matters more than it looks: the frontend worker cannot tell a
4877+
* rejected oversized frame from a dead socket, and for a dead socket it
4878+
* deliberately _Exit()s the process (closing the kernel IPC handle is the only
4879+
* portable way to cancel daemon session ownership from a thread blocked in
4880+
* stdio). So passing an oversized reply DOWN to the transport killed the whole
4881+
* server — every tool gone for the rest of the session, exit=1, empty stderr.
4882+
* The substitution therefore has to happen here, at the layer that still holds
4883+
* the request id.
4884+
*
4885+
* Reproduced end-to-end before fixing, on a 20k-node fixture: LIMIT 10000 ->
4886+
* 8,529,990 bytes ok; LIMIT 20000 -> SERVER DIED exit=1. After: the same query
4887+
* returns this error and a follow-up query on the SAME session succeeds. */
4888+
TEST(daemon_application_oversized_reply_is_a_jsonrpc_error_not_a_death) {
4889+
cbm_jsonrpc_request_t request = {0};
4890+
request.has_id = true;
4891+
request.id = 42;
4892+
4893+
/* A reply that FITS must pass through byte-identical — the guard must not
4894+
* touch the overwhelming majority of replies. */
4895+
char *small = strdup("{\"jsonrpc\":\"2.0\",\"id\":42,\"result\":{}}");
4896+
ASSERT_NOT_NULL(small);
4897+
char *kept = cbm_daemon_application_framable_response_for_test(small, &request);
4898+
ASSERT_TRUE(kept == small); /* same pointer: untouched */
4899+
ASSERT_STR_EQ(kept, "{\"jsonrpc\":\"2.0\",\"id\":42,\"result\":{}}");
4900+
free(kept);
4901+
4902+
/* A reply that CANNOT be framed must come back as a JSON-RPC error instead.
4903+
* Passing it on is what killed the server: the frontend worker cannot tell a
4904+
* rejected oversized frame from a dead socket, and for a dead socket it
4905+
* deliberately _Exit()s the process — so every tool on that server was gone
4906+
* for the rest of the session, with exit=1 and an empty stderr (#1375).
4907+
* Reproduced end-to-end on a 20k-node fixture before fixing: LIMIT 10000 ->
4908+
* 8,529,990 bytes ok, LIMIT 20000 -> SERVER DIED. After: the same query
4909+
* returns this error and a follow-up query on the SAME session succeeds. */
4910+
size_t oversized = (size_t)CBM_DAEMON_RUNTIME_APPLICATION_PAYLOAD_MAX + 1U;
4911+
char *big = malloc(oversized + 1U);
4912+
ASSERT_NOT_NULL(big);
4913+
memset(big, 'x', oversized);
4914+
big[oversized] = '\0';
4915+
4916+
char *replaced = cbm_daemon_application_framable_response_for_test(big, &request);
4917+
ASSERT_NOT_NULL(replaced);
4918+
ASSERT_TRUE(replaced != big); /* substituted, not passed through */
4919+
4920+
/* The replacement must itself fit, or it reproduces the bug it replaces. */
4921+
ASSERT_TRUE(strlen(replaced) <= (size_t)CBM_DAEMON_RUNTIME_APPLICATION_PAYLOAD_MAX);
4922+
/* ...and carry the caller's id, or the client cannot match reply to request. */
4923+
ASSERT_NOT_NULL(strstr(replaced, "\"error\""));
4924+
ASSERT_NOT_NULL(strstr(replaced, "\"id\":42"));
4925+
ASSERT_NOT_NULL(strstr(replaced, "-32603"));
4926+
/* ...and say what happened and what to do: the failure it replaces was a
4927+
* SILENT exit, so an opaque "internal error" would be no improvement. */
4928+
ASSERT_NOT_NULL(strstr(replaced, "response too large"));
4929+
ASSERT_NOT_NULL(strstr(replaced, "LIMIT"));
4930+
free(replaced);
4931+
4932+
/* An unparseable message has no id to echo, but must still yield an error
4933+
* rather than NULL — NULL is turned back into a hard failure by the caller. */
4934+
char *big2 = malloc(oversized + 1U);
4935+
ASSERT_NOT_NULL(big2);
4936+
memset(big2, 'y', oversized);
4937+
big2[oversized] = '\0';
4938+
char *anonymous = cbm_daemon_application_framable_response_for_test(big2, NULL);
4939+
ASSERT_NOT_NULL(anonymous);
4940+
ASSERT_NOT_NULL(strstr(anonymous, "\"error\""));
4941+
free(anonymous);
4942+
PASS();
4943+
}
4944+
48734945
SUITE(daemon_application) {
4946+
RUN_TEST(daemon_application_oversized_reply_is_a_jsonrpc_error_not_a_death);
48744947
RUN_TEST(daemon_application_new_session_does_not_retain_initial_store);
48754948
RUN_TEST(daemon_application_request_cancel_is_scoped_to_exact_token);
48764949
RUN_TEST(daemon_application_requires_immutable_explicit_context);

0 commit comments

Comments
 (0)