From e0d0e42e6dedf3072c73d7f23188c159ad039d06 Mon Sep 17 00:00:00 2001 From: Gotham-Zolio <18781106300@163.com> Date: Fri, 11 Sep 2026 12:07:46 -0400 Subject: [PATCH] fix: say goodbye before dropping the socket, and specify that Every one of E7's 45 server logs ended the same way: 1 violation(s) ConnectionClosedError: no close frame received or sent The C++ client dropped the TCP connection without sending a WebSocket close frame. It went unnoticed through every previous conformance run because in all of them the *server* ran out of steps first and closed the connection itself. E7 is the first experiment where the client finishes first - which is also what a real env client does when it reaches its episode budget. Three things were wrong, in three places. **The client** did not send one. It now sends status 1000, per RFC 6455 section 5.5.1, before closing the socket. **The specification had a hole.** Section 7 described four ways the server closes and said nothing at all about the client stopping. New section 7.5: a client may stop at any time and that is not an error - the server keeps no state that outlives the connection - but it SHOULD send a close frame, so that an operator reading the logs does not have to wonder whether a client crashed. **The conformance server was stricter than the specification.** It counted any client disconnect as a violation, including a clean one. That is exactly the failure its two severity levels exist to prevent, and it took a real experiment to expose it. A clean close now satisfies 7.5; a missing close frame is a note, which is the level a SHOULD deserves. Verified both ways: the new client reports `ok 7.5` and no violations, and the previous binary, kept for the comparison, reports `note 7.5` and still no violations. The cross-language CI job passes locally against the modified client, ldd allowlist included. Co-Authored-By: Claude Opus 5 (1M context) --- SPEC.md | 24 ++++++++++++++++++++++++ examples/README.md | 2 +- examples/conformance_server.py | 16 ++++++++++++++++ examples/plugrl_client.cpp | 31 ++++++++++++++++++++++++++++++- 4 files changed, 71 insertions(+), 2 deletions(-) diff --git a/SPEC.md b/SPEC.md index a9f44f6..1833148 100644 --- a/SPEC.md +++ b/SPEC.md @@ -485,6 +485,30 @@ A client should nonetheless be prepared to receive a **text** frame where it expected binary, and treat it as a fatal server-side error rather than attempting to unpack it. +### 7.5 The client stopping + +Everything above is the server closing. A client may also stop first - it +has collected the episodes it was asked for, or its operator interrupted it - +and that is not an error. The server keeps no state that outlives the +connection, so a client that disappears costs nothing beyond the feedback it +had not yet sent. + +A client that is finished **SHOULD** send a WebSocket close frame with status +**1000 (normal closure)** before dropping the socket, as RFC 6455 section +5.5.1 asks. Nothing breaks without one - the server sees the connection end +either way - but the difference is visible in its logs, as +`ConnectionClosedError: no close frame received or sent` rather than a clean +`ConnectionClosedOK`, and an operator reading those logs should not have to +wonder whether a client crashed. + +`examples/conformance_server.py` reports a missing close frame as a note, not +a violation, which is the level this rule deserves. + +> **Historical note.** Until 2026-09-11 the C++ reference client did not send +> one. It went unnoticed because in every test until then the *server* ran +> out of steps first and closed the connection itself; E7, where the client +> finishes first, made it visible on all 45 runs. + --- ## 8. Conformance checklist diff --git a/examples/README.md b/examples/README.md index 2f6bca8..b62f592 100644 --- a/examples/README.md +++ b/examples/README.md @@ -41,7 +41,7 @@ see whether the values it prints are the ones the server sent. |---|---|---| | Language | Python | C++17 | | Dependencies | `msgpack`, `websockets` | **none** | -| Lines | 275 | 814 | +| Lines | 275 | 843 | | Notably absent | numpy, and every `plugrl_*` package | libstdc++ and libc are the only links | The C++ one is the interesting case. It was written on a machine with no diff --git a/examples/conformance_server.py b/examples/conformance_server.py index fde59ca..40444f7 100644 --- a/examples/conformance_server.py +++ b/examples/conformance_server.py @@ -31,6 +31,7 @@ import numpy as np import websockets.asyncio.server as ws_server +import websockets.exceptions as ws_exceptions # Use the real codec, not a re-implementation: the point is to test a client # against what plugrl-server actually does. @@ -245,6 +246,21 @@ async def handle(self, websocket) -> None: self.check_feedback(payload) awaiting = str(MessageType.INFER) self.exchanges += 1 + except ws_exceptions.ConnectionClosedOK: + # The client said goodbye and left. SPEC.md section 7.5 allows + # that at any point, so it is not a failure - a client that has + # collected the episodes it wanted is finished, and the server + # does not get to call that a protocol violation. + report.ok("7.5 a client that stops sends a close frame") + except ws_exceptions.ConnectionClosedError: + # It vanished without a close frame. Section 7.5 asks for one, + # and nothing breaks without it, so this is a note. + report.advise( + False, + "7.5 a client that stops sends a close frame", + "the connection dropped with no close frame; RFC 6455 asks " + "for one and SPEC.md section 7.5 repeats the ask", + ) except Exception as exc: # noqa: BLE001 - the report is the output report.fail("connection", f"{type(exc).__name__}: {exc}") finally: diff --git a/examples/plugrl_client.cpp b/examples/plugrl_client.cpp index eb28927..4f6fce6 100644 --- a/examples/plugrl_client.cpp +++ b/examples/plugrl_client.cpp @@ -445,7 +445,36 @@ class WebSocket { handshake(host, port); } - ~WebSocket() { if (fd_ >= 0) ::close(fd_); } + // RFC 6455 section 5.5.1: say goodbye before dropping the socket. Without + // this the server sees the connection vanish and reports + // `ConnectionClosedError: no close frame received or sent` - which is what + // it did on all 45 runs of E7, where the client finishes first. It only + // went unnoticed before because in every earlier test the *server* ran out + // of steps first and closed the connection itself. + void close_cleanly() { + if (fd_ < 0) return; + std::string frame; + frame.push_back(static_cast(0x88)); // FIN + close opcode + frame.push_back(static_cast(0x80 | 2)); // masked, 2-byte payload + uint8_t key[4]; + for (int i = 0; i < 4; ++i) key[i] = static_cast(rng_() & 0xff); + frame.append(reinterpret_cast(key), 4); + uint8_t status[2] = {0x03, 0xe8}; // 1000, normal closure + for (int i = 0; i < 2; ++i) + frame.push_back(static_cast(status[i] ^ key[i % 4])); + try { + write_all(frame.data(), frame.size()); + } catch (const std::exception&) { + // Already gone. Nothing useful to do on the way out. + } + ::close(fd_); + fd_ = -1; + } + + ~WebSocket() { + close_cleanly(); + if (fd_ >= 0) ::close(fd_); + } void send_binary(const std::string& payload) { std::string frame;