Skip to content

EW-9330 Implement connect() handler - #6059

Merged
fhanau merged 1 commit into
mainfrom
felix/experimental-tcp-ingress
Mar 23, 2026
Merged

EW-9330 Implement connect() handler#6059
fhanau merged 1 commit into
mainfrom
felix/experimental-tcp-ingress

Conversation

@fhanau

@fhanau fhanau commented Feb 11, 2026

Copy link
Copy Markdown
Contributor

This PR implements support for defining a connect() handler, which is behind an experimental compat flag for now. This is largely based on James' previous PR for this (#1429), but includes changes to switch to the socket-based API, drop the deferred proxying code, add more tests, add proper tracing support etc. I don't have a strong background on how workerd/KJ streams work yet – there might be trivial issues in how streams are being used. You'll notice that this PR differs from the internal spec in that it still provides an event to the tail handler and there are several other issues – see the open questions below where I'm looking for input on how to best address this.

Open questions:

  • This still provides a ConnectEvent with a cfJson field to the JS connect handler: Based on the internal discussion, a cfJson field should not be needed, but the parameters set to be provided in server.c++ (clientIp, clientPid, clientUid) feel like they might be useful – should there be a different way to provide such metadata? Otherwise I'll change this to just provide a plain socket as the first parameter to the handler (as discussed on the internal spec)
  • [Unchanged from previous PR] The headers provided by the incoming connect() call should be passed through instead of being discarded, right?
  • We currently construct a neuterable stream in ServiceWorkerGlobalScope::connect() without really needing to do so – there should be a cleaner way to get an owned AsyncIoStream?
  • I assume that HTTP response codes are not needed for TCP? This is relevant for the tail worker return event, which has an optional return code.
  • Is there cleanup potential in WorkerEntrypoint::connect()? A lot of this is adapted from request(), there's likely some logic towards the end that's only needed when doing deferred proxying
  • Do we need to use HttpRewriter at all for TCP in server.c++? server.c++'s TcpListener has remnants of support for this left, but since it's not dealing with HTTP, rewriting might not be needed
  • Windows tests are failing – perhaps related to EW-9330 Implement connect() handler #6059 (comment)?
  • Miniflare tests are failing – looks like the new code path actually does get enabled for them based on having the "experimental" compat flag. As seen in https://github.com/cloudflare/workerd/actions/runs/22805783114/job/66155261833?pr=6059 (CI run on commit 7713c75 where the wrangler CI jobs were put in a different order), the miniflare tests are the only failing ones Add support for worker connect handler in miniflare workers-sdk#12775 has been merged, the next daily miniflare should be sufficient to fix CI and unblock this PR.
  • I'll clean up/squash commits before merge.

The following assumptions were made in implementing this:

  • The affected tests have been marked with the Bazel "exclusive" tag and will run one at a time to avoid flakes due to tests trying to use the same TCP port at the same time – I considered using different ports in different tests but we'd still have the issue between regular and @all-autogates variants; the Bazel sandbox might also offer ways to work around this but as long as Bazel does not support sandboxing on Windows we'd still be facing flakes in CI. If there's concerns about a CI slowdown here, I can implement a mechanism to provide a random unused port instead.
  • I'm assuming that with the socket-based API, neuterable streams are not needed since deferred proxying does not need to be implemented.
  • We may want TLS support in the future, but this is not included for now.
  • I'm assuming that connect_handler doesn't need to be a user span for now, just how fetch_handler isn't one.
  • Kenton suggested adding a proxyTo() function – I've implemented this but it can land separately, this is available on the felix/030226-proxyTo branch.
  • After factoring out some code shared with the HTTP/fetch code path there's some remaining code duplication – lmk if you can point to any more spots that would be good candidates for deduplication that wouldn't make code harder to maintain.

@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch 2 times, most recently from 0143a03 to 01fb3c7 Compare February 17, 2026 00:30
@codspeed-hq

codspeed-hq Bot commented Feb 17, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 70 untouched benchmarks
⏩ 129 skipped benchmarks1


Comparing felix/experimental-tcp-ingress (04e4c46) with main (5279734)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch from 01fb3c7 to 545a1ab Compare February 17, 2026 20:49
@github-actions

github-actions Bot commented Feb 17, 2026

Copy link
Copy Markdown

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch 2 times, most recently from 3948217 to 6303fbd Compare February 25, 2026 22:24
Comment thread src/workerd/api/tests/connect-handler-test.js
Comment thread src/workerd/api/tests/connect-handler-test.js Outdated
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/api/global-scope.h Outdated
@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch 3 times, most recently from 26ab0db to fe861bd Compare March 3, 2026 18:29
@fhanau fhanau changed the title [DRAFT] EW-9330 Implement connect() handler EW-9330 Implement connect() handler Mar 3, 2026
@fhanau
fhanau marked this pull request as ready for review March 3, 2026 18:32
@fhanau
fhanau requested review from a team as code owners March 3, 2026 18:32
Comment thread types/generated-snapshot/experimental/index.ts Outdated
Comment thread src/workerd/api/global-scope.h Outdated
Comment thread samples/tcp-ingress/worker.js Outdated
Comment thread src/workerd/api/tests/connect-handler-test-proxy.js

@jasnell jasnell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review findings:

  • [HIGH] NeuterableIoStream made globally refcounted — affects all existing users
  • [HIGH] isDefaultFetchPort logic semantically wrong for TCP ingress
  • [HIGH] exceptionToPropagate has external linkage; should be static or in anonymous namespace
  • [MEDIUM] Stack-allocated HttpHeaderTable in connect handler; should reuse existing table
  • [MEDIUM] TODO(now) needs resolution before merge
  • [MEDIUM] KJ_LOG(WARNING) on every accepted TCP connection is noisy
  • [MEDIUM] Unused rewriter member in TcpListener
  • [LOW] Misnamed variable in writeOnsetInfo trace code
  • [LOW] Test decodes chunks twice
  • [LOW] ConnectHandler parameter named connect shadows member name

This review was generated with AI assistance and may contain inaccuracies.

Comment thread src/workerd/util/stream-utils.h Outdated
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/io/worker-entrypoint.c++
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/io/worker-entrypoint.c++ Outdated
Comment thread src/workerd/server/server.c++
Comment thread src/workerd/server/server.c++
Comment thread src/workerd/io/trace.c++ Outdated
Comment thread src/workerd/api/tests/connect-handler-test.js Outdated
Comment thread src/workerd/api/global-scope.h Outdated
@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch 2 times, most recently from b626b42 to 027d7d8 Compare March 6, 2026 22:11
Comment thread samples/tcp-ingress/worker.js
Comment thread src/workerd/server/server.c++ Outdated
Comment thread src/workerd/server/server.c++ Outdated
@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch 2 times, most recently from e696e2f to 0b46650 Compare March 11, 2026 15:55
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/api/global-scope.c++ Outdated

@dom96 dom96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Comment thread samples/tcp-ingress/worker.js
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/api/global-scope.c++ Outdated
Comment thread src/workerd/io/worker-entrypoint.c++
Comment thread src/workerd/io/worker-interface.capnp Outdated
Comment thread src/workerd/server/server.c++
Comment thread src/workerd/server/workerd.capnp
@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch 2 times, most recently from 46a18de to 3b66e00 Compare March 16, 2026 23:40
@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch from 946fc30 to e8b3d5b Compare March 17, 2026 21:59
@codecov-commenter

codecov-commenter commented Mar 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 67.60563% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.64%. Comparing base (5279734) to head (04e4c46).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/workerd/io/worker-entrypoint.c++ 61.17% 32 Missing and 1 partial ⚠️
src/workerd/api/trace.c++ 0.00% 8 Missing and 2 partials ⚠️
src/workerd/io/trace.c++ 59.09% 7 Missing and 2 partials ⚠️
src/workerd/api/global-scope.c++ 65.21% 5 Missing and 3 partials ⚠️
src/workerd/server/server.c++ 87.03% 6 Missing and 1 partial ⚠️
src/workerd/api/sockets.c++ 81.81% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6059      +/-   ##
==========================================
+ Coverage   70.63%   70.64%   +0.01%     
==========================================
  Files         425      425              
  Lines      116651   116826     +175     
  Branches    18853    18881      +28     
==========================================
+ Hits        82391    82531     +140     
- Misses      23023    23047      +24     
- Partials    11237    11248      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch 2 times, most recently from 939fd78 to 78df968 Compare March 17, 2026 23:11
Comment thread samples/tcp-ingress/config.capnp Outdated
Comment thread samples/tcp-ingress/worker.js
Comment thread src/workerd/api/global-scope.h Outdated
@fhanau
fhanau force-pushed the felix/experimental-tcp-ingress branch from 78df968 to e8d3178 Compare March 23, 2026 22:06
Requires experimental flag for now. Implemented using a socket-based interface,
unlike the original PR. See samples/tcp-ingress for an example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants