perf(cursor): decompress gzip response frames off the async runtime - #256
Conversation
The Cursor adapter decompressed gzipped Connect response frames
synchronously while polling the upstream byte stream, so a large frame
occupied a Tokio worker and delayed unrelated sessions.
Add `decode_gzip_frame_async`, which keeps small frames inline and hands
larger ones to `spawn_blocking`. `ReadState::ingest` becomes async and
awaits it; the gzip branch now uses a `Cow`, mirroring the idiom already
in `client.rs`. Taking `Bytes` makes the move into the blocking task a
refcount bump rather than a copy.
The threshold is measured, not guessed. A new `decode_gzip_frame` bench
over a realistic ~4:1 protobuf-shaped fixture gives medians of 9.5 us at
1 KiB, 26.2 us at 4 KiB, 118.5 us at 16 KiB and 456 us at 64 KiB, against
a 12.0 us `spawn_blocking` round trip. 4 KiB is the largest power of two
inside Tokio's 100 us blocking budget, and Cursor's per-token delta
frames sit well below it, so they stay inline.
The 64 MiB decompressed-size cap, the malformed/truncated-frame handling
and the `cursor gzip: {error}` surface are unchanged. The remaining gzip
call sites (client/stream/response/sse) are left synchronous: they are on
`#[allow(dead_code)]` modules retained pending the #170 follow-up and
never touch the async runtime.
Closes #254
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous gzip frame decoding for the Cursor adapter, decoding small frames inline and offloading larger frames (> 4 KiB) to Tokio's blocking thread pool to prevent blocking the executor. It also adds corresponding benchmarks and unit tests. The review feedback suggests a medium-severity improvement to avoid an unnecessary clone of frame.payload when calling decode_gzip_frame_async in the streaming hot path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance
Performance Changes
Comparing |
…size Review of the previous commit found the threshold gated the inline decode path on the one quantity that does not bound the work. Deflate reaches ~1032:1, so a 4 KiB frame can inflate to 4.19 MB and hold a Tokio worker for ~1 ms — about 10x the 100 us budget the threshold existed to respect. The benchmark fixture pinned the ratio near 4:1, so it could never have surfaced this. Bound the inline path by decompressed output instead. `decode_gzip_frame_within` inflates at most `budget + 1` bytes and reports `None` when the frame is too large, leaving the compressed length as nothing more than a cheap pre-filter. Worst-case inline work is now ~54-65 us for realistic data; incompressible data is faster still, since it inflates via stored blocks. Bound concurrent offloads with a semaphore sized to available parallelism. Tokio's blocking pool defaults to 512 threads and is shared with credential I/O, token counting, reload and state persistence, so unbounded submission traded runtime starvation for CPU and memory exhaustion. The permit lives inside the blocking closure: a permit held by the caller's future would be released on cancellation while the unabortable decode kept running, bounding admission rather than occupancy. Add a `#[cfg(test)]` counter so removing the offload fails a test — verified by mutation, since the previous tests passed either way. Correct the bench fixture, which overshot its labelled size by up to 25% and so measured a "4 KiB" frame that was really 4,275 bytes. Make the tuning constants `pub(crate)`: they are a heuristic, not published API. Refs #254
The offload counter was global while five other tests reach the same offload arm, so a concurrent increment could satisfy the one-sided `> before` assertion even if the decode under test had regressed to inline. The mutation check that appeared to validate it was run filtered, where no other test can increment — it demonstrated the failure for the wrong reason. Serialize every test that reaches `decode_gzip_frame_async` behind a test-only observer lock, which makes exact deltas attributable. The offload path now asserts `before + 1`, and the inline path asserts the count is unchanged — the latter was previously untested, so routing small frames through `spawn_blocking` unconditionally would have left the suite green despite discarding half the design. Both directions are now verified by unfiltered mutation runs against the full workspace suite. Add a one-sided guard that concurrent offloads never exceed the semaphore's slot count. It can false-pass when work does not overlap, but cannot false-fail under any scheduling, so it stays deterministic; the cancellation invariant is left untested rather than guarded by a timing-dependent test, since this repo has already had to remove one. Also correct the output-budget doc: the probe materializes `budget + 1` bytes, so 32 KiB is the largest complete output accepted inline rather than the maximum produced. Refs #254
Greptile SummaryMoves Cursor Connect gzip decompression off Tokio workers while preserving bounded inline decoding and existing frame validation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/adapters/cursor/connect.rs | Adds output-bounded inline gzip probing, semaphore-limited blocking decompression, and coverage for decoding limits, corruption, dispatch selection, and concurrency. |
| src/adapters/cursor/agent.rs | Makes frame ingestion asynchronous so live Cursor gzip frames can use the new bounded offload path without changing event extraction behavior. |
| benches/gateway.rs | Adds representative Cursor gzip-decompression benchmarks with compressed fixtures constrained to their labeled size ranges. |
Sequence Diagram
sequenceDiagram
participant Cursor as Cursor upstream
participant Stream as Cursor event stream
participant Probe as Bounded gzip probe
participant Slots as Decode semaphore
participant Blocking as Tokio blocking pool
Cursor->>Stream: Connect response frame
alt Uncompressed frame
Stream->>Stream: Parse payload inline
else Gzip frame at or below compressed prefilter
Stream->>Probe: Inflate up to 32 KiB + 1
alt Complete output fits budget
Probe-->>Stream: Decoded bytes
else Output exceeds budget
Probe-->>Stream: Offload required
Stream->>Slots: Await permit
Slots->>Blocking: Decode under 64 MiB cap
Blocking-->>Stream: Decoded bytes or error
end
else Gzip frame above compressed prefilter
Stream->>Slots: Await permit
Slots->>Blocking: Decode under 64 MiB cap
Blocking-->>Stream: Decoded bytes or error
end
Stream->>Stream: Extract Cursor events
Reviews (3): Last reviewed commit: "perf(cursor): size the inline gzip buffe..." | Re-trigger Greptile
`frame` is owned by the decoder loop and its payload is not read after the decompression branch, so the `Bytes` clone only paid an atomic refcount round trip per gzipped frame. Move it instead.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous gzip decompression for Cursor Connect frames, offloading larger decodes to Tokio's blocking thread pool using a semaphore to limit concurrency, while keeping small decodes inline. It also adds corresponding benchmarks and tests. The review feedback suggests pre-allocating the decompression buffer in decode_gzip_frame_within to avoid multiple reallocations, and adding an assertion in the benchmark fixture to prevent silent protobuf length encoding corruption if the text length exceeds 127 bytes.
… varint Pre-size the inline probe's output buffer from the compressed length at a realistic ratio, capped by the budget, so a typical frame decodes without repeated growth. The off-thread decode keeps its unsized buffer: it does not run against the 100 microsecond worker budget. The bench fixture writes its protobuf length as a single byte, which is correct only while the text stays under 128 bytes (currently 121 at the largest arg). Assert it rather than let an edit to the static string silently corrupt the fixture.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous, bounded gzip decompression for Cursor Connect frames. It optimizes decompression by decoding small frames inline on the Tokio worker thread, while offloading larger frames to a bounded blocking pool controlled by a semaphore to limit CPU contention. It also adds comprehensive benchmarks and unit tests to verify the correctness, performance, and concurrency limits of the new async decompression path. There are no review comments to evaluate, and the implementation is robust and well-tested.
|
) * perf(cursor): move request protobuf framing off the async runtime `build_run_frames` ran synchronously on a Tokio worker inside `open_turn`, taking 424 us for a 4 KiB prompt with 16 tools -- Claude Code's normal request shape, and roughly 4x over Tokio's 100 us blocking budget. With an image attached it reached 1.4-7.5 ms. Extract the bounded-offload pattern from #256 into a shared `cursor::offload` module (`cpu_slots` + `spawn_bounded`) rather than hand-rolling it a third time, and move `connect.rs`'s gzip decode onto it. The permit is moved into the blocking closure so cancellation cannot free a slot while an unabortable task is still running. `AgentRunParams` becomes owned so it can move into `spawn_blocking`, and `open_turn` takes it by value. In `forward` this costs zero clones: prompt, images, tools and model_id are already owned; only `cwd` gains a `to_string()`. Framing offloads unconditionally. It runs once per request, before the body sender is spawned and before `.send().await`, so the round trip is off every streaming path -- and unlike gzip's frame size no cheap input measure bounds the work, since tool-schema encoding grows nonlinearly (25.9 us for one tool against 395.4 us for 16). Base64 image decoding offloads above 64 KiB of estimated decoded bytes, using base64's exact 4:3 expansion as a sound pre-decode upper bound. The threshold keeps headroom deliberately: the new benchmark covers the decode alone, while the inline path also pays `cursor_selected_images`, which walks the message JSON and clones each base64 string before the offload decision is made. New CodSpeed benches cover framing and image decode. Both are pure sync functions, since CodSpeed runs under Valgrind. Refs #259 * chore: update agent memory * chore: update agent memory * fix(cursor): keep framing failures local and isolate offload classes Code review of the framing offload found two behavior defects. Framing failures advanced ordered failover. `open_turn` mapped a framing error to `CursorError::internal`, which `map_client_error` stamps as `AdapterFailure::BeforeHeaders`, so `failover.rs` retried a deterministic local fault against every upstream -- each failing identically, since framing precedes any upstream contact -- and the client saw only `all upstreams failed`. This contradicted docs/upstreams-failover.md, which states Cursor adapter-owned errors return immediately. Move the framing offload out of `open_turn` into `forward`, so `open_turn` takes pre-built frames and does I/O only. The framing error now uses `own_error`, which sets `failure: None`, making it local by construction rather than by classification. It also lets the TODO(#170) retry reuse cheap `Bytes` clones instead of reframing. Sharing one semaphore across gzip decode and request preparation introduced cross-class head-of-line blocking that main does not have: response-path gzip is per-frame and latency-critical, request framing is once per request. Split into `gzip_slots` and `request_prep_slots` over one `spawn_bounded` helper, restoring gzip's pre-PR isolation. The doc comment no longer claims framing "cannot monopolize the slots" -- once-per-request bounds per-request multiplicity, not aggregate. Tests now cover the production wiring rather than a test-only replica, the cumulative multi-image threshold, and the load-bearing invariant that the permit lives inside the unabortable closure: moving it out makes the new cancellation test fail. Frame assertions now include the image bytes, MIME, tool description and schema, so dropping a payload field can no longer pass. `build_run_frames` and `decode_selected_images` are `#[doc(hidden)]`, and `AgentImage` no longer derives an unused `Clone` over decoded image bytes. A new bench measures the full inline path (extraction + clone + decode) that the 64 KiB threshold rests on: 54.55 us at 64 KiB against 112.7 us at 128 KiB. Refs #259 * chore: update agent memory * test(cursor): prove offload class isolation and reset path markers Add a behavioral test that the gzip and request-preparation admission classes are independent: saturating either semaphore must not stop the other from admitting work. Every prior test sampled one class at a time under a serializing lock, so pointing `spawn_bounded_gzip` back at `request_prep_slots()` would have left the suite green while restoring the response-path head-of-line blocking the split exists to remove. Verified non-vacuous by that exact mutation, which now fails in 5s. Reset `LAST_REQUEST_PREP_PATH` immediately before each asserted action. The marker is a process-wide test global written by several tests, so a stale value could satisfy an assertion and let a regression that stops recording the path pass depending on test order. Gate the capacity test's closures on per-task channels instead of a shared `Barrier`. A `spawn_blocking` task cannot be aborted, so an assertion panic left the closures parked on the barrier forever and Tokio teardown waited on them, turning a legible failure into a hung test process. Dropping the senders on unwind now wakes every closure. Refs #259 * chore: update agent memory * test(cursor): lock offload class isolation at the production call sites The wrapper-level isolation test proves `spawn_bounded_gzip` and `spawn_bounded_request_prep` draw on distinct semaphores, but it stays green if a production call site is rewired to the wrong class — the exact regression the split exists to prevent. Assert it where it matters instead: saturate every request-preparation permit and require the real `decode_gzip_frame_async` to still complete, then saturate every gzip permit and require the real `build_run_frames_async` and an over-threshold `decode_selected_images_async` to still complete. The gzip payload is incompressible and larger than `INLINE_GZIP_FRAME_BYTES`, so it provably takes the offload path rather than the inline one. Both directions are mutation-proven: pointing `decode_gzip_frame_async` at the request-preparation class, or `build_run_frames_async` at the gzip class, fails each test in 5s via its inner timeout rather than hanging. Refs #259 * chore(review): record out-of-scope failover finding for PR #272



Summary
Closes #254.
The Cursor adapter decompressed gzipped Connect response frames synchronously while polling the upstream byte stream, so a large frame occupied a Tokio worker and delayed unrelated sessions.
decode_gzip_frame_asyncnow decodes small frames inline and hands larger ones tospawn_blocking.ReadState::ingestbecameasync fnand awaits it at its single call site inside the existingunfold; the gzip branch uses aCow, mirroring the idiom already inclient.rs:136-145. The 64 MiB decompressed cap, the malformed/truncated-frame handling and thecursor gzip: {error}surface are unchanged.79287c4implements the offload;498a50dfixes what review found in it;c3dc2aemakes the offload tests attribute their observations to the decode under test;3fb3267and21598bfapply the ready-flip review round (drop a redundantBytesclone, pre-size the inline output buffer, assert the bench fixture's single-byte varint).The inline path is bounded by output, not by compressed size
The first commit gated the inline path on compressed length. That is unsound: deflate reaches ~1032:1, so a valid 4 KiB frame inflates to 4,191,746 bytes and holds a worker ~1.04 ms — roughly 10x the 100 µs budget the threshold existed to respect. Highly repetitive assistant output hits this naturally; it is not only an adversarial case. The original benchmark fixture pinned the compression ratio near 4:1, so it could never have surfaced the problem.
decode_gzip_frame_withinnow inflates at mostbudget + 1bytes and returnsNonewhen the frame is too large, leavingINLINE_GZIP_FRAME_BYTESas nothing more than a cheap pre-filter andINLINE_GZIP_OUTPUT_BYTES(32 KiB) as the real safety bound. Corrupt input still errors identically; over-budget frames are redone off-thread under the full 64 MiB cap, and that bounded re-inflate of the first 32 KiB is deliberate.Worst-case inline work is ~54–65 µs on realistic data. Incompressible data is faster (~8 µs), since it inflates via stored blocks — inflate throughput varies about an order of magnitude with redundancy, which is exactly why compressed length cannot bound the work.
Concurrent offloads are bounded
Tokio's blocking pool defaults to 512 threads over an unbounded queue and is shared with credential I/O, token counting, config reload and state persistence. Unbounded submission would have traded runtime starvation for CPU and memory exhaustion.
A semaphore sized to
available_parallelism(clamped 2..16) bounds concurrent decodes. The permit lives inside the blocking closure: held by the caller's future instead, it would be released on cancellation while the unabortable decode kept running, bounding admission rather than occupancy.Net effect versus
main: previously every concurrent stream inflated simultaneously, so peak decompressed buffers scaled with stream count. They now scale with these slots.Measurements
benches/gateway.rsgains adecode_gzip_framebench over a protobuf-shaped fixture at a realistic ratio. The fixture originally overshot its labelled size by up to 25% — the row labelled "4 KiB" was really 4,275 bytes, above the cutoff it was cited to justify — and now shrinks until0.75 x target <= compressed <= target.spawn_blockinground-trip overhead is reproducible on demand viacargo test -- --ignored --nocapture measure_spawn_blocking_round_trip. It is deliberately not a tracked benchmark:codspeed.ymlruns in Valgrind simulation mode, which counts instructions and so cannot represent thread-wakeup latency.Milestone / spec
None — a runtime-behavior fix on the Cursor agent transport (issue #170 lineage). Follows #246 / f4cd619 (
count_tokensoff the runtime) and #250 / a469eab (Codex WSturn_lock).Checklist
cargo buildpassescargo testpasses (new behavior is covered; tests run without network/loopback where possible)cargo clippy --all-targets -- -D warningscleancargo fmt --all --checkcleanconnect.rsis 713 lines, ~400 of which are tests; see belowdocs/updated if this change deviates from it — n/aconnect.rsexceeds the 500-line preference. Splitting the test module intoconnect/tests.rswould bring production code near 300 lines, but a pure move refactor on this repo trips the SonarCloudnew_coveragegate (the moved file counts wholesale as new code), so it belongs in its own PR.Notes for reviewers
Test coverage. gzip is covered at the unit level for inline success, inline corrupt, above-threshold success, above-threshold corrupt, over-limit, and the small-compressed/large-output case that motivated
498a50d; plus inline, above-threshold and invalid-gzip through the liveinto_event_streampath. Dispatch itself is now guarded in both directions: a#[cfg(test)]counter plus an observer lock that serializes every gzip-reaching test lets the inline and offload tests assert exact deltas (== before,== before + 1) instead of a one-sided> beforethat five unrelated tests on the same arm could satisfy. Both directions are confirmed by unfiltered mutation runs — forcing everything inline fails withleft: 0, right: 1, forcing everything off-thread fails withleft: 4, right: 3.async_gzip_offloads_never_exceed_slot_limitstays deliberately one-sided (observed peak never exceeds the slot count; it does not assert the peak is reached), because a lower bound there would depend on scheduler timing. The permit's cancellation invariant is left untested on purpose rather than guarded by a timing-dependent test — see the PR #182 precedent.Deliberately not fixed. Review flagged that
acquire().awaithas an unbounded waiter queue and suggestedtry_acquireplus overload errors. Declined: waiters retain already-received payloads rather than allocating, Tokio's semaphore is FIFO so excess streams are delayed rather than starved, and failing an interactive request to avoid a short wait is the worse trade. Total resident memory is bounded only by concurrent stream count, which the gateway does not cap anywhere —axum::serveruns without a concurrency-limit layer and inbound auth is optional. That is pre-existing and gateway-wide, tracked as #260 rather than bolted onto this path.Scope. The three related sites in #254 were measured in this pass, as the issue required; the results and the follow-up are on the issue. The remaining gzip call sites in
client.rs/stream.rs/response.rs/sse.rsstay synchronous — they sit on#[allow(dead_code)]modules retained pending the #170 follow-up (src/adapters/cursor/mod.rs:6-24) and never touch the async runtime.Summary by cubic
Move gzip decompression of Cursor Connect response frames off the async runtime to avoid blocking
tokioworkers; small frames decode inline based on output size, larger frames use a boundedspawn_blockingpath. No behavior changes; the 64 MiB cap and error handling remain the same.decode_gzip_frame_async(payload: Bytes)with an output-size budget (INLINE_GZIP_OUTPUT_BYTES = 32 KiB) and a 4 KiB compressed-size prefilter (INLINE_GZIP_FRAME_BYTES); larger or over-budget frames offload viaspawn_blockinggated by a semaphore sized toavailable_parallelism(clamped 2–16).decode_gzip_frame_withinto probe decompressed output up to the budget and decide inline vs offload; madeReadState::ingestasync, switched the gzip branch toCow, and now move the frame payload into the offload to avoid an extraBytesclone.OFFLOADED_DECODES, plus a guard that in-flight offloads never exceed the slot count.Written for commit 3fb3267. Summary will update on new commits.