feat(remote-control): compress textual tunnel responses with gzip - #3707
Conversation
🦋 Changeset detectedLatest commit: 82b77dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53c99ab639
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (encoding !== 'gzip' && encoding !== '*') continue; | ||
| const quality = params.map((param) => param.trim()).find((param) => param.startsWith('q=')); | ||
| if (quality === undefined || Number(quality.slice(2)) > 0) return true; |
There was a problem hiding this comment.
For a valid header such as Accept-Encoding: gzip;q=0, *;q=1, the explicit gzip entry excludes gzip and takes precedence over the wildcard. This loop skips the zero-quality entry and then returns true for *, causing the tunnel to send gzip to a client that explicitly cannot accept it. Resolve gzip's explicit quality first, and consult * only when gzip is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cff3d668e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| response.headers['content-encoding'] === undefined && | ||
| body.length >= GZIP_MIN_BODY_BYTES && | ||
| isGzipCompressibleType(contentType) && | ||
| acceptsGzipEncoding(parsed.headers) |
There was a problem hiding this comment.
When a forwarded request contains Range and a text file yields a body of at least 1 KB, this condition gzips the identity slice while preserving the origin's Content-Range. Range is forwarded unchanged, and kap-server emits such 206 responses for filesystem downloads in packages/kap-server/src/routes/workspaceFs.ts:254-260 and packages/kap-server/src/routes/fs.ts:645-651; the offsets and total then describe the uncompressed representation rather than the transmitted gzip representation, so resumed or multipart downloads cannot be safely reassembled. Skip compression for 206/Content-Range responses, or negotiate the encoding before applying the range.
Useful? React with 👍 / 👎.
| acceptsGzipEncoding(parsed.headers) | ||
| ) { | ||
| body = gzipSync(body); | ||
| headers.push('Content-Encoding', 'gzip'); |
There was a problem hiding this comment.
When a compressible response retains cacheable headers, adding Content-Encoding: gzip without adding or merging Vary: Accept-Encoding lets a browser or intermediary reuse that representation for a request which excludes gzip. This is reachable with the shipped server because packages/kap-server/src/routes/webAssets.ts:57-68 gives hashed assets a one-year immutable cache policy, while an SVG is compressible here but is not rewritten and therefore keeps that policy. Add Accept-Encoding to Vary whenever this branch performs compression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a483c4bc2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| isGzipCompressibleType(contentType) && | ||
| acceptsGzipEncoding(parsed.headers) | ||
| ) { | ||
| body = gzipSync(body); |
There was a problem hiding this comment.
When Remote Control returns a large textual workspace/file download, this synchronously compresses the entire response with no upper size limit; the filesystem routes stream arbitrarily large files, so gzipSync can block the shared Node event loop for seconds, stalling tunnel WebSocket traffic, heartbeat timers, and other kap-server requests. Use asynchronous/streaming gzip, or impose a bounded size above which compression is skipped.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1c57e6236
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .map((token) => token.trim()); | ||
| if (tokens.includes('*') || tokens.includes('accept-encoding')) varyCovers = true; | ||
| } | ||
| if (!varyCovers) headers.push('Vary', 'Accept-Encoding'); |
There was a problem hiding this comment.
The follow-up adds Vary only inside the gzip branch, so when a cacheable compressible asset is first requested without gzip—such as an immutable hashed JS or SVG from packages/kap-server/src/routes/webAssets.ts—the identity response remains unmarked. A shared cache can then reuse that identity representation for later requests with a different Accept-Encoding, defeating the compression this change relies on and even serving an unacceptable representation when identity has q=0; add or merge Vary: Accept-Encoding for both negotiated alternatives.
Useful? React with 👍 / 👎.
| body = await gzipAsync(body); | ||
| headers.push('Content-Encoding', 'gzip'); |
There was a problem hiding this comment.
When a textual download from workspaceFs.ts or fs.ts includes an ETag, filterResponseHeaders preserves that validator even though this branch changes the representation bytes. The resulting gzip and identity representations therefore share a strong validator; in particular, a later range response is deliberately left uncompressed but carries the same ETag, allowing clients or caches to revalidate or combine byte-distinct representations incorrectly. Remove the upstream ETag when compressing or replace it with an encoding-specific validator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbdf79ca06
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| headers.push('Content-Encoding', 'gzip'); | ||
| for (let index = 0; index < headers.length; index += 2) { | ||
| if (headers[index]!.toLowerCase() === 'etag') { | ||
| headers[index + 1] = headers[index + 1]!.replace(/"$/, '-gzip"'); |
There was a problem hiding this comment.
When a client revalidates a previously compressed response, it sends the newly exposed "…-gzip" validator in If-None-Match, but filterForwardRequestHeaders forwards that value unchanged to the local server. Routes such as packages/kap-server/src/routes/workspaceFs.ts:234-237 compare the header exactly against the identity ETag, so the comparison can never match and every revalidation returns and recompresses the full body instead of producing a 304. Translate conditional validators back before forwarding, evaluate them against the generated gzip variant, or omit the transformed ETag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82b77dc01e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (rewritten) headers.push('Cache-Control', 'no-cache'); | ||
| const negotiated = | ||
| response.headers['content-encoding'] === undefined && | ||
| response.statusCode !== 206 && |
There was a problem hiding this comment.
When a client resumes a gzip-capable textual attachment larger than 1 KB, the initial 200 response is gzipped while retaining Accept-Ranges: bytes, but this guard forces the subsequent 206 response back to identity encoding. The Content-Range emitted by packages/kap-server/src/routes/files.ts:145-158 and packages/kap-server/src/routes/sessionMedia.ts:83-97 therefore indexes different bytes from the representation being resumed, so a downloader following the advertised range support can append incompatible data or fail the resume. The current response.statusCode !== 206 guard is fresh evidence beyond the earlier issue: it prevents compressing only the partial response while leaving the full response range-advertised and compressed; strip range support from compressed 200 responses or keep encoding/range selection consistent.
Useful? React with 👍 / 👎.
1 participant
Related Issue
None — reported directly by an internal user; the problem is explained below.
Problem
Opening the Remote Control web UI for the first time frequently ends in a 504 from the relay. The entry bundle
/devices/:id/assets/index-*.jsis ~3.2 MB, and the tunnel base64-encodes the whole response before upload (~4.3 MB on the wire). The relay times the request out after 30s, so any device with less than ~2 Mbps uplink almost always fails (at 1 Mbps this single file takes ~34s). The slow leg is the user's machine → relay upload, so compression has to happen before the response enters the tunnel — compressing at the relay or gateway would be pointless.What changed
Tunnel client only (
packages/remote-control); the relay and the web UI are untouched:Accept-Encodingis read before request headers are filtered. The header is still stripped from the forwarded request as before — that strip protects the body rewrite, and this change does not alter it.rewriteRemoteControlResponse) and before the response is base64-encoded into the tunnel: if the browser accepts gzip, the body is textual (text/*,application/javascript,application/json,application/xml,image/svg+xml), and it is at least 1 KB, the final body is gzipped (zlib.gzipSync) andContent-Encoding: gzipis set.Content-Lengthwas already recomputed, and the header is not on the response blocklist, so it passes through as-is.Content-Encodingfrom upstream keep the existing behavior (rewrite skipped, passed through untouched, never compressed twice). When the browser does not accept gzip, the response bytes are exactly what they were before.Not compressing in kap-server on purpose: the client skips URL rewriting for responses that already have a content-encoding (compressed bytes cannot be string-replaced), so lazily loaded
/assets/chunks would break. Compressing after the rewrite gets the same result in one step without a decompress → rewrite → recompress round trip.Effect: the ~3.2 MB entry bundle drops to roughly 1 MB on the wire (~3x), so first paint on marginal uplinks goes from a 30s timeout to ~10s; the compression itself costs tens of milliseconds on the user's machine.
Tests: the existing tunnel end-to-end case is extended in place (test count unchanged): small bodies stay uncompressed and the
Accept-Encodingstrip is asserted unchanged; a large JS body assertsContent-Encoding: gzip, a Content-Length matching the gzipped bytes, and a gunzip equal to the rewritten body; a large binary body stays uncompressed. Locally:packages/remote-control19/19 green, typecheck and oxlint clean. Remotely on m0-dev:@moonshot-ai/remote-control,@moonshot-ai/kap-server, and@moonshot-ai/kimi-codesuites all green.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.