Skip to content

EW-9327 Implement connection string override support#6742

Draft
fhanau wants to merge 1 commit intomainfrom
felix/050426-connect-EW-9327
Draft

EW-9327 Implement connection string override support#6742
fhanau wants to merge 1 commit intomainfrom
felix/050426-connect-EW-9327

Conversation

@fhanau
Copy link
Copy Markdown
Contributor

@fhanau fhanau commented May 8, 2026

draft/proof-of-concept, not yet ready for review – some more internal discussion is needed. See the downstream PR for more context.

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

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

I'm Bonk, and I've done a quick review of your PR.

This PR adds host and port properties to Fetcher to support connection string override via a connect-override mechanism similar to Hyperdrive.

Issues found (ranked by severity):

  1. [HIGH] New Fetcher properties shadow JS RPC wildcard and are not compat-flag-gatedhost and port are added unconditionally to all Fetcher instances, including DurableObject stubs. The comment at line 453 explicitly warns about this.
  2. [HIGH] self->port is jsg::Optional<uint16_t> but passed un-unwrapped to kj::str() — The connect override lambda will stringify a kj::Maybe rather than a uint16_t.
  3. [MEDIUM] getPort() lacks KJ_LIFETIMEBOUND — minor, but noted per review checklist for methods returning views.

This review was generated by an AI assistant and may contain errors. Please verify all suggestions before applying.

Comment thread src/workerd/api/http.h
Comment on lines +464 to +465
JSG_LAZY_READONLY_INSTANCE_PROPERTY(host, getHost);
JSG_LAZY_READONLY_INSTANCE_PROPERTY(port, getPort);
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.

[HIGH] Missing compat flag gate — shadows JS RPC wildcard property.

The comment right above (lines 453–459) warns:

WARNING: New JSG_METHODs on Fetcher must be gated via compatibility flag to prevent conflicts with JS RPC methods (implemented via the wildcard property).

This applies equally to properties. JSG_LAZY_READONLY_INSTANCE_PROPERTY registers concrete property names (host, port) that will shadow JSG_WILDCARD_PROPERTY(getRpcMethod). Since DurableObject extends Fetcher, a DO stub whose RPC target exposes a host or port property/method will silently get undefined (the Fetcher property with no port set) instead of making the RPC call.

These should be gated behind a compat flag, or alternatively moved to a subclass that only the connection-string-override Fetcher uses (not all Fetchers globally).

Comment thread src/workerd/api/http.c++
// fetcher.
auto& globalScope = IoContext::current().getCurrentLock().getGlobalScope();
globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", getPort()),
[self = JSG_THIS](jsg::Lock& js) mutable { return self->connect(js, kj::str(KJ_ASSERT_NONNULL(self->randomHost), ":", self->port), kj::none); });
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.

[HIGH] self->port is jsg::Optional<uint16_t> — stringifying a kj::Maybe instead of the unwrapped value.

self->port has type jsg::Optional<uint16_t> (which is kj::Maybe<uint16_t>). Passing it to kj::str() will not produce the numeric string you want — it will format the Maybe wrapper. Compare with the Hyperdrive implementation where getPort() returns a plain uint16_t.

Suggested change
[self = JSG_THIS](jsg::Lock& js) mutable { return self->connect(js, kj::str(KJ_ASSERT_NONNULL(self->randomHost), ":", self->port), kj::none); });
[self = JSG_THIS](jsg::Lock& js) mutable { return self->connect(js, kj::str(KJ_ASSERT_NONNULL(self->randomHost), ":", KJ_ASSERT_NONNULL(self->port)), kj::none); });

Comment thread src/workerd/api/http.c++
// cloudflare:sockets's connect() will route connections to this magic hostname through the
// fetcher.
auto& globalScope = IoContext::current().getCurrentLock().getGlobalScope();
globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", getPort()),
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.

[LOW] Consider using getPort() rather than accessing port field directly for consistency.

The Hyperdrive implementation calls getPort() for both the setConnectOverride key and the lambda body. Here you mix getPort() (line 2091) and direct self->port access (line 2092). Using self->getPort() in the lambda would also need unwrapping but would be more consistent.

Also: this line calls getPort() which returns jsg::Optional<uint16_t>kj::str() with a Maybe will produce unexpected output here too. Should unwrap:

Suggested change
globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", getPort()),
globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", KJ_ASSERT_NONNULL(port)),

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 8, 2026

I'm Bonk, and I've done a quick review of your PR.

This PR adds host and port properties to Fetcher to support connection string override via a connect-override mechanism similar to Hyperdrive. I found 2 high-severity and 1 medium-severity issue and posted them as inline review comments on PR #6742:

  1. [HIGH] host and port properties are not compat-flag-gated — They're added unconditionally to all Fetcher instances (including DurableObject stubs), directly violating the warning comment at http.h:453-459. These instance properties shadow the JSG_WILDCARD_PROPERTY(getRpcMethod), meaning any DO RPC target with a host or port property/method will silently get undefined instead of the RPC call.

  2. [HIGH] self->port passed to kj::str() without unwrappingport is jsg::Optional<uint16_t> (i.e., kj::Maybe<uint16_t>), so stringifying it directly won't produce the expected numeric string. Needs KJ_ASSERT_NONNULL(self->port) like the Hyperdrive pattern uses.

  3. [MEDIUM] Same unwrap issue on line 2091getPort() returns jsg::Optional<uint16_t>, not a plain uint16_t, so the setConnectOverride key string also needs unwrapping.

github run

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.

1 participant