Skip to content

EW-9330 Implement connect() handler#6059

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

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

Conversation

@fhanau
Copy link
Copy Markdown
Contributor

@fhanau fhanau commented Feb 11, 2026

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
Copy link
Copy Markdown

codspeed-hq bot commented Feb 17, 2026

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
Copy link
Copy Markdown

github-actions bot commented Feb 17, 2026

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
@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
Copy link
Copy Markdown
Collaborator

@jasnell jasnell left a comment

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.

@fhanau fhanau force-pushed the felix/experimental-tcp-ingress branch 2 times, most recently from b626b42 to 027d7d8 Compare March 6, 2026 22:11
@fhanau
Copy link
Copy Markdown
Contributor Author

fhanau commented Mar 10, 2026

I believe I've addressed all suggestions, this is ready for re-review.

@fhanau fhanau force-pushed the felix/experimental-tcp-ingress branch 2 times, most recently from e696e2f to 0b46650 Compare March 11, 2026 15:55
Copy link
Copy Markdown
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

LGTM.

@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
Copy link
Copy Markdown

codecov-commenter commented Mar 17, 2026

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
@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
@fhanau fhanau force-pushed the felix/experimental-tcp-ingress branch from e8d3178 to 04e4c46 Compare March 23, 2026 22:29
@fhanau fhanau merged commit fa045bd into main Mar 23, 2026
22 of 23 checks passed
@fhanau fhanau deleted the felix/experimental-tcp-ingress branch March 23, 2026 23:04
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