fix(mcp): bound tool response size so large logs return instead of hanging - #89
Merged
Merged
Conversation
…nging `evaluate_formula` and `get_channel_data` serialized one entry per log record. Every MCP tool result travels as a single Server-Sent Event, and streamable-HTTP MCP clients cap one SSE event at 1 MiB (`DEFAULT_MAX_EVENT_SIZE_BYTES`); an event over that cap is discarded inside the client's SSE decoder, so the caller receives no value *and* no error and the call simply never returns. Past roughly 22,000 rows the response crossed the cap, which is why it read as a hang rather than a slowdown — the IPC layer's own 30s timeout never fired because nothing had actually timed out. Reproduced against the reference Python MCP client: the cliff sits exactly where the serialized payload crosses 1 MiB, and the raw HTTP body is delivered correctly in ~0.12s in the failing cases, confirming the loss is on the SSE-decode side rather than in the IPC or evaluation path. Two defenses: - Sample budget. `limit_samples` reduces any per-record series to `DEFAULT_MAX_POINTS` (2000), clamped to `MAX_POINTS_LIMIT` (10,000), reusing the chart's LTTB so peaks and dropouts survive. Both commands take an optional `max_points`; responses report `total_samples` and `downsampled`. `FindPeaks` is capped at `MAX_PEAKS` (500, most prominent first), since peak count scales with channel noise rather than with anything the caller asked for. - Byte guard. `json_result` serializes compactly (pretty-printing put one array element per line, roughly doubling the payload for no benefit) and refuses anything over `MAX_RESPONSE_BYTES` with an error naming `max_points` and the time range. An honest error beats silence. Statistics stay exact: `channel_series` is a new accessor for the full, un-downsampled series, and `get_channel_stats`, `find_peaks`, `correlate_channels` and `evaluate_formula`'s stats all read through it rather than through the now-downsampled `get_channel_data` response. New fields are `#[serde(default)]`, so payloads written before the budget existed still parse. Verified end to end through the reference MCP client: 500,000 rows now returns in 64ms with a 70 KB payload, and `stats.count` still reports every record. Fixes #88 Claude-Session: https://claude.ai/code/session_01CDnphU9sfMQiPBxHqXvqYo
Review follow-ups on the response-size fix. Ragged logs could panic the GUI. `Log::get_channel_data` is a `filter_map` that drops a row missing the column, and an analysis-derived `cached_data` is only as long as the algorithm made it, so a ragged log yields fewer values than times — misaligned from the first dropped row, not merely short. `filter_by_time_range`'s `zip` papered over the mismatch, and `downsample_lttb` indexes `values` off `times.len()`, so `limit_samples` panicked on the eframe update thread. `require_aligned` now rejects the pair in `channel_series` and `handle_evaluate_formula`, matching how `src/ui/chart.rs` already refuses to plot it: better a clear error than numbers attributed to the wrong timestamps. Peak truncation was invisible. `find_peaks` capped at 500 with nothing in the response to say so, which is exactly the silent data loss the sample budget was added to avoid elsewhere. `ResponseData::Peaks` now carries `total_peaks` and `truncated`, the tool reports both, and its description says the cap exists — a caller asked "how many boost spikes?" would otherwise read 500 off a channel with 41,000 and believe it. The byte guard now covers every tool. Seven handlers still pretty-printed straight into `CallToolResult`, bypassing `json_result`, which CLAUDE.md describes as the general backstop. None can reach 1 MiB today, but a future per-record tool added under that assumption would not have been covered. `json_result` is generic over `Serialize` so typed payloads route through it without an intermediate `Value`. CLAUDE.md corrections: `FindPeaks` selects by prominence but returns chronologically, and `handle_evaluate_formula`'s stats hold the exact-aggregate invariant via ordering (`compute_stats` before `limit_samples`) rather than via `channel_series`. Version bumped to 2.14.1 across Cargo.toml, Cargo.lock, README, the docs landing page and sitemap. 2.14.0 was already released by 5ae8e29, so without this the release workflow would re-tag it and the in-app updater would never offer the fix to existing installs. Claude-Session: https://claude.ai/code/session_01CDnphU9sfMQiPBxHqXvqYo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #88.
The bug
Every MCP tool result travels as a single Server-Sent Event, and streamable-HTTP MCP clients cap one SSE event at 1 MiB (
DEFAULT_MAX_EVENT_SIZE_BYTESin the reference client). An event over the cap is discarded inside the client's SSE decoder — the caller receives no value and no error.evaluate_formulaandget_channel_dataserialized one entry per log record, so past roughly 22,000 rows the payload crossed the cap and the call simply never returned. The reporter was right to rule out both of their leads: the IPC layer's 30s timeout never fired because nothing had actually timed out, and the expression engine was never the bottleneck.Reproduction
Driving the real MCP server with the reference Python MCP client against a stub IPC responder:
The raw HTTP body arrives correctly in the failing case, so the loss is entirely on SSE decode. The cliff sits exactly where the payload crosses 1,048,576 bytes. (The stub's values are shorter per row than real telemetry, which is why this cliff is at 28K and the reporter's at 23K — same byte boundary.)
The fix
Two defenses:
limit_samplesreduces any per-record series toDEFAULT_MAX_POINTS(2000), clamped toMAX_POINTS_LIMIT(10,000), reusing the chart's LTTB so peaks and dropouts survive rather than being strided past. Both commands take an optionalmax_points; responses reporttotal_samplesanddownsampled.find_peaksis capped atMAX_PEAKS(500) — its output scales with channel noise rather than with anything the caller asked for, so it had the same latent bug — and reportstotal_peaks/truncatedso a truncated list is never mistaken for a complete one.json_resultserializes compactly (the previousto_string_prettyput one array element per line, roughly doubling the payload for nothing) and refuses anything overMAX_RESPONSE_BYTES(512 KiB) with an error namingmax_pointsand the time range. Every tool handler routes through it, so a future per-record tool is covered by default.Statistics stay exact.
channel_seriesis a new accessor for the full un-downsampled series;get_channel_stats,find_peaksandcorrelate_channelsread through it, andevaluate_formulaholds the same invariant by callingcompute_statsbeforelimit_samples. Nothing computes aggregates over 2000 samples when the log has 178,000.Ragged logs no longer panic the GUI.
Log::get_channel_datais afilter_mapthat drops a row missing the column, so a ragged log yields a times/values pair that is misaligned, not merely short — anddownsample_lttbindexesvaluesofftimes.len().require_alignedrejects that pair up front, matching howsrc/ui/chart.rsalready refuses to plot it.New wire fields are
#[serde(default)], so payloads written before the budget existed still parse.Verification
End to end through the reference MCP client:
500,000 rows also returns in 64 ms with a 70 KB payload.
max_points: 999999clamps to 10,000.Full suite green (1098 tests, 16 new),
cargo fmt --all -- --checkandcargo clippy --all-targets -- -D warningsclean.Release
Version bumped to 2.14.1 across
Cargo.toml,Cargo.lock,README.md,docs/index.htmlanddocs/sitemap.xml. 2.14.0 was already released, so without the bump the release workflow (which greps the version out ofCargo.toml) would re-tag it and the in-app updater would never offer this fix to existing installs.https://claude.ai/code/session_01CDnphU9sfMQiPBxHqXvqYo
Generated by Claude Code