Set the non-blocking flag only when the mode change actually succeeded

Message

Adversarial review found the dangerous direction still open. conn-set-non-blocking! returns #f on failure rather than raising -- a closed socket, or fcntl refusing -- and the return was discarded, so the flag could claim a mode the socket was not in.

That inverse is worse than the bug the flag fixes. nowait's TLS arm has no readiness backstop (socket-ready? raises on a TLS connection, and fd-readiness would be the wrong question for TLS anyway), so on that arm the flag is the only thing preventing a blocking read: a lying flag turns a deaf pump into a hung one. sigil-tls records its own nonblocking field only after the fd changed mode, for this exact reason; match it.

I had checked for this and cleared it, by grepping for call sites passing #f. That check was wrong: it can only find a caller ASKING to re-block, never a call that was asked to unblock and failed.

Also from the same review:

The docstring's "CALL IT ALONE, ON A LOOP ... needs no scheduler" was false for the bare (ws-connect url) shape, which has neither a scheduler nor a timeout and so leaves the transport blocking and the pump deaf -- the very bug this change fixes, promised as fixed. The precondition is now stated with all three call shapes spelled out. The repo's own native-binary-client uses the bare shape.
Two adjacent comments gave incompatible accounts of what is load-bearing: one said the flag is the only thing making the read safe, the next said the flag may be untrustworthy and readiness is authoritative. Rewritten to say what is actually true -- plain TCP keeps readiness as defence in depth, TLS has no backstop and depends on the flag.
The harness reported SETUP-FAILED whenever the frame was not sent, but a client that dies before the 300ms timer leaves that flag false through no fault of the apparatus, so a genuine client failure was blamed on the setup. Split on whether the upgrade was seen.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AbXvyDXoTUyqV1tb1fkeFa

Changed

 src/sigil/websocket/connection.sgl                     | 63 ++++++++++++++++++++++++++++++++++++++++++++++-----------------
 test/integration/test-native-websocket-nowait-pump.mjs | 14 +++++++++++++-
 2 files changed, 59 insertions(+), 18 deletions(-)

Diff

src/sigil/websocket/connection.sglmodified

@@ -401,11 +401,19 @@

401

;; Make the HTTP upgrade read cooperative before it can

402

;; wait for a peer that accepted TLS and then went quiet.

403

(when (or (current-scheduler) upgrade-timeout-ms)

−404

(conn-set-non-blocking! socket #t)

−405

;; Record what we just did. Readers must not

−406

;; reconstruct this condition from the scheduler

−407

;; alone — see the field comment on non-blocking.

−408

(set-ws-connection-non-blocking! conn #t))

+404

;; RECORD ONLY IF THE CALL ACTUALLY SUCCEEDED. Both

+405

;; natives report failure by RETURNING #f rather than

+406

;; raising -- a closed socket, or fcntl refusing --

+407

;; so an unchecked call would let the flag claim a

+408

;; mode the socket is not in. That direction is worse

+409

;; than the bug this flag fixes: nowait's TLS arm has

+410

;; no readiness backstop, so a lying flag sends it

+411

;; into a BLOCKING mbedtls_ssl_read and the pump

+412

;; hangs instead of merely going deaf. sigil-tls sets

+413

;; its own `nonblocking` field only after the fd

+414

;; changed mode, for exactly this reason; match it.

+415

(when (conn-set-non-blocking! socket #t)

+416

(set-ws-connection-non-blocking! conn #t)))

417

;; Send handshake request

418

(conn-write socket request)

419

;; Validate response

@@ -702,12 +710,24 @@

710

;;; written in one segment followed by three seconds of silence, and the

711

;;; second frame surfaced 2997 ms late, with the wake frame.

712

;;;

−705

;;; THIS PROCEDURE IS SELF-SUFFICIENT: CALL IT ALONE, ON A LOOP.

+713

;;; CALL IT ALONE, ON A LOOP — BUT ONLY IF THE CONNECTION WAS OPENED SO

+714

;;; THAT ITS TRANSPORT IS NON-BLOCKING. That is the whole precondition, and

+715

;;; it is not a formality:

716

;;;

−707

;;; It first decodes anything already buffered, and then — when the

−708

;;; transport is non-blocking, which is what makes the read unable to park

−709

;;; — takes exactly one socket read. A pump needs nothing else, and needs

−710

;;; no scheduler.

+717

;;; (ws-connect url upgrade-timeout-ms: 5000) ; non-blocking -> self-sufficient

+718

;;; (with-async … (ws-connect url) …) ; non-blocking -> self-sufficient

+719

;;; (ws-connect url) ; BLOCKING -> BUFFER-ONLY, DEAF

+720

;;;

+721

;;; `ws-connect` puts the transport in non-blocking mode when a scheduler

+722

;;; is running OR an `upgrade-timeout-ms` was supplied. With NEITHER, the

+723

;;; socket stays blocking, this procedure cannot take its socket read

+724

;;; without risking a park, so it correctly declines to — and a pump that

+725

;;; calls it alone will never hear anything. Pass a timeout, or run under a

+726

;;; scheduler, or do not use a nowait-only pump.

+727

;;;

+728

;;; Given that precondition: it first decodes anything already buffered,

+729

;;; then takes exactly one socket read. A pump needs nothing else, and

+730

;;; needs no scheduler.

731

;;;

732

;;; DO NOT pair it with `socket-ready?`. An earlier revision of this

733

;;; docstring prescribed

@@ -808,13 +828,22 @@

828

;; `receive-message` returns 'need-more on a would-block

829

;; read, which becomes #f here.

830

(and (ws-connection-non-blocking conn)

−811

;; A scheduler normally makes the socket nonblocking,

−812

;; but NOWAIT must not rely on that ambient setup: a

−813

;; caller may inherit a connection whose flag was not

−814

;; retained by a wrapper/runtime boundary. For plain

−815

;; TCP, readiness is the authoritative no-park gate.

−816

;; TLS retains its own nonblocking state machine and

−817

;; may have decrypted bytes buffered above the fd.

+831

;; A SECOND, INDEPENDENT no-park gate for plain TCP,

+832

;; kept deliberately even though the flag above is now

+833

;; recorded rather than inferred. Defence in depth: if

+834

;; the flag were ever wrong, readiness still stops a

+835

;; plain socket parking.

+836

;;

+837

;; TLS GETS NO SUCH BACKSTOP, and that asymmetry is

+838

;; worth knowing: `socket-ready?` raises on a TLS

+839

;; connection (its native rejects anything that is not

+840

;; a plain socket), and TLS keeps its own nonblocking

+841

;; state machine with decrypted bytes possibly

+842

;; buffered above the fd, so fd-readiness would be the

+843

;; wrong question anyway. On the TLS arm the flag is

+844

;; the ONLY thing preventing a blocking read -- which

+845

;; is why ws-connect must not set it unless the mode

+846

;; change actually succeeded.

847

(or (tls-connection? (ws-connection-socket conn))

848

(socket-ready? (ws-connection-socket conn)))

849

(let ((result (receive-message conn)))

test/integration/test-native-websocket-nowait-pump.mjsmodified

@@ -25,6 +25,7 @@ function textFrame(text) {

}

let sentFrame = false;

+28

let upgradeSeen = false;

const server = net.createServer((socket) => {

let buffer = Buffer.alloc(0);

socket.on("data", (chunk) => {

@@ -45,6 +46,7 @@ const server = net.createServer((socket) => {

"ascii",

));

buffer = Buffer.alloc(0);

+49

upgradeSeen = true;

setTimeout(() => {

socket.write(textFrame(PAYLOAD));

sentFrame = true;

@@ -89,8 +91,18 @@ server.close();

// Positive control on the apparatus: if the server never got far enough to

// send the frame, the client's silence says nothing about the client. That

// must read as a setup failure, not as a regression.

+94

//

+95

// But "frame not sent" has two causes and they mean opposite things. The

+96

// frame goes out on a 300ms timer, so a client that dies BEFORE then -- a

+97

// failed upgrade, a missing dependency, a compile error in connection.sgl --

+98

// closes the socket first and leaves sentFrame false through no fault of the

+99

// apparatus. Reporting that as SETUP-FAILED would blame the harness for a

+100

// genuine client failure, so split the two on whether the upgrade completed.

101

if (!sentFrame) {

−93

  throw new Error(`SETUP-FAILED: server never sent its frame, so this run tests nothing\nstdout:\n${stdout}\nstderr:\n${stderr}`);

+102

const reached = upgradeSeen

+103

    ? "the client connected and completed its upgrade, then exited before the frame timer ran -- this is a CLIENT failure, not a setup failure"

+104

    : "the server never saw a completed upgrade request, so the apparatus never got far enough to test anything (SETUP-FAILED)";

+105

throw new Error(`no frame was sent: ${reached}\nstatus: ${status}\nstdout:\n${stdout}\nstderr:\n${stderr}`);

106

}

107

if (status !== 0 || !stdout.includes("native-websocket-nowait-pump-ok")) {

108

throw new Error(`nowait pump client failed (${status})\nstdout:\n${stdout}\nstderr:\n${stderr}`);