Connect: parallel gate lookups, zombie-socket freshness, gate fonts, per-message compression (B29-B32) - #1909
Draft
SawyerHood wants to merge 4 commits into
Draft
Connect: parallel gate lookups, zombie-socket freshness, gate fonts, per-message compression (B29-B32)#1909SawyerHood wants to merge 4 commits into
SawyerHood wants to merge 4 commits into
Conversation
The connect gate resolved the label, then verified the visitor's session, each a single-region D1 round trip awaited back to back on every uncached request. The per-isolate caches also held only settled values, so a page load's burst of asset requests all missed together and each issued its own query before the first answer landed. The 401/503 gate pages additionally blocked first paint on a cross-origin Google Fonts stylesheet. Start visitor cookie verification before awaiting the label lookup so the two overlap (tunnel dials skip it), share the in-flight promise per label and per cookie so a burst costs one query each, and render the gate pages with the system font stack. Co-Authored-By: Claude <noreply@anthropic.com>
A tunnel client whose network died without a close frame (laptop lid, cellular handoff, NAT timeout) leaves its socket in the DO with readyState OPEN and a working send(). tunnelSocket() only checked readyState, so every visitor request was proxied into the void and hung for the 30s response head timeout before a 504, for as long as the TCP zombie lingered (~80s). Liveness now also requires the later of the socket's accept time (stored in its attachment so it survives hibernation) and the runtime's last heartbeat auto-response timestamp to be within TUNNEL_STALE_MS (50s, two missed 20s client pings plus slack). Stale sockets are closed so visitors get the offline page immediately, their in-flight streams fail at once instead of waiting out the timeout, and the presence alarm stops advertising them. A socket accepted before this change (no timestamp of either kind) is stamped on first sight and earns one grace window. The client heartbeat deadline drops from 60s to 45s so a real drop is detected at the third tick (60s) rather than the fourth (80s), one tick after the relay goes offline. Co-Authored-By: Claude <noreply@anthropic.com>
The tunnel dial negotiates permessage-deflate and every relayed frame rode it, including body chunks whose origin response was already brotli/gzip encoded (static assets, compressed API JSON). Deflating those again cost CPU per chunk on the host and slightly grew the frames. TunnelSession.send now takes a compress option; executeHttp passes compress: false for body chunks when the origin response carries a non-identity Content-Encoding. Identity bodies and all control frames keep the extension. This deviates from the "all body-chunk frames" suggestion so uncompressed origin bodies (small responses below the server's compression threshold, plain-text tool output) still benefit from deflate on the wire. The host daemon's hand-written ws type shim gains the send options overload so it typechecks the shared session. Co-Authored-By: Claude <noreply@anthropic.com>
startVisitorAuth fires the session verification before the label lookup so the two D1 round trips overlap. When the gate returns before awaiting it (unknown label 404, machine info page served from the label cache), the pending D1 read was left dangling with no waitUntil, so the request context could close on it. session.ts now shares that pending lookup with every later request carrying the same cookie, so a never-settling promise would strand those requests. Register the promise with ctx.waitUntil, as the gate already does for markMachineSeen, and cover the early-return path. Co-Authored-By: Claude <noreply@anthropic.com>
SawyerHood
marked this pull request as ready for review
August 19, 2026 08:04
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this PR now. I will check security, code quality, performance, architecture, duplicate code, and the changed route. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| cookieHeader, | ||
| runtime.desktopSessionCookieName, | ||
| ); | ||
| const visitorAuth = isTunnelDial |
There was a problem hiding this comment.
🚨 slopcop/review — Medium: Bound the eager-auth cache.
This code starts session checks before the route needs them. Unknown labels and public routes still run HMAC work for each dotted cookie. verifySessionCookie caches each unique invalid value. Expired keys remain until the same key returns. Public requests can grow the isolate memory without a limit. Please skip auth for public routes and add a cache limit or cleanup.
| // last heartbeat it answered (TUNNEL_STALE_MS in apps/connect), so the | ||
| // client redials within one tick of the relay showing its visitors the | ||
| // offline page instead of ~30s later. | ||
| const HEARTBEAT_DEADLINE_MS = 45_000; |
There was a problem hiding this comment.
🚨 slopcop/review — Low: Add a direct heartbeat timer test.
This deadline changes the disconnect time from 80 seconds to 60 seconds. Current tests do not check the new third-tick boundary. Please add a fake-timer test that confirms the socket closes at the third interval.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary
This PR makes the remote bb connection path faster and more reliable. It combines duplicate database reads and rejects old sockets sooner. It also removes external gate fonts and avoids repeat compression.
Findings
- Medium: Visitor auth starts before the route needs it. Unknown labels and public routes still run HMAC work. Unique invalid cookies also fill an unbounded cache. Skip auth for public routes, and add a cache limit or cleanup.
- Low: The new heartbeat deadline changes the disconnect time from 80 seconds to 60 seconds. Add a direct timer test for this boundary.
Other review results
- I found no authentication bypass, trust-boundary error, or compression security issue.
- The socket freshness and compression paths have no other performance issue.
shareInflightduplicates the servercreateAsyncDeduperpattern. This is a small refactor option, not a required change.- The client and relay keep related heartbeat times in separate packages. A shared contract could prevent future drift.
- The host daemon protocol version does not need a change. This PR changes no server-daemon wire data.
Checks
- Connect passed 113 tests.
- Tunnel client passed 15 tests.
- Connect, tunnel client, and host daemon passed Turbo type checks.
- The local Wrangler gate returned the expected 401 sign-in page.
- Browser QA confirmed zero external stylesheets and the system font stack.
git diff --check origin/main...HEADpassed.
I posted the two specific items as inline comments. I used a comment review, as required by this rule.
SawyerHood
marked this pull request as draft
August 19, 2026 15:09
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
1 participant
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.
What was wrong
readyState === OPENas live. A client whose network died without a close frame left a zombie that still acceptedsend(). Every visitor request was proxied into it and hung 30 s before a 504, for as long as the zombie lingered (up to ~80 s).What changed
apps/connect/src/worker.ts: visitor cookie verification starts beforeawait resolveLabeland overlaps it (skipped on tunnel dials).apps/connect/src/session.ts: label and session caches also share the in-flight promise; a rejected lookup is dropped so the next request retries.apps/connect/src/tunnel-do.ts: liveness now requiresmax(acceptedAt, getWebSocketAutoResponseTimestamp) <= TUNNEL_STALE_MS (50 s).acceptedAtis stored in the socket attachment (survives hibernation). Stale sockets are closed; if no live socket remains, in-flight streams fail at once and the presence alarm stops. Sockets accepted before this change are stamped on first sight and get one grace window.packages/tunnel-client: heartbeat deadline 60 s -> 45 s (detects a dead link at the third 20 s tick, 60 s, instead of 80 s).TunnelSession.sendtakes acompressoption; body chunks for responses with a non-identityContent-Encodingare sent withcompress: false. Identity bodies and control frames keep deflate (deviation from "all body-chunk frames": small uncompressed responses still benefit).apps/host-daemon/src/ws.d.tsgains thesend(data, options)overload.How you verified
apps/connect/src/worker.test.ts(gate lookup overlap: session verified while label pending, no session check on tunnel dial, no unhandled rejection on 404; gate page has no font link; TunnelDO zombie sockets: 503 + close for stale, live with recent heartbeat, live just after accept, legacy grace window, in-flight streams fail immediately, route around zombie to fresh replacement without abandoning streams, alarm drops presence).apps/connect/src/session.test.ts(5 concurrent resolves -> 1 query; fresh not shared; rejected lookup retried; 5 concurrent session verifications -> 1 query; real in-memory SQLite).packages/tunnel-client/test/session-compress.test.ts(precompressed body chunkscompress:false, identitycompress:true, control frames compress;isPrecompressedResponseparsing).pnpm exec turbo run typecheck --filter=@bb/connect --filter=@bb/tunnel-client --filter=bb-plugin-connect --filter=@bb/host-daemon: 4/4 successful.pnpm exec turbo run test --filter=@bb/connect --filter=@bb/tunnel-client --filter=bb-plugin-connect: 112 / 15 / 78 passed.pnpm exec turbo run test --filter=@bb/host-daemon -- src/connect-tunnel: 8 passed.pnpm exec eslintandprettier --checkon changed files: clean.Fixes: part of the mobile / iOS Safari performance program (verified sweep report in the bb thread; no single issue).
Stack context
Standalone PR (no overlap with the
bb/mobile-perf/*stack); targetsmain.{acceptedAt}(previously none); sockets without it are handled (stamped on first sight). No CLI/config/plugin-API surface changes.