quic: apply multiple review fixups

Author: jasnell

lib/internal/quic/quic.js

@@ -33,6 +33,7 @@ const kEarlyDataRejected = Symbol('kEarlyDataRejected');
 const kFinishClose = Symbol('kFinishClose');
 const kGoaway = Symbol('kGoaway');
 const kHandshake = Symbol('kHandshake');
+const kHandshakeCompleted = Symbol('kHandshakeCompleted');
 const kHeaders = Symbol('kHeaders');
 const kKeylog = Symbol('kKeylog');
 const kListen = Symbol('kListen');
@@ -66,6 +67,7 @@ module.exports = {
   kFinishClose,
   kGoaway,
   kHandshake,
+  kHandshakeCompleted,
   kHeaders,
   kInspect,
   kKeylog,

lib/internal/quic/symbols.js

@@ -0,0 +1,66 @@
+// Flags: --experimental-quic --no-warnings
+
+// Test: When `endpoint.destroy(err)` is called *after* `endpoint.close()`
+// has already initiated graceful shutdown, the error must still surface
+// on `endpoint.closed` instead of being swallowed. First-error-wins
+// semantics: a fatal error reported via `destroy(err)` after a
+// graceful close was already in flight is still propagated through
+// `endpoint.closed`.
+
+import { hasQuic, skip, mustCall } from '../common/index.mjs';
+import assert from 'node:assert';
+
+const { rejects } = assert;
+
+if (!hasQuic) {
+  skip('QUIC is not enabled');
+}
+
+const { listen, connect } = await import('../common/quic.mjs');
+
+// Hold the connection open for a while so that close() has actual work
+// to wait on.
+const transportParams = { maxIdleTimeout: 5000 };
+
+const serverDone = Promise.withResolvers();
+
+const serverEndpoint = await listen(mustCall(async (serverSession) => {
+  // Don't observe `serverSession.closed` directly here - the cascade
+  // from `endpoint.destroy(err)` will reject it, and the outer
+  // `markPromiseAsHandled` in the cascade keeps that from surfacing as
+  // an unhandled rejection. We just need this callback to keep running
+  // until the test ends.
+  try {
+    await serverSession.closed;
+  } catch {
+    // Expected: the cascade destroys the session with an error.
+  }
+  serverDone.resolve();
+}), { transportParams });
+
+const clientSession = await connect(serverEndpoint.address, {
+  transportParams,
+});
+await clientSession.opened;
+
+// Step 1: kick off a graceful close. After this, the endpoint is in
+// the "closing" state - so `#isClosedOrClosing` is true and the
+// pre-fix `destroy()` would skip recording `#pendingError`.
+const closingPromise = serverEndpoint.close();
+
+// Step 2: while the graceful close is in flight, call destroy(err).
+// With the fix, the error is still recorded and surfaces on
+// `endpoint.closed`. Without the fix, it'd be silently dropped.
+const destroyError = new Error('destroy after close');
+const closedAssertion = rejects(serverEndpoint.closed, destroyError);
+
+serverEndpoint.destroy(destroyError);
+
+await closedAssertion;
+
+// `endpoint.close()` returns the same promise as `endpoint.closed`, so
+// it should reject with the same error as well.
+await rejects(closingPromise, destroyError);
+
+await serverDone.promise;
+clientSession.destroy();

test/parallel/test-quic-endpoint-destroy-after-close.mjs

@@ -0,0 +1,86 @@
+// Flags: --experimental-quic --no-warnings
+
+// Test: when `endpoint.destroy(err)` is called on a side that has open,
+// fully-handshaked sessions, the cascade through `session.destroy(err)`
+// passes close options derived from the error so each session emits a
+// CONNECTION_CLOSE on the wire. The peer learns about the teardown via
+// that frame, not via its own idle timer.
+//
+// How the test distinguishes the two cases:
+//
+//   * If CONNECTION_CLOSE is sent, the client's `session.closed`
+//     rejects after the network round-trip with an
+//     `ERR_QUIC_TRANSPORT_ERROR` carrying the cascaded code.
+//   * If CONNECTION_CLOSE is NOT sent, the client only learns of the
+//     teardown via its own idle timer, which hits `[kFinishClose]`
+//     case 3 (`/* Idle close */`) and resolves `session.closed`
+//     *cleanly*. The `mustCall` rejection-handler would then never
+//     fire and the test fails.
+//
+// A short `maxIdleTimeout` keeps the failure mode fast.
+
+import { hasQuic, skip, mustCall } from '../common/index.mjs';
+import assert from 'node:assert';
+
+const { rejects, strictEqual } = assert;
+
+if (!hasQuic) {
+  skip('QUIC is not enabled');
+}
+
+const { listen, connect } = await import('../common/quic.mjs');
+
+// `maxIdleTimeout` is measured in seconds. One second is far longer
+// than CONNECTION_CLOSE on loopback needs to win, while still short
+// enough that a regression in which `CONNECTION_CLOSE` is *not* sent
+// fails the test promptly: the idle-close path takes the
+// `[kFinishClose]` case-3 branch and resolves `session.closed` cleanly
+// instead of rejecting, so the rejection-handler `mustCall` below
+// would fail with "expected exactly 1, actual 0".
+const transportParams = { maxIdleTimeout: 1 };
+
+const serverError = new Error('cascade close frame test');
+
+// Capture the server-side session and wait for *its* `onhandshake` to
+// fire before triggering the cascade. The client's `session.opened`
+// resolves as soon as the client receives the server's TLS Finished,
+// which can land slightly *before* the server has processed the
+// client's reciprocal Finished. Without this synchronization the
+// server-side `kHandshakeCompleted` flag may still be `false` at
+// destroy time and the cascade would skip emitting `CONNECTION_CLOSE`
+// (which is the regression this test is designed to catch).
+const serverHandshake = Promise.withResolvers();
+const onsession = mustCall((serverSession) => {
+  serverSession.onhandshake = mustCall(() => {
+    serverHandshake.resolve();
+  });
+});
+const serverEndpoint = await listen(onsession, { transportParams });
+
+const clientSession = await connect(serverEndpoint.address, {
+  transportParams,
+});
+await clientSession.opened;
+await serverHandshake.promise;
+
+// Attach the rejection handlers BEFORE triggering destroy so neither
+// `serverEndpoint.closed` (rejects with `serverError` via the
+// `#pendingError` semantics from B7) nor `clientSession.closed`
+// (rejects with the transport error decoded from the CONNECTION_CLOSE
+// frame) ends up as an unhandled rejection in the brief window before
+// this test gets back to awaiting them.
+const serverClosedAssertion = rejects(serverEndpoint.closed, serverError);
+const clientClosedAssertion = rejects(clientSession.closed, mustCall((err) => {
+  strictEqual(err.code, 'ERR_QUIC_TRANSPORT_ERROR');
+  return true;
+}));
+
+serverEndpoint.destroy(serverError);
+
+await clientClosedAssertion;
+await serverClosedAssertion;
+
+// Explicit cleanup: the client-side session has been rejected via
+// CONNECTION_CLOSE but the underlying client endpoint is still alive.
+// Tear it down so the event loop drains promptly.
+clientSession.destroy();

test/parallel/test-quic-endpoint-destroy-cascade-close-frame.mjs

@@ -0,0 +1,74 @@
+// Flags: --experimental-quic --no-warnings
+
+// Test: When the user's `onsession` callback throws synchronously or
+// returns a rejected promise, the endpoint is destroyed with that
+// error rather than surfacing as an unhandled exception/rejection out
+// of the C++ -> JS callback boundary. The error is observable through
+// `endpoint.closed` rejecting with the thrown value.
+
+import { hasQuic, skip, mustCall } from '../common/index.mjs';
+import assert from 'node:assert';
+
+const { rejects } = assert;
+
+if (!hasQuic) {
+  skip('QUIC is not enabled');
+}
+
+const { listen, connect } = await import('../common/quic.mjs');
+
+const transportParams = { maxIdleTimeout: 1 };
+
+// -------------------------------------------------------------------
+// 1. Synchronous throw in onsession -> endpoint.closed rejects with
+//    that error.
+// -------------------------------------------------------------------
+{
+  const sessionError = new Error('sync onsession failure');
+
+  const serverEndpoint = await listen(mustCall(() => {
+    throw sessionError;
+  }), { transportParams });
+
+  // Attach the rejection handler BEFORE initiating the connection so
+  // there is no window where serverEndpoint.closed is rejected without
+  // a handler attached. The throw inside `onsession` is delivered
+  // synchronously from the C++ -> JS callback, so the rejection can
+  // arrive on the very next microtask after `connect()` returns.
+  const closedAssertion = rejects(serverEndpoint.closed, sessionError);
+
+  const clientSession = await connect(serverEndpoint.address, {
+    transportParams,
+  });
+
+  await closedAssertion;
+
+  // Explicitly tear down the client side. Even though the endpoint
+  // cascade now sends CONNECTION_CLOSE so the client learns about the
+  // teardown promptly, dropping our reference here keeps the test
+  // robust to network-dropped close packets and stops the event loop
+  // from waiting on the client's idle timer to expire.
+  clientSession.destroy();
+}
+
+// -------------------------------------------------------------------
+// 2. onsession returns a rejected promise -> endpoint.closed rejects
+//    with that error.
+// -------------------------------------------------------------------
+{
+  const sessionError = new Error('async onsession failure');
+
+  const serverEndpoint = await listen(mustCall(async () => {
+    throw sessionError;
+  }), { transportParams, onerror() {} });
+
+  const closedAssertion = rejects(serverEndpoint.closed, sessionError);
+
+  const clientSession = await connect(serverEndpoint.address, {
+    transportParams,
+  });
+
+  await closedAssertion;
+
+  clientSession.destroy();
+}