Repository navigation
fix(gateway): drop WebChat frames for closed sockets - #119
Conversation
Replies for a disconnected webchat conn_id stayed on the shared outgoing queue. After 64 of those, every live client failed to send until restart. Discard frames on unregister, and do not enqueue when no socket is registered. Supersedes #115.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Solid, correctly scoped fix for a real availability bug: closed WebChat sessions could pin frames on the shared outgoing list until MAX_QUEUE (64) blocked every live client. The two-sided change (drop on unregister + refuse enqueue without a live wsi) matches the root cause, helpers are clear, and the new test_ws cases lock the regression. I rebuilt and ran ./build/test_ws locally (all passed).
Must Fix
None.
Should Fix
-
CHANGELOG Unreleased — This is an operator-visible availability fix (WebChat dead until restart after ~64 abandoned replies). Other WebChat/gateway fixes already live under
## [Unreleased] ### Fixed. Add a short Fixed bullet so the behavior change is discoverable without reading the PR. -
ws_send_tocontract inws.h— The implementation now intentionally returns0while discarding the frame when no socket is registered (lines 312–315 inws.c). That matches the prior “nowsi→ 0” contract, but the header still says only “0 on success.” One sentence that absent/closed sockets drop the frame and still return 0 would keepwebchat_sendcallers honest.
Nice to Have
- A third regression that fills the queue to
MAX_QUEUEwith a live conn, unregisters it, then asserts a sibling can stillws_send_towould proveg_outgoing_countrecovery under the exact capacity cliff (the current 128-orphan cases already cover the practical failure mode). ws_push_incomingstill uses the pre-helper malloc/strduppattern; optional follow-up to sharews_msg_new/ws_free_outgoingfor symmetry only.
Positive Highlights
- Root cause analysis is accurate: old
ws_send_toenqueued before thewsicheck and returned success with a dead socket, so orphans were never dequeued. ws_drop_outgoing_lockedcorrectly splices mid-list and keeps siblingconn_idframes; the unregister test asserts that ordering.- Checking
g_conns[i].wsion unregister and find paths avoids treating a cleared slot as live. - Focused diff (gateway WS + tests only); good consolidation of #115.
Sent by Cursor Automation: Code review
| /* No live socket: enqueueing would pin the shared queue until restart. */ | ||
| if (!wsi) { | ||
| pthread_mutex_unlock(&g_mutex); | ||
| ws_free_outgoing(m); | ||
| return 0; |
There was a problem hiding this comment.
Should Fix (docs): This path returns 0 after freeing the frame when no live socket is registered. That is the right availability behavior and matches the old “no wsi → 0” contract, but ws.h still documents ws_send_to as “0 on success” only.
Please extend the ws_send_to docstring to state that a well-formed webchat:<id> with no registered socket drops the payload and still returns 0, so webchat_send / channel callers do not treat that as delivery confirmation.
| /** Replies to a session that was never connected must not fill MAX_QUEUE. */ | ||
| static int test_send_to_absent_conn_does_not_fill_queue(void) | ||
| { | ||
| char buf[64]; | ||
| size_t len; | ||
| int i; | ||
|
|
||
| ws_cleanup(); | ||
| for (i = 0; i < 128; i++) | ||
| ASSERT(ws_send_to("webchat:99", "ghost") == 0); | ||
| ASSERT(ws_register_conn(8, (ws_conn_t)(intptr_t)8) == 0); | ||
| ASSERT(ws_send_to("webchat:8", "ok") == 0); | ||
| ASSERT(ws_dequeue_outgoing(8, buf, sizeof(buf), &len) == 1); | ||
| ASSERT(strcmp(buf, "ok") == 0); | ||
| ASSERT(ws_has_pending_outgoing(99) == 0); | ||
| ws_cleanup(); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Nice to Have: Strong coverage for never-registered and post-unregister orphans. An extra case that enqueues until MAX_QUEUE on a live conn, unregisters it, then asserts a sibling ws_send_to still succeeds would pin the exact capacity cliff (g_outgoing_count recovery) in one shot. Not blocking; the 128-orphan loops already exercise the failure mode this PR fixes.
adriannoes
changed the title
WebChat stalling every live client until restart after ~64 abandoned replies is an operator-visible availability fix, so give it a Fixed bullet under Unreleased alongside the other gateway/WebChat entries.
ws_send_to intentionally frees the frame and returns 0 when the target socket is absent, unregistered, or closed (the no-live-wsi path), but the header only promised '0 on success'. State the drop in the contract so webchat_send callers can rely on the actual behavior.
|
Review response — done in e741b7c and d9e7f45: changelog entry for the WebChat fix, and ws.h now says frames for closed sockets are dropped while ws_send_to still returns 0. Deferred as follow-ups: a queue-full-at-capacity regression test (the 128-orphan tests already cover the failure mode) and sharing the msg_new/free helpers in ws_push_incoming. |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
1 participant
Summary
Consolidates one cursor high-severity fix (originally
cursor/high-severity-issues-9537, PR #115) as an operator-authored PR.Replies for a disconnected webchat conn_id stayed on the shared outgoing queue. After 64 of those, every live client failed to send until restart. Discard frames on unregister, and do not enqueue when no socket is registered.
Testing
make test_ws— passedCI=true make test— passedmake static— passedSupersedes #115.