A closed connection still decodes frames, resurrecting _state #115

New issue

Closed

opened 2026-08-22 11:41:23 +00:00 by yuris · 0 comments

yuris commented

2026-08-22 11:41:23 +00:00

Owner

A connection the consumer has closed still decodes frames, and decoding one
mutates state the SDK has already declared final. Surfaced by review of #113,
deliberately not folded into it.

Reproduction

Reachable on HTML5, on the path #113 documents. CloseConnection
(/tmp/verbatim/websocket/src/websocket.cpp:165-186 in extension-websocket
4.2.4) gates its entire body on STATE_CONNECTED, and the unconditional
SetState(conn, STATE_DISCONNECTED) is compiled only under HAVE_WSLAY. So on
HTML5 a close() during the handshake is a genuine no-op: the socket is never
closed, the browser still fires onopen, and the server's frames still arrive.

Nothing on this side stops them being decoded. stdb/init.lua:526 feeds
EVENT_MESSAGE straight into conn:handle_frame, and neither M.handle_frame
(stdb/connection.lua:1420) nor M.dispatch (:1394) checks whether the
connection is closed.

What one decoded frame does to a closed connection

credentials (stdb/connection.lua:965-991) runs on any InitialConnection or
IdentityToken and unconditionally:

resurrects the state — c._state = "connected" (:972), on a connection
whose closed flag is true
re-persists credentials to storage (:981-989), writing identity and
token back to the save slot after the consumer asked to be done
re-emits "connected" (:990), so on_event and the builder's
on_connect fire again post-close

Subscription and transaction handlers refill the cache on the same path.

Why it is worth fixing rather than documenting

M.state's own docstring (stdb/connection.lua:1428-1434) asserts the property
that is violated:

Never recomputed from the event stream, so it cannot drift from what those
already do.

and schedule_reconnect's terminal branch (stdb/init.lua:578-580) asserts it
even more explicitly:

Terminal either way: closing is db:close(), and a drop with reconnect
disabled will never be replaced either, so nothing after this point can ever
move state() off "closed" again.

Both are false today. _state is written "closed" at init.lua:581 and
:654, and credentials writes "connected" over it.

This already cost a bug. #113's send guard needs two conjuncts —
not closing and conn._state ~= "closed" — precisely because _state is not
sticky. A review pass recommended collapsing it to the _state test alone,
verified closing ⟹ _state == "closed" at the instant of close(), and missed
that something moves it back; the collapse reintroduced a silent strand (no
frame, no callback, no error, is_pending() true forever) and had to be
reverted. The non-stickiness is a trap that has already been fallen into once.

Suggested fix

A closed guard in M.dispatch — drop the frame, or emit unhandled, rather
than mutating state. That makes _state genuinely sticky and lets #113's
guards retire their second conjunct.

A smaller variant, if the broader fix is unwanted: have
schedule_reconnect's terminal branch set closing = true as well as
_state = "closed", since it already claims exactly that meaning. One sticky
test then replaces both conjuncts. This does not fix the re-persist, the
re-emit, or the cache refill.

Either way the docstrings at connection.lua:1428-1434 and init.lua:578-580
should match whatever ends up true.

Notes

Native is unaffected on the close() path: CloseConnection transitions to
STATE_DISCONNECTED there, so no EVENT_CONNECTED or later frame arrives.
After a terminal reconnect = false drop, no transport path delivers a frame
either — OnUpdate erases and destroys the connection in the same iteration
it delivers EVENT_DISCONNECTED (websocket.cpp:845-863), and the
frame_budget queue is cleared first. Only the HTML5 close() path is
reachable in a real program; a consumer hand-calling db:dispatch(...) can
reach it anywhere, which is what dev/test/test_init.lua's zombie test does
deliberately.
The zombie test pins the consequence (a send is not held), not this defect.
A regression test for the defect itself belongs with the fix.

A connection the consumer has closed still decodes frames, and decoding one mutates state the SDK has already declared final. Surfaced by review of #113, deliberately not folded into it. ## Reproduction Reachable on HTML5, on the path #113 documents. `CloseConnection` (`/tmp/verbatim/websocket/src/websocket.cpp:165-186` in extension-websocket 4.2.4) gates its **entire** body on `STATE_CONNECTED`, and the unconditional `SetState(conn, STATE_DISCONNECTED)` is compiled only under `HAVE_WSLAY`. So on HTML5 a `close()` during the handshake is a genuine no-op: the socket is never closed, the browser still fires `onopen`, and the server's frames still arrive. Nothing on this side stops them being decoded. `stdb/init.lua:526` feeds `EVENT_MESSAGE` straight into `conn:handle_frame`, and neither `M.handle_frame` (`stdb/connection.lua:1420`) nor `M.dispatch` (`:1394`) checks whether the connection is closed. ## What one decoded frame does to a closed connection `credentials` (`stdb/connection.lua:965-991`) runs on any `InitialConnection` or `IdentityToken` and unconditionally: - **resurrects the state** — `c._state = "connected"` (`:972`), on a connection whose `closed` flag is true - **re-persists credentials to storage** (`:981-989`), writing identity and token back to the save slot after the consumer asked to be done - **re-emits `"connected"`** (`:990`), so `on_event` and the builder's `on_connect` fire again post-close Subscription and transaction handlers refill the cache on the same path. ## Why it is worth fixing rather than documenting `M.state`'s own docstring (`stdb/connection.lua:1428-1434`) asserts the property that is violated: > Never recomputed from the event stream, so it cannot drift from what those > already do. and `schedule_reconnect`'s terminal branch (`stdb/init.lua:578-580`) asserts it even more explicitly: > Terminal either way: `closing` is `db:close()`, and a drop with reconnect > disabled will never be replaced either, so nothing after this point can ever > move `state()` off "closed" again. Both are false today. `_state` is written `"closed"` at `init.lua:581` and `:654`, and `credentials` writes `"connected"` over it. This already cost a bug. #113's send guard needs **two** conjuncts — `not closing and conn._state ~= "closed"` — precisely because `_state` is not sticky. A review pass recommended collapsing it to the `_state` test alone, verified `closing ⟹ _state == "closed"` at the instant of `close()`, and missed that something moves it back; the collapse reintroduced a silent strand (no frame, no callback, no error, `is_pending()` true forever) and had to be reverted. The non-stickiness is a trap that has already been fallen into once. ## Suggested fix A closed guard in `M.dispatch` — drop the frame, or emit `unhandled`, rather than mutating state. That makes `_state` genuinely sticky and lets #113's guards retire their second conjunct. A smaller variant, if the broader fix is unwanted: have `schedule_reconnect`'s terminal branch set `closing = true` as well as `_state = "closed"`, since it already claims exactly that meaning. One sticky test then replaces both conjuncts. This does not fix the re-persist, the re-emit, or the cache refill. Either way the docstrings at `connection.lua:1428-1434` and `init.lua:578-580` should match whatever ends up true. ## Notes - Native is unaffected on the `close()` path: `CloseConnection` transitions to `STATE_DISCONNECTED` there, so no `EVENT_CONNECTED` or later frame arrives. - After a terminal `reconnect = false` drop, no transport path delivers a frame either — `OnUpdate` erases and destroys the connection in the same iteration it delivers `EVENT_DISCONNECTED` (`websocket.cpp:845-863`), and the `frame_budget` queue is cleared first. Only the HTML5 `close()` path is reachable in a real program; a consumer hand-calling `db:dispatch(...)` can reach it anywhere, which is what `dev/test/test_init.lua`'s zombie test does deliberately. - The zombie test pins the *consequence* (a send is not held), not this defect. A regression test for the defect itself belongs with the fix.

yuris referenced this issue from a commit

2026-08-22 13:34:53 +00:00

Drop frames on a closed connection (#115)

yuris referenced this issue from a pull request that will close it,

2026-08-22 13:36:00 +00:00

fix(stdb): drop frames on a closed connection #116

yuris referenced this issue from a commit

2026-08-22 13:49:16 +00:00

Drop post-close frames at the transport too (#115 review)

yuris referenced this issue from a commit

2026-08-22 13:56:33 +00:00

fix(stdb): drop frames on a closed connection (#116)