Skip to content

Commit 7ce915a

Browse files
committed
Drop the ROLE_CHANGE (4001) close code
Reverts 9f3cb32. The server keeps NORMAL_CLOSURE on the role-change CLOSE, so there is no 4001 on the wire for this client to classify. 4001 existed so the client's verbatim CLOSE echo would prove to the server that it had read the server's CLOSE rather than having sent a voluntary CLOSE that crossed it. The server takes identical action on both branches -- the code only selected a log line -- while every client released before 9f3cb32 treats a code outside NORMAL_CLOSURE/GOING_AWAY as a head-of-line poison strike, escalating a routine demote to a PROTOCOL_VIOLATION terminal that quarantines the store-and-forward slot. Putting the code on the wire therefore costs data on mixed-version fleets and buys a diagnostic; it needs a negotiated capability first. Carrying the classification here anyway would leave the client claiming to understand a code nothing sends, and the poison-frame test would pin a contract with no counterparty. Strip both, and record in qwp-nack-policy-v2.md that a distinguishable role-change code is gated behind capability negotiation rather than simply absent.
1 parent dde3c15 commit 7ce915a

4 files changed

Lines changed: 15 additions & 75 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ public final class CursorWebSocketSendLoop implements QuietCloseable {
357357
private long highestOkFsn = -1L;
358358
// Zero-progress recycle pacer (I/O thread only; survives reconnects).
359359
// Counts consecutive strike-EXEMPT recycles -- orderly closes
360-
// (NORMAL_CLOSURE/GOING_AWAY/ROLE_CHANGE), non-orderly closes before any send on the
360+
// (NORMAL_CLOSURE/GOING_AWAY), non-orderly closes before any send on the
361361
// connection, and pre-send RETRIABLE_OTHER rejections -- with no
362362
// acceptance progress in between. These paths deliberately carry no
363363
// poison strike (they are not a verdict on the bytes), which also exempts
@@ -1629,7 +1629,7 @@ private void failPaced(Throwable initial) {
16291629

16301630
/**
16311631
* Recycle path for strike-exempt wire events: orderly closes
1632-
* (NORMAL_CLOSURE / GOING_AWAY / ROLE_CHANGE), non-orderly closes before any send on
1632+
* (NORMAL_CLOSURE / GOING_AWAY), non-orderly closes before any send on
16331633
* the connection, and RETRIABLE_OTHER rejections (pre- and post-send:
16341634
* NOT_WRITABLE is a node-state verdict, not a frame verdict). None of
16351635
* these implicate the head frame, so they carry no poison strike -- but that
@@ -2394,12 +2394,11 @@ public void onClose(int code, String reason) {
23942394
// after this connection already sent the head frame counts a poison
23952395
// strike; maxHeadFrameRejections consecutive strikes at the same
23962396
// head FSN with no ack progress escalate to a typed terminal. Orderly
2397-
// closes (ROLE_CHANGE role-change handoff, NORMAL_CLOSURE, GOING_AWAY
2398-
// restart drain) never count strikes — they are the server asking us
2399-
// to go elsewhere, not a verdict on the bytes.
2397+
// closes (NORMAL_CLOSURE role-change handoff, GOING_AWAY restart
2398+
// drain) never count strikes — they are the server asking us to go
2399+
// elsewhere, not a verdict on the bytes.
24002400
boolean orderly = code == WebSocketCloseCode.NORMAL_CLOSURE
2401-
|| code == WebSocketCloseCode.GOING_AWAY
2402-
|| code == WebSocketCloseCode.ROLE_CHANGE;
2401+
|| code == WebSocketCloseCode.GOING_AWAY;
24032402
LineSenderException cause = new LineSenderException(
24042403
"WebSocket closed by server: code=" + code + " reason=" + reason);
24052404
if (!orderly && nextWireSeq > 0) {

core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -84,16 +84,6 @@ public final class WebSocketCloseCode {
8484
* Reserved for future use.
8585
*/
8686
public static final int RESERVED = 1004;
87-
/**
88-
* Role-change close (4001). QWP application-defined code in the RFC 6455
89-
* Section 7.4.2 private-use range: the server closed because its role
90-
* changed (primary demoted); reconnect-eligible, not a verdict on the
91-
* bytes. Deliberately distinct from {@link #NORMAL_CLOSURE} so the
92-
* client's verbatim CLOSE echo proves to the server that the client
93-
* received the server's CLOSE (and, by TCP ordering, everything before
94-
* it -- the final durable ack included).
95-
*/
96-
public static final int ROLE_CHANGE = 4001;
9787
/**
9888
* TLS handshake (1015).
9989
* Reserved value. MUST NOT be sent in a Close frame.
@@ -144,8 +134,6 @@ public static String describe(int code) {
144134
return "Internal Error";
145135
case TLS_HANDSHAKE:
146136
return "TLS Handshake";
147-
case ROLE_CHANGE:
148-
return "Role Change (QWP)";
149137
default:
150138
if (code >= 3000 && code < 4000) {
151139
return "Library/Framework Code (" + code + ")";

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import io.questdb.client.cutlass.qwp.client.WebSocketResponse;
3333
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine;
3434
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop;
35-
import io.questdb.client.cutlass.qwp.websocket.WebSocketCloseCode;
3635
import io.questdb.client.network.PlainSocketFactory;
3736
import io.questdb.client.std.Files;
3837
import io.questdb.client.std.MemoryTag;
@@ -245,43 +244,6 @@ public void testNonOrderlyClosePoisonKeysOnOkLevelHeadOfLine() throws Exception
245244
});
246245
}
247246

248-
@Test
249-
public void testRoleChangeCloseIsOrderlyNeverCountsStrikes() throws Exception {
250-
// The server's role-change close (ROLE_CHANGE, 4001) is the demote
251-
// handoff: "go elsewhere", not a verdict on the bytes. Like
252-
// NORMAL_CLOSURE and GOING_AWAY it must never count a poison strike.
253-
// Dropping ROLE_CHANGE from the orderly set would pass the rest of
254-
// this suite while latching a PROTOCOL_VIOLATION terminal after
255-
// maxHeadFrameRejections demote closes at an unadvanced head FSN --
256-
// turning a routine failover into a false data-poison verdict.
257-
// Mirrors testNonOrderlyClosePoisonKeysOnOkLevelHeadOfLine with the
258-
// opposite expectation.
259-
TestUtils.assertMemoryLeak(() -> {
260-
List<WebSocketClient> clients = new ArrayList<>();
261-
try (CursorSendEngine engine = newEngine()) {
262-
appendFrames(engine, 2);
263-
CursorWebSocketSendLoop loop = newDurableLoop(engine, clients);
264-
setSentCount(loop, 2);
265-
266-
deliverOk(loop, 0, names("trades"), txns(7L));
267-
for (int i = 0; i < MAX_REJECTIONS + 1; i++) {
268-
// Role-change close after at least one send on this
269-
// connection: strike-exempt, so no number of repeats may
270-
// escalate. Restore the sent count after each recycle,
271-
// exactly like the non-orderly variant.
272-
setSentCount(loop, 2);
273-
deliverRoleChangeClose(loop);
274-
}
275-
276-
// Must NOT throw: orderly closes never escalate to a typed
277-
// terminal, however many accumulate at the same head FSN.
278-
loop.checkError();
279-
} finally {
280-
closeAll(clients);
281-
}
282-
});
283-
}
284-
285247
@Test
286248
public void testNackRecycleIsPacedAgainstHealthyServer() throws Exception {
287249
// A reachable, healthy server that NACKs the head frame (RETRIABLE)
@@ -945,16 +907,6 @@ private static void deliverOrderlyClose(CursorWebSocketSendLoop loop) throws Exc
945907
m.invoke(handler, 1001, "server draining"); // GOING_AWAY: orderly, strike-exempt
946908
}
947909

948-
private static void deliverRoleChangeClose(CursorWebSocketSendLoop loop) throws Exception {
949-
Field f = CursorWebSocketSendLoop.class.getDeclaredField("responseHandler");
950-
f.setAccessible(true);
951-
Object handler = f.get(loop);
952-
Method m = handler.getClass().getDeclaredMethod("onClose", int.class, String.class);
953-
m.setAccessible(true);
954-
// ROLE_CHANGE (4001): the demote handoff, orderly, strike-exempt
955-
m.invoke(handler, WebSocketCloseCode.ROLE_CHANGE, "role change: primary demoted");
956-
}
957-
958910
private static void deliverNonOrderlyClose(CursorWebSocketSendLoop loop) throws Exception {
959911
Field f = CursorWebSocketSendLoop.class.getDeclaredField("responseHandler");
960912
f.setAccessible(true);

design/qwp-nack-policy-v2.md

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,8 @@ is caught *behaviorally*:
6666
> in durable-ack mode the trim watermark advances only on durable coverage,
6767
> so every post-NACK recycle replays from the durable watermark and re-OKs
6868
> frames *behind* the suspect — those re-OKs say nothing about the poisoned
69-
> bytes and must not launder the count. Orderly closes (`ROLE_CHANGE` (4001)
70-
> role-change handoff, `NORMAL_CLOSURE`, `GOING_AWAY` restart drain) never
71-
> count strikes.
69+
> bytes and must not launder the count. Orderly closes (`NORMAL_CLOSURE`
70+
> role-change handoff, `GOING_AWAY` restart drain) never count strikes.
7271
7372
Below the escalation threshold, a RETRIABLE NACK's recycle is **paced**: the
7473
server is reachable (it just answered), so the reconnect succeeds immediately
@@ -100,11 +99,13 @@ diagnostics only.
10099

101100
The server already handles it at the right layer (Invariant B work): the
102101
read-only gate and the commit-path authorization refusal both set
103-
`roleChangeClosePending` and close with a reconnect-eligible `ROLE_CHANGE`
104-
(4001, private-use range; distinct from `NORMAL_CLOSURE` so the client's
105-
verbatim CLOSE echo is distinguishable from a voluntary client CLOSE that
106-
crossed the server's CLOSE on the wire) instead of NACKing `SECURITY_ERROR`
107-
(`QwpIngressProcessorState`). The client
102+
`roleChangeClosePending` and close with a reconnect-eligible
103+
`NORMAL_CLOSURE` instead of NACKing `SECURITY_ERROR`
104+
(`QwpIngressProcessorState`). A private-use code would let the server tell
105+
the client's verbatim CLOSE echo apart from a voluntary client CLOSE that
106+
crossed it on the wire, but deployed fleets classify anything outside
107+
`NORMAL_CLOSURE`/`GOING_AWAY` as a poison strike, so that needs a
108+
negotiated capability first. The client
108109
reconnects, hits the 421 role reject on the now-replica, and retries from SF
109110
until a primary is reachable. Consequently:
110111

0 commit comments

Comments
 (0)