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);