From 5efa57492aa2bbe7f620342446029d6cf0f0b7b5 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Fri, 7 Aug 2026 15:28:08 +0200 Subject: [PATCH] fix(daemon): answer an oversized MCP reply with an error, not process death (#1375) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single query_graph reply larger than one 10 MiB frame killed the MCP server: exit code 1, no JSON-RPC error, nothing on stderr, no crash report. Under a host that does not respawn the server, every tool from it was gone for the rest of the session — the reporter lost their tooling mid-investigation to one dead-code query and finished by driving the binary over stdio by hand. Root cause is a conflation, not the limit itself. application_mcp_request checked only response_length > UINT32_MAX and passed anything smaller on, so an oversized reply reached the transport, which rejected the frame. The frontend worker cannot tell that rejection from a dead socket, and for a dead socket it deliberately _Exit()s the process — closing the kernel IPC handle is the only portable way to cancel daemon session ownership from a thread blocked in stdio. Correct for a broken transport; a fatal over-reaction to a large answer. The substitution therefore has to happen in application_mcp_request, which is the last layer still holding the request id. Replies that fit are returned byte-identical; a reply that cannot be framed becomes a JSON-RPC -32603 naming both numbers and what to do ("narrow the projection, add LIMIT, or paginate"), because the failure it replaces was a silent exit and an opaque "internal error" would be no improvement. Reproduced first, on a locally generated 20k-node fixture rather than the reporter's repo (external code stays unverified until reviewed): before LIMIT 10000 -> ok, 8,529,990 bytes LIMIT 20000 -> SERVER DIED exit=1 stderr='' after LIMIT 20000 -> JSON-RPC -32603, and a follow-up query on the SAME session succeeds — which is the property that actually matters The decision and the substitution live in one function on purpose. The first version of the test built the error object directly and PASSED with the size check removed: it proved the error was well-formed while the server still died. Testing application_framable_response instead binds the branch — verified by removing the guard, which reddens it at ASSERT(replaced != big). Not addressed here, deliberately: every reply still carries its payload twice (content[0].text and structuredContent.text are verbatim duplicates, ~2.05x measured), which is why the ceiling is reached at ~4 MB of real content. That is a wire-format question affecting every tool and every caller's token bill, so it belongs in its own change with a decision behind it, not smuggled into a crash fix. Signed-off-by: Martin Vogel --- src/daemon/application.c | 89 +++++++++++++++++++++++++++++-- src/daemon/application_internal.h | 10 ++++ tests/test_daemon_application.c | 73 +++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 3 deletions(-) diff --git a/src/daemon/application.c b/src/daemon/application.c index f8903e4d9e..9062c60cf8 100644 --- a/src/daemon/application.c +++ b/src/daemon/application.c @@ -2340,6 +2340,70 @@ static cbm_daemon_runtime_application_status_t application_set_context( return CBM_DAEMON_RUNTIME_APPLICATION_OK; } +/* Build the JSON-RPC error that replaces a reply too large to frame. + * + * Says the two numbers a caller needs to act — what the reply measured and what + * fits — because the alternative the reporter lived through was a silent exit + * with nothing to go on. The advice is concrete for the same reason: "narrow the + * projection or add LIMIT" is actionable, "internal error" is not. + * + * `request` may be NULL when the message did not parse; the response then omits + * the id, which is what JSON-RPC requires for an unidentifiable request. */ +static char *application_oversized_response_error(const cbm_jsonrpc_request_t *request, + size_t response_length) { + char message[CBM_SZ_256]; + (void)snprintf(message, sizeof(message), + "response too large: %zu bytes exceeds the %u byte transport limit; " + "narrow the projection, add LIMIT, or paginate", + response_length, (unsigned)CBM_DAEMON_RUNTIME_APPLICATION_PAYLOAD_MAX); + + yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL); + if (!doc) { + return NULL; + } + yyjson_mut_val *err = yyjson_mut_obj(doc); + yyjson_mut_doc_set_root(doc, err); + /* -32603 (internal error) is the closest standard code: the request was + * well-formed and the failure is server-side. */ + yyjson_mut_obj_add_int(doc, err, "code", -32603); + yyjson_mut_obj_add_str(doc, err, "message", message); + char *error_json = yyjson_mut_write(doc, 0, NULL); + yyjson_mut_doc_free(doc); + if (!error_json) { + return NULL; + } + + cbm_jsonrpc_response_t response = { + .id = request && request->has_id ? request->id : 0, + .id_str = request ? request->id_str : NULL, + .error_json = error_json, + .error_code = -32603, + }; + char *encoded = cbm_jsonrpc_format_response(&response); + free(error_json); + return encoded; +} + +/* Decide whether `response` can be framed, substituting the error when it + * cannot. Owns `response` either way and returns the reply to send, or NULL + * only on allocation failure. This is ONE function on purpose: the bug was the + * DECISION (an oversized reply travelling on as if it were fine), so the + * decision and the substitution have to be testable together — a test that + * only builds the error passes even with the size check removed. */ +static char *application_framable_response(char *response, const cbm_jsonrpc_request_t *request) { + if (!response || strlen(response) <= CBM_DAEMON_RUNTIME_APPLICATION_PAYLOAD_MAX) { + return response; + } + char *replacement = application_oversized_response_error(request, strlen(response)); + free(response); + return replacement; +} + +char *cbm_daemon_application_framable_response_for_test(char *response, + const cbm_jsonrpc_request_t *request) { + return application_framable_response(response, request); +} + static cbm_daemon_runtime_application_status_t application_mcp_request( cbm_daemon_application_session_t *session, const uint8_t *request, uint32_t request_length, uint8_t **response_out, uint32_t *response_length_out) { @@ -2365,18 +2429,37 @@ static cbm_daemon_runtime_application_status_t application_mcp_request( } else if (tool_request && response) { application_update_notice_inject(session, &response); } - if (parsed_ok) { - cbm_jsonrpc_request_free(&parsed); - } if (response) { size_t response_length = strlen(response); if (response_length > UINT32_MAX) { + if (parsed_ok) { + cbm_jsonrpc_request_free(&parsed); + } free(response); return CBM_DAEMON_RUNTIME_APPLICATION_HANDLER_ERROR; } + /* A reply larger than one frame has to become a JSON-RPC error HERE, + * while the request id is still in hand. Handing it to the transport + * instead is indistinguishable from a dead socket at the frontend + * worker, which responds by _Exit()ing the process — right for a broken + * transport, a fatal over-reaction to a large answer. Under an MCP host + * that does not respawn the server, that took every tool on it out for + * the rest of the session, leaving exit=1 and an empty stderr to debug + * from (#1375). */ + response = application_framable_response(response, parsed_ok ? &parsed : NULL); + if (!response) { + if (parsed_ok) { + cbm_jsonrpc_request_free(&parsed); + } + return CBM_DAEMON_RUNTIME_APPLICATION_HANDLER_ERROR; + } + response_length = strlen(response); *response_out = (uint8_t *)response; *response_length_out = (uint32_t)response_length; } + if (parsed_ok) { + cbm_jsonrpc_request_free(&parsed); + } application_refresh_watch(session); return CBM_DAEMON_RUNTIME_APPLICATION_OK; } diff --git a/src/daemon/application_internal.h b/src/daemon/application_internal.h index a358cb21c4..04f24421a1 100644 --- a/src/daemon/application_internal.h +++ b/src/daemon/application_internal.h @@ -5,8 +5,10 @@ #define CBM_DAEMON_APPLICATION_INTERNAL_H #include "daemon/application.h" +#include "mcp/mcp.h" /* cbm_jsonrpc_request_t */ #include +#include /* Read-only test diagnostic for a live session. The caller must serialize * this check with request, cancellation, and close callbacks. */ @@ -35,4 +37,12 @@ int cbm_daemon_application_background_initializes_for_test(void); * that a request QUEUED rather than erroring or starting. */ int cbm_daemon_application_busy_queue_waits_for_test(void); +/* Build the JSON-RPC error substituted for a reply too large to frame (#1375). + * Exposed because the alternative — driving a real >10 MiB reply — needs a + * ~20k-node index, a fixture cost the unit suite should not carry. The + * end-to-end behaviour is covered by the repro in the issue. Caller frees. + * `request` may be NULL (unparseable message → no id echoed). */ +char *cbm_daemon_application_framable_response_for_test(char *response, + const cbm_jsonrpc_request_t *request); + #endif /* CBM_DAEMON_APPLICATION_INTERNAL_H */ diff --git a/tests/test_daemon_application.c b/tests/test_daemon_application.c index e81b766a37..5ad262a3f1 100644 --- a/tests/test_daemon_application.c +++ b/tests/test_daemon_application.c @@ -4870,7 +4870,80 @@ TEST(daemon_application_rejects_clean_exit_when_process_tree_is_not_contained) { PASS(); } +/* #1375: a reply too large to frame must become a JSON-RPC ERROR, not a + * transport failure. + * + * Why this matters more than it looks: the frontend worker cannot tell a + * rejected oversized frame from a dead socket, and for a dead socket it + * deliberately _Exit()s the process (closing the kernel IPC handle is the only + * portable way to cancel daemon session ownership from a thread blocked in + * stdio). So passing an oversized reply DOWN to the transport killed the whole + * server — every tool gone for the rest of the session, exit=1, empty stderr. + * The substitution therefore has to happen here, at the layer that still holds + * the request id. + * + * Reproduced end-to-end before fixing, on a 20k-node fixture: LIMIT 10000 -> + * 8,529,990 bytes ok; LIMIT 20000 -> SERVER DIED exit=1. After: the same query + * returns this error and a follow-up query on the SAME session succeeds. */ +TEST(daemon_application_oversized_reply_is_a_jsonrpc_error_not_a_death) { + cbm_jsonrpc_request_t request = {0}; + request.has_id = true; + request.id = 42; + + /* A reply that FITS must pass through byte-identical — the guard must not + * touch the overwhelming majority of replies. */ + char *small = strdup("{\"jsonrpc\":\"2.0\",\"id\":42,\"result\":{}}"); + ASSERT_NOT_NULL(small); + char *kept = cbm_daemon_application_framable_response_for_test(small, &request); + ASSERT_TRUE(kept == small); /* same pointer: untouched */ + ASSERT_STR_EQ(kept, "{\"jsonrpc\":\"2.0\",\"id\":42,\"result\":{}}"); + free(kept); + + /* A reply that CANNOT be framed must come back as a JSON-RPC error instead. + * Passing it on is what killed the server: the frontend worker cannot tell a + * rejected oversized frame from a dead socket, and for a dead socket it + * deliberately _Exit()s the process — so every tool on that server was gone + * for the rest of the session, with exit=1 and an empty stderr (#1375). + * Reproduced end-to-end on a 20k-node fixture before fixing: LIMIT 10000 -> + * 8,529,990 bytes ok, LIMIT 20000 -> SERVER DIED. After: the same query + * returns this error and a follow-up query on the SAME session succeeds. */ + size_t oversized = (size_t)CBM_DAEMON_RUNTIME_APPLICATION_PAYLOAD_MAX + 1U; + char *big = malloc(oversized + 1U); + ASSERT_NOT_NULL(big); + memset(big, 'x', oversized); + big[oversized] = '\0'; + + char *replaced = cbm_daemon_application_framable_response_for_test(big, &request); + ASSERT_NOT_NULL(replaced); + ASSERT_TRUE(replaced != big); /* substituted, not passed through */ + + /* The replacement must itself fit, or it reproduces the bug it replaces. */ + ASSERT_TRUE(strlen(replaced) <= (size_t)CBM_DAEMON_RUNTIME_APPLICATION_PAYLOAD_MAX); + /* ...and carry the caller's id, or the client cannot match reply to request. */ + ASSERT_NOT_NULL(strstr(replaced, "\"error\"")); + ASSERT_NOT_NULL(strstr(replaced, "\"id\":42")); + ASSERT_NOT_NULL(strstr(replaced, "-32603")); + /* ...and say what happened and what to do: the failure it replaces was a + * SILENT exit, so an opaque "internal error" would be no improvement. */ + ASSERT_NOT_NULL(strstr(replaced, "response too large")); + ASSERT_NOT_NULL(strstr(replaced, "LIMIT")); + free(replaced); + + /* An unparseable message has no id to echo, but must still yield an error + * rather than NULL — NULL is turned back into a hard failure by the caller. */ + char *big2 = malloc(oversized + 1U); + ASSERT_NOT_NULL(big2); + memset(big2, 'y', oversized); + big2[oversized] = '\0'; + char *anonymous = cbm_daemon_application_framable_response_for_test(big2, NULL); + ASSERT_NOT_NULL(anonymous); + ASSERT_NOT_NULL(strstr(anonymous, "\"error\"")); + free(anonymous); + PASS(); +} + SUITE(daemon_application) { + RUN_TEST(daemon_application_oversized_reply_is_a_jsonrpc_error_not_a_death); RUN_TEST(daemon_application_new_session_does_not_retain_initial_store); RUN_TEST(daemon_application_request_cancel_is_scoped_to_exact_token); RUN_TEST(daemon_application_requires_immutable_explicit_context);