Repository navigation
fix(qwp): close browser WebSockets that refuse the client's close code - #80
glasstiger wants to merge 5 commits into
Conversation
A browser's WebSocket.close() accepts only 1000 and 3000-4999 and throws InvalidAccessError for anything else. The client closes with 1001 in QwpClient.close(), and with 1002 or 1011 on protocol errors, failed sends, failed reconnects and a cancelled query that does not drain. In a browser that close threw, the fallback found no terminate() to call, and the connection settled as closed with a synthetic 1006 while the native socket stayed open. Pooled query sockets outlived QwpClient.close(), and a reconnect: false session whose cancel did not drain kept receiving the cancelled result until the page unloaded. closeSocket() now retries a close that a socket without terminate() refuses, first with 1000 and the same reason, then with no code at all. A WHATWG socket validates its arguments before it changes state, so the retry is safe. ws accepts these codes and still sends them unchanged. A close that ws refuses is still terminated at once, because ws enters CLOSING before it validates and ignores any later close(). A browser still cannot drop a connection itself. Without flow control QuestDB reads the close only once the query has ended, so in a browser the query runs until then or until the browser abandons the closing handshake, after 60 seconds in Chromium. QWP.md now says so. Fixes #77
The comment said the close codes "still reach a Node server", but the server is QuestDB; `ws` is the Node client. It now says that from Node the codes still reach the server. Comment-only change.
ReviewReviewed head Verdict: approve. No Critical or Moderate findings. Minor
Evidence
Trade-offs
Submodules: none. Findings: 0 Critical, 0 Moderate, 1 Minor (fixed). |
The test that keeps a `ws` socket from being retried closed with 1000. That skips the retry with 1000 which startClosingHandshake() gives a refused code other than 1000, so moving the terminate() check after that retry still passed all of test/qwp. With `ws` that change postpones terminate() by the whole close deadline: `ws` enters CLOSING before it validates, and the retried close() sends nothing. The test now also closes with 1011, and fails on that variant. Test-only change.
…close In a browser, a failed ingress negotiation waited for its close(1002) before rethrowing. Since the refused 1002 is retried with 1000 and waits for the server's reply, the connect deadline could fire first and turn the terminal QwpDurableAckUnavailableError into a retryable upgrade timeout. Start the close without waiting for it, as Node's upgrade-validation failures do.
ReviewReviewed head Verdict: approve. No Critical or Moderate findings. Minor
Evidence
Trade-offsMeasured with fake sockets. In each case the browser now behaves as Node already did on both revisions; base was faster only because it left the old socket open and still streaming.
Submodules: none. Findings: 0 Critical, 0 Moderate, 1 Minor (fixed). |
Summary
Fixes #77. In a browser, the QWP client closed some WebSockets with codes that the browser's
WebSocket.close()refuses: 1001 inQwpClient.close(), and 1002 or 1011 on protocol errors, failed sends, failed reconnects and a cancelled query that does not drain. The close threw and the connection settled as closed, but the native socket stayed open. Pooled query sockets outlivedQwpClient.close(), and areconnect: falsesession whose cancel did not drain kept receiving the cancelled result.Changes
terminate()refuses a close,closeSocket()retries it, first with 1000 and the same reason, then with no code. Every caller of the connection'sclose()gets the retry. A WHATWG socket validates its arguments before it changes state, so retrying is safe.wsaccepts these codes, so on Node they are sent unchanged. A close thatwsrefuses is still terminated immediately, becausewsenters CLOSING before it validates and ignores any laterclose().QWP.md: a browser can only start the closing handshake. Without flow control, QuestDB reads the close only once the query has ended. In a browser the query therefore continues until then, or until the browser abandons the handshake, after 60 s in Chromium. The generated API reference is regenerated for theQWP.mdchange.Not marked breaking. For a browser user, the socket now closes and a 1000 close frame goes on the wire.
closedreports the server's close instead of a synthetic1006 "QWP WebSocket close failed". Node withwsis unchanged.Validation
OPENafterclient.close(), and the cancelled session's socket stayedOPEN, receiving 300 frames in 3 s. With the fix, the pooled socket isCLOSEDonceclient.close()resolves. The cancelled session's socket isCLOSINGat once and receives no further frames, and QuestDB logs the client's close with code 1000. Without flow control, QuestDB answered that close only after its 1-minutequery.timeoutaborted the query. A server that never reads makes Chromium drop the connection after 60 s.close()as a browser does. They cover the transport,QwpClient.close(), and the undrained cancel. Aws-like fake checks that a refused close is still terminated immediately. Two Chromium e2e tests check the native socket'sreadyStateafterQwpClient.close()and after areconnect: falsecancel times out. The new browser tests fail without the fix.pnpm test,pnpm test:dist,pnpm typecheck:dist,pnpm test:qwp-browserand the package checks pass.