/bonk review

Review posted on PR #6533 with 3 inline comments. Here's a summary of the findings:

[HIGH] Use-after-free on addressStr (sockets.c++:299): The doConnect coroutine captures addressStr as a non-owning kj::StringPtr, but the owning kj::String is moved into setupSocket() on line 323 after the coroutine is created. When the coroutine resumes after co_await ioContext.waitForOutputLocks(), it dereferences a dangling pointer. Fix: have the coroutine take ownership of the string (change parameter to kj::String).

[MEDIUM] ConnectionData member destruction order (sockets.h:189-192): The declared order puts connectTask before tlsStarter, meaning tlsStarter is destroyed first (reverse order). This contradicts the PR description's stated intent of cancelling the connect task before tlsStarter is destroyed. While currently safe due to coroutine cancellation semantics, the ordering should be swapped to make the safety guarantee explicit and resilient to future changes.

[LOW] Misleading function name (sockets.c++:354): The comment on connectImpl explains the output gate is now handled inside connectImplNoOutputLock, but the function name implies it does not handle output locks.

github run

What is the rationale for doing this? Why do we need to prevent network outputs while storage writes are pending?

That's how output gates work. We don't want applications to have to wait for writes to be confirmed durable before we allow them to continue executing. Instead, we make it impossible to observe that the app has continued executing until the write has become durable. If the write ends up failing, then we prevent the rest of the world from ever knowing that the app kept running -- so from the PoV of the rest of the world, the app stopped exactly where the failed write was performed. This is a huge optimization because it allows an app to perform several writes in rapid succession with only one round trip to durable storage (SRS followers).

https://blog.cloudflare.com/durable-objects-easy-fast-correct-choose-three/

shouldn't only writes be blocked instead?

Anything that can be observed by the outside world needs to be blocked. That means we have to block both writes and connection initiation -- the remote end can observe that it has received a connection, and in theory you could imagine a protocol where merely connecting to a remote service serves to notify it of something, thus having side effects.

/bonk review

Review posted on PR #6533 with 2 inline comments. Here's a summary:

All three previous findings are resolved by the rewrite to use newPromisedWorkerInterface():

• Previously HIGH (use-after-free) — eliminated entirely. The HttpClientAdapter copies host/headers internally, so the deferred coroutine operates on owned copies.
• Previously MEDIUM (destruction order) — the connectTask field was removed.
• Previously LOW (misleading name) — acknowledged with TODO(cleanup).

New findings (both LOW):

1. Comment on sockets.c++:261 exceeds typical line width — suggested wrapping.
2. Lambda body indentation in sockets-test.c++:80-82 is inconsistent — suggested fix.

The newPromisedWorkerInterface() approach is correct and much simpler than the previous fulfiller-based approach. The KJ HTTP adapter layer handles all string ownership internally, making the code safe without any manual lifetime management.

github run

/bonk review

Review posted on PR #6533 with 4 inline comments. Here's the summary:

All three findings from my previous reviews are resolved by the rewrite to newPromisedWorkerInterface().

New findings (2 MEDIUM, 2 LOW):

1. [MEDIUM] handleProxyStatus error-preservation pattern (sockets.c++:434-443) — uses a heap-allocated Maybe<kj::Exception> accessed via raw reference across two lambdas. Safe today because awaitIo guarantees func outlives errorHandler, but the lifetime dependency is implicit. Also, TypeErrors from rejected connects now get logged as "Socket proxy disconnected abruptly" when the autogate is on, which is misleading.

2. [MEDIUM] waitForOutputLocks() vs waitForOutputLocksIfNecessary() (sockets.c++:263) — the current code always allocates a promise even in non-actor contexts. Minor optimization opportunity, though READY_NOW is essentially free.

3. [LOW] Test co_await kj::evalLater pattern (sockets-test.c++:104-108) — correct but could use a comment explaining why a single turn suffices.

4. [LOW] Server-test branch duplication (server-test.c++:1258-1269) — the KJ_EXPECT_LOG can be hoisted before the shared connection logic to eliminate the else branch.

github run

/bonk review

Review posted successfully on PR #6533 with 3 inline comments. Here's a summary of what was posted:

Resolved from prior reviews:

• Previously HIGH (use-after-free on addressStr) — eliminated by the newPromisedWorkerInterface() rewrite
• Previously MEDIUM (ConnectionData destruction order) — connectTask field removed entirely
• Previously LOW (misleading function name) — acknowledged with TODO(cleanup)

New findings posted:

1. [MEDIUM] handleProxyStatus error-preservation pattern (sockets.c++:443) — the originalErrorRef raw reference captured by errorHandler has an implicit lifetime dependency on func owning originalError. Safe today but fragile. Also noted that TypeErrors now go through the misleading "Socket proxy disconnected abruptly" log path.
2. [LOW] Server-test branch duplication (server-test.c++:1258-1269) — the KJ_EXPECT_LOG can be hoisted before the shared connection logic to eliminate the else branch.
3. [LOW] Test lambda indentation (sockets-test.c++:81-84) — inconsistent alignment with suggested fix.

github run

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

Hmm I was not expecting my approval to dismiss @dom96's review, I was expecting him to take another look and say if he is happy with it now.