Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/workerd/api/tests/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ wd_test(
tags = ["exclusive"],
)

wd_test(
src = "socket-closed-test.wd-test",
args = ["--experimental"],
data = ["socket-closed-test.js"],
# Test uses TCP socket on port 8084 and may fail when running concurrently with other
# tests that do so.
tags = ["exclusive"],
)

wd_test(
src = "actor-alarms-test.wd-test",
args = ["--experimental"],
Expand Down
98 changes: 98 additions & 0 deletions src/workerd/api/tests/socket-closed-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// Copyright (c) 2017-2022 Cloudflare, Inc.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[LOW] Copyright year range should be updated for a new file.

Suggested change
// Copyright (c) 2017-2022 Cloudflare, Inc.
// Copyright (c) 2017-2026 Cloudflare, Inc.

// Licensed under the Apache 2.0 license found in the LICENSE file or at:
// https://opensource.org/licenses/Apache-2.0

// Reproduction tests for: socket.closed rejects with undefined when socket.close()
// is called after remote FIN.
//
// When the remote peer sends FIN (closes its write side) with allowHalfOpen=false
// (the default), calling socket.close() after draining the readable to EOF causes
// socket.closed to reject with undefined instead of resolving.
//
// Two code paths race:
// Path A (handleReadableEof): queues maybeCloseWriteSide as a microtask on EOF
// Path B (Socket::close): calls writable abort(js, kj::none) synchronously
//
// Path B runs first (synchronous after for-await exits), putting the writable into
// Errored(undefined). When Path A's microtask fires, close(js) fails because the
// writable is errored, and the catch handler rejects closedResolver with undefined.

import { connect } from 'cloudflare:sockets';
import { strictEqual, notStrictEqual } from 'assert';

// Server: writes a small payload then closes (sends FIN).
export default {
async connect(socket) {
const writer = socket.writable.getWriter();
await writer.write(new TextEncoder().encode('ping'));
await writer.close();
},
};
Comment on lines +25 to +30
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[MEDIUM] The connect handler has an unhandled promise rejection. When the client-side test calls socket.close() and tears down the connection, the server-side writer.close() on line 28 rejects with Error: Network connection lost. This shows up in all CI logs as:

workerd/io/worker.c++:2452: info: uncaught exception; source = Uncaught (in promise); stack = Error: Network connection lost.
    at async Object.connect (worker:28:5)

Wrapping the body in a try/catch would silence this:

Suggested change
async connect(socket) {
const writer = socket.writable.getWriter();
await writer.write(new TextEncoder().encode('ping'));
await writer.close();
},
};
async connect(socket) {
const writer = socket.writable.getWriter();
try {
await writer.write(new TextEncoder().encode('ping'));
await writer.close();
} catch {
// Client may disconnect before close completes — expected in these tests.
}
},


// Test 1 (core bug): socket.close() after EOF causes socket.closed to reject with undefined.
// The for-await exits on EOF, then socket.close() races with the maybeCloseWriteSide microtask.
export const closedRejectsUndefinedAfterCloseRace = {
async test() {
const socket = connect('localhost:8084');
await socket.opened;

const dec = new TextDecoder();
let data = '';
for await (const chunk of socket.readable) {
data += dec.decode(chunk, { stream: true });
}
data += dec.decode();
strictEqual(data, 'ping');

// This is what application cleanup code does — close the socket after the read loop.
// It races with maybeCloseWriteSide (triggered by EOF with allowHalfOpen=false).
socket.close();

// socket.closed must resolve (not reject with undefined).
try {
await socket.closed;
} catch (e) {
notStrictEqual(
e,
undefined,
'socket.closed rejected with undefined — should resolve or reject with a real Error'
);
}
},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[HIGH] This test correctly reproduces the bug from #6479, but since the bug is not yet fixed, the test fails at @all-compat-flags across all CI platforms, breaking the build.

The @all-compat-flags variant (compat date 2999-12-31) enables internal_writable_stream_abort_clears_queue (field @57, enable date 2024-09-02). With this flag, Socket::close()writable->getController().abort(js, kj::none) immediately calls drain()doError(js, undefined), putting the writable into Errored(undefined) state synchronously. Then when the maybeCloseWriteSide microtask fires, isClosedOrClosing() returns false (it doesn't check for Errored state), so it calls close() on the errored stream, which rejects with undefined, and the catch handler rejects closedResolver with undefined.

Without the flag (at the default compat date 2000-01-01), the lazy abort path handles things differently and the race doesn't manifest — that's why @ and @all-autogates pass.

Per danlapid's comment, the specific flag to investigate is internal_writable_stream_abort_clears_queue. Adding "internal_writable_stream_abort_does_not_clear_queue" to the test's compatibilityFlags would confirm this and make @all-compat-flags pass — though the real fix belongs in Socket::close() or maybeCloseWriteSide.

};

// Test 2: if socket.closed rejects via any path, the reason must not be undefined.
export const closedRejectionReasonIsNeverUndefined = {
async test() {
const socket = connect('localhost:8084');
await socket.opened;

// Close immediately — don't drain readable.
socket.close();

try {
await socket.closed;
} catch (e) {
notStrictEqual(e, undefined, 'socket.closed rejected with undefined');
}
},
};

// Test 3: without socket.close(), a clean remote FIN should resolve socket.closed.
export const closedResolvesAfterRemoteEofWithoutExplicitClose = {
async test() {
const socket = connect('localhost:8084');
await socket.opened;

const dec = new TextDecoder();
let data = '';
for await (const chunk of socket.readable) {
data += dec.decode(chunk, { stream: true });
}
data += dec.decode();
strictEqual(data, 'ping');

// Do NOT call socket.close() — let maybeCloseWriteSide handle everything.
await socket.closed;
},
};
18 changes: 18 additions & 0 deletions src/workerd/api/tests/socket-closed-test.wd-test
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
using Workerd = import "/workerd/workerd.capnp";

const unitTests :Workerd.Config = (
services = [
( name = "socket-closed-test",
worker = (
modules = [
(name = "worker", esModule = embed "socket-closed-test.js"),
],
compatibilityFlags = ["nodejs_compat_v2", "experimental"],
)
),
( name = "internet", network = ( allow = ["private"] ) )
],
sockets = [
(name = "tcp", address = "*:8084", tcp = (), service = "socket-closed-test"),
]
);
Loading