Repository navigation
Drop every second socket error that has no listener - #250
Conversation
workerd emits a socket error twice. The message is not always the proxy failure. A TLS failure or a reset can abort the isolate the same way. Co-authored-by: Jared Wray <me@jaredwray.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #250 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 44 44
Lines 1723 1720 -3
Branches 385 386 +1
=========================================
- Hits 1723 1720 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 751328b7c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (event === "error" && this.listenerCount("error") === 0) { | ||
| return false; |
There was a problem hiding this comment.
When a socket emits its first error without a listener, this branch returns false and bypasses Node's unhandled-error behavior. The Worker installs this patch globally, so unrelated sockets can lose real failures, not only workerd's duplicate emit. Record whether this socket emitted an error. Suppress the emit only when that record exists. The changed test uses listener-free emits only after one handled emit, so it does not cover this case.
AGENTS.md reference: AGENTS.md:L68-L71
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The guard now records each socket after its first error. A later error with no listener is dropped. The first error with no listener still throws. A later error that has a listener is still delivered. Commit cecd702. pnpm test passed: 562 tests, line coverage 100%.
The guard now records a socket after its first error. A later error with no listener is dropped. The first error still throws. Co-authored-by: Jared Wray <me@jaredwray.com>
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix. Follow-up to #249. The socket guard drops a later
errorthat has no listener. The firsterrorstill uses Node behavior.Why
#249 is merged. Its guard dropped a second socket
erroronly when the message containedproxy request failed. workerdnode:netemitserrortwice for one failed socket. The driver listens withonce, so the second emit has no listener and aborts the isolate. The message is not always the proxy failure. A TLS failure or a reset can abort the isolate the same way.The guard records each socket after its first
error. A latererrorwith no listener is dropped. The firsterrorwith no listener still throws. A latererrorthat has a listener is still delivered.tls.TLSSocketuses the sameemit.Verification
pnpm test: 562 tests passed. Line coverage 100% (1439/1439).error, a second proxyerror, a TLS handshakeerror, and a latererrorthat still has a listener.Test audit
No test was deleted. The socket guard test was extended. One new test proves the first listener-less
errorstill throws. The old suite did not cover that case.Do not merge, release, or deploy this PR.